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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] locking: smp_cond_load_relaxed_timeout breaks control dependency for ACQUIRE semantics
--

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

asm-generic: barrier: Add smp_cond_load_relaxed_timeout()

This commit adds smp_cond_load_relaxed_timeout(), extending
smp_cond_load_relaxed() to allow waiting for a duration with a periodic
time-check. It uses cpu_poll_relax() in a loop and defers the potentially
costly time evaluation to a slowpath.

> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index b99cb57dfcccc..ac0a8b5e0ffe7 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;	\
> +	u64 __scl_timeout = NSEC_PER_USEC, __scl_time_end = 0;		\
> +	s64 __scl_time_now;						\
> +	bool __scl_timed_out = false;					\
> +									\
> +	for (;;) {							\
> +		VAL = READ_ONCE(*__PTR);				\
> +		if (cond_expr || __scl_timed_out)			\

[Severity: High]
In include/asm-generic/barrier.h, __smp_cond_load_relaxed_timeout() evaluates
cond_expr alongside __scl_timed_out. Does this break the control dependency
required for ACQUIRE semantics?

If __scl_timed_out is true from the previous iteration, this branch is taken
independently of cond_expr. Because the branch outcome is independent of the
loaded value VAL, the compiler can optimize the exit into an unconditional
break, meaning the CPU has no hardware-level control dependency to enforce.

Since smp_cond_load_acquire_timeout() (added in a subsequent patch) relies on
smp_cond_load_relaxed_timeout() to provide a control dependency on the loaded
value which is then upgraded to an ACQUIRE barrier by
smp_acquire__after_ctrl_dep(), this could allow subsequent critical section
memory accesses to be reordered before the lock acquisition load.

If the returned VAL concurrently satisfies the caller's acquisition check
at the exact moment of timeout, the caller will enter the critical section
without hardware-enforced LOAD-STORE ordering.

> +			break;						\
> +		cpu_poll_relax(__PTR, VAL, (u64)__scl_timeout);		\
> +		if (++__scl_count < __scl_spin)				\
> +			continue;					\
[ ... ]
> +		if (__scl_time_now <= 0 ||				\
> +		    __scl_time_end <= __scl_time_now) {			\
> +			__scl_timed_out = true;				\
> +			continue;					\
> +		}							\
[ ... ]
> +	}								\
> +	(typeof(*(ptr)))VAL;						\
> +})
> +
> +#define smp_cond_load_relaxed_timeout(ptr, cond_expr,			\
> +				      time_expr_ns, timeout_ns)		\
> +({									\
> +	s64 __scl_timeout_ns = (s64)(timeout_ns);			\
> +	__unqual_scalar_typeof(*(ptr)) VAL;				\
> +									\
> +	if (unlikely(__scl_timeout_ns < 0))				\
> +		VAL = READ_ONCE(*(ptr));				\

[Severity: High]
In include/asm-generic/barrier.h, smp_cond_load_relaxed_timeout() handles
negative timeouts. Does this negative timeout path also bypass the control
dependency?

Because this path executes VAL = READ_ONCE(*(ptr)); and returns without
executing a conditional branch on VAL, no control dependency is established
for the acquire barrier to upgrade.

> +	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/20261016070837.440748-1-ankur.a.arora@oracle.com?part=1