tracing: Have tracepoint reg functions allowed to fail

PATCH New Steven Rostedt 1 participant 5 messages Nov 29, 2016

Subsystems (4)

x86 PowerPC Tracing I2C

lore

Share & tools

lore

Steven Rostedt

author maintainer

2016-11-29 15:01 UTC #0 lore raw

cover tracing: Have tracepoint reg functions allowed to fail

I added a 'trace_event=*:*' to my command line and it crashed the system. The reason was because it enabled the tracepoint benchmark which has a reg function that creates a thread. But the kernel command line tracepoint enabling is done before the scheduler is set up, and thus the creation of a thread killed the kernel.

But then I noticed that there's no way for the reg to tell the calling function that it failed to set up. I changed that so that it can fail and notify the users that the tracepoint did not get enabled due to a failure.

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
ftrace/core

Head SHA1: 9d08cef4a375e171008ed3a041e930aae20cc940

Steven Rostedt (Red Hat) (4):
tracing: Have the reg function allow to fail
tracing: Do not start benchmark on boot up
tracing: Have system enable return error if one of the events fail
tracing: Allow benchmark to be enabled at early_initcall()

----

 arch/powerpc/include/asm/trace.h                  |  4 ++--
 arch/powerpc/platforms/powernv/opal-tracepoints.c |  6 ++++--
 arch/powerpc/platforms/pseries/lpar.c             |  6 ++++--
 arch/x86/include/asm/trace/exceptions.h           |  2 +-
 arch/x86/include/asm/trace/irq_vectors.h          |  2 +-
 arch/x86/kernel/tracepoint.c                      |  3 ++-
 drivers/i2c/i2c-core.c                            |  3 ++-
 include/linux/tracepoint-defs.h                   |  2 +-
 include/linux/tracepoint.h                        |  2 +-
 include/trace/events/i2c.h                        |  2 +-
 kernel/trace/trace_benchmark.c                    | 26 +++++++++++++++++++++--
 kernel/trace/trace_benchmark.h                    |  2 +-
 kernel/trace/trace_events.c                       | 13 ++++++++++--
 kernel/tracepoint.c                               | 12 ++++++++---
 samples/trace_events/trace-events-sample.c        |  3 ++-
 samples/trace_events/trace-events-sample.h        |  2 +-
 16 files changed, 67 insertions(+), 23 deletions(-)

Steven Rostedt

author maintainer

2016-11-29 15:00 UTC #1 lore raw

PATCH 1/4 tracing: Have the reg function allow to fail

From: "Steven Rostedt (Red Hat)" <[email protected]>

Some tracepoints have a registration function that gets enabled when the tracepoint is enabled. There may be cases that the registraction function must fail (for example, can't allocate enough memory). In this case, the tracepoint should also fail to register, otherwise the user would not know way the tracepoint is not working.

Cc David Howells <[email protected]>

Cc Seiji Aguchi <[email protected]>

Cc Anton Blanchard <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/powerpc/include/asm/trace.h                  |  4 ++--
 arch/powerpc/platforms/powernv/opal-tracepoints.c |  6 ++++--
 arch/powerpc/platforms/pseries/lpar.c             |  6 ++++--
 arch/x86/include/asm/trace/exceptions.h           |  2 +-
 arch/x86/include/asm/trace/irq_vectors.h          |  2 +-
 arch/x86/kernel/tracepoint.c                      |  3 ++-
 drivers/i2c/i2c-core.c                            |  3 ++-
 include/linux/tracepoint-defs.h                   |  2 +-
 include/linux/tracepoint.h                        |  2 +-
 include/trace/events/i2c.h                        |  2 +-
 kernel/trace/trace_benchmark.c                    |  3 ++-
 kernel/trace/trace_benchmark.h                    |  2 +-
 kernel/tracepoint.c                               | 12 +++++++++---
 samples/trace_events/trace-events-sample.c        |  3 ++-
 samples/trace_events/trace-events-sample.h        |  2 +-
 15 files changed, 34 insertions(+), 20 deletions(-)

arch/powerpc/include/asm/trace.h +2 -2

index 32e36b16773f..c05cef6ee06c 100644
@@ -54,7 +54,7 @@ DEFINE_EVENT(ppc64_interrupt_class, timer_interrupt_exit,
 );
 
 #ifdef CONFIG_PPC_PSERIES
-extern void hcall_tracepoint_regfunc(void);
+extern int hcall_tracepoint_regfunc(void);
 extern void hcall_tracepoint_unregfunc(void);
 
 TRACE_EVENT_FN_COND(hcall_entry,

@@ -104,7 +104,7 @@ TRACE_EVENT_FN_COND(hcall_exit,
 #endif
 
 #ifdef CONFIG_PPC_POWERNV
-extern void opal_tracepoint_regfunc(void);
+extern int opal_tracepoint_regfunc(void);
 extern void opal_tracepoint_unregfunc(void);
 
 TRACE_EVENT_FN(opal_entry,

arch/powerpc/platforms/powernv/opal-tracepoints.c +4 -2

index 1e496b780efd..3c447002edff 100644
@@ -6,9 +6,10 @@
 #ifdef HAVE_JUMP_LABEL
 struct static_key opal_tracepoint_key = STATIC_KEY_INIT;
 
-void opal_tracepoint_regfunc(void)
+int opal_tracepoint_regfunc(void)
 {
 	static_key_slow_inc(&opal_tracepoint_key);
+	return 0;
 }
 
 void opal_tracepoint_unregfunc(void)

@@ -25,9 +26,10 @@ void opal_tracepoint_unregfunc(void)
 /* NB: reg/unreg are called while guarded with the tracepoints_mutex */
 extern long opal_tracepoint_refcount;
 
-void opal_tracepoint_regfunc(void)
+int opal_tracepoint_regfunc(void)
 {
 	opal_tracepoint_refcount++;
+	return 0;
 }
 
 void opal_tracepoint_unregfunc(void)

arch/powerpc/platforms/pseries/lpar.c +4 -2

index aa35245d8d6d..c0423ce3955c 100644
@@ -661,9 +661,10 @@ EXPORT_SYMBOL(arch_free_page);
 #ifdef HAVE_JUMP_LABEL
 struct static_key hcall_tracepoint_key = STATIC_KEY_INIT;
 
-void hcall_tracepoint_regfunc(void)
+int hcall_tracepoint_regfunc(void)
 {
 	static_key_slow_inc(&hcall_tracepoint_key);
+	return 0;
 }
 
 void hcall_tracepoint_unregfunc(void)

@@ -680,9 +681,10 @@ void hcall_tracepoint_unregfunc(void)
 /* NB: reg/unreg are called while guarded with the tracepoints_mutex */
 extern long hcall_tracepoint_refcount;
 
-void hcall_tracepoint_regfunc(void)
+int hcall_tracepoint_regfunc(void)
 {
 	hcall_tracepoint_refcount++;
+	return 0;
 }
 
 void hcall_tracepoint_unregfunc(void)

arch/x86/include/asm/trace/exceptions.h +1 -1

index 2fbc66c7885b..2422b14c50a7 100644
@@ -6,7 +6,7 @@
 
 #include <linux/tracepoint.h>
 
-extern void trace_irq_vector_regfunc(void);
+extern int trace_irq_vector_regfunc(void);
 extern void trace_irq_vector_unregfunc(void);
 
 DECLARE_EVENT_CLASS(x86_exceptions,

arch/x86/include/asm/trace/irq_vectors.h +1 -1

index 38a09a13a9bc..32dd6a9e343c 100644
@@ -6,7 +6,7 @@
 
 #include <linux/tracepoint.h>
 
-extern void trace_irq_vector_regfunc(void);
+extern int trace_irq_vector_regfunc(void);
 extern void trace_irq_vector_unregfunc(void);
 
 DECLARE_EVENT_CLASS(x86_irq_vector,

arch/x86/kernel/tracepoint.c +2 -1

index 1c113db9ed57..15515132bf0d 100644
@@ -34,7 +34,7 @@ static void switch_idt(void *arg)
 	local_irq_restore(flags);
 }
 
-void trace_irq_vector_regfunc(void)
+int trace_irq_vector_regfunc(void)
 {
 	mutex_lock(&irq_vector_mutex);
 	if (!trace_irq_vector_refcount) {

@@ -44,6 +44,7 @@ void trace_irq_vector_regfunc(void)
 	}
 	trace_irq_vector_refcount++;
 	mutex_unlock(&irq_vector_mutex);
+	return 0;
 }
 
 void trace_irq_vector_unregfunc(void)

drivers/i2c/i2c-core.c +2 -1

index b432b64e307a..6a2b995d7fc4 100644
@@ -77,9 +77,10 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver);
 static struct static_key i2c_trace_msg = STATIC_KEY_INIT_FALSE;
 static bool is_registered;
 
-void i2c_transfer_trace_reg(void)
+int i2c_transfer_trace_reg(void)
 {
 	static_key_slow_inc(&i2c_trace_msg);
+	return 0;
 }
 
 void i2c_transfer_trace_unreg(void)

include/linux/tracepoint-defs.h +1 -1

index 4ac89acb6136..a03192052066 100644
@@ -29,7 +29,7 @@ struct tracepoint_func {
 struct tracepoint {
 	const char *name;		/* Tracepoint name */
 	struct static_key key;
-	void (*regfunc)(void);
+	int (*regfunc)(void);
 	void (*unregfunc)(void);
 	struct tracepoint_func __rcu *funcs;
 };

include/linux/tracepoint.h +1 -1

index be586c632a0c..f72fcfe0e66a 100644
@@ -81,7 +81,7 @@ static inline void tracepoint_synchronize_unregister(void)
 }
 
 #ifdef CONFIG_HAVE_SYSCALL_TRACEPOINTS
-extern void syscall_regfunc(void);
+extern int syscall_regfunc(void);
 extern void syscall_unregfunc(void);
 #endif /* CONFIG_HAVE_SYSCALL_TRACEPOINTS */

include/trace/events/i2c.h +1 -1

index fe17187df65d..4abb8eab34d3 100644
@@ -20,7 +20,7 @@
 /*
  * drivers/i2c/i2c-core.c
  */
-extern void i2c_transfer_trace_reg(void);
+extern int i2c_transfer_trace_reg(void);
 extern void i2c_transfer_trace_unreg(void);
 
 /*

kernel/trace/trace_benchmark.c +2 -1

index 0f109c4130d3..f76d0416dd83 100644
@@ -164,11 +164,12 @@ static int benchmark_event_kthread(void *arg)
  * When the benchmark tracepoint is enabled, it calls this
  * function and the thread that calls the tracepoint is created.
  */
-void trace_benchmark_reg(void)
+int trace_benchmark_reg(void)
 {
 	bm_event_thread = kthread_run(benchmark_event_kthread,
 				      NULL, "event_benchmark");
 	WARN_ON(!bm_event_thread);
+	return 0;
 }
 
 /*

kernel/trace/trace_benchmark.h +1 -1

index 3c1df1df4e29..ebdbfc2f2a64 100644
@@ -6,7 +6,7 @@
 
 #include <linux/tracepoint.h>
 
-extern void trace_benchmark_reg(void);
+extern int trace_benchmark_reg(void);
 extern void trace_benchmark_unreg(void);
 
 #define BENCHMARK_EVENT_STRLEN		128

kernel/tracepoint.c +9 -3

index d0639d917899..1f9a31f934a4 100644
@@ -194,9 +194,13 @@ static int tracepoint_add_func(struct tracepoint *tp,
 			       struct tracepoint_func *func, int prio)
 {
 	struct tracepoint_func *old, *tp_funcs;
+	int ret;
 
-	if (tp->regfunc && !static_key_enabled(&tp->key))
-		tp->regfunc();
+	if (tp->regfunc && !static_key_enabled(&tp->key)) {
+		ret = tp->regfunc();
+		if (ret < 0)
+			return ret;
+	}
 
 	tp_funcs = rcu_dereference_protected(tp->funcs,
 			lockdep_is_held(&tracepoints_mutex));

@@ -529,7 +533,7 @@ EXPORT_SYMBOL_GPL(for_each_kernel_tracepoint);
 /* NB: reg/unreg are called while guarded with the tracepoints_mutex */
 static int sys_tracepoint_refcount;
 
-void syscall_regfunc(void)
+int syscall_regfunc(void)
 {
 	struct task_struct *p, *t;

@@ -541,6 +545,8 @@ void syscall_regfunc(void)
 		read_unlock(&tasklist_lock);
 	}
 	sys_tracepoint_refcount++;
+
+	return 0;
 }
 
 void syscall_unregfunc(void)

samples/trace_events/trace-events-sample.c +2 -1

index 880a7d1d27d2..30e282d33d4d 100644
@@ -79,7 +79,7 @@ static int simple_thread_fn(void *arg)
 
 static DEFINE_MUTEX(thread_mutex);
 
-void foo_bar_reg(void)
+int foo_bar_reg(void)
 {
 	pr_info("Starting thread for foo_bar_fn\n");
 	/*

@@ -90,6 +90,7 @@ void foo_bar_reg(void)
 	mutex_lock(&thread_mutex);
 	simple_tsk_fn = kthread_run(simple_thread_fn, NULL, "event-sample-fn");
 	mutex_unlock(&thread_mutex);
+	return 0;
 }
 
 void foo_bar_unreg(void)

samples/trace_events/trace-events-sample.h +1 -1

index d6b75bb495b3..76a75ab7a608 100644
@@ -354,7 +354,7 @@ TRACE_EVENT_CONDITION(foo_bar_with_cond,
 	TP_printk("foo %s %d", __get_str(foo), __entry->bar)
 );
 
-void foo_bar_reg(void);
+int foo_bar_reg(void);
 void foo_bar_unreg(void);
 
 /*

Signature · 1 line

2.10.2

Steven Rostedt

author maintainer

2016-11-29 15:00 UTC #2 lore raw

PATCH 2/4 tracing: Do not start benchmark on boot up

In reply to Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>

Trace events are enabled every early on boot up via the boot command line parameter. The benchmark tool creates a new thread to perform the trace event benchmarking. But at start up, it is called before scheduling is set up and this crashes the kerenl.

Have the benchmark fail to register when started via the kernel command line.

Also, since the registering of a tracepoint now can handle failure cases, return -ENOMEM instead of warning if the thread can not be created.

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/trace_benchmark.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

kernel/trace/trace_benchmark.c +11 -1

index f76d0416dd83..2bc7dc3e8ff8 100644
@@ -166,9 +166,18 @@ static int benchmark_event_kthread(void *arg)
  */
 int trace_benchmark_reg(void)
 {
+	if (system_state != SYSTEM_RUNNING) {
+		pr_warning("trace benchmark cannot be started via kernel command line\n");
+		return -EBUSY;
+	}
+
 	bm_event_thread = kthread_run(benchmark_event_kthread,
 				      NULL, "event_benchmark");
-	WARN_ON(!bm_event_thread);
+	if (!bm_event_thread) {
+		pr_warning("trace benchmark failed to create kernel thread\n");
+		return -ENOMEM;
+	}
+
 	return 0;
 }

@@ -183,6 +192,7 @@ void trace_benchmark_unreg(void)
 		return;
 
 	kthread_stop(bm_event_thread);
+	bm_event_thread = NULL;
 
 	strcpy(bm_str, "START");
 	bm_total = 0;

Signature · 1 line

2.10.2

Steven Rostedt

author maintainer

2016-11-29 15:00 UTC #3 lore raw

PATCH 3/4 tracing: Have system enable return error if one of the events fail

In reply to Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>

If one of the events of within a system fails to enable when "1" is written to the system "enable" file, it should return an error. Note, some events may still be enabled, but the user should know that something did go wrong.

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/trace_events.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

kernel/trace/trace_events.c +11 -2

index d35fc2b0d304..93116549a284 100644
@@ -702,6 +702,7 @@ __ftrace_set_clr_event_nolock(struct trace_array *tr, const char *match,
 	struct trace_event_call *call;
 	const char *name;
 	int ret = -EINVAL;
+	int eret = 0;
 
 	list_for_each_entry(file, &tr->events, list) {

@@ -725,9 +726,17 @@ __ftrace_set_clr_event_nolock(struct trace_array *tr, const char *match,
 		if (event && strcmp(event, name) != 0)
 			continue;
 
-		ftrace_event_enable_disable(file, set);
+		ret = ftrace_event_enable_disable(file, set);
 
-		ret = 0;
+		/*
+		 * Save the first error and return that. Some events
+		 * may still have been enabled, but let the user
+		 * know that something went wrong.
+		 */
+		if (ret && !eret)
+			eret = ret;
+
+		ret = eret;
 	}
 
 	return ret;

Signature · 1 line

2.10.2

Steven Rostedt

author maintainer

2016-11-29 15:01 UTC #4 lore raw

PATCH 4/4 tracing: Allow benchmark to be enabled at early_initcall()

In reply to Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>

The trace event start up selftests fails when the trace benchmark is enabled, because it is disabled during boot. It really only needs to be disabled before scheduling is set up, as it creates a thread.

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/trace_benchmark.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

kernel/trace/trace_benchmark.c +12 -1

index 2bc7dc3e8ff8..e3b488825ae3 100644
@@ -21,6 +21,8 @@ static u64 bm_stddev;
 static unsigned int bm_avg;
 static unsigned int bm_std;
 
+static bool ok_to_run;
+
 /*
  * This gets called in a loop recording the time it took to write
  * the tracepoint. What it writes is the time statistics of the last

@@ -166,7 +168,7 @@ static int benchmark_event_kthread(void *arg)
  */
 int trace_benchmark_reg(void)
 {
-	if (system_state != SYSTEM_RUNNING) {
+	if (!ok_to_run) {
 		pr_warning("trace benchmark cannot be started via kernel command line\n");
 		return -EBUSY;
 	}

@@ -207,3 +209,12 @@ void trace_benchmark_unreg(void)
 	bm_avg = 0;
 	bm_stddev = 0;
 }
+
+static __init int ok_to_run_trace_benchmark(void)
+{
+	ok_to_run = true;
+
+	return 0;
+}
+
+early_initcall(ok_to_run_trace_benchmark);

Signature · 1 line

2.10.2