[PATCH 0/4] mm: migrate: more folio conversion

[PATCH 0/4] mm: migrate: more folio conversion

[Kefeng Wang]

This patch series converts several functions to use folio in migrate.c.

Kefeng Wang (4):
  mm: migrate: use a folio in add_page_for_migration()
  mm: migrate: convert numamigrate_isolate_page() to
    numamigrate_isolate_folio()
  mm: migrate: make migrate_misplaced_page() to take a folio
  mm: migrate: use __folio_test_movable()

 mm/migrate.c | 101 +++++++++++++++++++++++++--------------------------
 1 file changed, 50 insertions(+), 51 deletions(-)

-- 
2.41.0

[PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Kefeng Wang]

Use a folio in add_page_for_migration() to save compound_head() calls.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/migrate.c | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index e21d5a7e7447..b0c318bc531c 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2057,6 +2057,7 @@ static int add_page_for_migration(struct mm_struct *mm, const void __user *p,
 	struct vm_area_struct *vma;
 	unsigned long addr;
 	struct page *page;
+	struct folio *folio;
 	int err;
 	bool isolated;
 
@@ -2079,45 +2080,42 @@ static int add_page_for_migration(struct mm_struct *mm, const void __user *p,
 	if (!page)
 		goto out;
 
-	if (is_zone_device_page(page))
-		goto out_putpage;
+	folio = page_folio(page);
+	if (folio_is_zone_device(folio))
+		goto out_putfolio;
 
 	err = 0;
-	if (page_to_nid(page) == node)
-		goto out_putpage;
+	if (folio_nid(folio) == node)
+		goto out_putfolio;
 
 	err = -EACCES;
-	if (page_mapcount(page) > 1 && !migrate_all)
-		goto out_putpage;
+	if (folio_estimated_sharers(folio) > 1 && !migrate_all)
+		goto out_putfolio;
 
-	if (PageHuge(page)) {
-		if (PageHead(page)) {
-			isolated = isolate_hugetlb(page_folio(page), pagelist);
+	if (folio_test_hugetlb(folio)) {
+		if (folio_test_large(folio)) {
+			isolated = isolate_hugetlb(folio, pagelist);
 			err = isolated ? 1 : -EBUSY;
 		}
 	} else {
-		struct page *head;
-
-		head = compound_head(page);
-		isolated = isolate_lru_page(head);
+		isolated = folio_isolate_lru(folio);
 		if (!isolated) {
 			err = -EBUSY;
-			goto out_putpage;
+			goto out_putfolio;
 		}
 
 		err = 1;
-		list_add_tail(&head->lru, pagelist);
-		mod_node_page_state(page_pgdat(head),
-			NR_ISOLATED_ANON + page_is_file_lru(head),
-			thp_nr_pages(head));
+		list_add_tail(&folio->lru, pagelist);
+		node_stat_mod_folio(folio,
+			NR_ISOLATED_ANON + folio_is_file_lru(folio),
+			folio_nr_pages(folio));
 	}
-out_putpage:
+out_putfolio:
 	/*
-	 * Either remove the duplicate refcount from
-	 * isolate_lru_page() or drop the page ref if it was
-	 * not isolated.
+	 * Either remove the duplicate refcount from folio_isolate_lru()
+	 * or drop the folio ref if it was not isolated.
 	 */
-	put_page(page);
+	folio_put(folio);
 out:
 	mmap_read_unlock(mm);
 	return err;
-- 
2.41.0

[PATCH 2/4] mm: migrate: convert numamigrate_isolate_page() to numamigrate_isolate_folio()

[Kefeng Wang]

Rename numamigrate_isolate_page() to numamigrate_isolate_folio(), then
make it takes a folio and use folio API to save compound_head() calls.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/migrate.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index b0c318bc531c..39e0d35bccb3 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2476,15 +2476,15 @@ static struct folio *alloc_misplaced_dst_folio(struct folio *src,
 	return __folio_alloc_node(gfp, order, nid);
 }
 
-static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
+static int numamigrate_isolate_folio(pg_data_t *pgdat, struct folio *folio)
 {
-	int nr_pages = thp_nr_pages(page);
-	int order = compound_order(page);
+	int nr_pages = folio_nr_pages(folio);
+	int order = folio_order(folio);
 
-	VM_BUG_ON_PAGE(order && !PageTransHuge(page), page);
+	VM_BUG_ON_FOLIO(order && !folio_test_pmd_mappable(folio), folio);
 
 	/* Do not migrate THP mapped by multiple processes */
-	if (PageTransHuge(page) && total_mapcount(page) > 1)
+	if (folio_test_pmd_mappable(folio) && folio_estimated_sharers(folio) > 1)
 		return 0;
 
 	/* Avoid migrating to a node that is nearly full */
@@ -2501,18 +2501,18 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 		return 0;
 	}
 
-	if (!isolate_lru_page(page))
+	if (!folio_isolate_lru(folio))
 		return 0;
 
-	mod_node_page_state(page_pgdat(page), NR_ISOLATED_ANON + page_is_file_lru(page),
+	node_stat_mod_folio(folio, NR_ISOLATED_ANON + folio_is_file_lru(folio),
 			    nr_pages);
 
 	/*
-	 * Isolating the page has taken another reference, so the
-	 * caller's reference can be safely dropped without the page
+	 * Isolating the folio has taken another reference, so the
+	 * caller's reference can be safely dropped without the folio
 	 * disappearing underneath us during migration.
 	 */
-	put_page(page);
+	folio_put(folio);
 	return 1;
 }
 
@@ -2546,7 +2546,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
 	if (page_is_file_lru(page) && PageDirty(page))
 		goto out;
 
-	isolated = numamigrate_isolate_page(pgdat, page);
+	isolated = numamigrate_isolate_folio(pgdat, page_folio(page));
 	if (!isolated)
 		goto out;
 
-- 
2.41.0

[PATCH 3/4] mm: migrate: make migrate_misplaced_page() to take a folio

[Kefeng Wang]

Make migrate_misplaced_page() to take a folio and use folio API
to save compound_head() calls.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/migrate.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 39e0d35bccb3..4be61f944cac 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2525,17 +2525,18 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
 			   int node)
 {
 	pg_data_t *pgdat = NODE_DATA(node);
+	struct folio *folio = page_folio(page);
 	int isolated;
 	int nr_remaining;
 	unsigned int nr_succeeded;
 	LIST_HEAD(migratepages);
-	int nr_pages = thp_nr_pages(page);
+	int nr_pages = folio_nr_pages(folio);
 
 	/*
 	 * Don't migrate file pages that are mapped in multiple processes
 	 * with execute permissions as they are probably shared libraries.
 	 */
-	if (page_mapcount(page) != 1 && page_is_file_lru(page) &&
+	if (folio_estimated_sharers(folio) != 1 && folio_is_file_lru(folio) &&
 	    (vma->vm_flags & VM_EXEC))
 		goto out;
 
@@ -2543,29 +2544,29 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
 	 * Also do not migrate dirty pages as not all filesystems can move
 	 * dirty pages in MIGRATE_ASYNC mode which is a waste of cycles.
 	 */
-	if (page_is_file_lru(page) && PageDirty(page))
+	if (folio_is_file_lru(folio) && folio_test_dirty(folio))
 		goto out;
 
-	isolated = numamigrate_isolate_folio(pgdat, page_folio(page));
+	isolated = numamigrate_isolate_folio(pgdat, folio);
 	if (!isolated)
 		goto out;
 
-	list_add(&page->lru, &migratepages);
+	list_add(&folio->lru, &migratepages);
 	nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_folio,
 				     NULL, node, MIGRATE_ASYNC,
 				     MR_NUMA_MISPLACED, &nr_succeeded);
 	if (nr_remaining) {
 		if (!list_empty(&migratepages)) {
-			list_del(&page->lru);
-			mod_node_page_state(page_pgdat(page), NR_ISOLATED_ANON +
-					page_is_file_lru(page), -nr_pages);
-			putback_lru_page(page);
+			list_del(&folio->lru);
+			node_stat_mod_folio(folio, NR_ISOLATED_ANON +
+					folio_is_file_lru(folio), -nr_pages);
+			folio_putback_lru(folio);
 		}
 		isolated = 0;
 	}
 	if (nr_succeeded) {
 		count_vm_numa_events(NUMA_PAGE_MIGRATE, nr_succeeded);
-		if (!node_is_toptier(page_to_nid(page)) && node_is_toptier(node))
+		if (!node_is_toptier(folio_nid(folio)) && node_is_toptier(node))
 			mod_node_page_state(pgdat, PGPROMOTE_SUCCESS,
 					    nr_succeeded);
 	}
@@ -2573,7 +2574,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
 	return isolated;
 
 out:
-	put_page(page);
+	folio_put(folio);
 	return 0;
 }
 #endif /* CONFIG_NUMA_BALANCING */
-- 
2.41.0

[PATCH 4/4] mm: migrate: use __folio_test_movable()

[Kefeng Wang]

Use __folio_test_movable(), no need to convert from folio to page.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/migrate.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 4be61f944cac..8590e6f68cf3 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -157,8 +157,8 @@ void putback_movable_pages(struct list_head *l)
 		list_del(&folio->lru);
 		/*
 		 * We isolated non-lru movable folio so here we can use
-		 * __PageMovable because LRU folio's mapping cannot have
-		 * PAGE_MAPPING_MOVABLE.
+		 * __folio_test_movable because LRU folio's mapping cannot
+		 * have PAGE_MAPPING_MOVABLE.
 		 */
 		if (unlikely(__folio_test_movable(folio))) {
 			VM_BUG_ON_FOLIO(!folio_test_isolated(folio), folio);
@@ -943,7 +943,7 @@ static int move_to_new_folio(struct folio *dst, struct folio *src,
 				enum migrate_mode mode)
 {
 	int rc = -EAGAIN;
-	bool is_lru = !__PageMovable(&src->page);
+	bool is_lru = !__folio_test_movable(src);
 
 	VM_BUG_ON_FOLIO(!folio_test_locked(src), src);
 	VM_BUG_ON_FOLIO(!folio_test_locked(dst), dst);
@@ -990,7 +990,7 @@ static int move_to_new_folio(struct folio *dst, struct folio *src,
 	 * src is freed; but stats require that PageAnon be left as PageAnon.
 	 */
 	if (rc == MIGRATEPAGE_SUCCESS) {
-		if (__PageMovable(&src->page)) {
+		if (__folio_test_movable(src)) {
 			VM_BUG_ON_FOLIO(!folio_test_isolated(src), src);
 
 			/*
@@ -1082,7 +1082,7 @@ static void migrate_folio_done(struct folio *src,
 	/*
 	 * Compaction can migrate also non-LRU pages which are
 	 * not accounted to NR_ISOLATED_*. They can be recognized
-	 * as __PageMovable
+	 * as __folio_test_movable
 	 */
 	if (likely(!__folio_test_movable(src)))
 		mod_node_page_state(folio_pgdat(src), NR_ISOLATED_ANON +
@@ -1103,7 +1103,7 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
 	int rc = -EAGAIN;
 	int page_was_mapped = 0;
 	struct anon_vma *anon_vma = NULL;
-	bool is_lru = !__PageMovable(&src->page);
+	bool is_lru = !__folio_test_movable(src);
 	bool locked = false;
 	bool dst_locked = false;
 
@@ -1261,7 +1261,7 @@ static int migrate_folio_move(free_folio_t put_new_folio, unsigned long private,
 	int rc;
 	int page_was_mapped = 0;
 	struct anon_vma *anon_vma = NULL;
-	bool is_lru = !__PageMovable(&src->page);
+	bool is_lru = !__folio_test_movable(src);
 	struct list_head *prev;
 
 	__migrate_folio_extract(dst, &page_was_mapped, &anon_vma);
-- 
2.41.0

Re: [PATCH 0/4] mm: migrate: more folio conversion

[David Hildenbrand]

On 02.08.23 11:53, Kefeng Wang wrote:
> This patch series converts several functions to use folio in migrate.c.

Please clearly spell out in the patch descriptions when you are changing 
mapcount logic and which effects that might have (both, positive and 
negative) for PTE-mapped THP.

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Matthew Wilcox]

On Wed, Aug 02, 2023 at 05:53:43PM +0800, Kefeng Wang wrote:
>  	err = -EACCES;
> -	if (page_mapcount(page) > 1 && !migrate_all)
> -		goto out_putpage;
> +	if (folio_estimated_sharers(folio) > 1 && !migrate_all)
> +		goto out_putfolio;

I do not think this is the correct change.  Maybe leave this line
alone.

> -	if (PageHuge(page)) {
> -		if (PageHead(page)) {
> -			isolated = isolate_hugetlb(page_folio(page), pagelist);
> +	if (folio_test_hugetlb(folio)) {
> +		if (folio_test_large(folio)) {

This makes no sense when you read it.  All hugetlb folios are large,
by definition.  Think about what this code used to do, and what it
should be changed to.

> +			isolated = isolate_hugetlb(folio, pagelist);
>  			err = isolated ? 1 : -EBUSY;
>  		}
>  	} else {
> -		struct page *head;
> -
> -		head = compound_head(page);
> -		isolated = isolate_lru_page(head);
> +		isolated = folio_isolate_lru(folio);
>  		if (!isolated) {
>  			err = -EBUSY;
> -			goto out_putpage;
> +			goto out_putfolio;
>  		}
>  
>  		err = 1;
> -		list_add_tail(&head->lru, pagelist);
> -		mod_node_page_state(page_pgdat(head),
> -			NR_ISOLATED_ANON + page_is_file_lru(head),
> -			thp_nr_pages(head));
> +		list_add_tail(&folio->lru, pagelist);
> +		node_stat_mod_folio(folio,
> +			NR_ISOLATED_ANON + folio_is_file_lru(folio),
> +			folio_nr_pages(folio));
>  	}

Re: [PATCH 2/4] mm: migrate: convert numamigrate_isolate_page() to numamigrate_isolate_folio()

[Matthew Wilcox]

On Wed, Aug 02, 2023 at 05:53:44PM +0800, Kefeng Wang wrote:
> -static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
> +static int numamigrate_isolate_folio(pg_data_t *pgdat, struct folio *folio)
>  {
> -	int nr_pages = thp_nr_pages(page);
> -	int order = compound_order(page);
> +	int nr_pages = folio_nr_pages(folio);
> +	int order = folio_order(folio);
>  
> -	VM_BUG_ON_PAGE(order && !PageTransHuge(page), page);
> +	VM_BUG_ON_FOLIO(order && !folio_test_pmd_mappable(folio), folio);

I don't know why we have this assertion.  I would be inclined to delete
it as part of generalising the migration code to handle arbitrary sizes
of folio, rather than assert that we only support PMD size folios.

>  	/* Do not migrate THP mapped by multiple processes */
> -	if (PageTransHuge(page) && total_mapcount(page) > 1)
> +	if (folio_test_pmd_mappable(folio) && folio_estimated_sharers(folio) > 1)
>  		return 0;

I don't know if this is the right logic.  We've willing to move folios
mapped by multiple processes, as long as they're smaller than PMD size,
but once they get to PMD size they're magical and can't be moved?

Re: [PATCH 4/4] mm: migrate: use __folio_test_movable()

[Matthew Wilcox]

On Wed, Aug 02, 2023 at 05:53:46PM +0800, Kefeng Wang wrote:
> Use __folio_test_movable(), no need to convert from folio to page.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>

In my defence, I did many of these function conversions before
__folio_test_movable() existed ;-)

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Re: [PATCH 0/4] mm: migrate: more folio conversion

[Kefeng Wang]

On 2023/8/2 19:47, David Hildenbrand wrote:
> On 02.08.23 11:53, Kefeng Wang wrote:
>> This patch series converts several functions to use folio in migrate.c.
> 
> Please clearly spell out in the patch descriptions when you are changing 
> mapcount logic and which effects that might have (both, positive and 
> negative) for PTE-mapped THP.

Oh, I see your comments on another mail[1], the folio_estimated_sharers()
is not good to evaluate the sharing by multiple processes, will read more
history of the mail's discussion, maybe just ignore the first three patches.

Thank.


[1] [PATCH 0/2] don't use mapcount() to check large folio sharing
> 

Re: [PATCH 4/4] mm: migrate: use __folio_test_movable()

[David Hildenbrand]

On 02.08.23 11:53, Kefeng Wang wrote:
> Use __folio_test_movable(), no need to convert from folio to page.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/4] mm: migrate: convert numamigrate_isolate_page() to numamigrate_isolate_folio()

[Kefeng Wang]

On 2023/8/2 20:30, Matthew Wilcox wrote:
> On Wed, Aug 02, 2023 at 05:53:44PM +0800, Kefeng Wang wrote:
>> -static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>> +static int numamigrate_isolate_folio(pg_data_t *pgdat, struct folio *folio)
>>   {
>> -	int nr_pages = thp_nr_pages(page);
>> -	int order = compound_order(page);
>> +	int nr_pages = folio_nr_pages(folio);
>> +	int order = folio_order(folio);
>>   
>> -	VM_BUG_ON_PAGE(order && !PageTransHuge(page), page);
>> +	VM_BUG_ON_FOLIO(order && !folio_test_pmd_mappable(folio), folio);
> 
> I don't know why we have this assertion.  I would be inclined to delete
> it as part of generalising the migration code to handle arbitrary sizes
> of folio, rather than assert that we only support PMD size folios.

Ok, will drop it.
> 
>>   	/* Do not migrate THP mapped by multiple processes */
>> -	if (PageTransHuge(page) && total_mapcount(page) > 1)
>> +	if (folio_test_pmd_mappable(folio) && folio_estimated_sharers(folio) > 1)
>>   		return 0;
> 
> I don't know if this is the right logic.  We've willing to move folios
> mapped by multiple processes, as long as they're smaller than PMD size,
> but once they get to PMD size they're magical and can't be moved?

It seems that the logical is introduced by commit 04fa5d6a6547 ("mm:
migrate: check page_count of THP before migrating") and refactor by
340ef3902cf2 ("mm: numa: cleanup flow of transhuge page migration"),


   "Hugh Dickins pointed out that migrate_misplaced_transhuge_page() does
   not check page_count before migrating like base page migration and
   khugepage.  He could not see why this was safe and he is right."

For now, there is no migrate_misplaced_transhuge_page() and base/thp
page migrate's path is unified, there is a check(for old/new kernel) in 
migrate_misplaced_page(),

  "Don't migrate file pages that are mapped in multiple processes
  with execute permissions as they are probably shared libraries."

We could drop the above check in numamigrate_isolate_page(), but
according to 04fa5d6a6547, maybe disable migrate page shared by
multi-process during numa balance for both base/thp page.


> 
> 

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Kefeng Wang]

On 2023/8/2 20:21, Matthew Wilcox wrote:
> On Wed, Aug 02, 2023 at 05:53:43PM +0800, Kefeng Wang wrote:
>>   	err = -EACCES;
>> -	if (page_mapcount(page) > 1 && !migrate_all)
>> -		goto out_putpage;
>> +	if (folio_estimated_sharers(folio) > 1 && !migrate_all)
>> +		goto out_putfolio;
> 
> I do not think this is the correct change.  Maybe leave this line
> alone.

Ok, I am aware of the discussion about this in other mail, will not
change it(also the next two patch about this function), or wait the
new work of David.
> 
>> -	if (PageHuge(page)) {
>> -		if (PageHead(page)) {
>> -			isolated = isolate_hugetlb(page_folio(page), pagelist);
>> +	if (folio_test_hugetlb(folio)) {
>> +		if (folio_test_large(folio)) {
> 
> This makes no sense when you read it.  All hugetlb folios are large,
> by definition.  Think about what this code used to do, and what it
> should be changed to.

hugetlb folio is self large folio, will drop redundant check

Re: [PATCH 0/4] mm: migrate: more folio conversion

[David Hildenbrand]

On 02.08.23 14:38, Kefeng Wang wrote:
> 
> 
> On 2023/8/2 19:47, David Hildenbrand wrote:
>> On 02.08.23 11:53, Kefeng Wang wrote:
>>> This patch series converts several functions to use folio in migrate.c.
>>
>> Please clearly spell out in the patch descriptions when you are changing
>> mapcount logic and which effects that might have (both, positive and
>> negative) for PTE-mapped THP.
> 
> Oh, I see your comments on another mail[1], the folio_estimated_sharers()
> is not good to evaluate the sharing by multiple processes, will read more
> history of the mail's discussion, maybe just ignore the first three patches.

It might be good enough for some cases right now. My point is that we 
better just clearly spell out the possible effects of such a change.

(so far you didn't even mention them, and that's sub-optimal when buried 
in a "folio" conversion that better shouldn't change the functionality 
without spelling it out)

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Matthew Wilcox]

On Thu, Aug 03, 2023 at 03:13:21PM +0800, Kefeng Wang wrote:
> 
> 
> On 2023/8/2 20:21, Matthew Wilcox wrote:
> > On Wed, Aug 02, 2023 at 05:53:43PM +0800, Kefeng Wang wrote:
> > >   	err = -EACCES;
> > > -	if (page_mapcount(page) > 1 && !migrate_all)
> > > -		goto out_putpage;
> > > +	if (folio_estimated_sharers(folio) > 1 && !migrate_all)
> > > +		goto out_putfolio;
> > 
> > I do not think this is the correct change.  Maybe leave this line
> > alone.
> 
> Ok, I am aware of the discussion about this in other mail, will not
> change it(also the next two patch about this function), or wait the
> new work of David.
> > 
> > > -	if (PageHuge(page)) {
> > > -		if (PageHead(page)) {
> > > -			isolated = isolate_hugetlb(page_folio(page), pagelist);
> > > +	if (folio_test_hugetlb(folio)) {
> > > +		if (folio_test_large(folio)) {
> > 
> > This makes no sense when you read it.  All hugetlb folios are large,
> > by definition.  Think about what this code used to do, and what it
> > should be changed to.
> 
> hugetlb folio is self large folio, will drop redundant check

No, that's not the difference.  Keep thinking about it.  This is not
a mechanical translation!

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Kefeng Wang]

On 2023/8/3 20:30, Matthew Wilcox wrote:
> On Thu, Aug 03, 2023 at 03:13:21PM +0800, Kefeng Wang wrote:
>>
>>
>> On 2023/8/2 20:21, Matthew Wilcox wrote:
>>> On Wed, Aug 02, 2023 at 05:53:43PM +0800, Kefeng Wang wrote:
>>>>    	err = -EACCES;
>>>> -	if (page_mapcount(page) > 1 && !migrate_all)
>>>> -		goto out_putpage;
>>>> +	if (folio_estimated_sharers(folio) > 1 && !migrate_all)
>>>> +		goto out_putfolio;
>>>
>>> I do not think this is the correct change.  Maybe leave this line
>>> alone.
>>
>> Ok, I am aware of the discussion about this in other mail, will not
>> change it(also the next two patch about this function), or wait the
>> new work of David.
>>>
>>>> -	if (PageHuge(page)) {
>>>> -		if (PageHead(page)) {
>>>> -			isolated = isolate_hugetlb(page_folio(page), pagelist);
>>>> +	if (folio_test_hugetlb(folio)) {
>>>> +		if (folio_test_large(folio)) {
>>>
>>> This makes no sense when you read it.  All hugetlb folios are large,
>>> by definition.  Think about what this code used to do, and what it
>>> should be changed to.
>>
>> hugetlb folio is self large folio, will drop redundant check
> 
> No, that's not the difference.  Keep thinking about it.  This is not
> a mechanical translation!


   if (PageHuge(page))  // page must be a hugetlb page
	if (PageHead(page)) // page must be a head page, not tail
              isolate_hugetlb() // isolate the hugetlb page if head

After using folio,

   if (folio_test_hugetlb(folio)) // only check folio is hugetlb or not

I don't check the page is head or not, since the follow_page could
return a sub-page, so the check PageHead need be retained, right?

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 2411 bytes --]

On 3 Aug 2023, at 21:45, Kefeng Wang wrote:

> On 2023/8/3 20:30, Matthew Wilcox wrote:
>> On Thu, Aug 03, 2023 at 03:13:21PM +0800, Kefeng Wang wrote:
>>>
>>>
>>> On 2023/8/2 20:21, Matthew Wilcox wrote:
>>>> On Wed, Aug 02, 2023 at 05:53:43PM +0800, Kefeng Wang wrote:
>>>>>    	err = -EACCES;
>>>>> -	if (page_mapcount(page) > 1 && !migrate_all)
>>>>> -		goto out_putpage;
>>>>> +	if (folio_estimated_sharers(folio) > 1 && !migrate_all)
>>>>> +		goto out_putfolio;
>>>>
>>>> I do not think this is the correct change.  Maybe leave this line
>>>> alone.
>>>
>>> Ok, I am aware of the discussion about this in other mail, will not
>>> change it(also the next two patch about this function), or wait the
>>> new work of David.
>>>>
>>>>> -	if (PageHuge(page)) {
>>>>> -		if (PageHead(page)) {
>>>>> -			isolated = isolate_hugetlb(page_folio(page), pagelist);
>>>>> +	if (folio_test_hugetlb(folio)) {
>>>>> +		if (folio_test_large(folio)) {
>>>>
>>>> This makes no sense when you read it.  All hugetlb folios are large,
>>>> by definition.  Think about what this code used to do, and what it
>>>> should be changed to.
>>>
>>> hugetlb folio is self large folio, will drop redundant check
>>
>> No, that's not the difference.  Keep thinking about it.  This is not
>> a mechanical translation!
>
>
>   if (PageHuge(page))  // page must be a hugetlb page
> 	if (PageHead(page)) // page must be a head page, not tail
>              isolate_hugetlb() // isolate the hugetlb page if head
>
> After using folio,
>
>   if (folio_test_hugetlb(folio)) // only check folio is hugetlb or not
>
> I don't check the page is head or not, since the follow_page could
> return a sub-page, so the check PageHead need be retained, right?

Right. It will prevent the kernel from trying to isolate the same hugetlb page
twice when two pages are in the same hugetlb folio. But looking at the
code, if you try to isolate an already-isolated hugetlb folio, isolate_hugetlb()
would return false, no error would show up. But it changes err value
from -EACCES to -EBUSY and user will see a different page status than before.

I wonder why we do not have follow_folio() and returns -ENOENT error pointer
when addr points to a non head page. It would make this patch more folio if
follow_folio() can be used in place of follow_page(). One caveat is that
user will see -ENOENT instead of -EACCES after this change.


--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Kefeng Wang]

On 2023/8/4 10:42, Zi Yan wrote:
> On 3 Aug 2023, at 21:45, Kefeng Wang wrote:
> 
>> On 2023/8/3 20:30, Matthew Wilcox wrote:
>>> On Thu, Aug 03, 2023 at 03:13:21PM +0800, Kefeng Wang wrote:
>>>>
>>>>
>>>> On 2023/8/2 20:21, Matthew Wilcox wrote:
>>>>> On Wed, Aug 02, 2023 at 05:53:43PM +0800, Kefeng Wang wrote:
>>>>>>     	err = -EACCES;
>>>>>> -	if (page_mapcount(page) > 1 && !migrate_all)
>>>>>> -		goto out_putpage;
>>>>>> +	if (folio_estimated_sharers(folio) > 1 && !migrate_all)
>>>>>> +		goto out_putfolio;
>>>>>
>>>>> I do not think this is the correct change.  Maybe leave this line
>>>>> alone.
>>>>
>>>> Ok, I am aware of the discussion about this in other mail, will not
>>>> change it(also the next two patch about this function), or wait the
>>>> new work of David.
>>>>>
>>>>>> -	if (PageHuge(page)) {
>>>>>> -		if (PageHead(page)) {
>>>>>> -			isolated = isolate_hugetlb(page_folio(page), pagelist);
>>>>>> +	if (folio_test_hugetlb(folio)) {
>>>>>> +		if (folio_test_large(folio)) {
>>>>>
>>>>> This makes no sense when you read it.  All hugetlb folios are large,
>>>>> by definition.  Think about what this code used to do, and what it
>>>>> should be changed to.
>>>>
>>>> hugetlb folio is self large folio, will drop redundant check
>>>
>>> No, that's not the difference.  Keep thinking about it.  This is not
>>> a mechanical translation!
>>
>>
>>    if (PageHuge(page))  // page must be a hugetlb page
>> 	if (PageHead(page)) // page must be a head page, not tail
>>               isolate_hugetlb() // isolate the hugetlb page if head
>>
>> After using folio,
>>
>>    if (folio_test_hugetlb(folio)) // only check folio is hugetlb or not
>>
>> I don't check the page is head or not, since the follow_page could
>> return a sub-page, so the check PageHead need be retained, right?
> 
> Right. It will prevent the kernel from trying to isolate the same hugetlb page
> twice when two pages are in the same hugetlb folio. But looking at the
> code, if you try to isolate an already-isolated hugetlb folio, isolate_hugetlb()
> would return false, no error would show up. But it changes err value
> from -EACCES to -EBUSY and user will see a different page status than before.


When check man[1], the current -EACCES is not right, -EBUSY is not
precise but more suitable for this scenario,

  	-EACCES
               The page is mapped by multiple processes and can be moved
               only if MPOL_MF_MOVE_ALL is specified.

        -EBUSY The page is currently busy and cannot be moved.  Try again
               later.  This occurs if a page is undergoing I/O or another
               kernel subsystem is holding a reference to the page.
	-ENOENT
               The page is not present.

> 
> I wonder why we do not have follow_folio() and returns -ENOENT error pointer
> when addr points to a non head page. It would make this patch more folio if
> follow_folio() can be used in place of follow_page(). One caveat is that
> user will see -ENOENT instead of -EACCES after this change.
> 

-ENOENT is ok, but maybe the man need to be updated too.


	
[1] https://man7.org/linux/man-pages/man2/move_pages.2.html






> 
> --
> Best Regards,
> Yan, Zi

Re: [PATCH 2/4] mm: migrate: convert numamigrate_isolate_page() to numamigrate_isolate_folio()

[Hugh Dickins]

On Thu, 3 Aug 2023, Kefeng Wang wrote:
> On 2023/8/2 20:30, Matthew Wilcox wrote:
> > On Wed, Aug 02, 2023 at 05:53:44PM +0800, Kefeng Wang wrote:
> >>   	/* Do not migrate THP mapped by multiple processes */
> >> -	if (PageTransHuge(page) && total_mapcount(page) > 1)
> >> +	if (folio_test_pmd_mappable(folio) && folio_estimated_sharers(folio) >
> >> 1)
> >>     return 0;
> > 
> > I don't know if this is the right logic.  We've willing to move folios
> > mapped by multiple processes, as long as they're smaller than PMD size,
> > but once they get to PMD size they're magical and can't be moved?
> 
> It seems that the logical is introduced by commit 04fa5d6a6547 ("mm:
> migrate: check page_count of THP before migrating") and refactor by
> 340ef3902cf2 ("mm: numa: cleanup flow of transhuge page migration"),
> 
> 
>   "Hugh Dickins pointed out that migrate_misplaced_transhuge_page() does
>   not check page_count before migrating like base page migration and
>   khugepage.  He could not see why this was safe and he is right."
> 
> For now, there is no migrate_misplaced_transhuge_page() and base/thp
> page migrate's path is unified, there is a check(for old/new kernel) in
> migrate_misplaced_page(),

Right, Mel's comment on safety above comes from a time when
migrate_misplaced_transhuge_page() went its own way, and so did not
reach the careful reference count checking found in (the now named)
folio_migrate_mapping().  If migrate_misplaced_page() is now using
the standard migrate_pages() for small pages and THPs, then there
should no longer be safety concerns.

> 
>  "Don't migrate file pages that are mapped in multiple processes
>  with execute permissions as they are probably shared libraries."
> 
> We could drop the above check in numamigrate_isolate_page(), but
> according to 04fa5d6a6547, maybe disable migrate page shared by
> multi-process during numa balance for both base/thp page.

I'm out of touch with the NUMA balancing preferences at present,
but think that you're probably right that it should aim to treat
folio mapped into multiple processes the same way for small and large
and THP (except, of course, that large and THP, with multiple PTEs in
the same process, can present more challenges to how to go about that).

Hugh

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Kefeng Wang]

Hi Zi Yan and Matthew and Naoya,

On 2023/8/4 13:54, Kefeng Wang wrote:
> 
> 
> On 2023/8/4 10:42, Zi Yan wrote:
>> On 3 Aug 2023, at 21:45, Kefeng Wang wrote:
>>
>>> On 2023/8/3 20:30, Matthew Wilcox wrote:
>>>> On Thu, Aug 03, 2023 at 03:13:21PM +0800, Kefeng Wang wrote:
>>>>>

...

>>>
>>>
>>>    if (PageHuge(page))  // page must be a hugetlb page
>>>     if (PageHead(page)) // page must be a head page, not tail
>>>               isolate_hugetlb() // isolate the hugetlb page if head
>>>
>>> After using folio,
>>>
>>>    if (folio_test_hugetlb(folio)) // only check folio is hugetlb or not
>>>
>>> I don't check the page is head or not, since the follow_page could
>>> return a sub-page, so the check PageHead need be retained, right?
>>
>> Right. It will prevent the kernel from trying to isolate the same 
>> hugetlb page
>> twice when two pages are in the same hugetlb folio. But looking at the
>> code, if you try to isolate an already-isolated hugetlb folio, 
>> isolate_hugetlb()
>> would return false, no error would show up. But it changes err value
>> from -EACCES to -EBUSY and user will see a different page status than 
>> before.
> 

Before e66f17ff7177 ("mm/hugetlb: take page table lock in 
follow_huge_pmd()")
in v4.0, follow_page() will return NULL on tail page for Huagetlb page,
and move_pages() will return -ENOENT errno,but after that commit,
-EACCES is returned, which not match the manual,

> 
> When check man[1], the current -EACCES is not right, -EBUSY is not
> precise but more suitable for this scenario,
> 
>       -EACCES
>                The page is mapped by multiple processes and can be moved
>                only if MPOL_MF_MOVE_ALL is specified.
> 
>       -EBUSY The page is currently busy and cannot be moved.  Try again
>                later.  This occurs if a page is undergoing I/O or another
>                kernel subsystem is holding a reference to the page.
>      -ENOENT
>                The page is not present.
> 
>>
>> I wonder why we do not have follow_folio() and returns -ENOENT error 
>> pointer
>> when addr points to a non head page. It would make this patch more 
>> folio if
>> follow_folio() can be used in place of follow_page(). One caveat is that
>> user will see -ENOENT instead of -EACCES after this change.
>>
> 
> -ENOENT is ok, but maybe the man need to be updated too.

According to above analysis, -ENOENT is suitable when introduce the
follow_folio(), but when THP migrate support is introduced by
e8db67eb0ded ("mm: migrate: move_pages() supports thp migration") in
v4.14, the tail page will be turned into head page and return -EBUSY,

So should we unify errno(maybe use -ENOENT) about the tail page?


> 
> 
> 
> [1] https://man7.org/linux/man-pages/man2/move_pages.2.html

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 3719 bytes --]

On 7 Aug 2023, at 8:20, Kefeng Wang wrote:

> Hi Zi Yan and Matthew and Naoya,
>
> On 2023/8/4 13:54, Kefeng Wang wrote:
>>
>>
>> On 2023/8/4 10:42, Zi Yan wrote:
>>> On 3 Aug 2023, at 21:45, Kefeng Wang wrote:
>>>
>>>> On 2023/8/3 20:30, Matthew Wilcox wrote:
>>>>> On Thu, Aug 03, 2023 at 03:13:21PM +0800, Kefeng Wang wrote:
>>>>>>
>
> ...
>
>>>>
>>>>
>>>>    if (PageHuge(page))  // page must be a hugetlb page
>>>>     if (PageHead(page)) // page must be a head page, not tail
>>>>               isolate_hugetlb() // isolate the hugetlb page if head
>>>>
>>>> After using folio,
>>>>
>>>>    if (folio_test_hugetlb(folio)) // only check folio is hugetlb or not
>>>>
>>>> I don't check the page is head or not, since the follow_page could
>>>> return a sub-page, so the check PageHead need be retained, right?
>>>
>>> Right. It will prevent the kernel from trying to isolate the same hugetlb page
>>> twice when two pages are in the same hugetlb folio. But looking at the
>>> code, if you try to isolate an already-isolated hugetlb folio, isolate_hugetlb()
>>> would return false, no error would show up. But it changes err value
>>> from -EACCES to -EBUSY and user will see a different page status than before.
>>
>
> Before e66f17ff7177 ("mm/hugetlb: take page table lock in follow_huge_pmd()")
> in v4.0, follow_page() will return NULL on tail page for Huagetlb page,
> and move_pages() will return -ENOENT errno,but after that commit,
> -EACCES is returned, which not match the manual,
>
>>
>> When check man[1], the current -EACCES is not right, -EBUSY is not
>> precise but more suitable for this scenario,
>>
>>       -EACCES
>>                The page is mapped by multiple processes and can be moved
>>                only if MPOL_MF_MOVE_ALL is specified.
>>
>>       -EBUSY The page is currently busy and cannot be moved.  Try again
>>                later.  This occurs if a page is undergoing I/O or another
>>                kernel subsystem is holding a reference to the page.
>>      -ENOENT
>>                The page is not present.
>>
>>>
>>> I wonder why we do not have follow_folio() and returns -ENOENT error pointer
>>> when addr points to a non head page. It would make this patch more folio if
>>> follow_folio() can be used in place of follow_page(). One caveat is that
>>> user will see -ENOENT instead of -EACCES after this change.
>>>
>>
>> -ENOENT is ok, but maybe the man need to be updated too.
>
> According to above analysis, -ENOENT is suitable when introduce the
> follow_folio(), but when THP migrate support is introduced by
> e8db67eb0ded ("mm: migrate: move_pages() supports thp migration") in
> v4.14, the tail page will be turned into head page and return -EBUSY,
>
> So should we unify errno(maybe use -ENOENT) about the tail page?
>
>
>>
>>
>>
>> [1] https://man7.org/linux/man-pages/man2/move_pages.2.html

I think so. I think -EBUSY is more reasonable for tail pages. But there is
some subtle difference between THP and hugetlb from current code:

For THP, compound_head() is used to get the head page for isolation, this means
if user specifies a tail page address in move_pages(), the whole THP can be
migrated.

For hugetlb, only if user specifies the head page address of a hugetlb page,
the hugetlb page will be migrated. Otherwise, an error would show up.

Cc Mike to help us clarify the expected behavior of hugetlb.

Hi Mike, what is the expected behavior, if a user tries to use move_pages()
to migrate a non head page of a hugetlb page?

--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Kefeng Wang]

Hi Mike

On 2023/8/8 2:45, Zi Yan wrote:
> On 7 Aug 2023, at 8:20, Kefeng Wang wrote:
> 
>> Hi Zi Yan and Matthew and Naoya,
>>
>> On 2023/8/4 13:54, Kefeng Wang wrote:
>>>
>>>
>>> On 2023/8/4 10:42, Zi Yan wrote:
>>>> On 3 Aug 2023, at 21:45, Kefeng Wang wrote:
>>>>
>>>>> On 2023/8/3 20:30, Matthew Wilcox wrote:
>>>>>> On Thu, Aug 03, 2023 at 03:13:21PM +0800, Kefeng Wang wrote:
>>>>>>>
>>
>> ...
>>
>>>>>
>>>>>
>>>>>     if (PageHuge(page))  // page must be a hugetlb page
>>>>>      if (PageHead(page)) // page must be a head page, not tail
>>>>>                isolate_hugetlb() // isolate the hugetlb page if head
>>>>>
>>>>> After using folio,
>>>>>
>>>>>     if (folio_test_hugetlb(folio)) // only check folio is hugetlb or not
>>>>>
>>>>> I don't check the page is head or not, since the follow_page could
>>>>> return a sub-page, so the check PageHead need be retained, right?
>>>>
>>>> Right. It will prevent the kernel from trying to isolate the same hugetlb page
>>>> twice when two pages are in the same hugetlb folio. But looking at the
>>>> code, if you try to isolate an already-isolated hugetlb folio, isolate_hugetlb()
>>>> would return false, no error would show up. But it changes err value
>>>> from -EACCES to -EBUSY and user will see a different page status than before.
>>>
>>
>> Before e66f17ff7177 ("mm/hugetlb: take page table lock in follow_huge_pmd()")
>> in v4.0, follow_page() will return NULL on tail page for Huagetlb page,
>> and move_pages() will return -ENOENT errno,but after that commit,
>> -EACCES is returned, which not match the manual,
>>
>>>
>>> When check man[1], the current -EACCES is not right, -EBUSY is not
>>> precise but more suitable for this scenario,
>>>
>>>        -EACCES
>>>                 The page is mapped by multiple processes and can be moved
>>>                 only if MPOL_MF_MOVE_ALL is specified.
>>>
>>>        -EBUSY The page is currently busy and cannot be moved.  Try again
>>>                 later.  This occurs if a page is undergoing I/O or another
>>>                 kernel subsystem is holding a reference to the page.
>>>       -ENOENT
>>>                 The page is not present.
>>>
>>>>
>>>> I wonder why we do not have follow_folio() and returns -ENOENT error pointer
>>>> when addr points to a non head page. It would make this patch more folio if
>>>> follow_folio() can be used in place of follow_page(). One caveat is that
>>>> user will see -ENOENT instead of -EACCES after this change.
>>>>
>>>
>>> -ENOENT is ok, but maybe the man need to be updated too.
>>
>> According to above analysis, -ENOENT is suitable when introduce the
>> follow_folio(), but when THP migrate support is introduced by
>> e8db67eb0ded ("mm: migrate: move_pages() supports thp migration") in
>> v4.14, the tail page will be turned into head page and return -EBUSY,
>>
>> So should we unify errno(maybe use -ENOENT) about the tail page?
>>
>>
>>>
>>>
>>>
>>> [1] https://man7.org/linux/man-pages/man2/move_pages.2.html
> 
> I think so. I think -EBUSY is more reasonable for tail pages. But there is
> some subtle difference between THP and hugetlb from current code:
> 
> For THP, compound_head() is used to get the head page for isolation, this means
> if user specifies a tail page address in move_pages(), the whole THP can be
> migrated.
> 
> For hugetlb, only if user specifies the head page address of a hugetlb page,
> the hugetlb page will be migrated. Otherwise, an error would show up.
> 
> Cc Mike to help us clarify the expected behavior of hugetlb.
> 
> Hi Mike, what is the expected behavior, if a user tries to use move_pages()
> to migrate a non head page of a hugetlb page?

Could you give some advise, thanks

> 
> --
> Best Regards,
> Yan, Zi

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Mike Kravetz]

On 08/09/23 20:37, Kefeng Wang wrote:
> Hi Mike
> 
> On 2023/8/8 2:45, Zi Yan wrote:
> > On 7 Aug 2023, at 8:20, Kefeng Wang wrote:
> > 
> > > Hi Zi Yan and Matthew and Naoya,
> > > 
> > > On 2023/8/4 13:54, Kefeng Wang wrote:
> > > > 
> > > > 
> > > > On 2023/8/4 10:42, Zi Yan wrote:
> > > > > On 3 Aug 2023, at 21:45, Kefeng Wang wrote:
> > > > > 
> > > > > > On 2023/8/3 20:30, Matthew Wilcox wrote:
> > > > > > > On Thu, Aug 03, 2023 at 03:13:21PM +0800, Kefeng Wang wrote:
> > > > > > > > 
> > > 
> > > ...
> > > 
> > > > > > 
> > > > > > 
> > > > > >     if (PageHuge(page))  // page must be a hugetlb page
> > > > > >      if (PageHead(page)) // page must be a head page, not tail
> > > > > >                isolate_hugetlb() // isolate the hugetlb page if head
> > > > > > 
> > > > > > After using folio,
> > > > > > 
> > > > > >     if (folio_test_hugetlb(folio)) // only check folio is hugetlb or not
> > > > > > 
> > > > > > I don't check the page is head or not, since the follow_page could
> > > > > > return a sub-page, so the check PageHead need be retained, right?
> > > > > 
> > > > > Right. It will prevent the kernel from trying to isolate the same hugetlb page
> > > > > twice when two pages are in the same hugetlb folio. But looking at the
> > > > > code, if you try to isolate an already-isolated hugetlb folio, isolate_hugetlb()
> > > > > would return false, no error would show up. But it changes err value
> > > > > from -EACCES to -EBUSY and user will see a different page status than before.
> > > > 
> > > 
> > > Before e66f17ff7177 ("mm/hugetlb: take page table lock in follow_huge_pmd()")
> > > in v4.0, follow_page() will return NULL on tail page for Huagetlb page,
> > > and move_pages() will return -ENOENT errno,but after that commit,
> > > -EACCES is returned, which not match the manual,
> > > 
> > > > 
> > > > When check man[1], the current -EACCES is not right, -EBUSY is not
> > > > precise but more suitable for this scenario,
> > > > 
> > > >        -EACCES
> > > >                 The page is mapped by multiple processes and can be moved
> > > >                 only if MPOL_MF_MOVE_ALL is specified.
> > > > 
> > > >        -EBUSY The page is currently busy and cannot be moved.  Try again
> > > >                 later.  This occurs if a page is undergoing I/O or another
> > > >                 kernel subsystem is holding a reference to the page.
> > > >       -ENOENT
> > > >                 The page is not present.
> > > > 
> > > > > 
> > > > > I wonder why we do not have follow_folio() and returns -ENOENT error pointer
> > > > > when addr points to a non head page. It would make this patch more folio if
> > > > > follow_folio() can be used in place of follow_page(). One caveat is that
> > > > > user will see -ENOENT instead of -EACCES after this change.
> > > > > 
> > > > 
> > > > -ENOENT is ok, but maybe the man need to be updated too.
> > > 
> > > According to above analysis, -ENOENT is suitable when introduce the
> > > follow_folio(), but when THP migrate support is introduced by
> > > e8db67eb0ded ("mm: migrate: move_pages() supports thp migration") in
> > > v4.14, the tail page will be turned into head page and return -EBUSY,
> > > 
> > > So should we unify errno(maybe use -ENOENT) about the tail page?
> > > 
> > > 
> > > > 
> > > > 
> > > > 
> > > > [1] https://man7.org/linux/man-pages/man2/move_pages.2.html
> > 
> > I think so. I think -EBUSY is more reasonable for tail pages. But there is
> > some subtle difference between THP and hugetlb from current code:
> > 
> > For THP, compound_head() is used to get the head page for isolation, this means
> > if user specifies a tail page address in move_pages(), the whole THP can be
> > migrated.
> > 
> > For hugetlb, only if user specifies the head page address of a hugetlb page,
> > the hugetlb page will be migrated. Otherwise, an error would show up.
> > 
> > Cc Mike to help us clarify the expected behavior of hugetlb.
> > 
> > Hi Mike, what is the expected behavior, if a user tries to use move_pages()
> > to migrate a non head page of a hugetlb page?
> 
> Could you give some advise, thanks
> 

Sorry, I was away for a while.

It seems unfortunate that move_pages says the passed user addresses
should be aligned to page boundaries.  However, IIUC this is not checked
or enforced.  Otherwise, passing a hugetlb page should return the same
error.

One thought would be that hugetlb mappings should behave the same
non-hugetlb mappings.  If passed the address of a hugetlb tail page, align
the address to a hugetlb boundary and migrate the page.  This changes the
existing behavior.  However, it would be hard to imagine anyone depending
on this.

After taking a closer look at the add_page_for_migration(), it seems to
just ignore passed tail pages and do nothing for such passed addresses.
Correct?  Or, am I missing something?  Perhaps that is behavior we want/
need to preserve?
-- 
Mike Kravetz

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Mike Kravetz]

On 08/09/23 13:53, Mike Kravetz wrote:
> On 08/09/23 20:37, Kefeng Wang wrote:
> > > 
> > > Cc Mike to help us clarify the expected behavior of hugetlb.
> > > 
> > > Hi Mike, what is the expected behavior, if a user tries to use move_pages()
> > > to migrate a non head page of a hugetlb page?
> > 
> > Could you give some advise, thanks
> > 
> 
> Sorry, I was away for a while.
> 
> It seems unfortunate that move_pages says the passed user addresses
> should be aligned to page boundaries.  However, IIUC this is not checked
> or enforced.  Otherwise, passing a hugetlb page should return the same
> error.
> 
> One thought would be that hugetlb mappings should behave the same
> non-hugetlb mappings.  If passed the address of a hugetlb tail page, align
> the address to a hugetlb boundary and migrate the page.  This changes the
> existing behavior.  However, it would be hard to imagine anyone depending
> on this.
> 
> After taking a closer look at the add_page_for_migration(), it seems to
> just ignore passed tail pages and do nothing for such passed addresses.
> Correct?  Or, am I missing something?  Perhaps that is behavior we want/
> need to preserve?

My mistake, status -EACCES is returned when passing a tail page of a
hugetlb page.

Back to the question of 'What is the expected behavior if a tail page is
passed?'.  I do not think we have defined an expected behavior.  If
anything is 'expected' I would say it is -EACCES as returned today.

BTW - hugetlb pages not migrated due to passing a tail page does not
seem to contribute to a 'Positive return value' indicating the number of
non-migrated pages.
-- 
Mike Kravetz

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Kefeng Wang]

On 2023/8/10 6:44, Mike Kravetz wrote:
> On 08/09/23 13:53, Mike Kravetz wrote:
>> On 08/09/23 20:37, Kefeng Wang wrote:
>>>>
>>>> Cc Mike to help us clarify the expected behavior of hugetlb.
>>>>
>>>> Hi Mike, what is the expected behavior, if a user tries to use move_pages()
>>>> to migrate a non head page of a hugetlb page?
>>>
>>> Could you give some advise, thanks
>>>
>>
>> Sorry, I was away for a while.
>>
>> It seems unfortunate that move_pages says the passed user addresses
>> should be aligned to page boundaries.  However, IIUC this is not checked
>> or enforced.  Otherwise, passing a hugetlb page should return the same
>> error.
>>
>> One thought would be that hugetlb mappings should behave the same
>> non-hugetlb mappings.  If passed the address of a hugetlb tail page, align
>> the address to a hugetlb boundary and migrate the page.  This changes the
>> existing behavior.  However, it would be hard to imagine anyone depending
>> on this.
>>
>> After taking a closer look at the add_page_for_migration(), it seems to
>> just ignore passed tail pages and do nothing for such passed addresses.
>> Correct?  Or, am I missing something?  Perhaps that is behavior we want/
>> need to preserve?
> 
> My mistake, status -EACCES is returned when passing a tail page of a
> hugetlb page.
> 

As mentioned in previous mail， before e66f17ff7177 ("mm/hugetlb: take
page table lock in follow_huge_pmd()") in v4.0, follow_page() will
return NULL on tail page for Huagetlb page, so move_pages() will return
-ENOENT errno, but after that commit, -EACCES is returned.

Meanwhile, the behavior of THP/HUGETLB is different, the whole THP will 
be migrated on a tail page, but HUGETLB will return -EACCES(after v4.0)
or -ENOENT(before v4.0) on tail page.

> Back to the question of 'What is the expected behavior if a tail page is
> passed?'.  I do not think we have defined an expected behavior.  If
> anything is 'expected' I would say it is -EACCES as returned today.
> 

My question is,

Should we keep seem behavior between HUGETLB and THP, or only change the
errno from -EACCES to -ENOENT/-EBUSY.

I would like to drop PageHead() check for Hugetlb to keep seem behavior,
which will keep seem error code if isolate fail or success on head/tail
page.

Thanks.

> BTW - hugetlb pages not migrated due to passing a tail page does not
> seem to contribute to a 'Positive return value' indicating the number of
> non-migrated pages.

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Mike Kravetz]

On 08/10/23 09:49, Kefeng Wang wrote:
> 
> 
> On 2023/8/10 6:44, Mike Kravetz wrote:
> > On 08/09/23 13:53, Mike Kravetz wrote:
> > > On 08/09/23 20:37, Kefeng Wang wrote:
> > > > > 
> > > > > Cc Mike to help us clarify the expected behavior of hugetlb.
> > > > > 
> > > > > Hi Mike, what is the expected behavior, if a user tries to use move_pages()
> > > > > to migrate a non head page of a hugetlb page?
> > > > 
> > > > Could you give some advise, thanks
> > > > 
> > > 
> > > Sorry, I was away for a while.
> > > 
> > > It seems unfortunate that move_pages says the passed user addresses
> > > should be aligned to page boundaries.  However, IIUC this is not checked
> > > or enforced.  Otherwise, passing a hugetlb page should return the same
> > > error.
> > > 
> > > One thought would be that hugetlb mappings should behave the same
> > > non-hugetlb mappings.  If passed the address of a hugetlb tail page, align
> > > the address to a hugetlb boundary and migrate the page.  This changes the
> > > existing behavior.  However, it would be hard to imagine anyone depending
> > > on this.
> > > 
> > > After taking a closer look at the add_page_for_migration(), it seems to
> > > just ignore passed tail pages and do nothing for such passed addresses.
> > > Correct?  Or, am I missing something?  Perhaps that is behavior we want/
> > > need to preserve?
> > 
> > My mistake, status -EACCES is returned when passing a tail page of a
> > hugetlb page.
> > 
> 
> As mentioned in previous mail， before e66f17ff7177 ("mm/hugetlb: take
> page table lock in follow_huge_pmd()") in v4.0, follow_page() will
> return NULL on tail page for Huagetlb page, so move_pages() will return
> -ENOENT errno, but after that commit, -EACCES is returned.
> 
> Meanwhile, the behavior of THP/HUGETLB is different, the whole THP will be
> migrated on a tail page, but HUGETLB will return -EACCES(after v4.0)
> or -ENOENT(before v4.0) on tail page.
> 
> > Back to the question of 'What is the expected behavior if a tail page is
> > passed?'.  I do not think we have defined an expected behavior.  If
> > anything is 'expected' I would say it is -EACCES as returned today.
> > 
> 
> My question is,
> 
> Should we keep seem behavior between HUGETLB and THP, or only change the
> errno from -EACCES to -ENOENT/-EBUSY.

Just to be clear.  When you say "keep seem behavior between HUGETLB and THP",
are you saying that you would like hugetlb to perform migration of the entire
hugetlb page if a tail page is passed?

IMO, this would be ideal as it would mean that hugetlb and THP behave the same
when passed the address of a tail page.  The fewer places where hugetlb
behavior diverges, the better.  However, this does change behavior.

As mentioned above, I have a hard time imagining someone depending on the
behavior that passing the address of a hugetlb tail page returns error.  But,
this is almost impossible to predict.

Thoughts from others?  
-- 
Mike Kravetz

> 
> I would like to drop PageHead() check for Hugetlb to keep seem behavior,
> which will keep seem error code if isolate fail or success on head/tail
> page.
> 
> Thanks.

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Huang, Ying]

Kefeng Wang <wangkefeng.wang@huawei.com> writes:

> On 2023/8/4 10:42, Zi Yan wrote:
>> On 3 Aug 2023, at 21:45, Kefeng Wang wrote:
>> 
>>> On 2023/8/3 20:30, Matthew Wilcox wrote:
>>>> On Thu, Aug 03, 2023 at 03:13:21PM +0800, Kefeng Wang wrote:
>>>>>
>>>>>
>>>>> On 2023/8/2 20:21, Matthew Wilcox wrote:
>>>>>> On Wed, Aug 02, 2023 at 05:53:43PM +0800, Kefeng Wang wrote:
>>>>>>>     	err = -EACCES;
>>>>>>> -	if (page_mapcount(page) > 1 && !migrate_all)
>>>>>>> -		goto out_putpage;
>>>>>>> +	if (folio_estimated_sharers(folio) > 1 && !migrate_all)
>>>>>>> +		goto out_putfolio;
>>>>>>
>>>>>> I do not think this is the correct change.  Maybe leave this line
>>>>>> alone.
>>>>>
>>>>> Ok, I am aware of the discussion about this in other mail, will not
>>>>> change it(also the next two patch about this function), or wait the
>>>>> new work of David.
>>>>>>
>>>>>>> -	if (PageHuge(page)) {
>>>>>>> -		if (PageHead(page)) {
>>>>>>> -			isolated = isolate_hugetlb(page_folio(page), pagelist);
>>>>>>> +	if (folio_test_hugetlb(folio)) {
>>>>>>> +		if (folio_test_large(folio)) {
>>>>>>
>>>>>> This makes no sense when you read it.  All hugetlb folios are large,
>>>>>> by definition.  Think about what this code used to do, and what it
>>>>>> should be changed to.
>>>>>
>>>>> hugetlb folio is self large folio, will drop redundant check
>>>>
>>>> No, that's not the difference.  Keep thinking about it.  This is not
>>>> a mechanical translation!
>>>
>>>
>>>    if (PageHuge(page))  // page must be a hugetlb page
>>> 	if (PageHead(page)) // page must be a head page, not tail
>>>               isolate_hugetlb() // isolate the hugetlb page if head
>>>
>>> After using folio,
>>>
>>>    if (folio_test_hugetlb(folio)) // only check folio is hugetlb or not
>>>
>>> I don't check the page is head or not, since the follow_page could
>>> return a sub-page, so the check PageHead need be retained, right?
>> Right. It will prevent the kernel from trying to isolate the same
>> hugetlb page
>> twice when two pages are in the same hugetlb folio. But looking at the
>> code, if you try to isolate an already-isolated hugetlb folio, isolate_hugetlb()
>> would return false, no error would show up. But it changes err value
>> from -EACCES to -EBUSY and user will see a different page status than before.
>
>
> When check man[1], the current -EACCES is not right, -EBUSY is not
> precise but more suitable for this scenario,
>
>  	-EACCES
>               The page is mapped by multiple processes and can be moved
>               only if MPOL_MF_MOVE_ALL is specified.
>
>        -EBUSY The page is currently busy and cannot be moved.  Try again
>               later.  This occurs if a page is undergoing I/O or another
>               kernel subsystem is holding a reference to the page.
> 	-ENOENT
>               The page is not present.
>
>> I wonder why we do not have follow_folio() and returns -ENOENT error
>> pointer
>> when addr points to a non head page. It would make this patch more folio if
>> follow_folio() can be used in place of follow_page(). One caveat is that
>> user will see -ENOENT instead of -EACCES after this change.
>> 
>
> -ENOENT is ok, but maybe the man need to be updated too.
>
>
> 	
> [1] https://man7.org/linux/man-pages/man2/move_pages.2.html
>

I don't think -ENOENT is appropriate.  IIUC, -ENOENT means no need to
migrate.  Which isn't the case here apparently.

--
Best Regards,
Huang, Ying

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Huang, Ying]

Mike Kravetz <mike.kravetz@oracle.com> writes:

> On 08/10/23 09:49, Kefeng Wang wrote:
>> 
>> 
>> On 2023/8/10 6:44, Mike Kravetz wrote:
>> > On 08/09/23 13:53, Mike Kravetz wrote:
>> > > On 08/09/23 20:37, Kefeng Wang wrote:
>> > > > > 
>> > > > > Cc Mike to help us clarify the expected behavior of hugetlb.
>> > > > > 
>> > > > > Hi Mike, what is the expected behavior, if a user tries to use move_pages()
>> > > > > to migrate a non head page of a hugetlb page?
>> > > > 
>> > > > Could you give some advise, thanks
>> > > > 
>> > > 
>> > > Sorry, I was away for a while.
>> > > 
>> > > It seems unfortunate that move_pages says the passed user addresses
>> > > should be aligned to page boundaries.  However, IIUC this is not checked
>> > > or enforced.  Otherwise, passing a hugetlb page should return the same
>> > > error.
>> > > 
>> > > One thought would be that hugetlb mappings should behave the same
>> > > non-hugetlb mappings.  If passed the address of a hugetlb tail page, align
>> > > the address to a hugetlb boundary and migrate the page.  This changes the
>> > > existing behavior.  However, it would be hard to imagine anyone depending
>> > > on this.
>> > > 
>> > > After taking a closer look at the add_page_for_migration(), it seems to
>> > > just ignore passed tail pages and do nothing for such passed addresses.
>> > > Correct?  Or, am I missing something?  Perhaps that is behavior we want/
>> > > need to preserve?
>> > 
>> > My mistake, status -EACCES is returned when passing a tail page of a
>> > hugetlb page.
>> > 
>> 
>> As mentioned in previous mail， before e66f17ff7177 ("mm/hugetlb: take
>> page table lock in follow_huge_pmd()") in v4.0, follow_page() will
>> return NULL on tail page for Huagetlb page, so move_pages() will return
>> -ENOENT errno, but after that commit, -EACCES is returned.
>> 
>> Meanwhile, the behavior of THP/HUGETLB is different, the whole THP will be
>> migrated on a tail page, but HUGETLB will return -EACCES(after v4.0)
>> or -ENOENT(before v4.0) on tail page.
>> 
>> > Back to the question of 'What is the expected behavior if a tail page is
>> > passed?'.  I do not think we have defined an expected behavior.  If
>> > anything is 'expected' I would say it is -EACCES as returned today.
>> > 
>> 
>> My question is,
>> 
>> Should we keep seem behavior between HUGETLB and THP, or only change the
>> errno from -EACCES to -ENOENT/-EBUSY.
>
> Just to be clear.  When you say "keep seem behavior between HUGETLB and THP",
> are you saying that you would like hugetlb to perform migration of the entire
> hugetlb page if a tail page is passed?
>
> IMO, this would be ideal as it would mean that hugetlb and THP behave the same
> when passed the address of a tail page.  The fewer places where hugetlb
> behavior diverges, the better.  However, this does change behavior.

A separate patch will be needed for behavior change.

> As mentioned above, I have a hard time imagining someone depending on the
> behavior that passing the address of a hugetlb tail page returns error.  But,
> this is almost impossible to predict.
>
> Thoughts from others?  

--
Best Regards,
Huang, Ying

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 3867 bytes --]

On 14 Aug 2023, at 23:56, Huang, Ying wrote:

> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>
>> On 2023/8/4 10:42, Zi Yan wrote:
>>> On 3 Aug 2023, at 21:45, Kefeng Wang wrote:
>>>
>>>> On 2023/8/3 20:30, Matthew Wilcox wrote:
>>>>> On Thu, Aug 03, 2023 at 03:13:21PM +0800, Kefeng Wang wrote:
>>>>>>
>>>>>>
>>>>>> On 2023/8/2 20:21, Matthew Wilcox wrote:
>>>>>>> On Wed, Aug 02, 2023 at 05:53:43PM +0800, Kefeng Wang wrote:
>>>>>>>>     	err = -EACCES;
>>>>>>>> -	if (page_mapcount(page) > 1 && !migrate_all)
>>>>>>>> -		goto out_putpage;
>>>>>>>> +	if (folio_estimated_sharers(folio) > 1 && !migrate_all)
>>>>>>>> +		goto out_putfolio;
>>>>>>>
>>>>>>> I do not think this is the correct change.  Maybe leave this line
>>>>>>> alone.
>>>>>>
>>>>>> Ok, I am aware of the discussion about this in other mail, will not
>>>>>> change it(also the next two patch about this function), or wait the
>>>>>> new work of David.
>>>>>>>
>>>>>>>> -	if (PageHuge(page)) {
>>>>>>>> -		if (PageHead(page)) {
>>>>>>>> -			isolated = isolate_hugetlb(page_folio(page), pagelist);
>>>>>>>> +	if (folio_test_hugetlb(folio)) {
>>>>>>>> +		if (folio_test_large(folio)) {
>>>>>>>
>>>>>>> This makes no sense when you read it.  All hugetlb folios are large,
>>>>>>> by definition.  Think about what this code used to do, and what it
>>>>>>> should be changed to.
>>>>>>
>>>>>> hugetlb folio is self large folio, will drop redundant check
>>>>>
>>>>> No, that's not the difference.  Keep thinking about it.  This is not
>>>>> a mechanical translation!
>>>>
>>>>
>>>>    if (PageHuge(page))  // page must be a hugetlb page
>>>> 	if (PageHead(page)) // page must be a head page, not tail
>>>>               isolate_hugetlb() // isolate the hugetlb page if head
>>>>
>>>> After using folio,
>>>>
>>>>    if (folio_test_hugetlb(folio)) // only check folio is hugetlb or not
>>>>
>>>> I don't check the page is head or not, since the follow_page could
>>>> return a sub-page, so the check PageHead need be retained, right?
>>> Right. It will prevent the kernel from trying to isolate the same
>>> hugetlb page
>>> twice when two pages are in the same hugetlb folio. But looking at the
>>> code, if you try to isolate an already-isolated hugetlb folio, isolate_hugetlb()
>>> would return false, no error would show up. But it changes err value
>>> from -EACCES to -EBUSY and user will see a different page status than before.
>>
>>
>> When check man[1], the current -EACCES is not right, -EBUSY is not
>> precise but more suitable for this scenario,
>>
>>  	-EACCES
>>               The page is mapped by multiple processes and can be moved
>>               only if MPOL_MF_MOVE_ALL is specified.
>>
>>        -EBUSY The page is currently busy and cannot be moved.  Try again
>>               later.  This occurs if a page is undergoing I/O or another
>>               kernel subsystem is holding a reference to the page.
>> 	-ENOENT
>>               The page is not present.
>>
>>> I wonder why we do not have follow_folio() and returns -ENOENT error
>>> pointer
>>> when addr points to a non head page. It would make this patch more folio if
>>> follow_folio() can be used in place of follow_page(). One caveat is that
>>> user will see -ENOENT instead of -EACCES after this change.
>>>
>>
>> -ENOENT is ok, but maybe the man need to be updated too.
>>
>>
>> 	
>> [1] https://man7.org/linux/man-pages/man2/move_pages.2.html
>>
>
> I don't think -ENOENT is appropriate.  IIUC, -ENOENT means no need to
> migrate.  Which isn't the case here apparently.

Are you referring to a comment or the man page? The man page says
-ENOENT means the page is not present. Or you think it also implies
there is no need to migrate? If yes, we probably need to update the man
page.


--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Huang, Ying]

Zi Yan <ziy@nvidia.com> writes:

> On 14 Aug 2023, at 23:56, Huang, Ying wrote:
>
>> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>>
>>> On 2023/8/4 10:42, Zi Yan wrote:
>>>> On 3 Aug 2023, at 21:45, Kefeng Wang wrote:
>>>>
>>>>> On 2023/8/3 20:30, Matthew Wilcox wrote:
>>>>>> On Thu, Aug 03, 2023 at 03:13:21PM +0800, Kefeng Wang wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2023/8/2 20:21, Matthew Wilcox wrote:
>>>>>>>> On Wed, Aug 02, 2023 at 05:53:43PM +0800, Kefeng Wang wrote:
>>>>>>>>>     	err = -EACCES;
>>>>>>>>> -	if (page_mapcount(page) > 1 && !migrate_all)
>>>>>>>>> -		goto out_putpage;
>>>>>>>>> +	if (folio_estimated_sharers(folio) > 1 && !migrate_all)
>>>>>>>>> +		goto out_putfolio;
>>>>>>>>
>>>>>>>> I do not think this is the correct change.  Maybe leave this line
>>>>>>>> alone.
>>>>>>>
>>>>>>> Ok, I am aware of the discussion about this in other mail, will not
>>>>>>> change it(also the next two patch about this function), or wait the
>>>>>>> new work of David.
>>>>>>>>
>>>>>>>>> -	if (PageHuge(page)) {
>>>>>>>>> -		if (PageHead(page)) {
>>>>>>>>> -			isolated = isolate_hugetlb(page_folio(page), pagelist);
>>>>>>>>> +	if (folio_test_hugetlb(folio)) {
>>>>>>>>> +		if (folio_test_large(folio)) {
>>>>>>>>
>>>>>>>> This makes no sense when you read it.  All hugetlb folios are large,
>>>>>>>> by definition.  Think about what this code used to do, and what it
>>>>>>>> should be changed to.
>>>>>>>
>>>>>>> hugetlb folio is self large folio, will drop redundant check
>>>>>>
>>>>>> No, that's not the difference.  Keep thinking about it.  This is not
>>>>>> a mechanical translation!
>>>>>
>>>>>
>>>>>    if (PageHuge(page))  // page must be a hugetlb page
>>>>> 	if (PageHead(page)) // page must be a head page, not tail
>>>>>               isolate_hugetlb() // isolate the hugetlb page if head
>>>>>
>>>>> After using folio,
>>>>>
>>>>>    if (folio_test_hugetlb(folio)) // only check folio is hugetlb or not
>>>>>
>>>>> I don't check the page is head or not, since the follow_page could
>>>>> return a sub-page, so the check PageHead need be retained, right?
>>>> Right. It will prevent the kernel from trying to isolate the same
>>>> hugetlb page
>>>> twice when two pages are in the same hugetlb folio. But looking at the
>>>> code, if you try to isolate an already-isolated hugetlb folio, isolate_hugetlb()
>>>> would return false, no error would show up. But it changes err value
>>>> from -EACCES to -EBUSY and user will see a different page status than before.
>>>
>>>
>>> When check man[1], the current -EACCES is not right, -EBUSY is not
>>> precise but more suitable for this scenario,
>>>
>>>  	-EACCES
>>>               The page is mapped by multiple processes and can be moved
>>>               only if MPOL_MF_MOVE_ALL is specified.
>>>
>>>        -EBUSY The page is currently busy and cannot be moved.  Try again
>>>               later.  This occurs if a page is undergoing I/O or another
>>>               kernel subsystem is holding a reference to the page.
>>> 	-ENOENT
>>>               The page is not present.
>>>
>>>> I wonder why we do not have follow_folio() and returns -ENOENT error
>>>> pointer
>>>> when addr points to a non head page. It would make this patch more folio if
>>>> follow_folio() can be used in place of follow_page(). One caveat is that
>>>> user will see -ENOENT instead of -EACCES after this change.
>>>>
>>>
>>> -ENOENT is ok, but maybe the man need to be updated too.
>>>
>>>
>>> 	
>>> [1] https://man7.org/linux/man-pages/man2/move_pages.2.html
>>>
>>
>> I don't think -ENOENT is appropriate.  IIUC, -ENOENT means no need to
>> migrate.  Which isn't the case here apparently.
>
> Are you referring to a comment or the man page? The man page says
> -ENOENT means the page is not present. Or you think it also implies
> there is no need to migrate? If yes, we probably need to update the man
> page.

Is it possible that a page isn't present, but we need to migrate it?

--
Best Regards,
Huang, Ying

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Mike Kravetz]

On 08/15/23 11:58, Huang, Ying wrote:
> Mike Kravetz <mike.kravetz@oracle.com> writes:
> 
> > On 08/10/23 09:49, Kefeng Wang wrote:
> >> 
> >> 
> >> On 2023/8/10 6:44, Mike Kravetz wrote:
> >> > On 08/09/23 13:53, Mike Kravetz wrote:
> >> > > On 08/09/23 20:37, Kefeng Wang wrote:
> >> > > > > 
> >> > > > > Cc Mike to help us clarify the expected behavior of hugetlb.
> >> > > > > 
> >> > > > > Hi Mike, what is the expected behavior, if a user tries to use move_pages()
> >> > > > > to migrate a non head page of a hugetlb page?
> >> > > > 
> >> > > > Could you give some advise, thanks
> >> > > > 
> >> > > 
> >> > > Sorry, I was away for a while.
> >> > > 
> >> > > It seems unfortunate that move_pages says the passed user addresses
> >> > > should be aligned to page boundaries.  However, IIUC this is not checked
> >> > > or enforced.  Otherwise, passing a hugetlb page should return the same
> >> > > error.
> >> > > 
> >> > > One thought would be that hugetlb mappings should behave the same
> >> > > non-hugetlb mappings.  If passed the address of a hugetlb tail page, align
> >> > > the address to a hugetlb boundary and migrate the page.  This changes the
> >> > > existing behavior.  However, it would be hard to imagine anyone depending
> >> > > on this.
> >> > > 
> >> > > After taking a closer look at the add_page_for_migration(), it seems to
> >> > > just ignore passed tail pages and do nothing for such passed addresses.
> >> > > Correct?  Or, am I missing something?  Perhaps that is behavior we want/
> >> > > need to preserve?
> >> > 
> >> > My mistake, status -EACCES is returned when passing a tail page of a
> >> > hugetlb page.
> >> > 
> >> 
> >> As mentioned in previous mail， before e66f17ff7177 ("mm/hugetlb: take
> >> page table lock in follow_huge_pmd()") in v4.0, follow_page() will
> >> return NULL on tail page for Huagetlb page, so move_pages() will return
> >> -ENOENT errno, but after that commit, -EACCES is returned.
> >> 
> >> Meanwhile, the behavior of THP/HUGETLB is different, the whole THP will be
> >> migrated on a tail page, but HUGETLB will return -EACCES(after v4.0)
> >> or -ENOENT(before v4.0) on tail page.
> >> 
> >> > Back to the question of 'What is the expected behavior if a tail page is
> >> > passed?'.  I do not think we have defined an expected behavior.  If
> >> > anything is 'expected' I would say it is -EACCES as returned today.
> >> > 
> >> 
> >> My question is,
> >> 
> >> Should we keep seem behavior between HUGETLB and THP, or only change the
> >> errno from -EACCES to -ENOENT/-EBUSY.
> >
> > Just to be clear.  When you say "keep seem behavior between HUGETLB and THP",
> > are you saying that you would like hugetlb to perform migration of the entire
> > hugetlb page if a tail page is passed?
> >
> > IMO, this would be ideal as it would mean that hugetlb and THP behave the same
> > when passed the address of a tail page.  The fewer places where hugetlb
> > behavior diverges, the better.  However, this does change behavior.
> 
> A separate patch will be needed for behavior change.
> 

Correct.

Since the goal of this series is to convert to folios, we should maintain the
existing behavior and errno (-EACCES).  In a subsequent patch, we can
change behavior.

That would be my suggestion.
-- 
Mike Kravetz

Re: [PATCH 1/4] mm: migrate: use a folio in add_page_for_migration()

[Kefeng Wang]

On 2023/8/16 5:12, Mike Kravetz wrote:
> On 08/15/23 11:58, Huang, Ying wrote:
>> Mike Kravetz <mike.kravetz@oracle.com> writes:
>>
>>> On 08/10/23 09:49, Kefeng Wang wrote:
>>>>
>>>>
>>>> On 2023/8/10 6:44, Mike Kravetz wrote:
>>>>> On 08/09/23 13:53, Mike Kravetz wrote:
>>>>>> On 08/09/23 20:37, Kefeng Wang wrote:
>>>>>>>>
>>>>>>>> Cc Mike to help us clarify the expected behavior of hugetlb.
>>>>>>>>
>>>>>>>> Hi Mike, what is the expected behavior, if a user tries to use move_pages()
>>>>>>>> to migrate a non head page of a hugetlb page?
>>>>>>>
>>>>>>> Could you give some advise, thanks
>>>>>>>
>>>>>>
>>>>>> Sorry, I was away for a while.
>>>>>>
>>>>>> It seems unfortunate that move_pages says the passed user addresses
>>>>>> should be aligned to page boundaries.  However, IIUC this is not checked
>>>>>> or enforced.  Otherwise, passing a hugetlb page should return the same
>>>>>> error.
>>>>>>
>>>>>> One thought would be that hugetlb mappings should behave the same
>>>>>> non-hugetlb mappings.  If passed the address of a hugetlb tail page, align
>>>>>> the address to a hugetlb boundary and migrate the page.  This changes the
>>>>>> existing behavior.  However, it would be hard to imagine anyone depending
>>>>>> on this.
>>>>>>
>>>>>> After taking a closer look at the add_page_for_migration(), it seems to
>>>>>> just ignore passed tail pages and do nothing for such passed addresses.
>>>>>> Correct?  Or, am I missing something?  Perhaps that is behavior we want/
>>>>>> need to preserve?
>>>>>
>>>>> My mistake, status -EACCES is returned when passing a tail page of a
>>>>> hugetlb page.
>>>>>
>>>>
>>>> As mentioned in previous mail， before e66f17ff7177 ("mm/hugetlb: take
>>>> page table lock in follow_huge_pmd()") in v4.0, follow_page() will
>>>> return NULL on tail page for Huagetlb page, so move_pages() will return
>>>> -ENOENT errno, but after that commit, -EACCES is returned.
>>>>
>>>> Meanwhile, the behavior of THP/HUGETLB is different, the whole THP will be
>>>> migrated on a tail page, but HUGETLB will return -EACCES(after v4.0)
>>>> or -ENOENT(before v4.0) on tail page.
>>>>
>>>>> Back to the question of 'What is the expected behavior if a tail page is
>>>>> passed?'.  I do not think we have defined an expected behavior.  If
>>>>> anything is 'expected' I would say it is -EACCES as returned today.
>>>>>
>>>>
>>>> My question is,
>>>>
>>>> Should we keep seem behavior between HUGETLB and THP, or only change the
>>>> errno from -EACCES to -ENOENT/-EBUSY.
>>>
>>> Just to be clear.  When you say "keep seem behavior between HUGETLB and THP",
>>> are you saying that you would like hugetlb to perform migration of the entire
>>> hugetlb page if a tail page is passed?
>>>
>>> IMO, this would be ideal as it would mean that hugetlb and THP behave the same
>>> when passed the address of a tail page.  The fewer places where hugetlb
>>> behavior diverges, the better.  However, this does change behavior.
>>
>> A separate patch will be needed for behavior change.
>>
> 
> Correct.
> 
> Since the goal of this series is to convert to folios, we should maintain the
> existing behavior and errno (-EACCES).  In a subsequent patch, we can
> change behavior.
> 
> That would be my suggestion.


Thanks all, will following the suggestion and re-post.