Re: bpf memory model. Was: [PATCH v4 bpf-next 1/9] bpf: introduce bpf_spin_lock

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Wed, Jan 30, 2019 at 09:58:50AM +0100, Peter Zijlstra wrote:
> On Tue, Jan 29, 2019 at 06:32:13PM -0800, Alexei Starovoitov wrote:
> > On Tue, Jan 29, 2019 at 10:16:54AM +0100, Peter Zijlstra wrote:
> > > On Mon, Jan 28, 2019 at 01:56:24PM -0800, Alexei Starovoitov wrote:
> > > > On Mon, Jan 28, 2019 at 10:24:08AM +0100, Peter Zijlstra wrote:
> > > 
> > > > > Ah, but the loop won't be in the BPF program itself. The BPF program
> > > > > would only have had the BPF_SPIN_LOCK instruction, the JIT them emits
> > > > > code similar to queued_spin_lock()/queued_spin_unlock() (or calls to
> > > > > out-of-line versions of them).
> > > > 
> > > > As I said we considered exactly that and such approach has a lot of downsides
> > > > comparing with the helper approach.
> > > > Pretty much every time new feature is added we're evaluating whether it
> > > > should be new instruction or new helper. 99% of the time we go with new helper.
> > > 
> > > Ah; it seems I'm confused on helper vs instruction. As in, I've no idea
> > > what a helper is.
> > 
> > bpf helper is a normal kernel function that can be called from bpf program.
> > In assembler it's a direct function call.
> 
> Ah, it is what is otherwise known as a standard library,
> 
> > > > > There isn't anything that mandates the JIT uses the exact same locking
> > > > > routines the interpreter does, is there?
> > > > 
> > > > sure. This bpf_spin_lock() helper can be optimized whichever way the kernel wants.
> > > > Like bpf_map_lookup_elem() call is _inlined_ by the verifier for certain map types.
> > > > JITs don't even need to do anything. It looks like function call from bpf prog
> > > > point of view, but in JITed code it is a sequence of native instructions.
> > > > 
> > > > Say tomorrow we find out that bpf_prog->bpf_spin_lock()->queued_spin_lock()
> > > > takes too much time then we can inline fast path of queued_spin_lock
> > > > directly into bpf prog and save function call cost.
> > > 
> > > OK, so then the JIT can optimize helpers. Would it not make sense to
> > > have the simple test-and-set spinlock in the generic code and have the
> > > JITs use arch_spinlock_t where appropriate?
> > 
> > I think that pretty much the same as what I have with qspinlock.
> > Instead of taking a risk how JIT writers implement bpf_spin_lock optimization
> > I'm using qspinlock on architectures that are known to support it.
> 
> I see the argument for it...
> 
> > So instead of starting with dumb test-and-set there will be faster
> > qspinlock from the start on x86, arm64 and few others archs.
> > Those are the archs we care about the most anyway. Other archs can take
> > time to optimize it (if optimizations are necessary at all).
> > In general hacking JITs is much harder and more error prone than
> > changing core and adding helpers. Hence we avoid touching JITs
> > as much as possible.
> 
> So archs/JITs are not trivially able to override those helper functions?
> Because for example ARM (32bit) doesn't do qspinlock but it's
> arch_spinlock_t is _much_ better than a TAS lock.

JITs can override. There is no 'ready to use' facility for all types
of helpers to do that, but it's easy enough to add.
Having said that I'm going to reject arm32 JIT patches that
are trying to use arch_spinlock instead of generic bpf_spin_lock.
The last thing arm32 jit needs is this type of optimization.
Other JITs is a different story.