[DTrace-devel] [PATCH v2] Implement the trunc() action

[Kris Van Hees]

On Fri, Aug 18, 2023 at 04:14:01PM -0400, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

Thanks.

> with minor comments...
> 
> On 8/18/23 09:44, 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.
> 
> How true is this?? trunc0 maybe, because it tries doing more aggs after 
> the trunc?? But even this test gets lucky?? The rest of the tests should 
> be okay, because the only trunc is at the very end.? So instead of "some 
> tests will not" maybe just "trunc0 might not"?? And if that's the case, 
> it shouldn't be in the commit message but an "@@unstable" in the test 
> itself?

I will change "will" to "may".  I know there are timing related issues that
can interfere, so there is nothing wrong with being cautious.

> > diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
> > @@ -1086,6 +1086,35 @@ no_mem:
> > +/*
> > + * Remove the given aggregation entry on the producer side.  We use the global
> > + * key scratch space because this function cannot be called from a call chain
> > + * that already uses that global key scratch.
> > + */
> 
> On the producer side?? Maybe just "in the BPF map"?

The BPF map *is* on the producer side.  The producer/consumer terminology is
common to DTrace.

> > +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;
> > +	char			*key = dtp->dt_aggregate.dtat_key;
> > +
> > +	memset(key, 0, dtp->dt_maxtuplesize);
> > +	memcpy(key, agd->dtada_key, agd->dtada_desc->dtagd_ksize);
> > +
> > +	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, key);
> > +		close(fd);
> > +	}
> > +
> > +	return DTRACE_WORKSTATUS_OKAY;
> > +}
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > @@ -2287,32 +2282,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);
> > +
> 
> I know we do this in other places, but (mainly just for my personal 
> education I guess) do we care about memory leaks for stuff like this?

Why would this be a memory leak?  All such nodes are free'd after the
compilation they belong to completes.

> >   	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");
> >   }
> >   
>