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

[sashiko-bot@kernel.org]

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.

> +
> +	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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104130836.134790-1-thomas.hellstrom@linux.intel.com?part=1