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