[DTrace-devel] [PATCH v2 03/19] Deprecate enabled probe ID (epid)
Kris Van Hees
kris.van.hees at oracle.com
Mon Sep 9 17:31:34 UTC 2024
On Sun, Sep 08, 2024 at 12:31:12PM -0400, Eugene Loh wrote:
> On 9/5/24 23:31, Kris Van Hees wrote:
>
> > Great work! Some comments below...
> >
> > Note that with this work, we are breaking the libdtrace API
>
> Okay, but perhaps it was already broken, at least through disuse and
> changing ioctl stuff???
There probably were already smoe breakage changes - I am actually not sure.
But this definitely is one - but as I wrote, I don't think we should really
care too much right now.
> I'll post a revised patch (and small revisions of two other patches due to
> the index/id naming change). In a few cases, however, I comment below:
>
> > - but that might
> > be a small price to pay for cleaning up some needed things. However, that
> > also means that we could just break it a tiny bit more and make things easier,
> > perhaps. (Yes, that goes a bit against my earlier advice but it really makes
> > no sense (in retrospect) to keep supporting something that we are breaking in
> > others ways anyway.)
> >
> > See below for details...
> >
> > On Tue, Sep 03, 2024 at 01:16:43AM -0400, eugene.loh at oracle.com wrote:
> > > From: Eugene Loh <eugene.loh at oracle.com>
> > > However, its value was opaque and therefore difficult to use.
> > I would drop this sentence. I think the preceding paragraph sufficiently
> > describes it. And you risk confusing the reader concerning whether the epipd
> > was difficult to use in the dtrace internals or as a user (who won't read this
> > anyway).
>
> Difficult for the user, but even if a user doesn't read this text the point
> still matters for the reader. I think it's worth building the case for
> deprecating something that, after all, was part of D. Noting that the value
> to the user was poor helps build this case. I added a couple of words to
> clarify "to the user."
>
> > > Deprecate the use of EPID:
> > >
> > > *) So we rearrange as follows:
> > >
> > > old new
> > >
> > > 0: pad (size) pad (size) <= buffer start
> > > 4: pad (additional) specid
> > > 8: epid <= buffer start prid
> > > 12: specid stid
> > > 16: data[n] data[n]
> > >
> > > So now, we say there is no longer an 8-byte pad before the
> > > buffer start; rather, the buffer starts with a 4-byte pad.
> > I am not sure whether this layout comparison is comprehensible to anyone not
> > familiar with this code.
>
> Yeah. Incidentally, I had moved specid so that prid/stid were in the same
> 8-byte block so that someone could read it as a single 8-byte EPID if they
> wanted to. I'm assuming you see no value in that. So I moved specid back
> to where it used to be and I think this simplifies the description of what's
> going on. Anyhow, I revised this explanation. Hopefully it's sufficient
> now.
I like the specid first actually :) Since it reflects the designation of the
data.
> > It may need a bit more explanation concerning how
> > this is used. E.g. the size that is referred to above is a 32-bit integer
> > emitted by the perf ring buffer code, giving the size of the data blob that
> > follows. And this is the reason that we need to do this little shuffle with
> > the padding because (1) data needs to be written to the temporary buffer
> > storage with proper alignment, (2) data should be aligned in the ring buffer,
> > and (3) the perf event header + this size results in the data starting at
> > a multiple of 8 + 4, i.e. not a 8-byte boundary.
> >
> > > diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
> > > @@ -156,12 +157,12 @@ dt_clause_create(dtrace_hdl_t *dtp, dtrace_difo_t *dp)
> > > /*
> > > * Generate a symbol name.
> > > */
> > > - len = snprintf(NULL, 0, "dt_clause_%d", dtp->dt_clause_nextid) + 1;
> > > + len = snprintf(NULL, 0, "dt_clause_%d", sdp->dtsd_index) + 1;
> > > name = dt_alloc(dtp, len);
> > > if (name == NULL)
> > > longjmp(yypcb->pcb_jmpbuf, EDT_NOMEM);
> > > - snprintf(name, len, "dt_clause_%d", dtp->dt_clause_nextid++);
> > > + snprintf(name, len, "dt_clause_%d", sdp->dtsd_index);
> > Here (and elsewhere) this should be replaced by:
> >
> > if (asprintf(&name, "dt_clause_%d", sdp->dtsd_id) == -1)
> > longjmp(yypcb->pcb_jmpbuf, EDT_NOMEM);
>
> Hmm. But:
> 1) These were pre-existing constructs.
> 2) The other other two sites used alloca() and handling the free()s would
> require a bit more rearranging.
>
> So, okay, I updated this case but left the other two cases alone. I don't
> object to asprintf() but those other two cases would seem to be material for
> their own patch.
OK
> > > /*
> > > * Add the symbol to the BPF identifier table and associate the DIFO
> > > diff --git a/libdtrace/dt_map.c b/libdtrace/dt_map.c
> > > @@ -85,98 +85,16 @@ dt_datadesc_finalize(dtrace_hdl_t *dtp, dtrace_datadesc_t *ddp)
> > > -void
> > > -dt_epid_destroy(dtrace_hdl_t *dtp)
> > > -{
> > > - size_t i;
> > > -
> > > - assert((dtp->dt_pdesc != NULL && dtp->dt_ddesc != NULL &&
> > > - dtp->dt_maxprobe > 0) || (dtp->dt_pdesc == NULL &&
> > > - dtp->dt_ddesc == NULL && dtp->dt_maxprobe == 0));
> > > -
> > > - if (dtp->dt_pdesc == NULL)
> > > - return;
> > > -
> > > - for (i = 0; i < dtp->dt_maxprobe; i++) {
> > > - if (dtp->dt_ddesc[i] == NULL) {
> > > - assert(dtp->dt_pdesc[i] == NULL);
> > > - continue;
> > > - }
> > > -
> > > - dt_datadesc_release(dtp, dtp->dt_ddesc[i]);
> > > - assert(dtp->dt_pdesc[i] != NULL);
> > > - }
> > > -
> > > - free(dtp->dt_pdesc);
> > > - dtp->dt_pdesc = NULL;
> > > + dtrace_difo_t *rdp = dt_dlib_get_func_difo(dtp, dtp->dt_stmts[stid]->dtsd_clause);
> > > + *ddp = dt_datadesc_hold(rdp->dtdo_ddesc); // FIXME what releases the hold?
> > Well, you are the one requesting the hold, so you decide when it is to be
> > released :-)
> >
> > In all seriousness, I believe dtrace_stmt_destroy() is the place you are
> > looking for.
>
> Actually, this is a short-lived usage by the consumer. So stmt_destroy()
> would not seem to be the right place and, indeed, I just decided to drop the
> hold/release altogether.
Yes, that shouold be ok.
> > > - free(dtp->dt_ddesc);
> > > - dtp->dt_ddesc = NULL;
> > > - dtp->dt_nextepid = 0;
> > > - dtp->dt_maxprobe = 0;
> > > + return (*ddp == NULL) ? -1 : 0;
> > > }
> > > uint32_t
> > > diff --git a/test/unittest/actions/setopt/tst.badopt.r b/test/unittest/actions/setopt/tst.badopt.r
> > > index 29e39fd4..e2f6d2c3 100644
> > > --- a/test/unittest/actions/setopt/tst.badopt.r
> > > +++ b/test/unittest/actions/setopt/tst.badopt.r
> > > @@ -1,16 +1,16 @@
> > > -- @@stderr --
> > > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
> > > +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
> > Perhaps it would be more efficient to update runtest.sh to support both the
> > old and the new error reporting output, and changing both into a single unified
> > form that substitutes changeable elements.
>
> I don't get this. First, there is no reason to support the old output...
> that would be a rather remarkable historical vestige to keep in the code
> base. Second, it seems simpler and more stringent just to compare to a
> results file in some simple, straightforward way. You say "perhaps" so I
> just left it as is.
I was more thinking about making the output we compare less dependent on how
things are phrased so that we do not have to update .r files whenever there is
a change in how we output the data. Except for a test or two that verify the
exact phrasing we expect.
> > E.g.
> > dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
> >
> > and
> >
> > dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
> >
> > could both be rewritten using pattern amtching and substitutions to be:
> >
> > dtrace: error for probe (dtrace:::BEGIN): couldn't set option "Nixon" to "1": Invalid option name
> >
> > and tests that are supposed to exercise specific aspects of the error message
> > or that depend on specific details could test that explicitly?
> >
> > > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "Harding" to "1": Invalid option name
> > > +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "Harding" to "1": Invalid option name
> > > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "Hoover" to "1": Invalid option name
> > > +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "Hoover" to "1": Invalid option name
> > > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "Bush" to "1": Invalid option name
> > > +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "Bush" to "1": Invalid option name
> > > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "quiet" to "um, no": Invalid value for specified option
> > > +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "quiet" to "um, no": Invalid value for specified option
> > > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "aggrate" to "0.5hz": Invalid value for specified option
> > > +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "aggrate" to "0.5hz": Invalid value for specified option
> > > -dtrace: error on enabled probe ID 2 (ID 1: dtrace:::BEGIN): couldn't set option "bufsize" to "1m": Operation illegal when tracing is active
> > > +dtrace: error in dt_clause_0 for probe ID 1 (dtrace:::BEGIN): couldn't set option "bufsize" to "1m": Operation illegal when tracing is active
More information about the DTrace-devel
mailing list