Re: [PATCH v15 15/16] barrier: timeout validity checks for smp_cond_load_relaxed_timeout()

[ ... ]

+static const struct smp_cond_expiry_params expiry_params_list[] = {
+	/* timeout_ns is invalid/out-of-range */
+	{ .clk_unit = 0, .timeout_ns = -1LL,	.miniters = -1, .maxiters = 0, .desc = "invalid (-1LL)", },
+	{ .clk_unit = 0, .timeout_ns = ~0ULL,	.miniters = -1, .maxiters = 0, .desc = "invalid (~0ULL)", },
+	{ .clk_unit = 0, .timeout_ns = S64_MAX+1ULL, .miniters = -1, .maxiters = 0, .desc = "out-of-range (S64_MAX+1)", },
+	{ .clk_unit = 0, .timeout_ns = U64_MAX,	.miniters = -1, .maxiters = 0, .desc = "out-of-range (U64_MAX)", },
+	{ .clk_unit = 0, .timeout_ns = 0,	.miniters = -1, .maxiters = 1, .desc = "degenerate (0)",    },
+
+	/* timeout_ns is valid */
+	{ .clk_unit = (0x1ULL << 28), .timeout_ns = 1,		    .miniters = 1,	      .maxiters = -1, .desc = "1",    },
+	{ .clk_unit = (0x1ULL << 28), .timeout_ns = (0x1ULL << 30), .miniters = 1 << (30-28), .maxiters = -1, .desc = "1<<30",   },
+	{ .clk_unit = (0x1ULL << 28), .timeout_ns = S32_MAX,	    .miniters = 1 << (31-28), .maxiters = -1, .desc = "S32_MAX", },
+	{ .clk_unit = (0x1ULL << 28), .timeout_ns = U32_MAX,	    .miniters = 1 << (32-28), .maxiters = -1, .desc = "U32_MAX", },
+	{ .clk_unit = (0x1ULL << 28), .timeout_ns = (0x1ULL << 33), .miniters = 1 << (33-28), .maxiters = -1, .desc = "1<<33",   },
+	{ .clk_unit = (0x1ULL << 58), .timeout_ns = S64_MAX,	    .miniters = 1 << (63-58), .maxiters = -1, .desc = "S64_MAX", },

                                                                                                                  ^^^^^^^^^^^^

+};

Can the last row actually test timeout expiry? The synthetic clock overflows
before the deadline can be reached.

synthetic_clock() advances by clk_unit per evaluation:

	clk->end_time += clk->extra;
	clk->niters++;
	return clk->end_time;

With clk_unit = 1<<58 and timeout_ns = S64_MAX, the deadline computed in
include/asm-generic/barrier.h becomes __scl_time_end = 2^58 + S64_MAX, which
needs the clock to reach 33 * 2^58 to expire. But the 32nd evaluation already
yields 32 * 2^58 == 2^63, which is S64_MIN as an s64, so the very next check
in __smp_cond_load_relaxed_timeout() breaks on the failure arm:

	if (__scl_time_now <= 0 || __scl_timeout <= 0) {
		VAL = READ_ONCE(*__PTR);
		break;
	}

So niters == 32 exactly and the break is 'time_expr_ns returned a negative
value' rather than 'timeout expired'. .miniters = 1 << (63-58) == 32 makes
KUNIT_EXPECT_GE(test, clk.niters, 32) pass by exactly zero margin, but this
doesn't distinguish an implementation that honours the timeout from one that
bails out early on clock failure.

+static void test_smp_cond_relaxed(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_unit,
+		.niters = 0,
+	};
+	s64 runtime;

         ^^^^^^^^^^^

+
+	flag = 0;
+	smp_cond_load_relaxed_timeout(&flag,
+				      0,
+				      synthetic_clock(&clk),
+				      p->timeout_ns);
+
+	runtime = (u64)clk.end_time - (u64)clk.start_time;

                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

runtime is declared s64 but assigned an unsigned expression. For the S64_MAX
row this stores 2^63, which is a negative s64. The following check only passes
because typeof(right) is u64, which converts runtime back to unsigned:

	if (p->maxiters != 0)
		KUNIT_EXPECT_GE(test, runtime, p->timeout_ns);

Would declaring runtime as u64 (matching the casts on both operands) be
clearer?

[ ... ]


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