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

[Lai Jiangshan <laijs@cn.fujitsu.com>]

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