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

[bot+bpf-ci]

> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 1244dcac2294..cb46e9fcc87c 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2551,6 +2551,16 @@ config FFS_KUNIT_TEST
>  	  For more information on KUnit and unit tests in general,
>  	  please refer to Documentation/dev-tools/kunit/.
>
> +config BARRIER_TIMEOUT_KUNIT_TEST
> +	tristate "KUnit tests for smp_cond_load_*_timeout()" if !KUNIT_ALL_TESTS
> +	depends on KUNIT
> +	default KUNIT_ALL_TESTS
> +	help
> +	  Builds KUnit tests that validate wake-up and timeout handling paths in
> +	  smp_cond_load_relaxed_timeout() and smp_cond_load_acquire_timeout().
> +
> +	  If unsure, say N.
> +
>  config TEST_KSTRTOX
>  	tristate "Test kstrto*() family of functions at runtime"

[ ... ]

> diff --git a/lib/tests/barrier-timeout-test.c b/lib/tests/barrier-timeout-test.c
> new file mode 100644
> index 000000000000..60f121fe5472
> --- /dev/null
> +++ b/lib/tests/barrier-timeout-test.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit tests exercising smp_cond_load_relaxed_timeout().
> + *
> + * Copyright (c) 2026, Oracle Corp.
> + * Author: Ankur Arora <ankur.a.arora@xxxxxxxxxx>
> + */

[ ... ]

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