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

Groups > linux.kernel > #1460066 > 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 Davidlohr Bueso <dave@stgolabs.net> - 2016-08-11 00:30 +0200
    Re: spin_lock implicit/explicit memory barrier "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-08-11 01:00 +0200
      Re: spin_lock implicit/explicit memory barrier Davidlohr Bueso <dave@stgolabs.net> - 2016-08-11 01:40 +0200
        Re: spin_lock implicit/explicit memory barrier Peter Zijlstra <peterz@infradead.org> - 2016-08-11 10:20 +0200
          Re: spin_lock implicit/explicit memory barrier Davidlohr Bueso <dave@stgolabs.net> - 2016-08-11 20:40 +0200
            Re: spin_lock implicit/explicit memory barrier Boqun Feng <boqun.feng@gmail.com> - 2016-08-12 05:10 +0200
              Re: spin_lock implicit/explicit memory barrier Peter Zijlstra <peterz@infradead.org> - 2016-08-19 16:10 +0200
    Re: spin_lock implicit/explicit memory barrier Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-08-11 02:10 +0200

#1460066 — Re: spin_lock implicit/explicit memory barrier

On Wed, 10 Aug 2016, Paul E. McKenney wrote:

>On Wed, Aug 10, 2016 at 08:21:22PM +0200, Manfred Spraul wrote:

>>                                                               4)
>> spin_unlock_wait() and spin_unlock() pair
>> http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n291
>> http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n409
>> The data from the simple op must be observed by the following
>> complex op. Right now, there is still an smp_rmb() in line 300: The
>> control barrier from the loop inside spin_unlock_wait() is upgraded
>> to an acquire barrier by an additional smp_rmb(). Is this smp_rmb()
>> required? If I understand commit 2c6100227116 ("locking/qspinlock:
>> Fix spin_unlock_wait() some more") right, with this commit qspinlock
>> handle this case without the smp_rmb(). What I don't know if powerpc
>> is using qspinlock already, or if powerpc works without the
>> smp_rmb(). -- Manfred|

No, ppc doesn't use qspinlocks, but as mentioned, spin_unlock_wait for
tickets are now at least an acquire (ppc is stronger), which match that
unlock store-release you are concerned about, this is as of 726328d92a4
(locking/spinlock, arch: Update and fix spin_unlock_wait() implementations).

This is exactly what you are doing by upgrading the ctrl dependency to 
the acquire barrier in http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n291
and therefore we don't need it explicitly -- it also makes the comment
wrt spin_unlock_wait obsolete. Or am I'm misunderstanding you?

Thanks,
Davidlohr

[toc] | [next] | [standalone]

#1460078

On Wed, Aug 10, 2016 at 03:23:16PM -0700, Davidlohr Bueso wrote:
> On Wed, 10 Aug 2016, Paul E. McKenney wrote:
> 
> >On Wed, Aug 10, 2016 at 08:21:22PM +0200, Manfred Spraul wrote:
> 
> >>                                                              4)
> >>spin_unlock_wait() and spin_unlock() pair
> >>http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n291
> >>http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n409
> >>The data from the simple op must be observed by the following
> >>complex op. Right now, there is still an smp_rmb() in line 300: The
> >>control barrier from the loop inside spin_unlock_wait() is upgraded
> >>to an acquire barrier by an additional smp_rmb(). Is this smp_rmb()
> >>required? If I understand commit 2c6100227116 ("locking/qspinlock:
> >>Fix spin_unlock_wait() some more") right, with this commit qspinlock
> >>handle this case without the smp_rmb(). What I don't know if powerpc
> >>is using qspinlock already, or if powerpc works without the
> >>smp_rmb(). -- Manfred|
> 
> No, ppc doesn't use qspinlocks, but as mentioned, spin_unlock_wait for
> tickets are now at least an acquire (ppc is stronger), which match that
> unlock store-release you are concerned about, this is as of 726328d92a4
> (locking/spinlock, arch: Update and fix spin_unlock_wait() implementations).
> 
> This is exactly what you are doing by upgrading the ctrl dependency
> to the acquire barrier in
> http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n291
> and therefore we don't need it explicitly -- it also makes the comment
> wrt spin_unlock_wait obsolete. Or am I'm misunderstanding you?

Ah, I was looking at 4.7 rather than current mainline.  Perhaps Manfred
was doing the same.

							Thanx, Paul

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

#1460093

On Wed, 10 Aug 2016, Paul E. McKenney wrote:

>On Wed, Aug 10, 2016 at 03:23:16PM -0700, Davidlohr Bueso wrote:
>> On Wed, 10 Aug 2016, Paul E. McKenney wrote:
>>
>> >On Wed, Aug 10, 2016 at 08:21:22PM +0200, Manfred Spraul wrote:
>>
>> >>                                                              4)
>> >>spin_unlock_wait() and spin_unlock() pair
>> >>http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n291
>> >>http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n409
>> >>The data from the simple op must be observed by the following
>> >>complex op. Right now, there is still an smp_rmb() in line 300: The
>> >>control barrier from the loop inside spin_unlock_wait() is upgraded
>> >>to an acquire barrier by an additional smp_rmb(). Is this smp_rmb()
>> >>required? If I understand commit 2c6100227116 ("locking/qspinlock:
>> >>Fix spin_unlock_wait() some more") right, with this commit qspinlock
>> >>handle this case without the smp_rmb(). What I don't know if powerpc
>> >>is using qspinlock already, or if powerpc works without the
>> >>smp_rmb(). -- Manfred|
>>
>> No, ppc doesn't use qspinlocks, but as mentioned, spin_unlock_wait for
>> tickets are now at least an acquire (ppc is stronger), which match that
>> unlock store-release you are concerned about, this is as of 726328d92a4
>> (locking/spinlock, arch: Update and fix spin_unlock_wait() implementations).
>>
>> This is exactly what you are doing by upgrading the ctrl dependency
>> to the acquire barrier in
>> http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n291
>> and therefore we don't need it explicitly -- it also makes the comment
>> wrt spin_unlock_wait obsolete. Or am I'm misunderstanding you?
>
>Ah, I was looking at 4.7 rather than current mainline.  Perhaps Manfred
>was doing the same.

Right, and therefore backporting gets icky as any versions < 4.8 will
require this explicit smp_rmb :-(  Given that the this complex vs simple
ops race goes way back to 3.12, I see these options:

(1) As Manfred suggested, have a patch 1 that fixes the race against mainline
with the redundant smp_rmb, then apply a second patch that gets rid of it
for mainline, but only backport the original patch 1 down to 3.12.

(2) Backport 726328d92a4 all the way down to 3.12.

(3) Have two patches, one for upstream and one for backporting (not sure how
that would fly though).

I'm in favor of (1) as it seems the least error prone, but long as we do get
rid of the redundant barrier. For the case of any smp_mb__after_unlock_lock
calls we end up needing for ppc, this would probably need backporting as is
afaict.

Thanks,
Davidlohr

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

#1460246

On Wed, Aug 10, 2016 at 04:29:22PM -0700, Davidlohr Bueso wrote:

> (1) As Manfred suggested, have a patch 1 that fixes the race against mainline
> with the redundant smp_rmb, then apply a second patch that gets rid of it
> for mainline, but only backport the original patch 1 down to 3.12.

I have not followed the thread closely, but this seems like the best
option. Esp. since 726328d92a42 ("locking/spinlock, arch: Update and fix
spin_unlock_wait() implementations") is incomplete, it relies on at
least 6262db7c088b ("powerpc/spinlock: Fix spin_unlock_wait()") to sort
PPC.

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

#1460734

On Thu, 11 Aug 2016, Peter Zijlstra wrote:

>On Wed, Aug 10, 2016 at 04:29:22PM -0700, Davidlohr Bueso wrote:
>
>> (1) As Manfred suggested, have a patch 1 that fixes the race against mainline
>> with the redundant smp_rmb, then apply a second patch that gets rid of it
>> for mainline, but only backport the original patch 1 down to 3.12.
>
>I have not followed the thread closely, but this seems like the best
>option. Esp. since 726328d92a42 ("locking/spinlock, arch: Update and fix
>spin_unlock_wait() implementations") is incomplete, it relies on at
>least 6262db7c088b ("powerpc/spinlock: Fix spin_unlock_wait()") to sort
>PPC.

Yeah, and we'd also need the arm bits; which reminds me, aren't alpha
ldl_l/stl_c sequences also exposed to this delaying of the publishing
when a non-owner peeks at the lock? Right now sysv sem's would be busted
when doing either is_locked or unlock_wait, shouldn't these be pimped up
to full smp_mb()s?

Thanks,
Davidlohr

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

#1460925

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

On Thu, Aug 11, 2016 at 11:31:06AM -0700, Davidlohr Bueso wrote:
> On Thu, 11 Aug 2016, Peter Zijlstra wrote:
> 
> > On Wed, Aug 10, 2016 at 04:29:22PM -0700, Davidlohr Bueso wrote:
> > 
> > > (1) As Manfred suggested, have a patch 1 that fixes the race against mainline
> > > with the redundant smp_rmb, then apply a second patch that gets rid of it
> > > for mainline, but only backport the original patch 1 down to 3.12.
> > 
> > I have not followed the thread closely, but this seems like the best
> > option. Esp. since 726328d92a42 ("locking/spinlock, arch: Update and fix
> > spin_unlock_wait() implementations") is incomplete, it relies on at
> > least 6262db7c088b ("powerpc/spinlock: Fix spin_unlock_wait()") to sort
> > PPC.
> 
> Yeah, and we'd also need the arm bits; which reminds me, aren't alpha
> ldl_l/stl_c sequences also exposed to this delaying of the publishing
> when a non-owner peeks at the lock? Right now sysv sem's would be busted
> when doing either is_locked or unlock_wait, shouldn't these be pimped up
> to full smp_mb()s?
> 

You are talking about a similar problem as this one:

http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1018307.html

right?

The trick of this problem is whether the barrier or operation in
spin_lock() could order the STORE part of the lock-acquire with memory
operations in critical sections.

On PPC, we use lwsync, which doesn't order STORE->LOAD, so there is
problem. On ARM64 and qspinlock in x86, there are similiar reasons.

But if an arch implements its spin_lock() with a full barrier, even
though the atomic is implemented by ll/sc, the STORE part of which can't
be reordered with memory operations in the critcal sections. I think
maybe that's the case for alpha(and also for ARM32).

Regards,
Boqun

> Thanks,
> Davidlohr
> 

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

#1466457

On Fri, Aug 12, 2016 at 10:59:46AM +0800, Boqun Feng wrote:
> But if an arch implements its spin_lock() with a full barrier, even
> though the atomic is implemented by ll/sc, the STORE part of which can't
> be reordered with memory operations in the critcal sections. I think
> maybe that's the case for alpha(and also for ARM32).

Correct, Alpha only has a full fence and uses that after the ll/sc to
provide acquire semantics, ARM has other barriers but too uses a full
barrier here.

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

#1460105

On Wed, 2016-08-10 at 15:23 -0700, Davidlohr Bueso wrote:
> On Wed, 10 Aug 2016, Paul E. McKenney wrote:
> 
> > 
> > On Wed, Aug 10, 2016 at 08:21:22PM +0200, Manfred Spraul wrote:
>                                                               4)
> > > spin_unlock_wait() and spin_unlock() pair
> > > http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n291
> > > http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n409
> > > The data from the simple op must be observed by the following
> > > complex op. Right now, there is still an smp_rmb() in line 300: The
> > > control barrier from the loop inside spin_unlock_wait() is upgraded
> > > to an acquire barrier by an additional smp_rmb(). Is this smp_rmb()
> > > required? If I understand commit 2c6100227116 ("locking/qspinlock:
> > > Fix spin_unlock_wait() some more") right, with this commit qspinlock
> > > handle this case without the smp_rmb(). What I don't know if powerpc
> > > is using qspinlock already, or if powerpc works without the
> > > smp_rmb(). -- Manfred|
> 
> > No, ppc doesn't use qspinlocks, 

 ... yet. There are patches pending to add support for them

> but as mentioned, spin_unlock_wait for
> > tickets are now at least an acquire (ppc is stronger), 

The unlock path for qspinlock for us will be a release.

> which match that
> unlock store-release you are concerned about, this is as of 726328d92a4
> (locking/spinlock, arch: Update and fix spin_unlock_wait() implementations).
> 
> This is exactly what you are doing by upgrading the ctrl dependency to 
> the acquire barrier in http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/tree/ipc/sem.c#n291
> and therefore we don't need it explicitly -- it also makes the comment
> wrt spin_unlock_wait obsolete. Or am I'm misunderstanding you?

Ben.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web