Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Iterating the entire buddy allocator tree to dump allocated blocks via `pr_info` while holding the driver lock can cause severe CPU stalls and system lockups. - [Medium] The patch includes an unused DRM header (`<drm/drm_print.h>`), fails to use `drm_printer`, and has a mismatch between the expected output in the commit message and the actual implementation. --
--- Patch [7]: [PATCH V11 07/11] gpu/buddy: Add routine to dump allocated buddy blocks --- commit dd1833d79b732eba09f7b2487608ee8cabc53593 Author: Tejas Upadhyay <[email protected]> gpu/buddy: Add routine to dump allocated buddy blocks To implement the ability to see allocated blocks under a specific VRAM instance in the drm driver, new api is introduced. While existing structs often show the free block list, this addition provides a comprehensive view of all currently resident VRAM allocations. > Dump will look like, > > [ +0.000003] xe 0000:03:00.0: [drm] 0x00000002f8000000-0x00000002f8800000: > 8388608 > [ +0.000005] xe 0000:03:00.0: [drm] 0x00000002f8800000-0x00000002f8840000: > 262144 [Severity: Medium] Is this expected output accurate for this patch? The commit message implies the use of a DRM-specific printing interface like drm_printf to provide the "xe ... [drm]" prefix. However, gpu_buddy_dump_allocated_blocks() does not accept a drm_printer argument and relies on gpu_buddy_block_print() which unconditionally uses pr_info() instead. [ ... ] > diff --git a/drivers/gpu/buddy.c b/drivers/gpu/buddy.c > index 4d5ac375a538c..e11e9cfb8807e 100644 > --- a/drivers/gpu/buddy.c > +++ b/drivers/gpu/buddy.c > @@ -10,6 +10,7 @@ > #include <linux/sizes.h> > > #include <linux/gpu_buddy.h> > +#include <drm/drm_print.h> [Severity: Medium] Is this include necessary? Since this code relies on pr_info() and doesn't take a drm_printer context, it looks like this include is unused and adds a DRM dependency to the generic allocator. [ ... ] > +void gpu_buddy_dump_allocated_blocks(struct gpu_buddy *mm) > +{ > + struct gpu_buddy_block *block; > + LIST_HEAD(dfs); > + int i; > + > + gpu_buddy_driver_lock_held(mm); > + > + for (i = 0; i < mm->n_roots; ++i) > + list_add_tail(&mm->roots[i]->tmp_link, &dfs); > + > + do { > + block = list_first_entry_or_null(&dfs, > + struct gpu_buddy_block, > + tmp_link); > + if (!block) > + break; > + > + list_del(&block->tmp_link); > + > + if (gpu_buddy_block_is_allocated(block)) > + gpu_buddy_block_print(mm, block); [Severity: High] Could this cause a regression with CPU stalls or trigger watchdogs on highly fragmented systems? When called, this loop iterates over the entire allocator tree while holding the driver lock. In a modern GPU with multi-gigabyte VRAM, this tree can contain a massive number of blocks. gpu_buddy_block_print() unconditionally calls pr_info() for every block: drivers/gpu/buddy.c:gpu_buddy_block_print() { ... pr_info("%#018llx-%#018llx: %llu\n", start, start + size, size); } Would outputting thousands or millions of lines to the kernel ring buffer and serial console synchronously under a global lock block other operations for an extended period? > + > + if (gpu_buddy_block_is_split(block)) { > + list_add(&block->right->tmp_link, &dfs); > + list_add(&block->left->tmp_link, &dfs); > + } > + } while (1); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
