Re: [Bpf] [PATCH bpf-next] bpf, docs: Add extended call instructions

[David Vernet <void@manifault.com>]

On Sat, Mar 11, 2023 at 03:53:47PM -0600, David Vernet wrote:
> On Sat, Mar 11, 2023 at 09:00:19PM +0000, Dave Thaler wrote:
> > David Vernet <void@manifault.com> wrote:
> > [...]
> > > > +BPF_CALL  0x8    0x0  call helper function imm    see `Helper functions`_
> > > > +BPF_CALL  0x8    0x1  call PC += offset           see `eBPF functions`_
> > > > +BPF_CALL  0x8    0x2  call runtime function imm   see `Runtime functions`_
> > > 
> > > The names "Helper functions", "eBPF functions", and "Runtime functions"
> > > feel, for lack of a better term, insufficiently distinct. I realize that it's very tricky
> > > to get the naming right here given that some of these terms (helpers +
> > > runtime functions) are conceptually the same thing, but as a reader with no
> > > background I expect I'd be confused by what the distinctions are as they all
> > > kind of sound like the same thing. What do you think of this as an alternative:
> > > 
> > > 1. Standard helper functions
> > > 2. BPF-local functions
> > > 3. Platform-specific helper functions
> > 
> > I like where you're going with this.  However, the fact is that some of the existing
> > Helper functions are actually very platform-specific and won't apply to other
> > platforms. So retroactively labeling all of them "standard" is somewhat problematic
> > in my view.
> 
> That makes sense. For what it's worth, I was envisioning us specifying
> both the helper number (and likely the semantics of those helpers) in
> the standard, and just skipping over any which are Linux-specific.
> That's of course assuming we do decide to include functions in the
> standard, which to my understanding is not yet finalized.
> 
> Regardless, I'll certainly defer to your expertise on when it's
> appropriate to use the word "standard", and I could see why it would be
> problematic to do so here.
> 
> > 
> > I do like the idea of using "<some adjective> helper functions" for both 1 and 3
> > though.  Since we might not choose to standardize all the existing type 1 functions,
> > maybe "Platform-agnostic helper functions" is synonymous and pairs nicely
> > With "Platform-specific helper functions" as a term.  And then we could just have
> > a note in the linux-notes.rst saying Linux has some platform-specific helper functions that for historical reasons are used with the platform-agnostic helper function
> > Instruction.
> 
> That's a reasonable option as well. The only thing that gives me pause
> is that, as you know, the plan of record for now in Linux is to have all
> new BPF -> main kernel functions added as kfuncs. That includes features
> which are "platform agnostic", such as BPF iterators. I know you've
> previously raised the idea of having the traditional helpers be used as
> standard / platform-agnostic helpers in BPF office hours, so this isn't
> out of the blue. It seems that the time has come to discuss it more
> concretely.

One thing to clarify -- I'm _not_ saying we should revisit the kfunc vs.
BPF helper discussion. Rather, just that we should decide exactly what
the older BPF helper instruction encoding means in terms of a generic
BPF instruction set.

> > 
> > > The idea with the latter is of course that the platform can choose to
> > > implement whatever helper functions (kfuncs for Linux) apply exclusively to
> > > that platform. I think we'd need some formal encoding for this in the
> > > standard, so it seems appropriate to apply it here. What do you think?
> > 
> > Agree with that.
> > 
> > > > +BPF_EXIT  0x9    0x0  return                      BPF_JMP only
> > > > +BPF_JLT   0xa    any  PC += offset if dst < src   unsigned
> > > > +BPF_JLE   0xb    any  PC += offset if dst <= src  unsigned
> > > > +BPF_JSLT  0xc    any  PC += offset if dst < src   signed
> > > > +BPF_JSLE  0xd    any  PC += offset if dst <= src  signed
> > > > +========  =====  ===  ==========================
> > > > +========================
> > > >
> > > >  The eBPF program needs to store the return value into register R0
> > > > before doing a  BPF_EXIT.
> > > > @@ -272,6 +274,18 @@ set of function calls exposed by the runtime.
> > > > Each helper  function is identified by an integer used in a ``BPF_CALL``
> > > instruction.
> > > >  The available helper functions may differ for each program type.
> > > >
> > > > +Runtime functions
> > > > +~~~~~~~~~~~~~~~~~
> > > > +Runtime functions are like helper functions except that they are not
> > > > +specific to eBPF programs.  They use a different numbering space from
> > > > +helper functions,
> > > 
> > > Per my suggestion above, should we rephrase this as "platform-specific"
> > > helper functions? E.g. something like:
> > > 
> > > Platform-specific helper functions are helper functions that may be unique to
> > > a particular platform. An encoding for a platform-specific function on one
> > > platform may or may not correspond to the same function on another
> > > platform. Platforms are not required to implement any platform-specific
> > > function.
> > 
> > That looks good to me, will incorporate.
> > 
> > > 
> > > As alluded to above, the fact that they're not specific to BPF seems like an
> > > implementation detail from the perspective of the encoding / standard.
> > > Thoughts?
> > > 
> > > > +but otherwise the same considerations apply.
> > > > +
> > > > +eBPF functions
> > > > +~~~~~~~~~~~~~~
> > > > +eBPF functions are functions exposed by the same eBPF program as the
> > > > +caller, and are referenced by offset from the call instruction, similar to
> > > ``BPF_JA``.
> > > > +A ``BPF_EXIT`` within the eBPF function will return to the caller.
> > > 
> > > Suggestion: Can we simply say BPF instead of eBPF? At this point I'm not sure
> > > what the 'e' distinction really buys us, though I'm sure I'm missing context
> > > from (many) prior discussions. I also don't want to bikeshed too much on this
> > > point for your patch, so if it becomes a "thing" then feel free to ignore.
> > 
> > Will remove for consistency with the other patches I submitted that already
> > omitted the "e".   I think Alexei had the same comment a while back and
> > I missed updating this proposed section at the time.  Thanks.
> > 
> > Dave
> > 
> > > 
> > > Thanks,
> > > David