Re: [PATCH 03/13] drm/xe: Drop queued page faults when device I/O is blocked

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

On Wed, Sep 02, 2026 at 10:19:22AM +0530, Yadav, Arvind wrote:
> 
> On 01-09-2026 02:13, Rodrigo Vivi wrote:
> > On Thu, Aug 27, 2026 at 03:47:51PM +0530, Arvind Yadav wrote:
> > > Page-fault work may still be queued when PCI error recovery starts or
> > > the device becomes permanently wedged. Servicing these faults can migrate
> > > memory or update page tables after device I/O has been blocked.
> > > 
> > > Check the device state before and after fault servicing to cover a reset
> > > racing with the worker. Drop the active fault and its chained faults
> > > without sending a hardware response, and invalidate the cached fault
> > > state.
> > > 
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Assisted-by: Claude:claude-opus-4-8
> > > Signed-off-by: Arvind Yadav <arvind.yadav@intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_pagefault.c | 40 +++++++++++++++++++++++++++++++
> > >   1 file changed, 40 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
> > > index 2e415995f067..f486165f2323 100644
> > > --- a/drivers/gpu/drm/xe/xe_pagefault.c
> > > +++ b/drivers/gpu/drm/xe/xe_pagefault.c
> > > @@ -592,6 +592,38 @@ static void xe_pagefault_save_to_vm(struct xe_device *xe, struct xe_pagefault *p
> > >   	xe_vm_put(vm);
> > >   }
> > > +static bool
> > > +xe_pagefault_drop_if_blocked(struct xe_pagefault_queue *pf_queue,
> > > +			     struct xe_pagefault_work *pf_work,
> > > +			     struct xe_pagefault *pf,
> > > +			     u64 *cache_start)
> > > +{
> > > +	struct xe_pagefault *next;
> > > +
> > > +	if (!xe_device_io_blocked(pf_work->xe))
> > > +		return false;
> > > +
> > > +	/*
> > > +	 * cache_start is private to this worker invocation. pf_work->cache is
> > > +	 * shared with fault producers and must be invalidated under the queue
> > > +	 * lock.
> > > +	 */
> > > +	xe_pagefault_cache_start_invalidate(*cache_start);
> > > +
> > > +	guard(spinlock_irq)(&pf_queue->lock);
> > > +
> > > +	xe_pagefault_cache_invalidate(pf_queue, pf_work);
> > > +
> > > +	while (pf) {
> > > +		next = pf->consumer.next;
> > > +		pf->consumer.next = NULL;
> > > +		pf->consumer.alloc_state = XE_PAGEFAULT_ALLOC_STATE_FREE;
> > > +		pf = next;
> > is there some helper we could re-use? I know we cannot use the pop directly,
> > because it would deadlock, but perhaps we can unify some code here...

Why would it deadlock... xe_device_io_blocked is just an couple of
atomics, right?

So why wouldn't this work to squash all faults.

diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
index 2e415995f067..1c9e5539b90d 100644
--- a/drivers/gpu/drm/xe/xe_pagefault.c
+++ b/drivers/gpu/drm/xe/xe_pagefault.c
@@ -477,6 +477,12 @@ static bool xe_pagefault_queue_pop(struct xe_pagefault_queue *pf_queue,
                lpf = xe_pagefault_queue_tail_fault(pf_queue);
                xe_pagefault_queue_advance(pf_queue);

+               if (xe_device_io_blocked(xe)) {
+                       lpf->consumer.alloc_state =
+                               XE_PAGEFAULT_ALLOC_STATE_FREE;
+                       continue;
+               }
+
                if (lpf->consumer.alloc_state !=
                    XE_PAGEFAULT_ALLOC_STATE_QUEUED)
                        continue;


> 
> 
> Noted,
> 
> > 
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > > +
> > >   static void xe_pagefault_queue_work(struct work_struct *w)
> > >   {
> > >   	struct xe_pagefault_work *pf_work =
> > > @@ -615,6 +647,10 @@ static void xe_pagefault_queue_work(struct work_struct *w)
> > >   		int err = 0;
> > >   		bool invalidated = false;
> > > +		if (xe_pagefault_drop_if_blocked(pf_queue, pf_work, pf,
> > > +						 &cache_start))
> > > +			continue;
> > do we really need to loop inside the loop or only check and let this main loop goes?
> 
> 
> Yes, The inner walk is needed. Chained faults have already been removed from
> the queue and linked through consumer.next. The outer pop loop will not see
> them.
> 
> > 
> > > +
> > >   		/* Last fault same address, ack immediately */
> > >   		if (xe_pagefault_match(pf, cache_start, cache_end, cache_asid)) {
> > >   			xe_gt_stats_incr(gt, XE_GT_STATS_ID_LAST_PAGEFAULT_COUNT, 1);
> > > @@ -623,6 +659,10 @@ static void xe_pagefault_queue_work(struct work_struct *w)
> > >   		err = xe_pagefault_service(pf);
> > > +		if (xe_pagefault_drop_if_blocked(pf_queue, pf_work, pf,
> > > +						 &cache_start))
> > do we really need to check after?
> 
> 
> Yes. The device can enter reset or become wedged while
> xe_pagefault_service() is running. The first check alone does not cover that
> race.
> 

But this is still toctou - really everything in patch is as immediately
after either check the state can change. I can't say I'm a fan of any
weak checks in this code...

But just for thought: after this point all we do is CT which should be
down and we'd drop it there before touching the hardware.

Matt

> > 
> > > +			continue;
> > > +
> > >   		if (err) {
> > now this err far from where it is collected is pretty bad...
> 
> 
> Agreed. i will keeps err next to its use.
> 
> Thanks,
> Arvind
> 
> > 
> > >   			if (!(pf->consumer.access_type & XE_PAGEFAULT_ACCESS_PREFETCH)) {
> > >   				xe_pagefault_save_to_vm(gt_to_xe(gt), pf);
> > > -- 
> > > 2.43.0
> > >