netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Will Deacon <will.deacon@arm.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Alexei Starovoitov <ast@kernel.org>,
	davem@davemloft.net, daniel@iogearbox.net,
	jakub.kicinski@netronome.com, netdev@vger.kernel.org,
	kernel-team@fb.com, mingo@redhat.com,
	Paul McKenney <paulmck@linux.vnet.ibm.com>,
	jannh@google.com
Subject: Re: bpf memory model. Was: [PATCH v4 bpf-next 1/9] bpf: introduce bpf_spin_lock
Date: Wed, 30 Jan 2019 18:11:00 +0000	[thread overview]
Message-ID: <20190130181100.GA18558@fuggles.cambridge.arm.com> (raw)
In-Reply-To: <20190128215623.6eqskzhklydhympa@ast-mbp>

Hi Alexei,

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:
> > On Fri, Jan 25, 2019 at 04:17:26PM -0800, Alexei Starovoitov wrote:
> > > What I want to avoid is to define the whole execution ordering model upfront.
> > > We cannot say that BPF ISA is weakly ordered like alpha.
> > > Most of the bpf progs are written and running on x86. We shouldn't
> > > twist bpf developer's arm by artificially relaxing memory model.
> > > BPF memory model is equal to memory model of underlying architecture.
> > > What we can do is to make it bpf progs a bit more portable with
> > > smp_rmb instructions, but we must not force weak execution on the developer.
> > 
> > Well, I agree with only introducing bits you actually need, and my
> > smp_rmb() example might have been poorly chosen, smp_load_acquire() /
> > smp_store_release() might have been a far more useful example.
> > 
> > But I disagree with the last part; we have to pick a model now;
> > otherwise you'll pain yourself into a corner.
> > 
> > Also; Alpha isn't very relevant these days; however ARM64 does seem to
> > be gaining a lot of attention and that is very much a weak architecture.
> > Adding strongly ordered assumptions to BPF now, will penalize them in
> > the long run.
> 
> arm64 is gaining attention just like riscV is gaining it too.
> BPF jit for arm64 is very solid, while BPF jit for riscV is being worked on.
> BPF is not picking sides in CPU HW and ISA battles.

It's not about picking a side, it's about providing an abstraction of the
various CPU architectures out there so that the programmer doesn't need to
worry about where their program may run. Hell, even if you just said "eBPF
follows x86 semantics" that would be better than saying nothing (and then we
could have a discussion about whether x86 semantics are really what you
want).

> Memory model is CPU HW design decision. BPF ISA cannot dictate HW design.
> We're not saying today that BPF is strongly ordered.
> BPF load/stores are behaving differently on x86 vs arm64.
> We can add new instructions, but we cannot 'define' how load/stores behave
> from memory model perspective.
> For example, take atomicity of single byte load/store.
> Not all archs have them atomic, but we cannot say to bpf programmers
> to always assume non-atomic byte loads.

Hmm, I don't think this is a good approach to take for the future of eBPF.
Assuming that a desirable property of an eBPF program is portability between
CPU architectures, then you're effectively forcing the programmer to "assume
the worst", where the worst is almost certainly unusable for practical
purposes.

One easy thing you could consider would be to allow tagging of an eBPF
program with its supported target architectures (the JIT will refuse to
accept it for other architectures). This would at least prevent remove the
illusion of portability and force the programmer to be explicit.

However, I think we'd much better off if we defined some basic ordering
primitives such as relaxed and RCpc-style acquire/release operations
(including atomic RmW), single-copy atomic memory accesses up to the native
machine size and a full-fence instruction. If your program uses something
that the underlying arch doesn't support, then it is rejected (e.g. 64-bit
READ_ONCE on a 32-bit arch)

That should map straightforwardly to all modern architectures and allow for
efficient codegen on x86 and arm64. It would probably require a bunch of new
BPF instructions that would be defined to be atomic (you already have XADD
as a relaxed atomic add).

Apologies if this sounds patronising, but I'm keen to help figure out the
semantics *now* so that we don't end up having to infer them later on, which
is the usual painful case for memory models. I suspect Peter and Paul would
also prefer to attack it that way around. I appreciate that the temptation
is to avoid the problem by deferring to the underlying hardware memory
model, but I think that will create more problems than it solves and we're
here to help you get this right.

Will

  parent reply	other threads:[~2019-01-30 18:11 UTC|newest]

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-24  4:13 [PATCH v4 bpf-next 0/9] introduce bpf_spin_lock Alexei Starovoitov
2019-01-24  4:13 ` [PATCH v4 bpf-next 1/9] bpf: " Alexei Starovoitov
2019-01-24 18:01   ` Peter Zijlstra
2019-01-24 18:56     ` Peter Zijlstra
2019-01-24 23:42       ` Paul E. McKenney
2019-01-25  0:05         ` Alexei Starovoitov
2019-01-25  1:22           ` Paul E. McKenney
2019-01-25  1:46             ` Jann Horn
2019-01-25  2:38               ` Alexei Starovoitov
2019-01-25  4:27                 ` Alexei Starovoitov
2019-01-25  4:31                   ` Paul E. McKenney
2019-01-25  4:47                     ` Alexei Starovoitov
2019-01-25 16:02                       ` Paul E. McKenney
2019-01-25  4:11               ` Paul E. McKenney
2019-01-25 16:18                 ` Jann Horn
2019-01-25 22:51                   ` Paul E. McKenney
2019-01-25 23:44                     ` Alexei Starovoitov
2019-01-26  0:43                       ` Jann Horn
2019-01-26  0:59                         ` Jann Horn
2019-01-24 23:58     ` Alexei Starovoitov
2019-01-25  0:18       ` Jann Horn
2019-01-25  2:49         ` Alexei Starovoitov
2019-01-25  2:29       ` Eric Dumazet
2019-01-25  2:34         ` Alexei Starovoitov
2019-01-25  2:44           ` Eric Dumazet
2019-01-25  2:57             ` Alexei Starovoitov
2019-01-25  8:38               ` Peter Zijlstra
2019-01-25  9:10       ` Peter Zijlstra
2019-01-25 23:42         ` Alexei Starovoitov
2019-01-28  8:24           ` Peter Zijlstra
2019-01-28  8:31           ` Peter Zijlstra
2019-01-28  8:35             ` Peter Zijlstra
2019-01-28 20:49               ` Alexei Starovoitov
2019-01-28  8:43           ` Peter Zijlstra
2019-01-28 21:37             ` Alexei Starovoitov
2019-01-29  8:59               ` Peter Zijlstra
2019-01-30  2:20                 ` Alexei Starovoitov
2019-01-25  9:59       ` Peter Zijlstra
2019-01-25 10:09       ` Peter Zijlstra
2019-01-25 10:23       ` Peter Zijlstra
2019-01-26  0:17         ` bpf memory model. Was: " Alexei Starovoitov
2019-01-28  9:24           ` Peter Zijlstra
2019-01-28 21:56             ` Alexei Starovoitov
2019-01-29  9:16               ` Peter Zijlstra
2019-01-30  2:32                 ` Alexei Starovoitov
2019-01-30  8:58                   ` Peter Zijlstra
2019-01-30 19:36                     ` Alexei Starovoitov
2019-01-30 18:11               ` Will Deacon [this message]
2019-01-30 18:36                 ` Paul E. McKenney
2019-01-30 19:51                   ` Alexei Starovoitov
2019-01-30 21:05                     ` Paul E. McKenney
2019-01-30 22:57                       ` Alexei Starovoitov
2019-01-31 14:01                         ` Paul E. McKenney
2019-01-31 18:47                           ` Alexei Starovoitov
2019-02-01 14:05                             ` Paul E. McKenney
2019-01-30 19:50                 ` Alexei Starovoitov
2019-01-24  4:13 ` [PATCH v4 bpf-next 2/9] bpf: add support for bpf_spin_lock to cgroup local storage Alexei Starovoitov
2019-01-24  4:13 ` [PATCH v4 bpf-next 3/9] tools/bpf: sync include/uapi/linux/bpf.h Alexei Starovoitov
2019-01-24  4:13 ` [PATCH v4 bpf-next 4/9] selftests/bpf: add bpf_spin_lock tests Alexei Starovoitov
2019-01-24  4:13 ` [PATCH v4 bpf-next 5/9] selftests/bpf: add bpf_spin_lock C test Alexei Starovoitov
2019-01-24  4:14 ` [PATCH v4 bpf-next 6/9] bpf: introduce BPF_F_LOCK flag Alexei Starovoitov
2019-01-24  4:14 ` [PATCH v4 bpf-next 7/9] tools/bpf: sync uapi/bpf.h Alexei Starovoitov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190130181100.GA18558@fuggles.cambridge.arm.com \
    --to=will.deacon@arm.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=ast@kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=jakub.kicinski@netronome.com \
    --cc=jannh@google.com \
    --cc=kernel-team@fb.com \
    --cc=mingo@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=peterz@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).