Re: [PATCH bpf-next v2] bpf, docs: add LOAD_ACQUIRE and STORE_RELEASE instructions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The newly added 8-bit and 16-bit atomic instructions lack an explicit conformance group assignment.
- [Medium] The documentation omits the use of the `offset` field for LOAD_ACQ address calculation and lacks explicit pseudo-code for both instructions.
- [Low] The documentation explicitly states that 8-bit and 16-bit LOAD_ACQ values are zero-extended, creating an ambiguity regarding 32-bit values.
--

commit 442218b588f0cff62adb7ca3a7b372a864af4450
Author: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

bpf, docs: add LOAD_ACQUIRE and STORE_RELEASE instructions

This commit updates the BPF instruction set documentation to include the
LOAD_ACQUIRE and STORE_RELEASE atomic instruction modifiers. It adds the
missing entries for these instructions, including 8-bit and 16-bit widths,
and explains their complex atomic encoding.

> diff --git a/Documentation/bpf/standardization/instruction-set.rst b/Documentation/bpf/standardization/instruction-set.rst
> index 39c74611752b6..e8b33374bd090 100644
> --- a/Documentation/bpf/standardization/instruction-set.rst
> +++ b/Documentation/bpf/standardization/instruction-set.rst
> @@ -668,7 +668,8 @@ that use the ``ATOMIC`` mode modifier as follows:
>    part of the "atomic32" conformance group.
>  * ``{ATOMIC, DW, STX}`` for 64-bit operations, which are
>    part of the "atomic64" conformance group.
> -* 8-bit and 16-bit wide atomic operations are not supported.
> +* ``{ATOMIC, H, STX}`` (only for LOAD_ACQ/STORE_REL)
> +* ``{ATOMIC, B, STX}`` (only for LOAD_ACQ/STORE_REL)

[Severity: Medium]
Since unassigned instructions default to the base32 conformance group, does
this inadvertently make these 8-bit and 16-bit instructions mandatory for all
implementations?

Should these be assigned to the atomic32, atomic64, or a new conformance group
to avoid imposing this requirement on runtimes that may not support them?

[ ... ]

> @@ -721,6 +724,14 @@ The ``CMPXCHG`` operation atomically compares the value addressed by
>  value that was at ``dst + offset`` before the operation is zero-extended
>  and loaded back to ``R0``.
>  
> +The ``LOAD_ACQ`` and ``STORE_REL`` operations allow using lighter load and
> +store memory barriers rather than full barriers. The corresponding accesses
> +must be aligned, but are allowed for any access size (8-bit up to 64-bit
> +operations), with 8-bit and 16-bit ``LOAD_ACQ`` loaded values being
> +zero-extended. As atomics are encoded as stores, the meaning of dst and src

[Severity: Low]
Does explicitly singling out 8-bit and 16-bit sizes for zero-extension create
ambiguity for 32-bit operations?

While 32-bit operations writing to a register zero-extend to 64 bits by default,
readers might assume 32-bit LOAD_ACQ values require different handling if only
the smaller widths are explicitly mentioned.

> +are different for ``LOAD_ACQ``, effectively using src as memory based
> +pointer and dst as destination register for the fetched value.

[Severity: Medium]
Does the LOAD_ACQ instruction also use the offset field for the address
calculation?

The text states it uses src as the memory pointer, but omits the offset field.
Other atomic operations explicitly use dst + offset.

Would it also be clearer to provide explicit pseudo-code definitions for both
LOAD_ACQ and STORE_REL (e.g., dst = load_acquire((size *)(src + offset))) to
remove ambiguity for JIT compiler and emulator implementers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-bpf-insn-doc-v2-1-8c43c037d599@bootlin.com?part=1