[DTrace-devel] [PATCH v3 05/11] speculations: use a destructor rather than hand-freeing
Eugene Loh
eugene.loh at oracle.com
Mon Nov 1 22:18:35 UTC 2021
Thanks for the new version. I'll renew my (admittedly unconfident)
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
On 11/1/21 10:29 AM, 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 | 91 ++++++++++++++++++++++++++----------------
> libdtrace/dt_impl.h | 5 +--
> 2 files changed, 58 insertions(+), 38 deletions(-)
>
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index 877540a4fad0..54e30a38184b 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,
> @@ -1977,11 +1994,11 @@ dt_print_trace(dtrace_hdl_t *dtp, FILE *fp, dtrace_recdesc_t *rec,
> * the specs map, and that draining has not been set in it, and atomically
> * bumps the written value.
> *
> - * - 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
> - * dtp->dt_spec_bufs instead of being consumed.
> + * - Speculations drain from the perf ring buffer into possibly many
> + * dt_spec_buf_data instances, 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
> + * put into dtp->dt_spec_bufs instead of being consumed.
> *
> * - commit / discard set the specs map entry's drained value to 1, which
> * indicates that it is drainable by userspace and prevents further
> @@ -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,15 +2101,25 @@ dt_spec_buf_destroy(dtrace_hdl_t *dtp, dt_spec_buf_t *dtsb,
> }
>
> dtsb->dtsb_size = 0;
> +}
>
> - if (remove_specid) {
> - dt_ident_t *idp = dt_dlib_get_map(dtp, "specs");
> +/*
> + * Remove a speculation's buffers, the speculation itself, and the record of it
> + * in the BPF specs map (which frees the ID for reuse).
> + *
> + * 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.
> + */
> +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 (idp)
> - dt_bpf_map_delete(idp->di_id, &dtsb->dtsb_id);
> - dt_list_delete(&dtp->dt_spec_bufs, dtsb);
> - dt_free(dtp, dtsb);
> - }
> + dt_spec_buf_data_destroy(dtp, dtsb);
> +
> + if (idp)
> + dt_bpf_map_delete(idp->di_id, &dtsb->dtsb_id);
> + dt_free(dtp, dtsb);
> }
>
> /*
> @@ -2210,7 +2237,7 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
> return DTRACE_WORKSTATUS_OKAY;
>
> tmpl.dtsb_id = specid;
> - dtsb = dt_htab_lookup(dtp->dt_specs_byid, &tmpl);
> + dtsb = dt_htab_lookup(dtp->dt_spec_bufs, &tmpl);
>
> /*
> * Discard when over the specsize.
> @@ -2348,7 +2375,7 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
> assert(specid == 0);
>
> tmpl.dtsb_id = *(uint32_t *)recdata;
> - dtsb = dt_htab_lookup(dtp->dt_specs_byid, &tmpl);
> + dtsb = dt_htab_lookup(dtp->dt_spec_bufs, &tmpl);
>
> /*
> * Speculation exists and is not already being drained.
> @@ -2481,7 +2508,7 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
> dtsd != NULL; dtsd = next) {
>
> dtsb = dtsd->dtsd_dtsb;
> - dtsd->dtsd_read += dt_list_length(&dtsd->dtsd_dtsb->dtsb_dsbd_list);
> + dtsd->dtsd_read += dt_list_length(&dtsb->dtsb_dsbd_list);
>
> if (dtsb->dtsb_committing) {
> if ((ret = dt_commit_one_spec(dtp, fp, dtsb,
> @@ -2489,19 +2516,18 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
> flow, quiet, peekflags,
> last, arg)) !=
> DTRACE_WORKSTATUS_OKAY) {
> - dt_spec_buf_destroy(dtp, dtsb, 0);
> + dt_spec_buf_data_destroy(dtp, dtsb);
> return ret;
> }
> }
>
> next = dt_list_next(dtsd);
>
> - if (dtsd->dtsd_read < dtsd->dtsd_dtsb->dtsb_spec.written)
> - dt_spec_buf_destroy(dtp, dtsb, 0);
> + if (dtsd->dtsd_read < dtsb->dtsb_spec.written)
> + dt_spec_buf_data_destroy(dtp, dtsb);
> else {
> - dt_spec_buf_destroy(dtp, dtsb, 1);
> - dt_htab_delete(dtp->dt_specs_byid, dtsd->dtsd_dtsb);
> dt_list_delete(&dtp->dt_spec_bufs_draining, dtsd);
> + dt_htab_delete(dtp->dt_spec_bufs, dtsb);
> dt_free(dtp, dtsd);
> }
> }
> @@ -2909,13 +2935,12 @@ dt_consume_proc_exits(dtrace_hdl_t *dtp)
> pthread_mutex_unlock(&dph->dph_lock);
> }
>
> -
> int
> dt_consume_init(dtrace_hdl_t *dtp)
> {
> - dtp->dt_specs_byid = dt_htab_create(dtp, &dt_spec_buf_htab_ops);
> + dtp->dt_spec_bufs = dt_htab_create(dtp, &dt_spec_buf_htab_ops);
>
> - if (!dtp->dt_specs_byid)
> + if (!dtp->dt_spec_bufs)
> return dt_set_errno(dtp, EDT_NOMEM);
> return 0;
> }
> @@ -2923,7 +2948,6 @@ dt_consume_init(dtrace_hdl_t *dtp)
> void
> dt_consume_fini(dtrace_hdl_t *dtp)
> {
> - dt_spec_buf_t *dtsb;
> dtsb_draining_t *dtsd;
>
> while ((dtsd = dt_list_next(&dtp->dt_spec_bufs_draining)) != NULL) {
> @@ -2931,10 +2955,7 @@ dt_consume_fini(dtrace_hdl_t *dtp)
> dt_free(dtp, dtsd);
> }
>
> - while ((dtsb = dt_list_next(&dtp->dt_spec_bufs)) != NULL)
> - dt_spec_buf_destroy(dtp, dtsb, 1);
> -
> - dt_htab_destroy(dtp, dtp->dt_specs_byid);
> + dt_htab_destroy(dtp, dtp->dt_spec_bufs);
> }
>
> dtrace_workstatus_t
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 50a1260c18ca..839660d99c17 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -248,7 +248,7 @@ typedef struct dt_spec_buf_data {
> } dt_spec_buf_data_t;
>
> typedef struct dt_spec_buf {
> - dt_list_t dtsb_list; /* list of dt_spec_buf */
> + dtrace_hdl_t *dtsb_dtp; /* backpointer to the dtrace instance */
> int32_t dtsb_id; /* speculation ID */
> size_t dtsb_size; /* size of all buffers in this spec */
> int dtsb_committing; /* when draining, nonzero if commit */
> @@ -403,9 +403,8 @@ struct dtrace_hdl {
> hrtime_t dt_laststatus; /* last status */
> hrtime_t dt_lastswitch; /* last switch of buffer data */
> hrtime_t dt_lastagg; /* last snapshot of aggregation data */
> - dt_list_t dt_spec_bufs; /* List of spec bufs */
> dt_list_t dt_spec_bufs_draining; /* List of spec bufs being drained */
> - dt_htab_t *dt_specs_byid;/* spec ID -> list of dt_spec_bufs_head_t */
> + dt_htab_t *dt_spec_bufs;/* spec ID -> list of dt_spec_bufs_head_t */
> char *dt_sprintf_buf; /* buffer for dtrace_sprintf() */
> int dt_sprintf_buflen; /* length of dtrace_sprintf() buffer */
> pthread_mutex_t dt_sprintf_lock; /* lock for dtrace_sprintf() buffer */
More information about the DTrace-devel
mailing list