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()
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


\
 
 \ /
  Last update: 2010-04-01 03:19    [W:2.840 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site