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 07:30:01PM +0100, Thomas Hellström wrote:
> On Thu, 2026-01-29 at 10:03 -0800, Matthew Brost wrote:
> > 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 think if the lock is needed, the typical approach would be to split
> bb_new into alloc() outside the lock and init() inside the lock if
> needed. It looks like tlb_inval is using GFP_ATOMIC as well, so should
> be able to benefit. That touches drm code, though, but I think
> GFP_KERNEL should be used throughout unless we have a backup path or
> don't care about failures.
> 

+1. I talked this over with Thomas a agree spliting alloc() and init()
is the best solution + likely can use this going forward or in some
existing cases.

> > 
> > > 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.
> 
> So then basically all (or most) calls that touch a shadow-enabled sa
> manager need to be called under the lock?
> 

Correct. So I think the guards in the correct places.

Matt

> > 
> > > 
> > > 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.
> 
> Oh, so we're updating a buffer simultaneously executing on the GPU?
> 
> > 
> > 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.
> 
> I still don't fully get the flow, I must admit, and the interaction
> with the CPU-buffer we already have there on DGFX...
> 
> Thanks,
> Thomas
> 
> 
> > 
> > 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);