[PATCH v1 0/2] mm: migrate: vm event counter for hugepage migration

[PATCH v1 0/2] mm: migrate: vm event counter for hugepage migration

[Naoya Horiguchi]

Hi everyone,

I wrote patches introducing separate vm event counters for hugepage migration
(both for hugetlb and thp.)
Hugepage migration is different from normal page migration in event frequency
and/or how likely it succeeds, so maintaining statistics for them in mixed
counters might not be helpful both for develors and users.

Thanks,
Naoya Horiguchi
---
Summary:

Naoya Horiguchi (2):
      mm: migrate: add vm event counters thp_migrate_(success|fail)
      mm: migrate: add vm event counters hugetlb_migrate_(success|fail)

 include/linux/vm_event_item.h |   7 +++
 mm/migrate.c                  | 103 +++++++++++++++++++++++++++++++++++-------
 mm/vmstat.c                   |   8 ++++
 3 files changed, 102 insertions(+), 16 deletions(-)

[PATCH v1 1/2] mm: migrate: add vm event counters thp_migrate_(success|fail)

[Naoya Horiguchi]

Currenly we have some vm event counters for page migration, but all
migration events are counted in a single value regardless of page size.
That is not good for end users who are interested in knowing whether
hugepage migration works.  So this patch is suggesting to add separate
counters for thp migration.

Note that when thp migration fails due to ENOMEM or the lack of thp
migration support, the event is not counted in thp_migrate_fail and we
transparently retry the subpages' migrations.

Another note is that the return value of migrate_pages(), which is
claimed as "the number of pages that were not migrated (positive) or an
error code (negative)," doesn't consider the page size now.  We could do
this for example by counting a single failure of thp migration event as
512, but this patch doesn't do it for simplicity.  It seems to me that
there is no migrate_pages()'s caller which cares about the number itself
of the positive return value, so this should not be critical for now.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 include/linux/vm_event_item.h |  4 ++
 mm/migrate.c                  | 93 +++++++++++++++++++++++++++++++++++--------
 mm/vmstat.c                   |  4 ++
 3 files changed, 85 insertions(+), 16 deletions(-)

diff --git v4.16-mmotm-2018-04-10-17-02/include/linux/vm_event_item.h v4.16-mmotm-2018-04-10-17-02_patched/include/linux/vm_event_item.h
index 5c7f010..fa2d2e0 100644
--- v4.16-mmotm-2018-04-10-17-02/include/linux/vm_event_item.h
+++ v4.16-mmotm-2018-04-10-17-02_patched/include/linux/vm_event_item.h
@@ -88,6 +88,10 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
 		THP_ZERO_PAGE_ALLOC_FAILED,
 		THP_SWPOUT,
 		THP_SWPOUT_FALLBACK,
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+		THP_MIGRATE_SUCCESS,
+		THP_MIGRATE_FAIL,
+#endif
 #endif
 #ifdef CONFIG_MEMORY_BALLOON
 		BALLOON_INFLATE,
diff --git v4.16-mmotm-2018-04-10-17-02/mm/migrate.c v4.16-mmotm-2018-04-10-17-02_patched/mm/migrate.c
index bb6367d..46ff23a 100644
--- v4.16-mmotm-2018-04-10-17-02/mm/migrate.c
+++ v4.16-mmotm-2018-04-10-17-02_patched/mm/migrate.c
@@ -1348,6 +1348,69 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
 	return rc;
 }
 
+enum migrate_result_type {
+       MIGRATE_SUCCEED,
+       MIGRATE_FAIL,
+       MIGRATE_RETRY,
+       MIGRATE_RESULT_TYPES
+};
+
+enum migrate_page_type {
+       MIGRATE_PAGE_NORMAL,
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+       MIGRATE_PAGE_THP,
+#endif
+       MIGRATE_PAGE_TYPES
+};
+
+static struct migrate_event {
+       int succeeded;
+       int failed;
+} migrate_events[] = {
+       { PGMIGRATE_SUCCESS,    PGMIGRATE_FAIL },
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+       { THP_MIGRATE_SUCCESS,  THP_MIGRATE_FAIL },
+#endif
+};
+
+static inline enum migrate_page_type get_type(struct page *page)
+{
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+	if (PageTransHuge(page))
+		return MIGRATE_PAGE_THP;
+#endif
+	return MIGRATE_PAGE_NORMAL;
+}
+
+static inline int get_count(int array[][MIGRATE_PAGE_TYPES], int type)
+{
+	int i, ret;
+
+	for (i = 0, ret = 0; i < MIGRATE_PAGE_TYPES; i++)
+		ret += array[type][i];
+	return ret;
+}
+
+static inline void reset_nr_retry(int array[][MIGRATE_PAGE_TYPES])
+{
+	int i;
+
+	for (i = 0; i < MIGRATE_PAGE_TYPES; i++)
+		array[MIGRATE_RETRY][i] = 0;
+}
+
+static inline void update_vm_migrate_events(int array[][MIGRATE_PAGE_TYPES])
+{
+	int i;
+
+	for (i = 0; i < MIGRATE_PAGE_TYPES; i++) {
+		count_vm_events(migrate_events[i].succeeded,
+				array[MIGRATE_SUCCEED][i]);
+		count_vm_events(migrate_events[i].failed,
+				array[MIGRATE_FAIL][i]);
+	}
+}
+
 /*
  * migrate_pages - migrate the pages specified in a list, to the free pages
  *		   supplied as the target for the page migration
@@ -1373,9 +1436,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 		free_page_t put_new_page, unsigned long private,
 		enum migrate_mode mode, int reason)
 {
-	int retry = 1;
-	int nr_failed = 0;
-	int nr_succeeded = 0;
+	int counts[MIGRATE_RESULT_TYPES][MIGRATE_PAGE_TYPES] = {0};
 	int pass = 0;
 	struct page *page;
 	struct page *page2;
@@ -1385,13 +1446,16 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 	if (!swapwrite)
 		current->flags |= PF_SWAPWRITE;
 
-	for(pass = 0; pass < 10 && retry; pass++) {
-		retry = 0;
+	for (pass = 0; !pass || (pass < 10 && get_count(counts, MIGRATE_RETRY));
+	     pass++) {
+		reset_nr_retry(counts);
 
 		list_for_each_entry_safe(page, page2, from, lru) {
+			enum migrate_page_type mpt;
 retry:
 			cond_resched();
 
+			mpt = get_type(page);
 			if (PageHuge(page))
 				rc = unmap_and_move_huge_page(get_new_page,
 						put_new_page, private, page,
@@ -1423,13 +1487,13 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 						goto retry;
 					}
 				}
-				nr_failed++;
+				counts[MIGRATE_FAIL][mpt]++;
 				goto out;
 			case -EAGAIN:
-				retry++;
+				counts[MIGRATE_RETRY][mpt]++;
 				break;
 			case MIGRATEPAGE_SUCCESS:
-				nr_succeeded++;
+				counts[MIGRATE_SUCCEED][mpt]++;
 				break;
 			default:
 				/*
@@ -1438,19 +1502,16 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 				 * removed from migration page list and not
 				 * retried in the next outer loop.
 				 */
-				nr_failed++;
+				counts[MIGRATE_FAIL][mpt]++;
 				break;
 			}
 		}
 	}
-	nr_failed += retry;
-	rc = nr_failed;
+	rc = get_count(counts, MIGRATE_FAIL) + get_count(counts, MIGRATE_RETRY);
 out:
-	if (nr_succeeded)
-		count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
-	if (nr_failed)
-		count_vm_events(PGMIGRATE_FAIL, nr_failed);
-	trace_mm_migrate_pages(nr_succeeded, nr_failed, mode, reason);
+	update_vm_migrate_events(counts);
+	trace_mm_migrate_pages(get_count(counts, MIGRATE_SUCCEED), rc,
+			       mode, reason);
 
 	if (!swapwrite)
 		current->flags &= ~PF_SWAPWRITE;
diff --git v4.16-mmotm-2018-04-10-17-02/mm/vmstat.c v4.16-mmotm-2018-04-10-17-02_patched/mm/vmstat.c
index 536332e..57e9cc3 100644
--- v4.16-mmotm-2018-04-10-17-02/mm/vmstat.c
+++ v4.16-mmotm-2018-04-10-17-02_patched/mm/vmstat.c
@@ -1263,6 +1263,10 @@ const char * const vmstat_text[] = {
 	"thp_zero_page_alloc_failed",
 	"thp_swpout",
 	"thp_swpout_fallback",
+#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
+	"thp_migrate_success",
+	"thp_migrate_fail",
+#endif
 #endif
 #ifdef CONFIG_MEMORY_BALLOON
 	"balloon_inflate",
-- 
2.7.0

Re: [PATCH v1 1/2] mm: migrate: add vm event counters thp_migrate_(success|fail)

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5125 bytes --]

Hi Naoya,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mmotm/master]
[also build test WARNING on next-20180411]
[cannot apply to v4.16]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Naoya-Horiguchi/mm-migrate-add-vm-event-counters-thp_migrate_-success-fail/20180412-011244
base:   git://git.cmpxchg.org/linux-mmotm.git master
config: i386-randconfig-a1-201814 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   mm/migrate.c: In function 'migrate_pages':
>> mm/migrate.c:1426:2: warning: missing braces around initializer [-Wmissing-braces]
     int counts[MIGRATE_RESULT_TYPES][MIGRATE_PAGE_TYPES] = {0};
     ^
   mm/migrate.c:1426:2: warning: (near initialization for 'counts[0]') [-Wmissing-braces]

vim +1426 mm/migrate.c

  1400	
  1401	/*
  1402	 * migrate_pages - migrate the pages specified in a list, to the free pages
  1403	 *		   supplied as the target for the page migration
  1404	 *
  1405	 * @from:		The list of pages to be migrated.
  1406	 * @get_new_page:	The function used to allocate free pages to be used
  1407	 *			as the target of the page migration.
  1408	 * @put_new_page:	The function used to free target pages if migration
  1409	 *			fails, or NULL if no special handling is necessary.
  1410	 * @private:		Private data to be passed on to get_new_page()
  1411	 * @mode:		The migration mode that specifies the constraints for
  1412	 *			page migration, if any.
  1413	 * @reason:		The reason for page migration.
  1414	 *
  1415	 * The function returns after 10 attempts or if no pages are movable any more
  1416	 * because the list has become empty or no retryable pages exist any more.
  1417	 * The caller should call putback_movable_pages() to return pages to the LRU
  1418	 * or free list only if ret != 0.
  1419	 *
  1420	 * Returns the number of pages that were not migrated, or an error code.
  1421	 */
  1422	int migrate_pages(struct list_head *from, new_page_t get_new_page,
  1423			free_page_t put_new_page, unsigned long private,
  1424			enum migrate_mode mode, int reason)
  1425	{
> 1426		int counts[MIGRATE_RESULT_TYPES][MIGRATE_PAGE_TYPES] = {0};
  1427		int pass = 0;
  1428		struct page *page;
  1429		struct page *page2;
  1430		int swapwrite = current->flags & PF_SWAPWRITE;
  1431		int rc;
  1432	
  1433		if (!swapwrite)
  1434			current->flags |= PF_SWAPWRITE;
  1435	
  1436		for (pass = 0; !pass || (pass < 10 && get_count(counts, MIGRATE_RETRY));
  1437		     pass++) {
  1438			reset_nr_retry(counts);
  1439	
  1440			list_for_each_entry_safe(page, page2, from, lru) {
  1441				enum migrate_page_type mpt;
  1442	retry:
  1443				cond_resched();
  1444	
  1445				mpt = get_type(page);
  1446				if (PageHuge(page))
  1447					rc = unmap_and_move_huge_page(get_new_page,
  1448							put_new_page, private, page,
  1449							pass > 2, mode, reason);
  1450				else
  1451					rc = unmap_and_move(get_new_page, put_new_page,
  1452							private, page, pass > 2, mode,
  1453							reason);
  1454	
  1455				switch(rc) {
  1456				case -ENOMEM:
  1457					/*
  1458					 * THP migration might be unsupported or the
  1459					 * allocation could've failed so we should
  1460					 * retry on the same page with the THP split
  1461					 * to base pages.
  1462					 *
  1463					 * Head page is retried immediately and tail
  1464					 * pages are added to the tail of the list so
  1465					 * we encounter them after the rest of the list
  1466					 * is processed.
  1467					 */
  1468					if (PageTransHuge(page)) {
  1469						lock_page(page);
  1470						rc = split_huge_page_to_list(page, from);
  1471						unlock_page(page);
  1472						if (!rc) {
  1473							list_safe_reset_next(page, page2, lru);
  1474							goto retry;
  1475						}
  1476					}
  1477					counts[MIGRATE_FAIL][mpt]++;
  1478					goto out;
  1479				case -EAGAIN:
  1480					counts[MIGRATE_RETRY][mpt]++;
  1481					break;
  1482				case MIGRATEPAGE_SUCCESS:
  1483					counts[MIGRATE_SUCCEED][mpt]++;
  1484					break;
  1485				default:
  1486					/*
  1487					 * Permanent failure (-EBUSY, -ENOSYS, etc.):
  1488					 * unlike -EAGAIN case, the failed page is
  1489					 * removed from migration page list and not
  1490					 * retried in the next outer loop.
  1491					 */
  1492					counts[MIGRATE_FAIL][mpt]++;
  1493					break;
  1494				}
  1495			}
  1496		}
  1497		rc = get_count(counts, MIGRATE_FAIL) + get_count(counts, MIGRATE_RETRY);
  1498	out:
  1499		update_vm_migrate_events(counts);
  1500		trace_mm_migrate_pages(get_count(counts, MIGRATE_SUCCEED), rc,
  1501				       mode, reason);
  1502	
  1503		if (!swapwrite)
  1504			current->flags &= ~PF_SWAPWRITE;
  1505	
  1506		return rc;
  1507	}
  1508	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31930 bytes --]

[PATCH v1 2/2] mm: migrate: add vm event counters hugetlb_migrate_(success|fail)

[Naoya Horiguchi]

>From the same motivation as the previous patch, this patch is suggesting
to add separate counters for hugetlb migration.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 include/linux/vm_event_item.h |  3 +++
 mm/migrate.c                  | 10 ++++++++++
 mm/vmstat.c                   |  4 ++++
 3 files changed, 17 insertions(+)

diff --git v4.16-mmotm-2018-04-10-17-02/include/linux/vm_event_item.h v4.16-mmotm-2018-04-10-17-02_patched/include/linux/vm_event_item.h
index fa2d2e0..24966ae 100644
--- v4.16-mmotm-2018-04-10-17-02/include/linux/vm_event_item.h
+++ v4.16-mmotm-2018-04-10-17-02_patched/include/linux/vm_event_item.h
@@ -62,6 +62,9 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
 #endif
 #ifdef CONFIG_HUGETLB_PAGE
 		HTLB_BUDDY_PGALLOC, HTLB_BUDDY_PGALLOC_FAIL,
+#ifdef CONFIG_MIGRATION
+		HTLB_MIGRATE_SUCCESS, HTLB_MIGRATE_FAIL,
+#endif
 #endif
 		UNEVICTABLE_PGCULLED,	/* culled to noreclaim list */
 		UNEVICTABLE_PGSCANNED,	/* scanned for reclaimability */
diff --git v4.16-mmotm-2018-04-10-17-02/mm/migrate.c v4.16-mmotm-2018-04-10-17-02_patched/mm/migrate.c
index 46ff23a..279b143 100644
--- v4.16-mmotm-2018-04-10-17-02/mm/migrate.c
+++ v4.16-mmotm-2018-04-10-17-02_patched/mm/migrate.c
@@ -1357,6 +1357,9 @@ enum migrate_result_type {
 
 enum migrate_page_type {
        MIGRATE_PAGE_NORMAL,
+#ifdef CONFIG_HUGETLB_PAGE
+       MIGRATE_PAGE_HUGETLB,
+#endif
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
        MIGRATE_PAGE_THP,
 #endif
@@ -1368,6 +1371,9 @@ static struct migrate_event {
        int failed;
 } migrate_events[] = {
        { PGMIGRATE_SUCCESS,    PGMIGRATE_FAIL },
+#ifdef CONFIG_HUGETLB_PAGE
+       { HTLB_MIGRATE_SUCCESS, HTLB_MIGRATE_FAIL },
+#endif
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
        { THP_MIGRATE_SUCCESS,  THP_MIGRATE_FAIL },
 #endif
@@ -1375,6 +1381,10 @@ static struct migrate_event {
 
 static inline enum migrate_page_type get_type(struct page *page)
 {
+#ifdef CONFIG_HUGETLB_PAGE
+	if (PageHuge(page))
+		return MIGRATE_PAGE_HUGETLB;
+#endif
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 	if (PageTransHuge(page))
 		return MIGRATE_PAGE_THP;
diff --git v4.16-mmotm-2018-04-10-17-02/mm/vmstat.c v4.16-mmotm-2018-04-10-17-02_patched/mm/vmstat.c
index 57e9cc3..27a005f 100644
--- v4.16-mmotm-2018-04-10-17-02/mm/vmstat.c
+++ v4.16-mmotm-2018-04-10-17-02_patched/mm/vmstat.c
@@ -1236,6 +1236,10 @@ const char * const vmstat_text[] = {
 #ifdef CONFIG_HUGETLB_PAGE
 	"htlb_buddy_alloc_success",
 	"htlb_buddy_alloc_fail",
+#ifdef CONFIG_MIGRATION
+	"htlb_migrate_success",
+	"htlb_migrate_fail",
+#endif
 #endif
 	"unevictable_pgs_culled",
 	"unevictable_pgs_scanned",
-- 
2.7.0

Re: [PATCH v1 0/2] mm: migrate: vm event counter for hugepage migration

[Michal Hocko]

On Wed 11-04-18 17:09:25, Naoya Horiguchi wrote:
> Hi everyone,
> 
> I wrote patches introducing separate vm event counters for hugepage migration
> (both for hugetlb and thp.)
> Hugepage migration is different from normal page migration in event frequency
> and/or how likely it succeeds, so maintaining statistics for them in mixed
> counters might not be helpful both for develors and users.

This is quite a lot of code to be added se we should better document
what it is intended for. Sure I understand your reasonaning about huge
pages are more likely to fail but is this really worth a separate
counter? Do you have an example of how this would be useful?

If we are there then what about different huge page sizes (for hugetlb)?
Do we need per-hstate stats?

In other words, is this really worth it?

>  include/linux/vm_event_item.h |   7 +++
>  mm/migrate.c                  | 103 +++++++++++++++++++++++++++++++++++-------
>  mm/vmstat.c                   |   8 ++++
>  3 files changed, 102 insertions(+), 16 deletions(-)

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1 0/2] mm: migrate: vm event counter for hugepage migration

[Naoya Horiguchi]

On Thu, Apr 12, 2018 at 08:18:59AM +0200, Michal Hocko wrote:
> On Wed 11-04-18 17:09:25, Naoya Horiguchi wrote:
> > Hi everyone,
> > 
> > I wrote patches introducing separate vm event counters for hugepage migration
> > (both for hugetlb and thp.)
> > Hugepage migration is different from normal page migration in event frequency
> > and/or how likely it succeeds, so maintaining statistics for them in mixed
> > counters might not be helpful both for develors and users.
> 
> This is quite a lot of code to be added se we should better document
> what it is intended for. Sure I understand your reasonaning about huge
> pages are more likely to fail but is this really worth a separate
> counter? Do you have an example of how this would be useful?

Our customers periodically collect some log info to understand what
happened after system failures happen.  Then if we have separate counters
for hugepage migration and the values show some anomaly, that might
help admins and developers understand the issue more quickly.
We have other ways to get this info like checking /proc/pid/pagemap and
/proc/kpageflags, but they are costly and most users decide not to
collect them in periodical logging.

> 
> If we are there then what about different huge page sizes (for hugetlb)?
> Do we need per-hstate stats?

Yes, per-hstate counters are better. And existing hugetlb counters
htlb_buddy_alloc_* are also affected by this point.

> 
> In other words, is this really worth it?

Actually, I'm not sure at this point.

Thanks,
Naoya Horiguchi

> 
> >  include/linux/vm_event_item.h |   7 +++
> >  mm/migrate.c                  | 103 +++++++++++++++++++++++++++++++++++-------
> >  mm/vmstat.c                   |   8 ++++
> >  3 files changed, 102 insertions(+), 16 deletions(-)
> 
> -- 
> Michal Hocko
> SUSE Labs
> 

Re: [PATCH v1 0/2] mm: migrate: vm event counter for hugepage migration

[Michal Hocko]

On Thu 12-04-18 07:40:41, Naoya Horiguchi wrote:
> On Thu, Apr 12, 2018 at 08:18:59AM +0200, Michal Hocko wrote:
> > On Wed 11-04-18 17:09:25, Naoya Horiguchi wrote:
> > > Hi everyone,
> > > 
> > > I wrote patches introducing separate vm event counters for hugepage migration
> > > (both for hugetlb and thp.)
> > > Hugepage migration is different from normal page migration in event frequency
> > > and/or how likely it succeeds, so maintaining statistics for them in mixed
> > > counters might not be helpful both for develors and users.
> > 
> > This is quite a lot of code to be added se we should better document
> > what it is intended for. Sure I understand your reasonaning about huge
> > pages are more likely to fail but is this really worth a separate
> > counter? Do you have an example of how this would be useful?
> 
> Our customers periodically collect some log info to understand what
> happened after system failures happen.  Then if we have separate counters
> for hugepage migration and the values show some anomaly, that might
> help admins and developers understand the issue more quickly.
> We have other ways to get this info like checking /proc/pid/pagemap and
> /proc/kpageflags, but they are costly and most users decide not to
> collect them in periodical logging.

Wouldn't tracepoints be more suitable for that purpose? They can collect
more valuable information.

> > If we are there then what about different huge page sizes (for hugetlb)?
> > Do we need per-hstate stats?
> 
> Yes, per-hstate counters are better. And existing hugetlb counters
> htlb_buddy_alloc_* are also affected by this point.

The thing is that this would bloat the code and the vmstat output even more.
I am not really convinced this is a great idea for something that
tracepoints would handle as well.
-- 
Michal Hocko
SUSE Labs