Re: [RFC PATCH v25 09/14] lib: percpu_counter_tree: test the limit counters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] lib: percpu_counter_tree: unreachable subtraction in
  limit_worker_fn()
- [Low] lib: percpu_counter_tree: unordered sh->charged sub in
  limit_worker_fn()
--

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

lib: percpu_counter_tree: test the limit counters

This commit adds KUnit and userspace hpcc harness tests for
percpu_counter_tree_limit, along with a stall hook in the userspace shim
to simulate concurrent updates while an addition carry is in flight.

> diff --git a/lib/tests/percpu_counter_tree_kunit.c b/lib/tests/percpu_counter_tree_kunit.c
> index 12bd5a4bbb459..bd99ab7ca1c07 100644
> --- a/lib/tests/percpu_counter_tree_kunit.c
> +++ b/lib/tests/percpu_counter_tree_kunit.c
> @@ -612,6 +612,177 @@ static void hpcc_test_contended_carries(struct kunit *test)
>  		do_hpcc_test_contended_carries(test, 1UL << batch_size_order);
>  }
>  
> +#define LIMIT_NR_OPS	100000

[ ... ]

> +static int limit_worker_fn(void *data)
> +{
> +	struct limit_data *ld = data;
> +	struct limit_shared *sh = ld->sh;
> +	unsigned int i;
> +
> +	/* Pairs with the release of started: max, target and nr_workers are set. */
> +	wait_event(limit_wq, smp_load_acquire(&sh->started));
> +	/* Start together, so that the workers hold their amounts at the same time. */
> +	atomic_inc(&sh->nr_ready);
> +	while (atomic_read(&sh->nr_ready) < sh->nr_workers)
> +		cpu_relax();
> +	for (i = 0; i < LIMIT_NR_OPS; i++) {
> +		u32 x = limit_rnd(ld);
> +		unsigned long amount;
> +
> +		if (ld->held < sh->target) {
> +			/* Mostly small amounts, and one in 16 of a huge page. */
> +			if ((x >> 4) & 15)
> +				amount = 1 + (x >> 8) % (4 * sh->batch_size);
> +			else
> +				amount = 512;
> +			if (percpu_counter_tree_limit_add(&sh->limit, sh->max, amount)) {
> +				unsigned long charged;
> +
> +				charged = atomic_long_add_return(amount, &sh->charged);
> +				if (charged > sh->max)
> +					atomic_long_inc(&sh->nr_over);
> +				ld->held += amount;
> +				atomic_long_inc(&sh->nr_added);
> +			} else {
> +				atomic_long_inc(&sh->nr_failed);
> +			}
> +		} else if (ld->held) {
> +			amount = 1 + (x >> 8) % ld->held;
> +			atomic_long_sub(amount, &sh->charged);
> +			percpu_counter_tree_limit_sub(&sh->limit, amount);
> +			ld->held -= amount;
> +		}
> +		if (!(i & 63))
> +			cond_resched();
> +	}
> +	atomic_long_sub(ld->held, &sh->charged);
> +	percpu_counter_tree_limit_sub(&sh->limit, ld->held);

[Severity: Low]
Can the relaxed atomic_long_sub() on sh->charged be reordered after
percpu_counter_tree_limit_sub() on weakly ordered architectures?

The comment above limit_worker_fn() notes that a free subtracts from
sh->charged before it reaches the limit counter so sh->charged never
exceeds the limit counter.  However, non-value-returning atomic_long_sub()
is unordered, and percpu_counter_tree_limit_sub() updates the per-CPU
level 0 counter via this_cpu_try_cmpxchg() without a barrier when there is
no carry:

CPU0                              CPU1
limit_worker_fn()                 limit_worker_fn()
  atomic_long_sub(..., &sh->charged) (reordered)
  percpu_counter_tree_limit_sub()
                                    percpu_counter_tree_limit_add() -> true
                                    charged = atomic_long_add_return(...,
                                                              &sh->charged)
                                    if (charged > sh->max)
                                      atomic_long_inc(&sh->nr_over);
  /* sh->charged sub becomes visible */

This can cause a false-positive KUnit failure on sh->nr_over.  Using
atomic_long_sub_return() or smp_mb__after_atomic() before
percpu_counter_tree_limit_sub() in both the loop and the final cleanup
would enforce the intended ordering.

> +	ld->held = 0;
> +	complete_work();
> +	return 0;
> +}
> +
> +static void do_hpcc_test_limit(struct kunit *test, unsigned long batch_size)
> +{

[ ... ]

> +	accuracy = percpu_counter_tree_approximate_accuracy(&sh->limit.counters[0]);
> +	/* A limit of twice the accuracy, oversubscribed twice: mostly within its accuracy. */
> +	sh->max = 2 * accuracy + 1;
> +	sh->batch_size = batch_size;
> +	init_kthreads();
> +	cpus_read_lock();
> +	cpus = select_worker_cpus();
> +	sh->target = 2 * sh->max / cpumask_weight(cpus) + 512;

[Severity: Medium]
Does setting sh->target to 2 * sh->max / cpumask_weight(cpus) + 512 prevent
limit_worker_fn() from reaching its in-loop subtraction branch?

In limit_worker_fn(), a worker only subtracts when ld->held >= sh->target
and never frees any held amount when percpu_counter_tree_limit_add()
fails.  Because the total held across all workers cannot exceed sh->max,
no single worker can ever reach ld->held >= sh->target when
sh->target > sh->max (which holds on 2 to 8 CPUs for both batch sizes, and
up to 64 CPUs for batch_size = 2 due to the + 512 offset).  Even with more
CPUs where sh->target <= sh->max, all workers start together at
sh->nr_ready and average sh->max / nr_workers each when the counter fills,
so once total held reaches sh->max with every worker below sh->target, no
further addition can succeed and no worker can subtract for the rest of
LIMIT_NR_OPS.

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