Messages in this thread | | | Date | Wed, 31 Mar 2010 08:36:56 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH] rcu: don't call rcu_preempt_note_context_switch() in rcu_check_callbacks() |
| |
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
| |