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@xxxxxxxxxxxxxxxxx/