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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 6 Jun 2016 15:56:04 +0200
From:	Michal Hocko <mhocko@...nel.org>
To:	js1304@...il.com
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Vlastimil Babka <vbabka@...e.cz>, mgorman@...hsingularity.net,
	Minchan Kim <minchan@...nel.org>,
	Alexander Potapenko <glider@...gle.com>,
	Hugh Dickins <hughd@...gle.com>, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH v2 6/7] mm/page_owner: use stackdepot to store stacktrace

On Thu 26-05-16 11:37:54, Joonsoo Kim wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@....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?

> dynamic allocation after kernel build:
> 0 bytes -> 327680 bytes

And this is the additional dynamic allocation after the kernel build.

> 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@....com>

Other than the small remark below I haven't spotted anything wrong and
I like the approach.

Acked-by: Michal Hocko <mhocko@...e.com>

> ---
>  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)
-- 
Michal Hocko
SUSE Labs