Re: [PATCH v3] 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 1 potential issue(s) to consider:
- [Medium] drm/xe: BOs purged via xe_ttm_bo_purge() misreported in xe_bo_print_numa_info()
--

commit 1dc9067175a153440fcff8007b4ca1cbbb81027f
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 takes a GEM
handle written by userspace and reports a snapshot of the buffer object's
size, placement, and backing page distribution across NUMA nodes.

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 42a6aa1a2e4a0..ee232e73bc3b9 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/purged\n");
> +		return;
> +	}
> +
> +	if (tt && !ttm_tt_is_populated(tt)) {
> +		drm_puts(p, "pages: not populated\n");
> +		return;
> +	}

[Severity: Medium]
As reported by Sashiko on v2 [1], in xe_bo_print_numa_info_locked(),
should this also check xe_bo_is_purged(bo) before checking
!ttm_tt_is_populated(tt)?

While purging via the shrinker sets TTM_TT_FLAG_BACKED_UP on
tt->page_flags, DONTNEED buffer objects can also be purged via
xe_ttm_bo_purge() during eviction in xe_bo_move(), swap notification in
xe_ttm_bo_swap_notify(), or VRAM page retirement:

drivers/gpu/drm/xe/xe_bo.c:xe_ttm_bo_purge() {
    ...
	ret = ttm_bo_validate(ttm_bo, &place, ctx);
	if (ret)
		return ret;

	/* Commit the state transition only once invalidation was queued */
	xe_bo_set_purgeable_state(bo, XE_MADV_PURGEABLE_PURGED);
    ...
}

Validating with an empty placement invokes ttm_bo_pipeline_gutting(),
which unpopulates or replaces bo->ttm.ttm without setting
TTM_TT_FLAG_SWAPPED or TTM_TT_FLAG_BACKED_UP. As a result, when a handle
for a BO purged via xe_ttm_bo_purge() is written to bo_numa,
ttm_tt_is_swapped(tt) is false and xe_bo_print_numa_info_locked() reports
"pages: not populated" instead of "pages: swapped/purged".

[1] https://lore.kernel.org/r/sashiko-outbox-197338@kernel.org

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