[DTrace-devel] [PATCH v3 05/11] speculations: use a destructor rather than hand-freeing
Nick Alcock
nick.alcock at oracle.com
Mon Nov 1 14:29:41 UTC 2021
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 */
--
2.33.1.257.g9e0974a4e8
More information about the DTrace-devel
mailing list