[DTrace-devel] [PATCH 06/13] Add provider-dependent struct in dt_probe_t
Kris Van Hees
kris.van.hees at oracle.com
Tue Jul 7 23:02:33 PDT 2020
On Tue, Jul 07, 2020 at 10:11:44PM -0700, Eugene Loh wrote:
> I'm trying to incorporate all your suggestions, but have a few questions
> here:
>
>
> On 07/06/2020 02:50 PM, Kris Van Hees wrote:
> > On Wed, Jul 01, 2020 at 10:41:11PM -0400, eugene.loh at oracle.com wrote:
> >> From: Eugene Loh <eugene.loh at oracle.com>
> >>
> >> The dt_probe_t struct had members like event_id and event_fd.
> >> This made sense for the providers we were supporting, since they
> >> were all tracepoint-like. For future providers, however, those
> >> members might not make sense and others will be needed. Therefore,
> >> replace event_id and event_fd with an opaque pointer to a
> >> provider-dependent struct.
> >>
> >> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> >> ---
> >> libdtrace/dt_probe.c | 19 +++-------
> >> libdtrace/dt_probe.h | 4 +--
> >> libdtrace/dt_prov_dtrace.c | 42 +++++++++++++++-------
> >> libdtrace/dt_prov_fbt.c | 41 +++++++++++++++------
> >> libdtrace/dt_prov_sdt.c | 72 +++++++++++++++++++++++--------------
> >> libdtrace/dt_prov_syscall.c | 34 +++++++++++++-----
> >> libdtrace/dt_provider.h | 6 ++--
> >> 7 files changed, 143 insertions(+), 75 deletions(-)
> >>
> >> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> >> index be407dd2..496ba11e 100644
> >> --- a/libdtrace/dt_probe.c
> >> +++ b/libdtrace/dt_probe.c
> >> @@ -505,6 +505,7 @@ dt_probe_destroy(dt_probe_t *prp)
> >>
> >> if (prp->prov && prp->prov->impl && prp->prov->impl->probe_fini)
> >> prp->prov->impl->probe_fini(dtp, prp);
> >> + dt_free(dtp, prp->prv_data);
> > OK
> >
> >>
> >> dt_node_list_free(&prp->nargs);
> >> dt_node_list_free(&prp->xargs);
> >> @@ -665,7 +666,8 @@ dt_probe_tag(dt_probe_t *prp, uint_t argn, dt_node_t *dnp)
> >> }
> >>
> >> dt_probe_t *
> >> -dt_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const char *prv,
> >> +dt_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const void *prv_data,
> >> + const char *prv,
> >> const char *mod, const char *fun, const char *prb)
> > Please move the prv_data to the end of the argument list, which is more
> > consistent with other code that supports opaque pointers. It also shows its
> > status as auxilliary information rather than being mixed with the elements
> > that are part of the generic probe. And perhaps name it datap since that is
> > the name you used for the provider-specific data variable in other places.
> >
> > I would also modify dt_probe_insert() to free datap (if not NULL) if the probe
> > cannot be created. That avoids needing to have each caller check if the probe
> > got created, and if not, free datap.
>
> I don't think this really works. The datap points to provider-specific
> stuff. It might even point to memory that contains pointers to other
> things. So the provider needs to free datap.
>
> Another option is that the provider only allocates datap but doesn't set
> up the memory to which datap points, doing that only once the probe has
> been inserted. But then if there is a problem with initializing the
> memory to which datap points, would we want to delete the probe we just
> inserted? We do not yet have a good function for that (deleting one
> probe, vs cleaning them all up at the end... e.g., never do we decrement
> dt_probe_id).
>
> Anyhow, one can shoehorn the change you're talking about, but it strikes
> me as pretty ugly.
>
> > You already do a freeing of the datap in
> > dt_probr_destroy() so doing it in dt_probe_insert() as well (when needed) is
> > OK.
>
> Well, no. I mean, yes, there is a free(datap) in dt_probe_destroy(),
> but it comes right after calling the provider-specific fini() function.
> So, do we want dt_probe_insert(), called by a provider, to then call a
> provider-specific callback we need to define? My head is spinning just
> thinking of the back-and-forth.
If we anticipate that the provider-specific data will contains pointers to
other memory blocks that would need to be cleaned up along with it (I cannot
really think of a need for that in any provider we're planning to support but
it might turn out to be needed) then we will be needing a function to destroy
the provider-specific data anyhow. I think it is safe right now to just have
dt_probe_insert() do it (similar to dt_probe_destroy())
The fact that the one in dt_probe_destroy() comes after calling a probe_fini()
(if one exists) is not really a good argument. You wouldn't want the fini()
to do the cleanup of stuff referenced from datap and then have datap free'd
by dt_probe_destroy(). If you envision more complex datap variants where there
is additional allocated data, then we really need to have a function to handle
the freeing of datap and related structures. And that can then be called from
dt_probe_insert() if the probe creation fails.
I still think a plan free of datap in dt_probe_insert() suffices for now, and
we can always implement a function to do the freeing later if it turns out to
be necessary. Right now it seems to be a bit of over-engineering.
> > The caller can count on the fact that if the probe could not be created,
> > datap is no longer valid.
> >
> >> {
> >> dt_probe_t *prp;
> >> @@ -711,8 +713,7 @@ dt_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const char *prv,
> >>
> >> prp->desc = desc;
> >> prp->prov = prov;
> >> - prp->event_id = -1;
> >> - prp->event_fd = -1;
> >> + prp->prv_data = prv_data;
> >>
> >> dt_htab_insert(dtp->dt_byprv, prp);
> >> dt_htab_insert(dtp->dt_bymod, prp);
> >> @@ -872,26 +873,16 @@ dt_probe_delete(dtrace_hdl_t *dtp, dt_probe_t *prp)
> >> static void
> >> dt_probe_args_info(dtrace_hdl_t *dtp, dt_probe_t *prp)
> >> {
> >> - int id = DTRACE_IDNONE;
> >> int argc = 0;
> >> dt_argdesc_t *argv = NULL;
> >> int i, nc, xc;
> >> dtrace_typeinfo_t dtt;
> >>
> >> - /*
> >> - * If we already have an event ID information for this probe, there is
> >> - * no need to retrieve it again.
> >> - */
> >> - if (prp->event_id != -1)
> >> - return;
> >> -
> >> if (!prp->prov->impl->probe_info)
> >> return;
> >> - if (prp->prov->impl->probe_info(dtp, prp, &id, &argc, &argv) < 0)
> >> + if (prp->prov->impl->probe_info(dtp, prp, &argc, &argv) < 0)
> >> return;
> >>
> >> - if (id > 0)
> >> - prp->event_id = id;
> >> if (!argc || !argv)
> >> return;
> >>
> >> diff --git a/libdtrace/dt_probe.h b/libdtrace/dt_probe.h
> >> index 7feeeff2..92768f9b 100644
> >> --- a/libdtrace/dt_probe.h
> >> +++ b/libdtrace/dt_probe.h
> >> @@ -38,8 +38,7 @@ typedef struct dt_probe {
> >> struct dt_hentry he_fun; /* function name htab links */
> >> struct dt_hentry he_prb; /* probe name htab link */
> >> struct dt_hentry he_fqn; /* fully qualified name htab link */
> >> - int event_id; /* tracing event id */
> >> - int event_fd; /* perf event file descriptor */
> >> + void *prv_data; /* provider-specific data */
> > OK.
> >
> >> dt_ident_t *pr_ident; /* pointer to probe identifier */
> >> const char *pr_name; /* pointer to name component */
> >> dt_node_t *nargs; /* native argument list */
> >> @@ -70,6 +69,7 @@ extern int dt_probe_define(dt_provider_t *, dt_probe_t *,
> >> extern dt_node_t *dt_probe_tag(dt_probe_t *, uint_t, dt_node_t *);
> >>
> >> extern dt_probe_t *dt_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov,
> >> + const void *prv_data,
> >> const char *prv, const char *mod,
> >> const char *fun, const char *prb);
> > Move to end of parameter list.
> >
> >> extern dt_probe_t *dt_probe_lookup(dtrace_hdl_t *dtp,
> >> diff --git a/libdtrace/dt_prov_dtrace.c b/libdtrace/dt_prov_dtrace.c
> >> index 34c99388..c5c2a84d 100644
> >> --- a/libdtrace/dt_prov_dtrace.c
> >> +++ b/libdtrace/dt_prov_dtrace.c
> >> @@ -15,6 +15,11 @@
> >> #include "dt_provider.h"
> >> #include "dt_probe.h"
> >>
> >> +typedef struct prv_data {
> >> + int event_id;
> >> + int event_fd;
> >> +} prv_data_t;
> > Since multiple providers use the tracepoint-based mechanism, the provider
> > specific data is identical for these providers and can therefore be declared
> > in a more central location. I would put it in dt_provider.h (where the
> > prototypes for tp_event_info() and tp_attach() sre defined..
> >
> > I would not use the same name for the struct, arg name and member name.
> > Since this is common for all tracepoint-based providers, how about using
> > tp_probe (and tp_probe_t)? Other names can be profile_probe_t, and so on.
> > That is similar to the naming in DTrace v1 (at the kernel level) where e.g.
> > FBT specific information was in fbt_probe_t.
> >
> >> +
> >> static const char prvname[] = "dtrace";
> >> static const char modname[] = "";
> >> static const char funname[] = "";
> >> @@ -34,18 +39,26 @@ static const dtrace_pattr_t pattr = {
> >> static int populate(dtrace_hdl_t *dtp)
> >> {
> >> dt_provider_t *prv;
> >> - int n = 0;
> >> + int i, n = 0;
> >> + char *prbnames[] = { "BEGIN", "END", "ERROR" };
> > This (along with the code below) is a bit of an issue because the ERROR probe
> > is not the same as the BEGIN and END probes. BEGIN/END are implemented as
> > uprobes, whereas ERROR is not implemented as a probe that is attached to
> > anything at the kernel level. All three belong in the dtrace provider, but
> > the behaviour is definitely different.
> >
> >> prv = dt_provider_create(dtp, prvname, &dt_dtrace, &pattr);
> >> if (prv == NULL)
> >> return 0;
> >>
> >> - if (dt_probe_insert(dtp, prv, prvname, modname, funname, "BEGIN"))
> >> - n++;
> >> - if (dt_probe_insert(dtp, prv, prvname, modname, funname, "END"))
> >> - n++;
> >> - if (dt_probe_insert(dtp, prv, prvname, modname, funname, "ERROR"))
> >> - n++;
> >> + for (i = 0; i < sizeof (prbnames) / sizeof (*prbnames); i++) {
> >> + prv_data_t *datap;
> >> +
> >> + datap = dt_zalloc(dtp, sizeof (prv_data_t));
> >> + if (datap == NULL)
> >> + continue;
> >> + datap->event_id = -1;
> >> + datap->event_fd = -1;
> > I think that the above should be done in a tp_probe_alloc() function in the
> > SDT provider code (where the other generic tp_* functions are) since we should
> > use the same struct for all tracepoint-based providers anyway.
> >
> > So the next statement becomes:
> >
> > if (dt_probe_insert(dtp, prv, datap, prvname, modname, funname,
> > prbnames[i], tp_probe_event()))
> >
> > It is also possible to add a tp_probe_insert() function that wraps the call
> > to dt_probe_insert along with allocation of the tp_probe_t. Either solution
> > seems reasonable to me.
> >
> > If you prefer the tp_probe_insert() wrapper function, adjust your reading of
> > my further comments in that context (I am commenting as if there is a
> > tp_probe_alloc() function that will be used in dt_probe_insert() calls).
>
> If we want to share "tp" code, I think tp_probe_insert() is the way to
> go. It'll be in the revised patch set. No sense in having a common
> function that simply allocs when there is more common functionality that
> can be swept in.
OK
> >> + if (dt_probe_insert(dtp, prv, datap, prvname, modname, funname, prbnames[i]))
> > The datap argument would move to the end per my comments above.
> >
> >> + n++;
> >> + else
> >> + dt_free(dtp, datap);
> > Already done in dt_probe_insert() if the probe cannot be created (see above).
> >> + }
> >>
> >> return n;
> >> }
> >> @@ -230,7 +243,7 @@ out:
> >> }
> >>
> >> static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> >> - int *idp, int *argcp, dt_argdesc_t **argvp)
> >> + int *argcp, dt_argdesc_t **argvp)
> >> {
> >> char *spec;
> >> char *fn = NULL;
> >> @@ -238,8 +251,12 @@ static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> >> FILE *f;
> >> int rc = -ENOENT;
> >> size_t len;
> >> + int *idp = &((prv_data_t *)(prp->prv_data))->event_id;
> >> +
> >> + /* if we have an event ID, no need to retrieve it again */
> >> + if (*idp != -1)
> >> + return -1;
> > I think it would be more clear if you did this as:
> >
> > struct tp_probe_t *datap = prp->prv_data;
> >
> > if (datap->event_id != -1)
> > return -1;
> >
> >> - *idp = -1;
> >> *argcp = 0; /* no arguments */
> >> *argvp = NULL;
> > and then later on you can do:
> >
> > rc = tp_event_info(dtp, f, 0, datap, NULL, NULL);
> >
> > and tp_event_info() can take a tp_probe_t * argument and do with it whatever
> > it needs to (in this case, assign event_id).
> >
> >> @@ -295,10 +312,11 @@ out:
> >> static int probe_fini(dtrace_hdl_t *dtp, dt_probe_t *prp)
> >> {
> >> int fd;
> >> + prv_data_t *datap = prp->prv_data;
> >>
> >> - if (prp->event_fd != -1) {
> >> - close(prp->event_fd);
> >> - prp->event_fd = -1;
> >> + if (datap->event_fd != -1) {
> >> + close(datap->event_fd);
> >> + datap->event_fd = -1;
> >> }
> > Make this into a standard tp_probe_finit and use it in providers that are
> > tracepoint-based and need no further cleanup? Or you could introduce a new
> > provider function (detach) to handle this, and leave probe_fini in cases
> > where we need to perform further cleanup, Perhaps that is nice (even though
> > it means more functions) because it provides a balanced set of provider
> > operations attach vs detach, populate/provide vs probe_fini). What do you
> > think?
>
> Since you have question marks, I'm assuming it's okay not to do this.
>
> First, having separate detach and fini functions makes no sense to me.
> I understand the balance, but we intentionally separate populate/provide
> from attach since we want to list probes but not necessarily create more
> stuff than we have to. In contrast, I am unaware that we currently are
> interested in separating detach from fini.
>
> As far as a standard tp fini function goes, the code is only a few lines
> and is used only by dtrace and fbt, not by sdt or syscall. So the value
> proposition of a common function is different from the other tp stuff
> we're doing.
As I point out further down, you can have a standard tp_probe_fini() that
is called from the provider-specific probe_fini() functions. The standard
one handles the closing of the event_fd, and if providers need to do other
custom cleanup work they can do that before and/or after calling
tp_probe_fini(). It still makes sense to put the common code in its own
function so that we have a single place where this (tiny) bit of work is
done.
> >> fd = open(UPROBE_EVENTS, O_WRONLY | O_APPEND);
> >> diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> >> index cb80082e..a58584c0 100644
> >> --- a/libdtrace/dt_prov_fbt.c
> >> +++ b/libdtrace/dt_prov_fbt.c
> >> @@ -37,6 +37,11 @@
> >> #include "dt_probe.h"
> >> #include "dt_pt_regs.h"
> >>
> >> +typedef struct prv_data {
> >> + int event_id;
> >> + int event_fd;
> >> +} prv_data_t;
> > See above.
> >
> >> +
> >> static const char prvname[] = "fbt";
> >> static const char modname[] = "vmlinux";
> >>
> >> @@ -65,9 +70,10 @@ static int populate(dtrace_hdl_t *dtp)
> >> char buf[256];
> >> char *p;
> >> const char *mod = modname;
> >> - int n = 0;
> >> + int i, n = 0;
> >> dtrace_syminfo_t sip;
> >> dtrace_probedesc_t pd;
> >> + char *prbnames[] = { "entry", "return" };
> > Not necessary (see below).
> >
> >> prv = dt_provider_create(dtp, prvname, &dt_fbt, &pattr);
> >> if (prv == NULL)
> >> @@ -136,10 +142,20 @@ static int populate(dtrace_hdl_t *dtp)
> >> if (dt_probe_lookup(dtp, &pd) != NULL)
> >> continue;
> >>
> >> - if (dt_probe_insert(dtp, prv, prvname, mod, buf, "entry"))
> >> - n++;
> >> - if (dt_probe_insert(dtp, prv, prvname, mod, buf, "return"))
> >> - n++;
> >> + for (i = 0; i < sizeof (prbnames) / sizeof (*prbnames); i++) {
> >> + prv_data_t *datap;
> >> +
> >> + datap = dt_zalloc(dtp, sizeof (prv_data_t));
> >> + if (datap == NULL)
> >> + continue;
> >> + datap->event_id = -1;
> >> + datap->event_fd = -1;
> >> + if (dt_probe_insert(dtp, prv, datap, prvname, mod,
> >> + buf, prbnames[i]))
> >> + n++;
> >> + else
> >> + dt_free(dtp, datap);
> >> + }
> > This could become (based on earlier comments):
> >
> > if (dt_probe_insert(dtp, prv, prvname, mod, buf, "entry",
> > tp_probe_alloc()))
> > n++
> > if (dt_probe_insert(dtp, prv, prvname, mod, buf, "return",
> > tp_probe_alloc()))
> > n++
> >> }
> >>
> >> fclose(f);
> >> @@ -273,15 +289,19 @@ static void trampoline(dt_pcb_t *pcb)
> >> }
> >>
> >> static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> >> - int *idp, int *argcp, dt_argdesc_t **argvp)
> >> + int *argcp, dt_argdesc_t **argvp)
> >> {
> >> int fd;
> >> char *fn;
> >> size_t len;
> >> FILE *f;
> >> int rc = -1;
> >> + int *idp = &((prv_data_t *)(prp->prv_data))->event_id;
> >> +
> >> + /* if we have an event ID, no need to retrieve it again */
> >> + if (*idp != -1)
> >> + return -1;
> > See above (for dt_prov_dtrace).
> >
> >> - *idp = -1;
> >> *argcp = 0; /* no arguments by default */
> >> *argvp = NULL;
> >>
> >> @@ -335,10 +355,11 @@ out:
> >> static int probe_fini(dtrace_hdl_t *dtp, dt_probe_t *prp)
> >> {
> >> int fd;
> >> + prv_data_t *datap = prp->prv_data;
> >>
> >> - if (prp->event_fd != -1) {
> >> - close(prp->event_fd);
> >> - prp->event_fd = -1;
> >> + if (datap->event_fd != -1) {
> >> + close(datap->event_fd);
> >> + datap->event_fd = -1;
> >> }
> > See above (for dt_prov_dtrace). If you introduce a detach(dtp, prp) then you
> > can have a custom detahc here that calls tp_detach(dtp, prp) to do the actual
> > detach and then do the rest of the cleanup. Or if you prefer to stick with
> > probe_fini (no detach) then you can have this on call a tp_probe_fini(dtp, prp)
> > to handle the common detach-cleanup and then continue with the provider specific
> > cleanup.
> >
> >> fd = open(KPROBE_EVENTS, O_WRONLY | O_APPEND);
> >> diff --git a/libdtrace/dt_prov_sdt.c b/libdtrace/dt_prov_sdt.c
> >> index 1a445c65..9a76f029 100644
> >> --- a/libdtrace/dt_prov_sdt.c
> >> +++ b/libdtrace/dt_prov_sdt.c
> >> @@ -34,6 +34,11 @@
> >> #include "dt_probe.h"
> >> #include "dt_pt_regs.h"
> >>
> >> +typedef struct prv_data {
> >> + int event_id;
> >> + int event_fd;
> >> +} prv_data_t;
> > See above. Put this in dt_provider.h, with names tp_probe / tp_probe_t.
> >
> >> +
> >> static const char prvname[] = "sdt";
> >> static const char modname[] = "vmlinux";
> >>
> >> @@ -226,29 +231,31 @@ done:
> >> return 0;
> >> }
> >>
> >> -int tp_event_attach(dtrace_hdl_t *dtp, dt_probe_t *prp, int bpf_fd)
> >> +int tp_event_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> > tp_attach (per comments to an earlier patch)
>
> Sorry, I don't understand what this means. I guess I do not know which
> "comments to an earlier patch" you mean. Are you saying
> s/tp_event_attach/tp_attach/ ?
Yes
> >> {
> >> - if (prp->event_fd == -1) {
> >> - int fd;
> >> - struct perf_event_attr attr = { 0, };
> >> + prv_data_t *datap = prp->prv_data;
> > tp_probe_t *
> >
> > Add:
> > if (datap->event_id == -1)
> > return 0;
> >
> > because if there is no event id, there is nothing to attach to, and we are
> > done. But it might still be a valid attach request, so we return 0.
>
> I made the change, but I confess I do not understand what scenario
> you're talking about.
It's just a matter of guarding against trying to create a tracepoint for
event id -1, i.e. if we end up here ater we failed to get an event id (or if
we end up here for a probe that doesn't use a perf event).
> >> + if (datap->event_fd == -1) {
> >> + int fd;
> >> + struct perf_event_attr attr = { 0, };
> >>
> >> - attr.type = PERF_TYPE_TRACEPOINT;
> >> - attr.sample_type = PERF_SAMPLE_RAW;
> >> - attr.sample_period = 1;
> >> - attr.wakeup_events = 1;
> >> - attr.config = prp->event_id;
> >> + attr.type = PERF_TYPE_TRACEPOINT;
> >> + attr.sample_type = PERF_SAMPLE_RAW;
> >> + attr.sample_period = 1;
> >> + attr.wakeup_events = 1;
> >> + attr.config = datap->event_id;
> >>
> >> - fd = perf_event_open(&attr, -1, 0, -1, 0);
> >> - if (fd < 0)
> >> - return dt_set_errno(dtp, errno);
> >> + fd = perf_event_open(&attr, -1, 0, -1, 0);
> >> + if (fd < 0)
> >> + return dt_set_errno(dtp, errno);
> >>
> >> - prp->event_fd = fd;
> >> - }
> >> + datap->event_fd = fd;
> >> + }
> >>
> >> - if (ioctl(prp->event_fd, PERF_EVENT_IOC_SET_BPF, bpf_fd) < 0)
> >> - return dt_set_errno(dtp, errno);
> >> + if (ioctl(datap->event_fd, PERF_EVENT_IOC_SET_BPF, bpf_fd) < 0)
> >> + return dt_set_errno(dtp, errno);
> >>
> >> - return 0;
> >> + return 0;
> >> }
> >>
> >> /*
> >> @@ -275,8 +282,10 @@ static int populate(dtrace_hdl_t *dtp)
> >> return 0;
> >>
> >> while (fgets(buf, sizeof(buf), f)) {
> >> - int dummy;
> >> - char str[sizeof(buf)];
> >> + int dummy;
> >> + char str[sizeof(buf)];
> >> + const char *mod, *prb;
> >> + prv_data_t *datap;
> >>
> >> p = strchr(buf, '\n');
> >> if (p)
> >> @@ -301,13 +310,21 @@ static int populate(dtrace_hdl_t *dtp)
> >> strcmp(buf, UPROBES) == 0)
> >> continue;
> >>
> >> - if (dt_probe_insert(dtp, prv, prvname, buf, "", p))
> >> - n++;
> >> + mod = buf;
> >> + prb = p;
> >> } else {
> >> - if (dt_probe_insert(dtp, prv, prvname, modname, "",
> >> - buf))
> >> - n++;
> >> + mod = modname;
> >> + prb = buf;
> >> }
> >> + datap = dt_zalloc(dtp, sizeof (prv_data_t));
> >> + if (datap == NULL)
> >> + continue;
> >> + datap->event_id = -1;
> >> + datap->event_fd = -1;
> >> + if (dt_probe_insert(dtp, prv, datap, prvname, mod, "", prb))
> >> + n++;
> >> + else
> >> + dt_free(dtp, datap);
> > This can become:
> >
> > if (dt_probe_insert(dtp, prv, prvname, buf, "", p,
> > tp_probe_alloc()))
> > n++;
> > } else {
> > if (dt_probe_insert(dtp, prv, prvname, modname, "", buf,
> > tp_probe_alloc()))
> > n++;
> > }
> >
> >> }
> >>
> >> fclose(f);
> >> @@ -408,13 +425,16 @@ static void trampoline(dt_pcb_t *pcb)
> >> }
> >>
> >> static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> >> - int *idp, int *argcp, dt_argdesc_t **argvp)
> >> + int *argcp, dt_argdesc_t **argvp)
> >> {
> >> FILE *f;
> >> char fn[256];
> >> int rc;
> >> + int *idp = &((prv_data_t *)(prp->prv_data))->event_id;
> >>
> >> - *idp = -1;
> >> + /* if we have an event ID, no need to retrieve it again */
> >> + if (*idp != -1)
> >> + return -1;
> > See above.
> >
> >> strcpy(fn, EVENTSFS);
> >> strcat(fn, prp->desc->mod);
> >> diff --git a/libdtrace/dt_prov_syscall.c b/libdtrace/dt_prov_syscall.c
> >> index cb54ce5a..ecab4545 100644
> >> --- a/libdtrace/dt_prov_syscall.c
> >> +++ b/libdtrace/dt_prov_syscall.c
> >> @@ -35,6 +35,11 @@
> >> #include "dt_probe.h"
> >> #include "dt_pt_regs.h"
> >>
> >> +typedef struct prv_data {
> >> + int event_id;
> >> + int event_fd;
> >> +} prv_data_t;
> > See above.
> >
> >> static const char prvname[] = "syscall";
> >> static const char modname[] = "vmlinux";
> >>
> >> @@ -85,6 +90,7 @@ static int populate(dtrace_hdl_t *dtp)
> >>
> >> while (fgets(buf, sizeof(buf), f)) {
> >> char *p;
> >> + char *prb;
> >>
> >> /* Here buf is "group:event". */
> >> p = strchr(buf, '\n');
> >> @@ -107,22 +113,31 @@ static int populate(dtrace_hdl_t *dtp)
> >> p = buf;
> >> if (memcmp(p, PROV_PREFIX, sizeof(PROV_PREFIX) - 1) != 0)
> >> continue;
> >> -
> > I think the empty line here adds clarity, in the sense that the conditional
> > is important to highlight as a condition to not continue execution in this
> > function. The next statement is part of the actual work done in this function.
> >
> >> p += sizeof(PROV_PREFIX) - 1;
> >> +
> >> /*
> >> * Now p will be just "event", and we are only interested in
> >> * events that match "sys_enter_*" or "sys_exit_*".
> >> */
> >> + prb = NULL;
> >> if (!memcmp(p, ENTRY_PREFIX, sizeof(ENTRY_PREFIX) - 1)) {
> >> p += sizeof(ENTRY_PREFIX) - 1;
> >> - if (dt_probe_insert(dtp, prv, prvname, modname, p,
> >> - "entry"))
> >> - n++;
> >> + prb = "entry";
> >> } else if (!memcmp(p, EXIT_PREFIX, sizeof(EXIT_PREFIX) - 1)) {
> >> p += sizeof(EXIT_PREFIX) - 1;
> >> - if (dt_probe_insert(dtp, prv, prvname, modname, p,
> >> - "return"))
> >> + prb = "return";
> >> + }
> >> + if (prb != NULL) {
> >> + prv_data_t *datap = dt_zalloc(dtp, sizeof (prv_data_t));
> >> +
> >> + if (datap == NULL)
> >> + continue;
> >> + datap->event_id = -1;
> >> + datap->event_fd = -1;
> >> + if (dt_probe_insert(dtp, prv, datap, prvname, modname, p, prb))
> >> n++;
> >> + else
> >> + dt_free(dtp, datap);
> > Becomes:
> > if (!memcmp(p, ENTRY_PREFIX, sizeof(ENTRY_PREFIX) - 1)) {
> > p += sizeof(ENTRY_PREFIX) - 1;
> > if (dt_probe_insert(dtp, prv, prvname, modname, p,
> > "entry", tp_probe_alloc()))
> > n++;
> > } else if (!memcmp(p, EXIT_PREFIX, sizeof(EXIT_PREFIX) - 1)) {
> > p += sizeof(EXIT_PREFIX) - 1;
> > if (dt_probe_insert(dtp, prv, prvname, modname, p,
> > "return", tp_probe_alloc()))
> > n++;
> >
> >> }
> >> }
> >>
> >> @@ -235,13 +250,16 @@ static void trampoline(dt_pcb_t *pcb)
> >> }
> >>
> >> static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> >> - int *idp, int *argcp, dt_argdesc_t **argvp)
> >> + int *argcp, dt_argdesc_t **argvp)
> >> {
> >> FILE *f;
> >> char fn[256];
> >> int rc;
> >> + int *idp = &((prv_data_t *)(prp->prv_data))->event_id;
> >>
> >> - *idp = -1;
> >> + /* if we have an event ID, no need to retrieve it again */
> >> + if (*idp != -1)
> >> + return -1;
> > See above.
> >
> >> /*
> >> * We know that the probe name is either "entry" or "return", so we can
> >> diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
> >> index acd01983..9ea89898 100644
> >> --- a/libdtrace/dt_provider.h
> >> +++ b/libdtrace/dt_provider.h
> >> @@ -61,19 +61,19 @@ typedef struct dt_provimpl {
> >> int (*populate)(dtrace_hdl_t *dtp); /* register probes */
> >> int (*probe_info)(dtrace_hdl_t *dtp, /* get probe info */
> >> const struct dt_probe *prp,
> >> - int *idp, int *argcp, dt_argdesc_t **argvp);
> >> + int *argcp, dt_argdesc_t **argvp);
> >> int (*provide)(dtrace_hdl_t *dtp, /* provide probes */
> >> const dtrace_probedesc_t *pdp);
> >> void (*trampoline)(dt_pcb_t *pcb); /* generate BPF trampoline */
> >> int (*attach)(dtrace_hdl_t *dtp, /* attach BPF program to probe */
> >> - struct dt_probe *prp, int bpf_fd);
> >> + const struct dt_probe *prp, int bpf_fd);
> >> int (*probe_fini)(dtrace_hdl_t *dtp, /* probe cleanup */
> >> struct dt_probe *prb);
> > const dt_probe *prp
> >
> >> } dt_provimpl_t;
> >>
> >> extern int tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, int *idp,
> >> int *argcp, dt_argdesc_t **argvp);
> >> -extern int tp_event_attach(dtrace_hdl_t *dtp, struct dt_probe *prp, int bpf_fd);
> >> +extern int tp_event_attach(dtrace_hdl_t *dtp, const struct dt_probe *prp, int bpf_fd);
> > tp_attach (per comments to an earlier patch)
> >
> >> extern dt_provimpl_t dt_dtrace;
> >> extern dt_provimpl_t dt_fbt;
> >> --
> >> 2.18.2
> >>
> >>
> >> _______________________________________________
> >> DTrace-devel mailing list
> >> DTrace-devel at oss.oracle.com
> >> https://oss.oracle.com/mailman/listinfo/dtrace-devel
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list