Re: [DTrace-devel] [PATCH 04/14] Track uprobe provider descriptions

[Kris Van Hees <kris.van.hees@oracle.com>]

On Fri, Jun 07, 2024 at 05:40:51PM -0400, Eugene Loh wrote:
> On 6/4/24 17:10, Kris Van Hees wrote:
> 
> > On Tue, Jun 04, 2024 at 02:11:03PM -0400, eugene.loh--- via DTrace-devel wrote:
> > > From: Eugene Loh <eugene.loh@oracle.com>
> > > 
> > > A uprobe will have to decide which of its clauses to run for a
> > > given pid.  For now, keep the provider description for each clause.
> > > There are many shortcomings to the way this is done in this patch,
> > > but it is quick and dirty and helps us bootstrap real support.
> > > Clean this up later, probably turning it into a growable array.
> > This is the wrong way to look at it, I think.  The underlying concept is that
> > DTrace supports specifying probes that do not match anything yet (-Z).  Those
> > are called retained enablings and they can exist for more than just uprobes.
> > So this needs to be more generic.
> 
> Okay.  I looked at legacy-DTrace retained enablings.  They seem to be a
> linked list -- (mainly) of arrays of dtrace_ecbdesc pointers. And
> dtrace_ecbdesc has dtrace_probedesc, dtrace_preddesc, and dtrace_actdesc. 
> In DTrace on eBPF, dtrace_ecbdesc is much simpler -- it's really just a
> dtrace_probedesc.  So it seems to me we could just make our retained
> enablings be an array of probe desc pointers.  That would be a small change
> to this (small) patch.
> 
> > We also shouldn't do this non-discriminately for all clauses all the time.
> > Obviously, when -Z is not used, there is no point in doing this because there
> > cannot be a match-after-start.
> 
> FWIW, that's not how legacy DTrace seems to work:  there *can* be
> match-after-start without -Z.  The -Z option only addresses whether there is
> an initial match.  If there is, then -Z is not needed, not even for
> subsequent matches to work.

Yes, I didnt think of that.  But -Z is still relevant in view of the stuff I
mention below...

> > Even when -Z is specified, you might be able
> > to determine whether the probe specification for a probe matches perfectly
> > or whether it needs to be retained for match-after-start.
> 
> Maybe.  But what if you have pid1234:a.out:main:go, but pid 1234 has not yet
> started up?  (Yes, that's an extremely weird example.)

No, when a pid* probe is not able to be resolved, dtrace flags an error:

$ sudo ./build/run-dtrace -n 'pid12:a.out:main:go,BEGIN { exit(0); }'
dtrace: invalid probe specifier pid12:a.out:main:go,BEGIN { exit(0); }: failed to grab process 12
# sudo ./build/run-dtrace -Zn 'pid12:a.out:main:go,BEGIN { exit(0); }'
dtrace: invalid probe specifier pid12:a.out:main:go,BEGIN { exit(0); }: failed to grab process 12

> Mostly (going back to your "shouldn't non-discriminately" comment), trying
> to decide whether to keep a probe desc seems unnecessary. Just keep them
> all.  They will likely be dwarfed by dtp->dt_probes[] anyhow.

The issue is not space...  but any retained enabling would have to be
re-evaluated everytime we need to see if there are new probes to be enabled.
And this will be happening while we are already tracing, so avoiding needless
matching is probably worth it.

> > So, let's put some thought into designing this in the more generic case, so
> > that we can avoid going down a rabbit hole that gets tough to recover from.
> 
> What do you think of:
> 
>     struct dtrace_hdl {
>         [...]
>         dt_list_t dt_enablings; /* list of (to be) enabled probes */
> +       dtrace_probedesc_t **dt_retained;
> +       int dt_nretained;
> +       int dt_maxretained;

I don't think you need both varoables.  Just keeping dt_retained_sz (which is
your dt_maxretained, but made consistent with dt_probes_sz) is enough since
you can know what the last entry is when looping over it by checking if the
element is NULL.  There should never be gaps, so the first NULL indicates that
you reached the end.

>         struct dt_xlator **dt_xlatormap; /* dt_xlator_t's indexed by dx_id
> */
>         [...]
>     }
> 
> 
> > (And whatever we store the retained enablings in probably should be added to
> >   dtrace_hdl_t after dt_enablings.)
> 
> No problem.
> 
> > > diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
> > > @@ -215,6 +215,7 @@ dt_compile_one_clause(dtrace_hdl_t *dtp, dt_node_t *cnp, dt_node_t *pnp)
> > >   	 */
> > >   	dt_cg(yypcb, cnp);
> > >   	sdp->dtsd_clause = dt_clause_create(dtp, dt_as(yypcb));
> > > +	dtp->dt_uprovdescs[dtp->dt_clause_nextid - 1] = strdup(pnp->dn_desc->prv);
> > >   	assert(yypcb->pcb_stmt == sdp);
> > >   	if (dtrace_stmt_add(yypcb->pcb_hdl, yypcb->pcb_prog, sdp) != 0)
> > > diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> > > @@ -310,6 +310,7 @@ struct dtrace_hdl {
> > >   	dt_pcb_t *dt_pcb;	/* pointer to current parsing control block */
> > >   	ulong_t dt_gen;		/* compiler generation number */
> > >   	uint_t dt_clause_nextid; /* next ID to use for programs */
> > > +	char *dt_uprovdescs[256]; /* uprobe provider descriptor per clause... FIXME turn this into a growable array */
> > >   	dt_list_t dt_programs;	/* linked list of dtrace_prog_t's */
> > >   	dt_list_t dt_xlators;	/* linked list of dt_xlator_t's */
> > >   	dt_list_t dt_enablings;	/* list of (to be) enabled probes */