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

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