On Mon, Sep 09, 2013 at 03:14:52PM +0200, Peter Zijlstra wrote:
> On Mon, Sep 09, 2013 at 08:55:04AM -0400, Steven Rostedt wrote:
> > On Mon, 9 Sep 2013 14:45:49 +0200
> > Frederic Weisbecker <fweis...@gmail.com> wrote:
> >
> >
> > > > This just proves that the caller of rcu_is_cpu_idle() must disable
> > > > preemption itself for the entire time that it needs to use the result
> > > > of rcu_is_cpu_idle().
> > >
> > > Sorry, I don't understand your point here. What's wrong with checking the
> > > ret from another CPU?
> >
> > Hmm, OK, this is why that code is in desperate need of a comment.
> >
> > From reading the context a bit more, it seems that the per cpu value is
> > more a "per task" value that happens to be using per cpu variables, and
> > changes on context switches. Is that correct?
> >
> > Anyway, it requires a comment to explain that we are not checking the
> > CPU state, but really the current task state, otherwise that 'ret'
> > value wouldn't travel with the task, but would stick with the CPU.
>
> Egads.. and the only reason we couldn't do the immediate load is because
> of that atomic mess.
>
> Also, if its per-task, why don't we have this in the task struct? The
> current scheme makes the context switch more expensive -- is this the
> right trade-off?
There are constraints based on the task, but RCU really is
paying attention to CPUs, not than tasks. (With the exception of
TREE_PREEMPT_RCU, which does keep lists of tasks that it has to pay
attention to, namely those that have been preempted within their current
RCU read-side critical section.)
I suppose that we could move it to the task structure, but that would
make for some "interesting" interactions between context switch and RCU.
Given previous experience with this sort of thing, I do not believe that
this would be a win.
Now, it might well be possible to make rcu_dynticks.dynticks be
manipulated by non-atomic operations, and that is on my list, but it
will require quite a bit of care and testing.
> So maybe something like:
>
> int rcu_is_cpu_idle(void)
> {
> /*
> * Comment explaining that rcu_dynticks.dynticks really is a
> * per-task something and we need preemption-safe loading.
> */
> atomic_t dynticks = this_cpu_read(rcu_dynticks.dynticks);
> return !(__atomic_read(&dynticks) & 0x01);
> }
>
> Where __atomic_read() would be like atomic_read() but without the
> volatile crap since that's entirely redundant here I think.
>
> The this_cpu_read() should ensure we get a preemption-safe copy of the
> value.
>
> Once that this_cpu stuff grows preemption checks we'd need something
> like __raw_this_cpu_read() or whatever the variant without preemption
> checks will be called.
Yay! Yet another formulation of the per-CPU code for this function! ;-)
Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
