Skip site navigation (1)Skip section navigation (2)
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>