Date: Tue, 13 Jun 2006 01:06:11 GMT From: John Birrell <jb@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 99105 for review Message-ID: <200606130106.k5D16BEI054851@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=99105 Change 99105 by jb@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 */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200606130106.k5D16BEI054851>