Re: [PATCH v3] lib: test bitmap vs IDA vs Maple Tree performance for region allocations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Silent integer truncation for capacities > UINT_MAX leads to corrupted benchmark state and false positive warnings.
- [Medium] Unhandled -ENOMEM from internal allocations triggers false positive WARN_ON.
--

--- Patch [1]: [PATCH v3] lib: test bitmap vs IDA vs Maple Tree performance for region allocations ---
commit d6b29e543ce674f28d588ee458044d2815d73caa
Author: Yury Norov <ynorov@nvidia.com>

lib: test bitmap vs IDA vs Maple Tree performance for region allocations
    
Compare the cost of allocating and freeing variable-sized regions using
a bitmap, IDA and a Maple Tree. All implementations process the same
randomly generated sequence of regions containing up to 32 entries, until
the configured capacity is exhausted.
    
Link: https://lore.kernel.org/all/20260710-chid-maple-v1-1-4ee869055268@nvidia.com/

> diff --git a/lib/region_alloc_benchmark.c b/lib/region_alloc_benchmark.c
> index 0000000000000..e88b4cf55c629
> --- /dev/null
> +++ b/lib/region_alloc_benchmark.c

[ ... ]

> +static unsigned long capacities[64] = { 1000000, 100000, 10000, 1000, 100, 10 };
> +static unsigned int cap_cnt = 6;
> +
> +module_param_array(capacities, ulong, &cap_cnt, 0400);
> +MODULE_PARM_DESC(capacities, "Region capacities to benchmark");
> +
> +static unsigned long __init benchmark_bitmap(unsigned long cap)
> +{

[ ... ]

> +	alloc_time = ktime_get();
> +	for (cnt = 0; cnt <= cap; cnt++) {
> +		idx = bitmap_find_next_zero_area(bitmap, cap, 0, reg_sz[cnt], 0);
> +		if (idx >= cap)
> +			break;
> +
> +		reg_idx[cnt] = idx;
> +		bitmap_set(bitmap, idx, reg_sz[cnt]);

[Severity: Medium]
Since the capacities array is populated from a module parameter using ulong,
but bitmap_set() expects an unsigned int for the start index, could passing
a capacity larger than UINT_MAX cause the start index to silently truncate
to 32 bits here?

[ ... ]

> +static unsigned long __init benchmark_ida(unsigned long cap)
> +{

[ ... ]

> +	alloc_time = ktime_get();
> +	for (cnt = 0; cnt <= cap; cnt++) {
> +		for (off = 0; off < reg_sz[cnt]; off++) {
> +			id = ida_alloc_max(&ida, cap - 1, GFP_KERNEL);

[Severity: Medium]
Similarly, ida_alloc_max() takes an unsigned int max parameter. Does this
code truncate the 64-bit capacity bounds when cap exceeds UINT_MAX?

> +			if (id < 0)
> +				break;
> +
> +			if (!off)
> +				reg_idx[cnt] = id;
> +		}
> +		if (id < 0) {
> +			while (off--)
> +				ida_free(&ida, reg_idx[cnt] + off);
> +			break;
> +		}
> +		WARN_ON(id != reg_idx[cnt] + reg_sz[cnt] - 1);
> +		nr_ids += reg_sz[cnt];
> +	}
> +	alloc_time = ktime_get() - alloc_time;
> +
> +	WARN_ON(id != -ENOSPC);

[Severity: Medium]
If the system is under memory pressure, ida_alloc_max() can return -ENOMEM
since it allocates with GFP_KERNEL. Will this trigger a false positive
warning if internal allocations fail before reaching the capacity limit?

[ ... ]

> +static unsigned long __init benchmark_maple_tree(unsigned long cap)
> +{

[ ... ]

> +	alloc_time = ktime_get() - alloc_time;
> +
> +	WARN_ON(ret != -EBUSY);

[Severity: Medium]
Like the IDA implementation, mtree_alloc_range() can fail with -ENOMEM.
Can this code trigger a system warning if memory is exhausted early?

[ ... ]

> +static int __init region_alloc_benchmark(void)
> +{

[ ... ]

> +		bitmap_count = benchmark_bitmap(capacities[i]);
> +		maple_count  = benchmark_maple_tree(capacities[i]);
> +		ida_count    = benchmark_ida(capacities[i]);
> +
> +		WARN_ON(bitmap_count != ida_count);
> +		WARN_ON(bitmap_count != maple_count);

[Severity: Medium]
If one of the allocators fails early due to -ENOMEM while the others
succeed, could that cause their region counts to diverge and trigger
these assertions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717053241.916441-1-ynorov@nvidia.com?part=1