Re: [PATCH v12 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] smp_cond_load_relaxed_timeout() passes an unvalidated, potentially negative timeout to cpu_poll_relax() before checking for expiration, causing unintended excessive sleeps on architectures like arm64.
--

commit 188f153f9d611077a8cfb23fad92d4d764992236
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.

Also, this tests only smp_cond_load_relaxed_timeout(). The acquire
variant uses an identical clock path and testing wouldn't add anything.

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

[ ... ]

> @@ -112,8 +114,63 @@ static void test_smp_cond_timeout(struct kunit *test)

[ ... ]

> +static const struct smp_cond_expiry_params expiry_params_list[] = {
> +	{ .clk_inc = (0x1ULL << 28), .timeout = -1LL,		.niters = 1,			.desc = "-1LL",    },

[Severity: High]
This is a pre-existing issue, but does adding this test for a -1LL timeout
expose an unintended massive sleep on architectures like arm64?

Looking at include/asm-generic/barrier.h, smp_cond_load_relaxed_timeout() 
passes the unvalidated timeout to cpu_poll_relax() on the first iteration 
before evaluating expiration:

#define smp_cond_load_relaxed_timeout(ptr, cond_expr, \
                                      time_expr_ns, timeout_ns) \
({ \
	...
	s64 __timeout = (s64)timeout_ns; \
	...
	for (;;) { \
		VAL = READ_ONCE(*__PTR); \
		if (cond_expr) \
			break; \
		cpu_poll_relax(__PTR, VAL, (u64)__timeout); \
		...

Since the -1LL timeout is cast to u64, it becomes U64_MAX. On architectures
like arm64, cpu_poll_relax() maps to __cmpwait_relaxed_timeout() where
this huge value is converted into cycles for a WFET instruction.

Could this stall the core completely until the next asynchronous event wakes
it up, breaking lockless wait latency guarantees?

The KUnit test appears to pass silently here because it uses synthetic_clock()
which measures iterations rather than actual wall-clock time, ignoring
the hardware stalling effect.

> +	{ .clk_inc = (0x1ULL << 28), .timeout = (0x1ULL << 30), .niters = 1 + (1 << (30-28)),	.desc = "1<<30",   },
> +	{ .clk_inc = (0x1ULL << 28), .timeout = S32_MAX,	.niters = 1 + (1 << (31-28)),	.desc = "S32_MAX", },
> +	{ .clk_inc = (0x1ULL << 28), .timeout = U32_MAX,	.niters = 1 + (1 << (32-28)),	.desc = "U32_MAX", },
> +	{ .clk_inc = (0x1ULL << 28), .timeout = (0x1ULL << 33), .niters = 1 + (1 << (33-28)),	.desc = "1<<33",   },
> +};

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