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 <[email protected]> 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/[email protected]/ > 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/[email protected]?part=1
