lkml.org 
[lkml]   [2010]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] rcu: don't call rcu_preempt_note_context_switch() in rcu_check_callbacks()
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


\
 
 \ /
  Last update: 2010-04-01 02:59    [from the cache]
©2003-2011 Jasper Spaans