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

[Alexander Potapenko]

On Wed, Dec 16, 2020 at 2:06 PM Vijayanand Jitta <vjitta@xxxxxxxxxxxxxx> wrote:
>
>
>
> On 12/16/2020 1:56 PM, Alexander Potapenko wrote:
> > On Wed, Dec 16, 2020 at 4:43 AM Vijayanand Jitta <vjitta@xxxxxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 12/14/2020 4:02 PM, Vijayanand Jitta wrote:
> >>>
> >>>
> >>> On 12/14/2020 3:04 PM, Alexander Potapenko wrote:
> >>>> On Mon, Dec 14, 2020 at 5:02 AM Vijayanand Jitta <vjitta@xxxxxxxxxxxxxx> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 12/11/2020 6:55 PM, Alexander Potapenko wrote:
> >>>>>> On Fri, Dec 11, 2020 at 1:45 PM Vijayanand Jitta <vjitta@xxxxxxxxxxxxxx> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On 12/11/2020 2:06 PM, Alexander Potapenko wrote:
> >>>>>>>> On Thu, Dec 10, 2020 at 6:01 AM <vjitta@xxxxxxxxxxxxxx> wrote:
> >>>>>>>>>
> >>>>>>>>> From: Yogesh Lal <ylal@xxxxxxxxxxxxxx>
> >>>>>>>>>
> >>>>>>>>> Add a kernel parameter stack_hash_order to configure STACK_HASH_SIZE.
> >>>>>>>>>
> >>>>>>>>> Aim is to have configurable value for STACK_HASH_SIZE, so that one
> >>>>>>>>> can configure it depending on usecase there by reducing the static
> >>>>>>>>> memory overhead.
> >>>>>>>>>
> >>>>>>>>> One example is of Page Owner, default value of STACK_HASH_SIZE lead
> >>>>>>>>> stack depot to consume 8MB of static memory. Making it configurable
> >>>>>>>>> and use lower value helps to enable features like CONFIG_PAGE_OWNER
> >>>>>>>>> without any significant overhead.
> >>>>>>>>
> >>>>>>>> Can we go with a static CONFIG_ parameter instead?
> >>>>>>>> Guess most users won't bother changing the default anyway, and for
> >>>>>>>> CONFIG_PAGE_OWNER users changing the size at boot time is not strictly
> >>>>>>>> needed.
> >>>>>>>>
> >>>>>>> Thanks for review.
> >>>>>>>
> >>>>>>> One advantage of having run time parameter is we can simply set it to a
> >>>>>>> lower value at runtime if page_owner=off thereby reducing the memory
> >>>>>>> usage or use default value if we want to use page owner so, we have some
> >>>>>>> some flexibility here. This is not possible with static parameter as we
> >>>>>>> have to have some predefined value.
> >>>>>>
> >>>>>> If we are talking about a configuration in which page_owner is the
> >>>>>> only stackdepot user in the system, then for page_owner=off it
> >>>>>> probably makes more sense to disable stackdepot completely instead of
> >>>>>> setting it to a lower value. This is a lot easier to do in terms of
> >>>>>> correctness.
> >>>>>> But if there are other users (e.g. KASAN), their stackdepot usage may
> >>>>>> actually dominate that of page_owner.
> >>>>>>
> >>>>>>>>> -static struct stack_record *stack_table[STACK_HASH_SIZE] = {
> >>>>>>>>> -       [0 ...  STACK_HASH_SIZE - 1] = NULL
> >>>>>>>>> +static unsigned int stack_hash_order = 20;
> >>>>>>>>
> >>>>>>>> Please initialize with MAX_STACK_HASH_ORDER instead.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Sure, will update this.
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>>>>> +
> >>>>>>>>> +static int __init init_stackdepot(void)
> >>>>>>>>> +{
> >>>>>>>>> +       size_t size = (STACK_HASH_SIZE * sizeof(struct stack_record *));
> >>>>>>>>> +
> >>>>>>>>> +       stack_table = vmalloc(size);
> >>>>>>>>> +       memcpy(stack_table, stack_table_def, size);
> >>>>>>>>
> >>>>>>>> Looks like you are assuming stack_table_def already contains some data
> >>>>>>>> by this point.
> >>>>>>>> But if STACK_HASH_SIZE shrinks this memcpy() above will just copy some
> >>>>>>>> part of the table, whereas the rest will be lost.
> >>>>>>>> We'll need to:
> >>>>>>>> - either explicitly decide we can afford losing this data (no idea how
> >>>>>>>> bad this can potentially be),
> >>>>>>>> - or disallow storing anything prior to full stackdepot initialization
> >>>>>>>> (then we don't need stack_table_def),
> >>>>>>>> - or carefully move all entries to the first part of the table.
> >>>>>>>>
> >>>>>>>> Alex
> >>>>>>>>
> >>>>>>>
> >>>>>>> The hash for stack_table_def is computed using the run time parameter
> >>>>>>> stack_hash_order, though stack_table_def is a bigger array it will only
> >>>>>>> use the entries that are with in the run time configured STACK_HASH_SIZE
> >>>>>>> range. so, there will be no data loss during copy.
> >>>>>>
> >>>>>> Do we expect any data to be stored into stack_table_def before
> >>>>>> setup_stack_hash_order() is called?
> >>>>>> If the answer is no, then we could probably drop stack_table_def and
> >>>>>> allocate the table right in setup_stack_hash_order()?
> >>>>>>
> >>>>>
> >>>>> Yes, we do see an allocation from stack depot even before init is called
> >>>>> from kasan, thats the reason for having stack_table_def.
> >>>>> This is the issue reported due to that on v2, so i added stack_table_def.
> >>>>> https://lkml.org/lkml/2020/12/3/839
> >>>>
> >>>> But at that point STACK_HASH_SIZE is still equal to 1L <<
> >>>> MAX_STACK_HASH_ORDER, isn't it?
> >>>> Then we still need to take care of the records that fit into the
> >>>> bigger array, but not the smaller one.
> >>>>
> >>>
> >>> At this point early_param is already called which sets stack_hash_order.
> >>> So, STACK_HASH_SIZE will be set to this updated value and not
> >>> MAX_STACK_HASH_SIZE.So, no additional entires in the bigger array.
> >>>
> >>> Thanks,
> >>> Vijay
> >>>
> >>
> >> Let me know if there are any other concerns.
> >
> > I still think there are implicit assumptions that should at least be
> > written down in the comments.
>
> Sure, will add the comments.
>
> > As far as I understand the code, here is what happens here:
> >
> > 0. No stacks are recorded.
> > 1. early_param is called to set stack_hash_order to a value
> > potentially smaller than MAX_STACK_HASH_SIZE.
> > 2. KASAN (or other users) records some stacks into stack_table_def
> > (capped at new STACK_HASH_SIZE)
> > 3. init_stackdepot() allocates a new stack_table and copies the
> > contents of stack_table_def into it
> > 4. Further stacks are recorded into the new stack_table
> >
> > If this is how the things work, I agree we don't need to account for
> > the part of stack_table_def past STACK_HASH_SIZE.
> > Not allocating stack_table when setting stack_hash_order is probably
> > also justified, as we don't have SLAB or vmalloc at that point.
> >
>
> That's Right.
>
> > But I am still curious if a runtime parameter that disables the
> > stackdepot completely will solve your problem.
> > Allocating a small amount of memory when you actually don't want to
> > allocate any sounds suboptimal.
> >
>
> I think disabling stack depot completely should be fine, we can make
> STACK_HASH_SIZE as runtime parameter instead of stack_hash_order and set
> stack_hash_size to 0 to disable stack depot completely.

To reiterate, I think you don't need a tunable stack_hash_order
parameter if the only use case is to disable the stack depot.
Maybe it is enough to just add a boolean flag?
Or even go further and disable the stack depot in the same place that
disables page owner, as the user probably doesn't want to set two
flags instead of one?

> Minchan,
> This should be fine right ? Do you see any issue with disabling
> stack depot completely ?
>
> Thanks,
> Vijay
>
> >> Thanks,
> >> Vijay
> >>
> >>>>> Thanks,
> >>>>> Vijay
> >>>>>
> >>>>>>> Thanks,
> >>>>>>> Vijay
> >>>>>>>
> >>>>>>> --
> >>>>>>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> >>>>>>> member of Code Aurora Forum, hosted by The Linux Foundation
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> --
> >>>>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> >>>>> member of Code Aurora Forum, hosted by The Linux Foundation
> >>>>
> >>>>
> >>>>
> >>>
> >>
> >> --
> >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> >> member of Code Aurora Forum, hosted by The Linux Foundation
> >
> >
> >
>
> --
> 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