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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

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
>