[DTrace-devel] [PATCH] uprobes: don't grab the process over and over again
Nick Alcock
nick.alcock at oracle.com
Tue Jun 13 18:46:08 UTC 2023
In a foolish quest to push knowledge of libproc out of dtprobed.c, I had
the lowest-level uprobe_spec_by_addr() function, that gets given an
address and needs to know what mapping it is part of also be the
function that grabs the process with libproc's Pgrab(). This seems neat,
but Pgrab() is *expensive*, and this is needlessly inefficient: all
probes for a given mapping are registered in one go, always, but we are
grabbing the mapping *and the pid* over and over again for every probe.
As a fix for this, lift the Pgrab()/Prelease() pair out of
uprobe_spec_by_addr() into process_dof() in dtprobed. This massively
speeds up dtprobed when large numbers of probes are involved.
(We are still grabbing the process once per lump-of-DOF, but arranging
to grab it only once for all the DOF in a process is more complex, since
it requires us to maintain the grab across ioctl()s but not keep it for
too long even though we never get told when the initial splurge of DOF
contribution at process startup is about to be over. We'll get there,
but not quite yet.)
Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
---
dtprobed/dtprobed.c | 22 ++++++++++++++++---
libcommon/uprobes.c | 44 +++++++++-----------------------------
libcommon/uprobes.h | 9 ++++----
libdtrace/dt_prov_dtrace.c | 2 +-
4 files changed, 35 insertions(+), 42 deletions(-)
diff --git a/dtprobed/dtprobed.c b/dtprobed/dtprobed.c
index de2472ae7753..507a412ad0de 100644
--- a/dtprobed/dtprobed.c
+++ b/dtprobed/dtprobed.c
@@ -411,7 +411,7 @@ dof_read(fuse_req_t req, int in)
* process's dynamic linker.
*/
static void
-create_probe(pid_t pid, dof_parsed_t *provider, dof_parsed_t *probe,
+create_probe(ps_prochandle *P, dof_parsed_t *provider, dof_parsed_t *probe,
dof_parsed_t *tp)
{
const char *mod, *fun, *prb;
@@ -420,7 +420,7 @@ create_probe(pid_t pid, dof_parsed_t *provider, dof_parsed_t *probe,
fun = mod + strlen(mod) + 1;
prb = fun + strlen(fun) + 1;
- free(uprobe_create_from_addr(pid, tp->tracepoint.addr,
+ free(uprobe_create_from_addr(P, tp->tracepoint.addr,
tp->tracepoint.is_enabled, provider->provider.name,
mod, fun, prb));
}
@@ -672,11 +672,19 @@ static int
process_dof(fuse_req_t req, int out, int in, pid_t pid,
dof_helper_t *dh, const void *in_buf)
{
+ int perr = 0;
+ ps_prochandle *P;
dof_parsed_t *provider;
const char *errmsg;
size_t i;
size_t tries = 0;
+ if ((P = Pgrab(pid, 2, 0, NULL, &perr)) == NULL) {
+ fuse_log(FUSE_LOG_ERR, "%i: dtprobed: process grab failed: %s\n",
+ pid, strerror(perr));
+ goto proc_err;
+ }
+
do {
errmsg = "DOF parser write failed";
while ((errno = dof_parser_host_write(out, dh,
@@ -722,17 +730,25 @@ process_dof(fuse_req_t req, int out, int in, pid_t pid,
* Ignore errors here: we want to create as many probes
* as we can, even if creation of some of them fails.
*/
- create_probe(pid, provider, probe, tp);
+ create_probe(P, provider, probe, tp);
free(tp);
}
free(probe);
}
free(provider);
+ Prelease(P, PS_RELEASE_NORMAL);
+ Pfree(P);
+
return 0;
err:
fuse_log(FUSE_LOG_ERR, "%i: dtprobed: parser error: %s\n", pid, errmsg);
+
+ Prelease(P, PS_RELEASE_NORMAL);
+ Pfree(P);
+
+proc_err:
dof_parser_tidy(1);
return -1;
}
diff --git a/libcommon/uprobes.c b/libcommon/uprobes.c
index e910b9e6af44..cf82882c8fe7 100644
--- a/libcommon/uprobes.c
+++ b/libcommon/uprobes.c
@@ -16,26 +16,15 @@
#include <tracefs.h>
/*
- * Return a uprobe spec for a given address in a given PID (or
- * process handle, to use an already-grabbed process).
+ * Return a uprobe spec for a given address in a given process handle.
*/
char *
-uprobe_spec_by_addr(pid_t pid, ps_prochandle *P, uint64_t addr,
- prmap_t *mapp_)
+uprobe_spec_by_addr(ps_prochandle *P, uint64_t addr, prmap_t *mapp_)
{
- int free_p = 0;
- int perr = 0;
char *spec = NULL;
const prmap_t *mapp, *first_mapp;
char *mapfile_name = NULL;
- if (!P) {
- P = Pgrab(pid, 2, 0, NULL, &perr);
- if (P == NULL)
- return NULL;
- free_p = 1;
- }
-
mapp = Paddr_to_map(P, addr);
if (mapp == NULL)
goto out;
@@ -66,21 +55,6 @@ uprobe_spec_by_addr(pid_t pid, ps_prochandle *P, uint64_t addr,
out:
free(mapfile_name);
-
- if (free_p) {
- /*
- * Some things in the prmap aren't valid once the prochandle is
- * freed.
- */
- if (mapp_) {
- mapp_->pr_mapaddrname = NULL;
- mapp_->pr_file = NULL;
- }
-
- Prelease(P, PS_RELEASE_NORMAL);
- Pfree(P);
- }
-
return spec;
}
@@ -315,19 +289,21 @@ uprobe_create(dev_t dev, ino_t ino, uint64_t addr, const char *spec, int isret,
}
/*
- * Create a uprobe given a particular pid and address. Return the probe's name
- * as a new dynamically-allocated string, or NULL on error. If prv/mod/fun/prb
- * are set, they are passed down as the name of the corresponding DTrace probe.
+ * Create a uprobe given a particular process and address. Return the probe's
+ * name as a new dynamically-allocated string, or NULL on error. If
+ * prv/mod/fun/prb are set, they are passed down as the name of the
+ * corresponding DTrace probe.
*/
char *
-uprobe_create_from_addr(pid_t pid, uint64_t addr, int is_enabled, const char *prv,
- const char *mod, const char *fun, const char *prb)
+uprobe_create_from_addr(ps_prochandle *P, uint64_t addr, int is_enabled,
+ const char *prv, const char *mod, const char *fun,
+ const char *prb)
{
char *spec;
char *name;
prmap_t mapp;
- spec = uprobe_spec_by_addr(pid, NULL, addr, &mapp);
+ spec = uprobe_spec_by_addr(P, addr, &mapp);
if (!spec)
return NULL;
diff --git a/libcommon/uprobes.h b/libcommon/uprobes.h
index bd5f79d1cc9b..dac2872e5268 100644
--- a/libcommon/uprobes.h
+++ b/libcommon/uprobes.h
@@ -13,7 +13,7 @@
#include <libproc.h>
#include <unistd.h>
-extern char *uprobe_spec_by_addr(pid_t pid, ps_prochandle *P, uint64_t addr,
+extern char *uprobe_spec_by_addr(ps_prochandle *P, uint64_t addr,
prmap_t *mapp);
extern char *uprobe_name(dev_t dev, ino_t ino, uint64_t addr, int isret,
int is_enabled);
@@ -23,9 +23,10 @@ extern char *uprobe_create_named(dev_t dev, ino_t ino, uint64_t addr,
const char *fun, const char *prb);
extern char *uprobe_create(dev_t dev, ino_t ino, uint64_t addr, const char *spec,
int isret, int is_enabled);
-extern char *uprobe_create_from_addr(pid_t pid, uint64_t addr, int is_enabled,
- const char *prv, const char *mod,
- const char *fun, const char *prb);
+extern char *uprobe_create_from_addr(ps_prochandle *P, uint64_t addr,
+ int is_enabled, const char *prv,
+ const char *mod, const char *fun,
+ const char *prb);
extern int uprobe_delete(dev_t dev, ino_t ino, uint64_t addr, int isret,
int is_enabled);
extern char *uprobe_encode_name(const char *);
diff --git a/libdtrace/dt_prov_dtrace.c b/libdtrace/dt_prov_dtrace.c
index 0c3bb1357f22..474274558be3 100644
--- a/libdtrace/dt_prov_dtrace.c
+++ b/libdtrace/dt_prov_dtrace.c
@@ -187,7 +187,7 @@ static char *uprobe_spec(const char *prb)
/* look up function and thus addr */
if (Pxlookup_by_name(P, -1, PR_OBJ_EVERY, fun, &sym, NULL) == 0)
- spec = uprobe_spec_by_addr(getpid(), P, sym.st_value, NULL);
+ spec = uprobe_spec_by_addr(P, sym.st_value, NULL);
free(fun);
Prelease(P, PS_RELEASE_NORMAL);
base-commit: 42f15d3235bcf816ed814b88d64492a63fc5f0f0
--
2.40.1.269.g9f1f0b2736
More information about the DTrace-devel
mailing list