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

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Jan 29, 2026 at 04:39:37PM +0100, Thomas Hellström wrote:
> 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.
> 

This is a good point. We might need to rethink this one. Would
GFP_NOWAIT be better here? Also as you have hit on - only the allocation
path can fail which is handled gracefully (i.e., the shrinker path
doesn't rely on allocations).

> 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.

The lock protects:

 - xe_sa_bo_swap_shadow through xe_sa_bo_sync_shadow, so this includes
   xe_bb_ccs_new as it picks the correct SA based on shadow state.

> 
> 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...."

This could be improved.

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

The vCPU is updating the shadow buffer which isn't programmed on the GPU
so if it interrupted mid-instruction writing, the GPU doesn't hang on a
partially written instruction.

Some back ground, I had suggested a AVX based solution to write out /
clear instructions solution [1] but it was rejected in favor of a shadow
buffer solution which appears to have lockdep issues.

Any ideas here would be helpful.

Matt

[1] https://patchwork.freedesktop.org/series/156482/

> 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);