[PATCH] mm: migrate: Correct the hugetlb migration stats

[PATCH] mm: migrate: Correct the hugetlb migration stats

[Baolin Wang]

Now hugetlb migration is also available for some scenarios, such as
soft offling or memory compaction. So we should correct the migration
stats for hugetlb with using compound_nr() instead of thp_nr_pages()
to get the number of pages.

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 a11e948..2b45a29 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1475,7 +1475,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 			 * during migration.
 			 */
 			is_thp = PageTransHuge(page) && !PageHuge(page);
-			nr_subpages = thp_nr_pages(page);
+			nr_subpages = compound_nr(page);
 			cond_resched();
 
 			if (PageHuge(page))
@@ -1540,7 +1540,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 					nr_failed += nr_subpages;
 					goto out;
 				}
-				nr_failed++;
+				nr_failed += nr_subpages;
 				goto out;
 			case -EAGAIN:
 				if (is_thp) {
@@ -1550,14 +1550,14 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 				retry++;
 				break;
 			case MIGRATEPAGE_SUCCESS:
+				nr_succeeded += nr_subpages;
 				if (is_thp) {
 					nr_thp_succeeded++;
-					nr_succeeded += nr_subpages;
 					break;
 				}
-				nr_succeeded++;
 				break;
 			default:
+				nr_failed += nr_subpages;
 				/*
 				 * Permanent failure (-EBUSY, etc.):
 				 * unlike -EAGAIN case, the failed page is
@@ -1566,10 +1566,8 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 				 */
 				if (is_thp) {
 					nr_thp_failed++;
-					nr_failed += nr_subpages;
 					break;
 				}
-				nr_failed++;
 				break;
 			}
 		}
-- 
1.8.3.1

Re: [PATCH] mm: migrate: Correct the hugetlb migration stats

[Zi Yan]

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

On 29 Oct 2021, at 3:42, Baolin Wang wrote:

> Now hugetlb migration is also available for some scenarios, such as
> soft offling or memory compaction. So we should correct the migration

hugetlb migration is available at the time if (PageHuge(page)) branch
is added. I am not sure what is new here.

> stats for hugetlb with using compound_nr() instead of thp_nr_pages()
> to get the number of pages.

nr_failed records the number of pages, not subpages. It is returned to
user space when move_pages() syscall is used. After your change,
if users try to migrate a list of pages including THPs and/or hugetlb
pages and some of THPs and/or hugetlb fail to migrate, move_pages()
will return a number larger than the number of pages the users tried
to migrate. I am not sure this is the change we want. Or at least,
the comment of migrate_pages() and the manpage of move_pages() need
to be changed and linux-api mailing list should be cc’d.

>
> 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 a11e948..2b45a29 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1475,7 +1475,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  			 * during migration.
>  			 */
>  			is_thp = PageTransHuge(page) && !PageHuge(page);
> -			nr_subpages = thp_nr_pages(page);
> +			nr_subpages = compound_nr(page);
>  			cond_resched();
>
>  			if (PageHuge(page))
> @@ -1540,7 +1540,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  					nr_failed += nr_subpages;
>  					goto out;
>  				}
> -				nr_failed++;
> +				nr_failed += nr_subpages;
>  				goto out;
>  			case -EAGAIN:
>  				if (is_thp) {
> @@ -1550,14 +1550,14 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  				retry++;
>  				break;
>  			case MIGRATEPAGE_SUCCESS:
> +				nr_succeeded += nr_subpages;
>  				if (is_thp) {
>  					nr_thp_succeeded++;
> -					nr_succeeded += nr_subpages;
>  					break;
>  				}
> -				nr_succeeded++;
>  				break;
>  			default:
> +				nr_failed += nr_subpages;
>  				/*
>  				 * Permanent failure (-EBUSY, etc.):
>  				 * unlike -EAGAIN case, the failed page is
> @@ -1566,10 +1566,8 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  				 */
>  				if (is_thp) {
>  					nr_thp_failed++;
> -					nr_failed += nr_subpages;
>  					break;
>  				}
> -				nr_failed++;
>  				break;
>  			}
>  		}
> -- 
> 1.8.3.1

--
Best Regards,
Yan, Zi

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

Re: [PATCH] mm: migrate: Correct the hugetlb migration stats

[Baolin Wang]

On 2021/10/29 23:43, Zi Yan wrote:
> On 29 Oct 2021, at 3:42, Baolin Wang wrote:
> 
>> Now hugetlb migration is also available for some scenarios, such as
>> soft offling or memory compaction. So we should correct the migration
> 
> hugetlb migration is available at the time if (PageHuge(page)) branch
> is added. I am not sure what is new here.

No new things actually, sorry for confusing and will update the commit 
message in next version.

> 
>> stats for hugetlb with using compound_nr() instead of thp_nr_pages()
>> to get the number of pages.
> 
> nr_failed records the number of pages, not subpages. It is returned to

I also think nr_failed should record the number of pages, not the number 
of hugetlb, if I understand you correctly.

> user space when move_pages() syscall is used. After your change,
> if users try to migrate a list of pages including THPs and/or hugetlb
> pages and some of THPs and/or hugetlb fail to migrate, move_pages()
> will return a number larger than the number of pages the users tried

OK, thanks for pointing out the issue.

But before my patch, we've already returned the number of pages 
successed or failed for THP migration, instead of the number of THP. 
That means if we just move only 1 page by move_pages() and if this page 
is 2M THP, so move_pages() will return 512 if failed to migrate, which 
is larger than the page count specified from user.

if (err > 0)
	err += nr_pages - i - 1;

On the other hand, the stats of PGMIGRATE_SUCCESS/PGMIGRATE_FAIL should 
stand for the number of pages, instead of the number of hugetlb. Also 
for hugetlb migration when memory compaction, we've already counted the 
number of pages for a hugetlb into cc->nr_migratepages, if the hugetlb 
migration failed, the trace stat of compaction will be confusing if we 
return the number of hugetlb.

trace_mm_compaction_migratepages(cc->nr_migratepages, err, 
                                   &cc->migratepages);

So I think the stats of hugetlb migration should be consistent with THP.

> to migrate. I am not sure this is the change we want. Or at least,
> the comment of migrate_pages() and the manpage of move_pages() need
> to be changed and linux-api mailing list should be cc’d.

I don't think we should update the comments of migrate_pages(), "Returns 
the number of pages that were not migrated" makes sense to me if I 
understand correctly.

For the manpage of move_pages(), as you said, the the returned 
non-migrate page numbers can be larger than the numbers specified from 
user if failed to migrate a THP or a hugetlb. I am not sure if we should 
change the manpage, since the THP already did, but I can send a patch to 
update the manpage if you think this is still necessary. Thanks.

>>
>> 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 a11e948..2b45a29 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1475,7 +1475,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>   			 * during migration.
>>   			 */
>>   			is_thp = PageTransHuge(page) && !PageHuge(page);
>> -			nr_subpages = thp_nr_pages(page);
>> +			nr_subpages = compound_nr(page);
>>   			cond_resched();
>>
>>   			if (PageHuge(page))
>> @@ -1540,7 +1540,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>   					nr_failed += nr_subpages;
>>   					goto out;
>>   				}
>> -				nr_failed++;
>> +				nr_failed += nr_subpages;
>>   				goto out;
>>   			case -EAGAIN:
>>   				if (is_thp) {
>> @@ -1550,14 +1550,14 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>   				retry++;
>>   				break;
>>   			case MIGRATEPAGE_SUCCESS:
>> +				nr_succeeded += nr_subpages;
>>   				if (is_thp) {
>>   					nr_thp_succeeded++;
>> -					nr_succeeded += nr_subpages;
>>   					break;
>>   				}
>> -				nr_succeeded++;
>>   				break;
>>   			default:
>> +				nr_failed += nr_subpages;
>>   				/*
>>   				 * Permanent failure (-EBUSY, etc.):
>>   				 * unlike -EAGAIN case, the failed page is
>> @@ -1566,10 +1566,8 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>   				 */
>>   				if (is_thp) {
>>   					nr_thp_failed++;
>> -					nr_failed += nr_subpages;
>>   					break;
>>   				}
>> -				nr_failed++;
>>   				break;
>>   			}
>>   		}
>> -- 
>> 1.8.3.1
> 
> --
> Best Regards,
> Yan, Zi
> 

Re: [PATCH] mm: migrate: Correct the hugetlb migration stats

[Zi Yan]

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

On 1 Nov 2021, at 2:54, Baolin Wang wrote:

> On 2021/10/29 23:43, Zi Yan wrote:
>> On 29 Oct 2021, at 3:42, Baolin Wang wrote:
>>
>>> Now hugetlb migration is also available for some scenarios, such as
>>> soft offling or memory compaction. So we should correct the migration
>>
>> hugetlb migration is available at the time if (PageHuge(page)) branch
>> is added. I am not sure what is new here.
>
> No new things actually, sorry for confusing and will update the commit message in next version.
>
>>
>>> stats for hugetlb with using compound_nr() instead of thp_nr_pages()
>>> to get the number of pages.
>>
>> nr_failed records the number of pages, not subpages. It is returned to
>
> I also think nr_failed should record the number of pages, not the number of hugetlb, if I understand you correctly.
>
>> user space when move_pages() syscall is used. After your change,
>> if users try to migrate a list of pages including THPs and/or hugetlb
>> pages and some of THPs and/or hugetlb fail to migrate, move_pages()
>> will return a number larger than the number of pages the users tried
>
> OK, thanks for pointing out the issue.
>
> But before my patch, we've already returned the number of pages successed or failed for THP migration, instead of the number of THP. That means if we just move only 1 page by

Ah, you are right.

> move_pages() and if this page is 2M THP, so move_pages() will return 512 if failed to migrate, which is larger than the page count specified from user.
>
> if (err > 0)
> 	err += nr_pages - i - 1;

I am not sure this is right for user-space.

>
> On the other hand, the stats of PGMIGRATE_SUCCESS/PGMIGRATE_FAIL should stand for the number of pages, instead of the number of hugetlb. Also for hugetlb migration when memory compaction, we've already counted the number of pages for a hugetlb into cc->nr_migratepages, if the hugetlb migration failed, the trace stat of compaction will be confusing if we return the number of hugetlb.
>
> trace_mm_compaction_migratepages(cc->nr_migratepages, err,                                   &cc->migratepages);
>
> So I think the stats of hugetlb migration should be consistent with THP.

It makes sense to me.

>
>> to migrate. I am not sure this is the change we want. Or at least,
>> the comment of migrate_pages() and the manpage of move_pages() need
>> to be changed and linux-api mailing list should be cc’d.
>
> I don't think we should update the comments of migrate_pages(), "Returns the number of pages that were not migrated" makes sense to me if I understand correctly.
>
> For the manpage of move_pages(), as you said, the the returned non-migrate page numbers can be larger than the numbers specified from user if failed to migrate a THP or a hugetlb. I am not sure if we should change the manpage, since the THP already did, but I can send a patch to update the manpage if you think this is still necessary. Thanks.

I am not sure changing manpage would help the users of move_pages() after
think about it again, since users might not know all the THP and/or hugetlb
information	when they call move_pages() and they just pass a list of N pages.

I just wonder if we could fix the rc value of migrate_pages to return
the number of {base page, THP, hugetlb} instead, so that move_pages()
can get its return value right.

Thanks.

>
>>>
>>> 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 a11e948..2b45a29 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -1475,7 +1475,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>   			 * during migration.
>>>   			 */
>>>   			is_thp = PageTransHuge(page) && !PageHuge(page);
>>> -			nr_subpages = thp_nr_pages(page);
>>> +			nr_subpages = compound_nr(page);
>>>   			cond_resched();
>>>
>>>   			if (PageHuge(page))
>>> @@ -1540,7 +1540,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>   					nr_failed += nr_subpages;
>>>   					goto out;
>>>   				}
>>> -				nr_failed++;
>>> +				nr_failed += nr_subpages;
>>>   				goto out;
>>>   			case -EAGAIN:
>>>   				if (is_thp) {
>>> @@ -1550,14 +1550,14 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>   				retry++;
>>>   				break;
>>>   			case MIGRATEPAGE_SUCCESS:
>>> +				nr_succeeded += nr_subpages;
>>>   				if (is_thp) {
>>>   					nr_thp_succeeded++;
>>> -					nr_succeeded += nr_subpages;
>>>   					break;
>>>   				}
>>> -				nr_succeeded++;
>>>   				break;
>>>   			default:
>>> +				nr_failed += nr_subpages;
>>>   				/*
>>>   				 * Permanent failure (-EBUSY, etc.):
>>>   				 * unlike -EAGAIN case, the failed page is
>>> @@ -1566,10 +1566,8 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>   				 */
>>>   				if (is_thp) {
>>>   					nr_thp_failed++;
>>> -					nr_failed += nr_subpages;
>>>   					break;
>>>   				}
>>> -				nr_failed++;
>>>   				break;
>>>   			}
>>>   		}
>>> -- 
>>> 1.8.3.1
>>
>> --
>> Best Regards,
>> Yan, Zi
>>


--
Best Regards,
Yan, Zi

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

Re: [PATCH] mm: migrate: Correct the hugetlb migration stats

[Baolin Wang]

On 2021/11/1 23:12, Zi Yan wrote:
> On 1 Nov 2021, at 2:54, Baolin Wang wrote:
> 
>> On 2021/10/29 23:43, Zi Yan wrote:
>>> On 29 Oct 2021, at 3:42, Baolin Wang wrote:
>>>
>>>> Now hugetlb migration is also available for some scenarios, such as
>>>> soft offling or memory compaction. So we should correct the migration
>>>
>>> hugetlb migration is available at the time if (PageHuge(page)) branch
>>> is added. I am not sure what is new here.
>>
>> No new things actually, sorry for confusing and will update the commit message in next version.
>>
>>>
>>>> stats for hugetlb with using compound_nr() instead of thp_nr_pages()
>>>> to get the number of pages.
>>>
>>> nr_failed records the number of pages, not subpages. It is returned to
>>
>> I also think nr_failed should record the number of pages, not the number of hugetlb, if I understand you correctly.
>>
>>> user space when move_pages() syscall is used. After your change,
>>> if users try to migrate a list of pages including THPs and/or hugetlb
>>> pages and some of THPs and/or hugetlb fail to migrate, move_pages()
>>> will return a number larger than the number of pages the users tried
>>
>> OK, thanks for pointing out the issue.
>>
>> But before my patch, we've already returned the number of pages successed or failed for THP migration, instead of the number of THP. That means if we just move only 1 page by
> 
> Ah, you are right.
> 
>> move_pages() and if this page is 2M THP, so move_pages() will return 512 if failed to migrate, which is larger than the page count specified from user.
>>
>> if (err > 0)
>> 	err += nr_pages - i - 1;
> 
> I am not sure this is right for user-space.
> 
>>
>> On the other hand, the stats of PGMIGRATE_SUCCESS/PGMIGRATE_FAIL should stand for the number of pages, instead of the number of hugetlb. Also for hugetlb migration when memory compaction, we've already counted the number of pages for a hugetlb into cc->nr_migratepages, if the hugetlb migration failed, the trace stat of compaction will be confusing if we return the number of hugetlb.
>>
>> trace_mm_compaction_migratepages(cc->nr_migratepages, err,                                   &cc->migratepages);
>>
>> So I think the stats of hugetlb migration should be consistent with THP.
> 
> It makes sense to me.
> 
>>
>>> to migrate. I am not sure this is the change we want. Or at least,
>>> the comment of migrate_pages() and the manpage of move_pages() need
>>> to be changed and linux-api mailing list should be cc’d.
>>
>> I don't think we should update the comments of migrate_pages(), "Returns the number of pages that were not migrated" makes sense to me if I understand correctly.
>>
>> For the manpage of move_pages(), as you said, the the returned non-migrate page numbers can be larger than the numbers specified from user if failed to migrate a THP or a hugetlb. I am not sure if we should change the manpage, since the THP already did, but I can send a patch to update the manpage if you think this is still necessary. Thanks.
> 
> I am not sure changing manpage would help the users of move_pages() after
> think about it again, since users might not know all the THP and/or hugetlb
> information	when they call move_pages() and they just pass a list of N pages. >
> I just wonder if we could fix the rc value of migrate_pages to return
> the number of {base page, THP, hugetlb} instead, so that move_pages()
> can get its return value right.

IMO it will break the usage in other places if we change the rc value of 
migrate_pages(), for example, the page migration when doing memory 
compaction as I said before, which will expect the number of normal 
pages. Meanwhile the THP page can be split into normal pages during 
migration, so it will not be consistent if we return the number of THP.

Changing the return value of migrate_pages() will make things more 
complicated, and I am not sure whether it is worth doing. Any 
suggestion? Thanks.

Re: [PATCH] mm: migrate: Correct the hugetlb migration stats

[Zi Yan]

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

On 2 Nov 2021, at 2:08, Baolin Wang wrote:

> On 2021/11/1 23:12, Zi Yan wrote:
>> On 1 Nov 2021, at 2:54, Baolin Wang wrote:
>>
>>> On 2021/10/29 23:43, Zi Yan wrote:
>>>> On 29 Oct 2021, at 3:42, Baolin Wang wrote:
>>>>
>>>>> Now hugetlb migration is also available for some scenarios, such as
>>>>> soft offling or memory compaction. So we should correct the migration
>>>>
>>>> hugetlb migration is available at the time if (PageHuge(page)) branch
>>>> is added. I am not sure what is new here.
>>>
>>> No new things actually, sorry for confusing and will update the commit message in next version.
>>>
>>>>
>>>>> stats for hugetlb with using compound_nr() instead of thp_nr_pages()
>>>>> to get the number of pages.
>>>>
>>>> nr_failed records the number of pages, not subpages. It is returned to
>>>
>>> I also think nr_failed should record the number of pages, not the number of hugetlb, if I understand you correctly.
>>>
>>>> user space when move_pages() syscall is used. After your change,
>>>> if users try to migrate a list of pages including THPs and/or hugetlb
>>>> pages and some of THPs and/or hugetlb fail to migrate, move_pages()
>>>> will return a number larger than the number of pages the users tried
>>>
>>> OK, thanks for pointing out the issue.
>>>
>>> But before my patch, we've already returned the number of pages successed or failed for THP migration, instead of the number of THP. That means if we just move only 1 page by
>>
>> Ah, you are right.
>>
>>> move_pages() and if this page is 2M THP, so move_pages() will return 512 if failed to migrate, which is larger than the page count specified from user.
>>>
>>> if (err > 0)
>>> 	err += nr_pages - i - 1;
>>
>> I am not sure this is right for user-space.
>>
>>>
>>> On the other hand, the stats of PGMIGRATE_SUCCESS/PGMIGRATE_FAIL should stand for the number of pages, instead of the number of hugetlb. Also for hugetlb migration when memory compaction, we've already counted the number of pages for a hugetlb into cc->nr_migratepages, if the hugetlb migration failed, the trace stat of compaction will be confusing if we return the number of hugetlb.
>>>
>>> trace_mm_compaction_migratepages(cc->nr_migratepages, err,                                   &cc->migratepages);
>>>
>>> So I think the stats of hugetlb migration should be consistent with THP.
>>
>> It makes sense to me.
>>
>>>
>>>> to migrate. I am not sure this is the change we want. Or at least,
>>>> the comment of migrate_pages() and the manpage of move_pages() need
>>>> to be changed and linux-api mailing list should be cc’d.
>>>
>>> I don't think we should update the comments of migrate_pages(), "Returns the number of pages that were not migrated" makes sense to me if I understand correctly.
>>>
>>> For the manpage of move_pages(), as you said, the the returned non-migrate page numbers can be larger than the numbers specified from user if failed to migrate a THP or a hugetlb. I am not sure if we should change the manpage, since the THP already did, but I can send a patch to update the manpage if you think this is still necessary. Thanks.
>>
>> I am not sure changing manpage would help the users of move_pages() after
>> think about it again, since users might not know all the THP and/or hugetlb
>> information	when they call move_pages() and they just pass a list of N pages. >
>> I just wonder if we could fix the rc value of migrate_pages to return
>> the number of {base page, THP, hugetlb} instead, so that move_pages()
>> can get its return value right.
>
> IMO it will break the usage in other places if we change the rc value of migrate_pages(), for example, the page migration when doing memory compaction as I said before, which will expect the number of normal pages. Meanwhile the THP page can be split into normal pages during migration, so it will not be consistent if we return the number of THP.

You mean the above trace_mm_compaction_migratepages()? I checked all migrate_pages()
callers and none of them cares about the actual number of non-migrated pages, except
do_move_pages_to_node() and trace_mm_compaction_migratepages(). The former expects
the number of before-split-and-not-subpage pages, whereas the latter expects, like
you said, the number of base pages.

>
> Changing the return value of migrate_pages() will make things more complicated, and I am not sure whether it is worth doing. Any suggestion? Thanks.

How about 1) fixing migrate_pages() to return the number of before-split-and-not-subpage
pages, and 2) replace err with nr_succeeded (you can get it via *ret_succeeded in
migrate_pages()) in trace_mm_compaction_migratepages()? As a result, user-space move_pages()
will be fixed and trace_mm_compaction_migratepages() gives a
different but correct number as you want (you can still get nr_nonmigrated =
nr_migratepages - nr_succeeded).



--
Best Regards,
Yan, Zi

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

Re: [PATCH] mm: migrate: Correct the hugetlb migration stats

[Baolin Wang]

On 2021/11/3 3:30, Zi Yan wrote:
> On 2 Nov 2021, at 2:08, Baolin Wang wrote:
> 
>> On 2021/11/1 23:12, Zi Yan wrote:
>>> On 1 Nov 2021, at 2:54, Baolin Wang wrote:
>>>
>>>> On 2021/10/29 23:43, Zi Yan wrote:
>>>>> On 29 Oct 2021, at 3:42, Baolin Wang wrote:
>>>>>
>>>>>> Now hugetlb migration is also available for some scenarios, such as
>>>>>> soft offling or memory compaction. So we should correct the migration
>>>>>
>>>>> hugetlb migration is available at the time if (PageHuge(page)) branch
>>>>> is added. I am not sure what is new here.
>>>>
>>>> No new things actually, sorry for confusing and will update the commit message in next version.
>>>>
>>>>>
>>>>>> stats for hugetlb with using compound_nr() instead of thp_nr_pages()
>>>>>> to get the number of pages.
>>>>>
>>>>> nr_failed records the number of pages, not subpages. It is returned to
>>>>
>>>> I also think nr_failed should record the number of pages, not the number of hugetlb, if I understand you correctly.
>>>>
>>>>> user space when move_pages() syscall is used. After your change,
>>>>> if users try to migrate a list of pages including THPs and/or hugetlb
>>>>> pages and some of THPs and/or hugetlb fail to migrate, move_pages()
>>>>> will return a number larger than the number of pages the users tried
>>>>
>>>> OK, thanks for pointing out the issue.
>>>>
>>>> But before my patch, we've already returned the number of pages successed or failed for THP migration, instead of the number of THP. That means if we just move only 1 page by
>>>
>>> Ah, you are right.
>>>
>>>> move_pages() and if this page is 2M THP, so move_pages() will return 512 if failed to migrate, which is larger than the page count specified from user.
>>>>
>>>> if (err > 0)
>>>> 	err += nr_pages - i - 1;
>>>
>>> I am not sure this is right for user-space.
>>>
>>>>
>>>> On the other hand, the stats of PGMIGRATE_SUCCESS/PGMIGRATE_FAIL should stand for the number of pages, instead of the number of hugetlb. Also for hugetlb migration when memory compaction, we've already counted the number of pages for a hugetlb into cc->nr_migratepages, if the hugetlb migration failed, the trace stat of compaction will be confusing if we return the number of hugetlb.
>>>>
>>>> trace_mm_compaction_migratepages(cc->nr_migratepages, err,                                   &cc->migratepages);
>>>>
>>>> So I think the stats of hugetlb migration should be consistent with THP.
>>>
>>> It makes sense to me.
>>>
>>>>
>>>>> to migrate. I am not sure this is the change we want. Or at least,
>>>>> the comment of migrate_pages() and the manpage of move_pages() need
>>>>> to be changed and linux-api mailing list should be cc’d.
>>>>
>>>> I don't think we should update the comments of migrate_pages(), "Returns the number of pages that were not migrated" makes sense to me if I understand correctly.
>>>>
>>>> For the manpage of move_pages(), as you said, the the returned non-migrate page numbers can be larger than the numbers specified from user if failed to migrate a THP or a hugetlb. I am not sure if we should change the manpage, since the THP already did, but I can send a patch to update the manpage if you think this is still necessary. Thanks.
>>>
>>> I am not sure changing manpage would help the users of move_pages() after
>>> think about it again, since users might not know all the THP and/or hugetlb
>>> information	when they call move_pages() and they just pass a list of N pages. >
>>> I just wonder if we could fix the rc value of migrate_pages to return
>>> the number of {base page, THP, hugetlb} instead, so that move_pages()
>>> can get its return value right.
>>
>> IMO it will break the usage in other places if we change the rc value of migrate_pages(), for example, the page migration when doing memory compaction as I said before, which will expect the number of normal pages. Meanwhile the THP page can be split into normal pages during migration, so it will not be consistent if we return the number of THP.
> 
> You mean the above trace_mm_compaction_migratepages()? I checked all migrate_pages()

Right.

> callers and none of them cares about the actual number of non-migrated pages, except
> do_move_pages_to_node() and trace_mm_compaction_migratepages(). The former expects
> the number of before-split-and-not-subpage pages, whereas the latter expects, like
> you said, the number of base pages.

Yes.

>>
>> Changing the return value of migrate_pages() will make things more complicated, and I am not sure whether it is worth doing. Any suggestion? Thanks.
> 
> How about 1) fixing migrate_pages() to return the number of before-split-and-not-subpage
> pages, and 2) replace err with nr_succeeded (you can get it via *ret_succeeded in
> migrate_pages()) in trace_mm_compaction_migratepages()? As a result, user-space move_pages()
> will be fixed and trace_mm_compaction_migratepages() gives a
> different but correct number as you want (you can still get nr_nonmigrated =
> nr_migratepages - nr_succeeded).

OK, sounds reasonable to me, I can try it. Thanks for your input.

Re: [PATCH] mm: migrate: Correct the hugetlb migration stats

[Baolin Wang]

> On 2021/11/3 3:30, Zi Yan wrote:
>> On 2 Nov 2021, at 2:08, Baolin Wang wrote:
>>
>>> On 2021/11/1 23:12, Zi Yan wrote:
>>>> On 1 Nov 2021, at 2:54, Baolin Wang wrote:
>>>>
>>>>> On 2021/10/29 23:43, Zi Yan wrote:
>>>>>> On 29 Oct 2021, at 3:42, Baolin Wang wrote:
>>>>>>
>>>>>>> Now hugetlb migration is also available for some scenarios, such as
>>>>>>> soft offling or memory compaction. So we should correct the 
>>>>>>> migration
>>>>>>
>>>>>> hugetlb migration is available at the time if (PageHuge(page)) branch
>>>>>> is added. I am not sure what is new here.
>>>>>
>>>>> No new things actually, sorry for confusing and will update the 
>>>>> commit message in next version.
>>>>>
>>>>>>
>>>>>>> stats for hugetlb with using compound_nr() instead of thp_nr_pages()
>>>>>>> to get the number of pages.
>>>>>>
>>>>>> nr_failed records the number of pages, not subpages. It is 
>>>>>> returned to
>>>>>
>>>>> I also think nr_failed should record the number of pages, not the 
>>>>> number of hugetlb, if I understand you correctly.
>>>>>
>>>>>> user space when move_pages() syscall is used. After your change,
>>>>>> if users try to migrate a list of pages including THPs and/or hugetlb
>>>>>> pages and some of THPs and/or hugetlb fail to migrate, move_pages()
>>>>>> will return a number larger than the number of pages the users tried
>>>>>
>>>>> OK, thanks for pointing out the issue.
>>>>>
>>>>> But before my patch, we've already returned the number of pages 
>>>>> successed or failed for THP migration, instead of the number of 
>>>>> THP. That means if we just move only 1 page by
>>>>
>>>> Ah, you are right.
>>>>
>>>>> move_pages() and if this page is 2M THP, so move_pages() will 
>>>>> return 512 if failed to migrate, which is larger than the page 
>>>>> count specified from user.
>>>>>
>>>>> if (err > 0)
>>>>>     err += nr_pages - i - 1;
>>>>
>>>> I am not sure this is right for user-space.
>>>>
>>>>>
>>>>> On the other hand, the stats of PGMIGRATE_SUCCESS/PGMIGRATE_FAIL 
>>>>> should stand for the number of pages, instead of the number of 
>>>>> hugetlb. Also for hugetlb migration when memory compaction, we've 
>>>>> already counted the number of pages for a hugetlb into 
>>>>> cc->nr_migratepages, if the hugetlb migration failed, the trace 
>>>>> stat of compaction will be confusing if we return the number of 
>>>>> hugetlb.
>>>>>
>>>>> trace_mm_compaction_migratepages(cc->nr_migratepages, 
>>>>> err,                                   &cc->migratepages);
>>>>>
>>>>> So I think the stats of hugetlb migration should be consistent with 
>>>>> THP.
>>>>
>>>> It makes sense to me.
>>>>
>>>>>
>>>>>> to migrate. I am not sure this is the change we want. Or at least,
>>>>>> the comment of migrate_pages() and the manpage of move_pages() need
>>>>>> to be changed and linux-api mailing list should be cc’d.
>>>>>
>>>>> I don't think we should update the comments of migrate_pages(), 
>>>>> "Returns the number of pages that were not migrated" makes sense to 
>>>>> me if I understand correctly.
>>>>>
>>>>> For the manpage of move_pages(), as you said, the the returned 
>>>>> non-migrate page numbers can be larger than the numbers specified 
>>>>> from user if failed to migrate a THP or a hugetlb. I am not sure if 
>>>>> we should change the manpage, since the THP already did, but I can 
>>>>> send a patch to update the manpage if you think this is still 
>>>>> necessary. Thanks.
>>>>
>>>> I am not sure changing manpage would help the users of move_pages() 
>>>> after
>>>> think about it again, since users might not know all the THP and/or 
>>>> hugetlb
>>>> information    when they call move_pages() and they just pass a list 
>>>> of N pages. >
>>>> I just wonder if we could fix the rc value of migrate_pages to return
>>>> the number of {base page, THP, hugetlb} instead, so that move_pages()
>>>> can get its return value right.
>>>
>>> IMO it will break the usage in other places if we change the rc value 
>>> of migrate_pages(), for example, the page migration when doing memory 
>>> compaction as I said before, which will expect the number of normal 
>>> pages. Meanwhile the THP page can be split into normal pages during 
>>> migration, so it will not be consistent if we return the number of THP.
>>
>> You mean the above trace_mm_compaction_migratepages()? I checked all 
>> migrate_pages()
> 
> Right.
> 
>> callers and none of them cares about the actual number of non-migrated 
>> pages, except
>> do_move_pages_to_node() and trace_mm_compaction_migratepages(). The 
>> former expects
>> the number of before-split-and-not-subpage pages, whereas the latter 
>> expects, like
>> you said, the number of base pages.
> 
> Yes.
> 
>>>
>>> Changing the return value of migrate_pages() will make things more 
>>> complicated, and I am not sure whether it is worth doing. Any 
>>> suggestion? Thanks.
>>
>> How about 1) fixing migrate_pages() to return the number of 
>> before-split-and-not-subpage
>> pages, and 2) replace err with nr_succeeded (you can get it via 
>> *ret_succeeded in
>> migrate_pages()) in trace_mm_compaction_migratepages()? As a result, 
>> user-space move_pages()
>> will be fixed and trace_mm_compaction_migratepages() gives a
>> different but correct number as you want (you can still get 
>> nr_nonmigrated =
>> nr_migratepages - nr_succeeded).
> 
> OK, sounds reasonable to me, I can try it. Thanks for your input.

After more thinking, I am still strugglling to handle the THP split 
case. Suppose the move_pages() tries to move 1 page, which is a 2M THP. 
If the 2M THP is split into 512 normal pages during migration, and 256 
normal pages are migrated successfully, others are failed. So we should 
return 1 or 256 non-migrated pages from migrate_pages()?

Anyway I posted new RFC patchset [1] according to your suggestion, we 
can talk about there.

[1] 
https://lore.kernel.org/linux-mm/cover.1635936218.git.baolin.wang@linux.alibaba.com/