Re: [PATCH 1/3] drm/gpuvm: allow locking external objects in two passes

On Fri, Aug 14, 2026 at 07:51:07AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Low] Missing WRITE_ONCE() for vm_bo->evicted causes a data race with a newly introduced lockless reader.
> 
> Pre-existing issues:
> - [High] The xe driver bypasses drm_gpuvm_bo_evict() when clearing vm_bo->evicted, which permanently leaks the newly introduced num_evicted counter.
> --
> 
> --- Patch [1]: [PATCH 1/3] drm/gpuvm: allow locking external objects in two passes ---
> commit 8dc4616b6b0a651e19f9a8c0a513982ffa694e85
> Author: Matthew Brost <[email protected]>
> 
> drm/gpuvm: allow locking external objects in two passes
> 
> This patch introduces an optimization to drm_gpuvm by allowing a split
> in the drm_exec transaction. It locks private and evicted objects first,
> then locks resident external objects later in a second pass. This reduces
> the time locks are held for resident shared objects during migration.
> 
> > diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> > index d1c80ad3deadb..f6ec90640d372 100644
> > --- a/drivers/gpu/drm/drm_gpuvm.c
> > +++ b/drivers/gpu/drm/drm_gpuvm.c
> [ ... ]
> > @@ -1220,16 +1301,88 @@ drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
> >  }
> >  EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_vm);
> >  
> [ ... ]
> > +static bool
> > +drm_gpuvm_prepare_skip(struct drm_gpuvm_bo *vm_bo,
> > +		       enum drm_gpuvm_exec_pass pass)
> > +{
> > +	drm_gpuvm_pass_assert_held(vm_bo->vm, pass);
> > +
> > +	switch (pass) {
> > +	case DRM_GPUVM_EXEC_PASS_EARLY:
> > +		vm_bo->lock_skipped = !READ_ONCE(vm_bo->evicted);
> > +		break;
> > +	case DRM_GPUVM_EXEC_PASS_LATE:
> [ ... ]
> > @@ -1959,6 +2389,11 @@ EXPORT_SYMBOL_GPL(drm_gpuvm_bo_extobj_add);
> >   * @evict: indicates whether the object is evicted
> >   *
> >   * Adds a &drm_gpuvm_bo to or removes it from the &drm_gpuvm's evicted list.
> > + *
> > + * An external object of a %DRM_GPUVM_RESV_PROTECTED &drm_gpuvm is the
> > + * exception: the evicted list is protected by the GPUVM's common dma-resv
> > + * there, which this does not hold, so such an object is only accounted for
> > + * and is put on the list later, by drm_gpuvm_prepare_objects().
> >   */
> >  void
> >  drm_gpuvm_bo_evict(struct drm_gpuvm_bo *vm_bo, bool evict)
> > @@ -1966,6 +2401,7 @@ drm_gpuvm_bo_evict(struct drm_gpuvm_bo *vm_bo, bool evict)
> >  	struct drm_gpuvm *gpuvm = vm_bo->vm;
> >  	struct drm_gem_object *obj = vm_bo->obj;
> >  	bool lock = !drm_gpuvm_resv_protected(gpuvm);
> > +	bool was_evicted = vm_bo->evicted;
> >  
> >  	dma_resv_assert_held(obj->resv);
> >  	vm_bo->evicted = evict;
> 
> [Severity: Low]
> Does this write need to use WRITE_ONCE()?
> 
> This plain write pairs with the lockless READ_ONCE(vm_bo->evicted) added
> in drm_gpuvm_prepare_skip(). Without a WRITE_ONCE() here, KCSAN will report
> a data race.
> 

Correct, look preexisting though. Will fix.

> > @@ -1974,8 +2410,16 @@ drm_gpuvm_bo_evict(struct drm_gpuvm_bo *vm_bo, bool evict)
> >  	 * internal spinlocks, since in this case the evicted list is protected
> >  	 * with the VM's common dma-resv lock.
> >  	 */
> > -	if (drm_gpuvm_is_extobj(gpuvm, obj) && !lock)
> > +	if (drm_gpuvm_is_extobj(gpuvm, obj) && !lock) {
> > +		/*
> > +		 * Count them instead, so drm_gpuvm_needs_two_pass() can tell
> > +		 * whether any are evicted without walking the list. The
> > +		 * object's dma-resv is held, so the transition is stable.
> > +		 */
> > +		if (evict != was_evicted)
> > +			atomic_add(evict ? 1 : -1, &gpuvm->extobj.num_evicted);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but the xe driver manually clears
> the evicted flag (vm_bo->evicted = false) during validation in
> drivers/gpu/drm/xe/xe_vm.c:xe_gpuvm_validate() instead of using the
> drm_gpuvm_bo_evict() API.
> 
> Because drm_gpuvm_bo_evict() is bypassed for clearing the flag, this counter
> decrement will never execute for the xe driver. Does this mean the
> num_evicted counter will permanently leak, continuously incrementing and
> permanently forcing drm_gpuvm_needs_two_pass() to return true, thereby
> defeating the optimization?

Xe uses the drm_gpuvm_bo_evict API in the following patch when enabling
two-pass GPUVM locking. So correct, but non-issue in the context of the
series.

Matt

> 
> >  		return;
> > +	}
> >  
> >  	if (evict)
> >  		drm_gpuvm_bo_list_add(vm_bo, evict, lock);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1