[DTrace-devel] [PATCH 6/6] Implement clear()
Eugene Loh
eugene.loh at oracle.com
Fri Feb 24 02:29:03 UTC 2023
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
with comments...
E.g., if xfail is changed for a test, should the copyright year be
updated? Whichever position is chosen could perhaps be documented in
the CODING-STYLE file.
And, for the entire patch series, copyright years should be updated.
E.g., dt_aggregation.c, etc.
On 2/22/23 10:17, Kris Van Hees via DTrace-devel wrote:
> The clear() action is implemented by incrementing the global generation
> counter for an aggregation name and clearing the data for matching
> aggregtions within the consumer. If aggregation data in a snapshot is
s/aggregtions/aggregations/
> found to have an older generation, it is discarded. The producer side
> will already clear its data when the generation counter is found to be
> higher than the generation of the data in a CPU's agg buffer.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
> libdtrace/dt_aggregate.c | 41 ++++++++++++++++++-----
> libdtrace/dt_cg.c | 7 ----
> libdtrace/dt_consume.c | 32 ++++++++++++++++++
> test/unittest/aggs/tst.clear.d | 1 -
> test/unittest/aggs/tst.clearavg.d | 1 -
> test/unittest/aggs/tst.clearavg2.d | 2 +-
> test/unittest/aggs/tst.cleardenormalize.d | 1 -
> test/unittest/aggs/tst.clearlquantize.d | 1 -
> test/unittest/aggs/tst.clearnormalize.d | 1 -
> 9 files changed, 65 insertions(+), 22 deletions(-)
>
> diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
> index 2129c867..4adb3382 100644
> --- a/libdtrace/dt_aggregate.c
> +++ b/libdtrace/dt_aggregate.c
> @@ -446,8 +446,17 @@ dt_aggregate_clear_one(const dtrace_aggdata_t *agd, void *arg)
> dtrace_recdesc_t *rec = &agg->dtagd_drecs[DT_AGGDATA_RECORD];
> int64_t *vals = (int64_t *)
> &agd->dtada_data[rec->dtrd_offset];
> + uint64_t agen;
> int i, max_cpus = dtp->dt_conf.max_cpuid + 1;
>
> + /*
> + * We can pass the entire key because we know that the first uint32_t
> + * in the key is the aggreggation ID we need.
s/aggreggation/aggregation/
> + */
> + if (dt_bpf_map_lookup(dtp->dt_genmap_fd, agd->dtada_key, &agen) < 0)
> + agen = 0;
> + *(uint64_t *)agd->dtada_data = agen;
> +
> switch (rec->dtrd_action) {
> case DT_AGG_MIN:
> *vals = INT64_MAX;
> @@ -483,12 +492,14 @@ dt_aggregate_snap_one(dtrace_hdl_t *dtp, int aggid, int cpu, const char *key,
> dtrace_recdesc_t *rec;
> char *ptr;
> uint64_t hval = aggid;
> + uint64_t dgen;
> size_t ndx = hval % agh->dtah_size;
> size_t off, size;
> int i, rval;
>
> - /* Skip this aggregation if the data counter is 0. */
> - if (*(int64_t *)data == 0)
> + /* Data generation: skip if 0 */
> + dgen = *(uint64_t *)data;
> + if (dgen == 0)
> return 0;
>
> /* Retrieve the aggregation description. */
> @@ -499,6 +510,7 @@ dt_aggregate_snap_one(dtrace_hdl_t *dtp, int aggid, int cpu, const char *key,
> /* See if we already have an entry for this aggregation. */
> for (h = agh->dtah_hash[ndx]; h != NULL; h = h->dtahe_next) {
> dt_ident_t *aid;
> + uint64_t hgen;
>
> /* Hash value needs to match. */
> if (h->dtahe_hval != hval)
> @@ -519,6 +531,16 @@ dt_aggregate_snap_one(dtrace_hdl_t *dtp, int aggid, int cpu, const char *key,
> goto hashnext;
> }
>
> + /*
> + * Skip if data gen is older than hash gen.
> + * Reset hash data if data gen is newer than hash gen.
> + */
> + hgen = *(int64_t *)agd->dtada_data;
> + if (dgen < hgen)
> + return 0;
> + if (dgen > hgen)
> + dt_aggregate_clear_one(agd, dtp);
I'm having trouble thinking about this logic. Let's say the current
generation > dgen > hgen. So, since dgen>hgen, we call
dt_aggregate_clear_one(), which is good. But then we add the data to
our hash entry??? Is that so? Wouldn't that be incorrect? Or, what am
I missing?
> +
> aid = dt_idhash_lookup(dtp->dt_aggs, agg->dtagd_name);
> assert(aid != NULL);
> dt_agg_one_agg(aid, &agg->dtagd_drecs[DT_AGGDATA_RECORD],
> @@ -642,7 +664,7 @@ dtrace_aggregate_snap(dtrace_hdl_t *dtp)
> int fd = dt_bpf_map_lookup_fd(dtp->dt_aggmap_fd, &cpu);
>
> if (fd < 0)
> - return -1;
> + return DTRACE_WORKSTATUS_ERROR;
>
> rval = dt_aggregate_snap_cpu(dtp, cpu, fd);
> close(fd);
> @@ -650,7 +672,7 @@ dtrace_aggregate_snap(dtrace_hdl_t *dtp)
> return rval;
> }
>
> - return 0;
> + return DTRACE_WORKSTATUS_OKAY;
> }
>
> static int
> @@ -1219,7 +1241,9 @@ dt_aggregate_walk_sorted(dtrace_hdl_t *dtp, dtrace_aggregate_f *func,
> dt_ahash_t *hash = &agp->dtat_hash;
> size_t i, nentries = 0;
>
> +#if 0
> dtrace_aggregate_snap(dtp);
> +#endif
Why not just delete?
>
> /*
> * Count how many aggregations have data in the buffers. If there are
> @@ -1353,6 +1377,10 @@ dtrace_aggregate_walk_joined(dtrace_hdl_t *dtp, dtrace_aggid_t *aggvars,
> int i, j;
> dtrace_optval_t sortpos = dtp->dt_options[DTRACEOPT_AGGSORTPOS];
>
> +#if 0
> + dtrace_aggregate_snap(dtp);
> +#endif
> +
Why add dead code?
> /*
> * If the sorting position is greater than the number of aggregation
> * variable IDs, we silently set it to 0.
> @@ -1423,11 +1451,6 @@ dtrace_aggregate_walk_joined(dtrace_hdl_t *dtp, dtrace_aggid_t *aggvars,
> map[aggvar] = i + 1;
> }
>
> - /*
> - * Retrieve the aggregation data.
> - */
> - dtrace_aggregate_snap(dtp);
> -
> /*
> * We need to take two passes over the data to size our allocation, so
> * we'll use the first pass to also fill in the zero-filled data to be
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 38c43df3..b72da90a 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -1350,14 +1350,7 @@ dt_cg_act_clear(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> dnerror(dnp, D_CLEAR_AGGBAD,
> "undefined aggregation: @%s\n", aid->di_name);
>
> - /*
> - * FIXME: Needs implementation
> - * TODO: Emit code to clear the given aggregation.
> - * DEPENDS ON: How aggregations are implemented using eBPF (hashmap?).
> - * AGGID = aid->di_id
> - */
> dt_cg_store_val(pcb, anp, DTRACEACT_LIBACT, NULL, DT_ACT_CLEAR);
> - dnerror(dnp, D_UNKNOWN, "clear() is not implemented (yet)\n");
> }
>
> /*
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index 34009e6b..e90edae9 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -1512,6 +1512,33 @@ dt_normalize(dtrace_hdl_t *dtp, caddr_t base, dtrace_recdesc_t *rec)
> return 0;
> }
>
> +static int
> +dt_clear(dtrace_hdl_t *dtp, caddr_t base, dtrace_recdesc_t *rec)
> +{
> + dtrace_aggid_t aid;
> + uint64_t gen;
> + caddr_t addr;
> +
> + /* We have just one record: the aggregation ID. */
> + addr = base + rec->dtrd_offset;
> +
> + if (rec->dtrd_size != sizeof(dtrace_aggid_t))
> + return dt_set_errno(dtp, EDT_BADNORMAL);
> +
> + aid = *((dtrace_aggid_t *)addr);
> +
> + if (dt_bpf_map_lookup(dtp->dt_genmap_fd, &aid, &gen) < 0)
> + return -1;
> + gen++;
> + if (dt_bpf_map_update(dtp->dt_genmap_fd, &aid, &gen) < 0)
> + return -1;
> +
> + /* Also clear our own copy of the data, in case it gets printed. */
> + dtrace_aggregate_walk(dtp, dt_aggregate_clear_one, dtp);
> +
> + return 0;
> +}
> +
> #ifdef FIXME
> static int
> dt_clear_agg(const dtrace_aggdata_t *aggdata, void *arg)
How about removing the dead dt_clear_agg()?
> @@ -2325,6 +2352,11 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
> return DTRACE_WORKSTATUS_ERROR;
>
> i++;
> + continue;
> + case DT_ACT_CLEAR:
> + if (dt_clear(dtp, data, rec) != 0)
> + return DTRACE_WORKSTATUS_ERROR;
> +
> continue;
> case DT_ACT_FTRUNCATE:
> if (fp == NULL)
> diff --git a/test/unittest/aggs/tst.clear.d b/test/unittest/aggs/tst.clear.d
> index 408dd373..f574b5d2 100644
> --- a/test/unittest/aggs/tst.clear.d
> +++ b/test/unittest/aggs/tst.clear.d
> @@ -4,7 +4,6 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 - requires clear() */
>
> /*
> * ASSERTION:
> diff --git a/test/unittest/aggs/tst.clearavg.d b/test/unittest/aggs/tst.clearavg.d
> index 58d54742..16d18f34 100644
> --- a/test/unittest/aggs/tst.clearavg.d
> +++ b/test/unittest/aggs/tst.clearavg.d
> @@ -4,7 +4,6 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 - requires clear() */
>
> /*
> * ASSERTION:
> diff --git a/test/unittest/aggs/tst.clearavg2.d b/test/unittest/aggs/tst.clearavg2.d
> index 3d918502..984ce82c 100644
> --- a/test/unittest/aggs/tst.clearavg2.d
> +++ b/test/unittest/aggs/tst.clearavg2.d
> @@ -4,7 +4,7 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 - requires clear() */
> +
> /*
> * ASSERTION:
> * Positive avg() test
> diff --git a/test/unittest/aggs/tst.cleardenormalize.d b/test/unittest/aggs/tst.cleardenormalize.d
> index 277b7c7f..191ecad7 100644
> --- a/test/unittest/aggs/tst.cleardenormalize.d
> +++ b/test/unittest/aggs/tst.cleardenormalize.d
> @@ -4,7 +4,6 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 - requires clear() */
>
> /*
> * ASSERTION:
> diff --git a/test/unittest/aggs/tst.clearlquantize.d b/test/unittest/aggs/tst.clearlquantize.d
> index dcd4fef7..7e47e3aa 100644
> --- a/test/unittest/aggs/tst.clearlquantize.d
> +++ b/test/unittest/aggs/tst.clearlquantize.d
> @@ -4,7 +4,6 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 - requires clear() */
>
> /*
> * ASSERTION:
> diff --git a/test/unittest/aggs/tst.clearnormalize.d b/test/unittest/aggs/tst.clearnormalize.d
> index 211371d1..34d225f0 100644
> --- a/test/unittest/aggs/tst.clearnormalize.d
> +++ b/test/unittest/aggs/tst.clearnormalize.d
> @@ -4,7 +4,6 @@
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 - requires clear() */
>
> /*
> * ASSERTION:
More information about the DTrace-devel
mailing list