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

["Yadav, Arvind" <arvind.yadav@intel.com>]

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


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.

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