Skip to contentSkip to conversation

kexec jump: ftrace_enabled_save/restore

PATCH In review Huang Ying 3 participants 7 messages Aug 8, 2008 – Aug 12, 2008

Subsystems (6)
Tracing Live update kexec handover (KHO) ftrace +2 more subsystems
lore
Share & tools
lore

Huang Ying

author
#0 lore raw

PATCH 7/8 kexec jump: ftrace_enabled_save/restore

Add ftrace_enabled_save/restore, used to disable ftrace for a while. This is used by kexec jump.

Signed-off-by Huang Ying <[email protected]>
---
 include/linux/ftrace.h |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
include/linux/ftrace.h +18 -0
@@ -98,6 +98,24 @@ static inline void tracer_disable(void)
 #endif
 }
 
+static inline int ftrace_enabled_save(void)
+{
+#ifdef CONFIG_FTRACE
+	int saved_ftrace_enabled = ftrace_enabled;
+	ftrace_enabled = 0;
+	return saved_ftrace_enabled;
+#else
+	return 0;
+#endif
+}
+
+static inline void ftrace_enabled_restore(int enabled)
+{
+#ifdef CONFIG_FTRACE
+	ftrace_enabled = enabled;
+#endif
+}
+
 #ifdef CONFIG_FRAME_POINTER
 /* TODO: need to fix this for ARM */
 # define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0))

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#1 lore raw
Quoted 5 lines · Huang Ying

Add ftrace_enabled_save/restore, used to disable ftrace for a while. This is used by kexec jump.

Signed-off-by Huang Ying <[email protected]>

CCing Steven Rostedt for ftrace related changes.

Quoted 22 lines
---
 include/linux/ftrace.h |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
include/linux/ftrace.h +11 -0
@@ -98,6 +98,24 @@ static inline void tracer_disable(void)
 #endif
 }
 
+static inline int ftrace_enabled_save(void)
+{
+#ifdef CONFIG_FTRACE
+	int saved_ftrace_enabled = ftrace_enabled;
+	ftrace_enabled = 0;
+	return saved_ftrace_enabled;
+#else
+	return 0;
+#endif
+}
+

include/linux/ftrace.h static inline void tracer_disable(void) · line 121

Quoted 10 lines
include/linux/ftrace.h +7 -0
+static inline void ftrace_enabled_restore(int enabled)
+{
+#ifdef CONFIG_FTRACE
+	ftrace_enabled = enabled;
+#endif
+}
+
 #ifdef CONFIG_FRAME_POINTER
 /* TODO: need to fix this for ARM */
 # define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0))

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

maintainer
#2 lore raw
Quoted 9 lines · Vivek Goyal
Quoted 5 lines · Huang Ying

Add ftrace_enabled_save/restore, used to disable ftrace for a while. This is used by kexec jump.

Signed-off-by Huang Ying <[email protected]>

CCing Steven Rostedt for ftrace related changes.

Thanks,

Quoted 24 lines
Quoted 24 lines
---
 include/linux/ftrace.h |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
include/linux/ftrace.h +13 -0
@@ -98,6 +98,24 @@ static inline void tracer_disable(void)
 #endif
 }
 
+static inline int ftrace_enabled_save(void)
+{
+#ifdef CONFIG_FTRACE
+	int saved_ftrace_enabled = ftrace_enabled;
+	ftrace_enabled = 0;
+	return saved_ftrace_enabled;
+#else
+	return 0;
+#endif
+}
+
+static inline void ftrace_enabled_restore(int enabled)
+{
Quoted 8 lines

include/linux/ftrace.h static inline void tracer_disable(void) · line 121

Quoted 8 lines
include/linux/ftrace.h +5 -0
+#ifdef CONFIG_FTRACE
+	ftrace_enabled = enabled;
+#endif
+}
+
 #ifdef CONFIG_FRAME_POINTER
 /* TODO: need to fix this for ARM */
 # define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0))

The only problem with this approach is what happens if the user changes the enabled in between these two calls. This would make ftrace inconsistent.

I have a patch from the -rt tree that handles what you want. It is attached below. Not sure how well it will apply to mainline.

I really need to go through the rt patch set and start submitting a bunch of clean-up/fixes to mainline. We've been meaning to do it, just have been distracted :-(

-- Steve

From: Steven Rostedt <[email protected]>
Subject: ftrace: cpu hotplug fix

Peter Zijlstra found that taking down and bringing up a new CPU caused ftrace to crash the kernel. This was due to some arch calls that were being traced by the function tracer before the smp_processor_id was set up. Since the function tracer uses smp_processor_id it caused a triple fault.

Instead of adding notrace all over the architecture code to prevent this problem, it is easier to simply disable the function tracer when bringing up a new CPU.

Signed-off-by Steven Rostedt <[email protected]>
---
 include/linux/ftrace.h            |   11 ++++++++---
 kernel/cpu.c                      |    9 +++++++++
 kernel/trace/ftrace.c             |   23 +++++++++++++++++++++++
 kernel/trace/trace_irqsoff.c      |    3 +++
 kernel/trace/trace_sched_wakeup.c |    2 +-
 5 files changed, 44 insertions(+), 4 deletions(-)
include/linux/ftrace.h +8 -3
Index: linux-2.6.26/include/linux/ftrace.h
===================================================================
@@ -33,10 +33,15 @@ void clear_ftrace_function(void);
 
 extern void ftrace_stub(unsigned long a0, unsigned long a1);
 
+void ftrace_enable(void);
+void ftrace_disable(void);
+
 #else /* !CONFIG_FTRACE */
-# define register_ftrace_function(ops) do { } while (0)
-# define unregister_ftrace_function(ops) do { } while (0)
-# define clear_ftrace_function(ops) do { } while (0)
+# define register_ftrace_function(ops)		do { } while (0)
+# define unregister_ftrace_function(ops)	do { } while (0)
+# define clear_ftrace_function(ops)		do { } while (0)
+# define ftrace_enable()			do { } while (0)
+# define ftrace_disable()			do { } while (0)
 #endif /* CONFIG_FTRACE */
 
 #ifdef CONFIG_DYNAMIC_FTRACE
kernel/cpu.c +9 -0
Index: linux-2.6.26/kernel/cpu.c
===================================================================
@@ -14,6 +14,7 @@
 #include <linux/kthread.h>
 #include <linux/stop_machine.h>
 #include <linux/mutex.h>
+#include <linux/ftrace.h>
 
 /* Serializes the updates to cpu_online_map, cpu_present_map */
 static DEFINE_MUTEX(cpu_add_remove_lock);

@@ -300,8 +301,16 @@ static int __cpuinit _cpu_up(unsigned in
 		goto out_notify;
 	}
 
+	/*
+	 * Disable function tracing while bringing up a new CPU.
+	 * We don't want to trace functions that can not handle a
+	 * smp_processor_id() call.
+	 */
+	ftrace_disable();
+
 	/* Arch-specific enabling code. */
 	ret = __cpu_up(cpu);
+	ftrace_enable();
 	if (ret != 0)
 		goto out_notify;
 	BUG_ON(!cpu_online(cpu));
kernel/trace/ftrace.c +23 -0
Index: linux-2.6.26/kernel/trace/ftrace.c
===================================================================
@@ -151,6 +151,29 @@ static int __unregister_ftrace_function(
 	return ret;
 }
 
+static int save_ftrace_enabled;
+
+void ftrace_disable(void)
+{
+	mutex_lock(&ftrace_sysctl_lock);
+
+	save_ftrace_enabled = ftrace_enabled;
+	ftrace_enabled = 0;
+}
+
+void ftrace_enable(void)
+{
+	/* ftrace_enable must be paired with ftrace_disable */
+	if (!mutex_is_locked(&ftrace_sysctl_lock)) {
+		WARN_ON(1);
+		return;
+	}
+
+	ftrace_enabled = save_ftrace_enabled;
+
+	mutex_unlock(&ftrace_sysctl_lock);
+}
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 static struct task_struct *ftraced_task;
kernel/trace/trace_irqsoff.c +3 -0
Index: linux-2.6.26/kernel/trace/trace_irqsoff.c
===================================================================
@@ -77,6 +77,9 @@ irqsoff_tracer_call(unsigned long ip, un
 	long disabled;
 	int cpu;
 
+	if (unlikely(!ftrace_enabled))
+		return;
+
 	/*
 	 * Does not matter if we preempt. We test the flags
 	 * afterward, to see if irqs are disabled or not.
kernel/trace/trace_sched_wakeup.c +1 -1
Index: linux-2.6.26/kernel/trace/trace_sched_wakeup.c
===================================================================
@@ -44,7 +44,7 @@ wakeup_tracer_call(unsigned long ip, uns
 	int resched;
 	int cpu;
 
-	if (likely(!wakeup_task))
+	if (likely(!wakeup_task) || !ftrace_enabled)
 		return;
 
 	resched = need_resched();

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

author
#3 lore raw

Hi, Steven,

On Fri, 2008-08-08 at 10:30 -0400, Steven Rostedt wrote:
[...]

Quoted 10 lines

The only problem with this approach is what happens if the user changes the enabled in between these two calls. This would make ftrace inconsistent.

I have a patch from the -rt tree that handles what you want. It is attached below. Not sure how well it will apply to mainline.

I really need to go through the rt patch set and start submitting a bunch of clean-up/fixes to mainline. We've been meaning to do it, just have been distracted :-(

Your version is better in general sense. Thank you very much!

But in this specific situation of kexec/kjump. The execution environment is that other CPUs are disabled, local irq is disabled, and it is not permitted to switch to other process. But it is safe and sufficient to use non-locked version here.

So to satisfy both demands, I think it is better to provide both version, locked and non-locked. What do you think about that?

Best Regards,
Huang Ying

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

maintainer
#4 lore raw
Quoted 17 lines · Huang Ying

Hi, Steven,

On Fri, 2008-08-08 at 10:30 -0400, Steven Rostedt wrote:
[...]

Quoted 10 lines

The only problem with this approach is what happens if the user changes the enabled in between these two calls. This would make ftrace inconsistent.

I have a patch from the -rt tree that handles what you want. It is attached below. Not sure how well it will apply to mainline.

I really need to go through the rt patch set and start submitting a bunch of clean-up/fixes to mainline. We've been meaning to do it, just have been distracted :-(

Your version is better in general sense. Thank you very much!

Quoted 7 lines

But in this specific situation of kexec/kjump. The execution environment is that other CPUs are disabled, local irq is disabled, and it is not permitted to switch to other process. But it is safe and sufficient to use non-locked version here.

So to satisfy both demands, I think it is better to provide both version, locked and non-locked. What do you think about that?

Sounds good,

I'm looking forward to the patch ;-)

-- Steve

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#5 lore raw
In reply to Huang Ying
Quoted 17 lines · Huang Ying

Hi, Steven,

On Fri, 2008-08-08 at 10:30 -0400, Steven Rostedt wrote:
[...]

Quoted 10 lines

The only problem with this approach is what happens if the user changes the enabled in between these two calls. This would make ftrace inconsistent.

I have a patch from the -rt tree that handles what you want. It is attached below. Not sure how well it will apply to mainline.

I really need to go through the rt patch set and start submitting a bunch of clean-up/fixes to mainline. We've been meaning to do it, just have been distracted :-(

Your version is better in general sense. Thank you very much!

Quoted 8 lines

But in this specific situation of kexec/kjump. The execution environment is that other CPUs are disabled, local irq is disabled, and it is not permitted to switch to other process. But it is safe and sufficient to use non-locked version here.

So to satisfy both demands, I think it is better to provide both version, locked and non-locked. What do you think about that?

Huang,

So you want to use a non-locked version from optimization point of view? So that we don't end up taking and release a lock?

Thanks
Vivek

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

author
#6 lore raw

Hi, Vivek,

On Mon, 2008-08-11 at 09:51 -0400, Vivek Goyal wrote:
[...]

Quoted 2 lines

So you want to use a non-locked version from optimization point of view? So that we don't end up taking and release a lock?

Not from optimization point of view. machine_kexec() may be called from crash_kexec(), where it is not permitted to take and release a lock.

Best Regards,
Huang Ying

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec