Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] TOCTOU race on `fpriv->xcp_id` causes pool allocation mismatches and potential NUMA pool pollution. --
--- Patch [33]: [PATCH v3 33/33] drm/amdgpu: Preallocate system BO pages outside the reservation lock --- commit 4e5e075ea8b2b453d727a0f32343eda951ff2819 Author: Matthew Brost <[email protected]> drm/amdgpu: Preallocate system BO pages outside the reservation lock This commit introduces out-of-lock preallocation for pure system buffer objects to avoid stalling under the reservation lock. By mirroring the Xe driver's approach, it fills the full page backing up front before taking the lock. > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c [ ... ] > @@ -443,10 +448,57 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, > void *data, > flags |= AMDGPU_GEM_CREATE_NO_CPU_ACCESS; > } > > + /* > + * For system-only (pure GTT) BOs, preallocate the whole page backing > + * up front, outside the reservation lock. Populate under the lock then > + * just installs these pages instead of reclaiming/compacting in the > + * critical section. Best-effort: a short fill falls back to the normal > + * in-lock allocation for the missing pages. > + */ > + if (args->in.domains == AMDGPU_GEM_DOMAIN_GTT) { > + struct ttm_resource_manager *gtt_man = > + ttm_manager_type(&adev->mman.bdev, TTM_PL_TT); > + int32_t xcp_id = adev->gmc.mem_partitions ? fpriv->xcp_id : 0; > + int32_t pool_id = amdgpu_ttm_tt_pool_id(adev, xcp_id); [ ... ] > @@ -455,7 +507,8 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void > *data, > retry: > r = amdgpu_gem_object_create(adev, size, args->in.alignment, > initial_domain, > - flags, ttm_bo_type_device, resv, &gobj, > fpriv->xcp_id + 1); > + flags, ttm_bo_type_device, resv, &gobj, > + fpriv->xcp_id + 1, prealloc_pool ? > &prealloc : NULL); [Severity: Medium] Is it possible for fpriv->xcp_id to change between these two reads? If a concurrent thread updates fpriv->xcp_id (for example, via context creation in amdgpu_xcp_select_scheds changing it from AMDGPU_XCP_NO_PARTITION to a specific partition), the first read could select the generic TTM pool for preallocation. The second read could then evaluate to a specific partition, initializing the BO with the partition-specific TTM pool. Could this mismatch result in pages allocated from the generic pool being consumed by the partition pool, potentially degrading NUMA locality or causing accounting mismatches? Would it be safer to read fpriv->xcp_id once into a local variable and pass that same value to both amdgpu_ttm_tt_pool_id() and amdgpu_gem_object_create()? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=33
