Re: [PATCH v8 08/12] drm/xe: Chain page faults via queue-resident cache to avoid fault storms

On Wed, Jul 29, 2026 at 11:20:39AM -0700, Matthew Brost wrote:
> On Wed, Jul 29, 2026 at 02:31:38PM +0200, Francois Dugast wrote:
> > Hi,
> > 
> > Patch size makes review a bit longer but I cannot suggest a good way to break
> > it down.
> > 
> > On Fri, Jul 24, 2026 at 04:25:57PM -0700, Matthew Brost wrote:
> > > Some Xe platforms can generate pagefault storms where many faults target
> > > the same address range in a short time window (e.g. many EU threads
> > > faulting the same page). The current worker/locking model effectively
> > > serializes faults for a given range and repeatedly performs VMA/range
> > > lookups for each fault, which creates head-of-queue blocking and wastes
> > > CPU in the hot path.
> > > 
> > > Introduce a page fault chaining cache that coalesces faults targeting
> > > the samr ASID and address range.
> > 
> > s/samr/same/
> > 
> 
> +1
> 
> > > 
> > > Each worker tracks the active fault range it is servicing. Fault entries
> > > reside in stable queue storage, allowing the IRQ handler to match new
> > > faults against the worker cache and directly chain cache hits onto the
> > > active entry without allocation or waiting for dequeue. Once the leading
> > > fault completes, the worker acknowledges the entire chain.
> > > 
> > > A small allocation state is added to each entry so queue, worker, and
> > > IRQ pathd can safely reference the same fault object. This prevents
> > 
> > s/pathd/paths/
> > 
> 
> +1
> 
> > > reuse while the fault is active and guarantees that chained faults
> > > remain valid until acknowledged.
> > > 
> > > Fault handlers also record the serviced range so subsequent faults can
> > > be acknowledged without re-running the full resolution path.
> > > 
> > > This removes repeated fault resolution during fault storms and
> > > significantly improves forward progress in SVM workloads.
> > > 
> > > Since threaded prefetches now use a dedicated prefetch workqueue
> > > (usm.prefetch_wq) rather than sharing the page fault workqueue, page
> > > fault servicing can no longer deadlock on vm->lock, so this cache does
> > > not need an -EAGAIN retry path for a failed vm->lock acquisition.
> > > 
> > > Assisted-by: ChatGPT:gpt-5 # Documentation
> > > Signed-off-by: Matthew Brost <[email protected]>
> > > Co-authored-by: Copilot <[email protected]>
> > 
> > Please remove this "Co-authored-by".
> > 
> > (Already mentioned in patch #3 but repeating here not to forget)
> > 
> 
> +1
> 
> > > ---
> > >  drivers/gpu/drm/xe/xe_pagefault.c       | 435 +++++++++++++++++++++---
> > >  drivers/gpu/drm/xe/xe_pagefault.h       |  71 ++++
> > >  drivers/gpu/drm/xe/xe_pagefault_types.h |  81 +++--
> > >  drivers/gpu/drm/xe/xe_svm.c             |  16 +-
> > >  drivers/gpu/drm/xe/xe_svm.h             |   9 +-
> > >  5 files changed, 527 insertions(+), 85 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
> > > index 65be8dd09e2f..b897698a4cfc 100644
> > > --- a/drivers/gpu/drm/xe/xe_pagefault.c
> > > +++ b/drivers/gpu/drm/xe/xe_pagefault.c
> > > @@ -35,6 +35,70 @@
> > >   * xe_pagefault.c implements the consumer layer.
> > >   */
> > >  
> > > +/**
> > > + * DOC: Xe page fault cache
> > > + *
> > > + * Some Xe hardware can trigger “fault storms,” which are many page faults to
> > > + * the same address within a short period of time. An example is many EU threads
> > > + * faulting on the same page simultaneously. With the current page fault locking
> > > + * structure, only one page fault for a given address range can be processed at
> > > + * a time. This causes head-of-queue blocking across workers, killing
> > > + * parallelism. If the page fault handler must repeatedly look up resources
> > > + * (VMAs, ranges) to determine that the pages are valid for each fault in the
> > > + * storm, the time complexity grows rapidly.
> > > + *
> > > + * To address this, each page fault worker maintains a cache of the active fault
> > > + * being processed. Subsequent faults that hit in the cache are chained to the
> > > + * pending fault, and all chained faults are acknowledged once the initial fault
> > > + * completes. This alleviates head-of-queue blocking and quickly chains faults
> > > + * in the upper layers, avoiding expensive lookups in the main fault-handling
> > > + * path.
> > > + *
> > > + * Faults are buffered in the page fault queue in a way that provides stable
> > > + * storage for outstanding faults. In particular, faults may be chained directly
> > > + * while still resident in the queue storage (i.e., outside the worker’s current
> > > + * head/tail dequeue position). This allows the IRQ handler to match newly
> > > + * arrived faults against the per-worker cache and immediately chain cache hits
> > > + * onto the active fault under the queue lock, without allocating memory or
> > > + * waiting for the worker to pop the fault first.
> > > + *
> > > + * A per-fault state field is used to assert correctness of these invariants.
> > > + * The state tracks whether an entry is free, queued, chained, or currently
> > > + * active. Transitions are performed under the page fault queue lock, and the
> > > + * worker acknowledges faults by walking the chain and returning entries to the
> > > + * free state once they are complete.
> > > + */
> > > +
> > > +/**
> > > + * enum xe_pagefault_alloc_state - lifetime state for a page fault queue entry
> > > + * @XE_PAGEFAULT_ALLOC_STATE_FREE:
> > > + *	Entry is unused and may be overwritten by the producer, consumer retry
> > > + *	or requeue..
> > > + * @XE_PAGEFAULT_ALLOC_STATE_QUEUED:
> > > + *	Entry has been enqueued and may be dequeued by a worker.
> > > + * @XE_PAGEFAULT_ALLOC_STATE_ACTIVE:
> > > + *	Entry has been dequeued and is the worker's currently serviced fault.
> > > + *	The worker may attach additional faults to it via consumer.next.
> > > + * @XE_PAGEFAULT_ALLOC_STATE_CHAINED:
> > > + *	Entry is not independently serviced; it has been chained onto an
> > > + *	ACTIVE entry via consumer.next and will be acknowledged when the
> > > + *	leading fault completes.
> > > + *
> > > + * The page fault queue provides stable storage for outstanding faults so the
> > > + * IRQ handler can chain new cache hits directly onto a worker's active fault.
> > > + * Because entries may remain referenced outside the consumer dequeue window,
> > > + * the producer must only write into entries in the FREE state.
> > > + *
> > > + * State transitions are protected by the page fault queue lock. Workers return
> > > + * entries to FREE after acknowledging the fault (either as ACTIVE or CHAINED).
> > > + */
> > > +enum xe_pagefault_alloc_state {
> > > +	XE_PAGEFAULT_ALLOC_STATE_FREE		= 0,
> > > +	XE_PAGEFAULT_ALLOC_STATE_QUEUED		= 1,
> > > +	XE_PAGEFAULT_ALLOC_STATE_CHAINED	= 2,
> > > +	XE_PAGEFAULT_ALLOC_STATE_ACTIVE		= 3,
> > > +};
> > > +
> > >  static int xe_pagefault_entry_size(void)
> > >  {
> > >  	/*
> > > @@ -77,7 +141,7 @@ static int xe_pagefault_begin(struct drm_exec *exec, struct xe_vma *vma,
> > >  }
> > >  
> > >  static int xe_pagefault_handle_vma(struct xe_gt *gt, struct xe_vma *vma,
> > > -				   bool atomic)
> > > +				   struct xe_pagefault *pf, bool atomic)
> > >  {
> > >  	struct xe_vm *vm = xe_vma_vm(vma);
> > >  	struct xe_tile *tile = gt_to_tile(gt);
> > > @@ -102,8 +166,11 @@ static int xe_pagefault_handle_vma(struct xe_gt *gt, struct xe_vma *vma,
> > >  
> > >  	/* Check if VMA is valid, opportunistic check only */
> > >  	if (xe_vm_has_valid_gpu_mapping(tile, vma->tile_present,
> > > -					vma->tile_invalidated) && !atomic)
> > > +					vma->tile_invalidated) && !atomic) {
> > > +		xe_pagefault_set_start_addr(pf, xe_vma_start(vma));
> > > +		xe_pagefault_set_end_addr(pf, xe_vma_end(vma));
> > >  		return 0;
> > > +	}
> > >  
> > >  	do {
> > >  		if (xe_vma_is_userptr(vma) &&
> > > @@ -141,6 +208,10 @@ static int xe_pagefault_handle_vma(struct xe_gt *gt, struct xe_vma *vma,
> > >  	} while (err == -EAGAIN);
> > >  
> > >  	if (!err) {
> > > +		/* Give hint to immediately ack faults */
> > > +		xe_pagefault_set_start_addr(pf, xe_vma_start(vma));
> > > +		xe_pagefault_set_end_addr(pf, xe_vma_end(vma));
> > > +
> > >  		dma_fence_wait(fence, false);
> > >  		dma_fence_put(fence);
> > >  	}
> > > @@ -208,10 +279,10 @@ static int xe_pagefault_service(struct xe_pagefault *pf)
> > >  	atomic = xe_pagefault_access_is_atomic(pf->consumer.access_type);
> > >  
> > >  	if (xe_vma_is_cpu_addr_mirror(vma))
> > > -		err = xe_svm_handle_pagefault(vm, vma, gt,
> > > +		err = xe_svm_handle_pagefault(vm, vma, pf, gt,
> > >  					      pf->consumer.page_addr, atomic);
> > >  	else
> > > -		err = xe_pagefault_handle_vma(gt, vma, atomic);
> > > +		err = xe_pagefault_handle_vma(gt, vma, pf, atomic);
> > >  
> > >  unlock_vm:
> > >  	up_read(&vm->lock);
> > > @@ -220,21 +291,221 @@ static int xe_pagefault_service(struct xe_pagefault *pf)
> > >  	return err;
> > >  }
> > >  
> > > -static bool xe_pagefault_queue_pop(struct xe_pagefault_queue *pf_queue,
> > > -				   struct xe_pagefault *pf)
> > > +#define XE_PAGEFAULT_CACHE_START_INVALID	U64_MAX
> > > +#define xe_pagefault_cache_start_invalidate(val)	\
> > > +	(val = XE_PAGEFAULT_CACHE_START_INVALID)
> > > +
> > > +static void
> > > +xe_pagefault_cache_invalidate(struct xe_pagefault_queue *pf_queue,
> > > +			      struct xe_pagefault_work *pf_work)
> > >  {
> > > -	bool found_fault = false;
> > > +	lockdep_assert_held(&pf_queue->lock);
> > > +
> > > +	xe_pagefault_cache_start_invalidate(pf_work->cache.start);
> > > +}
> > > +
> > > +static bool xe_pagefault_queue_full(struct xe_pagefault_queue *pf_queue)
> > > +{
> > > +	lockdep_assert_held(&pf_queue->lock);
> > > +
> > > +	return CIRC_SPACE(pf_queue->head, pf_queue->tail,
> > > +			  pf_queue->size) <= xe_pagefault_entry_size();
> > > +}
> > > +
> > > +static struct xe_pagefault *
> > > +xe_pagefault_queue_add(struct xe_pagefault_queue *pf_queue,
> > > +		       struct xe_pagefault *pf)
> > > +{
> > > +	struct xe_device *xe = container_of(pf_queue, typeof(*xe),
> > > +					    usm.pf_queue);
> > > +	struct xe_pagefault *lpf;
> > > +
> > > +	lockdep_assert_held(&pf_queue->lock);
> > > +
> > > +	do {
> > > +		/* Not possible, warn on and drop page fault */
> > > +		if (WARN_ON(xe_pagefault_queue_full(pf_queue)))
> > > +			return NULL;
> > >  
> > > -	spin_lock_irq(&pf_queue->lock);
> > > -	if (pf_queue->tail != pf_queue->head) {
> > > -		memcpy(pf, pf_queue->data + pf_queue->tail, sizeof(*pf));
> > > -		pf_queue->tail = (pf_queue->tail + xe_pagefault_entry_size()) %
> > > +		lpf = (pf_queue->data + pf_queue->head);
> > > +		pf_queue->head = (pf_queue->head + xe_pagefault_entry_size()) %
> > >  			pf_queue->size;
> > > -		found_fault = true;
> > > +	} while (lpf->consumer.alloc_state != XE_PAGEFAULT_ALLOC_STATE_FREE);
> > > +
> > > +	xe_assert(xe, lpf != pf);
> > > +	memcpy(lpf, pf, sizeof(*pf));
> > > +	lpf->consumer.alloc_state = XE_PAGEFAULT_ALLOC_STATE_QUEUED;
> > > +
> > > +	return lpf;
> > > +}
> > > +
> > > +static struct xe_pagefault *
> > > +xe_pagefault_queue_unchain_requeue(struct xe_pagefault_queue *pf_queue,
> > > +				   struct xe_pagefault *pf, struct xe_gt *gt)
> > > +{
> > > +	struct xe_device *xe = container_of(pf_queue, typeof(*xe),
> > > +					    usm.pf_queue);
> > > +	struct xe_pagefault *next = pf->consumer.next, *lpf;
> > > +
> > > +	lockdep_assert_held(&pf_queue->lock);
> > > +	xe_assert(xe, pf->consumer.alloc_state ==
> > > +		  XE_PAGEFAULT_ALLOC_STATE_CHAINED);
> > > +
> > > +	pf->consumer.alloc_state = XE_PAGEFAULT_ALLOC_STATE_FREE;
> > > +	lpf = xe_pagefault_queue_add(pf_queue, pf);
> > > +	if (lpf) {
> > > +		lpf->consumer.next = NULL;
> > > +		lpf->consumer.fault_type_level |= XE_PAGEFAULT_REQUEUE_MASK;
> > > +	}
> > > +
> > > +	return next;
> > > +}
> > > +
> > > +static bool xe_pagefault_match(struct xe_pagefault *pf, u64 start,
> > > +			       u64 end, u64 cache_asid)
> > > +{
> > > +	struct xe_device *xe = gt_to_xe(pf->gt);
> > > +	u64 page_addr = pf->consumer.page_addr;
> > > +	u32 pf_asid = pf->consumer.asid;
> > > +
> > > +	xe_assert(xe, pf->consumer.alloc_state !=
> > > +		  XE_PAGEFAULT_ALLOC_STATE_FREE);
> > > +
> > > +	return page_addr >= start && page_addr < end &&
> > > +		pf_asid == cache_asid;
> > > +}
> > 
> > Sashiko's comment here seems valid: access type should be taken into consideration
> > when evaluating if the page faults match (atomic).
> > 
> 
> I don't think that is a major concern. Generally, when we move memory,
> you end up with the highest permission level available. Atomics are an
> exception because we typically have to move the memory. We more or less
> always attempt a move, but we try much harder for atomics.
> 
> If, for some reason, we encounter a read or write fault, fail to move
> the memory, and then chain an atomic fault and acknowledgment, the
> worst-case outcome is that the atomic operation will fault again, and
> we'll handle it at that point.

Sounds reasonable, thanks for clarifying.

Francois

> 
> > > +
> > > +static bool xe_pagefault_try_chain(struct xe_pagefault_queue *pf_queue,
> > > +				   struct xe_pagefault *pf)
> > > +{
> > > +	struct xe_device *xe = container_of(pf_queue, typeof(*xe),
> > > +					    usm.pf_queue);
> > > +	struct xe_pagefault_work *pf_work;
> > > +	bool requeue = FIELD_GET(XE_PAGEFAULT_REQUEUE_MASK,
> > > +				 pf->consumer.fault_type_level);
> > > +	int i;
> > > +
> > > +	lockdep_assert_held(&pf_queue->lock);
> > > +	xe_assert(xe, pf->consumer.alloc_state ==
> > > +		  XE_PAGEFAULT_ALLOC_STATE_QUEUED);
> > > +
> > > +	/*
> > > +	 * If this is a retry, we may already have a chain attached. In that
> > > +	 * case, we cannot hit in the cache because chains cannot easily be
> > > +	 * combined.
> > > +	 */
> > > +	if (pf->consumer.next)
> > > +		return false;
> > > +
> > > +	for (i = 0, pf_work = xe->usm.pf_workers;
> > > +	     i < xe->info.num_pf_work; ++i, ++pf_work) {
> > > +		u64 start = pf_work->cache.start;
> > > +		u64 end = requeue ? start + SZ_4K : pf_work->cache.end;
> > > +		u32 asid = pf_work->cache.asid;
> > > +
> > > +		if (xe_pagefault_match(pf, start, end, asid)) {
> > > +			xe_assert(xe, pf_work->cache.pf->consumer.alloc_state ==
> > > +				  XE_PAGEFAULT_ALLOC_STATE_ACTIVE);
> > > +
> > > +			pf->consumer.alloc_state =
> > > +				XE_PAGEFAULT_ALLOC_STATE_CHAINED;
> > > +			pf->consumer.next = pf_work->cache.pf->consumer.next;
> > > +			pf_work->cache.pf->consumer.next = pf;
> > > +
> > > +			return true;
> > > +		}
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > > +
> > > +static void xe_pagefault_queue_advance(struct xe_pagefault_queue *pf_queue)
> > > +{
> > > +	lockdep_assert_held(&pf_queue->lock);
> > > +
> > > +	pf_queue->tail = (pf_queue->tail + xe_pagefault_entry_size()) %
> > > +		pf_queue->size;
> > > +}
> > > +
> > > +static struct xe_pagefault *
> > > +xe_pagefault_queue_tail_fault(struct xe_pagefault_queue *pf_queue)
> > > +{
> > > +	lockdep_assert_held(&pf_queue->lock);
> > > +
> > > +	return pf_queue->data + pf_queue->tail;
> > > +}
> > > +
> > > +static bool xe_pagefault_queue_empty(struct xe_pagefault_queue *pf_queue)
> > > +{
> > > +	lockdep_assert_held(&pf_queue->lock);
> > > +
> > > +	return pf_queue->head == pf_queue->tail;
> > > +}
> > > +
> > > +static bool xe_pagefault_queue_pop(struct xe_pagefault_queue *pf_queue,
> > > +				   struct xe_pagefault **pf, int id)
> > > +{
> > > +	struct xe_device *xe = container_of(pf_queue, typeof(*xe),
> > > +					    usm.pf_queue);
> > > +	struct xe_pagefault_work *pf_work;
> > > +	struct xe_pagefault *lpf;
> > > +	size_t align = SZ_2M;
> > > +
> > > +	guard(spinlock_irq)(&pf_queue->lock);
> > > +
> > > +	for (*pf = NULL; !*pf;) {
> > > +		if (xe_pagefault_queue_empty(pf_queue))
> > > +			return false;
> > > +
> > > +		lpf = xe_pagefault_queue_tail_fault(pf_queue);
> > > +		xe_pagefault_queue_advance(pf_queue);
> > > +
> > > +		if (lpf->consumer.alloc_state !=
> > > +		    XE_PAGEFAULT_ALLOC_STATE_QUEUED)
> > > +			continue;
> > > +
> > > +		if (xe_pagefault_try_chain(pf_queue, lpf))
> > > +			continue;
> > > +
> > > +		*pf = lpf;	/* Hand back page fault for processing */
> > > +	}
> > > +
> > > +	/*
> > > +	 * No cache hit; allocate a new cache entry. We assume most faults
> > > +	 * within a 2M range will hit the same pages. If this assumption proves
> > > +	 * false, the mismatched fault is requeued after the initial fault is
> > > +	 * acknowledged.
> > > +	 */
> > > +	pf_work = xe->usm.pf_workers + id;
> > > +	if (FIELD_GET(XE_PAGEFAULT_REQUEUE_MASK,
> > > +		      lpf->consumer.fault_type_level))
> > > +		align = SZ_4K;
> > > +	pf_work->cache.start = ALIGN_DOWN(lpf->consumer.page_addr, align);
> > > +	pf_work->cache.end = pf_work->cache.start + align;
> > > +	pf_work->cache.asid = lpf->consumer.asid;
> > > +	pf_work->cache.pf = lpf;
> > > +	lpf->consumer.alloc_state = XE_PAGEFAULT_ALLOC_STATE_ACTIVE;
> > > +
> > > +	/* Drain queue until empty or new fault found */
> > > +	while (1) {
> > > +		if (xe_pagefault_queue_empty(pf_queue))
> > > +			break;
> > > +
> > > +		lpf = xe_pagefault_queue_tail_fault(pf_queue);
> > > +
> > > +		if (lpf->consumer.alloc_state !=
> > > +		    XE_PAGEFAULT_ALLOC_STATE_QUEUED) {
> > > +			xe_pagefault_queue_advance(pf_queue);
> > > +			continue;
> > > +		}
> > > +
> > > +		if (!xe_pagefault_try_chain(pf_queue, lpf))
> > > +			break;
> > > +
> > > +		xe_pagefault_queue_advance(pf_queue);
> > >  	}
> > > -	spin_unlock_irq(&pf_queue->lock);
> > >  
> > > -	return found_fault;
> > > +	return true;
> > >  }
> > >  
> > >  static void xe_pagefault_print(struct xe_pagefault *pf)
> > > @@ -295,36 +566,83 @@ static void xe_pagefault_queue_work(struct work_struct *w)
> > >  		container_of(w, typeof(*pf_work), work);
> > >  	struct xe_device *xe = pf_work->xe;
> > >  	struct xe_pagefault_queue *pf_queue = &xe->usm.pf_queue;
> > > -	struct xe_pagefault pf;
> > > +	struct xe_pagefault *pf;
> > >  	ktime_t start = xe_gt_stats_ktime_get();
> > > -	struct xe_gt *gt = NULL;
> > >  	unsigned long threshold;
> > > +	u64 cache_start = XE_PAGEFAULT_CACHE_START_INVALID, cache_end = 0;
> > > +	u32 cache_asid = 0;
> > >  
> > >  #define USM_QUEUE_MAX_RUNTIME_MS      20
> > >  	threshold = jiffies + msecs_to_jiffies(USM_QUEUE_MAX_RUNTIME_MS);
> > >  
> > > -	while (xe_pagefault_queue_pop(pf_queue, &pf)) {
> > > -		int err;
> > > +	while (xe_pagefault_queue_pop(pf_queue, &pf, pf_work->id)) {
> > > +		struct xe_gt *gt = pf->gt;
> > > +		u32 asid = pf->consumer.asid;
> > > +		int err = 0;
> > > +		bool invalidated = false;
> > >  
> > > -		if (!pf.gt)	/* Fault squashed during reset */
> > > -			continue;
> > > +		/* Last fault same address, ack immediately */
> > > +		if (xe_pagefault_match(pf, cache_start, cache_end, cache_asid))
> > > +			goto ack_fault;
> > > +
> > > +		err = xe_pagefault_service(pf);
> > >  
> > > -		gt = pf.gt;
> > > -		err = xe_pagefault_service(&pf);
> > >  		if (err) {
> > > -			if (!(pf.consumer.access_type & XE_PAGEFAULT_ACCESS_PREFETCH)) {
> > > -				xe_pagefault_save_to_vm(gt_to_xe(pf.gt), &pf);
> > > -				xe_pagefault_print(&pf);
> > > -				xe_gt_info(pf.gt, "Fault response: Unsuccessful %pe\n",
> > > +			if (!(pf->consumer.access_type & XE_PAGEFAULT_ACCESS_PREFETCH)) {
> > > +				xe_pagefault_save_to_vm(gt_to_xe(gt), pf);
> > > +				xe_pagefault_cache_start_invalidate(cache_start);
> > > +				xe_pagefault_print(pf);
> > > +				xe_gt_info(pf->gt, "Fault response: Unsuccessful %pe\n",
> > >  					   ERR_PTR(err));
> > >  			} else {
> > > -				xe_gt_stats_incr(pf.gt, XE_GT_STATS_ID_INVALID_PREFETCH_PAGEFAULT_COUNT, 1);
> > > -				xe_gt_dbg(pf.gt, "Prefetch Fault response: Unsuccessful %pe\n",
> > > +				xe_gt_stats_incr(pf->gt, XE_GT_STATS_ID_INVALID_PREFETCH_PAGEFAULT_COUNT, 1);
> > > +				xe_gt_dbg(pf->gt, "Prefetch Fault response: Unsuccessful %pe\n",
> > >  					  ERR_PTR(err));
> > >  			}
> > > +		} else {
> > > +			/* Cache valid fault locally */
> > > +			cache_start = xe_pagefault_start_addr(pf);
> > > +			cache_end = xe_pagefault_end_addr(pf);
> > > +			cache_asid = asid;
> > >  		}
> > >  
> > > -		pf.producer.ops->ack_fault(&pf, err);
> > > +ack_fault:
> > > +		xe_assert(xe, pf->consumer.alloc_state ==
> > > +			  XE_PAGEFAULT_ALLOC_STATE_ACTIVE);
> > > +		xe_assert(xe, pf == pf_work->cache.pf);
> > > +
> > > +		while (pf) {
> > > +			xe_assert(xe, pf->consumer.alloc_state ==
> > > +				  XE_PAGEFAULT_ALLOC_STATE_ACTIVE);
> > > +
> > > +			pf->producer.ops->ack_fault(pf, err);
> > > +
> > > +			spin_lock_irq(&pf_queue->lock);
> > > +
> > > +			if (!invalidated) {
> > > +				invalidated = true;
> > > +				xe_pagefault_cache_invalidate(pf_queue,
> > > +							      pf_work);
> > > +			}
> > > +
> > > +			pf->consumer.alloc_state = XE_PAGEFAULT_ALLOC_STATE_FREE;
> > > +			pf = pf->consumer.next;
> > > +
> > > +			/*
> > > +			 * Requeue chained faults which do not match the last
> > > +			 * fault processed
> > > +			 */
> > > +			while (pf && !xe_pagefault_match(pf, cache_start,
> > > +							 cache_end, cache_asid))
> > > +				pf = xe_pagefault_queue_unchain_requeue(pf_queue, pf, gt);
> > > +
> > > +
> > 
> > Extra blank line.
> > 
> 
> Will fix.
> 
> > > +			/* Ensure resets are safe */
> > > +			if (pf)
> > > +				pf->consumer.alloc_state =
> > > +					XE_PAGEFAULT_ALLOC_STATE_ACTIVE;
> > > +			spin_unlock_irq(&pf_queue->lock);
> > > +		}
> > >  
> > >  		if (time_after(jiffies, threshold)) {
> > >  			queue_work(xe->usm.pagefault_wq, w);
> > > @@ -333,10 +651,8 @@ static void xe_pagefault_queue_work(struct work_struct *w)
> > >  	}
> > >  #undef USM_QUEUE_MAX_RUNTIME_MS
> > >  
> > > -	if (gt)
> > > -		xe_gt_stats_incr(xe_root_mmio_gt(gt_to_xe(gt)),
> > > -				 XE_GT_STATS_ID_PAGEFAULT_US,
> > > -				 xe_gt_stats_ktime_us_delta(start));
> > > +	xe_gt_stats_incr(xe_root_mmio_gt(xe), XE_GT_STATS_ID_PAGEFAULT_US,
> > > +			 xe_gt_stats_ktime_us_delta(start));
> > >  }
> > >  
> > >  static int xe_pagefault_queue_init(struct xe_device *xe,
> > > @@ -431,6 +747,7 @@ int xe_pagefault_init(struct xe_device *xe)
> > >  
> > >  		pf_work->xe = xe;
> > >  		pf_work->id = i;
> > > +		xe_pagefault_cache_start_invalidate(pf_work->cache.start);
> > >  		INIT_WORK(&pf_work->work, xe_pagefault_queue_work);
> > >  	}
> > >  
> > > @@ -454,15 +771,23 @@ static void xe_pagefault_queue_reset(struct xe_device *xe, struct xe_gt *gt,
> > >  
> > >  	/* Squash all pending faults on the GT */
> > >  
> > > -	spin_lock_irq(&pf_queue->lock);
> > > -	for (i = pf_queue->tail; i != pf_queue->head;
> > > -	     i = (i + xe_pagefault_entry_size()) % pf_queue->size) {
> > > +	guard(spinlock_irq)(&pf_queue->lock);
> > > +
> > > +	for (i = 0; i < pf_queue->size; i += xe_pagefault_entry_size()) {
> > >  		struct xe_pagefault *pf = pf_queue->data + i;
> > > +		bool active = pf->consumer.alloc_state ==
> > > +			XE_PAGEFAULT_ALLOC_STATE_ACTIVE;
> > >  
> > > -		if (pf->gt == gt)
> > > -			pf->gt = NULL;
> > > +		if (pf->gt != gt || active) {
> > > +			if (active)
> > > +				pf->consumer.next = NULL;
> > > +			continue;
> > > +		}
> > > +
> > > +		pf->consumer.alloc_state =
> > > +			XE_PAGEFAULT_ALLOC_STATE_FREE;
> > > +		pf->consumer.next = NULL;
> > >  	}
> > > -	spin_unlock_irq(&pf_queue->lock);
> > >  }
> > >  
> > >  /**
> > > @@ -478,14 +803,6 @@ void xe_pagefault_reset(struct xe_device *xe, struct xe_gt *gt)
> > >  	xe_pagefault_queue_reset(xe, gt, &xe->usm.pf_queue);
> > >  }
> > >  
> > > -static bool xe_pagefault_queue_full(struct xe_pagefault_queue *pf_queue)
> > > -{
> > > -	lockdep_assert_held(&pf_queue->lock);
> > > -
> > > -	return CIRC_SPACE(pf_queue->head, pf_queue->tail, pf_queue->size) <=
> > > -		xe_pagefault_entry_size();
> > > -}
> > > -
> > >  /*
> > >   * This function can race with multiple page fault producers, but worst case we
> > >   * stick a page fault on the same queue for consumption.
> > > @@ -511,18 +828,28 @@ int xe_pagefault_handler(struct xe_device *xe, struct xe_pagefault *pf)
> > >  {
> > >  	struct xe_pagefault_queue *pf_queue = &xe->usm.pf_queue;
> > >  	unsigned long flags;
> > > -	int work_index;
> > >  	bool full;
> > >  
> > >  	spin_lock_irqsave(&pf_queue->lock, flags);
> > > -	work_index = xe_pagefault_work_index(xe);
> > >  	full = xe_pagefault_queue_full(pf_queue);
> > >  	if (!full) {
> > > -		memcpy(pf_queue->data + pf_queue->head, pf, sizeof(*pf));
> > > -		pf_queue->head = (pf_queue->head + xe_pagefault_entry_size()) %
> > > -			pf_queue->size;
> > > -		queue_work(xe->usm.pagefault_wq,
> > > -			   &xe->usm.pf_workers[work_index].work);
> > > +		struct xe_pagefault *lpf;
> > > +		bool empty = xe_pagefault_queue_empty(pf_queue);
> > > +
> > > +		lpf = xe_pagefault_queue_add(pf_queue, pf);
> > > +		if (lpf) {
> > > +			lpf->consumer.next = NULL;
> > > +
> > > +			if (xe_pagefault_try_chain(pf_queue, lpf)) {
> > > +				if (empty)
> > > +					xe_pagefault_queue_advance(pf_queue);
> > > +			} else {
> > > +				int work_index = xe_pagefault_work_index(xe);
> > > +
> > > +				queue_work(xe->usm.pagefault_wq,
> > > +					   &xe->usm.pf_workers[work_index].work);
> > > +			}
> > > +		}
> > >  	} else {
> > >  		drm_warn(&xe->drm,
> > >  			 "PageFault Queue full, shouldn't be possible\n");
> > > diff --git a/drivers/gpu/drm/xe/xe_pagefault.h b/drivers/gpu/drm/xe/xe_pagefault.h
> > > index bd0cdf9ed37f..feaf2a69674a 100644
> > > --- a/drivers/gpu/drm/xe/xe_pagefault.h
> > > +++ b/drivers/gpu/drm/xe/xe_pagefault.h
> > > @@ -6,6 +6,8 @@
> > >  #ifndef _XE_PAGEFAULT_H_
> > >  #define _XE_PAGEFAULT_H_
> > >  
> > > +#include "xe_pagefault_types.h"
> > > +
> > >  struct xe_device;
> > >  struct xe_gt;
> > >  struct xe_pagefault;
> > > @@ -16,4 +18,73 @@ void xe_pagefault_reset(struct xe_device *xe, struct xe_gt *gt);
> > >  
> > >  int xe_pagefault_handler(struct xe_device *xe, struct xe_pagefault *pf);
> > >  
> > > +#define XE_PAGEFAULT_END_ADDR_MASK	(~0xfffull)
> > > +
> > > +/**
> > > + * xe_pagefault_set_end_addr() - store serviced range end for a pagefault
> > > + * @pf: Pagefault entry
> > > + * @end_addr: Inclusive end address of the serviced fault range
> > > + *
> > > + * The pagefault consumer stores the resolved fault range so subsequent faults
> > > + * hitting the same range can be immediately acknowledged without re-running
> > > + * the full fault handling path.
> > > + *
> > > + * The end address shares storage with other consumer metadata and therefore
> > > + * must be masked with %XE_PAGEFAULT_END_ADDR_MASK before storing. Bits outside
> > > + * the mask are reserved for internal state tracking and must be preserved.
> > > + */
> > > +static inline void
> > > +xe_pagefault_set_end_addr(struct xe_pagefault *pf, u64 end_addr)
> > > +{
> > > +	pf->consumer.end_addr &= ~XE_PAGEFAULT_END_ADDR_MASK;
> > > +	pf->consumer.end_addr |= end_addr;
> > > +}
> > > +
> > > +/**
> > > + * xe_pagefault_end_addr() - read serviced range end for a pagefault
> > > + * @pf: Pagefault entry
> > > + *
> > > + * Returns the inclusive end address of the range previously recorded by
> > > + * xe_pagefault_set_end_addr(). Only the bits covered by
> > > + * %XE_PAGEFAULT_END_ADDR_MASK are returned; other bits in the storage are
> > > + * reserved for internal state.
> > > + *
> > > + * Return: End address of the serviced fault range.
> > > + */
> > > +static inline u64 xe_pagefault_end_addr(struct xe_pagefault *pf)
> > > +{
> > > +	return pf->consumer.end_addr & XE_PAGEFAULT_END_ADDR_MASK;
> > > +}
> > > +
> > > +#undef XE_PAGEFAULT_END_ADDR_MASK
> > > +
> > > +/**
> > > + * xe_pagefault_set_start_addr() - store serviced range start for a pagefault
> > > + * @pf: Pagefault entry
> > > + * @start_addr: Start address of the serviced fault range
> > > + *
> > > + * The pagefault consumer stores the resolved fault range so subsequent faults
> > > + * hitting the same range can be immediately acknowledged without re-running
> > > + * the full fault handling path.
> > > + */
> > > +static inline void
> > > +xe_pagefault_set_start_addr(struct xe_pagefault *pf, u64 start_addr)
> > > +{
> > > +	pf->consumer.page_addr = start_addr;
> > > +}
> > > +
> > > +/**
> > > + * xe_pagefault_start_addr() - read serviced range start for a pagefault
> > > + * @pf: Pagefault entry
> > > + *
> > > + * Returns the inclusive start address of the range previously recorded by
> > > + * xe_pagefault_set_start_addr().
> > > + *
> > > + * Return: Start address of the serviced fault range.
> > > + */
> > > +static inline u64 xe_pagefault_start_addr(struct xe_pagefault *pf)
> > > +{
> > > +	return pf->consumer.page_addr;
> > > +}
> > > +
> > >  #endif
> > > diff --git a/drivers/gpu/drm/xe/xe_pagefault_types.h b/drivers/gpu/drm/xe/xe_pagefault_types.h
> > > index d349d79bc95e..a1fff0e47fa5 100644
> > > --- a/drivers/gpu/drm/xe/xe_pagefault_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_pagefault_types.h
> > > @@ -60,36 +60,58 @@ struct xe_pagefault {
> > >  	/**
> > >  	 * @consumer: State for the software handling the fault. Populated by
> > >  	 * the producer and may be modified by the consumer to communicate
> > > -	 * information back to the producer upon fault acknowledgment.
> > > +	 * information back to the producer upon fault acknowledgment. After
> > > +	 * fault acknowledgment, the producer should only access consumer fields
> > > +	 * via well defined helpers.
> > >  	 */
> > >  	struct {
> > > -		/** @consumer.page_addr: address of page fault */
> > > -		u64 page_addr;
> > > -		/** @consumer.asid: address space ID */
> > > -		u32 asid;
> > >  		/**
> > > -		 * @consumer.access_type: access type and prefetch flag packed
> > > -		 * into a u8.
> > > +		 * @consumer.page_addr: address of page fault, populated by
> > > +		 * consumer after fault completion
> > >  		 */
> > > -		u8 access_type;
> > > +		u64 page_addr;
> > > +		union {
> > > +			struct {
> > > +				/**
> > > +				 * @consumer.alloc_state: page fault allocation
> > > +				 * state
> > > +				 */
> > > +				u8 alloc_state;
> > > +				/**
> > > +				 * @consumer.access_type: access type, u8 rather
> > > +				 * than enum to keep size compact
> > > +				 */
> > > +				u8 access_type;
> > >  #define XE_PAGEFAULT_ACCESS_TYPE_MASK	GENMASK(1, 0)
> > >  #define XE_PAGEFAULT_ACCESS_PREFETCH	BIT(7)
> > > -		/**
> > > -		 * @consumer.fault_type_level: fault type and level, u8 rather
> > > -		 * than enum to keep size compact
> > > -		 */
> > > -		u8 fault_type_level;
> > > +				/**
> > > +				 * @consumer.fault_type_level: fault type and
> > > +				 * level, u8 rather than enum to keep size
> > > +				 * compact
> > > +				 */
> > > +				u8 fault_type_level;
> > >  #define XE_PAGEFAULT_TYPE_LEVEL_NACK		0xff	/* Producer indicates nack fault */
> > > -#define XE_PAGEFAULT_LEVEL_MASK			GENMASK(3, 0)
> > > -#define XE_PAGEFAULT_TYPE_MASK			GENMASK(7, 4)
> > > -		/** @consumer.engine_class_instance: engine class and instance */
> > > -		u8 engine_class_instance;
> > > +#define XE_PAGEFAULT_LEVEL_MASK			GENMASK(2, 0)
> > > +#define XE_PAGEFAULT_TYPE_MASK			GENMASK(6, 3)
> > > +#define XE_PAGEFAULT_REQUEUE_MASK		BIT(7)
> > > +				/** @consumer.engine_class_instance: engine class and instance */
> > > +				u8 engine_class_instance;
> > >  #define XE_PAGEFAULT_ENGINE_CLASS_MASK		GENMASK(3, 0)
> > >  #define XE_PAGEFAULT_ENGINE_INSTANCE_MASK	GENMASK(7, 4)
> > > -		/** @pad: alignment padding */
> > > -		u8 pad;
> > > -		/** @consumer.reserved: reserved bits for future expansion */
> > > -		u64 reserved;
> > > +				/** @consumer.asid: address space ID */
> > > +				u32 asid;
> > > +			};
> > > +			/**
> > > +			 * @consumer.end_addr: end address of page fault,
> > > +			 * populated by consumer after fault completion
> > > +			 */
> > > +			u64 end_addr;
> > > +		};
> > > +		/**
> > > +		 * @consumer.next: next pagefault chained to this fault,
> > > +		 * protected by pf_queue lock
> > > +		 */
> > > +		struct xe_pagefault *next;
> > >  	} consumer;
> > >  	/**
> > >  	 * @producer: State for the producer (i.e., HW/FW interface). Populated
> > > @@ -131,7 +153,7 @@ struct xe_pagefault_queue {
> > >  	u32 head;
> > >  	/** @tail: Tail pointer in bytes, moved by consumer, protected by @lock */
> > >  	u32 tail;
> > > -	/** @lock: protects page fault queue */
> > > +	/** @lock: protects page fault queue, workers caches */
> > >  	spinlock_t lock;
> > >  };
> > >  
> > > @@ -146,6 +168,21 @@ struct xe_pagefault_work {
> > >  	struct xe_device *xe;
> > >  	/** @id: Identifier for this work item */
> > >  	int id;
> > > +	/**
> > > +	 * @cache: Page fault cache for the currently processed fault
> > > +	 *
> > > +	 * Protected by the page fault queue lock.
> > > +	 */
> > > +	struct {
> > > +		/** @cache.start: Start address of the current page fault */
> > > +		u64 start;
> > > +		/** @cache.end: End address of the current page fault */
> > > +		u64 end;
> > > +		/** @cache.asid: Address space ID of the current page fault */
> > > +		u32 asid;
> > > +		/** @cache.pf: Pointer to the current page fault */
> > > +		struct xe_pagefault *pf;
> > > +	} cache;
> > >  	/** @work: Work item used to process the page fault */
> > >  	struct work_struct work;
> > >  };
> > > diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
> > > index 6a470a02fee7..627a741293d5 100644
> > > --- a/drivers/gpu/drm/xe/xe_svm.c
> > > +++ b/drivers/gpu/drm/xe/xe_svm.c
> > > @@ -15,6 +15,7 @@
> > >  #include "xe_gt_stats.h"
> > >  #include "xe_migrate.h"
> > >  #include "xe_module.h"
> > > +#include "xe_pagefault.h"
> > >  #include "xe_pm.h"
> > >  #include "xe_pt.h"
> > >  #include "xe_svm.h"
> > > @@ -1264,8 +1265,8 @@ DECL_SVM_RANGE_US_STATS(bind, BIND)
> > >  DECL_SVM_RANGE_US_STATS(fault, PAGEFAULT)
> > >  
> > >  static int __xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
> > > -				     struct xe_gt *gt, u64 fault_addr,
> > > -				     bool need_vram)
> > > +				     struct xe_pagefault *pf, struct xe_gt *gt,
> > > +				     u64 fault_addr, bool need_vram)
> > >  {
> > >  	int devmem_possible = IS_DGFX(vm->xe) &&
> > >  		IS_ENABLED(CONFIG_DRM_XE_PAGEMAP);
> > > @@ -1424,6 +1425,10 @@ static int __xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
> > >  	xe_svm_range_bind_us_stats_incr(gt, range, bind_start);
> > >  
> > >  out:
> > > +	/* Give hint to immediately ack faults */
> > > +	xe_pagefault_set_start_addr(pf,  xe_svm_range_start(range));
> > > +	xe_pagefault_set_end_addr(pf, xe_svm_range_end(range));
> > > +
> > 
> > I have not checked the GT stats from later patches in the series but are we able
> > to chain enough faults by setting start/end so late during page fault handling?
> > 
> 
> This doesn't affect the chain-building process. We blindly attempt to
> build chains on 2 MB-aligned boundaries. This is about communicating
> back that we serviced for the fault and determining whether faults in
> the chain should be acknowledged or retried. As a result, the placement
> doesn't matter.
> 
> I had considered dynamically updating the chain size/window after
> looking up the range or VMA, but that isn't implemented in this series.
> If we went that route, then yes, we'd want to do it immediately after
> the range/VMA lookup. We'd also probably want to use try-locking on the
> VMA lock to avoid HoQ blocking.
> 
> The chain reconstruction is a bit tricky here, which is why I haven't
> implemented it yet.
> 
> Matt
> 
> > Would there be a way to move it closer to the beginning of
> > __xe_svm_handle_pagefault()?
> > 
> > Francois
> > 
> > >  	xe_svm_range_fault_us_stats_incr(gt, range, start);
> > >  	mutex_unlock(&range->lock);
> > >  	drm_gpusvm_range_put(&range->base);
> > > @@ -1446,6 +1451,7 @@ static int __xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
> > >   * xe_svm_handle_pagefault() - SVM handle page fault
> > >   * @vm: The VM.
> > >   * @vma: The CPU address mirror VMA.
> > > + * @pf: Pagefault structure
> > >   * @gt: The gt upon the fault occurred.
> > >   * @fault_addr: The GPU fault address.
> > >   * @atomic: The fault atomic access bit.
> > > @@ -1456,8 +1462,8 @@ static int __xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
> > >   * Return: 0 on success, negative error code on error.
> > >   */
> > >  int xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
> > > -			    struct xe_gt *gt, u64 fault_addr,
> > > -			    bool atomic)
> > > +			    struct xe_pagefault *pf, struct xe_gt *gt,
> > > +			    u64 fault_addr, bool atomic)
> > >  {
> > >  	int need_vram, ret;
> > >  retry:
> > > @@ -1465,7 +1471,7 @@ int xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
> > >  	if (need_vram < 0)
> > >  		return need_vram;
> > >  
> > > -	ret =  __xe_svm_handle_pagefault(vm, vma, gt, fault_addr,
> > > +	ret =  __xe_svm_handle_pagefault(vm, vma, pf, gt, fault_addr,
> > >  					 need_vram ? true : false);
> > >  	if (ret == -EAGAIN) {
> > >  		/*
> > > diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
> > > index 46be2e5c6f7f..2a0dc0d125c9 100644
> > > --- a/drivers/gpu/drm/xe/xe_svm.h
> > > +++ b/drivers/gpu/drm/xe/xe_svm.h
> > > @@ -21,6 +21,7 @@ struct drm_file;
> > >  struct xe_bo;
> > >  struct xe_gt;
> > >  struct xe_device;
> > > +struct xe_pagefault;
> > >  struct xe_vram_region;
> > >  struct xe_tile;
> > >  struct xe_vm;
> > > @@ -109,8 +110,8 @@ void xe_svm_fini(struct xe_vm *vm);
> > >  void xe_svm_close(struct xe_vm *vm);
> > >  
> > >  int xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
> > > -			    struct xe_gt *gt, u64 fault_addr,
> > > -			    bool atomic);
> > > +			    struct xe_pagefault *pf, struct xe_gt *gt,
> > > +			    u64 fault_addr, bool atomic);
> > >  
> > >  bool xe_svm_has_mapping(struct xe_vm *vm, u64 start, u64 end);
> > >  
> > > @@ -298,8 +299,8 @@ void xe_svm_close(struct xe_vm *vm)
> > >  
> > >  static inline
> > >  int xe_svm_handle_pagefault(struct xe_vm *vm, struct xe_vma *vma,
> > > -			    struct xe_gt *gt, u64 fault_addr,
> > > -			    bool atomic)
> > > +			    struct xe_pagefault *pf, struct xe_gt *gt,
> > > +			    u64 fault_addr, bool atomic)
> > >  {
> > >  	return 0;
> > >  }
> > > -- 
> > > 2.34.1
> > >