Date: Wed, 16 Jul 2014 11:48:21 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mark Johnston <markj@FreeBSD.org> 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: <20140716084821.GU93733@kib.kiev.ua> In-Reply-To: <20140716041123.GA20065@raichu> References: <201407140438.s6E4cHCh016707@svn.freebsd.org> <20140714081050.GE93733@kib.kiev.ua> <20140716041123.GA20065@raichu>
next in thread | previous in thread | raw e-mail | index | archive | help
--cdjqm/zyp5sS28aW Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jul 16, 2014 at 12:11:23AM -0400, Mark Johnston wrote: > 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 > > >=20 > > > 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 t= rap. > > > =20 > > > This makes it possible to run most (but not all) of the DTrace test= s under > > > common/safety/ without triggering a kernel panic. > > > =20 > > > Submitted by: Anton Rang <anton.rang@isilon.com> (original version) > > > Phabric: D95 > > >=20 > > > 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 > > >=20 > > > Modified: head/sys/amd64/amd64/exception.S > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > > > --- head/sys/amd64/amd64/exception.S Mon Jul 14 00:16:49 2014 (r26859= 9) > > > +++ head/sys/amd64/amd64/exception.S Mon Jul 14 04:38:17 2014 (r26860= 0) > > > @@ -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. >=20 > 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. Ok. >=20 > 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. Looks fine. This should also fix the issue of kgdb and ddb not able to unwind the trap frames. >=20 > Thanks, > -Mark >=20 > 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 */ > =20 > 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 > =20 > -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); > =20 > @@ -604,6 +605,19 @@ out: > return; > } > =20 > +/* > + * 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 !=3D 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 s= ymindx, > int size; > u_int8_t *instr, *limit; > =20 > - if (strncmp(name, "dtrace_", 7) =3D=3D 0 && > - strncmp(name, "dtrace_safe_", 12) !=3D 0) { > + if ((strncmp(name, "dtrace_", 7) =3D=3D 0 && > + strncmp(name, "dtrace_safe_", 12) !=3D 0) || > + strcmp(name, "_trap") =3D=3D 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); > } --cdjqm/zyp5sS28aW Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJTxjxUAAoJEJDCuSvBvK1BhnEQAKW4QFoXHHLVsVb8cY6biKd6 ldJQpVUMEK8vGU6y0lQLqc2Wf01IXqdrfln2UktFZpXkwXY4vONqwF3n4BCKZ+VL F1Wpq6pKXLFSaFUalp1tuSUBzYsoNahNS5t4mf99OjT+7e5fgbbjOkIfoOAt9hU/ 2DRX85WGDFavmoDc2JmB27nvDkcgprSDOYC4vj0J3FMsYJ0MEpw26xbwW7ca1t15 /NLSoC4SSfEF3PnyC7t33db+GqzXERQTeJZH+dO81yjLdfstbhW3fZ5LS6tApbkW NVPu1yKkVAfk2m/AwGN5AJafU32D9rHqEtZxYmTQdDNY8ffASumCsktu+ZhJY/XL 8Fdhh7OlrR3YB+InUalikL/Nk5ExPFAddyZp9zjfrvle5QeZiSYmopLMvnLBqMyy ai002i/Og9ke1tcT23BYvnuSE7Zkj7sS3Y95LkedSNx7LaVB2Mh3h3PY2bmyHzdX 2Odlfc9n8F0BM9FslnsyUxO0EXe+Kr/Y02NoVt6ybHsON6rCFndpxcxHeIGettOM q68XDXm/jHANfVfbHAxEPr2HlcJ2PqFI6+qhcn5wFgF1bmcJ9FRe6xa8kB9vZs2w ySgFP/kFflbMvrRqvFCmwfn4rpiR1+eLqhJrrhtjbxZr8o9SJ45WiK1XaqrJ+PTW 9r2HU8RAALgM4vjKdq42 =COs1 -----END PGP SIGNATURE----- --cdjqm/zyp5sS28aW--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140716084821.GU93733>