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

[Ankur Arora <ankur.a.arora@oracle.com>]

sashiko-bot@kernel.org writes:

> 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.

Yes this is a bit of an open question.

>> +			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.

smp_cond_load_relaxed_timeout() is not called from the acquire path.

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


--
ankur