Re: [PATCH v3 20/33] drm/xe: Add a page defragmentation worker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The defrag worker can be scheduled during probe teardown but is never canceled if probe fails early, leading to a UAF of the `xe_device`.
- [High] The defrag worker continuously polls at a high frequency (40Hz / 25ms) while the device is runtime-suspended or idle, causing significant power regression.
- [High] The defrag worker enters an infinite 40Hz retry loop on the same BO when memory is fragmented, bypassing exponential backoff.
--

--- Patch [20]: [PATCH v3 20/33] drm/xe: Add a page defragmentation worker ---
commit 1f23cf5472cb7a98f4987467477a919155eb0dc0
Author: Matthew Brost <matthew.brost@intel.com>
Subject: drm/xe: Add a page defragmentation worker

This patch adds a delayed worker to the DRM XE driver to periodically
defragment buffer objects whose backing TT pages were allocated below the
optimal order. It bounds the work done per run and applies exponential
backoff on failures.

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
[ ... ]
> +static void xe_bo_defrag_worker(struct work_struct *w)
> +{
[ ... ]
> +	if (!drm_dev_enter(&xe->drm, &idx))
> +		return;
> +
> +	if (!xe_pm_runtime_get_if_in_use(xe)) {
> +		/*
> +		 * The device is (runtime) suspending/suspended. Reschedule so
> +		 * the pass retries once it is active again: the enqueue-time
> +		 * kick in xe_bo_defrag_add() only fires on an empty->non-empty
> +		 * transition, so it will not re-arm the worker while the list
> +		 * is still non-empty. Requeue here to avoid stranding tracked
> +		 * BOs. The get attempt does not resume the device, so this is
> +		 * just a lightweight CPU-side timer while suspended.
> +		 */
> +		if (atomic_read(&xe->mem.defrag.count))
> +			xe_bo_defrag_schedule(xe);
> +		drm_dev_exit(idx);
> +		return;
> +	}

[Severity: High]
Will this cause the worker to endlessly poll at a high frequency while the
device is suspended?

Looking at xe_bo_defrag_worker() in xe_bo.c, when the device is runtime
suspended, xe_pm_runtime_get_if_in_use(xe) returns false. The worker then
hits this early return path and schedules itself via
xe_bo_defrag_schedule(xe) if the defrag count is non-zero.

Since this rescheduling uses the current interval_ms (which is typically
25ms) and applies no backoff, the worker will repeatedly wake up the CPU
every 25ms (40Hz) endlessly while the device is suspended, preventing deep
sleep C-states.

> +
> +	/*
> +	 * Process at most XE_BO_DEFRAG_SIZE_LIMIT bytes of newly (re)allocated
[ ... ]
> +		if (ret || needs_more) {
> +			scoped_guard(spinlock, &xe->mem.defrag.lock) {
> +				if (ret)
> +					/*
> +					 * Abort the pass and retry the whole
> +					 * list later, backing off exponentially
> +					 * on every failure.
> +					 */
> +					xe->mem.defrag.interval_ms =
> +						min(xe->mem.defrag.interval_ms * 2,
> +						    (unsigned int)XE_BO_DEFRAG_INTERVAL_MAX_MS);
> +				else
> +					/* Progress, reset interval */
> +					xe->mem.defrag.interval_ms = XE_BO_DEFRAG_INTERVAL_MS;
> +			}
> +
> +			xe_bo_put(bo);
> +			requeue = true;
> +			break;
> +		}

[Severity: High]
Does this create an infinite 40Hz retry loop on the same BO when memory is
fragmented, bypassing the exponential backoff?

When xe_bo_defrag_one() successfully satisfies an allocation using
lower-order pages, ret is 0 but needs_more is true. In this block, because
ret is 0, the else branch resets the interval_ms to 25ms, and the loop
breaks with requeue set to true.

Since the BO is never moved to the back of the list or removed, it remains
at the head. On the next run in 25ms, the worker attempts to defrag the
exact same BO again, likely hitting the exact same fragmentation fallback,
and reschedules at 25ms again.

> +
> +		xe_bo_put(bo);
> +	}
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
[ ... ]
> @@ -1069,6 +1069,15 @@ int xe_device_probe(struct xe_device *xe)
>  			return err;
>  	}
>  
> +	/*
> +	 * Register the defrag worker teardown now that the migrate contexts it
> +	 * depends on are initialized, so devm's reverse-order cleanup stops the
> +	 * worker before those contexts are torn down.
> +	 */
> +	err = xe_bo_defrag_init(xe);
> +	if (err)
> +		return err;

[Severity: Critical]
Is there a use-after-free risk if the device probe fails before this point?

The devm action to cleanly cancel the worker (xe_bo_defrag_init) is
registered late in xe_device_probe(). If the probe fails before this
registration (e.g., during xe_display_init_early() or xe_gt_init()), the
devm teardown will unpin BOs.

Unpinning a BO calls xe_bo_unpin() -> xe_bo_defrag_update(), which can
schedule the worker. Because the devm cleanup action was never registered,
the worker won't be cancelled during teardown, leaving it active after the
xe struct is freed.

> +
>  	err = xe_pagefault_init(xe);
>  	if (err)
>  		return err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711025619.2540575-1-matthew.brost@intel.com?part=20