[lkml]   [2013]   [Oct]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 5/6] net: __this_cpu_inc in route.c
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

 \ /
  Last update: 2013-10-16 12:41    [W:0.076 / U:1.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site