Re: [PATCH v16 06/16] asm-generic: barrier: Add smp_cond_load_acquire_timeout()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] barrier: smp_cond_load_acquire_timeout() lacks ACQUIRE ordering on timeout
- [Medium] barrier: __smp_cond_load_relaxed_timeout() blocks for 1us on first iteration
--

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

asm-generic: barrier: Add smp_cond_load_acquire_timeout()

This commit adds smp_cond_load_acquire_timeout(), which serves as the acquire
variant of smp_cond_load_relaxed_timeout(). It performs its own error checking
and reuses the relaxed logic for the wait, relying on a control dependency
followed by an acquire barrier to provide LOAD->STORE ordering.

> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index ac0a8b5e0ffe7..fc244dbd040b0 100644
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h
[ ... ]
> @@ -383,6 +383,53 @@ do {									\
[ ... ]
> +	VAL = __smp_cond_load_relaxed_timeout(ptr,			\
> +					      cond_expr,		\
> +					      time_expr_ns,		\
> +					      __scl_timeout_ns);	\

[Severity: Medium]
Will this macro block for 1us on the first iteration even if the caller
requests a very short or zero timeout?

Looking at __smp_cond_load_relaxed_timeout(), it initializes
__scl_timeout to NSEC_PER_USEC and unconditionally calls cpu_poll_relax()
before checking the actual timeout_ns:

    u64 __scl_timeout = NSEC_PER_USEC, __scl_time_end = 0;
    ...
    for (;;) {
        VAL = READ_ONCE(*__PTR);
        if (cond_expr || __scl_timed_out)
            break;
        cpu_poll_relax(__PTR, VAL, (u64)__scl_timeout);

If an architecture implements cpu_poll_relax() with a precise timeout, could
this penalize callers who requested a minimum wait with an unnecessary 1000ns
delay?

> +	/*								\
> +	 * We return having waited on the control dependency,		\
> +	 * (cond_expr || __scl_timed_out). Since we break out only	\
> +	 * after either of these conditions are true, the conditional	\
> +	 * separates the execution stream into two happened-before	\
> +	 * and happened-after sequences. And since STOREs are not	\
> +	 * speculated, this takes cares of the LOAD->STORE ordering.	\
> +	 *								\
> +	 * Provide the remaining LOAD->LOAD ordering.			\
> +	 */								\
> +	smp_acquire__after_ctrl_dep();					\

[Severity: High]
Does this code actually provide ACQUIRE ordering on the timeout path?

Inside __smp_cond_load_relaxed_timeout(), the loop exits via:

    if (cond_expr || __scl_timed_out)
        break;

When a timeout occurs, __scl_timed_out is set to true, and the loop continues
to the next iteration. On this final iteration, the compiler statically knows
that __scl_timed_out is true.

Could it optimize the branch into an unconditional jump to break, destroying
the control dependency?

Without a control dependency, smp_acquire__after_ctrl_dep() only provides
LOAD->LOAD ordering, and the LOAD->STORE ordering required by ACQUIRE
semantics is lost on weakly-ordered CPUs. Can this lead to data corruption?

> +__scl_out:								\
> +	(typeof(*(ptr)))VAL;						\
> +})
> +#endif

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