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

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

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?

> 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"?

> +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?

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