Re: [PATCH v2 6/7] mm/page_owner: use stackdepot to store stacktrace

[Joonsoo Kim <iamjoonsoo.kim@lge.com>]

On Mon, Jun 06, 2016 at 03:56:04PM +0200, Michal Hocko wrote:
> On Thu 26-05-16 11:37:54, 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. (4GB memory system with page_owner)
> 
> I still have troubles to understand your numbers
> 
> > static allocation:
> > 92274688 bytes -> 25165824 bytes
> 
> I assume that the first numbers refers to the static allocation for the
> given amount of memory while the second one is the dynamic after the
> boot, right?

No, first number refers to the static allocation before the patch and
second one is for after the patch.

> 
> > dynamic allocation after kernel build:
> > 0 bytes -> 327680 bytes
> 
> And this is the additional dynamic allocation after the kernel build.

This is the additional dynamic allocation after booting + the kernel
build. (before the patch -> after the patch)

> > total:
> > 92274688 bytes -> 25493504 bytes
> > 
> > 72% reduction in total.
> > 
> > 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.
> > 
> > v2:
> > o calculate memory saving with including dynamic allocation
> > after kernel build
> > o change maximum stacktrace entry size due to possible stack overflow
> > 
> > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> Other than the small remark below I haven't spotted anything wrong and
> I like the approach.
> 
> Acked-by: Michal Hocko <mhocko@suse.com>

Thanks.

> > ---
> >  include/linux/page_ext.h |   4 +-
> >  lib/Kconfig.debug        |   1 +
> >  mm/page_owner.c          | 138 ++++++++++++++++++++++++++++++++++++++++-------
> >  3 files changed, 122 insertions(+), 21 deletions(-)
> > 
> [...]
> > @@ -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"
> >  
> 
> This is still 128B of the stack which is a lot in the allocation paths
> so can we add something like
> 
> /*
>  * TODO: teach PAGE_OWNER_STACK_DEPTH (__dump_page_owner and save_stack)
>  * to use off stack temporal storage
>  */
> > +#define PAGE_OWNER_STACK_DEPTH (16)

Will add.

Thanks.