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. > @@ -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? > return; > + } > > if (evict) > drm_gpuvm_bo_list_add(vm_bo, evict, lock); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
