Re: [RFC PATCH v23 02/10] lib: test hierarchical per-cpu counters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] lib/tests: unlocked cpu_online_mask read passed to work_on_cpu()
- [Medium] lib/tests: missing release/acquire ordering on sh.started in contention test
- [Low] lib/tests: unlocked num_online_cpus() check in do_hpcc_test_saturated_compare()
--

commit 48b3ce088047ee1ccff5c20155903c1a143e78df
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

lib: test hierarchical per-cpu counters

Introduce KUnit tests for hierarchical per-CPU counters covering
single-threaded updates, concurrent multi-threaded updates, contended
carries, and saturated counter comparisons.

Link: https://lkml.kernel.org/r/20260227153730.1556542-3-mathieu.desnoyers@efficios.com

> diff --git a/lib/tests/percpu_counter_tree_kunit.c b/lib/tests/percpu_counter_tree_kunit.c
> new file mode 100644
> index 0000000000000..58b2e488e3c6b
> --- /dev/null
> +++ b/lib/tests/percpu_counter_tree_kunit.c

[ ... ]

> +static void do_hpcc_test_single_thread(struct kunit *test, int _cpu0, int _cpu1,
> +				       enum up_test_inc_type type)
> +{
> +	unsigned long batch_size_order = 5;
> +	int cpu0 = _cpu0;
> +	int cpu1 = _cpu1;
> +	unsigned long deadline = jiffies + SINGLE_THREAD_MAX_SECONDS * HZ;
> +	int ret, i;
> +
> +	ret = init_counters(1UL << batch_size_order);
> +	KUNIT_EXPECT_EQ(test, ret, 0);
> +	if (ret)
> +		return;
> +	cpus_read_lock();
> +	for (i = 0; i < 10000 && time_before(jiffies, deadline); i++) {
> +		struct single_thread_update u0 = { .counter_index = 0 };
> +		struct single_thread_update u1 = { .counter_index = 1 };
> +		long increment;
> +
> +		switch (type) {
> +		case INC_ONE:
> +			increment = 1;
> +			break;
> +		case INC_MINUS_ONE:
> +			increment = -1;
> +			break;
> +		case INC_RANDOM:
> +			increment = (long)get_random_long() % 50000;
> +			break;
> +		default:
> +			WARN_ON_ONCE(1);
> +			increment = 0;
> +		}
> +		if (_cpu0 < 0)
> +			cpu0 = cpumask_any_distribute(cpu_online_mask);
> +		if (_cpu1 < 0)
> +			cpu1 = cpumask_any_distribute(cpu_online_mask);
> +		u0.increment = increment;
> +		u1.increment = increment;
> +		work_on_cpu(cpu0, single_thread_update_fn, &u0);
> +		work_on_cpu(cpu1, single_thread_update_fn, &u1);
> +		check_counters(test);
> +	}
> +	cpus_read_unlock();
> +	fini_counters();
> +}
> +
> +static void hpcc_test_single_thread_first(struct kunit *test)
> +{
> +	int cpu = cpumask_first(cpu_online_mask);
> +
> +	do_hpcc_test_single_thread(test, cpu, cpu, INC_ONE);
> +	do_hpcc_test_single_thread(test, cpu, cpu, INC_MINUS_ONE);
> +	do_hpcc_test_single_thread(test, cpu, cpu, INC_RANDOM);
> +}
> +
> +static void hpcc_test_single_thread_first_random(struct kunit *test)
> +{
> +	int cpu = cpumask_first(cpu_online_mask);
> +
> +	do_hpcc_test_single_thread(test, cpu, -1, INC_ONE);
> +	do_hpcc_test_single_thread(test, cpu, -1, INC_MINUS_ONE);
> +	do_hpcc_test_single_thread(test, cpu, -1, INC_RANDOM);
> +}

[Severity: Medium]
Can cpu go offline before or between the calls to
do_hpcc_test_single_thread()?

Both hpcc_test_single_thread_first() and
hpcc_test_single_thread_first_random() sample cpumask_first(cpu_online_mask)
without holding cpus_read_lock(), and then pass that cpu across three
separate calls to do_hpcc_test_single_thread(), each of which acquires and
releases cpus_read_lock() on its own.

Because _cpu0 >= 0 (and _cpu1 >= 0 in hpcc_test_single_thread_first()),
do_hpcc_test_single_thread() never re-reads or validates cpu0 or cpu1 under
cpus_read_lock() before passing them to work_on_cpu(), which requires the
caller to ensure the target CPU cannot go offline.

If CPU hot-unplug runs concurrently under CONFIG_HOTPLUG_CPU=y, could
work_on_cpu() be called on an offline CPU?

[ ... ]

> +static int contention_worker_fn(void *data)
> +{
> +	struct contention_data *cd = data;
> +	struct contention_shared *sh = cd->sh;
> +	int round;
> +
> +	wait_event(contention_wq, READ_ONCE(sh->started));
> +	for (round = 1; round <= CONTENTION_NR_ROUNDS; round++) {
> +		if (cd->coordinator) {
> +			atomic_set_release(&sh->round, round);
> +			contention_add(cd);
> +			while (atomic_read_acquire(&sh->nr_done) < round * (sh->nr_workers - 1))
> +				cond_resched();
> +#ifdef CONFIG_SMP
> +			if (percpu_counter_tree_check_residuals(&counter[0]))
> +				sh->nr_errors++;
> +#endif
> +		} else {
> +			while (atomic_read_acquire(&sh->round) < round)
> +				cond_resched();
> +			contention_add(cd);
> +			/* Order the additions before the coordinator check. */
> +			atomic_inc_return_release(&sh->nr_done);
> +		}
> +	}
> +	complete_work();
> +	return 0;
> +}
> +
> +static void do_hpcc_test_contended_carries(struct kunit *test, unsigned long batch_size)
> +{

[ ... ]

> +	for_each_cpu(cpu, cpus) {
> +		cd[cpu].sh = &sh;
> +		cd[cpu].coordinator = first;
> +		cd[cpu].seed = get_random_u32() | 1;
> +		task[cpu] = kthread_create_on_cpu(contention_worker_fn, &cd[cpu], cpu,
> +						  "kunit_hpcc_contention/%u");
> +		KUNIT_EXPECT_FALSE(test, IS_ERR(task[cpu]));
> +		if (IS_ERR(task[cpu]))
> +			break;
> +		atomic_inc(&kernel_threads_to_run);
> +		get_task_struct(task[cpu]);
> +		wake_up_process(task[cpu]);
> +		sh.nr_workers++;
> +		first = false;
> +	}
> +	/* Start the workers once their number is known. */
> +	WRITE_ONCE(sh.started, true);
> +	wake_up_all(&contention_wq);

[Severity: Medium]
On weakly-ordered architectures, can the coordinator thread in
contention_worker_fn() observe sh->started == true before seeing all
increments to sh.nr_workers?

In do_hpcc_test_contended_carries(), each worker kthread (starting with the
coordinator on the first iteration) is woken via wake_up_process(task[cpu])
inside the loop while sh.nr_workers++ is still being incremented on
subsequent iterations, and completion is published with
WRITE_ONCE(sh.started, true).

In contention_worker_fn(), if the already-running coordinator hits the
wait_event(contention_wq, READ_ONCE(sh->started)) fastpath before
wake_up_all() is called, it does not go through the waitqueue spinlock and
there is no release/acquire pairing ordering the updates to sh.nr_workers
before the coordinator reads sh->nr_workers in:

	while (atomic_read_acquire(&sh->nr_done) < round * (sh->nr_workers - 1))

Could this cause the coordinator to wait for fewer workers than were created
and call percpu_counter_tree_check_residuals(&counter[0]) while other workers
are still updating counter[0]? Should sh.started use smp_store_release() and
smp_load_acquire()?

[ ... ]

> +static void do_hpcc_test_saturated_compare(struct kunit *test, unsigned long batch_size,
> +					   long inc)
> +{

[ ... ]

> +	cpus_read_lock();
> +	for_each_online_cpu(cpu) {
> +		work_on_cpu(cpu, saturate_worker_fn, &sd);
> +		KUNIT_EXPECT_FALSE(test, sd.overrun);
> +		if (sd.overrun)
> +			break;
> +	}
> +	cpus_read_unlock();
> +
> +	precise = percpu_counter_tree_precise_sum(c);
> +	approx = percpu_counter_tree_approximate_sum(c);
> +	kunit_info(test, "batch_size=%lu inc=%ld precise=%ld approx=%ld accuracy=%lu\n",
> +		   batch_size, inc, precise, approx, accuracy);
> +
> +	check_residuals(test, c);
> +	check_residuals(test, &probe);
> +
> +	/* Accuracy contract. */
> +	KUNIT_EXPECT_GE(test, approx, precise - (long)accuracy);
> +	KUNIT_EXPECT_LE(test, approx, precise + (long)accuracy);
> +	/* Saturating every possible CPU reaches the accuracy exactly. */
> +	if (num_online_cpus() == nr_cpu_ids)
> +		KUNIT_EXPECT_EQ(test, (precise - approx) * inc, (long)accuracy);

[Severity: Low]
In do_hpcc_test_saturated_compare(), what happens if a CPU is offline during
the for_each_online_cpu() saturation loop and comes online after
cpus_read_unlock()?

Because num_online_cpus() == nr_cpu_ids is checked after cpus_read_unlock()
is dropped, a CPU brought online in that window would make
num_online_cpus() == nr_cpu_ids true even though saturate_worker_fn() did not
run on it, causing the KUNIT_EXPECT_EQ(test, (precise - approx) * inc,
(long)accuracy) assertion to fail spuriously.

Should num_online_cpus() == nr_cpu_ids be recorded while cpus_read_lock() is
still held?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102190708.1287100-1-mathieu.desnoyers@efficios.com?part=2