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
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 +, 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 (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.
> > > + */
> > > + movqdtrace_trap_func,%rax
> > > + testq %rax,%rax
> > > + je skiphook
> > > + call*%rax
> > > + testq %rax,%rax
> > > + jne skiptrap
> > > + movq%rsp,%rdi
> > > +skiphook:
> > > +#endif
> > > calltrap
> > 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.
Ok.
>
> 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.
>
> 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.
> - */
> - movqdtrace_trap_func,%rax
> - testq %rax,%rax
> - je skiphook
> - call*%rax
> - testq %rax,%rax
> - jne skiptrap
> - movq%rsp,%rdi
> -skiphook:
> -#endif
> - calltrap
> -#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
> #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/cd
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
On Mon, Jul 14, 2014 at 11:10:50AM +0300, Konstantin Belousov wrote:
> On Mon, Jul 14, 2014 at 04:38:17AM +, 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 (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.SMon Jul 14 00:16:49 2014
> > (r268599)
> > +++ head/sys/amd64/amd64/exception.SMon 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.
> > +*/
> > + movqdtrace_trap_func,%rax
> > + testq %rax,%rax
> > + je skiphook
> > + call*%rax
> > + testq %rax,%rax
> > + jne skiptrap
> > + movq%rsp,%rdi
> > +skiphook:
> > +#endif
> > calltrap
> 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.
-*/
- movqdtrace_trap_func,%rax
- testq %rax,%rax
- je skiphook
- call*%rax
- testq %rax,%rax
- jne skiptrap
- movq%rsp,%rdi
-skiphook:
-#endif
- calltrap
-#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
#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)
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
On Mon, Jul 14, 2014 at 04:38:17AM +, 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 (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.
> + */
> + movqdtrace_trap_func,%rax
> + testq %rax,%rax
> + je skiphook
> + call*%rax
> + testq %rax,%rax
> + jne skiptrap
> + movq%rsp,%rdi
> +skiphook:
> +#endif
> calltrap
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.
> +#ifdef KDTRACE_HOOKS
> +skiptrap:
> +#endif
> MEXITCOUNT
> jmp doreti /* Handle any pending ASTs */
>
>
> Modified: head/sys/amd64/amd64/trap.c
> ==
> --- head/sys/amd64/amd64/trap.c Mon Jul 14 00:16:49 2014
> (r268599)
> +++ head/sys/amd64/amd64/trap.c Mon Jul 14 04:38:17 2014
> (r268600)
> @@ -218,18 +218,6 @@ trap(struct trapframe *frame)
> goto out;
> }
>
> -#ifdef KDTRACE_HOOKS
> - /*
> - * A trap can occur while DTrace executes a probe. Before
> - * executing the probe, DTrace blocks re-scheduling and sets
> - * a flag in its per-cpu flags to indicate that it doesn't
> - * want to fault. On returning from the probe, the no-fault
> - * flag is cleared and finally re-scheduling is enabled.
> - */
> - if (dtrace_trap_func != NULL && (*dtrace_trap_func)(frame, type))
> - goto out;
> -#endif
> -
> if ((frame->tf_rflags & PSL_I) == 0) {
> /*
>* Buggy application or kernel code has disabled
>
> Modified: head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c
> ==
> --- head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c Mon Jul 14 00:16:49
> 2014(r268599)
> +++ head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c Mon Jul 14 04:38:17
> 2014(r268600)
> @@ -462,29 +462,27 @@ dtrace_gethrestime(void)
> return (current_time.tv_sec * 10ULL + current_time.tv_nsec);
> }
>
> -/* Function to handle DTrace traps during probes. See amd64/amd64/trap.c */
> +/*
> + * Function to handle DTrace traps during probes. See
> amd64/amd64/exception.S.
> + */
> int
> -dtrace_trap(struct trapframe *frame, u_int type)
> +dtrace_trap(struct trapframe *frame)
> {
> /*
>* A trap can occur while DTrace executes a probe. Before
>* executing the probe, DTrace blocks re-scheduling and sets
> - * a flag in it's per-cpu flags to indicate that it doesn't
> + * a flag in its per-cpu flags to indicate that it doesn't
>* want to fault. On returning from the probe, the no-fault
>* flag is cleared and finally re-scheduling is enabled.
>*
>* Check if DTrace has enabled 'no-fault' mode:
> - *
>*/
> if ((cpu_core[curcpu].cpuc_dtrace_flags & CPU_DTRACE_NOFAULT) != 0) {
> /*
>* There are only a couple of trap types that are expected.
>* All the rest will be handled in the usual way.
>*/
> - switch (type) {
> - /* Privilieged instruction fault. */
> - case T_PRIVINFLT:
> - break;
> + switch (frame->tf_trapno) {
>
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
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 (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.SMon Jul 14 00:16:49 2014
(r268599)
+++ head/sys/amd64/amd64/exception.SMon 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.
+*/
+ movqdtrace_trap_func,%rax
+ testq %rax,%rax
+ je skiphook
+ call*%rax
+ testq %rax,%rax
+ jne skiptrap
+ movq%rsp,%rdi
+skiphook:
+#endif
calltrap
+#ifdef KDTRACE_HOOKS
+skiptrap:
+#endif
MEXITCOUNT
jmp doreti /* Handle any pending ASTs */
Modified: head/sys/amd64/amd64/trap.c
==
--- head/sys/amd64/amd64/trap.c Mon Jul 14 00:16:49 2014(r268599)
+++ head/sys/amd64/amd64/trap.c Mon Jul 14 04:38:17 2014(r268600)
@@ -218,18 +218,6 @@ trap(struct trapframe *frame)
goto out;
}
-#ifdef KDTRACE_HOOKS
- /*
-* A trap can occur while DTrace executes a probe. Before
-* executing the probe, DTrace blocks re-scheduling and sets
-* a flag in its per-cpu flags to indicate that it doesn't
-* want to fault. On returning from the probe, the no-fault
-* flag is cleared and finally re-scheduling is enabled.
-*/
- if (dtrace_trap_func != NULL && (*dtrace_trap_func)(frame, type))
- goto out;
-#endif
-
if ((frame->tf_rflags & PSL_I) == 0) {
/*
* Buggy application or kernel code has disabled
Modified: head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c
==
--- head/sys/cddl/dev/dtrace/amd64/dtrace_subr.cMon Jul 14 00:16:49
2014(r268599)
+++ head/sys/cddl/dev/dtrace/amd64/dtrace_subr.cMon Jul 14 04:38:17
2014(r268600)
@@ -462,29 +462,27 @@ dtrace_gethrestime(void)
return (current_time.tv_sec * 10ULL + current_time.tv_nsec);
}
-/* Function to handle DTrace traps during probes. See amd64/amd64/trap.c */
+/*
+ * Function to handle DTrace traps during probes. See amd64/amd64/exception.S.
+ */
int
-dtrace_trap(struct trapframe *frame, u_int type)
+dtrace_trap(struct trapframe *frame)
{
/*
* A trap can occur while DTrace executes a probe. Before
* executing the probe, DTrace blocks re-scheduling and sets
-* a flag in it's per-cpu flags to indicate that it doesn't
+* a flag in its per-cpu flags to indicate that it doesn't
* want to fault. On returning from the probe, the no-fault
* flag is cleared and finally re-scheduling is enabled.
*
* Check if DTrace has enabled 'no-fault' mode:
-*
*/
if ((cpu_core[curcpu].cpuc_dtrace_flags & CPU_DTRACE_NOFAULT) != 0) {
/*
* There are only a couple of trap types that are expected.
* All the rest will be handled in the usual way.
*/
- switch (type) {
- /* Privilieged instruction fault. */
- case T_PRIVINFLT:
- break;
+ switch (frame->tf_trapno) {
/* General protection fault. */
case T_PROTFLT:
/* Flag an illegal operation. */
Modified: head/sys/cddl/dev/dtrace/i386/dtrace_subr.c
==
--- head/sys/cddl/dev/dtrace/i386/dtrace_subr.c Mon Jul 14 00:16:49 2014
(r268599)
+++ head/sys/cddl/dev/dtrace/i386/dtrace_subr.c Mon Jul 14 04:38:17 2014
(r268600)
