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

[Mark Wielaard <mjw at redhat dot com>]

On Wed, 2012-09-05 at 11:43 -0400, Serguei Makarov wrote:
> The first incremental step to the runtime tweaks required to sanely
> implement PR6580. Namely, the kernel-side backtrace can be unwound one
> step at a time using _stp_stack_kernel_get, with the already unwound
> portion of the backtrace cached within the probe context.
> 
> This allows us to implement stack(n) (which returns the PC n levels
> deep within the stack) without having to resort to tokenizing a
> backtrace string each time.

Yeah, the tokenizing is really not a very efficient representation.

> The patch itself seems to be sound, but testing it drew attention to
> memory corruption errors in the existing DWARF unwinder (see PR14546).

Thanks for the testcase, this is hopefully resolved with commit 8f095f.




> diff --git a/runtime/common_probe_context.h
> b/runtime/common_probe_context.h
> index df07b90..61c1b26 100644
> --- a/runtime/common_probe_context.h
> +++ b/runtime/common_probe_context.h
> @@ -118,5 +118,12 @@ cycles_t cycles_sum;
>  
>  /* Current state of the unwinder (as used in the unwind.c dwarf
> unwinder). */
>  #if defined(STP_NEED_UNWIND_DATA)
> -struct unwind_context uwcontext;
> +// Invariants (uwcontext currently refers to uwcontext_kernel):
> +//   uwcache.depth == 0 --> uwcontext needs to be initialized
> +//   uwcache.depth > 0  --> uwcontext is state after uwcache.depth
> unwind steps
> +//   uwcache.depth >= n --> uwcache.data[n] contains PC at depth n
> +//     EXCEPT WHEN n == 0, uwcache.data[n] == 0 (current PC not yet
> retrieved)
> +struct unwind_cache uwcache;
> +struct unwind_context uwcontext_user;
> +struct unwind_context uwcontext_kernel;
>  #endif

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

User backtraces are a little special because depending on architecture
and probe point they might be able to start with a full set of registers
to feed to the DWARF unwinder, or might need to do a full DWARF unwind
from the current kernel probe point to the user/kernel boundary. This is
what _stp_get_uregs in stack.c does.

> +/* 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? 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().

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

Whitespace change?

> +struct unwind_cache {
> +  // XXX use more compact representation?
> +  unsigned depth;
> +  unsigned done; // if nonzero, unwind cannot proceed
> +  unsigned long data[MAXBACKTRACE];
> +};

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.

Cheers,

Mark