[DTrace-devel] [PATCH] Implement the trunc() action
Kris Van Hees
kris.van.hees at oracle.com
Fri Aug 4 16:38:18 UTC 2023
On Fri, Aug 04, 2023 at 10:30:43AM -0400, Eugene Loh via DTrace-devel wrote:
> Just a progress report on this one... The patch looks good to me, but the
> tests don't seem to work for me. The patch takes the xfail off of negtrunc
> and trunc, but trunc0 is the test that is starting to pass for me. Also, I
> have a separate test that sets up some agg with multiple keys, waits a few
> seconds, truncates, waits a few more seconds, and exits. The truncation
> seems incomplete: I can see (in different ways, but including "bpftool
> map") that some keys that should be removed nonetheless remain, and then
> dtrace finds and reports them. More specifically, I have a UEKr6 VM on
> which I see all those problems and a UEKr7 VM on which I see only the
> testsuite problems (but no issue with my special test). I'll poke more, but
> this is frustrating for a patch that isn't that long and looks fine to me.
This is almost certainly artifacts due to *rate implementation issues. The
issue is that trunc() is executed at the consumer side, and therefore the
moment at which it is processed affects what gets truncated. But data is
often still being generated between the point of trunc() being indicated in
the output buffer and it actually being executed. And due to the way *rate
settings are currently (incorrectly) implemented, the aggregation data may
have been retrieved at a point that does not sufficiently coincide with the
retrieval of the output buffer data.
Yet, I think a problem that might be popping up is that bpf_map_delete_elem()
could be failing somehow. I'm investigating that possibility.
> On 8/3/23 01:09, Kris Van Hees via DTrace-devel wrote:
> > Some tests will not yield the desired results yet due to issues with
> > switchrate/aggrate/statusrate implementation details that those
> > tests depend on.
> >
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> > libdtrace/dt_aggregate.c | 22 ++++++++++++++++++++++
> > libdtrace/dt_cg.c | 27 ++++-----------------------
> > libdtrace/dt_consume.c | 19 +++++++++++++------
> > libdtrace/dt_open.c | 2 +-
> > test/unittest/aggs/tst.negtrunc.d | 1 -
> > test/unittest/aggs/tst.trunc.d | 1 -
> > 6 files changed, 40 insertions(+), 32 deletions(-)
> >
> > diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
> > index 7c09908d..24faf1c1 100644
> > --- a/libdtrace/dt_aggregate.c
> > +++ b/libdtrace/dt_aggregate.c
> > @@ -1086,6 +1086,26 @@ no_mem:
> > return dt_set_errno(dtp, EDT_NOMEM);
> > }
> > +static int
> > +dt_aggwalk_remove(dtrace_hdl_t *dtp, dt_ahashent_t *h)
> > +{
> > + dtrace_aggdata_t *agd = &h->dtahe_data;
> > + int i, ncpus = dtp->dt_conf.num_online_cpus;
> > +
> > + for (i = 0; i < ncpus; i++) {
> > + int cpu = dtp->dt_conf.cpus[i].cpu_id;
> > + int fd = dt_bpf_map_lookup_fd(dtp->dt_aggmap_fd, &cpu);
> > +
> > + if (fd < 0)
> > + return DTRACE_WORKSTATUS_ERROR;
> > +
> > + dt_bpf_map_delete(fd, agd->dtada_key);
> > + close(fd);
> > + }
> > +
> > + return DTRACE_WORKSTATUS_OKAY;
> > +}
> > +
> > static int
> > dt_aggwalk_rval(dtrace_hdl_t *dtp, dt_ahashent_t *h, int rval)
> > {
> > @@ -1148,6 +1168,8 @@ dt_aggwalk_rval(dtrace_hdl_t *dtp, dt_ahashent_t *h, int rval)
> > dt_free(dtp, aggdata->dtada_percpu);
> > }
> > + dt_aggwalk_remove(dtp, h);
> > +
> > dt_free(dtp, aggdata->dtada_key);
> > dt_free(dtp, aggdata->dtada_data);
> > dt_free(dtp, h);
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index 5391a143..82db9bd7 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -2261,11 +2261,6 @@ dt_cg_act_trunc(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> > dt_node_t *anp, *trunc;
> > dt_ident_t *aid;
> > char n[DT_TYPE_NAMELEN];
> > - int argc = 0;
> > -
> > - for (anp = dnp->dn_args; anp != NULL; anp = anp->dn_list)
> > - argc++;
> > - assert(argc == 1 || argc == 2);
> > anp = dnp->dn_args;
> > assert(anp != NULL);
> > @@ -2276,32 +2271,18 @@ dt_cg_act_trunc(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> > dnp->dn_ident->di_name,
> > dt_node_type_name(anp, n, sizeof(n)));
> > - trunc = anp->dn_list;
> > - if (argc == 2)
> > - assert(trunc != NULL && dt_node_is_scalar(trunc));
> > -
> > aid = anp->dn_ident;
> > if (aid->di_gen == pcb->pcb_hdl->dt_gen &&
> > !(aid->di_flags & DT_IDFLG_MOD))
> > dnerror(dnp, D_TRUNC_AGGBAD,
> > "undefined aggregation: @%s\n", aid->di_name);
> > - /*
> > - * FIXME: Needs implementation
> > - * TODO: Emit code to truncate the given aggregation.
> > - * DEPENDS ON: How aggregations are implemented using eBPF (hashmap?).
> > - * AGGID = aid->di_id
> > - */
> > + trunc = anp->dn_list;
> > + if (trunc == NULL)
> > + trunc = dt_node_int(0);
> > +
> > dt_cg_store_val(pcb, anp, DTRACEACT_LIBACT, NULL, DT_ACT_TRUNC);
> > -#ifdef FIXME
> > - /*
> > - * FIXME: There is an optional trunction value.
> > - * if (argc == 1), the optional value is missing, but "0" may be
> > - * specified.
> > - */
> > dt_cg_store_val(pcb, trunc, DTRACEACT_LIBACT, NULL, DT_ACT_TRUNC);
> > -#endif
> > - dnerror(dnp, D_UNKNOWN, "trunc() is not implemented (yet)\n");
> > }
> > static void
> > diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> > index 43a9cea3..a67c0f7b 100644
> > --- a/libdtrace/dt_consume.c
> > +++ b/libdtrace/dt_consume.c
> > @@ -1537,7 +1537,6 @@ dt_clear(dtrace_hdl_t *dtp, caddr_t base, dtrace_recdesc_t *rec)
> > return 0;
> > }
> > -#ifdef FIXME
> > typedef struct dt_trunc {
> > dtrace_aggid_t dttd_id;
> > uint64_t dttd_remaining;
> > @@ -1594,19 +1593,19 @@ dt_trunc(dtrace_hdl_t *dtp, caddr_t base, dtrace_recdesc_t *rec)
> > addr = base + rec->dtrd_offset;
> > switch (rec->dtrd_size) {
> > - case sizeof(uint64_t):
> > + case sizeof(int64_t):
> > /* LINTED - alignment */
> > remaining = *((int64_t *)addr);
> > break;
> > - case sizeof(uint32_t):
> > + case sizeof(int32_t):
> > /* LINTED - alignment */
> > remaining = *((int32_t *)addr);
> > break;
> > - case sizeof(uint16_t):
> > + case sizeof(int16_t):
> > /* LINTED - alignment */
> > remaining = *((int16_t *)addr);
> > break;
> > - case sizeof(uint8_t):
> > + case sizeof(int8_t):
> > remaining = *((int8_t *)addr);
> > break;
> > default:
> > @@ -1626,7 +1625,6 @@ dt_trunc(dtrace_hdl_t *dtp, caddr_t base, dtrace_recdesc_t *rec)
> > return 0;
> > }
> > -#endif
> > static int
> > dt_print_datum(dtrace_hdl_t *dtp, FILE *fp, dtrace_recdesc_t *rec,
> > @@ -2344,6 +2342,15 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
> > if (dt_clear(dtp, data, rec) != 0)
> > return DTRACE_WORKSTATUS_ERROR;
> > + continue;
> > + case DT_ACT_TRUNC:
> > + if (i == epd->dtdd_nrecs - 1)
> > + return dt_set_errno(dtp, EDT_BADTRUNC);
> > +
> > + if (dt_trunc(dtp, data, rec) != 0)
> > + return DTRACE_WORKSTATUS_ERROR;
> > +
> > + i++;
> > continue;
> > case DT_ACT_FTRUNCATE:
> > if (fp == NULL)
> > diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> > index b6a0b623..93e8535a 100644
> > --- a/libdtrace/dt_open.c
> > +++ b/libdtrace/dt_open.c
> > @@ -343,7 +343,7 @@ static const dt_ident_t _dtrace_globals[] = {
> > DT_ATTR_STABCMN, DT_VERS_1_0,
> > &dt_idops_func, "void(@, size_t, [size_t])" },
> > { "trunc", DT_IDENT_ACTFUNC, 0, DT_ACT_TRUNC, DT_ATTR_STABCMN,
> > - DT_VERS_1_0, &dt_idops_func, "void(@, [uint64_t])" },
> > + DT_VERS_1_0, &dt_idops_func, "void(@, [int64_t])" },
> > { "uaddr", DT_IDENT_ACTFUNC, 0, DT_ACT_UADDR, DT_ATTR_STABCMN,
> > DT_VERS_1_2, &dt_idops_func, "_usymaddr(uintptr_t)" },
> > { "ucaller", DT_IDENT_SCALAR, 0, DIF_VAR_UCALLER, DT_ATTR_STABCMN,
> > diff --git a/test/unittest/aggs/tst.negtrunc.d b/test/unittest/aggs/tst.negtrunc.d
> > index 2c35151b..97db1967 100644
> > --- a/test/unittest/aggs/tst.negtrunc.d
> > +++ b/test/unittest/aggs/tst.negtrunc.d
> > @@ -4,7 +4,6 @@
> > * Licensed under the Universal Permissive License v 1.0 as shown at
> > * http://oss.oracle.com/licenses/upl.
> > */
> > -/* @@xfail: dtv2 - requires trunc() */
> > #pragma D option quiet
> > diff --git a/test/unittest/aggs/tst.trunc.d b/test/unittest/aggs/tst.trunc.d
> > index 0c0496a5..dbaeb09a 100644
> > --- a/test/unittest/aggs/tst.trunc.d
> > +++ b/test/unittest/aggs/tst.trunc.d
> > @@ -4,7 +4,6 @@
> > * Licensed under the Universal Permissive License v 1.0 as shown at
> > * http://oss.oracle.com/licenses/upl.
> > */
> > -/* @@xfail: dtv2 - requires trunc() */
> > #pragma D option quiet
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list