Messages in this thread | | | From | Rusty Russell <> | Subject | Re: [PATCH RFC] percpu: add data dependency barrier in percpu accessors and operations | Date | Tue, 15 Jul 2014 21:20:52 +0930 |
| |
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes: > On Wed, Jul 09, 2014 at 10:25:44AM +0930, Rusty Russell wrote: >> Tejun Heo <tj@kernel.org> writes: >> > Hello, Paul. >> >> Rusty wakes up... > > ;-) > >> >> Good point. How about per-CPU variables that are introduced by >> >> loadable modules? (I would guess that there are plenty of memory >> >> barriers in the load process, given that text and data also needs >> >> to be visible to other CPUs.) >> > >> > (cc'ing Rusty, hi!) >> > >> > Percpu initialization happens in post_relocation() before >> > module_finalize(). There seem to be enough operations which can act >> > as write barrier afterwards but nothing seems explicit. >> > >> > I have no idea how we're guaranteeing that .data is visible to all >> > cpus without barrier from reader side. Maybe we don't allow something >> > like the following? >> > >> > module init built-in code >> > >> > static int mod_static_var = X; if (builtin_ptr) >> > builtin_ptr = &mod_static_var; WARN_ON(*builtin_ptr != X); >> > >> > Rusty, can you please enlighten me? >> >> Subtle, but I think in theory (though not in practice) this can happen. >> >> Making this this assigner's responsibility is nasty, since we reasonably >> assume that .data is consistent across CPUs once code is executing >> (similarly on boot). >> >> >> Again, it won't help for the allocator to strongly order the >> >> initialization to zero if there are additional initializations of some >> >> fields to non-zero values. And again, it should be a lot easier to >> >> require the smp_store_release() or whatever uniformly than only in cases >> >> where additional initialization occurred. >> > >> > This one is less murky as we can say that the cpu which allocated owns >> > the zeroing; however, it still deviates from requiring the one which >> > makes changes to take care of barriering for those changes, which is >> > what makes me feel a bit uneasy. IOW, it's the allocator which >> > cleared the memory, why should its users worry about in-flight >> > operations from it? That said, this poses a lot less issues compared >> > to percpu ones as passing normal pointers to other cpus w/o going >> > through proper set of barriers is a special thing to do anyway. >> >> I think that the implicit per-cpu allocations done by modules need to >> be consistent once the module is running. >> >> I'm deeply reluctant to advocate it in the other per-cpu cases though. >> Once we add a barrier, it's impossible to remove: callers may subtly >> rely on the behavior. >> >> "Magic barrier sprinkles" is a bad path to start down, IMHO. > > Here is the sort of thing that I would be concerned about: > > p = alloc_percpu(struct foo); > for_each_possible_cpu(cpu) > initialize(per_cpu_ptr(p, cpu); > gp = p; > > We clearly need a memory barrier in there somewhere, and it cannot > be buried in alloc_percpu(). Some cases avoid trouble due to locking, > for example, initialize() might acquire a per-CPU lock and later uses > might acquire that same lock. Clearly, use of a global lock would not > be helpful from a scalability viewpoint.
I agree with Christoph: there's no per-cpu-unique peculiarity here. Anyone who exposes a pointer needs a barrier first.
And the per-cpu allocation for modules is under a mutex, so that case is already covered.
Cheers, Rusty.
| |