Re: [PATCH v15 10/16] bpf/rqspinlock: Use smp_cond_load_acquire_timeout()

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 4835 bytes --]

> diff --git a/kernel/bpf/rqspinlock.c b/kernel/bpf/rqspinlock.c
> index 1b249c6f0674..9e8f19afd7b0 100644
> --- a/kernel/bpf/rqspinlock.c
> +++ b/kernel/bpf/rqspinlock.c

[ ... ]

> @@ -269,6 +267,18 @@ static noinline s64 clock_deadlock(rqspinlock_t *lock, u32 mask,
>   */
>  #define RES_RESET_TIMEOUT(ts, _duration) ({ (ts).timeout_end = 0; (ts).duration = _duration; })
>
> +/*
> + * Limit how often we invoke clock_deadlock() while spin-waiting in
> + * smp_cond_load_acquire_timeout() or atomic_cond_read_acquire_timeout().
> + *
> + * We only override the default value for architectures without a waiting
> + * implementation.
> + */
> +#ifndef CPU_POLL_RELAX_WAITS
> +#undef SMP_TIMEOUT_POLL_COUNT
> +#define SMP_TIMEOUT_POLL_COUNT	(16*1024)
> +#endif
> +

Does this CPU_POLL_RELAX_WAITS check correctly detect the waiting
implementation at runtime for arm64?

On arm64, CPU_POLL_RELAX_WAITS is defined unconditionally at
arch/arm64/include/asm/barrier.h:233, but cpu_poll_relax() picks its
implementation at runtime:

  #define cpu_poll_relax(ptr, val, timeout_ns) do {                  \
          if (alternative_has_cap_unlikely(ARM64_HAS_WFXT))          \
                  __cmpwait_relaxed_timeout(ptr, val, timeout_ns);   \
          else if (arch_timer_evtstrm_available())                   \
                  __cmpwait_relaxed(ptr, val);                       \
          else                                                       \
                  cpu_relax();                                       \
  } while (0)

So on every arm64 build SMP_TIMEOUT_POLL_COUNT stays at 1 ('Wait mode. No
need to poll.' per include/asm-generic/barrier.h:280-281) and the 16k
override is skipped. On arm64 hardware that has neither FEAT_WFXT nor an
available arch-timer event stream, cpu_poll_relax() degrades to a bare
cpu_relax(), and __smp_cond_load_relaxed_timeout() then evaluates
time_expr_ns on every loop iteration:

    cpu_poll_relax(__PTR, VAL, (u64)__scl_timeout);      /* == cpu_relax() */
    if (++__scl_count < __scl_spin)                      /* __scl_spin == 1 */
            continue;
    __scl_time_now = (s64)(time_expr_ns);                /* clock_deadlock() */

That means one noinline clock_deadlock() call with ktime_get_mono_fast_ns()
per cpu_relax() for the whole 250 ms / 500 ms wait, in both rqspinlock wait
loops (rqspinlock.c:427 and :590). Before this patch the same platform
amortized the clock read 1-in-65536 (the u16 'spin' wrap in the amortized
RES_CHECK_TIMEOUT), and the arm64 private copy removed earlier in the
series (f90a6cd43b71) amortized it 1-in-200 with a dedicated
non-event-stream path, whose comment called this case out explicitly: 'it
will not happen when event stream support is unavailable, time_expr check
is amortized.'

The commit message asserts 'We only do this for architectures lacking a
waiting implementation of cpu_poll_relax()', which is true at compile time
but not at run time for arm64 without WFXT/evtstrm. Gating on something
that reflects the runtime choice (or clamping __scl_spin when the waiting
path is not taken) would restore the amortization.

[ ... ]

>      Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>      Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>
>      Note:
>        - when overriding the SMP_TIMEOUT_POLL_COUNT value check for
>          CPU_POLL_RELAX_WAITS instead of directly checking for CONFIG_ARM64.

The commit message states:

>      Finally, redefine SMP_TIMEOUT_POLL_COUNT to be 16k to be similar to
>      the spin-count used in the amortized version.

Is the 16k value correct? The amortized version's spin count is 64k, not
16k. RES_CHECK_TIMEOUT_AMORTIZED gates on 'if (!(ts).spin++)'
(kernel/bpf/rqspinlock.c:244) and 'spin' is 'u16' (struct
rqspinlock_timeout, rqspinlock.c:80-85), so clock_deadlock() ran once per
65536 evaluations. The new SMP_TIMEOUT_POLL_COUNT of 16*1024 therefore
samples the clock 4x more often than the code it is said to match.

The direction is harmless, but the stated justification for the constant is
off by 4x, and it is the only rationale a reviewer has for the value.

Also, the macro excerpt quoted in the changelog does not match the tree:

        __time_now = (time_expr_ns);
        if (__time_now <= 0 || __time_now >= __time_end) {

The actual code (include/asm-generic/barrier.h:351-357) uses
__scl_-prefixed names and tests '__scl_timeout <= 0' after recomputing
'__scl_timeout = __scl_time_end - __scl_time_now'. It is equivalent, but
quoting code that is not in the tree makes the equality argument harder to
check.


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