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

Groups > linux.kernel > #1622347 > unrolled thread

[PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

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

  [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-12 19:00 +0200
    Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to  sync_exp_work_done() Peter Zijlstra <peterz@infradead.org> - 2017-04-13 11:20 +0200
      Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to  sync_exp_work_done() Peter Zijlstra <peterz@infradead.org> - 2017-04-13 11:40 +0200
      Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to  sync_exp_work_done() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-13 18:20 +0200
        Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to  sync_exp_work_done() Peter Zijlstra <peterz@infradead.org> - 2017-04-13 18:30 +0200
          Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to  sync_exp_work_done() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-13 19:00 +0200
            Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to  sync_exp_work_done() Peter Zijlstra <peterz@infradead.org> - 2017-04-13 19:20 +0200
              Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to  sync_exp_work_done() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-13 19:50 +0200
                Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to  sync_exp_work_done() Peter Zijlstra <peterz@infradead.org> - 2017-04-13 20:00 +0200
                  Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to  sync_exp_work_done() Peter Zijlstra <peterz@infradead.org> - 2017-04-13 20:10 +0200
                    Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to  sync_exp_work_done() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-20 01:30 +0200
                  Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to  sync_exp_work_done() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-20 01:30 +0200
                    Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to  sync_exp_work_done() Peter Zijlstra <peterz@infradead.org> - 2017-04-20 13:20 +0200
                      Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to  sync_exp_work_done() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-20 17:10 +0200
                        Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to  sync_exp_work_done() Peter Zijlstra <peterz@infradead.org> - 2017-04-20 17:10 +0200

#1622347 — [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

The sync_exp_work_done() function needs to fully order the
counter-check operation against anything happening after the
corresponding grace period.  This is a theoretical bug, as all current
architectures either provide full ordering for atomic operations
on the one hand or implement smp_mb__before_atomic() as smp_mb()
on the other.  However, a little future-proofing is a good thing,
especially given that smp_mb__before_atomic() is only required to
provide acquire semantics rather than full ordering.  This commit
therefore adds smp_mb__after_atomic() after the atomic_long_inc()
in sync_exp_work_done().

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcu/tree_exp.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index a7b639ccd46e..e0cafa5f3269 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -247,6 +247,7 @@ static bool sync_exp_work_done(struct rcu_state *rsp, atomic_long_t *stat,
 		/* Ensure test happens before caller kfree(). */
 		smp_mb__before_atomic(); /* ^^^ */
 		atomic_long_inc(stat);
+		smp_mb__after_atomic(); /* ^^^ */
 		return true;
 	}
 	return false;
-- 
2.5.2

[toc] | [next] | [standalone]

#1622844 — Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

On Wed, Apr 12, 2017 at 09:55:43AM -0700, Paul E. McKenney wrote:
> However, a little future-proofing is a good thing,
> especially given that smp_mb__before_atomic() is only required to
> provide acquire semantics rather than full ordering.  This commit
> therefore adds smp_mb__after_atomic() after the atomic_long_inc()
> in sync_exp_work_done().

Oh!? As far as I'm away the smp_mb__{before,after}_atomic() really must
provide full MB, no confusion about that.

We have other primitives for acquire/release.

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

#1622858 — Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

On Thu, Apr 13, 2017 at 11:18:32AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 12, 2017 at 09:55:43AM -0700, Paul E. McKenney wrote:
> > However, a little future-proofing is a good thing,
> > especially given that smp_mb__before_atomic() is only required to
> > provide acquire semantics rather than full ordering.  This commit
> > therefore adds smp_mb__after_atomic() after the atomic_long_inc()
> > in sync_exp_work_done().
> 
> Oh!? As far as I'm away the smp_mb__{before,after}_atomic() really must

s/away/aware/ typing hard

> provide full MB, no confusion about that.
> 
> We have other primitives for acquire/release.

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

#1623157 — Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

On Thu, Apr 13, 2017 at 11:18:32AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 12, 2017 at 09:55:43AM -0700, Paul E. McKenney wrote:
> > However, a little future-proofing is a good thing,
> > especially given that smp_mb__before_atomic() is only required to
> > provide acquire semantics rather than full ordering.  This commit
> > therefore adds smp_mb__after_atomic() after the atomic_long_inc()
> > in sync_exp_work_done().
> 
> Oh!? As far as I'm away the smp_mb__{before,after}_atomic() really must
> provide full MB, no confusion about that.
> 
> We have other primitives for acquire/release.

Hmmm...  Rechecking atomic_ops.txt, it does appear that you are quite
correct.  Adding Will and Dmitry on CC, but dropping this patch for now.

							Thanx, Paul

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

#1623168 — Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

On Thu, Apr 13, 2017 at 09:10:42AM -0700, Paul E. McKenney wrote:
> On Thu, Apr 13, 2017 at 11:18:32AM +0200, Peter Zijlstra wrote:
> > On Wed, Apr 12, 2017 at 09:55:43AM -0700, Paul E. McKenney wrote:
> > > However, a little future-proofing is a good thing,
> > > especially given that smp_mb__before_atomic() is only required to
> > > provide acquire semantics rather than full ordering.  This commit
> > > therefore adds smp_mb__after_atomic() after the atomic_long_inc()
> > > in sync_exp_work_done().
> > 
> > Oh!? As far as I'm away the smp_mb__{before,after}_atomic() really must
> > provide full MB, no confusion about that.
> > 
> > We have other primitives for acquire/release.
> 
> Hmmm...  Rechecking atomic_ops.txt, it does appear that you are quite
> correct.  Adding Will and Dmitry on CC, but dropping this patch for now.

I'm afraid that document is woefully out dated. I'm surprised it says
anything on the subject.

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

#1623188 — Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

On Thu, Apr 13, 2017 at 06:24:09PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 13, 2017 at 09:10:42AM -0700, Paul E. McKenney wrote:
> > On Thu, Apr 13, 2017 at 11:18:32AM +0200, Peter Zijlstra wrote:
> > > On Wed, Apr 12, 2017 at 09:55:43AM -0700, Paul E. McKenney wrote:
> > > > However, a little future-proofing is a good thing,
> > > > especially given that smp_mb__before_atomic() is only required to
> > > > provide acquire semantics rather than full ordering.  This commit
> > > > therefore adds smp_mb__after_atomic() after the atomic_long_inc()
> > > > in sync_exp_work_done().
> > > 
> > > Oh!? As far as I'm away the smp_mb__{before,after}_atomic() really must
> > > provide full MB, no confusion about that.
> > > 
> > > We have other primitives for acquire/release.
> > 
> > Hmmm...  Rechecking atomic_ops.txt, it does appear that you are quite
> > correct.  Adding Will and Dmitry on CC, but dropping this patch for now.
> 
> I'm afraid that document is woefully out dated. I'm surprised it says
> anything on the subject.

And there is some difference of opinion.  Some believe that the
smp_mb__before_atomic() only guarantees acquire and smp_mb__after_atomic()
only guarantees release, but all current architectures provide full
ordering, as you noted and as stated in atomic_ops.txt.

How do we decide?

Once we do decide, atomic_ops.txt of course needs to be updated accordingly.

							Thanx, Paul

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

#1623208 — Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

On Thu, Apr 13, 2017 at 09:57:55AM -0700, Paul E. McKenney wrote:
> On Thu, Apr 13, 2017 at 06:24:09PM +0200, Peter Zijlstra wrote:
> > On Thu, Apr 13, 2017 at 09:10:42AM -0700, Paul E. McKenney wrote:
> > > On Thu, Apr 13, 2017 at 11:18:32AM +0200, Peter Zijlstra wrote:
> > > > On Wed, Apr 12, 2017 at 09:55:43AM -0700, Paul E. McKenney wrote:
> > > > > However, a little future-proofing is a good thing,
> > > > > especially given that smp_mb__before_atomic() is only required to
> > > > > provide acquire semantics rather than full ordering.  This commit
> > > > > therefore adds smp_mb__after_atomic() after the atomic_long_inc()
> > > > > in sync_exp_work_done().
> > > > 
> > > > Oh!? As far as I'm away the smp_mb__{before,after}_atomic() really must
> > > > provide full MB, no confusion about that.
> > > > 
> > > > We have other primitives for acquire/release.
> > > 
> > > Hmmm...  Rechecking atomic_ops.txt, it does appear that you are quite
> > > correct.  Adding Will and Dmitry on CC, but dropping this patch for now.
> > 
> > I'm afraid that document is woefully out dated. I'm surprised it says
> > anything on the subject.
> 
> And there is some difference of opinion.  Some believe that the
> smp_mb__before_atomic() only guarantees acquire and smp_mb__after_atomic()
> only guarantees release, but all current architectures provide full
> ordering, as you noted and as stated in atomic_ops.txt.

Which 'some' think it only provides acquire/release ?

I made very sure -- when I renamed/audited/wrote all this -- that they
indeed do a full memory barrier.

> How do we decide?

I say its a full mb, always was.

People used it to create acquire/release _like_ constructs, because we
simply didn't have anything else.

Also, I think Linus once opined that acquire/release is part of a
store/load (hence smp_store_release/smp_load_acquire) and not a barrier.

> Once we do decide, atomic_ops.txt of course needs to be updated accordingly.

There was so much missing there that I didn't quite know where to start.

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

#1623221 — Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

On Thu, Apr 13, 2017 at 07:10:27PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 13, 2017 at 09:57:55AM -0700, Paul E. McKenney wrote:
> > On Thu, Apr 13, 2017 at 06:24:09PM +0200, Peter Zijlstra wrote:
> > > On Thu, Apr 13, 2017 at 09:10:42AM -0700, Paul E. McKenney wrote:
> > > > On Thu, Apr 13, 2017 at 11:18:32AM +0200, Peter Zijlstra wrote:
> > > > > On Wed, Apr 12, 2017 at 09:55:43AM -0700, Paul E. McKenney wrote:
> > > > > > However, a little future-proofing is a good thing,
> > > > > > especially given that smp_mb__before_atomic() is only required to
> > > > > > provide acquire semantics rather than full ordering.  This commit
> > > > > > therefore adds smp_mb__after_atomic() after the atomic_long_inc()
> > > > > > in sync_exp_work_done().
> > > > > 
> > > > > Oh!? As far as I'm away the smp_mb__{before,after}_atomic() really must
> > > > > provide full MB, no confusion about that.
> > > > > 
> > > > > We have other primitives for acquire/release.
> > > > 
> > > > Hmmm...  Rechecking atomic_ops.txt, it does appear that you are quite
> > > > correct.  Adding Will and Dmitry on CC, but dropping this patch for now.
> > > 
> > > I'm afraid that document is woefully out dated. I'm surprised it says
> > > anything on the subject.
> > 
> > And there is some difference of opinion.  Some believe that the
> > smp_mb__before_atomic() only guarantees acquire and smp_mb__after_atomic()
> > only guarantees release, but all current architectures provide full
> > ordering, as you noted and as stated in atomic_ops.txt.
> 
> Which 'some' think it only provides acquire/release ?
> 
> I made very sure -- when I renamed/audited/wrote all this -- that they
> indeed do a full memory barrier.
> 
> > How do we decide?
> 
> I say its a full mb, always was.
> 
> People used it to create acquire/release _like_ constructs, because we
> simply didn't have anything else.
> 
> Also, I think Linus once opined that acquire/release is part of a
> store/load (hence smp_store_release/smp_load_acquire) and not a barrier.
> 
> > Once we do decide, atomic_ops.txt of course needs to be updated accordingly.
> 
> There was so much missing there that I didn't quite know where to start.

Well, if there are no objections, I will fix up the smp_mb__before_atomic()
and smp_mb__after_atomic() pieces.

I suppose that one alternative is the new variant of kerneldoc, though
very few of these functions have comment headers, let alone kerneldoc
headers.  Which reminds me, the question of spin_unlock_wait() and
spin_is_locked() semantics came up a bit ago.  Here is what I believe
to be the case.  Does this match others' expectations?

o	spin_unlock_wait() semantics:

	1.	Any access in any critical section prior to the
		spin_unlock_wait() is visible to all code following
		(in program order) the spin_unlock_wait().

	2.	Any access prior (in program order) to the
		spin_unlock_wait() is visible to any critical
		section following the spin_unlock_wait().

o	spin_is_locked() semantics: Half of spin_unlock_wait(),
	but only if it returns false:

	1.	Any access in any critical section prior to the
		spin_unlock_wait() is visible to all code following
		(in program order) the spin_unlock_wait().

							Thanx, Paul

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

#1623230 — Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

On Thu, Apr 13, 2017 at 10:39:51AM -0700, Paul E. McKenney wrote:

> Well, if there are no objections, I will fix up the smp_mb__before_atomic()
> and smp_mb__after_atomic() pieces.

Feel free.

> I suppose that one alternative is the new variant of kerneldoc, though
> very few of these functions have comment headers, let alone kerneldoc
> headers.  Which reminds me, the question of spin_unlock_wait() and
> spin_is_locked() semantics came up a bit ago.  Here is what I believe
> to be the case.  Does this match others' expectations?
> 
> o	spin_unlock_wait() semantics:
> 
> 	1.	Any access in any critical section prior to the
> 		spin_unlock_wait() is visible to all code following
> 		(in program order) the spin_unlock_wait().
> 
> 	2.	Any access prior (in program order) to the
> 		spin_unlock_wait() is visible to any critical
> 		section following the spin_unlock_wait().
> 
> o	spin_is_locked() semantics: Half of spin_unlock_wait(),
> 	but only if it returns false:
> 
> 	1.	Any access in any critical section prior to the
> 		spin_unlock_wait() is visible to all code following
> 		(in program order) the spin_unlock_wait().

Urgh.. yes those are pain. The best advise is to not use them.

  055ce0fd1b86 ("locking/qspinlock: Add comments")

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

#1623237 — Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

On Thu, Apr 13, 2017 at 07:51:36PM +0200, Peter Zijlstra wrote:

> > I suppose that one alternative is the new variant of kerneldoc, though
> > very few of these functions have comment headers, let alone kerneldoc
> > headers.  Which reminds me, the question of spin_unlock_wait() and
> > spin_is_locked() semantics came up a bit ago.  Here is what I believe
> > to be the case.  Does this match others' expectations?
> > 
> > o	spin_unlock_wait() semantics:
> > 
> > 	1.	Any access in any critical section prior to the
> > 		spin_unlock_wait() is visible to all code following
> > 		(in program order) the spin_unlock_wait().
> > 
> > 	2.	Any access prior (in program order) to the
> > 		spin_unlock_wait() is visible to any critical
> > 		section following the spin_unlock_wait().
> > 
> > o	spin_is_locked() semantics: Half of spin_unlock_wait(),
> > 	but only if it returns false:
> > 
> > 	1.	Any access in any critical section prior to the
> > 		spin_unlock_wait() is visible to all code following
> > 		(in program order) the spin_unlock_wait().
> 
> Urgh.. yes those are pain. The best advise is to not use them.
> 
>   055ce0fd1b86 ("locking/qspinlock: Add comments")

The big problem with spin_unlock_wait(), aside from the icky barrier
semantics, is that it tends to end up prone to starvation. So where
spin_lock()+spin_unlock() have guaranteed fwd progress if the lock is
fair (ticket,queued,etc..) spin_unlock_wait() must often lack that
guarantee.

Equally, spin_unlock_wait() was intended to be 'cheap' and be a
read-only loop, but in order to satisfy the barrier requirements, it
ends up doing stores anyway (see for example the arm64 and ppc
implementations).

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

#1626874 — Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

On Thu, Apr 13, 2017 at 07:59:07PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 13, 2017 at 07:51:36PM +0200, Peter Zijlstra wrote:
> 
> > > I suppose that one alternative is the new variant of kerneldoc, though
> > > very few of these functions have comment headers, let alone kerneldoc
> > > headers.  Which reminds me, the question of spin_unlock_wait() and
> > > spin_is_locked() semantics came up a bit ago.  Here is what I believe
> > > to be the case.  Does this match others' expectations?
> > > 
> > > o	spin_unlock_wait() semantics:
> > > 
> > > 	1.	Any access in any critical section prior to the
> > > 		spin_unlock_wait() is visible to all code following
> > > 		(in program order) the spin_unlock_wait().
> > > 
> > > 	2.	Any access prior (in program order) to the
> > > 		spin_unlock_wait() is visible to any critical
> > > 		section following the spin_unlock_wait().
> > > 
> > > o	spin_is_locked() semantics: Half of spin_unlock_wait(),
> > > 	but only if it returns false:
> > > 
> > > 	1.	Any access in any critical section prior to the
> > > 		spin_unlock_wait() is visible to all code following
> > > 		(in program order) the spin_unlock_wait().
> > 
> > Urgh.. yes those are pain. The best advise is to not use them.
> > 
> >   055ce0fd1b86 ("locking/qspinlock: Add comments")
> 
> The big problem with spin_unlock_wait(), aside from the icky barrier
> semantics, is that it tends to end up prone to starvation. So where
> spin_lock()+spin_unlock() have guaranteed fwd progress if the lock is
> fair (ticket,queued,etc..) spin_unlock_wait() must often lack that
> guarantee.
> 
> Equally, spin_unlock_wait() was intended to be 'cheap' and be a
> read-only loop, but in order to satisfy the barrier requirements, it
> ends up doing stores anyway (see for example the arm64 and ppc
> implementations).

Good points, and my proposed patch includes verbiage urging the use
of something else to get the job done.  Does that work?

							Thanx, Paul

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

#1626875 — Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

On Thu, Apr 13, 2017 at 07:51:36PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 13, 2017 at 10:39:51AM -0700, Paul E. McKenney wrote:
> 
> > Well, if there are no objections, I will fix up the smp_mb__before_atomic()
> > and smp_mb__after_atomic() pieces.
> 
> Feel free.

How about if I add this in the atomic_ops.txt description of these 
two primitives?

	Preceding a non-value-returning read-modify-write atomic
	operation with smp_mb__before_atomic() and following it with
	smp_mb__after_atomic() provides the same full ordering that is
	provided by value-returning read-modify-write atomic operations.

> > I suppose that one alternative is the new variant of kerneldoc, though
> > very few of these functions have comment headers, let alone kerneldoc
> > headers.  Which reminds me, the question of spin_unlock_wait() and
> > spin_is_locked() semantics came up a bit ago.  Here is what I believe
> > to be the case.  Does this match others' expectations?
> > 
> > o	spin_unlock_wait() semantics:
> > 
> > 	1.	Any access in any critical section prior to the
> > 		spin_unlock_wait() is visible to all code following
> > 		(in program order) the spin_unlock_wait().
> > 
> > 	2.	Any access prior (in program order) to the
> > 		spin_unlock_wait() is visible to any critical
> > 		section following the spin_unlock_wait().
> > 
> > o	spin_is_locked() semantics: Half of spin_unlock_wait(),
> > 	but only if it returns false:
> > 
> > 	1.	Any access in any critical section prior to the
> > 		spin_unlock_wait() is visible to all code following
> > 		(in program order) the spin_unlock_wait().
> 
> Urgh.. yes those are pain. The best advise is to not use them.
> 
>   055ce0fd1b86 ("locking/qspinlock: Add comments")

Ah, I must confess that I missed that one.  Would you be OK with the
following patch, which adds a docbook header comment for both of them?

							Thanx, Paul

------------------------------------------------------------------------

commit 5789953adc360b4d3685dc89513655e6bfb83980
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Wed Apr 19 16:20:07 2017 -0700

    atomics: Add header comment so spin_unlock_wait() and spin_is_locked()
    
    There is material describing the ordering guarantees provided by
    spin_unlock_wait() and spin_is_locked(), but it is not necessarily
    easy to find.  This commit therefore adds a docbook header comment
    to both functions informally describing their semantics.
    
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 59248dcc6ef3..2647dc7f3ea9 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -369,11 +369,49 @@ static __always_inline int spin_trylock_irq(spinlock_t *lock)
 	raw_spin_trylock_irqsave(spinlock_check(lock), flags); \
 })
 
+/**
+ * spin_unlock_wait - Interpose between successive critical sections
+ * @lock: the spinlock whose critical sections are to be interposed.
+ *
+ * Semantically this is equivalent to a spin_lock() immediately
+ * followed by a spin_unlock().  However, most architectures have
+ * more efficient implementations in which the spin_unlock_wait()
+ * cannot block concurrent lock acquisition, and in some cases
+ * where spin_unlock_wait() does not write to the lock variable.
+ * Nevertheless, spin_unlock_wait() can have high overhead, so if
+ * you feel the need to use it, please check to see if there is
+ * a better way to get your job done.
+ *
+ * The ordering guarantees provided by spin_unlock_wait() are:
+ *
+ * 1.  All accesses preceding the spin_unlock_wait() happen before
+ *     any accesses in later critical sections for this same lock.
+ * 2.  All accesses following the spin_unlock_wait() happen after
+ *     any accesses in earlier critical sections for this same lock.
+ */
 static __always_inline void spin_unlock_wait(spinlock_t *lock)
 {
 	raw_spin_unlock_wait(&lock->rlock);
 }
 
+/**
+ * spin_is_locked - Conditionally interpose after prior critical sections
+ * @lock: the spinlock whose critical sections are to be interposed.
+ *
+ * Semantically this is equivalent to a spin_trylock(), and, if
+ * the spin_trylock() succeeds, immediately followed by a (mythical)
+ * spin_unlock_relaxed().  The return value from spin_trylock() is returned
+ * by spin_is_locked().  Note that all current architectures have extremely
+ * efficient implementations in which the spin_is_locked() does not even
+ * write to the lock variable.
+ *
+ * A successful spin_is_locked() primitive in some sense "takes its place"
+ * after some critical section for the lock in question.  Any accesses
+ * following a successful spin_is_locked() call will therefore happen
+ * after any accesses by any of the preceding critical section for that
+ * same lock.  Note however, that spin_is_locked() provides absolutely no
+ * ordering guarantees for code preceding the call to that spin_is_locked().
+ */
 static __always_inline int spin_is_locked(spinlock_t *lock)
 {
 	return raw_spin_is_locked(&lock->rlock);

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

#1627332 — Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

On Wed, Apr 19, 2017 at 04:23:52PM -0700, Paul E. McKenney wrote:
> On Thu, Apr 13, 2017 at 07:51:36PM +0200, Peter Zijlstra wrote:
> > On Thu, Apr 13, 2017 at 10:39:51AM -0700, Paul E. McKenney wrote:
> > 
> > > Well, if there are no objections, I will fix up the smp_mb__before_atomic()
> > > and smp_mb__after_atomic() pieces.
> > 
> > Feel free.
> 
> How about if I add this in the atomic_ops.txt description of these 
> two primitives?
> 
> 	Preceding a non-value-returning read-modify-write atomic
> 	operation with smp_mb__before_atomic() and following it with
> 	smp_mb__after_atomic() provides the same full ordering that is
> 	provided by value-returning read-modify-write atomic operations.

That seems correct. It also already seems a direct implication of the
extant text though. But as you're wont to say, people need repetition
and pointing out the obvious etc..

The way I read that document, specifically this:

"For example, smp_mb__before_atomic() can be used like so:

        obj->dead = 1;
        smp_mb__before_atomic();
        atomic_dec(&obj->ref_count);

It makes sure that all memory operations preceding the atomic_dec()
call are strongly ordered with respect to the atomic counter
operation."

Leaves no question that these operations must be full barriers.

And therefore, your paragraph that basically states that:

	smp_mb__before_atomic();
	atomic_inc_return_relaxed();
	smp_mb__after_atomic();

equals:

	atomic_inc_return();

is implied, no?

> commit 5789953adc360b4d3685dc89513655e6bfb83980
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date:   Wed Apr 19 16:20:07 2017 -0700
> 
>     atomics: Add header comment so spin_unlock_wait() and spin_is_locked()
>     
>     There is material describing the ordering guarantees provided by
>     spin_unlock_wait() and spin_is_locked(), but it is not necessarily
>     easy to find.  This commit therefore adds a docbook header comment
>     to both functions informally describing their semantics.
>     
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> index 59248dcc6ef3..2647dc7f3ea9 100644
> --- a/include/linux/spinlock.h
> +++ b/include/linux/spinlock.h
> @@ -369,11 +369,49 @@ static __always_inline int spin_trylock_irq(spinlock_t *lock)
>  	raw_spin_trylock_irqsave(spinlock_check(lock), flags); \
>  })
>  
> +/**
> + * spin_unlock_wait - Interpose between successive critical sections
> + * @lock: the spinlock whose critical sections are to be interposed.
> + *
> + * Semantically this is equivalent to a spin_lock() immediately
> + * followed by a spin_unlock().  However, most architectures have
> + * more efficient implementations in which the spin_unlock_wait()
> + * cannot block concurrent lock acquisition, and in some cases
> + * where spin_unlock_wait() does not write to the lock variable.
> + * Nevertheless, spin_unlock_wait() can have high overhead, so if
> + * you feel the need to use it, please check to see if there is
> + * a better way to get your job done.
> + *
> + * The ordering guarantees provided by spin_unlock_wait() are:
> + *
> + * 1.  All accesses preceding the spin_unlock_wait() happen before
> + *     any accesses in later critical sections for this same lock.
> + * 2.  All accesses following the spin_unlock_wait() happen after
> + *     any accesses in earlier critical sections for this same lock.
> + */
>  static __always_inline void spin_unlock_wait(spinlock_t *lock)
>  {
>  	raw_spin_unlock_wait(&lock->rlock);
>  }

ACK

>  
> +/**
> + * spin_is_locked - Conditionally interpose after prior critical sections
> + * @lock: the spinlock whose critical sections are to be interposed.
> + *
> + * Semantically this is equivalent to a spin_trylock(), and, if
> + * the spin_trylock() succeeds, immediately followed by a (mythical)
> + * spin_unlock_relaxed().  The return value from spin_trylock() is returned
> + * by spin_is_locked().  Note that all current architectures have extremely
> + * efficient implementations in which the spin_is_locked() does not even
> + * write to the lock variable.
> + *
> + * A successful spin_is_locked() primitive in some sense "takes its place"
> + * after some critical section for the lock in question.  Any accesses
> + * following a successful spin_is_locked() call will therefore happen
> + * after any accesses by any of the preceding critical section for that
> + * same lock.  Note however, that spin_is_locked() provides absolutely no
> + * ordering guarantees for code preceding the call to that spin_is_locked().
> + */
>  static __always_inline int spin_is_locked(spinlock_t *lock)
>  {
>  	return raw_spin_is_locked(&lock->rlock);

I'm current confused on this one. The case listed in the qspinlock code
doesn't appear to exist in the kernel anymore (or at least, I'm having
trouble finding it).

That said, I'm also not sure spin_is_locked() provides an acquire, as
that comment has an explicit smp_acquire__after_ctrl_dep();

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

#1627574 — Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

On Thu, Apr 20, 2017 at 01:17:43PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 19, 2017 at 04:23:52PM -0700, Paul E. McKenney wrote:
> > On Thu, Apr 13, 2017 at 07:51:36PM +0200, Peter Zijlstra wrote:
> > > On Thu, Apr 13, 2017 at 10:39:51AM -0700, Paul E. McKenney wrote:
> > > 
> > > > Well, if there are no objections, I will fix up the smp_mb__before_atomic()
> > > > and smp_mb__after_atomic() pieces.
> > > 
> > > Feel free.
> > 
> > How about if I add this in the atomic_ops.txt description of these 
> > two primitives?
> > 
> > 	Preceding a non-value-returning read-modify-write atomic
> > 	operation with smp_mb__before_atomic() and following it with
> > 	smp_mb__after_atomic() provides the same full ordering that is
> > 	provided by value-returning read-modify-write atomic operations.
> 
> That seems correct. It also already seems a direct implication of the
> extant text though. But as you're wont to say, people need repetition
> and pointing out the obvious etc..

Especially given that it never is obvious until you understand it.
At which point you don't need the documentation.  Therefore, documentation
is mostly useful to people who are missing a few pieces of the overall
puzzle.  Which we all were at some time in the past.  ;-)

> The way I read that document, specifically this:
> 
> "For example, smp_mb__before_atomic() can be used like so:
> 
>         obj->dead = 1;
>         smp_mb__before_atomic();
>         atomic_dec(&obj->ref_count);
> 
> It makes sure that all memory operations preceding the atomic_dec()
> call are strongly ordered with respect to the atomic counter
> operation."
> 
> Leaves no question that these operations must be full barriers.
> 
> And therefore, your paragraph that basically states that:
> 
> 	smp_mb__before_atomic();
> 	atomic_inc_return_relaxed();
> 	smp_mb__after_atomic();
> 
> equals:
> 
> 	atomic_inc_return();
> 
> is implied, no?

That is a reasonable argument, but some very intelligent people didn't
make that leap when reading it, so more redundancy appears to be needed.

> > commit 5789953adc360b4d3685dc89513655e6bfb83980
> > Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Date:   Wed Apr 19 16:20:07 2017 -0700
> > 
> >     atomics: Add header comment so spin_unlock_wait() and spin_is_locked()
> >     
> >     There is material describing the ordering guarantees provided by
> >     spin_unlock_wait() and spin_is_locked(), but it is not necessarily
> >     easy to find.  This commit therefore adds a docbook header comment
> >     to both functions informally describing their semantics.
> >     
> >     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> > index 59248dcc6ef3..2647dc7f3ea9 100644
> > --- a/include/linux/spinlock.h
> > +++ b/include/linux/spinlock.h
> > @@ -369,11 +369,49 @@ static __always_inline int spin_trylock_irq(spinlock_t *lock)
> >  	raw_spin_trylock_irqsave(spinlock_check(lock), flags); \
> >  })
> >  
> > +/**
> > + * spin_unlock_wait - Interpose between successive critical sections
> > + * @lock: the spinlock whose critical sections are to be interposed.
> > + *
> > + * Semantically this is equivalent to a spin_lock() immediately
> > + * followed by a spin_unlock().  However, most architectures have
> > + * more efficient implementations in which the spin_unlock_wait()
> > + * cannot block concurrent lock acquisition, and in some cases
> > + * where spin_unlock_wait() does not write to the lock variable.
> > + * Nevertheless, spin_unlock_wait() can have high overhead, so if
> > + * you feel the need to use it, please check to see if there is
> > + * a better way to get your job done.
> > + *
> > + * The ordering guarantees provided by spin_unlock_wait() are:
> > + *
> > + * 1.  All accesses preceding the spin_unlock_wait() happen before
> > + *     any accesses in later critical sections for this same lock.
> > + * 2.  All accesses following the spin_unlock_wait() happen after
> > + *     any accesses in earlier critical sections for this same lock.
> > + */
> >  static __always_inline void spin_unlock_wait(spinlock_t *lock)
> >  {
> >  	raw_spin_unlock_wait(&lock->rlock);
> >  }
> 
> ACK

Very good, adding your Acked-by.

> > +/**
> > + * spin_is_locked - Conditionally interpose after prior critical sections
> > + * @lock: the spinlock whose critical sections are to be interposed.
> > + *
> > + * Semantically this is equivalent to a spin_trylock(), and, if
> > + * the spin_trylock() succeeds, immediately followed by a (mythical)
> > + * spin_unlock_relaxed().  The return value from spin_trylock() is returned
> > + * by spin_is_locked().  Note that all current architectures have extremely
> > + * efficient implementations in which the spin_is_locked() does not even
> > + * write to the lock variable.
> > + *
> > + * A successful spin_is_locked() primitive in some sense "takes its place"
> > + * after some critical section for the lock in question.  Any accesses
> > + * following a successful spin_is_locked() call will therefore happen
> > + * after any accesses by any of the preceding critical section for that
> > + * same lock.  Note however, that spin_is_locked() provides absolutely no
> > + * ordering guarantees for code preceding the call to that spin_is_locked().
> > + */
> >  static __always_inline int spin_is_locked(spinlock_t *lock)
> >  {
> >  	return raw_spin_is_locked(&lock->rlock);
> 
> I'm current confused on this one. The case listed in the qspinlock code
> doesn't appear to exist in the kernel anymore (or at least, I'm having
> trouble finding it).
> 
> That said, I'm also not sure spin_is_locked() provides an acquire, as
> that comment has an explicit smp_acquire__after_ctrl_dep();

OK, I have dropped this portion of the patch for the moment.

Going forward, exactly what semantics do you believe spin_is_locked()
provides?

Do any of the current implementations need to change to provide the
semantics expected by the various use cases?

							Thanx, Paul

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

#1627578 — Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to sync_exp_work_done()

On Thu, Apr 20, 2017 at 08:03:21AM -0700, Paul E. McKenney wrote:
> On Thu, Apr 20, 2017 at 01:17:43PM +0200, Peter Zijlstra wrote:

> > > +/**
> > > + * spin_is_locked - Conditionally interpose after prior critical sections
> > > + * @lock: the spinlock whose critical sections are to be interposed.
> > > + *
> > > + * Semantically this is equivalent to a spin_trylock(), and, if
> > > + * the spin_trylock() succeeds, immediately followed by a (mythical)
> > > + * spin_unlock_relaxed().  The return value from spin_trylock() is returned
> > > + * by spin_is_locked().  Note that all current architectures have extremely
> > > + * efficient implementations in which the spin_is_locked() does not even
> > > + * write to the lock variable.
> > > + *
> > > + * A successful spin_is_locked() primitive in some sense "takes its place"
> > > + * after some critical section for the lock in question.  Any accesses
> > > + * following a successful spin_is_locked() call will therefore happen
> > > + * after any accesses by any of the preceding critical section for that
> > > + * same lock.  Note however, that spin_is_locked() provides absolutely no
> > > + * ordering guarantees for code preceding the call to that spin_is_locked().
> > > + */
> > >  static __always_inline int spin_is_locked(spinlock_t *lock)
> > >  {
> > >  	return raw_spin_is_locked(&lock->rlock);
> > 
> > I'm current confused on this one. The case listed in the qspinlock code
> > doesn't appear to exist in the kernel anymore (or at least, I'm having
> > trouble finding it).
> > 
> > That said, I'm also not sure spin_is_locked() provides an acquire, as
> > that comment has an explicit smp_acquire__after_ctrl_dep();
> 
> OK, I have dropped this portion of the patch for the moment.
> 
> Going forward, exactly what semantics do you believe spin_is_locked()
> provides?
> 
> Do any of the current implementations need to change to provide the
> semantics expected by the various use cases?

I don't have anything other than the comment I wrote back then. I would
have to go audit all spin_is_locked() implementations and users (again).

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web