Re: [PATCH] mm/migrate.c: Remove MIGRATE_PFN_LOCKED

On Tuesday, 26 October 2021 11:57:06 AM AEDT Ralph Campbell wrote:

On 10/24/21 21:16, Alistair Popple wrote:

quoted

MIGRATE_PFN_LOCKED is used to indicate to migrate_vma_prepare() that a
source page was already locked during migrate_vma_collect(). If it
wasn't then the a second attempt is made to lock the page. However if
the first attempt failed it's unlikely a second attempt will succeed,
and the retry adds complexity. So clean this up by removing the retry
and MIGRATE_PFN_LOCKED flag.

Destination pages are also meant to have the MIGRATE_PFN_LOCKED flag
set, but nothing actually checks that.

Signed-off-by: Alistair Popple <apopple@nvidia.com>

You can add:
Reviewed-by: Ralph Campbell <redacted>

Thanks!

quoted

  
  		/*
  		 * Optimize for the common case where page is only mapped once

@@ -2379,7 +2378,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
  		if (trylock_page(page)) {
  			pte_t swp_pte;
  
-			mpfn |= MIGRATE_PFN_LOCKED;
+			migrate->cpages++;
  			ptep_get_and_clear(mm, addr, ptep);

I was looking at try_to_migrate_one() and looking at the differences with
the code here to insert the migration PTE and noticed that instead of
ptet_get_and_clear() it has:
	pteval = ptep_clear_flush(vma, address, pvmw.pte);

	/* Move the dirty bit to the page. Now the pte is gone. */
	if (pte_dirty(pteval))
		set_page_dirty(page);
	update_hiwater_rss(mm);

I know that is pre-existing, probably a separate patch if it is an issue.

I don't think it is an issue today because migrate_vma only supports private,
non-shared pages. At some point though it may be extended to support
file-backed pages and this would be easy to miss so will put a patch together.

quoted

  
  			/* Setup special migration page table entry */

@@ -2413,6 +2412,9 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
  
  			if (pte_present(pte))
  				unmapped++;
+		} else {
+			put_page(page);
+			mpfn = 0;
  		}
  
  next:

@@ -2517,15 +2519,17 @@ static bool migrate_vma_check_page(struct page *page)
  }
  
  /*
- * migrate_vma_prepare() - lock pages and isolate them from the lru
+ * migrate_vma_unmap() - replace page mapping with special migration pte entry
   * @migrate: migrate struct containing all migration information
   *
- * This locks pages that have been collected by migrate_vma_collect(). Once each
- * page is locked it is isolated from the lru (for non-device pages). Finally,
- * the ref taken by migrate_vma_collect() is dropped, as locked pages cannot be
- * migrated by concurrent kernel threads.
+ * Isolate pages from the LRU and replace mappings (CPU page table pte) with a
+ * special migration pte entry and check if it has been pinned. Pinned pages are
+ * restored because we cannot migrate them.
+ *
+ * This is the last step before we call the device driver callback to allocate
+ * destination memory and copy contents of original page over to new page.
   */
-static void migrate_vma_prepare(struct migrate_vma *migrate)
+static void migrate_vma_unmap(struct migrate_vma *migrate)
  {
  	const unsigned long npages = migrate->npages;
  	const unsigned long start = migrate->start;

@@ -2534,32 +2538,12 @@ static void migrate_vma_prepare(struct migrate_vma *migrate)
  
  	lru_add_drain();
  
-	for (i = 0; (i < npages) && migrate->cpages; i++) {
+	for (i = 0; i < npages; i++) {
  		struct page *page = migrate_pfn_to_page(migrate->src[i]);
-		bool remap = true;
  
  		if (!page)
  			continue;
  
-		if (!(migrate->src[i] & MIGRATE_PFN_LOCKED)) {
-			/*
-			 * Because we are migrating several pages there can be
-			 * a deadlock between 2 concurrent migration where each
-			 * are waiting on each other page lock.
-			 *
-			 * Make migrate_vma() a best effort thing and backoff
-			 * for any page we can not lock right away.
-			 */
-			if (!trylock_page(page)) {
-				migrate->src[i] = 0;
-				migrate->cpages--;
-				put_page(page);
-				continue;
-			}
-			remap = false;
-			migrate->src[i] |= MIGRATE_PFN_LOCKED;
-		}
-
  		/* ZONE_DEVICE pages are not on LRU */
  		if (!is_zone_device_page(page)) {
  			if (!PageLRU(page) && allow_drain) {

@@ -2569,16 +2553,9 @@ static void migrate_vma_prepare(struct migrate_vma *migrate)
  			}
  
  			if (isolate_lru_page(page)) {
-				if (remap) {
-					migrate->src[i] &= ~MIGRATE_PFN_MIGRATE;
-					migrate->cpages--;
-					restore++;
-				} else {
-					migrate->src[i] = 0;
-					unlock_page(page);
-					migrate->cpages--;
-					put_page(page);
-				}
+				migrate->src[i] &= ~MIGRATE_PFN_MIGRATE;
+				migrate->cpages--;
+				restore++;
  				continue;
  			}
  

@@ -2586,80 +2563,20 @@ static void migrate_vma_prepare(struct migrate_vma *migrate)
  			put_page(page);
  		}
  
-		if (!migrate_vma_check_page(page)) {
-			if (remap) {
-				migrate->src[i] &= ~MIGRATE_PFN_MIGRATE;
-				migrate->cpages--;
-				restore++;
-
-				if (!is_zone_device_page(page)) {
-					get_page(page);
-					putback_lru_page(page);
-				}
-			} else {
-				migrate->src[i] = 0;
-				unlock_page(page);
-				migrate->cpages--;
+		if (page_mapped(page))
+			try_to_migrate(page, 0);
  
-				if (!is_zone_device_page(page))
-					putback_lru_page(page);
-				else
-					put_page(page);
+		if (page_mapped(page) || !migrate_vma_check_page(page)) {
+			if (!is_zone_device_page(page)) {
+				get_page(page);
+				putback_lru_page(page);
  			}
-		}
-	}
-
-	for (i = 0, addr = start; i < npages && restore; i++, addr += PAGE_SIZE) {
-		struct page *page = migrate_pfn_to_page(migrate->src[i]);
-
-		if (!page || (migrate->src[i] & MIGRATE_PFN_MIGRATE))
-			continue;
  
-		remove_migration_pte(page, migrate->vma, addr, page);
-
-		migrate->src[i] = 0;
-		unlock_page(page);
-		put_page(page);
-		restore--;
-	}
-}
-
-/*
- * migrate_vma_unmap() - replace page mapping with special migration pte entry
- * @migrate: migrate struct containing all migration information
- *
- * Replace page mapping (CPU page table pte) with a special migration pte entry
- * and check again if it has been pinned. Pinned pages are restored because we
- * cannot migrate them.
- *
- * This is the last step before we call the device driver callback to allocate
- * destination memory and copy contents of original page over to new page.
- */
-static void migrate_vma_unmap(struct migrate_vma *migrate)
-{
-	const unsigned long npages = migrate->npages;
-	const unsigned long start = migrate->start;
-	unsigned long addr, i, restore = 0;
-
-	for (i = 0; i < npages; i++) {
-		struct page *page = migrate_pfn_to_page(migrate->src[i]);
-
-		if (!page || !(migrate->src[i] & MIGRATE_PFN_MIGRATE))
+			migrate->src[i] &= ~MIGRATE_PFN_MIGRATE;
+			migrate->cpages--;
+			restore++;
  			continue;
-
-		if (page_mapped(page)) {
-			try_to_migrate(page, 0);
-			if (page_mapped(page))
-				goto restore;
  		}
-
-		if (migrate_vma_check_page(page))
-			continue;
-
-restore:
-		migrate->src[i] &= ~MIGRATE_PFN_MIGRATE;
-		migrate->cpages--;
-		restore++;
  	}
  
  	for (addr = start, i = 0; i < npages && restore; addr += PAGE_SIZE, i++) {

@@ -2672,12 +2589,8 @@ static void migrate_vma_unmap(struct migrate_vma *migrate)
  
  		migrate->src[i] = 0;
  		unlock_page(page);
+		put_page(page);
  		restore--;
-
-		if (is_zone_device_page(page))
-			put_page(page);
-		else
-			putback_lru_page(page);
  	}
  }
  

@@ -2700,8 +2613,8 @@ static void migrate_vma_unmap(struct migrate_vma *migrate)
   * it for all those entries (ie with MIGRATE_PFN_VALID and MIGRATE_PFN_MIGRATE
   * flag set).  Once these are allocated and copied, the caller must update each
   * corresponding entry in the dst array with the pfn value of the destination
- * page and with the MIGRATE_PFN_VALID and MIGRATE_PFN_LOCKED flags set
- * (destination pages must have their struct pages locked, via lock_page()).
+ * page and with MIGRATE_PFN_VALID. Destination pages must be locked via
+ * lock_page().
   *
   * Note that the caller does not have to migrate all the pages that are marked
   * with MIGRATE_PFN_MIGRATE flag in src array unless this is a migration from

@@ -2770,8 +2683,6 @@ int migrate_vma_setup(struct migrate_vma *args)
  
  	migrate_vma_collect(args);
  
-	if (args->cpages)
-		migrate_vma_prepare(args);
  	if (args->cpages)
  		migrate_vma_unmap(args);