[DTrace-devel] [PATCH v2 03/19] Deprecate enabled probe ID (epid)
Eugene Loh
eugene.loh at oracle.com
Sun Sep 8 16:31:12 UTC 2024
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???
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.
> 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.
>> /*
>> * 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.
>>
>> - 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.
> 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