Re: [PATCH 03/22] Action clear() should clear only one aggregation

[Kris Van Hees <kris.van.hees@oracle.com>]

On Thu, Aug 29, 2024 at 01:22:00AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> In dt_clear(), we:
> 
> *)  extract an aggregation ID (dtrace_aggid_t)
> 
> *)  increment the gen counter for this aggregation ID
> 
> *)  clear the consumer copy of the aggregation, using:
> 
>       /* Also clear our own copy of the data, in case it gets printed. */
>       dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, dtp);
> 
> However, this clears all the aggregations.  While the next snapshot of
> the aggregations will refresh our copies of the aggregations -- after all,
> dtrace_aggregate_snap() itself starts with a call to dtrace_aggregate_clear()
> -- if we print aggregations before the next snapshot, we would display an
> erroneously cleared aggregation.
> 
> Therefore, change dt_clear() to clear only those aggregations that
> correspond to the specified aggregation ID.
> 
> In order that only a specific aggregation ID can be cleared, we
> modify the last argument to dt_aggregate_clear_one() to be not
> simply the dtp, but a pointer to a struct that has both the dtp
> and the aggregation ID.  The previous "clear all" behavior can be
> specified with an aggregation ID of DTRACE_AGGVARIDNONE.
> 
> Though trunc() appears not to be similarly afflicted, add a test for
> it too.
> 
> Orabug: 36900180
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

... though with minor comments/changes as listed below.

> ---
>  libdtrace/dt_aggregate.c           | 16 ++++++++++++---
>  libdtrace/dt_aggregate.h           |  5 +++++
>  libdtrace/dt_consume.c             |  4 +++-
>  test/unittest/aggs/tst.clear-one.d | 33 ++++++++++++++++++++++++++++++
>  test/unittest/aggs/tst.clear-one.r | 13 ++++++++++++
>  test/unittest/aggs/tst.trunc-one.d | 33 ++++++++++++++++++++++++++++++
>  test/unittest/aggs/tst.trunc-one.r | 10 +++++++++
>  7 files changed, 110 insertions(+), 4 deletions(-)
>  create mode 100644 test/unittest/aggs/tst.clear-one.d
>  create mode 100644 test/unittest/aggs/tst.clear-one.r
>  create mode 100644 test/unittest/aggs/tst.trunc-one.d
>  create mode 100644 test/unittest/aggs/tst.trunc-one.r
> 
> diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
> index 329b8ffa..8825739c 100644
> --- a/libdtrace/dt_aggregate.c
> +++ b/libdtrace/dt_aggregate.c
> @@ -504,7 +504,9 @@ dt_aggregate_clear_one_percpu(const dtrace_aggdata_t *agd,
>  int
>  dt_aggregate_clear_one(const dtrace_aggdata_t *agd, void *arg)
>  {
> -	dtrace_hdl_t		*dtp = arg;
> +	dt_clear_arg_t		*dca = arg;
> +	dtrace_hdl_t		*dtp = dca->dtp;
> +	dtrace_aggid_t		aid = dca->aid;
>  	dtrace_aggdesc_t	*agg = agd->dtada_desc;
>  	dtrace_recdesc_t	*rec = &agg->dtagd_drecs[DT_AGGDATA_RECORD];
>  	int64_t			*vals = (int64_t *)
> @@ -512,6 +514,9 @@ dt_aggregate_clear_one(const dtrace_aggdata_t *agd, void *arg)
>  	uint64_t		agen;
>  	int			max_cpus = dtp->dt_conf.max_cpuid + 1;
>  
> +	if (aid != DTRACE_AGGVARIDNONE && aid != agg->dtagd_varid)
> +		return DTRACE_AGGWALK_NEXT;
> +
>  	/*
>  	 * We can pass the entire key because we know that the first uint32_t
>  	 * in the key is the aggregation ID we need.
> @@ -609,7 +614,9 @@ dt_aggregate_snap_one(dtrace_hdl_t *dtp, int aggid, int cpu, const char *key,
>  		 */
>  		hgen = *(int64_t *)agd->dtada_data;
>  		if (dgen > hgen) {
> -			dt_aggregate_clear_one(agd, dtp);
> +			dt_clear_arg_t arg = { dtp, DTRACE_AGGVARIDNONE };
> +
> +			dt_aggregate_clear_one(agd, &arg);
>  			hgen = *(int64_t *)agd->dtada_data;
>  		}
>  
> @@ -1859,10 +1866,13 @@ dtrace_aggregate_print(dtrace_hdl_t *dtp, FILE *fp,
>  	return 0;
>  }
>  
> +/* FIXME: dtrace_aggregate_clear() is called from only one site.  Should we inline it? */

Removing the FIXME.  Yes, it needs to remain as-is because it is part of the
libdtrace API.

>  void
>  dtrace_aggregate_clear(dtrace_hdl_t *dtp)
>  {
> -	dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, dtp);
> +	dt_clear_arg_t arg = { dtp, DTRACE_AGGVARIDNONE };
> +
> +	dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, &arg);
>  }
>  
>  void
> diff --git a/libdtrace/dt_aggregate.h b/libdtrace/dt_aggregate.h
> index 4f8d09bf..0dc126e9 100644
> --- a/libdtrace/dt_aggregate.h
> +++ b/libdtrace/dt_aggregate.h
> @@ -12,6 +12,11 @@
>  extern "C" {
>  #endif
>  
> +typedef struct dt_clear_arg {
> +	dtrace_hdl_t	*dtp;
> +	dtrace_aggid_t	aid;
> +} dt_clear_arg_t;
> +
>  typedef struct dt_aggregate dt_aggregate_t;
>  
>  #define DT_AGGDATA_COUNTER	0
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index 0e30ddbd..f2d5cb74 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -1552,6 +1552,7 @@ dt_clear(dtrace_hdl_t *dtp, caddr_t base, dtrace_recdesc_t *rec)
>  	dtrace_aggid_t	aid;
>  	uint64_t	gen;
>  	caddr_t		addr;
> +	dt_clear_arg_t	arg = { dtp, 0 };

0 -> DTRACE_AGGVARIDNONE

>  
>  	/* We have just one record: the aggregation ID. */
>  	addr = base + rec->dtrd_offset;
> @@ -1568,7 +1569,8 @@ dt_clear(dtrace_hdl_t *dtp, caddr_t base, dtrace_recdesc_t *rec)
>  		return -1;
>  
>  	/* Also clear our own copy of the data, in case it gets printed. */
> -	dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, dtp);
> +	arg.aid = aid;
> +	dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, &arg);
>  
>  	return 0;
>  }
> diff --git a/test/unittest/aggs/tst.clear-one.d b/test/unittest/aggs/tst.clear-one.d
> new file mode 100644
> index 00000000..f96c2db0
> --- /dev/null
> +++ b/test/unittest/aggs/tst.clear-one.d
> @@ -0,0 +1,33 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2024, 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.
> + */
> +
> +/*
> + * ASSERTION: Clearing one aggregation clears only that one.
> + *
> + * SECTION: Aggregations/Clearing aggregations
> + */
> +/* @@nosort */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> +	@a[1] = sum(100);
> +	@a[2] = sum( 20);
> +	@a[3] = sum(  3);
> +	@b[4] = sum(400);
> +	@b[5] = sum( 50);
> +	@b[6] = sum(  6);
> +	@c[7] = sum(700);
> +	@c[8] = sum( 80);
> +	@c[9] = sum(  9);
> +	clear(@b);
> +	printa(@a);
> +	printa(@b);
> +	printa(@c);
> +	exit(0);
> +}
> diff --git a/test/unittest/aggs/tst.clear-one.r b/test/unittest/aggs/tst.clear-one.r
> new file mode 100644
> index 00000000..101d173f
> --- /dev/null
> +++ b/test/unittest/aggs/tst.clear-one.r
> @@ -0,0 +1,13 @@
> +
> +        3                3
> +        2               20
> +        1              100
> +
> +        4                0
> +        5                0
> +        6                0
> +
> +        9                9
> +        8               80
> +        7              700
> +
> diff --git a/test/unittest/aggs/tst.trunc-one.d b/test/unittest/aggs/tst.trunc-one.d
> new file mode 100644
> index 00000000..0556bb34
> --- /dev/null
> +++ b/test/unittest/aggs/tst.trunc-one.d
> @@ -0,0 +1,33 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2024, 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.
> + */
> +
> +/*
> + * ASSERTION: Truncating one aggregation truncates only that one.
> + *
> + * SECTION: Aggregations/Clearing aggregations
> + */
> +/* @@nosort */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> +	@a[1] = sum(100);
> +	@a[2] = sum( 20);
> +	@a[3] = sum(  3);
> +	@b[4] = sum(400);
> +	@b[5] = sum( 50);
> +	@b[6] = sum(  6);
> +	@c[7] = sum(700);
> +	@c[8] = sum( 80);
> +	@c[9] = sum(  9);
> +	trunc(@b);
> +	printa(@a);
> +	printa(@b);
> +	printa(@c);
> +	exit(0);
> +}
> diff --git a/test/unittest/aggs/tst.trunc-one.r b/test/unittest/aggs/tst.trunc-one.r
> new file mode 100644
> index 00000000..a611026d
> --- /dev/null
> +++ b/test/unittest/aggs/tst.trunc-one.r
> @@ -0,0 +1,10 @@
> +
> +        3                3
> +        2               20
> +        1              100
> +
> +
> +        9                9
> +        8               80
> +        7              700
> +
> -- 
> 2.43.5
>