Re: [PATCH v15 04/16] arm64: support WFET in smp_cond_load_relaxed_timeout()

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2458 bytes --]

> arm64: support WFET in smp_cond_load_relaxed_timeout()
>
> To handle WFET use __cmpwait_timeout() similarly to __cmpwait(). These
> call out to the respective __cmpwait_case_timeout_##sz(),
> __cmpwait_case_##sz() functions.

This isn't a bug, but could the changelog say why the WFET path is
preferred over the event-stream path, and note how the deadline is
derived (and on which counter base), rather than describing which macros
call which?

> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
> index d186a4558776..d1f4f571adaa 100644
> --- a/arch/arm64/include/asm/barrier.h
> +++ b/arch/arm64/include/asm/barrier.h

[ ... ]

> diff --git a/arch/arm64/include/asm/cmpxchg.h b/arch/arm64/include/asm/cmpxchg.h
> index 6cf3cd6873f5..7985ae5ceb0f 100644
> --- a/arch/arm64/include/asm/cmpxchg.h
> +++ b/arch/arm64/include/asm/cmpxchg.h

[ ... ]

> @@ -235,20 +237,52 @@ __CMPWAIT_CASE( ,  , 64);
>
>  #undef __CMPWAIT_CASE
>
> +#define __CMPWAIT_TIMEOUT_CASE(w, sfx, sz)				\
> +static inline void __cmpwait_case_timeout_##sz(volatile void *ptr,	\
> +					       unsigned long val,	\
> +					       u64 timeout_ns)		\
> +{									\
> +	unsigned long tmp;						\
> +	u64 ecycles = __delay_cycles() +				\
> +			nsecs_to_cycles(timeout_ns);			\
> +	asm volatile(							\
> +	"	sevl\n"							\
> +	"	wfe\n"							\
> +	"	ldxr" #sfx "\t%" #w "[tmp], %[v]\n"			\
> +	"	eor	%" #w "[tmp], %" #w "[tmp], %" #w "[val]\n"	\
> +	"	cbnz	%" #w "[tmp], 2f\n"				\
> +	"	msr s0_3_c1_c0_0, %[ecycles]\n"				\
> +	"2:"								\
> +	: [tmp] "=&r" (tmp), [v] "+Q" (*(u##sz *)ptr)			\
> +	: [val] "r" (val), [ecycles] "r" (ecycles));			\
> +}

This isn't a bug, but since only the final wait instruction differs
from __CMPWAIT_CASE (the sevl/wfe/ldxr/eor/cbnz prologue and the
operand lists are identical), would folding the two into one macro that
takes the wait instruction (and the optional deadline operand) read
better, in the style of __XCHG_CASE()? And is the 1f -> 2f label
renumbering needed?

> +
> +__CMPWAIT_TIMEOUT_CASE(w, b, 8);
> +__CMPWAIT_TIMEOUT_CASE(w, h, 16);
> +__CMPWAIT_TIMEOUT_CASE(w,  , 32);
> +__CMPWAIT_TIMEOUT_CASE( ,  , 64);
> +
> +#undef __CMPWAIT_TIMEOUT_CASE

[ ... ]


---
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/33438155296