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

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 2011-09-20 at 09:51 -0500, Christoph Lameter wrote:

> I see that __this_cpu_xx operations may not work as intended in
> preemptable contexts and there we could have more changes.

Then why do you use it in slub.c?

in mm/slab.c slab_alloc():

redo:

	/*
	 * Must read kmem_cache cpu data via this cpu ptr. Preemption is
	 * enabled. We may switch back and forth between cpus while
	 * reading from one cpu area. That does not matter as long
	 * as we end up on the original cpu again when doing the cmpxchg.
	 */
	c = __this_cpu_ptr(s->cpu_slab);

The __this_cpu_*() is in preempt enabled location. In fact, the three
this_cpu_write()'s in acquire_slab() is done within a spinlock and thus
with preemption disabled.

> 
> this_cpu ops are single instructions that do not guarantee any consistency
> with other this_cpu_ops. If you want consistency (same per cpu area data)
> then preemption needs to be disabled.

The point is, people get it wrong all the time. In fact, we should
really require that ALL USES of this_cpu_*() must be with preemption
disabled. Regardless. Because anytime you touch a per cpu variable,
there's usually a reason that it is on the cpu you are on. If you don't
have preemption disabled (and I don't count the silly:

#define _this_cpu_generic_read(pcp)					\
({	typeof(pcp) ret__;						\
	preempt_disable();						\
	ret__ = *this_cpu_ptr(&(pcp));					\
	preempt_enable();						\
	ret__;								\
})

Which is totally useless), there's probably a bug somewhere in there.
Just like the code in memcg.

Monkeying around with per cpu data is tricky. If you start doing it in
preempt enabled code, you are most certainly about to get it wrong. Why
have this super optimization. A preempt_disable() is a single operation
that touches cache hot data.

I say nuke all users of preempt_enabled per_cpu touching, and be safe.

-- Steve