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

[Peter Zijlstra <peterz@infradead.org>]

On Wed, Jul 01, 2015 at 03:18:04PM -0700, Paul E. McKenney wrote:
> On Wed, Jul 01, 2015 at 12:27:17PM +0200, Peter Zijlstra wrote:

> > 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.

Fair enough.

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

Yeah, I'm not sure how much sense smp_store_acquire() makes, but I'm
fairly sure this isn't the first time I've wondered about it.

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

Note that this is unsigned integers, but yes I know, you've said. But
they cannot unilaterally change this 'undefined' behaviour because its
been defined as 'whatever the hardware does' for such a long time.

Likewise they can dream all they want about breaking our concurrent code
and state we should use the brand spanking new primitives, sod 30 years
of existing code, but that's just not realistic either.

Even if we didn't 'have' to support a wide range of compiler versions,
most of which do not even support these new fangled primitives, who is
going to audit our existing many million lines of code? Not to mention
the many more million lines of code in other projects that rely on these
same things.

Its really time for them to stop wanking and stare reality in the face.

> > > +/* 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.

Yes it said; but why? Surely _rcu_barrier() can do the
->expedited_sequence thing itself, that hardly seems worthy of a
wrapper.