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    [W:0.040 / U:29.264 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site