Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Massive memory footprint in KUnit test triggers OOM in constrained environments - [Medium] Benchmark logic bypasses the measured path in the baseline allocator --
--- Patch [2]: [PATCH v7 2/2] gpu/tests/buddy: add dirty tracker performance KUnit test --- commit 7af49c21f49c14e81ea1bc47365402c2ec976aec Author: Arunpravin Paneer Selvam <[email protected]> gpu/tests/buddy: add dirty tracker performance KUnit test This commit adds a KUnit test `gpu_test_buddy_dirty_tracker_performance` to demonstrate the performance benefits of the decoupled dirty-tracker design compared to the dual-tree approach. It measures contiguous allocation latency and repeated allocation throughput after fragmentation. > diff --git a/drivers/gpu/tests/gpu_buddy_test.c > b/drivers/gpu/tests/gpu_buddy_test.c > index f8e56da5058e6..3fc2bfa66ee15 100644 > --- a/drivers/gpu/tests/gpu_buddy_test.c > +++ b/drivers/gpu/tests/gpu_buddy_test.c [ ... ] > +static void gpu_test_buddy_dirty_tracker_performance(struct kunit *test) > +{ > + struct gpu_buddy_block *block, *tmp; > + unsigned long elapsed_ms; [ ... ] > + KUNIT_ASSERT_FALSE_MSG(test, gpu_buddy_init(&mm, SZ_4G, SZ_4K), > + "buddy_init failed\n"); > + > + for (i = 0; i < SZ_4G / SZ_4K; i++) > + KUNIT_ASSERT_FALSE_MSG(test, > + gpu_buddy_alloc_blocks(&mm, 0, SZ_4G, > SZ_4K, SZ_4K, > + &allocated, 0), > + "buddy_alloc hit an error size=%u\n", > SZ_4K); [Severity: Medium] Will this massive memory footprint trigger an Out-Of-Memory panic in constrained environments? Allocating a 4 GiB pool using 4 KiB block granularity requires creating over 1 million struct gpu_buddy_block nodes dynamically from the slab. This will consume well over 100 MiB of kernel memory. Default UML environments for KUnit often have a memory limit of 256 MiB or less, and this exhaustive allocation is likely to crash the test suite. [ ... ] > + /* > + * Repeated alloc throughput from a maximally fragmented pool > + * > + * Fill a 4 GiB pool with 4 KiB allocations, free even-indexed blocks > + * as cleared and odd-indexed blocks as dirty. The alternating pattern > + * ensures every adjacent buddy pair has one cleared half and one dirty > + * half, so each pair lands on opposite sides of the old merge barrier. > + * Each of the 16 384 x 256 KiB allocations in the timed loop has to > + * pay the __force_merge() cost on the alloc path under the old design. > + * With the dirty-tracker design the pool collapses to one max_order > + * block during free(), so each alloc is a simple O(log N) split. > + */ [ ... ] > + start = ktime_get(); > + for (i = 0; i < SZ_4G / SZ_256K; i++) > + KUNIT_ASSERT_FALSE_MSG(test, > + gpu_buddy_alloc_blocks(&mm, 0, SZ_4G, > SZ_256K, SZ_4K, > + &results, 0), > + "buddy_alloc hit an error size=%u\n", > SZ_256K); [Severity: Medium] Does this benchmark logic bypass the measured path in the baseline allocator? The commit message states this measures the __force_merge() cost on the alloc path under the old design. However, the flags parameter is passed as 0 here, which omits GPU_BUDDY_CONTIGUOUS_ALLOCATION. Without the contiguous flag, the baseline allocator does not enforce that the 256 KiB allocation must be a single block. Instead, it will fall back to satisfying the 256 KiB request with 64 independent 4 KiB blocks. Because 4 KiB blocks are readily available in the fragmented pool, the allocation succeeds without ever invoking the __force_merge() fallback. Are we actually comparing the cost of 64 tree lookups against 1 tree lookup, rather than measuring the __force_merge() overhead? > + end = ktime_get(); > + elapsed_ms = ktime_to_ms(ktime_sub(end, start)); > + > + kunit_info(test, "Repeated 256 KiB allocs from fragmented pool: %lu > ms\n", > + elapsed_ms); > + > + gpu_buddy_free_list(&mm, &results, 0); > + gpu_buddy_fini(&mm); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
