Date: Wed, 19 Apr 2017 16:23:52 -0700
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: linux-kernel@vger.kernel.org, mingo@kernel.org, jiangshanlai@gmail.com,
        dipankar@in.ibm.com, akpm@linux-foundation.org,
        mathieu.desnoyers@efficios.com, josh@joshtriplett.org,
        tglx@linutronix.de, rostedt@goodmis.org, dhowells@redhat.com,
        edumazet@google.com, fweisbec@gmail.com, oleg@redhat.com,
        bobby.prani@gmail.com, dvyukov@google.com, will.deacon@arm.com
Subject: Re: [PATCH tip/core/rcu 07/13] rcu: Add smp_mb__after_atomic() to
 sync_exp_work_done()
Reply-To: paulmck@linux.vnet.ibm.com
References: <20170412165441.GA17149@linux.vnet.ibm.com>
 <1492016149-18834-7-git-send-email-paulmck@linux.vnet.ibm.com>
 <20170413091832.phnfppqjjy6sislo@hirez.programming.kicks-ass.net>
 <20170413161042.GA3956@linux.vnet.ibm.com>
 <20170413162409.q5gsqfytjyirgfep@hirez.programming.kicks-ass.net>
 <20170413165755.GJ3956@linux.vnet.ibm.com>
 <20170413171027.snjqn4u54t2kdzgx@hirez.programming.kicks-ass.net>
 <20170413173951.GM3956@linux.vnet.ibm.com>
 <20170413175136.5qnzvqrmzyuvlqsj@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170413175136.5qnzvqrmzyuvlqsj@hirez.programming.kicks-ass.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Message-Id: <20170419232352.GC3956@linux.vnet.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5055
Lines: 117

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);