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

[bot+bpf-ci]

> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index 8c37de192944..8b166acfba12 100644
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h
> @@ -379,6 +379,54 @@ do {									\
>  })
>  #endif
>
> +/**
> + * smp_cond_load_acquire_timeout() - (Spin) wait for cond with ACQUIRE ordering
> + * until a timeout expires.
> + * @ptr: pointer to the variable to wait on.
> + * @cond_expr: boolean expression to wait for.
> + * @time_expr_ns: monotonic expression that evaluates to time in ns or,
> + *  on failure, returns a negative value.

The documentation says @time_expr_ns "on failure, returns a negative value",
but the implementation in __smp_cond_load_relaxed_timeout() exits when
__scl_time_now <= 0, treating zero as a failure too. Should the
documentation say "zero or a negative value" to match what
smp_cond_load_relaxed_timeout() documents (barrier.h:317-318)?

> + * @timeout_ns: timeout value in ns
> + * (Both of the above are assumed to be compatible with s64.)
> + *
> + * Equivalent to using smp_cond_load_acquire() on the condition variable with
> + * a timeout.
> + */
> +#ifndef smp_cond_load_acquire_timeout
> +#define smp_cond_load_acquire_timeout(ptr, cond_expr,			\
> +				      time_expr_ns, timeout_ns)		\
> +({									\
> +	__unqual_scalar_typeof(*(ptr)) VAL;				\
> +	VAL = smp_cond_load_relaxed_timeout(ptr, cond_expr,		\
> +					     time_expr_ns,		\
> +					     timeout_ns);		\
> +	/*								\
> +	 * We arrive here once the loop condition is hit, on timeout,	\
> +	 * or, if we hit both the timeout and the loop condition.	\
> +	 *								\
> +	 * For the first case, we come here having already evaluated	\
> +	 * the control dependency.					\
> +	 * In the last case -- low probability, possible in the last	\
> +	 * iteration, especially on architectures with waiting		\
> +	 * cpu_poll_relax() -- the control dependency has not been	\
> +	 * evaluated.							\
> +	 *								\
> +	 * So, force it to be re-evaluated before			\
> +	 * smp_acquire__after_ctrl_dep() to provide ACQUIRE ordering	\
> +	 * for both.							\
> +	 *								\
> +	 * The other case is of pure timeout, where again we don't have \
> +	 * the advantage of having the control dependency. Given that	\
> +	 * this is the slowpath, we go with a full smp_load_acquire().	\
> +	 */								\
> +	if (cond_expr)							\
> +		smp_acquire__after_ctrl_dep();				\
> +	else								\
> +		VAL = smp_load_acquire(ptr);				\

The macro evaluates cond_expr one extra time here, after the
smp_cond_load_relaxed_timeout() call has already returned. This means
cond_expr is silently required to be side-effect free and a pure function
of VAL.

The kernel-doc says the macro is "Equivalent to using
smp_cond_load_acquire() on the condition variable with a timeout"
(barrier.h:392-393), but in smp_cond_load_acquire() cond_expr is only
evaluated immediately after a fresh READ_ONCE inside the loop. If cond_expr
also depends on state other than VAL, or has side effects, the two
evaluations can disagree:

  VAL = smp_cond_load_relaxed_timeout(ptr, cond_expr, ...);  /* succeeded, cond was true */
  if (cond_expr)                                             /* now false */
          smp_acquire__after_ctrl_dep();
  else
          VAL = smp_load_acquire(ptr);                       /* successful result discarded */

When that happens the successful wait's value is thrown away and replaced
by a fresh read, and any side effects inside cond_expr run one extra time.

This pattern is not hypothetical for the smp_cond_load_acquire() family.
In-tree conditions that are not pure functions of VAL include:

  arch/arm64/mm/mmu.c:
    smp_cond_load_acquire(&idmap_kpti_bbml3_flag, VAL == num_online_cpus());

  include/asm-generic/ticket_spinlock.h:
    atomic_cond_read_acquire(&lock->val, ticket == (u16)VAL);

  kernel/bpf/rqspinlock.c:
    res_smp_cond_load_acquire(&lock->locked, !VAL || RES_CHECK_TIMEOUT(ts, ret, _Q_LOCKED_MASK));

No current caller in the series is broken by this - every new user passes
a condition that is a pure function of VAL. But should the requirement be
documented, or should the equivalence claim in the kernel-doc be weakened?

> +	(typeof(*(ptr)))VAL;						\
> +})
> +#endif
> +
>  /*
>   * pmem_wmb() ensures that all stores for which the modification
>   * are written to persistent storage by preceding instructions have


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