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

[Paul E. McKenney]

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@xxxxxxxxxxxxxxxxxx>
> > 
> > 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?

							Thanx, Paul

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