Re: [PATCH] rcu: don't call rcu_preempt_note_context_switch() in rcu_check_callbacks()

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

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