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

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

\
 
 \ /
  Last update: 2021-11-03 12:09    [W:0.098 / U:2.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site