Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Mempool deadlock in `gpu_buddy_reset_clear` and free paths during extent allocation when multiple extents are required within a single locked section. --
--- Patch [1]: [PATCH v7 1/2] gpu/buddy: replace dual-tree/force_merge with decoupled dirty tracker --- commit e1f55d34ee0b3bd852cd9853e793c8cbdf7fc6f6 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 design with a single free tree and an out-of-band dirty tracker using an augmented interval rbtree. It also moves extent allocations to a mempool to guarantee progress and avoid deadlocks associated with __GFP_NOFAIL. > diff --git a/drivers/gpu/buddy.c b/drivers/gpu/buddy.c > --- a/drivers/gpu/buddy.c > +++ b/drivers/gpu/buddy.c > @@ -34,6 +35,441 @@ > #endif > > static struct kmem_cache *slab_blocks; > +static struct kmem_cache *slab_extents; > + > +/* > + * A single reserved extent suffices. Every allocation uses GFP_KERNEL > + * from sleepable context, so the underlying slab alloc almost always > + * succeeds via reclaim; the reserve only backstops the rare case where > + * it still returns NULL (e.g. the current task is an OOM victim), > + * guaranteeing a non-NULL extent without __GFP_NOFAIL. Because each > + * alloc can independently wait for reclaim, the reserve need not scale > + * with the number of extents added in one locked section (e.g. by > + * gpu_buddy_reset_clear()). > + */ > +#define GPU_DIRTY_EXTENT_POOL_MIN 1 [ ... ] > +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); > +} [ ... ] > +static int gpu_dirty_tracker_init(struct gpu_dirty_tracker *dirty_tracker) > +{ > + dirty_tracker->root = RB_ROOT; > + dirty_tracker->total_dirty = 0; > + > + dirty_tracker->extent_pool = > + mempool_create_slab_pool(GPU_DIRTY_EXTENT_POOL_MIN, > slab_extents); > + if (!dirty_tracker->extent_pool) > + return -ENOMEM; > + > + return 0; > +} [ ... ] > @@ -572,42 +1055,39 @@ static int split_block(struct gpu_buddy *mm, > */ > void gpu_buddy_reset_clear(struct gpu_buddy *mm, bool is_clear) > { > - enum gpu_buddy_free_tree src_tree, dst_tree; > - u64 root_size, size, start; > - unsigned int order; > - int i; > + unsigned int i; > > gpu_buddy_driver_lock_held(mm); [ ... ] > + for (i = 0; i <= mm->max_order; ++i) { > - struct rb_root *root = &mm->free_trees[src_tree][i]; > struct gpu_buddy_block *block, *tmp; > > - rbtree_postorder_for_each_entry_safe(block, tmp, root, rb) { > - rbtree_remove(mm, block); > + rbtree_postorder_for_each_entry_safe(block, tmp, > + &mm->free_tree[i], rb) { > if (is_clear) { > - mark_cleared(block); > - mm->clear_avail += gpu_buddy_block_size(mm, > block); > + if (!gpu_buddy_block_is_clear(block)) > + block->header |= GPU_BUDDY_HEADER_CLEAR; > + block->has_clear = true; > + } else if (gpu_buddy_block_is_clear(block)) { > + block->header &= ~GPU_BUDDY_HEADER_CLEAR; > + block->has_clear = false; > + gpu_dirty_tracker_mark_dirty(&mm->dirty, > + > gpu_buddy_block_offset(block), > + > gpu_buddy_block_size(mm, block)); [Severity: High] Could this loop trigger a mempool deadlock during memory pressure? In gpu_dirty_tracker_init(), the extent pool is created with a minimum size of GPU_DIRTY_EXTENT_POOL_MIN (1). However, this loop in gpu_buddy_reset_clear() holds the driver lock and iterates over the free tree, potentially calling gpu_dirty_tracker_mark_dirty() multiple times. If the underlying slab allocation fails during extent_alloc(), mempool_alloc() will use the single reserved extent. If a second allocation fails within the same locked section, mempool_alloc() with GFP_KERNEL will sleep and retry indefinitely. Since the driver lock is held, no other thread can free blocks to return extents to the mempool, resulting in a deadlock. Does the mempool reserve need to scale with the maximum number of extents that can be allocated in a single locked section? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
