Re: [PATCH v14 15/15] barrier: add clock tests for smp_cond_load_relaxed_timeout()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] smp_cond_load_relaxed_timeout() effectively doubles the intended timeout on architectures with a waiting cpu_poll_relax() (e.g., ARM64).
- [High] Passing U64_MAX or S64_MAX to smp_cond_load_relaxed_timeout() causes an immediate timeout instead of an infinite wait.
--

--- Patch [15]: [PATCH v14 15/15] barrier: add clock tests for smp_cond_load_relaxed_timeout() ---
commit 9919abc9f7cd30b282ef4aefe31726e940d7bd0d
Author: Ankur Arora <ankur.a.arora@oracle.com>

barrier: add clock tests for smp_cond_load_relaxed_timeout()

Add a few clock tests for smp_cond_load_relaxed_timeout(). These ensure that
the implementation doesn't do anything funny stuff with the clock (like
multiple accesses per iteration).

>     Also ensure that we handle edge cases sanely. Note that two edge cases
>     fail: S64_MAX and U64_MAX. However, both of those are quite far out
>     and if needed, can be addressed in the implementation of the interface.

[Severity: High]
This isn't a bug introduced by this patch, but passing U64_MAX or S64_MAX to
smp_cond_load_relaxed_timeout() causes an immediate timeout instead of an
infinite wait.

A caller passing ~0ULL (a common idiom for 'no timeout') will silently have it
cast to s64, turning it into -1. After the first spurious wake, the condition
if (__timeout <= 0) triggers and it returns immediately. For S64_MAX, the
addition __time_now + __timeout overflows into a negative number, resulting
in the same immediate failure:

include/asm-generic/barrier.h:smp_cond_load_relaxed_timeout() {
    ...
    s64 __timeout = (s64)timeout_ns;
    ...
    if (__time_now <= 0 || __timeout <= 0) {
        VAL = READ_ONCE(*__PTR);
        break;
    ...
}

Should we fix the interface to correctly handle standard infinite values rather
than leaving this API trap?

> diff --git a/lib/tests/barrier-timeout-test.c b/lib/tests/barrier-timeout-test.c
> index 4226fb6456b35..ec78f85330062 100644
> --- a/lib/tests/barrier-timeout-test.c
> +++ b/lib/tests/barrier-timeout-test.c

[ ... ]

> +static void test_smp_cond_expiry(struct kunit *test)
> +{
> +	const struct smp_cond_expiry_params *p = test->param_value;
> +	struct clock_state clk = {
> +		.start_time = 0,
> +		.end_time = 0,
> +		.extra = p->clk_inc,
> +		.niters = 0,
> +	};
> +	s64 runtime;
> +
> +	flag = 0;
> +	smp_cond_load_relaxed_timeout(&flag,
> +				      0,
> +				      synthetic_clock(&clk),
> +				      p->timeout);
> +
> +	runtime = (u64)clk.end_time - (u64)clk.start_time;
> +	KUNIT_EXPECT_EQ(test, clk.niters, p->niters);
> +	KUNIT_EXPECT_GE(test, runtime, p->timeout);

[Severity: High]
This is a pre-existing issue, but smp_cond_load_relaxed_timeout() effectively
doubles the intended timeout on architectures with a waiting cpu_poll_relax()
(e.g., ARM64).

The KUnit tests added here only verify the minimum runtime, which passes
despite the runtime being double. On ARM64, the late initialization of
__time_end using __time_now after cpu_poll_relax() erroneously adds the
physical sleep duration to the timeout deadline:

include/asm-generic/barrier.h:smp_cond_load_relaxed_timeout() {
    ...
    for (;;) {
        ...
        cpu_poll_relax(__PTR, VAL, (u64)__timeout);
        ...
        __time_now = (s64)(time_expr_ns);
        if (unlikely(__time_end == 0))
            __time_end = __time_now + __timeout;
    ...
}

Because __time_now is already Start + timeout_ns when WFET wakes, the new
deadline becomes Start + 2 * timeout_ns, causing the loop to sleep a second
time.

Could the tests be updated to also enforce an upper bound on the runtime to
catch these latency issues, or could we fix the macro itself to initialize
__time_end before polling?

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