[PATCH 0/5] page_owner improvements for debugging

[PATCH 0/5] page_owner improvements for debugging

[Vlastimil Babka]

Hi,

I know it's merge window, but this might potentially help us with some
outstanding bugs if page_owner was enabled e.g. for trinity runs, so here
you go.

Patch 1 is a bug fix, patch 2 reduces page_owner overhead when compiled in
but not enabled on boot. Patch 3 is something I suggested before [1] and it
was deemed a good idea, that the page_owner info should follow the page during
migration. Patch 4 allows us again to know that a migration happened and for
which reason.

Patch 5 will hopefully help us when debugging, as it makes all the info be
printed as part of e.g. VM_BUG_ON_PAGE(). Until now it was only accessible via
/sys file.

Patches are based on today's -next. Hugh's migration patches caused conflicts
for patches 3 and 4 when rebasing from 4.3.

[1] https://lkml.org/lkml/2015/7/23/47

Vlastimil Babka (5):
  mm, page_owner: print migratetype of a page, not pageblock
  mm, page_owner: convert page_owner_inited to static key
  mm, page_owner: copy page owner info during migration
  mm, page_owner: track last migrate reason
  mm, page_owner: dump page owner info from dump_page()

 Documentation/vm/page_owner.txt |  9 +++---
 include/linux/page_ext.h        |  1 +
 include/linux/page_owner.h      | 50 ++++++++++++++++++++++++---------
 mm/debug.c                      |  2 ++
 mm/migrate.c                    | 11 ++++++--
 mm/page_owner.c                 | 61 +++++++++++++++++++++++++++++++++++++----
 mm/vmstat.c                     |  2 +-
 7 files changed, 110 insertions(+), 26 deletions(-)

-- 
2.6.2

[PATCH 1/5] mm, page_owner: print migratetype of a page, not pageblock

[Vlastimil Babka]

The information in /sys/kernel/debug/page_owner includes the migratetype
declared during the page allocation via gfp_flags. This is also checked against
the pageblock's migratetype, and reported as Fallback allocation if these two
differ (although in fact fallback allocation is not the only reason why they
can differ).

However, the migratetype actually printed is the one of the pageblock, not of
the page itself, so it's the same for all pages in the pageblock. This is
apparently a bug, noticed when working on other page_owner improvements. Fixed.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/page_owner.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_owner.c b/mm/page_owner.c
index 983c3a1..a9f16b8 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -113,7 +113,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 			"PFN %lu Block %lu type %d %s Flags %s%s%s%s%s%s%s%s%s%s%s%s\n",
 			pfn,
 			pfn >> pageblock_order,
-			pageblock_mt,
+			page_mt,
 			pageblock_mt != page_mt ? "Fallback" : "        ",
 			PageLocked(page)	? "K" : " ",
 			PageError(page)		? "E" : " ",
-- 
2.6.2

[PATCH 2/5] mm, page_owner: convert page_owner_inited to static key

[Vlastimil Babka]

CONFIG_PAGE_OWNER attempts to impose negligible runtime overhead when enabled
during compilation, but not actually enabled during runtime by boot param
page_owner=on. This overhead can be further reduced using the static key
mechanism, which this patch does.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 Documentation/vm/page_owner.txt |  9 +++++----
 include/linux/page_owner.h      | 22 ++++++++++------------
 mm/page_owner.c                 |  9 +++++----
 mm/vmstat.c                     |  2 +-
 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/Documentation/vm/page_owner.txt b/Documentation/vm/page_owner.txt
index 8f3ce9b..ffff143 100644
--- a/Documentation/vm/page_owner.txt
+++ b/Documentation/vm/page_owner.txt
@@ -28,10 +28,11 @@ with page owner and page owner is disabled in runtime due to no enabling
 boot option, runtime overhead is marginal. If disabled in runtime, it
 doesn't require memory to store owner information, so there is no runtime
 memory overhead. And, page owner inserts just two unlikely branches into
-the page allocator hotpath and if it returns false then allocation is
-done like as the kernel without page owner. These two unlikely branches
-would not affect to allocation performance. Following is the kernel's
-code size change due to this facility.
+the page allocator hotpath and if not enabled, then allocation is done
+like as the kernel without page owner. These two unlikely branches should
+not affect to allocation performance, especially if the static keys jump
+label patching functionality is available. Following is the kernel's code
+size change due to this facility.
 
 - Without page owner
    text    data     bss     dec     hex filename
diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h
index cacaabe..8e2eb15 100644
--- a/include/linux/page_owner.h
+++ b/include/linux/page_owner.h
@@ -1,8 +1,10 @@
 #ifndef __LINUX_PAGE_OWNER_H
 #define __LINUX_PAGE_OWNER_H
 
+#include <linux/jump_label.h>
+
 #ifdef CONFIG_PAGE_OWNER
-extern bool page_owner_inited;
+extern struct static_key_false page_owner_inited;
 extern struct page_ext_operations page_owner_ops;
 
 extern void __reset_page_owner(struct page *page, unsigned int order);
@@ -12,27 +14,23 @@ extern gfp_t __get_page_owner_gfp(struct page *page);
 
 static inline void reset_page_owner(struct page *page, unsigned int order)
 {
-	if (likely(!page_owner_inited))
-		return;
-
-	__reset_page_owner(page, order);
+	if (static_branch_unlikely(&page_owner_inited))
+		__reset_page_owner(page, order);
 }
 
 static inline void set_page_owner(struct page *page,
 			unsigned int order, gfp_t gfp_mask)
 {
-	if (likely(!page_owner_inited))
-		return;
-
-	__set_page_owner(page, order, gfp_mask);
+	if (static_branch_unlikely(&page_owner_inited))
+		__set_page_owner(page, order, gfp_mask);
 }
 
 static inline gfp_t get_page_owner_gfp(struct page *page)
 {
-	if (likely(!page_owner_inited))
+	if (static_branch_unlikely(&page_owner_inited))
+		return __get_page_owner_gfp(page);
+	else
 		return 0;
-
-	return __get_page_owner_gfp(page);
 }
 #else
 static inline void reset_page_owner(struct page *page, unsigned int order)
diff --git a/mm/page_owner.c b/mm/page_owner.c
index a9f16b8..7664b85 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -5,10 +5,11 @@
 #include <linux/bootmem.h>
 #include <linux/stacktrace.h>
 #include <linux/page_owner.h>
+#include <linux/jump_label.h>
 #include "internal.h"
 
 static bool page_owner_disabled = true;
-bool page_owner_inited __read_mostly;
+DEFINE_STATIC_KEY_FALSE(page_owner_inited);
 
 static void init_early_allocated_pages(void);
 
@@ -37,7 +38,7 @@ static void init_page_owner(void)
 	if (page_owner_disabled)
 		return;
 
-	page_owner_inited = true;
+	static_branch_enable(&page_owner_inited);
 	init_early_allocated_pages();
 }
 
@@ -157,7 +158,7 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	struct page *page;
 	struct page_ext *page_ext;
 
-	if (!page_owner_inited)
+	if (!static_branch_unlikely(&page_owner_inited))
 		return -EINVAL;
 
 	page = NULL;
@@ -305,7 +306,7 @@ static int __init pageowner_init(void)
 {
 	struct dentry *dentry;
 
-	if (!page_owner_inited) {
+	if (!static_branch_unlikely(&page_owner_inited)) {
 		pr_info("page_owner is disabled\n");
 		return 0;
 	}
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 34f480b..d9be9ab 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1131,7 +1131,7 @@ static void pagetypeinfo_showmixedcount(struct seq_file *m, pg_data_t *pgdat)
 #ifdef CONFIG_PAGE_OWNER
 	int mtype;
 
-	if (!page_owner_inited)
+	if (!static_branch_unlikely(&page_owner_inited))
 		return;
 
 	drain_all_pages(NULL);
-- 
2.6.2

[PATCH 3/5] mm, page_owner: copy page owner info during migration

[Vlastimil Babka]

The page_owner mechanism stores gfp_flags of an allocation and stack trace
that lead to it. During page migration, the original information is
essentially replaced by the allocation of free page as the migration target.
Arguably this is less useful and might lead to all the page_owner info for
migratable pages gradually converge towards compaction or numa balancing
migrations. It has also lead to inaccuracies such as one fixed by commit
e2cfc91120fa ("mm/page_owner: set correct gfp_mask on page_owner").

This patch thus introduces copying the page_owner info during migration.
However, since the fact that the page has been migrated from its original
place might be useful for debugging, the next patch will introduce a way to
track that information as well.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/page_owner.h | 10 +++++++++-
 mm/migrate.c               |  2 ++
 mm/page_owner.c            | 16 ++++++++++++++++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h
index 8e2eb15..6440daa 100644
--- a/include/linux/page_owner.h
+++ b/include/linux/page_owner.h
@@ -11,6 +11,7 @@ extern void __reset_page_owner(struct page *page, unsigned int order);
 extern void __set_page_owner(struct page *page,
 			unsigned int order, gfp_t gfp_mask);
 extern gfp_t __get_page_owner_gfp(struct page *page);
+extern void __copy_page_owner(struct page *oldpage, struct page *newpage);
 
 static inline void reset_page_owner(struct page *page, unsigned int order)
 {
@@ -32,6 +33,11 @@ static inline gfp_t get_page_owner_gfp(struct page *page)
 	else
 		return 0;
 }
+static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
+{
+	if (static_branch_unlikely(&page_owner_inited))
+		__copy_page_owner(oldpage, newpage);
+}
 #else
 static inline void reset_page_owner(struct page *page, unsigned int order)
 {
@@ -44,6 +50,8 @@ static inline gfp_t get_page_owner_gfp(struct page *page)
 {
 	return 0;
 }
-
+static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
+{
+}
 #endif /* CONFIG_PAGE_OWNER */
 #endif /* __LINUX_PAGE_OWNER_H */
diff --git a/mm/migrate.c b/mm/migrate.c
index 1ae0113..9f82e03 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -38,6 +38,7 @@
 #include <linux/balloon_compaction.h>
 #include <linux/mmu_notifier.h>
 #include <linux/page_idle.h>
+#include <linux/page_owner.h>
 
 #include <asm/tlbflush.h>
 
@@ -775,6 +776,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
 		set_page_memcg(page, NULL);
 		if (!PageAnon(page))
 			page->mapping = NULL;
+		copy_page_owner(page, newpage);
 	}
 	return rc;
 }
diff --git a/mm/page_owner.c b/mm/page_owner.c
index 7664b85..7ebd3d0 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -84,6 +84,22 @@ gfp_t __get_page_owner_gfp(struct page *page)
 	return page_ext->gfp_mask;
 }
 
+void __copy_page_owner(struct page *oldpage, struct page *newpage)
+{
+	struct page_ext *old_ext = lookup_page_ext(oldpage);
+	struct page_ext *new_ext = lookup_page_ext(newpage);
+	int i;
+
+	new_ext->order = old_ext->order;
+	new_ext->gfp_mask = old_ext->gfp_mask;
+	new_ext->nr_entries = old_ext->nr_entries;
+
+	for (i = 0; i < ARRAY_SIZE(new_ext->trace_entries); i++)
+		new_ext->trace_entries[i] = old_ext->trace_entries[i];
+
+	__set_bit(PAGE_EXT_OWNER, &new_ext->flags);
+}
+
 static ssize_t
 print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 		struct page *page, struct page_ext *page_ext)
-- 
2.6.2

[PATCH 4/5] mm, page_owner: track last migrate reason

[Vlastimil Babka]

During migration, page_owner info is now copied with the rest of the page, so
the stacktrace leading to free page allocation during migration is overwritten.
For debugging purposes, it might be however useful to know that the page has
been migrated since its initial allocation. This might happen many times during
the lifetime for different reasons, and fully tracking this, especially with
stacktraces would incur extra memory costs. As a compromise, store the
migrate_reason of the last migration that occurred to the page. This is enough
to distinguish compaction, numa balancing etc.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/page_ext.h   |  1 +
 include/linux/page_owner.h |  9 +++++++++
 mm/migrate.c               |  9 ++++++---
 mm/page_owner.c            | 16 ++++++++++++++++
 4 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/include/linux/page_ext.h b/include/linux/page_ext.h
index 17f118a..e1fe7cf 100644
--- a/include/linux/page_ext.h
+++ b/include/linux/page_ext.h
@@ -45,6 +45,7 @@ struct page_ext {
 	unsigned int order;
 	gfp_t gfp_mask;
 	unsigned int nr_entries;
+	int last_migrate_reason;
 	unsigned long trace_entries[8];
 #endif
 };
diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h
index 6440daa..555893b 100644
--- a/include/linux/page_owner.h
+++ b/include/linux/page_owner.h
@@ -12,6 +12,7 @@ extern void __set_page_owner(struct page *page,
 			unsigned int order, gfp_t gfp_mask);
 extern gfp_t __get_page_owner_gfp(struct page *page);
 extern void __copy_page_owner(struct page *oldpage, struct page *newpage);
+extern void __set_page_owner_migrate_reason(struct page *page, int reason);
 
 static inline void reset_page_owner(struct page *page, unsigned int order)
 {
@@ -38,6 +39,11 @@ static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
 	if (static_branch_unlikely(&page_owner_inited))
 		__copy_page_owner(oldpage, newpage);
 }
+static inline void set_page_owner_migrate_reason(struct page *page, int reason)
+{
+	if (static_branch_unlikely(&page_owner_inited))
+		__set_page_owner_migrate_reason(page, reason);
+}
 #else
 static inline void reset_page_owner(struct page *page, unsigned int order)
 {
@@ -53,5 +59,8 @@ static inline gfp_t get_page_owner_gfp(struct page *page)
 static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
 {
 }
+static inline void set_page_owner_migrate_reason(struct page *page, int reason)
+{
+}
 #endif /* CONFIG_PAGE_OWNER */
 #endif /* __LINUX_PAGE_OWNER_H */
diff --git a/mm/migrate.c b/mm/migrate.c
index 9f82e03..5a4fb1f 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -954,8 +954,10 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
 	}
 
 	rc = __unmap_and_move(page, newpage, force, mode);
-	if (rc == MIGRATEPAGE_SUCCESS)
+	if (rc == MIGRATEPAGE_SUCCESS) {
 		put_new_page = NULL;
+		set_page_owner_migrate_reason(newpage, reason);
+	}
 
 out:
 	if (rc != -EAGAIN) {
@@ -1020,7 +1022,7 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
 static int unmap_and_move_huge_page(new_page_t get_new_page,
 				free_page_t put_new_page, unsigned long private,
 				struct page *hpage, int force,
-				enum migrate_mode mode)
+				enum migrate_mode mode, int reason)
 {
 	int rc = -EAGAIN;
 	int *result = NULL;
@@ -1078,6 +1080,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
 	if (rc == MIGRATEPAGE_SUCCESS) {
 		hugetlb_cgroup_migrate(hpage, new_hpage);
 		put_new_page = NULL;
+		set_page_owner_migrate_reason(new_hpage, reason);
 	}
 
 	unlock_page(hpage);
@@ -1150,7 +1153,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 			if (PageHuge(page))
 				rc = unmap_and_move_huge_page(get_new_page,
 						put_new_page, private, page,
-						pass > 2, mode);
+						pass > 2, mode, reason);
 			else
 				rc = unmap_and_move(get_new_page, put_new_page,
 						private, page, pass > 2, mode,
diff --git a/mm/page_owner.c b/mm/page_owner.c
index 7ebd3d0..388898f 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -73,10 +73,18 @@ void __set_page_owner(struct page *page, unsigned int order, gfp_t gfp_mask)
 	page_ext->order = order;
 	page_ext->gfp_mask = gfp_mask;
 	page_ext->nr_entries = trace.nr_entries;
+	page_ext->last_migrate_reason = -1;
 
 	__set_bit(PAGE_EXT_OWNER, &page_ext->flags);
 }
 
+void __set_page_owner_migrate_reason(struct page *page, int reason)
+{
+	struct page_ext *page_ext = lookup_page_ext(page);
+
+	page_ext->last_migrate_reason = reason;
+}
+
 gfp_t __get_page_owner_gfp(struct page *page)
 {
 	struct page_ext *page_ext = lookup_page_ext(page);
@@ -152,6 +160,14 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 	if (ret >= count)
 		goto err;
 
+	if (page_ext->last_migrate_reason != -1) {
+		ret += snprintf(kbuf + ret, count - ret,
+			"Page has been migrated, last migrate reason: %d\n",
+			page_ext->last_migrate_reason);
+		if (ret >= count)
+			goto err;
+	}
+
 	ret += snprintf(kbuf + ret, count - ret, "\n");
 	if (ret >= count)
 		goto err;
-- 
2.6.2

[PATCH 5/5] mm, page_owner: dump page owner info from dump_page()

[Vlastimil Babka]

The page_owner mechanism is useful for dealing with memory leaks. By reading
/sys/kernel/debug/page_owner one can determine the stack traces leading to
allocations of all pages, and find e.g. a buggy driver.

This information might be also potentially useful for debugging, such as the
VM_BUG_ON_PAGE() calls to dump_page(). So let's print the stored info from
dump_page().

Example output:

[  199.188777] page:ffffea0002900540 count:2 mapcount:0 mapping:ffff880131993e18 index:0x34e
[  199.202832] flags: 0x1fffff80020048(uptodate|active|mappedtodisk)
[  199.207048] page dumped because: VM_BUG_ON_PAGE(1)
[  199.207048] page->mem_cgroup:ffff880138efdc00
[  199.207050] page allocated via order 0, mask 0x213da, migratetype 2, trace:
[  199.207050]  [<ffffffff811622c5>] __alloc_pages_nodemask+0x175/0x900
[  199.207057]  [<ffffffff811a69c1>] alloc_pages_current+0x91/0x100
[  199.207061]  [<ffffffff81158da1>] __page_cache_alloc+0xb1/0xf0
[  199.207066]  [<ffffffff81165eeb>] __do_page_cache_readahead+0xdb/0x200
[  199.207067]  [<ffffffff81166155>] ondemand_readahead+0x145/0x270
[  199.207069]  [<ffffffff811662ec>] page_cache_async_readahead+0x6c/0x70
[  199.207070]  [<ffffffff8115a838>] generic_file_read_iter+0x378/0x590
[  199.207074]  [<ffffffff811cd2d7>] __vfs_read+0xa7/0xd0
[  199.207074] page has been migrated, last migrate reason: 0

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/page_owner.h |  9 +++++++++
 mm/debug.c                 |  2 ++
 mm/page_owner.c            | 18 ++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h
index 555893b..46f1b93 100644
--- a/include/linux/page_owner.h
+++ b/include/linux/page_owner.h
@@ -13,6 +13,7 @@ extern void __set_page_owner(struct page *page,
 extern gfp_t __get_page_owner_gfp(struct page *page);
 extern void __copy_page_owner(struct page *oldpage, struct page *newpage);
 extern void __set_page_owner_migrate_reason(struct page *page, int reason);
+extern void __dump_page_owner(struct page *page);
 
 static inline void reset_page_owner(struct page *page, unsigned int order)
 {
@@ -44,6 +45,11 @@ static inline void set_page_owner_migrate_reason(struct page *page, int reason)
 	if (static_branch_unlikely(&page_owner_inited))
 		__set_page_owner_migrate_reason(page, reason);
 }
+static inline void dump_page_owner(struct page *page)
+{
+	if (static_branch_unlikely(&page_owner_inited))
+		__dump_page_owner(page);
+}
 #else
 static inline void reset_page_owner(struct page *page, unsigned int order)
 {
@@ -62,5 +68,8 @@ static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
 static inline void set_page_owner_migrate_reason(struct page *page, int reason)
 {
 }
+static inline void dump_page_owner(struct page *page)
+{
+}
 #endif /* CONFIG_PAGE_OWNER */
 #endif /* __LINUX_PAGE_OWNER_H */
diff --git a/mm/debug.c b/mm/debug.c
index 8362765..93373d1 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -9,6 +9,7 @@
 #include <linux/mm.h>
 #include <linux/trace_events.h>
 #include <linux/memcontrol.h>
+#include <linux/page_owner.h>
 
 static const struct trace_print_flags pageflag_names[] = {
 	{1UL << PG_locked,		"locked"	},
@@ -98,6 +99,7 @@ void dump_page_badflags(struct page *page, const char *reason,
 	if (page->mem_cgroup)
 		pr_alert("page->mem_cgroup:%p\n", page->mem_cgroup);
 #endif
+	dump_page_owner(page);
 }
 
 void dump_page(struct page *page, const char *reason)
diff --git a/mm/page_owner.c b/mm/page_owner.c
index 388898f..d7e0aaf 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -183,6 +183,24 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 	return -ENOMEM;
 }
 
+void __dump_page_owner(struct page *page)
+{
+	struct page_ext *page_ext = lookup_page_ext(page);
+	struct stack_trace trace = {
+		.nr_entries = page_ext->nr_entries,
+		.entries = &page_ext->trace_entries[0],
+	};
+
+	pr_alert("page allocated via order %u, mask 0x%x, migratetype %d, trace:\n",
+			page_ext->order, page_ext->gfp_mask,
+			gfpflags_to_migratetype(page_ext->gfp_mask));
+	print_stack_trace(&trace, 0);
+
+	if (page_ext->last_migrate_reason != -1)
+		pr_alert("page has been migrated, last migrate reason: %d\n",
+			page_ext->last_migrate_reason);
+}
+
 static ssize_t
 read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 {
-- 
2.6.2

checkpatch false warning. was: [PATCH 3/5] mm, page_owner: copy page owner info during migration

[Vlastimil Babka]

On 11/04/2015 04:00 PM, Vlastimil Babka wrote:
> The page_owner mechanism stores gfp_flags of an allocation and stack trace
> that lead to it. During page migration, the original information is
> essentially replaced by the allocation of free page as the migration target.
> Arguably this is less useful and might lead to all the page_owner info for
> migratable pages gradually converge towards compaction or numa balancing
> migrations. It has also lead to inaccuracies such as one fixed by commit
> e2cfc91120fa ("mm/page_owner: set correct gfp_mask on page_owner").

Hi,

This patch gives me the following checkpatch warning:

ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit e2cfc91120fa ("mm/page_owner: set correct gfp_mask on page_owner")'
#12: 
e2cfc91120fa ("mm/page_owner: set correct gfp_mask on page_owner").

Some fuzzing showed that it's complaining about the missing word "commit"
which is however on the end of the previous line. Can that be fixed please?
I can't speak perl myself.

Thanks,
Vlastimil
 
> This patch thus introduces copying the page_owner info during migration.
> However, since the fact that the page has been migrated from its original
> place might be useful for debugging, the next patch will introduce a way to
> track that information as well.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>   include/linux/page_owner.h | 10 +++++++++-
>   mm/migrate.c               |  2 ++
>   mm/page_owner.c            | 16 ++++++++++++++++
>   3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h
> index 8e2eb15..6440daa 100644
> --- a/include/linux/page_owner.h
> +++ b/include/linux/page_owner.h
> @@ -11,6 +11,7 @@ extern void __reset_page_owner(struct page *page, unsigned int order);
>   extern void __set_page_owner(struct page *page,
>   			unsigned int order, gfp_t gfp_mask);
>   extern gfp_t __get_page_owner_gfp(struct page *page);
> +extern void __copy_page_owner(struct page *oldpage, struct page *newpage);
>   
>   static inline void reset_page_owner(struct page *page, unsigned int order)
>   {
> @@ -32,6 +33,11 @@ static inline gfp_t get_page_owner_gfp(struct page *page)
>   	else
>   		return 0;
>   }
> +static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
> +{
> +	if (static_branch_unlikely(&page_owner_inited))
> +		__copy_page_owner(oldpage, newpage);
> +}
>   #else
>   static inline void reset_page_owner(struct page *page, unsigned int order)
>   {
> @@ -44,6 +50,8 @@ static inline gfp_t get_page_owner_gfp(struct page *page)
>   {
>   	return 0;
>   }
> -
> +static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
> +{
> +}
>   #endif /* CONFIG_PAGE_OWNER */
>   #endif /* __LINUX_PAGE_OWNER_H */
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 1ae0113..9f82e03 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -38,6 +38,7 @@
>   #include <linux/balloon_compaction.h>
>   #include <linux/mmu_notifier.h>
>   #include <linux/page_idle.h>
> +#include <linux/page_owner.h>
>   
>   #include <asm/tlbflush.h>
>   
> @@ -775,6 +776,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
>   		set_page_memcg(page, NULL);
>   		if (!PageAnon(page))
>   			page->mapping = NULL;
> +		copy_page_owner(page, newpage);
>   	}
>   	return rc;
>   }
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 7664b85..7ebd3d0 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -84,6 +84,22 @@ gfp_t __get_page_owner_gfp(struct page *page)
>   	return page_ext->gfp_mask;
>   }
>   
> +void __copy_page_owner(struct page *oldpage, struct page *newpage)
> +{
> +	struct page_ext *old_ext = lookup_page_ext(oldpage);
> +	struct page_ext *new_ext = lookup_page_ext(newpage);
> +	int i;
> +
> +	new_ext->order = old_ext->order;
> +	new_ext->gfp_mask = old_ext->gfp_mask;
> +	new_ext->nr_entries = old_ext->nr_entries;
> +
> +	for (i = 0; i < ARRAY_SIZE(new_ext->trace_entries); i++)
> +		new_ext->trace_entries[i] = old_ext->trace_entries[i];
> +
> +	__set_bit(PAGE_EXT_OWNER, &new_ext->flags);
> +}
> +
>   static ssize_t
>   print_page_owner(char __user *buf, size_t count, unsigned long pfn,
>   		struct page *page, struct page_ext *page_ext)
> 

Re: checkpatch false warning. was: [PATCH 3/5] mm, page_owner: copy page owner info during migration

[Joe Perches]

On Wed, 2015-11-04 at 16:10 +0100, Vlastimil Babka wrote:
> On 11/04/2015 04:00 PM, Vlastimil Babka wrote:
> > The page_owner mechanism stores gfp_flags of an allocation and stack trace
> > that lead to it. During page migration, the original information is
> > essentially replaced by the allocation of free page as the migration target.
> > Arguably this is less useful and might lead to all the page_owner info for
> > migratable pages gradually converge towards compaction or numa balancing
> > migrations. It has also lead to inaccuracies such as one fixed by commit
> > e2cfc91120fa ("mm/page_owner: set correct gfp_mask on page_owner").
> 
> Hi,
> 
> This patch gives me the following checkpatch warning:
> 
> ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("")' - ie: 'commit e2cfc91120fa ("mm/page_owner: set correct gfp_mask on page_owner")'
> #12: 
> e2cfc91120fa ("mm/page_owner: set correct gfp_mask on page_owner").
> 
> Some fuzzing showed that it's complaining about the missing word "commit"
> which is however on the end of the previous line. Can that be fixed please?

Not trivially, no.

> I can't speak perl myself.

No one speaks perl fluently.

checkpatch is not perfect.  It never will be, it's just
a brainless script.  Please ignore messages from it that
are obviously incorrect.

Re: [PATCH 5/5] mm, page_owner: dump page owner info from dump_page()

[Kirill A. Shutemov]

On Wed, Nov 04, 2015 at 04:01:01PM +0100, Vlastimil Babka wrote:
> The page_owner mechanism is useful for dealing with memory leaks. By reading
> /sys/kernel/debug/page_owner one can determine the stack traces leading to
> allocations of all pages, and find e.g. a buggy driver.
> 
> This information might be also potentially useful for debugging, such as the
> VM_BUG_ON_PAGE() calls to dump_page(). So let's print the stored info from
> dump_page().
> 
> Example output:
> 
> [  199.188777] page:ffffea0002900540 count:2 mapcount:0 mapping:ffff880131993e18 index:0x34e
> [  199.202832] flags: 0x1fffff80020048(uptodate|active|mappedtodisk)
> [  199.207048] page dumped because: VM_BUG_ON_PAGE(1)
> [  199.207048] page->mem_cgroup:ffff880138efdc00
> [  199.207050] page allocated via order 0, mask 0x213da, migratetype 2, trace:

Can we decode gfp_mask and migratetype into something more human readable?

> [  199.207050]  [<ffffffff811622c5>] __alloc_pages_nodemask+0x175/0x900
> [  199.207057]  [<ffffffff811a69c1>] alloc_pages_current+0x91/0x100
> [  199.207061]  [<ffffffff81158da1>] __page_cache_alloc+0xb1/0xf0
> [  199.207066]  [<ffffffff81165eeb>] __do_page_cache_readahead+0xdb/0x200
> [  199.207067]  [<ffffffff81166155>] ondemand_readahead+0x145/0x270
> [  199.207069]  [<ffffffff811662ec>] page_cache_async_readahead+0x6c/0x70
> [  199.207070]  [<ffffffff8115a838>] generic_file_read_iter+0x378/0x590
> [  199.207074]  [<ffffffff811cd2d7>] __vfs_read+0xa7/0xd0
> [  199.207074] page has been migrated, last migrate reason: 0

Same here.

> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  include/linux/page_owner.h |  9 +++++++++
>  mm/debug.c                 |  2 ++
>  mm/page_owner.c            | 18 ++++++++++++++++++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h
> index 555893b..46f1b93 100644
> --- a/include/linux/page_owner.h
> +++ b/include/linux/page_owner.h
> @@ -13,6 +13,7 @@ extern void __set_page_owner(struct page *page,
>  extern gfp_t __get_page_owner_gfp(struct page *page);
>  extern void __copy_page_owner(struct page *oldpage, struct page *newpage);
>  extern void __set_page_owner_migrate_reason(struct page *page, int reason);
> +extern void __dump_page_owner(struct page *page);
>  
>  static inline void reset_page_owner(struct page *page, unsigned int order)
>  {
> @@ -44,6 +45,11 @@ static inline void set_page_owner_migrate_reason(struct page *page, int reason)
>  	if (static_branch_unlikely(&page_owner_inited))
>  		__set_page_owner_migrate_reason(page, reason);
>  }
> +static inline void dump_page_owner(struct page *page)
> +{
> +	if (static_branch_unlikely(&page_owner_inited))
> +		__dump_page_owner(page);
> +}
>  #else
>  static inline void reset_page_owner(struct page *page, unsigned int order)
>  {
> @@ -62,5 +68,8 @@ static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
>  static inline void set_page_owner_migrate_reason(struct page *page, int reason)
>  {
>  }
> +static inline void dump_page_owner(struct page *page)
> +{
> +}
>  #endif /* CONFIG_PAGE_OWNER */
>  #endif /* __LINUX_PAGE_OWNER_H */
> diff --git a/mm/debug.c b/mm/debug.c
> index 8362765..93373d1 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -9,6 +9,7 @@
>  #include <linux/mm.h>
>  #include <linux/trace_events.h>
>  #include <linux/memcontrol.h>
> +#include <linux/page_owner.h>
>  
>  static const struct trace_print_flags pageflag_names[] = {
>  	{1UL << PG_locked,		"locked"	},
> @@ -98,6 +99,7 @@ void dump_page_badflags(struct page *page, const char *reason,
>  	if (page->mem_cgroup)
>  		pr_alert("page->mem_cgroup:%p\n", page->mem_cgroup);
>  #endif
> +	dump_page_owner(page);

I tend to put dump_page() into random places during debug. Dumping page
owner for all dump_page() cases can be too verbose.

Can we introduce dump_page_verbose() which would do usual dump_page() plus
dump_page_owner()?

>  }
>  
>  void dump_page(struct page *page, const char *reason)
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 388898f..d7e0aaf 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -183,6 +183,24 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
>  	return -ENOMEM;
>  }
>  
> +void __dump_page_owner(struct page *page)
> +{
> +	struct page_ext *page_ext = lookup_page_ext(page);
> +	struct stack_trace trace = {
> +		.nr_entries = page_ext->nr_entries,
> +		.entries = &page_ext->trace_entries[0],
> +	};
> +
> +	pr_alert("page allocated via order %u, mask 0x%x, migratetype %d, trace:\n",
> +			page_ext->order, page_ext->gfp_mask,
> +			gfpflags_to_migratetype(page_ext->gfp_mask));
> +	print_stack_trace(&trace, 0);
> +
> +	if (page_ext->last_migrate_reason != -1)
> +		pr_alert("page has been migrated, last migrate reason: %d\n",
> +			page_ext->last_migrate_reason);
> +}
> +
>  static ssize_t
>  read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>  {
> -- 
> 2.6.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
 Kirill A. Shutemov

Re: [PATCH 5/5] mm, page_owner: dump page owner info from dump_page()

[Sasha Levin]

On 11/04/2015 02:41 PM, Kirill A. Shutemov wrote:
>> +	dump_page_owner(page);
> I tend to put dump_page() into random places during debug. Dumping page
> owner for all dump_page() cases can be too verbose.
> 
> Can we introduce dump_page_verbose() which would do usual dump_page() plus
> dump_page_owner()?
> 

Is there any existing piece of code that would use dump_page() rather than
dump_page_verbose()?


Thanks,
Sasha

Re: [PATCH 5/5] mm, page_owner: dump page owner info from dump_page()

[Kirill A. Shutemov]

On Wed, Nov 04, 2015 at 03:12:39PM -0500, Sasha Levin wrote:
> On 11/04/2015 02:41 PM, Kirill A. Shutemov wrote:
> >> +	dump_page_owner(page);
> > I tend to put dump_page() into random places during debug. Dumping page
> > owner for all dump_page() cases can be too verbose.
> > 
> > Can we introduce dump_page_verbose() which would do usual dump_page() plus
> > dump_page_owner()?
> > 
> 
> Is there any existing piece of code that would use dump_page() rather than
> dump_page_verbose()?

Good point. I think not.

So we can leave dump_page() with dump_page_owner() stuff and have
__dump_page() or something as lighter version.

-- 
 Kirill A. Shutemov

Re: [PATCH 1/5] mm, page_owner: print migratetype of a page, not pageblock

[Joonsoo Kim]

On Wed, Nov 04, 2015 at 04:00:57PM +0100, Vlastimil Babka wrote:
> The information in /sys/kernel/debug/page_owner includes the migratetype
> declared during the page allocation via gfp_flags. This is also checked against
> the pageblock's migratetype, and reported as Fallback allocation if these two
> differ (although in fact fallback allocation is not the only reason why they
> can differ).
> 
> However, the migratetype actually printed is the one of the pageblock, not of
> the page itself, so it's the same for all pages in the pageblock. This is
> apparently a bug, noticed when working on other page_owner improvements. Fixed.

We can guess page migratetype through gfp_mask output although it isn't
easy task for now. But, there is no way to know pageblock migratetype.
I used this to know how memory is fragmented.

Thanks.

> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/page_owner.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 983c3a1..a9f16b8 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -113,7 +113,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
>  			"PFN %lu Block %lu type %d %s Flags %s%s%s%s%s%s%s%s%s%s%s%s\n",
>  			pfn,
>  			pfn >> pageblock_order,
> -			pageblock_mt,
> +			page_mt,
>  			pageblock_mt != page_mt ? "Fallback" : "        ",
>  			PageLocked(page)	? "K" : " ",
>  			PageError(page)		? "E" : " ",
> -- 
> 2.6.2
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/5] mm, page_owner: copy page owner info during migration

[Joonsoo Kim]

On Wed, Nov 04, 2015 at 04:00:59PM +0100, Vlastimil Babka wrote:
> The page_owner mechanism stores gfp_flags of an allocation and stack trace
> that lead to it. During page migration, the original information is
> essentially replaced by the allocation of free page as the migration target.
> Arguably this is less useful and might lead to all the page_owner info for
> migratable pages gradually converge towards compaction or numa balancing
> migrations. It has also lead to inaccuracies such as one fixed by commit
> e2cfc91120fa ("mm/page_owner: set correct gfp_mask on page_owner").
> 
> This patch thus introduces copying the page_owner info during migration.
> However, since the fact that the page has been migrated from its original
> place might be useful for debugging, the next patch will introduce a way to
> track that information as well.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  include/linux/page_owner.h | 10 +++++++++-
>  mm/migrate.c               |  2 ++
>  mm/page_owner.c            | 16 ++++++++++++++++
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h
> index 8e2eb15..6440daa 100644
> --- a/include/linux/page_owner.h
> +++ b/include/linux/page_owner.h
> @@ -11,6 +11,7 @@ extern void __reset_page_owner(struct page *page, unsigned int order);
>  extern void __set_page_owner(struct page *page,
>  			unsigned int order, gfp_t gfp_mask);
>  extern gfp_t __get_page_owner_gfp(struct page *page);
> +extern void __copy_page_owner(struct page *oldpage, struct page *newpage);
>  
>  static inline void reset_page_owner(struct page *page, unsigned int order)
>  {
> @@ -32,6 +33,11 @@ static inline gfp_t get_page_owner_gfp(struct page *page)
>  	else
>  		return 0;
>  }
> +static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
> +{
> +	if (static_branch_unlikely(&page_owner_inited))
> +		__copy_page_owner(oldpage, newpage);
> +}
>  #else
>  static inline void reset_page_owner(struct page *page, unsigned int order)
>  {
> @@ -44,6 +50,8 @@ static inline gfp_t get_page_owner_gfp(struct page *page)
>  {
>  	return 0;
>  }
> -
> +static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
> +{
> +}
>  #endif /* CONFIG_PAGE_OWNER */
>  #endif /* __LINUX_PAGE_OWNER_H */
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 1ae0113..9f82e03 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -38,6 +38,7 @@
>  #include <linux/balloon_compaction.h>
>  #include <linux/mmu_notifier.h>
>  #include <linux/page_idle.h>
> +#include <linux/page_owner.h>
>  
>  #include <asm/tlbflush.h>
>  
> @@ -775,6 +776,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
>  		set_page_memcg(page, NULL);
>  		if (!PageAnon(page))
>  			page->mapping = NULL;
> +		copy_page_owner(page, newpage);
>  	}
>  	return rc;
>  }
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 7664b85..7ebd3d0 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -84,6 +84,22 @@ gfp_t __get_page_owner_gfp(struct page *page)
>  	return page_ext->gfp_mask;
>  }
>  
> +void __copy_page_owner(struct page *oldpage, struct page *newpage)
> +{
> +	struct page_ext *old_ext = lookup_page_ext(oldpage);
> +	struct page_ext *new_ext = lookup_page_ext(newpage);
> +	int i;
> +
> +	new_ext->order = old_ext->order;
> +	new_ext->gfp_mask = old_ext->gfp_mask;
> +	new_ext->nr_entries = old_ext->nr_entries;
> +
> +	for (i = 0; i < ARRAY_SIZE(new_ext->trace_entries); i++)
> +		new_ext->trace_entries[i] = old_ext->trace_entries[i];
> +
> +	__set_bit(PAGE_EXT_OWNER, &new_ext->flags);
> +}
> +

Need to clear PAGE_EXT_OWNER bit in oldppage.

Thanks.

>  static ssize_t
>  print_page_owner(char __user *buf, size_t count, unsigned long pfn,
>  		struct page *page, struct page_ext *page_ext)
> -- 
> 2.6.2
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/5] mm, page_owner: print migratetype of a page, not pageblock

[Vlastimil Babka]

On 11/05/2015 09:09 AM, Joonsoo Kim wrote:
> On Wed, Nov 04, 2015 at 04:00:57PM +0100, Vlastimil Babka wrote:
>> The information in /sys/kernel/debug/page_owner includes the migratetype
>> declared during the page allocation via gfp_flags. This is also checked against
>> the pageblock's migratetype, and reported as Fallback allocation if these two
>> differ (although in fact fallback allocation is not the only reason why they
>> can differ).
>> 
>> However, the migratetype actually printed is the one of the pageblock, not of
>> the page itself, so it's the same for all pages in the pageblock. This is
>> apparently a bug, noticed when working on other page_owner improvements. Fixed.
> 
> We can guess page migratetype through gfp_mask output although it isn't
> easy task for now. But, there is no way to know pageblock migratetype.
> I used this to know how memory is fragmented.

Ah, I see. How bout just we print both migratetypes then and remove the
"Fallback" part, which can be trivially deduced from them (and as I noted it's
somewhat misleading anyway)?

> Thanks.
> 
>> 
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>> ---
>>  mm/page_owner.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/mm/page_owner.c b/mm/page_owner.c
>> index 983c3a1..a9f16b8 100644
>> --- a/mm/page_owner.c
>> +++ b/mm/page_owner.c
>> @@ -113,7 +113,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
>>  			"PFN %lu Block %lu type %d %s Flags %s%s%s%s%s%s%s%s%s%s%s%s\n",
>>  			pfn,
>>  			pfn >> pageblock_order,
>> -			pageblock_mt,
>> +			page_mt,
>>  			pageblock_mt != page_mt ? "Fallback" : "        ",
>>  			PageLocked(page)	? "K" : " ",
>>  			PageError(page)		? "E" : " ",
>> -- 
>> 2.6.2
>> 
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

Re: [PATCH 3/5] mm, page_owner: copy page owner info during migration

[Vlastimil Babka]

On 11/05/2015 09:10 AM, Joonsoo Kim wrote:
> On Wed, Nov 04, 2015 at 04:00:59PM +0100, Vlastimil Babka wrote:
>> +void __copy_page_owner(struct page *oldpage, struct page *newpage)
>> +{
>> +	struct page_ext *old_ext = lookup_page_ext(oldpage);
>> +	struct page_ext *new_ext = lookup_page_ext(newpage);
>> +	int i;
>> +
>> +	new_ext->order = old_ext->order;
>> +	new_ext->gfp_mask = old_ext->gfp_mask;
>> +	new_ext->nr_entries = old_ext->nr_entries;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(new_ext->trace_entries); i++)
>> +		new_ext->trace_entries[i] = old_ext->trace_entries[i];
>> +
>> +	__set_bit(PAGE_EXT_OWNER, &new_ext->flags);
>> +}
>> +
> 
> Need to clear PAGE_EXT_OWNER bit in oldppage.

Hm, I thought that the freeing of the oldpage, which follows the migration,
would take care of that. And if it hit some bug and dump_page before being
freed, we would still have some info to print?

Thanks

> Thanks.
> 
>>  static ssize_t
>>  print_page_owner(char __user *buf, size_t count, unsigned long pfn,
>>  		struct page *page, struct page_ext *page_ext)
>> -- 
>> 2.6.2
>> 
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

Re: [PATCH 1/5] mm, page_owner: print migratetype of a page, not pageblock

[Joonsoo Kim]

On Thu, Nov 05, 2015 at 09:15:01AM +0100, Vlastimil Babka wrote:
> On 11/05/2015 09:09 AM, Joonsoo Kim wrote:
> > On Wed, Nov 04, 2015 at 04:00:57PM +0100, Vlastimil Babka wrote:
> >> The information in /sys/kernel/debug/page_owner includes the migratetype
> >> declared during the page allocation via gfp_flags. This is also checked against
> >> the pageblock's migratetype, and reported as Fallback allocation if these two
> >> differ (although in fact fallback allocation is not the only reason why they
> >> can differ).
> >> 
> >> However, the migratetype actually printed is the one of the pageblock, not of
> >> the page itself, so it's the same for all pages in the pageblock. This is
> >> apparently a bug, noticed when working on other page_owner improvements. Fixed.
> > 
> > We can guess page migratetype through gfp_mask output although it isn't
> > easy task for now. But, there is no way to know pageblock migratetype.
> > I used this to know how memory is fragmented.
> 
> Ah, I see. How bout just we print both migratetypes then and remove the
> "Fallback" part, which can be trivially deduced from them (and as I noted it's
> somewhat misleading anyway)?

I'm okay with your new suggestion.

Thanks.

Re: [PATCH 3/5] mm, page_owner: copy page owner info during migration

[Joonsoo Kim]

On Thu, Nov 05, 2015 at 09:17:33AM +0100, Vlastimil Babka wrote:
> On 11/05/2015 09:10 AM, Joonsoo Kim wrote:
> > On Wed, Nov 04, 2015 at 04:00:59PM +0100, Vlastimil Babka wrote:
> >> +void __copy_page_owner(struct page *oldpage, struct page *newpage)
> >> +{
> >> +	struct page_ext *old_ext = lookup_page_ext(oldpage);
> >> +	struct page_ext *new_ext = lookup_page_ext(newpage);
> >> +	int i;
> >> +
> >> +	new_ext->order = old_ext->order;
> >> +	new_ext->gfp_mask = old_ext->gfp_mask;
> >> +	new_ext->nr_entries = old_ext->nr_entries;
> >> +
> >> +	for (i = 0; i < ARRAY_SIZE(new_ext->trace_entries); i++)
> >> +		new_ext->trace_entries[i] = old_ext->trace_entries[i];
> >> +
> >> +	__set_bit(PAGE_EXT_OWNER, &new_ext->flags);
> >> +}
> >> +
> > 
> > Need to clear PAGE_EXT_OWNER bit in oldppage.
> 
> Hm, I thought that the freeing of the oldpage, which follows the migration,
> would take care of that. And if it hit some bug and dump_page before being
> freed, we would still have some info to print?

Okay. I missed that.

Thanks.

Re: [PATCH 3/5] mm, page_owner: copy page owner info during migration

[Hugh Dickins]

On Wed, 4 Nov 2015, Vlastimil Babka wrote:

> The page_owner mechanism stores gfp_flags of an allocation and stack trace
> that lead to it. During page migration, the original information is
> essentially replaced by the allocation of free page as the migration target.
> Arguably this is less useful and might lead to all the page_owner info for
> migratable pages gradually converge towards compaction or numa balancing
> migrations. It has also lead to inaccuracies such as one fixed by commit
> e2cfc91120fa ("mm/page_owner: set correct gfp_mask on page_owner").
> 
> This patch thus introduces copying the page_owner info during migration.
> However, since the fact that the page has been migrated from its original
> place might be useful for debugging, the next patch will introduce a way to
> track that information as well.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  include/linux/page_owner.h | 10 +++++++++-
>  mm/migrate.c               |  2 ++
>  mm/page_owner.c            | 16 ++++++++++++++++
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h
> index 8e2eb15..6440daa 100644
> --- a/include/linux/page_owner.h
> +++ b/include/linux/page_owner.h
> @@ -11,6 +11,7 @@ extern void __reset_page_owner(struct page *page, unsigned int order);
>  extern void __set_page_owner(struct page *page,
>  			unsigned int order, gfp_t gfp_mask);
>  extern gfp_t __get_page_owner_gfp(struct page *page);
> +extern void __copy_page_owner(struct page *oldpage, struct page *newpage);
>  
>  static inline void reset_page_owner(struct page *page, unsigned int order)
>  {
> @@ -32,6 +33,11 @@ static inline gfp_t get_page_owner_gfp(struct page *page)
>  	else
>  		return 0;
>  }
> +static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
> +{
> +	if (static_branch_unlikely(&page_owner_inited))
> +		__copy_page_owner(oldpage, newpage);
> +}
>  #else
>  static inline void reset_page_owner(struct page *page, unsigned int order)
>  {
> @@ -44,6 +50,8 @@ static inline gfp_t get_page_owner_gfp(struct page *page)
>  {
>  	return 0;
>  }
> -
> +static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
> +{
> +}
>  #endif /* CONFIG_PAGE_OWNER */
>  #endif /* __LINUX_PAGE_OWNER_H */
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 1ae0113..9f82e03 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -38,6 +38,7 @@
>  #include <linux/balloon_compaction.h>
>  #include <linux/mmu_notifier.h>
>  #include <linux/page_idle.h>
> +#include <linux/page_owner.h>
>  
>  #include <asm/tlbflush.h>
>  
> @@ -775,6 +776,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
>  		set_page_memcg(page, NULL);
>  		if (!PageAnon(page))
>  			page->mapping = NULL;
> +		copy_page_owner(page, newpage);

Would it be possible to move that line into migrate_page_copy()?

I don't think it's wrong where you placed it, but that block is really
about resetting the old page ready for freeing, and I'd prefer to keep
all the transference of properties from old to new in migrate_page_copy()
if we can.

But check how that behaves in the migrate_misplaced_transhuge_page()
case: I haven't studied long enough, but I think you may have been missing
to copy_page_owner in that case; but beware of its "fail_putback", which
for some things nastily entails undoing what's already been done.

Hugh

>  	}
>  	return rc;
>  }
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 7664b85..7ebd3d0 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -84,6 +84,22 @@ gfp_t __get_page_owner_gfp(struct page *page)
>  	return page_ext->gfp_mask;
>  }
>  
> +void __copy_page_owner(struct page *oldpage, struct page *newpage)
> +{
> +	struct page_ext *old_ext = lookup_page_ext(oldpage);
> +	struct page_ext *new_ext = lookup_page_ext(newpage);
> +	int i;
> +
> +	new_ext->order = old_ext->order;
> +	new_ext->gfp_mask = old_ext->gfp_mask;
> +	new_ext->nr_entries = old_ext->nr_entries;
> +
> +	for (i = 0; i < ARRAY_SIZE(new_ext->trace_entries); i++)
> +		new_ext->trace_entries[i] = old_ext->trace_entries[i];
> +
> +	__set_bit(PAGE_EXT_OWNER, &new_ext->flags);
> +}
> +
>  static ssize_t
>  print_page_owner(char __user *buf, size_t count, unsigned long pfn,
>  		struct page *page, struct page_ext *page_ext)
> -- 
> 2.6.2
> 
> --

Re: [PATCH 3/5] mm, page_owner: copy page owner info during migration

[Vlastimil Babka]

On 11/08/2015 10:29 PM, Hugh Dickins wrote:
>
> Would it be possible to move that line into migrate_page_copy()?
>
> I don't think it's wrong where you placed it, but that block is really
> about resetting the old page ready for freeing, and I'd prefer to keep
> all the transference of properties from old to new in migrate_page_copy()
> if we can.

OK, makes sense, will do in v2.

> But check how that behaves in the migrate_misplaced_transhuge_page()
> case: I haven't studied long enough, but I think you may have been missing
> to copy_page_owner in that case;

You're right, I missed that path :/

> but beware of its "fail_putback", which
> for some things nastily entails undoing what's already been done.

Yeah, I think I don't need to reset page owner info in the fail_putback 
path, for the same reason I don't reset it from the old page when 
migration is successful. The page is going to be freed anyway, and if it 
somehow hits a bug before that, we will still have something to print 
(after patch 5).

Thanks!