[v2,05/19] lib/stackdepot: use fixed-sized slots for stack records
Commit Message
From: Andrey Konovalov <andreyknvl@google.com>
Instead of storing stack records in stack depot pools one right after
another, use fixed-sized slots.
Add a new Kconfig option STACKDEPOT_MAX_FRAMES that allows to select
the size of the slot in frames. Use 64 as the default value, which is
the maximum stack trace size both KASAN and KMSAN use right now.
Also add descriptions for other stack depot Kconfig options.
This is preparatory patch for implementing the eviction of stack records
from the stack depot.
Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
Changes v1->v2:
- Add and use STACKDEPOT_MAX_FRAMES Kconfig option.
---
lib/Kconfig | 10 ++++++++--
lib/stackdepot.c | 13 +++++++++----
2 files changed, 17 insertions(+), 6 deletions(-)
Comments
On Wed, 13 Sept 2023 at 19:14, <andrey.konovalov@linux.dev> wrote:
>
> From: Andrey Konovalov <andreyknvl@google.com>
>
> Instead of storing stack records in stack depot pools one right after
> another, use fixed-sized slots.
>
> Add a new Kconfig option STACKDEPOT_MAX_FRAMES that allows to select
> the size of the slot in frames. Use 64 as the default value, which is
> the maximum stack trace size both KASAN and KMSAN use right now.
>
> Also add descriptions for other stack depot Kconfig options.
>
> This is preparatory patch for implementing the eviction of stack records
> from the stack depot.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>
> ---
>
> Changes v1->v2:
> - Add and use STACKDEPOT_MAX_FRAMES Kconfig option.
> ---
> lib/Kconfig | 10 ++++++++--
> lib/stackdepot.c | 13 +++++++++----
> 2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/lib/Kconfig b/lib/Kconfig
> index c686f4adc124..7c32f424a6f3 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -708,13 +708,19 @@ config ARCH_STACKWALK
> bool
>
> config STACKDEPOT
> - bool
> + bool "Stack depot: stack trace storage that avoids duplication"
> select STACKTRACE
>
> config STACKDEPOT_ALWAYS_INIT
> - bool
> + bool "Always initialize stack depot during early boot"
> select STACKDEPOT
This makes both STACKDEPOT and STACKDEPOT_ALWAYS_INIT configurable by
users: https://www.kernel.org/doc/html/next/kbuild/kconfig-language.html#menu-attributes
Usually the way to add documentation for non-user-configurable options
is to add text in the "help" section of the config.
I think the change here is not what was intended.
> +config STACKDEPOT_MAX_FRAMES
> + int "Maximum number of frames in trace saved in stack depot"
> + range 1 256
> + default 64
> + depends on STACKDEPOT
> +
> config REF_TRACKER
> bool
> depends on STACKTRACE_SUPPORT
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 9a004f15f59d..128ece21afe9 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -58,9 +58,12 @@ struct stack_record {
> u32 hash; /* Hash in the hash table */
> u32 size; /* Number of stored frames */
> union handle_parts handle;
> - unsigned long entries[]; /* Variable-sized array of frames */
> + unsigned long entries[CONFIG_STACKDEPOT_MAX_FRAMES]; /* Frames */
> };
>
> +#define DEPOT_STACK_RECORD_SIZE \
> + ALIGN(sizeof(struct stack_record), 1 << DEPOT_STACK_ALIGN)
> +
> static bool stack_depot_disabled;
> static bool __stack_depot_early_init_requested __initdata = IS_ENABLED(CONFIG_STACKDEPOT_ALWAYS_INIT);
> static bool __stack_depot_early_init_passed __initdata;
> @@ -258,9 +261,7 @@ static struct stack_record *
> depot_alloc_stack(unsigned long *entries, int size, u32 hash, void **prealloc)
> {
> struct stack_record *stack;
> - size_t required_size = struct_size(stack, entries, size);
> -
> - required_size = ALIGN(required_size, 1 << DEPOT_STACK_ALIGN);
> + size_t required_size = DEPOT_STACK_RECORD_SIZE;
>
> /* Check if there is not enough space in the current pool. */
> if (unlikely(pool_offset + required_size > DEPOT_POOL_SIZE)) {
> @@ -295,6 +296,10 @@ depot_alloc_stack(unsigned long *entries, int size, u32 hash, void **prealloc)
> if (stack_pools[pool_index] == NULL)
> return NULL;
>
> + /* Limit number of saved frames to CONFIG_STACKDEPOT_MAX_FRAMES. */
> + if (size > CONFIG_STACKDEPOT_MAX_FRAMES)
> + size = CONFIG_STACKDEPOT_MAX_FRAMES;
> +
> /* Save the stack trace. */
> stack = stack_pools[pool_index] + pool_offset;
> stack->hash = hash;
> --
> 2.25.1
>
On Fri, Sep 15, 2023 at 10:56 AM Marco Elver <elver@google.com> wrote:
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -708,13 +708,19 @@ config ARCH_STACKWALK
> > bool
> >
> > config STACKDEPOT
> > - bool
> > + bool "Stack depot: stack trace storage that avoids duplication"
> > select STACKTRACE
> >
> > config STACKDEPOT_ALWAYS_INIT
> > - bool
> > + bool "Always initialize stack depot during early boot"
> > select STACKDEPOT
>
> This makes both STACKDEPOT and STACKDEPOT_ALWAYS_INIT configurable by
> users: https://www.kernel.org/doc/html/next/kbuild/kconfig-language.html#menu-attributes
>
> Usually the way to add documentation for non-user-configurable options
> is to add text in the "help" section of the config.
>
> I think the change here is not what was intended.
Ah, didn't know about that. Will fix in v3. Thanks!
@@ -708,13 +708,19 @@ config ARCH_STACKWALK
bool
config STACKDEPOT
- bool
+ bool "Stack depot: stack trace storage that avoids duplication"
select STACKTRACE
config STACKDEPOT_ALWAYS_INIT
- bool
+ bool "Always initialize stack depot during early boot"
select STACKDEPOT
+config STACKDEPOT_MAX_FRAMES
+ int "Maximum number of frames in trace saved in stack depot"
+ range 1 256
+ default 64
+ depends on STACKDEPOT
+
config REF_TRACKER
bool
depends on STACKTRACE_SUPPORT
@@ -58,9 +58,12 @@ struct stack_record {
u32 hash; /* Hash in the hash table */
u32 size; /* Number of stored frames */
union handle_parts handle;
- unsigned long entries[]; /* Variable-sized array of frames */
+ unsigned long entries[CONFIG_STACKDEPOT_MAX_FRAMES]; /* Frames */
};
+#define DEPOT_STACK_RECORD_SIZE \
+ ALIGN(sizeof(struct stack_record), 1 << DEPOT_STACK_ALIGN)
+
static bool stack_depot_disabled;
static bool __stack_depot_early_init_requested __initdata = IS_ENABLED(CONFIG_STACKDEPOT_ALWAYS_INIT);
static bool __stack_depot_early_init_passed __initdata;
@@ -258,9 +261,7 @@ static struct stack_record *
depot_alloc_stack(unsigned long *entries, int size, u32 hash, void **prealloc)
{
struct stack_record *stack;
- size_t required_size = struct_size(stack, entries, size);
-
- required_size = ALIGN(required_size, 1 << DEPOT_STACK_ALIGN);
+ size_t required_size = DEPOT_STACK_RECORD_SIZE;
/* Check if there is not enough space in the current pool. */
if (unlikely(pool_offset + required_size > DEPOT_POOL_SIZE)) {
@@ -295,6 +296,10 @@ depot_alloc_stack(unsigned long *entries, int size, u32 hash, void **prealloc)
if (stack_pools[pool_index] == NULL)
return NULL;
+ /* Limit number of saved frames to CONFIG_STACKDEPOT_MAX_FRAMES. */
+ if (size > CONFIG_STACKDEPOT_MAX_FRAMES)
+ size = CONFIG_STACKDEPOT_MAX_FRAMES;
+
/* Save the stack trace. */
stack = stack_pools[pool_index] + pool_offset;
stack->hash = hash;