tracing: Annotate ftrace_graph_hash pointer with __rcu
PATCH 1/1 tracing: Annotate ftrace_graph_hash pointer with __rcu
Fix following instances of sparse error kernel/trace/ftrace.c:5664:29: error: incompatible types in comparison kernel/trace/ftrace.c:5785:21: error: incompatible types in comparison kernel/trace/ftrace.c:5864:36: error: incompatible types in comparison kernel/trace/ftrace.c:5866:25: error: incompatible types in comparison
Use rcu_dereference_protected to access the __rcu annotated pointer.
--- kernel/trace/ftrace.c | 2 +- kernel/trace/trace.h | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-)
kernel/trace/ftrace.c +1 -1
@@ -5596,7 +5596,7 @@ static const struct file_operations ftrace_notrace_fops = {static DEFINE_MUTEX (graph_lock); -struct ftrace_hash *ftrace_graph_hash =EMPTY_HASH ; +struct ftrace_hash __rcu *ftrace_graph_hash =EMPTY_HASH ;struct ftrace_hash *ftrace_graph_notrace_hash =EMPTY_HASH ;enum graph_filter_type {
kernel/trace/trace.h +6 -3
@@ -950,22 +950,25 @@ extern void __trace_graph_return(struct trace_array *tr, unsigned long flags, int pc);#ifdef CONFIG_DYNAMIC_FTRACE -extern struct ftrace_hash *ftrace_graph_hash; +extern struct ftrace_hash __rcu *ftrace_graph_hash;extern struct ftrace_hash *ftrace_graph_notrace_hash;static inline int ftrace_graph_addr (struct ftrace_graph_ent *trace) {unsigned long addr = trace->func;int ret =0 ; +struct ftrace_hash *hash;preempt_disable_notrace (); -if (ftrace_hash_empty (ftrace_graph_hash)) { + hash =rcu_dereference_protected (ftrace_graph_hash, !preemptible ()); + +if (ftrace_hash_empty (hash)) { ret =1 ; goto out; } -if (ftrace_lookup_ip (ftrace_graph_hash, addr)) { +if (ftrace_lookup_ip (hash, addr)) {/* * This needs to be cleared on the return functions
Signature · 6 lines
2.24.1 _______________________________________________ Linux-kernel-mentees mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
Quoted 39 lines · Amol Grover
Fix following instances of sparse error
kernel/trace/ftrace.c:5664:29: error: incompatible types in comparisonkernel/trace/ftrace.c:5785:21: error: incompatible types in comparisonkernel/trace/ftrace.c:5864:36: error: incompatible types in comparisonkernel/trace/ftrace.c:5866:25: error: incompatible types in comparisonUse
rcu_dereference_protectedto access the__rcuannotated pointer.Signed-off-by Amol Grover <[email protected]>--- kernel/trace/ftrace.c | 2 +- kernel/trace/trace.h | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-)kernel/trace/ftrace.c +1
-1@@ -5596,7 +5596,7 @@ static const struct file_operations ftrace_notrace_fops = {static DEFINE_MUTEX (graph_lock); -struct ftrace_hash *ftrace_graph_hash =EMPTY_HASH ; +struct ftrace_hash __rcu *ftrace_graph_hash =EMPTY_HASH ;struct ftrace_hash *ftrace_graph_notrace_hash =EMPTY_HASH ;enum graph_filter_type {kernel/trace/trace.h +1
-1@@ -950,22 +950,25 @@ extern void __trace_graph_return(struct trace_array *tr, unsigned long flags, int pc);#ifdef CONFIG_DYNAMIC_FTRACE -extern struct ftrace_hash *ftrace_graph_hash; +extern struct ftrace_hash __rcu *ftrace_graph_hash;extern struct ftrace_hash *ftrace_graph_notrace_hash;
kernel/trace/trace.h extern void __trace_graph_return(struct trace_array *tr, · line 964
Quoted 10 lines
kernel/trace/trace.h +2
-1static inline int ftrace_graph_addr (struct ftrace_graph_ent *trace) {unsigned long addr = trace->func;int ret =0 ; +struct ftrace_hash *hash;preempt_disable_notrace (); -if (ftrace_hash_empty (ftrace_graph_hash)) { + hash =rcu_dereference_protected (ftrace_graph_hash, !preemptible ());
I think you can use rcu_dereference_sched() here? That way no need to pass !preemptible.
A preempt-disabled section is an RCU "sched flavor" section. Flavors are consolidated in the backend, but in the front end the dereference API still do have flavors.
thanks,
- Joel
Quoted 14 lines
kernel/trace/trace.h +3
-1+ +if (ftrace_hash_empty (hash)) { ret =1 ; goto out; } -if (ftrace_lookup_ip (ftrace_graph_hash, addr)) { +if (ftrace_lookup_ip (hash, addr)) {/* * This needs to be cleared on the return functions Signature · 1 line
2.24.1
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
Steven Rostedt
maintainerQuoted 11 lines · On Mon, 3 Feb 2020 11:33:01 -0500 Joel Fernandes <[email protected]> wrote:
kernel/trace/trace.h extern void __trace_graph_return(struct trace_array *tr, · line 964
Quoted 4 lines
kernel/trace/trace.h +1
-1preempt_disable_notrace (); -if (ftrace_hash_empty (ftrace_graph_hash)) { + hash =rcu_dereference_protected (ftrace_graph_hash, !preemptible ());I think you can use
rcu_dereference_sched()here? That way no need to pass !preemptible.A preempt-disabled section is an RCU "sched flavor" section. Flavors are consolidated in the backend, but in the front end the dereference API still do have flavors.
Agreed, Amol, can you send an update?
-- Steve
Steven Rostedt
maintainerQuoted 24 lines · On Mon, 3 Feb 2020 11:33:01 -0500 Joel Fernandes <[email protected]> wrote:
Quoted 10 lines
kernel/trace/trace.h +1
-1@@ -950,22 +950,25 @@ extern void __trace_graph_return(struct trace_array *tr, unsigned long flags, int pc);#ifdef CONFIG_DYNAMIC_FTRACE -extern struct ftrace_hash *ftrace_graph_hash; +extern struct ftrace_hash __rcu *ftrace_graph_hash;extern struct ftrace_hash *ftrace_graph_notrace_hash;kernel/trace/trace.h extern void __trace_graph_return(struct trace_array *tr, · line 964
Quoted 10 lines
kernel/trace/trace.h +2
-1static inline int ftrace_graph_addr (struct ftrace_graph_ent *trace) {unsigned long addr = trace->func;int ret =0 ; +struct ftrace_hash *hash;preempt_disable_notrace (); -if (ftrace_hash_empty (ftrace_graph_hash)) { + hash =rcu_dereference_protected (ftrace_graph_hash, !preemptible ());I think you can use
rcu_dereference_sched()here? That way no need to pass !preemptible.
Quoted 3 lines
A preempt-disabled section is an RCU "sched flavor" section. Flavors are consolidated in the backend, but in the front end the dereference API still do have flavors.
Unfortunately, doing it with rcu_dereference_sched() causes a lockdep splat :-P. This is because ftrace can execute when rcu is not "watching" and that will trigger a lockdep error. That means, this origin patch *is* correct. I'm re-applying this one.
-- Steve
Quoted 28 lines · Steven Rostedt
Quoted 24 lines · On Mon, 3 Feb 2020 11:33:01 -0500 Joel Fernandes <[email protected]> wrote:
Quoted 10 lines
kernel/trace/trace.h +1
-1@@ -950,22 +950,25 @@ extern void __trace_graph_return(struct trace_array *tr, unsigned long flags, int pc);#ifdef CONFIG_DYNAMIC_FTRACE -extern struct ftrace_hash *ftrace_graph_hash; +extern struct ftrace_hash __rcu *ftrace_graph_hash;extern struct ftrace_hash *ftrace_graph_notrace_hash;kernel/trace/trace.h extern void __trace_graph_return(struct trace_array *tr, · line 964
Quoted 10 lines
kernel/trace/trace.h +2
-1static inline int ftrace_graph_addr (struct ftrace_graph_ent *trace) {unsigned long addr = trace->func;int ret =0 ; +struct ftrace_hash *hash;preempt_disable_notrace (); -if (ftrace_hash_empty (ftrace_graph_hash)) { + hash =rcu_dereference_protected (ftrace_graph_hash, !preemptible ());I think you can use
rcu_dereference_sched()here? That way no need to pass !preemptible.
Quoted 9 lines
Quoted 3 lines
A preempt-disabled section is an RCU "sched flavor" section. Flavors are consolidated in the backend, but in the front end the dereference API still do have flavors.
Unfortunately, doing it with
rcu_dereference_sched()causes a lockdep splat :-P. This is because ftrace can execute when rcu is not "watching" and that will trigger a lockdep error. That means, this origin patch *is* correct. I'm re-applying this one.
Something new to learn, thank you for the information Steve!
Thanks
Amol
Quoted 1 line
-- Steve
Quoted 28 lines · Steven Rostedt
Quoted 24 lines · On Mon, 3 Feb 2020 11:33:01 -0500 Joel Fernandes <[email protected]> wrote:
Quoted 10 lines
kernel/trace/trace.h +1
-1@@ -950,22 +950,25 @@ extern void __trace_graph_return(struct trace_array *tr, unsigned long flags, int pc);#ifdef CONFIG_DYNAMIC_FTRACE -extern struct ftrace_hash *ftrace_graph_hash; +extern struct ftrace_hash __rcu *ftrace_graph_hash;extern struct ftrace_hash *ftrace_graph_notrace_hash;kernel/trace/trace.h extern void __trace_graph_return(struct trace_array *tr, · line 964
Quoted 10 lines
kernel/trace/trace.h +2
-1static inline int ftrace_graph_addr (struct ftrace_graph_ent *trace) {unsigned long addr = trace->func;int ret =0 ; +struct ftrace_hash *hash;preempt_disable_notrace (); -if (ftrace_hash_empty (ftrace_graph_hash)) { + hash =rcu_dereference_protected (ftrace_graph_hash, !preemptible ());I think you can use
rcu_dereference_sched()here? That way no need to pass !preemptible.
Quoted 8 lines
Quoted 3 lines
A preempt-disabled section is an RCU "sched flavor" section. Flavors are consolidated in the backend, but in the front end the dereference API still do have flavors.
Unfortunately, doing it with
rcu_dereference_sched()causes a lockdep splat :-P. This is because ftrace can execute when rcu is not "watching" and that will trigger a lockdep error. That means, this origin patch *is* correct. I'm re-applying this one.
I strongly recommend a comment stating why disabling preemption prevents ftrace_graph_hash from going away. I see the synchronize_rcu() after the rcu_assign_pointer() in ftrace_graph_release(), but I don't see anything that waits on CPUs that RCU is not watching.
Of course, event tracing -makes- RCU watch when needed, but if that was set up, then lockdep would not have complained.
So what am I missing?
Thanx, Paul
Steven Rostedt
maintainerQuoted 9 lines · On Wed, 5 Feb 2020 05:11:10 -0800 "Paul E. McKenney" <[email protected]> wrote:
I strongly recommend a comment stating why disabling preemption prevents
ftrace_graph_hashfrom going away. I see thesynchronize_rcu()after thercu_assign_pointer()inftrace_graph_release(), but I don't see anything that waits on CPUs that RCU is not watching.Of course, event tracing -makes- RCU watch when needed, but if that was set up, then lockdep would not have complained.
So what am I missing?
Keep looking in your INBOX and look at the patch I asked you to ack or complain about ;-)
-- Steve
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
Steven Rostedt
maintainerQuoted 8 lines · On Wed, 5 Feb 2020 08:14:09 -0500 Steven Rostedt <[email protected]> wrote:
Quoted 5 lines · On Wed, 5 Feb 2020 05:11:10 -0800 "Paul E. McKenney" <[email protected]> wrote:
I strongly recommend a comment stating why disabling preemption prevents
ftrace_graph_hashfrom going away. I see thesynchronize_rcu()after thercu_assign_pointer()inftrace_graph_release(), but I don't see anything that waits on CPUs that RCU is not watching.
Quoted 7 lines
Quoted 4 lines
Of course, event tracing -makes- RCU watch when needed, but if that was set up, then lockdep would not have complained.
So what am I missing?
Keep looking in your INBOX and look at the patch I asked you to ack or complain about ;-)
Actually, looking at the code myself, it appears to be missing the ftrace_sync. Thus, this is a bug, and requires the ftrace sync, as synchronize_rcu() is not strong enough here.
Patch in process!
-- Steve
Quoted 11 lines · Steven Rostedt
Quoted 8 lines · On Wed, 5 Feb 2020 08:14:09 -0500 Steven Rostedt <[email protected]> wrote:
Quoted 5 lines · On Wed, 5 Feb 2020 05:11:10 -0800 "Paul E. McKenney" <[email protected]> wrote:
I strongly recommend a comment stating why disabling preemption prevents
ftrace_graph_hashfrom going away. I see thesynchronize_rcu()after thercu_assign_pointer()inftrace_graph_release(), but I don't see anything that waits on CPUs that RCU is not watching.
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
Of course, event tracing -makes- RCU watch when needed, but if that was set up, then lockdep would not have complained.
So what am I missing?
Keep looking in your INBOX and look at the patch I asked you to ack or complain about ;-)
Not yet seeing it, but...
Quoted 5 lines
Actually, looking at the code myself, it appears to be missing the
ftrace_sync. Thus, this is a bug, and requires the ftrace sync, assynchronize_rcu()is not strong enough here.Patch in process!
Thank you for chasing it down!
Thanx, Paul
Quoted 39 lines · Amol Grover
Fix following instances of sparse error
kernel/trace/ftrace.c:5664:29: error: incompatible types in comparisonkernel/trace/ftrace.c:5785:21: error: incompatible types in comparisonkernel/trace/ftrace.c:5864:36: error: incompatible types in comparisonkernel/trace/ftrace.c:5866:25: error: incompatible types in comparisonUse
rcu_dereference_protectedto access the__rcuannotated pointer.Signed-off-by Amol Grover <[email protected]>--- kernel/trace/ftrace.c | 2 +- kernel/trace/trace.h | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-)kernel/trace/ftrace.c +1
-1@@ -5596,7 +5596,7 @@ static const struct file_operations ftrace_notrace_fops = {static DEFINE_MUTEX (graph_lock); -struct ftrace_hash *ftrace_graph_hash =EMPTY_HASH ; +struct ftrace_hash __rcu *ftrace_graph_hash =EMPTY_HASH ;struct ftrace_hash *ftrace_graph_notrace_hash =EMPTY_HASH ;enum graph_filter_type {kernel/trace/trace.h +1
-1@@ -950,22 +950,25 @@ extern void __trace_graph_return(struct trace_array *tr, unsigned long flags, int pc);#ifdef CONFIG_DYNAMIC_FTRACE -extern struct ftrace_hash *ftrace_graph_hash; +extern struct ftrace_hash __rcu *ftrace_graph_hash;extern struct ftrace_hash *ftrace_graph_notrace_hash;
kernel/trace/trace.h extern void __trace_graph_return(struct trace_array *tr, · line 964
Quoted 10 lines
kernel/trace/trace.h +2
-1static inline int ftrace_graph_addr (struct ftrace_graph_ent *trace) {unsigned long addr = trace->func;int ret =0 ; +struct ftrace_hash *hash;preempt_disable_notrace (); -if (ftrace_hash_empty (ftrace_graph_hash)) { + hash =rcu_dereference_protected (ftrace_graph_hash, !preemptible ());
BTW I think set_ftrace_early_graph() should use rcu_assign_pointer() so that the early writes to the hash are release-barrier'ed before the global pointer is updated. I will send a patch for that.
thanks,
- Joel
Quoted 14 lines
kernel/trace/trace.h +3
-1+ +if (ftrace_hash_empty (hash)) { ret =1 ; goto out; } -if (ftrace_lookup_ip (ftrace_graph_hash, addr)) { +if (ftrace_lookup_ip (hash, addr)) {/* * This needs to be cleared on the return functions Signature · 1 line
2.24.1
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees