Re: [PATCH v15 14/16] barrier: add tests for smp_cond_load_*_timeout()

[ ... ]

[ ... ]

+static void test_smp_cond_timeout(struct kunit *test)
+{
+	const struct smp_cond_update_params *p = test->param_value;
+	struct clock_state clk = {
+		.start_time = local_clock(),
+		.end_time = local_clock(),
+	};
+	s64 runtime, timeout_ns = TIMEOUT_MSEC * NSEC_PER_MSEC;
+	unsigned int result;
+
+	flag = 0;
+	if (p->acquire) {
+		result = smp_cond_load_acquire_timeout(&flag,
+						       (VAL & TEST_FLAG_VAL),
+						       p->clock(&clk),
+						       timeout_ns);
+	} else {
+		result = smp_cond_load_relaxed_timeout(&flag,
+						       (VAL & TEST_FLAG_VAL),
+						       p->clock(&clk),
+						       timeout_ns);
+	}
+
+	runtime = clk.end_time - clk.start_time;
+	KUNIT_EXPECT_EQ(test, (bool)(result & TEST_FLAG_VAL), p->succeeds);
+	if (!p->succeeds)
+		KUNIT_EXPECT_GE(test, runtime, timeout_ns);

Could the elapsed-time assertion produce spurious failures when the test
migrates between CPUs?

The test runs with preemption and migration enabled, and can spend 2 ms
inside smp_cond_load_*_timeout() spinning on cpu_relax(). There is no
preempt_disable(), migrate_disable(), or CPU affinity set anywhere in
the file.

Samples can therefore come from different CPUs:

    .start_time = local_clock(),           /* test_smp_cond_timeout() */
    ...
    clk->end_time = local_clock();         /* basic_clock() */
    runtime = clk.end_time - clk.start_time;
    KUNIT_EXPECT_GE(test, runtime, timeout_ns);

local_clock() is only monotonic per CPU. With
CONFIG_HAVE_UNSTABLE_SCHED_CLOCK and !sched_clock_stable(),
local_clock_noinstr() returns sched_clock_local(this_scd()), whose
per-CPU value is only clamped into [gtod, gtod + TICK_NSEC].

include/linux/sched/clock.h states this explicitly:

    # when comparing cpu_clock(i) to cpu_clock(j) for i != j, time can
    # go backwards !!

The permitted inter-CPU skew is up to one tick, which is 4 ms at HZ=250,
twice the 2 ms timeout under test.

Concrete failing interleaving (D = skew, CPU A ahead of CPU B):

  1. test_smp_cond_timeout() on CPU A samples start_time = a0.
  2. Task migrates to CPU B. The macro's first time_expr_ns evaluation
     samples t1 = a0 - D and sets __scl_time_end = a0 - D + timeout_ns.
  3. Task migrates back to CPU A. A later sample satisfies
     __scl_timeout <= 0 as soon as a0 + elapsed >= a0 - D + timeout_ns,
     i.e. elapsed >= timeout_ns - D, and that sample is stored in
     clk->end_time.
  4. runtime = timeout_ns - D < timeout_ns, so KUNIT_EXPECT_GE fails
     even though the implementation behaved correctly.

This also conflicts with the documented contract in
include/asm-generic/barrier.h which requires "expression that evaluates
to monotonic time (in ns)".

Would pinning the test (migrate_disable()/preempt_disable() around the
spin, or sampling with a globally monotonic source) make the 2 ms
assertion sound?

+}

[ ... ]

+kunit_test_suite(barrier_timeout_test_suite);
+
+MODULE_DESCRIPTION("KUnit tests for smp_cond_load_relaxed_timeout()");
+MODULE_LICENSE("GPL");

Does the MODULE_DESCRIPTION accurately describe the module's coverage?

The file header comment and MODULE_DESCRIPTION both claim the module
only covers the relaxed variant:

    /*
     * KUnit tests exercising smp_cond_load_relaxed_timeout().

    MODULE_DESCRIPTION("KUnit tests for smp_cond_load_relaxed_timeout()");

But half of the parameter table selects the acquire variant, and
test_smp_cond_timeout() calls it:

    update_params_list[] = {
        { .clock = &mocked_clock, .succeeds = true, .acquire = true,  },
        { .clock = &basic_clock, .succeeds = false,  .acquire = true, },
    };

    if (p->acquire) {
            result = smp_cond_load_acquire_timeout(&flag, ... );
    }

Everything else in the patch names both: the commit subject is
"barrier: add tests for smp_cond_load_*_timeout()", the suite name is
"smp-cond-load-*-timeout", and the new Kconfig help text correctly says
"smp_cond_load_relaxed_timeout() and smp_cond_load_acquire_timeout()".

MODULE_DESCRIPTION is user-visible via modinfo, so should both the
header comment and MODULE_DESCRIPTION name smp_cond_load_*_timeout()?


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