Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Missing fallback eviction on final `drm_pagemap_populate_mm()` failure leaves the SVM range in an unsupported mixed memory state. - [Medium] Non-atomic read of bitfield `migrate_devmem` races with concurrent `WRITE_ONCE` updates to the shared `__flags` 16-bit word. - [Medium] Silently ignoring the return value of `drm_gpusvm_range_evict()` masks critical errors like `-ENOMEM`. --
commit e6b94e0182e48c1aeaf810ada96dbc179693edad Author: Junhua Shen <[email protected]> drm/amdgpu: add SVM range migration helpers for drm_pagemap This commit adds per-range migration helpers for SVM VRAM migration via drm_pagemap. It implements functions to check migration capabilities, wrap RAM-to-VRAM migration with eviction retries, and check device memory support. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range_migrate.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range_migrate.c > new file mode 100644 > index 0000000000000..91dfd0420c001 > --- /dev/null > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_svm_range_migrate.c > @@ -0,0 +1,120 @@ [ ... ] > +static bool > +range_in_vram(struct drm_gpusvm_range *range) > +{ > + struct drm_gpusvm_pages_flags flags = { > + /* Pairs with WRITE_ONCE in drm_gpusvm_get_pages() */ > + .__flags = READ_ONCE(range->pages.flags.__flags), > + }; > + > + return flags.has_devmem_pages; > +} > + > +static bool > +range_needs_migrate_to_vram(struct drm_gpusvm_range *range) > +{ > + if (!range->pages.flags.migrate_devmem) [Severity: Medium] Since migrate_devmem is part of a bitfield that shares the 16-bit __flags word with dynamically updated flags like has_devmem_pages, does this direct access introduce a data race? Concurrent threads, such as MMU notifiers, update these flags using WRITE_ONCE(svm_pages->flags.__flags, ...). Under the C11 memory model, reading a bitfield non-atomically while an atomic write occurs to the same memory location can cause torn reads. Would it be better to use READ_ONCE() to read the entire flags word into a local variable before checking the bit, similar to how range_in_vram() does it above? > + return false; > + if (range_in_vram(range)) > + return false; > + > + return true; > +} [ ... ] > +int > +amdgpu_svm_range_migrate_to_vram(struct amdgpu_svm *svm, > + struct drm_gpusvm_range *range) > +{ > + struct drm_pagemap *dpagemap; > + unsigned long start, end; > + int ret, retries = 1; > + > + if (!range_needs_migrate_to_vram(range)) > + return 0; > + > + dpagemap = amdgpu_svm_get_dpagemap(svm); > + if (!dpagemap) > + return -ENODEV; > + > + start = drm_gpusvm_range_start(range); > + end = drm_gpusvm_range_end(range); > + > + do { > + ret = drm_pagemap_populate_mm(dpagemap, start, end, > + svm->gpusvm.mm, 0); > + > + if (ret == -EBUSY && retries) > + drm_gpusvm_range_evict(&svm->gpusvm, range); [Severity: Medium] Should we check the return value of drm_gpusvm_range_evict() here? If the eviction fails due to -ENOMEM when allocating the PFN array, or -ETIME from a timeout, the code will loop and retry the migration, which will fail again with -EBUSY. The caller would then receive -EBUSY instead of the true underlying error, masking the failure and misdirecting upper-layer error handling. > + } while (ret == -EBUSY && retries--); [Severity: High] What happens if the final retry of drm_pagemap_populate_mm() fails with -EBUSY when retries is 0? Since the loop exits without calling drm_gpusvm_range_evict(), the range is left partially migrated. AMDGPU SVM sets allow_mixed = false, so a subsequent amdgpu_svm_range_get_pages() on this range will return -EOPNOTSUPP. If the migration attributes later change to prefer system memory, the GPU fault handler will skip VRAM migration and directly call amdgpu_svm_range_get_pages(), which fails with -EOPNOTSUPP. The error handler then calls amdgpu_svm_range_evict(), but because has_devmem_pages is false (since it was never successfully mapped to the GPU), it returns early doing nothing. Could this create a permanent failure loop that breaks GPU access to the range? > + > + if (ret) { > + AMDGPU_SVM_TRACE("migrate_to_vram failed: ret=%d > [0x%lx-0x%lx]\n", > + ret, start, end); > + return ret; > + } > + > + return 0; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=17
