Messages in this thread | | | Subject | Re: [PATCH v3] lib: stackdepot: Add support to configure STACK_HASH_SIZE | From | Vijayanand Jitta <> | Date | Mon, 14 Dec 2020 16:02:35 +0530 |
| |
On 12/14/2020 3:04 PM, Alexander Potapenko wrote: > On Mon, Dec 14, 2020 at 5:02 AM Vijayanand Jitta <vjitta@codeaurora.org> wrote: >> >> >> >> On 12/11/2020 6:55 PM, Alexander Potapenko wrote: >>> On Fri, Dec 11, 2020 at 1:45 PM Vijayanand Jitta <vjitta@codeaurora.org> wrote: >>>> >>>> >>>> >>>> On 12/11/2020 2:06 PM, Alexander Potapenko wrote: >>>>> On Thu, Dec 10, 2020 at 6:01 AM <vjitta@codeaurora.org> wrote: >>>>>> >>>>>> From: Yogesh Lal <ylal@codeaurora.org> >>>>>> >>>>>> 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
>> 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
| |