Re: [PATCH v5 tip/core/rcu 14/16] rcu: Remove redundant preempt_disable() from rcu_note_voluntary_context_switch()

[Peter Zijlstra <peterz@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 1584 bytes --]

On Wed, Aug 13, 2014 at 07:07:51AM -0700, Paul E. McKenney wrote:
> On Wed, Aug 13, 2014 at 12:56:18PM +0200, Peter Zijlstra wrote:
> > On Mon, Aug 11, 2014 at 03:49:03PM -0700, Paul E. McKenney wrote:
> > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > > 
> > > In theory, synchronize_sched() requires a read-side critical section to
> > > order against.  In practice, preemption can be thought of as being
> > > disabled across every machine instruction.  So this commit removes
> > > the redundant preempt_disable() from rcu_note_voluntary_context_switch().
> > 
> > >  #define rcu_note_voluntary_context_switch(t) \
> > >  	do { \
> > > -		preempt_disable(); /* Exclude synchronize_sched(); */ \
> > >  		if (ACCESS_ONCE((t)->rcu_tasks_holdout)) \
> > >  			ACCESS_ONCE((t)->rcu_tasks_holdout) = 0; \
> > > -		preempt_enable(); \
> > >  	} while (0)
> > 
> > But that's more than 1 instruction.
> 
> Yeah, the commit log could use some help.  The instruction in question
> is the store.  The "if" is just an optimization.
> 
> So suppose that this sequence is preempted between the "if" and the store,
> and that the synchronize_sched() (and quite a bit more besides!) takes
> place during this preemption.  The task is still in a quiescent state
> at the time of the store, so the store is still legitimate.
> 
> That said, it might be better to just leave preemption disabled, as that
> certainly makes things simpler.  Thoughts?

A comment explaining it should be fine I think. I was just raising the
obvious fail in the changelog.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]