Re: [PATCH V17 05/12] drm/xe: Guard teardown paths against purged BOs

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Calling xe_bo_lock() in xe_pt_destroy() causes a recursive self-deadlock because the page table BO shares the VM's dma_resv lock, which is already held by callers.
- [Medium] The patch assumes that pinned kernel BOs (Page Table, Exec Queue) can be purged, adding dead code guards. Pinned kernel BOs can never be purged.
--

--- Patch [5]: [PATCH V17 05/12] drm/xe: Guard teardown paths against purged BOs ---
commit 77ed38896aea34c9e5dc27cf110f773e0afaba91
Author: Tejas Upadhyay <[email protected]>
Subject: drm/xe: Guard teardown paths against purged BOs

This commit adds guards using xe_bo_is_purged() to teardown and update
paths. It prevents unpinning or mapping a BO that has already been
purged during VRAM page offlining.

> diff --git a/drivers/gpu/drm/xe/xe_bo.h b/drivers/gpu/drm/xe/xe_bo.h
> index eede678ad303a..dfcd0e57073be 100644
> --- a/drivers/gpu/drm/xe/xe_bo.h
> +++ b/drivers/gpu/drm/xe/xe_bo.h
> @@ -364,7 +364,8 @@ static inline void xe_bo_unpin_map_no_vm(struct xe_bo *bo)
>  {
>  	if (likely(bo)) {
>  		xe_bo_lock(bo, false);
> -		xe_bo_unpin(bo);
> +		if (!xe_bo_is_purged(bo))
> +			xe_bo_unpin(bo);
>  		xe_bo_unlock(bo);

[Severity: Medium]
Will this condition ever be true? xe_bo_unpin_map_no_vm() is used exclusively
for kernel BOs, which cannot be purged. Since kernel BOs cannot transition to
the purged state, this guard appears to be dead code.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index d6c7f346f49bc..5432710c1cfb6 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -1575,8 +1575,12 @@ void xe_exec_queue_update_run_ticks(struct xe_exec_queue *q)
>  	 * errors.
>  	 */
>  	lrc = q->lrc[0];
> -	new_ts = xe_lrc_update_timestamp(lrc, &old_ts);
> -	q->xef->run_ticks[q->class] += (new_ts - old_ts) * q->width;
> +	xe_bo_lock(lrc->bo, false);
> +	if (!xe_bo_is_purged(lrc->bo)) {

[Severity: Medium]
Since LRC BOs are permanently pinned kernel BOs, is it possible for them to
ever be purged? If kernel BOs are never purged, this check and the new
locking operations are unnecessary dead code.

> +		new_ts = xe_lrc_update_timestamp(lrc, &old_ts);
> +		q->xef->run_ticks[q->class] += (new_ts - old_ts) * q->width;
> +	}
> +	xe_bo_unlock(lrc->bo);
>  
>  	drm_dev_exit(idx);
>  }
> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> index 5d990c1c3740a..b6575a6bada44 100644
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
> @@ -212,7 +212,10 @@ void xe_pt_destroy(struct xe_pt *pt, u32 flags, struct llist_head *deferred)
>  		return;
>  
>  	XE_WARN_ON(!list_empty(&pt->bo->ttm.base.gpuva.list));
> -	xe_bo_unpin(pt->bo);
> +	xe_bo_lock(pt->bo, false);

[Severity: Critical]
Does calling xe_bo_lock() here cause a recursive self-deadlock?

When tearing down a VM, xe_vm_pt_destroy() asserts that the VM's lock is held
and then calls xe_pt_destroy(). The page table BO shares the VM's dma_resv
lock.

Because the lock is already held by the current thread, won't attempting to
acquire the non-recursive ww_mutex again here cause a guaranteed deadlock when
destroying or modifying page tables?

Additionally, as with the other kernel BOs, can a page table BO ever actually
be purged?

> +	if (!xe_bo_is_purged(pt->bo))
> +		xe_bo_unpin(pt->bo);
> +	xe_bo_unlock(pt->bo);
>  	xe_bo_put_deferred(pt->bo, deferred);
>  
>  	if (pt->level > 0 && pt->num_live) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5