Re: [PATCH v5 20/25] drm/xe: Add ULLS migration job support to migration layer

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

On Fri, Sep 04, 2026 at 02:44:39AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] ULLS_EXIT_JIFFIES evaluates to 0 on HZ=100 configurations, causing a tight spin loop.
> - [High] Deadlock caused by synchronous dma_fence_wait while holding reclaim-tainted job_mutex.
> - [Medium] Asynchronous runtime PM put used during device teardown path.
> --
> 
> commit 1b246ef0dc00023e2aba23868ed86c7ff7c9257a
> Author: Matthew Brost <matthew.brost@intel.com>
> 
> drm/xe: Add ULLS migration job support to migration layer
> 
> Add function to enter ULLS mode for migration job and delayed worker to
> exit (power saving). ULLS mode expected to entered upon page fault or
> SVM prefetch. ULLS mode exit delay is currently set to 5ms.
> 
> Link: https://patch.msgid.link/20260228013501.106680-21-matthew.brost@intel.com
> 
> > diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> > index 471ae5741836e..588e1c9085f31 100644
> > --- a/drivers/gpu/drm/xe/xe_migrate.c
> > +++ b/drivers/gpu/drm/xe/xe_migrate.c
> 
> [ ... ]
> 
> > @@ -77,6 +80,14 @@ struct xe_migrate {
> >  	struct dma_fence *fence;
> >  	/** @min_chunk_size: For dgfx, Minimum chunk size */
> >  	u64 min_chunk_size;
> > +	/** @ulls: ULLS support */
> > +	struct {
> > +		/** @ulls.enabled: ULLS is enabled */
> > +		bool enabled;
> > +#define ULLS_EXIT_JIFFIES	(HZ / 200)	/* 5ms */
> 
> [Severity: High]
> Does this calculation truncate to 0 when HZ=100?
> 
> If HZ < 200, ULLS_EXIT_JIFFIES evaluates to 0. When xe_migrate_ulls_exit
> executes while the queue is not idle, it reschedules itself with a delay
> of 0. This results in an immediate re-execution of the worker, creating a
> tight spin loop and causing a soft lockup with 100% CPU utilization until
> the queue becomes idle.
> 

Also right. Should be: ((5 * HZ) / MSEC_PER_SEC)

Matt

> > +		/** @ulls.exit_work: ULLS exit worker */
> > +		struct delayed_work exit_work;
> > +	} ulls;
> >  };
> >  
> >  #define MAX_PREEMPTDISABLE_TRANSFER SZ_8M /* Around 1ms. */
> > @@ -98,6 +109,16 @@ struct xe_migrate {
> >  static void xe_migrate_fini(void *arg)
> >  {
> >  	struct xe_migrate *m = arg;
> > +	struct xe_device *xe = tile_to_xe(m->tile);
> > +
> > +	disable_delayed_work_sync(&m->ulls.exit_work);
> > +	mutex_lock(&m->job_mutex);
> > +	if (m->ulls.enabled) {
> > +		xe_force_wake_put(gt_to_fw(m->q->hwe->gt), m->q->hwe->domain);
> > +		xe_pm_runtime_put(xe);
> 
> [Severity: Medium]
> Will this asynchronous put call in the teardown path be canceled by the
> subsequent PM disable?
> 
> xe_migrate_fini acts as the devm cleanup action. It calls xe_pm_runtime_put
> which queues an asynchronous suspend. However, during driver teardown,
> pm_runtime_disable will cancel this pending async work before it executes,
> leaving the hardware incorrectly in an active power state.
> 
> Should this be a synchronous pm_runtime_put_sync instead?
> 
> > +		m->ulls.enabled = false;
> > +	}
> > +	mutex_unlock(&m->job_mutex);
> >  
> >  	xe_vm_lock(m->q->vm, false);
> >  	xe_bo_unpin(m->pt_bo);
> 
> [ ... ]
> 
> > +static void xe_migrate_ulls_exit(struct work_struct *work)
> > +{
> > +	struct xe_migrate *m = container_of(work, struct xe_migrate,
> > +					    ulls.exit_work.work);
> 
> [ ... ]
> 
> > +	mutex_lock(&m->job_mutex);
> > +
> > +	if (!xe_exec_queue_is_idle(m->q, 1))
> > +		goto unlock_exit;
> > +
> > +	xe_sched_job_get(job);
> > +	xe_sched_job_arm(job);
> > +	job->is_ulls = true;
> > +	job->is_ulls_last = true;
> > +	fence = dma_fence_get(&job->drm.s_fence->finished);
> > +	xe_sched_job_push(job);
> > +
> > +	/* Serialize force wake put */
> > +	dma_fence_wait(fence, false);
> 
> [Severity: High]
> Does waiting on this dma-fence while holding the reclaim-tainted job_mutex
> create a deadlock?
> 
> m->job_mutex is explicitly annotated as being part of the memory reclaim path
> via fs_reclaim_acquire/release in xe_migrate_init. By synchronously
> waiting on a dma-fence with dma_fence_wait while holding this mutex, a
> circular dependency is created with the memory allocator.
> 
> If the system is under memory pressure and triggers reclaim while signaling
> the fence, or if a GPU reset (which must allocate memory) is needed to signal
> the fence, the reclaim path will attempt to acquire job_mutex and block
> forever.
> 
> > +	dma_fence_put(fence);
> > +
> > +	m->ulls.enabled = false;
> > +unlock_exit:
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260904022207.3490018-1-matthew.brost@intel.com?part=20