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 11:35:44AM -0800, Matthew Brost wrote:
> 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.
> 

Thomas - since this is a Fixes patch should be start with this patch as
is (maybe s/ATOMIC/NOWAIT) for easy backporting then following patch in
the series does alloc() / init() split? I have feeling the split will a
fairly big diff given it will likely touch 4 layers.

Matt

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