Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Deadlock via mempool exhaustion under the buddy allocator lock - [Medium] has_clear flag is not reset upon allocation, corrupting the free tree's clear-steering heuristic --
--- Patch [1]: [PATCH v8 1/2] gpu/buddy: replace dual-tree/force_merge with decoupled dirty tracker --- commit 2bd11b8a5badb80e1216b4be0ddab3e1dbd3cdbb Author: Arunpravin Paneer Selvam <[email protected]> gpu/buddy: replace dual-tree/force_merge with decoupled dirty tracker This commit replaces the dual-tree buddy allocator with a single free tree and a decoupled dirty tracker to avoid merging barriers during mixed workloads. It removes the O(N) __force_merge scan and instead uses an augmented interval rbtree for tracking dirty extents. > diff --git a/drivers/gpu/buddy.c b/drivers/gpu/buddy.c > --- a/drivers/gpu/buddy.c > +++ b/drivers/gpu/buddy.c [ ... ] > +static struct gpu_dirty_extent *extent_alloc(struct gpu_dirty_tracker > *dirty_tracker) > +{ > + /* > + * The void free/reset paths must record an extent and cannot handle > + * failure, so the mempool reserve guarantees a non-NULL return > + * without __GFP_NOFAIL. GFP_KERNEL is safe under the buddy lock: no > + * driver frees buddy blocks from a shrinker, so reclaim cannot > + * recurse into the lock we hold. > + */ > + return mempool_alloc(dirty_tracker->extent_pool, GFP_KERNEL); > +} [Severity: High] Could this lead to a deadlock through mempool exhaustion under the buddy allocator lock? The comment assumes that because GFP_KERNEL can sleep for reclaim, it will eventually succeed without __GFP_NOFAIL. However, during an OOM event, kmalloc can fail and return NULL. If this happens during a loop in gpu_buddy_reset_clear() or __gpu_buddy_free_list() while holding the driver lock, the first allocation failure consumes the single reserved mempool element. A subsequent allocation failure in the same loop would block in mempool_alloc() waiting for a free element. Since the blocking thread holds the driver lock, no other thread can enter the allocator to free extents, which seems like it would cause an unrecoverable deadlock. [ ... ] > +static inline enum gpu_block_state > +gpu_block_cached_state(struct gpu_buddy_block *block) > +{ > + if (gpu_buddy_block_is_clear(block)) > + return GPU_BLOCK_CLEAR; > + if (block->has_clear) > + return GPU_BLOCK_MIXED; > + return GPU_BLOCK_DIRTY; > +} [Severity: Medium] Will this read a stale has_clear flag for blocks that were allocated without being split? Looking at mark_allocated(), the has_clear flag is not reset when the block changes to the allocated state. If a block previously evaluated as GPU_BLOCK_MIXED is allocated at its natural order and given to the driver, has_clear remains true. When the block is later freed, __gpu_buddy_free() uses this cached state instead of querying the dirty tracker, inserting the now fully dirty block into the free tree as MIXED. Would it be better to reset has_clear when the block is marked as allocated to prevent the tree's augmentation from diverging from the actual dirty tracker state? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
