[DTrace-devel] [PATCH 1/6] bpf: add 'agggen' BPF map

[Eugene Loh]

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

It's a little weird that the name dt_genmap_fd does not have the 
substring "agg" in it, though.  The fd is definitely related to aggs, 
and the map is even called agggen.

If this mechanism will also be used for trunc(), it'd be nice (but not 
necessary) to mention this in the commit message.

To me, the commit message is unnecessarily vague when it could be more 
specific.  E.g., "It will be used..."  Okay, but how?  To me, a more 
specific roadmap could be given:

*)  The consumer will write the per-aggid counter to the map:  1 at 
first, and incrementing it at every clear().

*)  The producer will use the counter to mark the per-aggid, per-CPU, 
per-key aggs that it writes and maintains.

On 2/22/23 10:17, Kris Van Hees via DTrace-devel wrote:
> The 'agggen' BPF map associates a generation counter with an aggregation
> ID.  It will be used to determine whether the aggregation data in the
> 'aggs' map for aggregations associated with a given aggregation ID is
> valid or stale.
>
> The initial generation counter for each aggregation is 1 (because 0 is
> used to indicate a no-data condition).
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_bpf.c                  | 38 +++++++++++++++++++++++------
>   libdtrace/dt_dlibs.c                |  1 +
>   libdtrace/dt_impl.h                 |  1 +
>   test/stress/buffering/err.resize2.r |  2 +-
>   4 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index f0f46c5c..6587ea95 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -477,18 +477,23 @@ gmap_create_state(dtrace_hdl_t *dtp)
>   }
>   
>   /*
> - * Create the 'aggs' BPF map.
> + * Create the 'aggs' BPF map (and its companion 'agggen' BPF map).
>    *
> - * Aggregation data buffer map, associated with each CPU.  The map is
> - * implemented as a global array-of-maps indexed by CPU id.  The associated
> - * value is a map with a singleton element (key 0).
> + * - aggs:	Aggregation data buffer map, associated with each CPU.  The map
> + *		is implemented as a global array-of-maps indexed by CPU id.
> + *		The associated value is a map with a singleton element (key 0).
> + * - agggen:	Aggregation generation counters.  The map associates a
> + *		generation counter with each aggregation ID.  The counter is
> + *		used to determine whether the aggregation data is valid.  The
> + *		initial value is 1.
>    */
>   static int
>   gmap_create_aggs(dtrace_hdl_t *dtp)
>   {
> -	size_t	ncpus = dtp->dt_conf.max_cpuid + 1;
> -	size_t	nelems = 0;
> -	int	i;
> +	size_t		ncpus = dtp->dt_conf.max_cpuid + 1;
> +	size_t		nelems = 0;
> +	uint32_t	aggc =  dt_idhash_peekid(dtp->dt_aggs);
> +	uint32_t	i;
>   
>   	/* Only create the map if it is used. */
>   	if (dtp->dt_maxaggdsize == 0)
> @@ -508,6 +513,8 @@ gmap_create_aggs(dtrace_hdl_t *dtp)
>   						BPF_MAP_TYPE_HASH,
>   						dtp->dt_maxtuplesize,
>   						dtp->dt_maxaggdsize, nelems);
> +	if (dtp->dt_aggmap_fd == -1)
> +		return -1;
>   
>   	for (i = 0; i < dtp->dt_conf.num_online_cpus; i++) {
>   		int	cpu = dtp->dt_conf.cpus[i].cpu_id;
> @@ -524,8 +531,23 @@ gmap_create_aggs(dtrace_hdl_t *dtp)
>   		dt_bpf_map_update(dtp->dt_aggmap_fd, &cpu, &fd);
>   	}
>   
> +	/* Create the agg generation value array. */
> +	dtp->dt_genmap_fd = create_gmap(dtp, "agggen", BPF_MAP_TYPE_ARRAY,
> +					sizeof(uint32_t), sizeof(uint64_t),
> +					aggc);
> +	if (dtp->dt_genmap_fd == -1)
> +		 return -1;
> +
> +	for (i = 0; i < aggc; i++) {
> +		uint64_t	val = 1;
>   
> -	return dtp->dt_aggmap_fd;
> +		if (dt_bpf_map_update(dtp->dt_genmap_fd, &i, &val) == -1)
> +			return dt_bpf_error(dtp,
> +					    "cannot update BPF map 'agggen': %s\n",
> +					    strerror(errno));
> +	}
> +
> +	return 0;
>   }
>   
>   /*
> diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> index c302b19c..eea0ba41 100644
> --- a/libdtrace/dt_dlibs.c
> +++ b/libdtrace/dt_dlibs.c
> @@ -57,6 +57,7 @@ static const dt_ident_t		dt_bpf_symbols[] = {
>   
>   	/* BPF maps */
>   	DT_BPF_SYMBOL(aggs, DT_IDENT_PTR),
> +	DT_BPF_SYMBOL(agggen, DT_IDENT_PTR),
>   	DT_BPF_SYMBOL(buffers, DT_IDENT_PTR),
>   	DT_BPF_SYMBOL(cpuinfo, DT_IDENT_PTR),
>   	DT_BPF_SYMBOL(dvars, DT_IDENT_PTR),
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 3e14d69d..32b0c923 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -380,6 +380,7 @@ struct dtrace_hdl {
>   	int dt_proc_fd;		/* file descriptor for proc eventfd */
>   	int dt_stmap_fd;	/* file descriptor for the 'state' BPF map */
>   	int dt_aggmap_fd;	/* file descriptor for the 'aggs' BPF map */
> +	int dt_genmap_fd;	/* file descriptor for the 'agggen' BPF map */
>   	dtrace_handle_err_f *dt_errhdlr; /* error handler, if any */
>   	void *dt_errarg;	/* error handler argument */
>   	dtrace_handle_drop_f *dt_drophdlr; /* drop handler, if any */
> diff --git a/test/stress/buffering/err.resize2.r b/test/stress/buffering/err.resize2.r
> index a4fbe390..2b5728ca 100644
> --- a/test/stress/buffering/err.resize2.r
> +++ b/test/stress/buffering/err.resize2.r
> @@ -1,3 +1,3 @@
>   -- @@stderr --
>   dtrace: script 'test/stress/buffering/err.resize2.d' matched 1 probe
> -dtrace: could not enable tracing: failed to create BPF map 'aggs_0': Too big
> +dtrace: could not enable tracing: failed to create BPF map 'aggs': Too big