Re: [PATCH v2] drm/xe: Add a per-client debugfs file reporting bo NUMA node distribution

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

On Wed, 2026-11-04 at 13:24 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> - [Medium] drm/xe: shrinker-purged BOs misreported as swapped in
> xe_bo_print_numa_info()
> - [Medium] drm/xe: DEFER_BACKING VRAM BOs misreported in
> xe_bo_print_numa_info_locked()
> --
> 
> commit 66cc9eac8b32df8912217731dcdb5be7a56a8d77
> Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> 
> drm/xe: Add a per-client debugfs file reporting bo NUMA node
> distribution
> 
> This commit adds a per-client "bo_numa" debugfs file that implements
> a
> write-request / read-response protocol to report the size, placement,
> and
> per-NUMA-node page counts of a GEM buffer object specified by handle.
> 
> > diff --git a/drivers/gpu/drm/xe/xe_bo.c
> > b/drivers/gpu/drm/xe/xe_bo.c
> > index 42a6aa1a2e4a0..bf3f6d9e4c61c 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.c
> > +++ b/drivers/gpu/drm/xe/xe_bo.c
> 
> [ ... ]
> 
> > @@ -4248,6 +4250,113 @@ void
> > xe_bo_runtime_pm_release_mmap_offset(struct xe_bo *bo)
> >  	list_del_init(&bo->vram_userfault_link);
> >  }
> >  
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > +static void xe_bo_count_numa_pages(struct ttm_tt *tt, unsigned
> > long *nodes)
> > +{
> > +	pgoff_t i;
> > +
> > +	for (i = 0; i < tt->num_pages; ++i)
> > +		nodes[page_to_nid(tt->pages[i])]++;
> > +}
> > +
> > +static void xe_bo_print_numa_info_locked(struct xe_bo *bo,
> > unsigned long *nodes,
> > +					 struct drm_printer *p)
> > +{
> > +	struct ttm_resource *res = bo->ttm.resource;
> > +	struct ttm_tt *tt = bo->ttm.ttm;
> > +	unsigned long total = 0;
> > +	const char *name;
> > +	int nid;
> > +
> > +	xe_bo_assert_held(bo);
> > +
> > +	name = res ? xe_mem_type_to_name[res->mem_type] : NULL;
> > +	drm_printf(p, "size: %zu\n", xe_bo_size(bo));
> > +	if (name)
> > +		drm_printf(p, "placement: %s\n", name);
> > +	else if (res)
> > +		drm_printf(p, "placement: mem_type %u\n", res-
> > >mem_type);
> > +	else
> > +		drm_puts(p, "placement: none\n");
> > +
> > +	/* The pages of sg bos are owned by the exporter and not
> > visible. */
> > +	if (bo->ttm.type == ttm_bo_type_sg) {
> > +		drm_puts(p, "pages: unknown (sg bo)\n");
> > +		return;
> > +	}
> > +
> > +	if (tt && ttm_tt_is_swapped(tt)) {
> > +		drm_puts(p, "pages: swapped\n");
> > +		return;
> > +	}
> 
> [Severity: Medium]
> In drivers/gpu/drm/xe/xe_bo.c, can xe_bo_print_numa_info_locked()
> misreport
> shrinker-purged buffer objects as swapped?
> 
> When a buffer object marked DONTNEED is reclaimed by the shrinker via
> xe_bo_shrink() -> xe_bo_shrink_purge(), it invokes ttm_bo_shrink()
> with
> .purge = true:
> 
> drivers/gpu/drm/xe/xe_bo.c:xe_bo_shrink_purge() {
> 	lret = ttm_bo_shrink(ctx, bo, (struct ttm_bo_shrink_flags)
> 			     {.purge = true,
> 			      .writeback = false,
> 			      .allow_move = false});
> }
> 
> Inside ttm_bo_shrink() -> ttm_tt_backup(), ttm_pool_backup() frees
> the
> pages and returns the freed page count, causing ttm_tt_backup() to
> set
> TTM_TT_FLAG_BACKED_UP on tt->page_flags even when purging. For
> example,
> xe_ttm_tt_populate() specifically checks !xe_tt->purgeable alongside
> ttm_tt_is_backed_up(tt):
> 
> drivers/gpu/drm/xe/xe_bo.c:xe_ttm_tt_populate() {
> 	if (ttm_tt_is_backed_up(tt) && !xe_tt->purgeable) {
> 		err = ttm_tt_restore(ttm_dev, tt, ctx);
> 	} else {
> 		ttm_tt_clear_backed_up(tt);
> }
> 
> Because ttm_tt_is_swapped(tt) tests TTM_TT_FLAG_SWAPPED |
> TTM_TT_FLAG_BACKED_UP without checking xe_bo_is_purged(bo) or
> xe_tt->purgeable, a BO purged by the shrinker will be reported as
> "pages: swapped" rather than purged or not populated.

Could rename this as swapped/purged. I'll update a V3.

> 
> > +
> > +	if (tt && !ttm_tt_is_populated(tt)) {
> > +		drm_puts(p, "pages: not populated\n");
> > +		return;
> > +	}
> 
> [Severity: Medium]
> What happens in xe_bo_print_numa_info_locked() in
> drivers/gpu/drm/xe/xe_bo.c
> when a VRAM buffer object is created with
> DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING
> and then validated into VRAM?
> 
> In xe_bo_init_locked(), XE_BO_FLAG_DEFER_BACKING initially places the
> BO in
> sys_placement, which causes ttm_bo_validate() to allocate an
> unpopulated
> bo->ttm.ttm:
> 
> drivers/gpu/drm/xe/xe_bo.c:xe_bo_init_locked() {
> 	/* Defer populating type_sg bos */
> 	placement = (type == ttm_bo_type_sg ||
> 		     bo->flags & XE_BO_FLAG_DEFER_BACKING) ?
> &sys_placement :
> 		&bo->placement;
> }
> 
> When the BO is subsequently validated into VRAM via xe_bo_validate()
> ->
> ttm_bo_validate() -> xe_bo_move(), move_lacks_source is true because
> the
> ttm_tt was never populated, so xe_bo_move() takes the
> ttm_bo_move_null()
> path instead of ttm_bo_move_accel_cleanup():
> 
> drivers/gpu/drm/xe/xe_bo.c:xe_bo_move() {
> 	} else {
> 		/*
> 		 * ttm_bo_move_accel_cleanup() may blow up if
> 		 * bo->resource == NULL, so just attach the
> 		 * fence and set the new resource.
> 		 */
> 		dma_resv_add_fence(ttm_bo->base.resv, fence,
> 				   DMA_RESV_USAGE_KERNEL);
> 		ttm_bo_move_null(ttm_bo, new_mem);
> 	}
> }
> 
> Because ttm_bo_move_null() does not destroy the unused bo->ttm.ttm,
> tt
> remains non-NULL and unpopulated while the BO is populated in VRAM.
> Wouldn't
> xe_bo_print_numa_info_locked() then report "placement: vram0"
> followed by
> "pages: not populated", unlike normal VRAM BOs where tt is NULL?

This is a separate bug that needs to be fixed. The tt should be
dropped.