[DTrace-devel] [PATCH v2 05/11] speculations: use a destructor rather than hand-freeing

[Eugene Loh]

Another unconfident:
    Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
And a few comments/questions...

On 10/20/21 2:53 PM, Nick Alcock wrote:
> Combined with the member-destroying ht_htab_destroy in the previous
> commit, this lets us do away with the dt_spec_bufs list and just have
> the dt_specs_byid hash own its own members.  (We rename that hash
> to dt_spec_bufs because that's just a better name.)
>
> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> ---
>   libdtrace/dt_consume.c | 84 ++++++++++++++++++++++++++----------------
>   libdtrace/dt_impl.h    |  5 +--
>   2 files changed, 54 insertions(+), 35 deletions(-)
>
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index 877540a4fad0..b80f071956b7 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -393,7 +393,24 @@ dt_spec_buf_cmp(const dt_spec_buf_t *p,
>   }
>   
>   DEFINE_HE_STD_LINK_FUNCS(dt_spec_buf, dt_spec_buf_t, dtsb_he);
> -DEFINE_HTAB_STD_OPS(dt_spec_buf);
> +
> +static void
> +dt_spec_buf_destroy(dtrace_hdl_t *dtp, dt_spec_buf_t *dtsb);
> +
> +static void *
> +dt_spec_buf_del_buf(dt_spec_buf_t *head, dt_spec_buf_t *ent)
> +{
> +	head = dt_spec_buf_del(head, ent);
> +	dt_spec_buf_destroy(ent->dtsb_dtp, ent);
> +	return head;
> +}
> +
> +static dt_htab_ops_t dt_spec_buf_htab_ops = {
> +	.hval = (htab_hval_fn)dt_spec_buf_hval,
> +	.cmp = (htab_cmp_fn)dt_spec_buf_cmp,
> +	.add = (htab_add_fn)dt_spec_buf_add,
> +	.del = (htab_del_fn)dt_spec_buf_del_buf
> +};
>   
>   static int
>   dt_flowindent(dtrace_hdl_t *dtp, dtrace_probedata_t *data, dtrace_epid_t last,
> @@ -1980,7 +1997,7 @@ dt_print_trace(dtrace_hdl_t *dtp, FILE *fp, dtrace_recdesc_t *rec,
>    *  - Speculations drain from the perf ring buffer into dt_spec_buf_datas, one
>    *    dt_spec_buf_data per ring-buffer of data fetched from the kernel with a
>    *    nonzero specid (one speculated clause); these are chained into
> - *    dt_spec_bufs, one per speculation ID, and these are chained into
> + *    dt_spec_bufs, one per speculation ID, and these are put into
>    *    dtp->dt_spec_bufs instead of being consumed.

I'm having trouble digesting this comment block.  There is no 
"dt_spec_buf_datas."  We do not chain into dt_spec_bufs.  Or by 
dt_spec_bufs do you mean some dtsb_dsbd_list?

>    *
>    *  - commit / discard set the specs map entry's drained value to 1, which
> @@ -2026,9 +2043,11 @@ dt_spec_buf_create(dtrace_hdl_t *dtp, int32_t spec)
>   		goto oom;
>   	dtsb->dtsb_id = spec;
>   
> -	if (dt_htab_insert(dtp->dt_specs_byid, dtsb) < 0)
> +	/* Needed for the htab destructor */
> +	dtsb->dtsb_dtp = dtp;
> +
> +	if (dt_htab_insert(dtp->dt_spec_bufs, dtsb) < 0)
>   		goto oom;
> -	dt_list_append(&dtp->dt_spec_bufs, dtsb);
>   	return dtsb;
>   oom:
>   	dt_free(dtp, dtsb);
> @@ -2067,12 +2086,10 @@ oom:
>   }
>   
>   /*
> - * Remove a speculation's data and possibly the speculation buffer itself and
> - * the record of it in the BPF specs map (which frees the ID for reuse).
> + * Remove a speculation buffer's data.  The buffer is left alone.
>    */
>   static void
> -dt_spec_buf_destroy(dtrace_hdl_t *dtp, dt_spec_buf_t *dtsb,
> -		    int remove_specid)
> +dt_spec_buf_data_destroy(dtrace_hdl_t *dtp, dt_spec_buf_t *dtsb)
>   {
>   	dt_spec_buf_data_t	*dsbd;
>   
> @@ -2084,17 +2101,26 @@ dt_spec_buf_destroy(dtrace_hdl_t *dtp, dt_spec_buf_t *dtsb,
>   	}
>   
>   	dtsb->dtsb_size = 0;
> +}
> +
> +/*
> + * Remove a speculation's buffers, and the speculation itself, and the record of
> + * it in the BPF specs map (which frees the ID for reuse).

Remove first "and"?

> + *
> + * Note: if the speculation is being committed, it will also be interned on the
> + * dtp->dt_spec_bufs_draining list.  Such entries must be removed first.
> + */


Does discussion of dt_spec_bufs_draining really belong here?

> +static void
> +dt_spec_buf_destroy(dtrace_hdl_t *dtp, dt_spec_buf_t *dtsb)
> +{
> +	dt_ident_t *idp = dt_dlib_get_map(dtp, "specs");
>   
> -	if (remove_specid) {
> -		dt_ident_t *idp = dt_dlib_get_map(dtp, "specs");
> +	dt_spec_buf_data_destroy(dtp, dtsb);
>   
> -		if (idp)
> -			dt_bpf_map_delete(idp->di_id, &dtsb->dtsb_id);
> -		dt_list_delete(&dtp->dt_spec_bufs, dtsb);
> -		dt_free(dtp, dtsb);
> -	}
> +	if (idp)
> +		dt_bpf_map_delete(idp->di_id, &dtsb->dtsb_id);
> +	dt_free(dtp, dtsb);
>   }
> -
>   /*
>    * Peeking flags (values for the peekflag parameter for functions that have
>    * one).

Restore that blank line after dt_spec_buf_destroy()'s closing brace.