I absolutely welcome getting rid of macros in this file, but may I request splitting the rte_atomicNN deprecation and the file refactoring into separate commits? I am not convinced refactoring was necessary for the deprecation here, one-line change to `rte_atomic_fetch_add_explicit((type __rte_atomic *)...)` or like would probably do the job.
If refactor, other examples should also be considered, although we of course can do it macro by macro. But in any case we should follow some standard template to reduce number of ways we do stuff in this file if possible. Regardless of the process, please see some stylistic comments below. (Technically, the code looks fine.) > -----Original Message----- > From: Stephen Hemminger <[email protected]> > Sent: Saturday 23 May 2026 20:56 > To: [email protected] > Cc: Stephen Hemminger <[email protected]>; Konstantin Ananyev > <[email protected]>; > Marat Khalili <[email protected]> > Subject: [PATCH v3 04/27] bpf: replace atomic op macro with typed helpers > > The BPF_ST_ATOMIC_REG macro token-pasted the legacy rte_atomicNN_*() > API names. It also stacked three casts on the destination pointer > and reached a 'return 0' out of the macro into the caller's control > flow. > > Replace it with two small static-inline helpers, bpf_atomic32() and > bpf_atomic64(), that dispatch on ins->imm internally and use the C11 > atomic intrinsics directly. The destination is cast once, to a > properly __rte_atomic-qualified pointer. [...] nit: Not sure number of casts is a relevant metric, but I don't see any improvement anyway, probably not worth talking about it. // snip > @@ -105,6 +83,69 @@ > reg[EBPF_REG_0] = op(p[0]); \ > } while (0) > > +/* > + * Atomic ops on the BPF target memory. > + * > + * BPF atomic instructions encode the destination as base register + > + * signed offset, with the value to combine taken from src_reg. nit: This applies to any eBPF instruction. And the part that is unusual about them that they can modify the source register is not mentioned. > + * > + * Memory order: seq_cst preserves the previous behavior of > + * rte_atomicNN_add() / rte_atomicNN_exchange() and matches what the > + * Linux kernel BPF interpreter does for these opcodes. Not sure we should refer to the macros we are removing from the code in the comment, this information belongs to the commit message. > + * > + * Returns 0 on unsupported sub-op (validator should have rejected it), > + * 1 otherwise. This is an unusual convention, we typically use negative value for errors, or booleans in some cases. To clarify, the original `return 0` was specifying return value from the program, not an error code. For historical reasons in rare cases when the VM cannot continue the program returns zero (maybe we should reconsider it). > + */ > +static inline int > +bpf_atomic32(const struct rte_bpf *bpf, uint64_t reg[EBPF_REG_NUM], > + const struct ebpf_insn *ins) > +{ > + /* need to casts to make bpf memory suitable for C11 atomic */ Typo in "need to casts"? (Also not sure what we warn about here.) > + uint32_t __rte_atomic *dst > + = (uint32_t __rte_atomic *)(uintptr_t)(reg[ins->dst_reg] + > ins->off); nit: Is it `uint32_t __rte_atomic *` or `RTE_ATOMIC(uint32_t) *`? I honestly don't know why we have both, but the latter seems more popular in the codebase. > + uint32_t val = (uint32_t)reg[ins->src_reg]; > + > + switch (ins->imm) { > + case BPF_ATOMIC_ADD: > + rte_atomic_fetch_add_explicit(dst, val, > rte_memory_order_seq_cst); > + return 1; > + case BPF_ATOMIC_XCHG: > + reg[ins->src_reg] = rte_atomic_exchange_explicit(dst, val, > + > rte_memory_order_seq_cst); nit: This is not a typical style of indentation for this file. > + return 1; > + default: > + RTE_BPF_LOG_LINE(ERR, > + "%s(%p): unsupported atomic operation at pc: %#zx;", > + __func__, bpf, > + (uintptr_t)ins - (uintptr_t)bpf->prm.ins); > + return 0; This was an optional defensive programming. Since other functions like `bpf_alu_be` do not have any default label we can arguably just remove it here and return void (also not accept bpf argument). With the macro it all was at least transparent to the caller. > + } > +} // snip the rest
