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:46 -0400, Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:
> > [ Added Mathieu as I've been discussing this with him on IRC ]
> [...]
> > My suggestion is to nuke the __this_cpu*() functions and have them
> > called this_cpu*(). And change the locations that allow for preemption
> > enabled to be called preemptsafe_cpu_*() just like the irqsafe_cpu*()
> > are used.
> > 
> > Thoughts?
> 
> I fully agreed with this proposal.
> 
> this_cpu_*() should be a sane default, which is cases that require
> preemption to be disabled. Warning about use without preemption disabled
> is important, because most users will _assume_ that they stay on the
> same CPU across multiple calls.
> 
> preemptsafe_cpu_*() is an optimisation made for those who know that
> they don't care about having consistent view of the variables across
> multiple operations, e.g. statistics, or deal with this explicitly, like
> SLUB. Typical use require either add_return or cmpxchg for validation.

The funny thing is, the few uses where preemption is enabled in slub,
happens to use the __this_cpu_() version. Which shows me that this whole
concept is completely flawed.

> 
> In the past, we used to have something like raw_smp_processor_id()
> though, for sites where racy use does not matter. So probably having a
> raw_this_cpu_*() that behaves like "this_cpu_*()" proposed here, but
> does not check for preemption disabled would also be useful.

I'm going with all users of this_cpu_*() must be used in a preempt
disabled section. Byte the bullet. I doubt any sane benchmark shows any
improvement by removing preempt_disable() from small code paths. If a
longer code path is needed, then the code was probably buggy to begin
with.

For those cases that really do not care about preemption enabled, maybe
we could add a single this_cpu() function that allows that.


this_cpu_snapshot();

This would be the only function that can be used with preemption
enabled. It is read only (no modification of the variable). It is used
in cases that you really don't care what CPU you are on. For instance
the const_udelay() could use this.

The term "snapshot" should document very nicely that you do not care
that you migrated in the next moment. As a snapshot is just a quick
instance of time.

I really mean all other users of this_cpu_*(), including the cmpxchg and
friends, still need to have preemption disabled.

-- Steve