Messages in this thread | | | Subject | Re: [RFC][PATCH 2/5] mm: Switch mod_state() to __this_cpu_read() | From | Steven Rostedt <> | Date | Tue, 20 Sep 2011 10:01:38 -0400 |
| |
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
| |