Re: [PATCH v2 1/6] locking/pvqspinlock: Unconditional PV kick with _Q_SLOW_VAL

Waiman Long
2015-07-16T15:04:35Z

On 07/16/2015 10:07 AM, Waiman Long wrote:
> On 07/16/2015 01:42 AM, Peter Zijlstra wrote:
>> On Wed, Jul 15, 2015 at 08:18:23PM -0400, Waiman Long wrote:
>>> On 07/15/2015 05:10 AM, Peter Zijlstra wrote:
>>>> /*
>>>> + * A failed cmpxchg doesn't provide any memory-ordering
>>>> guarantees,
>>>> + * so we need a barrier to order the read of the node data in
>>>> + * pv_unhash *after* we've read the lock being _Q_SLOW_VAL.
>>>> + *
>>>> + * Matches the cmpxchg() in pv_wait_head() setting _Q_SLOW_VAL.
>>>> + */
>>>> + smp_rmb();
>>> According to memory_barriers.txt, cmpxchg() is a full memory
>>> barrier. It
>>> didn't say a failed cmpxchg will lose its memory guarantee. So is the
>>> documentation right?
>> The documentation is not entirely clear on this; but there are hints
>> that this is so.
>>
>>> Or is that true for some architectures? I think it is
>>> not true for x86.
>> On x86 LOCK CMPXCHG is always a sync point, but yes there are archs for
>> which a failed cmpxchg does _NOT_ provide any barrier semantics.
>>
>> The reason I started looking was because Will made Argh64 one of those.
>
> That is what I suspected. In that case, I am fine with the patch as
> smp_rmb() is an nop in x86 anyway.
>
> Acked-by: Waiman Long <Waiman.Long@hp.com>
>
> BTW, I think we also need to update the documentation to make it clear
> that a failed cmpxchg() or atomic_cmpxchg() may not be a full memory
> barrier as most people may not be aware of that.
>

I suspect that there may be other places in the kernel that have similar
problem. An alternative will be to strengthen cmpxchg() in architectures
like arm64 to act as full memory barrier no matter the result and
defined relaxed version with no such guarantee.

Cheers,
Longman