On Thu, Apr 18, 2019 at 10:41:22AM +0200, Thomas Gleixner wrote:
> The struct stack_trace indirection in the stack depot functions is a truly
> pointless excercise which requires horrible code at the callsites.
>
> Provide interfaces based on plain storage arrays.
>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Acked-by: Alexander Potapenko <glider@xxxxxxxxxx>
> ---
> include/linux/stackdepot.h | 4 ++
> lib/stackdepot.c | 66 ++++++++++++++++++++++++++++++++-------------
> 2 files changed, 51 insertions(+), 19 deletions(-)
>
> --- a/include/linux/stackdepot.h
> +++ b/include/linux/stackdepot.h
> @@ -26,7 +26,11 @@ typedef u32 depot_stack_handle_t;
> struct stack_trace;
>
> depot_stack_handle_t depot_save_stack(struct stack_trace *trace, gfp_t flags);
> +depot_stack_handle_t stack_depot_save(unsigned long *entries,
> + unsigned int nr_entries, gfp_t gfp_flags);
>
> void depot_fetch_stack(depot_stack_handle_t handle, struct stack_trace *trace);
> +unsigned int stack_depot_fetch(depot_stack_handle_t handle,
> + unsigned long **entries);
>
> #endif
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -194,40 +194,56 @@ static inline struct stack_record *find_
> return NULL;
> }
>
> -void depot_fetch_stack(depot_stack_handle_t handle, struct stack_trace *trace)
> +/**
> + * stack_depot_fetch - Fetch stack entries from a depot
> + *
Nit: kernel-doc will complain about missing description of @handle.
> + * @entries: Pointer to store the entries address
> + */
> +unsigned int stack_depot_fetch(depot_stack_handle_t handle,
> + unsigned long **entries)
> {
> union handle_parts parts = { .handle = handle };
> void *slab = stack_slabs[parts.slabindex];
> size_t offset = parts.offset << STACK_ALLOC_ALIGN;
> struct stack_record *stack = slab + offset;
>
> - trace->nr_entries = trace->max_entries = stack->size;
> - trace->entries = stack->entries;
> - trace->skip = 0;
> + *entries = stack->entries;
> + return stack->size;
> +}
> +EXPORT_SYMBOL_GPL(stack_depot_fetch);
> +
> +void depot_fetch_stack(depot_stack_handle_t handle, struct stack_trace *trace)
> +{
> + unsigned int nent = stack_depot_fetch(handle, &trace->entries);
> +
> + trace->max_entries = trace->nr_entries = nent;
> }
> EXPORT_SYMBOL_GPL(depot_fetch_stack);
>
> /**
> - * depot_save_stack - save stack in a stack depot.
> - * @trace - the stacktrace to save.
> - * @alloc_flags - flags for allocating additional memory if required.
> + * stack_depot_save - Save a stack trace from an array
> *
> - * Returns the handle of the stack struct stored in depot.
> + * @entries: Pointer to storage array
> + * @nr_entries: Size of the storage array
> + * @alloc_flags: Allocation gfp flags
> + *
> + * Returns the handle of the stack struct stored in depot
Can you please s/Returns/Return:/ so that kernel-doc will recognize this as
return section.
> */
> -depot_stack_handle_t depot_save_stack(struct stack_trace *trace,
> - gfp_t alloc_flags)
> +depot_stack_handle_t stack_depot_save(unsigned long *entries,
> + unsigned int nr_entries,
> + gfp_t alloc_flags)
> {
> - u32 hash;
> - depot_stack_handle_t retval = 0;
> struct stack_record *found = NULL, **bucket;
> - unsigned long flags;
> + depot_stack_handle_t retval = 0;
> struct page *page = NULL;
> void *prealloc = NULL;
> + unsigned long flags;
> + u32 hash;
>
> - if (unlikely(trace->nr_entries == 0))
> + if (unlikely(nr_entries == 0))
> goto fast_exit;
>
> - hash = hash_stack(trace->entries, trace->nr_entries);
> + hash = hash_stack(entries, nr_entries);
> bucket = &stack_table[hash & STACK_HASH_MASK];
>
> /*
> @@ -235,8 +251,8 @@ depot_stack_handle_t depot_save_stack(st
> * The smp_load_acquire() here pairs with smp_store_release() to
> * |bucket| below.
> */
> - found = find_stack(smp_load_acquire(bucket), trace->entries,
> - trace->nr_entries, hash);
> + found = find_stack(smp_load_acquire(bucket), entries,
> + nr_entries, hash);
> if (found)
> goto exit;
>
> @@ -264,10 +280,10 @@ depot_stack_handle_t depot_save_stack(st
>
> spin_lock_irqsave(&depot_lock, flags);
>
> - found = find_stack(*bucket, trace->entries, trace->nr_entries, hash);
> + found = find_stack(*bucket, entries, nr_entries, hash);
> if (!found) {
> struct stack_record *new =
> - depot_alloc_stack(trace->entries, trace->nr_entries,
> + depot_alloc_stack(entries, nr_entries,
> hash, &prealloc, alloc_flags);
> if (new) {
> new->next = *bucket;
> @@ -297,4 +313,16 @@ depot_stack_handle_t depot_save_stack(st
> fast_exit:
> return retval;
> }
> +EXPORT_SYMBOL_GPL(stack_depot_save);
> +
> +/**
> + * depot_save_stack - save stack in a stack depot.
> + * @trace - the stacktrace to save.
> + * @alloc_flags - flags for allocating additional memory if required.
> + */
> +depot_stack_handle_t depot_save_stack(struct stack_trace *trace,
> + gfp_t alloc_flags)
> +{
> + return stack_depot_save(trace->entries, trace->nr_entries, alloc_flags);
> +}
> EXPORT_SYMBOL_GPL(depot_save_stack);
>
>
--
Sincerely yours,
Mike.
