Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [Critical] The SVM BO eviction path migrates VRAM to system memory synchronously without waiting for pending GPU operations (`ttm_bo_wait_ctx`), risking data corruption and violating `ctx->no_wait_gpu`. - [High] Using `GFP_KERNEL` allocations inside `drm_pagemap_evict_to_ram()` while holding the BO's `dma_resv` lock creates a deadlock risk with memory reclaim. - [High] Early return during SVM BO eviction bypasses `amdgpu_bo_move_notify()` and TTM BO statistics tracking. Pre-existing issues: - [High] `amdgpu_svm_copy_to_ram` completely ignores the `pre_migrate_fence`, risking data corruption on the destination. -- commit 9be611c0396613a1f2f1bba63eacdcd1d3dd9964 Author: Junhua Shen <[email protected]> drm/amdgpu: implement synchronous TTM eviction for SVM BOs Implement the TTM eviction path for SVM (Shared Virtual Memory) BOs, enabling VRAM overcommit scenarios where device-private pages must be migrated back to system memory when VRAM is full. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_migrate.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_migrate.c > index 3756404ec334c..88079221eeca7 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_migrate.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_migrate.c > @@ -645,6 +645,34 @@ static const struct drm_pagemap_devmem_ops > amdgpu_pagemap_ops = { > .copy_to_ram = amdgpu_svm_copy_to_ram, [Severity: High] This is a pre-existing issue, but does amdgpu_svm_copy_to_ram() ignore the pre_migrate_fence? Looking at the implementation of amdgpu_svm_copy_to_ram() referenced here: amdgpu_svm_copy_to_ram(struct page **pages, struct drm_pagemap_addr *pagemap_addr, unsigned long npages, struct dma_fence *pre_migrate_fence) { ... } The function takes pre_migrate_fence as a parameter but does not appear to wait on it. During P2P migration, could ignoring this fence cause the source device to begin copying data into the destination device's memory before the destination is ready, leading to data corruption? [ ... ] > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 6c6ab4dd6ea95..9d3deec7cedec 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c [ ... ] > @@ -575,6 +585,16 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, > bool evict, > abo->flags &= ~AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED; > } > > + /* SVM BO eviction: migrate device-private pages back to RAM */ > + if (amdgpu_svm_bo_is_svm(bo) && > + old_mem->mem_type == TTM_PL_VRAM && > + new_mem->mem_type == TTM_PL_SYSTEM) { > + r = amdgpu_svm_bo_evict(abo); [Severity: Critical] Does this migration skip synchronizing with pending GPU operations? Since amdgpu_svm_bo_evict() synchronously migrates VRAM to system memory via SDMA, missing a call to ttm_bo_wait_ctx() could allow the copy to start while the GPU is still executing shaders that access this VRAM. Additionally, does bypassing ttm_bo_wait_ctx() ignore the ctx->no_wait_gpu flag, potentially causing illegal stalls if a non-blocking move is requested? [Severity: High] Can this lead to a deadlock with memory reclaim? This path calls amdgpu_svm_bo_evict() while the TTM buffer object's dma_resv lock is held. This delegates to drm_pagemap_evict_to_ram(), which performs an allocation using GFP_KERNEL: drm_pagemap_evict_to_ram() { ... buf = kvcalloc(npages, 2 * sizeof(*src) + sizeof(*pagemap_addr) + sizeof(*pages), GFP_KERNEL); ... } Allocating with GFP_KERNEL while holding a dma_resv lock can invoke the shrinker (fs_reclaim). If the shrinker attempts to acquire dma_resv locks or waits on fences depending on the held lock, will this cause a circular lock dependency? > + if (!r) > + ttm_bo_move_null(bo, new_mem); > + return r; [Severity: High] Does returning early here bypass mandatory notifications and statistics updates? A successful eviction returns immediately, skipping the call to amdgpu_bo_move_notify() which handles VM tracking cleanup and tracepoints. It also entirely skips the statistics updates at the tail of amdgpu_bo_move(): amdgpu_bo_move() { ... if (evict) atomic64_inc(&adev->num_evictions); atomic64_add(bo->base.size, &adev->num_bytes_moved); return 0; } Will missing these updates cause underreporting of evictions and moved bytes, impacting driver debugging metrics and migration throttling? > + } > + > if (adev->mman.buffer_funcs_enabled && > ((old_mem->mem_type == TTM_PL_SYSTEM && > new_mem->mem_type == TTM_PL_VRAM) || -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=15
