Re: [Bpf] BPF ISA conformance groups

[Alexei Starovoitov <[e-mail]>]

On Thu, Dec 7, 2023 at 1:51 PM David Vernet <[e-mail]> wrote:
>
> On Sat, Dec 02, 2023 at 11:51:50AM -0800, dthaler1968=[e-mail] wrote:
> > >From David Vernet's WG summary:
> > > After this update, the discussion moved to a topic for the BPF ISA
> > document that has yet to be resolved:
> > > ISA RFC compliance. Dave pointed out that we still need to specify which
> > instructions in the ISA are
> > > MUST, SHOULD, etc, to ensure interoperability.  Several different options
> > were presented, including
> > >  having individual-instruction granularity, following the clang CPU
> > versioning convention, and grouping
> > > instructions by logical functionality.
> > >
> > > We did not obtain consensus at the conference on which was the best way
> > forward. Some of the points raised include the following:
> > >
> > > - Following the clang CPU versioning labels is somewhat arbitrary. It
> > >   may not be appropriate to standardize around grouping that is a result
> > >   of largely organic historical artifacts.
> > > - If we decide to do logical grouping, there is a danger of
> > >   bikeshedding. Looking at anecdotes from industry, some vendors such as
> > >   Netronome elected to not support particular instructions for
> > >   performance reasons.
> >
> > My sense of the feedback in general was to group instructions by logical
> > functionality, and only create separate
> > conformance groups where there is some legitimate technical reason that a
> > runtime might not want to support
> > a given set of instructions.  Based on discussion during the meeting, here's
> > a strawman set of conformance
> > groups to kick off discussion.  I've tried to use short (like 6 characters
> > or fewer) names for ease of display in
> > document tables, and potentially in command line options to tools that might
> > want to use them.
> >
> > A given runtime platform would be compliant to some set of the following
> > conformance groups:
> >
> > 1. "basic": all instructions not covered by another group below.
> > 2. "atomic": all Atomic operations.  I think Christoph argued for this one
> > in the meeting.
> > 3. "divide": all division and modulo operations.  Alexei said in the meeting
> > that he'd heard demand for this one.
> > 4. "legacy": all legacy packet access instructions (deprecated).
> > 5. "map": 64-bit immediate instructions that deal with map fds or map
> > indices.
> > 6. "code": 64-bit immediate instruction that has a "code pointer" type.
> > 7. "func": program-local functions.
>
> I thought for a while about whether this should be part of the basic
> conformance group, and talked through it with Jakub Kicinski. I do think
> it makes sense to keep it separate like this. For e.g. devices with
> Harvard architectures, it could get quite non-trivial for the verifier
> to determine whether accesses to arguments stored in special register
> are safe. Definitely not impossible, and overall very useful to support
> this, but in order to ease vendor adoption it's probably best to keep
> this separate.
>
> > Things that I *think* don't need a separate conformance group (can just be
> > in "basic") include:
> > a. Call helper function by address or BTF ID.  A runtime that doesn't
> > support these simply won't expose any
> >     such helper functions to BPF programs.
> > b. Platform variable instructions (dst = var_addr(imm)).  A runtime that
> > doesn't support this simply won't
> >     expose any platform variables to BPF programs.
> >
> > Comments? (Let the bikeshedding begin...)
>
> This list seems logical to me,

I think we should do just two categories: legacy and the rest,
since any scheme will be flawed and infinite bikeshedding will ensue.
For example, let's take a look at #2 atomic...
Should it include or exclude atomic_add insn ? It was added
at the very beginning of BPF ISA and was used from day one.
Without it it's impossible to count stats. The typical network or
tracing use case needs to count events and one cannot do it without
atomic increment. Eventually per-cpu maps were added as an alternative.
I suspect any platform that supports #1 basic insn without
atomic_add will not be practically useful.
Should atomic_add be a part of "basic" then? But it's atomic.
Then what about atomic_fetch_add insn? It's pretty close semantically.
Part of atomic or part of basic?

Another example, #3 divide. bpf cpu=v1 ISA only has unsigned div/mod.
Eventually we added a signed version. Integer division is one of the
slowest operations in a HW. Different cpus have different flavors
of them 64/32 64/64 32/32, etc. All with different quirks.
cpu=v1 had modulo insn because in tracing one often needs to do it
to select a slot in a table, but in networking there is rarely a need.
So bpf offload into netronome HW doesn't support it (iirc).
Should div/mod signed/unsigned be a part of basic? or separate?
Only 32 or 64 bit?

Hence my point: legacy and the rest (as of cpu=v4) are the only two categories
we should have in _this_ version of the standard.
Rest assured we will add new insn in the coming months.
I suggest we figure out conformance groups for future insns at that time.
That would be the time to argue and actually extract value out of discussion.
Retroactive bike shedding is a bike shedding and nothing else.