Re: [RFC PATCH v23 10/10] lib: add a benchmark of hierarchical per-cpu counters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] lib: stack-allocated percpu_counter in bench triggers debugobjects WARN_ON
- [Low] lib: missing kernel-parameters.txt entry for percpu_counter_tree_bench.batch
--

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

lib: add a benchmark of hierarchical per-cpu counters

This commit adds a KUnit benchmark comparing hierarchical per-CPU counters
with struct percpu_counter across update, lifecycle, and churn workloads.
It introduces CONFIG_PERCPU_COUNTER_TREE_BENCH and the batch module
parameter to configure the hierarchical counter batch size.

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

[ ... ]

> +/* Like the RSS counters by default. */
> +static unsigned int batch = 32;
> +module_param(batch, uint, 0444);
> +MODULE_PARM_DESC(batch, "Batch size of the hierarchical counters (power of 2, 2 to 128)");

[Severity: Low]
Should percpu_counter_tree_bench.batch= also be documented in
Documentation/admin-guide/kernel-parameters.txt?

Both the commit message and the CONFIG_PERCPU_COUNTER_TREE_BENCH help text in
lib/Kconfig advertise passing percpu_counter_tree_bench.batch= on the kernel
command line alongside percpu_counter_tree.arity=, which is documented there.

[ ... ]

> +static int bench_lifecycle_fn(void *data)
> +{
> +	struct bench_thread *t = data;
> +	struct percpu_counter_tree c[LIFECYCLE_NR];
> +	struct percpu_counter pc[LIFECYCLE_NR];
> +	u64 t0;
> +	int i;
> +
> +	if (bench_wait_go(t->sh))
> +		return 0;
> +	t0 = ktime_get_ns();
> +	for (i = 0; t->sh->impl == IMPL_PERCPU_COUNTER && i < LIFECYCLE_ITER; i++) {
> +		if (percpu_counter_init_many(pc, 0, GFP_KERNEL, LIFECYCLE_NR))
> +			break;
> +		percpu_counter_add(&pc[0], 1);
> +		percpu_counter_add(&pc[0], -1);
> +		percpu_counter_destroy_many(pc, LIFECYCLE_NR);
> +		cond_resched();
> +	}

[ ... ]

> +static int bench_churn_fn(void *data)
> +{
> +	struct bench_thread *t = data;
> +	unsigned int pin_period = t->sh->pin_period;
> +	struct percpu_counter_tree tmp[LIFECYCLE_NR], *c;
> +	struct percpu_counter tmp_pc[LIFECYCLE_NR], *pc;
> +	u64 t0;
> +	int i;
> +
> +	if (bench_wait_go(t->sh))
> +		return 0;
> +	t0 = ktime_get_ns();
> +	for (i = 0; t->sh->impl == IMPL_PERCPU_COUNTER && i < CHURN_ITER; i++) {
> +		bool pin = pin_period && !(i % pin_period);
> +
> +		pc = pin ? &t->pinned_pc[t->nr_pinned * LIFECYCLE_NR] : tmp_pc;
> +		if (percpu_counter_init_many(pc, 0, GFP_KERNEL, LIFECYCLE_NR))
> +			break;

[Severity: Medium]
Will passing stack-allocated struct percpu_counter arrays (pc in
bench_lifecycle_fn() and tmp_pc in bench_churn_fn()) to
percpu_counter_init_many() trigger a debugobjects WARN_ON(1) when
CONFIG_DEBUG_OBJECTS_PERCPU_COUNTER is enabled?

The call chain is:

  percpu_counter_init_many()
    __percpu_counter_init_many()
      debug_percpu_counter_activate()
        debug_object_init()
          __debug_object_init(..., onstack = 0)
            lookup_object_or_alloc()
              debug_object_is_on_stack()

Because debug_object_init() is called with onstack = 0 while pc and tmp_pc
reside on the kthread stack, debug_object_is_on_stack() will warn that a
non-onstack object is on the stack.

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