Re: [PATCH v3 33/33] drm/amdgpu: Preallocate system BO pages outside the reservation lock

[sashiko-bot@kernel.org]

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 <matthew.brost@intel.com>
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/20260711025619.2540575-1-matthew.brost@intel.com?part=33