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

[fche at redhat dot com (Frank Ch. Eigler)]

smakarov wrote:

> 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)

Looking at the complexity, I wonder if we need such a cache at all.
Not many stap scripts seem to require more than one backtrace/stack
type call within them.  The opportunity for savings in the PR6580
world is mainly to avoid unwinding *deeper than necessary*, not to
avoid unwinding *repeatedly*.  (The case of caching user-regs from a
full kernel unwind is special.)


> +        /* We always start by fetching the current PC if it's not yet known. */
> +        if (c->uwcache.depth == 0 && c->uwcache.data[0] == 0) {
> +                if (! c->kregs) {
> +                        /* Even the current PC is unknown; so we have absolutely no data
> +                         * at any depth. Note that unlike _stp_stack_kernel_print(), we
> +                         * can't fall back to calling dump_trace() to obtain the
> +                         * backtrace. */
> +                        c->uwcache.done = 1; return 0;

Why can't we fall back to dump_trace in a non-printing case?  
Were you going to change the _print case to rely on this _get?


> +        if (c->uwcache.depth == 0) { /* need to initialize uwcontext */
> +                if (c->uwcache.depth == 0 && c->uregs == &c->uwcontext_kernel.info.regs) {
> +                        /* Unwinder needs the reg state, clear uregs ref. */
> +                        c->uregs = NULL;
> +                        c->probe_flags &= ~_STP_PROBE_STATE_FULL_UREGS;

We should have formal bitfields for this sort of thing, if we really need it.


> +function __stack_raw (n:long) %{ /* pragma:unwind */
> +         STAP_RETVALUE = _stp_stack_kernel_get (CONTEXT, (unsigned)STAP_ARG_n);
> +%}

Is there a risk here, considering that the new _stp_stack_kernel_get
doesn't check its 'n' parameter super well?


> +  o->newline() << "#if defined(STP_NEED_UNWIND_DATA)";
> +  o->newline() << "c->uwcache.depth = 0;";
> +  o->newline() << "c->uwcache.done = 0;";
> +  o->newline() << "c->uwcache.data[0] = 0;";
> +  o->newline() << "#endif";

That seems like too much work to impose on all probes.


- FChE