[DTrace-devel] [PATCH 3/6] agg: split clearing of one aggregation into its own function

[Kris Van Hees]

On Thu, Feb 23, 2023 at 07:50:13PM -0500, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> 
> with a few minor comments below...
> 
> On 2/22/23 10:17, Kris Van Hees via DTrace-devel wrote:
> > The new dt_aggregate_clear_one() function is designed to allow it to
> > be passed a callback function to the aggregation walk functions.  It
> 
> Is a word missing after "passed"?  Like "passed in" or something?

"passed to"

> 
> > will be used in that manner in future patches.
> > 
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> >   libdtrace/dt_aggregate.c | 70 +++++++++++++++++++++-------------------
> >   libdtrace/dt_impl.h      |  1 +
> >   2 files changed, 37 insertions(+), 34 deletions(-)
> > 
> > diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
> > index 1f71d9d0..41fa0da1 100644
> > --- a/libdtrace/dt_aggregate.c
> > +++ b/libdtrace/dt_aggregate.c
> > @@ -438,6 +438,40 @@ dt_agg_one_agg(dt_ident_t *aid, dtrace_recdesc_t *rec, char *dst,
> >   	}
> >   }
> > +int
> > +dt_aggregate_clear_one(const dtrace_aggdata_t *agd, void *arg)
> > +{
> > +	dtrace_hdl_t		*dtp = arg;
> > +	dtrace_aggdesc_t	*agg = agd->dtada_desc;
> > +	dtrace_recdesc_t	*rec = &agg->dtagd_drecs[DT_AGGDATA_RECORD];
> > +	int64_t			*vals = (int64_t *)
> > +					&agd->dtada_data[rec->dtrd_offset];
> > +	int			i, max_cpus = dtp->dt_conf.max_cpuid + 1;
> > +
> > +	switch (rec->dtrd_action) {
> > +	case DT_AGG_MIN:
> > +		*vals = INT64_MAX;
> > +		if (agd->dtada_percpu)
> > +			for (i = 0; i < max_cpus; i++)
> > +				*((uint64_t*)agd->dtada_percpu[i]) = INT64_MAX;
> > +		break;
> > +	case DT_AGG_MAX:
> > +		*vals = INT64_MIN;
> > +		if (agd->dtada_percpu)
> > +			for (i = 0; i < max_cpus; i++)
> > +				*((uint64_t*)agd->dtada_percpu[i]) = INT64_MIN;
> > +		break;
> > +	default:
> > +		memset(vals, 0, rec->dtrd_size);
> > +		if (agd->dtada_percpu)
> > +			for (i = 0; i < max_cpus; i++)
> > +				memset(agd->dtada_percpu[i], 0, rec->dtrd_size);
> > +		break;
> > +	}
> > +
> > +	return 0;
> 
> dt_aggregate_clear_one() should return DTRACE_AGGWALK_NEXT rather than 0,
> even though they're the same thing.

Good point.

> 
> > +}
> > +
> >   static int
> >   dt_aggregate_snap_one(dtrace_hdl_t *dtp, int aggid, int cpu, const char *key,
> >   		      const char *data)
> > @@ -1695,41 +1729,9 @@ dtrace_aggregate_clear(dtrace_hdl_t *dtp)
> >   	dt_aggregate_t		*agp = &dtp->dt_aggregate;
> >   	dt_ahash_t		*hash = &agp->dtat_hash;
> >   	dt_ahashent_t		*h;
> > -	int			i, max_cpus = dtp->dt_conf.max_cpuid + 1;
> > -	for (h = hash->dtah_all; h != NULL; h = h->dtahe_nextall) {
> > -		dtrace_aggdesc_t	*agg;
> > -		dtrace_aggdata_t	*agd;
> > -		dtrace_recdesc_t	*rec;
> > -		int64_t			*vals;
> > -
> > -		agg = h->dtahe_data.dtada_desc;
> > -		agd = &h->dtahe_data;
> > -		rec = &agg->dtagd_drecs[DT_AGGDATA_RECORD];
> > -		vals = (int64_t *)&agd->dtada_data[rec->dtrd_offset];
> > -
> > -		switch (rec->dtrd_action) {
> > -		case DT_AGG_MIN:
> > -			*vals = INT64_MAX;
> > -			if (agd->dtada_percpu)
> > -				for (i = 0; i < max_cpus; i++)
> > -					*((uint64_t*)agd->dtada_percpu[i]) = INT64_MAX;
> > -			break;
> > -		case DT_AGG_MAX:
> > -			*vals = INT64_MIN;
> > -			if (agd->dtada_percpu)
> > -				for (i = 0; i < max_cpus; i++)
> > -					*((uint64_t*)agd->dtada_percpu[i]) = INT64_MIN;
> > -			break;
> > -		default:
> > -			memset(vals, 0, rec->dtrd_size);
> > -			if (agd->dtada_percpu)
> > -				for (i = 0; i < max_cpus; i++)
> > -					memset(agd->dtada_percpu[i], 0, rec->dtrd_size);
> > -			break;
> > -		}
> > -
> > -	}
> > +	for (h = hash->dtah_all; h != NULL; h = h->dtahe_nextall)
> > +		dt_aggregate_clear_one(&h->dtahe_data, dtp);
> 
> Should that for loop be replaced with:
>     dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, dtp)?
> You will use such a construct in a future patch anyhow.

Good idea.

> 
> >   }
> >   void
> > diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> > index 32b0c923..5d09c6e0 100644
> > --- a/libdtrace/dt_impl.h
> > +++ b/libdtrace/dt_impl.h
> > @@ -756,6 +756,7 @@ extern dtrace_difo_t *dt_difo_copy(dtrace_hdl_t *dtp, const dtrace_difo_t *odp);
> >   extern int dt_aggregate_go(dtrace_hdl_t *);
> >   extern int dt_aggregate_init(dtrace_hdl_t *);
> >   extern void dt_aggregate_destroy(dtrace_hdl_t *);
> > +extern int dt_aggregate_clear_one(const dtrace_aggdata_t *, void *);
> >   extern int dt_consume_init(dtrace_hdl_t *);
> >   extern void dt_consume_fini(dtrace_hdl_t *);
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel