Messages in this thread | | | Subject | Re: [PATCH] mm: migrate: Correct the hugetlb migration stats | From | Baolin Wang <> | Date | Wed, 3 Nov 2021 19:09:45 +0800 |
| |
> 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/
| |