[DTrace-devel] [PATCH] Action clear() should clear only one aggregation
Kris Van Hees
kris.van.hees at oracle.com
Wed Aug 14 15:57:42 UTC 2024
On Tue, Aug 13, 2024 at 10:59:25PM -0400, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
> FIXME: Is libdtrace/libdtrace.ver correctly updated?
>
> 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.
>
> Note that while we have "aggregation-walk" functions like
>
> dtrace_aggregate_walk()
> dtrace_aggregate_walk_joined()
> dtrace_aggregate_walk_*sorted() (many of these)
>
> the first one (dtrace_aggregate_walk) is used only twice:
>
> - in dtrace_aggregate_clear() to clear all aggregations
>
> - in dt_clear(), where we should clear only a specific aggregation ID
>
> Therefore, rename dtrace_aggregate_walk() to be
> dtrace_aggregate_walk_oneid() that takes an aggid argument. If
> aggid==DTRACE_AGGVARIDNONE, walk all aggregations. Otherwise, walk
> only those aggregations that match the specified id.
I think that keeping dtrace_aggregate_walk() as-is would be better, along with
adding a function dtrace_aggregate_clear_one(dtp, aid) that performs the
lookup from aid to dtrae_aggdata_t *agd, and then calls
dt__aggregate_clear_one(agd, dtp) to do the actual clearing. There is no need
to go through a walker function when we already know we need to operate on
just one aggregation.
> Though trunc() appears not to be similarly afflicted, add a test for it
> too.
>
> Orabug: 36900180
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> libdtrace/dt_aggregate.c | 8 ++++++--
> libdtrace/dt_consume.c | 2 +-
> libdtrace/dtrace.h | 4 ++--
> libdtrace/libdtrace.ver | 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 +++++++++
> 8 files changed, 100 insertions(+), 7 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 51f0d622..e06025de 100644
> --- a/libdtrace/dt_aggregate.c
> +++ b/libdtrace/dt_aggregate.c
> @@ -1278,7 +1278,8 @@ dt_aggregate_qsort(dtrace_hdl_t *dtp, void *base, size_t nel, size_t width,
> }
>
> int
> -dtrace_aggregate_walk(dtrace_hdl_t *dtp, dtrace_aggregate_f *func, void *arg)
> +dtrace_aggregate_walk_oneid(dtrace_hdl_t *dtp, dtrace_aggregate_f *func,
> + void *arg, dtrace_aggid_t aid)
> {
> dt_ahashent_t *h, *next;
> dt_ahash_t *hash = &dtp->dt_aggregate.dtat_hash;
> @@ -1291,6 +1292,8 @@ dtrace_aggregate_walk(dtrace_hdl_t *dtp, dtrace_aggregate_f *func, void *arg)
> */
> next = h->dtahe_nextall;
>
> + if (aid != DTRACE_AGGVARIDNONE && aid != h->dtahe_data.dtada_desc->dtagd_varid)
> + continue;
> if (dt_aggwalk_rval(dtp, h, func(&h->dtahe_data, arg)) == -1)
> return -1;
> }
> @@ -1820,10 +1823,11 @@ 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? */
> void
> dtrace_aggregate_clear(dtrace_hdl_t *dtp)
> {
> - dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, dtp);
> + dtrace_aggregate_walk_oneid(dtp, dt_aggregate_clear_one, dtp, DTRACE_AGGVARIDNONE);
> }
>
> void
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index 8d8c0f54..238b93ca 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -1551,7 +1551,7 @@ 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);
> + dtrace_aggregate_walk_oneid(dtp, dt_aggregate_clear_one, dtp, aid);
>
> return 0;
> }
> diff --git a/libdtrace/dtrace.h b/libdtrace/dtrace.h
> index 1f54c10c..4df3ebf4 100644
> --- a/libdtrace/dtrace.h
> +++ b/libdtrace/dtrace.h
> @@ -396,8 +396,8 @@ extern int dtrace_aggregate_snap(dtrace_hdl_t *dtp);
> extern int dtrace_aggregate_print(dtrace_hdl_t *dtp, FILE *fp,
> dtrace_aggregate_walk_f *func); /* func must take dtrace_print_aggdata_t */
>
> -extern int dtrace_aggregate_walk(dtrace_hdl_t *dtp, dtrace_aggregate_f *func,
> - void *arg);
> +extern int dtrace_aggregate_walk_oneid(dtrace_hdl_t *dtp, dtrace_aggregate_f *func,
> + void *arg, dtrace_aggid_t);
>
> extern int dtrace_aggregate_walk_joined(dtrace_hdl_t *dtp,
> dtrace_aggid_t *aggvars, int naggvars,
> diff --git a/libdtrace/libdtrace.ver b/libdtrace/libdtrace.ver
> index edbd5a1d..1510ad95 100644
> --- a/libdtrace/libdtrace.ver
> +++ b/libdtrace/libdtrace.ver
> @@ -1,6 +1,6 @@
> #
> # Oracle Linux DTrace.
> -# Copyright (c) 2009, 2023, Oracle and/or its affiliates. All rights reserved.
> +# Copyright (c) 2009, 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.
>
> @@ -10,7 +10,7 @@ LIBDTRACE_1.0 {
> dtrace_aggregate_clear;
> dtrace_aggregate_print;
> dtrace_aggregate_snap;
> - dtrace_aggregate_walk;
> + dtrace_aggregate_walk_oneid;
> dtrace_aggregate_walk_joined;
> dtrace_aggregate_walk_keyrevsorted;
> dtrace_aggregate_walk_keysorted;
> 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
>
More information about the DTrace-devel
mailing list