Re: [PATCH RFC tip/core/rcu 05/14] rcu: Abstract sequence counting from synchronize_sched_expedited()

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Wed, Jul 01, 2015 at 12:27:17PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 30, 2015 at 03:25:45PM -0700, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > 
> > This commit creates rcu_exp_gp_seq_start() and rcu_exp_gp_seq_end() to
> > bracket an expedited grace period, rcu_exp_gp_seq_snap() to snapshot the
> > sequence counter, and rcu_exp_gp_seq_done() to check to see if a full
> > expedited grace period has elapsed since the snapshot.  These will be
> > applied to synchronize_rcu_expedited().  These are defined in terms of
> > underlying rcu_seq_start(), rcu_seq_end(), rcu_seq_snap(), rcu_seq_done(),
> > which will be applied to _rcu_barrier().
> 
> It would be good to explain why you cannot use seqcount primitives.
> They're >.< close.

They are indeed!  I gave it some thought, but it would inflict an
unnecessary smp_mb() on seqlocks, as you note below.

> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >  kernel/rcu/tree.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 58 insertions(+), 10 deletions(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index c58fd27b4a22..f96500e462fd 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -3307,6 +3307,60 @@ void cond_synchronize_sched(unsigned long oldstate)
> >  }
> >  EXPORT_SYMBOL_GPL(cond_synchronize_sched);
> >  
> > +/* Adjust sequence number for start of update-side operation. */
> > +static void rcu_seq_start(unsigned long *sp)
> > +{
> > +	WRITE_ONCE(*sp, *sp + 1);
> > +	smp_mb(); /* Ensure update-side operation after counter increment. */
> > +	WARN_ON_ONCE(!(*sp & 0x1));
> > +}
> 
> That wants to be an ACQUIRE, right?

I cannot put the acquire in the WARN_ON_ONCE() because there
are configurations where WARN_ON_ONCE() is compiled out.  I could
conditionally compile, but given that this is nothing like a fastpath,
I cannot really justify doing that.

We could define an smp_store_acquire(), but that would require a full
barrier against subsequent loads.  The C++ committee hit this one when
trying to implement seqeunce locking using the C/C++11 atomics.  ;-)

> > +
> > +/* Adjust sequence number for end of update-side operation. */
> > +static void rcu_seq_end(unsigned long *sp)
> > +{
> > +	smp_mb(); /* Ensure update-side operation before counter increment. */
> 
> And that wants to be a RELEASE, right?
> 
> > +	WRITE_ONCE(*sp, *sp + 1);
> 
> 	smp_store_release();
> 
> even if balanced against a full barrier, might be better here?

I -think- it -might- be, and if it was in a fastpath, I might be
more motivated to worry about it.  I am not so sure that pairing an
smp_store_release() with a full memory barrier is in any way an aid to
readability, though.

> > +	WARN_ON_ONCE(*sp & 0x1);
> > +}
> 
> And the only difference between these and
> raw_write_seqcount_{begin,end}() is the smp_wmb() vs your smp_mb().
> 
> Since seqcounts have a distinct read vs writer side, we really only care
> about limiting the stores. I suspect you really do care about reads
> between these 'sequence points'. A few words to that effect could
> explain the existence of these primitives.

Excellent point!  I have updated the commit log accordingly.

> > +/* Take a snapshot of the update side's sequence number. */
> > +static unsigned long rcu_seq_snap(unsigned long *sp)
> > +{
> > +	unsigned long s;
> > +
> > +	smp_mb(); /* Caller's modifications seen first by other CPUs. */
> > +	s = (READ_ONCE(*sp) + 3) & ~0x1;
> > +	smp_mb(); /* Above access must not bleed into critical section. */
> 
> 	smp_load_acquire() then?

I have transitivity concerns.  Which might well be baseless, but again,
this is nowhere near a fastpath.

> > +	return s;
> > +}
> > +
> > +/*
> > + * Given a snapshot from rcu_seq_snap(), determine whether or not a
> > + * full update-side operation has occurred.
> > + */
> > +static bool rcu_seq_done(unsigned long *sp, unsigned long s)
> > +{
> > +	return ULONG_CMP_GE(READ_ONCE(*sp), s);
> 
> I'm always amused you're not wanting to rely on 2s complement for
> integer overflow. I _know_ its undefined behaviour in the C rule book,
> but the entire rest of the kernel hard assumes it.

I take it you have never seen the demonic glow in the eyes of a compiler
implementer when thinking of all the code that can be broken^W^W^W^W^W
optimizations that are enabled by relying on undefined behavior for
signed integer overflow?  ;-)

> > +}
> > +
> > +/* Wrapper functions for expedited grace periods.  */
> > +static void rcu_exp_gp_seq_start(struct rcu_state *rsp)
> > +{
> > +	rcu_seq_start(&rsp->expedited_sequence);
> > +}
> > +static void rcu_exp_gp_seq_end(struct rcu_state *rsp)
> > +{
> > +	rcu_seq_end(&rsp->expedited_sequence);
> > +}
> > +static unsigned long rcu_exp_gp_seq_snap(struct rcu_state *rsp)
> > +{
> > +	return rcu_seq_snap(&rsp->expedited_sequence);
> > +}
> > +static bool rcu_exp_gp_seq_done(struct rcu_state *rsp, unsigned long s)
> > +{
> > +	return rcu_seq_done(&rsp->expedited_sequence, s);
> > +}
> 
> This is wrappers for wrappers sake? Why?

For _rcu_barrier(), as noted in the commit log.

							Thanx, Paul