Re: [PATCH 2/3] mm: migrate: Correct the hugetlb migration stats

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2021/11/16 12:21, Andrew Morton wrote:
> On Sun,  7 Nov 2021 16:57:26 +0800 Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
> 
>> Correct the migration stats for hugetlb with using compound_nr() instead
>> of thp_nr_pages(),
> 
> It would be helpful to explain why using thp_nr_pages() was wrong.

Sure. Using thp_nr_pages() to get the number of subpages for a hugetlb 
is incorrect, since the number of subpages in te hugetlb is not always 
HPAGE_PMD_NR.

> And to explain the end user visible effects of this bug so we can

Actually not also user visible effect, but also hugetlb migration stats 
in kernel are incorrect. For he end user visible effects, like I 
described in patch 1,  the syscall move_pages() can return a 
non-migrated number larger than the number of pages the users tried to 
migrate, when a THP page is failed to migrate. This is confusing for users.

> decide whether -stable backporting is desirable.
> 
>> meanwhile change 'nr_failed_pages' to record the
>> number of normal pages failed to migrate, including THP and hugetlb,
>> and 'nr_succeeded' will record the number of normal pages migrated
>> successfully.
>>
>> Reviewed-by: Zi Yan <ziy@nvidia.com>
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> ---
>>   mm/migrate.c | 17 ++++++++---------
>>   1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 9aafdab..756190b 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1436,9 +1436,9 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>>    * It is caller's responsibility to call putback_movable_pages() to return pages
>>    * to the LRU or free list only if ret != 0.
>>    *
>> - * Returns the number of {normal page, THP} that were not migrated, or an error code.
>> - * The number of THP splits will be considered as the number of non-migrated THP,
>> - * no matter how many subpages of the THP are migrated successfully.
>> + * Returns the number of {normal page, THP, hugetlb} that were not migrated, or
> 
> This is a bit confusing.
> 
> If migrate_pages() failed to migrate one 4k pages then it returns 1,
> yes?

Yes.

> 
> But if migrate_pages() fails to migrate one 2MB hugepage then will it
> return 1 or will it return 512?

It will return 1 too.

> 
> And how can the caller actually _use_ this return value without knowing
> the above information?

IMHO other migration scenarios do not care about the actual non-migrated 
number of pages except the memory compaction migration, and I've fixed 
the memory compaction migration in patch 3.

So for the syscall move_pages(), we can change the return value to 
return the number of {normal page, THP, hugetlb} instead, and the number 
of THP splits will be considered as the number of non-migrated THP, no 
matter how many subpages of the THP are migrated successfully.

> In other words, perhaps this function should simply return pass/fail?

I think we've returned the successful migration pages through the 
parameter '*ret_succeeded' of the migrate_pages().