On Tue, Dec 12, 2023 at 02:55:19PM -0800, Alexei Starovoitov wrote:
> On Tue, Dec 12, 2023 at 2:01 PM <dthaler1968@xxxxxxxxxxxxxx> wrote:
> >
> > > > 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?
> > >
> > > I think it's reasonable to expect that if you require an atomic add, that you
> > > may also require the other atomic instructions as well and that it would be
> > > logical to group them together, yes. I believe that Netronome supports all of
> > > the atomic instructions, as one example. If you're providing a BPF runtime in
> > > an environment where atomic adds are required, I think it stands to reason
> > > that you should probably support the other atomics as well, no?
> >
> > I agree.
>
> Your logical reasoning is indeed correct and
> I agree with it,
> but reality is different :)
>
> drivers/net/ethernet/netronome/nfp/bpf/jit.c:
> static int mem_atomic8(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
> {
> if (meta->insn.imm != BPF_ADD)
> return -EOPNOTSUPP;
>
> return mem_xadd(nfp_prog, meta, true);
> }
>
> It only supports atomic_add and no other atomics.
Ahh, I misunderstood when I discussed with Kuba. I guess they supported
only atomic_add because packets can be delivered out of order. So fair
enough on that point, but I still stand by the claim though that if you
need one type of atomic, it's reasonable to infer that you may need all
of them. I would be curious to hear how much work it would have been to
add support for the others. If there was an atomic conformance group,
maybe they would have.
> > > > 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).
> > >
> > > Correct, my understanding is that BPF offload in netronome supports neither
> > > division nor modulo.
> >
> > In my opinion, this is a valid technical reason to put them into a separate
> > conformance group, to allow hardware offload cards to support BPF without
> > requiring division/modulo which they might not have space or other budget for.
>
> Also logically correct and I agree with, but reality proves all of us wrong.
> netronome doesn't support modulo,
> but it supports integer division when the verifier can determine
> property of the constant.
> BPF_ALU64 | BPF_DIV | BPF_K works for positive imm32,
> but BPF_X works when the verifier is smart with plenty of quirks
> and subtle conditions.
> It works with the help of cool math reciprocal_value_adv()
> in include/linux/reciprocal_div.h
> which converts div to shifts and muls.
>
> So should div_K and div_X be in separate groups ?
> Should mod_[K|X] be there as well or not?
Also fair enough r.e. the positive imm32 division. I clearly should have
read the netronome jit code. For devices I do agree with you that it's
questionable whether whether compliance is realistic. But I think we
just don't _really_ know at this point. And regardless, I do think
there's value in providing some kind of structure to inform what classes
of instructions should typically be provided when possible. I also
wonder whether we need to consider the cost calculus for whether they
would bother to support certain instructions if there was a conformance
group. To the atomic point above, I would be surprised if it would have
been hugely difficult to add support for the others for Netronome.
Also, I don't think we should only be looking at Netronome as the
canonical example here. From my understanding, while there's still quite
a bit of work to be done, a lot of Cilium progs can run on both Windows
and Linux.
> To determine the grouping should we use logic or reality?
Let's bear in mind that "reality" in the context of this conversation is
a single vendor. Part of the goal of the standard is to implement
something that has broad applicability. So while reality is of course
very relevant, I think we should also apply some degree of logic to
inform future implementations.
> I'm arguing that whatever clean and logical grouping we can come up with
> it won't stand a test of real use.
Well, maybe not for Netronome, or maybe not even for any vendor (though
we have no way of knowing that yet), but what about for other contexts
like Windows / Linux cross-platform compat?
The answer might be "no", but to go back to my previous message, if
we're going to get rid of conformance groups I think we should at least
be explicit that we're doing so because we don't think compliance is a
realistic goal in general.
Attachment:
signature.asc
Description: PGP signature
