From: Boqun Feng <boqun.feng@gmail.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>,
linux-kernel@vger.kernel.org, torvalds@linux-foundation.org,
manfred@colorfullife.com, dave@stgolabs.net,
paulmck@linux.vnet.ibm.com, Waiman.Long@hpe.com, tj@kernel.org,
pablo@netfilter.org, kaber@trash.net, davem@davemloft.net,
oleg@redhat.com, netfilter-devel@vger.kernel.org,
sasha.levin@oracle.com, hofrat@osadl.org, jejb@parisc-linux.org,
chris@zankel.net, rth@twiddle.net, dhowells@redhat.com,
schwidefsky@de.ibm.com, mpe@ellerman.id.au, ralf@linux-mips.org,
linux@armlinux.org.uk, rkuo@codeaurora.org, vgupta@synopsys.com,
james.hogan@imgtec.com, realmz6@gmail.com,
ysato@users.sourceforge.jp, tony.luck@intel.com,
cmetcalf@mellanox.com
Subject: Re: [PATCH -v4 5/7] locking, arch: Update spin_unlock_wait()
Date: Tue, 7 Jun 2016 20:45:53 +0800 [thread overview]
Message-ID: <20160607124553.GG23133@insomnia> (raw)
In-Reply-To: <20160607120016.GG30154@twins.programming.kicks-ass.net>
[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]
On Tue, Jun 07, 2016 at 02:00:16PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 07, 2016 at 07:43:15PM +0800, Boqun Feng wrote:
> > On Mon, Jun 06, 2016 at 06:08:36PM +0200, Peter Zijlstra wrote:
> > > diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
> > > index ce2f75e32ae1..e1c29d352e0e 100644
> > > --- a/kernel/locking/qspinlock.c
> > > +++ b/kernel/locking/qspinlock.c
> > > @@ -395,6 +395,8 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
> > > * pending stuff.
> > > *
> > > * p,*,* -> n,*,*
> > > + *
> > > + * RELEASE, such that the stores to @node must be complete.
> > > */
> > > old = xchg_tail(lock, tail);
> > > next = NULL;
> > > @@ -405,6 +407,15 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
> > > */
> > > if (old & _Q_TAIL_MASK) {
> > > prev = decode_tail(old);
> > > + /*
> > > + * The above xchg_tail() is also load of @lock which generates,
> > > + * through decode_tail(), a pointer.
> > > + *
> > > + * The address dependency matches the RELEASE of xchg_tail()
> > > + * such that the access to @prev must happen after.
> > > + */
> > > + smp_read_barrier_depends();
> >
> > Should this barrier be put before decode_tail()? Because it's the
> > dependency old -> prev that we want to protect here.
>
> I don't think it matters one way or the other. The old->prev
> transformation is pure; it doesn't depend on any state other than old.
>
But wouldn't the old -> prev transformation get broken on Alpha
semantically in theory? Because Alpha could reorder the LOAD part of the
xchg_tail() and decode_tail(), which results in prev points to other
cpu's mcs_node.
Though, this is fine in current code, because xchg_release() is actually
xchg() on Alpha, and Alpha doesn't use qspinlock.
> I put it between prev and dereferences of prev, because that's what made
> most sense to me; but really anywhere between the load of @old and the
> first dereference of @prev is fine I suspect.
I understand the barrier here serves more for a documentation purpose,
and I don't want to a paranoid ;-) I'm fine with the current place, just
thought we could put it at somewhere more conforming to our memory
model.
/me feel myself a paranoid anyway...
Regards,
Boqun
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
next prev parent reply other threads:[~2016-06-07 12:42 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-02 11:51 [PATCH -v4 0/7] spin_unlock_wait borkage and assorted bits Peter Zijlstra
2016-06-02 11:51 ` [PATCH -v4 1/7] locking: Replace smp_cond_acquire with smp_cond_load_acquire Peter Zijlstra
2016-06-02 11:51 ` [PATCH -v4 2/7] locking: Introduce smp_acquire__after_ctrl_dep Peter Zijlstra
2016-06-02 11:52 ` [PATCH -v4 3/7] locking: Move smp_cond_load_acquire() to asm-generic/barrier.h Peter Zijlstra
2016-06-02 11:52 ` [PATCH -v4 4/7] locking, tile: Provide TILE specific smp_acquire__after_ctrl_dep Peter Zijlstra
2016-06-02 11:52 ` [PATCH -v4 5/7] locking, arch: Update spin_unlock_wait() Peter Zijlstra
2016-06-02 14:24 ` Boqun Feng
2016-06-02 14:44 ` Peter Zijlstra
2016-06-02 15:11 ` Boqun Feng
2016-06-02 15:57 ` Boqun Feng
2016-06-02 16:04 ` Peter Zijlstra
2016-06-02 16:34 ` Peter Zijlstra
2016-06-02 17:57 ` Will Deacon
2016-06-02 21:51 ` Peter Zijlstra
2016-06-03 12:47 ` Will Deacon
2016-06-03 13:42 ` Peter Zijlstra
2016-06-03 17:35 ` Will Deacon
2016-06-03 19:13 ` Peter Zijlstra
2016-06-03 13:48 ` Peter Zijlstra
2016-06-06 16:08 ` Peter Zijlstra
2016-06-07 11:43 ` Boqun Feng
2016-06-07 12:00 ` Peter Zijlstra
2016-06-07 12:45 ` Boqun Feng [this message]
2016-06-07 17:36 ` Peter Zijlstra
2016-06-02 11:52 ` [PATCH -v4 6/7] locking: Update spin_unlock_wait users Peter Zijlstra
2016-06-02 11:52 ` [PATCH -v4 7/7] locking,netfilter: Fix nf_conntrack_lock() Peter Zijlstra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160607124553.GG23133@insomnia \
--to=boqun.feng@gmail.com \
--cc=Waiman.Long@hpe.com \
--cc=chris@zankel.net \
--cc=cmetcalf@mellanox.com \
--cc=dave@stgolabs.net \
--cc=davem@davemloft.net \
--cc=dhowells@redhat.com \
--cc=hofrat@osadl.org \
--cc=james.hogan@imgtec.com \
--cc=jejb@parisc-linux.org \
--cc=kaber@trash.net \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=manfred@colorfullife.com \
--cc=mpe@ellerman.id.au \
--cc=netfilter-devel@vger.kernel.org \
--cc=oleg@redhat.com \
--cc=pablo@netfilter.org \
--cc=paulmck@linux.vnet.ibm.com \
--cc=peterz@infradead.org \
--cc=ralf@linux-mips.org \
--cc=realmz6@gmail.com \
--cc=rkuo@codeaurora.org \
--cc=rth@twiddle.net \
--cc=sasha.levin@oracle.com \
--cc=schwidefsky@de.ibm.com \
--cc=tj@kernel.org \
--cc=tony.luck@intel.com \
--cc=torvalds@linux-foundation.org \
--cc=vgupta@synopsys.com \
--cc=will.deacon@arm.com \
--cc=ysato@users.sourceforge.jp \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).