[DTrace-devel] [PATCH 01/19] Change probes from having lists of clauses to lists of stmts
Kris Van Hees
kris.van.hees at oracle.com
Wed Sep 18 14:18:27 UTC 2024
On Thu, Aug 29, 2024 at 01:25:40AM -0400, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
> Each stmt has a clause and a probe description. Traditionally,
> we have added clauses to probes. Further, we have generated an
> enabled probe ID for each probe/clause combination, and the
> consumer has use the EPID to determine the PRID as well as the
> data descriptor for the clause.
>
> In an up-coming patch, we will move to a new scheme, in which a
> widened EPID will have the PRID in its upper 32 bits and a stmt
> ID in the lower 32 bits. The stmt ID will be used to identify
> clauses to call as well as the data descriptor for the clause.
No widening of EPID is happening anymore so the above paragraph needs
to be amended. Or can be omitted actually. I don't think it needed.
> In this patch, change from probes having clauses associated
> with them to having stmts.
>
> We move the definition of dt_probe_clause_t in dt_probe.c to
> be a definition of dt_probe_stmt_t in dt_impl.h so that the
> type will become available for providers that will need it.
No further patch seems to be using dt_probe_stmt_t *and* even if they
need to, they can just include probe.h, so I would still leave the
type in dt_probe.c.
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> libdtrace/dt_cg.c | 17 +++++----
> libdtrace/dt_impl.h | 5 +++
> libdtrace/dt_probe.c | 79 +++++++++++++++++++++++-------------------
> libdtrace/dt_probe.h | 12 +++----
> libdtrace/dt_program.c | 8 ++---
> 5 files changed, 66 insertions(+), 55 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 4fab8eed..a7861829 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -844,9 +844,10 @@ typedef struct {
> } dt_clause_arg_t;
>
> static int
> -dt_cg_call_clause(dtrace_hdl_t *dtp, dt_ident_t *idp, dt_clause_arg_t *arg)
> +dt_cg_call_clause(dtrace_hdl_t *dtp, dtrace_stmtdesc_t *sdp, dt_clause_arg_t *arg)
> {
> dt_irlist_t *dlp = arg->dlp;
> + dt_ident_t *idp = sdp->dtsd_clause;
>
> /*
> * if (*dctx.act != act) // ldw %r0, [%r9 + DCTX_ACT]
> @@ -874,14 +875,12 @@ dt_cg_call_clause(dtrace_hdl_t *dtp, dt_ident_t *idp, dt_clause_arg_t *arg)
> }
>
> void
> -dt_cg_tramp_call_clauses(dt_pcb_t *pcb, const dt_probe_t *prp,
> - dt_activity_t act)
> +dt_cg_tramp_call_clauses(dt_pcb_t *pcb, const dt_probe_t *prp, dt_activity_t act)
> {
> dt_irlist_t *dlp = &pcb->pcb_ir;
> dt_clause_arg_t arg = { dlp, act, pcb->pcb_exitlbl };
>
> - dt_probe_clause_iter(pcb->pcb_hdl, prp,
> - (dt_clause_f *)dt_cg_call_clause, &arg);
> + dt_probe_stmt_iter(pcb->pcb_hdl, prp, (dt_stmt_f *)dt_cg_call_clause, &arg);
> }
>
> static int
> @@ -990,9 +989,10 @@ dt_cg_tramp_epilogue_advance(dt_pcb_t *pcb, dt_activity_t act)
> }
>
> static int
> -dt_cg_tramp_error_call_clause(dtrace_hdl_t *dtp, dt_ident_t *idp,
> - dt_irlist_t *dlp)
> +dt_cg_tramp_error_call_clause(dtrace_hdl_t *dtp, dtrace_stmtdesc_t *sdp, dt_irlist_t *dlp)
> {
> + dt_ident_t *idp = sdp->dtsd_clause;
> +
> /*
> * dt_error_#(dctx); // mov %r1, %r9
> * // call dt_error_#
> @@ -1029,8 +1029,7 @@ dt_cg_tramp_error(dt_pcb_t *pcb)
> TRACE_REGSET("Trampoline: Begin");
> emit(dlp, BPF_MOV_REG(BPF_REG_9, BPF_REG_1));
>
> - dt_probe_clause_iter(dtp, dtp->dt_error,
> - (dt_clause_f *)dt_cg_tramp_error_call_clause, dlp);
> + dt_probe_stmt_iter(dtp, dtp->dt_error, (dt_stmt_f *)dt_cg_tramp_error_call_clause, dlp);
>
> emit(dlp, BPF_MOV_IMM(BPF_REG_0, 0));
> emit(dlp, BPF_RETURN());
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 44dd1415..98fddc23 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -258,6 +258,11 @@ typedef struct dt_percpu_drops {
>
> typedef uint32_t dt_version_t; /* encoded version (see below) */
>
> +typedef struct dt_probe_stmt {
> + dt_list_t list;
> + dtrace_stmtdesc_t *stmt;
> +} dt_probe_stmt_t;
> +
> struct dtrace_hdl {
> const dtrace_vector_t *dt_vector; /* library vector, if vectored open */
> void *dt_varg; /* vector argument, if vectored open */
> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> index ab90d2ed..bb28bbed 100644
> --- a/libdtrace/dt_probe.c
> +++ b/libdtrace/dt_probe.c
> @@ -24,11 +24,6 @@
> #include <dt_list.h>
> #include <dt_bpf.h>
>
> -typedef struct dt_probe_clause {
> - dt_list_t list;
> - dt_ident_t *clause;
> -} dt_probe_clause_t;
> -
> typedef struct dt_probe_dependent {
> dt_list_t list;
> dt_probe_t *probe;
> @@ -467,7 +462,7 @@ dt_probe_enable(dtrace_hdl_t *dtp, dt_probe_t *prp)
> void
> dt_probe_destroy(dt_probe_t *prp)
> {
> - dt_probe_clause_t *pcp, *pcp_next;
> + dt_probe_stmt_t *psp, *psp_next;
> dt_probe_instance_t *pip, *pip_next;
> dt_probe_dependent_t *dep, *dep_next;
> dtrace_hdl_t *dtp;
> @@ -499,9 +494,18 @@ dt_probe_destroy(dt_probe_t *prp)
> dt_free(dtp, prp->nargv);
> dt_free(dtp, prp->xargv);
>
> - for (pcp = dt_list_next(&prp->clauses); pcp != NULL; pcp = pcp_next) {
> - pcp_next = dt_list_next(pcp);
> - dt_free(dtp, pcp);
> + for (psp = dt_list_next(&prp->stmts); psp != NULL; psp = psp_next) {
> + psp_next = dt_list_next(psp);
> +
> + /*
> + * FIXME? Is there nothing inside we also need to free?
> + *
> + * If psp->stmt was created with dt_probe_error_stmt(), maybe:
> + * dt_ident_t *idp = psp->stmt->dtsd_clause;
> + * dt_iddtor_difo(idp);
> + * dt_ident_destroy(idp);
> + */
Since the identifier for the clause is added to the identifier hash, it will
be cleaned up when that hash is deleted, and the idops associated with the
clause identifier ensures that the identifier cleanup also gets rid of the
DIFO.
So the FIXME can be dropped.
> + dt_free(dtp, psp);
> }
>
> for (dep = dt_list_next(&prp->dependents); dep != NULL; dep = dep_next) {
> @@ -1283,24 +1287,24 @@ dtrace_probe_iter(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp,
> }
>
> /*
> - * Create an ERROR-probe specific copy of a given clause.
> + * Create an ERROR-probe specific copy of a given stmt.
> *
> - * A modified copy of the clause is necessary because the ERROR probe may share
> - * some clauses with other probes, and yet it needs to be handled differently.
> + * A modified copy of the stmt is necessary because the ERROR probe may share
> + * some stmts with other probes, and yet it needs to be handled differently.
> *
> - * The following modifications are made in the copy of the clause:
> - *
> - * - it is named dt_error_N where N is taken from the original clause
> - * dt_clause_N (which also guarantees uniqueness)
> + * In the copy of the stmt, the clause is named dt_error_N, where N is taken
> + * from the original stmt's dt_clause_N (which also guarantees uniqueness).
> */
> -dt_ident_t *
> -dt_probe_error_clause(dtrace_hdl_t *dtp, dt_ident_t *idp)
> +static dtrace_stmtdesc_t *
> +dt_probe_error_stmt(dtrace_hdl_t *dtp, dtrace_stmtdesc_t *sdp)
> {
> char *name;
> int len;
> + dt_ident_t *idp = sdp->dtsd_clause;
> dtrace_difo_t *dp = dt_dlib_get_func_difo(dtp, idp);
> dt_ident_t *nidp = NULL;
> dtrace_difo_t *ndp;
> + dtrace_stmtdesc_t *nsdp = NULL;
>
> /*
> * Copy the DIFO.
> @@ -1316,7 +1320,6 @@ dt_probe_error_clause(dtrace_hdl_t *dtp, dt_ident_t *idp)
> name = dt_alloc(dtp, len);
> if (name == NULL)
> goto no_mem;
> -
> snprintf(name, len, "dt_error_%s", idp->di_name + strlen("dt_clause_"));
>
> /*
> @@ -1330,7 +1333,12 @@ dt_probe_error_clause(dtrace_hdl_t *dtp, dt_ident_t *idp)
>
> dt_ident_set_data(nidp, ndp);
>
> - return nidp;
> + nsdp = dt_alloc(dtp, sizeof(dtrace_stmtdesc_t));
> + if (nsdp == NULL)
> + goto no_mem;
> + nsdp->dtsd_clause = nidp;
> +
> + return nsdp;
>
> no_mem:
> if (ndp != NULL)
> @@ -1343,40 +1351,39 @@ no_mem:
> }
>
> int
> -dt_probe_add_clause(dtrace_hdl_t *dtp, dt_probe_t *prp, dt_ident_t *idp)
> +dt_probe_add_stmt(dtrace_hdl_t *dtp, dt_probe_t *prp, dtrace_stmtdesc_t *sdp)
> {
> - dt_probe_clause_t *pcp;
> + dt_probe_stmt_t *psp;
>
> - pcp = dt_zalloc(dtp, sizeof(dt_probe_clause_t));
> - if (pcp == NULL)
> + psp = dt_zalloc(dtp, sizeof(dt_probe_stmt_t));
> + if (psp == NULL)
> return dt_set_errno(dtp, EDT_NOMEM);
>
> if (prp == dtp->dt_error) {
> - pcp->clause = dt_probe_error_clause(dtp, idp);
> - if (pcp->clause == NULL) {
> - dt_free(dtp, pcp);
> + psp->stmt = dt_probe_error_stmt(dtp, sdp);
> + if (psp->stmt == NULL) {
> + dt_free(dtp, psp);
> return 0;
> }
> } else
> - pcp->clause = idp;
> + psp->stmt = sdp;
>
> - dt_list_append(&prp->clauses, pcp);
> + dt_list_append(&prp->stmts, psp);
>
> return 0;
> }
>
> int
> -dt_probe_clause_iter(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> - dt_clause_f *func, void *arg)
> +dt_probe_stmt_iter(dtrace_hdl_t *dtp, const dt_probe_t *prp, dt_stmt_f *func, void *arg)
> {
> - dt_probe_clause_t *pcp;
> - int rc;
> + dt_probe_stmt_t *psp;
> + int rc;
>
> assert(func != NULL);
>
> - for (pcp = dt_list_next(&prp->clauses); pcp != NULL;
> - pcp = dt_list_next(pcp)) {
> - rc = func(dtp, pcp->clause, arg);
> + for (psp = dt_list_next(&prp->stmts); psp != NULL;
> + psp = dt_list_next(psp)) {
> + rc = func(dtp, psp->stmt, arg);
>
> if (rc != 0)
> return rc;
> diff --git a/libdtrace/dt_probe.h b/libdtrace/dt_probe.h
> index b4c1a3e4..2a78cb9c 100644
> --- a/libdtrace/dt_probe.h
> +++ b/libdtrace/dt_probe.h
> @@ -32,7 +32,7 @@ typedef struct dt_probe_instance {
>
> typedef struct dt_probe {
> dt_list_t list; /* prev/next in enablings chain */
> - dt_list_t clauses; /* clauses to attach */
> + dt_list_t stmts; /* stmts */
> dt_list_t dependents; /* dependenct probes to attach */
> const dtrace_probedesc_t *desc; /* probe description (id, name) */
> dt_provider_t *prov; /* pointer to containing provider */
> @@ -86,11 +86,11 @@ typedef int dt_probe_f(dtrace_hdl_t *dtp, dt_probe_t *prp, void *arg);
> extern int dt_probe_iter(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp,
> dt_probe_f *pfunc, dtrace_probe_f *dfunc, void *arg);
>
> -extern int dt_probe_add_clause(dtrace_hdl_t *dtp, dt_probe_t *prp,
> - dt_ident_t *idp);
> -typedef int dt_clause_f(dtrace_hdl_t *dtp, dt_ident_t *idp, void *arg);
> -extern int dt_probe_clause_iter(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> - dt_clause_f *func, void *arg);
> +extern int dt_probe_add_stmt(dtrace_hdl_t *dtp, dt_probe_t *prp,
> + dtrace_stmtdesc_t *sdp);
> +typedef int dt_stmt_f(dtrace_hdl_t *dtp, dtrace_stmtdesc_t *sdp, void *arg);
> +extern int dt_probe_stmt_iter(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> + dt_stmt_f *func, void *arg);
>
> extern int dt_probe_add_dependent(dtrace_hdl_t *dtp, dt_probe_t *prp,
> dt_probe_t *idprp);
> diff --git a/libdtrace/dt_program.c b/libdtrace/dt_program.c
> index bdb434e0..afbf7265 100644
> --- a/libdtrace/dt_program.c
> +++ b/libdtrace/dt_program.c
> @@ -139,8 +139,8 @@ dtrace_program_info(dtrace_hdl_t *dtp, dtrace_prog_t *pgp,
> }
>
> typedef struct pi_state {
> - int *cnt;
> - dt_ident_t *idp;
> + int *cnt;
> + dtrace_stmtdesc_t *sdp;
> } pi_state_t;
>
> static int
> @@ -151,7 +151,7 @@ dt_stmt_probe(dtrace_hdl_t *dtp, dt_probe_t *prp, pi_state_t *st)
> dt_probe_info(dtp, prp->desc, &p);
> dt_probe_enable(dtp, prp);
>
> - dt_probe_add_clause(dtp, prp, st->idp);
> + dt_probe_add_stmt(dtp, prp, st->sdp);
> (*st->cnt)++;
>
> return 0;
> @@ -166,7 +166,7 @@ dt_prog_stmt(dtrace_hdl_t *dtp, dtrace_prog_t *pgp, dtrace_stmtdesc_t *sdp,
> int rc;
>
> st.cnt = cnt;
> - st.idp = sdp->dtsd_clause;
> + st.sdp = sdp;
> rc = dt_probe_iter(dtp, pdp, (dt_probe_f *)dt_stmt_probe, NULL, &st);
>
> /*
> --
> 2.43.5
>
More information about the DTrace-devel
mailing list