Re: [PATCH v12 10/15] bpf/rqspinlock: Use smp_cond_load_acquire_timeout()

[bot+bpf-ci]

> bpf/rqspinlock: Use smp_cond_load_acquire_timeout()
>
> Switch out the conditional load interfaces used by rqspinlock
> to smp_cond_read_acquire_timeout() and its wrapper,
> atomic_cond_read_acquire_timeout().
>
> Both these handle the timeout and amortize as needed, so use the
> non-amortized RES_CHECK_TIMEOUT.
>
> RES_CHECK_TIMEOUT does double duty here -- presenting the current
> clock value, the timeout/deadlock error from clock_deadlock() to
> the cond-load and, returning the error value via ret.
>
> For correctness, we need to ensure that the error case of the
> cond-load interface always agrees with that in clock_deadlock().
>
> For the most part, this is fine because there's no independent clock,
> or double reads from the clock in cond-load -- either of which could
> lead to its internal state going out of sync from that of
> clock_deadlock().
>
> There is, however, an edge case where clock_deadlock() checks for:
>
>         if (time > ts->timeout_end)
>                 return -ETIMEDOUT;
>
> while smp_cond_load_acquire_timeout() checks for:
>
>         __time_now = (time_expr_ns);
>         if (__time_now <= 0 || __time_now >= __time_end) {
>                 VAL = READ_ONCE(*__PTR);
>                 break;
>         }
>
> This runs into a problem when (__time_now == __time_end) since
> clock_deadlock() does not treat it as a timeout condition but
> the second clause in the conditional above does.
> So, add an equality check in clock_deadlock().
>
> Finally, redefine SMP_TIMEOUT_POLL_COUNT to be 16k to be similar to
> the spin-count used in the amortized version. We only do this for
> non-arm64 as that uses a waiting implementation.
>
> Cc: bpf@xxxxxxxxxxxxxxx
> Cc: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> Acked-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> Signed-off-by: Ankur Arora <ankur.a.arora@xxxxxxxxxx>

Besides switching to the new cond-load interface, this commit also
changes the boundary comparison in clock_deadlock() from:

        if (time > ts->timeout_end)
                return -ETIMEDOUT;

to:

        if (time >= ts->timeout_end)
                return -ETIMEDOUT;

so that time == timeout_end is now treated as a timeout, matching the
__time_now >= __time_end check in smp_cond_load_acquire_timeout().

The time > ts->timeout_end check, and the timeout_end field itself, were
first introduced in the then-named check_timeout() helper by:

  commit 14c48ee81452 ("rqspinlock: Add support for timeouts")

Should this commit carry a Fixes: tag for that boundary change?

  Fixes: 14c48ee81452 ("rqspinlock: Add support for timeouts")

This is offered tentatively: under the previous res_smp_cond_load_acquire
interface the '>' comparison was internally consistent, and the boundary
divergence only becomes observable once this commit switches to the
'__time_now >= __time_end' interface. So this may read more as a
self-contained adjustment than a fix for a pre-existing bug.


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