Re: [PATCH 1/2] drm/xe/vf: Fix fs_reclaim warning with CCS save/restore BB allocation

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi.

On Thu, 2026-01-29 at 12:51 +0000, Satyanarayana K V P wrote:
> CCS save/restore batch buffers are attached during BO allocation and
> detached during BO teardown. The shrinker triggers xe_bo_move(),
> which is
> used for both allocation and deletion paths.
> 
> When BO allocation and shrinking occur concurrently, a circular
> locking
> dependency involving fs_reclaim and swap_guard can occur, leading to
> a
> deadlock such as:
> 
> ======================================================
> WARNING: possible circular locking dependency detected
> ------------------------------------------------------
> 
>       CPU0                    CPU1
>       ----                    ----
>  lock(fs_reclaim);
>                               lock(&sa_manager->swap_guard);
>                               lock(fs_reclaim);
>  lock(&sa_manager->swap_guard);
> 
>  *** DEADLOCK ***
> =====================================================
> 
> To avoid this, allocate CCS save/restore BB BOs using GFP_ATOMIC,
> preventing reclaim from being invoked in this context.
> 
> Fixes: 864690cf4dd62 ("drm/xe/vf: Attach and detach CCS copy commands
> with BO")
> Signed-off-by: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>
> Suggested-by: Matthew Brost <matthew.brost@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>

If shrinking and allocation is indeed happening concurrently, then
GFP_ATOMIC is highly likely to fail. In fact it shouldn't be used if we
can't gracefully recover from a failure or if the failure doesn't
matter at all, like in debugging cases where we can lose data.

I'm trying to wrap my head around the sa manager shadow approach and it
seems to me like external code putting the sa manager in a particular
state and the internal lock is accessed from outside the sa code with
no clearly defined locking rules / asserts? IMO it's a very odd and
fragile construct, in particular when used with guard() where it's
unclear exactly what scope is needing locking.

I'm trying to find some documentation to explain why this is used and
the only thing I can find is 

"Directly clearing the BB lacks atomicity and can lead to undefined
behavior if the vCPU is halted mid-operation...."

But what if the vCPU is halted during xe_sa_bo_sync_shadow()? What is
exactly is the atomicity in this case? 

Thanks,
Thomas


> ---
>  drivers/gpu/drm/xe/xe_bb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_bb.c b/drivers/gpu/drm/xe/xe_bb.c
> index 8b678297aaa2..355365625df9 100644
> --- a/drivers/gpu/drm/xe/xe_bb.c
> +++ b/drivers/gpu/drm/xe/xe_bb.c
> @@ -62,7 +62,7 @@ struct xe_bb *xe_bb_new(struct xe_gt *gt, u32
> dwords, bool usm)
>  struct xe_bb *xe_bb_ccs_new(struct xe_gt *gt, u32 dwords,
>  			    enum xe_sriov_vf_ccs_rw_ctxs ctx_id)
>  {
> -	struct xe_bb *bb = kmalloc(sizeof(*bb), GFP_KERNEL);
> +	struct xe_bb *bb = kmalloc(sizeof(*bb), GFP_ATOMIC);
>  	struct xe_device *xe = gt_to_xe(gt);
>  	struct xe_sa_manager *bb_pool;
>  	int err;
> @@ -78,7 +78,7 @@ struct xe_bb *xe_bb_ccs_new(struct xe_gt *gt, u32
> dwords,
>  	 */
>  
>  	bb_pool = xe->sriov.vf.ccs.contexts[ctx_id].mem.ccs_bb_pool;
> -	bb->bo = xe_sa_bo_new(bb_pool, 4 * (dwords + 1));
> +	bb->bo = __xe_sa_bo_new(bb_pool, 4 * (dwords + 1),
> GFP_ATOMIC);
>  
>  	if (IS_ERR(bb->bo)) {
>  		err = PTR_ERR(bb->bo);