Re: [PATCH 5/6] net: __this_cpu_inc in route.c

[Peter Zijlstra]

On Wed, Oct 16, 2013 at 02:22:49AM -0700, Eric Dumazet wrote:
> On Wed, 2013-10-16 at 10:46 +0200, Peter Zijlstra wrote:
> 
> > Are we sure all !x86 implementations will DTRT in that it will increment
> > some CPU and not get horribly confused? I suppose it would; but is
> > that a guarantee given someplace?
> 
> I think we should be fine, these are only stats exposed
> on /proc/net/stat/rt_cache

My concern was really that an unprotected __this_cpu op would complete
coherent and not corrupt state. If we cannot guarantee this it should
always be a full BUG to use it without proper protection.

For x86 its fairly easy to see its correct this way; but for load-store
archs this isn't immediately obvious to me.

Suppose; r1 contains our per-cpu pointer:

	LOAD  r2, per-cpu-base
	ADD   r1, r2
	LOAD  r2, $(r1) # load value
	INC   r2
	STORE $(r1), r2 # store value

If such a thing is done without preempt disable; we could be
preempted/migrated at any place along that chain. In that case the STORE
could be to another CPUs memory (we get migrated near the INC) and could
conflict with a per-cpu operation on that CPU corrupting state.

If I look at percpu.h; the generic __this_cpu versions look like they
generate the above for such archs.

In that case; I don't see how even for statistics (where we really don't
care what cpu the op happens on, as long as it happens to a cpu,
coherently) it is correct to use the raw_this_cpu stuff without
preemption protection.

In fact; I think the comment near __this_cpu_read actually alludes to
this.
--
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/