Re: [Bpf] [PATCH bpf-next v2] bpf, docs: Improve English readability

[Alexei Starovoitov <[e-mail]>]

On Thu, Jul 6, 2023 at 3:04 PM Dave Thaler <[e-mail]> wrote:
>
> > -----Original Message-----
> > From: Alexei Starovoitov <[e-mail]>
> > Sent: Thursday, July 6, 2023 1:42 PM
> > To: Dave Thaler <[e-mail]>
> > Cc: [e-mail]; [e-mail]; Dave Thaler
> > <[e-mail]>
> > Subject: Re: [PATCH bpf-next v2] bpf, docs: Improve English readability
> >
> > On Thu, Jul 06, 2023 at 04:05:37PM +0000, Dave Thaler wrote:
> > > From: Dave Thaler <[e-mail]>
> > >
> > > Signed-off-by: Dave Thaler <[e-mail]>
> > > --
> > > V1 -> V2: addressed comments from Alexei
> > > ---
> > >  Documentation/bpf/instruction-set.rst | 59 ++++++++++++++++++++-------
> > >  Documentation/bpf/linux-notes.rst     |  5 +++
> > >  2 files changed, 50 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/Documentation/bpf/instruction-set.rst
> > > b/Documentation/bpf/instruction-set.rst
> > > index 751e657973f..740989f4c1e 100644
> > > --- a/Documentation/bpf/instruction-set.rst
> > > +++ b/Documentation/bpf/instruction-set.rst
> > > @@ -7,6 +7,9 @@ eBPF Instruction Set Specification, v1.0
> > >
> > >  This document specifies version 1.0 of the eBPF instruction set.
> > >
> > > +The eBPF instruction set consists of eleven 64 bit registers, a
> > > +program counter, and an implementation-specific amount (e.g., 512 bytes)
> > of stack space.
> > > +
> > >  Documentation conventions
> > >  =========================
> > >
> > > @@ -27,12 +30,24 @@ The eBPF calling convention is defined as:
> > >  * R6 - R9: callee saved registers that function calls will preserve
> > >  * R10: read-only frame pointer to access stack
> > >
> > > -R0 - R5 are scratch registers and eBPF programs needs to spill/fill
> > > them if -necessary across calls.
> > > +Registers R0 - R5 are caller-saved registers, meaning the BPF program
> > > +needs to either spill them to the BPF stack or move them to callee
> > > +saved registers if these arguments are to be reused across multiple
> > > +function calls. Spilling means that the value in the register is
> > > +moved to the BPF stack. The reverse operation of moving the variable
> > from the BPF stack to the register is called filling.
> > > +The reason for spilling/filling is due to the limited number of registers.
> >
> > imo this extended explanation goes too far.
> > It's also not entirely correct. We could have an ISA with limited number of
> > registers where every register is callee saved. A bit absurd, but possible.
> > Or went with SPARC style register windows.
>
> At https://lore.kernel.org/bpf/[e-mail]/ you said about the above
> "I like above clarification though."

That was on "30 Sep 2022".
I like to change my mind often enough to confuse everyone :)

> I think it's important for interoperability to define which registers are caller-saved
> and which are not, so a compiler (or even verifier) can be used for multiple runtimes.

but it really doesn't belong in the ISA doc.
We've discussed it at length.
We need the psABI doc.
ISA doc is a description of instructions and _not_ how they shape
into functions and programs.
More below.

>
> > > +
> > > +Upon entering execution of an eBPF program, registers R1 - R5
> > > +initially can contain the input arguments for the program (similar to the
> > argc/argv pair for a typical C program).
> >
> > argc/argv is only for main(). We don't have main() concept in BPF ISA.
> > argc/argv is also not a property of ISA.
>
> That's why it's "similar to".  I think the analogy helps understanding for new readers.

argc is a K&R C thing. Not even a calling convention.
That sentence is a combination of analogies from different areas.
argc could be interpreted that the first argument is a count
and a second argument is an array.
See the confusion it might cause?

>
> > > +The actual number of registers used, and their meaning, is defined by
> > > +the program type; for example, a networking program might have an
> > > +argument that includes network packet data and/or metadata.
> >
> > that makes things even more confusing.
> >
> > tbh none of the above changes make the doc easier to read.
>
> The program type defines the number and meaning of any arguments passed
> to the program.  In the ISA that means the number of registered used to
> pass inputs, and their contents.

Not at all. ISA is an instruction set only and this doc is for ISA.
What different program types should accept belongs in a different doc.
And it's not a psABI doc.
More below.

>
> > >  Instruction encoding
> > >  ====================
> > >
> > > +An eBPF program is a sequence of instructions.
> >
> > Kinda true, but it opens the door for plenty of bike shedding.
> > Is it contiguous sequence? what about subprograms?
> > Is BPF program a one function or multiple functions?
>
> The term "subprogram" is not currently part of the
> instruction-set.rst doc.   "Program-local functions"
> are, and the text says they're part of the same BPF program.
> Hence the doc already says a BPF program can have multiple
> functions.

Yes. It's a mess, but we must not make it worse.
instruction-set.rst is for instructions.
Definition of BPF program belongs to a different doc.

> > etc.
> > Just not worth it.
> > This is ISA doc.
> >
> > > +
> > >  eBPF has two instruction encodings:
> > >
> > >  * the basic instruction encoding, which uses 64 bits to encode an
> > > instruction @@ -74,7 +89,7 @@ For example::
> > >    07     1       0        00 00  11 22 33 44  r1 += 0x11223344 // big
> > >
> > >  Note that most instructions do not use all of the fields.
> > > -Unused fields shall be cleared to zero.
> > > +Unused fields must be set to zero.
> >
> > How is this better?
>
> It uses the language common in RFCs.
>
> > >  As discussed below in `64-bit immediate instructions`_, a 64-bit
> > > immediate  instruction uses a 64-bit immediate value that is constructed as
> > follows.
> > > @@ -103,7 +118,9 @@ instruction are reserved and shall be cleared to
> > zero.
> > >  Instruction classes
> > >  -------------------
> > >
> > > -The three LSB bits of the 'opcode' field store the instruction class:
> > > +The encoding of the 'opcode' field varies and can be determined from
> > > +the three least significant bits (LSB) of the 'opcode' field which
> > > +holds the "instruction class", as follows:
> >
> > same question. Don't see an improvement in wording.
>
> 1. The acronym LSB was not defined and does not have an asterisk by it in
> the https://www.rfc-editor.org/materials/abbrev.expansion.txt list.
>
> 2. "LSB bits" is redundant.
>
> 3. Putting "instruction class" in quotes is common when defining by use
> the first time.

ok. fair enough.

>
> linux-notes.rst is not in the ISA doc.   The ISA doc says the value is implementation
> defined.  linux-notes.rst says what Linux does for things the ISA doc leaves up
> to the implementation.

I see no value in "Linux currently supports 512 bytes of stack space" sentence.
It's too ambiguous to be useful.

instruction-set.rst is a description of ISA. We should remove things
from it that don't belong instead of doubling down on the current mess.

we need the psABI doc that will not be standardized as Jose recommended.
That doc will describe recommended calling convention, argument
promotion, stack usage, relocation, function/subprogram definition, etc
psABI probably should include BTF.ext description, but I'm open to alternatives.

BPF program types supported by the kernel is a 3rd document.
There we can explain that XDP prog takes a single ctx argument and it's
a pointer to ...
Such info is linux specific. Based on ISA and psABI one can come up
with different program/map types and semantics of their arguments
while being fully compliant with ISA and psABI docs.
Such doc can start its journey in linux-notes.rst and then split, if necessary.