Re: [PATCH v5 01/38] stackdepot: reserve 5 extra bits in depot_stack_handle_t

[Andrey Konovalov <andreyknvl@google.com>]

On Wed, Mar 25, 2020 at 5:12 PM <glider@google.com> wrote:
>
> Some users (currently only KMSAN) may want to use spare bits in
> depot_stack_handle_t. Let them do so and provide get_dsh_extra_bits()
> and set_dsh_extra_bits() to access those bits.
>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> To: Alexander Potapenko <glider@google.com>
> Cc: Vegard Nossum <vegard.nossum@oracle.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Marco Elver <elver@google.com>
> Cc: Andrey Konovalov <andreyknvl@google.com>
> Cc: linux-mm@kvack.org
> ---
>
> Change-Id: I23580dbde85908eeda0bdd8f83a8c3882ab3e012
> ---
>  include/linux/stackdepot.h |  8 ++++++++
>  lib/stackdepot.c           | 24 +++++++++++++++++++++++-
>  2 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
> index 24d49c732341a..ac1b5a78d7f65 100644
> --- a/include/linux/stackdepot.h
> +++ b/include/linux/stackdepot.h
> @@ -12,6 +12,11 @@
>  #define _LINUX_STACKDEPOT_H
>
>  typedef u32 depot_stack_handle_t;
> +/*
> + * Number of bits in the handle that stack depot doesn't use. Users may store
> + * information in them.
> + */

Could it be that stack depot starts using those bits at some point in
the future and then external users will get broken? If not, maybe it
makes sense to change the language here that stack depot explicitly
dedicates these 5 bits to external use. Otherwise this looks a bit
confusing IMO.

> +#define STACK_DEPOT_EXTRA_BITS 5
>
>  depot_stack_handle_t stack_depot_save(unsigned long *entries,
>                                       unsigned int nr_entries, gfp_t gfp_flags);
> @@ -20,5 +25,8 @@ unsigned int stack_depot_fetch(depot_stack_handle_t handle,
>                                unsigned long **entries);
>
>  unsigned int filter_irq_stacks(unsigned long *entries, unsigned int nr_entries);
> +depot_stack_handle_t set_dsh_extra_bits(depot_stack_handle_t handle,
> +                                       unsigned int bits);
> +unsigned int get_dsh_extra_bits(depot_stack_handle_t handle);
>
>  #endif
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 2caffc64e4c82..195ce3dc7c37e 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -40,8 +40,10 @@
>  #define STACK_ALLOC_ALIGN 4
>  #define STACK_ALLOC_OFFSET_BITS (STACK_ALLOC_ORDER + PAGE_SHIFT - \
>                                         STACK_ALLOC_ALIGN)
> +
>  #define STACK_ALLOC_INDEX_BITS (DEPOT_STACK_BITS - \
> -               STACK_ALLOC_NULL_PROTECTION_BITS - STACK_ALLOC_OFFSET_BITS)
> +               STACK_ALLOC_NULL_PROTECTION_BITS - \
> +               STACK_ALLOC_OFFSET_BITS - STACK_DEPOT_EXTRA_BITS)
>  #define STACK_ALLOC_SLABS_CAP 8192
>  #define STACK_ALLOC_MAX_SLABS \
>         (((1LL << (STACK_ALLOC_INDEX_BITS)) < STACK_ALLOC_SLABS_CAP) ? \
> @@ -54,6 +56,7 @@ union handle_parts {
>                 u32 slabindex : STACK_ALLOC_INDEX_BITS;
>                 u32 offset : STACK_ALLOC_OFFSET_BITS;
>                 u32 valid : STACK_ALLOC_NULL_PROTECTION_BITS;
> +               u32 extra : STACK_DEPOT_EXTRA_BITS;
>         };
>  };
>
> @@ -72,6 +75,24 @@ static int next_slab_inited;
>  static size_t depot_offset;
>  static DEFINE_SPINLOCK(depot_lock);
>
> +depot_stack_handle_t set_dsh_extra_bits(depot_stack_handle_t handle,
> +                                       u32 bits)
> +{
> +       union handle_parts parts = { .handle = handle };
> +
> +       parts.extra = bits & ((1U << STACK_DEPOT_EXTRA_BITS) - 1);
> +       return parts.handle;
> +}
> +EXPORT_SYMBOL_GPL(set_dsh_extra_bits);
> +
> +u32 get_dsh_extra_bits(depot_stack_handle_t handle)
> +{
> +       union handle_parts parts = { .handle = handle };
> +
> +       return parts.extra;
> +}
> +EXPORT_SYMBOL_GPL(get_dsh_extra_bits);
> +
>  static bool init_stack_slab(void **prealloc)
>  {
>         if (!*prealloc)
> @@ -136,6 +157,7 @@ static struct stack_record *depot_alloc_stack(unsigned long *entries, int size,
>         stack->handle.slabindex = depot_index;
>         stack->handle.offset = depot_offset >> STACK_ALLOC_ALIGN;
>         stack->handle.valid = 1;
> +       stack->handle.extra = 0;
>         memcpy(stack->entries, entries, size * sizeof(unsigned long));
>         depot_offset += required_size;
>
> --
> 2.25.1.696.g5e7596f4ac-goog
>