Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1460916 > unrolled thread

Re: spin_lock implicit/explicit memory barrier

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: spin_lock implicit/explicit memory barrier Boqun Feng <boqun.feng@gmail.com> - 2016-08-12 04:50 +0200
    Re: spin_lock implicit/explicit memory barrier Manfred Spraul <manfred@colorfullife.com> - 2016-08-12 20:50 +0200
      Re: spin_lock implicit/explicit memory barrier Boqun Feng <boqun.feng@gmail.com> - 2016-08-22 11:20 +0200

#1460916 — Re: spin_lock implicit/explicit memory barrier

[Multipart message — attachments visible in raw view] — view raw

On Wed, Aug 10, 2016 at 12:17:57PM -0700, Davidlohr Bueso wrote:
> On Wed, 10 Aug 2016, Manfred Spraul wrote:
> 
> > On 08/10/2016 02:05 AM, Benjamin Herrenschmidt wrote:
> > > On Tue, 2016-08-09 at 20:52 +0200, Manfred Spraul wrote:
> > > > Hi Benjamin, Hi Michael,
> > > > 
> > > > regarding commit 51d7d5205d33 ("powerpc: Add smp_mb() to
> > > > arch_spin_is_locked()"):
> > > > 
> > > > For the ipc/sem code, I would like to replace the spin_is_locked() with
> > > > a smp_load_acquire(), see:
> > > > 
> > > > http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n367
> > > > 
> > > > http://www.ozlabs.org/~akpm/mmots/broken-out/ipc-semc-fix-complex_count-vs-simple-op-race.patch
> > > > 
> > > > To my understanding, I must now add a smp_mb(), otherwise it would be
> > > > broken on PowerPC:
> > > > 
> > > > The approach that the memory barrier is added into spin_is_locked()
> > > > doesn't work because the code doesn't use spin_is_locked().
> > > > 
> > > > Correct?
> > > Right, otherwise you aren't properly ordered. The current powerpc locks provide
> > > good protection between what's inside vs. what's outside the lock but not vs.
> > > the lock *value* itself, so if, like you do in the sem code, use the lock
> > > value as something that is relevant in term of ordering, you probably need
> > > an explicit full barrier.
> 
> But the problem here is with spin_unlock_wait() (for ll/sc spin_lock) not seeing the
> store that makes the lock visibly taken and both threads end up exiting out of sem_lock();
> similar scenario to the spin_is_locked commit mentioned above, which is crossing of
> locks.
> 
> Now that spin_unlock_wait() always implies at least an load-acquire barrier (for both
> ticket and qspinlocks, which is still x86 only), we wait on the full critical region.
> 
> So this patch takes this locking scheme:
> 
>   CPU0			      CPU1
>   spin_lock(l)		      spin_lock(L)
>   spin_unlock_wait(L)	      if (spin_is_locked(l))
>   foo()			 foo()
> 
> ... and converts it now to:
> 
>   CPU0			      CPU1
>   complex_mode = true	      spin_lock(l)
>   smp_mb()				  <--- do we want a smp_mb() here?
>   spin_unlock_wait(l)	      if (!smp_load_acquire(complex_mode))
>   foo()			 foo()
> 
> We should not be doing an smp_mb() right after a spin_lock(), makes no sense. The
> spinlock machinery should guarantee us the barriers in the unorthodox locking cases,
> such as this.
> 

Right.

If you have:

6262db7c088b ("powerpc/spinlock: Fix spin_unlock_wait()")

you don't need smp_mb() after spin_lock() on PPC.

And, IIUC, if you have:

3a5facd09da8 ("arm64: spinlock: fix spin_unlock_wait for LSE atomics")
d86b8da04dfa ("arm64: spinlock: serialise spin_unlock_wait against
concurrent lockers")

you don't need smp_mb() after spin_lock() on ARM64.

And, IIUC, if you have:

2c6100227116 ("locking/qspinlock: Fix spin_unlock_wait() some more")

you don't need smp_mb() after spin_lock() on x86 with qspinlock.

Regards,
Boqun

> Thanks,

> Davidlohr

[toc] | [next] | [standalone]

#1461406

Hi Boqun,

On 08/12/2016 04:47 AM, Boqun Feng wrote:
>> We should not be doing an smp_mb() right after a spin_lock(), makes no sense. The
>> spinlock machinery should guarantee us the barriers in the unorthodox locking cases,
>> such as this.
>>
Do we really want to go there?
Trying to handle all unorthodox cases will end up as an endless list of 
patches, and guaranteed to be stale architectures.

> Right.
>
> If you have:
>
> 6262db7c088b ("powerpc/spinlock: Fix spin_unlock_wait()")
>
> you don't need smp_mb() after spin_lock() on PPC.
>
> And, IIUC, if you have:
>
> 3a5facd09da8 ("arm64: spinlock: fix spin_unlock_wait for LSE atomics")
> d86b8da04dfa ("arm64: spinlock: serialise spin_unlock_wait against
> concurrent lockers")
>
> you don't need smp_mb() after spin_lock() on ARM64.
>
> And, IIUC, if you have:
>
> 2c6100227116 ("locking/qspinlock: Fix spin_unlock_wait() some more")
>
> you don't need smp_mb() after spin_lock() on x86 with qspinlock.

I would really prefer the other approach:
- spin_lock() is an acquire, that's it. No further guarantees, e.g. 
ordering of writing the lock.
- spin_unlock() is a release, that's it.
- generic smp_mb__after_before_whatever(). And architectures can 
override the helpers.
E.g. if qspinlocks on x86 can implement the smp_mb__after_spin_lock() 
for free, then the helper can be a nop.

Right now, we start to hardcode something into the architectures - for 
some callers.
Other callers use solutions such as smp_mb__after_unlock_lock(), i.e. 
arch dependent workarounds in arch independent code.

And: We unnecessarily add overhead.
Both ipc/sem and netfilter do loops over many spinlocks:
>        for (i = 0; i < CONNTRACK_LOCKS; i++) {
>                 spin_unlock_wait(&nf_conntrack_locks[i]);
>         }
One memory barrier would be sufficient, but due to embedding we end up 
with CONNTRACK_LOCKS barriers.

Should I create a patch?
(i.e. documentation and generic helpers)

--
     Manfred

[toc] | [prev] | [next] | [standalone]

#1467491

[Multipart message — attachments visible in raw view] — view raw

On Fri, Aug 12, 2016 at 08:43:55PM +0200, Manfred Spraul wrote:
> Hi Boqun,
> 
> On 08/12/2016 04:47 AM, Boqun Feng wrote:
> > > We should not be doing an smp_mb() right after a spin_lock(), makes no sense. The
> > > spinlock machinery should guarantee us the barriers in the unorthodox locking cases,
> > > such as this.
> > > 
> Do we really want to go there?
> Trying to handle all unorthodox cases will end up as an endless list of
> patches, and guaranteed to be stale architectures.
> 

To be honest, the only unorthodox case here is we try to use
spin_unlock_wait() to achieve some kind of ordering with a lock
critical section, like we do in sem code and do_exit(). Spinlocks are
mostly used for exclusive lock critical sections, which are what we care
about most and what we should make as fast as possible.

> > Right.
> > 
> > If you have:
> > 
> > 6262db7c088b ("powerpc/spinlock: Fix spin_unlock_wait()")
> > 
> > you don't need smp_mb() after spin_lock() on PPC.
> > 
> > And, IIUC, if you have:
> > 
> > 3a5facd09da8 ("arm64: spinlock: fix spin_unlock_wait for LSE atomics")
> > d86b8da04dfa ("arm64: spinlock: serialise spin_unlock_wait against
> > concurrent lockers")
> > 
> > you don't need smp_mb() after spin_lock() on ARM64.
> > 
> > And, IIUC, if you have:
> > 
> > 2c6100227116 ("locking/qspinlock: Fix spin_unlock_wait() some more")
> > 
> > you don't need smp_mb() after spin_lock() on x86 with qspinlock.
> 
> I would really prefer the other approach:
> - spin_lock() is an acquire, that's it. No further guarantees, e.g. ordering
> of writing the lock.
> - spin_unlock() is a release, that's it.

No objection to these. That's how we implement and document locks now.

> - generic smp_mb__after_before_whatever(). And architectures can override
> the helpers.
> E.g. if qspinlocks on x86 can implement the smp_mb__after_spin_lock() for
> free, then the helper can be a nop.
> 

If you are going to use smp_mb__after_before_whatever() to fix the
spin_unlock_wait() problem, the helper for qspinlocks on x86 can not be
free, because qspinlocks on x86 use separate READs and WRITEs in
spin_lock(), and we need a full barrier to order the WRITEs of the lock
acquisition with READs in critical sections to let spin_unlock_wait()
work.

> Right now, we start to hardcode something into the architectures - for some
> callers.
> Other callers use solutions such as smp_mb__after_unlock_lock(), i.e. arch
> dependent workarounds in arch independent code.
> 

Please note that fixes above in spin_unlock_wait() and
smp_mb__after_unlock_lock() are for two different problems,
smp_mb__after_unlock_lock() is to upgrade a lock+unlock pair into a full
barrier, which is useful to RCpc archs, e.g. PowerPC.

> And: We unnecessarily add overhead.
> Both ipc/sem and netfilter do loops over many spinlocks:
> >        for (i = 0; i < CONNTRACK_LOCKS; i++) {
> >                 spin_unlock_wait(&nf_conntrack_locks[i]);
> >         }
> One memory barrier would be sufficient, but due to embedding we end up with
> CONNTRACK_LOCKS barriers.
> 

We can move the smp_mb() out of the spin_unlock_wait(), if we make sure
all the users are using the proper barriers, so this overhead is easily
fixed.

> Should I create a patch?
> (i.e. documentation and generic helpers)
> 

There have been some discussions in this thread:

http://marc.info/?l=linux-arm-kernel&m=144862480822027

, which may be helpful ;-)

Regards,
Boqun

> --
>     Manfred

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web