Re: [PATCH v3 1/3] drm/xe/mm: add XE MEM POOL manager with shadow support

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Wed, 2026-04-01 at 16:15 +0000, Satyanarayana K V P wrote:
> Add a xe_mem_pool manager to allocate sub-ranges from a BO-backed
> pool
> using drm_mm.
> 
> Signed-off-by: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Maarten Lankhorst <dev@lankhorst.se>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> 
> ---
> V2 -> V3:
> - Renamed xe_mm_suballoc to xe_mem_pool_manager.
> - Splitted xe_mm_suballoc_manager_init() into xe_mem_pool_init() and
> xe_mem_pool_shadow_init() (Michal)
> - Made xe_mm_sa_manager structure private. (Matt)
> - Introduced init flags to initialize allocated pools.
> 
> V1 -> V2:
> - Renamed xe_drm_mm to xe_mm_suballoc (Thomas)
> - Removed memset during manager init and insert (Matt)

Hey, Run a kreview using review-prompts with Claude:

> diff --git a/drivers/gpu/drm/xe/xe_mem_pool.c
b/drivers/gpu/drm/xe/xe_mem_pool.c
> new file mode 100644

[ ... ]

> +static void xe_mem_pool_fini(struct drm_device *drm, void *arg)
> +{
> +	struct xe_mem_pool_manager *pool_manager = arg;
> +
> +	drm_mm_takedown(&pool_manager->base);
> +
> +	if (pool_manager->resv_alloc) {
> +		drm_mm_remove_node(pool_manager->resv_alloc);
> +		kfree(pool_manager->resv_alloc);
> +	}

The node removal happens after drm_mm_takedown(), but drm_mm_takedown()
requires the allocator to be clean before it is called.

When XE_MEM_POOL_BO_FLAG_INIT_CMD_BB_END_HIGHEST is used,
xe_mem_pool_init()
calls xe_mem_pool_init_flags() which inserts a reserved node and stores
it
in pool_manager->resv_alloc.  On device teardown, xe_mem_pool_fini() is
invoked with that node still allocated inside the drm_mm.

drm_mm_takedown() in drivers/gpu/drm/drm_mm.c does:

    void drm_mm_takedown(struct drm_mm *mm)
    {
        if (WARN(!drm_mm_clean(mm),
                 "Memory manager not clean during takedown.\n"))
            show_leaks(mm);
    }

drm_mm_clean() returns list_empty(drm_mm_nodes(mm)), which is false
when
resv_alloc is still inserted.  So the WARN fires on every teardown of a
pool created with BB_END_HIGHEST.

Shouldn't drm_mm_remove_node(pool_manager->resv_alloc) be called before
drm_mm_takedown(), so the allocator is clean at the point of takedown?

[ ... ]

> +/**
> + * xe_mem_pool_remove_node() - Remove a node from the DRM MM
manager.
> + * @node: the DRM MM node to remove.
> + *
> + * Return: None.
> + */
> +void xe_mem_pool_remove_node(struct drm_mm_node *node)
> +{
> +	return drm_mm_remove_node(node);
> +}

This isn't a bug, but returning a void expression from a void function
is unusual - the return statement here implies a return value where
there
is none.  The body could just be drm_mm_remove_node(node) without the
return.