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.
Thanks.
>
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
>>> ---
>>> mm/migrate.c | 10 ++++------
>>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index a11e948..2b45a29 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -1475,7 +1475,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>> * during migration.
>>> */
>>> is_thp = PageTransHuge(page) && !PageHuge(page);
>>> - nr_subpages = thp_nr_pages(page);
>>> + nr_subpages = compound_nr(page);
>>> cond_resched();
>>>
>>> if (PageHuge(page))
>>> @@ -1540,7 +1540,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>> nr_failed += nr_subpages;
>>> goto out;
>>> }
>>> - nr_failed++;
>>> + nr_failed += nr_subpages;
>>> goto out;
>>> case -EAGAIN:
>>> if (is_thp) {
>>> @@ -1550,14 +1550,14 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>> retry++;
>>> break;
>>> case MIGRATEPAGE_SUCCESS:
>>> + nr_succeeded += nr_subpages;
>>> if (is_thp) {
>>> nr_thp_succeeded++;
>>> - nr_succeeded += nr_subpages;
>>> break;
>>> }
>>> - nr_succeeded++;
>>> break;
>>> default:
>>> + nr_failed += nr_subpages;
>>> /*
>>> * Permanent failure (-EBUSY, etc.):
>>> * unlike -EAGAIN case, the failed page is
>>> @@ -1566,10 +1566,8 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>> */
>>> if (is_thp) {
>>> nr_thp_failed++;
>>> - nr_failed += nr_subpages;
>>> break;
>>> }
>>> - nr_failed++;
>>> break;
>>> }
>>> }
>>> --
>>> 1.8.3.1
>>
>> --
>> Best Regards,
>> Yan, Zi
>>
--
Best Regards,
Yan, Zi
Attachment:
signature.asc
Description: OpenPGP digital signature
