Re: [RFC][PATCH 2/5] mm: Switch mod_state() to __this_cpu_read()

Steven Rostedt
2011-09-20T14:01:44Z

On Tue, 2011-09-20 at 15:49 +0200, Thomas Gleixner wrote:

> The best thing is to remove the this_cpu_* hackery alltogether and
> just keep the __this_cpu_* versions (along with proper debugging) and
> git rid of the silly underscores.

I think renaming all the __this_cpu_*() to this_cpu_*() and then
removing all the current this_cpu_*() usages is the best thing.

That is, make it mandatory that all this_cpu_*() users are with preempt
disabled.

>
> > I think it would make more sence if __this_cpu_read() could be made to
> > trigger a warning if used in context where preemption could be off.
>
> It should have had such a warning in the first place and the warning
> needs to yell about preemptible (i.e. unprotected) context and not the
> other way round.
>
> But instead of just slapping smp_processor_id() checks into those
> functions we should add a more sensible debug interface like:
>
> debug_check_percpu_access()

Sure, I'd do something like that in the this_cpu_*() code. But since
smp_processor_id() already had the bug triggering. I just did the easy
approach as more of a proof of concept (hence the RFC in the patch set).

>
> and the per cpu sections which require protection over a series (1
> .. N) of this_cpu_* operations want to have
>
> this_cpu_start()
> this_cpu_end()
>
> or similar annotations around them.

I agree about the concept but hate the naming.

On IRC, Peter suggested a local_lock_t type. Give a named protection
area. I like this approach as we can add debugging infrastructure to
this concept.

local_lock_t my_vars;

local_lock(my_vars);
var = this_cpu_read();
/* play with var */
local_unlock(my_vars);

Peter even suggested for those locations that disable interrupts for
this work we could document that with:

local_lock_irqsave(my_vars, flags);
[...]
local_unlock_irqrestore(my_vars, flags);

This documents nicely the area of the protected variables. The
local_lock() could simply turn into a preempt_disable(), but we could
add hooks into lockdep or something to perform other checks. Even have
sparse or coccinelle find usages of var outside of local_lock(), and
report them.

As the kernel gets more complex, especially with the focus on scaling to
huge number of CPUs, and moving towards per_cpu data handling, having
debugging capabilities of this sort is critical.

>
> This allows us to do proper analysis of this_cpu usage and makes the
> code understandable.

Totally agree!

-- Steve