[DTrace-devel] [PATCH] uprobes: don't grab the process over and over again
Kris Van Hees
kris.van.hees at oracle.com
Mon Sep 11 17:33:10 UTC 2023
On Tue, Jun 13, 2023 at 07:46:08PM +0100, Nick Alcock via DTrace-devel wrote:
> 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>
Reviewed-by: Kris Van Hees <kris.van.hees 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