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

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

[Minchan Kim]

Sorry if this mail corrupts the mail thread or had heavy mangling
since I lost this mail from my mailbox so I am sending this mail by
web gmail.

On Thu, Oct 22, 2020 at 10:18 AM <vjitta@codeaurora.org> wrote:
>
> From: Yogesh Lal <ylal@codeaurora.org>
>
> Use STACK_HASH_ORDER_SHIFT to configure STACK_HASH_SIZE.
>
> Aim is to have configurable value for  STACK_HASH_SIZE,
> so depend on use case one can configure it.
>
> 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.
>
> Signed-off-by: Yogesh Lal <ylal@codeaurora.org>
> Signed-off-by: Vinayak Menon <vinmenon@codeaurora.org>
> Signed-off-by: Vijayanand Jitta <vjitta@codeaurora.org>
> ---
>  lib/Kconfig      | 9 +++++++++
>  lib/stackdepot.c | 3 +--
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 18d76b6..b3f8259 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -651,6 +651,15 @@ config STACKDEPOT
>         bool
>         select STACKTRACE
>
> +config STACK_HASH_ORDER_SHIFT
> +       int "stack depot hash size (12 => 4KB, 20 => 1024KB)"
> +       range 12 20
> +       default 20
> +       depends on STACKDEPOT
> +       help
> +        Select the hash size as a power of 2 for the stackdepot hash table.
> +        Choose a lower value to reduce the memory impact.
> +
>  config SBITMAP
>         bool
>
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 2caffc6..413c20b 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -142,8 +142,7 @@ 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 STACK_HASH_SIZE (1L << CONFIG_STACK_HASH_ORDER_SHIFT)
>  #define STACK_HASH_MASK (STACK_HASH_SIZE - 1)
>  #define STACK_HASH_SEED 0x9747b28c
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> 2.7.4
>

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.


-- 
Kind regards,
Minchan Kim

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

[Vijayanand Jitta]

On 11/4/2020 4:57 AM, Minchan Kim wrote:
> Sorry if this mail corrupts the mail thread or had heavy mangling
> since I lost this mail from my mailbox so I am sending this mail by
> web gmail.
> 
> On Thu, Oct 22, 2020 at 10:18 AM <vjitta@codeaurora.org> wrote:
>>
>> From: Yogesh Lal <ylal@codeaurora.org>
>>
>> Use STACK_HASH_ORDER_SHIFT to configure STACK_HASH_SIZE.
>>
>> Aim is to have configurable value for  STACK_HASH_SIZE,
>> so depend on use case one can configure it.
>>
>> 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.
>>
>> Signed-off-by: Yogesh Lal <ylal@codeaurora.org>
>> Signed-off-by: Vinayak Menon <vinmenon@codeaurora.org>
>> Signed-off-by: Vijayanand Jitta <vjitta@codeaurora.org>
>> ---
>>  lib/Kconfig      | 9 +++++++++
>>  lib/stackdepot.c | 3 +--
>>  2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/Kconfig b/lib/Kconfig
>> index 18d76b6..b3f8259 100644
>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -651,6 +651,15 @@ config STACKDEPOT
>>         bool
>>         select STACKTRACE
>>
>> +config STACK_HASH_ORDER_SHIFT
>> +       int "stack depot hash size (12 => 4KB, 20 => 1024KB)"
>> +       range 12 20
>> +       default 20
>> +       depends on STACKDEPOT
>> +       help
>> +        Select the hash size as a power of 2 for the stackdepot hash table.
>> +        Choose a lower value to reduce the memory impact.
>> +
>>  config SBITMAP
>>         bool
>>
>> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
>> index 2caffc6..413c20b 100644
>> --- a/lib/stackdepot.c
>> +++ b/lib/stackdepot.c
>> @@ -142,8 +142,7 @@ 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 STACK_HASH_SIZE (1L << CONFIG_STACK_HASH_ORDER_SHIFT)
>>  #define STACK_HASH_MASK (STACK_HASH_SIZE - 1)
>>  #define STACK_HASH_SEED 0x9747b28c
>>
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>> 2.7.4
>>
> 
> 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.
> 
> 

This idea looks fine to me. Andrew and others would like to hear your
comments as well on this before implementing.

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

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

[Vijayanand Jitta]

On 11/4/2020 3:59 PM, Vijayanand Jitta wrote:
> 
> 
> On 11/4/2020 4:57 AM, Minchan Kim wrote:
>> Sorry if this mail corrupts the mail thread or had heavy mangling
>> since I lost this mail from my mailbox so I am sending this mail by
>> web gmail.
>>
>> On Thu, Oct 22, 2020 at 10:18 AM <vjitta@codeaurora.org> wrote:
>>>
>>> From: Yogesh Lal <ylal@codeaurora.org>
>>>
>>> Use STACK_HASH_ORDER_SHIFT to configure STACK_HASH_SIZE.
>>>
>>> Aim is to have configurable value for  STACK_HASH_SIZE,
>>> so depend on use case one can configure it.
>>>
>>> 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.
>>>
>>> Signed-off-by: Yogesh Lal <ylal@codeaurora.org>
>>> Signed-off-by: Vinayak Menon <vinmenon@codeaurora.org>
>>> Signed-off-by: Vijayanand Jitta <vjitta@codeaurora.org>
>>> ---
>>>  lib/Kconfig      | 9 +++++++++
>>>  lib/stackdepot.c | 3 +--
>>>  2 files changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/Kconfig b/lib/Kconfig
>>> index 18d76b6..b3f8259 100644
>>> --- a/lib/Kconfig
>>> +++ b/lib/Kconfig
>>> @@ -651,6 +651,15 @@ config STACKDEPOT
>>>         bool
>>>         select STACKTRACE
>>>
>>> +config STACK_HASH_ORDER_SHIFT
>>> +       int "stack depot hash size (12 => 4KB, 20 => 1024KB)"
>>> +       range 12 20
>>> +       default 20
>>> +       depends on STACKDEPOT
>>> +       help
>>> +        Select the hash size as a power of 2 for the stackdepot hash table.
>>> +        Choose a lower value to reduce the memory impact.
>>> +
>>>  config SBITMAP
>>>         bool
>>>
>>> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
>>> index 2caffc6..413c20b 100644
>>> --- a/lib/stackdepot.c
>>> +++ b/lib/stackdepot.c
>>> @@ -142,8 +142,7 @@ 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 STACK_HASH_SIZE (1L << CONFIG_STACK_HASH_ORDER_SHIFT)
>>>  #define STACK_HASH_MASK (STACK_HASH_SIZE - 1)
>>>  #define STACK_HASH_SEED 0x9747b28c
>>>
>>> --
>>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>>> 2.7.4
>>>
>>
>> 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.
>>
>>
> 
> This idea looks fine to me. Andrew and others would like to hear your
> comments as well on this before implementing.
> 
> Thanks,
> Vijay
> 

Awaiting for comments from Andrew and others.

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

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

[Andrew Morton]

On Thu, 12 Nov 2020 18:26:24 +0530 Vijayanand Jitta <vjitta@codeaurora.org> wrote:

> >> 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.
> >>
> >>
> > 
> > This idea looks fine to me. Andrew and others would like to hear your
> > comments as well on this before implementing.
> > 
> > Thanks,
> > Vijay
> > 
> 
> Awaiting for comments from Andrew and others.

I don't actually understand the problem.

What is it about page-owner that causes stackdepot to consume
additional memory?  As far as I can tell, sizeof(struct stack_record)
isn't affected by page-owner?

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

[Minchan Kim]

On Thu, Nov 12, 2020 at 02:56:49PM -0800, Andrew Morton wrote:
> On Thu, 12 Nov 2020 18:26:24 +0530 Vijayanand Jitta <vjitta@codeaurora.org> wrote:
> 
> > >> 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.
> > >>
> > >>
> > > 
> > > This idea looks fine to me. Andrew and others would like to hear your
> > > comments as well on this before implementing.
> > > 
> > > Thanks,
> > > Vijay
> > > 
> > 
> > Awaiting for comments from Andrew and others.
> 
> I don't actually understand the problem.
> 
> What is it about page-owner that causes stackdepot to consume
> additional memory?  As far as I can tell, sizeof(struct stack_record)
> isn't affected by page-owner?
> 

Thing is once we build stackdepot due to the dependency from page_owner,
it will consume 8M regardless of using page_owner.

#define STACK_HASH_SIZE (1L << CONFIG_STACK_HASH_ORDER_SHIFT)

static struct stack_record *stack_table[STACK_HASH_SIZE] = {
	[0 ...  STACK_HASH_SIZE - 1] = NULL
};

So if we decide the size option at build time, we should consume
the memory anyway regardless of page_owner enabling in runtime.

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

[Zhenhua Huang]

On Wed, Nov 04, 2020 at 07:27:03AM +0800, Minchan Kim wrote:
> Sorry if this mail corrupts the mail thread or had heavy mangling
> since I lost this mail from my mailbox so I am sending this mail by
> web gmail.
> 
> On Thu, Oct 22, 2020 at 10:18 AM <vjitta@codeaurora.org> wrote:
> >
> > From: Yogesh Lal <ylal@codeaurora.org>
> >
> > Use STACK_HASH_ORDER_SHIFT to configure STACK_HASH_SIZE.
> >
> > Aim is to have configurable value for  STACK_HASH_SIZE,
> > so depend on use case one can configure it.
> >
> > 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.
> >
> > Signed-off-by: Yogesh Lal <ylal@codeaurora.org>
> > Signed-off-by: Vinayak Menon <vinmenon@codeaurora.org>
> > Signed-off-by: Vijayanand Jitta <vjitta@codeaurora.org>
> > ---
> >  lib/Kconfig      | 9 +++++++++
> >  lib/stackdepot.c | 3 +--
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index 18d76b6..b3f8259 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -651,6 +651,15 @@ config STACKDEPOT
> >         bool
> >         select STACKTRACE
> >
> > +config STACK_HASH_ORDER_SHIFT
> > +       int "stack depot hash size (12 => 4KB, 20 => 1024KB)"
> > +       range 12 20
> > +       default 20
> > +       depends on STACKDEPOT
> > +       help
> > +        Select the hash size as a power of 2 for the stackdepot hash 
> > table.
> > +        Choose a lower value to reduce the memory impact.
> > +
> >  config SBITMAP
> >         bool
> >
> > diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> > index 2caffc6..413c20b 100644
> > --- a/lib/stackdepot.c
> > +++ b/lib/stackdepot.c
> > @@ -142,8 +142,7 @@ 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 STACK_HASH_SIZE (1L << CONFIG_STACK_HASH_ORDER_SHIFT)
> >  #define STACK_HASH_MASK (STACK_HASH_SIZE - 1)
> >  #define STACK_HASH_SEED 0x9747b28c
> >
> > --
> > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
> > of Code Aurora Forum, hosted by The Linux Foundation
> > 2.7.4
> >
> 
> 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.
Seems we have other users like kasan, and dma_buf_ref which we introduced.
Also we can't guarantee there will not be any other users for stackdepot, so
it's better we not depend on only page owner?
> 
> 
> -- 
> Kind regards,
> Minchan Kim
> 

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

[Minchan Kim]

On Thu, Nov 19, 2020 at 11:34:32AM +0800, Zhenhua Huang wrote:
> On Wed, Nov 04, 2020 at 07:27:03AM +0800, Minchan Kim wrote:
> > Sorry if this mail corrupts the mail thread or had heavy mangling
> > since I lost this mail from my mailbox so I am sending this mail by
> > web gmail.
> > 
> > On Thu, Oct 22, 2020 at 10:18 AM <vjitta@codeaurora.org> wrote:
> > >
> > > From: Yogesh Lal <ylal@codeaurora.org>
> > >
> > > Use STACK_HASH_ORDER_SHIFT to configure STACK_HASH_SIZE.
> > >
> > > Aim is to have configurable value for  STACK_HASH_SIZE,
> > > so depend on use case one can configure it.
> > >
> > > 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.
> > >
> > > Signed-off-by: Yogesh Lal <ylal@codeaurora.org>
> > > Signed-off-by: Vinayak Menon <vinmenon@codeaurora.org>
> > > Signed-off-by: Vijayanand Jitta <vjitta@codeaurora.org>
> > > ---
> > >  lib/Kconfig      | 9 +++++++++
> > >  lib/stackdepot.c | 3 +--
> > >  2 files changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/Kconfig b/lib/Kconfig
> > > index 18d76b6..b3f8259 100644
> > > --- a/lib/Kconfig
> > > +++ b/lib/Kconfig
> > > @@ -651,6 +651,15 @@ config STACKDEPOT
> > >         bool
> > >         select STACKTRACE
> > >
> > > +config STACK_HASH_ORDER_SHIFT
> > > +       int "stack depot hash size (12 => 4KB, 20 => 1024KB)"
> > > +       range 12 20
> > > +       default 20
> > > +       depends on STACKDEPOT
> > > +       help
> > > +        Select the hash size as a power of 2 for the stackdepot hash 
> > > table.
> > > +        Choose a lower value to reduce the memory impact.
> > > +
> > >  config SBITMAP
> > >         bool
> > >
> > > diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> > > index 2caffc6..413c20b 100644
> > > --- a/lib/stackdepot.c
> > > +++ b/lib/stackdepot.c
> > > @@ -142,8 +142,7 @@ 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 STACK_HASH_SIZE (1L << CONFIG_STACK_HASH_ORDER_SHIFT)
> > >  #define STACK_HASH_MASK (STACK_HASH_SIZE - 1)
> > >  #define STACK_HASH_SEED 0x9747b28c
> > >
> > > --
> > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
> > > of Code Aurora Forum, hosted by The Linux Foundation
> > > 2.7.4
> > >
> > 
> > 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.
> Seems we have other users like kasan, and dma_buf_ref which we introduced.
> Also we can't guarantee there will not be any other users for stackdepot, so
> it's better we not depend on only page owner?

I didn't mean to make it page_owner dependent. What I suggested is just
to define kernel parameter for stackdeport hash size so admin could
override it at right size when we really need it.