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" <[email protected]> > > > > > > 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.
pgpw8arwIAyeD.pgp
Description: PGP signature
