Hi,
According to talk with Zi Yan [1], this patch set changes the return
value of migrate_pages() to avoid returning a number which is larger
than the number of pages the users tried to migrate by move_pages() syscall.
Also fix the hugetlb migration stats and migration stats in
trace_mm_compaction_migratepages(). Please help to review. Thanks.
[1] https://lore.kernel.org/linux-mm/[email protected]/
Changes from RFC:
- Increase nr_thp_failed when THP is split, no matter how many subpages are
migrated successfully.
- Add reviewed-by tags.
Baolin Wang (3):
mm: migrate: Fix the return value of migrate_pages()
mm: migrate: Correct the hugetlb migration stats
mm: compaction: Fix the migration stats in
trace_mm_compaction_migratepages()
include/trace/events/compaction.h | 24 +++-----------
mm/compaction.c | 7 ++--
mm/migrate.c | 68 ++++++++++++++++++++++++++++-----------
3 files changed, 57 insertions(+), 42 deletions(-)
--
1.8.3.1
As Zi Yan pointed out, 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.
Since other migration scenarios do not care about the actual non-migrated
number of pages except the memory compaction migration which will fix in
following patch. Thus we can change the return value to return the number
of {normal page, THP, hugetlb} instead to avoid this issue, 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. Meanwhile we should
still keep the migration counters using the number of normal pages.
Co-developed-by: Zi Yan <[email protected]>
Signed-off-by: Zi Yan <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
mm/migrate.c | 63 +++++++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 47 insertions(+), 16 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index a11e948..9aafdab 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1428,7 +1428,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
* @mode: The migration mode that specifies the constraints for
* page migration, if any.
* @reason: The reason for page migration.
- * @ret_succeeded: Set to the number of pages migrated successfully if
+ * @ret_succeeded: Set to the number of normal pages migrated successfully if
* the caller passes a non-NULL pointer.
*
* The function returns after 10 attempts or if no pages are movable any more
@@ -1436,7 +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 pages that were not migrated, or an error code.
+ * 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.
*/
int migrate_pages(struct list_head *from, new_page_t get_new_page,
free_page_t put_new_page, unsigned long private,
@@ -1445,6 +1447,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
int retry = 1;
int thp_retry = 1;
int nr_failed = 0;
+ int nr_failed_pages = 0;
int nr_succeeded = 0;
int nr_thp_succeeded = 0;
int nr_thp_failed = 0;
@@ -1456,13 +1459,16 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
int swapwrite = current->flags & PF_SWAPWRITE;
int rc, nr_subpages;
LIST_HEAD(ret_pages);
+ LIST_HEAD(thp_split_pages);
bool nosplit = (reason == MR_NUMA_MISPLACED);
+ bool no_subpage_counting = false;
trace_mm_migrate_pages_start(mode, reason);
if (!swapwrite)
current->flags |= PF_SWAPWRITE;
+thp_subpage_migration:
for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
retry = 0;
thp_retry = 0;
@@ -1511,18 +1517,20 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
case -ENOSYS:
/* THP migration is unsupported */
if (is_thp) {
- if (!try_split_thp(page, &page2, from)) {
+ nr_thp_failed++;
+ if (!try_split_thp(page, &page2, &thp_split_pages)) {
nr_thp_split++;
goto retry;
}
- nr_thp_failed++;
- nr_failed += nr_subpages;
+ nr_failed_pages += nr_subpages;
break;
}
/* Hugetlb migration is unsupported */
- nr_failed++;
+ if (!no_subpage_counting)
+ nr_failed++;
+ nr_failed_pages++;
break;
case -ENOMEM:
/*
@@ -1531,16 +1539,19 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
* THP NUMA faulting doesn't split THP to retry.
*/
if (is_thp && !nosplit) {
- if (!try_split_thp(page, &page2, from)) {
+ nr_thp_failed++;
+ if (!try_split_thp(page, &page2, &thp_split_pages)) {
nr_thp_split++;
goto retry;
}
- nr_thp_failed++;
- nr_failed += nr_subpages;
+ nr_failed_pages += nr_subpages;
goto out;
}
- nr_failed++;
+
+ if (!no_subpage_counting)
+ nr_failed++;
+ nr_failed_pages++;
goto out;
case -EAGAIN:
if (is_thp) {
@@ -1566,17 +1577,37 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
*/
if (is_thp) {
nr_thp_failed++;
- nr_failed += nr_subpages;
+ nr_failed_pages += nr_subpages;
break;
}
- nr_failed++;
+
+ if (!no_subpage_counting)
+ nr_failed++;
+ nr_failed_pages++;
break;
}
}
}
- nr_failed += retry + thp_retry;
+ nr_failed += retry;
nr_thp_failed += thp_retry;
- rc = nr_failed;
+ /*
+ * Try to migrate subpages of fail-to-migrate THPs, no nr_failed
+ * counting in this round, since all subpages of a THP is counted
+ * as 1 failure in the first round.
+ */
+ if (!list_empty(&thp_split_pages)) {
+ /*
+ * Move non-migrated pages (after 10 retries) to ret_pages
+ * to avoid migrating them again.
+ */
+ list_splice_init(from, &ret_pages);
+ list_splice_init(&thp_split_pages, from);
+ no_subpage_counting = true;
+ retry = 1;
+ goto thp_subpage_migration;
+ }
+
+ rc = nr_failed + nr_thp_failed;
out:
/*
* Put the permanent failure page back to migration list, they
@@ -1585,11 +1616,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
list_splice(&ret_pages, from);
count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
- count_vm_events(PGMIGRATE_FAIL, nr_failed);
+ count_vm_events(PGMIGRATE_FAIL, nr_failed_pages);
count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
- trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
+ trace_mm_migrate_pages(nr_succeeded, nr_failed_pages, nr_thp_succeeded,
nr_thp_failed, nr_thp_split, mode, reason);
if (!swapwrite)
--
1.8.3.1
Now the migrate_pages() has changed to return the number of {normal page,
THP, hugetlb} instead, thus we should not use the return value to calculate
the number of pages migrated successfully. Instead we can just use the
'nr_succeeded' which indicates the number of normal pages migrated successfully
to calculate the non-migrated pages in trace_mm_compaction_migratepages().
Reviewed-by: Steven Rostedt (VMware) <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
include/trace/events/compaction.h | 24 ++++--------------------
mm/compaction.c | 7 ++++---
2 files changed, 8 insertions(+), 23 deletions(-)
diff --git a/include/trace/events/compaction.h b/include/trace/events/compaction.h
index 54e5bf0..7d48e70 100644
--- a/include/trace/events/compaction.h
+++ b/include/trace/events/compaction.h
@@ -68,10 +68,9 @@
TRACE_EVENT(mm_compaction_migratepages,
TP_PROTO(unsigned long nr_all,
- int migrate_rc,
- struct list_head *migratepages),
+ unsigned int nr_succeeded),
- TP_ARGS(nr_all, migrate_rc, migratepages),
+ TP_ARGS(nr_all, nr_succeeded),
TP_STRUCT__entry(
__field(unsigned long, nr_migrated)
@@ -79,23 +78,8 @@
),
TP_fast_assign(
- unsigned long nr_failed = 0;
- struct list_head *page_lru;
-
- /*
- * migrate_pages() returns either a non-negative number
- * with the number of pages that failed migration, or an
- * error code, in which case we need to count the remaining
- * pages manually
- */
- if (migrate_rc >= 0)
- nr_failed = migrate_rc;
- else
- list_for_each(page_lru, migratepages)
- nr_failed++;
-
- __entry->nr_migrated = nr_all - nr_failed;
- __entry->nr_failed = nr_failed;
+ __entry->nr_migrated = nr_succeeded;
+ __entry->nr_failed = nr_all - nr_succeeded;
),
TP_printk("nr_migrated=%lu nr_failed=%lu",
diff --git a/mm/compaction.c b/mm/compaction.c
index 6e44609..b4e94cd 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2280,6 +2280,7 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
unsigned long last_migrated_pfn;
const bool sync = cc->mode != MIGRATE_ASYNC;
bool update_cached;
+ unsigned int nr_succeeded = 0;
/*
* These counters track activities during zone compaction. Initialize
@@ -2398,10 +2399,10 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
err = migrate_pages(&cc->migratepages, compaction_alloc,
compaction_free, (unsigned long)cc, cc->mode,
- MR_COMPACTION, NULL);
+ MR_COMPACTION, &nr_succeeded);
- trace_mm_compaction_migratepages(cc->nr_migratepages, err,
- &cc->migratepages);
+ trace_mm_compaction_migratepages(cc->nr_migratepages,
+ nr_succeeded);
/* All pages were either migrated or will be released */
cc->nr_migratepages = 0;
--
1.8.3.1
Correct the migration stats for hugetlb with using compound_nr() instead
of thp_nr_pages(), 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 <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
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
+ * 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.
*/
int migrate_pages(struct list_head *from, new_page_t get_new_page,
free_page_t put_new_page, unsigned long private,
@@ -1481,7 +1481,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))
@@ -1530,7 +1530,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
/* Hugetlb migration is unsupported */
if (!no_subpage_counting)
nr_failed++;
- nr_failed_pages++;
+ nr_failed_pages += nr_subpages;
break;
case -ENOMEM:
/*
@@ -1551,7 +1551,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
if (!no_subpage_counting)
nr_failed++;
- nr_failed_pages++;
+ nr_failed_pages += nr_subpages;
goto out;
case -EAGAIN:
if (is_thp) {
@@ -1561,12 +1561,11 @@ 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:
/*
@@ -1583,7 +1582,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
if (!no_subpage_counting)
nr_failed++;
- nr_failed_pages++;
+ nr_failed_pages += nr_subpages;
break;
}
}
--
1.8.3.1
On 7 Nov 2021, at 3:57, Baolin Wang wrote:
> As Zi Yan pointed out, 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.
>
> Since other migration scenarios do not care about the actual non-migrated
> number of pages except the memory compaction migration which will fix in
> following patch. Thus we can change the return value to return the number
> of {normal page, THP, hugetlb} instead to avoid this issue, 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. Meanwhile we should
> still keep the migration counters using the number of normal pages.
>
> Co-developed-by: Zi Yan <[email protected]>
> Signed-off-by: Zi Yan <[email protected]>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
> mm/migrate.c | 63 +++++++++++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 47 insertions(+), 16 deletions(-)
LGTM. Thanks. Reviewed-by: Zi Yan <[email protected]>
--
Best Regards,
Yan, Zi
On Sun, 7 Nov 2021 16:57:26 +0800 Baolin Wang <[email protected]> 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.
And to explain the end user visible effects of this bug so we can
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 <[email protected]>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
> 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?
But if migrate_pages() fails to migrate one 2MB hugepage then will it
return 1 or will it return 512?
And how can the caller actually _use_ this return value without knowing
the above information?
In other words, perhaps this function should simply return pass/fail?
On 2021/11/16 12:21, Andrew Morton wrote:
> On Sun, 7 Nov 2021 16:57:26 +0800 Baolin Wang <[email protected]> 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 <[email protected]>
>> Signed-off-by: Baolin Wang <[email protected]>
>> ---
>> 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().
On 11/7/21 01:57, Baolin Wang wrote:
> As Zi Yan pointed out, 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.
>
> Since other migration scenarios do not care about the actual non-migrated
> number of pages except the memory compaction migration which will fix in
> following patch. Thus we can change the return value to return the number
> of {normal page, THP, hugetlb} instead to avoid this issue, 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. Meanwhile we should
> still keep the migration counters using the number of normal pages.
>
> Co-developed-by: Zi Yan <[email protected]>
> Signed-off-by: Zi Yan <[email protected]>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
> mm/migrate.c | 63 +++++++++++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 47 insertions(+), 16 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index a11e948..9aafdab 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1428,7 +1428,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
> * @mode: The migration mode that specifies the constraints for
> * page migration, if any.
> * @reason: The reason for page migration.
> - * @ret_succeeded: Set to the number of pages migrated successfully if
> + * @ret_succeeded: Set to the number of normal pages migrated successfully if
> * the caller passes a non-NULL pointer.
> *
> * The function returns after 10 attempts or if no pages are movable any more
> @@ -1436,7 +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 pages that were not migrated, or an error code.
> + * 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.
> */
> int migrate_pages(struct list_head *from, new_page_t get_new_page,
> free_page_t put_new_page, unsigned long private,
> @@ -1445,6 +1447,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> int retry = 1;
> int thp_retry = 1;
> int nr_failed = 0;
> + int nr_failed_pages = 0;
> int nr_succeeded = 0;
> int nr_thp_succeeded = 0;
> int nr_thp_failed = 0;
> @@ -1456,13 +1459,16 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> int swapwrite = current->flags & PF_SWAPWRITE;
> int rc, nr_subpages;
> LIST_HEAD(ret_pages);
> + LIST_HEAD(thp_split_pages);
> bool nosplit = (reason == MR_NUMA_MISPLACED);
> + bool no_subpage_counting = false;
>
> trace_mm_migrate_pages_start(mode, reason);
>
> if (!swapwrite)
> current->flags |= PF_SWAPWRITE;
>
> +thp_subpage_migration:
> for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
> retry = 0;
> thp_retry = 0;
> @@ -1511,18 +1517,20 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> case -ENOSYS:
> /* THP migration is unsupported */
> if (is_thp) {
> - if (!try_split_thp(page, &page2, from)) {
> + nr_thp_failed++;
> + if (!try_split_thp(page, &page2, &thp_split_pages)) {
Does thp_split_pages need to be initialized before this call?
--
Mike Kravetz
On 11/15/21 22:03, Baolin Wang wrote:
>
>
> On 2021/11/16 12:21, Andrew Morton wrote:
>> On Sun, 7 Nov 2021 16:57:26 +0800 Baolin Wang <[email protected]> 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.
>
Correct. However, prior to this patch the return value from thp_nr_pages
was never used for hugetlb pages; only THP. So, this really did not have any
bad side effects prior to this patch that I can see.
>> 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.
>
It looks like hugetlb pages were never taken into account when originally
defining the migration stats. In the documentation (page_migration.rst) it
only talks about Normal and THP pages. It does not mention how hugetlb pages
are counted.
Currently, hugetlb pages count as 'a single page' in the stats
PGMIGRATE_SUCCESS/FAIL. Correct? After this change we will increment these
stats by the number of sub-pages. Correct?
I 'think' this is OK since the behavior is not really defined today. But, we
are changing user visible output.
Perhaps we should go ahead and document the hugetlb behavior when making these
changes?
--
Mike Kravetz
On 2021/11/24 2:46, Mike Kravetz wrote:
> On 11/7/21 01:57, Baolin Wang wrote:
>> As Zi Yan pointed out, 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.
>>
>> Since other migration scenarios do not care about the actual non-migrated
>> number of pages except the memory compaction migration which will fix in
>> following patch. Thus we can change the return value to return the number
>> of {normal page, THP, hugetlb} instead to avoid this issue, 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. Meanwhile we should
>> still keep the migration counters using the number of normal pages.
>>
>> Co-developed-by: Zi Yan <[email protected]>
>> Signed-off-by: Zi Yan <[email protected]>
>> Signed-off-by: Baolin Wang <[email protected]>
>> ---
>> mm/migrate.c | 63 +++++++++++++++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 47 insertions(+), 16 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index a11e948..9aafdab 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1428,7 +1428,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>> * @mode: The migration mode that specifies the constraints for
>> * page migration, if any.
>> * @reason: The reason for page migration.
>> - * @ret_succeeded: Set to the number of pages migrated successfully if
>> + * @ret_succeeded: Set to the number of normal pages migrated successfully if
>> * the caller passes a non-NULL pointer.
>> *
>> * The function returns after 10 attempts or if no pages are movable any more
>> @@ -1436,7 +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 pages that were not migrated, or an error code.
>> + * 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.
>> */
>> int migrate_pages(struct list_head *from, new_page_t get_new_page,
>> free_page_t put_new_page, unsigned long private,
>> @@ -1445,6 +1447,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>> int retry = 1;
>> int thp_retry = 1;
>> int nr_failed = 0;
>> + int nr_failed_pages = 0;
>> int nr_succeeded = 0;
>> int nr_thp_succeeded = 0;
>> int nr_thp_failed = 0;
>> @@ -1456,13 +1459,16 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>> int swapwrite = current->flags & PF_SWAPWRITE;
>> int rc, nr_subpages;
>> LIST_HEAD(ret_pages);
>> + LIST_HEAD(thp_split_pages);
>> bool nosplit = (reason == MR_NUMA_MISPLACED);
>> + bool no_subpage_counting = false;
>>
>> trace_mm_migrate_pages_start(mode, reason);
>>
>> if (!swapwrite)
>> current->flags |= PF_SWAPWRITE;
>>
>> +thp_subpage_migration:
>> for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
>> retry = 0;
>> thp_retry = 0;
>> @@ -1511,18 +1517,20 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>> case -ENOSYS:
>> /* THP migration is unsupported */
>> if (is_thp) {
>> - if (!try_split_thp(page, &page2, from)) {
>> + nr_thp_failed++;
>> + if (!try_split_thp(page, &page2, &thp_split_pages)) {
>
> Does thp_split_pages need to be initialized before this call?
The declaration "LIST_HEAD(thp_split_pages);" already did list
initialization, right?
On 2021/11/24 3:25, Mike Kravetz wrote:
> On 11/15/21 22:03, Baolin Wang wrote:
>>
>>
>> On 2021/11/16 12:21, Andrew Morton wrote:
>>> On Sun, 7 Nov 2021 16:57:26 +0800 Baolin Wang <[email protected]> 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.
>>
>
> Correct. However, prior to this patch the return value from thp_nr_pages
> was never used for hugetlb pages; only THP. So, this really did not have any
> bad side effects prior to this patch that I can see. >
>>> 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.
>>
>
> It looks like hugetlb pages were never taken into account when originally
> defining the migration stats. In the documentation (page_migration.rst) it
> only talks about Normal and THP pages. It does not mention how hugetlb pages
> are counted.
>
> Currently, hugetlb pages count as 'a single page' in the stats
> PGMIGRATE_SUCCESS/FAIL. Correct? After this change we will increment these
> stats by the number of sub-pages. Correct?
Right.
>
> I 'think' this is OK since the behavior is not really defined today. But, we
> are changing user visible output.
Actually we did not change the user visible output for a hugetlb
migration. Since we still return the number of hugetlb failed to migrate
as before (though previous hugetlb behavior is not reasonable), not the
number of hguetlb subpages. We just correct the hugetlb migration stats
for the hugetlb in kernel, like PGMIGRATE_SUCCESS/FAIL stats.
>
> Perhaps we should go ahead and document the hugetlb behavior when making these
> changes?
Sure. How about adding below modification for hugetlb?
diff --git a/Documentation/vm/page_migration.rst
b/Documentation/vm/page_migration.rst
index 08810f5..8c5cb81 100644
--- a/Documentation/vm/page_migration.rst
+++ b/Documentation/vm/page_migration.rst
@@ -263,15 +263,15 @@ Monitoring Migration
The following events (counters) can be used to monitor page migration.
1. PGMIGRATE_SUCCESS: Normal page migration success. Each count means
that a
- page was migrated. If the page was a non-THP page, then this counter is
- increased by one. If the page was a THP, then this counter is
increased by
- the number of THP subpages. For example, migration of a single 2MB
THP that
- has 4KB-size base pages (subpages) will cause this counter to
increase by
- 512.
+ page was migrated. If the page was a non-THP and non-hugetlb page, then
+ this counter is increased by one. If the page was a THP or hugetlb, then
+ this counter is increased by the number of THP or hugetlb subpages.
+ For example, migration of a single 2MB THP that has 4KB-size base pages
+ (subpages) will cause this counter to increase by 512.
2. PGMIGRATE_FAIL: Normal page migration failure. Same counting rules
as for
PGMIGRATE_SUCCESS, above: this will be increased by the number of
subpages,
- if it was a THP.
+ if it was a THP or hugetlb.
3. THP_MIGRATION_SUCCESS: A THP was migrated without being split.
On 11/24/21 02:30, Baolin Wang wrote:
>
>
> On 2021/11/24 2:46, Mike Kravetz wrote:
>> On 11/7/21 01:57, Baolin Wang wrote:
>>> @@ -1511,18 +1517,20 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>> case -ENOSYS:
>>> /* THP migration is unsupported */
>>> if (is_thp) {
>>> - if (!try_split_thp(page, &page2, from)) {
>>> + nr_thp_failed++;
>>> + if (!try_split_thp(page, &page2, &thp_split_pages)) {
>>
>> Does thp_split_pages need to be initialized before this call?
>
> The declaration "LIST_HEAD(thp_split_pages);" already did list initialization, right?
Correct. My bad! I keep forgetting,
#define LIST_HEAD(name) \
struct list_head name = LIST_HEAD_INIT(name)
--
Mike Kravetz
On 11/24/21 02:47, Baolin Wang wrote:
>
>
> On 2021/11/24 3:25, Mike Kravetz wrote:
>> On 11/15/21 22:03, Baolin Wang wrote:
>>> On 2021/11/16 12:21, Andrew Morton wrote:
>>>> On Sun, 7 Nov 2021 16:57:26 +0800 Baolin Wang <[email protected]> wrote:
>>>>
>> I 'think' this is OK since the behavior is not really defined today. But, we
>> are changing user visible output.
>
> Actually we did not change the user visible output for a hugetlb migration. Since we still return the number of hugetlb failed to migrate as before (though previous hugetlb behavior is not reasonable), not the number of hguetlb subpages. We just correct the hugetlb migration stats for the hugetlb in kernel, like PGMIGRATE_SUCCESS/FAIL stats.
>
Yes, the values returned by move_pages() will not change.
The 'stats' in the kernel which are changing are user visible. Specifically.
the fields pgmigrate_success and pgmigrate_fail in the file /proc/vmstat.
The values reported there for migrated hugetlb pages is changing as a result
of this series.
In addition, if someone monitors the trace point at the end of migrate_pages
they will start seeing different values.
As mentioned, these values are not currently documented for hugetlb pages so
I think it is OK to change. If someone thinks otherwise, please speak up!
Making them be similar to what is reported for THP pages would be a good
thing.
>>
>> Perhaps we should go ahead and document the hugetlb behavior when making these
>> changes?
>
> Sure. How about adding below modification for hugetlb?
Yes, please do make the below changes as well.
--
Mike Kravetz
> diff --git a/Documentation/vm/page_migration.rst b/Documentation/vm/page_migration.rst
> index 08810f5..8c5cb81 100644
> --- a/Documentation/vm/page_migration.rst
> +++ b/Documentation/vm/page_migration.rst
> @@ -263,15 +263,15 @@ Monitoring Migration
> The following events (counters) can be used to monitor page migration.
>
> 1. PGMIGRATE_SUCCESS: Normal page migration success. Each count means that a
> - page was migrated. If the page was a non-THP page, then this counter is
> - increased by one. If the page was a THP, then this counter is increased by
> - the number of THP subpages. For example, migration of a single 2MB THP that
> - has 4KB-size base pages (subpages) will cause this counter to increase by
> - 512.
> + page was migrated. If the page was a non-THP and non-hugetlb page, then
> + this counter is increased by one. If the page was a THP or hugetlb, then
> + this counter is increased by the number of THP or hugetlb subpages.
> + For example, migration of a single 2MB THP that has 4KB-size base pages
> + (subpages) will cause this counter to increase by 512.
>
> 2. PGMIGRATE_FAIL: Normal page migration failure. Same counting rules as for
> PGMIGRATE_SUCCESS, above: this will be increased by the number of subpages,
> - if it was a THP.
> + if it was a THP or hugetlb.
>
> 3. THP_MIGRATION_SUCCESS: A THP was migrated without being split.
On 2021/11/25 3:05, Mike Kravetz wrote:
> On 11/24/21 02:47, Baolin Wang wrote:
>>
>>
>> On 2021/11/24 3:25, Mike Kravetz wrote:
>>> On 11/15/21 22:03, Baolin Wang wrote:
>>>> On 2021/11/16 12:21, Andrew Morton wrote:
>>>>> On Sun, 7 Nov 2021 16:57:26 +0800 Baolin Wang <[email protected]> wrote:
>>>>>
>>> I 'think' this is OK since the behavior is not really defined today. But, we
>>> are changing user visible output.
>>
>> Actually we did not change the user visible output for a hugetlb migration. Since we still return the number of hugetlb failed to migrate as before (though previous hugetlb behavior is not reasonable), not the number of hguetlb subpages. We just correct the hugetlb migration stats for the hugetlb in kernel, like PGMIGRATE_SUCCESS/FAIL stats.
>>
>
> Yes, the values returned by move_pages() will not change.
>
> The 'stats' in the kernel which are changing are user visible. Specifically.
> the fields pgmigrate_success and pgmigrate_fail in the file /proc/vmstat.
> The values reported there for migrated hugetlb pages is changing as a result
> of this series.
>
> In addition, if someone monitors the trace point at the end of migrate_pages
> they will start seeing different values.
Right, agree.
>
> As mentioned, these values are not currently documented for hugetlb pages so
> I think it is OK to change. If someone thinks otherwise, please speak up!
>
> Making them be similar to what is reported for THP pages would be a good
> thing.
OK.
>>>
>>> Perhaps we should go ahead and document the hugetlb behavior when making these
>>> changes?
>>
>> Sure. How about adding below modification for hugetlb?
>
> Yes, please do make the below changes as well.
Thanks.
Andrew, I am not sure you can help to fold below changes into your mm
branch, or you want me to resend this patch set with adding below
changes, or just send an incremental patch to add hugetlb documentation?
diff --git a/Documentation/vm/page_migration.rst
b/Documentation/vm/page_migration.rst
index 08810f5..8c5cb81 100644
--- a/Documentation/vm/page_migration.rst
+++ b/Documentation/vm/page_migration.rst
@@ -263,15 +263,15 @@ Monitoring Migration
The following events (counters) can be used to monitor page migration.
1. PGMIGRATE_SUCCESS: Normal page migration success. Each count means
that a
- page was migrated. If the page was a non-THP page, then this counter is
- increased by one. If the page was a THP, then this counter is
increased by
- the number of THP subpages. For example, migration of a single 2MB
THP that
- has 4KB-size base pages (subpages) will cause this counter to
increase by
- 512.
+ page was migrated. If the page was a non-THP and non-hugetlb page, then
+ this counter is increased by one. If the page was a THP or hugetlb, then
+ this counter is increased by the number of THP or hugetlb subpages.
+ For example, migration of a single 2MB THP that has 4KB-size base pages
+ (subpages) will cause this counter to increase by 512.
2. PGMIGRATE_FAIL: Normal page migration failure. Same counting rules
as for
PGMIGRATE_SUCCESS, above: this will be increased by the number of
subpages,
- if it was a THP.
+ if it was a THP or hugetlb.