tracing/ftrace: don't trace on early stage of secondary cpu boot
PATCH 1/2 tracing/ftrace: don't trace on early stage of secondary cpu boot
Impact: fix a crash/hard reboot while enabling cpu on runtime
On some archs, the boot of a secondary cpu can have an early fragile state. On x86-64, the pda is not initialized on the first stage of a cpu boot but it is needed to get the cpu number and the current task pointer. These datas are needed during tracing. As they were dereferenced at this stage, we got a crash while turning on a cpu on runtime while tracing.
Some other archs like ia64 can have such kind of issue too.
This patch implements a function to verify if the cpu has been correctly initialized before tracing it. By default, the check does nothing but archs can override it (as does x86-64 in this patch).
--- arch/x86/kernel/ftrace.c | 13 +++++++++++++ include/linux/ftrace.h | 8 ++++++++ kernel/trace/ftrace.c | 6 ++++++ kernel/trace/trace.c | 6 +++--- 4 files changed, 30 insertions(+), 3 deletions(-)
arch/x86/kernel/ftrace.c +13 -0
@@ -24,6 +24,19 @@#include <asm/nmi.h> +#ifdef CONFIG_X86_64 +/* +* A cpu doesn't want to be traced if pda hasn't yet been initialized. +* Pda contains the cpu number and current task pointer. A lot +* of functions use them, not only ftrace. +* We use stack_smp_processor_id() here to avoid using the pda. +*/ +bool ftrace_in_early_cpuinit (void ) +{ +return !cpu_isset (stack_smp_processor_id (), cpu_online_map); +} +#endif +#ifdef CONFIG_DYNAMIC_FTRACE union ftrace_code_union {
include/linux/ftrace.h +8 -0
@@ -78,6 +78,14 @@ void clear_ftrace_function(void);extern void ftrace_stub (unsigned long a0,unsigned long a1); +/* +* On some archs, the secondary cpu boot is sometimes fragile. +* Some important datas could be not yet initialized. In such states, +* tracing is too dangerous and we use this function to detect these +* kinds of context. Archs which have this sort of issues can override it. +*/ +extern bool ftrace_in_early_cpuinit (void ); +#else /* !CONFIG_FUNCTION_TRACER */ # define register_ftrace_function (ops) do { } while (0)# define unregister_ftrace_function (ops) do { } while (0)
kernel/trace/ftrace.c +6 -0
@@ -1962,6 +1962,12 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,return ret; } +/* Overriden by archs if needed */ +bool __attribute__((weak))ftrace_in_early_cpuinit (void ) +{ +return false; +} +#ifdef CONFIG_FUNCTION_GRAPH_TRACER static atomic_t ftrace_graph_active;
kernel/trace/trace.c +3 -3
@@ -1175,7 +1175,7 @@ function_trace_call_preempt_only(unsigned long ip, unsigned long parent_ip)int cpu, resched;int pc; -if (unlikely (!ftrace_function_enabled)) +if (unlikely (!ftrace_function_enabled ||ftrace_in_early_cpuinit ()))return ; pc =preempt_count ();
@@ -1202,7 +1202,7 @@ function_trace_call(unsigned long ip, unsigned long parent_ip)int cpu;int pc; -if (unlikely (!ftrace_function_enabled)) +if (unlikely (!ftrace_function_enabled ||ftrace_in_early_cpuinit ()))return ;/*
@@ -1233,7 +1233,7 @@ int trace_graph_entry(struct ftrace_graph_ent *trace)int cpu;int pc; -if (!ftrace_trace_task (current)) +if (!ftrace_trace_task (current) ||ftrace_in_early_cpuinit ())return 0 ;if (!ftrace_graph_addr (trace->func))
Signature · 1 line
1.6.0.4
Steven Rostedt
maintainerQuoted 32 lines · Frederic Weisbecker
Impact: fix a crash/hard reboot while enabling cpu on runtime
On some archs, the boot of a secondary cpu can have an early fragile state. On x86-64, the pda is not initialized on the first stage of a cpu boot but it is needed to get the cpu number and the current task pointer. These datas are needed during tracing. As they were dereferenced at this stage, we got a crash while turning on a cpu on runtime while tracing.
Some other archs like ia64 can have such kind of issue too.
This patch implements a function to verify if the cpu has been correctly initialized before tracing it. By default, the check does nothing but archs can override it (as does x86-64 in this patch).
Signed-off-by Frederic Weisbecker <[email protected]>--- arch/x86/kernel/ftrace.c | 13 +++++++++++++ include/linux/ftrace.h | 8 ++++++++ kernel/trace/ftrace.c | 6 ++++++ kernel/trace/trace.c | 6 +++--- 4 files changed, 30 insertions(+), 3 deletions(-)arch/x86/kernel/ftrace.c +2
-0@@ -24,6 +24,19 @@#include <asm/nmi.h> +#ifdef CONFIG_X86_64 +/*
arch/x86/kernel/ftrace.c line 38
Quoted 10 lines
arch/x86/kernel/ftrace.c +10
-0+* A cpu doesn't want to be traced if pda hasn't yet been initialized. +* Pda contains the cpu number and current task pointer. A lot +* of functions use them, not only ftrace. +* We use stack_smp_processor_id() here to avoid using the pda. +*/ +bool ftrace_in_early_cpuinit (void ) +{ +return !cpu_isset (stack_smp_processor_id (), cpu_online_map); +} +#endif
Is there something lighter weight than this? This is called during the function tracer and that is called at ever function. I would also rather this be an inline function defined in asm/ftrace.h if possible.
Quoted 63 lines
arch/x86/kernel/ftrace.c +1
-0+#ifdef CONFIG_DYNAMIC_FTRACE union ftrace_code_union {include/linux/ftrace.h +8
-0@@ -78,6 +78,14 @@ void clear_ftrace_function(void);extern void ftrace_stub (unsigned long a0,unsigned long a1); +/* +* On some archs, the secondary cpu boot is sometimes fragile. +* Some important datas could be not yet initialized. In such states, +* tracing is too dangerous and we use this function to detect these +* kinds of context. Archs which have this sort of issues can override it. +*/ +extern bool ftrace_in_early_cpuinit (void ); +#else /* !CONFIG_FUNCTION_TRACER */ # define register_ftrace_function (ops) do { } while (0)# define unregister_ftrace_function (ops) do { } while (0)kernel/trace/ftrace.c +6
-0@@ -1962,6 +1962,12 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,return ret; } +/* Overriden by archs if needed */ +bool __attribute__((weak))ftrace_in_early_cpuinit (void ) +{ +return false; +} +#ifdef CONFIG_FUNCTION_GRAPH_TRACER static atomic_t ftrace_graph_active;kernel/trace/trace.c +2
-2@@ -1175,7 +1175,7 @@ function_trace_call_preempt_only(unsigned long ip, unsigned long parent_ip)int cpu, resched;int pc; -if (unlikely (!ftrace_function_enabled)) +if (unlikely (!ftrace_function_enabled ||ftrace_in_early_cpuinit ()))return ; pc =preempt_count ();@@ -1202,7 +1202,7 @@ function_trace_call(unsigned long ip, unsigned long parent_ip)int cpu;int pc; -if (unlikely (!ftrace_function_enabled)) +if (unlikely (!ftrace_function_enabled ||ftrace_in_early_cpuinit ()))return ;/*
kernel/trace/trace.c int trace_graph_entry(struct ftrace_graph_ent *trace) · line 1237
Quoted 6 lines
kernel/trace/trace.c +1
-1@@ -1233,7 +1233,7 @@ int trace_graph_entry(struct ftrace_graph_ent *trace)int cpu;int pc; -if (!ftrace_trace_task (current)) +if (!ftrace_trace_task (current) ||ftrace_in_early_cpuinit ())return 0 ;
Actually, didn't you say current is not available either? We are testing current first.
I still wonder if there's a better way to find out if it is safe to run. Perhaps the arch code can test the %gs register to see if it is actually something valid?
-- Steve
Quoted 7 lines
kernel/trace/trace.c +0
-0if (!ftrace_graph_addr (trace->func))Signature · 1 line
1.6.0.4
2008/12/24 Steven Rostedt <[email protected]>:
Quoted 33 lines · Frederic Weisbecker
Quoted 31 lines
Impact: fix a crash/hard reboot while enabling cpu on runtime
On some archs, the boot of a secondary cpu can have an early fragile state. On x86-64, the pda is not initialized on the first stage of a cpu boot but it is needed to get the cpu number and the current task pointer. These datas are needed during tracing. As they were dereferenced at this stage, we got a crash while turning on a cpu on runtime while tracing.
Some other archs like ia64 can have such kind of issue too.
This patch implements a function to verify if the cpu has been correctly initialized before tracing it. By default, the check does nothing but archs can override it (as does x86-64 in this patch).
Signed-off-by Frederic Weisbecker <[email protected]>--- arch/x86/kernel/ftrace.c | 13 +++++++++++++ include/linux/ftrace.h | 8 ++++++++ kernel/trace/ftrace.c | 6 ++++++ kernel/trace/trace.c | 6 +++--- 4 files changed, 30 insertions(+), 3 deletions(-)arch/x86/kernel/ftrace.c +0
-0@@ -24,6 +24,19 @@#include <asm/nmi.h>
Quoted 17 lines
arch/x86/kernel/ftrace.c line 38
Quoted 12 lines
arch/x86/kernel/ftrace.c +12
-0+#ifdef CONFIG_X86_64 +/* +* A cpu doesn't want to be traced if pda hasn't yet been initialized. +* Pda contains the cpu number and current task pointer. A lot +* of functions use them, not only ftrace. +* We use stack_smp_processor_id() here to avoid using the pda. +*/ +bool ftrace_in_early_cpuinit (void ) +{ +return !cpu_isset (stack_smp_processor_id (), cpu_online_map); +} +#endif Is there something lighter weight than this? This is called during the function tracer and that is called at ever function. I would also rather this be an inline function defined in
asm/ftrace.hif possible.
I first thought about annotate those first functions with "notrace". But the code inside
cpu_init/pda_init could change later and call external/traced function. The problem with inlining is that I would loose the weak weight of the function....
Quoted 66 lines
Quoted 66 lines
arch/x86/kernel/ftrace.c +1
-0+#ifdef CONFIG_DYNAMIC_FTRACE union ftrace_code_union {include/linux/ftrace.h +8
-0@@ -78,6 +78,14 @@ void clear_ftrace_function(void);extern void ftrace_stub (unsigned long a0,unsigned long a1); +/* +* On some archs, the secondary cpu boot is sometimes fragile. +* Some important datas could be not yet initialized. In such states, +* tracing is too dangerous and we use this function to detect these +* kinds of context. Archs which have this sort of issues can override it. +*/ +extern bool ftrace_in_early_cpuinit (void ); +#else /* !CONFIG_FUNCTION_TRACER */ # define register_ftrace_function (ops) do { } while (0)# define unregister_ftrace_function (ops) do { } while (0)kernel/trace/ftrace.c +6
-0@@ -1962,6 +1962,12 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,return ret; } +/* Overriden by archs if needed */ +bool __attribute__((weak))ftrace_in_early_cpuinit (void ) +{ +return false; +} +#ifdef CONFIG_FUNCTION_GRAPH_TRACER static atomic_t ftrace_graph_active;kernel/trace/trace.c +2
-2@@ -1175,7 +1175,7 @@ function_trace_call_preempt_only(unsigned long ip, unsigned long parent_ip)int cpu, resched;int pc; -if (unlikely (!ftrace_function_enabled)) +if (unlikely (!ftrace_function_enabled ||ftrace_in_early_cpuinit ()))return ; pc =preempt_count ();@@ -1202,7 +1202,7 @@ function_trace_call(unsigned long ip, unsigned long parent_ip)int cpu;int pc; -if (unlikely (!ftrace_function_enabled)) +if (unlikely (!ftrace_function_enabled ||ftrace_in_early_cpuinit ()))return ;/* @@ -1233,7 +1233,7 @@ int trace_graph_entry(struct ftrace_graph_ent *trace)int cpu;int pc;
Quoted 6 lines
kernel/trace/trace.c int trace_graph_entry(struct ftrace_graph_ent *trace) · line 1237
Quoted 3 lines
kernel/trace/trace.c +1
-1-if (!ftrace_trace_task (current)) +if (!ftrace_trace_task (current) ||ftrace_in_early_cpuinit ())return 0 ;Actually, didn't you say current is not available either? We are testing current first.
Oops. Strange, my tests didn't give me any problem. I will fix it.
Quoted 5 lines
I still wonder if there's a better way to find out if it is safe to run. Perhaps the arch code can test the %gs register to see if it is actually something valid?
-- Steve
I could test if MSR_GS_BASE references cpu_pda(cpu) but that remains as much heavy...
Steven Rostedt
maintainerQuoted 12 lines · Fr?d?ric Weisbecker
Quoted 11 lines
kernel/trace/trace.c int trace_graph_entry(struct ftrace_graph_ent *trace) · line 1237
Quoted 8 lines
/*kernel/trace/trace.c +1
-1@@ -1233,7 +1233,7 @@ int trace_graph_entry(struct ftrace_graph_ent *trace)int cpu;int pc; -if (!ftrace_trace_task (current)) +if (!ftrace_trace_task (current) ||ftrace_in_early_cpuinit ())return 0 ;Actually, didn't you say current is not available either? We are testing current first.
Quoted 2 lines
Oops. Strange, my tests didn't give me any problem. I will fix it.
Are you sure 'current' is dangerous? Perhaps we do not need to worry here, since we are testing current against something else, and if the CPU is just coming on line, it will not be the task we want.
Quoted 10 lines
Quoted 7 lines
I still wonder if there's a better way to find out if it is safe to run. Perhaps the arch code can test the %gs register to see if it is actually something valid?
-- Steve
I could test if
MSR_GS_BASEreferencescpu_pda(cpu)but that remains as much heavy...
Perhaps just reading the %gs register and see if it faults would be good enough?
long dummy; int faulted=0; asm( "1: movq %%gs:0, %0\n" "2: ".section .fixup \"ax\"\n" "3: movl $1, %1\n" " jmp 2b\n" ".previous" _ASM_EXTABLE(1b, 3b) : "=r"(dummy), "=r"(faulted) : "1"(faulted));
Would this work? On the good cases, all we do is a read of the %gs section, which is the same as doing a smp_process_id(). On the bad case, we fault, but we also catch that fact. We are more interested in the good case being fast. but I'm not sure how well the start up code could take a fault, even when it is expected.
-- Steve
2008/12/24 Steven Rostedt <[email protected]>:
Quoted 14 lines · Fr?d?ric Weisbecker
Quoted 12 lines
Quoted 11 lines
kernel/trace/trace.c int trace_graph_entry(struct ftrace_graph_ent *trace) · line 1237
Quoted 8 lines
/*kernel/trace/trace.c +1
-1@@ -1233,7 +1233,7 @@ int trace_graph_entry(struct ftrace_graph_ent *trace)int cpu;int pc; -if (!ftrace_trace_task (current)) +if (!ftrace_trace_task (current) ||ftrace_in_early_cpuinit ())return 0 ;Actually, didn't you say current is not available either? We are testing current first.
Quoted 6 lines
Quoted 2 lines
Oops. Strange, my tests didn't give me any problem. I will fix it.
Are you sure 'current' is dangerous? Perhaps we do not need to worry here, since we are testing current against something else, and if the CPU is just coming on line, it will not be the task we want.
Yes, a simple local_irq_save might call trace_hardirqs_off(), and then increment something in current....
And current is found toward reading pda, hence a crash...
Quoted 29 lines
Quoted 10 lines
Quoted 7 lines
I still wonder if there's a better way to find out if it is safe to run. Perhaps the arch code can test the %gs register to see if it is actually something valid?
-- Steve
I could test if
MSR_GS_BASEreferencescpu_pda(cpu)but that remains as much heavy...Perhaps just reading the %gs register and see if it faults would be good enough?
long dummy; int faulted=0; asm( "1: movq %%gs:0, %0\n" "2: ".section .fixup \"ax\"\n" "3: movl $1, %1\n" " jmp 2b\n" ".previous" _ASM_EXTABLE(1b, 3b) : "=r"(dummy), "=r"(faulted) : "1"(faulted));
Quoted 5 lines
Would this work? On the good cases, all we do is a read of the %gs section, which is the same as doing a
smp_process_id(). On the bad case, we fault, but we also catch that fact. We are more interested in the good case being fast. but I'm not sure how well the start up code could take a fault, even when it is expected.
But on the worst case, that could cause a fault recursion since the first line of do_page_fault in x86 is:
tsk = current;
Quoted 3 lines
-- Steve
Steven Rostedt
maintainerQuoted 5 lines · Fr?d?ric Weisbecker
I first thought about annotate those first functions with "notrace". But the code inside
cpu_init/pda_init could change later and call external/traced function.
We could also hit it with the CFLAGS_REMOVE_common.o = -pg hammer. Those functions will not change much, and I doubt they will call other functions for the same reason that we can not call them. A simple preempt_disable here will crash the box.
-- Steve
2008/12/24 Steven Rostedt <[email protected]>:
Quoted 8 lines · Fr?d?ric Weisbecker
Quoted 5 lines
I first thought about annotate those first functions with "notrace". But the code inside
cpu_init/pda_init could change later and call external/traced function.
Quoted 6 lines
We could also hit it with the CFLAGS_REMOVE_common.o = -pg hammer. Those functions will not change much, and I doubt they will call other functions for the same reason that we can not call them. A simple
preempt_disablehere will crash the box.-- Steve
Ok, I will try with this solution.
Thanks.
2008/12/24 Steven Rostedt <[email protected]>:
Quoted 123 lines · Frederic Weisbecker
Quoted 33 lines
Impact: fix a crash/hard reboot while enabling cpu on runtime
On some archs, the boot of a secondary cpu can have an early fragile state. On x86-64, the pda is not initialized on the first stage of a cpu boot but it is needed to get the cpu number and the current task pointer. These datas are needed during tracing. As they were dereferenced at this stage, we got a crash while turning on a cpu on runtime while tracing.
Some other archs like ia64 can have such kind of issue too.
This patch implements a function to verify if the cpu has been correctly initialized before tracing it. By default, the check does nothing but archs can override it (as does x86-64 in this patch).
Signed-off-by Frederic Weisbecker <[email protected]>--- arch/x86/kernel/ftrace.c | 13 +++++++++++++ include/linux/ftrace.h | 8 ++++++++ kernel/trace/ftrace.c | 6 ++++++ kernel/trace/trace.c | 6 +++--- 4 files changed, 30 insertions(+), 3 deletions(-)arch/x86/kernel/ftrace.c +2
-0@@ -24,6 +24,19 @@#include <asm/nmi.h> +#ifdef CONFIG_X86_64 +/* arch/x86/kernel/ftrace.c line 38
Quoted 10 lines
arch/x86/kernel/ftrace.c +10
-0+* A cpu doesn't want to be traced if pda hasn't yet been initialized. +* Pda contains the cpu number and current task pointer. A lot +* of functions use them, not only ftrace. +* We use stack_smp_processor_id() here to avoid using the pda. +*/ +bool ftrace_in_early_cpuinit (void ) +{ +return !cpu_isset (stack_smp_processor_id (), cpu_online_map); +} +#endif Is there something lighter weight than this? This is called during the function tracer and that is called at ever function. I would also rather this be an inline function defined in
asm/ftrace.hif possible.Quoted 63 lines
arch/x86/kernel/ftrace.c +1
-0+#ifdef CONFIG_DYNAMIC_FTRACE union ftrace_code_union {include/linux/ftrace.h +8
-0@@ -78,6 +78,14 @@ void clear_ftrace_function(void);extern void ftrace_stub (unsigned long a0,unsigned long a1); +/* +* On some archs, the secondary cpu boot is sometimes fragile. +* Some important datas could be not yet initialized. In such states, +* tracing is too dangerous and we use this function to detect these +* kinds of context. Archs which have this sort of issues can override it. +*/ +extern bool ftrace_in_early_cpuinit (void ); +#else /* !CONFIG_FUNCTION_TRACER */ # define register_ftrace_function (ops) do { } while (0)# define unregister_ftrace_function (ops) do { } while (0)kernel/trace/ftrace.c +6
-0@@ -1962,6 +1962,12 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,return ret; } +/* Overriden by archs if needed */ +bool __attribute__((weak))ftrace_in_early_cpuinit (void ) +{ +return false; +} +#ifdef CONFIG_FUNCTION_GRAPH_TRACER static atomic_t ftrace_graph_active;kernel/trace/trace.c +2
-2@@ -1175,7 +1175,7 @@ function_trace_call_preempt_only(unsigned long ip, unsigned long parent_ip)int cpu, resched;int pc; -if (unlikely (!ftrace_function_enabled)) +if (unlikely (!ftrace_function_enabled ||ftrace_in_early_cpuinit ()))return ; pc =preempt_count ();@@ -1202,7 +1202,7 @@ function_trace_call(unsigned long ip, unsigned long parent_ip)int cpu;int pc; -if (unlikely (!ftrace_function_enabled)) +if (unlikely (!ftrace_function_enabled ||ftrace_in_early_cpuinit ()))return ;/* kernel/trace/trace.c int trace_graph_entry(struct ftrace_graph_ent *trace) · line 1237
Quoted 6 lines
kernel/trace/trace.c +1
-1@@ -1233,7 +1233,7 @@ int trace_graph_entry(struct ftrace_graph_ent *trace)int cpu;int pc; -if (!ftrace_trace_task (current)) +if (!ftrace_trace_task (current) ||ftrace_in_early_cpuinit ())return 0 ;Actually, didn't you say current is not available either? We are testing current first.
Quoted 3 lines
I still wonder if there's a better way to find out if it is safe to run. Perhaps the arch code can test the %gs register to see if it is actually something valid?
One other solution, would be to define raw_smp_processor_id() to stack_smp_processor_id() and current to cpu_pda(stack_smp_processor_id()) if function_tracer is configured. That would even let one to trace those cpu boot functions.
But the other side effect with this is that perhaps "current" would be retrieved with one pointer more to dereference....
2008/12/24 Frédéric Weisbecker <[email protected]>:
Quoted 3 lines
One other solution, would be to define
raw_smp_processor_id()tostack_smp_processor_id()and current to cpu_pda(stack_smp_processor_id()) iffunction_traceris configured.
Sorry, I meant cpu_pda(stack_smp_processor_id())->pcurrent
Steven Rostedt
maintainerQuoted 9 lines · Fr?d?ric Weisbecker
One other solution, would be to define
raw_smp_processor_id()tostack_smp_processor_id()and current to cpu_pda(stack_smp_processor_id()) iffunction_traceris configured. That would even let one to trace those cpu boot functions.But the other side effect with this is that perhaps "current" would be retrieved with one pointer more to dereference....
Nah, I think that is a bit too intrusive. We really do not want to call the function tracer if it can not handle a simple smp_processor_id() call. You never know what else might turn up in this area.
-- Steve