Message-ID: <4952B7EA.8060100@gmail.com>
Date:	Wed, 24 Dec 2008 23:30:02 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Steven Rostedt <rostedt@...dmis.org>
CC:	Ingo Molnar <mingo@...e.hu>,
	Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] tracing/ftrace: don't trace on early stage of a secondary
 cpu boot

Steven Rostedt wrote:
> Cool, so the CFLAGS_REMOVE solution worked.
> 
> I'm the last one to criticize someones language, especially when it is not 
> their native tongue. The closest language outside of English that I know 
> is German, and I would do horrible if I had to write this in German.


Me too. I can't imagine this. I studied more the german than the english language,
but I wouldn't dare send only one "Impact" line in german. It would be worst than my
english.


> 
> But since this is going to be a public record of what you changed, it 
> should be a bit more clear.

Of course. And thanks for these comments!


> 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
> 
> s/on the first/in the first/
> 
>> it is needed to get the cpu number and the current task pointer. These datas
> 
> s/These datas are/This data is/
> 
>> are needed during tracing. As they were dereferenced at this stage, we got a
>> crash while turning on a cpu on runtime while tracing.
> 
> "we got a crash while tracing a cpu being enabled at runtime"
> 
>> Some other archs like ia64 can have such kind of issue too.
>>
>> Changes on v2:
>>
>> We drop the previous solution of a per-arch called function to guess the current state
> 
> s/drop/dropped/
> 
>> of a cpu. That could make slow the tracing.
> 
> "That could slow down the tracing."
> 
>> This patch just drop the -pg flag on arch/x86/kernel/cpu/common.c where
> 
> "This patch removes the -pg flag..."
> 
>> live the low level cpu boot functions, and on start_secondary() and a helper
> 
> "where the low level cpu boot functions exist"
> 
> s/, and on/, on/
> 
>> function used at this stage.
>>
>> Signed-off-by: Frederic Weisbecker <fweisbec@...il.com>
> 
> Again, I'm still impressed by your ability to communicate in a language 
> other than your own. And perhaps my corrections are incorrect too ;-)
> 
> Acked-by: Steven Rostedt <srostedt@...hat.com>
> 
> -- Steve

Thanks. Note that french people like me are silently cheating with the english language.
We have a lot of similar words and we can survive while mumbling english, even with only few
basis, but don't tell anyone, it's a secret ;-)
 
I applied your comments, see the patch below.
Thanks again.
--

From: Frederic Weisbecker <fweisbec@...il.com>
Subject: [PATCH v3] 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. This data
is needed during tracing. As they were dereferenced at this stage, we got a
crash while tracing a cpu being enabled at runtime.

Some other archs like ia64 can have such kind of issue too.

Changes on v2:

We dropped the previous solution of a per-arch called function to guess the current state
of a cpu. That could slow down the tracing.
This patch removes the -pg flag on arch/x86/kernel/cpu/common.c where
the low level cpu boot functions exist, on start_secondary() and a helper
function used at this stage.

Changes on v3:

Fix my evil english in the v2 (Thanks Steven).

Signed-off-by: Frederic Weisbecker <fweisbec@...il.com>
Acked-by: Steven Rostedt <srostedt@...hat.com>
---
 arch/x86/include/asm/msr.h   |    3 ++-
 arch/x86/kernel/cpu/Makefile |    5 +++++
 arch/x86/kernel/smpboot.c    |    2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 4640ddd..638bf62 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -85,7 +85,8 @@ static inline void native_write_msr(unsigned int msr,
 	asm volatile("wrmsr" : : "c" (msr), "a"(low), "d" (high) : "memory");
 }
 
-static inline int native_write_msr_safe(unsigned int msr,
+/* Can be uninlined because referenced by paravirt */
+notrace static inline int native_write_msr_safe(unsigned int msr,
 					unsigned low, unsigned high)
 {
 	int err;
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index fc99173..c381330 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -2,6 +2,11 @@
 # Makefile for x86-compatible CPU details, features and quirks
 #
 
+# Don't trace early stages of a secondary CPU boot
+ifdef CONFIG_FUNCTION_TRACER
+CFLAGS_REMOVE_common.o = -pg
+endif
+
 obj-y			:= intel_cacheinfo.o addon_cpuid_features.o
 obj-y			+= proc.o capflags.o powerflags.o common.o
 obj-y			+= vmware.o hypervisor.o
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index b1d571b..31869bf 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -282,7 +282,7 @@ static int __cpuinitdata unsafe_smp;
 /*
  * Activate a secondary processor.
  */
-static void __cpuinit start_secondary(void *unused)
+notrace static void __cpuinit start_secondary(void *unused)
 {
 	/*
 	 * Don't put *anything* before cpu_init(), SMP booting is too
-- 
1.6.0.4


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists