Intel-XE Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Matthew Brost <matthew.brost@intel.com>
To: "Yadav, Arvind" <arvind.yadav@intel.com>
Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>,
	intel-xe@lists.freedesktop.org, himal.prasad.ghimiray@intel.com,
	pallavi.mishra@intel.com
Subject: Re: [PATCH v5 6/9] drm/xe/madvise: Implement per-VMA purgeable state tracking
Date: Tue, 24 Feb 2026 08:36:54 -0800	[thread overview]
Message-ID: <aZ3TpuSFKzY4y9sg@lstrano-desk.jf.intel.com> (raw)
In-Reply-To: <5aaab739-2291-441e-937b-746495ce7d58@intel.com>

On Tue, Feb 24, 2026 at 08:37:44PM +0530, Yadav, Arvind wrote:
> 
> On 24-02-2026 18:18, Thomas Hellström wrote:
> > On Wed, 2026-02-11 at 20:56 +0530, Arvind Yadav wrote:
> > > Track purgeable state per-VMA instead of using a coarse shared
> > > BO check. This prevents purging shared BOs until all VMAs across
> > > all VMs are marked DONTNEED.
> > > 
> > > Add xe_bo_all_vmas_dontneed() to check all VMAs before marking
> > > a BO purgeable. Add xe_bo_recheck_purgeable_on_vma_unbind() to
> > > handle state transitions when VMAs are destroyed - if all
> > > remaining VMAs are DONTNEED the BO can become purgeable, or if
> > > no VMAs remain it transitions to WILLNEED.
> > > 
> > > The per-VMA purgeable_state field stores the madvise hint for
> > > each mapping. Shared BOs can only be purged when all VMAs
> > > unanimously indicate DONTNEED.
> > > 
> > > One thing to note: when the last VMA goes away, we default back to
> > > WILLNEED. DONTNEED is a per-mapping hint, and without any mappings
> > > there is no remaining madvise state to justify purging. This prevents
> > > BOs from becoming purgeable solely due to being temporarily unmapped.
> > > 
> > > v3:
> > >    - This addresses Thomas Hellström's feedback: "loop over all vmas
> > >      attached to the bo and check that they all say WONTNEED. This
> > > will
> > >      also need a check at VMA unbinding"
> > > 
> > > v4:
> > >    - @madv_purgeable atomic_t → u32 change across all relevant
> > >      patches (Matt)
> > > 
> > > v5:
> > >    - Call xe_bo_recheck_purgeable_on_vma_unbind() from
> > > xe_vma_destroy()
> > >      right after drm_gpuva_unlink() where we already hold the BO lock,
> > >      drop the trylock-based late destroy path (Matt)
> > >    - Move purgeable_state into xe_vma_mem_attr with the other madvise
> > >      attributes (Matt)
> > >    - Drop READ_ONCE since the BO lock already protects us (Matt)
> > >    - Keep returning false when there are no VMAs - otherwise we'd mark
> > >      BOs purgeable without any user hint (Matt)
> > >    - Use xe_bo_set_purgeable_state() instead of direct
> > > initialization(Matt)
> > >    - use xe_assert instead of drm_war (Thomas)
> > Typo.
> 
> 
> Noted,
> 
> > 
> > There were also a couple of review issues in my reply here:
> > 
> > https://patchwork.freedesktop.org/patch/699451/?series=156651&rev=5
> > 
> > that were never addressed or at least commented upon.
> > 
> > The comment there on retaining purgeable state after the last vma is
> > unmapped could be discussed, though.
> > 
> > Let's say we unmap a vma marking a bo purgeable. It then becomes either
> > purged or non-purgeable.
> > 
> > Then an app tries to access it either using a new vma or CPU map. Then
> > it will typically succeed, or might occasionally fail if the bo
> > happened to be purged in between.
> > 
> > How do we handle new vma map requests and cpu-faults to a bo in
> > purgeable state? Do we block those?
> 
> 
> @Thomas,
> 
> The implementation already blocks new access to purged BOs:
>  1. New VMA mappings (Patch 0005): vma_lock_and_validate() rejects MAP
> operations to purged BOs with -EINVAL via the check_purged flag.
>  2. CPU faults (Patch 0004): Both xe_bo_cpu_prep() and xe_gem_mmap_offset()
> return errors (-EFAULT / VM_FAULT_SIGBUS) when accessing purged BOs.
>  3 . "Once purged, always purged": Even when the last VMA is unmapped,
> xe_bo_recompute_purgeable_state() preserves the PURGED state - it never
> transitions back to WILLNEED or DONTNEED (see early return at the top of the
> function).
> 
> The only way forward for the application is to destroy the purged BO and
> create a new one.
> 
> Regarding the 'no VMAs → WILLNEED' logic: this only applies to non-purged
> BOs that happen to be temporarily unmapped. Purged BOs remain permanently
> invalid.

So I think xe_bo_all_vmas_dontneed() isn't 100% correct...

I think should return an enum...

enum xe_bo_vmas_purge_state {	/* Maybe a better name? */
	XE_BO_VMAS_STATE_DONTNEED = 0,
	XE_BO_VMAS_STATE_WILLNEED = 1,
	XE_BO_VMAS_STATE_NO_VMAS = 2,
};


Then in xe_bo_recompute_purgeable_state() something like this:

void xe_bo_recompute_purgeable_state(struct xe_bo *bo)
{
	enum xe_bo_vma_purge_state state;

	if (!bo)
		return;

	xe_bo_assert_held(bo);

	/*
	 * Once purged, always purged. Cannot transition back to WILLNEED.
	 * This matches i915 semantics where purged BOs are permanently invalid.
	 */
	if (bo->madv_purgeable == XE_MADV_PURGEABLE_PURGED)
		return;

	state = xe_bo_all_vmas_dontneed(bo);
	if (state == XE_BO_VMAS_STATE_DONTNEED) {
		/* All VMAs are DONTNEED - mark BO purgeable */
		if (bo->madv_purgeable != XE_MADV_PURGEABLE_DONTNEED)
			xe_bo_set_purgeable_state(bo, XE_MADV_PURGEABLE_DONTNEED);
	} else if (state == XE_BO_VMAS_STATE_WILLNEED) {
		/* At least one VMA is WILLNEED - BO must not be purgeable */
		if (bo->madv_purgeable != XE_MADV_PURGEABLE_WILLNEED)
			xe_bo_set_purgeable_state(bo, XE_MADV_PURGEABLE_WILLNEED);
	}
}

I think would avoid the last unbind unintentionally flipping from
DONTNEED -> WILLNEED.

What do you both of you (Thomas, Arvind) think?

Matt

> 
> Thanks,
> Arvind
> > 
> > Thanks,
> > Thomas
> > 
> > 
> > 
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > Signed-off-by: Arvind Yadav <arvind.yadav@intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_svm.c        |  1 +
> > >   drivers/gpu/drm/xe/xe_vm.c         |  9 ++-
> > >   drivers/gpu/drm/xe/xe_vm_madvise.c | 98
> > > ++++++++++++++++++++++++++++--
> > >   drivers/gpu/drm/xe/xe_vm_madvise.h |  3 +
> > >   drivers/gpu/drm/xe/xe_vm_types.h   | 11 ++++
> > >   5 files changed, 116 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_svm.c
> > > b/drivers/gpu/drm/xe/xe_svm.c
> > > index cda3bf7e2418..329c77aa5c20 100644
> > > --- a/drivers/gpu/drm/xe/xe_svm.c
> > > +++ b/drivers/gpu/drm/xe/xe_svm.c
> > > @@ -318,6 +318,7 @@ static void xe_vma_set_default_attributes(struct
> > > xe_vma *vma)
> > >   		.preferred_loc.migration_policy =
> > > DRM_XE_MIGRATE_ALL_PAGES,
> > >   		.pat_index = vma->attr.default_pat_index,
> > >   		.atomic_access = DRM_XE_ATOMIC_UNDEFINED,
> > > +		.purgeable_state = XE_MADV_PURGEABLE_WILLNEED,
> > >   	};
> > >   	xe_vma_mem_attr_copy(&vma->attr, &default_attr);
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > > index 71cf3ce6c62b..e84b9e7cb5eb 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -39,6 +39,7 @@
> > >   #include "xe_tile.h"
> > >   #include "xe_tlb_inval.h"
> > >   #include "xe_trace_bo.h"
> > > +#include "xe_vm_madvise.h"
> > >   #include "xe_wa.h"
> > >   static struct drm_gem_object *xe_vm_obj(struct xe_vm *vm)
> > > @@ -1085,6 +1086,7 @@ static struct xe_vma *xe_vma_create(struct
> > > xe_vm *vm,
> > >   static void xe_vma_destroy_late(struct xe_vma *vma)
> > >   {
> > >   	struct xe_vm *vm = xe_vma_vm(vma);
> > > +	struct xe_bo *bo = xe_vma_bo(vma);
> > >   	if (vma->ufence) {
> > >   		xe_sync_ufence_put(vma->ufence);
> > > @@ -1099,7 +1101,7 @@ static void xe_vma_destroy_late(struct xe_vma
> > > *vma)
> > >   	} else if (xe_vma_is_null(vma) ||
> > > xe_vma_is_cpu_addr_mirror(vma)) {
> > >   		xe_vm_put(vm);
> > >   	} else {
> > > -		xe_bo_put(xe_vma_bo(vma));
> > > +		xe_bo_put(bo);
> > >   	}
> > >   	xe_vma_free(vma);
> > > @@ -1125,6 +1127,7 @@ static void vma_destroy_cb(struct dma_fence
> > > *fence,
> > >   static void xe_vma_destroy(struct xe_vma *vma, struct dma_fence
> > > *fence)
> > >   {
> > >   	struct xe_vm *vm = xe_vma_vm(vma);
> > > +	struct xe_bo *bo = xe_vma_bo(vma);
> > >   	lockdep_assert_held_write(&vm->lock);
> > >   	xe_assert(vm->xe, list_empty(&vma->combined_links.destroy));
> > > @@ -1133,9 +1136,10 @@ static void xe_vma_destroy(struct xe_vma *vma,
> > > struct dma_fence *fence)
> > >   		xe_assert(vm->xe, vma->gpuva.flags &
> > > XE_VMA_DESTROYED);
> > >   		xe_userptr_destroy(to_userptr_vma(vma));
> > >   	} else if (!xe_vma_is_null(vma) &&
> > > !xe_vma_is_cpu_addr_mirror(vma)) {
> > > -		xe_bo_assert_held(xe_vma_bo(vma));
> > > +		xe_bo_assert_held(bo);
> > >   		drm_gpuva_unlink(&vma->gpuva);
> > > +		xe_bo_recompute_purgeable_state(bo);
> > >   	}
> > >   	xe_vm_assert_held(vm);
> > > @@ -2681,6 +2685,7 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm
> > > *vm, struct drm_gpuva_ops *ops,
> > >   				.atomic_access =
> > > DRM_XE_ATOMIC_UNDEFINED,
> > >   				.default_pat_index = op-
> > > > map.pat_index,
> > >   				.pat_index = op->map.pat_index,
> > > +				.purgeable_state =
> > > XE_MADV_PURGEABLE_WILLNEED,
> > >   			};
> > >   			flags |= op->map.vma_flags &
> > > XE_VMA_CREATE_MASK;
> > > diff --git a/drivers/gpu/drm/xe/xe_vm_madvise.c
> > > b/drivers/gpu/drm/xe/xe_vm_madvise.c
> > > index d9cfba7bfe0b..c184426546a2 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm_madvise.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm_madvise.c
> > > @@ -12,6 +12,7 @@
> > >   #include "xe_pat.h"
> > >   #include "xe_pt.h"
> > >   #include "xe_svm.h"
> > > +#include "xe_vm.h"
> > >   struct xe_vmas_in_madvise_range {
> > >   	u64 addr;
> > > @@ -183,6 +184,89 @@ static void madvise_pat_index(struct xe_device
> > > *xe, struct xe_vm *vm,
> > >   	}
> > >   }
> > > +/**
> > > + * xe_bo_all_vmas_dontneed() - Check if all VMAs of a BO are marked
> > > DONTNEED
> > > + * @bo: Buffer object
> > > + *
> > > + * Check all VMAs across all VMs to determine if BO can be purged.
> > > + * Shared BOs require unanimous DONTNEED state from all mappings.
> > > + *
> > > + * Caller must hold BO dma-resv lock.
> > > + *
> > > + * Return: true if all VMAs are DONTNEED, false otherwise
> > > + */
> > > +static bool xe_bo_all_vmas_dontneed(struct xe_bo *bo)
> > > +{
> > > +	struct drm_gpuvm_bo *vm_bo;
> > > +	struct drm_gpuva *gpuva;
> > > +	struct drm_gem_object *obj = &bo->ttm.base;
> > > +	bool has_vmas = false;
> > > +
> > > +	xe_bo_assert_held(bo);
> > > +
> > > +	drm_gem_for_each_gpuvm_bo(vm_bo, obj) {
> > > +		drm_gpuvm_bo_for_each_va(gpuva, vm_bo) {
> > > +			struct xe_vma *vma = gpuva_to_vma(gpuva);
> > > +
> > > +			has_vmas = true;
> > > +
> > > +			/* Any non-DONTNEED VMA prevents purging */
> > > +			if (vma->attr.purgeable_state !=
> > > XE_MADV_PURGEABLE_DONTNEED)
> > > +				return false;
> > > +		}
> > > +	}
> > > +
> > > +	/*
> > > +	 * No VMAs => no mapping-level DONTNEED hint.
> > > +	 * Default to WILLNEED to avoid making BOs purgeable without
> > > +	 * explicit user intent.
> > > +	 */
> > > +	if (!has_vmas)
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +/**
> > > + * xe_bo_recompute_purgeable_state() - Recompute BO purgeable state
> > > from VMAs
> > > + * @bo: Buffer object
> > > + *
> > > + * Walk all VMAs to determine if BO should be purgeable or not.
> > > + * Shared BOs require unanimous DONTNEED state from all mappings.
> > > + *
> > > + * Locking: Caller must hold BO dma-resv lock. When iterating GPUVM
> > > lists,
> > > + * VM lock must also be held (write) to prevent concurrent VMA
> > > modifications.
> > > + * This is satisfied at both call sites:
> > > + * - xe_vma_destroy(): holds vm->lock write
> > > + * - madvise_purgeable(): holds vm->lock write (from madvise ioctl
> > > path)
> > > + *
> > > + * Return: nothing
> > > + */
> > > +void xe_bo_recompute_purgeable_state(struct xe_bo *bo)
> > > +{
> > > +	if (!bo)
> > > +		return;
> > > +
> > > +	xe_bo_assert_held(bo);
> > > +
> > > +	/*
> > > +	 * Once purged, always purged. Cannot transition back to
> > > WILLNEED.
> > > +	 * This matches i915 semantics where purged BOs are
> > > permanently invalid.
> > > +	 */
> > > +	if (bo->madv_purgeable == XE_MADV_PURGEABLE_PURGED)
> > > +		return;
> > > +
> > > +	if (xe_bo_all_vmas_dontneed(bo)) {
> > > +		/* All VMAs are DONTNEED - mark BO purgeable */
> > > +		if (bo->madv_purgeable !=
> > > XE_MADV_PURGEABLE_DONTNEED)
> > > +			xe_bo_set_purgeable_state(bo,
> > > XE_MADV_PURGEABLE_DONTNEED);
> > > +	} else {
> > > +		/* At least one VMA is WILLNEED - BO must not be
> > > purgeable */
> > > +		if (bo->madv_purgeable !=
> > > XE_MADV_PURGEABLE_WILLNEED)
> > > +			xe_bo_set_purgeable_state(bo,
> > > XE_MADV_PURGEABLE_WILLNEED);
> > > +	}
> > > +}
> > > +
> > >   /**
> > >    * madvise_purgeable - Handle purgeable buffer object advice
> > >    * @xe: XE device
> > > @@ -231,14 +315,20 @@ static void __maybe_unused
> > > madvise_purgeable(struct xe_device *xe,
> > >   		switch (op->purge_state_val.val) {
> > >   		case DRM_XE_VMA_PURGEABLE_STATE_WILLNEED:
> > > -			xe_bo_set_purgeable_state(bo,
> > > XE_MADV_PURGEABLE_WILLNEED);
> > > +			vmas[i]->attr.purgeable_state =
> > > XE_MADV_PURGEABLE_WILLNEED;
> > > +
> > > +			/* Update BO purgeable state */
> > > +			xe_bo_recompute_purgeable_state(bo);
> > >   			break;
> > >   		case DRM_XE_VMA_PURGEABLE_STATE_DONTNEED:
> > > -			xe_bo_set_purgeable_state(bo,
> > > XE_MADV_PURGEABLE_DONTNEED);
> > > +			vmas[i]->attr.purgeable_state =
> > > XE_MADV_PURGEABLE_DONTNEED;
> > > +
> > > +			/* Update BO purgeable state */
> > > +			xe_bo_recompute_purgeable_state(bo);
> > >   			break;
> > >   		default:
> > > -			drm_warn(&vm->xe->drm, "Invalid madvice
> > > value = %d\n",
> > > -				 op->purge_state_val.val);
> > > +			/* Should never hit - values validated in
> > > madvise_args_are_sane() */
> > > +			xe_assert(vm->xe, 0);
> > >   			return;
> > >   		}
> > >   	}
> > > diff --git a/drivers/gpu/drm/xe/xe_vm_madvise.h
> > > b/drivers/gpu/drm/xe/xe_vm_madvise.h
> > > index b0e1fc445f23..39acd2689ca0 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm_madvise.h
> > > +++ b/drivers/gpu/drm/xe/xe_vm_madvise.h
> > > @@ -8,8 +8,11 @@
> > >   struct drm_device;
> > >   struct drm_file;
> > > +struct xe_bo;
> > >   int xe_vm_madvise_ioctl(struct drm_device *dev, void *data,
> > >   			struct drm_file *file);
> > > +void xe_bo_recompute_purgeable_state(struct xe_bo *bo);
> > > +
> > >   #endif
> > > diff --git a/drivers/gpu/drm/xe/xe_vm_types.h
> > > b/drivers/gpu/drm/xe/xe_vm_types.h
> > > index 43203e90ee3e..fd563039e8f4 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> > > @@ -94,6 +94,17 @@ struct xe_vma_mem_attr {
> > >   	 * same as default_pat_index unless overwritten by madvise.
> > >   	 */
> > >   	u16 pat_index;
> > > +
> > > +	/**
> > > +	 * @purgeable_state: Purgeable hint for this VMA mapping
> > > +	 *
> > > +	 * Per-VMA purgeable state from madvise. Valid states are
> > > WILLNEED (0)
> > > +	 * or DONTNEED (1). Shared BOs require all VMAs to be
> > > DONTNEED before
> > > +	 * the BO can be purged. PURGED state exists only at BO
> > > level.
> > > +	 *
> > > +	 * Protected by BO dma-resv lock. Set via
> > > DRM_IOCTL_XE_MADVISE.
> > > +	 */
> > > +	u32 purgeable_state;
> > >   };
> > >   struct xe_vma {

  reply	other threads:[~2026-02-24 16:37 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-02-11 15:26 [PATCH v5 0/9] drm/xe/madvise: Add support for purgeable buffer objects Arvind Yadav
2026-02-11 15:26 ` [PATCH v5 1/9] drm/xe/uapi: Add UAPI " Arvind Yadav
2026-02-24 10:50   ` Thomas Hellström
2026-02-26 17:58   ` Souza, Jose
2026-02-27  9:32     ` Yadav, Arvind
2026-02-11 15:26 ` [PATCH v5 2/9] drm/xe/bo: Add purgeable bo state tracking and field madv to xe_bo Arvind Yadav
2026-02-11 16:00   ` Matthew Brost
2026-02-11 15:26 ` [PATCH v5 3/9] drm/xe/madvise: Implement purgeable buffer object support Arvind Yadav
2026-02-24 12:21   ` Thomas Hellström
2026-02-24 14:56     ` Yadav, Arvind
2026-02-11 15:26 ` [PATCH v5 4/9] drm/xe/bo: Handle CPU faults on purged buffer objects Arvind Yadav
2026-02-11 15:26 ` [PATCH v5 5/9] drm/xe/vm: Prevent binding of " Arvind Yadav
2026-02-11 16:17   ` Matthew Brost
2026-02-11 15:26 ` [PATCH v5 6/9] drm/xe/madvise: Implement per-VMA purgeable state tracking Arvind Yadav
2026-02-24 12:48   ` Thomas Hellström
2026-02-24 15:07     ` Yadav, Arvind
2026-02-24 16:36       ` Matthew Brost [this message]
2026-02-25  5:35         ` Yadav, Arvind
2026-02-25  8:21           ` Thomas Hellström
2026-02-25  9:04             ` Matthew Brost
2026-02-25  9:18               ` Thomas Hellström
2026-02-25  9:40                 ` Yadav, Arvind
2026-02-25 18:32                   ` Matthew Brost
2026-02-11 15:26 ` [PATCH v5 7/9] drm/xe/madvise: Block imported and exported dma-bufs Arvind Yadav
2026-02-24 14:15   ` Thomas Hellström
2026-02-11 15:26 ` [PATCH v5 8/9] drm/xe/bo: Add purgeable shrinker state helpers Arvind Yadav
2026-02-24 14:21   ` Thomas Hellström
2026-02-24 15:09     ` Yadav, Arvind
2026-02-11 15:26 ` [PATCH v5 9/9] drm/xe/madvise: Enable purgeable buffer object IOCTL support Arvind Yadav
2026-02-11 15:40   ` Matthew Brost
2026-02-11 15:46 ` [PATCH v5 0/9] drm/xe/madvise: Add support for purgeable buffer objects Matthew Brost
2026-02-25 10:10   ` Yadav, Arvind
2026-02-11 16:21 ` ✗ CI.checkpatch: warning for drm/xe/madvise: Add support for purgeable buffer objects (rev6) Patchwork
2026-02-11 16:22 ` ✓ CI.KUnit: success " Patchwork
2026-02-11 17:11 ` ✗ Xe.CI.BAT: failure " Patchwork
2026-02-13  1:15 ` ✗ Xe.CI.FULL: " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aZ3TpuSFKzY4y9sg@lstrano-desk.jf.intel.com \
    --to=matthew.brost@intel.com \
    --cc=arvind.yadav@intel.com \
    --cc=himal.prasad.ghimiray@intel.com \
    --cc=intel-xe@lists.freedesktop.org \
    --cc=pallavi.mishra@intel.com \
    --cc=thomas.hellstrom@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox