uprobes: don't grab the process over and over again

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@oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

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;
 }	

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;
 

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 *);

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);