Re: [PATCH tip/core/rcu 02/18] rcu: Move rcu_report_exp_rnp() to allow consolidation

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On Oct 7, 2015, at 3:51 AM, Peter Zijlstra peterz@infradead.org wrote:

> On Tue, Oct 06, 2015 at 01:58:50PM -0700, Paul E. McKenney wrote:
>> On Tue, Oct 06, 2015 at 10:29:37PM +0200, Peter Zijlstra wrote:
>> > On Tue, Oct 06, 2015 at 09:29:21AM -0700, Paul E. McKenney wrote:
>> > > +static void __maybe_unused rcu_report_exp_rnp(struct rcu_state *rsp,
>> > > +					      struct rcu_node *rnp, bool wake)
>> > > +{
>> > > +	unsigned long flags;
>> > > +	unsigned long mask;
>> > > +
>> > > +	raw_spin_lock_irqsave(&rnp->lock, flags);
>> > 
>> > Normally we require a comment with barriers, explaining the order and
>> > the pairing etc.. :-)
>> > 
>> > > +	smp_mb__after_unlock_lock();
>> 
>> Hmmmm...  That is not good.
>> 
>> Worse yet, I am missing comments on most of the pre-existing barriers
>> of this form.
> 
> Yes I noticed.. :/
> 
>> The purpose is to enforce the heavy-weight grace-period memory-ordering
>> guarantees documented in the synchronize_sched() header comment and
>> elsewhere.
> 
>> They pair with anything you might use to check for violation
>> of these guarantees, or, simiarly, any ordering that you might use when
>> relying on these guarantees.
> 
> I'm sure you know what that means, but I've no clue ;-) That is, I
> wouldn't know where to start looking in the RCU implementation to verify
> the barrier is either needed or sufficient. Unless you mean _everywhere_
> :-)

One example is the new membarrier system call. It relies on synchronize_sched()
to enforce this:

from kernel/membarrier.c:

 * All memory accesses performed in program order from each targeted thread
 * is guaranteed to be ordered with respect to sys_membarrier(). If we use
 * the semantic "barrier()" to represent a compiler barrier forcing memory
 * accesses to be performed in program order across the barrier, and
 * smp_mb() to represent explicit memory barriers forcing full memory
 * ordering across the barrier, we have the following ordering table for
 * each pair of barrier(), sys_membarrier() and smp_mb():
 *
 * The pair ordering is detailed as (O: ordered, X: not ordered):
 *
 *                        barrier()   smp_mb() sys_membarrier()
 *        barrier()          X           X            O
 *        smp_mb()           X           O            O
 *        sys_membarrier()   O           O            O

And include/uapi/linux/membarrier.h:

 * @MEMBARRIER_CMD_SHARED:  Execute a memory barrier on all running threads.
 *                          Upon return from system call, the caller thread
 *                          is ensured that all running threads have passed
 *                          through a state where all memory accesses to
 *                          user-space addresses match program order between
 *                          entry to and return from the system call
 *                          (non-running threads are de facto in such a
 *                          state). This covers threads from all processes
 *                          running on the system. This command returns 0.

I hope this sheds light on a userspace-facing interface to
synchronize_sched() and clarifies its expected semantic a bit.

Thanks,

Mathieu


> 
>> I could add something like  "/* Enforce GP memory ordering. */"
>> 
>> Or perhaps "/* See synchronize_sched() header. */"
>> 
>> I do not propose reproducing the synchronize_sched() header on each
>> of these.  That would be verbose, even for me!  ;-)
>> 
>> Other thoughts?
> 
> Well, this is an UNLOCK+LOCK on non-matching lock variables upgrade to
> full barrier thing, right?
> 
> To me its not clear which UNLOCK we even match here. I've just read the
> sync_sched() header, but that doesn't help me either, so referring to
> that isn't really helpful either.
> 
> In any case, I don't want to make too big a fuzz here, but I just
> stumbled over a lot of unannotated barriers and figured I ought to say
> something about it.

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com