lkml.org 
[lkml]   [2021]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] mm: migrate: Correct the hugetlb migration stats
From
Date


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
>

\
 
 \ /
  Last update: 2021-11-01 07:54    [W:0.122 / U:0.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site