Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Alistair Popple <apopple@nvidia.com>
To:     linux-mm@kvack.org
Cc:     akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
        Alistair Popple <apopple@nvidia.com>,
        "Sierra Guiza, Alejandro (Alex)" <alex.sierra@amd.com>,
        Chaitanya Kulkarni <kch@nvidia.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Felix Kuehling <Felix.Kuehling@amd.com>,
        Jason Gunthorpe <jgg@nvidia.com>,
        John Hubbard <jhubbard@nvidia.com>,
        Logan Gunthorpe <logang@deltatee.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Muchun Song <songmuchun@bytedance.com>,
        Pasha Tatashin <pasha.tatashin@soleen.com>,
        Ralph Campbell <rcampbell@nvidia.com>
Subject: [PATCH] mm/gup.c: Refactor check_and_migrate_movable_pages()
Date:   Thu,  4 Aug 2022 13:22:41 +1000
Message-Id: <20220804032241.859891-1-apopple@nvidia.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?UpWjfcVvshv/oLREmResnbZQt/lvDswYKI6X24t3fA3HI1eJeIoOGf1d22a1?=
 =?us-ascii?Q?OI4iXXeao1l0qIlcPYTLX2BPwydjfpacacCUXUMoAdnzvmtCvFbKg623Pfrq?=
 =?us-ascii?Q?ij5yzi1NOLSEfTIK/M/pH+KlonlnKwAslJTtE/Q3EWJQmoSZUIXp+Mg/uuPg?=
 =?us-ascii?Q?Q9tRFVhdp0JB/QUkBGFehVAI8H7cEpk8F76v9Aj6cXIUDQQAkGOzGbphYdxg?=
 =?us-ascii?Q?3l5ik9L4IH3OWvI3ylrOEFax1uHsDeor6OIFxbG+b/IfJCOZXlCpoNZjOo0n?=
 =?us-ascii?Q?ryGT8zwaLOOGPVNnrwSkovKEnbSszHSMT3e5s+IepLh/GQLZuqMUHjrH8Qze?=
 =?us-ascii?Q?Tfs5OKrUXB2lZE9yCDqlvmAK0A2J0dKaI/kwETZJuNuwGltZ51PhVr2Ts7Ka?=
 =?us-ascii?Q?Uef8mG6pPzRoT6zyr4xiyGUoctbUfSXvZu93JXejp0BeqWx3cpdavaqNuvNc?=
 =?us-ascii?Q?Q8dIom3xUqzuojrj8ku0GscvnKKe85LsWSaKRv9JE0X4dOL6pLKr4Yw7guv3?=
 =?us-ascii?Q?xb21WglwVw5Bx0D6johhHhLWM0AtuZk27jnjaLqFXtQg39s+U/gEN1CbVxX/?=
 =?us-ascii?Q?mQ9o+QKNPS8vr3muSxaWAYSewgA0pYhNblInCSmJBhrx54/1dQvy3MeDK15G?=
 =?us-ascii?Q?bKbwR9J8B+QTFO6/rmpUAGOT3jdFWVDxPXFdEUBFo8riSZFTysg8ZIp6I3xA?=
 =?us-ascii?Q?4HWnsgI7to3FYZQ1SRnLuhZVGUuiG5ooi4iaNB4QDA+9+ppdOXgUpR8UaOy7?=
 =?us-ascii?Q?0+Z8+eFMa/y1gk2yBC1974Um/cIuB5pDiHdWgpI/qQVDZViKMd1Jw8N1HIip?=
 =?us-ascii?Q?ORTAp0OGi1IQh+1IbIKAWFM6e/IEdzKZrXk/Y4YDnDNJoS2McTd1pwyaLS+C?=
 =?us-ascii?Q?iG3Ajl/TeuOXASE0nloKW2IVwuxejZp0hR/9zIrW6A5V52yMSB5vDDiTRcqA?=
 =?us-ascii?Q?4BgQmqgV7E5tOUZglvljXqKInNv8SSxtY8Et5F5AdvsS7qJpZBtHBEy8/RKw?=
 =?us-ascii?Q?Mce0Pr2MAzgIiHml3OXSZ8twSEbyqIpR71msuiVcYhTKrRwXfj/RE0Y0weZC?=
 =?us-ascii?Q?aEZIF7tkglfHsujzOe0LzMtiIH7GDLb83FlkywGThMR7wKZbWbOS52jF/xxA?=
 =?us-ascii?Q?oFpQGCJCmNqJY5mJh26g1kz0ILpuanjVxBUwYh75bd4ab3C5uRZadRLQbdm1?=
 =?us-ascii?Q?RLXDkpExjj7LuX4wgCaEtEmQwe3FEh0Uc8Bt6zFJNe/6ARYcxmu+ucPvIYA0?=
 =?us-ascii?Q?Q4bpVqAqkKpVuKKhLrNQyYm4T+9XeyjcgeKpF9yVd895FKcC42r+BYFC562G?=
 =?us-ascii?Q?+GlohrxPJHj7m3u9+jfTjM7lZZOnN8WBYIzFTYMbdnMiawZTm2DJCgHHtFFD?=
 =?us-ascii?Q?Re71Zv2ZxWXAmnoL7FLDdZEMCH67spr6XD6ijm9Iu0PQtPkAXMzpm/mKLd73?=
 =?us-ascii?Q?/s361TXWWXJfHE/IP2I1RLzWJLO2ZxyZiD1+aso7YwrI/uj3vQX03ZkZGgm0?=
 =?us-ascii?Q?nVI8CX6A2HPk6Uznu69H4NAWXse7FuFYjhGxjfxki0lZg4mfcQaCRUkmxq4T?=
 =?us-ascii?Q?F/2GxbQkgQ6qjp9kZbj4nvD0phi/zGCNhlfcRT2T?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3a7411ce-1d32-4a16-6fb1-08da75c89f04
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3176.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Aug 2022 03:22:54.9998
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WikvfDg7JXiKWJHwr+Hv2eYJaSYQgWlOV93gqvpjfKLfRu9tapOzgmVumLCnCMAXsi1MigpnRhv5Vb1XhQF8FQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4988
X-Spam-Status: No, score=-1.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

When pinning pages with FOLL_LONGTERM check_and_migrate_movable_pages()
is called to migrate pages out of zones which should not contain any
longterm pinned pages.

When migration succeeds all pages will have been unpinned so pinning
needs to be retried. Migration can also fail, in which case the pages
will also have been unpinned but the operation should not be retried. If
all pages are in the correct zone nothing will be unpinned and no retry
is required.

The logic in check_and_migrate_movable_pages() tracks unnecessary state
and the return codes for each case are difficult to follow. Refactor the
code to clean this up. No behaviour change is intended.

Signed-off-by: Alistair Popple <apopple@nvidia.com>
Cc: "Sierra Guiza, Alejandro (Alex)" <alex.sierra@amd.com>
Cc: Chaitanya Kulkarni <kch@nvidia.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Felix Kuehling <Felix.Kuehling@amd.com>
Cc: Jason Gunthorpe <jgg@nvidia.com>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Cc: Miaohe Lin <linmiaohe@huawei.com>
Cc: Muchun Song <songmuchun@bytedance.com>
Cc: Pasha Tatashin <pasha.tatashin@soleen.com>
Cc: Ralph Campbell <rcampbell@nvidia.com>

---

Originally posted as "mm/gup.c: Simplify and fix
check_and_migrate_movable_pages() return codes"[1].

Changes from that version:

 - Restore the original isolation failure behaviour and don't fail the
   pup. Instead retry indefinitely.
 - Unpin all pages on retry or failure rather than just failure.

Jason - I dropped your Reviewed-by. I had to remove the changes to make
error handling follow convention as we need to always unpin the pages.
We also need the list_empty() checks because we may or may not have
pages in the list if we found coherent pages. So there isn't much I
could see to simplify, but let me know if you spot some.

[1] https://lore.kernel.org/linux-mm/814dee5d3aadd38c3370eaaf438ba7eee9bf9d2b.1659399696.git-series.apopple@nvidia.com/
---
 mm/gup.c | 61 +++++++++++++++++++++++++-------------------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index c6d060dee9e0..5ce0106a92f8 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1901,20 +1901,20 @@ struct page *get_dump_page(unsigned long addr)
 
 #ifdef CONFIG_MIGRATION
 /*
- * Check whether all pages are pinnable, if so return number of pages.  If some
- * pages are not pinnable, migrate them, and unpin all pages. Return zero if
- * pages were migrated, or if some pages were not successfully isolated.
- * Return negative error if migration fails.
+ * Check whether all pages are pinnable. If some pages are not pinnable migrate
+ * them and unpin all the pages. Returns -EAGAIN if pages were unpinned or zero
+ * if all pages are pinnable and in the right zone. Other errors indicate
+ * migration failure.
  */
 static long check_and_migrate_movable_pages(unsigned long nr_pages,
 					    struct page **pages,
 					    unsigned int gup_flags)
 {
-	unsigned long isolation_error_count = 0, i;
+	unsigned long i;
 	struct folio *prev_folio = NULL;
 	LIST_HEAD(movable_page_list);
 	bool drain_allow = true, coherent_pages = false;
-	int ret = 0;
+	int ret = -EAGAIN;
 
 	for (i = 0; i < nr_pages; i++) {
 		struct folio *folio = page_folio(pages[i]);
@@ -1946,10 +1946,10 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 				unpin_user_page(&folio->page);
 			}
 
-			ret = migrate_device_coherent_page(&folio->page);
-			if (ret)
-				goto unpin_pages;
-
+			if (migrate_device_coherent_page(&folio->page)) {
+				ret = -EBUSY;
+				break;
+			}
 			continue;
 		}
 
@@ -1959,9 +1959,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 		 * Try to move out any movable page before pinning the range.
 		 */
 		if (folio_test_hugetlb(folio)) {
-			if (isolate_hugetlb(&folio->page,
-						&movable_page_list))
-				isolation_error_count++;
+			isolate_hugetlb(&folio->page, &movable_page_list);
 			continue;
 		}
 
@@ -1970,29 +1968,25 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 			drain_allow = false;
 		}
 
-		if (folio_isolate_lru(folio)) {
-			isolation_error_count++;
+		if (folio_isolate_lru(folio))
 			continue;
-		}
+
 		list_add_tail(&folio->lru, &movable_page_list);
 		node_stat_mod_folio(folio,
 				    NR_ISOLATED_ANON + folio_is_file_lru(folio),
 				    folio_nr_pages(folio));
 	}
 
-	if (!list_empty(&movable_page_list) || isolation_error_count ||
-	    coherent_pages)
-		goto unpin_pages;
-
 	/*
-	 * If list is empty, and no isolation errors, means that all pages are
-	 * in the correct zone.
+	 * All pages are still pinned and in the correct zone.
 	 */
-	return nr_pages;
+	if (list_empty(&movable_page_list) && !coherent_pages)
+		return 0;
 
-unpin_pages:
 	/*
-	 * pages[i] might be NULL if any device coherent pages were found.
+	 * Unpin all pages. If device coherent pages were found
+	 * migrate_deivce_coherent_page() will have already dropped the pin and
+	 * set pages[i] == NULL.
 	 */
 	for (i = 0; i < nr_pages; i++) {
 		if (!pages[i])
@@ -2010,15 +2004,14 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 			.gfp_mask = GFP_USER | __GFP_NOWARN,
 		};
 
-		ret = migrate_pages(&movable_page_list, alloc_migration_target,
-				    NULL, (unsigned long)&mtc, MIGRATE_SYNC,
-				    MR_LONGTERM_PIN, NULL);
-		if (ret > 0) /* number of pages not migrated */
+		if (migrate_pages(&movable_page_list, alloc_migration_target,
+					NULL, (unsigned long)&mtc, MIGRATE_SYNC,
+					MR_LONGTERM_PIN, NULL))
 			ret = -ENOMEM;
-	}
 
-	if (ret && !list_empty(&movable_page_list))
 		putback_movable_pages(&movable_page_list);
+	}
+
 	return ret;
 }
 #else
@@ -2026,7 +2019,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 					    struct page **pages,
 					    unsigned int gup_flags)
 {
-	return nr_pages;
+	return 0;
 }
 #endif /* CONFIG_MIGRATION */
 
@@ -2054,10 +2047,10 @@ static long __gup_longterm_locked(struct mm_struct *mm,
 		if (rc <= 0)
 			break;
 		rc = check_and_migrate_movable_pages(rc, pages, gup_flags);
-	} while (!rc);
+	} while (rc == -EAGAIN);
 	memalloc_pin_restore(flags);
 
-	return rc;
+	return rc ? rc : nr_pages;
 }
 
 static bool is_valid_gup_flags(unsigned int gup_flags)
-- 
2.35.1