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 12:01:33PM -0700, Matthew Brost wrote:
> 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.
> 

So it probably is;

 - If in xe_gem_object_close if bind count == 0, remove from bulk
 - If in a bind operation, bind count == 0 and gem handle closed, remove from bulk

I will do this part in an independent follow up.

Matt

> 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