Re: [RFC PATCH v24 06/10] lib: percpu_counter_tree: add lazy tree activation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] lib: percpu_counter_tree_kunit: missing release barrier on sh.go
--

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

lib: percpu_counter_tree: add lazy tree activation

This commit introduces lazy tree initialization and activation for
percpu_counter_tree so counters can remain exact without allocating per-CPU
or tree memory until activated. It also adds KUnit and userspace tests for
lazy counter updates and concurrent activation.

> diff --git a/lib/tests/percpu_counter_tree_kunit.c b/lib/tests/percpu_counter_tree_kunit.c
> index 5774278ea1403..156bb63de3466 100644
> --- a/lib/tests/percpu_counter_tree_kunit.c
> +++ b/lib/tests/percpu_counter_tree_kunit.c
> @@ -796,6 +798,220 @@ static void hpcc_test_saturated_compare(struct kunit *test)

[ ... ]

> +static void lazy_race_fn(struct work_struct *work)
> +{
> +	struct lazy_race_work *w = container_of(work, struct lazy_race_work, work);
> +	struct lazy_race_shared *sh = w->sh;
> +	long sum = 0;
> +	int i;
> +
> +	for (i = 0; i < LAZY_RACE_NR_INC; i++) {
> +		long inc = (long)(get_random_u32() % 65) - 32;
> +
> +		if (i == LAZY_RACE_NR_FIRST) {
> +			/* Fully ordered: the first updates are visible before the pause. */
> +			atomic_long_add_return(sum, &sh->first_sum);
> +			atomic_inc_return(&sh->paused);
> +			while (!READ_ONCE(sh->go))
> +				cond_resched();
> +			if (!w->owner) {
> +				mutex_lock(&sh->activate_lock);
> +				if (percpu_counter_tree_activate_many(sh->c, 1, GFP_KERNEL) ||
> +				    !percpu_counter_tree_active(sh->c))
> +					atomic_inc(&sh->activate_errors);
> +				mutex_unlock(&sh->activate_lock);
> +			}
> +		}
> +		if (w->owner)
> +			percpu_counter_tree_add_owner(sh->c, inc);
> +		else
> +			percpu_counter_tree_add(sh->c, inc);
> +		sum += inc;
> +		if (!(i & 1023))
> +			cond_resched();
> +	}
> +	atomic_long_add(sum, &sh->expect);
> +	kfree(w);
> +	complete_work();
> +}
> +
> +static void hpcc_test_lazy_activation_race(struct kunit *test)
> +{
> +	struct percpu_counter_tree pct;
> +	int round;
> +
> +	if (num_online_cpus() < 2)
> +		kunit_skip(test, "needs at least 2 online CPUs");
> +	for (round = 0; round < 4; round++) {
> +		struct lazy_race_shared sh = { .c = &pct };
> +		unsigned long timeout = jiffies + 10 * HZ;
> +		int cpu, nr_workers = 0;
> +
> +		KUNIT_ASSERT_EQ(test, 0, percpu_counter_tree_init_many_lazy(&pct, 1, 32));
> +		mutex_init(&sh.activate_lock);
> +		init_kthreads();
> +		cpus_read_lock();
> +		for_each_online_cpu(cpu) {
> +			struct lazy_race_work *w;
> +
> +			if (nr_workers > LAZY_RACE_MAX_REMOTE)
> +				break;
> +			w = kzalloc_obj(*w);
> +			KUNIT_EXPECT_NOT_NULL(test, w);
> +			if (!w)
> +				break;
> +			INIT_WORK(&w->work, lazy_race_fn);
> +			w->sh = &sh;
> +			/* The first worker is the owner, the others update remotely. */
> +			w->owner = !nr_workers;
> +			atomic_inc(&kernel_threads_to_run);
> +			queue_work_on(cpu, hpcc_test_wq, &w->work);
> +			nr_workers++;
> +		}
> +		cpus_read_unlock();
> +		while (atomic_read(&sh.paused) < nr_workers && time_before(jiffies, timeout))
> +			usleep_range(100, 200);
> +		smp_mb();	/* Read the counter after the first updates of the workers. */
> +		/* Every worker, the owner included, has updates left for after the activation. */
> +		KUNIT_EXPECT_EQ(test, atomic_read(&sh.paused), nr_workers);
> +		/* Before activation, the owner and atomic updates are exact. */
> +		check_lazy_counter(test, &pct, atomic_long_read(&sh.first_sum));
> +		WRITE_ONCE(sh.go, true);

[Severity: Low]
In hpcc_test_lazy_activation_race(), can a weakly ordered CPU reorder the
relaxed WRITE_ONCE(sh.go, true) before the loads of pct.bias and
pct.atomic_bias inside check_lazy_counter()?

Before activation, pct.level0 is NULL, so
percpu_counter_tree_precise_sum_unbiased() returns early after
smp_load_acquire(&counter->level0) and atomic_long_read(&counter->atomic_bias)
without issuing a barrier after reading atomic_bias and bias.

Once sh.go becomes true in lazy_race_fn(), the owner worker immediately calls
percpu_counter_tree_add_owner(sh->c, inc) without acquiring sh->activate_lock,
and remote workers proceed to activate and update sh->c.

Should sh.go be set with smp_store_release(&sh.go, true) (or preceded by
smp_mb()) so the pre-activation reads in check_lazy_counter() complete before
the paused workers resume modifying pct?

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