Re: [RFC][PATCH 0/5] Introduce checks for preemptable code for this_cpu_read/write()

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 2011-09-20 at 11:08 -0500, Christoph Lameter wrote:
> On Tue, 20 Sep 2011, Steven Rostedt wrote:
> 
> > On Tue, 2011-09-20 at 09:57 -0500, Christoph Lameter wrote:
> >
> > > this_cpu_xx functions are made for those locations that have
> > > preemption enabled. If you can use those function (classic case is a
> > > per cpu counter increment in the network subsystem) then you can avoid
> > > preempt disable/enable or get_cpu/put_cpu.
> >
> > If the variables are used for a very short time, then the latencies
> > introduced by a simple:
> >
> > 	var = get_cpu_var(my_var);
> > 	if (var)
> > 		do_something_quick();
> > 	put_cpu_var(my_var);
> >
> > Otherwise if that do_something_quick(); migrates, it may be doing
> > something it shouldn't be doing!
> 
> That is obviously a use case in which this_cpu_xx ops could not be used
> since we must stay on the same cpu. I think there is still a lot of
> confusion on your part. The example here shows it.

No, the problem is that you do not understand what I'm saying.

You are saying that you have other tricks to use per cpu data even
though that task that uses it can migrate to other cpus.

But the issue I have with this, is that this_cpu() is all about tricks.
The normal use case of per_cpu data is to disable preemption, use the
local variable and then enable preemption. That's the normal case and
should be what this_cpu_*() should be the default for.

But instead, you say this_cpu() is for data that doesn't care what CPU
it is at the time. It's really just as snapshot of the data. Which to me
seems to be a special use case that should not be the default. It is
confusing and hard to understand. Basically, you want __this_cpu_*() as
the normal case, which is ass backwards.

If your page allocator has special per_cpu tricks, then change the API
of it to something completely different to what the rest of the kernel
uses. Basic English shows to me that when I use "this_cpu_*()" I'm
getting a variable that is for the current CPU that the task is on, and
I should stay on that CPU. If you want something that is tricky, add
that to the name. Don't make everyone use this confusing "__this_cpu*()"
code which seems to be inconsistent in your own code too.

Have the "snapshot" version. This documents very nicely that the
variable that you read or modify is simply a one shot deal and not to
use it later. Don't use a common term like "this_cpu" and expect
everyone to understand that it should only be used for per cpu tricks.

I personally believe that we shouldn't even have a modification version
of the snapshot code. But perhaps it is fine for inc and dec and
cmpxchg. Put "snapshot" in the name to document that this is a hack to
make things work, and let all other users in the kernel use a name that
is not confusing.

-- Steve