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

[Eugene Loh]

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?

> 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.

> +}
> +
>   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.

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