Re: [PATCH v2 4/5] drm/gpusvm: move struct drm_gpusvm_pages out of struct drm_gpusvm_range

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Concurrent read-modify-write on `pages[i].flags.unmapped` bitfield causes a data race with lockless readers.
--

commit 48046c418df88520e7f847caa854ce19133469f6
Author: Honglei Huang <honghuan@amd.com>

drm/gpusvm: move struct drm_gpusvm_pages out of struct drm_gpusvm_range

This commit abstracts the tracking of physical pages from the MM VA range
in the DRM GPU SVM framework. It removes the embedded pages object from
drm_gpusvm_range, transferring the management of device physical pages
directly to the driver layer using drm_gpusvm_pages helpers.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 958cb605aedd5..bb186719b17c4 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c

[ ... ]

> @@ -1799,20 +1799,28 @@ EXPORT_SYMBOL_GPL(drm_gpusvm_has_mapping);
>  /**
>   * drm_gpusvm_range_set_unmapped() - Mark a GPU SVM range as unmapped
>   * @range: Pointer to the GPU SVM range structure.
> + * @pages: Pointer to the GPU SVM pages structure(s).
> + * @pages_count: Number of GPU SVM pages structure(s) passed in.
>   * @mmu_range: Pointer to the MMU notifier range structure.
>   *
>   * This function marks a GPU SVM range as unmapped and sets the partial_unmap flag
>   * if the range partially falls within the provided MMU notifier range.
>   */
>  void drm_gpusvm_range_set_unmapped(struct drm_gpusvm_range *range,
> +				   struct drm_gpusvm_pages *pages,
> +				   unsigned int pages_count,
>  				   const struct mmu_notifier_range *mmu_range)
>  {
> +	unsigned int i;
> +
>  	lockdep_assert_held_write(&range->gpusvm->notifier_lock);
>  
> -	range->pages.flags.unmapped = true;
> +	range->flags.unmapped = true;
> +	for (i = 0; i < pages_count; ++i)
> +		pages[i].flags.unmapped = true;

[Severity: Medium]
Could this bitfield assignment introduce a regression by causing a data race
with lockless readers?

Since unmapped shares a 16-bit union (__flags) with has_devmem_pages and
has_dma_mapping, the C compiler implements this as a read-modify-write on
the entire 16-bit word.

Meanwhile, lockless readers like xe_svm_range_in_vram() in xe_svm.c read
this word concurrently:

xe_svm_range_in_vram() {
    struct drm_gpusvm_pages_flags flags = {
        /* Pairs with WRITE_ONCE in drm_gpusvm.c */
        .__flags = READ_ONCE(range->pages.flags.__flags),
    };
    ...
}

Does this unmarked RMW conflict with the lockless READ_ONCE(), potentially
allowing compiler load/store tearing? This regression might lead to KCSAN data
race splats or stale reads of has_devmem_pages.

To satisfy the C memory model, should writers use
WRITE_ONCE(pages[i].flags.__flags, new_flags.__flags) as is already done
in drm_gpusvm_get_pages()?

>  	if (drm_gpusvm_range_start(range) < mmu_range->start ||
>  	    drm_gpusvm_range_end(range) > mmu_range->end)
> -		range->pages.flags.partial_unmap = true;
> +		range->flags.partial_unmap = true;
>  }
>  EXPORT_SYMBOL_GPL(drm_gpusvm_range_set_unmapped);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617103602.1491609-1-honglei1.huang@amd.com?part=4