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

[Vijayanand Jitta]

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.

>> -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 struct stack_record *stack_table_def[MAX_STACK_HASH_SIZE] __initdata = {
>> +       [0 ...  MAX_STACK_HASH_SIZE - 1] = NULL
>>  };
>> +static struct stack_record **stack_table __refdata = stack_table_def;
>> +
>> +static int __init setup_stack_hash_order(char *str)
>> +{
>> +       kstrtouint(str, 0, &stack_hash_order);
>> +       if (stack_hash_order > MAX_STACK_HASH_ORDER)
>> +               stack_hash_order = MAX_STACK_HASH_ORDER;
>> +       return 0;
>> +}
>> +early_param("stack_hash_order", setup_stack_hash_order);
>> +
>> +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.

Thanks,
Vijay

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation