Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1393040 > unrolled thread

[PATCH 0/6] mm/page_owner: use tackdepot to store stacktrace

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] mm/page_owner: use tackdepot to store stacktrace js1304@gmail.com - 2016-05-03 07:30 +0200
    [PATCH 5/6] tools/vm/page_owner: increase temporary buffer size js1304@gmail.com - 2016-05-03 07:30 +0200
    [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace js1304@gmail.com - 2016-05-03 07:30 +0200
      Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace Michal Hocko <mhocko@kernel.org> - 2016-05-03 11:00 +0200
        Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-04 04:20 +0200
          Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-04 04:40 +0200
            Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace Michal Hocko <mhocko@kernel.org> - 2016-05-04 11:30 +0200
              Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace Joonsoo Kim <js1304@gmail.com> - 2016-05-04 17:40 +0200
          Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace Michal Hocko <mhocko@kernel.org> - 2016-05-04 11:30 +0200
            Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace Joonsoo Kim <js1304@gmail.com> - 2016-05-04 17:40 +0200
              Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace Joonsoo Kim <js1304@gmail.com> - 2016-05-04 17:50 +0200
                Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace Michal Hocko <mhocko@kernel.org> - 2016-05-04 21:50 +0200
              Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace Michal Hocko <mhocko@kernel.org> - 2016-05-04 21:50 +0200
                Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace Joonsoo Kim <js1304@gmail.com> - 2016-05-10 09:10 +0200
                  Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace Michal Hocko <mhocko@kernel.org> - 2016-05-10 11:00 +0200
      Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace Vlastimil Babka <vbabka@suse.cz> - 2016-05-12 14:00 +0200
    [PATCH 3/6] mm/page_owner: copy last_migrate_reason in copy_page_owner() js1304@gmail.com - 2016-05-03 07:30 +0200
      Re: [PATCH 3/6] mm/page_owner: copy last_migrate_reason in  copy_page_owner() Vlastimil Babka <vbabka@suse.cz> - 2016-05-10 17:20 +0200
        Re: [PATCH 3/6] mm/page_owner: copy last_migrate_reason in  copy_page_owner() Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-12 05:00 +0200
          Re: [PATCH 3/6] mm/page_owner: copy last_migrate_reason in  copy_page_owner() Vlastimil Babka <vbabka@suse.cz> - 2016-05-12 08:50 +0200
    [PATCH 4/6] mm/page_owner: introduce split_page_owner and replace manual handling js1304@gmail.com - 2016-05-03 07:30 +0200
      Re: [PATCH 4/6] mm/page_owner: introduce split_page_owner and replace  manual handling Vlastimil Babka <vbabka@suse.cz> - 2016-05-10 17:20 +0200
    [PATCH 2/6] mm/page_owner: initialize page owner without holding the zone lock js1304@gmail.com - 2016-05-03 07:30 +0200
      Re: [PATCH 2/6] mm/page_owner: initialize page owner without holding  the zone lock Vlastimil Babka <vbabka@suse.cz> - 2016-05-10 17:10 +0200
    [PATCH 1/6] mm/compaction: split freepages without holding the zone lock js1304@gmail.com - 2016-05-03 07:30 +0200
      Re: [PATCH 1/6] mm/compaction: split freepages without holding the  zone lock Vlastimil Babka <vbabka@suse.cz> - 2016-05-10 17:00 +0200
        Re: [PATCH 1/6] mm/compaction: split freepages without holding the  zone lock Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-12 05:00 +0200

Page 1 of 2  [1] 2  Next page →

#1393040 — [PATCH 0/6] mm/page_owner: use tackdepot to store stacktrace

From: Joonsoo Kim <iamjoonsoo.kim@lge.com>

This patchset changes a way to store stacktrace in page_owner in order to
reduce memory usage. Below is motivation of this patchset coped
from the patch 6.

Currently, we store each page's allocation stacktrace on corresponding
page_ext structure and it requires a lot of memory. This causes the problem
that memory tight system doesn't work well if page_owner is enabled.
Moreover, even with this large memory consumption, we cannot get full
stacktrace because we allocate memory at boot time and just maintain
8 stacktrace slots to balance memory consumption. We could increase it
to more but it would make system unusable or change system behaviour.

To solve the problem, this patch uses stackdepot to store stacktrace.

Thanks.

Joonsoo Kim (6):
  mm/compaction: split freepages without holding the zone lock
  mm/page_owner: initialize page owner without holding the zone lock
  mm/page_owner: copy last_migrate_reason in copy_page_owner()
  mm/page_owner: introduce split_page_owner and replace manual handling
  tools/vm/page_owner: increase temporary buffer size
  mm/page_owner: use stackdepot to store stacktrace

 include/linux/mm.h         |   1 -
 include/linux/page_ext.h   |   4 +-
 include/linux/page_owner.h |  12 ++--
 lib/Kconfig.debug          |   1 +
 mm/compaction.c            |  45 +++++++++++----
 mm/page_alloc.c            |  37 +-----------
 mm/page_isolation.c        |   9 ++-
 mm/page_owner.c            | 136 ++++++++++++++++++++++++++++++++++++++-------
 tools/vm/page_owner_sort.c |   9 ++-
 9 files changed, 173 insertions(+), 81 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1393041 — [PATCH 5/6] tools/vm/page_owner: increase temporary buffer size

From: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Page owner will be changed to store more deep stacktrace
so current temporary buffer size isn't enough. Increase it.

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 tools/vm/page_owner_sort.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/vm/page_owner_sort.c b/tools/vm/page_owner_sort.c
index 77147b4..f1c055f 100644
--- a/tools/vm/page_owner_sort.c
+++ b/tools/vm/page_owner_sort.c
@@ -79,12 +79,12 @@ static void add_list(char *buf, int len)
 	}
 }
 
-#define BUF_SIZE	1024
+#define BUF_SIZE	(128 * 1024)
 
 int main(int argc, char **argv)
 {
 	FILE *fin, *fout;
-	char buf[BUF_SIZE];
+	char *buf;
 	int ret, i, count;
 	struct block_list *list2;
 	struct stat st;
@@ -107,6 +107,11 @@ int main(int argc, char **argv)
 	max_size = st.st_size / 100; /* hack ... */
 
 	list = malloc(max_size * sizeof(*list));
+	buf = malloc(BUF_SIZE);
+	if (!list || !buf) {
+		printf("Out of memory\n");
+		exit(1);
+	}
 
 	for ( ; ; ) {
 		ret = read_block(buf, BUF_SIZE, fin);
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1393042 — [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace

From: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Currently, we store each page's allocation stacktrace on corresponding
page_ext structure and it requires a lot of memory. This causes the problem
that memory tight system doesn't work well if page_owner is enabled.
Moreover, even with this large memory consumption, we cannot get full
stacktrace because we allocate memory at boot time and just maintain
8 stacktrace slots to balance memory consumption. We could increase it
to more but it would make system unusable or change system behaviour.

To solve the problem, this patch uses stackdepot to store stacktrace.
It obviously provides memory saving but there is a drawback that
stackdepot could fail.

stackdepot allocates memory at runtime so it could fail if system has
not enough memory. But, most of allocation stack are generated at very
early time and there are much memory at this time. So, failure would not
happen easily. And, one failure means that we miss just one page's
allocation stacktrace so it would not be a big problem. In this patch,
when memory allocation failure happens, we store special stracktrace
handle to the page that is failed to save stacktrace. With it, user
can guess memory usage properly even if failure happens.

Memory saving looks as following. (Boot 4GB memory system with page_owner)

92274688 bytes -> 25165824 bytes

72% reduction in static allocation size. Even if we should add up size of
dynamic allocation memory, it would not that big because stacktrace is
mostly duplicated.

Note that implementation looks complex than someone would imagine because
there is recursion issue. stackdepot uses page allocator and page_owner
is called at page allocation. Using stackdepot in page_owner could re-call
page allcator and then page_owner. That is a recursion. To detect and
avoid it, whenever we obtain stacktrace, recursion is checked and
page_owner is set to dummy information if found. Dummy information means
that this page is allocated for page_owner feature itself
(such as stackdepot) and it's understandable behavior for user.

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 include/linux/page_ext.h |   4 +-
 lib/Kconfig.debug        |   1 +
 mm/page_owner.c          | 128 ++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 114 insertions(+), 19 deletions(-)

diff --git a/include/linux/page_ext.h b/include/linux/page_ext.h
index e1fe7cf..03f2a3e 100644
--- a/include/linux/page_ext.h
+++ b/include/linux/page_ext.h
@@ -3,6 +3,7 @@
 
 #include <linux/types.h>
 #include <linux/stacktrace.h>
+#include <linux/stackdepot.h>
 
 struct pglist_data;
 struct page_ext_operations {
@@ -44,9 +45,8 @@ struct page_ext {
 #ifdef CONFIG_PAGE_OWNER
 	unsigned int order;
 	gfp_t gfp_mask;
-	unsigned int nr_entries;
 	int last_migrate_reason;
-	unsigned long trace_entries[8];
+	depot_stack_handle_t handle;
 #endif
 };
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5d57177..a32fd24 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -248,6 +248,7 @@ config PAGE_OWNER
 	depends on DEBUG_KERNEL && STACKTRACE_SUPPORT
 	select DEBUG_FS
 	select STACKTRACE
+	select STACKDEPOT
 	select PAGE_EXTENSION
 	help
 	  This keeps track of what call chain is the owner of a page, may
diff --git a/mm/page_owner.c b/mm/page_owner.c
index 7b5a834..7875de5 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -7,11 +7,18 @@
 #include <linux/page_owner.h>
 #include <linux/jump_label.h>
 #include <linux/migrate.h>
+#include <linux/stackdepot.h>
+
 #include "internal.h"
 
+#define PAGE_OWNER_STACK_DEPTH (64)
+
 static bool page_owner_disabled = true;
 DEFINE_STATIC_KEY_FALSE(page_owner_inited);
 
+static depot_stack_handle_t dummy_handle;
+static depot_stack_handle_t failure_handle;
+
 static void init_early_allocated_pages(void);
 
 static int early_page_owner_param(char *buf)
@@ -34,11 +41,41 @@ static bool need_page_owner(void)
 	return true;
 }
 
+static noinline void register_dummy_stack(void)
+{
+	unsigned long entries[4];
+	struct stack_trace dummy;
+
+	dummy.nr_entries = 0;
+	dummy.max_entries = ARRAY_SIZE(entries);
+	dummy.entries = &entries[0];
+	dummy.skip = 0;
+
+	save_stack_trace(&dummy);
+	dummy_handle = depot_save_stack(&dummy, GFP_KERNEL);
+}
+
+static noinline void register_failure_stack(void)
+{
+	unsigned long entries[4];
+	struct stack_trace failure;
+
+	failure.nr_entries = 0;
+	failure.max_entries = ARRAY_SIZE(entries);
+	failure.entries = &entries[0];
+	failure.skip = 0;
+
+	save_stack_trace(&failure);
+	failure_handle = depot_save_stack(&failure, GFP_KERNEL);
+}
+
 static void init_page_owner(void)
 {
 	if (page_owner_disabled)
 		return;
 
+	register_dummy_stack();
+	register_failure_stack();
 	static_branch_enable(&page_owner_inited);
 	init_early_allocated_pages();
 }
@@ -59,21 +96,56 @@ void __reset_page_owner(struct page *page, unsigned int order)
 	}
 }
 
-void __set_page_owner(struct page *page, unsigned int order, gfp_t gfp_mask)
+static inline bool check_recursive_alloc(struct stack_trace *trace,
+					unsigned long ip)
 {
-	struct page_ext *page_ext = lookup_page_ext(page);
+	int i, count;
+
+	if (!trace->nr_entries)
+		return false;
+
+	for (i = 0, count = 0; i < trace->nr_entries; i++) {
+		if (trace->entries[i] == ip && ++count == 2)
+			return true;
+	}
+
+	return false;
+}
+
+static noinline depot_stack_handle_t save_stack(gfp_t flags)
+{
+	unsigned long entries[PAGE_OWNER_STACK_DEPTH];
 	struct stack_trace trace = {
 		.nr_entries = 0,
-		.max_entries = ARRAY_SIZE(page_ext->trace_entries),
-		.entries = &page_ext->trace_entries[0],
-		.skip = 3,
+		.entries = entries,
+		.max_entries = PAGE_OWNER_STACK_DEPTH,
+		.skip = 0
 	};
+	depot_stack_handle_t handle;
 
 	save_stack_trace(&trace);
+	if (trace.nr_entries != 0 &&
+	    trace.entries[trace.nr_entries-1] == ULONG_MAX)
+		trace.nr_entries--;
+
+	if (check_recursive_alloc(&trace, _RET_IP_))
+		return dummy_handle;
+
+	handle = depot_save_stack(&trace, flags);
+	if (!handle)
+		handle = failure_handle;
+
+	return handle;
+}
 
+noinline void __set_page_owner(struct page *page, unsigned int order,
+					gfp_t gfp_mask)
+{
+	struct page_ext *page_ext = lookup_page_ext(page);
+
+	page_ext->handle = save_stack(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);
@@ -100,15 +172,11 @@ 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->last_migrate_reason = old_ext->last_migrate_reason;
-	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];
+	new_ext->handle = old_ext->handle;
 
 	/*
 	 * We don't clear the bit on the oldpage as it's going to be freed
@@ -124,14 +192,18 @@ void __copy_page_owner(struct page *oldpage, struct page *newpage)
 
 static ssize_t
 print_page_owner(char __user *buf, size_t count, unsigned long pfn,
-		struct page *page, struct page_ext *page_ext)
+		struct page *page, struct page_ext *page_ext,
+		depot_stack_handle_t handle)
 {
 	int ret;
 	int pageblock_mt, page_mt;
 	char *kbuf;
+	unsigned long entries[PAGE_OWNER_STACK_DEPTH];
 	struct stack_trace trace = {
-		.nr_entries = page_ext->nr_entries,
-		.entries = &page_ext->trace_entries[0],
+		.nr_entries = 0,
+		.entries = entries,
+		.max_entries = PAGE_OWNER_STACK_DEPTH,
+		.skip = 0
 	};
 
 	kbuf = kmalloc(count, GFP_KERNEL);
@@ -160,6 +232,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 	if (ret >= count)
 		goto err;
 
+	depot_fetch_stack(handle, &trace);
 	ret += snprint_stack_trace(kbuf + ret, count - ret, &trace, 0);
 	if (ret >= count)
 		goto err;
@@ -190,10 +263,14 @@ err:
 void __dump_page_owner(struct page *page)
 {
 	struct page_ext *page_ext = lookup_page_ext(page);
+	unsigned long entries[PAGE_OWNER_STACK_DEPTH];
 	struct stack_trace trace = {
-		.nr_entries = page_ext->nr_entries,
-		.entries = &page_ext->trace_entries[0],
+		.nr_entries = 0,
+		.entries = entries,
+		.max_entries = PAGE_OWNER_STACK_DEPTH,
+		.skip = 0
 	};
+	depot_stack_handle_t handle;
 	gfp_t gfp_mask = page_ext->gfp_mask;
 	int mt = gfpflags_to_migratetype(gfp_mask);
 
@@ -202,6 +279,13 @@ void __dump_page_owner(struct page *page)
 		return;
 	}
 
+	handle = READ_ONCE(page_ext->handle);
+	if (!handle) {
+		pr_alert("page_owner info is not active (free page?)\n");
+		return;
+	}
+
+	depot_fetch_stack(handle, &trace);
 	pr_alert("page allocated via order %u, migratetype %s, gfp_mask %#x(%pGg)\n",
 		 page_ext->order, migratetype_names[mt], gfp_mask, &gfp_mask);
 	print_stack_trace(&trace, 0);
@@ -217,6 +301,7 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	unsigned long pfn;
 	struct page *page;
 	struct page_ext *page_ext;
+	depot_stack_handle_t handle;
 
 	if (!static_branch_unlikely(&page_owner_inited))
 		return -EINVAL;
@@ -263,10 +348,19 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 		if (!test_bit(PAGE_EXT_OWNER, &page_ext->flags))
 			continue;
 
+		/*
+		 * Access to page_ext->handle isn't synchronous so we should
+		 * be careful to access it.
+		 */
+		handle = READ_ONCE(page_ext->handle);
+		if (!handle)
+			continue;
+
 		/* Record the next PFN to read in the file offset */
 		*ppos = (pfn - min_low_pfn) + 1;
 
-		return print_page_owner(buf, count, pfn, page, page_ext);
+		return print_page_owner(buf, count, pfn, page,
+				page_ext, handle);
 	}
 
 	return 0;
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1393200 — Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace

On Tue 03-05-16 14:23:04, Joonsoo Kim wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> Currently, we store each page's allocation stacktrace on corresponding
> page_ext structure and it requires a lot of memory. This causes the problem
> that memory tight system doesn't work well if page_owner is enabled.
> Moreover, even with this large memory consumption, we cannot get full
> stacktrace because we allocate memory at boot time and just maintain
> 8 stacktrace slots to balance memory consumption. We could increase it
> to more but it would make system unusable or change system behaviour.
> 
> To solve the problem, this patch uses stackdepot to store stacktrace.
> It obviously provides memory saving but there is a drawback that
> stackdepot could fail.
> 
> stackdepot allocates memory at runtime so it could fail if system has
> not enough memory. But, most of allocation stack are generated at very
> early time and there are much memory at this time. So, failure would not
> happen easily. And, one failure means that we miss just one page's
> allocation stacktrace so it would not be a big problem. In this patch,
> when memory allocation failure happens, we store special stracktrace
> handle to the page that is failed to save stacktrace. With it, user
> can guess memory usage properly even if failure happens.
> 
> Memory saving looks as following. (Boot 4GB memory system with page_owner)
> 
> 92274688 bytes -> 25165824 bytes

It is not clear to me whether this is after a fresh boot or some workload
which would grow the stack depot as well. What is a usual cap for the
memory consumption.

> 72% reduction in static allocation size. Even if we should add up size of
> dynamic allocation memory, it would not that big because stacktrace is
> mostly duplicated.
> 
> Note that implementation looks complex than someone would imagine because
> there is recursion issue. stackdepot uses page allocator and page_owner
> is called at page allocation. Using stackdepot in page_owner could re-call
> page allcator and then page_owner. That is a recursion.

This is rather fragile. How do we check there is no lock dependency
introduced later on - e.g. split_page called from a different
locking/reclaim context than alloc_pages? Would it be safer to
use ~__GFP_DIRECT_RECLAIM for those stack allocations? Or do you think
there would be too many failed allocations? This alone wouldn't remove a
need for the recursion detection but it sounds less tricky.

> To detect and
> avoid it, whenever we obtain stacktrace, recursion is checked and
> page_owner is set to dummy information if found. Dummy information means
> that this page is allocated for page_owner feature itself
> (such as stackdepot) and it's understandable behavior for user.
> 
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

I like the idea in general I just wish this would be less subtle. Few
more comments below.

[...]
> -void __set_page_owner(struct page *page, unsigned int order, gfp_t gfp_mask)
> +static inline bool check_recursive_alloc(struct stack_trace *trace,
> +					unsigned long ip)
>  {
> -	struct page_ext *page_ext = lookup_page_ext(page);
> +	int i, count;
> +
> +	if (!trace->nr_entries)
> +		return false;
> +
> +	for (i = 0, count = 0; i < trace->nr_entries; i++) {
> +		if (trace->entries[i] == ip && ++count == 2)
> +			return true;
> +	}

This would deserve a comment I guess. Btw, don't we have a better and
more robust way to detect the recursion? Per task_struct flag or
something like that?

[...]
> +static noinline depot_stack_handle_t save_stack(gfp_t flags)
> +{
> +	unsigned long entries[PAGE_OWNER_STACK_DEPTH];
>  	struct stack_trace trace = {
>  		.nr_entries = 0,
> -		.max_entries = ARRAY_SIZE(page_ext->trace_entries),
> -		.entries = &page_ext->trace_entries[0],
> -		.skip = 3,
> +		.entries = entries,
> +		.max_entries = PAGE_OWNER_STACK_DEPTH,
> +		.skip = 0
>  	};
[...]
>  void __dump_page_owner(struct page *page)
>  {
>  	struct page_ext *page_ext = lookup_page_ext(page);
> +	unsigned long entries[PAGE_OWNER_STACK_DEPTH];

This is worrying because of the excessive stack consumption while we
might be in a deep call chain already. Can we preallocate a hash table
for few buffers when the feature is enabled? This would require locking
of course but chances are that contention wouldn't be that large.

>  	struct stack_trace trace = {
> -		.nr_entries = page_ext->nr_entries,
> -		.entries = &page_ext->trace_entries[0],
> +		.nr_entries = 0,
> +		.entries = entries,
> +		.max_entries = PAGE_OWNER_STACK_DEPTH,
> +		.skip = 0
>  	};
> +	depot_stack_handle_t handle;
>  	gfp_t gfp_mask = page_ext->gfp_mask;
>  	int mt = gfpflags_to_migratetype(gfp_mask);
>  

Thanks!
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1393897 — Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace

On Tue, May 03, 2016 at 10:53:56AM +0200, Michal Hocko wrote:
> On Tue 03-05-16 14:23:04, Joonsoo Kim wrote:
> > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > 
> > Currently, we store each page's allocation stacktrace on corresponding
> > page_ext structure and it requires a lot of memory. This causes the problem
> > that memory tight system doesn't work well if page_owner is enabled.
> > Moreover, even with this large memory consumption, we cannot get full
> > stacktrace because we allocate memory at boot time and just maintain
> > 8 stacktrace slots to balance memory consumption. We could increase it
> > to more but it would make system unusable or change system behaviour.
> > 
> > To solve the problem, this patch uses stackdepot to store stacktrace.
> > It obviously provides memory saving but there is a drawback that
> > stackdepot could fail.
> > 
> > stackdepot allocates memory at runtime so it could fail if system has
> > not enough memory. But, most of allocation stack are generated at very
> > early time and there are much memory at this time. So, failure would not
> > happen easily. And, one failure means that we miss just one page's
> > allocation stacktrace so it would not be a big problem. In this patch,
> > when memory allocation failure happens, we store special stracktrace
> > handle to the page that is failed to save stacktrace. With it, user
> > can guess memory usage properly even if failure happens.
> > 
> > Memory saving looks as following. (Boot 4GB memory system with page_owner)
> > 
> > 92274688 bytes -> 25165824 bytes
> 
> It is not clear to me whether this is after a fresh boot or some workload
> which would grow the stack depot as well. What is a usual cap for the
> memory consumption.

It is static allocation size after a fresh boot. I didn't add size of
dynamic allocation memory so it could be larger a little. See below line.
> 
> > 72% reduction in static allocation size. Even if we should add up size of
> > dynamic allocation memory, it would not that big because stacktrace is
> > mostly duplicated.
> > 
> > Note that implementation looks complex than someone would imagine because
> > there is recursion issue. stackdepot uses page allocator and page_owner
> > is called at page allocation. Using stackdepot in page_owner could re-call
> > page allcator and then page_owner. That is a recursion.
> 
> This is rather fragile. How do we check there is no lock dependency
> introduced later on - e.g. split_page called from a different
> locking/reclaim context than alloc_pages? Would it be safer to

There is no callsite that calls set_page_owner() with
__GFP_DIRECT_RECLAIM. So, there would be no lock/context dependency
now.

split_page() doesn't call set_page_owner(). Instead, it calls
split_page_owner() and just copies previous entry. Since it doesn't
require any new stackdepot entry, it is safe in any context.

> use ~__GFP_DIRECT_RECLAIM for those stack allocations? Or do you think
> there would be too many failed allocations? This alone wouldn't remove a
> need for the recursion detection but it sounds less tricky.
> 
> > To detect and
> > avoid it, whenever we obtain stacktrace, recursion is checked and
> > page_owner is set to dummy information if found. Dummy information means
> > that this page is allocated for page_owner feature itself
> > (such as stackdepot) and it's understandable behavior for user.
> > 
> > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> I like the idea in general I just wish this would be less subtle. Few
> more comments below.
> 
> [...]
> > -void __set_page_owner(struct page *page, unsigned int order, gfp_t gfp_mask)
> > +static inline bool check_recursive_alloc(struct stack_trace *trace,
> > +					unsigned long ip)
> >  {
> > -	struct page_ext *page_ext = lookup_page_ext(page);
> > +	int i, count;
> > +
> > +	if (!trace->nr_entries)
> > +		return false;
> > +
> > +	for (i = 0, count = 0; i < trace->nr_entries; i++) {
> > +		if (trace->entries[i] == ip && ++count == 2)
> > +			return true;
> > +	}
> 
> This would deserve a comment I guess. Btw, don't we have a better and
> more robust way to detect the recursion? Per task_struct flag or
> something like that?

Okay. I will add a comment.

I already considered task_struct flag and I know that it is a better
solution. But, I don't think that this debugging feature deserve to
use such precious flag. This implementation isn't efficient but I
think that it is at least robust.

> [...]
> > +static noinline depot_stack_handle_t save_stack(gfp_t flags)
> > +{
> > +	unsigned long entries[PAGE_OWNER_STACK_DEPTH];
> >  	struct stack_trace trace = {
> >  		.nr_entries = 0,
> > -		.max_entries = ARRAY_SIZE(page_ext->trace_entries),
> > -		.entries = &page_ext->trace_entries[0],
> > -		.skip = 3,
> > +		.entries = entries,
> > +		.max_entries = PAGE_OWNER_STACK_DEPTH,
> > +		.skip = 0
> >  	};
> [...]
> >  void __dump_page_owner(struct page *page)
> >  {
> >  	struct page_ext *page_ext = lookup_page_ext(page);
> > +	unsigned long entries[PAGE_OWNER_STACK_DEPTH];
> 
> This is worrying because of the excessive stack consumption while we
> might be in a deep call chain already. Can we preallocate a hash table
> for few buffers when the feature is enabled? This would require locking
> of course but chances are that contention wouldn't be that large.

Make sense but I'm not sure that excessive stack consumption would
cause real problem. For example, if direct reclaim is triggered during
allocation, it may go more deeper than this path. I'd like to
postpone to handle this issue until stack breakage is reported due to
this feature.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1393907 — Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace

On Wed, May 04, 2016 at 11:14:50AM +0900, Joonsoo Kim wrote:
> On Tue, May 03, 2016 at 10:53:56AM +0200, Michal Hocko wrote:
> > On Tue 03-05-16 14:23:04, Joonsoo Kim wrote:
> > > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > > 
> > > Currently, we store each page's allocation stacktrace on corresponding
> > > page_ext structure and it requires a lot of memory. This causes the problem
> > > that memory tight system doesn't work well if page_owner is enabled.
> > > Moreover, even with this large memory consumption, we cannot get full
> > > stacktrace because we allocate memory at boot time and just maintain
> > > 8 stacktrace slots to balance memory consumption. We could increase it
> > > to more but it would make system unusable or change system behaviour.
> > > 
> > > To solve the problem, this patch uses stackdepot to store stacktrace.
> > > It obviously provides memory saving but there is a drawback that
> > > stackdepot could fail.
> > > 
> > > stackdepot allocates memory at runtime so it could fail if system has
> > > not enough memory. But, most of allocation stack are generated at very
> > > early time and there are much memory at this time. So, failure would not
> > > happen easily. And, one failure means that we miss just one page's
> > > allocation stacktrace so it would not be a big problem. In this patch,
> > > when memory allocation failure happens, we store special stracktrace
> > > handle to the page that is failed to save stacktrace. With it, user
> > > can guess memory usage properly even if failure happens.
> > > 
> > > Memory saving looks as following. (Boot 4GB memory system with page_owner)
> > > 
> > > 92274688 bytes -> 25165824 bytes
> > 
> > It is not clear to me whether this is after a fresh boot or some workload
> > which would grow the stack depot as well. What is a usual cap for the
> > memory consumption.
> 
> It is static allocation size after a fresh boot. I didn't add size of
> dynamic allocation memory so it could be larger a little. See below line.
> > 
> > > 72% reduction in static allocation size. Even if we should add up size of
> > > dynamic allocation memory, it would not that big because stacktrace is
> > > mostly duplicated.
> > > 
> > > Note that implementation looks complex than someone would imagine because
> > > there is recursion issue. stackdepot uses page allocator and page_owner
> > > is called at page allocation. Using stackdepot in page_owner could re-call
> > > page allcator and then page_owner. That is a recursion.
> > 
> > This is rather fragile. How do we check there is no lock dependency
> > introduced later on - e.g. split_page called from a different
> > locking/reclaim context than alloc_pages? Would it be safer to
> 
> There is no callsite that calls set_page_owner() with
> __GFP_DIRECT_RECLAIM. So, there would be no lock/context dependency
> now.
> 
> split_page() doesn't call set_page_owner(). Instead, it calls
> split_page_owner() and just copies previous entry. Since it doesn't
> require any new stackdepot entry, it is safe in any context.
> 
> > use ~__GFP_DIRECT_RECLAIM for those stack allocations? Or do you think
> > there would be too many failed allocations? This alone wouldn't remove a
> > need for the recursion detection but it sounds less tricky.
> > 
> > > To detect and
> > > avoid it, whenever we obtain stacktrace, recursion is checked and
> > > page_owner is set to dummy information if found. Dummy information means
> > > that this page is allocated for page_owner feature itself
> > > (such as stackdepot) and it's understandable behavior for user.
> > > 
> > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > 
> > I like the idea in general I just wish this would be less subtle. Few
> > more comments below.
> > 
> > [...]
> > > -void __set_page_owner(struct page *page, unsigned int order, gfp_t gfp_mask)
> > > +static inline bool check_recursive_alloc(struct stack_trace *trace,
> > > +					unsigned long ip)
> > >  {
> > > -	struct page_ext *page_ext = lookup_page_ext(page);
> > > +	int i, count;
> > > +
> > > +	if (!trace->nr_entries)
> > > +		return false;
> > > +
> > > +	for (i = 0, count = 0; i < trace->nr_entries; i++) {
> > > +		if (trace->entries[i] == ip && ++count == 2)
> > > +			return true;
> > > +	}
> > 
> > This would deserve a comment I guess. Btw, don't we have a better and
> > more robust way to detect the recursion? Per task_struct flag or
> > something like that?
> 
> Okay. I will add a comment.
> 
> I already considered task_struct flag and I know that it is a better
> solution. But, I don't think that this debugging feature deserve to
> use such precious flag. This implementation isn't efficient but I
> think that it is at least robust.
> 
> > [...]
> > > +static noinline depot_stack_handle_t save_stack(gfp_t flags)
> > > +{
> > > +	unsigned long entries[PAGE_OWNER_STACK_DEPTH];
> > >  	struct stack_trace trace = {
> > >  		.nr_entries = 0,
> > > -		.max_entries = ARRAY_SIZE(page_ext->trace_entries),
> > > -		.entries = &page_ext->trace_entries[0],
> > > -		.skip = 3,
> > > +		.entries = entries,
> > > +		.max_entries = PAGE_OWNER_STACK_DEPTH,
> > > +		.skip = 0
> > >  	};
> > [...]
> > >  void __dump_page_owner(struct page *page)
> > >  {
> > >  	struct page_ext *page_ext = lookup_page_ext(page);
> > > +	unsigned long entries[PAGE_OWNER_STACK_DEPTH];
> > 
> > This is worrying because of the excessive stack consumption while we
> > might be in a deep call chain already. Can we preallocate a hash table
> > for few buffers when the feature is enabled? This would require locking
> > of course but chances are that contention wouldn't be that large.
> 
> Make sense but I'm not sure that excessive stack consumption would
> cause real problem. For example, if direct reclaim is triggered during
> allocation, it may go more deeper than this path. I'd like to
> postpone to handle this issue until stack breakage is reported due to
> this feature.

Oops... I think more deeply and change my mind. In recursion case,
stack is consumed more than 1KB and it would be a problem. I think
that best approach is using preallocated per cpu entry. It will also
close recursion detection issue by paying interrupt on/off overhead.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1394113 — Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace

On Wed 04-05-16 11:35:00, Joonsoo Kim wrote:
[...]
> Oops... I think more deeply and change my mind. In recursion case,
> stack is consumed more than 1KB and it would be a problem. I think
> that best approach is using preallocated per cpu entry. It will also
> close recursion detection issue by paying interrupt on/off overhead.

I was thinking about per-cpu solution as well but the thing is that the
stackdepot will allocate and until you drop __GFP_DIRECT_RECLAIM then
per-cpu is not safe. I haven't checked the implamentation of
depot_save_stack but I assume it will not schedule in other places.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1394478 — Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace

2016-05-04 18:23 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> On Wed 04-05-16 11:35:00, Joonsoo Kim wrote:
> [...]
>> Oops... I think more deeply and change my mind. In recursion case,
>> stack is consumed more than 1KB and it would be a problem. I think
>> that best approach is using preallocated per cpu entry. It will also
>> close recursion detection issue by paying interrupt on/off overhead.
>
> I was thinking about per-cpu solution as well but the thing is that the
> stackdepot will allocate and until you drop __GFP_DIRECT_RECLAIM then
> per-cpu is not safe. I haven't checked the implamentation of
> depot_save_stack but I assume it will not schedule in other places.

I will think more.

Thanks for review!

Thanks.

[toc] | [prev] | [next] | [standalone]

#1394112 — Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace

On Wed 04-05-16 11:14:50, Joonsoo Kim wrote:
> On Tue, May 03, 2016 at 10:53:56AM +0200, Michal Hocko wrote:
> > On Tue 03-05-16 14:23:04, Joonsoo Kim wrote:
[...]
> > > Memory saving looks as following. (Boot 4GB memory system with page_owner)
> > > 
> > > 92274688 bytes -> 25165824 bytes
> > 
> > It is not clear to me whether this is after a fresh boot or some workload
> > which would grow the stack depot as well. What is a usual cap for the
> > memory consumption.
> 
> It is static allocation size after a fresh boot. I didn't add size of
> dynamic allocation memory so it could be larger a little. See below line.
> > 
> > > 72% reduction in static allocation size. Even if we should add up size of
> > > dynamic allocation memory, it would not that big because stacktrace is
> > > mostly duplicated.

This would be true only if most of the allocation stacks are basically
same after the boot which I am not really convinced is true. But you are
right that the number of sublicates will grow only a little. I was
interested about how much is that little ;)

> > > Note that implementation looks complex than someone would imagine because
> > > there is recursion issue. stackdepot uses page allocator and page_owner
> > > is called at page allocation. Using stackdepot in page_owner could re-call
> > > page allcator and then page_owner. That is a recursion.
> > 
> > This is rather fragile. How do we check there is no lock dependency
> > introduced later on - e.g. split_page called from a different
> > locking/reclaim context than alloc_pages? Would it be safer to
> 
> There is no callsite that calls set_page_owner() with
> __GFP_DIRECT_RECLAIM. So, there would be no lock/context dependency
> now.

I am confused now. prep_new_page is called with the gfp_mask of the
original request, no?

> split_page() doesn't call set_page_owner(). Instead, it calls
> split_page_owner() and just copies previous entry. Since it doesn't
> require any new stackdepot entry, it is safe in any context.

Ohh, you are right. I have missed patch 4
(http://lkml.kernel.org/r/1462252984-8524-5-git-send-email-iamjoonsoo.kim@lge.com)

> > use ~__GFP_DIRECT_RECLAIM for those stack allocations? Or do you think
> > there would be too many failed allocations? This alone wouldn't remove a
> > need for the recursion detection but it sounds less tricky.
> > 
> > > To detect and
> > > avoid it, whenever we obtain stacktrace, recursion is checked and
> > > page_owner is set to dummy information if found. Dummy information means
> > > that this page is allocated for page_owner feature itself
> > > (such as stackdepot) and it's understandable behavior for user.
> > > 
> > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > 
> > I like the idea in general I just wish this would be less subtle. Few
> > more comments below.
> > 
> > [...]
> > > -void __set_page_owner(struct page *page, unsigned int order, gfp_t gfp_mask)
> > > +static inline bool check_recursive_alloc(struct stack_trace *trace,
> > > +					unsigned long ip)
> > >  {
> > > -	struct page_ext *page_ext = lookup_page_ext(page);
> > > +	int i, count;
> > > +
> > > +	if (!trace->nr_entries)
> > > +		return false;
> > > +
> > > +	for (i = 0, count = 0; i < trace->nr_entries; i++) {
> > > +		if (trace->entries[i] == ip && ++count == 2)
> > > +			return true;
> > > +	}
> > 
> > This would deserve a comment I guess. Btw, don't we have a better and
> > more robust way to detect the recursion? Per task_struct flag or
> > something like that?
> 
> Okay. I will add a comment.
> 
> I already considered task_struct flag and I know that it is a better
> solution. But, I don't think that this debugging feature deserve to
> use such precious flag. This implementation isn't efficient but I
> think that it is at least robust.

I guess there are many holes in task_structs where a single bool would
comfortably fit in. But I do not consider this to be a large issue. It
is just the above looks quite ugly.

> > [...]
> > > +static noinline depot_stack_handle_t save_stack(gfp_t flags)
> > > +{
> > > +	unsigned long entries[PAGE_OWNER_STACK_DEPTH];
> > >  	struct stack_trace trace = {
> > >  		.nr_entries = 0,
> > > -		.max_entries = ARRAY_SIZE(page_ext->trace_entries),
> > > -		.entries = &page_ext->trace_entries[0],
> > > -		.skip = 3,
> > > +		.entries = entries,
> > > +		.max_entries = PAGE_OWNER_STACK_DEPTH,
> > > +		.skip = 0
> > >  	};
> > [...]
> > >  void __dump_page_owner(struct page *page)
> > >  {
> > >  	struct page_ext *page_ext = lookup_page_ext(page);
> > > +	unsigned long entries[PAGE_OWNER_STACK_DEPTH];
> > 
> > This is worrying because of the excessive stack consumption while we
> > might be in a deep call chain already. Can we preallocate a hash table
> > for few buffers when the feature is enabled? This would require locking
> > of course but chances are that contention wouldn't be that large.
> 
> Make sense but I'm not sure that excessive stack consumption would
> cause real problem. For example, if direct reclaim is triggered during
> allocation, it may go more deeper than this path.

Do we really consume 512B of stack during reclaim. That sounds more than
worrying to me.

> I'd like to postpone to handle this issue until stack breakage is
> reported due to this feature.

I dunno, but I would expect that a debugging feature wouldn't cause
problems like that. It is more than sad when you cannot debug your
issue just because of the stack consumption...

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1394473 — Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace

2016-05-04 18:21 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> On Wed 04-05-16 11:14:50, Joonsoo Kim wrote:
>> On Tue, May 03, 2016 at 10:53:56AM +0200, Michal Hocko wrote:
>> > On Tue 03-05-16 14:23:04, Joonsoo Kim wrote:
> [...]
>> > > Memory saving looks as following. (Boot 4GB memory system with page_owner)
>> > >
>> > > 92274688 bytes -> 25165824 bytes
>> >
>> > It is not clear to me whether this is after a fresh boot or some workload
>> > which would grow the stack depot as well. What is a usual cap for the
>> > memory consumption.
>>
>> It is static allocation size after a fresh boot. I didn't add size of
>> dynamic allocation memory so it could be larger a little. See below line.
>> >
>> > > 72% reduction in static allocation size. Even if we should add up size of
>> > > dynamic allocation memory, it would not that big because stacktrace is
>> > > mostly duplicated.
>
> This would be true only if most of the allocation stacks are basically
> same after the boot which I am not really convinced is true. But you are
> right that the number of sublicates will grow only a little. I was
> interested about how much is that little ;)

After a fresh boot, it just uses 14 order-2 pages.

>> > > Note that implementation looks complex than someone would imagine because
>> > > there is recursion issue. stackdepot uses page allocator and page_owner
>> > > is called at page allocation. Using stackdepot in page_owner could re-call
>> > > page allcator and then page_owner. That is a recursion.
>> >
>> > This is rather fragile. How do we check there is no lock dependency
>> > introduced later on - e.g. split_page called from a different
>> > locking/reclaim context than alloc_pages? Would it be safer to
>>
>> There is no callsite that calls set_page_owner() with
>> __GFP_DIRECT_RECLAIM. So, there would be no lock/context dependency
>> now.
>
> I am confused now. prep_new_page is called with the gfp_mask of the
> original request, no?

Yes. I assume that set_page_owner() in prep_new_page() is okay. Sorry
for confusion.

>> split_page() doesn't call set_page_owner(). Instead, it calls
>> split_page_owner() and just copies previous entry. Since it doesn't
>> require any new stackdepot entry, it is safe in any context.
>
> Ohh, you are right. I have missed patch 4
> (http://lkml.kernel.org/r/1462252984-8524-5-git-send-email-iamjoonsoo.kim@lge.com)
>
>> > use ~__GFP_DIRECT_RECLAIM for those stack allocations? Or do you think
>> > there would be too many failed allocations? This alone wouldn't remove a
>> > need for the recursion detection but it sounds less tricky.
>> >
>> > > To detect and
>> > > avoid it, whenever we obtain stacktrace, recursion is checked and
>> > > page_owner is set to dummy information if found. Dummy information means
>> > > that this page is allocated for page_owner feature itself
>> > > (such as stackdepot) and it's understandable behavior for user.
>> > >
>> > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>> >
>> > I like the idea in general I just wish this would be less subtle. Few
>> > more comments below.
>> >
>> > [...]
>> > > -void __set_page_owner(struct page *page, unsigned int order, gfp_t gfp_mask)
>> > > +static inline bool check_recursive_alloc(struct stack_trace *trace,
>> > > +                                 unsigned long ip)
>> > >  {
>> > > - struct page_ext *page_ext = lookup_page_ext(page);
>> > > + int i, count;
>> > > +
>> > > + if (!trace->nr_entries)
>> > > +         return false;
>> > > +
>> > > + for (i = 0, count = 0; i < trace->nr_entries; i++) {
>> > > +         if (trace->entries[i] == ip && ++count == 2)
>> > > +                 return true;
>> > > + }
>> >
>> > This would deserve a comment I guess. Btw, don't we have a better and
>> > more robust way to detect the recursion? Per task_struct flag or
>> > something like that?
>>
>> Okay. I will add a comment.
>>
>> I already considered task_struct flag and I know that it is a better
>> solution. But, I don't think that this debugging feature deserve to
>> use such precious flag. This implementation isn't efficient but I
>> think that it is at least robust.
>
> I guess there are many holes in task_structs where a single bool would
> comfortably fit in. But I do not consider this to be a large issue. It
> is just the above looks quite ugly.
>
>> > [...]
>> > > +static noinline depot_stack_handle_t save_stack(gfp_t flags)
>> > > +{
>> > > + unsigned long entries[PAGE_OWNER_STACK_DEPTH];
>> > >   struct stack_trace trace = {
>> > >           .nr_entries = 0,
>> > > -         .max_entries = ARRAY_SIZE(page_ext->trace_entries),
>> > > -         .entries = &page_ext->trace_entries[0],
>> > > -         .skip = 3,
>> > > +         .entries = entries,
>> > > +         .max_entries = PAGE_OWNER_STACK_DEPTH,
>> > > +         .skip = 0
>> > >   };
>> > [...]
>> > >  void __dump_page_owner(struct page *page)
>> > >  {
>> > >   struct page_ext *page_ext = lookup_page_ext(page);
>> > > + unsigned long entries[PAGE_OWNER_STACK_DEPTH];
>> >
>> > This is worrying because of the excessive stack consumption while we
>> > might be in a deep call chain already. Can we preallocate a hash table
>> > for few buffers when the feature is enabled? This would require locking
>> > of course but chances are that contention wouldn't be that large.
>>
>> Make sense but I'm not sure that excessive stack consumption would
>> cause real problem. For example, if direct reclaim is triggered during
>> allocation, it may go more deeper than this path.
>
> Do we really consume 512B of stack during reclaim. That sounds more than
> worrying to me.

Hmm...I checked it by ./script/stackusage and result is as below.

shrink_zone() 128
shrink_zone_memcg() 248
shrink_active_list() 176

We have a call path that shrink_zone() -> shrink_zone_memcg() ->
shrink_active_list().
I'm not sure whether it is the deepest path or not.

>> I'd like to postpone to handle this issue until stack breakage is
>> reported due to this feature.
>
> I dunno, but I would expect that a debugging feature wouldn't cause
> problems like that. It is more than sad when you cannot debug your
> issue just because of the stack consumption...

Okay. I will think more.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1394486 — Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace

2016-05-05 0:30 GMT+09:00 Joonsoo Kim <js1304@gmail.com>:
> 2016-05-04 18:21 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
>> On Wed 04-05-16 11:14:50, Joonsoo Kim wrote:
>>> On Tue, May 03, 2016 at 10:53:56AM +0200, Michal Hocko wrote:
>>> > On Tue 03-05-16 14:23:04, Joonsoo Kim wrote:
>> [...]
>>> > > Memory saving looks as following. (Boot 4GB memory system with page_owner)
>>> > >
>>> > > 92274688 bytes -> 25165824 bytes
>>> >
>>> > It is not clear to me whether this is after a fresh boot or some workload
>>> > which would grow the stack depot as well. What is a usual cap for the
>>> > memory consumption.
>>>
>>> It is static allocation size after a fresh boot. I didn't add size of
>>> dynamic allocation memory so it could be larger a little. See below line.
>>> >
>>> > > 72% reduction in static allocation size. Even if we should add up size of
>>> > > dynamic allocation memory, it would not that big because stacktrace is
>>> > > mostly duplicated.
>>
>> This would be true only if most of the allocation stacks are basically
>> same after the boot which I am not really convinced is true. But you are
>> right that the number of sublicates will grow only a little. I was
>> interested about how much is that little ;)
>
> After a fresh boot, it just uses 14 order-2 pages.

I missed to add other information. Even after building the kernel,
it takes 20 order-2 pages. 20 * 4 * 4KB = 320 KB.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1394680 — Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace

On Thu 05-05-16 00:45:45, Joonsoo Kim wrote:
> 2016-05-05 0:30 GMT+09:00 Joonsoo Kim <js1304@gmail.com>:
> > 2016-05-04 18:21 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> >> On Wed 04-05-16 11:14:50, Joonsoo Kim wrote:
> >>> On Tue, May 03, 2016 at 10:53:56AM +0200, Michal Hocko wrote:
> >>> > On Tue 03-05-16 14:23:04, Joonsoo Kim wrote:
> >> [...]
> >>> > > Memory saving looks as following. (Boot 4GB memory system with page_owner)
> >>> > >
> >>> > > 92274688 bytes -> 25165824 bytes
> >>> >
> >>> > It is not clear to me whether this is after a fresh boot or some workload
> >>> > which would grow the stack depot as well. What is a usual cap for the
> >>> > memory consumption.
> >>>
> >>> It is static allocation size after a fresh boot. I didn't add size of
> >>> dynamic allocation memory so it could be larger a little. See below line.
> >>> >
> >>> > > 72% reduction in static allocation size. Even if we should add up size of
> >>> > > dynamic allocation memory, it would not that big because stacktrace is
> >>> > > mostly duplicated.
> >>
> >> This would be true only if most of the allocation stacks are basically
> >> same after the boot which I am not really convinced is true. But you are
> >> right that the number of sublicates will grow only a little. I was
> >> interested about how much is that little ;)
> >
> > After a fresh boot, it just uses 14 order-2 pages.
> 
> I missed to add other information. Even after building the kernel,
> it takes 20 order-2 pages. 20 * 4 * 4KB = 320 KB.

Something like that would be useful to mention in the changelog because
measuring right after the fresh boot without any reasonable workload
sounds suspicious.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1394677 — Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace

On Thu 05-05-16 00:30:35, Joonsoo Kim wrote:
> 2016-05-04 18:21 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
[...]
> > Do we really consume 512B of stack during reclaim. That sounds more than
> > worrying to me.
> 
> Hmm...I checked it by ./script/stackusage and result is as below.
> 
> shrink_zone() 128
> shrink_zone_memcg() 248
> shrink_active_list() 176
> 
> We have a call path that shrink_zone() -> shrink_zone_memcg() ->
> shrink_active_list().
> I'm not sure whether it is the deepest path or not.

This is definitely not the deepest path. Slab shrinkers can take more
but 512B is still a lot. Some call paths are already too deep when
calling into the allocator and some of them already use GFP_NOFS to
prevent from potentially deep callchain slab shrinkers. Anyway worth
exploring for better solutions.

And I believe it would be better to solve this in the stackdepot
directly so other users do not have to invent their own ways around the
same issue. I have just checked the code and set_track uses save_stack
which does the same thing and it seems to be called from the slab
allocator. I have missed this usage before so the problem already does
exist. It would be unfair to request you to fix that in order to add a
new user. It would be great if this got addressed though.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1397749 — Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace

2016-05-05 4:40 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> On Thu 05-05-16 00:30:35, Joonsoo Kim wrote:
>> 2016-05-04 18:21 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> [...]
>> > Do we really consume 512B of stack during reclaim. That sounds more than
>> > worrying to me.
>>
>> Hmm...I checked it by ./script/stackusage and result is as below.
>>
>> shrink_zone() 128
>> shrink_zone_memcg() 248
>> shrink_active_list() 176
>>
>> We have a call path that shrink_zone() -> shrink_zone_memcg() ->
>> shrink_active_list().
>> I'm not sure whether it is the deepest path or not.
>
> This is definitely not the deepest path. Slab shrinkers can take more
> but 512B is still a lot. Some call paths are already too deep when
> calling into the allocator and some of them already use GFP_NOFS to
> prevent from potentially deep callchain slab shrinkers. Anyway worth
> exploring for better solutions.
>
> And I believe it would be better to solve this in the stackdepot
> directly so other users do not have to invent their own ways around the
> same issue. I have just checked the code and set_track uses save_stack
> which does the same thing and it seems to be called from the slab
> allocator. I have missed this usage before so the problem already does
> exist. It would be unfair to request you to fix that in order to add a
> new user. It would be great if this got addressed though.

Yes, fixing it in stackdepot looks more reasonable.
Then, I will just change PAGE_OWNER_STACK_DEPTH from 64 to 16 and
leave the code as is for now. With this change, we will just consume 128B stack
and would not cause stack problem. If anyone has an objection,
please let me know.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1397868 — Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace

On Tue 10-05-16 16:07:14, Joonsoo Kim wrote:
> 2016-05-05 4:40 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > On Thu 05-05-16 00:30:35, Joonsoo Kim wrote:
> >> 2016-05-04 18:21 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > [...]
> >> > Do we really consume 512B of stack during reclaim. That sounds more than
> >> > worrying to me.
> >>
> >> Hmm...I checked it by ./script/stackusage and result is as below.
> >>
> >> shrink_zone() 128
> >> shrink_zone_memcg() 248
> >> shrink_active_list() 176
> >>
> >> We have a call path that shrink_zone() -> shrink_zone_memcg() ->
> >> shrink_active_list().
> >> I'm not sure whether it is the deepest path or not.
> >
> > This is definitely not the deepest path. Slab shrinkers can take more
> > but 512B is still a lot. Some call paths are already too deep when
> > calling into the allocator and some of them already use GFP_NOFS to
> > prevent from potentially deep callchain slab shrinkers. Anyway worth
> > exploring for better solutions.
> >
> > And I believe it would be better to solve this in the stackdepot
> > directly so other users do not have to invent their own ways around the
> > same issue. I have just checked the code and set_track uses save_stack
> > which does the same thing and it seems to be called from the slab
> > allocator. I have missed this usage before so the problem already does
> > exist. It would be unfair to request you to fix that in order to add a
> > new user. It would be great if this got addressed though.
> 
> Yes, fixing it in stackdepot looks more reasonable.
> Then, I will just change PAGE_OWNER_STACK_DEPTH from 64 to 16 and
> leave the code as is for now. With this change, we will just consume 128B stack
> and would not cause stack problem. If anyone has an objection,
> please let me know.

128B is still quite a lot but considering there is a plan to make it
more robust I can live with it as a temporary workaround.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1399964 — Re: [PATCH 6/6] mm/page_owner: use stackdepot to store stacktrace

On 05/03/2016 07:23 AM, js1304@gmail.com wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>
> Currently, we store each page's allocation stacktrace on corresponding
> page_ext structure and it requires a lot of memory. This causes the problem
> that memory tight system doesn't work well if page_owner is enabled.
> Moreover, even with this large memory consumption, we cannot get full
> stacktrace because we allocate memory at boot time and just maintain
> 8 stacktrace slots to balance memory consumption. We could increase it
> to more but it would make system unusable or change system behaviour.
>
> To solve the problem, this patch uses stackdepot to store stacktrace.

FTR, this sounds useful and I've read your discussion with Michal, so 
I'll wait for the next version.

Thanks

[toc] | [prev] | [next] | [standalone]

#1393045 — [PATCH 3/6] mm/page_owner: copy last_migrate_reason in copy_page_owner()

From: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Currently, copy_page_owner() doesn't copy all the owner information.
It skips last_migrate_reason because copy_page_owner() is used for
migration and it will be properly set soon. But, following patch
will use copy_page_owner() and this skip will cause the problem that
allocated page has uninitialied last_migrate_reason. To prevent it,
this patch also copy last_migrate_reason in copy_page_owner().

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 mm/page_owner.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/page_owner.c b/mm/page_owner.c
index 792b56d..6693959 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -101,6 +101,7 @@ void __copy_page_owner(struct page *oldpage, struct page *newpage)
 
 	new_ext->order = old_ext->order;
 	new_ext->gfp_mask = old_ext->gfp_mask;
+	new_ext->last_migrate_reason = old_ext->last_migrate_reason;
 	new_ext->nr_entries = old_ext->nr_entries;
 
 	for (i = 0; i < ARRAY_SIZE(new_ext->trace_entries); i++)
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1398204 — Re: [PATCH 3/6] mm/page_owner: copy last_migrate_reason in copy_page_owner()

On 05/03/2016 07:23 AM, js1304@gmail.com wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>
> Currently, copy_page_owner() doesn't copy all the owner information.
> It skips last_migrate_reason because copy_page_owner() is used for
> migration and it will be properly set soon. But, following patch
> will use copy_page_owner() and this skip will cause the problem that
> allocated page has uninitialied last_migrate_reason. To prevent it,
> this patch also copy last_migrate_reason in copy_page_owner().

Hmm it's a corner case, but if the "new" page was dumped e.g. due to a 
bug during the migration, is the copied migrate reason from the "old" 
page actually meaningful? I'd say it might be misleading and it's 
simpler to just make sure it's initialized to -1.

> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> ---
>   mm/page_owner.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 792b56d..6693959 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -101,6 +101,7 @@ void __copy_page_owner(struct page *oldpage, struct page *newpage)
>
>   	new_ext->order = old_ext->order;
>   	new_ext->gfp_mask = old_ext->gfp_mask;
> +	new_ext->last_migrate_reason = old_ext->last_migrate_reason;
>   	new_ext->nr_entries = old_ext->nr_entries;
>
>   	for (i = 0; i < ARRAY_SIZE(new_ext->trace_entries); i++)
>

[toc] | [prev] | [next] | [standalone]

#1399652 — Re: [PATCH 3/6] mm/page_owner: copy last_migrate_reason in copy_page_owner()

On Tue, May 10, 2016 at 05:13:12PM +0200, Vlastimil Babka wrote:
> On 05/03/2016 07:23 AM, js1304@gmail.com wrote:
> >From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> >
> >Currently, copy_page_owner() doesn't copy all the owner information.
> >It skips last_migrate_reason because copy_page_owner() is used for
> >migration and it will be properly set soon. But, following patch
> >will use copy_page_owner() and this skip will cause the problem that
> >allocated page has uninitialied last_migrate_reason. To prevent it,
> >this patch also copy last_migrate_reason in copy_page_owner().
> 
> Hmm it's a corner case, but if the "new" page was dumped e.g. due to
> a bug during the migration, is the copied migrate reason from the
> "old" page actually meaningful? I'd say it might be misleading and
> it's simpler to just make sure it's initialized to -1.

Hmm... if it is the case, other fields are also misleading. I think
that we can tolerate this corner case and keeping function semantic as
function name suggests is better practice.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1399689 — Re: [PATCH 3/6] mm/page_owner: copy last_migrate_reason in copy_page_owner()

On 05/12/2016 04:58 AM, Joonsoo Kim wrote:
> On Tue, May 10, 2016 at 05:13:12PM +0200, Vlastimil Babka wrote:
>
> Hmm... if it is the case, other fields are also misleading. I think
> that we can tolerate this corner case and keeping function semantic as
> function name suggests is better practice.

Hmm, OK. Let's not complicate it by specially marking pages that are 
under migration for now.

> Thanks.
>

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web