Re: [PATCH v4 3/5] drm/xe: Leave the VM bulk move on BO release, not on GEM close

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/xe: bulk_move detach in xe_ttm_bo_release_notify() violates TTM invariants
--

commit 5d85520c1e2e507cc032c4248bd48c50a734e710
Author: Matthew Brost <matthew.brost@intel.com>

drm/xe: Leave the VM bulk move on BO release, not on GEM close

This commit removes xe_gem_object_close() and moves the detachment of a
VM-private BO from its VM's LRU bulk move into xe_ttm_bo_release_notify()
so that BOs outliving their GEM handles remain part of the VM bulk move
until release.

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 42a6aa1a2e4a0..ba16b260dbcb7 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -1869,6 +1869,14 @@ static void xe_ttm_bo_release_notify(struct ttm_buffer_object *ttm_bo)
>  	if (!xe_ttm_bo_lock_in_destructor(ttm_bo))
>  		return;
>  
> +	/*
> +	 * Leave the VM's bulk move, which is only kept alive by the VM, and
> +	 * whose LRU position no longer says anything about a BO on its way
> +	 * out.
> +	 */
> +	if (ttm_bo->bulk_move)
> +		ttm_bo_set_bulk_move(ttm_bo, NULL);

[Severity: High]
Does detaching from bulk_move in xe_ttm_bo_release_notify() happen too late
in the TTM teardown sequence?

When the final reference to a VM-private user BO on a non-fault-mode VM is
dropped via xe_gem_object_free() -> ttm_bo_fini() -> ttm_bo_put() ->
ttm_bo_release(), ttm_bo_release() checks bo->bulk_move and individualizes
bo->base.resv before invoking bdev->funcs->release_notify():

drivers/gpu/drm/ttm/ttm_bo.c:ttm_bo_release() {
    ...
	WARN_ON_ONCE(bo->pin_count);
	WARN_ON_ONCE(bo->bulk_move);

	if (!bo->deleted) {
		ret = ttm_bo_individualize_resv(bo);
		if (ret) {
			/* Last resort, if we fail to allocate memory for the
			 * fences block for the BO to become idle
			 */
			dma_resv_wait_timeout(bo->base.resv,
					      DMA_RESV_USAGE_BOOKKEEP, false,
					      30 * HZ);
		}

		if (bdev->funcs->release_notify)
			bdev->funcs->release_notify(bo);
    ...
}

Because bo->bulk_move is still set upon entering ttm_bo_release(), releasing
such a BO will immediately trigger WARN_ON_ONCE(bo->bulk_move).

In addition, ttm_bo_individualize_resv() switches bo->base.resv away from
xe_vm_resv(vm) to &bo->base._resv under bo->bdev->lru_lock and then drops
lru_lock while the BO's resource is still linked in vm->lru_bulk_move:

drivers/gpu/drm/ttm/ttm_bo.c:ttm_bo_individualize_resv() {
    ...
		spin_lock(&bo->bdev->lru_lock);
		bo->base.resv = &bo->base._resv;
		spin_unlock(&bo->bdev->lru_lock);
    ...
}

In the window between ttm_bo_individualize_resv() and
ttm_bo_set_bulk_move(ttm_bo, NULL) in xe_ttm_bo_release_notify(), another
thread holding xe_vm_resv(vm) can access vm->lru_bulk_move while the dying
BO is still pos->first or pos->last:

- Calling xe_vm_bump_lru() -> ttm_lru_bulk_move_tail() will fail
  dma_resv_assert_held(pos->first->bo->base.resv) or
  dma_resv_assert_held(pos->last->bo->base.resv) in
  drivers/gpu/drm/ttm/ttm_resource.c:

drivers/gpu/drm/ttm/ttm_resource.c:ttm_lru_bulk_move_tail() {
    ...
			lockdep_assert_held(&pos->first->bo->bdev->lru_lock);
			dma_resv_assert_held(pos->first->bo->base.resv);
			dma_resv_assert_held(pos->last->bo->base.resv);
    ...
}

- Adding or moving another resource in the same bucket via
  ttm_resource_add_bulk_move() -> ttm_lru_bulk_move_add() will trigger
  WARN_ON(pos->first->bo->base.resv != res->bo->base.resv):

drivers/gpu/drm/ttm/ttm_resource.c:ttm_lru_bulk_move_add() {
    ...
	} else {
		WARN_ON(pos->first->bo->base.resv != res->bo->base.resv);
		ttm_lru_bulk_move_pos_tail(pos, res);
	}
}

Conversely, if ttm_bo_individualize_resv() fails (for example, if
dma_resv_copy_fences() returns -ENOMEM), bo->base.resv remains
xe_vm_resv(vm), while xe_ttm_bo_lock_in_destructor() only trylocks
&ttm_bo->base._resv. Calling ttm_bo_set_bulk_move(ttm_bo, NULL) in
xe_ttm_bo_release_notify() will then fail dma_resv_assert_held(bo->base.resv)
in ttm_bo_set_bulk_move() and ttm_resource_is_swapped(), and mutate
vm->lru_bulk_move without holding xe_vm_resv(vm).

Should bulk_move be cleared before ttm_bo_release() runs (or before
ttm_bo_individualize_resv() detaches bo->base.resv from the VM reservation
object) while holding the VM reservation lock?

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