This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCHv4] powerpc: Fix write-after-destroy in lock elision
Torvald Riegel <triegel@redhat.com> writes:
> For the next time, or when you change non-arch-specific code, please
> remember to document memory orders; a brief comment is often sufficient
> but reveals the intent behind a specific bit of implementation. For
> example:
>
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-lock.c b/sysdeps/unix/sysv/linux/powerpc/elision-lock.c
>> index 4589491..044803a 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/elision-lock.c
>> +++ b/sysdeps/unix/sysv/linux/powerpc/elision-lock.c
>> @@ -45,7 +45,7 @@
>> int
>> __lll_lock_elision (int *lock, short *adapt_count, EXTRAARG int pshared)
>> {
>> - if (*adapt_count > 0)
>> + if (atomic_load_relaxed (adapt_count) > 0)
>
> Here, you could have just addded:
> /* adapt_count is accessed concurrently but is just a hint. Thus,
> use atomic accesses but relaxed MO is sufficient. */
>
> This helps others to more quickly understand what was intended. This
> isn't critical in this case because we have well-documented similar code
> for other architectures, but generally it is better to document why a
> choice was made if it is not immediately obvious, or cannot be checked
> for consistency based on just local information (eg, the source code
> lines around the statement in question); concurrent code obviously can't
> be cross-checked with just local information...
I agree with you.
I added this comment in this patch and pushed it as e9a96ea.
Thanks!
--
Tulio Magno