Re: [PATCH v6 16/24] drm/xe: Add CPU bind layer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent RB-tree corruption in `xe_range_fence_insert()` during PREFETCH operations.
- [High] Memory leak of `pt_job_ops` and deferred page table BOs when a PT job is dropped by the DRM scheduler.
--

commit dab33f2788d31662c4784993dd54e7509cc455e6
Author: Matthew Brost <matthew.brost@intel.com>
drm/xe: Add CPU bind layer

This commit introduces a dedicated CPU bind layer at the device level,
decoupling it from the migrate layer. It updates the PT layer to generate
a single bind job when pages are mirrored across tiles. The patch includes
a significant refactor to ensure functional equivalence.

Link: https://patch.msgid.link/20260228013501.106680-17-matthew.brost@intel.com

> diff --git a/drivers/gpu/drm/xe/xe_sched_job.c b/drivers/gpu/drm/xe/xe_sched_job.c
> --- a/drivers/gpu/drm/xe/xe_sched_job.c
> +++ b/drivers/gpu/drm/xe/xe_sched_job.c
> @@ -73,8 +73,9 @@ static void job_free(struct xe_sched_job *job)
>  	struct xe_exec_queue *q = job->q;
>  	bool is_migration = xe_sched_job_is_migration(q);
>  
> -	kmem_cache_free(xe_exec_queue_is_parallel(job->q) || is_migration ?
> -			xe_sched_job_parallel_slab : xe_sched_job_slab, job);
> +	kmem_cache_free(job->is_pt_job || xe_exec_queue_is_parallel(job->q) ||
> +			is_migration ? xe_sched_job_parallel_slab :
> +			xe_sched_job_slab, job);
>  }

[Severity: High]
This is a pre-existing issue, but does dropping a PT job leak the pt_job_ops
and its deferred page table BOs?

When a page table update job is created (e.g. in
xe_cpu_bind_update_pgtables_job()), a reference to pt_job_ops is acquired via
xe_pt_job_ops_get(). This is normally released when the job executes in
guc_exec_queue_run_job().

However, if the execution queue is destroyed before execution, the DRM
scheduler aborts the job and calls the backend's .free_job callback
(guc_exec_queue_free_job()). That callback invokes job_free() which frees the
job memory, but fails to call xe_pt_job_ops_put() for PT jobs. This appears
to permanently leak the xe_pt_job_ops structure and the BOs it tracks.

> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
[ ... ]
> @@ -2821,67 +2824,61 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
[ ... ]
> -	if (xe_range_fence_insert(&vm->rftree[tile->id], rfence,
> +	if (xe_range_fence_insert(&vm->rftree, rfence,
>  				  &xe_range_fence_kfree_ops,
> -				  pt_update_ops->start,
> -				  pt_update_ops->last, fence))
> +				  vops->start, vops->last, fence))

[Severity: High]
This isn't a bug introduced by this patch, but does xe_range_fence_insert()
have a concurrent RB-tree corruption risk here?

In xe_vm_bind_ioctl(), if an operation does not modify the GPU VA (such as
DRM_XE_VM_BIND_OP_PREFETCH on non-userptr memory),
XE_VMA_OPS_FLAG_DOWNGRADE_LOCK is set and vm->lock is downgraded to a read
lock (downgrade_write(&vm->lock)).

When multiple prefetch binds on the same VM execute concurrently, they will
reach xe_pt_update_ops_run() and unconditionally call xe_range_fence_insert()
to track the page table update. Since xe_range_fence_insert() modifies the
RB-tree directly without any internal synchronization, this concurrent access
under a read lock could lead to structural corruption of the shared
vm->rftree.

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