PERFORCE change 99105 for review

[John Birrell]

http://perforce.freebsd.org/chv.cgi?CH=99105

Change 99105 by jb at jb_freebsd2 on 2006/06/13 01:05:26

	There are a heap of functions that are unsafe to instrument, mostly,
	I suspect, due to witness in something that DTrace is calling from 
	the probe context when it shouldn't be.
	
	The list of prefixes being filtered out is way larger than necessary,
	but I've only had time to do an iterative test over all combinations
	of [a-z][a-z]*. Each test takes a while because you actually have to
	use the system in the mean time. It will only go kaboom due to an
	illegal function call at the time the function is actually called, not
	when it is instrumented.
	
	Until more work is done on this provider, it should be used with care.
	Enabling all probes if currently doesn't filter out may still cause
	a crash if you happen to fire one that I haven't seen called.
	
	For the brave hearted, you can use the wild card probe naming that
	DTrace supports rather than the sysctl filters I've built in here. Those
	sysctl filters are there so that I can choose a small number of
	probes and then run through Sun's tests. This avoids me having to edit
	all the test files.

Affected files ...

.. //depot/projects/dtrace/src/sys/cddl/dev/fbt/fbt.c#2 edit

Differences ...

==== //depot/projects/dtrace/src/sys/cddl/dev/fbt/fbt.c#2 (text+ko) ====

@@ -53,6 +53,7 @@
 #include <sys/selinfo.h>
 #include <sys/smp.h>
 #include <sys/syscall.h>
+#include <sys/sysctl.h>
 #include <sys/sysent.h>
 #include <sys/sysproto.h>
 #include <sys/uio.h>
@@ -64,6 +65,23 @@
 MALLOC_DECLARE(M_FBT);
 MALLOC_DEFINE(M_FBT, "fbt", "Function Boundary Tracing");
 
+SYSCTL_DECL(_debug_dtrace);
+int	fbt_debug = 0;
+int	fbt_filter_0 = 0;
+int	fbt_filter_1 = 0;
+int	fbt_filter_2 = 0;
+int	fbt_filter_3 = 0;
+int	fbt_filter_len = 0;
+char	fbt_filter_str[64];
+TUNABLE_INT("debug.dtrace.fbt_debug", &fbt_debug);
+SYSCTL_INT(_debug_dtrace, OID_AUTO, fbt_debug, CTLFLAG_RW, &fbt_debug, 0, "");
+SYSCTL_INT(_debug_dtrace, OID_AUTO, fbt_filter_0, CTLFLAG_RW, &fbt_filter_0, 0, "");
+SYSCTL_INT(_debug_dtrace, OID_AUTO, fbt_filter_1, CTLFLAG_RW, &fbt_filter_1, 0, "");
+SYSCTL_INT(_debug_dtrace, OID_AUTO, fbt_filter_2, CTLFLAG_RW, &fbt_filter_2, 0, "");
+SYSCTL_INT(_debug_dtrace, OID_AUTO, fbt_filter_3, CTLFLAG_RW, &fbt_filter_3, 0, "");
+SYSCTL_INT(_debug_dtrace, OID_AUTO, fbt_filter_len, CTLFLAG_RW, &fbt_filter_len, 0, "");
+SYSCTL_STRING(_debug_dtrace, OID_AUTO, fbt_filter_str, CTLFLAG_RW, fbt_filter_str, 0, "");
+
 #define	FBT_PUSHL_EBP		0x55
 #define	FBT_MOVL_ESP_EBP0_V0	0x8b
 #define	FBT_MOVL_ESP_EBP1_V0	0xec
@@ -214,35 +232,74 @@
 		return (0);
 	}
 
-	/* XXX
-	 *
-	 * If the module is the kernel, there are a few functions that
-	 * are called (unwisely) from the DTrace probe context that
-	 * will make the kernel go kaboom if instrumented. Don't ask 
-	 * me how I know! 8-)
-	 *
-	 * In the long term, I think we need to allow a developer to
-	 * specify if a function is not safe to call from a DTrace
-	 * probe. This is probably something we can do with SYSINIT.
-	 * There aren't many functions.
-	 *
-	 * Of the functions listed here, some are just listed because I
-	 * can't help putting a printf() in the probe context to work
-	 * out what is going on.
-	 *
-	 * The rest should be implemented as 'dtrace_' prefixed functions
-	 * like Sun does. I now see why they do that. 8-)
+	/*
+	 * Still trying to narrow down why some of these things are
+	 * being called from the probe context. At the moment I suspect
+	 * there is some call which executes witness code and that
+	 * is *certain* to end in tears.
+	 */
+	if (name[0] == '_' ||
+	    strcmp(name, "trap") == 0 ||
+	    strcmp(name, "userret") == 0 ||
+	    strncmp(name, "realtimer", 9) == 0 ||
+	    strncmp(name, "ithread", 7) == 0 ||
+	    strncmp(name, "itimer", 6) == 0 ||
+	    strncmp(name, "ioapic", 6) == 0 ||
+	    strncmp(name, "isitmy", 6) == 0 ||
+	    strncmp(name, "db_", 3) == 0 ||
+	    strncmp(name, "gdb_", 4) == 0 ||
+	    strncmp(name, "kdb_", 4) == 0 ||
+	    strncmp(name, "intr_", 5) == 0 ||
+
+	    strncmp(name, "vm", 2) == 0 ||
+	    strncmp(name, "th", 2) == 0 ||
+	    strncmp(name, "sh", 2) == 0 ||
+	    strncmp(name, "fi", 2) == 0 ||
+	    strncmp(name, "fd", 2) == 0 ||
+	    strncmp(name, "str", 3) == 0 ||
+	    strncmp(name, "nfs", 3) == 0 ||
+	    strncmp(name, "ata_", 4) == 0 ||
+	    strncmp(name, "acpi", 4) == 0 ||
+	    strncmp(name, "bufobj", 6) == 0 ||
+	    strncmp(name, "linker", 6) == 0 ||
+	    strncmp(name, "witness", 7) == 0 ||
+	    strncmp(name, "spinlock", 8) == 0 ||
+	    strcmp(name, "pcpu_find") == 0)
+		return (0);
+
+	/*
+	 * These filters are set by sysctl to help debugging the initial
+	 * port of this provider. Normally the wildcard would be 
+	 * specified to dtrace via the probe name (e.g. fbt::abc*:entry)
+	 * rather than using these. However, to apply a filter to narrow
+	 * the problem down while running Sun's tests, it is easier to
+	 * set these filters rather than have to modify the tests all the
+	 * time.
 	 */
-	if (strcmp(lf->filename, "kernel") == 0) {
-		if (strcmp(name, "trap") == 0 ||
-		    strcmp(name, "putchar") == 0 ||
-		    strcmp(name, "kvprintf") == 0 ||
-		    strcmp(name, "printf") == 0 ||
-		    strcmp(name, "getnanouptime") == 0 ||
-		    strcmp(name, "pcpu_find") == 0)
+	if (fbt_filter_0 != 0) {
+		if (name[0] != fbt_filter_0)
 			return (0);
+		if (fbt_filter_1 != 0) {
+			if (name[1] != fbt_filter_1)
+				return (0);
+			if (fbt_filter_2 != 0) {
+				if (name[2] != fbt_filter_2)
+					return (0);
+				if (fbt_filter_3 != 0) {
+					if (name[3] != fbt_filter_3)
+						return (0);
+				}
+			}
+		}
 	}
 
+	/*
+	 * This is an alternate form of filter. The one above is easier to
+	 * set from a shell script though.
+	 */
+	if (fbt_filter_len > 0 && strncmp(name, fbt_filter_str, fbt_filter_len) != 0)
+		return (0);
+
 	size = symval->size;
 
 	instr = (u_int8_t *) symval->value;
@@ -272,8 +329,6 @@
 	fbt_probetab[FBT_ADDR2NDX(instr)] = fbt;
 
 	lf->fbt_nentries++;
-/* XXX Do we have a problem with return probes? */
-if (strcmp(name, "ioctl") != 0) return (0);
 
 	retfbt = NULL;
 again:
@@ -408,7 +463,6 @@
 		 */
 		return;
 	}
-if (strcmp(modname, "kernel") != 0) return;
 
 	/*
 	 * List the functions in the module and the symbol values.
@@ -479,8 +533,11 @@
 		return;
 	}
 
-	for (; fbt != NULL; fbt = fbt->fbtp_next)
+	for (; fbt != NULL; fbt = fbt->fbtp_next) {
+		if (fbt_debug)
+			printf("fbt_enable %s\n",fbt->fbtp_name);
 		*fbt->fbtp_patchpoint = fbt->fbtp_patchval;
+	}
 }
 
 /* ARGSUSED */