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 <[email protected]> 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/[email protected]?part=4
