Messages in this thread | | | Date | Wed, 31 Mar 2010 18:17:47 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH] rcu: don't call rcu_preempt_note_context_switch() in rcu_check_callbacks() |
| |
On Thu, Apr 01, 2010 at 08:56:05AM +0800, Lai Jiangshan wrote: > 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().
Possibly by moving the clearing of RCU_READ_UNLOCK_NEED_QS to rcu_preempt_check_callbacks() -- or to rcu_preempt_qs(). The latter is in some sense cleaner, but higher overhead and probably unnecessary. Hmmm... Alternatively, require that all callers to rcu_preempt_qs() disable irqs. This affects only one callsite, which has a local_irq_disable() immediately following anyway. ;-)
Thanx, Paul
| |