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

[Steven Rostedt]

On Wed, 2011-09-21 at 10:16 -0500, Christoph Lameter wrote:

> > My argument is that this_cpu_* is just confusing. Rename your use case
> > and keep this_cpu_*() as what you want __this_cpu_*() to be.
> 
> Thought about this a bit last night. I think the main issue are these
> this_cpu_read() and this_cpu_write() operations since people use those
> irresponsibly. It usually does not make sense to read a value from a
> random cpu nor does writing make sense. The situation is different for
> per cpu counter increments where it does not matter which cpus counter is
> incremented since we sum them up later anyways.
> 
> How about getting rid of this_cpu_read() and this_cpu_write() entirely?
> 
> Only allow __this_cpu_read and __this_cpu_write. There we check that the
> caller has disabled preemption.

The problem I have with this, is that this does not help at all. We are
back to the word "this_cpu" when you really do mean "any_cpu". We
optimize the implementation to only write to the current cpu we are on,
but that is an optimization not the design, as the process could migrate
before and after the call to your this_cpu_*() operation, which makes it
no longer "this cpu". The bigger view of this design is that
incrementing a cpu variable and then summing it up, means that you do
not care which CPU variable you updated. Do not call it "this_cpu"!
Yes, for optimization sake, we happen to use the current CPU, but if we
read/wrote to any CPU variable, the algorithm still works. Hence, it is
not "this_cpu" but "any_cpu".


> 
> For the rare special cases (are there any?) that are legitimate use cases
> for this_cpu_read/write we can use manual determination of per cpu
> pointers and then just do a load via the pointer?
> 
> Or alternatively give this_cpu_read and write special names that make
> their dangerousness clear.

Right, we need to change all "this_cpu_*()" operations that are made to
be safe under preempt enabled areas to "any_cpu_*()". And use
this_cpu_*() for the current __this_cpu_*().

This would clear up the confusion about using "this_cpu" vs "__this_cpu"
because "__" is truly meaningless.

> 
> In the case of slub there are only some this_cpu_write() things that can
> be __this_cpu_write without a problem.
> 
> The __this_cpu_ptr() can become this_cpu_ptr as far as I can tell. This
> should make it consistent so that we can check for disabled preemption for
> all __this_cpu thingies.


Again, lets just bite the bullet and rename them to something that is
understandable for everyone. This would make all of us happy.

I'm not against your code, I'm against the naming convention you decided
to use. It makes it confusing to something that is complex and complex
code needs to try to be a simple as possible.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/