From: Yogesh Lal <[email protected]>
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.
Suggested-by: Minchan Kim <[email protected]>
Signed-off-by: Yogesh Lal <[email protected]>
Signed-off-by: Vijayanand Jitta <[email protected]>
---
lib/stackdepot.c | 31 +++++++++++++++++++++++++++----
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index 81c69c0..e0eebfd 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -30,6 +30,7 @@
#include <linux/stackdepot.h>
#include <linux/string.h>
#include <linux/types.h>
+#include <linux/vmalloc.h>
#define DEPOT_STACK_BITS (sizeof(depot_stack_handle_t) * 8)
@@ -141,14 +142,36 @@ static struct stack_record *depot_alloc_stack(unsigned long *entries, int size,
return stack;
}
-#define STACK_HASH_ORDER 20
-#define STACK_HASH_SIZE (1L << STACK_HASH_ORDER)
+#define MAX_STACK_HASH_ORDER 20
+#define MAX_STACK_HASH_SIZE (1L << MAX_STACK_HASH_ORDER)
+#define STACK_HASH_SIZE (1L << stack_hash_order)
#define STACK_HASH_MASK (STACK_HASH_SIZE - 1)
#define STACK_HASH_SEED 0x9747b28c
-static struct stack_record *stack_table[STACK_HASH_SIZE] = {
- [0 ... STACK_HASH_SIZE - 1] = NULL
+static unsigned int stack_hash_order = 20;
+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);
+ return 0;
+}
+early_initcall(init_stackdepot);
/* Calculate hash for a stack */
static inline u32 hash_stack(unsigned long *entries, unsigned int size)
--
2.7.4
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On Fri, Dec 11, 2020 at 1:45 PM Vijayanand Jitta <[email protected]> wrote:
>
>
>
> On 12/11/2020 2:06 PM, Alexander Potapenko wrote:
> > On Thu, Dec 10, 2020 at 6:01 AM <[email protected]> wrote:
> >>
> >> From: Yogesh Lal <[email protected]>
> >>
> >> 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()?
> Thanks,
> Vijay
>
> --
> 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
On Thu, Dec 10, 2020 at 6:01 AM <[email protected]> wrote:
>
> From: Yogesh Lal <[email protected]>
>
> 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.
> -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.
> +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
On 12/11/2020 2:06 PM, Alexander Potapenko wrote:
> On Thu, Dec 10, 2020 at 6:01 AM <[email protected]> wrote:
>>
>> From: Yogesh Lal <[email protected]>
>>
>> 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
On 12/11/2020 6:55 PM, Alexander Potapenko wrote:
> On Fri, Dec 11, 2020 at 1:45 PM Vijayanand Jitta <[email protected]> wrote:
>>
>>
>>
>> On 12/11/2020 2:06 PM, Alexander Potapenko wrote:
>>> On Thu, Dec 10, 2020 at 6:01 AM <[email protected]> wrote:
>>>>
>>>> From: Yogesh Lal <[email protected]>
>>>>
>>>> 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
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
On 12/14/2020 3:04 PM, Alexander Potapenko wrote:
> On Mon, Dec 14, 2020 at 5:02 AM Vijayanand Jitta <[email protected]> wrote:
>>
>>
>>
>> On 12/11/2020 6:55 PM, Alexander Potapenko wrote:
>>> On Fri, Dec 11, 2020 at 1:45 PM Vijayanand Jitta <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>> On 12/11/2020 2:06 PM, Alexander Potapenko wrote:
>>>>> On Thu, Dec 10, 2020 at 6:01 AM <[email protected]> wrote:
>>>>>>
>>>>>> From: Yogesh Lal <[email protected]>
>>>>>>
>>>>>> 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
On Mon, Dec 14, 2020 at 5:02 AM Vijayanand Jitta <[email protected]> wrote:
>
>
>
> On 12/11/2020 6:55 PM, Alexander Potapenko wrote:
> > On Fri, Dec 11, 2020 at 1:45 PM Vijayanand Jitta <[email protected]> wrote:
> >>
> >>
> >>
> >> On 12/11/2020 2:06 PM, Alexander Potapenko wrote:
> >>> On Thu, Dec 10, 2020 at 6:01 AM <[email protected]> wrote:
> >>>>
> >>>> From: Yogesh Lal <[email protected]>
> >>>>
> >>>> 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.
> 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
--
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
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: 1333d0ba67aa139cd33d20039e3a1dd9c79ec546 ("[PATCH v3] lib: stackdepot: Add support to configure STACK_HASH_SIZE")
url: https://github.com/0day-ci/linux/commits/vjitta-codeaurora-org/lib-stackdepot-Add-support-to-configure-STACK_HASH_SIZE/20201210-130554
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git a2f5ea9e314ba6778f885c805c921e9362ec0420
in testcase: locktorture
version:
with following parameters:
runtime: 300s
test: default
test-description: This torture test consists of creating a number of kernel threads which acquire the lock and hold it for specific amount of time, thus simulating different critical region behaviors.
test-url: https://www.kernel.org/doc/Documentation/locking/locktorture.txt
on test machine: qemu-system-i386 -enable-kvm -cpu SandyBridge -smp 2 -m 8G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+-----------------------------------------------------------+------------+------------+
| | a2f5ea9e31 | 1333d0ba67 |
+-----------------------------------------------------------+------------+------------+
| boot_successes | 4 | 15 |
| boot_failures | 0 | 10 |
| WARNING:at_kernel/locking/lockdep.c:#lockdep_register_key | 0 | 10 |
| EIP:lockdep_register_key | 0 | 10 |
+-----------------------------------------------------------+------------+------------+
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 9.506645] WARNING: CPU: 1 PID: 193 at kernel/locking/lockdep.c:1177 lockdep_register_key+0x178/0x210
[ 9.507533] Modules linked in: bochs_drm(+) drm_vram_helper drm_ttm_helper ttm crc32_pclmul drm_kms_helper rapl snd_pcm drm snd_timer drm_panel_orientation_quirks fb snd fbdev i2c_piix4 i2c_core piix(+) qemu_fw_cfg
[ 9.509376] CPU: 1 PID: 193 Comm: udevd Not tainted 5.10.0-rc7-00035-g1333d0ba67aa #1
[ 9.510125] EIP: lockdep_register_key+0x178/0x210
[ 9.510589] Code: b5 00 20 b0 42 85 c0 0f 84 35 ff ff ff 89 58 04 8b 1d 6c 72 be 42 85 db 0f 85 5f ff ff ff e9 29 ff ff ff 8d b4 26 00 00 00 00 <0f> 0b 8d 65 f4 5b 5e 5f 5d c3 8d b6 00 00 00 00 89 c2 b8 08 70 be
[ 9.512369] EAX: 00000001 EBX: 42782b48 ECX: 00000003 EDX: 41dff601
[ 9.512966] ESI: 42782b48 EDI: 42782b6c EBP: 42515c48 ESP: 42515c38
[ 9.513565] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068 EFLAGS: 00010202
[ 9.514245] CR0: 80050033 CR2: 3fda9000 CR3: 024fc9a0 CR4: 000406f0
[ 9.514843] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 9.515462] DR6: fffe0ff0 DR7: 00000400
[ 9.515831] Call Trace:
[ 9.516079] ? alloc_workqueue+0x13f/0x3b0
[ 9.516489] ? ttm_mem_global_init+0x35/0x2e0 [ttm]
[ 9.516961] ? slob_alloc+0x1dd/0x1f0
[ 9.517382] ? ttm_bo_device_init+0x15a/0x290 [ttm]
[ 9.517857] ? drm_vram_helper_alloc_mm+0x5f/0xe0 [drm_vram_helper]
[ 9.518464] ? bochs_mm_init+0x16/0x30 [bochs_drm]
[ 9.518920] ? bochs_pci_probe+0xef/0x140 [bochs_drm]
[ 9.519506] ? pci_device_probe+0xcc/0x140
[ 9.519901] ? really_probe+0x1bd/0x3f0
[ 9.520274] ? driver_probe_device+0x55/0x180
[ 9.520694] ? mutex_lock_nested+0x14/0x20
[ 9.521096] ? __device_driver_lock+0x1d/0x40
[ 9.521515] ? device_driver_attach+0x49/0x50
[ 9.521942] ? __driver_attach+0x89/0x130
[ 9.522323] ? device_driver_attach+0x50/0x50
[ 9.522737] ? bus_for_each_dev+0x4c/0x80
[ 9.523126] ? driver_attach+0x14/0x20
[ 9.523506] ? device_driver_attach+0x50/0x50
[ 9.523964] ? bus_add_driver+0x14f/0x1c0
[ 9.524353] ? pci_bus_num_vf+0x10/0x10
[ 9.524727] ? driver_register+0x61/0xb0
[ 9.525108] ? 0xdfbab000
[ 9.525363] ? __pci_register_driver+0x4d/0x60
[ 9.525796] ? bochs_init+0x20/0x1000 [bochs_drm]
[ 9.526245] ? do_one_initcall+0x54/0x230
[ 9.526636] ? slob_alloc+0x90/0x1f0
[ 9.527031] ? __kmalloc+0x62/0x1a0
[ 9.527392] ? do_init_module+0x1a/0x3f0
[ 9.527777] ? do_init_module+0x49/0x3f0
[ 9.528157] ? __vfree+0x1d/0x50
[ 9.528480] ? load_module+0x1098/0x1260
[ 9.528860] ? __ia32_sys_finit_module+0x89/0xd0
[ 9.529290] ? do_int80_syscall_32+0x2c/0x40
[ 9.529701] ? entry_INT80_32+0xed/0xed
[ 9.530078] ---[ end trace 735bd35c911b2630 ]---
To reproduce:
# build kernel
cd linux
cp config-5.10.0-rc7-00035-g1333d0ba67aa .config
make HOSTCC=gcc-9 CC=gcc-9 ARCH=i386 olddefconfig prepare modules_prepare bzImage modules
make HOSTCC=gcc-9 CC=gcc-9 ARCH=i386 INSTALL_MOD_PATH=<mod-install-dir> modules_install
cd <mod-install-dir>
find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
Thanks,
Rong Chen
On 12/16/2020 7:04 PM, Alexander Potapenko wrote:
>>> 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?
>>
>> There are multiple users of stackdepot they might still want to use
>> stack depot but with a lower memory footprint instead of MAX_SIZE
>> so, a configurable size might help here ?
>
> 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.
>>> 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?
>>>
>>
>> Since, page owner is not the only user of stack depot we can't take that
>> decision of disabling stack depot if page owner is disabled.
>
> Agreed, but if multiple subsystems want to use stackdepot together, it
> is even harder to estimate the total memory consumption.
> How likely is it that none of them will need MAX_SIZE?
>
>>>> 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
>>>
>>>
>>>
>>
>> --
>> 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
On Thu, Dec 17, 2020 at 6:42 AM Vijayanand Jitta <[email protected]> wrote:
>
> On 12/16/2020 7:04 PM, Alexander Potapenko wrote:
> >>> 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?
> >>
> >> There are multiple users of stackdepot they might still want to use
> >> stack depot but with a lower memory footprint instead of MAX_SIZE
> >> so, a configurable size might help here ?
> >
> > 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.
>
> >>> 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?
> >>>
> >>
> >> Since, page owner is not the only user of stack depot we can't take that
> >> decision of disabling stack depot if page owner is disabled.
> >
> > Agreed, but if multiple subsystems want to use stackdepot together, it
> > is even harder to estimate the total memory consumption.
> > How likely is it that none of them will need MAX_SIZE?
> >
> >>>> Minchan,
> >>>> This should be fine right ? Do you see any issue with disabling
> >>>> stack depot completely ?
+kasan-dev
On Thu, Dec 10, 2020 at 6:04 AM <[email protected]> wrote:
>
> From: Yogesh Lal <[email protected]>
>
> 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.
>
> Suggested-by: Minchan Kim <[email protected]>
> Signed-off-by: Yogesh Lal <[email protected]>
> Signed-off-by: Vijayanand Jitta <[email protected]>
> ---
> lib/stackdepot.c | 31 +++++++++++++++++++++++++++----
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 81c69c0..e0eebfd 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -30,6 +30,7 @@
> #include <linux/stackdepot.h>
> #include <linux/string.h>
> #include <linux/types.h>
> +#include <linux/vmalloc.h>
>
> #define DEPOT_STACK_BITS (sizeof(depot_stack_handle_t) * 8)
>
> @@ -141,14 +142,36 @@ static struct stack_record *depot_alloc_stack(unsigned long *entries, int size,
> return stack;
> }
>
> -#define STACK_HASH_ORDER 20
> -#define STACK_HASH_SIZE (1L << STACK_HASH_ORDER)
> +#define MAX_STACK_HASH_ORDER 20
> +#define MAX_STACK_HASH_SIZE (1L << MAX_STACK_HASH_ORDER)
> +#define STACK_HASH_SIZE (1L << stack_hash_order)
> #define STACK_HASH_MASK (STACK_HASH_SIZE - 1)
> #define STACK_HASH_SEED 0x9747b28c
>
> -static struct stack_record *stack_table[STACK_HASH_SIZE] = {
> - [0 ... STACK_HASH_SIZE - 1] = NULL
> +static unsigned int stack_hash_order = 20;
> +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);
Can interrupts happen at this point in time? If yes, they can
use/modify stack_table_def concurrently.
> + return 0;
> +}
> +early_initcall(init_stackdepot);
>
> /* Calculate hash for a stack */
> static inline u32 hash_stack(unsigned long *entries, unsigned int size)
> --
> 2.7.4
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On Thu, Dec 17, 2020 at 11:25 AM Dmitry Vyukov <[email protected]> wrote:
>
> On Thu, Dec 10, 2020 at 6:04 AM <[email protected]> wrote:
> >
> > From: Yogesh Lal <[email protected]>
> >
> > 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.
> >
> > Suggested-by: Minchan Kim <[email protected]>
> > Signed-off-by: Yogesh Lal <[email protected]>
> > Signed-off-by: Vijayanand Jitta <[email protected]>
> > ---
> > lib/stackdepot.c | 31 +++++++++++++++++++++++++++----
> > 1 file changed, 27 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> > index 81c69c0..e0eebfd 100644
> > --- a/lib/stackdepot.c
> > +++ b/lib/stackdepot.c
> > @@ -30,6 +30,7 @@
> > #include <linux/stackdepot.h>
> > #include <linux/string.h>
> > #include <linux/types.h>
> > +#include <linux/vmalloc.h>
> >
> > #define DEPOT_STACK_BITS (sizeof(depot_stack_handle_t) * 8)
> >
> > @@ -141,14 +142,36 @@ static struct stack_record *depot_alloc_stack(unsigned long *entries, int size,
> > return stack;
> > }
> >
> > -#define STACK_HASH_ORDER 20
> > -#define STACK_HASH_SIZE (1L << STACK_HASH_ORDER)
> > +#define MAX_STACK_HASH_ORDER 20
> > +#define MAX_STACK_HASH_SIZE (1L << MAX_STACK_HASH_ORDER)
> > +#define STACK_HASH_SIZE (1L << stack_hash_order)
> > #define STACK_HASH_MASK (STACK_HASH_SIZE - 1)
> > #define STACK_HASH_SEED 0x9747b28c
> >
> > -static struct stack_record *stack_table[STACK_HASH_SIZE] = {
> > - [0 ... STACK_HASH_SIZE - 1] = NULL
> > +static unsigned int stack_hash_order = 20;
> > +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)
Can interrupts happen here?
> > + 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);
>
> Can interrupts happen at this point in time? If yes, they can
> use/modify stack_table_def concurrently.
>
> > + return 0;
> > +}
> > +early_initcall(init_stackdepot);
> >
> > /* Calculate hash for a stack */
> > static inline u32 hash_stack(unsigned long *entries, unsigned int size)
> > --
> > 2.7.4
> > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> > 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?
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).
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.
> 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
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.
"
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
On Mon, Dec 21, 2020 at 12:15 PM Vijayanand Jitta <[email protected]> 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
On Mon, Dec 21, 2020 at 04:04:09PM +0100, Alexander Potapenko wrote:
> On Mon, Dec 21, 2020 at 12:15 PM Vijayanand Jitta <[email protected]> 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?
On 12/22/2020 1:59 AM, Minchan Kim wrote:
> On Mon, Dec 21, 2020 at 04:04:09PM +0100, Alexander Potapenko wrote:
>> On Mon, Dec 21, 2020 at 12:15 PM Vijayanand Jitta <[email protected]> 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?
>
Michan, We would still need config option so that we can reduce the
memory consumption on low ram devices using config.
Alex, On this,
"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 see kernel param swiotlb does similar thing i.e; '0' to disable and
set a value to configure size.
I am fine with either of the approaches,
1. I can split this patch into two
i) A bool variable to enable/disable stack depot.
ii) A config for the size.
(or)
2. A run time param - '0' to disable and set a valid size to enable.
Let me know your comments.
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation
>
> Michan, We would still need config option so that we can reduce the
> memory consumption on low ram devices using config.
>
> Alex, On this,
> "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 see kernel param swiotlb does similar thing i.e; '0' to disable and
> set a value to configure size.
>
> I am fine with either of the approaches,
>
> 1. I can split this patch into two
> i) A bool variable to enable/disable stack depot.
> ii) A config for the size.
I still believe this is a more appropriate solution.
Thanks in advance!
On 12/23/2020 8:10 PM, Alexander Potapenko wrote:
>>
>> Michan, We would still need config option so that we can reduce the
>> memory consumption on low ram devices using config.
>>
>> Alex, On this,
>> "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 see kernel param swiotlb does similar thing i.e; '0' to disable and
>> set a value to configure size.
>>
>> I am fine with either of the approaches,
>>
>> 1. I can split this patch into two
>> i) A bool variable to enable/disable stack depot.
>> ii) A config for the size.
>
> I still believe this is a more appropriate solution.
>
> Thanks in advance!
>
Thanks, Will work on a patch with above approach.
Thanks,
Vijay
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation