| Message ID | 71a4b6c22f208728fe8c78ad26375436c4ff9704.1636275127.git.baolin.wang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Improve the migration stats | expand |
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. 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 <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? 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 <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().
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 <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. > 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?
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 <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. >> > > 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: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 <baolin.wang@linux.alibaba.com> 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.
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 <baolin.wang@linux.alibaba.com> 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.
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; } }