Re: [PATCH v3 23/33] drm/xe: Defrag using out-of-lock page preallocation

[Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>]

On Fri, Jul 10, 2026 at 07:56:09PM -0700, Matthew Brost wrote:
>Convert xe_bo_defrag_one() to a three-phase scheme that moves the
>expensive beneficial-order allocations out of the BO dma-resv lock:
>
> 1. Under the lock, re-check eligibility, capture the tt cpu-caching and
>    read the exact sub-optimal page count (ttm_tt_suboptimal_pages()),
>    bounded by the run budget.
> 2. Drop the lock and preallocate that many beneficial-order chunks via
>    ttm_pool_prealloc_fill(); reclaim/compaction stalls and the cpu
>    cache mode change both happen here, outside the lock.
> 3. Re-take the lock, re-check, and validate consuming the prealloc bag.
>    Any shortfall harvests the old backing instead of reclaiming, so the
>    move never stalls under the lock; the BO re-queues for a later pass.
>

Assisted-by: LLM

But as lock is released between phase 1 and 3, there can be BO mismatch
between phase-3 and phase-1. More below.

>With both allocation and caching hoisted out, the BO-lock held time
>drops to copy-bound (median ~0.27ms, p95 ~2ms); the remaining tail is
>lock-wait behind active rendering, as intended.
>
>Cc: Carlos Santa <carlos.santa@intel.com>
>Cc: Ryan Neph <ryanneph@google.com>
>Cc: Christian Koenig <christian.koenig@amd.com>
>Cc: Huang Rui <ray.huang@amd.com>
>Cc: Matthew Auld <matthew.auld@intel.com>
>Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>Cc: Maxime Ripard <mripard@kernel.org>
>Cc: Thomas Zimmermann <tzimmermann@suse.de>
>Cc: David Airlie <airlied@gmail.com>
>Cc: Simona Vetter <simona@ffwll.ch>
>Cc: dri-devel@lists.freedesktop.org
>Cc: linux-kernel@vger.kernel.org
>Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>Assisted-by: GitHub_Copilot:claude-opus-4.8
>Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>
>---
>
>v3:
> - Use DIV_ROUND_UP_ULL() for the 64-bit preallocation chunk count
>   (Sashiko)
>---
> drivers/gpu/drm/xe/xe_bo.c | 46 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 45 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>index 0bdd1c920404..8b89d34f37d4 100644
>--- a/drivers/gpu/drm/xe/xe_bo.c
>+++ b/drivers/gpu/drm/xe/xe_bo.c
>@@ -1258,16 +1258,56 @@ static int xe_bo_defrag_one(struct xe_device *xe, struct xe_bo *bo,
> 		.defrag_bytes_remaining = budget,
> 	};
> 	struct ttm_buffer_object *ttm_bo = &bo->ttm;
>+	struct ttm_pool *pool = &ttm_bo->bdev->pool;
>+	struct ttm_pool_prealloc pp = {};
> 	struct ttm_placement placement;
> 	struct ttm_place place;
>+	enum ttm_caching tt_caching;
>+	unsigned int order, want = 0;
> 	int ret = 0;
>
> 	*consumed = 0;
> 	*needs_more = false;
>
>+	/*
>+	 * Phase 1: under the BO lock, re-check eligibility and estimate how
>+	 * many beneficial-order chunks the move will (re)allocate, bounded by
>+	 * the run's byte budget. Then drop the lock so the high-order, possibly
>+	 * reclaim/compaction stalling allocations happen unlocked.
>+	 */
>+	xe_bo_lock(bo, false);
>+	if (!xe_bo_needs_defrag(bo)) {
>+		xe_bo_defrag_remove(bo);
>+		xe_bo_unlock(bo);
>+		goto out;
>+	}
>+	tt_caching = bo->ttm.ttm->caching;
>+	order = ttm_pool_prealloc_order(pool);
>+	if (order && ttm_bo->ttm) {
>+		u32 suboptimal = ttm_tt_suboptimal_pages(ttm_bo->ttm);
>+		u64 cap = min_t(u64, budget, xe_bo_size(bo)) >> PAGE_SHIFT;
>+
>+		/* Prealloc only the beneficial-order chunks the move replaces. */
>+		want = DIV_ROUND_UP_ULL(min_t(u64, suboptimal, cap), 1UL << order);
>+	}
>+	xe_bo_unlock(bo);

As mentioned above, BO can change after we release the lock here.
So, 'want' can be less than what BO needs. This is probably ok, as if fetching
from prealloc fails, TTM will fall back to fresh alloc.
But if tt_caching changes after releaseing the lock here, it is never checked
in phase-3 and page cache setting will be different than that of the BO.

>+
>+	/* Phase 2: preallocate outside the lock; */
>+	if (want) {
>+		ret = ttm_pool_prealloc_fill(pool, tt_caching, &pp, want);
>+		if (ret || !pp.count) {

'!pp.count' is treated as error but 'pp.count < want' is not. Should we?

Niranjana

>+			ret = ret ?: -ENOMEM;
>+			xe_gt_stats_incr(xe_root_mmio_gt(xe),
>+					 XE_GT_STATS_ID_DEFRAG_FAILED_COUNT, 1);
>+			goto out_err;
>+		}
>+	}
>+
>+	/*
>+	 * Phase 3: re-take the lock, re-check, and validate using the prealloc.
>+	 */
> 	xe_bo_lock(bo, false);
>
>-	/* Re-check eligibility under the BO lock. */
> 	if (!xe_bo_needs_defrag(bo)) {
> 		xe_bo_defrag_remove(bo);
> 		goto unlock;
>@@ -1277,6 +1317,7 @@ static int xe_bo_defrag_one(struct xe_device *xe, struct xe_bo *bo,
> 	place.flags |= TTM_PL_FLAG_TEMPORARY;
> 	placement.num_placement = 1;
> 	placement.placement = &place;
>+	ctx.prealloc = &pp;
>
> 	/*
> 	 * On success the move reallocates the backing at beneficial order and
>@@ -1321,6 +1362,9 @@ static int xe_bo_defrag_one(struct xe_device *xe, struct xe_bo *bo,
>
> unlock:
> 	xe_bo_unlock(bo);
>+out_err:
>+	ttm_pool_prealloc_fini(pool, &pp);
>+out:
> 	return ret;
> }
>
>-- 
>2.34.1
>