Re: [PATCH] drm/xe: Flush L2 asynchronously in xe_bo_trigger_rebind()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] drm/xe: wrong exec queue in xe_vma_tlb_flush_client() causes NULL deref and race
- [High] drm/xe: missing tile offset when indexing q->tlb_inval in xe_vm_flush_vm_bo_tlb_async()
- [High] drm/xe: xe_bo_trigger_rebind() ignores ctx->no_wait_gpu when queueing async TLB flush
- [High] drm/xe: GFP_KERNEL allocations in xe_vm_flush_vm_bo_tlb_async() under fs_reclaim
- [Medium] drm/xe: xe_bo_move() synchronously and uninterruptibly waits on async TLB flush
--

commit 1eb2e98a6dad350728e769ae239024f455ba2958
Author: Matthew Brost <matthew.brost@intel.com>

drm/xe: Flush L2 asynchronously in xe_bo_trigger_rebind()

This commit replaces the synchronous BO idle wait and VMA TLB invalidation in
xe_bo_trigger_rebind() on L2-flush-optimized platforms with asynchronous TLB
invalidation jobs queued via xe_vm_flush_vm_bo_tlb_async(). Each job waits on
the BO's in-flight GPU work and installs its completion fence into the BO's
dma-resv object with DMA_RESV_USAGE_KERNEL.

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 42a6aa1a2e4a0..0ffe4cc46b3b3 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -809,6 +809,9 @@ static int xe_bo_trigger_rebind(struct xe_device *xe, struct xe_bo *bo,
>  			 */
>  			if (!xe_device_is_l2_flush_optimized(xe))
>  				continue;
> +
> +			if (!xe_vm_flush_vm_bo_tlb_async(vm, bo, vm_bo))
> +				continue;

[Severity: High]
How does calling xe_vm_flush_vm_bo_tlb_async() before the ctx->no_wait_gpu
check interact with non-blocking callers such as the shrinker?

In the shrinker path, xe_shrinker_walk() runs its first pass with
ctx->no_wait_gpu = true (and in direct reclaim where
ttm_bo_shrink_avoid_wait() is true, ctx.no_wait_gpu remains true for the
entire shrinker invocation). For an already-idle purgeable BO in XE_PL_TT,
ttm_bo_shrink_suitable() succeeds because bo->base.resv is signaled, and
xe_bo_shrink() runs:

drivers/gpu/drm/xe/xe_bo.c:xe_bo_shrink() {
	if (xe_tt->purgeable) {
		if (bo->resource->mem_type != XE_PL_SYSTEM)
			lret = xe_bo_move_notify(xe_bo, ctx);
		if (!lret)
			lret = xe_bo_shrink_purge(ctx, bo, scanned);
}

When xe_bo_move_notify() -> xe_bo_trigger_rebind() calls
xe_vm_flush_vm_bo_tlb_async(), it queues an async TLB invalidation job on
gt->tlb_inval.job_wq and installs an unsignaled DMA_RESV_USAGE_KERNEL fence
into bo->ttm.base.resv. Immediately afterward, xe_bo_shrink_purge() calls
ttm_bo_wait_ctx(bo, ctx):

drivers/gpu/drm/xe/xe_bo.c:xe_bo_shrink_purge() {
	if (bo->resource->mem_type != XE_PL_SYSTEM) {
		struct ttm_resource *new_resource;

		lret = ttm_bo_wait_ctx(bo, ctx);
		if (lret)
			return lret;
}

Because ctx->no_wait_gpu is true and bo->ttm.base.resv now contains the
unsignaled flush fence, ttm_bo_wait_ctx() fails with -EBUSY after
drm_gpuvm_bo_evict() and xe_bo_vunmap() have already run. The same -EBUSY
failure can happen in xe_ttm_bo_purge(), or if a subsequent vm_bo on the same
BO falls through to the if (!idle) block in xe_bo_trigger_rebind().

Conversely, if a BO in XE_PL_TT has in-flight GPU work and xe_bo_move() is
called with ctx->no_wait_gpu = true, xe_bo_trigger_rebind() previously
returned -EBUSY at the ctx->no_wait_gpu check, whereas now it succeeds and
xe_bo_move() proceeds to wait unconditionally on DMA_RESV_USAGE_BOOKKEEP.
Should xe_bo_trigger_rebind() skip the async path or honor ctx->no_wait_gpu
before queueing unsignaled fences into the BO's dma-resv?

[Severity: Medium]
Also, on L2-flush-optimized platforms (where !IS_DGFX(xe) is true), does
xe_bo_move() still end up stalling synchronously on the newly queued flush
fences?

When evicting a BO from XE_PL_TT to XE_PL_SYSTEM, xe_bo_move() calls
xe_bo_move_notify(bo, ctx) and then immediately waits on
DMA_RESV_USAGE_BOOKKEEP before checking handle_system_ccs or scheduling a
migration copy:

drivers/gpu/drm/xe/xe_bo.c:xe_bo_move() {
	if (!move_lacks_source && !xe_bo_is_pinned(bo)) {
		ret = xe_bo_move_notify(bo, ctx);
		if (ret)
			goto out;
	}

	if (old_mem_type == XE_PL_TT &&
	    new_mem->mem_type == XE_PL_SYSTEM) {
		long timeout = dma_resv_wait_timeout(ttm_bo->base.resv,
						     DMA_RESV_USAGE_BOOKKEEP,
						     false,
						     MAX_SCHEDULE_TIMEOUT);
}

Because DMA_RESV_USAGE_BOOKKEEP is a superset of DMA_RESV_USAGE_KERNEL, won't
this wait in xe_bo_move() still block the calling thread synchronously (and
uninterruptibly, with intr = false instead of ctx->interruptible) on both all
in-flight GPU work and the async TLB invalidation jobs just queued by
xe_vm_flush_vm_bo_tlb_async()?

> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index dfbabfb19cbba..6c8164fc7c200 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
[ ... ]
> @@ -4582,6 +4583,130 @@ int xe_vm_invalidate_vma(struct xe_vma *vma)
>  	return ret;
>  }
>  
> +/*
> + * xe_vma_tlb_flush_client - Queue an async TLB flush for one VMA on one client
> + *
> + * Create and push a TLB invalidation job on the tile migrate (kernel) exec
> + * queue covering @vma's range, depending on @dep (the BO's in-flight GPU work)
> + * so the flush only fires once the GPU is done with the current mapping. The
> + * job's completion fence is installed into @resv as a KERNEL fence so the
> + * subsequent migration waits on the flush. No PTEs are zapped; this only
> + * flushes L2 via the TLB invalidation.
> + */
> +static int xe_vma_tlb_flush_client(struct xe_vm *vm, struct xe_vma *vma,
> +				   struct xe_tile *tile, struct xe_gt *gt,
> +				   struct dma_resv *resv, struct dma_fence *dep,
> +				   int type)
> +{
> +	struct xe_exec_queue *q = xe_migrate_exec_queue(tile->migrate);
> +	struct xe_tlb_inval_job *job;
> +	struct dma_fence *fence;
> +	int err;
> +
> +	job = xe_tlb_inval_job_create(q, &gt->tlb_inval,
> +				      q->tlb_inval[type].dep_scheduler, vm,
> +				      xe_vma_start(vma), xe_vma_end(vma), type);
> +	if (IS_ERR(job))
> +		return PTR_ERR(job);
> +
> +	err = xe_tlb_inval_job_alloc_dep(job);
> +	if (err)
> +		goto out_put;
> +
> +	err = dma_resv_reserve_fences(resv, 1);
> +	if (err)
> +		goto out_put;
> +
> +	/* Cannot fail; takes its own ref on @dep and returns a referenced fence. */
> +	fence = xe_tlb_inval_job_push(job, dep);

[Severity: Critical]
Is xe_migrate_exec_queue(tile->migrate) the right exec queue to pass to
xe_tlb_inval_job_create() and xe_tlb_inval_job_push() here, or should this be
using xe_cpu_bind_queue(vm->xe->cpu_bind)?

Inside xe_tlb_inval_job_push(), job arming, pushing, and updating last_fence
are serialized by xe_cpu_bind_job_lock():

drivers/gpu/drm/xe/xe_tlb_inval_job.c:xe_tlb_inval_job_push() {
	...
	xe_cpu_bind_job_lock(cpu_bind, job->q);
	...
	drm_sched_job_arm(&job->dep.drm);
	dma_fence_get(&job->dep.drm.s_fence->finished);
	drm_sched_entity_push_job(&job->dep.drm);

	/* Let the upper layers fish this out */
	xe_exec_queue_tlb_inval_last_fence_set(job->q, job->vm,
					       &job->dep.drm.s_fence->finished,
					       job->idx);

	xe_cpu_bind_job_unlock(cpu_bind, job->q);
}

And xe_cpu_bind_job_lock() checks whether q is cpu_bind->q:

drivers/gpu/drm/xe/xe_cpu_bind.c:xe_cpu_bind_job_lock() {
	bool is_cpu_bind = is_cpu_bind_queue(cpu_bind, q);

	if (is_cpu_bind)
		mutex_lock(&cpu_bind->job_mutex);
	else
		xe_vm_assert_held(q->user_vm);	/* User queues VM's should be locked */
}

Because tile->migrate->q is a separate queue from xe->cpu_bind->q and has
q->user_vm == NULL:

1. With CONFIG_PROVE_LOCKING enabled, is_cpu_bind is false, so
   xe_vm_assert_held(q->user_vm) dereferences a NULL q->user_vm pointer.
   In addition, xe_exec_queue_tlb_inval_last_fence_set() calls
   xe_exec_queue_last_fence_lockdep_assert() -> xe_cpu_bind_job_lock_assert(),
   which triggers xe_assert(xe, q == cpu_bind->q) and
   lockdep_assert_held(&cpu_bind->job_mutex).

2. With CONFIG_PROVE_LOCKING disabled, xe_cpu_bind_job_lock() is a no-op for
   tile->migrate->q. Since xe_vm_flush_vm_bo_tlb_async() only holds the per-BO
   dma-resv lock, concurrent evictions of BOs with distinct dma-resv locks can
   call drm_sched_job_arm(), drm_sched_entity_push_job(), and
   xe_exec_queue_tlb_inval_last_fence_set() concurrently on the shared
   tile->migrate->q without any lock, corrupting the drm_sched_entity SPSC
   queue and racing on q->tlb_inval[idx].last_fence.

[Severity: High]
Also, is it safe to call xe_vma_tlb_flush_client() and
xe_tlb_inval_job_alloc_dep() from the shrinker under fs_reclaim?

On L2-flush-optimized platforms, __xe_shrinker_walk() walks XE_PL_TT BOs
during slab shrinking (xe_shrinker_scan() and xe_shrinker_scan_purgeable())
and calls xe_bo_shrink() -> xe_bo_move_notify() -> xe_bo_trigger_rebind() ->
xe_vm_flush_vm_bo_tlb_async().

Within this path, dma_resv_get_singleton(), xe_tlb_inval_job_create(),
dma_resv_reserve_fences(), and xe_tlb_inval_job_alloc_dep() perform GFP_KERNEL
allocations, and xe_tlb_inval_job_alloc_dep() explicitly asserts
might_alloc(GFP_KERNEL):

drivers/gpu/drm/xe/xe_tlb_inval_job.c:xe_tlb_inval_job_alloc_dep() {
	xe_assert(gt_to_xe(job->q->gt), !xa_load(&job->dep.drm.dependencies, 0));
	might_alloc(GFP_KERNEL);

	return drm_sched_job_add_dependency(&job->dep.drm,
					    dma_fence_get_stub());
}

Won't reaching this from the shrinker while fs_reclaim is held trigger a
lockdep recursive-reclaim warning and risk recursive memory reclaim deadlocks?

[ ... ]
> +		for_each_tile(tile, xe, id) {
> +			if (!(vma->tile_present & BIT(id)))
> +				continue;
> +
> +			err = xe_vma_tlb_flush_client(vm, vma, tile,
> +						      tile->primary_gt, resv, dep,
> +						      XE_EXEC_QUEUE_TLB_INVAL_PRIMARY_GT);
> +			if (err)
> +				goto out;
> +
> +			if (tile->media_gt) {
> +				err = xe_vma_tlb_flush_client(vm, vma, tile,
> +							      tile->media_gt, resv, dep,
> +							      XE_EXEC_QUEUE_TLB_INVAL_MEDIA_GT);
> +				if (err)
> +					goto out;
> +			}
> +		}

[Severity: High]
On multi-tile devices, shouldn't the index passed to xe_vma_tlb_flush_client()
be offset by the tile ID?

In alloc_dep_schedulers(), q->tlb_inval[] is populated across all tiles and
GTs as a flat array indexed by i = id * (XE_EXEC_QUEUE_TLB_INVAL_MEDIA_GT + 1)
+ j:

drivers/gpu/drm/xe/xe_exec_queue.c:alloc_dep_schedulers() {
	for_each_tile(tile, xe, id) {
		for (j = 0; j < (XE_EXEC_QUEUE_TLB_INVAL_MEDIA_GT + 1); ++j, ++i) {
			...
			if (j == XE_EXEC_QUEUE_TLB_INVAL_PRIMARY_GT)
				gt = tile->primary_gt;
			else
				gt = tile->media_gt;

			if (!gt)
				continue;
			...
			q->tlb_inval[i].dep_scheduler = dep_scheduler;
		}
	}
}

Passing the unshifted XE_EXEC_QUEUE_TLB_INVAL_PRIMARY_GT (0) and
XE_EXEC_QUEUE_TLB_INVAL_MEDIA_GT (1) for every tile causes tiles with id > 0
to use tile 0's dep_scheduler and overwrite tile 0's last_fence slots (and if
tile 0 has no media GT while a higher tile does, q->tlb_inval[1].dep_scheduler
will be NULL). Should this compute the index using the tile ID in the same way
as xe_pt_update_ops_run()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104221459.1534188-1-matthew.brost@intel.com?part=1