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

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

On Tue, Sep 01, 2026 at 10:30:41PM -0700, Matthew Brost wrote:
> 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;
> +               }
> +

^^^ This should actually go after the XE_PAGEFAULT_ALLOC_STATE_QUEUED check.

Matt

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