From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752783AbYLXCoe (ORCPT ); Tue, 23 Dec 2008 21:44:34 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751788AbYLXCoZ (ORCPT ); Tue, 23 Dec 2008 21:44:25 -0500 Received: from qw-out-2122.google.com ([74.125.92.26]:50860 "EHLO qw-out-2122.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751772AbYLXCoY (ORCPT ); Tue, 23 Dec 2008 21:44:24 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:to:subject:cc:in-reply-to:mime-version :content-type:content-transfer-encoding:content-disposition :references; b=cc41ElUNJv9KFvrUnBabhLOMzLPQqI/7imA6kAEEUtzFILPGj9plYleuI0f4JgeGW3 jL3VPOAsjK/X8j9xT9pCcM5D7oLcsip20qqlmZOYT6hEi0fQUDGBjYjMMC3Pxf14RdlF lK+l953l9fumiLEnQZp3F9DfyGp3Meg1oLdRk= Message-ID: Date: Wed, 24 Dec 2008 03:44:23 +0100 From: "=?ISO-8859-1?Q?Fr=E9d=E9ric_Weisbecker?=" To: "Steven Rostedt" Subject: Re: [PATCH 1/2] tracing/ftrace: don't trace on early stage of secondary cpu boot Cc: "Ingo Molnar" , "Linux Kernel" In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <49518535.8080105@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2008/12/24 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 >> --- >> 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 >> >> >> +#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...