Re: [PR6580; patch for review] first step of unwind_cache enhancement

[Serguei Makarov <smakarov at redhat dot com>]

> An unwind_context is pretty big.
> If at all possible I would only keep one.

I can only think of two solutions for (potential) memory savings here:
- keep only one uwcontext; if we call a user backtrace function after a kernel one (or vice versa), then we need to erase the current uwcontext for the user side and do a kernel unwind from the beginning in the same space
- keep both uwcontexts, but allocate them dynamically when needed (blegh) rather than embedding them in the probe context struct

Perhaps there's a better solution.

> +/* NB: Returns 0 for depth == 0 -- DWARF unwinder does not fetch
> + * current PC.  Moreover, uwcontext is assumed to have been
> + * initialized by the caller. */
> +unsigned long __stp_dwarf_stack_kernel_get(struct pt_regs *regs, int depth,
> +                                           struct unwind_context *uwcontext,
> +                                           struct unwind_cache *uwcache)
> +{
> +        struct unwind_frame_info *info = &uwcontext->info;
> +        int ret;
> +  
> +        /* check if answer is already defined in cache; this is probably
> +         * redundant, being already handled for us by the caller */
> +        if (unlikely(uwcache->depth >= depth))
> +                return uwcache->data[depth];
> +
> +        /* check if unwind cannot proceed further */
> +        if (uwcache->done)
> +                return 0;
> +
> +        if (uwcache->depth == 0) /* need to initialize uwcontext->info */
> +                arch_unw_init_frame_info(info, regs, 0);

> Where did the caller get the regs from?

The caller would be _stp_stack_kernel_get, which got the regs from c->kregs(). This function works for the kernel-side unwind only; there would be a separate function *_user_get for the user-side unwind in the next patch.

> It should also not just use 0
> for the last argument (sanitize). That depends on whether we are doing
> user unwinding and how we got the registers. See the check in
> _stp_get_uregs for _stp_task_pt_regs_valid().

Alright, I'll check what to do with this. This is basically identical to how __stp_dwarf_stack_kernel_print sets things up, though.

> >  #else
> >         /* Arch specific fallback for kernel backtraces. */
> > -       __stp_stack_print(regs, sym_flags, MAXBACKTRACE);
> > +       __stp_stack_print(regs, sym_flags, MAXBACKTRACE);       
> >  #endif
> >  }

> Whitespace change?

Thanks for the catch. Swore I had my emacs indentation set up adequately for this...

> You could use a signed value and make negative values say "done at
> -depth". Frank might be right that keeping the old PC values is not of
> that much use. You could just keep just the unwind_context and depth,
> then check the requested depth is one beyond what we have in the context
> and if not, just start from scratch again. Probably depends on some
> investigation of how often callers/backtraces are reused in a single
> probe handler.

Getting rid of the cache would probably save memory, but not make the logic much less complicated. (Still, the former might be worthwhile.) The current scheme gives maximum peace of mind on the tapset side to implement everything in terms of stack() without undue overhead.