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

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Oct 27, 2026 at 06:47:57PM +0000, sashiko-bot@kernel.org wrote:
> 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]

Ugh, this is correct - this patch doesn't work but the code prior to
this patch is also incorrect.

I'm not immediately spoting an obvisous solution, so I will just drop
this patch for now.

Matt

> 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