Re: [PATCH v4 04/12] drm/xe: Use a single page-fault queue with multiple workers

[Maciej Patelczyk <maciej.patelczyk@intel.com>]

On 06/05/2026 21:42, Matthew Brost wrote:

> On Wed, May 06, 2026 at 05:46:30PM +0200, Maciej Patelczyk wrote:
>> On 26/02/2026 05:28, Matthew Brost wrote:
>>
>>> With fine-grained page-fault locking, it no longer makes sense to
>>> maintain multiple page-fault queues, as we no longer hash queues based
>>> on the VM’s ASID. Multiple workers can pull page faults from a single
>>> queue, eliminating any head-of-queue blocking. Refactor the structures
>>> and code to use a single shared queue.
>>>
>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>> ---
>>>    drivers/gpu/drm/xe/xe_device_types.h    | 12 +++---
>>>    drivers/gpu/drm/xe/xe_pagefault.c       | 52 +++++++++++++------------
>>>    drivers/gpu/drm/xe/xe_pagefault_types.h | 17 +++++++-
>>>    3 files changed, 50 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>>> index 1eb0fe118940..0558dfd52541 100644
>>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>>> @@ -304,8 +304,8 @@ struct xe_device {
>>>    		struct xarray asid_to_vm;
>>>    		/** @usm.next_asid: next ASID, used to cyclical alloc asids */
>>>    		u32 next_asid;
>>> -		/** @usm.current_pf_queue: current page fault queue */
>>> -		u32 current_pf_queue;
>>> +		/** @usm.current_pf_work: current page fault work item */
>>> +		u32 current_pf_work;
>>>    		/** @usm.lock: protects UM state */
>>>    		struct rw_semaphore lock;
>>>    		/** @usm.pf_wq: page fault work queue, unbound, high priority */
>>> @@ -315,9 +315,11 @@ struct xe_device {
>>>    		 * yields the best bandwidth utilization of the kernel paging
>>>    		 * engine.
>>>    		 */
>>> -#define XE_PAGEFAULT_QUEUE_COUNT	4
>>> -		/** @usm.pf_queue: Page fault queues */
>>> -		struct xe_pagefault_queue pf_queue[XE_PAGEFAULT_QUEUE_COUNT];
>>> +#define XE_PAGEFAULT_WORK_COUNT	4
>>> +		/** @usm.pf_workers: Page fault workers */
>>> +		struct xe_pagefault_work pf_workers[XE_PAGEFAULT_WORK_COUNT];
>>> +		/** @usm.pf_queue: Page fault queue */
>>> +		struct xe_pagefault_queue pf_queue;
>>>    #if IS_ENABLED(CONFIG_DRM_XE_PAGEMAP)
>>>    		/** @usm.pagemap_shrinker: Shrinker for unused pagemaps */
>>>    		struct drm_pagemap_shrinker *dpagemap_shrinker;
>>> diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
>>> index a372db7cd839..7880fc7e7eb4 100644
>>> --- a/drivers/gpu/drm/xe/xe_pagefault.c
>>> +++ b/drivers/gpu/drm/xe/xe_pagefault.c
>>> @@ -222,6 +222,7 @@ static void xe_pagefault_queue_retry(struct xe_pagefault_queue *pf_queue,
>>>    		pf_queue->tail = pf_queue->size - xe_pagefault_entry_size();
>>>    	else
>>>    		pf_queue->tail -= xe_pagefault_entry_size();
>>> +	memcpy(pf_queue->data + pf_queue->tail, pf, sizeof(*pf));
>>>    	spin_unlock_irq(&pf_queue->lock);
>>>    }
>>> @@ -267,8 +268,10 @@ static void xe_pagefault_print(struct xe_pagefault *pf)
>>>    static void xe_pagefault_queue_work(struct work_struct *w)
>>>    {
>>> -	struct xe_pagefault_queue *pf_queue =
>>> -		container_of(w, typeof(*pf_queue), worker);
>>> +	struct xe_pagefault_work *pf_work =
>>> +		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;
>>>    	unsigned long threshold;
>>> @@ -285,7 +288,7 @@ static void xe_pagefault_queue_work(struct work_struct *w)
>>>    		if (err == -EAGAIN) {
>>>    			xe_pagefault_queue_retry(pf_queue, &pf);
>>> -			queue_work(gt_to_xe(pf.gt)->usm.pf_wq, w);
>>> +			queue_work(xe->usm.pf_wq, w);
>>>    			break;
>>>    		} else if (err) {
>>>    			if (!(pf.consumer.access_type & XE_PAGEFAULT_ACCESS_PREFETCH)) {
>>> @@ -302,7 +305,7 @@ static void xe_pagefault_queue_work(struct work_struct *w)
>>>    		pf.producer.ops->ack_fault(&pf, err);
>>>    		if (time_after(jiffies, threshold)) {
>>> -			queue_work(gt_to_xe(pf.gt)->usm.pf_wq, w);
>>> +			queue_work(xe->usm.pf_wq, w);
>>>    			break;
>>>    		}
>>>    	}
>>> @@ -348,7 +351,6 @@ static int xe_pagefault_queue_init(struct xe_device *xe,
>>>    		xe_pagefault_entry_size(), total_num_eus, pf_queue->size);
>>>    	spin_lock_init(&pf_queue->lock);
>>> -	INIT_WORK(&pf_queue->worker, xe_pagefault_queue_work);
>>>    	pf_queue->data = drmm_kzalloc(&xe->drm, pf_queue->size, GFP_KERNEL);
>>>    	if (!pf_queue->data)
>>> @@ -381,14 +383,20 @@ int xe_pagefault_init(struct xe_device *xe)
>>>    	xe->usm.pf_wq = alloc_workqueue("xe_page_fault_work_queue",
>>>    					WQ_UNBOUND | WQ_HIGHPRI,
>>> -					XE_PAGEFAULT_QUEUE_COUNT);
>>> +					XE_PAGEFAULT_WORK_COUNT);
>>>    	if (!xe->usm.pf_wq)
>>>    		return -ENOMEM;
>>> -	for (i = 0; i < XE_PAGEFAULT_QUEUE_COUNT; ++i) {
>>> -		err = xe_pagefault_queue_init(xe, xe->usm.pf_queue + i);
>>> -		if (err)
>>> -			goto err_out;
>>> +	err = xe_pagefault_queue_init(xe, &xe->usm.pf_queue);
>>> +	if (err)
>>> +		goto err_out;
>>> +
>>> +	for (i = 0; i < XE_PAGEFAULT_WORK_COUNT; ++i) {
>>> +		struct xe_pagefault_work *pf_work = xe->usm.pf_workers + i;
>>> +
>>> +		pf_work->xe = xe;
>>> +		pf_work->id = i;
>>> +		INIT_WORK(&pf_work->work, xe_pagefault_queue_work);
>>>    	}
>>>    	return devm_add_action_or_reset(xe->drm.dev, xe_pagefault_fini, xe);
>>> @@ -430,10 +438,7 @@ static void xe_pagefault_queue_reset(struct xe_device *xe, struct xe_gt *gt,
>>>     */
>>>    void xe_pagefault_reset(struct xe_device *xe, struct xe_gt *gt)
>>>    {
>>> -	int i;
>>> -
>>> -	for (i = 0; i < XE_PAGEFAULT_QUEUE_COUNT; ++i)
>>> -		xe_pagefault_queue_reset(xe, gt, xe->usm.pf_queue + i);
>>> +	xe_pagefault_queue_reset(xe, gt, &xe->usm.pf_queue);
>>>    }
>>>    static bool xe_pagefault_queue_full(struct xe_pagefault_queue *pf_queue)
>>> @@ -448,13 +453,11 @@ static bool xe_pagefault_queue_full(struct xe_pagefault_queue *pf_queue)
>>>     * This function can race with multiple page fault producers, but worst case we
>>>     * stick a page fault on the same queue for consumption.
>>>     */
>>> -static int xe_pagefault_queue_index(struct xe_device *xe)
>>> +static int xe_pagefault_work_index(struct xe_device *xe)
>>>    {
>>> -	u32 old_pf_queue = READ_ONCE(xe->usm.current_pf_queue);
>>> -
>>> -	WRITE_ONCE(xe->usm.current_pf_queue, (old_pf_queue + 1));
>>> +	lockdep_assert_held(&xe->usm.pf_queue.lock);
>>> -	return old_pf_queue % XE_PAGEFAULT_QUEUE_COUNT;
>>> +	return xe->usm.current_pf_work++ % XE_PAGEFAULT_WORK_COUNT;
>>>    }
>>>    /**
>>> @@ -469,22 +472,23 @@ static int xe_pagefault_queue_index(struct xe_device *xe)
>>>     */
>>>    int xe_pagefault_handler(struct xe_device *xe, struct xe_pagefault *pf)
>>>    {
>>> -	int queue_index = xe_pagefault_queue_index(xe);
>>> -	struct xe_pagefault_queue *pf_queue = xe->usm.pf_queue + queue_index;
>>> +	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.pf_wq, &pf_queue->worker);
>>> +		queue_work(xe->usm.pf_wq,
>>> +			   &xe->usm.pf_workers[work_index].work);
>>>    	} else {
>>>    		drm_warn(&xe->drm,
>>> -			 "PageFault Queue (%d) full, shouldn't be possible\n",
>>> -			 queue_index);
>>> +			 "PageFault Queue full, shouldn't be possible\n");
>>>    	}
>>>    	spin_unlock_irqrestore(&pf_queue->lock, flags);
>>> diff --git a/drivers/gpu/drm/xe/xe_pagefault_types.h b/drivers/gpu/drm/xe/xe_pagefault_types.h
>>> index b3289219b1be..45065c25c25f 100644
>>> --- a/drivers/gpu/drm/xe/xe_pagefault_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_pagefault_types.h
>>> @@ -131,8 +131,21 @@ struct xe_pagefault_queue {
>>>    	u32 tail;
>>>    	/** @lock: protects page fault queue */
>>>    	spinlock_t lock;
>>> -	/** @worker: to process page faults */
>>> -	struct work_struct worker;
>>> +};
>>> +
>>> +/**
>>> + * struct xe_pagefault_work - Xe page fault work item (consumer)
>>> + *
>>> + * Represents a worker that pops a &struct xe_pagefault from the page fault
>>> + * queue and processes it.
>>> + */
>>> +struct xe_pagefault_work {
>>> +	/** @xe: Back-pointer to the Xe device */
>>> +	struct xe_device *xe;
>>> +	/** @id: Identifier for this work item */
>>> +	int id;
>>> +	/** @work: Work item used to process the page fault */
>>> +	struct work_struct work;
>>>    };
>>>    #endif
>> Matt,
>>
>> There were total 4 pf_queues each of size = (total_num_eus +
>> XE_NUM_HW_ENGINES) * xe_pagefault_entry_size() * PF_MULTIPLIER additionally
>> bigger of roundup_pow_of_two().
>>
>> Each of this queue had a dedicated worker.
>>
>> There is a comment on queue calculation size in xe_pagefault_queue_init():
>>
>> "XXX: Multiplier required as compute UMD are getting PF queue errors
>>
>> without it. Follow on why this multiplier is required."
>>
>> PF queue errors could be due to slow pf processing by handler in KMD plus
>> generating PF for a single VM (asid) therefore hitting constantly single
>> queue.
>>
>>
>> Now there is a single queue which is 4 times smaller (overall) but it has 4
>> workers and there are optimizations which potentially drastically decrease
>> processing time.
>>
>> In the end it could resolve to a case where a single queue had 4 workers
>> instead of one which would be still faster than it is now.
>>
>> Still, not sure if queue size is not too small.
>>
>> Did you have a thought about it?
>>
>>
>> And I think this XXX comment becomes obsolete with such change.
>>
> I think the XXX comment was always wrong. We kept increasing the queue
> size because of random overflows, but the actual bug was that we didn’t
> round up to a power of two, and CIRC_SPACE relies on values being powers
> of two.
>
> I believe we never got around to deleting the XXX comment or removing
> the multiplier. We can handle this in a follow-up after this series, as
> I’d like a large change like this to sit for a while so we can test and
> ensure there are no regressions. Then we can clean up the XXX comment
> and the multiplier in a follow-up.
>
> Matt

All right,

Reviewed-by: Maciej Patelczyk <maciej.patelczyk@intel.com>


>> Regards,
>>
>> Maciej
>>
>>