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

From: Marco Elver <elver@google.com>
To: andrey.konovalov@linux.dev
Cc: Alexander Potapenko <glider@google.com>,
	Andrey Konovalov <andreyknvl@gmail.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	kasan-dev@googlegroups.com, Evgenii Stepanov <eugenis@google.com>,
	Oscar Salvador <osalvador@suse.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Andrey Konovalov <andreyknvl@google.com>
Subject: Re: [PATCH v2 05/19] lib/stackdepot: use fixed-sized slots for stack records
Date: Fri, 15 Sep 2023 10:55:47 +0200	[thread overview]
Message-ID: <CANpmjNP8O-GLQ9m06riX+kjbPSD9sBo+XGtTE2xW=pq9uJFGAg@mail.gmail.com> (raw)
In-Reply-To: <658f5f34d4f94721844ad8ba41452d54b4f8ace5.1694625260.git.andreyknvl@google.com>

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
>