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

[Minchan Kim]

On Mon, Dec 21, 2020 at 04:04:09PM +0100, Alexander Potapenko wrote:
> 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.

I am totally fine if we could save the static memory alloation when
the page_owner is not used.

IOW, page_owner=disable, stackdepot=disable will not consume the 8M
memory.
When we want to use page_owner, we could just do like this

	page_owner=enable, stackdepot=enable

(Maybe we need something to make warning if stackdepot is disabled
but someone want to use it, for example, KASAN?)

Vijayanand, If we could work this this, should we still need the
config option, then?