Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1175571 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC tip/core/rcu 05/14] rcu: Abstract sequence counting  from synchronize_sched_expedited() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-07-02 00:20 +0200
    Re: [PATCH RFC tip/core/rcu 05/14] rcu: Abstract sequence counting  from synchronize_sched_expedited() Peter Zijlstra <peterz@infradead.org> - 2015-07-02 11:00 +0200
      Re: [PATCH RFC tip/core/rcu 05/14] rcu: Abstract sequence counting  from synchronize_sched_expedited() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-07-02 16:20 +0200

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1175842

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1176022

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web