Messages in this thread | | | Subject | Re: [PATCH v3] lib: stackdepot: Add support to configure STACK_HASH_SIZE | From | Vijayanand Jitta <> | Date | Tue, 22 Dec 2020 11:25:34 +0530 |
| |
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 <vjitta@codeaurora.org> 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
| |