Re: [PATCH RFC] percpu: add data dependency barrier in percpu accessors and operations

[Rusty Russell <rusty@rustcorp.com.au>]

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