[RFC][PATCH 00/18 v2] ftrace/rcu: Handle unsafe RCU functions and ftrace callbacks

[RFC][PATCH 00/18 v2] ftrace/rcu: Handle unsafe RCU functions and ftrace callbacks

[Steven Rostedt]

This is my final draft of the patches. I'm starting to run them
through my formal tests now. They may change depending on the outcome
of the tests.

I'm also a bit tired, and I added the change logs last. Thus I may
need to go back and fix the change logs up too. But the code was done
when I was rather spunky. But that doesn't mean I didn't break anything.

Anyway, let me know what you think. I'm going to need acks for the
places I added the FTRACE_UNSAFE_RCU() annotations.

I added a checker that traces all functions with the non RCU safe
settings, and did a rcu_read_lock() with PROVE_RCU enabled. This caught
several functions that needed to be annotated. There may be more, but
the test gives us a way to find out.

There's some improvements that can be done to these patches, but for
3.12, I think this version is good enough. We can do the improvements
for 3.13.

Well, enjoy.

-- Steve


Steven Rostedt (Red Hat) (18):
      ftrace: Add hash list to save RCU unsafe functions
      ftrace: Do not set ftrace records for unsafe RCU when not allowed
      ftrace: Set ftrace internal function tracing RCU safe
      ftrace: Add test for ops against unsafe RCU functions in callback
      ftrace: Do not display non safe RCU functions in available_filter_functions
      ftrace: Add rcu_unsafe_filter_functions file
      ftrace: Add selftest to check if RCU unsafe functions are filtered properly
      ftrace/rcu: Do not trace debug_lockdep_rcu_enabled()
      ftrace: Fix a slight race in modifying what function callback gets traced
      ftrace: Create a RCU unsafe checker
      ftrace: Adde infrastructure to stop RCU unsafe checker from checking
      ftrace: Disable RCU unsafe checker when function graph is enabled
      ftrace: Disable the RCU unsafe checker when irqsoff is enabled
      ftrace/lockdep: Have the RCU lockdep splat show what function triggered
      ftrace/rcu: Mark functions that are RCU unsafe
      rcu/irq/x86: Mark functions that are RCU unsafe
      ftrace/cpuidle/x86: Mark functions that are RCU unsafe
      ftrace/sched: Mark functions that are RCU unsafe

----
 arch/x86/kernel/apic/apic.c           |    2 +
 arch/x86/kernel/irq.c                 |    1 +
 arch/x86/kernel/irq_work.c            |    3 +
 arch/x86/kernel/process.c             |    2 +
 arch/x86/kernel/smp.c                 |    8 ++
 drivers/cpuidle/cpuidle.c             |    2 +
 include/asm-generic/vmlinux.lds.h     |   10 ++
 include/linux/ftrace.h                |   46 +++++++
 kernel/cpu/idle.c                     |    2 +
 kernel/lockdep.c                      |    2 +
 kernel/rcupdate.c                     |    2 +-
 kernel/rcutiny.c                      |    1 +
 kernel/rcutree.c                      |    7 +
 kernel/sched/core.c                   |    2 +
 kernel/softirq.c                      |    2 +
 kernel/trace/Kconfig                  |   22 ++++
 kernel/trace/ftrace.c                 |  231 +++++++++++++++++++++++++++++++--
 kernel/trace/trace.h                  |   11 ++
 kernel/trace/trace_functions.c        |   76 ++++++++++-
 kernel/trace/trace_irqsoff.c          |   16 ++-
 kernel/trace/trace_selftest.c         |   94 ++++++++++++++
 kernel/trace/trace_selftest_dynamic.c |    7 +
 22 files changed, 532 insertions(+), 17 deletions(-)

[RFC][PATCH 01/18 v2] ftrace: Add hash list to save RCU unsafe functions

[Steven Rostedt]

[-- Attachment #1: 0001-ftrace-Add-hash-list-to-save-RCU-unsafe-functions.patch --]
[-- Type: text/plain, Size: 8967 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Some ftrace function tracing callbacks use RCU (perf), thus if
it gets called from tracing a function outside of the RCU tracking,
like in entering or leaving NO_HZ idle/userspace, the RCU read locks
in the callback are useless.

As normal function tracing does not use RCU, and function tracing
happens to help debug RCU, we do not want to limit all callbacks
from tracing these "unsafe" RCU functions. Instead, we create an
infrastructure that will let us mark these unsafe RCU functions
and we will only allow callbacks that explicitly say they do not
use RCU to be called by these functions.

This patch adds the infrastructure to create the hash of functions
that are RCU unsafe. This is done with the FTRACE_UNSAFE_RCU()
macro. It works like EXPORT_SYMBOL() does. Simply place the macro
under the RCU unsafe function:

void func(void)
{
	[...]
}
FTRACE_UNSAFE_RCU(func);

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/asm-generic/vmlinux.lds.h |   10 +++
 include/linux/ftrace.h            |   38 +++++++++++
 kernel/trace/ftrace.c             |  135 ++++++++++++++++++++++++++++++++++---
 3 files changed, 174 insertions(+), 9 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 69732d2..fdfddd2 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -93,6 +93,15 @@
 #define MCOUNT_REC()
 #endif
 
+#ifdef CONFIG_FUNCTION_TRACER
+#define FTRACE_UNSAFE_RCU()	. = ALIGN(8);				\
+			VMLINUX_SYMBOL(__start_ftrace_unsafe_rcu) = .;	\
+			*(_ftrace_unsafe_rcu)				\
+			VMLINUX_SYMBOL(__stop_ftrace_unsafe_rcu) = .;
+#else
+#define FTRACE_UNSAFE_RCU()
+#endif
+
 #ifdef CONFIG_TRACE_BRANCH_PROFILING
 #define LIKELY_PROFILE()	VMLINUX_SYMBOL(__start_annotated_branch_profile) = .; \
 				*(_ftrace_annotated_branch)			      \
@@ -479,6 +488,7 @@
 	MEM_DISCARD(init.data)						\
 	KERNEL_CTORS()							\
 	MCOUNT_REC()							\
+	FTRACE_UNSAFE_RCU()						\
 	*(.init.rodata)							\
 	FTRACE_EVENTS()							\
 	TRACE_SYSCALLS()						\
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 9f15c00..1d17a82 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -92,6 +92,9 @@ typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
  * STUB   - The ftrace_ops is just a place holder.
  * INITIALIZED - The ftrace_ops has already been initialized (first use time
  *            register_ftrace_function() is called, it will initialized the ops)
+ * RCU_SAFE - The callback uses no rcu type locking. This allows the
+ *            callback to be called in locations that RCU is not active.
+ *            (like going into or coming out of idle NO_HZ)
  */
 enum {
 	FTRACE_OPS_FL_ENABLED			= 1 << 0,
@@ -103,6 +106,7 @@ enum {
 	FTRACE_OPS_FL_RECURSION_SAFE		= 1 << 6,
 	FTRACE_OPS_FL_STUB			= 1 << 7,
 	FTRACE_OPS_FL_INITIALIZED		= 1 << 8,
+	FTRACE_OPS_FL_RCU_SAFE			= 1 << 9,
 };
 
 struct ftrace_ops {
@@ -219,6 +223,40 @@ static inline int ftrace_function_local_disabled(struct ftrace_ops *ops)
 extern void ftrace_stub(unsigned long a0, unsigned long a1,
 			struct ftrace_ops *op, struct pt_regs *regs);
 
+/*
+ * In order to speed up boot, save both the name and the
+ * address of the function to find to add to hashes. The
+ * list of function mcount addresses are sorted by the address,
+ * but we still need to use the name to find the actual location
+ * as the mcount call is usually not at the address of the
+ * start of the function.
+ */
+struct ftrace_func_finder {
+	const char		*name;
+	unsigned long		ip;
+};
+
+/*
+ * For functions that are called when RCU is not tracking the CPU
+ * (like for entering or leaving NO_HZ mode, and RCU then ignores
+ * the CPU), they need to be marked with FTRACE_UNSAFE_RCU().
+ * This will prevent all function tracing callbacks from calling
+ * this function unless the callback explicitly states that it
+ * doesn't use RCU with the FTRACE_OPS_FL_RCU_SAFE flag.
+ *
+ * The usage of FTRACE_UNSAFE_RCU() is the same as EXPORT_SYMBOL().
+ * Just place the macro underneath the function that is unsafe.
+ */
+#define FTRACE_UNSAFE_RCU(func) \
+	static struct ftrace_func_finder __ftrace_unsafe_rcu_##func	\
+	 __initdata = {							\
+		.name = #func,						\
+		.ip = (unsigned long)func,				\
+	};							\
+	struct ftrace_func_finder *__ftrace_unsafe_rcu_ptr_##func	\
+		  __attribute__((__section__("_ftrace_unsafe_rcu"))) =	\
+		&__ftrace_unsafe_rcu_##func
+
 #else /* !CONFIG_FUNCTION_TRACER */
 /*
  * (un)register_ftrace_function must be a macro since the ops parameter
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a6d098c..3750360 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1356,6 +1356,23 @@ ftrace_hash_rec_disable(struct ftrace_ops *ops, int filter_hash);
 static void
 ftrace_hash_rec_enable(struct ftrace_ops *ops, int filter_hash);
 
+static int ftrace_convert_size_to_bits(int size)
+{
+	int bits;
+
+	/*
+	 * Make the hash size about 1/2 the # found
+	 */
+	for (size /= 2; size; size >>= 1)
+		bits++;
+
+	/* Don't allocate too much */
+	if (bits > FTRACE_HASH_MAX_BITS)
+		bits = FTRACE_HASH_MAX_BITS;
+
+	return bits;
+}
+
 static int
 ftrace_hash_move(struct ftrace_ops *ops, int enable,
 		 struct ftrace_hash **dst, struct ftrace_hash *src)
@@ -1388,15 +1405,7 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable,
 		goto out;
 	}
 
-	/*
-	 * Make the hash size about 1/2 the # found
-	 */
-	for (size /= 2; size; size >>= 1)
-		bits++;
-
-	/* Don't allocate too much */
-	if (bits > FTRACE_HASH_MAX_BITS)
-		bits = FTRACE_HASH_MAX_BITS;
+	bits = ftrace_convert_size_to_bits(size);
 
 	ret = -ENOMEM;
 	new_hash = alloc_ftrace_hash(bits);
@@ -1486,6 +1495,74 @@ ftrace_ops_test(struct ftrace_ops *ops, unsigned long ip, void *regs)
 	}
 
 
+static __init int ftrace_find_ip(const void *a, const void *b)
+{
+	const struct dyn_ftrace *key = a;
+	const struct dyn_ftrace *rec = b;
+
+	if (key->ip == rec->ip)
+		return 0;
+
+	if (key->ip < rec->ip) {
+		/* If previous is less than, then this is our func */
+		rec--;
+		if (rec->ip < key->ip)
+			return 0;
+		return -1;
+	}
+
+	/* All else, we are greater */
+	return 1;
+}
+
+/*
+ * Find the mcount caller location given the ip address of the
+ * function that contains it. As the mcount caller is usually
+ * after the mcount itself.
+ *
+ * Done for just core functions at boot.
+ */
+static __init unsigned long ftrace_mcount_from_func(unsigned long ip)
+{
+	struct ftrace_page *pg, *last_pg = NULL;
+	struct dyn_ftrace *rec;
+	struct dyn_ftrace key;
+
+	key.ip = ip;
+
+	for (pg = ftrace_pages_start; pg; last_pg = pg, pg = pg->next) {
+		if (ip >= (pg->records[pg->index - 1].ip + MCOUNT_INSN_SIZE))
+			continue;
+
+		/*
+		 * Do the first record manually, as we need to check
+		 * the previous record from the previous page
+		 */
+		if (ip < pg->records[0].ip) {
+			/* First page? Then we found our record! */
+			if (!last_pg)
+				return pg->records[0].ip;
+
+			rec = &last_pg->records[last_pg->index - 1];
+			if (rec->ip < ip)
+				return pg->records[0].ip;
+
+			/* Not found */
+			return 0;
+		}
+
+		rec = bsearch(&key, pg->records + 1, pg->index - 1,
+			      sizeof(struct dyn_ftrace),
+			      ftrace_find_ip);
+		if (rec)
+			return rec->ip;
+
+		break;
+	}
+
+	return 0;
+}
+
 static int ftrace_cmp_recs(const void *a, const void *b)
 {
 	const struct dyn_ftrace *key = a;
@@ -4211,6 +4288,44 @@ struct notifier_block ftrace_module_exit_nb = {
 	.priority = INT_MIN,	/* Run after anything that can remove kprobes */
 };
 
+extern struct ftrace_func_finder *__start_ftrace_unsafe_rcu[];
+extern struct ftrace_func_finder *__stop_ftrace_unsafe_rcu[];
+
+static struct ftrace_hash *ftrace_unsafe_rcu;
+
+static void __init create_unsafe_rcu_hash(void)
+{
+	struct ftrace_func_finder *finder;
+	struct ftrace_func_finder **p;
+	unsigned long ip;
+	char str[KSYM_SYMBOL_LEN];
+	int count;
+
+	count = __stop_ftrace_unsafe_rcu - __start_ftrace_unsafe_rcu;
+	if (!count)
+		return;
+
+	count = ftrace_convert_size_to_bits(count);
+	ftrace_unsafe_rcu = alloc_ftrace_hash(count);
+
+	for (p = __start_ftrace_unsafe_rcu;
+	     p < __stop_ftrace_unsafe_rcu;
+	     p++) {
+		finder = *p;
+
+		ip = ftrace_mcount_from_func(finder->ip);
+		if (WARN_ON_ONCE(!ip))
+			continue;
+
+		/* Make sure this is our ip */
+		kallsyms_lookup(ip, NULL, NULL, NULL, str);
+		if (WARN_ON_ONCE(strcmp(str, finder->name) != 0))
+			continue;
+
+		add_hash_entry(ftrace_unsafe_rcu, ip);
+	}
+}
+
 extern unsigned long __start_mcount_loc[];
 extern unsigned long __stop_mcount_loc[];
 
@@ -4250,6 +4365,8 @@ void __init ftrace_init(void)
 	if (ret)
 		pr_warning("Failed to register trace ftrace module exit notifier\n");
 
+	create_unsafe_rcu_hash();
+
 	set_ftrace_early_filters();
 
 	return;
-- 
1.7.10.4

Re: [RFC][PATCH 01/18 v2] ftrace: Add hash list to save RCU unsafe functions

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:18AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Some ftrace function tracing callbacks use RCU (perf), thus if
> it gets called from tracing a function outside of the RCU tracking,
> like in entering or leaving NO_HZ idle/userspace, the RCU read locks
> in the callback are useless.
> 
> As normal function tracing does not use RCU, and function tracing
> happens to help debug RCU, we do not want to limit all callbacks
> from tracing these "unsafe" RCU functions. Instead, we create an
> infrastructure that will let us mark these unsafe RCU functions
> and we will only allow callbacks that explicitly say they do not
> use RCU to be called by these functions.
> 
> This patch adds the infrastructure to create the hash of functions
> that are RCU unsafe. This is done with the FTRACE_UNSAFE_RCU()
> macro. It works like EXPORT_SYMBOL() does. Simply place the macro
> under the RCU unsafe function:
> 
> void func(void)
> {
> 	[...]
> }
> FTRACE_UNSAFE_RCU(func);
> 
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  include/asm-generic/vmlinux.lds.h |   10 +++
>  include/linux/ftrace.h            |   38 +++++++++++
>  kernel/trace/ftrace.c             |  135 ++++++++++++++++++++++++++++++++++---
>  3 files changed, 174 insertions(+), 9 deletions(-)
> 
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 69732d2..fdfddd2 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -93,6 +93,15 @@
>  #define MCOUNT_REC()
>  #endif
> 
> +#ifdef CONFIG_FUNCTION_TRACER
> +#define FTRACE_UNSAFE_RCU()	. = ALIGN(8);				\
> +			VMLINUX_SYMBOL(__start_ftrace_unsafe_rcu) = .;	\
> +			*(_ftrace_unsafe_rcu)				\
> +			VMLINUX_SYMBOL(__stop_ftrace_unsafe_rcu) = .;
> +#else
> +#define FTRACE_UNSAFE_RCU()
> +#endif
> +
>  #ifdef CONFIG_TRACE_BRANCH_PROFILING
>  #define LIKELY_PROFILE()	VMLINUX_SYMBOL(__start_annotated_branch_profile) = .; \
>  				*(_ftrace_annotated_branch)			      \
> @@ -479,6 +488,7 @@
>  	MEM_DISCARD(init.data)						\
>  	KERNEL_CTORS()							\
>  	MCOUNT_REC()							\
> +	FTRACE_UNSAFE_RCU()						\
>  	*(.init.rodata)							\
>  	FTRACE_EVENTS()							\
>  	TRACE_SYSCALLS()						\
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 9f15c00..1d17a82 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -92,6 +92,9 @@ typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
>   * STUB   - The ftrace_ops is just a place holder.
>   * INITIALIZED - The ftrace_ops has already been initialized (first use time
>   *            register_ftrace_function() is called, it will initialized the ops)
> + * RCU_SAFE - The callback uses no rcu type locking. This allows the
> + *            callback to be called in locations that RCU is not active.
> + *            (like going into or coming out of idle NO_HZ)
>   */
>  enum {
>  	FTRACE_OPS_FL_ENABLED			= 1 << 0,
> @@ -103,6 +106,7 @@ enum {
>  	FTRACE_OPS_FL_RECURSION_SAFE		= 1 << 6,
>  	FTRACE_OPS_FL_STUB			= 1 << 7,
>  	FTRACE_OPS_FL_INITIALIZED		= 1 << 8,
> +	FTRACE_OPS_FL_RCU_SAFE			= 1 << 9,
>  };
> 
>  struct ftrace_ops {
> @@ -219,6 +223,40 @@ static inline int ftrace_function_local_disabled(struct ftrace_ops *ops)
>  extern void ftrace_stub(unsigned long a0, unsigned long a1,
>  			struct ftrace_ops *op, struct pt_regs *regs);
> 
> +/*
> + * In order to speed up boot, save both the name and the
> + * address of the function to find to add to hashes. The
> + * list of function mcount addresses are sorted by the address,
> + * but we still need to use the name to find the actual location
> + * as the mcount call is usually not at the address of the
> + * start of the function.
> + */
> +struct ftrace_func_finder {
> +	const char		*name;
> +	unsigned long		ip;
> +};
> +
> +/*
> + * For functions that are called when RCU is not tracking the CPU
> + * (like for entering or leaving NO_HZ mode, and RCU then ignores
> + * the CPU), they need to be marked with FTRACE_UNSAFE_RCU().
> + * This will prevent all function tracing callbacks from calling
> + * this function unless the callback explicitly states that it
> + * doesn't use RCU with the FTRACE_OPS_FL_RCU_SAFE flag.
> + *
> + * The usage of FTRACE_UNSAFE_RCU() is the same as EXPORT_SYMBOL().
> + * Just place the macro underneath the function that is unsafe.
> + */
> +#define FTRACE_UNSAFE_RCU(func) \
> +	static struct ftrace_func_finder __ftrace_unsafe_rcu_##func	\
> +	 __initdata = {							\
> +		.name = #func,						\
> +		.ip = (unsigned long)func,				\
> +	};							\
> +	struct ftrace_func_finder *__ftrace_unsafe_rcu_ptr_##func	\
> +		  __attribute__((__section__("_ftrace_unsafe_rcu"))) =	\
> +		&__ftrace_unsafe_rcu_##func
> +
>  #else /* !CONFIG_FUNCTION_TRACER */
>  /*
>   * (un)register_ftrace_function must be a macro since the ops parameter
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index a6d098c..3750360 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1356,6 +1356,23 @@ ftrace_hash_rec_disable(struct ftrace_ops *ops, int filter_hash);
>  static void
>  ftrace_hash_rec_enable(struct ftrace_ops *ops, int filter_hash);
> 
> +static int ftrace_convert_size_to_bits(int size)
> +{
> +	int bits;
> +
> +	/*
> +	 * Make the hash size about 1/2 the # found
> +	 */
> +	for (size /= 2; size; size >>= 1)
> +		bits++;
> +
> +	/* Don't allocate too much */
> +	if (bits > FTRACE_HASH_MAX_BITS)
> +		bits = FTRACE_HASH_MAX_BITS;
> +
> +	return bits;
> +}
> +
>  static int
>  ftrace_hash_move(struct ftrace_ops *ops, int enable,
>  		 struct ftrace_hash **dst, struct ftrace_hash *src)
> @@ -1388,15 +1405,7 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable,
>  		goto out;
>  	}
> 
> -	/*
> -	 * Make the hash size about 1/2 the # found
> -	 */
> -	for (size /= 2; size; size >>= 1)
> -		bits++;
> -
> -	/* Don't allocate too much */
> -	if (bits > FTRACE_HASH_MAX_BITS)
> -		bits = FTRACE_HASH_MAX_BITS;
> +	bits = ftrace_convert_size_to_bits(size);
> 
>  	ret = -ENOMEM;
>  	new_hash = alloc_ftrace_hash(bits);
> @@ -1486,6 +1495,74 @@ ftrace_ops_test(struct ftrace_ops *ops, unsigned long ip, void *regs)
>  	}
> 
> 
> +static __init int ftrace_find_ip(const void *a, const void *b)
> +{
> +	const struct dyn_ftrace *key = a;
> +	const struct dyn_ftrace *rec = b;
> +
> +	if (key->ip == rec->ip)
> +		return 0;
> +
> +	if (key->ip < rec->ip) {
> +		/* If previous is less than, then this is our func */
> +		rec--;
> +		if (rec->ip < key->ip)
> +			return 0;
> +		return -1;
> +	}
> +
> +	/* All else, we are greater */
> +	return 1;
> +}
> +
> +/*
> + * Find the mcount caller location given the ip address of the
> + * function that contains it. As the mcount caller is usually
> + * after the mcount itself.
> + *
> + * Done for just core functions at boot.
> + */
> +static __init unsigned long ftrace_mcount_from_func(unsigned long ip)
> +{
> +	struct ftrace_page *pg, *last_pg = NULL;
> +	struct dyn_ftrace *rec;
> +	struct dyn_ftrace key;
> +
> +	key.ip = ip;
> +
> +	for (pg = ftrace_pages_start; pg; last_pg = pg, pg = pg->next) {
> +		if (ip >= (pg->records[pg->index - 1].ip + MCOUNT_INSN_SIZE))
> +			continue;
> +
> +		/*
> +		 * Do the first record manually, as we need to check
> +		 * the previous record from the previous page
> +		 */
> +		if (ip < pg->records[0].ip) {
> +			/* First page? Then we found our record! */
> +			if (!last_pg)
> +				return pg->records[0].ip;
> +
> +			rec = &last_pg->records[last_pg->index - 1];
> +			if (rec->ip < ip)
> +				return pg->records[0].ip;
> +
> +			/* Not found */
> +			return 0;
> +		}
> +
> +		rec = bsearch(&key, pg->records + 1, pg->index - 1,
> +			      sizeof(struct dyn_ftrace),
> +			      ftrace_find_ip);
> +		if (rec)
> +			return rec->ip;
> +
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  static int ftrace_cmp_recs(const void *a, const void *b)
>  {
>  	const struct dyn_ftrace *key = a;
> @@ -4211,6 +4288,44 @@ struct notifier_block ftrace_module_exit_nb = {
>  	.priority = INT_MIN,	/* Run after anything that can remove kprobes */
>  };
> 
> +extern struct ftrace_func_finder *__start_ftrace_unsafe_rcu[];
> +extern struct ftrace_func_finder *__stop_ftrace_unsafe_rcu[];
> +
> +static struct ftrace_hash *ftrace_unsafe_rcu;
> +
> +static void __init create_unsafe_rcu_hash(void)
> +{
> +	struct ftrace_func_finder *finder;
> +	struct ftrace_func_finder **p;
> +	unsigned long ip;
> +	char str[KSYM_SYMBOL_LEN];
> +	int count;
> +
> +	count = __stop_ftrace_unsafe_rcu - __start_ftrace_unsafe_rcu;
> +	if (!count)
> +		return;
> +
> +	count = ftrace_convert_size_to_bits(count);
> +	ftrace_unsafe_rcu = alloc_ftrace_hash(count);
> +
> +	for (p = __start_ftrace_unsafe_rcu;
> +	     p < __stop_ftrace_unsafe_rcu;
> +	     p++) {
> +		finder = *p;
> +
> +		ip = ftrace_mcount_from_func(finder->ip);
> +		if (WARN_ON_ONCE(!ip))
> +			continue;
> +
> +		/* Make sure this is our ip */
> +		kallsyms_lookup(ip, NULL, NULL, NULL, str);
> +		if (WARN_ON_ONCE(strcmp(str, finder->name) != 0))
> +			continue;
> +
> +		add_hash_entry(ftrace_unsafe_rcu, ip);
> +	}
> +}
> +
>  extern unsigned long __start_mcount_loc[];
>  extern unsigned long __stop_mcount_loc[];
> 
> @@ -4250,6 +4365,8 @@ void __init ftrace_init(void)
>  	if (ret)
>  		pr_warning("Failed to register trace ftrace module exit notifier\n");
> 
> +	create_unsafe_rcu_hash();
> +
>  	set_ftrace_early_filters();
> 
>  	return;
> -- 
> 1.7.10.4
> 
> 

Re: [RFC][PATCH 01/18 v2] ftrace: Add hash list to save RCU unsafe functions

[Steven Rostedt]

On Sat, 31 Aug 2013 01:11:18 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:


> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index a6d098c..3750360 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1356,6 +1356,23 @@ ftrace_hash_rec_disable(struct ftrace_ops *ops, int filter_hash);
>  static void
>  ftrace_hash_rec_enable(struct ftrace_ops *ops, int filter_hash);
>  
> +static int ftrace_convert_size_to_bits(int size)
> +{
> +	int bits;
> +
> +	/*
> +	 * Make the hash size about 1/2 the # found
> +	 */
> +	for (size /= 2; size; size >>= 1)
> +		bits++;
> +
> +	/* Don't allocate too much */
> +	if (bits > FTRACE_HASH_MAX_BITS)
> +		bits = FTRACE_HASH_MAX_BITS;
> +
> +	return bits;
> +}
> +


Just found this bug. Strange that gcc never gave me a warning :-/

-- Steve

Re: [RFC][PATCH 01/18 v2] ftrace: Add hash list to save RCU unsafe functions

[Paul E. McKenney]

On Tue, Sep 03, 2013 at 05:15:16PM -0400, Steven Rostedt wrote:
> On Sat, 31 Aug 2013 01:11:18 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index a6d098c..3750360 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -1356,6 +1356,23 @@ ftrace_hash_rec_disable(struct ftrace_ops *ops, int filter_hash);
> >  static void
> >  ftrace_hash_rec_enable(struct ftrace_ops *ops, int filter_hash);
> >  
> > +static int ftrace_convert_size_to_bits(int size)
> > +{
> > +	int bits;
> > +
> > +	/*
> > +	 * Make the hash size about 1/2 the # found
> > +	 */
> > +	for (size /= 2; size; size >>= 1)
> > +		bits++;
> > +
> > +	/* Don't allocate too much */
> > +	if (bits > FTRACE_HASH_MAX_BITS)
> > +		bits = FTRACE_HASH_MAX_BITS;
> > +
> > +	return bits;
> > +}
> > +
> 
> Just found this bug. Strange that gcc never gave me a warning :-/

I can't give gcc too much trouble, as I also didn't give you an
uninitialized-variable warning.

							Thanx, Paul

Re: [RFC][PATCH 01/18 v2] ftrace: Add hash list to save RCU unsafe functions

[Steven Rostedt]

On Tue, 3 Sep 2013 15:18:08 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:


> > Just found this bug. Strange that gcc never gave me a warning :-/
> 
> I can't give gcc too much trouble, as I also didn't give you an
> uninitialized-variable warning.

I was also chasing down a nasty bug that looked to be a pointer
corruption somewhere. Still never found exactly where it happened, but
it always happened with the following conditions:

synchronize_sched() was in progress

The ftrace_unsafe_callback() was preempted by an interrupt

lockdep was processing a lock


A crash would happen which had memory corruption involved. But the
above seemed always to be in play.

Now, I changed the logic from disabling context level recursion to
disabling recursion at all. That is, the original code had used a
series of bits to test for recursion (via helper functions) that would
allow for the callback to be preempted by an interrupt, and then be
called again.

The new code sets a per_cpu flag, and will not allow the callback to
recurse if it were preempted by an interrupt. No real need to allow for
that anyway.

I can go and debug this further, because I'm nervous that lockdep may
have some kind of bug that the function tracer can trigger. But I'm not
too concerned about it.

Here's the diff of the new code against the previous code.

Paul, can I still keep all your acks and reviewed-bys on this?

-- Steve

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 310b727..899c8c1 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1373,7 +1373,7 @@ ftrace_hash_rec_enable(struct ftrace_ops *ops, int filter_hash);
 
 static int ftrace_convert_size_to_bits(int size)
 {
-	int bits;
+	int bits = 0;
 
 	/*
 	 * Make the hash size about 1/2 the # found
diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index f5a9031..0883069 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -561,16 +561,21 @@ static inline int init_func_cmd_traceon(void)
 
 #ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
 
+static DEFINE_PER_CPU(int, ftrace_rcu_running);
 static atomic_t ftrace_unsafe_rcu_disabled;
 
 void ftrace_unsafe_rcu_checker_disable(void)
 {
 	atomic_inc(&ftrace_unsafe_rcu_disabled);
+	/* Make sure the update is seen immediately */
+	smp_wmb();
 }
 
 void ftrace_unsafe_rcu_checker_enable(void)
 {
 	atomic_dec(&ftrace_unsafe_rcu_disabled);
+	/* Make sure the update is seen immediately */
+	smp_wmb();
 }
 
 static DEFINE_PER_CPU(unsigned long, ftrace_rcu_func);
@@ -588,15 +593,14 @@ static void
 ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
 		       struct ftrace_ops *op, struct pt_regs *pt_regs)
 {
-	int bit;
-
+	/* Make sure we see disabled or not first */
+	smp_rmb();
 	if (atomic_read(&ftrace_unsafe_rcu_disabled))
 		return;
 
 	preempt_disable_notrace();
 
-	bit = trace_test_and_set_recursion(TRACE_FTRACE_START, TRACE_FTRACE_MAX);
-	if (bit < 0)
+	if (this_cpu_read(ftrace_rcu_running))
 		goto out;
 
 	if (WARN_ONCE(ftrace_rcu_unsafe(ip),
@@ -604,13 +608,15 @@ ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
 		      (void *)ip))
 		goto out;
 
+	this_cpu_write(ftrace_rcu_running, 1);
 	this_cpu_write(ftrace_rcu_func, ip);
+
 	/* Should trigger a RCU splat if called from unsafe RCU function */
 	rcu_read_lock();
 	rcu_read_unlock();
 	this_cpu_write(ftrace_rcu_func, 0);
 
-	trace_clear_recursion(bit);
+	this_cpu_write(ftrace_rcu_running, 0);
  out:
 	preempt_enable_notrace();
 }

Re: [RFC][PATCH 01/18 v2] ftrace: Add hash list to save RCU unsafe functions

[Steven Rostedt]

On Tue, 3 Sep 2013 19:57:05 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
 
> I was also chasing down a nasty bug that looked to be a pointer
> corruption somewhere. Still never found exactly where it happened, but
> it always happened with the following conditions:
> 
> synchronize_sched() was in progress
> 
> The ftrace_unsafe_callback() was preempted by an interrupt
> 
> lockdep was processing a lock
> 
> 
> A crash would happen which had memory corruption involved. But the
> above seemed always to be in play.
> 

Now putting back the context level recursion checks, and also adding a
check for "oops_in_progress" to have the unsafe callback not call
rcu_read_lock() on oops, I can get decent backtraces.

This seems to be what I get consistently now:

[   28.583983] Testing kprobe tracing: [   28.609714] ------------[ cut here ]------------
[   28.610050] WARNING: CPU: 2 PID: 0 at /home/rostedt/work/git/linux-trace.git/kernel/lockdep.c:960 __bfs+0xe8/0x1d5()

[   28.610050] Modules linked in:
[   28.610050] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 3.11.0-rc3-test+ #220
[   28.610050] Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./To be filled by O.E.M., BIOS SDBLI944.86P 05/08/2007
[   28.610050]  00000000000003c0 ffff88007d503a18 ffffffff81548e29 0000000000000000
[   28.610050]  0000000000000000 ffff88007d503a58 ffffffff810427b1 ffffffff817fc1ba
[   28.610050]  ffffffff810944f6 0000000000000000 0000000000000002 ffffffff81093d06
[   28.610050] Call Trace:
[   28.610050]  <IRQ>  [<ffffffff81548e29>] dump_stack+0x52/0x89
[   28.610050]  [<ffffffff810427b1>] warn_slowpath_common+0x81/0x9b
[   28.610050]  [<ffffffff810944f6>] ? __bfs+0xe8/0x1d5
[   28.610050]  [<ffffffff81093d06>] ? noop_count+0xb/0xb
[   28.610050]  [<ffffffff810427e5>] warn_slowpath_null+0x1a/0x1c
[   28.610050]  [<ffffffff810944f6>] __bfs+0xe8/0x1d5
[   28.610050]  [<ffffffff81550c77>] ? sub_preempt_count+0xe/0xe0
[   28.610050]  [<ffffffff81095fa2>] ? print_irq_inversion_bug+0x1d2/0x1d2
[   28.610050]  [<ffffffff81095ff0>] check_usage_forwards+0x4e/0x87
[   28.610050]  [<ffffffff810965d0>] ? valid_state+0x2b/0x235
[   28.610050]  [<ffffffff810968d9>] mark_lock+0xff/0x1d8
[   28.610050]  [<ffffffff81096caa>] __lock_acquire+0x2f8/0xf57
[   28.610050]  [<ffffffff810df575>] ? ftrace_unsafe_callback+0x1f1/0x203
[   28.610050]  [<ffffffff810df465>] ? ftrace_unsafe_callback+0xe1/0x203
[   28.610050]  [<ffffffff81553dfc>] ? ftrace_call+0x5/0x2f
[   28.610050]  [<ffffffff810554dc>] ? __lock_task_sighand+0x9f/0xe9
[   28.610050]  [<ffffffff81097e70>] lock_acquire+0xf2/0x138
[   28.610050]  [<ffffffff810554dc>] ? __lock_task_sighand+0x9f/0xe9
[   28.610050]  [<ffffffff81053824>] ? task_pid_vnr+0xf/0xf
[   28.610050]  [<ffffffff8154cd40>] _raw_spin_lock+0x3b/0x4a
[   28.610050]  [<ffffffff810554dc>] ? __lock_task_sighand+0x9f/0xe9
[   28.610050]  [<ffffffff8105376a>] ? rcu_read_lock_held+0x37/0x3f
[   28.610050]  [<ffffffff81053824>] ? task_pid_vnr+0xf/0xf
[   28.610050]  [<ffffffff810554dc>] __lock_task_sighand+0x9f/0xe9
[   28.610050]  [<ffffffff81062e24>] ? __rcu_read_lock+0x4/0x1a
[   28.610050]  [<ffffffff81067f79>] run_posix_cpu_timers+0x141/0x4a8
[   28.610050]  [<ffffffff81062e24>] ? __rcu_read_lock+0x4/0x1a
[   28.610050]  [<ffffffff8107b97a>] ? trigger_load_balance+0x10/0x264
[   28.610050]  [<ffffffff81052aad>] update_process_times+0x6a/0x72
[   28.610050]  [<ffffffff810925b2>] ? tick_sched_handle.clone.9+0xc/0x55
[   28.610050]  [<ffffffff810925ec>] tick_sched_handle.clone.9+0x46/0x55
[   28.610050]  [<ffffffff810926d8>] tick_sched_timer+0x42/0x62
[   28.610050]  [<ffffffff81068b38>] __run_hrtimer+0xeb/0x1dc
[   28.610050]  [<ffffffff81092696>] ? tick_nohz_handler+0x9b/0x9b
[   28.610050]  [<ffffffff810693e4>] hrtimer_interrupt+0xed/0x1f3
[   28.610050]  [<ffffffff8102a160>] local_apic_timer_interrupt+0x57/0x5b
[   28.610050]  [<ffffffff8155629a>] smp_apic_timer_interrupt+0x2f/0x41
[   28.610050]  [<ffffffff81554e2f>] apic_timer_interrupt+0x6f/0x80
[   28.610050]  <EOI>  [<ffffffff810df575>] ? ftrace_unsafe_callback+0x1f1/0x203
[   28.610050]  [<ffffffff810df465>] ? ftrace_unsafe_callback+0xe1/0x203
[   28.610050]  [<ffffffff8100b2df>] ? default_idle+0x21/0x32
[   28.610050]  [<ffffffff8100b2dd>] ? default_idle+0x1f/0x32
[   28.610050]  [<ffffffff8100ba5c>] arch_cpu_idle+0x18/0x22
[   28.610050]  [<ffffffff81089b59>] cpu_startup_entry+0x109/0x16c
[   28.610050]  [<ffffffff81028ba0>] start_secondary+0x251/0x258
[   28.610050] ---[ end trace 19765ef14295ecea ]---

Oh yes, and it always happens on testing kprobes. But it happens when
that test does a synchronize_sched().

-- Steve

[RFC][PATCH 19/18] ftrace: Print a message when the rcu checker is disabled

[Steven Rostedt]

>From f8f5d278e272c42349b3cd32485faf426d0c459e Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
Date: Tue, 3 Sep 2013 20:47:59 -0400
Subject: [PATCH] ftrace: Print a message when the rcu checker is disabled

Let the user know that the RCU safety checker for function tracing
has been disabled. The checker only runs when the user specifically
configures it in the kernel, and this is done to search for locations
in the kernel that may be unsafe for a function trace callback to
use rcu_read_lock()s. But if things like function graph tracing is
started, which can live lock the machine when the checker is running,
it is disabled. It would be nice if the kernel let the user know that
the checker is disabled, and when it is re-enabled again.

As the checker only gets disabled and re-enabled by user actions
(starting and stoping the function graph tracer or the irqsoff tracer)
it is fine to have a printk display that it is disabled or not.

Suggested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_functions.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index 0883069..dd59827 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -569,6 +569,8 @@ void ftrace_unsafe_rcu_checker_disable(void)
 	atomic_inc(&ftrace_unsafe_rcu_disabled);
 	/* Make sure the update is seen immediately */
 	smp_wmb();
+	pr_info("Disabled FTRACE RCU checker (%pS)\n",
+		__builtin_return_address(0));
 }
 
 void ftrace_unsafe_rcu_checker_enable(void)
@@ -576,6 +578,8 @@ void ftrace_unsafe_rcu_checker_enable(void)
 	atomic_dec(&ftrace_unsafe_rcu_disabled);
 	/* Make sure the update is seen immediately */
 	smp_wmb();
+	pr_info("Enabled FTRACE RCU checker (%pS)\n",
+		__builtin_return_address(0));
 }
 
 static DEFINE_PER_CPU(unsigned long, ftrace_rcu_func);
-- 
1.8.1.4

Re: [RFC][PATCH 19/18] ftrace: Print a message when the rcu checker is disabled

[Paul E. McKenney]

On Tue, Sep 03, 2013 at 09:11:27PM -0400, Steven Rostedt wrote:
> From f8f5d278e272c42349b3cd32485faf426d0c459e Mon Sep 17 00:00:00 2001
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> Date: Tue, 3 Sep 2013 20:47:59 -0400
> Subject: [PATCH] ftrace: Print a message when the rcu checker is disabled
> 
> Let the user know that the RCU safety checker for function tracing
> has been disabled. The checker only runs when the user specifically
> configures it in the kernel, and this is done to search for locations
> in the kernel that may be unsafe for a function trace callback to
> use rcu_read_lock()s. But if things like function graph tracing is
> started, which can live lock the machine when the checker is running,
> it is disabled. It would be nice if the kernel let the user know that
> the checker is disabled, and when it is re-enabled again.
> 
> As the checker only gets disabled and re-enabled by user actions
> (starting and stoping the function graph tracer or the irqsoff tracer)
> it is fine to have a printk display that it is disabled or not.
> 
> Suggested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  kernel/trace/trace_functions.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> index 0883069..dd59827 100644
> --- a/kernel/trace/trace_functions.c
> +++ b/kernel/trace/trace_functions.c
> @@ -569,6 +569,8 @@ void ftrace_unsafe_rcu_checker_disable(void)
>  	atomic_inc(&ftrace_unsafe_rcu_disabled);
>  	/* Make sure the update is seen immediately */
>  	smp_wmb();
> +	pr_info("Disabled FTRACE RCU checker (%pS)\n",
> +		__builtin_return_address(0));
>  }
>  
>  void ftrace_unsafe_rcu_checker_enable(void)
> @@ -576,6 +578,8 @@ void ftrace_unsafe_rcu_checker_enable(void)
>  	atomic_dec(&ftrace_unsafe_rcu_disabled);
>  	/* Make sure the update is seen immediately */
>  	smp_wmb();
> +	pr_info("Enabled FTRACE RCU checker (%pS)\n",
> +		__builtin_return_address(0));
>  }
>  
>  static DEFINE_PER_CPU(unsigned long, ftrace_rcu_func);
> -- 
> 1.8.1.4
> 

Re: [RFC][PATCH 01/18 v2] ftrace: Add hash list to save RCU unsafe functions

[Paul E. McKenney]

On Tue, Sep 03, 2013 at 07:57:05PM -0400, Steven Rostedt wrote:
> On Tue, 3 Sep 2013 15:18:08 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> 
> > > Just found this bug. Strange that gcc never gave me a warning :-/
> > 
> > I can't give gcc too much trouble, as I also didn't give you an
> > uninitialized-variable warning.
> 
> I was also chasing down a nasty bug that looked to be a pointer
> corruption somewhere. Still never found exactly where it happened, but
> it always happened with the following conditions:
> 
> synchronize_sched() was in progress
> 
> The ftrace_unsafe_callback() was preempted by an interrupt
> 
> lockdep was processing a lock
> 
> 
> A crash would happen which had memory corruption involved. But the
> above seemed always to be in play.
> 
> Now, I changed the logic from disabling context level recursion to
> disabling recursion at all. That is, the original code had used a
> series of bits to test for recursion (via helper functions) that would
> allow for the callback to be preempted by an interrupt, and then be
> called again.
> 
> The new code sets a per_cpu flag, and will not allow the callback to
> recurse if it were preempted by an interrupt. No real need to allow for
> that anyway.
> 
> I can go and debug this further, because I'm nervous that lockdep may
> have some kind of bug that the function tracer can trigger. But I'm not
> too concerned about it.
> 
> Here's the diff of the new code against the previous code.
> 
> Paul, can I still keep all your acks and reviewed-bys on this?

Yep, but some questions below.

							Thanx, Paul

> -- Steve
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 310b727..899c8c1 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1373,7 +1373,7 @@ ftrace_hash_rec_enable(struct ftrace_ops *ops, int filter_hash);
> 
>  static int ftrace_convert_size_to_bits(int size)
>  {
> -	int bits;
> +	int bits = 0;
> 
>  	/*
>  	 * Make the hash size about 1/2 the # found
> diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> index f5a9031..0883069 100644
> --- a/kernel/trace/trace_functions.c
> +++ b/kernel/trace/trace_functions.c
> @@ -561,16 +561,21 @@ static inline int init_func_cmd_traceon(void)
> 
>  #ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> 
> +static DEFINE_PER_CPU(int, ftrace_rcu_running);
>  static atomic_t ftrace_unsafe_rcu_disabled;
> 
>  void ftrace_unsafe_rcu_checker_disable(void)
>  {
>  	atomic_inc(&ftrace_unsafe_rcu_disabled);
> +	/* Make sure the update is seen immediately */
> +	smp_wmb();

	smp_mb__after_atomic_inc()?

>  }
> 
>  void ftrace_unsafe_rcu_checker_enable(void)
>  {
>  	atomic_dec(&ftrace_unsafe_rcu_disabled);
> +	/* Make sure the update is seen immediately */
> +	smp_wmb();

	smp_mb__after_atomic_dec()?

>  }
> 
>  static DEFINE_PER_CPU(unsigned long, ftrace_rcu_func);
> @@ -588,15 +593,14 @@ static void
>  ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
>  		       struct ftrace_ops *op, struct pt_regs *pt_regs)
>  {
> -	int bit;
> -
> +	/* Make sure we see disabled or not first */
> +	smp_rmb();

	smp_mb__before_atomic_inc()?

>  	if (atomic_read(&ftrace_unsafe_rcu_disabled))
>  		return;
> 
>  	preempt_disable_notrace();
> 
> -	bit = trace_test_and_set_recursion(TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> -	if (bit < 0)
> +	if (this_cpu_read(ftrace_rcu_running))
>  		goto out;
> 
>  	if (WARN_ONCE(ftrace_rcu_unsafe(ip),
> @@ -604,13 +608,15 @@ ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
>  		      (void *)ip))
>  		goto out;
> 
> +	this_cpu_write(ftrace_rcu_running, 1);
>  	this_cpu_write(ftrace_rcu_func, ip);
> +
>  	/* Should trigger a RCU splat if called from unsafe RCU function */
>  	rcu_read_lock();
>  	rcu_read_unlock();
>  	this_cpu_write(ftrace_rcu_func, 0);
> 
> -	trace_clear_recursion(bit);
> +	this_cpu_write(ftrace_rcu_running, 0);
>   out:
>  	preempt_enable_notrace();
>  }
> 

Re: [RFC][PATCH 01/18 v2] ftrace: Add hash list to save RCU unsafe functions

[Steven Rostedt]

On Tue, 3 Sep 2013 18:24:04 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> On Tue, Sep 03, 2013 at 07:57:05PM -0400, Steven Rostedt wrote:

> > Paul, can I still keep all your acks and reviewed-bys on this?
> 
> Yep, but some questions below.
> 
> 							Thanx, Paul
> 
> > -- Steve
> > 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 310b727..899c8c1 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -1373,7 +1373,7 @@ ftrace_hash_rec_enable(struct ftrace_ops *ops, int filter_hash);
> > 
> >  static int ftrace_convert_size_to_bits(int size)
> >  {
> > -	int bits;
> > +	int bits = 0;
> > 
> >  	/*
> >  	 * Make the hash size about 1/2 the # found
> > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> > index f5a9031..0883069 100644
> > --- a/kernel/trace/trace_functions.c
> > +++ b/kernel/trace/trace_functions.c
> > @@ -561,16 +561,21 @@ static inline int init_func_cmd_traceon(void)
> > 
> >  #ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> > 
> > +static DEFINE_PER_CPU(int, ftrace_rcu_running);
> >  static atomic_t ftrace_unsafe_rcu_disabled;
> > 
> >  void ftrace_unsafe_rcu_checker_disable(void)
> >  {
> >  	atomic_inc(&ftrace_unsafe_rcu_disabled);
> > +	/* Make sure the update is seen immediately */
> > +	smp_wmb();
> 
> 	smp_mb__after_atomic_inc()?

Yep.

> 
> >  }
> > 
> >  void ftrace_unsafe_rcu_checker_enable(void)
> >  {
> >  	atomic_dec(&ftrace_unsafe_rcu_disabled);
> > +	/* Make sure the update is seen immediately */
> > +	smp_wmb();
> 
> 	smp_mb__after_atomic_dec()?

Yep.

> 
> >  }
> > 
> >  static DEFINE_PER_CPU(unsigned long, ftrace_rcu_func);
> > @@ -588,15 +593,14 @@ static void
> >  ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
> >  		       struct ftrace_ops *op, struct pt_regs *pt_regs)
> >  {
> > -	int bit;
> > -
> > +	/* Make sure we see disabled or not first */
> > +	smp_rmb();
> 
> 	smp_mb__before_atomic_inc()?

Yep.

I'll update it and restart my tests.

Thanks!

-- Steve

> 
> >  	if (atomic_read(&ftrace_unsafe_rcu_disabled))
> >  		return;
> > 
> >  	preempt_disable_notrace();
> > 
> > -	bit = trace_test_and_set_recursion(TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> > -	if (bit < 0)
> > +	if (this_cpu_read(ftrace_rcu_running))
> >  		goto out;
> > 
> >  	if (WARN_ONCE(ftrace_rcu_unsafe(ip),
> > @@ -604,13 +608,15 @@ ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
> >  		      (void *)ip))
> >  		goto out;
> > 
> > +	this_cpu_write(ftrace_rcu_running, 1);
> >  	this_cpu_write(ftrace_rcu_func, ip);
> > +
> >  	/* Should trigger a RCU splat if called from unsafe RCU function */
> >  	rcu_read_lock();
> >  	rcu_read_unlock();
> >  	this_cpu_write(ftrace_rcu_func, 0);
> > 
> > -	trace_clear_recursion(bit);
> > +	this_cpu_write(ftrace_rcu_running, 0);
> >   out:
> >  	preempt_enable_notrace();
> >  }
> > 

Re: [RFC][PATCH 01/18 v2] ftrace: Add hash list to save RCU unsafe functions

[Steven Rostedt]

On Tue, 3 Sep 2013 21:51:52 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
 
> > >  void ftrace_unsafe_rcu_checker_disable(void)
> > >  {
> > >  	atomic_inc(&ftrace_unsafe_rcu_disabled);
> > > +	/* Make sure the update is seen immediately */
> > > +	smp_wmb();
> > 
> > 	smp_mb__after_atomic_inc()?
> 
> Yep.
> 
> > 
> > >  }
> > > 
> > >  void ftrace_unsafe_rcu_checker_enable(void)
> > >  {
> > >  	atomic_dec(&ftrace_unsafe_rcu_disabled);
> > > +	/* Make sure the update is seen immediately */
> > > +	smp_wmb();
> > 
> > 	smp_mb__after_atomic_dec()?
> 
> Yep.
> 
> > 
> > >  }
> > > 
> > >  static DEFINE_PER_CPU(unsigned long, ftrace_rcu_func);
> > > @@ -588,15 +593,14 @@ static void
> > >  ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
> > >  		       struct ftrace_ops *op, struct pt_regs *pt_regs)
> > >  {
> > > -	int bit;
> > > -
> > > +	/* Make sure we see disabled or not first */
> > > +	smp_rmb();
> > 
> > 	smp_mb__before_atomic_inc()?
> 
> Yep.
> 
> I'll update it and restart my tests.
> 

Although, this will punish archs that need the mb(), as IIUC, smp_mb()
is heavier weight than smp_wmb() or smp_rmb().

-- Steve

Re: [RFC][PATCH 01/18 v2] ftrace: Add hash list to save RCU unsafe functions

[Steven Rostedt]

On Tue, 3 Sep 2013 18:24:04 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:


> >  static DEFINE_PER_CPU(unsigned long, ftrace_rcu_func);
> > @@ -588,15 +593,14 @@ static void
> >  ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
> >  		       struct ftrace_ops *op, struct pt_regs *pt_regs)
> >  {
> > -	int bit;
> > -
> > +	/* Make sure we see disabled or not first */
> > +	smp_rmb();
> 
> 	smp_mb__before_atomic_inc()?
> 

Ah, but this is before an atomic_read(), and not an atomic_inc(), thus
the normal smp_rmb() is still required.

-- Steve

> >  	if (atomic_read(&ftrace_unsafe_rcu_disabled))
> >  		return;
> > 
> >  	preempt_disable_notrace();
> > 

Re: [RFC][PATCH 01/18 v2] ftrace: Add hash list to save RCU unsafe functions

[Steven Rostedt]

On Tue, 3 Sep 2013 22:01:15 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 3 Sep 2013 18:24:04 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> 
> > >  static DEFINE_PER_CPU(unsigned long, ftrace_rcu_func);
> > > @@ -588,15 +593,14 @@ static void
> > >  ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
> > >  		       struct ftrace_ops *op, struct pt_regs *pt_regs)
> > >  {
> > > -	int bit;
> > > -
> > > +	/* Make sure we see disabled or not first */
> > > +	smp_rmb();
> > 
> > 	smp_mb__before_atomic_inc()?
> > 
> 
> Ah, but this is before an atomic_read(), and not an atomic_inc(), thus
> the normal smp_rmb() is still required.
> 

Here's the changes against this one: 

diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index cdcf187..9e6902a 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -569,14 +569,14 @@ void ftrace_unsafe_rcu_checker_disable(void)
 {
 	atomic_inc(&ftrace_unsafe_rcu_disabled);
 	/* Make sure the update is seen immediately */
-	smp_wmb();
+	smp_mb__after_atomic_inc();
 }
 
 void ftrace_unsafe_rcu_checker_enable(void)
 {
 	atomic_dec(&ftrace_unsafe_rcu_disabled);
 	/* Make sure the update is seen immediately */
-	smp_wmb();
+	smp_mb__after_atomic_dec();
 }
 
 static void



Which is nice, because the smp_mb() are now in the really slow path.

-- Steve

Re: [RFC][PATCH 01/18 v2] ftrace: Add hash list to save RCU unsafe functions

[Paul E. McKenney]

On Tue, Sep 03, 2013 at 10:03:25PM -0400, Steven Rostedt wrote:
> On Tue, 3 Sep 2013 22:01:15 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Tue, 3 Sep 2013 18:24:04 -0700
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > 
> > 
> > > >  static DEFINE_PER_CPU(unsigned long, ftrace_rcu_func);
> > > > @@ -588,15 +593,14 @@ static void
> > > >  ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
> > > >  		       struct ftrace_ops *op, struct pt_regs *pt_regs)
> > > >  {
> > > > -	int bit;
> > > > -
> > > > +	/* Make sure we see disabled or not first */
> > > > +	smp_rmb();
> > > 
> > > 	smp_mb__before_atomic_inc()?
> > > 
> > 
> > Ah, but this is before an atomic_read(), and not an atomic_inc(), thus
> > the normal smp_rmb() is still required.
> > 
> 
> Here's the changes against this one: 
> 
> diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> index cdcf187..9e6902a 100644
> --- a/kernel/trace/trace_functions.c
> +++ b/kernel/trace/trace_functions.c
> @@ -569,14 +569,14 @@ void ftrace_unsafe_rcu_checker_disable(void)
>  {
>  	atomic_inc(&ftrace_unsafe_rcu_disabled);
>  	/* Make sure the update is seen immediately */
> -	smp_wmb();
> +	smp_mb__after_atomic_inc();
>  }
> 
>  void ftrace_unsafe_rcu_checker_enable(void)
>  {
>  	atomic_dec(&ftrace_unsafe_rcu_disabled);
>  	/* Make sure the update is seen immediately */
> -	smp_wmb();
> +	smp_mb__after_atomic_dec();
>  }
> 
>  static void
> 
> 
> 
> Which is nice, because the smp_mb() are now in the really slow path.

Looks good!

But now that I look at it more carefully, including the comments...
The smp_mb__after_atomic_dec() isn't going to make the update be seen
faster -- instead, it will guarantee that if some other CPU sees this
CPU's later write, then that CPU will also see the results of the
atomic_dec().

							Thanx, Paul

Re: [RFC][PATCH 01/18 v2] ftrace: Add hash list to save RCU unsafe functions

[Steven Rostedt]

On Tue, 3 Sep 2013 21:18:28 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> On Tue, Sep 03, 2013 at 10:03:25PM -0400, Steven Rostedt wrote:
> > On Tue, 3 Sep 2013 22:01:15 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > On Tue, 3 Sep 2013 18:24:04 -0700
> > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > 
> > > 
> > > > >  static DEFINE_PER_CPU(unsigned long, ftrace_rcu_func);
> > > > > @@ -588,15 +593,14 @@ static void
> > > > >  ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
> > > > >  		       struct ftrace_ops *op, struct pt_regs *pt_regs)
> > > > >  {
> > > > > -	int bit;
> > > > > -
> > > > > +	/* Make sure we see disabled or not first */
> > > > > +	smp_rmb();
> > > > 
> > > > 	smp_mb__before_atomic_inc()?
> > > > 
> > > 
> > > Ah, but this is before an atomic_read(), and not an atomic_inc(), thus
> > > the normal smp_rmb() is still required.
> > > 
> > 
> > Here's the changes against this one: 
> > 
> > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> > index cdcf187..9e6902a 100644
> > --- a/kernel/trace/trace_functions.c
> > +++ b/kernel/trace/trace_functions.c
> > @@ -569,14 +569,14 @@ void ftrace_unsafe_rcu_checker_disable(void)
> >  {
> >  	atomic_inc(&ftrace_unsafe_rcu_disabled);
> >  	/* Make sure the update is seen immediately */
> > -	smp_wmb();
> > +	smp_mb__after_atomic_inc();
> >  }
> > 
> >  void ftrace_unsafe_rcu_checker_enable(void)
> >  {
> >  	atomic_dec(&ftrace_unsafe_rcu_disabled);
> >  	/* Make sure the update is seen immediately */
> > -	smp_wmb();
> > +	smp_mb__after_atomic_dec();
> >  }
> > 
> >  static void
> > 
> > 
> > 
> > Which is nice, because the smp_mb() are now in the really slow path.
> 
> Looks good!
> 
> But now that I look at it more carefully, including the comments...
> The smp_mb__after_atomic_dec() isn't going to make the update be seen
> faster -- instead, it will guarantee that if some other CPU sees this
> CPU's later write, then that CPU will also see the results of the
> atomic_dec().

I don't need to have it seen "faster", just before anything that comes
next after the call to ftrace_unsafe_rcu_checker_disable(). That's
what I meant by the comment. In other words, don't delay this write, it
needs to go first.

-- Steve

Re: [RFC][PATCH 01/18 v2] ftrace: Add hash list to save RCU unsafe functions

[Paul E. McKenney]

On Tue, Sep 03, 2013 at 10:01:15PM -0400, Steven Rostedt wrote:
> On Tue, 3 Sep 2013 18:24:04 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> 
> > >  static DEFINE_PER_CPU(unsigned long, ftrace_rcu_func);
> > > @@ -588,15 +593,14 @@ static void
> > >  ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
> > >  		       struct ftrace_ops *op, struct pt_regs *pt_regs)
> > >  {
> > > -	int bit;
> > > -
> > > +	/* Make sure we see disabled or not first */
> > > +	smp_rmb();
> > 
> > 	smp_mb__before_atomic_inc()?
> > 
> 
> Ah, but this is before an atomic_read(), and not an atomic_inc(), thus
> the normal smp_rmb() is still required.

Good point, color me blind, dazed, and confused.

							Thanx, Paul

> -- Steve
> 
> > >  	if (atomic_read(&ftrace_unsafe_rcu_disabled))
> > >  		return;
> > > 
> > >  	preempt_disable_notrace();
> > > 
> 

[RFC][PATCH 02/18 v2] ftrace: Do not set ftrace records for unsafe RCU when not allowed

[Steven Rostedt]

[-- Attachment #1: 0002-ftrace-Do-not-set-ftrace-records-for-unsafe-RCU-when.patch --]
[-- Type: text/plain, Size: 2614 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

For the ftrace_ops that use RCU read locks, and can not be called by
unsafe RCU functions (those outside of RCU tracking), have them not
update the RCU unsafe function records when they are being registered
or unregistered.

The ftrace function records store a counter of all the ftrace_ops
callbacks that are hooked to the function the record represents.
As unsafe RCU functions do not call callbacks that do not specify
that they do not use RCU, do not update those records.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 3750360..d61f431 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1168,6 +1168,12 @@ static struct ftrace_page *ftrace_new_pgs;
 static struct ftrace_page	*ftrace_pages_start;
 static struct ftrace_page	*ftrace_pages;
 
+/*
+ * Hash of functions that are not safe to be called by
+ * callbacks that use RCU read locks.
+ */
+static struct ftrace_hash *ftrace_unsafe_rcu;
+
 static bool ftrace_hash_empty(struct ftrace_hash *hash)
 {
 	return !hash || !hash->count;
@@ -1638,6 +1644,7 @@ static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
 {
 	struct ftrace_hash *hash;
 	struct ftrace_hash *other_hash;
+	struct ftrace_hash *rcu_hash;
 	struct ftrace_page *pg;
 	struct dyn_ftrace *rec;
 	int count = 0;
@@ -1647,6 +1654,12 @@ static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
 	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
 		return;
 
+	/* Ignore rcu unsafe functions unless ops handles them */
+	if (ops->flags & FTRACE_OPS_FL_RCU_SAFE)
+		rcu_hash = NULL;
+	else
+		rcu_hash = ftrace_unsafe_rcu;
+
 	/*
 	 * In the filter_hash case:
 	 *   If the count is zero, we update all records.
@@ -1680,6 +1693,10 @@ static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
 		int in_hash = 0;
 		int match = 0;
 
+		/* Ignore all in the rcu unsafe hash */
+		if (ftrace_lookup_ip(rcu_hash, rec->ip))
+			continue;
+
 		if (all) {
 			/*
 			 * Only the filter_hash affects all records.
@@ -4291,8 +4308,6 @@ struct notifier_block ftrace_module_exit_nb = {
 extern struct ftrace_func_finder *__start_ftrace_unsafe_rcu[];
 extern struct ftrace_func_finder *__stop_ftrace_unsafe_rcu[];
 
-static struct ftrace_hash *ftrace_unsafe_rcu;
-
 static void __init create_unsafe_rcu_hash(void)
 {
 	struct ftrace_func_finder *finder;
-- 
1.7.10.4

Re: [RFC][PATCH 02/18 v2] ftrace: Do not set ftrace records for unsafe RCU when not allowed

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:19AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> For the ftrace_ops that use RCU read locks, and can not be called by
> unsafe RCU functions (those outside of RCU tracking), have them not
> update the RCU unsafe function records when they are being registered
> or unregistered.
> 
> The ftrace function records store a counter of all the ftrace_ops
> callbacks that are hooked to the function the record represents.
> As unsafe RCU functions do not call callbacks that do not specify
> that they do not use RCU, do not update those records.
> 
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  kernel/trace/ftrace.c |   19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 3750360..d61f431 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1168,6 +1168,12 @@ static struct ftrace_page *ftrace_new_pgs;
>  static struct ftrace_page	*ftrace_pages_start;
>  static struct ftrace_page	*ftrace_pages;
> 
> +/*
> + * Hash of functions that are not safe to be called by
> + * callbacks that use RCU read locks.
> + */
> +static struct ftrace_hash *ftrace_unsafe_rcu;
> +
>  static bool ftrace_hash_empty(struct ftrace_hash *hash)
>  {
>  	return !hash || !hash->count;
> @@ -1638,6 +1644,7 @@ static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
>  {
>  	struct ftrace_hash *hash;
>  	struct ftrace_hash *other_hash;
> +	struct ftrace_hash *rcu_hash;
>  	struct ftrace_page *pg;
>  	struct dyn_ftrace *rec;
>  	int count = 0;
> @@ -1647,6 +1654,12 @@ static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
>  	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
>  		return;
> 
> +	/* Ignore rcu unsafe functions unless ops handles them */
> +	if (ops->flags & FTRACE_OPS_FL_RCU_SAFE)
> +		rcu_hash = NULL;
> +	else
> +		rcu_hash = ftrace_unsafe_rcu;
> +
>  	/*
>  	 * In the filter_hash case:
>  	 *   If the count is zero, we update all records.
> @@ -1680,6 +1693,10 @@ static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
>  		int in_hash = 0;
>  		int match = 0;
> 
> +		/* Ignore all in the rcu unsafe hash */
> +		if (ftrace_lookup_ip(rcu_hash, rec->ip))
> +			continue;
> +
>  		if (all) {
>  			/*
>  			 * Only the filter_hash affects all records.
> @@ -4291,8 +4308,6 @@ struct notifier_block ftrace_module_exit_nb = {
>  extern struct ftrace_func_finder *__start_ftrace_unsafe_rcu[];
>  extern struct ftrace_func_finder *__stop_ftrace_unsafe_rcu[];
> 
> -static struct ftrace_hash *ftrace_unsafe_rcu;
> -
>  static void __init create_unsafe_rcu_hash(void)
>  {
>  	struct ftrace_func_finder *finder;
> -- 
> 1.7.10.4
> 
> 

[RFC][PATCH 03/18 v2] ftrace: Set ftrace internal function tracing RCU safe

[Steven Rostedt]

[-- Attachment #1: 0003-ftrace-Set-ftrace-internal-function-tracing-RCU-safe.patch --]
[-- Type: text/plain, Size: 957 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Since none of the internal ftrace function tracing uses RCU in
their callbacks, it is OK to set the global_ops (the one that
they all use) to RCU safe.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index d61f431..a45deaa 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1146,7 +1146,9 @@ static struct ftrace_ops global_ops = {
 	.func			= ftrace_stub,
 	.notrace_hash		= EMPTY_HASH,
 	.filter_hash		= EMPTY_HASH,
-	.flags			= FTRACE_OPS_FL_RECURSION_SAFE | FTRACE_OPS_FL_INITIALIZED,
+	.flags			= FTRACE_OPS_FL_RECURSION_SAFE |
+				  FTRACE_OPS_FL_INITIALIZED |
+				  FTRACE_OPS_FL_RCU_SAFE,
 	INIT_REGEX_LOCK(global_ops)
 };
 
-- 
1.7.10.4

Re: [RFC][PATCH 03/18 v2] ftrace: Set ftrace internal function tracing RCU safe

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:20AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Since none of the internal ftrace function tracing uses RCU in
> their callbacks, it is OK to set the global_ops (the one that
> they all use) to RCU safe.
> 
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  kernel/trace/ftrace.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index d61f431..a45deaa 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1146,7 +1146,9 @@ static struct ftrace_ops global_ops = {
>  	.func			= ftrace_stub,
>  	.notrace_hash		= EMPTY_HASH,
>  	.filter_hash		= EMPTY_HASH,
> -	.flags			= FTRACE_OPS_FL_RECURSION_SAFE | FTRACE_OPS_FL_INITIALIZED,
> +	.flags			= FTRACE_OPS_FL_RECURSION_SAFE |
> +				  FTRACE_OPS_FL_INITIALIZED |
> +				  FTRACE_OPS_FL_RCU_SAFE,
>  	INIT_REGEX_LOCK(global_ops)
>  };
> 
> -- 
> 1.7.10.4
> 
> 

Re: [RFC][PATCH 03/18 v2] ftrace: Set ftrace internal function tracing RCU safe

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:20AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Since none of the internal ftrace function tracing uses RCU in
> their callbacks, it is OK to set the global_ops (the one that
> they all use) to RCU safe.
> 
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  kernel/trace/ftrace.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index d61f431..a45deaa 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1146,7 +1146,9 @@ static struct ftrace_ops global_ops = {
>  	.func			= ftrace_stub,
>  	.notrace_hash		= EMPTY_HASH,
>  	.filter_hash		= EMPTY_HASH,
> -	.flags			= FTRACE_OPS_FL_RECURSION_SAFE | FTRACE_OPS_FL_INITIALIZED,
> +	.flags			= FTRACE_OPS_FL_RECURSION_SAFE |
> +				  FTRACE_OPS_FL_INITIALIZED |
> +				  FTRACE_OPS_FL_RCU_SAFE,
>  	INIT_REGEX_LOCK(global_ops)
>  };
> 
> -- 
> 1.7.10.4
> 
> 

Re: [RFC][PATCH 03/18 v2] ftrace: Set ftrace internal function tracing RCU safe

[Steven Rostedt]

On Sat, 31 Aug 2013 12:44:31 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> On Sat, Aug 31, 2013 at 01:11:20AM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > 
> > Since none of the internal ftrace function tracing uses RCU in
> > their callbacks, it is OK to set the global_ops (the one that
> > they all use) to RCU safe.
> > 
> > Cc: Jiri Olsa <jolsa@redhat.com>
> > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

You must have liked this patch so much... that you acked it twice ;-)

-- Steve

> 
> > ---
> >  kernel/trace/ftrace.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index d61f431..a45deaa 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -1146,7 +1146,9 @@ static struct ftrace_ops global_ops = {
> >  	.func			= ftrace_stub,
> >  	.notrace_hash		= EMPTY_HASH,
> >  	.filter_hash		= EMPTY_HASH,
> > -	.flags			= FTRACE_OPS_FL_RECURSION_SAFE | FTRACE_OPS_FL_INITIALIZED,
> > +	.flags			= FTRACE_OPS_FL_RECURSION_SAFE |
> > +				  FTRACE_OPS_FL_INITIALIZED |
> > +				  FTRACE_OPS_FL_RCU_SAFE,
> >  	INIT_REGEX_LOCK(global_ops)
> >  };
> > 
> > -- 
> > 1.7.10.4
> > 
> > 

Re: [RFC][PATCH 03/18 v2] ftrace: Set ftrace internal function tracing RCU safe

[Paul E. McKenney]

On Tue, Sep 03, 2013 at 09:22:24AM -0400, Steven Rostedt wrote:
> On Sat, 31 Aug 2013 12:44:31 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > On Sat, Aug 31, 2013 at 01:11:20AM -0400, Steven Rostedt wrote:
> > > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > > 
> > > Since none of the internal ftrace function tracing uses RCU in
> > > their callbacks, it is OK to set the global_ops (the one that
> > > they all use) to RCU safe.
> > > 
> > > Cc: Jiri Olsa <jolsa@redhat.com>
> > > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > 
> > Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> You must have liked this patch so much... that you acked it twice ;-)

Better safe than sorry?  No, that doesn't sound quite right...

							Thanx, Paul

> -- Steve
> 
> > 
> > > ---
> > >  kernel/trace/ftrace.c |    4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > > index d61f431..a45deaa 100644
> > > --- a/kernel/trace/ftrace.c
> > > +++ b/kernel/trace/ftrace.c
> > > @@ -1146,7 +1146,9 @@ static struct ftrace_ops global_ops = {
> > >  	.func			= ftrace_stub,
> > >  	.notrace_hash		= EMPTY_HASH,
> > >  	.filter_hash		= EMPTY_HASH,
> > > -	.flags			= FTRACE_OPS_FL_RECURSION_SAFE | FTRACE_OPS_FL_INITIALIZED,
> > > +	.flags			= FTRACE_OPS_FL_RECURSION_SAFE |
> > > +				  FTRACE_OPS_FL_INITIALIZED |
> > > +				  FTRACE_OPS_FL_RCU_SAFE,
> > >  	INIT_REGEX_LOCK(global_ops)
> > >  };
> > > 
> > > -- 
> > > 1.7.10.4
> > > 
> > > 
> 

[RFC][PATCH 04/18 v2] ftrace: Add test for ops against unsafe RCU functions in callback

[Steven Rostedt]

[-- Attachment #1: 0004-ftrace-Add-test-for-ops-against-unsafe-RCU-functions.patch --]
[-- Type: text/plain, Size: 1640 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

When more than one ftrace_ops is registered, the list function is
is used to call all registered functions. It uses the filter and
notrace hashes from the ftrace_ops to determine if the corresponding
callback should be called or not.

Currently, it does not take into account for RCU unsafe functions.
If multiple functions are registered, and an RCU safe callback is
used on a RCU unsafe function, and the RCU unsafe callback says to
trace all functions, it will end up tracing this RCU unsafe function
and still suffer the problems when using RCU when RCU tracing is
off.

Add a test to the multiple ops list function to test if the ops in
question can use an RCU unsafe function or not, and if the function
being traced happens to be an RCU unsafe function.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a45deaa..06504b2 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1480,7 +1480,9 @@ ftrace_ops_test(struct ftrace_ops *ops, unsigned long ip, void *regs)
 	if ((ftrace_hash_empty(filter_hash) ||
 	     ftrace_lookup_ip(filter_hash, ip)) &&
 	    (ftrace_hash_empty(notrace_hash) ||
-	     !ftrace_lookup_ip(notrace_hash, ip)))
+	     !ftrace_lookup_ip(notrace_hash, ip)) &&
+	    (ops->flags & FTRACE_OPS_FL_RCU_SAFE ||
+	     !ftrace_lookup_ip(ftrace_unsafe_rcu, ip)))
 		ret = 1;
 	else
 		ret = 0;
-- 
1.7.10.4

Re: [RFC][PATCH 04/18 v2] ftrace: Add test for ops against unsafe RCU functions in callback

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:21AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> When more than one ftrace_ops is registered, the list function is
> is used to call all registered functions. It uses the filter and
> notrace hashes from the ftrace_ops to determine if the corresponding
> callback should be called or not.
> 
> Currently, it does not take into account for RCU unsafe functions.
> If multiple functions are registered, and an RCU safe callback is
> used on a RCU unsafe function, and the RCU unsafe callback says to
> trace all functions, it will end up tracing this RCU unsafe function
> and still suffer the problems when using RCU when RCU tracing is
> off.
> 
> Add a test to the multiple ops list function to test if the ops in
> question can use an RCU unsafe function or not, and if the function
> being traced happens to be an RCU unsafe function.
> 
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  kernel/trace/ftrace.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index a45deaa..06504b2 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1480,7 +1480,9 @@ ftrace_ops_test(struct ftrace_ops *ops, unsigned long ip, void *regs)
>  	if ((ftrace_hash_empty(filter_hash) ||
>  	     ftrace_lookup_ip(filter_hash, ip)) &&
>  	    (ftrace_hash_empty(notrace_hash) ||
> -	     !ftrace_lookup_ip(notrace_hash, ip)))
> +	     !ftrace_lookup_ip(notrace_hash, ip)) &&
> +	    (ops->flags & FTRACE_OPS_FL_RCU_SAFE ||
> +	     !ftrace_lookup_ip(ftrace_unsafe_rcu, ip)))
>  		ret = 1;
>  	else
>  		ret = 0;
> -- 
> 1.7.10.4
> 
> 

[RFC][PATCH 05/18 v2] ftrace: Do not display non safe RCU functions in available_filter_functions

[Steven Rostedt]

[-- Attachment #1: 0005-ftrace-Do-not-display-non-safe-RCU-functions-in-avai.patch --]
[-- Type: text/plain, Size: 1775 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

As available_filter_functions file displays functions that are generally
available for tracing, do not show the ones that are RCU unsafe. Otherwise
it may be confusing for perf users to see these functions in this file but
not be able to trace them.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace.h |    1 +
 kernel/trace/ftrace.c  |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 1d17a82..4709264 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -411,6 +411,7 @@ enum {
 	FTRACE_ITER_DO_HASH	= (1 << 3),
 	FTRACE_ITER_HASH	= (1 << 4),
 	FTRACE_ITER_ENABLED	= (1 << 5),
+	FTRACE_ITER_NO_UNSAFE	= (1 << 6),
 };
 
 void arch_ftrace_update_code(int command);
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 06504b2..be87ac9 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2645,7 +2645,10 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
 		     !ftrace_lookup_ip(ops->notrace_hash, rec->ip)) ||
 
 		    ((iter->flags & FTRACE_ITER_ENABLED) &&
-		     !(rec->flags & FTRACE_FL_ENABLED))) {
+		     !(rec->flags & FTRACE_FL_ENABLED)) ||
+
+		    ((iter->flags & FTRACE_ITER_NO_UNSAFE) &&
+		     ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip))) {
 
 			rec = NULL;
 			goto retry;
@@ -2773,6 +2776,7 @@ ftrace_avail_open(struct inode *inode, struct file *file)
 	iter = __seq_open_private(file, &show_ftrace_seq_ops, sizeof(*iter));
 	if (iter) {
 		iter->pg = ftrace_pages_start;
+		iter->flags = FTRACE_ITER_NO_UNSAFE;
 		iter->ops = &global_ops;
 	}
 
-- 
1.7.10.4

Re: [RFC][PATCH 05/18 v2] ftrace: Do not display non safe RCU functions in available_filter_functions

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:22AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> As available_filter_functions file displays functions that are generally
> available for tracing, do not show the ones that are RCU unsafe. Otherwise
> it may be confusing for perf users to see these functions in this file but
> not be able to trace them.
> 
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  include/linux/ftrace.h |    1 +
>  kernel/trace/ftrace.c  |    6 +++++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 1d17a82..4709264 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -411,6 +411,7 @@ enum {
>  	FTRACE_ITER_DO_HASH	= (1 << 3),
>  	FTRACE_ITER_HASH	= (1 << 4),
>  	FTRACE_ITER_ENABLED	= (1 << 5),
> +	FTRACE_ITER_NO_UNSAFE	= (1 << 6),
>  };
> 
>  void arch_ftrace_update_code(int command);
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 06504b2..be87ac9 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2645,7 +2645,10 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
>  		     !ftrace_lookup_ip(ops->notrace_hash, rec->ip)) ||
> 
>  		    ((iter->flags & FTRACE_ITER_ENABLED) &&
> -		     !(rec->flags & FTRACE_FL_ENABLED))) {
> +		     !(rec->flags & FTRACE_FL_ENABLED)) ||
> +

OK, I'll bite...  Why the blank line?

							Thanx, Paul

> +		    ((iter->flags & FTRACE_ITER_NO_UNSAFE) &&
> +		     ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip))) {
> 
>  			rec = NULL;
>  			goto retry;
> @@ -2773,6 +2776,7 @@ ftrace_avail_open(struct inode *inode, struct file *file)
>  	iter = __seq_open_private(file, &show_ftrace_seq_ops, sizeof(*iter));
>  	if (iter) {
>  		iter->pg = ftrace_pages_start;
> +		iter->flags = FTRACE_ITER_NO_UNSAFE;
>  		iter->ops = &global_ops;
>  	}
> 
> -- 
> 1.7.10.4
> 
> 

Re: [RFC][PATCH 05/18 v2] ftrace: Do not display non safe RCU functions in available_filter_functions

[Steven Rostedt]

On Sat, 2013-08-31 at 12:46 -0700, Paul E. McKenney wrote:

> >  void arch_ftrace_update_code(int command);
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 06504b2..be87ac9 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -2645,7 +2645,10 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
> >  		     !ftrace_lookup_ip(ops->notrace_hash, rec->ip)) ||
> > 
> >  		    ((iter->flags & FTRACE_ITER_ENABLED) &&
> > -		     !(rec->flags & FTRACE_FL_ENABLED))) {
> > +		     !(rec->flags & FTRACE_FL_ENABLED)) ||
> > +
> 
> OK, I'll bite...  Why the blank line?

For readability. Here's the full if statement:

                if (((iter->flags & FTRACE_ITER_FILTER) &&
                     !(ftrace_lookup_ip(ops->filter_hash, rec->ip))) ||

                    ((iter->flags & FTRACE_ITER_NOTRACE) &&
                     !ftrace_lookup_ip(ops->notrace_hash, rec->ip)) ||

                    ((iter->flags & FTRACE_ITER_ENABLED) &&
                     !(rec->flags & FTRACE_FL_ENABLED)) ||

                    ((iter->flags & FTRACE_ITER_UNSAFE_ONLY) &&
                     !ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip)) ||

                    ((iter->flags & FTRACE_ITER_NO_UNSAFE) &&
                     ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip))) {


They are grouped in pairs.

-- Steve

> 
> 							Thanx, Paul
> 
> > +		    ((iter->flags & FTRACE_ITER_NO_UNSAFE) &&
> > +		     ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip))) {
> > 
> >  			rec = NULL;
> >  			goto retry;
> > @@ -2773,6 +2776,7 @@ ftrace_avail_open(struct inode *inode, struct file *file)
> >  	iter = __seq_open_private(file, &show_ftrace_seq_ops, sizeof(*iter));
> >  	if (iter) {
> >  		iter->pg = ftrace_pages_start;
> > +		iter->flags = FTRACE_ITER_NO_UNSAFE;
> >  		iter->ops = &global_ops;
> >  	}
> > 
> > -- 
> > 1.7.10.4
> > 
> > 

Re: [RFC][PATCH 05/18 v2] ftrace: Do not display non safe RCU functions in available_filter_functions

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 04:35:03PM -0400, Steven Rostedt wrote:
> On Sat, 2013-08-31 at 12:46 -0700, Paul E. McKenney wrote:
> 
> > >  void arch_ftrace_update_code(int command);
> > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > > index 06504b2..be87ac9 100644
> > > --- a/kernel/trace/ftrace.c
> > > +++ b/kernel/trace/ftrace.c
> > > @@ -2645,7 +2645,10 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
> > >  		     !ftrace_lookup_ip(ops->notrace_hash, rec->ip)) ||
> > > 
> > >  		    ((iter->flags & FTRACE_ITER_ENABLED) &&
> > > -		     !(rec->flags & FTRACE_FL_ENABLED))) {
> > > +		     !(rec->flags & FTRACE_FL_ENABLED)) ||
> > > +
> > 
> > OK, I'll bite...  Why the blank line?
> 
> For readability. Here's the full if statement:
> 
>                 if (((iter->flags & FTRACE_ITER_FILTER) &&
>                      !(ftrace_lookup_ip(ops->filter_hash, rec->ip))) ||
> 
>                     ((iter->flags & FTRACE_ITER_NOTRACE) &&
>                      !ftrace_lookup_ip(ops->notrace_hash, rec->ip)) ||
> 
>                     ((iter->flags & FTRACE_ITER_ENABLED) &&
>                      !(rec->flags & FTRACE_FL_ENABLED)) ||
> 
>                     ((iter->flags & FTRACE_ITER_UNSAFE_ONLY) &&
>                      !ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip)) ||
> 
>                     ((iter->flags & FTRACE_ITER_NO_UNSAFE) &&
>                      ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip))) {
> 
> 
> They are grouped in pairs.

Fair enough!

							Thanx, Paul

> -- Steve
> 
> > 
> > 							Thanx, Paul
> > 
> > > +		    ((iter->flags & FTRACE_ITER_NO_UNSAFE) &&
> > > +		     ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip))) {
> > > 
> > >  			rec = NULL;
> > >  			goto retry;
> > > @@ -2773,6 +2776,7 @@ ftrace_avail_open(struct inode *inode, struct file *file)
> > >  	iter = __seq_open_private(file, &show_ftrace_seq_ops, sizeof(*iter));
> > >  	if (iter) {
> > >  		iter->pg = ftrace_pages_start;
> > > +		iter->flags = FTRACE_ITER_NO_UNSAFE;
> > >  		iter->ops = &global_ops;
> > >  	}
> > > 
> > > -- 
> > > 1.7.10.4
> > > 
> > > 
> 
> 

Re: [RFC][PATCH 05/18 v2] ftrace: Do not display non safe RCU functions in available_filter_functions

[Steven Rostedt]

On Sat, 31 Aug 2013 13:54:30 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> On Sat, Aug 31, 2013 at 04:35:03PM -0400, Steven Rostedt wrote:
> > On Sat, 2013-08-31 at 12:46 -0700, Paul E. McKenney wrote:
> > 
> > > >  void arch_ftrace_update_code(int command);
> > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > > > index 06504b2..be87ac9 100644
> > > > --- a/kernel/trace/ftrace.c
> > > > +++ b/kernel/trace/ftrace.c
> > > > @@ -2645,7 +2645,10 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
> > > >  		     !ftrace_lookup_ip(ops->notrace_hash, rec->ip)) ||
> > > > 
> > > >  		    ((iter->flags & FTRACE_ITER_ENABLED) &&
> > > > -		     !(rec->flags & FTRACE_FL_ENABLED))) {
> > > > +		     !(rec->flags & FTRACE_FL_ENABLED)) ||
> > > > +
> > > 
> > > OK, I'll bite...  Why the blank line?
> > 
> > For readability. Here's the full if statement:
> > 
> >                 if (((iter->flags & FTRACE_ITER_FILTER) &&
> >                      !(ftrace_lookup_ip(ops->filter_hash, rec->ip))) ||
> > 
> >                     ((iter->flags & FTRACE_ITER_NOTRACE) &&
> >                      !ftrace_lookup_ip(ops->notrace_hash, rec->ip)) ||
> > 
> >                     ((iter->flags & FTRACE_ITER_ENABLED) &&
> >                      !(rec->flags & FTRACE_FL_ENABLED)) ||
> > 
> >                     ((iter->flags & FTRACE_ITER_UNSAFE_ONLY) &&
> >                      !ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip)) ||
> > 
> >                     ((iter->flags & FTRACE_ITER_NO_UNSAFE) &&
> >                      ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip))) {
> > 
> > 
> > They are grouped in pairs.
> 
> Fair enough!

Can I take this as an Acked-by?

-- Steve

> 
> 							Thanx, Paul
> 
> > -- Steve
> > 
> > > 
> > > 							Thanx, Paul
> > > 
> > > > +		    ((iter->flags & FTRACE_ITER_NO_UNSAFE) &&
> > > > +		     ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip))) {
> > > > 
> > > >  			rec = NULL;
> > > >  			goto retry;
> > > > @@ -2773,6 +2776,7 @@ ftrace_avail_open(struct inode *inode, struct file *file)
> > > >  	iter = __seq_open_private(file, &show_ftrace_seq_ops, sizeof(*iter));
> > > >  	if (iter) {
> > > >  		iter->pg = ftrace_pages_start;
> > > > +		iter->flags = FTRACE_ITER_NO_UNSAFE;
> > > >  		iter->ops = &global_ops;
> > > >  	}
> > > > 
> > > > -- 
> > > > 1.7.10.4
> > > > 
> > > > 
> > 
> > 

Re: [RFC][PATCH 05/18 v2] ftrace: Do not display non safe RCU functions in available_filter_functions

[Paul E. McKenney]

On Tue, Sep 03, 2013 at 09:26:15AM -0400, Steven Rostedt wrote:
> On Sat, 31 Aug 2013 13:54:30 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > On Sat, Aug 31, 2013 at 04:35:03PM -0400, Steven Rostedt wrote:
> > > On Sat, 2013-08-31 at 12:46 -0700, Paul E. McKenney wrote:
> > > 
> > > > >  void arch_ftrace_update_code(int command);
> > > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > > > > index 06504b2..be87ac9 100644
> > > > > --- a/kernel/trace/ftrace.c
> > > > > +++ b/kernel/trace/ftrace.c
> > > > > @@ -2645,7 +2645,10 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
> > > > >  		     !ftrace_lookup_ip(ops->notrace_hash, rec->ip)) ||
> > > > > 
> > > > >  		    ((iter->flags & FTRACE_ITER_ENABLED) &&
> > > > > -		     !(rec->flags & FTRACE_FL_ENABLED))) {
> > > > > +		     !(rec->flags & FTRACE_FL_ENABLED)) ||
> > > > > +
> > > > 
> > > > OK, I'll bite...  Why the blank line?
> > > 
> > > For readability. Here's the full if statement:
> > > 
> > >                 if (((iter->flags & FTRACE_ITER_FILTER) &&
> > >                      !(ftrace_lookup_ip(ops->filter_hash, rec->ip))) ||
> > > 
> > >                     ((iter->flags & FTRACE_ITER_NOTRACE) &&
> > >                      !ftrace_lookup_ip(ops->notrace_hash, rec->ip)) ||
> > > 
> > >                     ((iter->flags & FTRACE_ITER_ENABLED) &&
> > >                      !(rec->flags & FTRACE_FL_ENABLED)) ||
> > > 
> > >                     ((iter->flags & FTRACE_ITER_UNSAFE_ONLY) &&
> > >                      !ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip)) ||
> > > 
> > >                     ((iter->flags & FTRACE_ITER_NO_UNSAFE) &&
> > >                      ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip))) {
> > > 
> > > 
> > > They are grouped in pairs.
> > 
> > Fair enough!
> 
> Can I take this as an Acked-by?

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> -- Steve
> 
> > 
> > 							Thanx, Paul
> > 
> > > -- Steve
> > > 
> > > > 
> > > > 							Thanx, Paul
> > > > 
> > > > > +		    ((iter->flags & FTRACE_ITER_NO_UNSAFE) &&
> > > > > +		     ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip))) {
> > > > > 
> > > > >  			rec = NULL;
> > > > >  			goto retry;
> > > > > @@ -2773,6 +2776,7 @@ ftrace_avail_open(struct inode *inode, struct file *file)
> > > > >  	iter = __seq_open_private(file, &show_ftrace_seq_ops, sizeof(*iter));
> > > > >  	if (iter) {
> > > > >  		iter->pg = ftrace_pages_start;
> > > > > +		iter->flags = FTRACE_ITER_NO_UNSAFE;
> > > > >  		iter->ops = &global_ops;
> > > > >  	}
> > > > > 
> > > > > -- 
> > > > > 1.7.10.4
> > > > > 
> > > > > 
> > > 
> > > 
> 

[RFC][PATCH 06/18 v2] ftrace: Add rcu_unsafe_filter_functions file

[Steven Rostedt]

[-- Attachment #1: 0006-ftrace-Add-rcu_unsafe_filter_functions-file.patch --]
[-- Type: text/plain, Size: 2859 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Since the RCU unsafe functions are no longer displayed by the
available_filter_functions, we still need a way to see these
functions in order to trace them. Create a new file that lists
the functions that were declared RCU unsafe.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace.h |    1 +
 kernel/trace/ftrace.c  |   31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 4709264..4752764 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -412,6 +412,7 @@ enum {
 	FTRACE_ITER_HASH	= (1 << 4),
 	FTRACE_ITER_ENABLED	= (1 << 5),
 	FTRACE_ITER_NO_UNSAFE	= (1 << 6),
+	FTRACE_ITER_UNSAFE_ONLY	= (1 << 7),
 };
 
 void arch_ftrace_update_code(int command);
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index be87ac9..a3e4b71 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2647,6 +2647,9 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
 		    ((iter->flags & FTRACE_ITER_ENABLED) &&
 		     !(rec->flags & FTRACE_FL_ENABLED)) ||
 
+		    ((iter->flags & FTRACE_ITER_UNSAFE_ONLY) &&
+		     !ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip)) ||
+
 		    ((iter->flags & FTRACE_ITER_NO_UNSAFE) &&
 		     ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip))) {
 
@@ -2784,6 +2787,24 @@ ftrace_avail_open(struct inode *inode, struct file *file)
 }
 
 static int
+ftrace_rcu_unsafe_open(struct inode *inode, struct file *file)
+{
+	struct ftrace_iterator *iter;
+
+	if (unlikely(ftrace_disabled))
+		return -ENODEV;
+
+	iter = __seq_open_private(file, &show_ftrace_seq_ops, sizeof(*iter));
+	if (iter) {
+		iter->pg = ftrace_pages_start;
+		iter->flags = FTRACE_ITER_UNSAFE_ONLY;
+		iter->ops = &global_ops;
+	}
+
+	return iter ? 0 : -ENOMEM;
+}
+
+static int
 ftrace_enabled_open(struct inode *inode, struct file *file)
 {
 	struct ftrace_iterator *iter;
@@ -3835,6 +3856,13 @@ static const struct file_operations ftrace_avail_fops = {
 	.release = seq_release_private,
 };
 
+static const struct file_operations ftrace_rcu_unsafe_fops = {
+	.open = ftrace_rcu_unsafe_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = seq_release_private,
+};
+
 static const struct file_operations ftrace_enabled_fops = {
 	.open = ftrace_enabled_open,
 	.read = seq_read,
@@ -4071,6 +4099,9 @@ static __init int ftrace_init_dyn_debugfs(struct dentry *d_tracer)
 	trace_create_file("available_filter_functions", 0444,
 			d_tracer, NULL, &ftrace_avail_fops);
 
+	trace_create_file("rcu_unsafe_filter_functions", 0444,
+			d_tracer, NULL, &ftrace_rcu_unsafe_fops);
+
 	trace_create_file("enabled_functions", 0444,
 			d_tracer, NULL, &ftrace_enabled_fops);
 
-- 
1.7.10.4

Re: [RFC][PATCH 06/18 v2] ftrace: Add rcu_unsafe_filter_functions file

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:23AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Since the RCU unsafe functions are no longer displayed by the
> available_filter_functions, we still need a way to see these
> functions in order to trace them. Create a new file that lists
> the functions that were declared RCU unsafe.
> 
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  include/linux/ftrace.h |    1 +
>  kernel/trace/ftrace.c  |   31 +++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 4709264..4752764 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -412,6 +412,7 @@ enum {
>  	FTRACE_ITER_HASH	= (1 << 4),
>  	FTRACE_ITER_ENABLED	= (1 << 5),
>  	FTRACE_ITER_NO_UNSAFE	= (1 << 6),
> +	FTRACE_ITER_UNSAFE_ONLY	= (1 << 7),
>  };
> 
>  void arch_ftrace_update_code(int command);
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index be87ac9..a3e4b71 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2647,6 +2647,9 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
>  		    ((iter->flags & FTRACE_ITER_ENABLED) &&
>  		     !(rec->flags & FTRACE_FL_ENABLED)) ||
> 
> +		    ((iter->flags & FTRACE_ITER_UNSAFE_ONLY) &&
> +		     !ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip)) ||
> +
>  		    ((iter->flags & FTRACE_ITER_NO_UNSAFE) &&
>  		     ftrace_lookup_ip(ftrace_unsafe_rcu, rec->ip))) {
> 
> @@ -2784,6 +2787,24 @@ ftrace_avail_open(struct inode *inode, struct file *file)
>  }
> 
>  static int
> +ftrace_rcu_unsafe_open(struct inode *inode, struct file *file)
> +{
> +	struct ftrace_iterator *iter;
> +
> +	if (unlikely(ftrace_disabled))
> +		return -ENODEV;
> +
> +	iter = __seq_open_private(file, &show_ftrace_seq_ops, sizeof(*iter));
> +	if (iter) {
> +		iter->pg = ftrace_pages_start;
> +		iter->flags = FTRACE_ITER_UNSAFE_ONLY;
> +		iter->ops = &global_ops;
> +	}
> +
> +	return iter ? 0 : -ENOMEM;
> +}
> +
> +static int
>  ftrace_enabled_open(struct inode *inode, struct file *file)
>  {
>  	struct ftrace_iterator *iter;
> @@ -3835,6 +3856,13 @@ static const struct file_operations ftrace_avail_fops = {
>  	.release = seq_release_private,
>  };
> 
> +static const struct file_operations ftrace_rcu_unsafe_fops = {
> +	.open = ftrace_rcu_unsafe_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = seq_release_private,
> +};
> +
>  static const struct file_operations ftrace_enabled_fops = {
>  	.open = ftrace_enabled_open,
>  	.read = seq_read,
> @@ -4071,6 +4099,9 @@ static __init int ftrace_init_dyn_debugfs(struct dentry *d_tracer)
>  	trace_create_file("available_filter_functions", 0444,
>  			d_tracer, NULL, &ftrace_avail_fops);
> 
> +	trace_create_file("rcu_unsafe_filter_functions", 0444,
> +			d_tracer, NULL, &ftrace_rcu_unsafe_fops);
> +
>  	trace_create_file("enabled_functions", 0444,
>  			d_tracer, NULL, &ftrace_enabled_fops);
> 
> -- 
> 1.7.10.4
> 
> 

[RFC][PATCH 07/18 v2] ftrace: Add selftest to check if RCU unsafe functions are filtered properly

[Steven Rostedt]

[-- Attachment #1: 0007-ftrace-Add-selftest-to-check-if-RCU-unsafe-functions.patch --]
[-- Type: text/plain, Size: 4528 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Add a boot time start up test that has a RCU safe ftrace_ops as well
as an unsafe one. Make sure the RCU safe ops can trace RCU unsafe
functions while the unsafe ftrace_ops can not.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace.h                  |    2 +
 kernel/trace/trace_selftest.c         |   94 +++++++++++++++++++++++++++++++++
 kernel/trace/trace_selftest_dynamic.c |    7 +++
 3 files changed, 103 insertions(+)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 502fed7..3578be6 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -648,6 +648,8 @@ extern unsigned long ftrace_update_tot_cnt;
 extern int DYN_FTRACE_TEST_NAME(void);
 #define DYN_FTRACE_TEST_NAME2 trace_selftest_dynamic_test_func2
 extern int DYN_FTRACE_TEST_NAME2(void);
+#define DYN_FTRACE_TEST_RCU_UNSAFE trace_selftest_dynamic_test_rcu_unsafe
+extern int DYN_FTRACE_TEST_RCU_UNSAFE(void);
 
 extern bool ring_buffer_expanded;
 extern bool tracing_selftest_disabled;
diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
index a7329b7..eeacb47 100644
--- a/kernel/trace/trace_selftest.c
+++ b/kernel/trace/trace_selftest.c
@@ -185,6 +185,97 @@ static void reset_counts(void)
 	trace_selftest_test_dyn_cnt = 0;
 }
 
+static struct ftrace_ops ftrace_rcu_safe_ops = {
+	.func		= trace_selftest_test_probe1_func,
+	.flags		= FTRACE_OPS_FL_RECURSION_SAFE | FTRACE_OPS_FL_RCU_SAFE,
+};
+
+static struct ftrace_ops ftrace_rcu_unsafe_ops = {
+	.func		= trace_selftest_test_probe2_func,
+	.flags		= FTRACE_OPS_FL_RECURSION_SAFE,
+};
+
+static int test_rcu_safe_unsafe(void)
+{
+	int save_ftrace_enabled = ftrace_enabled;
+	char *func_name;
+	int len;
+	int ret = 0;
+
+	printk(KERN_CONT "PASSED\n");
+	pr_info("Testing ftrace rcu safe unsafe: ");
+
+	ftrace_enabled = 1;
+
+	trace_selftest_test_probe1_cnt = 0;
+	trace_selftest_test_probe2_cnt = 0;
+
+	func_name = "*" __stringify(DYN_FTRACE_TEST_RCU_UNSAFE);
+	len = strlen(func_name);
+
+	ftrace_set_filter(&ftrace_rcu_safe_ops, func_name, len, 1);
+	ftrace_set_filter(&ftrace_rcu_unsafe_ops, func_name, len, 1);
+
+	/* Do single registrations first */
+	register_ftrace_function(&ftrace_rcu_safe_ops);
+
+	DYN_FTRACE_TEST_RCU_UNSAFE();
+
+	unregister_ftrace_function(&ftrace_rcu_safe_ops);
+
+	if (trace_selftest_test_probe1_cnt != 1) {
+		printk(KERN_CONT "rcu_safe expected 1 call but had %d ",
+		       trace_selftest_test_probe1_cnt);
+		goto out;
+	}
+
+	register_ftrace_function(&ftrace_rcu_unsafe_ops);
+
+	DYN_FTRACE_TEST_RCU_UNSAFE();
+
+	unregister_ftrace_function(&ftrace_rcu_unsafe_ops);
+
+	if (trace_selftest_test_probe2_cnt != 0) {
+		printk(KERN_CONT "rcu_safe expected 0 calls but had %d ",
+		       trace_selftest_test_probe2_cnt);
+		goto out;
+	}
+
+	/* Run both together, which uses the list op */
+
+	trace_selftest_test_probe1_cnt = 0;
+	trace_selftest_test_probe2_cnt = 0;
+
+	register_ftrace_function(&ftrace_rcu_safe_ops);
+	register_ftrace_function(&ftrace_rcu_unsafe_ops);
+
+	DYN_FTRACE_TEST_RCU_UNSAFE();
+	DYN_FTRACE_TEST_RCU_UNSAFE();
+	DYN_FTRACE_TEST_RCU_UNSAFE();
+
+	unregister_ftrace_function(&ftrace_rcu_unsafe_ops);
+	unregister_ftrace_function(&ftrace_rcu_safe_ops);
+
+
+	if (trace_selftest_test_probe1_cnt != 3) {
+		printk(KERN_CONT "rcu_safe expected 3 calls but had %d ",
+		       trace_selftest_test_probe1_cnt);
+		goto out;
+	}
+
+	if (trace_selftest_test_probe2_cnt != 0) {
+		printk(KERN_CONT "rcu_safe expected 0 calls but had %d ",
+		       trace_selftest_test_probe2_cnt);
+		goto out;
+	}
+
+	ret = 0;
+ out:
+	ftrace_enabled = save_ftrace_enabled;
+
+	return ret;
+}
+
 static int trace_selftest_ops(int cnt)
 {
 	int save_ftrace_enabled = ftrace_enabled;
@@ -401,6 +492,9 @@ int trace_selftest_startup_dynamic_tracing(struct tracer *trace,
 	if (!ret)
 		ret = trace_selftest_ops(2);
 
+	if (!ret)
+		ret = test_rcu_safe_unsafe();
+
 	return ret;
 }
 
diff --git a/kernel/trace/trace_selftest_dynamic.c b/kernel/trace/trace_selftest_dynamic.c
index b4c475a..5eefa56 100644
--- a/kernel/trace/trace_selftest_dynamic.c
+++ b/kernel/trace/trace_selftest_dynamic.c
@@ -11,3 +11,10 @@ int DYN_FTRACE_TEST_NAME2(void)
 	/* used to call mcount */
 	return 0;
 }
+
+int trace_selftest_dynamic_test_rcu_unsafe(void)
+{
+	/* used to call mcount */
+	return 0;
+}
+FTRACE_UNSAFE_RCU(trace_selftest_dynamic_test_rcu_unsafe);
-- 
1.7.10.4

Re: [RFC][PATCH 07/18 v2] ftrace: Add selftest to check if RCU unsafe functions are filtered properly

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:24AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Add a boot time start up test that has a RCU safe ftrace_ops as well
> as an unsafe one. Make sure the RCU safe ops can trace RCU unsafe
> functions while the unsafe ftrace_ops can not.
> 
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  kernel/trace/trace.h                  |    2 +
>  kernel/trace/trace_selftest.c         |   94 +++++++++++++++++++++++++++++++++
>  kernel/trace/trace_selftest_dynamic.c |    7 +++
>  3 files changed, 103 insertions(+)
> 
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 502fed7..3578be6 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -648,6 +648,8 @@ extern unsigned long ftrace_update_tot_cnt;
>  extern int DYN_FTRACE_TEST_NAME(void);
>  #define DYN_FTRACE_TEST_NAME2 trace_selftest_dynamic_test_func2
>  extern int DYN_FTRACE_TEST_NAME2(void);
> +#define DYN_FTRACE_TEST_RCU_UNSAFE trace_selftest_dynamic_test_rcu_unsafe
> +extern int DYN_FTRACE_TEST_RCU_UNSAFE(void);
> 
>  extern bool ring_buffer_expanded;
>  extern bool tracing_selftest_disabled;
> diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
> index a7329b7..eeacb47 100644
> --- a/kernel/trace/trace_selftest.c
> +++ b/kernel/trace/trace_selftest.c
> @@ -185,6 +185,97 @@ static void reset_counts(void)
>  	trace_selftest_test_dyn_cnt = 0;
>  }
> 
> +static struct ftrace_ops ftrace_rcu_safe_ops = {
> +	.func		= trace_selftest_test_probe1_func,
> +	.flags		= FTRACE_OPS_FL_RECURSION_SAFE | FTRACE_OPS_FL_RCU_SAFE,
> +};
> +
> +static struct ftrace_ops ftrace_rcu_unsafe_ops = {
> +	.func		= trace_selftest_test_probe2_func,
> +	.flags		= FTRACE_OPS_FL_RECURSION_SAFE,
> +};
> +
> +static int test_rcu_safe_unsafe(void)
> +{
> +	int save_ftrace_enabled = ftrace_enabled;
> +	char *func_name;
> +	int len;
> +	int ret = 0;
> +
> +	printk(KERN_CONT "PASSED\n");
> +	pr_info("Testing ftrace rcu safe unsafe: ");
> +
> +	ftrace_enabled = 1;
> +
> +	trace_selftest_test_probe1_cnt = 0;
> +	trace_selftest_test_probe2_cnt = 0;
> +
> +	func_name = "*" __stringify(DYN_FTRACE_TEST_RCU_UNSAFE);
> +	len = strlen(func_name);
> +
> +	ftrace_set_filter(&ftrace_rcu_safe_ops, func_name, len, 1);
> +	ftrace_set_filter(&ftrace_rcu_unsafe_ops, func_name, len, 1);
> +
> +	/* Do single registrations first */
> +	register_ftrace_function(&ftrace_rcu_safe_ops);
> +
> +	DYN_FTRACE_TEST_RCU_UNSAFE();
> +
> +	unregister_ftrace_function(&ftrace_rcu_safe_ops);
> +
> +	if (trace_selftest_test_probe1_cnt != 1) {
> +		printk(KERN_CONT "rcu_safe expected 1 call but had %d ",
> +		       trace_selftest_test_probe1_cnt);
> +		goto out;
> +	}
> +
> +	register_ftrace_function(&ftrace_rcu_unsafe_ops);
> +
> +	DYN_FTRACE_TEST_RCU_UNSAFE();
> +
> +	unregister_ftrace_function(&ftrace_rcu_unsafe_ops);
> +
> +	if (trace_selftest_test_probe2_cnt != 0) {
> +		printk(KERN_CONT "rcu_safe expected 0 calls but had %d ",
> +		       trace_selftest_test_probe2_cnt);
> +		goto out;
> +	}
> +
> +	/* Run both together, which uses the list op */
> +
> +	trace_selftest_test_probe1_cnt = 0;
> +	trace_selftest_test_probe2_cnt = 0;
> +
> +	register_ftrace_function(&ftrace_rcu_safe_ops);
> +	register_ftrace_function(&ftrace_rcu_unsafe_ops);
> +
> +	DYN_FTRACE_TEST_RCU_UNSAFE();
> +	DYN_FTRACE_TEST_RCU_UNSAFE();
> +	DYN_FTRACE_TEST_RCU_UNSAFE();
> +
> +	unregister_ftrace_function(&ftrace_rcu_unsafe_ops);
> +	unregister_ftrace_function(&ftrace_rcu_safe_ops);
> +
> +
> +	if (trace_selftest_test_probe1_cnt != 3) {
> +		printk(KERN_CONT "rcu_safe expected 3 calls but had %d ",
> +		       trace_selftest_test_probe1_cnt);
> +		goto out;
> +	}
> +
> +	if (trace_selftest_test_probe2_cnt != 0) {
> +		printk(KERN_CONT "rcu_safe expected 0 calls but had %d ",
> +		       trace_selftest_test_probe2_cnt);
> +		goto out;
> +	}
> +
> +	ret = 0;
> + out:
> +	ftrace_enabled = save_ftrace_enabled;
> +
> +	return ret;
> +}
> +
>  static int trace_selftest_ops(int cnt)
>  {
>  	int save_ftrace_enabled = ftrace_enabled;
> @@ -401,6 +492,9 @@ int trace_selftest_startup_dynamic_tracing(struct tracer *trace,
>  	if (!ret)
>  		ret = trace_selftest_ops(2);
> 
> +	if (!ret)
> +		ret = test_rcu_safe_unsafe();
> +
>  	return ret;
>  }
> 
> diff --git a/kernel/trace/trace_selftest_dynamic.c b/kernel/trace/trace_selftest_dynamic.c
> index b4c475a..5eefa56 100644
> --- a/kernel/trace/trace_selftest_dynamic.c
> +++ b/kernel/trace/trace_selftest_dynamic.c
> @@ -11,3 +11,10 @@ int DYN_FTRACE_TEST_NAME2(void)
>  	/* used to call mcount */
>  	return 0;
>  }
> +
> +int trace_selftest_dynamic_test_rcu_unsafe(void)
> +{
> +	/* used to call mcount */
> +	return 0;
> +}
> +FTRACE_UNSAFE_RCU(trace_selftest_dynamic_test_rcu_unsafe);
> -- 
> 1.7.10.4
> 
> 

[RFC][PATCH 08/18 v2] ftrace/rcu: Do not trace debug_lockdep_rcu_enabled()

[Steven Rostedt]

[-- Attachment #1: 0008-ftrace-rcu-Do-not-trace-debug_lockdep_rcu_enabled.patch --]
[-- Type: text/plain, Size: 1050 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

The function debug_lockdep_rcu_enabled() is part of the RCU lockdep
debugging, and is called very frequently. I found that if I enable
a lot of debugging and run the function graph tracer, this
function can cause a live lock of the system.

We don't usually trace lockdep infrastructure, no need to trace
this either.

Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/rcupdate.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
index cce6ba8..4f20c6c 100644
--- a/kernel/rcupdate.c
+++ b/kernel/rcupdate.c
@@ -122,7 +122,7 @@ struct lockdep_map rcu_sched_lock_map =
 	STATIC_LOCKDEP_MAP_INIT("rcu_read_lock_sched", &rcu_sched_lock_key);
 EXPORT_SYMBOL_GPL(rcu_sched_lock_map);
 
-int debug_lockdep_rcu_enabled(void)
+int notrace debug_lockdep_rcu_enabled(void)
 {
 	return rcu_scheduler_active && debug_locks &&
 	       current->lockdep_recursion == 0;
-- 
1.7.10.4

Re: [RFC][PATCH 08/18 v2] ftrace/rcu: Do not trace debug_lockdep_rcu_enabled()

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:25AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> The function debug_lockdep_rcu_enabled() is part of the RCU lockdep
> debugging, and is called very frequently. I found that if I enable
> a lot of debugging and run the function graph tracer, this
> function can cause a live lock of the system.
> 
> We don't usually trace lockdep infrastructure, no need to trace
> this either.
> 
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Or I can just take it, if you would rather.  (If so, 3.12 or 3.13?)

> ---
>  kernel/rcupdate.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
> index cce6ba8..4f20c6c 100644
> --- a/kernel/rcupdate.c
> +++ b/kernel/rcupdate.c
> @@ -122,7 +122,7 @@ struct lockdep_map rcu_sched_lock_map =
>  	STATIC_LOCKDEP_MAP_INIT("rcu_read_lock_sched", &rcu_sched_lock_key);
>  EXPORT_SYMBOL_GPL(rcu_sched_lock_map);
> 
> -int debug_lockdep_rcu_enabled(void)
> +int notrace debug_lockdep_rcu_enabled(void)
>  {
>  	return rcu_scheduler_active && debug_locks &&
>  	       current->lockdep_recursion == 0;
> -- 
> 1.7.10.4
> 
> 

Re: [RFC][PATCH 08/18 v2] ftrace/rcu: Do not trace debug_lockdep_rcu_enabled()

[Steven Rostedt]

On Sat, 2013-08-31 at 12:21 -0700, Paul E. McKenney wrote:
> On Sat, Aug 31, 2013 at 01:11:25AM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > 
> > The function debug_lockdep_rcu_enabled() is part of the RCU lockdep
> > debugging, and is called very frequently. I found that if I enable
> > a lot of debugging and run the function graph tracer, this
> > function can cause a live lock of the system.
> > 
> > We don't usually trace lockdep infrastructure, no need to trace
> > this either.
> > 
> > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> Or I can just take it, if you would rather.  (If so, 3.12 or 3.13?)
> 

Feel free to take it and I'll remove it from my queue. 3.12 please.

I'll just need to remember to turn off function graph tracing when
testing these patches again.

-- Steve

[RFC][PATCH 09/18 v2] ftrace: Fix a slight race in modifying what function callback gets traced

[Steven Rostedt]

[-- Attachment #1: 0009-ftrace-Fix-a-slight-race-in-modifying-what-function-.patch --]
[-- Type: text/plain, Size: 2027 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

There's a slight race when going from a list function to a non list
function. That is, when only one callback is registered to the function
tracer, it gets called directly by the mcount trampoline. But if this
function has filters, it may be called by the wrong functions.

As the list ops callback that handles multiple callbacks that are
registered to ftrace, it also handles what functions they call. While
the transaction is taking place, use the list function always, and
after all the updates are finished (only the functions that should be
traced are being traced), then we can update the trampoline to call
the function directly.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a3e4b71..33e4890 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2076,12 +2076,27 @@ int __weak ftrace_arch_code_modify_post_process(void)
 
 void ftrace_modify_all_code(int command)
 {
+	int update = command & FTRACE_UPDATE_TRACE_FUNC;
+
+	/*
+	 * If the ftrace_caller calls a ftrace_ops func directly,
+	 * we need to make sure that it only traces functions it
+	 * expects to trace. When doing the switch of functions,
+	 * we need to update to the ftrace_ops_list_func first
+	 * before the transition between old and new calls are set,
+	 * as the ftrace_ops_list_func will check the ops hashes
+	 * to make sure the ops are having the right functions
+	 * traced.
+	 */
+	if (update)
+		ftrace_update_ftrace_func(ftrace_ops_list_func);
+
 	if (command & FTRACE_UPDATE_CALLS)
 		ftrace_replace_code(1);
 	else if (command & FTRACE_DISABLE_CALLS)
 		ftrace_replace_code(0);
 
-	if (command & FTRACE_UPDATE_TRACE_FUNC)
+	if (update && ftrace_trace_function != ftrace_ops_list_func)
 		ftrace_update_ftrace_func(ftrace_trace_function);
 
 	if (command & FTRACE_START_FUNC_RET)
-- 
1.7.10.4

[RFC][PATCH 10/18 v2] ftrace: Create a RCU unsafe checker

[Steven Rostedt]

[-- Attachment #1: 0010-ftrace-Create-a-RCU-unsafe-checker.patch --]
[-- Type: text/plain, Size: 5114 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Knowing what functions are not safe to be traced by callbacks that use
RCU read locks, is not easy to figure out. By adding a function tracer
callback that is set as a non RCU safe callback that also uses
rcu_read_lock() and enables PROVE_RCU, it can be used to find locations
in the kernel that need to be tagged with FTRACE_UNSAFE_RCU().

On boot up, this callback gets registered so that all functions are
being traced, and there's no way to turn this off. In the future we
can make this enabled or disabled at run time, but for now it's only
used for debugging and should not be enabled by normal users.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/Kconfig           |   22 +++++++++++++++++++
 kernel/trace/ftrace.c          |   12 ++++++++++
 kernel/trace/trace.h           |    3 +++
 kernel/trace/trace_functions.c |   47 +++++++++++++++++++++++++++++++++++++++-
 4 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 015f85a..907b497 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -489,6 +489,28 @@ config FTRACE_MCOUNT_RECORD
 config FTRACE_SELFTEST
 	bool
 
+config FTRACE_UNSAFE_RCU_CHECKER
+        bool "Trace for unsafe RCU callers"
+	depends on DYNAMIC_FTRACE
+	depends on PROVE_LOCKING
+	select PROVE_RCU
+	help
+	 Some function tracing callbacks use RCU read lock (namely perf).
+	 There are some RCU critical functions that are called out
+	 of scope for RCU. For example, when NO_HZ_FULL is enabled,
+	 and coming out of user space. The CPU is not being tracked by
+	 RCU and all rcu_read_lock()s will be ignored. These functions
+	 need to be tagged as unsafe for RCU so that only function callbacks
+	 that specify that they do not use RCU will be called by them.
+
+	 This option will enable PROVE_RCU and will enable on boot up
+	 a function callback that traces all functions and uses an
+	 rcu_read_lock(). If any function that is not tagged as unsafe
+	 for RCU is called, a debug splat will be shown. This is used
+	 for finding unsafe RCU callers that need to be tagged.
+
+	 If you don't understand any of this, then say N.
+
 config FTRACE_STARTUP_TEST
 	bool "Perform a startup test on ftrace"
 	depends on GENERIC_TRACER
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 33e4890..69b7f62 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1359,6 +1359,13 @@ alloc_and_copy_ftrace_hash(int size_bits, struct ftrace_hash *hash)
 	return NULL;
 }
 
+#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
+bool ftrace_rcu_unsafe(unsigned long addr)
+{
+	return ftrace_lookup_ip(ftrace_unsafe_rcu, addr) != NULL;
+}
+#endif
+
 static void
 ftrace_hash_rec_disable(struct ftrace_ops *ops, int filter_hash);
 static void
@@ -4391,6 +4398,11 @@ static void __init create_unsafe_rcu_hash(void)
 		if (WARN_ON_ONCE(strcmp(str, finder->name) != 0))
 			continue;
 
+#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
+		/* When checker is enabled, show what was marked unsafe */
+		pr_info("add ftrace rcu unsafe %p (%s)\n",
+			(void *)ip, finder->name);
+#endif
 		add_hash_entry(ftrace_unsafe_rcu, ip);
 	}
 }
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 3578be6..e551316 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -760,6 +760,9 @@ static inline int ftrace_graph_addr(unsigned long addr)
 
 	return 0;
 }
+#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
+extern bool ftrace_rcu_unsafe(unsigned long addr);
+#endif
 #else
 static inline int ftrace_graph_addr(unsigned long addr)
 {
diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index 38fe148..9dd4627 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -559,9 +559,54 @@ static inline int init_func_cmd_traceon(void)
 }
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
+#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
+static void
+ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct pt_regs *pt_regs)
+{
+	int bit;
+
+	preempt_disable_notrace();
+
+	bit = trace_test_and_set_recursion(TRACE_FTRACE_START, TRACE_FTRACE_MAX);
+	if (bit < 0)
+		goto out;
+
+	if (WARN_ONCE(ftrace_rcu_unsafe(ip),
+		      "UNSAFE RCU function called %pS",
+		      (void *)ip))
+		goto out;
+
+	/* Should trigger a RCU splat if called from unsafe RCU function */
+	rcu_read_lock();
+	rcu_read_unlock();
+
+	trace_clear_recursion(bit);
+ out:
+	preempt_enable_notrace();
+}
+
+static struct ftrace_ops ftrace_unsafe_ops = {
+	.func			= ftrace_unsafe_callback,
+	.flags			= FTRACE_OPS_FL_RECURSION_SAFE
+};
+
+static __init void ftrace_start_unsafe_rcu(void)
+{
+	register_ftrace_function(&ftrace_unsafe_ops);
+}
+#else
+static inline void ftrace_start_unsafe_rcu(void) { }
+#endif
+
 static __init int init_function_trace(void)
 {
+	int ret;
+
 	init_func_cmd_traceon();
-	return register_tracer(&function_trace);
+	ret = register_tracer(&function_trace);
+	if (!ret)
+		ftrace_start_unsafe_rcu();
+	return ret;
 }
 core_initcall(init_function_trace);
-- 
1.7.10.4

Re: [RFC][PATCH 10/18 v2] ftrace: Create a RCU unsafe checker

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:27AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Knowing what functions are not safe to be traced by callbacks that use
> RCU read locks, is not easy to figure out. By adding a function tracer
> callback that is set as a non RCU safe callback that also uses
> rcu_read_lock() and enables PROVE_RCU, it can be used to find locations
> in the kernel that need to be tagged with FTRACE_UNSAFE_RCU().
> 
> On boot up, this callback gets registered so that all functions are
> being traced, and there's no way to turn this off. In the future we
> can make this enabled or disabled at run time, but for now it's only
> used for debugging and should not be enabled by normal users.
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  kernel/trace/Kconfig           |   22 +++++++++++++++++++
>  kernel/trace/ftrace.c          |   12 ++++++++++
>  kernel/trace/trace.h           |    3 +++
>  kernel/trace/trace_functions.c |   47 +++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index 015f85a..907b497 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -489,6 +489,28 @@ config FTRACE_MCOUNT_RECORD
>  config FTRACE_SELFTEST
>  	bool
> 
> +config FTRACE_UNSAFE_RCU_CHECKER
> +        bool "Trace for unsafe RCU callers"
> +	depends on DYNAMIC_FTRACE
> +	depends on PROVE_LOCKING
> +	select PROVE_RCU
> +	help
> +	 Some function tracing callbacks use RCU read lock (namely perf).
> +	 There are some RCU critical functions that are called out
> +	 of scope for RCU. For example, when NO_HZ_FULL is enabled,
> +	 and coming out of user space. The CPU is not being tracked by
> +	 RCU and all rcu_read_lock()s will be ignored. These functions
> +	 need to be tagged as unsafe for RCU so that only function callbacks
> +	 that specify that they do not use RCU will be called by them.
> +
> +	 This option will enable PROVE_RCU and will enable on boot up
> +	 a function callback that traces all functions and uses an
> +	 rcu_read_lock(). If any function that is not tagged as unsafe
> +	 for RCU is called, a debug splat will be shown. This is used
> +	 for finding unsafe RCU callers that need to be tagged.
> +
> +	 If you don't understand any of this, then say N.
> +
>  config FTRACE_STARTUP_TEST
>  	bool "Perform a startup test on ftrace"
>  	depends on GENERIC_TRACER
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 33e4890..69b7f62 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1359,6 +1359,13 @@ alloc_and_copy_ftrace_hash(int size_bits, struct ftrace_hash *hash)
>  	return NULL;
>  }
> 
> +#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> +bool ftrace_rcu_unsafe(unsigned long addr)
> +{
> +	return ftrace_lookup_ip(ftrace_unsafe_rcu, addr) != NULL;
> +}
> +#endif
> +
>  static void
>  ftrace_hash_rec_disable(struct ftrace_ops *ops, int filter_hash);
>  static void
> @@ -4391,6 +4398,11 @@ static void __init create_unsafe_rcu_hash(void)
>  		if (WARN_ON_ONCE(strcmp(str, finder->name) != 0))
>  			continue;
> 
> +#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> +		/* When checker is enabled, show what was marked unsafe */
> +		pr_info("add ftrace rcu unsafe %p (%s)\n",
> +			(void *)ip, finder->name);
> +#endif
>  		add_hash_entry(ftrace_unsafe_rcu, ip);
>  	}
>  }
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 3578be6..e551316 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -760,6 +760,9 @@ static inline int ftrace_graph_addr(unsigned long addr)
> 
>  	return 0;
>  }
> +#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> +extern bool ftrace_rcu_unsafe(unsigned long addr);
> +#endif
>  #else
>  static inline int ftrace_graph_addr(unsigned long addr)
>  {
> diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> index 38fe148..9dd4627 100644
> --- a/kernel/trace/trace_functions.c
> +++ b/kernel/trace/trace_functions.c
> @@ -559,9 +559,54 @@ static inline int init_func_cmd_traceon(void)
>  }
>  #endif /* CONFIG_DYNAMIC_FTRACE */
> 
> +#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> +static void
> +ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
> +		       struct ftrace_ops *op, struct pt_regs *pt_regs)
> +{
> +	int bit;
> +
> +	preempt_disable_notrace();
> +
> +	bit = trace_test_and_set_recursion(TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> +	if (bit < 0)
> +		goto out;
> +
> +	if (WARN_ONCE(ftrace_rcu_unsafe(ip),
> +		      "UNSAFE RCU function called %pS",
> +		      (void *)ip))
> +		goto out;
> +
> +	/* Should trigger a RCU splat if called from unsafe RCU function */
> +	rcu_read_lock();
> +	rcu_read_unlock();
> +
> +	trace_clear_recursion(bit);
> + out:
> +	preempt_enable_notrace();
> +}
> +
> +static struct ftrace_ops ftrace_unsafe_ops = {
> +	.func			= ftrace_unsafe_callback,
> +	.flags			= FTRACE_OPS_FL_RECURSION_SAFE
> +};
> +
> +static __init void ftrace_start_unsafe_rcu(void)
> +{
> +	register_ftrace_function(&ftrace_unsafe_ops);
> +}
> +#else
> +static inline void ftrace_start_unsafe_rcu(void) { }
> +#endif
> +
>  static __init int init_function_trace(void)
>  {
> +	int ret;
> +
>  	init_func_cmd_traceon();
> -	return register_tracer(&function_trace);
> +	ret = register_tracer(&function_trace);
> +	if (!ret)
> +		ftrace_start_unsafe_rcu();
> +	return ret;
>  }
>  core_initcall(init_function_trace);
> -- 
> 1.7.10.4
> 
> 

[RFC][PATCH 11/18 v2] ftrace: Adde infrastructure to stop RCU unsafe checker from checking

[Steven Rostedt]

[-- Attachment #1: 0011-ftrace-Adde-infrastructure-to-stop-RCU-unsafe-checke.patch --]
[-- Type: text/plain, Size: 2704 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

This is a light weight way to keep the rcu checker from checking
RCU safety. It adds a ftrace_unsafe_rcu_checker_disable/enable()
that increments or decrements a counter respectively. When the
counter is set, the RCU unsafe checker callback does not run the
tests to see if RCU is safe or not.

This is required by the graph tracer because the checks can cause
the graph tracer to live lock the system by its own calls.

It's also needed by the irqsoff tracer, because it may be called
in RCU unsafe regions and if its internal functions get traced
then the RCU unsafe checker may have some false positives.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace.h           |   12 +++++++++---
 kernel/trace/trace_functions.c |   15 +++++++++++++++
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index e551316..58e4c37 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -760,9 +760,6 @@ static inline int ftrace_graph_addr(unsigned long addr)
 
 	return 0;
 }
-#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
-extern bool ftrace_rcu_unsafe(unsigned long addr);
-#endif
 #else
 static inline int ftrace_graph_addr(unsigned long addr)
 {
@@ -1061,4 +1058,13 @@ int perf_ftrace_event_register(struct ftrace_event_call *call,
 #define perf_ftrace_event_register NULL
 #endif
 
+#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
+extern bool ftrace_rcu_unsafe(unsigned long addr);
+extern void ftrace_unsafe_rcu_checker_disable(void);
+extern void ftrace_unsafe_rcu_checker_enable(void);
+#else
+static inline void ftrace_unsafe_rcu_checker_disable(void) { }
+static inline void ftrace_unsafe_rcu_checker_enable(void) { }
+#endif
+
 #endif /* _LINUX_KERNEL_TRACE_H */
diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index 9dd4627..1d5f951 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -560,12 +560,27 @@ static inline int init_func_cmd_traceon(void)
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
 #ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
+static atomic_t ftrace_unsafe_rcu_disabled;
+
+void ftrace_unsafe_rcu_checker_disable(void)
+{
+	atomic_inc(&ftrace_unsafe_rcu_disabled);
+}
+
+void ftrace_unsafe_rcu_checker_enable(void)
+{
+	atomic_dec(&ftrace_unsafe_rcu_disabled);
+}
+
 static void
 ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
 		       struct ftrace_ops *op, struct pt_regs *pt_regs)
 {
 	int bit;
 
+	if (atomic_read(&ftrace_unsafe_rcu_disabled))
+		return;
+
 	preempt_disable_notrace();
 
 	bit = trace_test_and_set_recursion(TRACE_FTRACE_START, TRACE_FTRACE_MAX);
-- 
1.7.10.4

Re: [RFC][PATCH 11/18 v2] ftrace: Adde infrastructure to stop RCU unsafe checker from checking

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:28AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> This is a light weight way to keep the rcu checker from checking
> RCU safety. It adds a ftrace_unsafe_rcu_checker_disable/enable()
> that increments or decrements a counter respectively. When the
> counter is set, the RCU unsafe checker callback does not run the
> tests to see if RCU is safe or not.

Please add something saying what we do instead of testing RCU safety.
Looks to me like it skips not only the tests, but also invoking the
callback, but I could easily be wrong.

> This is required by the graph tracer because the checks can cause
> the graph tracer to live lock the system by its own calls.
> 
> It's also needed by the irqsoff tracer, because it may be called
> in RCU unsafe regions and if its internal functions get traced
> then the RCU unsafe checker may have some false positives.
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

With the augmented commit log as noted above:

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  kernel/trace/trace.h           |   12 +++++++++---
>  kernel/trace/trace_functions.c |   15 +++++++++++++++
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index e551316..58e4c37 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -760,9 +760,6 @@ static inline int ftrace_graph_addr(unsigned long addr)
> 
>  	return 0;
>  }
> -#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> -extern bool ftrace_rcu_unsafe(unsigned long addr);
> -#endif
>  #else
>  static inline int ftrace_graph_addr(unsigned long addr)
>  {
> @@ -1061,4 +1058,13 @@ int perf_ftrace_event_register(struct ftrace_event_call *call,
>  #define perf_ftrace_event_register NULL
>  #endif
> 
> +#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> +extern bool ftrace_rcu_unsafe(unsigned long addr);
> +extern void ftrace_unsafe_rcu_checker_disable(void);
> +extern void ftrace_unsafe_rcu_checker_enable(void);
> +#else
> +static inline void ftrace_unsafe_rcu_checker_disable(void) { }
> +static inline void ftrace_unsafe_rcu_checker_enable(void) { }
> +#endif
> +
>  #endif /* _LINUX_KERNEL_TRACE_H */
> diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> index 9dd4627..1d5f951 100644
> --- a/kernel/trace/trace_functions.c
> +++ b/kernel/trace/trace_functions.c
> @@ -560,12 +560,27 @@ static inline int init_func_cmd_traceon(void)
>  #endif /* CONFIG_DYNAMIC_FTRACE */
> 
>  #ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> +static atomic_t ftrace_unsafe_rcu_disabled;
> +
> +void ftrace_unsafe_rcu_checker_disable(void)
> +{
> +	atomic_inc(&ftrace_unsafe_rcu_disabled);
> +}
> +
> +void ftrace_unsafe_rcu_checker_enable(void)
> +{
> +	atomic_dec(&ftrace_unsafe_rcu_disabled);
> +}
> +
>  static void
>  ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
>  		       struct ftrace_ops *op, struct pt_regs *pt_regs)
>  {
>  	int bit;
> 
> +	if (atomic_read(&ftrace_unsafe_rcu_disabled))
> +		return;
> +
>  	preempt_disable_notrace();
> 
>  	bit = trace_test_and_set_recursion(TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> -- 
> 1.7.10.4
> 
> 

Re: [RFC][PATCH 11/18 v2] ftrace: Adde infrastructure to stop RCU unsafe checker from checking

[Steven Rostedt]

On Sat, 2013-08-31 at 12:52 -0700, Paul E. McKenney wrote:
> On Sat, Aug 31, 2013 at 01:11:28AM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > 
> > This is a light weight way to keep the rcu checker from checking
> > RCU safety. It adds a ftrace_unsafe_rcu_checker_disable/enable()
> > that increments or decrements a counter respectively. When the
> > counter is set, the RCU unsafe checker callback does not run the
> > tests to see if RCU is safe or not.
> 
> Please add something saying what we do instead of testing RCU safety.
> Looks to me like it skips not only the tests, but also invoking the
> callback, but I could easily be wrong.

Yeah, the change log sucks. I wrote the change logs after 1am and was
half asleep. I see I wrote "Add" as "Adde" ;-)

> 
> > This is required by the graph tracer because the checks can cause
> > the graph tracer to live lock the system by its own calls.
> > 
> > It's also needed by the irqsoff tracer, because it may be called
> > in RCU unsafe regions and if its internal functions get traced
> > then the RCU unsafe checker may have some false positives.
> > 
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> With the augmented commit log as noted above:
> 
> Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 

Thanks, come Tuesday, I'll update the logs.

-- Steve

> > ---
> >  kernel/trace/trace.h           |   12 +++++++++---
> >  kernel/trace/trace_functions.c |   15 +++++++++++++++
> >  2 files changed, 24 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index e551316..58e4c37 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -760,9 +760,6 @@ static inline int ftrace_graph_addr(unsigned long addr)

Re: [RFC][PATCH 11/18 v2] ftrace: Adde infrastructure to stop RCU unsafe checker from checking

[Steven Rostedt]

On Sat, 31 Aug 2013 12:52:58 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> On Sat, Aug 31, 2013 at 01:11:28AM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > 
> > This is a light weight way to keep the rcu checker from checking
> > RCU safety. It adds a ftrace_unsafe_rcu_checker_disable/enable()
> > that increments or decrements a counter respectively. When the
> > counter is set, the RCU unsafe checker callback does not run the
> > tests to see if RCU is safe or not.
> 
> Please add something saying what we do instead of testing RCU safety.
> Looks to me like it skips not only the tests, but also invoking the
> callback, but I could easily be wrong.

Now that I'm awake and also not on holiday, I can reply with a clear
mind.

When disabled, it forces the callback to return immediately. I may in
the future, have it disable the callback altogether.

Yes, we can miss checks, but it's better than the test live locking
the system :-)

> 
> > This is required by the graph tracer because the checks can cause
> > the graph tracer to live lock the system by its own calls.

I added this (I quoted the added text):

    This is a light weight way to keep the rcu checker from checking
    RCU safety. It adds a ftrace_unsafe_rcu_checker_disable/enable()
    that increments or decrements a counter respectively. When the
    counter is set, the RCU unsafe checker callback does not run the
    tests to see if RCU is safe or not. "But the callback still gets
    called. It just does not call rcu_read_(un)lock()."
    
    This is required by the graph tracer because the checks can cause
    the graph tracer to live lock the system by its own calls. "That is,
    the graph tracer will still trace rcu and the rcu debugging, and
    this will slow down the checker, which is still calling all other
    functions (in interrupts and faults), which can cause the timer
    interrupt to take oven a millisecond to complete, and it will then
    trigger once it finishes, live locking the system."


> > 
> > It's also needed by the irqsoff tracer, because it may be called
> > in RCU unsafe regions and if its internal functions get traced
> > then the RCU unsafe checker may have some false positives.
> > 
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> With the augmented commit log as noted above:

Is the above OK?

-- Steve

> 
> Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> > ---
> >  kernel/trace/trace.h           |   12 +++++++++---
> >  kernel/trace/trace_functions.c |   15 +++++++++++++++
> >  2 files changed, 24 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index e551316..58e4c37 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -760,9 +760,6 @@ static inline int ftrace_graph_addr(unsigned long addr)
> > 
> >  	return 0;
> >  }
> > -#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> > -extern bool ftrace_rcu_unsafe(unsigned long addr);
> > -#endif
> >  #else
> >  static inline int ftrace_graph_addr(unsigned long addr)
> >  {
> > @@ -1061,4 +1058,13 @@ int perf_ftrace_event_register(struct ftrace_event_call *call,
> >  #define perf_ftrace_event_register NULL
> >  #endif
> > 
> > +#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> > +extern bool ftrace_rcu_unsafe(unsigned long addr);
> > +extern void ftrace_unsafe_rcu_checker_disable(void);
> > +extern void ftrace_unsafe_rcu_checker_enable(void);
> > +#else
> > +static inline void ftrace_unsafe_rcu_checker_disable(void) { }
> > +static inline void ftrace_unsafe_rcu_checker_enable(void) { }
> > +#endif
> > +
> >  #endif /* _LINUX_KERNEL_TRACE_H */
> > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> > index 9dd4627..1d5f951 100644
> > --- a/kernel/trace/trace_functions.c
> > +++ b/kernel/trace/trace_functions.c
> > @@ -560,12 +560,27 @@ static inline int init_func_cmd_traceon(void)
> >  #endif /* CONFIG_DYNAMIC_FTRACE */
> > 
> >  #ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> > +static atomic_t ftrace_unsafe_rcu_disabled;
> > +
> > +void ftrace_unsafe_rcu_checker_disable(void)
> > +{
> > +	atomic_inc(&ftrace_unsafe_rcu_disabled);
> > +}
> > +
> > +void ftrace_unsafe_rcu_checker_enable(void)
> > +{
> > +	atomic_dec(&ftrace_unsafe_rcu_disabled);
> > +}
> > +
> >  static void
> >  ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
> >  		       struct ftrace_ops *op, struct pt_regs *pt_regs)
> >  {
> >  	int bit;
> > 
> > +	if (atomic_read(&ftrace_unsafe_rcu_disabled))
> > +		return;
> > +
> >  	preempt_disable_notrace();
> > 
> >  	bit = trace_test_and_set_recursion(TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> > -- 
> > 1.7.10.4
> > 
> > 

Re: [RFC][PATCH 11/18 v2] ftrace: Adde infrastructure to stop RCU unsafe checker from checking

[Paul E. McKenney]

On Tue, Sep 03, 2013 at 09:43:52AM -0400, Steven Rostedt wrote:
> On Sat, 31 Aug 2013 12:52:58 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > On Sat, Aug 31, 2013 at 01:11:28AM -0400, Steven Rostedt wrote:
> > > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > > 
> > > This is a light weight way to keep the rcu checker from checking
> > > RCU safety. It adds a ftrace_unsafe_rcu_checker_disable/enable()
> > > that increments or decrements a counter respectively. When the
> > > counter is set, the RCU unsafe checker callback does not run the
> > > tests to see if RCU is safe or not.
> > 
> > Please add something saying what we do instead of testing RCU safety.
> > Looks to me like it skips not only the tests, but also invoking the
> > callback, but I could easily be wrong.
> 
> Now that I'm awake and also not on holiday, I can reply with a clear
> mind.
> 
> When disabled, it forces the callback to return immediately. I may in
> the future, have it disable the callback altogether.
> 
> Yes, we can miss checks, but it's better than the test live locking
> the system :-)
> 
> > 
> > > This is required by the graph tracer because the checks can cause
> > > the graph tracer to live lock the system by its own calls.
> 
> I added this (I quoted the added text):
> 
>     This is a light weight way to keep the rcu checker from checking
>     RCU safety. It adds a ftrace_unsafe_rcu_checker_disable/enable()
>     that increments or decrements a counter respectively. When the
>     counter is set, the RCU unsafe checker callback does not run the
>     tests to see if RCU is safe or not. "But the callback still gets
>     called. It just does not call rcu_read_(un)lock()."
>     
>     This is required by the graph tracer because the checks can cause
>     the graph tracer to live lock the system by its own calls. "That is,
>     the graph tracer will still trace rcu and the rcu debugging, and
>     this will slow down the checker, which is still calling all other
>     functions (in interrupts and faults), which can cause the timer
>     interrupt to take oven a millisecond to complete, and it will then
>     trigger once it finishes, live locking the system."
> 
> 
> > > 
> > > It's also needed by the irqsoff tracer, because it may be called
> > > in RCU unsafe regions and if its internal functions get traced
> > > then the RCU unsafe checker may have some false positives.
> > > 
> > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > 
> > With the augmented commit log as noted above:
> 
> Is the above OK?

Works for me!

							Thanx, Paul

> -- Steve
> 
> > 
> > Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > > ---
> > >  kernel/trace/trace.h           |   12 +++++++++---
> > >  kernel/trace/trace_functions.c |   15 +++++++++++++++
> > >  2 files changed, 24 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > > index e551316..58e4c37 100644
> > > --- a/kernel/trace/trace.h
> > > +++ b/kernel/trace/trace.h
> > > @@ -760,9 +760,6 @@ static inline int ftrace_graph_addr(unsigned long addr)
> > > 
> > >  	return 0;
> > >  }
> > > -#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> > > -extern bool ftrace_rcu_unsafe(unsigned long addr);
> > > -#endif
> > >  #else
> > >  static inline int ftrace_graph_addr(unsigned long addr)
> > >  {
> > > @@ -1061,4 +1058,13 @@ int perf_ftrace_event_register(struct ftrace_event_call *call,
> > >  #define perf_ftrace_event_register NULL
> > >  #endif
> > > 
> > > +#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> > > +extern bool ftrace_rcu_unsafe(unsigned long addr);
> > > +extern void ftrace_unsafe_rcu_checker_disable(void);
> > > +extern void ftrace_unsafe_rcu_checker_enable(void);
> > > +#else
> > > +static inline void ftrace_unsafe_rcu_checker_disable(void) { }
> > > +static inline void ftrace_unsafe_rcu_checker_enable(void) { }
> > > +#endif
> > > +
> > >  #endif /* _LINUX_KERNEL_TRACE_H */
> > > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> > > index 9dd4627..1d5f951 100644
> > > --- a/kernel/trace/trace_functions.c
> > > +++ b/kernel/trace/trace_functions.c
> > > @@ -560,12 +560,27 @@ static inline int init_func_cmd_traceon(void)
> > >  #endif /* CONFIG_DYNAMIC_FTRACE */
> > > 
> > >  #ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> > > +static atomic_t ftrace_unsafe_rcu_disabled;
> > > +
> > > +void ftrace_unsafe_rcu_checker_disable(void)
> > > +{
> > > +	atomic_inc(&ftrace_unsafe_rcu_disabled);
> > > +}
> > > +
> > > +void ftrace_unsafe_rcu_checker_enable(void)
> > > +{
> > > +	atomic_dec(&ftrace_unsafe_rcu_disabled);
> > > +}
> > > +
> > >  static void
> > >  ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
> > >  		       struct ftrace_ops *op, struct pt_regs *pt_regs)
> > >  {
> > >  	int bit;
> > > 
> > > +	if (atomic_read(&ftrace_unsafe_rcu_disabled))
> > > +		return;
> > > +
> > >  	preempt_disable_notrace();
> > > 
> > >  	bit = trace_test_and_set_recursion(TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> > > -- 
> > > 1.7.10.4
> > > 
> > > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

[RFC][PATCH 12/18 v2] ftrace: Disable RCU unsafe checker when function graph is enabled

[Steven Rostedt]

[-- Attachment #1: 0012-ftrace-Disable-RCU-unsafe-checker-when-function-grap.patch --]
[-- Type: text/plain, Size: 1410 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Having the RCU unsafe checker running when function graph is enabled
can cause a live lock. That's because the RCU unsafe checker enables
full lockdep debugging on RCU which does a lot of interal calls that
may be traced by the function graph tacer. This adds quite a bit of
overhead and can possibly live lock the system.

Just do not do the RCU unsafe checks when function graph tracer is
enabled.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 69b7f62..310b727 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5118,6 +5118,12 @@ int register_ftrace_graph(trace_func_graph_ret_t retfunc,
 		goto out;
 	}
 
+	/*
+	 * The Unsafe RCU checker can live lock function graph tracing.
+	 * It's best to just disable it while doing the fgraph tracing.
+	 */
+	ftrace_unsafe_rcu_checker_disable();
+
 	ftrace_graph_return = retfunc;
 	ftrace_graph_entry = entryfunc;
 
@@ -5141,6 +5147,7 @@ void unregister_ftrace_graph(void)
 	ftrace_shutdown(&global_ops, FTRACE_STOP_FUNC_RET);
 	unregister_pm_notifier(&ftrace_suspend_notifier);
 	unregister_trace_sched_switch(ftrace_graph_probe_sched_switch, NULL);
+	ftrace_unsafe_rcu_checker_enable();
 
  out:
 	mutex_unlock(&ftrace_lock);
-- 
1.7.10.4

Re: [RFC][PATCH 12/18 v2] ftrace: Disable RCU unsafe checker when function graph is enabled

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:29AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Having the RCU unsafe checker running when function graph is enabled
> can cause a live lock. That's because the RCU unsafe checker enables
> full lockdep debugging on RCU which does a lot of interal calls that
> may be traced by the function graph tacer. This adds quite a bit of

s/tacer/tracer/  (Yeah, yeah, picky, picky!)

> overhead and can possibly live lock the system.
> 
> Just do not do the RCU unsafe checks when function graph tracer is
> enabled.
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

One question: How does the user/tester/developer know that RCU-unsafe
checks have been disabled by function-graph tracing?  Would it make
sense to print something to dmesg calling this out?  Or do the transitions
happen too often?

							Thanx, Paul

> ---
>  kernel/trace/ftrace.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 69b7f62..310b727 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5118,6 +5118,12 @@ int register_ftrace_graph(trace_func_graph_ret_t retfunc,
>  		goto out;
>  	}
> 
> +	/*
> +	 * The Unsafe RCU checker can live lock function graph tracing.
> +	 * It's best to just disable it while doing the fgraph tracing.
> +	 */
> +	ftrace_unsafe_rcu_checker_disable();
> +
>  	ftrace_graph_return = retfunc;
>  	ftrace_graph_entry = entryfunc;
> 
> @@ -5141,6 +5147,7 @@ void unregister_ftrace_graph(void)
>  	ftrace_shutdown(&global_ops, FTRACE_STOP_FUNC_RET);
>  	unregister_pm_notifier(&ftrace_suspend_notifier);
>  	unregister_trace_sched_switch(ftrace_graph_probe_sched_switch, NULL);
> +	ftrace_unsafe_rcu_checker_enable();
> 
>   out:
>  	mutex_unlock(&ftrace_lock);
> -- 
> 1.7.10.4
> 
> 

Re: [RFC][PATCH 12/18 v2] ftrace: Disable RCU unsafe checker when function graph is enabled

[Steven Rostedt]

On Sat, 2013-08-31 at 12:55 -0700, Paul E. McKenney wrote:
> On Sat, Aug 31, 2013 at 01:11:29AM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > 
> > Having the RCU unsafe checker running when function graph is enabled
> > can cause a live lock. That's because the RCU unsafe checker enables
> > full lockdep debugging on RCU which does a lot of interal calls that
> > may be traced by the function graph tacer. This adds quite a bit of
> 
> s/tacer/tracer/  (Yeah, yeah, picky, picky!)
> 
> > overhead and can possibly live lock the system.
> > 
> > Just do not do the RCU unsafe checks when function graph tracer is
> > enabled.
> > 
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> One question: How does the user/tester/developer know that RCU-unsafe
> checks have been disabled by function-graph tracing?  Would it make
> sense to print something to dmesg calling this out?  Or do the transitions
> happen too often?

Ideally, (for 3.13) I plan on having this be a sysctl switch. That will
be 1 or 0, depending on if it is enabled or not. But yeah, I can add a
printk to show that the kernel changed it. It doesn't happen often, and
mainly by a user (or selftest).

-- Steve

> 
> 							Thanx, Paul
> 
> > ---

Re: [RFC][PATCH 12/18 v2] ftrace: Disable RCU unsafe checker when function graph is enabled

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 04:42:17PM -0400, Steven Rostedt wrote:
> On Sat, 2013-08-31 at 12:55 -0700, Paul E. McKenney wrote:
> > On Sat, Aug 31, 2013 at 01:11:29AM -0400, Steven Rostedt wrote:
> > > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > > 
> > > Having the RCU unsafe checker running when function graph is enabled
> > > can cause a live lock. That's because the RCU unsafe checker enables
> > > full lockdep debugging on RCU which does a lot of interal calls that
> > > may be traced by the function graph tacer. This adds quite a bit of
> > 
> > s/tacer/tracer/  (Yeah, yeah, picky, picky!)
> > 
> > > overhead and can possibly live lock the system.
> > > 
> > > Just do not do the RCU unsafe checks when function graph tracer is
> > > enabled.
> > > 
> > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > 
> > One question: How does the user/tester/developer know that RCU-unsafe
> > checks have been disabled by function-graph tracing?  Would it make
> > sense to print something to dmesg calling this out?  Or do the transitions
> > happen too often?
> 
> Ideally, (for 3.13) I plan on having this be a sysctl switch. That will
> be 1 or 0, depending on if it is enabled or not. But yeah, I can add a
> printk to show that the kernel changed it. It doesn't happen often, and
> mainly by a user (or selftest).

It would be good.  Sooner or later someone is going to miss the fact
that they added an unsafe RCU read-side code sequence because they had
the checks suppressed during testing.  It would be good to have a way
to see that this happened.  ;-)

							Thanx, Paul

[RFC][PATCH 13/18 v2] ftrace: Disable the RCU unsafe checker when irqsoff is enabled

[Steven Rostedt]

[-- Attachment #1: 0013-ftrace-Disable-the-RCU-unsafe-checker-when-irqsoff-i.patch --]
[-- Type: text/plain, Size: 1984 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

The irqsoff tracer can be called during some of the RCU unsafe
regions. The proble is that some of the internal calls that it
makes may also be traced. For example, it uses spin locks. But if
the spin lock gets traced and the RCU unsafe checker runs, it will
trigger that RCU is not safe to use. But the only reason a spin lock
is being used in an RCU unsafe region is because the irqsoff trace
uses it, and causes a false positive.

Disable the unsafe RCU checker when irqsoff is enabled.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_irqsoff.c |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c
index 2aefbee..62d603c 100644
--- a/kernel/trace/trace_irqsoff.c
+++ b/kernel/trace/trace_irqsoff.c
@@ -584,9 +584,14 @@ static int start_irqsoff_tracer(struct trace_array *tr, int graph)
 
 	ret = register_irqsoff_function(graph, 0);
 
-	if (!ret && tracing_is_enabled())
+	if (!ret && tracing_is_enabled()) {
+		/*
+		 * irqsoff tracer can cause unsafe rcu checker
+		 * to have false positives.
+		 */
+		ftrace_unsafe_rcu_checker_disable();
 		tracer_enabled = 1;
-	else
+	} else
 		tracer_enabled = 0;
 
 	return ret;
@@ -594,6 +599,9 @@ static int start_irqsoff_tracer(struct trace_array *tr, int graph)
 
 static void stop_irqsoff_tracer(struct trace_array *tr, int graph)
 {
+	if (tracer_enabled)
+		ftrace_unsafe_rcu_checker_enable();
+
 	tracer_enabled = 0;
 
 	unregister_irqsoff_function(graph);
@@ -630,11 +638,15 @@ static void irqsoff_tracer_reset(struct trace_array *tr)
 
 static void irqsoff_tracer_start(struct trace_array *tr)
 {
+	if (!tracer_enabled)
+		ftrace_unsafe_rcu_checker_disable();
 	tracer_enabled = 1;
 }
 
 static void irqsoff_tracer_stop(struct trace_array *tr)
 {
+	if (tracer_enabled)
+		ftrace_unsafe_rcu_checker_enable();
 	tracer_enabled = 0;
 }
 
-- 
1.7.10.4

Re: [RFC][PATCH 13/18 v2] ftrace: Disable the RCU unsafe checker when irqsoff is enabled

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:30AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> The irqsoff tracer can be called during some of the RCU unsafe
> regions. The proble is that some of the internal calls that it

s/proble/problem/

> makes may also be traced. For example, it uses spin locks. But if
> the spin lock gets traced and the RCU unsafe checker runs, it will
> trigger that RCU is not safe to use. But the only reason a spin lock
> is being used in an RCU unsafe region is because the irqsoff trace
> uses it, and causes a false positive.
> 
> Disable the unsafe RCU checker when irqsoff is enabled.
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Looks like a good compromise, as did the previous one involving the
function graph tracer.  Again, though, should we do something to
indicate that RCU safety checks have been disabled?

							Thanx, Paul

> ---
>  kernel/trace/trace_irqsoff.c |   16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c
> index 2aefbee..62d603c 100644
> --- a/kernel/trace/trace_irqsoff.c
> +++ b/kernel/trace/trace_irqsoff.c
> @@ -584,9 +584,14 @@ static int start_irqsoff_tracer(struct trace_array *tr, int graph)
> 
>  	ret = register_irqsoff_function(graph, 0);
> 
> -	if (!ret && tracing_is_enabled())
> +	if (!ret && tracing_is_enabled()) {
> +		/*
> +		 * irqsoff tracer can cause unsafe rcu checker
> +		 * to have false positives.
> +		 */
> +		ftrace_unsafe_rcu_checker_disable();
>  		tracer_enabled = 1;
> -	else
> +	} else
>  		tracer_enabled = 0;
> 
>  	return ret;
> @@ -594,6 +599,9 @@ static int start_irqsoff_tracer(struct trace_array *tr, int graph)
> 
>  static void stop_irqsoff_tracer(struct trace_array *tr, int graph)
>  {
> +	if (tracer_enabled)
> +		ftrace_unsafe_rcu_checker_enable();
> +
>  	tracer_enabled = 0;
> 
>  	unregister_irqsoff_function(graph);
> @@ -630,11 +638,15 @@ static void irqsoff_tracer_reset(struct trace_array *tr)
> 
>  static void irqsoff_tracer_start(struct trace_array *tr)
>  {
> +	if (!tracer_enabled)
> +		ftrace_unsafe_rcu_checker_disable();
>  	tracer_enabled = 1;
>  }
> 
>  static void irqsoff_tracer_stop(struct trace_array *tr)
>  {
> +	if (tracer_enabled)
> +		ftrace_unsafe_rcu_checker_enable();
>  	tracer_enabled = 0;
>  }
> 
> -- 
> 1.7.10.4
> 
> 

[RFC][PATCH 14/18 v2] ftrace/lockdep: Have the RCU lockdep splat show what function triggered

[Steven Rostedt]

[-- Attachment #1: 0014-ftrace-lockdep-Have-the-RCU-lockdep-splat-show-what-.patch --]
[-- Type: text/plain, Size: 2905 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

When the RCU lockdep splat hits because of the unsafe RCU checker,
the backtrace does not always show the culprit. But the culprit was
passed to the unsafe RCU checker.

Save the ip of the function being traced in a per_cpu variable, and
when the RCU lockdep detects a problem, it can also print out what
function was being traced if it was caused by the unsafe RCU checker.

Cc:  Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace.h         |    6 ++++++
 kernel/lockdep.c               |    2 ++
 kernel/trace/trace_functions.c |   14 ++++++++++++++
 3 files changed, 22 insertions(+)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 4752764..3dbb946 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -885,4 +885,10 @@ unsigned long arch_syscall_addr(int nr);
 
 #endif /* CONFIG_FTRACE_SYSCALLS */
 
+#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
+extern void print_ftrace_rcu_func(int cpu);
+#else
+static inline void print_ftrace_rcu_func(int cpu) { }
+#endif
+
 #endif /* _LINUX_FTRACE_H */
diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index e16c45b..74272ed 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -4229,6 +4229,8 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)
 				: "",
 	       rcu_scheduler_active, debug_locks);
 
+	print_ftrace_rcu_func(raw_smp_processor_id());
+
 	/*
 	 * If a CPU is in the RCU-free window in idle (ie: in the section
 	 * between rcu_idle_enter() and rcu_idle_exit(), then RCU
diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index 1d5f951..f5a9031 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -560,6 +560,7 @@ static inline int init_func_cmd_traceon(void)
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
 #ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
+
 static atomic_t ftrace_unsafe_rcu_disabled;
 
 void ftrace_unsafe_rcu_checker_disable(void)
@@ -572,6 +573,17 @@ void ftrace_unsafe_rcu_checker_enable(void)
 	atomic_dec(&ftrace_unsafe_rcu_disabled);
 }
 
+static DEFINE_PER_CPU(unsigned long, ftrace_rcu_func);
+
+void print_ftrace_rcu_func(int cpu)
+{
+	unsigned long ip = per_cpu(ftrace_rcu_func, cpu);
+
+	if (ip)
+		printk("ftrace_rcu_func: %pS\n",
+		       (void *)per_cpu(ftrace_rcu_func, cpu));
+}
+
 static void
 ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
 		       struct ftrace_ops *op, struct pt_regs *pt_regs)
@@ -592,9 +604,11 @@ ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
 		      (void *)ip))
 		goto out;
 
+	this_cpu_write(ftrace_rcu_func, ip);
 	/* Should trigger a RCU splat if called from unsafe RCU function */
 	rcu_read_lock();
 	rcu_read_unlock();
+	this_cpu_write(ftrace_rcu_func, 0);
 
 	trace_clear_recursion(bit);
  out:
-- 
1.7.10.4

Re: [RFC][PATCH 14/18 v2] ftrace/lockdep: Have the RCU lockdep splat show what function triggered

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:31AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> When the RCU lockdep splat hits because of the unsafe RCU checker,
> the backtrace does not always show the culprit. But the culprit was
> passed to the unsafe RCU checker.
> 
> Save the ip of the function being traced in a per_cpu variable, and
> when the RCU lockdep detects a problem, it can also print out what
> function was being traced if it was caused by the unsafe RCU checker.
> 
> Cc:  Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  include/linux/ftrace.h         |    6 ++++++
>  kernel/lockdep.c               |    2 ++
>  kernel/trace/trace_functions.c |   14 ++++++++++++++
>  3 files changed, 22 insertions(+)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 4752764..3dbb946 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -885,4 +885,10 @@ unsigned long arch_syscall_addr(int nr);
> 
>  #endif /* CONFIG_FTRACE_SYSCALLS */
> 
> +#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> +extern void print_ftrace_rcu_func(int cpu);
> +#else
> +static inline void print_ftrace_rcu_func(int cpu) { }
> +#endif
> +
>  #endif /* _LINUX_FTRACE_H */
> diff --git a/kernel/lockdep.c b/kernel/lockdep.c
> index e16c45b..74272ed 100644
> --- a/kernel/lockdep.c
> +++ b/kernel/lockdep.c
> @@ -4229,6 +4229,8 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)
>  				: "",
>  	       rcu_scheduler_active, debug_locks);
> 
> +	print_ftrace_rcu_func(raw_smp_processor_id());
> +
>  	/*
>  	 * If a CPU is in the RCU-free window in idle (ie: in the section
>  	 * between rcu_idle_enter() and rcu_idle_exit(), then RCU
> diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> index 1d5f951..f5a9031 100644
> --- a/kernel/trace/trace_functions.c
> +++ b/kernel/trace/trace_functions.c
> @@ -560,6 +560,7 @@ static inline int init_func_cmd_traceon(void)
>  #endif /* CONFIG_DYNAMIC_FTRACE */
> 
>  #ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> +
>  static atomic_t ftrace_unsafe_rcu_disabled;
> 
>  void ftrace_unsafe_rcu_checker_disable(void)
> @@ -572,6 +573,17 @@ void ftrace_unsafe_rcu_checker_enable(void)
>  	atomic_dec(&ftrace_unsafe_rcu_disabled);
>  }
> 
> +static DEFINE_PER_CPU(unsigned long, ftrace_rcu_func);
> +
> +void print_ftrace_rcu_func(int cpu)
> +{
> +	unsigned long ip = per_cpu(ftrace_rcu_func, cpu);
> +
> +	if (ip)
> +		printk("ftrace_rcu_func: %pS\n",
> +		       (void *)per_cpu(ftrace_rcu_func, cpu));
> +}
> +
>  static void
>  ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
>  		       struct ftrace_ops *op, struct pt_regs *pt_regs)
> @@ -592,9 +604,11 @@ ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
>  		      (void *)ip))
>  		goto out;
> 
> +	this_cpu_write(ftrace_rcu_func, ip);
>  	/* Should trigger a RCU splat if called from unsafe RCU function */
>  	rcu_read_lock();
>  	rcu_read_unlock();
> +	this_cpu_write(ftrace_rcu_func, 0);
> 
>  	trace_clear_recursion(bit);
>   out:
> -- 
> 1.7.10.4
> 
> 

Re: [RFC][PATCH 14/18 v2] ftrace/lockdep: Have the RCU lockdep splat show what function triggered

[Peter Zijlstra]

On Sat, Aug 31, 2013 at 01:11:31AM -0400, Steven Rostedt wrote:
> +void print_ftrace_rcu_func(int cpu)
> +{
> +	unsigned long ip = per_cpu(ftrace_rcu_func, cpu);
> +
> +	if (ip)
> +		printk("ftrace_rcu_func: %pS\n",
> +		       (void *)per_cpu(ftrace_rcu_func, cpu));
> +}

That's missing { }.

Re: [RFC][PATCH 14/18 v2] ftrace/lockdep: Have the RCU lockdep splat show what function triggered

[Steven Rostedt]

On Thu, 5 Sep 2013 21:18:39 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Sat, Aug 31, 2013 at 01:11:31AM -0400, Steven Rostedt wrote:
> > +void print_ftrace_rcu_func(int cpu)
> > +{
> > +	unsigned long ip = per_cpu(ftrace_rcu_func, cpu);
> > +
> > +	if (ip)
> > +		printk("ftrace_rcu_func: %pS\n",
> > +		       (void *)per_cpu(ftrace_rcu_func, cpu));
> > +}
> 
> That's missing { }.

Hmm, that's an interesting point. Why the  { } because I break up the
printk for the 80 character limit?

Although, I'm still not convinced that it needs { }, as it looks to me
that it flows nicely without it. I can't find a place in CodingStyle
that says { } are needed here.

-- Steve

Re: [RFC][PATCH 14/18 v2] ftrace/lockdep: Have the RCU lockdep splat show what function triggered

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 5 Sep 2013 21:18:39 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Sat, Aug 31, 2013 at 01:11:31AM -0400, Steven Rostedt wrote:
> > > +void print_ftrace_rcu_func(int cpu)
> > > +{
> > > +	unsigned long ip = per_cpu(ftrace_rcu_func, cpu);
> > > +
> > > +	if (ip)
> > > +		printk("ftrace_rcu_func: %pS\n",
> > > +		       (void *)per_cpu(ftrace_rcu_func, cpu));
> > > +}
> > 
> > That's missing { }.
> 
> Hmm, that's an interesting point. Why the  { } because I break up the
> printk for the 80 character limit?

You probably shouldn't break it up - it looks uglier.

> Although, I'm still not convinced that it needs { }, as it looks to me 
> that it flows nicely without it. I can't find a place in CodingStyle 
> that says { } are needed here.

it's somewhat of a grey area - the section quoted below talks about it 
broadly - and it's typically understood to apply to multi-line statements 
as well, as it's easy to overlook and confuse multi-statements with 
multi-line statements...

Thanks,

	Ingo

----------------------->

Do not unnecessarily use braces where a single statement will do.

if (condition)
        action();

and

if (condition)
        do_this();
else
        do_that();

This does not apply if only one branch of a conditional statement is a 
single statement; in the latter case use braces in both branches:

if (condition) {
        do_this();
        do_that();
} else {
        otherwise();
}

Re: [RFC][PATCH 14/18 v2] ftrace/lockdep: Have the RCU lockdep splat show what function triggered

[Steven Rostedt]

On Fri, 6 Sep 2013 14:57:19 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Thu, 5 Sep 2013 21:18:39 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > On Sat, Aug 31, 2013 at 01:11:31AM -0400, Steven Rostedt wrote:
> > > > +void print_ftrace_rcu_func(int cpu)
> > > > +{
> > > > +	unsigned long ip = per_cpu(ftrace_rcu_func, cpu);
> > > > +
> > > > +	if (ip)
> > > > +		printk("ftrace_rcu_func: %pS\n",
> > > > +		       (void *)per_cpu(ftrace_rcu_func, cpu));
> > > > +}
> > > 
> > > That's missing { }.
> > 
> > Hmm, that's an interesting point. Why the  { } because I break up the
> > printk for the 80 character limit?
> 
> You probably shouldn't break it up - it looks uglier.

I thought for printk's it was fine to break after the comma, just not
the printk format line. That is,

	printk("ftrace_rcu_func: %pS\n", (void *)per_cpu(ftrace_rcu_func, cpu));

can go to:

	printk("ftrace_rcu_func: %pS\n",
	       (void *)per_cpu(ftrace_rcu_func, cpu));

But

	printk("this is a really long line and it goes on forever and might be too much to break up\n");

can't go to:

	printk("this is a really long line and it goes on forever and"
		" might be too much to break up\n");

> 
> > Although, I'm still not convinced that it needs { }, as it looks to me 
> > that it flows nicely without it. I can't find a place in CodingStyle 
> > that says { } are needed here.
> 
> it's somewhat of a grey area - the section quoted below talks about it 
> broadly - and it's typically understood to apply to multi-line statements 
> as well, as it's easy to overlook and confuse multi-statements with 
> multi-line statements...

Yeah, I read that part too.

Anyway, this conversation is all moot, as the patch is on hold, and we
may have a better way to solve this anyway, making the patch obsolete.

Thanks,

-- Steve

Re: [RFC][PATCH 14/18 v2] ftrace/lockdep: Have the RCU lockdep splat show what function triggered

[Peter Zijlstra]

On Sat, Aug 31, 2013 at 01:11:31AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Why put RHT in there? Surely greg and jonathan know you work for them?
:-)

> When the RCU lockdep splat hits because of the unsafe RCU checker,
> the backtrace does not always show the culprit. But the culprit was
> passed to the unsafe RCU checker.
> 
> Save the ip of the function being traced in a per_cpu variable, and
> when the RCU lockdep detects a problem, it can also print out what
> function was being traced if it was caused by the unsafe RCU checker.

So the below is an example of why this_cpu_* is utter shite, what
ensures the code there isn't preemptible.

That said, I suppose you've thought about that and there's something
obvious from the callpath but I can't be bothered to go hunt through the
series if the changelog can't be bothered to clarify such things.

> @@ -592,9 +604,11 @@ ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
>  		      (void *)ip))
>  		goto out;
>  
> +	this_cpu_write(ftrace_rcu_func, ip);
>  	/* Should trigger a RCU splat if called from unsafe RCU function */
>  	rcu_read_lock();
>  	rcu_read_unlock();
> +	this_cpu_write(ftrace_rcu_func, 0);
>  
>  	trace_clear_recursion(bit);
>   out:

I suppose this is all ok. I haven't read the entire series and I'm not
going to during my vacation.

One quick question though, why do we have to mark functions and can't
rely on the state RCU already keeps? Surely RCU knows when its 'enabled'
and thus safe to use RCU -- if only for debuggin.

For instance that scheduler_ipi() call can happen in either state, do we
really have to disallow it always?

Anyway, I suppose ACK on both these patches you asked me to look at, not
particularly harmful it seems, just not something I feel I've got me
head round atm.

Re: [RFC][PATCH 14/18 v2] ftrace/lockdep: Have the RCU lockdep splat show what function triggered

[Steven Rostedt]

On Thu, 5 Sep 2013 21:35:53 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Sat, Aug 31, 2013 at 01:11:31AM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Why put RHT in there? Surely greg and jonathan know you work for them?
> :-)
> 

It has nothing to do with Greg or Jonathan. Even though I think they
both told me the same thing. It has to do with anyone looking at a git
commit, and they will know that my work on this commit was sponsored by
Red Hat. Has nothing to do with those silly stat numbers that come out
every release ;-)



> > When the RCU lockdep splat hits because of the unsafe RCU checker,
> > the backtrace does not always show the culprit. But the culprit was
> > passed to the unsafe RCU checker.
> > 
> > Save the ip of the function being traced in a per_cpu variable, and
> > when the RCU lockdep detects a problem, it can also print out what
> > function was being traced if it was caused by the unsafe RCU checker.
> 
> So the below is an example of why this_cpu_* is utter shite, what
> ensures the code there isn't preemptible.

No disagreements with me about the utter shite, but what ensures it is
what is hidden from the patch. If I expand the code a little, I get
this:

        preempt_disable_notrace();

        if (this_cpu_read(ftrace_rcu_running))
                goto out;

        if (WARN_ONCE(ftrace_rcu_unsafe(ip),
                      "UNSAFE RCU function called %pS",
                      (void *)ip))
                goto out;

        this_cpu_write(ftrace_rcu_running, 1);
        this_cpu_write(ftrace_rcu_func, ip);

        /* Should trigger a RCU splat if called from unsafe RCU
        function */ rcu_read_lock();
        rcu_read_unlock();
        this_cpu_write(ftrace_rcu_func, 0);

        this_cpu_write(ftrace_rcu_running, 0);
 out:
        preempt_enable_notrace();



> 
> That said, I suppose you've thought about that and there's something
> obvious from the callpath but I can't be bothered to go hunt through the
> series if the changelog can't be bothered to clarify such things.
> 
> > @@ -592,9 +604,11 @@ ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
> >  		      (void *)ip))
> >  		goto out;
> >  
> > +	this_cpu_write(ftrace_rcu_func, ip);
> >  	/* Should trigger a RCU splat if called from unsafe RCU function */
> >  	rcu_read_lock();
> >  	rcu_read_unlock();
> > +	this_cpu_write(ftrace_rcu_func, 0);
> >  
> >  	trace_clear_recursion(bit);
> >   out:
> 
> I suppose this is all ok. I haven't read the entire series and I'm not
> going to during my vacation.
> 
> One quick question though, why do we have to mark functions and can't
> rely on the state RCU already keeps? Surely RCU knows when its 'enabled'
> and thus safe to use RCU -- if only for debuggin.

Funny you should say that. I just recently asked Paul about this very
topic. It may make most of this patch series moot.

> 
> For instance that scheduler_ipi() call can happen in either state, do we
> really have to disallow it always?

Nope! The one thing I fear with this method is, is the inconsistency of
seeing the scheduler_ipi() traced, and not seeing it. I predict getting
nasty bug reports from people telling me that the tracer is broken.

"why don't I see scheduler_ipi() traced here! I see it here!!!! The
tracer is shite!"

> 
> Anyway, I suppose ACK on both these patches you asked me to look at, not
> particularly harmful it seems, just not something I feel I've got me
> head round atm.

Thanks for taking some of your PTO out to do this! Although, it may all
be in vain if I go the RCU state route. I wont need either of these
patches then.

-- Steve

[RFC][PATCH 15/18 v2] ftrace/rcu: Mark functions that are RCU unsafe

[Steven Rostedt]

[-- Attachment #1: 0015-ftrace-rcu-Mark-functions-that-are-RCU-unsafe.patch --]
[-- Type: text/plain, Size: 2660 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Some callbacks of the function tracer use rcu_read_lock(). This means that
there's places that can not be traced because RCU is not tracking the CPU
for various reasons (like NO_HZ_FULL and coming back from userspace).

Thes functions need to be marked so that callbacks that use RCU do not
trace them.

Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/rcutiny.c |    1 +
 kernel/rcutree.c |    7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
index aa34411..911a61c 100644
--- a/kernel/rcutiny.c
+++ b/kernel/rcutiny.c
@@ -173,6 +173,7 @@ void rcu_irq_enter(void)
 	local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(rcu_irq_enter);
+FTRACE_UNSAFE_RCU(rcu_irq_enter);
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 
diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index 068de3a..ca53562 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -53,6 +53,7 @@
 #include <linux/delay.h>
 #include <linux/stop_machine.h>
 #include <linux/random.h>
+#include <linux/ftrace.h>
 
 #include "rcutree.h"
 #include <trace/events/rcu.h>
@@ -373,6 +374,7 @@ static void rcu_eqs_enter_common(struct rcu_dynticks *rdtp, long long oldval,
 	rcu_lockdep_assert(!lock_is_held(&rcu_sched_lock_map),
 			   "Illegal idle entry in RCU-sched read-side critical section.");
 }
+FTRACE_UNSAFE_RCU(rcu_eqs_enter_common);
 
 /*
  * Enter an RCU extended quiescent state, which can be either the
@@ -392,6 +394,7 @@ static void rcu_eqs_enter(bool user)
 		rdtp->dynticks_nesting -= DYNTICK_TASK_NEST_VALUE;
 	rcu_eqs_enter_common(rdtp, oldval, user);
 }
+FTRACE_UNSAFE_RCU(rcu_eqs_enter);
 
 /**
  * rcu_idle_enter - inform RCU that current CPU is entering idle
@@ -513,6 +516,7 @@ static void rcu_eqs_exit_common(struct rcu_dynticks *rdtp, long long oldval,
 			  idle->pid, idle->comm); /* must be idle task! */
 	}
 }
+FTRACE_UNSAFE_RCU(rcu_eqs_exit_common);
 
 /*
  * Exit an RCU extended quiescent state, which can be either the
@@ -553,6 +557,7 @@ void rcu_idle_exit(void)
 	local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(rcu_idle_exit);
+FTRACE_UNSAFE_RCU(rcu_idle_exit);
 
 #ifdef CONFIG_RCU_USER_QS
 /**
@@ -565,6 +570,7 @@ void rcu_user_exit(void)
 {
 	rcu_eqs_exit(1);
 }
+FTRACE_UNSAFE_RCU(rcu_user_exit);
 
 /**
  * rcu_user_exit_after_irq - inform RCU that we won't resume to userspace
@@ -625,6 +631,7 @@ void rcu_irq_enter(void)
 		rcu_eqs_exit_common(rdtp, oldval, true);
 	local_irq_restore(flags);
 }
+FTRACE_UNSAFE_RCU(rcu_irq_enter);
 
 /**
  * rcu_nmi_enter - inform RCU of entry to NMI context
-- 
1.7.10.4

Re: [RFC][PATCH 15/18 v2] ftrace/rcu: Mark functions that are RCU unsafe

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:32AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Some callbacks of the function tracer use rcu_read_lock(). This means that
> there's places that can not be traced because RCU is not tracking the CPU
> for various reasons (like NO_HZ_FULL and coming back from userspace).
> 
> Thes functions need to be marked so that callbacks that use RCU do not
> trace them.
> 
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Please let me know if you would like me to take this one.  If I don't
hear otherwise, I will assume that you are pushing it.

							Thanx, Paul

> ---
>  kernel/rcutiny.c |    1 +
>  kernel/rcutree.c |    7 +++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
> index aa34411..911a61c 100644
> --- a/kernel/rcutiny.c
> +++ b/kernel/rcutiny.c
> @@ -173,6 +173,7 @@ void rcu_irq_enter(void)
>  	local_irq_restore(flags);
>  }
>  EXPORT_SYMBOL_GPL(rcu_irq_enter);
> +FTRACE_UNSAFE_RCU(rcu_irq_enter);
> 
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> 
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index 068de3a..ca53562 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -53,6 +53,7 @@
>  #include <linux/delay.h>
>  #include <linux/stop_machine.h>
>  #include <linux/random.h>
> +#include <linux/ftrace.h>
> 
>  #include "rcutree.h"
>  #include <trace/events/rcu.h>
> @@ -373,6 +374,7 @@ static void rcu_eqs_enter_common(struct rcu_dynticks *rdtp, long long oldval,
>  	rcu_lockdep_assert(!lock_is_held(&rcu_sched_lock_map),
>  			   "Illegal idle entry in RCU-sched read-side critical section.");
>  }
> +FTRACE_UNSAFE_RCU(rcu_eqs_enter_common);
> 
>  /*
>   * Enter an RCU extended quiescent state, which can be either the
> @@ -392,6 +394,7 @@ static void rcu_eqs_enter(bool user)
>  		rdtp->dynticks_nesting -= DYNTICK_TASK_NEST_VALUE;
>  	rcu_eqs_enter_common(rdtp, oldval, user);
>  }
> +FTRACE_UNSAFE_RCU(rcu_eqs_enter);
> 
>  /**
>   * rcu_idle_enter - inform RCU that current CPU is entering idle
> @@ -513,6 +516,7 @@ static void rcu_eqs_exit_common(struct rcu_dynticks *rdtp, long long oldval,
>  			  idle->pid, idle->comm); /* must be idle task! */
>  	}
>  }
> +FTRACE_UNSAFE_RCU(rcu_eqs_exit_common);
> 
>  /*
>   * Exit an RCU extended quiescent state, which can be either the
> @@ -553,6 +557,7 @@ void rcu_idle_exit(void)
>  	local_irq_restore(flags);
>  }
>  EXPORT_SYMBOL_GPL(rcu_idle_exit);
> +FTRACE_UNSAFE_RCU(rcu_idle_exit);
> 
>  #ifdef CONFIG_RCU_USER_QS
>  /**
> @@ -565,6 +570,7 @@ void rcu_user_exit(void)
>  {
>  	rcu_eqs_exit(1);
>  }
> +FTRACE_UNSAFE_RCU(rcu_user_exit);
> 
>  /**
>   * rcu_user_exit_after_irq - inform RCU that we won't resume to userspace
> @@ -625,6 +631,7 @@ void rcu_irq_enter(void)
>  		rcu_eqs_exit_common(rdtp, oldval, true);
>  	local_irq_restore(flags);
>  }
> +FTRACE_UNSAFE_RCU(rcu_irq_enter);
> 
>  /**
>   * rcu_nmi_enter - inform RCU of entry to NMI context
> -- 
> 1.7.10.4
> 
> 

Re: [RFC][PATCH 15/18 v2] ftrace/rcu: Mark functions that are RCU unsafe

[Steven Rostedt]

On Sat, 2013-08-31 at 13:00 -0700, Paul E. McKenney wrote:
> On Sat, Aug 31, 2013 at 01:11:32AM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > 
> > Some callbacks of the function tracer use rcu_read_lock(). This means that
> > there's places that can not be traced because RCU is not tracking the CPU
> > for various reasons (like NO_HZ_FULL and coming back from userspace).
> > 
> > Thes functions need to be marked so that callbacks that use RCU do not
> > trace them.
> > 
> > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> Please let me know if you would like me to take this one.  If I don't
> hear otherwise, I will assume that you are pushing it.
> 

These I rather take, as they are all dependent on the previous changes.

Thanks!

-- Steve

Re: [RFC][PATCH 15/18 v2] ftrace/rcu: Mark functions that are RCU unsafe

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 04:43:24PM -0400, Steven Rostedt wrote:
> On Sat, 2013-08-31 at 13:00 -0700, Paul E. McKenney wrote:
> > On Sat, Aug 31, 2013 at 01:11:32AM -0400, Steven Rostedt wrote:
> > > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > > 
> > > Some callbacks of the function tracer use rcu_read_lock(). This means that
> > > there's places that can not be traced because RCU is not tracking the CPU
> > > for various reasons (like NO_HZ_FULL and coming back from userspace).
> > > 
> > > Thes functions need to be marked so that callbacks that use RCU do not
> > > trace them.
> > > 
> > > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > 
> > Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > Please let me know if you would like me to take this one.  If I don't
> > hear otherwise, I will assume that you are pushing it.
> > 
> 
> These I rather take, as they are all dependent on the previous changes.

Good point, applies to the other one we discussed as well.

							Thanx, Paul

[RFC][PATCH 16/18 v2] rcu/irq/x86: Mark functions that are RCU unsafe

[Steven Rostedt]

[-- Attachment #1: 0016-rcu-irq-x86-Mark-functions-that-are-RCU-unsafe.patch --]
[-- Type: text/plain, Size: 5035 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Some callbacks of the function tracer use rcu_read_lock(). This means that
there's places that can not be traced because RCU is not tracking the CPU
for various reasons (like NO_HZ_FULL and coming back from userspace).

Thes functions need to be marked so that callbacks that use RCU do not
trace them.

Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/apic/apic.c |    2 ++
 arch/x86/kernel/irq.c       |    1 +
 arch/x86/kernel/irq_work.c  |    3 +++
 arch/x86/kernel/smp.c       |    8 ++++++++
 kernel/softirq.c            |    2 ++
 5 files changed, 16 insertions(+)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index eca89c5..91af16b 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -931,6 +931,7 @@ void __irq_entry smp_apic_timer_interrupt(struct pt_regs *regs)
 
 	set_irq_regs(old_regs);
 }
+FTRACE_UNSAFE_RCU(smp_apic_timer_interrupt);
 
 void __irq_entry smp_trace_apic_timer_interrupt(struct pt_regs *regs)
 {
@@ -952,6 +953,7 @@ void __irq_entry smp_trace_apic_timer_interrupt(struct pt_regs *regs)
 
 	set_irq_regs(old_regs);
 }
+FTRACE_UNSAFE_RCU(smp_trace_apic_timer_interrupt);
 
 int setup_profiling_timer(unsigned int multiplier)
 {
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 3a8185c..fccd0d1 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -203,6 +203,7 @@ unsigned int __irq_entry do_IRQ(struct pt_regs *regs)
 	set_irq_regs(old_regs);
 	return 1;
 }
+FTRACE_UNSAFE_RCU(do_IRQ);
 
 /*
  * Handler for X86_PLATFORM_IPI_VECTOR.
diff --git a/arch/x86/kernel/irq_work.c b/arch/x86/kernel/irq_work.c
index 636a55e..a2199c2 100644
--- a/arch/x86/kernel/irq_work.c
+++ b/arch/x86/kernel/irq_work.c
@@ -7,6 +7,7 @@
 #include <linux/kernel.h>
 #include <linux/irq_work.h>
 #include <linux/hardirq.h>
+#include <linux/ftrace.h>
 #include <asm/apic.h>
 #include <asm/trace/irq_vectors.h>
 
@@ -28,6 +29,7 @@ void smp_irq_work_interrupt(struct pt_regs *regs)
 	__smp_irq_work_interrupt();
 	exiting_irq();
 }
+FTRACE_UNSAFE_RCU(smp_irq_work_interrupt);
 
 void smp_trace_irq_work_interrupt(struct pt_regs *regs)
 {
@@ -37,6 +39,7 @@ void smp_trace_irq_work_interrupt(struct pt_regs *regs)
 	trace_irq_work_exit(IRQ_WORK_VECTOR);
 	exiting_irq();
 }
+FTRACE_UNSAFE_RCU(smp_trace_irq_work_interrupt);
 
 void arch_irq_work_raise(void)
 {
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index cdaa347..3d702ef 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -23,6 +23,7 @@
 #include <linux/interrupt.h>
 #include <linux/cpu.h>
 #include <linux/gfp.h>
+#include <linux/ftrace.h>
 
 #include <asm/mtrr.h>
 #include <asm/tlbflush.h>
@@ -175,6 +176,7 @@ asmlinkage void smp_reboot_interrupt(void)
 	stop_this_cpu(NULL);
 	irq_exit();
 }
+FTRACE_UNSAFE_RCU(smp_reboot_interrupt);
 
 static void native_stop_other_cpus(int wait)
 {
@@ -264,6 +266,7 @@ void smp_reschedule_interrupt(struct pt_regs *regs)
 	 * KVM uses this interrupt to force a cpu out of guest mode
 	 */
 }
+FTRACE_UNSAFE_RCU(smp_reschedule_interrupt);
 
 static inline void smp_entering_irq(void)
 {
@@ -288,6 +291,7 @@ void smp_trace_reschedule_interrupt(struct pt_regs *regs)
 	 * KVM uses this interrupt to force a cpu out of guest mode
 	 */
 }
+FTRACE_UNSAFE_RCU(smp_trace_reschedule_interrupt);
 
 static inline void __smp_call_function_interrupt(void)
 {
@@ -301,6 +305,7 @@ void smp_call_function_interrupt(struct pt_regs *regs)
 	__smp_call_function_interrupt();
 	exiting_irq();
 }
+FTRACE_UNSAFE_RCU(smp_call_function_interrupt);
 
 void smp_trace_call_function_interrupt(struct pt_regs *regs)
 {
@@ -310,6 +315,7 @@ void smp_trace_call_function_interrupt(struct pt_regs *regs)
 	trace_call_function_exit(CALL_FUNCTION_VECTOR);
 	exiting_irq();
 }
+FTRACE_UNSAFE_RCU(smp_trace_call_function_interrupt);
 
 static inline void __smp_call_function_single_interrupt(void)
 {
@@ -323,6 +329,7 @@ void smp_call_function_single_interrupt(struct pt_regs *regs)
 	__smp_call_function_single_interrupt();
 	exiting_irq();
 }
+FTRACE_UNSAFE_RCU(smp_call_function_single_interrupt);
 
 void smp_trace_call_function_single_interrupt(struct pt_regs *regs)
 {
@@ -332,6 +339,7 @@ void smp_trace_call_function_single_interrupt(struct pt_regs *regs)
 	trace_call_function_single_exit(CALL_FUNCTION_SINGLE_VECTOR);
 	exiting_irq();
 }
+FTRACE_UNSAFE_RCU(smp_trace_call_function_single_interrupt);
 
 static int __init nonmi_ipi_setup(char *str)
 {
diff --git a/kernel/softirq.c b/kernel/softirq.c
index be3d351..7960e70 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -325,6 +325,7 @@ void irq_enter(void)
 
 	__irq_enter();
 }
+FTRACE_UNSAFE_RCU(irq_enter);
 
 static inline void invoke_softirq(void)
 {
@@ -367,6 +368,7 @@ void irq_exit(void)
 	tick_irq_exit();
 	rcu_irq_exit();
 }
+FTRACE_UNSAFE_RCU(irq_exit);
 
 /*
  * This function must run with irqs disabled!
-- 
1.7.10.4

Re: [RFC][PATCH 16/18 v2] rcu/irq/x86: Mark functions that are RCU unsafe

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:33AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Some callbacks of the function tracer use rcu_read_lock(). This means that
> there's places that can not be traced because RCU is not tracking the CPU
> for various reasons (like NO_HZ_FULL and coming back from userspace).
> 
> Thes functions need to be marked so that callbacks that use RCU do not
> trace them.
> 
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  arch/x86/kernel/apic/apic.c |    2 ++
>  arch/x86/kernel/irq.c       |    1 +
>  arch/x86/kernel/irq_work.c  |    3 +++
>  arch/x86/kernel/smp.c       |    8 ++++++++
>  kernel/softirq.c            |    2 ++
>  5 files changed, 16 insertions(+)
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index eca89c5..91af16b 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -931,6 +931,7 @@ void __irq_entry smp_apic_timer_interrupt(struct pt_regs *regs)
> 
>  	set_irq_regs(old_regs);
>  }
> +FTRACE_UNSAFE_RCU(smp_apic_timer_interrupt);
> 
>  void __irq_entry smp_trace_apic_timer_interrupt(struct pt_regs *regs)
>  {
> @@ -952,6 +953,7 @@ void __irq_entry smp_trace_apic_timer_interrupt(struct pt_regs *regs)
> 
>  	set_irq_regs(old_regs);
>  }
> +FTRACE_UNSAFE_RCU(smp_trace_apic_timer_interrupt);
> 
>  int setup_profiling_timer(unsigned int multiplier)
>  {
> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> index 3a8185c..fccd0d1 100644
> --- a/arch/x86/kernel/irq.c
> +++ b/arch/x86/kernel/irq.c
> @@ -203,6 +203,7 @@ unsigned int __irq_entry do_IRQ(struct pt_regs *regs)
>  	set_irq_regs(old_regs);
>  	return 1;
>  }
> +FTRACE_UNSAFE_RCU(do_IRQ);
> 
>  /*
>   * Handler for X86_PLATFORM_IPI_VECTOR.
> diff --git a/arch/x86/kernel/irq_work.c b/arch/x86/kernel/irq_work.c
> index 636a55e..a2199c2 100644
> --- a/arch/x86/kernel/irq_work.c
> +++ b/arch/x86/kernel/irq_work.c
> @@ -7,6 +7,7 @@
>  #include <linux/kernel.h>
>  #include <linux/irq_work.h>
>  #include <linux/hardirq.h>
> +#include <linux/ftrace.h>
>  #include <asm/apic.h>
>  #include <asm/trace/irq_vectors.h>
> 
> @@ -28,6 +29,7 @@ void smp_irq_work_interrupt(struct pt_regs *regs)
>  	__smp_irq_work_interrupt();
>  	exiting_irq();
>  }
> +FTRACE_UNSAFE_RCU(smp_irq_work_interrupt);
> 
>  void smp_trace_irq_work_interrupt(struct pt_regs *regs)
>  {
> @@ -37,6 +39,7 @@ void smp_trace_irq_work_interrupt(struct pt_regs *regs)
>  	trace_irq_work_exit(IRQ_WORK_VECTOR);
>  	exiting_irq();
>  }
> +FTRACE_UNSAFE_RCU(smp_trace_irq_work_interrupt);
> 
>  void arch_irq_work_raise(void)
>  {
> diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> index cdaa347..3d702ef 100644
> --- a/arch/x86/kernel/smp.c
> +++ b/arch/x86/kernel/smp.c
> @@ -23,6 +23,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/cpu.h>
>  #include <linux/gfp.h>
> +#include <linux/ftrace.h>
> 
>  #include <asm/mtrr.h>
>  #include <asm/tlbflush.h>
> @@ -175,6 +176,7 @@ asmlinkage void smp_reboot_interrupt(void)
>  	stop_this_cpu(NULL);
>  	irq_exit();
>  }
> +FTRACE_UNSAFE_RCU(smp_reboot_interrupt);
> 
>  static void native_stop_other_cpus(int wait)
>  {
> @@ -264,6 +266,7 @@ void smp_reschedule_interrupt(struct pt_regs *regs)
>  	 * KVM uses this interrupt to force a cpu out of guest mode
>  	 */
>  }
> +FTRACE_UNSAFE_RCU(smp_reschedule_interrupt);
> 
>  static inline void smp_entering_irq(void)
>  {
> @@ -288,6 +291,7 @@ void smp_trace_reschedule_interrupt(struct pt_regs *regs)
>  	 * KVM uses this interrupt to force a cpu out of guest mode
>  	 */
>  }
> +FTRACE_UNSAFE_RCU(smp_trace_reschedule_interrupt);
> 
>  static inline void __smp_call_function_interrupt(void)
>  {
> @@ -301,6 +305,7 @@ void smp_call_function_interrupt(struct pt_regs *regs)
>  	__smp_call_function_interrupt();
>  	exiting_irq();
>  }
> +FTRACE_UNSAFE_RCU(smp_call_function_interrupt);
> 
>  void smp_trace_call_function_interrupt(struct pt_regs *regs)
>  {
> @@ -310,6 +315,7 @@ void smp_trace_call_function_interrupt(struct pt_regs *regs)
>  	trace_call_function_exit(CALL_FUNCTION_VECTOR);
>  	exiting_irq();
>  }
> +FTRACE_UNSAFE_RCU(smp_trace_call_function_interrupt);
> 
>  static inline void __smp_call_function_single_interrupt(void)
>  {
> @@ -323,6 +329,7 @@ void smp_call_function_single_interrupt(struct pt_regs *regs)
>  	__smp_call_function_single_interrupt();
>  	exiting_irq();
>  }
> +FTRACE_UNSAFE_RCU(smp_call_function_single_interrupt);
> 
>  void smp_trace_call_function_single_interrupt(struct pt_regs *regs)
>  {
> @@ -332,6 +339,7 @@ void smp_trace_call_function_single_interrupt(struct pt_regs *regs)
>  	trace_call_function_single_exit(CALL_FUNCTION_SINGLE_VECTOR);
>  	exiting_irq();
>  }
> +FTRACE_UNSAFE_RCU(smp_trace_call_function_single_interrupt);
> 
>  static int __init nonmi_ipi_setup(char *str)
>  {
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index be3d351..7960e70 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -325,6 +325,7 @@ void irq_enter(void)
> 
>  	__irq_enter();
>  }
> +FTRACE_UNSAFE_RCU(irq_enter);
> 
>  static inline void invoke_softirq(void)
>  {
> @@ -367,6 +368,7 @@ void irq_exit(void)
>  	tick_irq_exit();
>  	rcu_irq_exit();
>  }
> +FTRACE_UNSAFE_RCU(irq_exit);
> 
>  /*
>   * This function must run with irqs disabled!
> -- 
> 1.7.10.4
> 
> 

[RFC][PATCH 17/18 v2] ftrace/cpuidle/x86: Mark functions that are RCU unsafe

[Steven Rostedt]

[-- Attachment #1: 0017-ftrace-cpuidle-x86-Mark-functions-that-are-RCU-unsaf.patch --]
[-- Type: text/plain, Size: 2254 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Some callbacks of the function tracer use rcu_read_lock(). This means that
there's places that can not be traced because RCU is not tracking the CPU
for various reasons (like NO_HZ_FULL and coming back from userspace).

Thes functions need to be marked so that callbacks that use RCU do not
trace them.

Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/process.c |    2 ++
 drivers/cpuidle/cpuidle.c |    2 ++
 kernel/cpu/idle.c         |    2 ++
 3 files changed, 6 insertions(+)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 83369e5..18739cd 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -17,6 +17,7 @@
 #include <linux/stackprotector.h>
 #include <linux/tick.h>
 #include <linux/cpuidle.h>
+#include <linux/ftrace.h>
 #include <trace/events/power.h>
 #include <linux/hw_breakpoint.h>
 #include <asm/cpu.h>
@@ -316,6 +317,7 @@ void default_idle(void)
 #ifdef CONFIG_APM_MODULE
 EXPORT_SYMBOL(default_idle);
 #endif
+FTRACE_UNSAFE_RCU(default_idle);
 
 #ifdef CONFIG_XEN
 bool xen_set_default_idle(void)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index fdc432f..9515457 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -19,6 +19,7 @@
 #include <linux/ktime.h>
 #include <linux/hrtimer.h>
 #include <linux/module.h>
+#include <linux/ftrace.h>
 #include <trace/events/power.h>
 
 #include "cpuidle.h"
@@ -168,6 +169,7 @@ int cpuidle_idle_call(void)
 
 	return 0;
 }
+FTRACE_UNSAFE_RCU(cpuidle_idle_call);
 
 /**
  * cpuidle_install_idle_handler - installs the cpuidle idle loop handler
diff --git a/kernel/cpu/idle.c b/kernel/cpu/idle.c
index e695c0a..d9d290f 100644
--- a/kernel/cpu/idle.c
+++ b/kernel/cpu/idle.c
@@ -5,6 +5,7 @@
 #include <linux/cpu.h>
 #include <linux/tick.h>
 #include <linux/mm.h>
+#include <linux/ftrace.h>
 #include <linux/stackprotector.h>
 
 #include <asm/tlb.h>
@@ -61,6 +62,7 @@ void __weak arch_cpu_idle(void)
 	cpu_idle_force_poll = 1;
 	local_irq_enable();
 }
+FTRACE_UNSAFE_RCU(arch_cpu_idle);
 
 /*
  * Generic idle loop implementation
-- 
1.7.10.4

RE: [RFC][PATCH 17/18 v2] ftrace/cpuidle/x86: Mark functions that are RCU unsafe

[Wysocki, Rafael J]

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>




Sent from a tablet, I apologize for formatting issues.

Steven Rostedt <rostedt@goodmis.org> wrote:
From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Some callbacks of the function tracer use rcu_read_lock(). This means that
there's places that can not be traced because RCU is not tracking the CPU
for various reasons (like NO_HZ_FULL and coming back from userspace).

Thes functions need to be marked so that callbacks that use RCU do not
trace them.

Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/process.c |    2 ++
 drivers/cpuidle/cpuidle.c |    2 ++
 kernel/cpu/idle.c         |    2 ++
 3 files changed, 6 insertions(+)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 83369e5..18739cd 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -17,6 +17,7 @@
 #include <linux/stackprotector.h>
 #include <linux/tick.h>
 #include <linux/cpuidle.h>
+#include <linux/ftrace.h>
 #include <trace/events/power.h>
 #include <linux/hw_breakpoint.h>
 #include <asm/cpu.h>
@@ -316,6 +317,7 @@ void default_idle(void)
 #ifdef CONFIG_APM_MODULE
 EXPORT_SYMBOL(default_idle);
 #endif
+FTRACE_UNSAFE_RCU(default_idle);

 #ifdef CONFIG_XEN
 bool xen_set_default_idle(void)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index fdc432f..9515457 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -19,6 +19,7 @@
 #include <linux/ktime.h>
 #include <linux/hrtimer.h>
 #include <linux/module.h>
+#include <linux/ftrace.h>
 #include <trace/events/power.h>

 #include "cpuidle.h"
@@ -168,6 +169,7 @@ int cpuidle_idle_call(void)

         return 0;
 }
+FTRACE_UNSAFE_RCU(cpuidle_idle_call);

 /**
  * cpuidle_install_idle_handler - installs the cpuidle idle loop handler
diff --git a/kernel/cpu/idle.c b/kernel/cpu/idle.c
index e695c0a..d9d290f 100644
--- a/kernel/cpu/idle.c
+++ b/kernel/cpu/idle.c
@@ -5,6 +5,7 @@
 #include <linux/cpu.h>
 #include <linux/tick.h>
 #include <linux/mm.h>
+#include <linux/ftrace.h>
 #include <linux/stackprotector.h>

 #include <asm/tlb.h>
@@ -61,6 +62,7 @@ void __weak arch_cpu_idle(void)
         cpu_idle_force_poll = 1;
         local_irq_enable();
 }
+FTRACE_UNSAFE_RCU(arch_cpu_idle);

 /*
  * Generic idle loop implementation
--
1.7.10.4


---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

Re: [RFC][PATCH 17/18 v2] ftrace/cpuidle/x86: Mark functions that are RCU unsafe

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:34AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Some callbacks of the function tracer use rcu_read_lock(). This means that
> there's places that can not be traced because RCU is not tracking the CPU
> for various reasons (like NO_HZ_FULL and coming back from userspace).
> 
> Thes functions need to be marked so that callbacks that use RCU do not
> trace them.
> 
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  arch/x86/kernel/process.c |    2 ++
>  drivers/cpuidle/cpuidle.c |    2 ++
>  kernel/cpu/idle.c         |    2 ++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 83369e5..18739cd 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -17,6 +17,7 @@
>  #include <linux/stackprotector.h>
>  #include <linux/tick.h>
>  #include <linux/cpuidle.h>
> +#include <linux/ftrace.h>
>  #include <trace/events/power.h>
>  #include <linux/hw_breakpoint.h>
>  #include <asm/cpu.h>
> @@ -316,6 +317,7 @@ void default_idle(void)
>  #ifdef CONFIG_APM_MODULE
>  EXPORT_SYMBOL(default_idle);
>  #endif
> +FTRACE_UNSAFE_RCU(default_idle);
> 
>  #ifdef CONFIG_XEN
>  bool xen_set_default_idle(void)
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index fdc432f..9515457 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -19,6 +19,7 @@
>  #include <linux/ktime.h>
>  #include <linux/hrtimer.h>
>  #include <linux/module.h>
> +#include <linux/ftrace.h>
>  #include <trace/events/power.h>
> 
>  #include "cpuidle.h"
> @@ -168,6 +169,7 @@ int cpuidle_idle_call(void)
> 
>  	return 0;
>  }
> +FTRACE_UNSAFE_RCU(cpuidle_idle_call);
> 
>  /**
>   * cpuidle_install_idle_handler - installs the cpuidle idle loop handler
> diff --git a/kernel/cpu/idle.c b/kernel/cpu/idle.c
> index e695c0a..d9d290f 100644
> --- a/kernel/cpu/idle.c
> +++ b/kernel/cpu/idle.c
> @@ -5,6 +5,7 @@
>  #include <linux/cpu.h>
>  #include <linux/tick.h>
>  #include <linux/mm.h>
> +#include <linux/ftrace.h>
>  #include <linux/stackprotector.h>
> 
>  #include <asm/tlb.h>
> @@ -61,6 +62,7 @@ void __weak arch_cpu_idle(void)
>  	cpu_idle_force_poll = 1;
>  	local_irq_enable();
>  }
> +FTRACE_UNSAFE_RCU(arch_cpu_idle);
> 
>  /*
>   * Generic idle loop implementation
> -- 
> 1.7.10.4
> 
> 

Re: [RFC][PATCH 17/18 v2] ftrace/cpuidle/x86: Mark functions that are RCU unsafe

[H. Peter Anvin]

On 08/30/2013 10:11 PM, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Some callbacks of the function tracer use rcu_read_lock(). This means that
> there's places that can not be traced because RCU is not tracking the CPU
> for various reasons (like NO_HZ_FULL and coming back from userspace).
> 
> Thes functions need to be marked so that callbacks that use RCU do not
> trace them.
> 
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: H. Peter Anvin <hpa@zytor.com>

[RFC][PATCH 18/18 v2] ftrace/sched: Mark functions that are RCU unsafe

[Steven Rostedt]

[-- Attachment #1: 0018-ftrace-sched-Mark-functions-that-are-RCU-unsafe.patch --]
[-- Type: text/plain, Size: 998 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Some callbacks of the function tracer use rcu_read_lock(). This means that
there's places that can not be traced because RCU is not tracking the CPU
for various reasons (like NO_HZ_FULL and coming back from userspace).

Thes functions need to be marked so that callbacks that use RCU do not
trace them.

Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/sched/core.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index b7c32cb..9bd2aea 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1440,6 +1440,7 @@ void scheduler_ipi(void)
 	}
 	irq_exit();
 }
+FTRACE_UNSAFE_RCU(scheduler_ipi);
 
 static void ttwu_queue_remote(struct task_struct *p, int cpu)
 {
@@ -3222,6 +3223,7 @@ int idle_cpu(int cpu)
 
 	return 1;
 }
+FTRACE_UNSAFE_RCU(idle_cpu);
 
 /**
  * idle_task - return the idle task for a given cpu.
-- 
1.7.10.4

Re: [RFC][PATCH 18/18 v2] ftrace/sched: Mark functions that are RCU unsafe

[Paul E. McKenney]

On Sat, Aug 31, 2013 at 01:11:35AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Some callbacks of the function tracer use rcu_read_lock(). This means that
> there's places that can not be traced because RCU is not tracking the CPU
> for various reasons (like NO_HZ_FULL and coming back from userspace).
> 
> Thes functions need to be marked so that callbacks that use RCU do not
> trace them.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  kernel/sched/core.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index b7c32cb..9bd2aea 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1440,6 +1440,7 @@ void scheduler_ipi(void)
>  	}
>  	irq_exit();
>  }
> +FTRACE_UNSAFE_RCU(scheduler_ipi);
> 
>  static void ttwu_queue_remote(struct task_struct *p, int cpu)
>  {
> @@ -3222,6 +3223,7 @@ int idle_cpu(int cpu)
> 
>  	return 1;
>  }
> +FTRACE_UNSAFE_RCU(idle_cpu);
> 
>  /**
>   * idle_task - return the idle task for a given cpu.
> -- 
> 1.7.10.4
> 
> 

Re: [RFC][PATCH 00/18 v2] ftrace/rcu: Handle unsafe RCU functions and ftrace callbacks

[Steven Rostedt]

On Sat, 31 Aug 2013 01:11:17 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> This is my final draft of the patches. I'm starting to run them
> through my formal tests now. They may change depending on the outcome
> of the tests.

Update: All my formal tests passed.

> 
> I'm also a bit tired, and I added the change logs last. Thus I may
> need to go back and fix the change logs up too. But the code was done
> when I was rather spunky. But that doesn't mean I didn't break anything.

I still probably need to update the change logs.

But other than that, I'm waiting on Acks or comments.

Thanks!

-- Steve