[PATCHv4] powerpc: Fix write-after-destroy in lock elision
Tulio Magno Quites Machado Filho
tuliom@linux.vnet.ibm.com
Tue Jan 3 19:25:00 GMT 2017
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
More information about the Libc-alpha
mailing list