rcu: don't call rcu_preempt_note_context_switch() in rcu_check_callbacks()
in [Kernel]
|
Prev: rcu: remove needless struct notifier_block predeclaration
Next: Localizatoare GPS / urmarire masini la distanta in timp real
From: Paul E. McKenney on 29 Mar 2010 00:50 On Mon, Mar 29, 2010 at 10:47:59AM +0800, Lai Jiangshan wrote: > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 7bit > > > Even though in user mode or idle mode, rcu_check_callbacks() is not > context switch, so we don't call rcu_preempt_note_context_switch() > in rcu_check_callbacks(). > > Though there is no harm that calls rcu_preempt_note_context_switch() > in rcu_check_callbacks(), but it is waste. > > rcu_check_callbacks() > rcu_sched_qs() > rcu_preempt_note_context_switch() > Now, ->rcu_read_lock_nesting == 0, so we just calls > rcu_preempt_qs(), but, rcu_preempt_check_callbacks() > will call it again and set the ->rcu_read_unlock_special > correct again. > > So let rcu_preempt_check_callbacks() handle things for us. Nice!!! But how about naming the new function that invokes rcu_preempt_note_context_switch() something like rcu_sched_note_context_switch(), and then leaving the name of rcu_sched_qs() the same (rather than changing it to __rcu_sched_qs(), as below)? This way, the names clearly call out what the function is doing. Or did I miss the point here? Thanx, Paul > Signed-off-by: Lai Jiangshan <laijs(a)cn.fujitsu.com> > --- > diff --git a/kernel/rcutree.c b/kernel/rcutree.c > index 3ec8160..c7847ba 100644 > --- a/kernel/rcutree.c > +++ b/kernel/rcutree.c > @@ -95,7 +95,7 @@ static int rcu_gp_in_progress(struct rcu_state *rsp) > * how many quiescent states passed, just if there was at least > * one since the start of the grace period, this just sets a flag. > */ > -void rcu_sched_qs(int cpu) > +static void __rcu_sched_qs(int cpu) > { > struct rcu_data *rdp; > > @@ -103,6 +103,11 @@ void rcu_sched_qs(int cpu) > rdp->passed_quiesc_completed = rdp->gpnum - 1; > barrier(); > rdp->passed_quiesc = 1; > +} > + > +void rcu_sched_qs(int cpu) > +{ > + __rcu_sched_qs(cpu); > rcu_preempt_note_context_switch(cpu); > } > > @@ -1138,12 +1143,12 @@ void rcu_check_callbacks(int cpu, int user) > * a quiescent state, so note it. > * > * No memory barrier is required here because both > - * rcu_sched_qs() and rcu_bh_qs() reference only CPU-local > + * __rcu_sched_qs() and rcu_bh_qs() reference only CPU-local > * variables that other CPUs neither access nor modify, > * at least not while the corresponding CPU is online. > */ > > - rcu_sched_qs(cpu); > + __rcu_sched_qs(cpu); > rcu_bh_qs(cpu); > > } else if (!in_softirq()) { > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Lai Jiangshan on 30 Mar 2010 05:50 Paul E. McKenney wrote: > On Mon, Mar 29, 2010 at 10:47:59AM +0800, Lai Jiangshan wrote: >> Content-Type: text/plain; charset=UTF-8 >> Content-Transfer-Encoding: 7bit >> >> >> Even though in user mode or idle mode, rcu_check_callbacks() is not >> context switch, so we don't call rcu_preempt_note_context_switch() >> in rcu_check_callbacks(). >> >> Though there is no harm that calls rcu_preempt_note_context_switch() >> in rcu_check_callbacks(), but it is waste. >> >> rcu_check_callbacks() >> rcu_sched_qs() >> rcu_preempt_note_context_switch() >> Now, ->rcu_read_lock_nesting == 0, so we just calls >> rcu_preempt_qs(), but, rcu_preempt_check_callbacks() >> will call it again and set the ->rcu_read_unlock_special >> correct again. >> >> So let rcu_preempt_check_callbacks() handle things for us. > > Nice!!! > > But how about naming the new function that invokes > rcu_preempt_note_context_switch() something like > rcu_sched_note_context_switch(), and then leaving the > name of rcu_sched_qs() the same (rather than changing > it to __rcu_sched_qs(), as below)? > > This way, the names clearly call out what the function > is doing. > If I understand right, it will become this: schedule() / run_ksoftirqd() / rcu_needs_cpu() rcu_sched_note_context_switch() rcu_sched_qs() rcu_preempt_note_context_switch() Right? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Paul E. McKenney on 30 Mar 2010 12:10 On Tue, Mar 30, 2010 at 05:43:33PM +0800, Lai Jiangshan wrote: > Paul E. McKenney wrote: > > On Mon, Mar 29, 2010 at 10:47:59AM +0800, Lai Jiangshan wrote: > >> Content-Type: text/plain; charset=UTF-8 > >> Content-Transfer-Encoding: 7bit > >> > >> > >> Even though in user mode or idle mode, rcu_check_callbacks() is not > >> context switch, so we don't call rcu_preempt_note_context_switch() > >> in rcu_check_callbacks(). > >> > >> Though there is no harm that calls rcu_preempt_note_context_switch() > >> in rcu_check_callbacks(), but it is waste. > >> > >> rcu_check_callbacks() > >> rcu_sched_qs() > >> rcu_preempt_note_context_switch() > >> Now, ->rcu_read_lock_nesting == 0, so we just calls > >> rcu_preempt_qs(), but, rcu_preempt_check_callbacks() > >> will call it again and set the ->rcu_read_unlock_special > >> correct again. > >> > >> So let rcu_preempt_check_callbacks() handle things for us. > > > > Nice!!! > > > > But how about naming the new function that invokes > > rcu_preempt_note_context_switch() something like > > rcu_sched_note_context_switch(), and then leaving the > > name of rcu_sched_qs() the same (rather than changing > > it to __rcu_sched_qs(), as below)? > > > > This way, the names clearly call out what the function > > is doing. > > > > If I understand right, it will become this: > > schedule() / run_ksoftirqd() / rcu_needs_cpu() > rcu_sched_note_context_switch() > rcu_sched_qs() > rcu_preempt_note_context_switch() Wow!!! That was a scare!!! I misread "run_ksoftirqd()" as "do_softirq(). ;-) And I am not seeing a call to rcu_sched_qs() in rcu_needs_cpu()... Here is how I believe it needs to go: schedule(): rcu_sched_note_context_switch() rcu_sched_qs() rcu_preempt_note_context_switch() run_ksoftirqd(): rcu_sched_qs() rcu_check_callbacks(): rcu_sched_qs() [if idle etc.] rcu_bh_qs() [if not in softirq] The reason we don't need rcu_bh_qs() from run_ksoftirqd() is that __do_softirq() already calls rcu_bh_qs(). Make sense, or am I missing something? Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Paul E. McKenney on 31 Mar 2010 11:40 On Tue, Mar 30, 2010 at 09:03:39AM -0700, Paul E. McKenney wrote: > On Tue, Mar 30, 2010 at 05:43:33PM +0800, Lai Jiangshan wrote: > > Paul E. McKenney wrote: > > > On Mon, Mar 29, 2010 at 10:47:59AM +0800, Lai Jiangshan wrote: > > >> Content-Type: text/plain; charset=UTF-8 > > >> Content-Transfer-Encoding: 7bit > > >> > > >> > > >> Even though in user mode or idle mode, rcu_check_callbacks() is not > > >> context switch, so we don't call rcu_preempt_note_context_switch() > > >> in rcu_check_callbacks(). > > >> > > >> Though there is no harm that calls rcu_preempt_note_context_switch() > > >> in rcu_check_callbacks(), but it is waste. > > >> > > >> rcu_check_callbacks() > > >> rcu_sched_qs() > > >> rcu_preempt_note_context_switch() > > >> Now, ->rcu_read_lock_nesting == 0, so we just calls > > >> rcu_preempt_qs(), but, rcu_preempt_check_callbacks() > > >> will call it again and set the ->rcu_read_unlock_special > > >> correct again. > > >> > > >> So let rcu_preempt_check_callbacks() handle things for us. > > > > > > Nice!!! > > > > > > But how about naming the new function that invokes > > > rcu_preempt_note_context_switch() something like > > > rcu_sched_note_context_switch(), and then leaving the > > > name of rcu_sched_qs() the same (rather than changing > > > it to __rcu_sched_qs(), as below)? > > > > > > This way, the names clearly call out what the function > > > is doing. > > > > > > > If I understand right, it will become this: > > > > schedule() / run_ksoftirqd() / rcu_needs_cpu() > > rcu_sched_note_context_switch() > > rcu_sched_qs() > > rcu_preempt_note_context_switch() > > Wow!!! That was a scare!!! I misread "run_ksoftirqd()" as > "do_softirq(). ;-) > > And I am not seeing a call to rcu_sched_qs() in rcu_needs_cpu()... > > Here is how I believe it needs to go: > > schedule(): > rcu_sched_note_context_switch() > rcu_sched_qs() > rcu_preempt_note_context_switch() > > run_ksoftirqd(): > rcu_sched_qs() > > rcu_check_callbacks(): > rcu_sched_qs() [if idle etc.] > rcu_bh_qs() [if not in softirq] > > The reason we don't need rcu_bh_qs() from run_ksoftirqd() is that > __do_softirq() already calls rcu_bh_qs(). > > Make sense, or am I missing something? And I was in fact missing something. The rcu_preempt_note_context_switch() function currently combines some work that needs to happen only at context-switch time with work that needs to happen all the time. At first glance, it appears that the big "if" statement in rcu_preempt_note_context_switch() need only happen for context switches. The remaining lines must happen unconditionally for context switches, and should be executed from rcu_check_callbacks() only if the current CPU is not in an RCU read-side critical section. Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Lai Jiangshan on 31 Mar 2010 21:00
Paul E. McKenney wrote: > On Tue, Mar 30, 2010 at 09:03:39AM -0700, Paul E. McKenney wrote: >> On Tue, Mar 30, 2010 at 05:43:33PM +0800, Lai Jiangshan wrote: >>> Paul E. McKenney wrote: >>>> On Mon, Mar 29, 2010 at 10:47:59AM +0800, Lai Jiangshan wrote: >>>>> Content-Type: text/plain; charset=UTF-8 >>>>> Content-Transfer-Encoding: 7bit >>>>> >>>>> >>>>> Even though in user mode or idle mode, rcu_check_callbacks() is not >>>>> context switch, so we don't call rcu_preempt_note_context_switch() >>>>> in rcu_check_callbacks(). >>>>> >>>>> Though there is no harm that calls rcu_preempt_note_context_switch() >>>>> in rcu_check_callbacks(), but it is waste. >>>>> >>>>> rcu_check_callbacks() >>>>> rcu_sched_qs() >>>>> rcu_preempt_note_context_switch() >>>>> Now, ->rcu_read_lock_nesting == 0, so we just calls >>>>> rcu_preempt_qs(), but, rcu_preempt_check_callbacks() >>>>> will call it again and set the ->rcu_read_unlock_special >>>>> correct again. >>>>> >>>>> So let rcu_preempt_check_callbacks() handle things for us. >>>> Nice!!! >>>> >>>> But how about naming the new function that invokes >>>> rcu_preempt_note_context_switch() something like >>>> rcu_sched_note_context_switch(), and then leaving the >>>> name of rcu_sched_qs() the same (rather than changing >>>> it to __rcu_sched_qs(), as below)? >>>> >>>> This way, the names clearly call out what the function >>>> is doing. >>>> >>> If I understand right, it will become this: >>> >>> schedule() / run_ksoftirqd() / rcu_needs_cpu() >>> rcu_sched_note_context_switch() >>> rcu_sched_qs() >>> rcu_preempt_note_context_switch() >> Wow!!! That was a scare!!! I misread "run_ksoftirqd()" as >> "do_softirq(). ;-) >> >> And I am not seeing a call to rcu_sched_qs() in rcu_needs_cpu()... >> >> Here is how I believe it needs to go: >> >> schedule(): >> rcu_sched_note_context_switch() >> rcu_sched_qs() >> rcu_preempt_note_context_switch() >> >> run_ksoftirqd(): >> rcu_sched_qs() >> >> rcu_check_callbacks(): >> rcu_sched_qs() [if idle etc.] >> rcu_bh_qs() [if not in softirq] >> >> The reason we don't need rcu_bh_qs() from run_ksoftirqd() is that >> __do_softirq() already calls rcu_bh_qs(). >> >> Make sense, or am I missing something? > > And I was in fact missing something. The rcu_preempt_note_context_switch() > function currently combines some work that needs to happen only at > context-switch time with work that needs to happen all the time. > > At first glance, it appears that the big "if" statement in > rcu_preempt_note_context_switch() need only happen for context switches. > The remaining lines must happen unconditionally for context switches, > and should be executed from rcu_check_callbacks() only if the current > CPU is not in an RCU read-side critical section. > I think rcu_preempt_check_callbacks() will do this work better in rcu_check_callbacks(). Thanks, Lai -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |