Date: Wed, 16 Jul 2014 00:11:23 -0400 From: Mark Johnston <markj@FreeBSD.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r268600 - in head/sys: amd64/amd64 cddl/dev/dtrace/amd64 cddl/dev/dtrace/i386 cddl/dev/dtrace/mips cddl/dev/dtrace/powerpc i386/i386 mips/mips powerpc/aim sys Message-ID: <20140716041123.GA20065@raichu> In-Reply-To: <20140714081050.GE93733@kib.kiev.ua> References: <201407140438.s6E4cHCh016707@svn.freebsd.org> <20140714081050.GE93733@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jul 14, 2014 at 11:10:50AM +0300, Konstantin Belousov wrote: > On Mon, Jul 14, 2014 at 04:38:17AM +0000, Mark Johnston wrote: > > Author: markj > > Date: Mon Jul 14 04:38:17 2014 > > New Revision: 268600 > > URL: http://svnweb.freebsd.org/changeset/base/268600 > > > > Log: > > Invoke the DTrace trap handler before calling trap() on amd64. This matches > > the upstream implementation and helps ensure that a trap induced by tracing > > fbt::trap:entry is handled without recursively generating another trap. > > > > This makes it possible to run most (but not all) of the DTrace tests under > > common/safety/ without triggering a kernel panic. > > > > Submitted by: Anton Rang <anton.rang@isilon.com> (original version) > > Phabric: D95 > > > > Modified: > > head/sys/amd64/amd64/exception.S > > head/sys/amd64/amd64/trap.c > > head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c > > head/sys/cddl/dev/dtrace/i386/dtrace_subr.c > > head/sys/cddl/dev/dtrace/mips/dtrace_subr.c > > head/sys/cddl/dev/dtrace/powerpc/dtrace_subr.c > > head/sys/i386/i386/trap.c > > head/sys/mips/mips/trap.c > > head/sys/powerpc/aim/trap.c > > head/sys/sys/dtrace_bsd.h > > > > Modified: head/sys/amd64/amd64/exception.S > > ============================================================================== > > --- head/sys/amd64/amd64/exception.S Mon Jul 14 00:16:49 2014 (r268599) > > +++ head/sys/amd64/amd64/exception.S Mon Jul 14 04:38:17 2014 (r268600) > > @@ -228,7 +228,24 @@ alltraps_pushregs_no_rdi: > > .type calltrap,@function > > calltrap: > > movq %rsp,%rdi > > +#ifdef KDTRACE_HOOKS > > + /* > > + * Give DTrace a chance to vet this trap and skip the call to trap() if > > + * it turns out that it was caused by a DTrace probe. > > + */ > > + movq dtrace_trap_func,%rax > > + testq %rax,%rax > > + je skiphook > > + call *%rax > > + testq %rax,%rax > > + jne skiptrap > > + movq %rsp,%rdi > > +skiphook: > > +#endif > > call trap > Why is this fragment done in asm ? I see it relatively easy to > either move to the start of trap(), or create a new wrapper around > current trap() which calls dtrace_trap_func conditionally, and then > resorts to the old trap. You're right, it looks like this could be done in C by introducing a wrapper. I'm not sure how moving the conditional call to dtrace_trap_func() around within trap() would fix the original problem, though. The diff below adds such a wrapper, and modifies DTrace to explicitly ignore it when creating function boundary probes. Additionally, trap() is marked __noinline to ensure that it can still be instrumented. I've named it "_trap" for now for lack of a better name, but that will need to change. Thanks, -Mark diff --git a/sys/amd64/amd64/exception.S b/sys/amd64/amd64/exception.S index bb5fd56..3d34724 100644 --- a/sys/amd64/amd64/exception.S +++ b/sys/amd64/amd64/exception.S @@ -228,24 +228,7 @@ alltraps_pushregs_no_rdi: .type calltrap,@function calltrap: movq %rsp,%rdi -#ifdef KDTRACE_HOOKS - /* - * Give DTrace a chance to vet this trap and skip the call to trap() if - * it turns out that it was caused by a DTrace probe. - */ - movq dtrace_trap_func,%rax - testq %rax,%rax - je skiphook - call *%rax - testq %rax,%rax - jne skiptrap - movq %rsp,%rdi -skiphook: -#endif - call trap -#ifdef KDTRACE_HOOKS -skiptrap: -#endif + call _trap MEXITCOUNT jmp doreti /* Handle any pending ASTs */ diff --git a/sys/amd64/amd64/trap.c b/sys/amd64/amd64/trap.c index d9203bc..545174a 100644 --- a/sys/amd64/amd64/trap.c +++ b/sys/amd64/amd64/trap.c @@ -97,7 +97,8 @@ PMC_SOFT_DEFINE( , , page_fault, write); #include <sys/dtrace_bsd.h> #endif -extern void trap(struct trapframe *frame); +extern void _trap(struct trapframe *frame); +extern void __noinline trap(struct trapframe *frame); extern void syscall(struct trapframe *frame); void dblfault_handler(struct trapframe *frame); @@ -604,6 +605,19 @@ out: return; } +/* + * Ensure that we ignore any DTrace-induced faults. This function cannot + * be instrumented, so it cannot generate such faults itself. + */ +void +_trap(struct trapframe *frame) +{ + + if (dtrace_trap_func != NULL && (*dtrace_trap_func)(frame)) + return; + trap(frame); +} + static int trap_pfault(frame, usermode) struct trapframe *frame; diff --git a/sys/cddl/dev/fbt/fbt.c b/sys/cddl/dev/fbt/fbt.c index 7e256e7..0cc2db2 100644 --- a/sys/cddl/dev/fbt/fbt.c +++ b/sys/cddl/dev/fbt/fbt.c @@ -232,13 +232,18 @@ fbt_provide_module_function(linker_file_t lf, int symindx, int size; u_int8_t *instr, *limit; - if (strncmp(name, "dtrace_", 7) == 0 && - strncmp(name, "dtrace_safe_", 12) != 0) { + if ((strncmp(name, "dtrace_", 7) == 0 && + strncmp(name, "dtrace_safe_", 12) != 0) || + strcmp(name, "_trap") == 0) { /* * Anything beginning with "dtrace_" may be called * from probe context unless it explicitly indicates * that it won't be called from probe context by * using the prefix "dtrace_safe_". + * + * Additionally, we avoid instrumenting _trap() to avoid the + * possibility of generating a fault in probe context before + * DTrace's fault handler is called. */ return (0); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140716041123.GA20065>