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

[Steven Rostedt]

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


--
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/