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(-)
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 <[email protected]>
---
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
From the same motivation as the previous patch, this patch is suggesting
to add separate counters for hugetlb migration.
Signed-off-by: Naoya Horiguchi <[email protected]>
---
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
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
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
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
>
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