Messages in this thread | | | Date | Thu, 2 Jul 2015 07:13:30 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH RFC tip/core/rcu 05/14] rcu: Abstract sequence counting from synchronize_sched_expedited() |
| |
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
| |