Re: [PATCHv4] powerpc: Fix write-after-destroy in lock elision

["Tulio Magno Quites Machado Filho" <tuliom at linux dot vnet dot ibm dot com>]

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