Re: [PATCH 06/15] ebpf-docs: Use standard type convention in standard doc

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: dthaler1968@googlemail.com
Cc: bpf@vger.kernel.org, Dave Thaler <dthaler@microsoft.com>
Subject: Re: [PATCH 06/15] ebpf-docs: Use standard type convention in standard doc
Date: Fri, 30 Sep 2022 13:49:20 -0700	[thread overview]
Message-ID: <20220930204920.gamkmalcr5g5u3bg@macbook-pro-4.dhcp.thefacebook.com> (raw)
In-Reply-To: <20220927185958.14995-6-dthaler1968@googlemail.com>

On Tue, Sep 27, 2022 at 06:59:49PM +0000, dthaler1968@googlemail.com wrote:
> From: Dave Thaler <dthaler@microsoft.com>
> 
> Signed-off-by: Dave Thaler <dthaler@microsoft.com>

Please always add commit log even it's just a copy paste of a subject line.

The patch subj should be 'bpf, docs:'.
I fixed it up while applying the first 5 patches.

> ---
>  Documentation/bpf/instruction-set.rst | 14 ++++++++++----
>  Documentation/bpf/linux-notes.rst     |  6 ++++++
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/bpf/instruction-set.rst b/Documentation/bpf/instruction-set.rst
> index 4997d2088..a24bc5d53 100644
> --- a/Documentation/bpf/instruction-set.rst
> +++ b/Documentation/bpf/instruction-set.rst
> @@ -7,6 +7,10 @@ eBPF Instruction Set Specification, v1.0
>  
>  This document specifies version 1.0 of the eBPF instruction set.
>  
> +Documentation conventions
> +=========================
> +
> +This specification uses the standard C types (uint32_t, etc.) in documentation.
>  
>  Registers and calling convention
>  ================================
> @@ -114,7 +118,9 @@ BPF_END   0xd0   byte swap operations (see `Byte swap instructions`_ below)
>  
>  ``BPF_ADD | BPF_X | BPF_ALU`` means::
>  
> -  dst_reg = (u32) dst_reg + (u32) src_reg;
> +  dst_reg = (uint32_t) dst_reg + (uint32_t) src_reg;
> +
> +where '(uint32_t)' indicates truncation to 32 bits.

This part I'm not excited about.
imo the "standard" types are too verbose. They make the doc harder to read.
I think it would be just fine for the standard to say in the conventions section
that u32 is a 32-bit unsigned integer and that's how it's used in the doc.
u32 would be a name that the doc uses. Name match with linux type is 'accidental'.

>  ``BPF_ADD | BPF_X | BPF_ALU64`` means::
>  
> @@ -122,7 +128,7 @@ BPF_END   0xd0   byte swap operations (see `Byte swap instructions`_ below)
>  
>  ``BPF_XOR | BPF_K | BPF_ALU`` means::
>  
> -  src_reg = (u32) src_reg ^ (u32) imm32
> +  src_reg = (uint32_t) src_reg ^ (uint32_t) imm32
>  
>  ``BPF_XOR | BPF_K | BPF_ALU64`` means::
>  
> @@ -276,11 +282,11 @@ BPF_XOR   0xa0   atomic xor
>  
>  ``BPF_ATOMIC | BPF_W  | BPF_STX`` with 'imm' = BPF_ADD means::
>  
> -  *(u32 *)(dst_reg + off16) += src_reg
> +  *(uint32_t *)(dst_reg + off16) += src_reg
>  
>  ``BPF_ATOMIC | BPF_DW | BPF_STX`` with 'imm' = BPF ADD means::
>  
> -  *(u64 *)(dst_reg + off16) += src_reg
> +  *(uint32_t *)(dst_reg + off16) += src_reg

Bug.
And the later patch fixes it :(
Let's not add it in the first place.

>  In addition to the simple atomic operations, there also is a modifier and
>  two complex atomic operations:
> diff --git a/Documentation/bpf/linux-notes.rst b/Documentation/bpf/linux-notes.rst
> index 1c31379b4..522ebe27d 100644
> --- a/Documentation/bpf/linux-notes.rst
> +++ b/Documentation/bpf/linux-notes.rst
> @@ -7,6 +7,12 @@ Linux implementation notes
>  
>  This document provides more details specific to the Linux kernel implementation of the eBPF instruction set.
>  
> +Arithmetic instructions
> +=======================
> +
> +While the eBPF instruction set document uses the standard C terminology as the cross-platform specification,
> +in the Linux kernel, uint32_t is expressed as u32, uint64_t is expressed as u64, etc.
> +
>  Byte swap instructions
>  ======================
>  
> -- 
> 2.33.4
> 