[PATCH 1/2] 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

[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 <fweisbec@gmail.com>
---
 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(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 1b43086..7693c53 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -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 {
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 677432b..9a86e3d 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -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)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 2f32969..463e1eb 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -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;
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index b07e34e..1ac9b98 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -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))
-- 
1.6.0.4

Re: [PATCH 1/2] tracing/ftrace: don't trace on early stage of secondary cpu boot

[Steven Rostedt]

On Wed, 24 Dec 2008, Frederic Weisbecker wrote:

> 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 <fweisbec@gmail.com>
> ---
>  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(-)
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 1b43086..7693c53 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -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

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.

> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  
>  union ftrace_code_union {
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 677432b..9a86e3d 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -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)
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 2f32969..463e1eb 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -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;
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index b07e34e..1ac9b98 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -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;

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


>  
>  	if (!ftrace_graph_addr(trace->func))
> -- 
> 1.6.0.4
> 
> 
> 

Re: [PATCH 1/2] tracing/ftrace: don't trace on early stage of secondary cpu boot

[Frédéric Weisbecker]

2008/12/24 Steven Rostedt <rostedt@goodmis.org>:
>
> On Wed, 24 Dec 2008, Frederic Weisbecker wrote:
>
>> 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 <fweisbec@gmail.com>
>> ---
>>  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(-)
>>
>> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
>> index 1b43086..7693c53 100644
>> --- a/arch/x86/kernel/ftrace.c
>> +++ b/arch/x86/kernel/ftrace.c
>> @@ -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
>
> 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.
>


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....


>> +
>>  #ifdef CONFIG_DYNAMIC_FTRACE
>>
>>  union ftrace_code_union {
>> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
>> index 677432b..9a86e3d 100644
>> --- a/include/linux/ftrace.h
>> +++ b/include/linux/ftrace.h
>> @@ -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)
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index 2f32969..463e1eb 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -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;
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index b07e34e..1ac9b98 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -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;
>
> 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.


> 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...

Re: [PATCH 1/2] tracing/ftrace: don't trace on early stage of secondary cpu boot

[Frédéric Weisbecker]

2008/12/24 Steven Rostedt <rostedt@goodmis.org>:
>
> On Wed, 24 Dec 2008, Frederic Weisbecker wrote:
>
>> 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 <fweisbec@gmail.com>
>> ---
>>  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(-)
>>
>> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
>> index 1b43086..7693c53 100644
>> --- a/arch/x86/kernel/ftrace.c
>> +++ b/arch/x86/kernel/ftrace.c
>> @@ -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
>
> 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.
>
>> +
>>  #ifdef CONFIG_DYNAMIC_FTRACE
>>
>>  union ftrace_code_union {
>> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
>> index 677432b..9a86e3d 100644
>> --- a/include/linux/ftrace.h
>> +++ b/include/linux/ftrace.h
>> @@ -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)
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index 2f32969..463e1eb 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -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;
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index b07e34e..1ac9b98 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -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;
>
> 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?


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....

Re: [PATCH 1/2] tracing/ftrace: don't trace on early stage of secondary cpu boot

[Frédéric Weisbecker]

2008/12/24 Frédéric Weisbecker <fweisbec@gmail.com>:
> 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.


Sorry, I meant cpu_pda(stack_smp_processor_id())->pcurrent

Re: [PATCH 1/2] tracing/ftrace: don't trace on early stage of secondary cpu boot

[Steven Rostedt]

On Wed, 24 Dec 2008, Fr?d?ric Weisbecker wrote:
> >>       /*
> >> @@ -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.
> 
> 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.

> 
> 
> > 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...

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

Re: [PATCH 1/2] tracing/ftrace: don't trace on early stage of secondary cpu boot

[Steven Rostedt]

On Wed, 24 Dec 2008, Fr?d?ric Weisbecker wrote:
> 
> 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....

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

Re: [PATCH 1/2] tracing/ftrace: don't trace on early stage of secondary cpu boot

[Frédéric Weisbecker]

2008/12/24 Steven Rostedt <rostedt@goodmis.org>:
>
> On Wed, 24 Dec 2008, Fr?d?ric Weisbecker wrote:
>> >>       /*
>> >> @@ -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.
>>
>> 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...

>>
>>
>> > 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...
>
> 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.


But on the worst case, that could cause a fault recursion since the first line
of do_page_fault in x86 is:

tsk = current;

> -- Steve
>
>

Re: [PATCH 1/2] tracing/ftrace: don't trace on early stage of secondary cpu boot

[Steven Rostedt]

On Wed, 24 Dec 2008, Fr?d?ric Weisbecker wrote:
> 
> 
> 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

Re: [PATCH 1/2] tracing/ftrace: don't trace on early stage of secondary cpu boot

[Frédéric Weisbecker]

2008/12/24 Steven Rostedt <rostedt@goodmis.org>:
>
> On Wed, 24 Dec 2008, Fr?d?ric Weisbecker wrote:
>>
>>
>> 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

Ok, I will try with this solution.

Thanks.