[PATCH v2 0/4] Some cleanup for page migration

[PATCH v2 0/4] Some cleanup for page migration

[Baolin Wang]

Hi,

This patch set did some cleanup and improvements for the page migration,
please help to review. Thanks a lot.

Changes from v1:
 - Add reviewed-by tags.
 - Add more comments for patch 1.
 - Drop unnecessary patch 5 from this patch set.

Baolin Wang (4):
  mm: migrate: Move the page count validation to the proper place
  mm: migrate: Introduce a local variable to get the number of pages
  mm: migrate: Fix the incorrect function name in comments
  mm: migrate: Change to use bool type for 'page_was_mapped'

 mm/migrate.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/4] mm: migrate: Move the page count validation to the proper place

[Baolin Wang]

We've got the expected count for anonymous page or file page by
expected_page_refs() at the beginning of migrate_page_move_mapping(),
thus we should move the page count validation a little forward to
reduce duplicated code.

Moreover the i_pages lock is not used to guarantee the page refcount
safety in migrate_page_move_mapping(), so we can move the getting page
count out of the i_pages lock. Since now the migration page has
established a migration pte under the page lock now, with the page
refcount freezing validation, to ensure that the page references
meet the migration requirement.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/migrate.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index c6eb2a8..8e9282f 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -386,11 +386,10 @@ int folio_migrate_mapping(struct address_space *mapping,
 	int expected_count = expected_page_refs(mapping, &folio->page) + extra_count;
 	long nr = folio_nr_pages(folio);
 
-	if (!mapping) {
-		/* Anonymous page without mapping */
-		if (folio_ref_count(folio) != expected_count)
-			return -EAGAIN;
+	if (folio_ref_count(folio) != expected_count)
+		return -EAGAIN;
 
+	if (!mapping) {
 		/* No turning back from here */
 		newfolio->index = folio->index;
 		newfolio->mapping = folio->mapping;
@@ -404,8 +403,7 @@ int folio_migrate_mapping(struct address_space *mapping,
 	newzone = folio_zone(newfolio);
 
 	xas_lock_irq(&xas);
-	if (folio_ref_count(folio) != expected_count ||
-	    xas_load(&xas) != folio) {
+	if (xas_load(&xas) != folio) {
 		xas_unlock_irq(&xas);
 		return -EAGAIN;
 	}
-- 
1.8.3.1

[PATCH v2 2/4] mm: migrate: Introduce a local variable to get the number of pages

[Baolin Wang]

Use thp_nr_pages() instead of compound_nr() to get the number of pages
for THP page, meanwhile introducing a local variable 'nr_pages' to
avoid getting the number of pages repeatedly.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
Reviewed-by: Yang Shi <shy828301@gmail.com>
---
 mm/migrate.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 8e9282f..9b162c0e 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2106,6 +2106,7 @@ static struct page *alloc_misplaced_dst_page_thp(struct page *page,
 static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 {
 	int page_lru;
+	int nr_pages = thp_nr_pages(page);
 
 	VM_BUG_ON_PAGE(compound_order(page) && !PageTransHuge(page), page);
 
@@ -2114,7 +2115,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 		return 0;
 
 	/* Avoid migrating to a node that is nearly full */
-	if (!migrate_balanced_pgdat(pgdat, compound_nr(page)))
+	if (!migrate_balanced_pgdat(pgdat, nr_pages))
 		return 0;
 
 	if (isolate_lru_page(page))
@@ -2122,7 +2123,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 
 	page_lru = page_is_file_lru(page);
 	mod_node_page_state(page_pgdat(page), NR_ISOLATED_ANON + page_lru,
-				thp_nr_pages(page));
+			    nr_pages);
 
 	/*
 	 * Isolating the page has taken another reference, so the
-- 
1.8.3.1

[PATCH v2 3/4] mm: migrate: Fix the incorrect function name in comments

[Baolin Wang]

since commit a98a2f0c8ce1 ("mm/rmap: split migration into its own function"),
the migration ptes establishment has been split into a separate
try_to_migrate() function, thus update the related comments.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
Reviewed-by: Yang Shi <shy828301@gmail.com>
Reviewed-by: Alistair Popple <apopple@nvidia.com>
---
 mm/migrate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 9b162c0e..433c083 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1005,7 +1005,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
 	}
 
 	/*
-	 * By try_to_unmap(), page->mapcount goes down to 0 here. In this case,
+	 * By try_to_migrate(), page->mapcount goes down to 0 here. In this case,
 	 * we cannot notice that anon_vma is freed while we migrates a page.
 	 * This get_anon_vma() delays freeing anon_vma pointer until the end
 	 * of migration. File cache pages are no problem because of page_lock()
-- 
1.8.3.1

[PATCH v2 4/4] mm: migrate: Change to use bool type for 'page_was_mapped'

[Baolin Wang]

Change to use bool type for 'page_was_mapped' variable making it
more readable.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
Reviewed-by: Yang Shi <shy828301@gmail.com>
---
 mm/migrate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 433c083..82ae6da 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -957,7 +957,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
 				int force, enum migrate_mode mode)
 {
 	int rc = -EAGAIN;
-	int page_was_mapped = 0;
+	bool page_was_mapped = false;
 	struct anon_vma *anon_vma = NULL;
 	bool is_lru = !__PageMovable(page);
 
@@ -1060,7 +1060,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
 		VM_BUG_ON_PAGE(PageAnon(page) && !PageKsm(page) && !anon_vma,
 				page);
 		try_to_migrate(page, 0);
-		page_was_mapped = 1;
+		page_was_mapped = true;
 	}
 
 	if (!page_mapped(page))
-- 
1.8.3.1

Re: [PATCH v2 1/4] mm: migrate: Move the page count validation to the proper place

[Matthew Wilcox]

On Sun, Aug 15, 2021 at 02:23:03PM +0800, Baolin Wang wrote:
> We've got the expected count for anonymous page or file page by
> expected_page_refs() at the beginning of migrate_page_move_mapping(),
> thus we should move the page count validation a little forward to
> reduce duplicated code.
> 
> Moreover the i_pages lock is not used to guarantee the page refcount
> safety in migrate_page_move_mapping(), so we can move the getting page
> count out of the i_pages lock. Since now the migration page has
> established a migration pte under the page lock now, with the page
> refcount freezing validation, to ensure that the page references
> meet the migration requirement.

I remain unconvinced by this.  

Looking at folio_migrate_mapping() a little more deeply, I don't
understand why we first check folio_ref_count() and then attempt
to free the refcount.  Why not just try to freeze it directly?

ie instead of your patch, this:

+++ b/mm/migrate.c
@@ -403,13 +403,8 @@ int folio_migrate_mapping(struct address_space *mapping,
        newzone = folio_zone(newfolio);

        xas_lock_irq(&xas);
-       if (folio_ref_count(folio) != expected_count ||
-           xas_load(&xas) != folio) {
-               xas_unlock_irq(&xas);
-               return -EAGAIN;
-       }
-
-       if (!folio_ref_freeze(folio, expected_count)) {
+       if (xas_load(&xas) != folio ||
+           !folio_ref_freeze(folio, expected_count)) {
                xas_unlock_irq(&xas);
                return -EAGAIN;
        }

And since we've got the lock on the page, how can somebody else be
removing it from the page cache?  I think that xas_load() can be
removed too.  So even more simply,

+++ b/mm/migrate.c
@@ -403,12 +403,6 @@ int folio_migrate_mapping(struct address_space *mapping,
        newzone = folio_zone(newfolio);

        xas_lock_irq(&xas);
-       if (folio_ref_count(folio) != expected_count ||
-           xas_load(&xas) != folio) {
-               xas_unlock_irq(&xas);
-               return -EAGAIN;
-       }
-
        if (!folio_ref_freeze(folio, expected_count)) {
                xas_unlock_irq(&xas);
                return -EAGAIN;

but I'm not really set up to test page migration.  Does your test suite
test migrating file-backed pages?

Re: [PATCH v2 1/4] mm: migrate: Move the page count validation to the proper place

[Baolin Wang]

On 2021/8/15 18:34, Matthew Wilcox wrote:
> On Sun, Aug 15, 2021 at 02:23:03PM +0800, Baolin Wang wrote:
>> We've got the expected count for anonymous page or file page by
>> expected_page_refs() at the beginning of migrate_page_move_mapping(),
>> thus we should move the page count validation a little forward to
>> reduce duplicated code.
>>
>> Moreover the i_pages lock is not used to guarantee the page refcount
>> safety in migrate_page_move_mapping(), so we can move the getting page
>> count out of the i_pages lock. Since now the migration page has
>> established a migration pte under the page lock now, with the page
>> refcount freezing validation, to ensure that the page references
>> meet the migration requirement.
> 
> I remain unconvinced by this.
> 
> Looking at folio_migrate_mapping() a little more deeply, I don't
> understand why we first check folio_ref_count() and then attempt
> to free the refcount.  Why not just try to freeze it directly?
> 
> ie instead of your patch, this:
> 
> +++ b/mm/migrate.c
> @@ -403,13 +403,8 @@ int folio_migrate_mapping(struct address_space *mapping,
>          newzone = folio_zone(newfolio);
> 
>          xas_lock_irq(&xas);
> -       if (folio_ref_count(folio) != expected_count ||
> -           xas_load(&xas) != folio) {
> -               xas_unlock_irq(&xas);
> -               return -EAGAIN;
> -       }
> -
> -       if (!folio_ref_freeze(folio, expected_count)) {
> +       if (xas_load(&xas) != folio ||
> +           !folio_ref_freeze(folio, expected_count)) {
>                  xas_unlock_irq(&xas);
>                  return -EAGAIN;
>          }

I think this is reasonable, like what we've done in __remove_mapping().

> And since we've got the lock on the page, how can somebody else be
> removing it from the page cache?  I think that xas_load() can be
> removed too.  So even more simply,

Good point, this is more simply.

> 
> +++ b/mm/migrate.c
> @@ -403,12 +403,6 @@ int folio_migrate_mapping(struct address_space *mapping,
>          newzone = folio_zone(newfolio);
> 
>          xas_lock_irq(&xas);
> -       if (folio_ref_count(folio) != expected_count ||
> -           xas_load(&xas) != folio) {
> -               xas_unlock_irq(&xas);
> -               return -EAGAIN;
> -       }
> -
>          if (!folio_ref_freeze(folio, expected_count)) {
>                  xas_unlock_irq(&xas);
>                  return -EAGAIN;
> 
> but I'm not really set up to test page migration.  Does your test suite
> test migrating file-backed pages?

Yes, I've tested above changes, and the mapped file pages migration 
works well. So can I resend this patch set with your Suggested-by tag? 
Thanks.