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

[Kris Van Hees]

On Tue, May 09, 2023 at 01:12:31PM -0400, Eugene Loh via DTrace-devel wrote:
> 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;

I think it can go either way.  I don't really see much of a preference (and I
know that e.g. the kernel source tree seems to be allergic to typedefs for
structs, so not much guidance there).  I don't think it matters much, so
sticking with the style of the file seems reasonable?

> > +
> >   #ifdef  __cplusplus
> >   }
> >   #endif
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel