Re: [PATCH v3] lib: stackdepot: Add support to configure STACK_HASH_SIZE

[Alexander Potapenko]

On Mon, Dec 21, 2020 at 12:15 PM Vijayanand Jitta <vjitta@xxxxxxxxxxxxxx> wrote:
>
>
>
> On 12/18/2020 2:10 PM, Vijayanand Jitta wrote:
> >
> >
> > On 12/17/2020 4:24 PM, Alexander Potapenko wrote:
> >>>> Can you provide an example of a use case in which the user wants to
> >>>> use the stack depot of a smaller size without disabling it completely,
> >>>> and that size cannot be configured statically?
> >>>> As far as I understand, for the page owner example you gave it's
> >>>> sufficient to provide a switch that can disable the stack depot if
> >>>> page_owner=off.
> >>>>
> >>> There are two use cases here,
> >>>
> >>> 1. We don't want to consume memory when page_owner=off ,boolean flag
> >>> would work here.
> >>>
> >>> 2. We would want to enable page_owner on low ram devices but we don't
> >>> want stack depot to consume 8 MB of memory, so for this case we would
> >>> need a configurable stack_hash_size so that we can still use page_owner
> >>> with lower memory consumption.
> >>>
> >>> So, a configurable stack_hash_size would work for both these use cases,
> >>> we can set it to '0' for first case and set the required size for the
> >>> second case.
> >>
> >> Will a combined solution with a boolean boot-time flag and a static
> >> CONFIG_STACKDEPOT_HASH_SIZE work for these cases?
> >> I suppose low-memory devices have a separate kernel config anyway?
> >>
> >
> > Yes, the combined solution will also work but i think having a single
> > run time config is simpler instead of having two things to configure.
> >
>
> To add to it we started of with a CONFIG first, after the comments from
> Minchan (https://lkml.org/lkml/2020/11/3/2121) we decided to switch to
> run time param.
>
> Quoting Minchan's comments below:
>
> "
> 1. When we don't use page_owner, we don't want to waste any memory for
> stackdepot hash array.
> 2. When we use page_owner, we want to have reasonable stackdeport hash array
>
> With this configuration, it couldn't meet since we always need to
> reserve a reasonable size for the array.
> Can't we make the hash size as a kernel parameter?
> With it, we could use it like this.
>
> 1. page_owner=off, stackdepot_stack_hash=0 -> no more wasted memory
> when we don't use page_owner
> 2. page_owner=on, stackdepot_stack_hash=8M -> reasonable hash size
> when we use page_owner.
> "

Minchan, what do you think about making the hash size itself a static
parameter, while letting the user disable stackdepot completely at
runtime?
As noted before, I am concerned that moving a low-level configuration
bit (which essentially means "save 8Mb - (1 << stackdepot_stack_hash)
of static memory") to the boot parameters will be unused by most
admins and may actually trick them into thinking they reduce the
overall stackdepot memory consumption noticeably.
I also suppose device vendors may prefer setting a fixed (maybe
non-default) hash size for low-memory devices rather than letting the
admins increase it.


Alex

PS. Sorry for being late to the party, I should have probably spoken
up in November, when you've been discussing the first version of this
patch.

> Thanks,
> Vijay
> >> My concern is that exposing yet another knob to users won't really
> >> solve their problems, because the hash size alone doesn't give enough
> >> control over stackdepot memory footprint (we also have stack_slabs,
> >> which may get way bigger than 8Mb).
> >>
> >
> > True, stack_slabs can consume more memory but they consume most only
> > when stack depot is used as they are allocated in stack_depot_save path.
> > when stack depot is not used they consume 8192 * sizeof(void) bytes at
> > max. So nothing much we can do here since static allocation is not much
> > and memory consumption depends up on stack depot usage, unlike
> > stack_hash_table where 8mb is preallocated.
> >
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member of Code Aurora Forum, hosted by The Linux Foundation



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg