Re: [PATCH] drm/pagemap: Prevent double migration of device pages

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

On Tue, Aug 04, 2026 at 10:12:10AM +0530, Yadav, Arvind wrote:
> 
> On 03-08-2026 23:24, Matthew Brost wrote:
> > On Mon, Aug 03, 2026 at 10:46:27AM -0700, Matthew Brost wrote:
> > > On Mon, Aug 03, 2026 at 02:55:53PM +0530, Arvind Yadav wrote:
> > > > A device page migrated to system memory by a CPU fault can remain
> > > > referenced for a short time after migration completes. During this
> > > > window, the raw-PFN eviction path can select the same device PFN and
> > > > migrate it again.
> > > > 
> > > So is the race a CPU immediately followed by an evict?
> 
> 
> Yes. The CPU-fault migration completes first, then eviction selects the same
> device folio before its remaining reference is dropped.
> 
> > > 
> > > > The first migration has already moved the memcg charge away from the
> > > > source folio. Migrating that source again can create an uncharged system
> > > > folio. Adding such a folio to the LRU can spin indefinitely in
> > > > folio_lruvec_lock_irqsave(), resulting in a soft lockup and an RCU stall.
> > > > 
> > > Do you have stack trace of this lockup? It would be good include that in
> > > this commit message.
> 
> 
> Yes, I have the trace. I will add in next version.
> 
> > > 
> > > > Track successfully migrated device PFNs in drm_pagemap_zdd for the
> > > > lifetime of the device-mapping generation.
> > > > 
> > > > Record successful migrations in both the CPU-fault and raw-PFN eviction
> > > > paths.
> > > > 
> > > > Make raw-PFN eviction skip retired PFNs, preventing an already migrated
> > > > device page from being handed to the migration path a second time.
> > > > 
> > > > Fixes: 99624bdff867 ("drm/gpusvm: Add support for GPU Shared Virtual Memory")
> > > > 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: Matthew Brost <matthew.brost@intel.com>
> > > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > > Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > > Assisted-by: Claude:claude-opus-4-8
> > > > Signed-off-by: Arvind Yadav <arvind.yadav@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/drm_pagemap.c | 188 +++++++++++++++++++++++++++++++++-
> > > >   1 file changed, 186 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_pagemap.c b/drivers/gpu/drm/drm_pagemap.c
> > > > index 7a056592ac66..f7040fc0dea6 100644
> > > > --- a/drivers/gpu/drm/drm_pagemap.c
> > > > +++ b/drivers/gpu/drm/drm_pagemap.c
> > > > @@ -7,6 +7,7 @@
> > > >   #include <linux/dma-mapping.h>
> > > >   #include <linux/migrate.h>
> > > >   #include <linux/pagemap.h>
> > > > +#include <linux/xarray.h>
> > > >   #include <drm/drm_drv.h>
> > > >   #include <drm/drm_pagemap.h>
> > > >   #include <drm/drm_pagemap_util.h>
> > > > @@ -66,6 +67,8 @@
> > > >    * @refcount: Reference count for the zdd
> > > >    * @devmem_allocation: device memory allocation
> > > >    * @dpagemap: Refcounted pointer to the underlying struct drm_pagemap.
> > > > + * @retired: Device PFNs already migrated to RAM. Entries remain until this
> > > > + * mapping generation is destroyed.
> > > >    *
> > > >    * This structure serves as a generic wrapper installed in
> > > >    * page->zone_device_data. It provides infrastructure for looking up a device
> > > > @@ -78,6 +81,7 @@ struct drm_pagemap_zdd {
> > > >   	struct kref refcount;
> > > >   	struct drm_pagemap_devmem *devmem_allocation;
> > > >   	struct drm_pagemap *dpagemap;
> > > > +	struct xarray retired;
> > > I don't think an xarray is the right data structure here, given that
> > > load/store operations are slower than direct memory loads and stores.
> > > 
> > > The CPU fault path is about as critical a code path as we can get, so I
> > > think it needs to be highly optimized.
> > > 
> > > I believe a bitmap [1] is the right data structure.
> > > 
> > > include/linux/bitmap.h
> > > 
> > > I'd suggest using an embedded bitmap here, sized based on the ZDD size.
> > > 
> > > For example:
> > > 
> > > /* last member */
> > > unsigned long retire_map[];
> > > Show more lines
> > > 4K, 64K -> length 1
> > > 2M -> length 8
> > > 
> > > Lastly, the only bits that are ever checked are those corresponding to
> > > the folio order. For example, if the folio order is 9, only bit 0 is set
> > > and checked, and the check loop increments based on the folio order.
> 
> 
> Agreed. I will replace the XArray with an embedded bitmap sized for the ZDD
> allocation and remove the reservation helpers.
> 
> > > >   };
> > > >   /**
> > > > @@ -101,6 +105,7 @@ drm_pagemap_zdd_alloc(struct drm_pagemap *dpagemap)
> > > >   	kref_init(&zdd->refcount);
> > > >   	zdd->devmem_allocation = NULL;
> > > >   	zdd->dpagemap = drm_pagemap_get(dpagemap);
> > > > +	xa_init(&zdd->retired);
> > > >   	return zdd;
> > > >   }
> > > > @@ -137,6 +142,7 @@ static void drm_pagemap_zdd_destroy(struct kref *ref)
> > > >   		if (devmem->ops->devmem_release)
> > > >   			devmem->ops->devmem_release(devmem);
> > > >   	}
> > > > +	xa_destroy(&zdd->retired);
> > > >   	kfree(zdd);
> > > >   	drm_pagemap_put(dpagemap);
> > > >   }
> > > > @@ -1102,12 +1108,169 @@ void drm_pagemap_put(struct drm_pagemap *dpagemap)
> > > >   }
> > > >   EXPORT_SYMBOL(drm_pagemap_put);
> > > > +/**
> > > > + * drm_pagemap_is_devmem_page() - Is @page a drm_pagemap device page
> > > > + * @page: The page to test
> > > > + *
> > > > + * Return: true for device-private or device-coherent pages, which carry a
> > > > + * struct drm_pagemap_zdd in their zone_device_data.
> > > > + */
> > > > +static bool drm_pagemap_is_devmem_page(const struct page *page)
> > > > +{
> > > > +	return is_device_private_page(page) || is_device_coherent_page(page);
> > > > +}
> > > > +
> > > > +static void
> > > > +drm_pagemap_release_retired_reservations(unsigned long *src_pfns,
> > > > +					 unsigned long npages)
> > > > +{
> > > This function won't be needed with above.
> 
> 
> Noted,
> 
> > > 
> > > > +	unsigned long i = 0;
> > > > +
> > > > +	while (i < npages) {
> > > > +		struct page *page = migrate_pfn_to_page(src_pfns[i]);
> > > > +		struct drm_pagemap_zdd *zdd;
> > > > +		struct folio *folio;
> > > > +		unsigned long pfn, nr, j;
> > > > +
> > > > +		if (!page || !(src_pfns[i] & MIGRATE_PFN_MIGRATE) ||
> > > > +		    !drm_pagemap_is_devmem_page(page)) {
> > > > +			i++;
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		folio = page_folio(page);
> > > > +		zdd = drm_pagemap_page_zone_device_data(page);
> > > > +		pfn = folio_pfn(folio);
> > > > +		nr = folio_nr_pages(folio);
> > > > +
> > > > +		for (j = 0; j < nr; j++)
> > > > +			xa_release(&zdd->retired, pfn + j);
> > > > +
> > > > +		i += nr;
> > > > +	}
> > > > +}
> > > > +
> > > > +/**
> > > > + * drm_pagemap_reserve_retired_pages() - Pre-reserve retirement slots
> > > > + * @src_pfns: migrate_vma source array after migrate_vma_setup()
> > > > + * @npages: number of entries in @src_pfns
> > > > + *
> > > > + * Reserve every base PFN because migration may split a large source
> > > > + * folio. Recording the result must not allocate.
> > > > + */
> > > > +static int drm_pagemap_reserve_retired_pages(unsigned long *src_pfns,
> > > > +					     unsigned long npages)
> > > This function won't be needed.
> 
> 
> Noted,
> 
> > > 
> > > > +{
> > > This function will look something like:
> > > 
> > > 	unsigned long i = 0;
> > > 	int err;
> > > 
> > > 	while (i < npages) {
> > > 		struct page *page = migrate_pfn_to_page(src_pfns[i]);
> > > 		struct folio *folio;
> > > 		struct drm_pagemap_zdd *zdd;
> > > 		unsigned long pfn, nr;
> > > 
> > > 		if (!page || !drm_pagemap_is_devmem_page(page))
> > > 			continue;
> > > 
> > > 		folio = page_folio(page);
> > > 		nr = folio_nr_pages(folio);
> > > 
> > > 		if (!(src_pfns[i] & MIGRATE_PFN_MIGRATE)) {
> > > 			i += nr;
> > > 			continue;
> > > 		}
> > > 
> > > 		zdd = drm_pagemap_page_zone_device_data(page);
> > > 		bitmap_set(zdd->retire_map, i, 1);
> > > 		i += nr;
> > > 	}
> > > 
> > > 	return 0;
> > ^^^
> > 
> > Opps, copy paste error. This function isn't needed and the above snippet
> > is for the function below (include there in previous reply).
> 
> 
> Noted,
> 
> > 
> > > > +	unsigned long i = 0;
> > > > +	int err;
> > > > +
> > > > +	while (i < npages) {
> > > > +		struct page *page = migrate_pfn_to_page(src_pfns[i]);
> > > > +		struct drm_pagemap_zdd *zdd;
> > > > +		unsigned long pfn, nr, k;
> > > > +
> > > > +		if (!page || !(src_pfns[i] & MIGRATE_PFN_MIGRATE) ||
> > > > +		    !drm_pagemap_is_devmem_page(page)) {
> > > > +			i++;
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		zdd = drm_pagemap_page_zone_device_data(page);
> > > > +		pfn = folio_pfn(page_folio(page));
> > > > +		nr = folio_nr_pages(page_folio(page));
> > > > +
> > > > +		for (k = 0; k < nr; k++) {
> > > > +			err = xa_reserve(&zdd->retired, pfn + k, GFP_KERNEL);
> > > > +			if (err) {
> > > > +				drm_pagemap_release_retired_reservations(src_pfns,
> > > > +									 npages);
> > > > +				return err;
> > > > +			}
> > > > +		}
> > > > +
> > > > +		i += nr;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * drm_pagemap_retire_migrated_pages() - Retire CPU-migrated device PFNs
> > > > + * @src_pfns: migrate_vma source array, valid after migrate_vma_pages()
> > > > + * @npages: number of entries in @src_pfns
> > > > + *
> > > > + * Record successful migrations before finalize unlocks the sources.
> > > > + * Release reservations for pages that were not migrated.
> > > > + */
> > > > +static void drm_pagemap_retire_migrated_pages(unsigned long *src_pfns,
> > > > +					      unsigned long npages)
> > > > +{
> > > > +	unsigned long i = 0;
> > > > +
> > > 
> > > This function will look something like:
> > > 
> > > 	unsigned long i = 0;
> > > 	int err;
> > > 
> > > 	while (i < npages) {
> > > 		struct page *page = migrate_pfn_to_page(src_pfns[i]);
> > > 		struct folio *folio;
> > > 		struct drm_pagemap_zdd *zdd;
> > > 		unsigned long pfn, nr;
> > > 
> > > 		if (!page || !drm_pagemap_is_devmem_page(page))
> > > 			continue;
> > > 
> > > 		folio = page_folio(page);
> > > 		nr = folio_nr_pages(folio);
> > > 
> > > 		if (!(src_pfns[i] & MIGRATE_PFN_MIGRATE)) {
> > > 			i += nr;
> > > 			continue;
> > > 		}
> > > 
> > > 		zdd = drm_pagemap_page_zone_device_data(page);
> > > 		WARN_ON_ONCE(__test_and_set_bit(i, zdd->retire_map));		
> > > 		i += nr;
> > > 	}
> > > 
> > > 	return 0;
> > > 
> > > 
> > > 
> > > > +	while (i < npages) {
> > > > +		struct page *page = migrate_pfn_to_page(src_pfns[i]);
> > > > +		struct drm_pagemap_zdd *zdd;
> > > > +		unsigned long pfn, nr, k;
> > > > +		bool migrated;
> > > > +
> > > > +		if (!page || !drm_pagemap_is_devmem_page(page)) {
> > > > +			i++;
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		zdd = drm_pagemap_page_zone_device_data(page);
> > > > +		pfn = folio_pfn(page_folio(page));
> > > > +		nr = folio_nr_pages(page_folio(page));
> > > > +		migrated = src_pfns[i] & MIGRATE_PFN_MIGRATE;
> > > > +
> > > > +		/* Keep later folio splits covered. */
> > > > +		for (k = 0; k < nr; k++) {
> > > > +			if (migrated)
> > > > +				WARN_ON_ONCE(xa_err(xa_store(&zdd->retired,
> > > > +							     pfn + k,
> > > > +							     xa_mk_value(1),
> > > > +							     GFP_NOWAIT)));
> > > > +			else
> > > > +				xa_release(&zdd->retired, pfn + k);
> > > > +		}
> > > > +
> > > > +		i += nr;
> > > > +	}
> > > > +}
> > > > +
> > > > +/**
> > > > + * drm_pagemap_skip_retired_pages() - Drop retired PFNs from a raw-PFN eviction
> > > > + * @src_pfns: source array after migrate_device_pfns() (MIGRATE_PFN encoded)
> > > > + * @npages: number of entries in @src_pfns
> > > > + *
> > > > + * Skip source PFNs already migrated to RAM by either migration path.
> > > > + */
> > > > +static void drm_pagemap_skip_retired_pages(unsigned long *src_pfns,
> > > > +					   unsigned long npages)
> > > > +{
> > > > +	unsigned long i;
> > > > +
> > > > +	for (i = 0; i < npages; i++) {
> > > > +		struct page *page = migrate_pfn_to_page(src_pfns[i]);
> > > > +		struct drm_pagemap_zdd *zdd;
> > > > +
> > > > +		if (!page || !(src_pfns[i] & MIGRATE_PFN_MIGRATE) ||
> > > > +		    !drm_pagemap_is_devmem_page(page))
> > > > +			continue;
> > > > +
> > > > +		zdd = drm_pagemap_page_zone_device_data(page);
> > > > +		if (xa_load(&zdd->retired, folio_pfn(page_folio(page))))
> > > if (__test_and_set_bit(i, zdd->retire_map))
> > > 
> > > > +			src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;
> > > Iterate based on nr.
> 
> 
> Agreed on iterating by folio order. I will use test_bit() here and set the

Yes, test_bit is better.

Matt

> bit only after successful migration, so a failed migration is not left
> falsely retired.
> 
> Thanks,
> Arvind
> 
> > > 
> > > Matt
> > > 
> > > > +	}
> > > > +}
> > > > +
> > > >   /**
> > > >    * drm_pagemap_evict_to_ram() - Evict GPU SVM range to RAM
> > > >    * @devmem_allocation: Pointer to the device memory allocation
> > > >    *
> > > > - * Similar to __drm_pagemap_migrate_to_ram but does not require mmap lock and
> > > > - * migration done via migrate_device_* functions.
> > > > + * Similar to __drm_pagemap_migrate_to_ram(), but uses the
> > > > + * migrate_device_* helpers and does not require the mmap lock. Device
> > > > + * PFNs already migrated by a CPU fault are skipped.
> > > >    *
> > > >    * Return: 0 on success, negative error code on failure.
> > > >    */
> > > > @@ -1149,6 +1312,17 @@ int drm_pagemap_evict_to_ram(struct drm_pagemap_devmem *devmem_allocation)
> > > >   	if (err)
> > > >   		goto err_free;
> > > > +	drm_pagemap_skip_retired_pages(src, npages);
> > > > +
> > > > +	/*
> > > > +	 * Reserve retirement entries before migration so recording successful
> > > > +	 * PFNs cannot fail. Otherwise, a retry could select and migrate the same
> > > > +	 * PFN again.
> > > > +	 */
> > > > +	err = drm_pagemap_reserve_retired_pages(src, npages);
> > > > +	if (err)
> > > > +		goto err_finalize;
> > > > +
> > > >   	err = drm_pagemap_migrate_populate_ram_pfn(NULL, NULL, npages, &mpages,
> > > >   						   src, dst, 0);
> > > >   	if (err || !mpages)
> > > > @@ -1179,6 +1353,7 @@ int drm_pagemap_evict_to_ram(struct drm_pagemap_devmem *devmem_allocation)
> > > >   	if (err)
> > > >   		drm_pagemap_migration_unlock_put_pages(npages, dst);
> > > >   	migrate_device_pages(src, dst, npages);
> > > > +	drm_pagemap_retire_migrated_pages(src, npages);
> > > >   	migrate_device_finalize(src, dst, npages);
> > > >   	drm_pagemap_migrate_unmap_pages(devmem_allocation->dev, pagemap_addr, dst, npages,
> > > >   					DMA_FROM_DEVICE, &state);
> > > > @@ -1276,6 +1451,14 @@ static int __drm_pagemap_migrate_to_ram(struct vm_area_struct *vas,
> > > >   	if (!migrate.cpages)
> > > >   		goto err_free;
> > > > +	/*
> > > > +	 * Reserve retirement entries before migration so recording successful
> > > > +	 * PFNs cannot fail. On failure, finalize can still restore the sources.
> > > > +	 */
> > > > +	err = drm_pagemap_reserve_retired_pages(migrate.src, npages);
> > > > +	if (err)
> > > > +		goto err_finalize;
> > > > +
> > > >   	ops = zdd->devmem_allocation->ops;
> > > >   	dev = zdd->devmem_allocation->dev;
> > > > @@ -1309,6 +1492,7 @@ static int __drm_pagemap_migrate_to_ram(struct vm_area_struct *vas,
> > > >   	if (err)
> > > >   		drm_pagemap_migration_unlock_put_pages(npages, migrate.dst);
> > > >   	migrate_vma_pages(&migrate);
> > > > +	drm_pagemap_retire_migrated_pages(migrate.src, npages);
> > > >   	migrate_vma_finalize(&migrate);
> > > >   	if (dev)
> > > >   		drm_pagemap_migrate_unmap_pages(dev, pagemap_addr, migrate.dst,
> > > > -- 
> > > > 2.43.0
> > > >