Re: [PATCH v14 01/15] asm-generic: barrier: Add smp_cond_load_relaxed_timeout()

[bot+bpf-ci]

> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index b99cb57dfccc..d1e9ed15bbfc 100644
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h
> @@ -273,6 +273,85 @@ do {									\
>  })
>  #endif
>
> +/*
> + * Number of times we iterate in the loop before doing the time check.
> + * Note that the iteration count assumes that the loop condition is
> + * relatively cheap.
> + */
> +#ifndef SMP_TIMEOUT_POLL_COUNT
> +#define SMP_TIMEOUT_POLL_COUNT		200
> +#endif
> +
> +/*
> + * cpu_poll_relax() stitches up two kinds of primitives: ones that provide
> + * a momentary blip in the pipeline (ex. cpu_relax() on x86).
> + * The second support waiting for @ptr value to change, coupled with a
> + * with a precise (or imprecise) timeout.

This isn't a bug, but could this sentence be tightened?  The phrase
"with a" is repeated across the line wrap, and "The second support"
reads a little off, e.g. "The second kind supports waiting for @ptr to
change, coupled with a (precise or imprecise) timeout".

> + *
> + * cpu_poll_relax() keeps them together, because its utility is in minimizing
> + * expensive operations while polling on @ptr waiting for it to change.
> + * The arguments to cpu_poll_relax() are only needed for the waiting
> + * primitives.
> + *
> + * Also note that platforms with a suitable cpu_poll_relax() are expected
> + * to define ARCH_HAS_CPU_RELAX.
> + */
> +#ifndef cpu_poll_relax
> +#define cpu_poll_relax(ptr, val, timeout_ns)	cpu_relax()
> +#endif
> +

[ ... ]

> +#ifndef smp_cond_load_relaxed_timeout
> +#define smp_cond_load_relaxed_timeout(ptr, cond_expr,			\
> +				      time_expr_ns, timeout_ns)		\
> +({									\
> +	typeof(ptr) __PTR = (ptr);					\
> +	__unqual_scalar_typeof(*(ptr)) VAL;				\
> +	u32 __count = 0, __spin = SMP_TIMEOUT_POLL_COUNT;		\
> +	s64 __timeout = (s64)timeout_ns;				\
> +	s64 __time_now, __time_end = 0;					\
> +									\
> +	for (;;) {							\
> +		VAL = READ_ONCE(*__PTR);				\
> +		if (cond_expr)						\
> +			break;						\
> +		cpu_poll_relax(__PTR, VAL, (u64)__timeout);		\
> +		if (++__count < __spin)					\
> +			continue;					\

The loop passes the full relative __timeout to cpu_poll_relax() on every
iteration and resets __count to 0 each cycle after the time-check.

For the asm-generic default this is harmless, since cpu_poll_relax()
expands to cpu_relax() and ignores its arguments.

But for an event-based wait that honours the timeout argument (for
example an arm64 WFE override), wouldn't each of the
SMP_TIMEOUT_POLL_COUNT iterations be allowed to block for up to the
entire remaining __timeout before the next time-check, delaying the
check and causing large overshoot?

This came up in review of an earlier version, where the suggestion was
to pass a bounded per-iteration timeout rather than the full remaining
value.  Would it be worth carrying that bound here so the arch overrides
land on a well-defined per-iteration wait?

> +		__time_now = (s64)(time_expr_ns);			\
> +		if (unlikely(__time_end == 0))				\
> +			__time_end = __time_now + __timeout;		\
> +		__timeout = __time_end - __time_now;			\
> +		if (__time_now <= 0 || __timeout <= 0) {		\
> +			VAL = READ_ONCE(*__PTR);			\
> +			break;						\
> +		}							\
> +		__count = 0;						\
> +	}								\
> +	(typeof(*(ptr)))VAL;						\
> +})
> +#endif


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