[DTrace-devel] [PATCH 2/4] bpf: store per-CPU drop counts in the 'cpuinfo' map

[Eugene Loh]

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

On 5/9/23 03:17, Kris Van Hees via DTrace-devel wrote:
> Drop counters for principal and aggregation buffers are tracked per-CPU
> and thus need to be stored per-CPU.  The 'cpuinfo' map is a convenient
> place to put those so add them in after the actual cpuinfo data for a
> given CPU.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_bpf.c      | 20 ++++++++++----------
>   libdtrace/dt_bpf_maps.h | 10 +++++++++-
>   2 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index f247d83d..3cf8ef89 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -604,22 +604,22 @@ gmap_create_buffers(dtrace_hdl_t *dtp)
>   static int
>   gmap_create_cpuinfo(dtrace_hdl_t *dtp)
>   {
> -	int		i, fd, rc;
> -	uint32_t	key = 0;
> -	dtrace_conf_t	*conf = &dtp->dt_conf;
> -	size_t		ncpus = conf->max_cpuid + 1;
> -	cpuinfo_t	*data;
> -	cpuinfo_t	*ci;
> -
> -	data = dt_zalloc(dtp, ncpus * sizeof(cpuinfo_t));
> +	int			i, fd, rc;
> +	uint32_t		key = 0;
> +	dtrace_conf_t		*conf = &dtp->dt_conf;
> +	size_t			ncpus = conf->max_cpuid + 1;
> +	dt_bpf_cpuinfo_t	*data;
> +	cpuinfo_t		*ci;
> +
> +	data = dt_zalloc(dtp, ncpus * sizeof(dt_bpf_cpuinfo_t));
>   	if (data == NULL)
>   		return dt_set_errno(dtp, EDT_NOMEM);
>   
>   	for (i = 0, ci = &conf->cpus[0]; i < ncpus; i++, ci++)
> -		memcpy(&data[ci->cpu_id], ci, sizeof(cpuinfo_t));
> +		memcpy(&data[ci->cpu_id].ci, ci, sizeof(cpuinfo_t));
>   
>   	fd = create_gmap(dtp, "cpuinfo", BPF_MAP_TYPE_PERCPU_ARRAY,
> -			 sizeof(uint32_t), sizeof(cpuinfo_t), 1);
> +			 sizeof(uint32_t), sizeof(dt_bpf_cpuinfo_t), 1);
>   	if (fd == -1)
>   		return -1;
>   
> diff --git a/libdtrace/dt_bpf_maps.h b/libdtrace/dt_bpf_maps.h
> index e33748b7..074a95db 100644
> --- a/libdtrace/dt_bpf_maps.h
> +++ b/libdtrace/dt_bpf_maps.h
> @@ -1,6 +1,6 @@
>   /*
>    * Oracle Linux DTrace.
> - * Copyright (c) 2019, 2021, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
>    * Licensed under the Universal Permissive License v 1.0 as shown at
>    * http://oss.oracle.com/licenses/upl.
>    */
> @@ -13,6 +13,7 @@ extern "C" {
>   #endif
>   
>   #include <stddef.h>
> +#include <dtrace/conf.h>
>   
>   typedef struct dt_bpf_probe	dt_bpf_probe_t;
>   struct dt_bpf_probe {
> @@ -29,6 +30,13 @@ struct dt_bpf_specs {
>   					 * drain this buffer */
>   };
>   
> +typedef struct dt_bpf_cpuinfo	dt_bpf_cpuinfo_t;
> +struct dt_bpf_cpuinfo {
> +	cpuinfo_t	ci;
> +	uint64_t	buf_drops;
> +	uint64_t	agg_drops;
> +};

I realize this is the style of this file (and a few other places), but 
why not use the much more common practice of combining the typedef with 
the struct declaration.  E.g.,

typedef struct dt_bpf_cpuinfo {
    ...yadayada...
} dt_bpf_cpuinfo_t;

> +
>   #ifdef  __cplusplus
>   }
>   #endif