[External] Re: [PATCH v5 2/3] riscv: introduce percpu.h into include/asm
yunhui cui
cuiyunhui at bytedance.com
Wed Jul 1 04:34:12 PDT 2026
Hi bot+bpf-ci,
On Wed, Jul 1, 2026 at 1:06 PM <bot+bpf-ci at kernel.org> wrote:
>
> > diff --git a/arch/riscv/include/asm/percpu.h b/arch/riscv/include/asm/percpu.h
> > new file mode 100644
> > index 000000000000..46f1901d3bb8
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/percpu.h
> > @@ -0,0 +1,283 @@
>
> [ ... ]
>
> > +#define PERCPU_8_16_OP(name, amo_insn, sz, sfx, val_type, new_val_expr, asm_op) \
> > +static inline void __percpu_##name##_amo_case_##sz(void *ptr, unsigned long val) \
> > +{ \
> > + if (IS_ENABLED(CONFIG_RISCV_ISA_ZABHA) && \
> > + riscv_has_extension_unlikely(RISCV_ISA_EXT_ZABHA)) { \
> > + asm volatile ("amo" #amo_insn #sfx " zero, %[val], %[ptr]" \
> > + : [ptr] "+A"(*(val_type *)ptr) \
> > + : [val] "r"((val_type)((new_val_expr) & PERCPU_8_16_GET_MASK(sz))) \
> > + : "memory"); \
> > + } else { \
> > + u32 *ptr32 = PERCPU_8_16_GET_PTR32(ptr); \
> > + const unsigned long shift = PERCPU_8_16_GET_SHIFT(ptr); \
> > + const u32 mask = PERCPU_8_16_GET_MASK(sz) << shift; \
> > + const val_type val_trunc = (val_type)((new_val_expr) \
> > + & PERCPU_8_16_GET_MASK(sz)); \
> > + u32 retx, rc; \
> > + val_type new_val_type; \
> > + \
This does not impact functional correctness. I will rename LR/SC
fallback local variables to use consistent naming, and keep the return
helper separate since it only handles add operations for now.
> > + asm volatile ( \
> > + "0: lr.w %0, %2\n" \
> > + "and %3, %0, %4\n" \
> > + "srl %3, %3, %5\n" \
> > + #asm_op " %3, %3, %6\n" \
> > + "and %3, %3, %8\n" \
> > + "sll %3, %3, %5\n" \
> > + "and %1, %0, %7\n" \
> > + "or %1, %1, %3\n" \
> > + "sc.w %1, %1, %2\n" \
> > + "bnez %1, 0b\n" \
> > + : "=&r"(retx), "=&r"(rc), "+A"(*ptr32), "=&r"(new_val_type) \
> > + : "r"(mask), "r"(shift), "r"(val_trunc), "r"(~mask), \
> > + "r"(PERCPU_8_16_GET_MASK(sz)) \
> > + : "memory"); \
> > + } \
> > +}
>
> [ ... ]
>
> > +#define PERCPU_8_16_RET_OP(name, amo_insn, sz, sfx, val_type, new_val_expr) \
> > +static inline val_type __percpu_##name##_return_amo_case_##sz(void *ptr, unsigned long val) \
> > +{ \
> > + if (IS_ENABLED(CONFIG_RISCV_ISA_ZABHA) && \
> > + riscv_has_extension_unlikely(RISCV_ISA_EXT_ZABHA)) { \
> > + register val_type ret; \
> > + asm volatile ("amo" #amo_insn #sfx " %[ret], %[val], %[ptr]" \
> > + : [ptr] "+A"(*(val_type *)ptr), [ret] "=r"(ret) \
> > + : [val] "r"((val_type)((new_val_expr) & PERCPU_8_16_GET_MASK(sz))) \
> > + : "memory"); \
> > + return ret + (val_type)((new_val_expr) & PERCPU_8_16_GET_MASK(sz)); \
> > + } else { \
> > + u32 *ptr32 = PERCPU_8_16_GET_PTR32(ptr); \
> > + const unsigned long shift = PERCPU_8_16_GET_SHIFT(ptr); \
> > + const u32 mask = (PERCPU_8_16_GET_MASK(sz) << shift); \
> > + const u32 inv_mask = ~mask; \
> > + const val_type val_trunc = (val_type)((new_val_expr) \
> > + & PERCPU_8_16_GET_MASK(sz)); \
> > + u32 old, new, tmp; \
>
> This isn't a bug, but the two LR/SC fallback loops name the same roles
> differently. The non-return loop uses:
>
> u32 retx, rc;
> val_type new_val_type;
>
> while the return loop above uses:
>
> u32 old, new, tmp;
>
> The loaded word, the scratch/merge register and the computed field play the
> same role in both loops. Could they use one consistent set of names (e.g.
> old/new/tmp) so the two loops read as clearly parallel? As a smaller point,
> new_val_type reads like a type name rather than a value, and new shadows a
> common keyword.
>
> > + \
> > + asm volatile ( \
> > + "0: lr.w %0, %3\n" \
> > + "and %1, %0, %4\n" \
> > + "srl %1, %1, %5\n" \
> > + "add %1, %1, %6\n" \
> > + "and %1, %1, %7\n" \
> > + "sll %1, %1, %5\n" \
> > + "and %2, %0, %8\n" \
> > + "or %2, %2, %1\n" \
> > + "sc.w %2, %2, %3\n" \
> > + "bnez %2, 0b\n" \
>
> This isn't a bug, but the read-modify-write LR/SC sequence here is nearly
> identical to the one in PERCPU_8_16_OP, differing only in operand numbering.
> The non-return loop parameterises the operation via #asm_op, but this loop
> hardcodes add:
>
> "add %1, %1, %6\n"
>
> Was the intent to keep the return path add-only, or could the two loops share
> one templated sequence?
>
> > + : "=&r"(old), "=&r"(tmp), "=&r"(new), "+A"(*ptr32) \
> > + : "r"(mask), "r"(shift), "r"(val_trunc), "r"(PERCPU_8_16_GET_MASK(sz)), \
> > + "r"(inv_mask) \
> > + : "memory"); \
> > + return (val_type)(tmp >> shift); \
> > + } \
> > +}
>
> [ ... ]
>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/28494034360
Thanks,
Yunhui
More information about the linux-riscv
mailing list