linux-kernel - Re: [PATCH] rcu: don't call rcu_preempt_note_context_switch() in rcu_check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4BB3EF25.1040804@cn.fujitsu.com>
Date:	Thu, 01 Apr 2010 08:56:05 +0800
From:	Lai Jiangshan <laijs@...fujitsu.com>
To:	paulmck@...ux.vnet.ibm.com
CC:	Ingo Molnar <mingo@...e.hu>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/