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

Kris Van Hees
2024-09-06T21:43:55Z

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
>