[DTrace-devel] [PATCH] Clean up agg identifier storage data handling
Eugene Loh
eugene.loh at oracle.com
Mon Dec 7 18:32:22 PST 2020
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
That said, *ideally* this patch would include the business of:
*) inlining dt_idhash_nextoff() into dt_ident_set_storage()
*) replacing the remaining uses of dt_idhash_nextoff() with
dt_idhash_datasize()
I can see, however, that these changes have been made part of the next
patch. So, well, so much for "ideal." We can settle for "good enough."
I'll try to remember when I get to that next patch, but I believe it
overlooks a mention of "idhash_nextoff". That is, try "git grep
idhash_nextoff". (Note the missing "dt_", which may be why it was
overlooked.) With the other nextoff work in that patch, this remaining
case also needs to be renamed.
On 12/07/2020 06:42 AM, Kris Van Hees wrote:
> The storage layout for aggregations is based on storing offset and
> size data in the identifier. This will now be done using the new
> dt_ident_set_storage() function. The dt_ident_t structure gets two
> new fields:
> - di_size: storage size of the identifier (only used for aggregations)
> - di_hash: idhash that the identifier belongs to
>
> This patch also introduces a DT_CG_AGG_SET_STORAGE() macro to handle
> the code for only setting the storage data the first time we encounter
> an aggregation.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
> libdtrace/dt_cg.c | 38 ++++++++++++++++++--------------------
> libdtrace/dt_ident.c | 18 ++++++++++++++----
> libdtrace/dt_ident.h | 3 +++
> 3 files changed, 35 insertions(+), 24 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index c056d4b5..463ba63d 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -2940,11 +2940,18 @@ if ((idp = dnp->dn_ident)->di_kind != DT_IDENT_FUNC)
> }
>
> /*
> + * Macro to set the storage data (offset and size) for the aggregation
> + * identifier (if not set yet).
> + *
> * We consume twice the required data size because of the odd/even data pair
> * mechanism to provide lockless, write-wait-free operation.
> */
> -#define DT_CG_AGG_OFFSET(pcb, sz) \
> - dt_idhash_nextoff((pcb)->pcb_hdl->dt_aggs, sizeof(uint64_t), 2 * (sz))
> +#define DT_CG_AGG_SET_STORAGE(aid, sz) \
> + do { \
> + if ((aid)->di_offset == -1) \
> + dt_ident_set_storage((aid), sizeof(uint64_t), \
> + 2 * (sz)); \
> + } while (0)
>
> /*
> * Prepare the aggregation buffer for updating for a specific aggregation, and
> @@ -3079,8 +3086,7 @@ dt_cg_agg_avg(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> {
> int sz = 2 * sizeof(uint64_t);
>
> - if (aid->di_offset == -1)
> - aid->di_offset = DT_CG_AGG_OFFSET(pcb, sz);
> + DT_CG_AGG_SET_STORAGE(aid, sz);
> }
>
> static void
> @@ -3106,8 +3112,7 @@ dt_cg_agg_count(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> {
> int sz = 1 * sizeof(uint64_t);
>
> - if (aid->di_offset == -1)
> - aid->di_offset = DT_CG_AGG_OFFSET(pcb, sz);
> + DT_CG_AGG_SET_STORAGE(aid, sz);
>
> TRACE_REGSET(" AggCnt: Begin");
>
> @@ -3284,8 +3289,7 @@ dt_cg_agg_llquantize(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> sz = (hmag - lmag + 1) * (steps - steps / factor) * 2 + 4;
> sz *= sizeof(uint64_t);
>
> - if (aid->di_offset == -1)
> - aid->di_offset = DT_CG_AGG_OFFSET(pcb, sz);
> + DT_CG_AGG_SET_STORAGE(aid, sz);
> }
>
> static void
> @@ -3491,8 +3495,7 @@ dt_cg_agg_lquantize(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> sz = nlevels + 2;
> sz *= sizeof(uint64_t);
>
> - if (aid->di_offset == -1)
> - aid->di_offset = DT_CG_AGG_OFFSET(pcb, sz);
> + DT_CG_AGG_SET_STORAGE(aid, sz);
>
> TRACE_REGSET(" AggLq : Begin");
>
> @@ -3524,8 +3527,7 @@ dt_cg_agg_max(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> {
> int sz = 1 * sizeof(uint64_t);
>
> - if (aid->di_offset == -1)
> - aid->di_offset = DT_CG_AGG_OFFSET(pcb, sz);
> + DT_CG_AGG_SET_STORAGE(aid, sz);
> }
>
> static void
> @@ -3534,8 +3536,7 @@ dt_cg_agg_min(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> {
> int sz = 1 * sizeof(uint64_t);
>
> - if (aid->di_offset == -1)
> - aid->di_offset = DT_CG_AGG_OFFSET(pcb, sz);
> + DT_CG_AGG_SET_STORAGE(aid, sz);
> }
>
> static void
> @@ -3547,8 +3548,7 @@ dt_cg_agg_quantize(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
>
> incr = dt_cg_agg_opt_incr(dnp, dnp->dn_aggfun->dn_args, "quantize", 2);
>
> - if (aid->di_offset == -1)
> - aid->di_offset = DT_CG_AGG_OFFSET(pcb, sz);
> + DT_CG_AGG_SET_STORAGE(aid, sz);
> }
>
> static void
> @@ -3557,8 +3557,7 @@ dt_cg_agg_stddev(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> {
> int sz = 4 * sizeof(uint64_t);
>
> - if (aid->di_offset == -1)
> - aid->di_offset = DT_CG_AGG_OFFSET(pcb, sz);
> + DT_CG_AGG_SET_STORAGE(aid, sz);
> }
>
> static void
> @@ -3567,8 +3566,7 @@ dt_cg_agg_sum(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> {
> int sz = 1 * sizeof(uint64_t);
>
> - if (aid->di_offset == -1)
> - aid->di_offset = DT_CG_AGG_OFFSET(pcb, sz);
> + DT_CG_AGG_SET_STORAGE(aid, sz);
> }
>
> typedef void dt_cg_aggfunc_f(dt_pcb_t *, dt_ident_t *, dt_node_t *,
> diff --git a/libdtrace/dt_ident.c b/libdtrace/dt_ident.c
> index 34b49728..aa436057 100644
> --- a/libdtrace/dt_ident.c
> +++ b/libdtrace/dt_ident.c
> @@ -802,14 +802,14 @@ dt_idhash_insert(dt_idhash_t *dhp, const char *name, ushort_t kind,
> if (dhp->dh_tmpl != NULL)
> dt_idhash_populate(dhp); /* fill hash w/ initial population */
>
> - idp = dt_ident_create(name, kind, flags, id,
> - attr, vers, ops, iarg, gen);
> -
> + idp = dt_ident_create(name, kind, flags, id, attr, vers, ops, iarg,
> + gen);
> if (idp == NULL)
> return (NULL);
>
> h = dt_strtab_hash(name, NULL) % dhp->dh_hashsz;
> idp->di_next = dhp->dh_hash[h];
> + idp->di_hash = dhp;
>
> dhp->dh_hash[h] = idp;
> dhp->dh_nelems++;
> @@ -830,6 +830,7 @@ dt_idhash_xinsert(dt_idhash_t *dhp, dt_ident_t *idp)
>
> h = dt_strtab_hash(idp->di_name, NULL) % dhp->dh_hashsz;
> idp->di_next = dhp->dh_hash[h];
> + idp->di_hash = dhp;
> idp->di_flags &= ~DT_IDFLG_ORPHAN;
>
> dhp->dh_hash[h] = idp;
> @@ -959,11 +960,13 @@ dt_ident_create(const char *name, ushort_t kind, ushort_t flags, uint_t id,
> idp->di_ctfp = NULL;
> idp->di_type = CTF_ERR;
> idp->di_offset = -1;
> + idp->di_size = 0;
> idp->di_next = NULL;
> idp->di_gen = gen;
> idp->di_lineno = yylineno;
> + idp->di_hash = NULL;
>
> - return (idp);
> + return idp;
> }
>
> /*
> @@ -1024,6 +1027,13 @@ dt_ident_set_data(dt_ident_t *idp, void *data)
> idp->di_data = data;
> }
>
> +void
> +dt_ident_set_storage(dt_ident_t *idp, uint_t alignment, uint_t size)
> +{
> + idp->di_offset = dt_idhash_nextoff(idp->di_hash, alignment, size);
> + idp->di_size = size;
> +}
> +
> void
> dt_ident_type_assign(dt_ident_t *idp, ctf_file_t *fp, ctf_id_t type)
> {
> diff --git a/libdtrace/dt_ident.h b/libdtrace/dt_ident.h
> index 99f16ad5..770d99e2 100644
> --- a/libdtrace/dt_ident.h
> +++ b/libdtrace/dt_ident.h
> @@ -59,9 +59,11 @@ typedef struct dt_ident {
> ctf_file_t *di_ctfp; /* CTF container for the variable data type */
> ctf_id_t di_type; /* CTF identifier for the variable data type */
> int di_offset; /* storage offset */
> + int di_size; /* storage size */
> struct dt_ident *di_next; /* pointer to next ident in hash chain */
> ulong_t di_gen; /* generation number (pass that created me) */
> int di_lineno; /* line number that defined this identifier */
> + struct dt_idhash *di_hash; /* idhash this identifier belongs to */
> } dt_ident_t;
>
> #define DT_IDENT_ARRAY 0 /* identifier is an array variable */
> @@ -157,6 +159,7 @@ extern dtrace_attribute_t dt_ident_cook(struct dt_node *,
>
> extern void dt_ident_set_id(dt_ident_t *, uint_t);
> extern void dt_ident_set_data(dt_ident_t *, void *);
> +extern void dt_ident_set_storage(dt_ident_t *, uint_t, uint_t);
> extern void dt_ident_type_assign(dt_ident_t *, ctf_file_t *, ctf_id_t);
> extern dt_ident_t *dt_ident_resolve(dt_ident_t *);
> extern size_t dt_ident_size(dt_ident_t *);
More information about the DTrace-devel
mailing list