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
