Re: [PATCH v2 05/19] lib/stackdepot: use fixed-sized slots for stack records

Marco Elver
2023-09-15T08:59:30Z

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
>