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

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 2011-09-19 at 17:02 -0500, Christoph Lameter wrote:
> On Mon, 19 Sep 2011, Steven Rostedt wrote:
> 
> > From: Steven Rostedt <srostedt@redhat.com>
> >
> > The code in mod_state() is already made to handle the raciness of
> > this_cpu_read(). Have the code use __this_cpu_read() instead so
> > the debug code does not trigger warnings about it.
> 
> Why would there be a warning triggered? this_cpu_read should take care of
> disabling preemption for the read if needed. In fact the fallback case
> does do exactly that.

What does disabling preemption for just reading the cpu variable help
anywhere? Once you read the variable, and enable preemption, you can
migrate. Then that info you have could be useless.


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

Hmm, maybe I should have called it raw_this_cpu_read() then it would
probably have better meaning. Just like smp_processor_id() and
raw_smp_processor_id().

The point is, most places use this_cpu_read(). Adding a "__" or "raw_"
prefix usually means that less checks are done. Think of the
copy_from_user(). You have __copy_from_user() that does less checks.

Most of the per_cpu() and get_cpu_var() code was blindly replaced with
the this_cpu_*() variants. The original code had preemption disabled
checks. Now they don't. Which means if you use the this_cpu_read() and
then make some decision on it, that read may be useless and buggy.

We have smp_processor_id() that does the check (and the old per_cpu()
and get_cpu_var()). Now we have this_cpu_read() which replaced most of
the per_cpu() code in the kernel, making them vulnerable to bugs when
preemption is enabled.

-- Steve