Message-ID: <5060297c-0b93-e132-f464-2c1b8d8888e3@linux.alibaba.com>
Date:   Wed, 3 Nov 2021 19:09:45 +0800
From:   Baolin Wang <baolin.wang@...ux.alibaba.com>
To:     Zi Yan <ziy@...dia.com>
Cc:     akpm@...ux-foundation.org, shy828301@...il.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: migrate: Correct the hugetlb migration stats


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

Powered by blists - more mailing lists