Re: [PATCH v15 01/16] asm-generic: barrier: Add smp_cond_load_relaxed_timeout()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unconditional 1 microsecond hardware stall for very short or zero timeouts.
- [High] Redundant clock evaluation delays loop exit upon successful hardware wakeup.
- [Low] Implicit dependency on `NSEC_PER_USEC` without including time headers.
--

commit b4e52f4320fa73ae0869ca7d777ad60945812b82
Author: Ankur Arora <ankur.a.arora@oracle.com>

asm-generic: barrier: Add smp_cond_load_relaxed_timeout()

This commit introduces smp_cond_load_relaxed_timeout() which extends
smp_cond_load_relaxed() to allow waiting for a duration. It defers the costly
time evaluation to the slow path to minimize the fast path penalty, using
cpu_poll_relax() to yield between condition checks.

> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index b99cb57dfcccc..8c37de1929443 100644
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h
[ ... ]
> +#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 __scl_count = 0, __scl_spin = SMP_TIMEOUT_POLL_COUNT;	\
> +	s64 __scl_timeout = NSEC_PER_USEC;				\

[Severity: Low]
Does this introduce an implicit dependency on NSEC_PER_USEC without including
the required time headers?

Callers using this macro in files that do not include <linux/time64.h> or
<vdso/time64.h> might face build failures.

> +	s64 __scl_time_now, __scl_time_end = 0;				\
> +									\
> +	for (;;) {							\
> +		VAL = READ_ONCE(*__PTR);				\
> +		if (cond_expr)						\
> +			break;						\
> +		cpu_poll_relax(__PTR, VAL, (u64)__scl_timeout);		\

[Severity: High]
Does this unconditionally initiate a 1us hardware stall on the first iteration,
even if the user requested a zero or very short timeout?

When timeout_ns is less than 1000ns, __scl_timeout is still initially set to
NSEC_PER_USEC. On architectures with precise waits (like ARM64 wfet), this
could force the CPU to sleep for 1 microsecond, degrading fast-path polling
performance.

> +		if (++__scl_count < __scl_spin)				\
> +			continue;					\
> +		__scl_time_now = (s64)(time_expr_ns);			\

[Severity: High]
Is it necessary to unconditionally evaluate the clock after waking up?

When cpu_poll_relax() wakes up because the lock was released, evaluating
time_expr_ns (which could be an expensive clock read) adds latency to the lock
handoff before checking the condition again.

Would it be better to re-evaluate the condition immediately after waking up,
bypassing the time check if the condition is met?

[ ... ]
> +#define smp_cond_load_relaxed_timeout(ptr, cond_expr,			\
> +				      time_expr_ns, timeout_ns)		\
> +({									\
> +	__unqual_scalar_typeof(*(ptr)) VAL;				\
> +	s64 __scl_timeout_ns = (s64)(timeout_ns);			\
> +									\
> +	if (__scl_timeout_ns < 0)					\

[Severity: High]
Should this condition also include the zero timeout case?

By using __scl_timeout_ns < 0, a timeout of exactly 0 is passed to
__smp_cond_load_relaxed_timeout(), falling through to the slow path and
triggering the 1 microsecond delay discussed above.

> +		VAL = READ_ONCE(*(ptr));				\
> +	else								\
> +		VAL = __smp_cond_load_relaxed_timeout(ptr, cond_expr,	\
> +						      time_expr_ns,	\
> +						      __scl_timeout_ns);\
> +	(typeof(*(ptr)))VAL;						\
> +})
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831202251.305046-1-ankur.a.arora@oracle.com?part=1