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 Thu, Jul 02, 2015 at 10:50:41AM +0200, Peter Zijlstra wrote:
> 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.

For pure unsigned arithmetic, their options are indeed limited.  For a
cast to signed, I am not so sure.  I have been using time_before() and
friends for jiffy comparisons, which does a cast to signed after the
subtraction.  Signed overflow is already unsafe with current compilers,
though the kernel suppresses these.

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

Indeed, I have been and will be continuing to make myself unpopular with
that topic.  ;-)

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

Ah, you want synchronize_rcu_expedited() and synchronize_sched_expedited()
to use rcu_seq_start() and friends directly.  I can certainly do that.

							Thanx, Paul