2011-04-22 23:04:13

by Vaibhav Nagarnaik

[permalink] [raw]
Subject: [PATCH 1/2] trace: Add trap entry/exit tracepoints

From: Jiaying Zhang <[email protected]>

Add an enum for trap definitions and add entry/exit tracepoints for
traps in the system.

$ echo 1 > debug/tracing/events/trap/enable
run gdb to genrate some trap events
$ cat debug/tracing/trace
<...>-13619 [003] 917.726602: trap_entry: number=3
<...>-13619 [003] 917.726612: trap_exit: number=3
<...>-13619 [003] 917.747263: trap_entry: number=1
<...>-13619 [003] 917.747272: trap_exit: number=1
<...>-13619 [003] 917.747567: trap_entry: number=3
<...>-13619 [003] 917.747570: trap_exit: number=3
<...>-13619 [003] 917.748101: trap_entry: number=1
<...>-13619 [003] 917.748103: trap_exit: number=1

$ echo 1 > tracing_enabled; ~/trap pagefault; echo 0 > tracing_enabled
$ cat trace | grep "trap-" | grep number=14 | head
trap-12528 [003] 1159.755792: trap_entry: number=14
trap-12528 [003] 1159.755801: trap_entry: number=14
trap-12528 [003] 1159.755804: trap_entry: number=14
trap-12528 [003] 1159.755807: trap_entry: number=14
trap-12528 [003] 1159.755810: trap_entry: number=14
trap-12528 [003] 1159.755817: trap_entry: number=14
trap-12528 [003] 1159.755819: trap_entry: number=14
trap-12528 [003] 1159.755821: trap_entry: number=14
trap-12528 [003] 1159.755824: trap_entry: number=14
trap-12528 [003] 1159.755826: trap_entry: number=14
$ cat trace | grep "trap-" | grep number=7 | head
trap-12528 [003] 1159.756283: trap_entry: number=7
trap-12529 [003] 1159.757427: trap_entry: number=7
trap-12530 [003] 1159.758277: trap_entry: number=7
trap-12531 [003] 1159.759172: trap_entry: number=7
trap-12532 [003] 1159.768643: trap_entry: number=7
trap-12533 [003] 1159.778195: trap_entry: number=7
trap-15026 [001] 1557.877722: trap_entry: number=7
trap-15253 [001] 1621.395067: trap_entry: number=7

Signed-off-by: Vaibhav Nagarnaik <[email protected]>
---
arch/x86/kernel/ptrace.c | 4 +++
arch/x86/kernel/traps.c | 16 ++++++++++++++
arch/x86/mm/fault.c | 13 ++++++++++-
include/trace/events/trap.h | 47 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 78 insertions(+), 2 deletions(-)
create mode 100644 include/trace/events/trap.h

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 45892dc..d788752 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -22,6 +22,8 @@
#include <linux/perf_event.h>
#include <linux/hw_breakpoint.h>

+#include <trace/events/trap.h>
+
#include <asm/uaccess.h>
#include <asm/pgtable.h>
#include <asm/system.h>
@@ -1330,8 +1332,10 @@ void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs,
struct siginfo info;

fill_sigtrap_info(tsk, regs, error_code, si_code, &info);
+ trace_trap_entry(tsk->thread.trap_no);
/* Send us the fake SIGTRAP */
force_sig_info(SIGTRAP, &info, tsk);
+ trace_trap_exit(tsk->thread.trap_no);
}


diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4857ff6..d450349 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -33,6 +33,9 @@
#include <linux/io.h>
#include <trace/events/irq.h>

+#define CREATE_TRACE_POINTS
+#include <trace/events/trap.h>
+
#ifdef CONFIG_EISA
#include <linux/ioport.h>
#include <linux/eisa.h>
@@ -123,6 +126,7 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
{
struct task_struct *tsk = current;

+ trace_trap_entry(trapnr);
#ifdef CONFIG_X86_32
if (regs->flags & X86_VM_MASK) {
/*
@@ -169,6 +173,7 @@ trap_signal:
force_sig_info(signr, info, tsk);
else
force_sig(signr, tsk);
+ trace_trap_exit(trapnr);
return;

kernel_trap:
@@ -177,6 +182,7 @@ kernel_trap:
tsk->thread.trap_no = trapnr;
die(str, regs, error_code);
}
+ trace_trap_exit(trapnr);
return;

#ifdef CONFIG_X86_32
@@ -184,6 +190,7 @@ vm86_trap:
if (handle_vm86_trap((struct kernel_vm86_regs *) regs,
error_code, trapnr))
goto trap_signal;
+ trace_trap_exit(trapnr);
return;
#endif
}
@@ -286,7 +293,9 @@ do_general_protection(struct pt_regs *regs, long error_code)
printk("\n");
}

+ trace_trap_entry(tsk->thread.trap_no);
force_sig(SIGSEGV, tsk);
+ trace_trap_exit(tsk->thread.trap_no);
return;

#ifdef CONFIG_X86_32
@@ -683,7 +692,9 @@ void math_error(struct pt_regs *regs, int error_code, int trapnr)
*/
return; /* Spurious trap, no error */
}
+ trace_trap_entry(task->thread.trap_no);
force_sig_info(SIGFPE, &info, task);
+ trace_trap_exit(task->thread.trap_no);
}

dotraplinkage void do_coprocessor_error(struct pt_regs *regs, long error_code)
@@ -704,11 +715,13 @@ do_simd_coprocessor_error(struct pt_regs *regs, long error_code)
dotraplinkage void
do_spurious_interrupt_bug(struct pt_regs *regs, long error_code)
{
+ trace_trap_entry(15);
conditional_sti(regs);
#if 0
/* No need to warn about this any longer. */
printk(KERN_INFO "Ignoring P6 Local APIC Spurious Interrupt Bug...\n");
#endif
+ trace_trap_exit(15);
}

asmlinkage void __attribute__((weak)) smp_thermal_interrupt(void)
@@ -780,6 +793,7 @@ EXPORT_SYMBOL_GPL(math_state_restore);
dotraplinkage void __kprobes
do_device_not_available(struct pt_regs *regs, long error_code)
{
+ trace_trap_entry(7);
#ifdef CONFIG_MATH_EMULATION
if (read_cr0() & X86_CR0_EM) {
struct math_emu_info info = { };
@@ -788,6 +802,7 @@ do_device_not_available(struct pt_regs *regs, long error_code)

info.regs = regs;
math_emulate(&info);
+ trace_trap_exit(7);
return;
}
#endif
@@ -795,6 +810,7 @@ do_device_not_available(struct pt_regs *regs, long error_code)
#ifdef CONFIG_X86_32
conditional_sti(regs);
#endif
+ trace_trap_exit(7);
}

#ifdef CONFIG_X86_32
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 20e3f87..55bdd31 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -11,6 +11,7 @@
#include <linux/kprobes.h> /* __kprobes, ... */
#include <linux/mmiotrace.h> /* kmmio_handler, ... */
#include <linux/perf_event.h> /* perf_sw_event */
+#include <trace/events/trap.h> /* trap trace events */
#include <linux/hugetlb.h> /* hstate_index_to_shift */

#include <asm/traps.h> /* dotraplinkage, ... */
@@ -955,8 +956,8 @@ static int fault_in_kernel_space(unsigned long address)
* and the problem, and then passes it off to one of the appropriate
* routines.
*/
-dotraplinkage void __kprobes
-do_page_fault(struct pt_regs *regs, unsigned long error_code)
+static __always_inline void
+__do_page_fault(struct pt_regs *regs, unsigned long error_code)
{
struct vm_area_struct *vma;
struct task_struct *tsk;
@@ -1164,3 +1165,11 @@ good_area:

up_read(&mm->mmap_sem);
}
+
+dotraplinkage void __kprobes
+do_page_fault(struct pt_regs *regs, unsigned long error_code)
+{
+ trace_trap_entry(14);
+ __do_page_fault(regs, error_code);
+ trace_trap_exit(14);
+}
diff --git a/include/trace/events/trap.h b/include/trace/events/trap.h
new file mode 100644
index 0000000..1519b62
--- /dev/null
+++ b/include/trace/events/trap.h
@@ -0,0 +1,47 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM trap
+
+#if !defined(_TRACE_TRAP_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_TRAP_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(trap_entry,
+
+ TP_PROTO(int id),
+
+ TP_ARGS(id),
+
+ TP_STRUCT__entry(
+ __field( int, id )
+ ),
+
+ TP_fast_assign(
+ __entry->id = id;
+ ),
+
+ TP_printk("number=%d", __entry->id)
+);
+
+TRACE_EVENT(trap_exit,
+
+ TP_PROTO(int id),
+
+ TP_ARGS(id),
+
+ TP_STRUCT__entry(
+ __field( int, id )
+ ),
+
+ TP_fast_assign(
+ __entry->id = id;
+ ),
+
+ TP_printk("number=%d", __entry->id)
+);
+
+#endif /* _TRACE_TRAP_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
+
--
1.7.3.1


2011-04-22 23:04:32

by Vaibhav Nagarnaik

[permalink] [raw]
Subject: [PATCH 2/2] x86: Change trap definitions to enumerated values

From: Aditya Kali <[email protected]>

The traps are referred to by their numbers and it can be difficult to
understand them while reading the code without context. This patch adds
enumeration of the trap numbers and replaced the numbers to the correct
enum for x86.

Signed-off-by: Vaibhav Nagarnaik <[email protected]>
---
arch/x86/include/asm/traps.h | 24 ++++++++
arch/x86/kernel/traps.c | 129 +++++++++++++++++++++++-------------------
2 files changed, 94 insertions(+), 59 deletions(-)

diff --git a/arch/x86/include/asm/traps.h b/arch/x86/include/asm/traps.h
index 0310da6..c561caf 100644
--- a/arch/x86/include/asm/traps.h
+++ b/arch/x86/include/asm/traps.h
@@ -87,4 +87,28 @@ asmlinkage void smp_thermal_interrupt(void);
asmlinkage void mce_threshold_interrupt(void);
#endif

+/* Interrupts/Exceptions */
+enum {
+ INTR_DIV_BY_ZERO = 0, /* 0 */
+ INTR_DEBUG, /* 1 */
+ INTR_NMI, /* 2 */
+ INTR_BREAKPOINT, /* 3 */
+ INTR_OVERFLOW, /* 4 */
+ INTR_BOUNDS_CHECK, /* 5 */
+ INTR_INVALID_OP, /* 6 */
+ INTR_NO_DEV, /* 7 */
+ INTR_DBL_FAULT, /* 8 */
+ INTR_SEG_OVERRUN, /* 9 */
+ INTR_INVALID_TSS, /* 10 */
+ INTR_NO_SEG, /* 11 */
+ INTR_STACK_FAULT, /* 12 */
+ INTR_GPF, /* 13 */
+ INTR_PAGE_FAULT, /* 14 */
+ INTR_SPURIOUS, /* 15 */
+ INTR_COPROCESSOR, /* 16 */
+ INTR_ALIGNMENT, /* 17 */
+ INTR_MCE, /* 18 */
+ INTR_SIMD_COPROCESSOR /* 19 */
+};
+
#endif /* _ASM_X86_TRAPS_H */
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index d450349..f6c5276 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -133,7 +133,7 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
* traps 0, 1, 3, 4, and 5 should be forwarded to vm86.
* On nmi (interrupt 2), do_trap should not be called.
*/
- if (trapnr < 6)
+ if (trapnr < INTR_INVALID_OP)
goto vm86_trap;
goto trap_signal;
}
@@ -220,27 +220,32 @@ dotraplinkage void do_##name(struct pt_regs *regs, long error_code) \
do_trap(trapnr, signr, str, regs, error_code, &info); \
}

-DO_ERROR_INFO(0, SIGFPE, "divide error", divide_error, FPE_INTDIV, regs->ip)
-DO_ERROR(4, SIGSEGV, "overflow", overflow)
-DO_ERROR(5, SIGSEGV, "bounds", bounds)
-DO_ERROR_INFO(6, SIGILL, "invalid opcode", invalid_op, ILL_ILLOPN, regs->ip)
-DO_ERROR(9, SIGFPE, "coprocessor segment overrun", coprocessor_segment_overrun)
-DO_ERROR(10, SIGSEGV, "invalid TSS", invalid_TSS)
-DO_ERROR(11, SIGBUS, "segment not present", segment_not_present)
+DO_ERROR_INFO(INTR_DIV_BY_ZERO, SIGFPE, "divide error", divide_error,
+ FPE_INTDIV, regs->ip)
+DO_ERROR(INTR_OVERFLOW, SIGSEGV, "overflow", overflow)
+DO_ERROR(INTR_BOUNDS_CHECK, SIGSEGV, "bounds", bounds)
+DO_ERROR_INFO(INTR_INVALID_OP, SIGILL, "invalid opcode", invalid_op,
+ ILL_ILLOPN, regs->ip)
+DO_ERROR(INTR_SEG_OVERRUN, SIGFPE, "coprocessor segment overrun",
+ coprocessor_segment_overrun)
+DO_ERROR(INTR_INVALID_TSS, SIGSEGV, "invalid TSS", invalid_TSS)
+DO_ERROR(INTR_NO_SEG, SIGBUS, "segment not present", segment_not_present)
#ifdef CONFIG_X86_32
-DO_ERROR(12, SIGBUS, "stack segment", stack_segment)
+DO_ERROR(INTR_STACK_FAULT, SIGBUS, "stack segment", stack_segment)
#endif
-DO_ERROR_INFO(17, SIGBUS, "alignment check", alignment_check, BUS_ADRALN, 0)
+DO_ERROR_INFO(INTR_ALIGNMENT, SIGBUS, "alignment check", alignment_check,
+ BUS_ADRALN, 0)

#ifdef CONFIG_X86_64
/* Runs on IST stack */
dotraplinkage void do_stack_segment(struct pt_regs *regs, long error_code)
{
if (notify_die(DIE_TRAP, "stack segment", regs, error_code,
- 12, SIGBUS) == NOTIFY_STOP)
+ INTR_STACK_FAULT, SIGBUS) == NOTIFY_STOP)
return;
preempt_conditional_sti(regs);
- do_trap(12, SIGBUS, "stack segment", regs, error_code, NULL);
+ do_trap(INTR_STACK_FAULT, SIGBUS, "stack segment", regs, error_code,
+ NULL);
preempt_conditional_cli(regs);
}

@@ -250,10 +255,10 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
struct task_struct *tsk = current;

/* Return not checked because double check cannot be ignored */
- notify_die(DIE_TRAP, str, regs, error_code, 8, SIGSEGV);
+ notify_die(DIE_TRAP, str, regs, error_code, INTR_DBL_FAULT, SIGSEGV);

tsk->thread.error_code = error_code;
- tsk->thread.trap_no = 8;
+ tsk->thread.trap_no = INTR_DBL_FAULT;

/*
* This is always a kernel trap and never fixable (and thus must
@@ -281,7 +286,7 @@ do_general_protection(struct pt_regs *regs, long error_code)
goto gp_in_kernel;

tsk->thread.error_code = error_code;
- tsk->thread.trap_no = 13;
+ tsk->thread.trap_no = INTR_GPF;

if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
printk_ratelimit()) {
@@ -310,9 +315,9 @@ gp_in_kernel:
return;

tsk->thread.error_code = error_code;
- tsk->thread.trap_no = 13;
+ tsk->thread.trap_no = INTR_GPF;
if (notify_die(DIE_GPF, "general protection fault", regs,
- error_code, 13, SIGSEGV) == NOTIFY_STOP)
+ error_code, INTR_GPF, SIGSEGV) == NOTIFY_STOP)
return;
die("general protection fault", regs, error_code);
}
@@ -381,7 +386,8 @@ io_check_error(unsigned char reason, struct pt_regs *regs)
static notrace __kprobes void
unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
{
- if (notify_die(DIE_NMIUNKNOWN, "nmi", regs, reason, 2, SIGINT) ==
+ if (notify_die(DIE_NMIUNKNOWN, "nmi", regs, reason, INTR_NMI,
+ SIGINT) ==
NOTIFY_STOP)
return;
#ifdef CONFIG_MCA
@@ -413,7 +419,8 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
* NMI, otherwise we may lose it, because the CPU-specific
* NMI can not be detected/processed on other CPUs.
*/
- if (notify_die(DIE_NMI, "nmi", regs, 0, 2, SIGINT) == NOTIFY_STOP)
+ if (notify_die(DIE_NMI, "nmi", regs, 0, INTR_NMI, SIGINT) ==
+ NOTIFY_STOP)
return;

/* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
@@ -469,22 +476,24 @@ void restart_nmi(void)
dotraplinkage void __kprobes do_int3(struct pt_regs *regs, long error_code)
{
#ifdef CONFIG_KGDB_LOW_LEVEL_TRAP
- if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, 3, SIGTRAP)
- == NOTIFY_STOP)
+ if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, INTR_BREAKPOINT,
+ SIGTRAP) == NOTIFY_STOP)
return;
#endif /* CONFIG_KGDB_LOW_LEVEL_TRAP */
#ifdef CONFIG_KPROBES
- if (notify_die(DIE_INT3, "int3", regs, error_code, 3, SIGTRAP)
+ if (notify_die(DIE_INT3, "int3", regs, error_code, INTR_BREAKPOINT,
+ SIGTRAP)
== NOTIFY_STOP)
return;
#else
- if (notify_die(DIE_TRAP, "int3", regs, error_code, 3, SIGTRAP)
+ if (notify_die(DIE_TRAP, "int3", regs, error_code, INTR_BREAKPOINT,
+ SIGTRAP)
== NOTIFY_STOP)
return;
#endif

preempt_conditional_sti(regs);
- do_trap(3, SIGTRAP, "int3", regs, error_code, NULL);
+ do_trap(INTR_BREAKPOINT, SIGTRAP, "int3", regs, error_code, NULL);
preempt_conditional_cli(regs);
}

@@ -583,7 +592,7 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)

if (regs->flags & X86_VM_MASK) {
handle_vm86_trap((struct kernel_vm86_regs *) regs,
- error_code, 1);
+ error_code, INTR_DEBUG);
preempt_conditional_cli(regs);
return;
}
@@ -611,14 +620,15 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
/*
* Note that we play around with the 'TS' bit in an attempt to get
* the correct behaviour even in the presence of the asynchronous
- * IRQ13 behaviour
+ * INTR_GPF behaviour
*/
void math_error(struct pt_regs *regs, int error_code, int trapnr)
{
struct task_struct *task = current;
siginfo_t info;
unsigned short err;
- char *str = (trapnr == 16) ? "fpu exception" : "simd exception";
+ char *str = (trapnr == INTR_COPROCESSOR) ? "fpu exception" :
+ "simd exception";

if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, SIGFPE) == NOTIFY_STOP)
return;
@@ -643,7 +653,7 @@ void math_error(struct pt_regs *regs, int error_code, int trapnr)
info.si_signo = SIGFPE;
info.si_errno = 0;
info.si_addr = (void __user *)regs->ip;
- if (trapnr == 16) {
+ if (trapnr == INTR_COPROCESSOR) {
unsigned short cwd, swd;
/*
* (~cwd & swd) will mask out exceptions that are not set to unmasked
@@ -687,8 +697,9 @@ void math_error(struct pt_regs *regs, int error_code, int trapnr)
info.si_code = FPE_FLTRES;
} else {
/*
- * If we're using IRQ 13, or supposedly even some trap 16
- * implementations, it's possible we get a spurious trap...
+ * If we're using INTR_GPF, or supposedly even some trap
+ * INTR_COPROCESSOR implementations, it's possible we get a
+ * spurious trap...
*/
return; /* Spurious trap, no error */
}
@@ -703,25 +714,25 @@ dotraplinkage void do_coprocessor_error(struct pt_regs *regs, long error_code)
ignore_fpu_irq = 1;
#endif

- math_error(regs, error_code, 16);
+ math_error(regs, error_code, INTR_COPROCESSOR);
}

dotraplinkage void
do_simd_coprocessor_error(struct pt_regs *regs, long error_code)
{
- math_error(regs, error_code, 19);
+ math_error(regs, error_code, INTR_SIMD_COPROCESSOR);
}

dotraplinkage void
do_spurious_interrupt_bug(struct pt_regs *regs, long error_code)
{
- trace_trap_entry(15);
+ trace_trap_entry(INTR_SPURIOUS);
conditional_sti(regs);
#if 0
/* No need to warn about this any longer. */
printk(KERN_INFO "Ignoring P6 Local APIC Spurious Interrupt Bug...\n");
#endif
- trace_trap_exit(15);
+ trace_trap_exit(INTR_SPURIOUS);
}

asmlinkage void __attribute__((weak)) smp_thermal_interrupt(void)
@@ -758,7 +769,7 @@ void __math_state_restore(void)
* 'math_state_restore()' saves the current math information in the
* old math state array, and gets the new ones from the current task
*
- * Careful.. There are problems with IBM-designed IRQ13 behaviour.
+ * Careful.. There are problems with IBM-designed INTR_GPF behaviour.
* Don't touch unless you *really* know how it works.
*
* Must be called with kernel preemption disabled (in this case,
@@ -793,7 +804,7 @@ EXPORT_SYMBOL_GPL(math_state_restore);
dotraplinkage void __kprobes
do_device_not_available(struct pt_regs *regs, long error_code)
{
- trace_trap_entry(7);
+ trace_trap_entry(INTR_NO_DEV);
#ifdef CONFIG_MATH_EMULATION
if (read_cr0() & X86_CR0_EM) {
struct math_emu_info info = { };
@@ -802,7 +813,7 @@ do_device_not_available(struct pt_regs *regs, long error_code)

info.regs = regs;
math_emulate(&info);
- trace_trap_exit(7);
+ trace_trap_exit(INTR_NO_DEV);
return;
}
#endif
@@ -810,7 +821,7 @@ do_device_not_available(struct pt_regs *regs, long error_code)
#ifdef CONFIG_X86_32
conditional_sti(regs);
#endif
- trace_trap_exit(7);
+ trace_trap_exit(INTR_NO_DEV);
}

#ifdef CONFIG_X86_32
@@ -833,10 +844,10 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
/* Set of traps needed for early debugging. */
void __init early_trap_init(void)
{
- set_intr_gate_ist(1, &debug, DEBUG_STACK);
+ set_intr_gate_ist(INTR_DEBUG, &debug, DEBUG_STACK);
/* int3 can be called from all */
- set_system_intr_gate_ist(3, &int3, DEBUG_STACK);
- set_intr_gate(14, &page_fault);
+ set_system_intr_gate_ist(INTR_BREAKPOINT, &int3, DEBUG_STACK);
+ set_intr_gate(INTR_PAGE_FAULT, &page_fault);
load_idt(&idt_descr);
}

@@ -852,30 +863,30 @@ void __init trap_init(void)
early_iounmap(p, 4);
#endif

- set_intr_gate(0, &divide_error);
- set_intr_gate_ist(2, &nmi, NMI_STACK);
+ set_intr_gate(INTR_DIV_BY_ZERO, &divide_error);
+ set_intr_gate_ist(INTR_NMI, &nmi, NMI_STACK);
/* int4 can be called from all */
- set_system_intr_gate(4, &overflow);
- set_intr_gate(5, &bounds);
- set_intr_gate(6, &invalid_op);
- set_intr_gate(7, &device_not_available);
+ set_system_intr_gate(INTR_OVERFLOW, &overflow);
+ set_intr_gate(INTR_BOUNDS_CHECK, &bounds);
+ set_intr_gate(INTR_INVALID_OP, &invalid_op);
+ set_intr_gate(INTR_NO_DEV, &device_not_available);
#ifdef CONFIG_X86_32
- set_task_gate(8, GDT_ENTRY_DOUBLEFAULT_TSS);
+ set_task_gate(INTR_DBL_FAULT, GDT_ENTRY_DOUBLEFAULT_TSS);
#else
- set_intr_gate_ist(8, &double_fault, DOUBLEFAULT_STACK);
+ set_intr_gate_ist(INTR_DBL_FAULT, &double_fault, DOUBLEFAULT_STACK);
#endif
- set_intr_gate(9, &coprocessor_segment_overrun);
- set_intr_gate(10, &invalid_TSS);
- set_intr_gate(11, &segment_not_present);
- set_intr_gate_ist(12, &stack_segment, STACKFAULT_STACK);
- set_intr_gate(13, &general_protection);
- set_intr_gate(15, &spurious_interrupt_bug);
- set_intr_gate(16, &coprocessor_error);
- set_intr_gate(17, &alignment_check);
+ set_intr_gate(INTR_SEG_OVERRUN, &coprocessor_segment_overrun);
+ set_intr_gate(INTR_INVALID_TSS, &invalid_TSS);
+ set_intr_gate(INTR_NO_SEG, &segment_not_present);
+ set_intr_gate_ist(INTR_STACK_FAULT, &stack_segment, STACKFAULT_STACK);
+ set_intr_gate(INTR_GPF, &general_protection);
+ set_intr_gate(INTR_SPURIOUS, &spurious_interrupt_bug);
+ set_intr_gate(INTR_COPROCESSOR, &coprocessor_error);
+ set_intr_gate(INTR_ALIGNMENT, &alignment_check);
#ifdef CONFIG_X86_MCE
- set_intr_gate_ist(18, &machine_check, MCE_STACK);
+ set_intr_gate_ist(INTR_MCE, &machine_check, MCE_STACK);
#endif
- set_intr_gate(19, &simd_coprocessor_error);
+ set_intr_gate(INTR_SIMD_COPROCESSOR, &simd_coprocessor_error);

/* Reserve all the builtin and the syscall vector: */
for (i = 0; i < FIRST_EXTERNAL_VECTOR; i++)
--
1.7.3.1

2011-04-25 23:40:01

by Vaibhav Nagarnaik

[permalink] [raw]
Subject: [PATCH 1/2] trace: Add trap entry/exit tracepoints

From: Jiaying Zhang <[email protected]>

For debugging and performance monitoring purpose, we often need to trace
trap entry and exit events. The following patch adds the event
definition for trap entry/exit and the instrumentation hooks for x86
platforms. Other platforms should be able to use these events as well
once they add the corresponding instrumentation.

$ echo 1 > debug/tracing/events/trap/enable
run gdb to genrate some trap events
$ cat debug/tracing/trace
<...>-13619 [003] 917.726602: trap_entry: number=3
<...>-13619 [003] 917.726612: trap_exit: number=3
<...>-13619 [003] 917.747263: trap_entry: number=1
<...>-13619 [003] 917.747272: trap_exit: number=1
<...>-13619 [003] 917.747567: trap_entry: number=3
<...>-13619 [003] 917.747570: trap_exit: number=3
<...>-13619 [003] 917.748101: trap_entry: number=1
<...>-13619 [003] 917.748103: trap_exit: number=1

$ echo 1 > tracing_enabled; ~/trap pagefault; echo 0 > tracing_enabled
$ cat trace | grep "trap-" | grep number=14 | head
trap-12528 [003] 1159.755792: trap_entry: number=14
trap-12528 [003] 1159.755801: trap_entry: number=14
trap-12528 [003] 1159.755804: trap_entry: number=14
trap-12528 [003] 1159.755807: trap_entry: number=14
trap-12528 [003] 1159.755810: trap_entry: number=14
trap-12528 [003] 1159.755817: trap_entry: number=14
trap-12528 [003] 1159.755819: trap_entry: number=14
trap-12528 [003] 1159.755821: trap_entry: number=14
trap-12528 [003] 1159.755824: trap_entry: number=14
trap-12528 [003] 1159.755826: trap_entry: number=14
$ cat trace | grep "trap-" | grep number=7 | head
trap-12528 [003] 1159.756283: trap_entry: number=7
trap-12529 [003] 1159.757427: trap_entry: number=7
trap-12530 [003] 1159.758277: trap_entry: number=7
trap-12531 [003] 1159.759172: trap_entry: number=7
trap-12532 [003] 1159.768643: trap_entry: number=7
trap-12533 [003] 1159.778195: trap_entry: number=7
trap-15026 [001] 1557.877722: trap_entry: number=7
trap-15253 [001] 1621.395067: trap_entry: number=7

Signed-off-by: Vaibhav Nagarnaik <[email protected]>
---
arch/x86/kernel/ptrace.c | 4 +++
arch/x86/kernel/traps.c | 16 +++++++++++++++
arch/x86/mm/fault.c | 13 ++++++++++-
include/trace/events/trap.h | 44 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 75 insertions(+), 2 deletions(-)
create mode 100644 include/trace/events/trap.h

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 45892dc..d788752 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -22,6 +22,8 @@
#include <linux/perf_event.h>
#include <linux/hw_breakpoint.h>

+#include <trace/events/trap.h>
+
#include <asm/uaccess.h>
#include <asm/pgtable.h>
#include <asm/system.h>
@@ -1330,8 +1332,10 @@ void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs,
struct siginfo info;

fill_sigtrap_info(tsk, regs, error_code, si_code, &info);
+ trace_trap_entry(tsk->thread.trap_no);
/* Send us the fake SIGTRAP */
force_sig_info(SIGTRAP, &info, tsk);
+ trace_trap_exit(tsk->thread.trap_no);
}


diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4857ff6..d450349 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -33,6 +33,9 @@
#include <linux/io.h>
#include <trace/events/irq.h>

+#define CREATE_TRACE_POINTS
+#include <trace/events/trap.h>
+
#ifdef CONFIG_EISA
#include <linux/ioport.h>
#include <linux/eisa.h>
@@ -123,6 +126,7 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
{
struct task_struct *tsk = current;

+ trace_trap_entry(trapnr);
#ifdef CONFIG_X86_32
if (regs->flags & X86_VM_MASK) {
/*
@@ -169,6 +173,7 @@ trap_signal:
force_sig_info(signr, info, tsk);
else
force_sig(signr, tsk);
+ trace_trap_exit(trapnr);
return;

kernel_trap:
@@ -177,6 +182,7 @@ kernel_trap:
tsk->thread.trap_no = trapnr;
die(str, regs, error_code);
}
+ trace_trap_exit(trapnr);
return;

#ifdef CONFIG_X86_32
@@ -184,6 +190,7 @@ vm86_trap:
if (handle_vm86_trap((struct kernel_vm86_regs *) regs,
error_code, trapnr))
goto trap_signal;
+ trace_trap_exit(trapnr);
return;
#endif
}
@@ -286,7 +293,9 @@ do_general_protection(struct pt_regs *regs, long error_code)
printk("\n");
}

+ trace_trap_entry(tsk->thread.trap_no);
force_sig(SIGSEGV, tsk);
+ trace_trap_exit(tsk->thread.trap_no);
return;

#ifdef CONFIG_X86_32
@@ -683,7 +692,9 @@ void math_error(struct pt_regs *regs, int error_code, int trapnr)
*/
return; /* Spurious trap, no error */
}
+ trace_trap_entry(task->thread.trap_no);
force_sig_info(SIGFPE, &info, task);
+ trace_trap_exit(task->thread.trap_no);
}

dotraplinkage void do_coprocessor_error(struct pt_regs *regs, long error_code)
@@ -704,11 +715,13 @@ do_simd_coprocessor_error(struct pt_regs *regs, long error_code)
dotraplinkage void
do_spurious_interrupt_bug(struct pt_regs *regs, long error_code)
{
+ trace_trap_entry(15);
conditional_sti(regs);
#if 0
/* No need to warn about this any longer. */
printk(KERN_INFO "Ignoring P6 Local APIC Spurious Interrupt Bug...\n");
#endif
+ trace_trap_exit(15);
}

asmlinkage void __attribute__((weak)) smp_thermal_interrupt(void)
@@ -780,6 +793,7 @@ EXPORT_SYMBOL_GPL(math_state_restore);
dotraplinkage void __kprobes
do_device_not_available(struct pt_regs *regs, long error_code)
{
+ trace_trap_entry(7);
#ifdef CONFIG_MATH_EMULATION
if (read_cr0() & X86_CR0_EM) {
struct math_emu_info info = { };
@@ -788,6 +802,7 @@ do_device_not_available(struct pt_regs *regs, long error_code)

info.regs = regs;
math_emulate(&info);
+ trace_trap_exit(7);
return;
}
#endif
@@ -795,6 +810,7 @@ do_device_not_available(struct pt_regs *regs, long error_code)
#ifdef CONFIG_X86_32
conditional_sti(regs);
#endif
+ trace_trap_exit(7);
}

#ifdef CONFIG_X86_32
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 20e3f87..55bdd31 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -11,6 +11,7 @@
#include <linux/kprobes.h> /* __kprobes, ... */
#include <linux/mmiotrace.h> /* kmmio_handler, ... */
#include <linux/perf_event.h> /* perf_sw_event */
+#include <trace/events/trap.h> /* trap trace events */
#include <linux/hugetlb.h> /* hstate_index_to_shift */

#include <asm/traps.h> /* dotraplinkage, ... */
@@ -955,8 +956,8 @@ static int fault_in_kernel_space(unsigned long address)
* and the problem, and then passes it off to one of the appropriate
* routines.
*/
-dotraplinkage void __kprobes
-do_page_fault(struct pt_regs *regs, unsigned long error_code)
+static __always_inline void
+__do_page_fault(struct pt_regs *regs, unsigned long error_code)
{
struct vm_area_struct *vma;
struct task_struct *tsk;
@@ -1164,3 +1165,11 @@ good_area:

up_read(&mm->mmap_sem);
}
+
+dotraplinkage void __kprobes
+do_page_fault(struct pt_regs *regs, unsigned long error_code)
+{
+ trace_trap_entry(14);
+ __do_page_fault(regs, error_code);
+ trace_trap_exit(14);
+}
diff --git a/include/trace/events/trap.h b/include/trace/events/trap.h
new file mode 100644
index 0000000..cb38eb6
--- /dev/null
+++ b/include/trace/events/trap.h
@@ -0,0 +1,44 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM trap
+
+#if !defined(_TRACE_TRAP_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_TRAP_H
+
+#include <linux/tracepoint.h>
+
+DECLARE_EVENT_CLASS(trap,
+
+ TP_PROTO(int id),
+
+ TP_ARGS(id),
+
+ TP_STRUCT__entry(
+ __field( int, id )
+ ),
+
+ TP_fast_assign(
+ __entry->id = id;
+ ),
+
+ TP_printk("number=%d", __entry->id)
+);
+
+DEFINE_EVENT(trap, trap_entry,
+
+ TP_PROTO(int id),
+
+ TP_ARGS(id)
+);
+
+DEFINE_EVENT(trap, trap_exit,
+
+ TP_PROTO(int id),
+
+ TP_ARGS(id)
+);
+
+#endif /* _TRACE_TRAP_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
+
--
1.7.3.1

2011-04-28 23:16:03

by Vaibhav Nagarnaik

[permalink] [raw]
Subject: Re: [PATCH 2/2] x86: Change trap definitions to enumerated values

All

Can you take a look at this patch and provide your thoughts?


Thanks
Vaibhav Nagarnaik



On Fri, Apr 22, 2011 at 4:03 PM, Vaibhav Nagarnaik
<[email protected]> wrote:
> From: Aditya Kali <[email protected]>
>
> The traps are referred to by their numbers and it can be difficult to
> understand them while reading the code without context. This patch adds
> enumeration of the trap numbers and replaced the numbers to the correct
> enum for x86.
>
> Signed-off-by: Vaibhav Nagarnaik <[email protected]>
> ---
> ?arch/x86/include/asm/traps.h | ? 24 ++++++++
> ?arch/x86/kernel/traps.c ? ? ?| ?129 +++++++++++++++++++++++-------------------
> ?2 files changed, 94 insertions(+), 59 deletions(-)
>
> diff --git a/arch/x86/include/asm/traps.h b/arch/x86/include/asm/traps.h
> index 0310da6..c561caf 100644
> --- a/arch/x86/include/asm/traps.h
> +++ b/arch/x86/include/asm/traps.h
> @@ -87,4 +87,28 @@ asmlinkage void smp_thermal_interrupt(void);
> ?asmlinkage void mce_threshold_interrupt(void);
> ?#endif
>
> +/* Interrupts/Exceptions */
> +enum {
> + ? ? ? INTR_DIV_BY_ZERO = 0, ? /* ?0 */
> + ? ? ? INTR_DEBUG, ? ? ? ? ? ? /* ?1 */
> + ? ? ? INTR_NMI, ? ? ? ? ? ? ? /* ?2 */
> + ? ? ? INTR_BREAKPOINT, ? ? ? ?/* ?3 */
> + ? ? ? INTR_OVERFLOW, ? ? ? ? ?/* ?4 */
> + ? ? ? INTR_BOUNDS_CHECK, ? ? ?/* ?5 */
> + ? ? ? INTR_INVALID_OP, ? ? ? ?/* ?6 */
> + ? ? ? INTR_NO_DEV, ? ? ? ? ? ?/* ?7 */
> + ? ? ? INTR_DBL_FAULT, ? ? ? ? /* ?8 */
> + ? ? ? INTR_SEG_OVERRUN, ? ? ? /* ?9 */
> + ? ? ? INTR_INVALID_TSS, ? ? ? /* 10 */
> + ? ? ? INTR_NO_SEG, ? ? ? ? ? ?/* 11 */
> + ? ? ? INTR_STACK_FAULT, ? ? ? /* 12 */
> + ? ? ? INTR_GPF, ? ? ? ? ? ? ? /* 13 */
> + ? ? ? INTR_PAGE_FAULT, ? ? ? ?/* 14 */
> + ? ? ? INTR_SPURIOUS, ? ? ? ? ?/* 15 */
> + ? ? ? INTR_COPROCESSOR, ? ? ? /* 16 */
> + ? ? ? INTR_ALIGNMENT, ? ? ? ? /* 17 */
> + ? ? ? INTR_MCE, ? ? ? ? ? ? ? /* 18 */
> + ? ? ? INTR_SIMD_COPROCESSOR ? /* 19 */
> +};
> +
> ?#endif /* _ASM_X86_TRAPS_H */
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index d450349..f6c5276 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -133,7 +133,7 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
> ? ? ? ? ? ? ? ? * traps 0, 1, 3, 4, and 5 should be forwarded to vm86.
> ? ? ? ? ? ? ? ? * On nmi (interrupt 2), do_trap should not be called.
> ? ? ? ? ? ? ? ? */
> - ? ? ? ? ? ? ? if (trapnr < 6)
> + ? ? ? ? ? ? ? if (trapnr < INTR_INVALID_OP)
> ? ? ? ? ? ? ? ? ? ? ? ?goto vm86_trap;
> ? ? ? ? ? ? ? ?goto trap_signal;
> ? ? ? ?}
> @@ -220,27 +220,32 @@ dotraplinkage void do_##name(struct pt_regs *regs, long error_code) ? ? ? \
> ? ? ? ?do_trap(trapnr, signr, str, regs, error_code, &info); ? ? ? ? ? \
> ?}
>
> -DO_ERROR_INFO(0, SIGFPE, "divide error", divide_error, FPE_INTDIV, regs->ip)
> -DO_ERROR(4, SIGSEGV, "overflow", overflow)
> -DO_ERROR(5, SIGSEGV, "bounds", bounds)
> -DO_ERROR_INFO(6, SIGILL, "invalid opcode", invalid_op, ILL_ILLOPN, regs->ip)
> -DO_ERROR(9, SIGFPE, "coprocessor segment overrun", coprocessor_segment_overrun)
> -DO_ERROR(10, SIGSEGV, "invalid TSS", invalid_TSS)
> -DO_ERROR(11, SIGBUS, "segment not present", segment_not_present)
> +DO_ERROR_INFO(INTR_DIV_BY_ZERO, SIGFPE, "divide error", divide_error,
> + ? ? ? ? ? ? ? FPE_INTDIV, regs->ip)
> +DO_ERROR(INTR_OVERFLOW, SIGSEGV, "overflow", overflow)
> +DO_ERROR(INTR_BOUNDS_CHECK, SIGSEGV, "bounds", bounds)
> +DO_ERROR_INFO(INTR_INVALID_OP, SIGILL, "invalid opcode", invalid_op,
> + ? ? ? ? ? ? ? ILL_ILLOPN, regs->ip)
> +DO_ERROR(INTR_SEG_OVERRUN, SIGFPE, "coprocessor segment overrun",
> + ? ? ? ? ? ? ? coprocessor_segment_overrun)
> +DO_ERROR(INTR_INVALID_TSS, SIGSEGV, "invalid TSS", invalid_TSS)
> +DO_ERROR(INTR_NO_SEG, SIGBUS, "segment not present", segment_not_present)
> ?#ifdef CONFIG_X86_32
> -DO_ERROR(12, SIGBUS, "stack segment", stack_segment)
> +DO_ERROR(INTR_STACK_FAULT, SIGBUS, "stack segment", stack_segment)
> ?#endif
> -DO_ERROR_INFO(17, SIGBUS, "alignment check", alignment_check, BUS_ADRALN, 0)
> +DO_ERROR_INFO(INTR_ALIGNMENT, SIGBUS, "alignment check", alignment_check,
> + ? ? ? ? ? ? ? BUS_ADRALN, 0)
>
> ?#ifdef CONFIG_X86_64
> ?/* Runs on IST stack */
> ?dotraplinkage void do_stack_segment(struct pt_regs *regs, long error_code)
> ?{
> ? ? ? ?if (notify_die(DIE_TRAP, "stack segment", regs, error_code,
> - ? ? ? ? ? ? ? ? ? ? ? 12, SIGBUS) == NOTIFY_STOP)
> + ? ? ? ? ? ? ? ? ? ? ? INTR_STACK_FAULT, SIGBUS) == NOTIFY_STOP)
> ? ? ? ? ? ? ? ?return;
> ? ? ? ?preempt_conditional_sti(regs);
> - ? ? ? do_trap(12, SIGBUS, "stack segment", regs, error_code, NULL);
> + ? ? ? do_trap(INTR_STACK_FAULT, SIGBUS, "stack segment", regs, error_code,
> + ? ? ? ? ? ? ? ? ? ? ? NULL);
> ? ? ? ?preempt_conditional_cli(regs);
> ?}
>
> @@ -250,10 +255,10 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
> ? ? ? ?struct task_struct *tsk = current;
>
> ? ? ? ?/* Return not checked because double check cannot be ignored */
> - ? ? ? notify_die(DIE_TRAP, str, regs, error_code, 8, SIGSEGV);
> + ? ? ? notify_die(DIE_TRAP, str, regs, error_code, INTR_DBL_FAULT, SIGSEGV);
>
> ? ? ? ?tsk->thread.error_code = error_code;
> - ? ? ? tsk->thread.trap_no = 8;
> + ? ? ? tsk->thread.trap_no = INTR_DBL_FAULT;
>
> ? ? ? ?/*
> ? ? ? ? * This is always a kernel trap and never fixable (and thus must
> @@ -281,7 +286,7 @@ do_general_protection(struct pt_regs *regs, long error_code)
> ? ? ? ? ? ? ? ?goto gp_in_kernel;
>
> ? ? ? ?tsk->thread.error_code = error_code;
> - ? ? ? tsk->thread.trap_no = 13;
> + ? ? ? tsk->thread.trap_no = INTR_GPF;
>
> ? ? ? ?if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
> ? ? ? ? ? ? ? ? ? ? ? ?printk_ratelimit()) {
> @@ -310,9 +315,9 @@ gp_in_kernel:
> ? ? ? ? ? ? ? ?return;
>
> ? ? ? ?tsk->thread.error_code = error_code;
> - ? ? ? tsk->thread.trap_no = 13;
> + ? ? ? tsk->thread.trap_no = INTR_GPF;
> ? ? ? ?if (notify_die(DIE_GPF, "general protection fault", regs,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? error_code, 13, SIGSEGV) == NOTIFY_STOP)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? error_code, INTR_GPF, SIGSEGV) == NOTIFY_STOP)
> ? ? ? ? ? ? ? ?return;
> ? ? ? ?die("general protection fault", regs, error_code);
> ?}
> @@ -381,7 +386,8 @@ io_check_error(unsigned char reason, struct pt_regs *regs)
> ?static notrace __kprobes void
> ?unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
> ?{
> - ? ? ? if (notify_die(DIE_NMIUNKNOWN, "nmi", regs, reason, 2, SIGINT) ==
> + ? ? ? if (notify_die(DIE_NMIUNKNOWN, "nmi", regs, reason, INTR_NMI,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? SIGINT) ==
> ? ? ? ? ? ? ? ? ? ? ? ?NOTIFY_STOP)
> ? ? ? ? ? ? ? ?return;
> ?#ifdef CONFIG_MCA
> @@ -413,7 +419,8 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
> ? ? ? ? * NMI, otherwise we may lose it, because the CPU-specific
> ? ? ? ? * NMI can not be detected/processed on other CPUs.
> ? ? ? ? */
> - ? ? ? if (notify_die(DIE_NMI, "nmi", regs, 0, 2, SIGINT) == NOTIFY_STOP)
> + ? ? ? if (notify_die(DIE_NMI, "nmi", regs, 0, INTR_NMI, SIGINT) ==
> + ? ? ? ? ? ? ? ? ? ? ? NOTIFY_STOP)
> ? ? ? ? ? ? ? ?return;
>
> ? ? ? ?/* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
> @@ -469,22 +476,24 @@ void restart_nmi(void)
> ?dotraplinkage void __kprobes do_int3(struct pt_regs *regs, long error_code)
> ?{
> ?#ifdef CONFIG_KGDB_LOW_LEVEL_TRAP
> - ? ? ? if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, 3, SIGTRAP)
> - ? ? ? ? ? ? ? ? ? ? ? == NOTIFY_STOP)
> + ? ? ? if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, INTR_BREAKPOINT,
> + ? ? ? ? ? ? ? ? ? ? ? SIGTRAP) == NOTIFY_STOP)
> ? ? ? ? ? ? ? ?return;
> ?#endif /* CONFIG_KGDB_LOW_LEVEL_TRAP */
> ?#ifdef CONFIG_KPROBES
> - ? ? ? if (notify_die(DIE_INT3, "int3", regs, error_code, 3, SIGTRAP)
> + ? ? ? if (notify_die(DIE_INT3, "int3", regs, error_code, INTR_BREAKPOINT,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? SIGTRAP)
> ? ? ? ? ? ? ? ? ? ? ? ?== NOTIFY_STOP)
> ? ? ? ? ? ? ? ?return;
> ?#else
> - ? ? ? if (notify_die(DIE_TRAP, "int3", regs, error_code, 3, SIGTRAP)
> + ? ? ? if (notify_die(DIE_TRAP, "int3", regs, error_code, INTR_BREAKPOINT,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? SIGTRAP)
> ? ? ? ? ? ? ? ? ? ? ? ?== NOTIFY_STOP)
> ? ? ? ? ? ? ? ?return;
> ?#endif
>
> ? ? ? ?preempt_conditional_sti(regs);
> - ? ? ? do_trap(3, SIGTRAP, "int3", regs, error_code, NULL);
> + ? ? ? do_trap(INTR_BREAKPOINT, SIGTRAP, "int3", regs, error_code, NULL);
> ? ? ? ?preempt_conditional_cli(regs);
> ?}
>
> @@ -583,7 +592,7 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
>
> ? ? ? ?if (regs->flags & X86_VM_MASK) {
> ? ? ? ? ? ? ? ?handle_vm86_trap((struct kernel_vm86_regs *) regs,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? error_code, 1);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? error_code, INTR_DEBUG);
> ? ? ? ? ? ? ? ?preempt_conditional_cli(regs);
> ? ? ? ? ? ? ? ?return;
> ? ? ? ?}
> @@ -611,14 +620,15 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
> ?/*
> ?* Note that we play around with the 'TS' bit in an attempt to get
> ?* the correct behaviour even in the presence of the asynchronous
> - * IRQ13 behaviour
> + * INTR_GPF behaviour
> ?*/
> ?void math_error(struct pt_regs *regs, int error_code, int trapnr)
> ?{
> ? ? ? ?struct task_struct *task = current;
> ? ? ? ?siginfo_t info;
> ? ? ? ?unsigned short err;
> - ? ? ? char *str = (trapnr == 16) ? "fpu exception" : "simd exception";
> + ? ? ? char *str = (trapnr == INTR_COPROCESSOR) ? "fpu exception" :
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "simd exception";
>
> ? ? ? ?if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, SIGFPE) == NOTIFY_STOP)
> ? ? ? ? ? ? ? ?return;
> @@ -643,7 +653,7 @@ void math_error(struct pt_regs *regs, int error_code, int trapnr)
> ? ? ? ?info.si_signo = SIGFPE;
> ? ? ? ?info.si_errno = 0;
> ? ? ? ?info.si_addr = (void __user *)regs->ip;
> - ? ? ? if (trapnr == 16) {
> + ? ? ? if (trapnr == INTR_COPROCESSOR) {
> ? ? ? ? ? ? ? ?unsigned short cwd, swd;
> ? ? ? ? ? ? ? ?/*
> ? ? ? ? ? ? ? ? * (~cwd & swd) will mask out exceptions that are not set to unmasked
> @@ -687,8 +697,9 @@ void math_error(struct pt_regs *regs, int error_code, int trapnr)
> ? ? ? ? ? ? ? ?info.si_code = FPE_FLTRES;
> ? ? ? ?} else {
> ? ? ? ? ? ? ? ?/*
> - ? ? ? ? ? ? ? ?* If we're using IRQ 13, or supposedly even some trap 16
> - ? ? ? ? ? ? ? ?* implementations, it's possible we get a spurious trap...
> + ? ? ? ? ? ? ? ?* If we're using INTR_GPF, or supposedly even some trap
> + ? ? ? ? ? ? ? ?* INTR_COPROCESSOR implementations, it's possible we get a
> + ? ? ? ? ? ? ? ?* spurious trap...
> ? ? ? ? ? ? ? ? */
> ? ? ? ? ? ? ? ?return; ? ? ? ? /* Spurious trap, no error */
> ? ? ? ?}
> @@ -703,25 +714,25 @@ dotraplinkage void do_coprocessor_error(struct pt_regs *regs, long error_code)
> ? ? ? ?ignore_fpu_irq = 1;
> ?#endif
>
> - ? ? ? math_error(regs, error_code, 16);
> + ? ? ? math_error(regs, error_code, INTR_COPROCESSOR);
> ?}
>
> ?dotraplinkage void
> ?do_simd_coprocessor_error(struct pt_regs *regs, long error_code)
> ?{
> - ? ? ? math_error(regs, error_code, 19);
> + ? ? ? math_error(regs, error_code, INTR_SIMD_COPROCESSOR);
> ?}
>
> ?dotraplinkage void
> ?do_spurious_interrupt_bug(struct pt_regs *regs, long error_code)
> ?{
> - ? ? ? trace_trap_entry(15);
> + ? ? ? trace_trap_entry(INTR_SPURIOUS);
> ? ? ? ?conditional_sti(regs);
> ?#if 0
> ? ? ? ?/* No need to warn about this any longer. */
> ? ? ? ?printk(KERN_INFO "Ignoring P6 Local APIC Spurious Interrupt Bug...\n");
> ?#endif
> - ? ? ? trace_trap_exit(15);
> + ? ? ? trace_trap_exit(INTR_SPURIOUS);
> ?}
>
> ?asmlinkage void __attribute__((weak)) smp_thermal_interrupt(void)
> @@ -758,7 +769,7 @@ void __math_state_restore(void)
> ?* 'math_state_restore()' saves the current math information in the
> ?* old math state array, and gets the new ones from the current task
> ?*
> - * Careful.. There are problems with IBM-designed IRQ13 behaviour.
> + * Careful.. There are problems with IBM-designed INTR_GPF behaviour.
> ?* Don't touch unless you *really* know how it works.
> ?*
> ?* Must be called with kernel preemption disabled (in this case,
> @@ -793,7 +804,7 @@ EXPORT_SYMBOL_GPL(math_state_restore);
> ?dotraplinkage void __kprobes
> ?do_device_not_available(struct pt_regs *regs, long error_code)
> ?{
> - ? ? ? trace_trap_entry(7);
> + ? ? ? trace_trap_entry(INTR_NO_DEV);
> ?#ifdef CONFIG_MATH_EMULATION
> ? ? ? ?if (read_cr0() & X86_CR0_EM) {
> ? ? ? ? ? ? ? ?struct math_emu_info info = { };
> @@ -802,7 +813,7 @@ do_device_not_available(struct pt_regs *regs, long error_code)
>
> ? ? ? ? ? ? ? ?info.regs = regs;
> ? ? ? ? ? ? ? ?math_emulate(&info);
> - ? ? ? ? ? ? ? trace_trap_exit(7);
> + ? ? ? ? ? ? ? trace_trap_exit(INTR_NO_DEV);
> ? ? ? ? ? ? ? ?return;
> ? ? ? ?}
> ?#endif
> @@ -810,7 +821,7 @@ do_device_not_available(struct pt_regs *regs, long error_code)
> ?#ifdef CONFIG_X86_32
> ? ? ? ?conditional_sti(regs);
> ?#endif
> - ? ? ? trace_trap_exit(7);
> + ? ? ? trace_trap_exit(INTR_NO_DEV);
> ?}
>
> ?#ifdef CONFIG_X86_32
> @@ -833,10 +844,10 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
> ?/* Set of traps needed for early debugging. */
> ?void __init early_trap_init(void)
> ?{
> - ? ? ? set_intr_gate_ist(1, &debug, DEBUG_STACK);
> + ? ? ? set_intr_gate_ist(INTR_DEBUG, &debug, DEBUG_STACK);
> ? ? ? ?/* int3 can be called from all */
> - ? ? ? set_system_intr_gate_ist(3, &int3, DEBUG_STACK);
> - ? ? ? set_intr_gate(14, &page_fault);
> + ? ? ? set_system_intr_gate_ist(INTR_BREAKPOINT, &int3, DEBUG_STACK);
> + ? ? ? set_intr_gate(INTR_PAGE_FAULT, &page_fault);
> ? ? ? ?load_idt(&idt_descr);
> ?}
>
> @@ -852,30 +863,30 @@ void __init trap_init(void)
> ? ? ? ?early_iounmap(p, 4);
> ?#endif
>
> - ? ? ? set_intr_gate(0, &divide_error);
> - ? ? ? set_intr_gate_ist(2, &nmi, NMI_STACK);
> + ? ? ? set_intr_gate(INTR_DIV_BY_ZERO, &divide_error);
> + ? ? ? set_intr_gate_ist(INTR_NMI, &nmi, NMI_STACK);
> ? ? ? ?/* int4 can be called from all */
> - ? ? ? set_system_intr_gate(4, &overflow);
> - ? ? ? set_intr_gate(5, &bounds);
> - ? ? ? set_intr_gate(6, &invalid_op);
> - ? ? ? set_intr_gate(7, &device_not_available);
> + ? ? ? set_system_intr_gate(INTR_OVERFLOW, &overflow);
> + ? ? ? set_intr_gate(INTR_BOUNDS_CHECK, &bounds);
> + ? ? ? set_intr_gate(INTR_INVALID_OP, &invalid_op);
> + ? ? ? set_intr_gate(INTR_NO_DEV, &device_not_available);
> ?#ifdef CONFIG_X86_32
> - ? ? ? set_task_gate(8, GDT_ENTRY_DOUBLEFAULT_TSS);
> + ? ? ? set_task_gate(INTR_DBL_FAULT, GDT_ENTRY_DOUBLEFAULT_TSS);
> ?#else
> - ? ? ? set_intr_gate_ist(8, &double_fault, DOUBLEFAULT_STACK);
> + ? ? ? set_intr_gate_ist(INTR_DBL_FAULT, &double_fault, DOUBLEFAULT_STACK);
> ?#endif
> - ? ? ? set_intr_gate(9, &coprocessor_segment_overrun);
> - ? ? ? set_intr_gate(10, &invalid_TSS);
> - ? ? ? set_intr_gate(11, &segment_not_present);
> - ? ? ? set_intr_gate_ist(12, &stack_segment, STACKFAULT_STACK);
> - ? ? ? set_intr_gate(13, &general_protection);
> - ? ? ? set_intr_gate(15, &spurious_interrupt_bug);
> - ? ? ? set_intr_gate(16, &coprocessor_error);
> - ? ? ? set_intr_gate(17, &alignment_check);
> + ? ? ? set_intr_gate(INTR_SEG_OVERRUN, &coprocessor_segment_overrun);
> + ? ? ? set_intr_gate(INTR_INVALID_TSS, &invalid_TSS);
> + ? ? ? set_intr_gate(INTR_NO_SEG, &segment_not_present);
> + ? ? ? set_intr_gate_ist(INTR_STACK_FAULT, &stack_segment, STACKFAULT_STACK);
> + ? ? ? set_intr_gate(INTR_GPF, &general_protection);
> + ? ? ? set_intr_gate(INTR_SPURIOUS, &spurious_interrupt_bug);
> + ? ? ? set_intr_gate(INTR_COPROCESSOR, &coprocessor_error);
> + ? ? ? set_intr_gate(INTR_ALIGNMENT, &alignment_check);
> ?#ifdef CONFIG_X86_MCE
> - ? ? ? set_intr_gate_ist(18, &machine_check, MCE_STACK);
> + ? ? ? set_intr_gate_ist(INTR_MCE, &machine_check, MCE_STACK);
> ?#endif
> - ? ? ? set_intr_gate(19, &simd_coprocessor_error);
> + ? ? ? set_intr_gate(INTR_SIMD_COPROCESSOR, &simd_coprocessor_error);
>
> ? ? ? ?/* Reserve all the builtin and the syscall vector: */
> ? ? ? ?for (i = 0; i < FIRST_EXTERNAL_VECTOR; i++)
> --
> 1.7.3.1
>
>

2011-04-28 23:36:34

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 1/2] trace: Add trap entry/exit tracepoints

On Mon, 2011-04-25 at 16:39 -0700, Vaibhav Nagarnaik wrote:
> From: Jiaying Zhang <[email protected]>
>
> For debugging and performance monitoring purpose, we often need to trace
> trap entry and exit events. The following patch adds the event
> definition for trap entry/exit and the instrumentation hooks for x86
> platforms. Other platforms should be able to use these events as well
> once they add the corresponding instrumentation.

I'm fine with this patch, but it requires Acks from Ingo or Thomas.


>
> $ echo 1 > debug/tracing/events/trap/enable
> run gdb to genrate some trap events
> $ cat debug/tracing/trace
> <...>-13619 [003] 917.726602: trap_entry: number=3
> <...>-13619 [003] 917.726612: trap_exit: number=3
> <...>-13619 [003] 917.747263: trap_entry: number=1
> <...>-13619 [003] 917.747272: trap_exit: number=1
> <...>-13619 [003] 917.747567: trap_entry: number=3
> <...>-13619 [003] 917.747570: trap_exit: number=3
> <...>-13619 [003] 917.748101: trap_entry: number=1
> <...>-13619 [003] 917.748103: trap_exit: number=1
>
> $ echo 1 > tracing_enabled; ~/trap pagefault; echo 0 > tracing_enabled
> $ cat trace | grep "trap-" | grep number=14 | head
> trap-12528 [003] 1159.755792: trap_entry: number=14
> trap-12528 [003] 1159.755801: trap_entry: number=14
> trap-12528 [003] 1159.755804: trap_entry: number=14
> trap-12528 [003] 1159.755807: trap_entry: number=14
> trap-12528 [003] 1159.755810: trap_entry: number=14
> trap-12528 [003] 1159.755817: trap_entry: number=14
> trap-12528 [003] 1159.755819: trap_entry: number=14
> trap-12528 [003] 1159.755821: trap_entry: number=14
> trap-12528 [003] 1159.755824: trap_entry: number=14
> trap-12528 [003] 1159.755826: trap_entry: number=14
> $ cat trace | grep "trap-" | grep number=7 | head
> trap-12528 [003] 1159.756283: trap_entry: number=7
> trap-12529 [003] 1159.757427: trap_entry: number=7
> trap-12530 [003] 1159.758277: trap_entry: number=7
> trap-12531 [003] 1159.759172: trap_entry: number=7
> trap-12532 [003] 1159.768643: trap_entry: number=7
> trap-12533 [003] 1159.778195: trap_entry: number=7
> trap-15026 [001] 1557.877722: trap_entry: number=7
> trap-15253 [001] 1621.395067: trap_entry: number=7
>
> Signed-off-by: Vaibhav Nagarnaik <[email protected]>


> +++ b/include/trace/events/trap.h

As this is placed in the generic code, I wonder if a
"trace_trap_entry(id)" is sufficient for all archs.

-- Steve

> @@ -0,0 +1,44 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM trap
> +
> +#if !defined(_TRACE_TRAP_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_TRAP_H
> +
> +#include <linux/tracepoint.h>
> +
> +DECLARE_EVENT_CLASS(trap,
> +
> + TP_PROTO(int id),
> +
> + TP_ARGS(id),
> +
> + TP_STRUCT__entry(
> + __field( int, id )
> + ),
> +
> + TP_fast_assign(
> + __entry->id = id;
> + ),
> +
> + TP_printk("number=%d", __entry->id)
> +);
> +
> +DEFINE_EVENT(trap, trap_entry,
> +
> + TP_PROTO(int id),
> +
> + TP_ARGS(id)
> +);
> +
> +DEFINE_EVENT(trap, trap_exit,
> +
> + TP_PROTO(int id),
> +
> + TP_ARGS(id)
> +);
> +
> +#endif /* _TRACE_TRAP_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> +

2011-04-28 23:40:41

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 2/2] x86: Change trap definitions to enumerated values

On Fri, 2011-04-22 at 16:03 -0700, Vaibhav Nagarnaik wrote:
> From: Aditya Kali <[email protected]>
>
> The traps are referred to by their numbers and it can be difficult to
> understand them while reading the code without context. This patch adds
> enumeration of the trap numbers and replaced the numbers to the correct
> enum for x86.

I actually like this patch, as I find floating numbers in code annoying.

>
> Signed-off-by: Vaibhav Nagarnaik <[email protected]>
> ---


> -DO_ERROR_INFO(0, SIGFPE, "divide error", divide_error, FPE_INTDIV, regs->ip)
> -DO_ERROR(4, SIGSEGV, "overflow", overflow)
> -DO_ERROR(5, SIGSEGV, "bounds", bounds)
> -DO_ERROR_INFO(6, SIGILL, "invalid opcode", invalid_op, ILL_ILLOPN, regs->ip)
> -DO_ERROR(9, SIGFPE, "coprocessor segment overrun", coprocessor_segment_overrun)
> -DO_ERROR(10, SIGSEGV, "invalid TSS", invalid_TSS)
> -DO_ERROR(11, SIGBUS, "segment not present", segment_not_present)
> +DO_ERROR_INFO(INTR_DIV_BY_ZERO, SIGFPE, "divide error", divide_error,
> + FPE_INTDIV, regs->ip)
> +DO_ERROR(INTR_OVERFLOW, SIGSEGV, "overflow", overflow)
> +DO_ERROR(INTR_BOUNDS_CHECK, SIGSEGV, "bounds", bounds)
> +DO_ERROR_INFO(INTR_INVALID_OP, SIGILL, "invalid opcode", invalid_op,
> + ILL_ILLOPN, regs->ip)
> +DO_ERROR(INTR_SEG_OVERRUN, SIGFPE, "coprocessor segment overrun",
> + coprocessor_segment_overrun)
> +DO_ERROR(INTR_INVALID_TSS, SIGSEGV, "invalid TSS", invalid_TSS)
> +DO_ERROR(INTR_NO_SEG, SIGBUS, "segment not present", segment_not_present)
> #ifdef CONFIG_X86_32
> -DO_ERROR(12, SIGBUS, "stack segment", stack_segment)
> +DO_ERROR(INTR_STACK_FAULT, SIGBUS, "stack segment", stack_segment)
> #endif
> -DO_ERROR_INFO(17, SIGBUS, "alignment check", alignment_check, BUS_ADRALN, 0)
> +DO_ERROR_INFO(INTR_ALIGNMENT, SIGBUS, "alignment check", alignment_check,
> + BUS_ADRALN, 0)

Could you format the above to make it look more tabular and easier to
read.

> #ifdef CONFIG_X86_32
> @@ -833,10 +844,10 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
> /* Set of traps needed for early debugging. */
> void __init early_trap_init(void)
> {
> - set_intr_gate_ist(1, &debug, DEBUG_STACK);
> + set_intr_gate_ist(INTR_DEBUG, &debug, DEBUG_STACK);
> /* int3 can be called from all */
> - set_system_intr_gate_ist(3, &int3, DEBUG_STACK);
> - set_intr_gate(14, &page_fault);
> + set_system_intr_gate_ist(INTR_BREAKPOINT, &int3, DEBUG_STACK);
> + set_intr_gate(INTR_PAGE_FAULT, &page_fault);
> load_idt(&idt_descr);
> }
>
> @@ -852,30 +863,30 @@ void __init trap_init(void)
> early_iounmap(p, 4);
> #endif
>
> - set_intr_gate(0, &divide_error);
> - set_intr_gate_ist(2, &nmi, NMI_STACK);
> + set_intr_gate(INTR_DIV_BY_ZERO, &divide_error);
> + set_intr_gate_ist(INTR_NMI, &nmi, NMI_STACK);
> /* int4 can be called from all */
> - set_system_intr_gate(4, &overflow);
> - set_intr_gate(5, &bounds);
> - set_intr_gate(6, &invalid_op);
> - set_intr_gate(7, &device_not_available);
> + set_system_intr_gate(INTR_OVERFLOW, &overflow);
> + set_intr_gate(INTR_BOUNDS_CHECK, &bounds);
> + set_intr_gate(INTR_INVALID_OP, &invalid_op);
> + set_intr_gate(INTR_NO_DEV, &device_not_available);
> #ifdef CONFIG_X86_32
> - set_task_gate(8, GDT_ENTRY_DOUBLEFAULT_TSS);
> + set_task_gate(INTR_DBL_FAULT, GDT_ENTRY_DOUBLEFAULT_TSS);
> #else
> - set_intr_gate_ist(8, &double_fault, DOUBLEFAULT_STACK);
> + set_intr_gate_ist(INTR_DBL_FAULT, &double_fault, DOUBLEFAULT_STACK);
> #endif
> - set_intr_gate(9, &coprocessor_segment_overrun);
> - set_intr_gate(10, &invalid_TSS);
> - set_intr_gate(11, &segment_not_present);
> - set_intr_gate_ist(12, &stack_segment, STACKFAULT_STACK);
> - set_intr_gate(13, &general_protection);
> - set_intr_gate(15, &spurious_interrupt_bug);
> - set_intr_gate(16, &coprocessor_error);
> - set_intr_gate(17, &alignment_check);
> + set_intr_gate(INTR_SEG_OVERRUN, &coprocessor_segment_overrun);
> + set_intr_gate(INTR_INVALID_TSS, &invalid_TSS);
> + set_intr_gate(INTR_NO_SEG, &segment_not_present);
> + set_intr_gate_ist(INTR_STACK_FAULT, &stack_segment, STACKFAULT_STACK);
> + set_intr_gate(INTR_GPF, &general_protection);
> + set_intr_gate(INTR_SPURIOUS, &spurious_interrupt_bug);
> + set_intr_gate(INTR_COPROCESSOR, &coprocessor_error);
> + set_intr_gate(INTR_ALIGNMENT, &alignment_check);
> #ifdef CONFIG_X86_MCE
> - set_intr_gate_ist(18, &machine_check, MCE_STACK);
> + set_intr_gate_ist(INTR_MCE, &machine_check, MCE_STACK);
> #endif
> - set_intr_gate(19, &simd_coprocessor_error);
> + set_intr_gate(INTR_SIMD_COPROCESSOR, &simd_coprocessor_error);

Could you format these too, maybe even ignore the 80 char limit if it
makes it look better.

The numbers were all one or two characters in length and kept the second
argument easy to read. Now with the larger delta in the difference of
characters, it puts a strain on ones eyes.

-- Steve

>
> /* Reserve all the builtin and the syscall vector: */
> for (i = 0; i < FIRST_EXTERNAL_VECTOR; i++)

2011-04-29 00:33:11

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH 1/2] trace: Add trap entry/exit tracepoints

On Fri, 22 Apr 2011, Vaibhav Nagarnaik wrote:
> #include <asm/uaccess.h>
> #include <asm/pgtable.h>
> #include <asm/system.h>
> @@ -1330,8 +1332,10 @@ void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs,
> struct siginfo info;
>
> fill_sigtrap_info(tsk, regs, error_code, si_code, &info);
> + trace_trap_entry(tsk->thread.trap_no);

What the heck? How is that a trap? The code is sending SIGTRAP not
entering a trap at all. What are you trying to measure ? The time it
takes to send SIGTRAP? So how is that useful as an extra event?

> /* Send us the fake SIGTRAP */
> force_sig_info(SIGTRAP, &info, tsk);
> + trace_trap_exit(tsk->thread.trap_no);
> }
>
>
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 4857ff6..d450349 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -33,6 +33,9 @@
> #include <linux/io.h>
> #include <trace/events/irq.h>
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/trap.h>
> +
> #ifdef CONFIG_EISA
> #include <linux/ioport.h>
> #include <linux/eisa.h>
> @@ -123,6 +126,7 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
> {
> struct task_struct *tsk = current;
>
> + trace_trap_entry(trapnr);

While the event of do_trap() itself might be interesting, it does not
matter at all how long it takes to handle it. That code is really not
so interesting.

> @@ -286,7 +293,9 @@ do_general_protection(struct pt_regs *regs, long error_code)
> printk("\n");
> }
>
> + trace_trap_entry(tsk->thread.trap_no);
> force_sig(SIGSEGV, tsk);
> + trace_trap_exit(tsk->thread.trap_no);

We really do not care how long the force_sig() call takes. It's
irrelevant. The only interesting thing here is that we ran into a GP
trap.

> +
> +dotraplinkage void __kprobes
> +do_page_fault(struct pt_regs *regs, unsigned long error_code)
> +{
> + trace_trap_entry(14);

Yuck. Magic number 14 ? Why not 42 ?

> + __do_page_fault(regs, error_code);
> + trace_trap_exit(14);
> +}

That page fault thing is the only interesting event in terms of
runtime, but I have yet to see a proper rationale for the whole thing
aside of that completly bogus changelog which tells what output I can
produce, but not why the hell it is a good idea to add all that trace
points.

Thanks,

tglx

2011-04-29 00:39:00

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH 2/2] x86: Change trap definitions to enumerated values

On Fri, 22 Apr 2011, Vaibhav Nagarnaik wrote:

> From: Aditya Kali <[email protected]>
>
> The traps are referred to by their numbers and it can be difficult to
> understand them while reading the code without context. This patch adds
> enumeration of the trap numbers and replaced the numbers to the correct
> enum for x86.

I really like that patch, but it should have been 1/2 as it has a
value independent of the trap trace stuff.

And if you had done that, then the trap trace patch could have used
those symbols instead of the numbers as well.

Now that's the 2/2 patch and leaves the (not so) shiny new trace
points with those weird numbers. :(

Thanks,

tglx

2011-04-30 00:53:07

by Vaibhav Nagarnaik

[permalink] [raw]
Subject: Re: [PATCH 1/2] trace: Add trap entry/exit tracepoints

On Thu, Apr 28, 2011 at 5:33 PM, Thomas Gleixner <[email protected]> wrote:
> On Fri, 22 Apr 2011, Vaibhav Nagarnaik wrote:
>> ?#include <asm/uaccess.h>
>> ?#include <asm/pgtable.h>
>> ?#include <asm/system.h>
>> @@ -1330,8 +1332,10 @@ void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs,
>> ? ? ? struct siginfo info;
>>
>> ? ? ? fill_sigtrap_info(tsk, regs, error_code, si_code, &info);
>> + ? ? trace_trap_entry(tsk->thread.trap_no);
>
> What the heck? How is that a trap? The code is sending SIGTRAP not
> entering a trap at all. What are you trying to measure ? The time it
> takes to send SIGTRAP? So how is that useful as an extra event?
>

It is not a trap. But it did get called from the do_debug trap handler. I have
moved these tracepoints to the do_debug function which is a better place for
them.

>> ? ? ? /* Send us the fake SIGTRAP */
>> ? ? ? force_sig_info(SIGTRAP, &info, tsk);
>> + ? ? trace_trap_exit(tsk->thread.trap_no);
>> ?}
>>
>>
>> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
>> index 4857ff6..d450349 100644
>> --- a/arch/x86/kernel/traps.c
>> +++ b/arch/x86/kernel/traps.c
>> @@ -33,6 +33,9 @@
>> ?#include <linux/io.h>
>> ?#include <trace/events/irq.h>
>>
>> +#define CREATE_TRACE_POINTS
>> +#include <trace/events/trap.h>
>> +
>> ?#ifdef CONFIG_EISA
>> ?#include <linux/ioport.h>
>> ?#include <linux/eisa.h>
>> @@ -123,6 +126,7 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
>> ?{
>> ? ? ? struct task_struct *tsk = current;
>>
>> + ? ? trace_trap_entry(trapnr);
>
> While the event of do_trap() itself might be interesting, it does not
> matter at all how long it takes to handle it. That code is really not
> so interesting.
>
I have updated the use case in the patch. We actually can find the instances
when the system moved between kernel and user spaces. The fact that we also
get timestamps is an added advantage.

>> @@ -286,7 +293,9 @@ do_general_protection(struct pt_regs *regs, long error_code)
>> ? ? ? ? ? ? ? printk("\n");
>> ? ? ? }
>>
>> + ? ? trace_trap_entry(tsk->thread.trap_no);
>> ? ? ? force_sig(SIGSEGV, tsk);
>> + ? ? trace_trap_exit(tsk->thread.trap_no);
>
> We really do not care how long the force_sig() call takes. It's
> irrelevant. The only interesting thing here is that we ran into a GP
> trap.
>
I agree, but it does provide the task which caused this and another data point
where the system had to move to kernel space.

>> +
>> +dotraplinkage void __kprobes
>> +do_page_fault(struct pt_regs *regs, unsigned long error_code)
>> +{
>> + ? ? trace_trap_entry(14);
>
> Yuck. Magic number 14 ? Why not 42 ?

Sorry for that. I thought that this patch would have lower resistance than the
next patch which converts the numbers to enum values. I have corrected it now
and am sending the enum patch first and use that as the base patch to use the
enum value (INTR_PAGE_FAULT) instead of magic number.

>
>> + ? ? __do_page_fault(regs, error_code);
>> + ? ? trace_trap_exit(14);
>> +}
>
> That page fault thing is the only interesting event in terms of
> runtime, but I have yet to see a proper rationale for the whole thing
> aside of that completly bogus changelog which tells what output I can
> produce, but not why the hell it is a good idea to add all that trace
> points.
>
I have updated the patch description. Hopefully it is better worded and
provides more context and support for the usefulness of it.

> Thanks,
>
> ? ? ? ?tglx
>
>

Thanks for looking at the patches.

Vaibhav Nagarnaik

2011-04-30 00:53:31

by Vaibhav Nagarnaik

[permalink] [raw]
Subject: [PATCH 2/2] trace: Add trap entry/exit tracepoints

From: Jiaying Zhang <[email protected]>

There are many ways that the system can move from user space to kernel
space and back. Of those, syscalls and interrupts are traced in ftrace.
Using traps is another way, but is not traced currently. Page faults,
general protection faults and others provide analysis of how the system
is behaving, who causes it and timing impact on the system performance
and running processes.

The following patch adds the event definition for trap entry/exit and
the instrumentation hooks for x86 platforms. Other platforms should be
able to use these events as well once they add the corresponding
instrumentation.

$ echo 1 > debug/tracing/events/trap/enable
run gdb to genrate some trap events
$ cat debug/tracing/trace
<...>-13619 [003] 917.726602: trap_entry: number=3
<...>-13619 [003] 917.726612: trap_exit: number=3
<...>-13619 [003] 917.747263: trap_entry: number=1
<...>-13619 [003] 917.747272: trap_exit: number=1
<...>-13619 [003] 917.747567: trap_entry: number=3
<...>-13619 [003] 917.747570: trap_exit: number=3
<...>-13619 [003] 917.748101: trap_entry: number=1
<...>-13619 [003] 917.748103: trap_exit: number=1

$ echo 1 > tracing_enabled; ~/trap pagefault; echo 0 > tracing_enabled
$ cat trace | grep "trap-" | grep number=14 | head
trap-12528 [003] 1159.755792: trap_entry: number=14
trap-12528 [003] 1159.755801: trap_entry: number=14
trap-12528 [003] 1159.755804: trap_entry: number=14
trap-12528 [003] 1159.755807: trap_entry: number=14
trap-12528 [003] 1159.755810: trap_entry: number=14
trap-12528 [003] 1159.755817: trap_entry: number=14
trap-12528 [003] 1159.755819: trap_entry: number=14
trap-12528 [003] 1159.755821: trap_entry: number=14
trap-12528 [003] 1159.755824: trap_entry: number=14
trap-12528 [003] 1159.755826: trap_entry: number=14
$ cat trace | grep "trap-" | grep number=7 | head
trap-12528 [003] 1159.756283: trap_entry: number=7
trap-12529 [003] 1159.757427: trap_entry: number=7
trap-12530 [003] 1159.758277: trap_entry: number=7
trap-12531 [003] 1159.759172: trap_entry: number=7
trap-12532 [003] 1159.768643: trap_entry: number=7
trap-12533 [003] 1159.778195: trap_entry: number=7
trap-15026 [001] 1557.877722: trap_entry: number=7
trap-15253 [001] 1621.395067: trap_entry: number=7

Signed-off-by: Vaibhav Nagarnaik <[email protected]>
---
arch/x86/kernel/ptrace.c | 2 +
arch/x86/kernel/traps.c | 14 +++++++++++++
arch/x86/mm/fault.c | 13 ++++++++++-
include/trace/events/trap.h | 44 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 71 insertions(+), 2 deletions(-)
create mode 100644 include/trace/events/trap.h

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 45892dc..e97cb2d 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -22,6 +22,8 @@
#include <linux/perf_event.h>
#include <linux/hw_breakpoint.h>

+#include <trace/events/trap.h>
+
#include <asm/uaccess.h>
#include <asm/pgtable.h>
#include <asm/system.h>
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index e058e8b..fbfd787 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -33,6 +33,9 @@
#include <linux/io.h>
#include <trace/events/irq.h>

+#define CREATE_TRACE_POINTS
+#include <trace/events/trap.h>
+
#ifdef CONFIG_EISA
#include <linux/ioport.h>
#include <linux/eisa.h>
@@ -123,6 +126,7 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
{
struct task_struct *tsk = current;

+ trace_trap_entry(trapnr);
#ifdef CONFIG_X86_32
if (regs->flags & X86_VM_MASK) {
/*
@@ -169,6 +173,7 @@ trap_signal:
force_sig_info(signr, info, tsk);
else
force_sig(signr, tsk);
+ trace_trap_exit(trapnr);
return;

kernel_trap:
@@ -177,6 +182,7 @@ kernel_trap:
tsk->thread.trap_no = trapnr;
die(str, regs, error_code);
}
+ trace_trap_exit(trapnr);
return;

#ifdef CONFIG_X86_32
@@ -184,6 +190,7 @@ vm86_trap:
if (handle_vm86_trap((struct kernel_vm86_regs *) regs,
error_code, trapnr))
goto trap_signal;
+ trace_trap_exit(trapnr);
return;
#endif
}
@@ -288,7 +295,9 @@ do_general_protection(struct pt_regs *regs, long error_code)
printk("\n");
}

+ trace_trap_entry(tsk->thread.trap_no);
force_sig(SIGSEGV, tsk);
+ trace_trap_exit(tsk->thread.trap_no);
return;

#ifdef CONFIG_X86_32
@@ -575,6 +584,7 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
SIGTRAP) == NOTIFY_STOP)
return;

+ trace_trap_entry(tsk->thread.trap_no);
/* It's safe to allow irq's after DR6 has been saved */
preempt_conditional_sti(regs);

@@ -582,6 +592,7 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
handle_vm86_trap((struct kernel_vm86_regs *) regs,
error_code, INTR_DEBUG);
preempt_conditional_cli(regs);
+ trace_trap_exit(tsk->thread.trap_no);
return;
}

@@ -602,6 +613,7 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
send_sigtrap(tsk, regs, error_code, si_code);
preempt_conditional_cli(regs);

+ trace_trap_exit(tsk->thread.trap_no);
return;
}

@@ -691,7 +703,9 @@ void math_error(struct pt_regs *regs, int error_code, int trapnr)
*/
return; /* Spurious trap, no error */
}
+ trace_trap_entry(task->thread.trap_no);
force_sig_info(SIGFPE, &info, task);
+ trace_trap_exit(task->thread.trap_no);
}

dotraplinkage void do_coprocessor_error(struct pt_regs *regs, long error_code)
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 20e3f87..507f5af 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -11,6 +11,7 @@
#include <linux/kprobes.h> /* __kprobes, ... */
#include <linux/mmiotrace.h> /* kmmio_handler, ... */
#include <linux/perf_event.h> /* perf_sw_event */
+#include <trace/events/trap.h> /* trap trace events */
#include <linux/hugetlb.h> /* hstate_index_to_shift */

#include <asm/traps.h> /* dotraplinkage, ... */
@@ -955,8 +956,8 @@ static int fault_in_kernel_space(unsigned long address)
* and the problem, and then passes it off to one of the appropriate
* routines.
*/
-dotraplinkage void __kprobes
-do_page_fault(struct pt_regs *regs, unsigned long error_code)
+static __always_inline void
+__do_page_fault(struct pt_regs *regs, unsigned long error_code)
{
struct vm_area_struct *vma;
struct task_struct *tsk;
@@ -1164,3 +1165,11 @@ good_area:

up_read(&mm->mmap_sem);
}
+
+dotraplinkage void __kprobes
+do_page_fault(struct pt_regs *regs, unsigned long error_code)
+{
+ trace_trap_entry(INTR_PAGE_FAULT);
+ __do_page_fault(regs, error_code);
+ trace_trap_exit(INTR_PAGE_FAULT);
+}
diff --git a/include/trace/events/trap.h b/include/trace/events/trap.h
new file mode 100644
index 0000000..cb38eb6
--- /dev/null
+++ b/include/trace/events/trap.h
@@ -0,0 +1,44 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM trap
+
+#if !defined(_TRACE_TRAP_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_TRAP_H
+
+#include <linux/tracepoint.h>
+
+DECLARE_EVENT_CLASS(trap,
+
+ TP_PROTO(int id),
+
+ TP_ARGS(id),
+
+ TP_STRUCT__entry(
+ __field( int, id )
+ ),
+
+ TP_fast_assign(
+ __entry->id = id;
+ ),
+
+ TP_printk("number=%d", __entry->id)
+);
+
+DEFINE_EVENT(trap, trap_entry,
+
+ TP_PROTO(int id),
+
+ TP_ARGS(id)
+);
+
+DEFINE_EVENT(trap, trap_exit,
+
+ TP_PROTO(int id),
+
+ TP_ARGS(id)
+);
+
+#endif /* _TRACE_TRAP_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
+
--
1.7.3.1

2011-04-30 00:53:33

by Vaibhav Nagarnaik

[permalink] [raw]
Subject: [PATCH 1/2] x86: Change trap definitions to enumerated values

From: Aditya Kali <[email protected]>

The traps are referred to by their numbers and it can be difficult to
understand them while reading the code without context. This patch adds
enumeration of the trap numbers and replaced the numbers to the correct
enum for x86.

Signed-off-by: Vaibhav Nagarnaik <[email protected]>
---
arch/x86/include/asm/traps.h | 24 ++++++++
arch/x86/kernel/traps.c | 121 +++++++++++++++++++++++-------------------
2 files changed, 91 insertions(+), 54 deletions(-)

diff --git a/arch/x86/include/asm/traps.h b/arch/x86/include/asm/traps.h
index 0310da6..c561caf 100644
--- a/arch/x86/include/asm/traps.h
+++ b/arch/x86/include/asm/traps.h
@@ -87,4 +87,28 @@ asmlinkage void smp_thermal_interrupt(void);
asmlinkage void mce_threshold_interrupt(void);
#endif

+/* Interrupts/Exceptions */
+enum {
+ INTR_DIV_BY_ZERO = 0, /* 0 */
+ INTR_DEBUG, /* 1 */
+ INTR_NMI, /* 2 */
+ INTR_BREAKPOINT, /* 3 */
+ INTR_OVERFLOW, /* 4 */
+ INTR_BOUNDS_CHECK, /* 5 */
+ INTR_INVALID_OP, /* 6 */
+ INTR_NO_DEV, /* 7 */
+ INTR_DBL_FAULT, /* 8 */
+ INTR_SEG_OVERRUN, /* 9 */
+ INTR_INVALID_TSS, /* 10 */
+ INTR_NO_SEG, /* 11 */
+ INTR_STACK_FAULT, /* 12 */
+ INTR_GPF, /* 13 */
+ INTR_PAGE_FAULT, /* 14 */
+ INTR_SPURIOUS, /* 15 */
+ INTR_COPROCESSOR, /* 16 */
+ INTR_ALIGNMENT, /* 17 */
+ INTR_MCE, /* 18 */
+ INTR_SIMD_COPROCESSOR /* 19 */
+};
+
#endif /* _ASM_X86_TRAPS_H */
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index fbfc162..e058e8b 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -129,7 +129,7 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
* traps 0, 1, 3, 4, and 5 should be forwarded to vm86.
* On nmi (interrupt 2), do_trap should not be called.
*/
- if (trapnr < 6)
+ if (trapnr < INTR_INVALID_OP)
goto vm86_trap;
goto trap_signal;
}
@@ -213,27 +213,29 @@ dotraplinkage void do_##name(struct pt_regs *regs, long error_code) \
do_trap(trapnr, signr, str, regs, error_code, &info); \
}

-DO_ERROR_INFO(0, SIGFPE, "divide error", divide_error, FPE_INTDIV, regs->ip)
-DO_ERROR(4, SIGSEGV, "overflow", overflow)
-DO_ERROR(5, SIGSEGV, "bounds", bounds)
-DO_ERROR_INFO(6, SIGILL, "invalid opcode", invalid_op, ILL_ILLOPN, regs->ip)
-DO_ERROR(9, SIGFPE, "coprocessor segment overrun", coprocessor_segment_overrun)
-DO_ERROR(10, SIGSEGV, "invalid TSS", invalid_TSS)
-DO_ERROR(11, SIGBUS, "segment not present", segment_not_present)
+DO_ERROR_INFO(INTR_DIV_BY_ZERO, SIGFPE, "divide error", divide_error, FPE_INTDIV, regs->ip)
+DO_ERROR_INFO(INTR_ALIGNMENT, SIGBUS, "alignment check", alignment_check, BUS_ADRALN, 0)
+DO_ERROR_INFO(INTR_INVALID_OP, SIGILL, "invalid opcode", invalid_op, ILL_ILLOPN, regs->ip)
+
+DO_ERROR(INTR_OVERFLOW, SIGSEGV, "overflow", overflow)
+DO_ERROR(INTR_BOUNDS_CHECK, SIGSEGV, "bounds", bounds)
+DO_ERROR(INTR_SEG_OVERRUN, SIGFPE, "coprocessor segment overrun", coprocessor_segment_overrun)
+DO_ERROR(INTR_INVALID_TSS, SIGSEGV, "invalid TSS", invalid_TSS)
+DO_ERROR(INTR_NO_SEG, SIGBUS, "segment not present", segment_not_present)
#ifdef CONFIG_X86_32
-DO_ERROR(12, SIGBUS, "stack segment", stack_segment)
+DO_ERROR(INTR_STACK_FAULT, SIGBUS, "stack segment", stack_segment)
#endif
-DO_ERROR_INFO(17, SIGBUS, "alignment check", alignment_check, BUS_ADRALN, 0)

#ifdef CONFIG_X86_64
/* Runs on IST stack */
dotraplinkage void do_stack_segment(struct pt_regs *regs, long error_code)
{
if (notify_die(DIE_TRAP, "stack segment", regs, error_code,
- 12, SIGBUS) == NOTIFY_STOP)
+ INTR_STACK_FAULT, SIGBUS) == NOTIFY_STOP)
return;
preempt_conditional_sti(regs);
- do_trap(12, SIGBUS, "stack segment", regs, error_code, NULL);
+ do_trap(INTR_STACK_FAULT, SIGBUS, "stack segment", regs, error_code,
+ NULL);
preempt_conditional_cli(regs);
}

@@ -243,10 +245,10 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
struct task_struct *tsk = current;

/* Return not checked because double check cannot be ignored */
- notify_die(DIE_TRAP, str, regs, error_code, 8, SIGSEGV);
+ notify_die(DIE_TRAP, str, regs, error_code, INTR_DBL_FAULT, SIGSEGV);

tsk->thread.error_code = error_code;
- tsk->thread.trap_no = 8;
+ tsk->thread.trap_no = INTR_DBL_FAULT;

/*
* This is always a kernel trap and never fixable (and thus must
@@ -274,7 +276,7 @@ do_general_protection(struct pt_regs *regs, long error_code)
goto gp_in_kernel;

tsk->thread.error_code = error_code;
- tsk->thread.trap_no = 13;
+ tsk->thread.trap_no = INTR_GPF;

if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
printk_ratelimit()) {
@@ -301,9 +303,9 @@ gp_in_kernel:
return;

tsk->thread.error_code = error_code;
- tsk->thread.trap_no = 13;
+ tsk->thread.trap_no = INTR_GPF;
if (notify_die(DIE_GPF, "general protection fault", regs,
- error_code, 13, SIGSEGV) == NOTIFY_STOP)
+ error_code, INTR_GPF, SIGSEGV) == NOTIFY_STOP)
return;
die("general protection fault", regs, error_code);
}
@@ -372,7 +374,8 @@ io_check_error(unsigned char reason, struct pt_regs *regs)
static notrace __kprobes void
unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
{
- if (notify_die(DIE_NMIUNKNOWN, "nmi", regs, reason, 2, SIGINT) ==
+ if (notify_die(DIE_NMIUNKNOWN, "nmi", regs, reason, INTR_NMI,
+ SIGINT) ==
NOTIFY_STOP)
return;
#ifdef CONFIG_MCA
@@ -404,7 +407,8 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
* NMI, otherwise we may lose it, because the CPU-specific
* NMI can not be detected/processed on other CPUs.
*/
- if (notify_die(DIE_NMI, "nmi", regs, 0, 2, SIGINT) == NOTIFY_STOP)
+ if (notify_die(DIE_NMI, "nmi", regs, 0, INTR_NMI, SIGINT) ==
+ NOTIFY_STOP)
return;

/* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
@@ -460,22 +464,24 @@ void restart_nmi(void)
dotraplinkage void __kprobes do_int3(struct pt_regs *regs, long error_code)
{
#ifdef CONFIG_KGDB_LOW_LEVEL_TRAP
- if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, 3, SIGTRAP)
- == NOTIFY_STOP)
+ if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, INTR_BREAKPOINT,
+ SIGTRAP) == NOTIFY_STOP)
return;
#endif /* CONFIG_KGDB_LOW_LEVEL_TRAP */
#ifdef CONFIG_KPROBES
- if (notify_die(DIE_INT3, "int3", regs, error_code, 3, SIGTRAP)
+ if (notify_die(DIE_INT3, "int3", regs, error_code, INTR_BREAKPOINT,
+ SIGTRAP)
== NOTIFY_STOP)
return;
#else
- if (notify_die(DIE_TRAP, "int3", regs, error_code, 3, SIGTRAP)
+ if (notify_die(DIE_TRAP, "int3", regs, error_code, INTR_BREAKPOINT,
+ SIGTRAP)
== NOTIFY_STOP)
return;
#endif

preempt_conditional_sti(regs);
- do_trap(3, SIGTRAP, "int3", regs, error_code, NULL);
+ do_trap(INTR_BREAKPOINT, SIGTRAP, "int3", regs, error_code, NULL);
preempt_conditional_cli(regs);
}

@@ -574,7 +580,7 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)

if (regs->flags & X86_VM_MASK) {
handle_vm86_trap((struct kernel_vm86_regs *) regs,
- error_code, 1);
+ error_code, INTR_DEBUG);
preempt_conditional_cli(regs);
return;
}
@@ -602,14 +608,15 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
/*
* Note that we play around with the 'TS' bit in an attempt to get
* the correct behaviour even in the presence of the asynchronous
- * IRQ13 behaviour
+ * INTR_GPF behaviour
*/
void math_error(struct pt_regs *regs, int error_code, int trapnr)
{
struct task_struct *task = current;
siginfo_t info;
unsigned short err;
- char *str = (trapnr == 16) ? "fpu exception" : "simd exception";
+ char *str = (trapnr == INTR_COPROCESSOR) ? "fpu exception" :
+ "simd exception";

if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, SIGFPE) == NOTIFY_STOP)
return;
@@ -634,7 +641,7 @@ void math_error(struct pt_regs *regs, int error_code, int trapnr)
info.si_signo = SIGFPE;
info.si_errno = 0;
info.si_addr = (void __user *)regs->ip;
- if (trapnr == 16) {
+ if (trapnr == INTR_COPROCESSOR) {
unsigned short cwd, swd;
/*
* (~cwd & swd) will mask out exceptions that are not set to unmasked
@@ -678,8 +685,9 @@ void math_error(struct pt_regs *regs, int error_code, int trapnr)
info.si_code = FPE_FLTRES;
} else {
/*
- * If we're using IRQ 13, or supposedly even some trap 16
- * implementations, it's possible we get a spurious trap...
+ * If we're using INTR_GPF, or supposedly even some trap
+ * INTR_COPROCESSOR implementations, it's possible we get a
+ * spurious trap...
*/
return; /* Spurious trap, no error */
}
@@ -692,23 +700,25 @@ dotraplinkage void do_coprocessor_error(struct pt_regs *regs, long error_code)
ignore_fpu_irq = 1;
#endif

- math_error(regs, error_code, 16);
+ math_error(regs, error_code, INTR_COPROCESSOR);
}

dotraplinkage void
do_simd_coprocessor_error(struct pt_regs *regs, long error_code)
{
- math_error(regs, error_code, 19);
+ math_error(regs, error_code, INTR_SIMD_COPROCESSOR);
}

dotraplinkage void
do_spurious_interrupt_bug(struct pt_regs *regs, long error_code)
{
+ trace_trap_entry(INTR_SPURIOUS);
conditional_sti(regs);
#if 0
/* No need to warn about this any longer. */
printk(KERN_INFO "Ignoring P6 Local APIC Spurious Interrupt Bug...\n");
#endif
+ trace_trap_exit(INTR_SPURIOUS);
}

asmlinkage void __attribute__((weak)) smp_thermal_interrupt(void)
@@ -745,7 +755,7 @@ void __math_state_restore(void)
* 'math_state_restore()' saves the current math information in the
* old math state array, and gets the new ones from the current task
*
- * Careful.. There are problems with IBM-designed IRQ13 behaviour.
+ * Careful.. There are problems with IBM-designed INTR_GPF behaviour.
* Don't touch unless you *really* know how it works.
*
* Must be called with kernel preemption disabled (in this case,
@@ -780,6 +790,7 @@ EXPORT_SYMBOL_GPL(math_state_restore);
dotraplinkage void __kprobes
do_device_not_available(struct pt_regs *regs, long error_code)
{
+ trace_trap_entry(INTR_NO_DEV);
#ifdef CONFIG_MATH_EMULATION
if (read_cr0() & X86_CR0_EM) {
struct math_emu_info info = { };
@@ -788,6 +799,7 @@ do_device_not_available(struct pt_regs *regs, long error_code)

info.regs = regs;
math_emulate(&info);
+ trace_trap_exit(INTR_NO_DEV);
return;
}
#endif
@@ -795,6 +807,7 @@ do_device_not_available(struct pt_regs *regs, long error_code)
#ifdef CONFIG_X86_32
conditional_sti(regs);
#endif
+ trace_trap_exit(INTR_NO_DEV);
}

#ifdef CONFIG_X86_32
@@ -817,10 +830,10 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
/* Set of traps needed for early debugging. */
void __init early_trap_init(void)
{
- set_intr_gate_ist(1, &debug, DEBUG_STACK);
+ set_intr_gate_ist(INTR_DEBUG, &debug, DEBUG_STACK);
/* int3 can be called from all */
- set_system_intr_gate_ist(3, &int3, DEBUG_STACK);
- set_intr_gate(14, &page_fault);
+ set_system_intr_gate_ist(INTR_BREAKPOINT, &int3, DEBUG_STACK);
+ set_intr_gate(INTR_PAGE_FAULT, &page_fault);
load_idt(&idt_descr);
}

@@ -836,30 +849,30 @@ void __init trap_init(void)
early_iounmap(p, 4);
#endif

- set_intr_gate(0, &divide_error);
- set_intr_gate_ist(2, &nmi, NMI_STACK);
+ set_intr_gate(INTR_DIV_BY_ZERO, &divide_error);
+ set_intr_gate_ist(INTR_NMI, &nmi, NMI_STACK);
/* int4 can be called from all */
- set_system_intr_gate(4, &overflow);
- set_intr_gate(5, &bounds);
- set_intr_gate(6, &invalid_op);
- set_intr_gate(7, &device_not_available);
+ set_system_intr_gate(INTR_OVERFLOW, &overflow);
+ set_intr_gate(INTR_BOUNDS_CHECK, &bounds);
+ set_intr_gate(INTR_INVALID_OP, &invalid_op);
+ set_intr_gate(INTR_NO_DEV, &device_not_available);
#ifdef CONFIG_X86_32
- set_task_gate(8, GDT_ENTRY_DOUBLEFAULT_TSS);
+ set_task_gate(INTR_DBL_FAULT, GDT_ENTRY_DOUBLEFAULT_TSS);
#else
- set_intr_gate_ist(8, &double_fault, DOUBLEFAULT_STACK);
+ set_intr_gate_ist(INTR_DBL_FAULT, &double_fault, DOUBLEFAULT_STACK);
#endif
- set_intr_gate(9, &coprocessor_segment_overrun);
- set_intr_gate(10, &invalid_TSS);
- set_intr_gate(11, &segment_not_present);
- set_intr_gate_ist(12, &stack_segment, STACKFAULT_STACK);
- set_intr_gate(13, &general_protection);
- set_intr_gate(15, &spurious_interrupt_bug);
- set_intr_gate(16, &coprocessor_error);
- set_intr_gate(17, &alignment_check);
+ set_intr_gate(INTR_SEG_OVERRUN, &coprocessor_segment_overrun);
+ set_intr_gate(INTR_INVALID_TSS, &invalid_TSS);
+ set_intr_gate(INTR_NO_SEG, &segment_not_present);
+ set_intr_gate_ist(INTR_STACK_FAULT, &stack_segment, STACKFAULT_STACK);
+ set_intr_gate(INTR_GPF, &general_protection);
+ set_intr_gate(INTR_SPURIOUS, &spurious_interrupt_bug);
+ set_intr_gate(INTR_COPROCESSOR, &coprocessor_error);
+ set_intr_gate(INTR_ALIGNMENT, &alignment_check);
#ifdef CONFIG_X86_MCE
- set_intr_gate_ist(18, &machine_check, MCE_STACK);
+ set_intr_gate_ist(INTR_MCE, &machine_check, MCE_STACK);
#endif
- set_intr_gate(19, &simd_coprocessor_error);
+ set_intr_gate(INTR_SIMD_COPROCESSOR, &simd_coprocessor_error);

/* Reserve all the builtin and the syscall vector: */
for (i = 0; i < FIRST_EXTERNAL_VECTOR; i++)
--
1.7.3.1

2011-05-03 18:19:13

by Don Zickus

[permalink] [raw]
Subject: Re: [PATCH 1/2] x86: Change trap definitions to enumerated values

On Fri, Apr 29, 2011 at 05:52:52PM -0700, Vaibhav Nagarnaik wrote:
> From: Aditya Kali <[email protected]>
>
> The traps are referred to by their numbers and it can be difficult to
> understand them while reading the code without context. This patch adds
> enumeration of the trap numbers and replaced the numbers to the correct
> enum for x86.

My only problem with this patch is you took a straight forward conversion
which didn't really change the functionality and sprinkled some trace
points in there.

I am ok with the conversion to something readable, but I don't know enough
about the trace point to know if those are correct spots.

Cheers,
Don

>
> Signed-off-by: Vaibhav Nagarnaik <[email protected]>
> ---
> arch/x86/include/asm/traps.h | 24 ++++++++
> arch/x86/kernel/traps.c | 121 +++++++++++++++++++++++-------------------
> 2 files changed, 91 insertions(+), 54 deletions(-)
>
> diff --git a/arch/x86/include/asm/traps.h b/arch/x86/include/asm/traps.h
> index 0310da6..c561caf 100644
> --- a/arch/x86/include/asm/traps.h
> +++ b/arch/x86/include/asm/traps.h
> @@ -87,4 +87,28 @@ asmlinkage void smp_thermal_interrupt(void);
> asmlinkage void mce_threshold_interrupt(void);
> #endif
>
> +/* Interrupts/Exceptions */
> +enum {
> + INTR_DIV_BY_ZERO = 0, /* 0 */
> + INTR_DEBUG, /* 1 */
> + INTR_NMI, /* 2 */
> + INTR_BREAKPOINT, /* 3 */
> + INTR_OVERFLOW, /* 4 */
> + INTR_BOUNDS_CHECK, /* 5 */
> + INTR_INVALID_OP, /* 6 */
> + INTR_NO_DEV, /* 7 */
> + INTR_DBL_FAULT, /* 8 */
> + INTR_SEG_OVERRUN, /* 9 */
> + INTR_INVALID_TSS, /* 10 */
> + INTR_NO_SEG, /* 11 */
> + INTR_STACK_FAULT, /* 12 */
> + INTR_GPF, /* 13 */
> + INTR_PAGE_FAULT, /* 14 */
> + INTR_SPURIOUS, /* 15 */
> + INTR_COPROCESSOR, /* 16 */
> + INTR_ALIGNMENT, /* 17 */
> + INTR_MCE, /* 18 */
> + INTR_SIMD_COPROCESSOR /* 19 */
> +};
> +
> #endif /* _ASM_X86_TRAPS_H */
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index fbfc162..e058e8b 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -129,7 +129,7 @@ do_trap(int trapnr, int signr, char *str, struct pt_regs *regs,
> * traps 0, 1, 3, 4, and 5 should be forwarded to vm86.
> * On nmi (interrupt 2), do_trap should not be called.
> */
> - if (trapnr < 6)
> + if (trapnr < INTR_INVALID_OP)
> goto vm86_trap;
> goto trap_signal;
> }
> @@ -213,27 +213,29 @@ dotraplinkage void do_##name(struct pt_regs *regs, long error_code) \
> do_trap(trapnr, signr, str, regs, error_code, &info); \
> }
>
> -DO_ERROR_INFO(0, SIGFPE, "divide error", divide_error, FPE_INTDIV, regs->ip)
> -DO_ERROR(4, SIGSEGV, "overflow", overflow)
> -DO_ERROR(5, SIGSEGV, "bounds", bounds)
> -DO_ERROR_INFO(6, SIGILL, "invalid opcode", invalid_op, ILL_ILLOPN, regs->ip)
> -DO_ERROR(9, SIGFPE, "coprocessor segment overrun", coprocessor_segment_overrun)
> -DO_ERROR(10, SIGSEGV, "invalid TSS", invalid_TSS)
> -DO_ERROR(11, SIGBUS, "segment not present", segment_not_present)
> +DO_ERROR_INFO(INTR_DIV_BY_ZERO, SIGFPE, "divide error", divide_error, FPE_INTDIV, regs->ip)
> +DO_ERROR_INFO(INTR_ALIGNMENT, SIGBUS, "alignment check", alignment_check, BUS_ADRALN, 0)
> +DO_ERROR_INFO(INTR_INVALID_OP, SIGILL, "invalid opcode", invalid_op, ILL_ILLOPN, regs->ip)
> +
> +DO_ERROR(INTR_OVERFLOW, SIGSEGV, "overflow", overflow)
> +DO_ERROR(INTR_BOUNDS_CHECK, SIGSEGV, "bounds", bounds)
> +DO_ERROR(INTR_SEG_OVERRUN, SIGFPE, "coprocessor segment overrun", coprocessor_segment_overrun)
> +DO_ERROR(INTR_INVALID_TSS, SIGSEGV, "invalid TSS", invalid_TSS)
> +DO_ERROR(INTR_NO_SEG, SIGBUS, "segment not present", segment_not_present)
> #ifdef CONFIG_X86_32
> -DO_ERROR(12, SIGBUS, "stack segment", stack_segment)
> +DO_ERROR(INTR_STACK_FAULT, SIGBUS, "stack segment", stack_segment)
> #endif
> -DO_ERROR_INFO(17, SIGBUS, "alignment check", alignment_check, BUS_ADRALN, 0)
>
> #ifdef CONFIG_X86_64
> /* Runs on IST stack */
> dotraplinkage void do_stack_segment(struct pt_regs *regs, long error_code)
> {
> if (notify_die(DIE_TRAP, "stack segment", regs, error_code,
> - 12, SIGBUS) == NOTIFY_STOP)
> + INTR_STACK_FAULT, SIGBUS) == NOTIFY_STOP)
> return;
> preempt_conditional_sti(regs);
> - do_trap(12, SIGBUS, "stack segment", regs, error_code, NULL);
> + do_trap(INTR_STACK_FAULT, SIGBUS, "stack segment", regs, error_code,
> + NULL);
> preempt_conditional_cli(regs);
> }
>
> @@ -243,10 +245,10 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
> struct task_struct *tsk = current;
>
> /* Return not checked because double check cannot be ignored */
> - notify_die(DIE_TRAP, str, regs, error_code, 8, SIGSEGV);
> + notify_die(DIE_TRAP, str, regs, error_code, INTR_DBL_FAULT, SIGSEGV);
>
> tsk->thread.error_code = error_code;
> - tsk->thread.trap_no = 8;
> + tsk->thread.trap_no = INTR_DBL_FAULT;
>
> /*
> * This is always a kernel trap and never fixable (and thus must
> @@ -274,7 +276,7 @@ do_general_protection(struct pt_regs *regs, long error_code)
> goto gp_in_kernel;
>
> tsk->thread.error_code = error_code;
> - tsk->thread.trap_no = 13;
> + tsk->thread.trap_no = INTR_GPF;
>
> if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
> printk_ratelimit()) {
> @@ -301,9 +303,9 @@ gp_in_kernel:
> return;
>
> tsk->thread.error_code = error_code;
> - tsk->thread.trap_no = 13;
> + tsk->thread.trap_no = INTR_GPF;
> if (notify_die(DIE_GPF, "general protection fault", regs,
> - error_code, 13, SIGSEGV) == NOTIFY_STOP)
> + error_code, INTR_GPF, SIGSEGV) == NOTIFY_STOP)
> return;
> die("general protection fault", regs, error_code);
> }
> @@ -372,7 +374,8 @@ io_check_error(unsigned char reason, struct pt_regs *regs)
> static notrace __kprobes void
> unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
> {
> - if (notify_die(DIE_NMIUNKNOWN, "nmi", regs, reason, 2, SIGINT) ==
> + if (notify_die(DIE_NMIUNKNOWN, "nmi", regs, reason, INTR_NMI,
> + SIGINT) ==
> NOTIFY_STOP)
> return;
> #ifdef CONFIG_MCA
> @@ -404,7 +407,8 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
> * NMI, otherwise we may lose it, because the CPU-specific
> * NMI can not be detected/processed on other CPUs.
> */
> - if (notify_die(DIE_NMI, "nmi", regs, 0, 2, SIGINT) == NOTIFY_STOP)
> + if (notify_die(DIE_NMI, "nmi", regs, 0, INTR_NMI, SIGINT) ==
> + NOTIFY_STOP)
> return;
>
> /* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
> @@ -460,22 +464,24 @@ void restart_nmi(void)
> dotraplinkage void __kprobes do_int3(struct pt_regs *regs, long error_code)
> {
> #ifdef CONFIG_KGDB_LOW_LEVEL_TRAP
> - if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, 3, SIGTRAP)
> - == NOTIFY_STOP)
> + if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, INTR_BREAKPOINT,
> + SIGTRAP) == NOTIFY_STOP)
> return;
> #endif /* CONFIG_KGDB_LOW_LEVEL_TRAP */
> #ifdef CONFIG_KPROBES
> - if (notify_die(DIE_INT3, "int3", regs, error_code, 3, SIGTRAP)
> + if (notify_die(DIE_INT3, "int3", regs, error_code, INTR_BREAKPOINT,
> + SIGTRAP)
> == NOTIFY_STOP)
> return;
> #else
> - if (notify_die(DIE_TRAP, "int3", regs, error_code, 3, SIGTRAP)
> + if (notify_die(DIE_TRAP, "int3", regs, error_code, INTR_BREAKPOINT,
> + SIGTRAP)
> == NOTIFY_STOP)
> return;
> #endif
>
> preempt_conditional_sti(regs);
> - do_trap(3, SIGTRAP, "int3", regs, error_code, NULL);
> + do_trap(INTR_BREAKPOINT, SIGTRAP, "int3", regs, error_code, NULL);
> preempt_conditional_cli(regs);
> }
>
> @@ -574,7 +580,7 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
>
> if (regs->flags & X86_VM_MASK) {
> handle_vm86_trap((struct kernel_vm86_regs *) regs,
> - error_code, 1);
> + error_code, INTR_DEBUG);
> preempt_conditional_cli(regs);
> return;
> }
> @@ -602,14 +608,15 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
> /*
> * Note that we play around with the 'TS' bit in an attempt to get
> * the correct behaviour even in the presence of the asynchronous
> - * IRQ13 behaviour
> + * INTR_GPF behaviour
> */
> void math_error(struct pt_regs *regs, int error_code, int trapnr)
> {
> struct task_struct *task = current;
> siginfo_t info;
> unsigned short err;
> - char *str = (trapnr == 16) ? "fpu exception" : "simd exception";
> + char *str = (trapnr == INTR_COPROCESSOR) ? "fpu exception" :
> + "simd exception";
>
> if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, SIGFPE) == NOTIFY_STOP)
> return;
> @@ -634,7 +641,7 @@ void math_error(struct pt_regs *regs, int error_code, int trapnr)
> info.si_signo = SIGFPE;
> info.si_errno = 0;
> info.si_addr = (void __user *)regs->ip;
> - if (trapnr == 16) {
> + if (trapnr == INTR_COPROCESSOR) {
> unsigned short cwd, swd;
> /*
> * (~cwd & swd) will mask out exceptions that are not set to unmasked
> @@ -678,8 +685,9 @@ void math_error(struct pt_regs *regs, int error_code, int trapnr)
> info.si_code = FPE_FLTRES;
> } else {
> /*
> - * If we're using IRQ 13, or supposedly even some trap 16
> - * implementations, it's possible we get a spurious trap...
> + * If we're using INTR_GPF, or supposedly even some trap
> + * INTR_COPROCESSOR implementations, it's possible we get a
> + * spurious trap...
> */
> return; /* Spurious trap, no error */
> }
> @@ -692,23 +700,25 @@ dotraplinkage void do_coprocessor_error(struct pt_regs *regs, long error_code)
> ignore_fpu_irq = 1;
> #endif
>
> - math_error(regs, error_code, 16);
> + math_error(regs, error_code, INTR_COPROCESSOR);
> }
>
> dotraplinkage void
> do_simd_coprocessor_error(struct pt_regs *regs, long error_code)
> {
> - math_error(regs, error_code, 19);
> + math_error(regs, error_code, INTR_SIMD_COPROCESSOR);
> }
>
> dotraplinkage void
> do_spurious_interrupt_bug(struct pt_regs *regs, long error_code)
> {
> + trace_trap_entry(INTR_SPURIOUS);
> conditional_sti(regs);
> #if 0
> /* No need to warn about this any longer. */
> printk(KERN_INFO "Ignoring P6 Local APIC Spurious Interrupt Bug...\n");
> #endif
> + trace_trap_exit(INTR_SPURIOUS);
> }
>
> asmlinkage void __attribute__((weak)) smp_thermal_interrupt(void)
> @@ -745,7 +755,7 @@ void __math_state_restore(void)
> * 'math_state_restore()' saves the current math information in the
> * old math state array, and gets the new ones from the current task
> *
> - * Careful.. There are problems with IBM-designed IRQ13 behaviour.
> + * Careful.. There are problems with IBM-designed INTR_GPF behaviour.
> * Don't touch unless you *really* know how it works.
> *
> * Must be called with kernel preemption disabled (in this case,
> @@ -780,6 +790,7 @@ EXPORT_SYMBOL_GPL(math_state_restore);
> dotraplinkage void __kprobes
> do_device_not_available(struct pt_regs *regs, long error_code)
> {
> + trace_trap_entry(INTR_NO_DEV);
> #ifdef CONFIG_MATH_EMULATION
> if (read_cr0() & X86_CR0_EM) {
> struct math_emu_info info = { };
> @@ -788,6 +799,7 @@ do_device_not_available(struct pt_regs *regs, long error_code)
>
> info.regs = regs;
> math_emulate(&info);
> + trace_trap_exit(INTR_NO_DEV);
> return;
> }
> #endif
> @@ -795,6 +807,7 @@ do_device_not_available(struct pt_regs *regs, long error_code)
> #ifdef CONFIG_X86_32
> conditional_sti(regs);
> #endif
> + trace_trap_exit(INTR_NO_DEV);
> }
>
> #ifdef CONFIG_X86_32
> @@ -817,10 +830,10 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
> /* Set of traps needed for early debugging. */
> void __init early_trap_init(void)
> {
> - set_intr_gate_ist(1, &debug, DEBUG_STACK);
> + set_intr_gate_ist(INTR_DEBUG, &debug, DEBUG_STACK);
> /* int3 can be called from all */
> - set_system_intr_gate_ist(3, &int3, DEBUG_STACK);
> - set_intr_gate(14, &page_fault);
> + set_system_intr_gate_ist(INTR_BREAKPOINT, &int3, DEBUG_STACK);
> + set_intr_gate(INTR_PAGE_FAULT, &page_fault);
> load_idt(&idt_descr);
> }
>
> @@ -836,30 +849,30 @@ void __init trap_init(void)
> early_iounmap(p, 4);
> #endif
>
> - set_intr_gate(0, &divide_error);
> - set_intr_gate_ist(2, &nmi, NMI_STACK);
> + set_intr_gate(INTR_DIV_BY_ZERO, &divide_error);
> + set_intr_gate_ist(INTR_NMI, &nmi, NMI_STACK);
> /* int4 can be called from all */
> - set_system_intr_gate(4, &overflow);
> - set_intr_gate(5, &bounds);
> - set_intr_gate(6, &invalid_op);
> - set_intr_gate(7, &device_not_available);
> + set_system_intr_gate(INTR_OVERFLOW, &overflow);
> + set_intr_gate(INTR_BOUNDS_CHECK, &bounds);
> + set_intr_gate(INTR_INVALID_OP, &invalid_op);
> + set_intr_gate(INTR_NO_DEV, &device_not_available);
> #ifdef CONFIG_X86_32
> - set_task_gate(8, GDT_ENTRY_DOUBLEFAULT_TSS);
> + set_task_gate(INTR_DBL_FAULT, GDT_ENTRY_DOUBLEFAULT_TSS);
> #else
> - set_intr_gate_ist(8, &double_fault, DOUBLEFAULT_STACK);
> + set_intr_gate_ist(INTR_DBL_FAULT, &double_fault, DOUBLEFAULT_STACK);
> #endif
> - set_intr_gate(9, &coprocessor_segment_overrun);
> - set_intr_gate(10, &invalid_TSS);
> - set_intr_gate(11, &segment_not_present);
> - set_intr_gate_ist(12, &stack_segment, STACKFAULT_STACK);
> - set_intr_gate(13, &general_protection);
> - set_intr_gate(15, &spurious_interrupt_bug);
> - set_intr_gate(16, &coprocessor_error);
> - set_intr_gate(17, &alignment_check);
> + set_intr_gate(INTR_SEG_OVERRUN, &coprocessor_segment_overrun);
> + set_intr_gate(INTR_INVALID_TSS, &invalid_TSS);
> + set_intr_gate(INTR_NO_SEG, &segment_not_present);
> + set_intr_gate_ist(INTR_STACK_FAULT, &stack_segment, STACKFAULT_STACK);
> + set_intr_gate(INTR_GPF, &general_protection);
> + set_intr_gate(INTR_SPURIOUS, &spurious_interrupt_bug);
> + set_intr_gate(INTR_COPROCESSOR, &coprocessor_error);
> + set_intr_gate(INTR_ALIGNMENT, &alignment_check);
> #ifdef CONFIG_X86_MCE
> - set_intr_gate_ist(18, &machine_check, MCE_STACK);
> + set_intr_gate_ist(INTR_MCE, &machine_check, MCE_STACK);
> #endif
> - set_intr_gate(19, &simd_coprocessor_error);
> + set_intr_gate(INTR_SIMD_COPROCESSOR, &simd_coprocessor_error);
>
> /* Reserve all the builtin and the syscall vector: */
> for (i = 0; i < FIRST_EXTERNAL_VECTOR; i++)
> --
> 1.7.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2011-05-03 19:00:52

by Vaibhav Nagarnaik

[permalink] [raw]
Subject: Re: [PATCH 1/2] x86: Change trap definitions to enumerated values

On Tue, May 3, 2011 at 6:18 AM, Don Zickus <[email protected]> wrote:
> On Fri, Apr 29, 2011 at 05:52:52PM -0700, Vaibhav Nagarnaik wrote:
>> From: Aditya Kali <[email protected]>
>>
>> The traps are referred to by their numbers and it can be difficult to
>> understand them while reading the code without context. This patch adds
>> enumeration of the trap numbers and replaced the numbers to the correct
>> enum for x86.
>
> My only problem with this patch is you took a straight forward conversion
> which didn't really change the functionality and sprinkled some trace
> points in there.

Actually, readability and maintainability was the goal to create the patch.
When the tracepoints are added, they can use the enum values rather than
floating magic numbers.

Thomas and Steven,
Would it make better sense to separate this patch into 2 patches, the first
one replacing the numbers to enums and the second one adding the tracepoints?


>
> I am ok with the conversion to something readable, but I don't know enough
> about the trace point to know if those are correct spots.
>
> Cheers,
> Don
>
>>
>> Signed-off-by: Vaibhav Nagarnaik <[email protected]>
>> ---


Thanks

Vaibhav Nagarnaik

2011-05-03 22:41:38

by Michael Rubin

[permalink] [raw]
Subject: Re: [PATCH 1/2] x86: Change trap definitions to enumerated values

On Tue, May 3, 2011 at 12:00 PM, Vaibhav Nagarnaik
<[email protected]> wrote:
> Would it make better sense to separate this patch into 2 patches, the first
> one replacing the numbers to enums and the second one adding the tracepoints?

Yes please do that.
Each patch should stand on its own with one simple purpose if possible.

mrubin

2011-05-03 22:54:43

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 1/2] x86: Change trap definitions to enumerated values

On Tue, 2011-05-03 at 15:41 -0700, Michael Rubin wrote:
> On Tue, May 3, 2011 at 12:00 PM, Vaibhav Nagarnaik
> <[email protected]> wrote:
> > Would it make better sense to separate this patch into 2 patches, the first
> > one replacing the numbers to enums and the second one adding the tracepoints?
>
> Yes please do that.
> Each patch should stand on its own with one simple purpose if possible.

Agreed. Don't break up patches just because of size, but instead with
purpose. Usually this breaks up large patches as you want to build up
something, piece by piece, where each piece has a specific, easy to
understand goal.

This is actually what is taking me so long to get my ftrace rewrite out.
Not the actual work, but coming up with a way to break up that work in
logical steps, that makes it easy to understand and review.

-- Steve

2011-05-03 23:09:05

by David Sharp

[permalink] [raw]
Subject: Re: [PATCH 1/2] x86: Change trap definitions to enumerated values

Well, these two patches were one patch originally, and we broke it up
before sending it to you, but we made a couple mistakes:

- missed that there were still tracepoints in this patch (it's a long,
repetitive patch).
- over-estimated the resistance to this patch, and under-estimated
resistance to the tracepoint patch, so put the tracepoint patch first.

We're learning...

On Tue, May 3, 2011 at 3:54 PM, Steven Rostedt <[email protected]> wrote:
> On Tue, 2011-05-03 at 15:41 -0700, Michael Rubin wrote:
>> On Tue, May 3, 2011 at 12:00 PM, Vaibhav Nagarnaik
>> <[email protected]> wrote:
>> > Would it make better sense to separate this patch into 2 patches, the first
>> > one replacing the numbers to enums and the second one adding the tracepoints?
>>
>> Yes please do that.
>> Each patch should stand on its own with one simple purpose if possible.
>
> Agreed. Don't break up patches just because of size, but instead with
> purpose. Usually this breaks up large patches as you want to build up
> something, piece by piece, where each piece has a specific, easy to
> understand goal.
>
> This is actually what is taking me so long to get my ftrace rewrite out.
> Not the actual work, but coming up with a way to break up that work in
> logical steps, that makes it easy to understand and review.
>
> -- Steve
>
>
>