On 2021/11/16 12:21, Andrew Morton wrote:
On Sun, 7 Nov 2021 16:57:26 +0800 Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
---
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().
