6271 dtrace caused excessive fork time

Reviewed by: Adam Leventhal <ahl@delphix.com>
Reviewed by: Dan McDonald <danmcd@omniti.com>
Reviewed by: Richard Lowe <richlowe@richlowe.net>
Approved by: Gordon Ross <gwr@nexenta.com>

6 files changed, 138 insertions, 24 deletions

diff --git a/usr/src/cmd/dtrace/test/tst/common/usdt/tst.sameprovmulti.ksh b/usr/src/cmd/dtrace/test/tst/common/usdt/tst.sameprovmulti.ksh
new file mode 100644
index 0000000000..04b76207ea
--- /dev/null
+++ b/usr/src/cmd/dtrace/test/tst/common/usdt/tst.sameprovmulti.ksh
@@ -0,0 +1,99 @@
+#
+# This file and its contents are supplied under the terms of the
+# Common Development and Distribution License ("CDDL"), version 1.0.
+# You may only use this file in accordance with the terms of version
+# 1.0 of the CDDL.
+#
+# A full copy of the text of the CDDL should have accompanied this
+# source.  A copy of the CDDL is also available via the Internet at
+# http://www.illumos.org/license/CDDL.
+#
+
+#
+# Copyright (c) 2015, Joyent, Inc. All rights reserved.
+#
+
+#
+# This test assures that we can have the same provider name across multiple
+# probe definitions, and that the result will be the union of those
+# definitions.  In particular, libusdt depends on this when (for example)
+# node modules that create a provider are loaded multiple times due to
+# being included by different modules.
+#
+
+if [ $# != 1 ]; then
+	echo expected one argument: '<'dtrace-path'>'
+	exit 2
+fi
+
+dtrace=$1
+DIR=/var/tmp/dtest.$$
+
+mkdir $DIR
+cd $DIR
+
+cat > test.c <<EOF
+#include <unistd.h>
+
+void
+main()
+{
+EOF
+
+objs=
+
+for oogle in bagnoogle stalloogle cockoogle; do
+	cat > $oogle.c <<EOF
+#include <sys/sdt.h>
+
+void
+$oogle()
+{
+	DTRACE_PROBE(doogle, $oogle);
+}
+EOF
+
+	cat > $oogle.d <<EOF
+provider doogle {
+	probe $oogle();
+};
+EOF
+
+	gcc -m32 -c $oogle.c
+
+	if [ $? -ne 0 ]; then
+		print -u2 "failed to compile $oogle.c"
+		exit 1
+	fi
+
+	$dtrace -G -32 -s $oogle.d $oogle.o -o $oogle.d.o
+
+	if [ $? -ne 0 ]; then
+		print -u2 "failed to process $oogle.d"
+		exit 1
+	fi
+
+	objs="$objs $oogle.o $oogle.d.o"
+	echo $oogle'();' >> test.c
+done
+
+echo "}" >> test.c
+
+gcc -m32 -o test test.c $objs
+
+if [ $? -ne 0 ]; then
+	print -u2 "failed to compile test.c"
+	exit 1
+fi
+
+$dtrace -n 'doogle$target:::{@[probename] = count()}' \
+    -n 'END{printa("%-10s %@d\n", @)}' -x quiet -x aggsortkey -Zc ./test
+
+if [ $? -ne 0 ]; then
+	print -u2 "failed to execute test"
+	exit 1
+fi
+
+cd /
+/usr/bin/rm -rf $DIR
+exit 0
diff --git a/usr/src/cmd/dtrace/test/tst/common/usdt/tst.sameprovmulti.ksh.out b/usr/src/cmd/dtrace/test/tst/common/usdt/tst.sameprovmulti.ksh.out
new file mode 100644
index 0000000000..bdeeb1ef29
--- /dev/null
+++ b/usr/src/cmd/dtrace/test/tst/common/usdt/tst.sameprovmulti.ksh.out
@@ -0,0 +1,4 @@
+bagnoogle  1
+cockoogle  1
+stalloogle 1
+
diff --git a/usr/src/pkg/manifests/system-dtrace-tests.mf b/usr/src/pkg/manifests/system-dtrace-tests.mf
index b5bf11045a..74edb5b47f 100644
--- a/usr/src/pkg/manifests/system-dtrace-tests.mf
+++ b/usr/src/pkg/manifests/system-dtrace-tests.mf
@@ -2088,6 +2088,8 @@ file path=opt/SUNWdtrt/tst/common/usdt/tst.noreapring.ksh mode=0444
 file path=opt/SUNWdtrt/tst/common/usdt/tst.onlyenabled.ksh mode=0444
 file path=opt/SUNWdtrt/tst/common/usdt/tst.reap.ksh mode=0444
 file path=opt/SUNWdtrt/tst/common/usdt/tst.reeval.ksh mode=0444
+file path=opt/SUNWdtrt/tst/common/usdt/tst.sameprovmulti.ksh mode=0444
+file path=opt/SUNWdtrt/tst/common/usdt/tst.sameprovmulti.ksh.out mode=0444
 file path=opt/SUNWdtrt/tst/common/usdt/tst.static.ksh mode=0444
 file path=opt/SUNWdtrt/tst/common/usdt/tst.static.ksh.out mode=0444
 file path=opt/SUNWdtrt/tst/common/usdt/tst.static2.ksh mode=0444
diff --git a/usr/src/uts/common/dtrace/dtrace.c b/usr/src/uts/common/dtrace/dtrace.c
index f2c3fec010..533ce81ab4 100644
--- a/usr/src/uts/common/dtrace/dtrace.c
+++ b/usr/src/uts/common/dtrace/dtrace.c
@@ -14809,8 +14809,8 @@ dtrace_helper_provider_add(dof_helper_t *dofhp, int gen)
 	 * Check to make sure this isn't a duplicate.
 	 */
 	for (i = 0; i < help->dthps_nprovs; i++) {
-		if (dofhp->dofhp_dof ==
-		    help->dthps_provs[i]->dthp_prov.dofhp_dof)
+		if (dofhp->dofhp_addr ==
+		    help->dthps_provs[i]->dthp_prov.dofhp_addr)
 			return (EALREADY);
 	}
 
@@ -15162,7 +15162,14 @@ dtrace_helper_slurp(dof_hdr_t *dof, dof_helper_t *dhp)
 	dtrace_enabling_destroy(enab);
 
 	if (dhp != NULL && nprovs > 0) {
+		/*
+		 * Now that this is in-kernel, we change the sense of the
+		 * members:  dofhp_dof denotes the in-kernel copy of the DOF
+		 * and dofhp_addr denotes the address at user-level.
+		 */
+		dhp->dofhp_addr = dhp->dofhp_dof;
 		dhp->dofhp_dof = (uint64_t)(uintptr_t)dof;
+
 		if (dtrace_helper_provider_add(dhp, gen) == 0) {
 			mutex_exit(&dtrace_lock);
 			dtrace_helper_provider_register(curproc, help, dhp);
diff --git a/usr/src/uts/common/dtrace/fasttrap.c b/usr/src/uts/common/dtrace/fasttrap.c
index 215b1a4111..3948dd258e 100644
--- a/usr/src/uts/common/dtrace/fasttrap.c
+++ b/usr/src/uts/common/dtrace/fasttrap.c
@@ -1784,6 +1784,18 @@ fasttrap_meta_provide(void *arg, dtrace_helper_provdesc_t *dhpv, pid_t pid)
 	return (provider);
 }
 
+/*
+ * We know a few things about our context here:  we know that the probe being
+ * created doesn't already exist (DTrace won't load DOF at the same address
+ * twice, even if explicitly told to do so) and we know that we are
+ * single-threaded with respect to the meta provider machinery. Knowing that
+ * this is a new probe and that there is no way for us to race with another
+ * operation on this provider allows us an important optimization: we need not
+ * lookup a probe before adding it.  Saving this lookup is important because
+ * this code is in the fork path for processes with USDT probes, and lookups
+ * here are potentially very expensive because of long hash conflicts on
+ * module, function and name (DTrace doesn't hash on provider name).
+ */
 /*ARGSUSED*/
 static void
 fasttrap_meta_create_probe(void *arg, void *parg,
@@ -1820,19 +1832,6 @@ fasttrap_meta_create_probe(void *arg, void *parg,
 			return;
 	}
 
-	/*
-	 * Grab the creation lock to ensure consistency between calls to
-	 * dtrace_probe_lookup() and dtrace_probe_create() in the face of
-	 * other threads creating probes.
-	 */
-	mutex_enter(&provider->ftp_cmtx);
-
-	if (dtrace_probe_lookup(provider->ftp_provid, dhpb->dthpb_mod,
-	    dhpb->dthpb_func, dhpb->dthpb_name) != 0) {
-		mutex_exit(&provider->ftp_cmtx);
-		return;
-	}
-
 	ntps = dhpb->dthpb_noffs + dhpb->dthpb_nenoffs;
 	ASSERT(ntps > 0);
 
@@ -1840,7 +1839,6 @@ fasttrap_meta_create_probe(void *arg, void *parg,
 
 	if (fasttrap_total > fasttrap_max) {
 		atomic_add_32(&fasttrap_total, -ntps);
-		mutex_exit(&provider->ftp_cmtx);
 		return;
 	}
 
@@ -1904,8 +1902,6 @@ fasttrap_meta_create_probe(void *arg, void *parg,
 	 */
 	pp->ftp_id = dtrace_probe_create(provider->ftp_provid, dhpb->dthpb_mod,
 	    dhpb->dthpb_func, dhpb->dthpb_name, FASTTRAP_OFFSET_AFRAMES, pp);
-
-	mutex_exit(&provider->ftp_cmtx);
 }
 
 /*ARGSUSED*/
diff --git a/usr/src/uts/common/sys/dtrace.h b/usr/src/uts/common/sys/dtrace.h
index 3b1869a4ff..b1bb6d8dd2 100644
--- a/usr/src/uts/common/sys/dtrace.h
+++ b/usr/src/uts/common/sys/dtrace.h
@@ -2131,12 +2131,18 @@ extern void dtrace_probe(dtrace_id_t, uintptr_t arg0, uintptr_t arg1,
  *
  * 1.2.4  Caller's context
  *
- *   dtms_create_probe() is called from either ioctl() or module load context.
- *   The DTrace framework is locked in such a way that meta providers may not
- *   register or unregister. This means that the meta provider cannot call
- *   dtrace_meta_register() or dtrace_meta_unregister(). However, the context is
- *   such that the provider may (and is expected to) call provider-related
- *   DTrace provider APIs including dtrace_probe_create().
+ *   dtms_create_probe() is called from either ioctl() or module load context
+ *   in the context of a newly-created provider (that is, a provider that
+ *   is a result of a call to dtms_provide_pid()). The DTrace framework is
+ *   locked in such a way that meta providers may not register or unregister,
+ *   such that no other thread can call into a meta provider operation and that
+ *   atomicity is assured with respect to meta provider operations across
+ *   dtms_provide_pid() and subsequent calls to dtms_create_probe().
+ *   The context is thus effectively single-threaded with respect to the meta
+ *   provider, and that the meta provider cannot call dtrace_meta_register()
+ *   or dtrace_meta_unregister(). However, the context is such that the
+ *   provider may (and is expected to) call provider-related DTrace provider
+ *   APIs including dtrace_probe_create().
  *
  * 1.3  void *dtms_provide_pid(void *arg, dtrace_meta_provider_t *mprov,
  *	      pid_t pid)