* [PATCH] rcu,ftrace: Fix ftrace recursion @ 2020-09-29 11:33 Peter Zijlstra 2020-09-29 14:31 ` Paul E. McKenney ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Peter Zijlstra @ 2020-09-29 11:33 UTC (permalink / raw) To: Paul McKenney Cc: linux-kernel, Steven Rostedt, Thomas Gleixner, Ingo Molnar, kim.phillips Kim reported that perf-ftrace made his box unhappy. It turns out that commit: ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") removed one too many notrace. Probably due to there not being a helpful comment. Reinstate the notrace and add a comment to avoid loosing it again. Fixes: ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") Reported-by: Kim Phillips <kim.phillips@amd.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- kernel/rcu/tree.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index ee5e595501e8..33020d84ec6b 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -1098,8 +1098,11 @@ noinstr bool __rcu_is_watching(void) * CPU can safely enter RCU read-side critical sections. In other words, * if the current CPU is not in its idle loop or is in an interrupt or * NMI handler, return true. + * + * Must be notrace because __ftrace_ops_list_func() / ftrace_ops_assist_func() + * will call this (for every function) outside of recursion protection. */ -bool rcu_is_watching(void) +notrace bool rcu_is_watching(void) { bool ret; ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] rcu,ftrace: Fix ftrace recursion 2020-09-29 11:33 [PATCH] rcu,ftrace: Fix ftrace recursion Peter Zijlstra @ 2020-09-29 14:31 ` Paul E. McKenney 2020-09-29 14:36 ` Steven Rostedt 2020-09-29 14:43 ` Kim Phillips 2 siblings, 0 replies; 11+ messages in thread From: Paul E. McKenney @ 2020-09-29 14:31 UTC (permalink / raw) To: Peter Zijlstra Cc: linux-kernel, Steven Rostedt, Thomas Gleixner, Ingo Molnar, kim.phillips On Tue, Sep 29, 2020 at 01:33:40PM +0200, Peter Zijlstra wrote: > > Kim reported that perf-ftrace made his box unhappy. It turns out that > commit: > > ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") > > removed one too many notrace. Probably due to there not being a helpful > comment. > > Reinstate the notrace and add a comment to avoid loosing it again. s/loosing/losing/, but otherwise: Reviewed-by: Paul E. McKenney <paulmck@kernel.org> But please let me know if you would prefer that I take it via -rcu. Thanx, Paul > Fixes: ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") > Reported-by: Kim Phillips <kim.phillips@amd.com> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > kernel/rcu/tree.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index ee5e595501e8..33020d84ec6b 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -1098,8 +1098,11 @@ noinstr bool __rcu_is_watching(void) > * CPU can safely enter RCU read-side critical sections. In other words, > * if the current CPU is not in its idle loop or is in an interrupt or > * NMI handler, return true. > + * > + * Must be notrace because __ftrace_ops_list_func() / ftrace_ops_assist_func() > + * will call this (for every function) outside of recursion protection. > */ > -bool rcu_is_watching(void) > +notrace bool rcu_is_watching(void) > { > bool ret; > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] rcu,ftrace: Fix ftrace recursion 2020-09-29 11:33 [PATCH] rcu,ftrace: Fix ftrace recursion Peter Zijlstra 2020-09-29 14:31 ` Paul E. McKenney @ 2020-09-29 14:36 ` Steven Rostedt 2020-09-29 14:41 ` Paul E. McKenney 2020-09-29 14:49 ` Peter Zijlstra 2020-09-29 14:43 ` Kim Phillips 2 siblings, 2 replies; 11+ messages in thread From: Steven Rostedt @ 2020-09-29 14:36 UTC (permalink / raw) To: Peter Zijlstra Cc: Paul McKenney, linux-kernel, Thomas Gleixner, Ingo Molnar, kim.phillips On Tue, 29 Sep 2020 13:33:40 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > Kim reported that perf-ftrace made his box unhappy. It turns out that > commit: > > ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") > > removed one too many notrace. Probably due to there not being a helpful > comment. > > Reinstate the notrace and add a comment to avoid loosing it again. > > Fixes: ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") > Reported-by: Kim Phillips <kim.phillips@amd.com> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > kernel/rcu/tree.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index ee5e595501e8..33020d84ec6b 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -1098,8 +1098,11 @@ noinstr bool __rcu_is_watching(void) > * CPU can safely enter RCU read-side critical sections. In other words, > * if the current CPU is not in its idle loop or is in an interrupt or > * NMI handler, return true. > + * > + * Must be notrace because __ftrace_ops_list_func() / ftrace_ops_assist_func() > + * will call this (for every function) outside of recursion protection. > */ > -bool rcu_is_watching(void) > +notrace bool rcu_is_watching(void) > { > bool ret; > I think the patch I suggested is more suitable. -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] rcu,ftrace: Fix ftrace recursion 2020-09-29 14:36 ` Steven Rostedt @ 2020-09-29 14:41 ` Paul E. McKenney 2020-09-29 14:54 ` Steven Rostedt 2020-09-29 14:49 ` Peter Zijlstra 1 sibling, 1 reply; 11+ messages in thread From: Paul E. McKenney @ 2020-09-29 14:41 UTC (permalink / raw) To: Steven Rostedt Cc: Peter Zijlstra, linux-kernel, Thomas Gleixner, Ingo Molnar, kim.phillips On Tue, Sep 29, 2020 at 10:36:20AM -0400, Steven Rostedt wrote: > On Tue, 29 Sep 2020 13:33:40 +0200 > Peter Zijlstra <peterz@infradead.org> wrote: > > > Kim reported that perf-ftrace made his box unhappy. It turns out that > > commit: > > > > ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") > > > > removed one too many notrace. Probably due to there not being a helpful > > comment. > > > > Reinstate the notrace and add a comment to avoid loosing it again. > > > > Fixes: ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") > > Reported-by: Kim Phillips <kim.phillips@amd.com> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > --- > > kernel/rcu/tree.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > index ee5e595501e8..33020d84ec6b 100644 > > --- a/kernel/rcu/tree.c > > +++ b/kernel/rcu/tree.c > > @@ -1098,8 +1098,11 @@ noinstr bool __rcu_is_watching(void) > > * CPU can safely enter RCU read-side critical sections. In other words, > > * if the current CPU is not in its idle loop or is in an interrupt or > > * NMI handler, return true. > > + * > > + * Must be notrace because __ftrace_ops_list_func() / ftrace_ops_assist_func() > > + * will call this (for every function) outside of recursion protection. > > */ > > -bool rcu_is_watching(void) > > +notrace bool rcu_is_watching(void) > > { > > bool ret; > > > > I think the patch I suggested is more suitable. OK, I will let you guys fight it out. ;-) Thanx, Paul ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] rcu,ftrace: Fix ftrace recursion 2020-09-29 14:41 ` Paul E. McKenney @ 2020-09-29 14:54 ` Steven Rostedt 2020-09-29 16:56 ` Paul E. McKenney 0 siblings, 1 reply; 11+ messages in thread From: Steven Rostedt @ 2020-09-29 14:54 UTC (permalink / raw) To: Paul E. McKenney Cc: Peter Zijlstra, linux-kernel, Thomas Gleixner, Ingo Molnar, kim.phillips On Tue, 29 Sep 2020 07:41:06 -0700 "Paul E. McKenney" <paulmck@kernel.org> wrote: > On Tue, Sep 29, 2020 at 10:36:20AM -0400, Steven Rostedt wrote: > > On Tue, 29 Sep 2020 13:33:40 +0200 > > Peter Zijlstra <peterz@infradead.org> wrote: > > > > > Kim reported that perf-ftrace made his box unhappy. It turns out that > > > commit: > > > > > > ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") > > > > > > removed one too many notrace. Probably due to there not being a helpful > > > comment. > > > > > > Reinstate the notrace and add a comment to avoid loosing it again. > > > > > > Fixes: ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") > > > Reported-by: Kim Phillips <kim.phillips@amd.com> > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > > --- > > > kernel/rcu/tree.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > index ee5e595501e8..33020d84ec6b 100644 > > > --- a/kernel/rcu/tree.c > > > +++ b/kernel/rcu/tree.c > > > @@ -1098,8 +1098,11 @@ noinstr bool __rcu_is_watching(void) > > > * CPU can safely enter RCU read-side critical sections. In other words, > > > * if the current CPU is not in its idle loop or is in an interrupt or > > > * NMI handler, return true. > > > + * > > > + * Must be notrace because __ftrace_ops_list_func() / ftrace_ops_assist_func() > > > + * will call this (for every function) outside of recursion protection. > > > */ > > > -bool rcu_is_watching(void) > > > +notrace bool rcu_is_watching(void) > > > { > > > bool ret; > > > > > > > I think the patch I suggested is more suitable. > > OK, I will let you guys fight it out. ;-) > Well, I think we should actually apply both, but the comment needs to be updated, as it will no longer be outside recursion. And the comment is wrong now as well, as its only outside recursion protection for the assist_func(). But it does prevent it from being always called for perf. * Make notrace because it can be called by the internal functions of * ftrace, and making this notrace removes unnecessary recursion calls. -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] rcu,ftrace: Fix ftrace recursion 2020-09-29 14:54 ` Steven Rostedt @ 2020-09-29 16:56 ` Paul E. McKenney 2020-09-29 17:04 ` Steven Rostedt 0 siblings, 1 reply; 11+ messages in thread From: Paul E. McKenney @ 2020-09-29 16:56 UTC (permalink / raw) To: Steven Rostedt Cc: Peter Zijlstra, linux-kernel, Thomas Gleixner, Ingo Molnar, kim.phillips On Tue, Sep 29, 2020 at 10:54:16AM -0400, Steven Rostedt wrote: > On Tue, 29 Sep 2020 07:41:06 -0700 > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > On Tue, Sep 29, 2020 at 10:36:20AM -0400, Steven Rostedt wrote: > > > On Tue, 29 Sep 2020 13:33:40 +0200 > > > Peter Zijlstra <peterz@infradead.org> wrote: > > > > > > > Kim reported that perf-ftrace made his box unhappy. It turns out that > > > > commit: > > > > > > > > ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") > > > > > > > > removed one too many notrace. Probably due to there not being a helpful > > > > comment. > > > > > > > > Reinstate the notrace and add a comment to avoid loosing it again. > > > > > > > > Fixes: ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") > > > > Reported-by: Kim Phillips <kim.phillips@amd.com> > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > > > --- > > > > kernel/rcu/tree.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > index ee5e595501e8..33020d84ec6b 100644 > > > > --- a/kernel/rcu/tree.c > > > > +++ b/kernel/rcu/tree.c > > > > @@ -1098,8 +1098,11 @@ noinstr bool __rcu_is_watching(void) > > > > * CPU can safely enter RCU read-side critical sections. In other words, > > > > * if the current CPU is not in its idle loop or is in an interrupt or > > > > * NMI handler, return true. > > > > + * > > > > + * Must be notrace because __ftrace_ops_list_func() / ftrace_ops_assist_func() > > > > + * will call this (for every function) outside of recursion protection. > > > > */ > > > > -bool rcu_is_watching(void) > > > > +notrace bool rcu_is_watching(void) > > > > { > > > > bool ret; > > > > > > > > > > I think the patch I suggested is more suitable. > > > > OK, I will let you guys fight it out. ;-) > > > > Well, I think we should actually apply both, but the comment needs to be > updated, as it will no longer be outside recursion. And the comment is > wrong now as well, as its only outside recursion protection for the > assist_func(). > > But it does prevent it from being always called for perf. > > * Make notrace because it can be called by the internal functions of > * ftrace, and making this notrace removes unnecessary recursion calls. Fair enough. ;-) If I don't hear otherwise by late today (Tuesday), Pacific Time, I will update the comment and pull it into -rcu. If you guys have some other route to mainline in mind, you have my Reviewed-by. Either way, just let me know. Thanx, Paul ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] rcu,ftrace: Fix ftrace recursion 2020-09-29 16:56 ` Paul E. McKenney @ 2020-09-29 17:04 ` Steven Rostedt 2020-10-05 0:18 ` Paul E. McKenney 0 siblings, 1 reply; 11+ messages in thread From: Steven Rostedt @ 2020-09-29 17:04 UTC (permalink / raw) To: Paul E. McKenney Cc: Peter Zijlstra, linux-kernel, Thomas Gleixner, Ingo Molnar, kim.phillips On Tue, 29 Sep 2020 09:56:40 -0700 "Paul E. McKenney" <paulmck@kernel.org> wrote: > > Well, I think we should actually apply both, but the comment needs to be > > updated, as it will no longer be outside recursion. And the comment is > > wrong now as well, as its only outside recursion protection for the > > assist_func(). > > > > But it does prevent it from being always called for perf. > > > > * Make notrace because it can be called by the internal functions of > > * ftrace, and making this notrace removes unnecessary recursion calls. > > Fair enough. ;-) > > If I don't hear otherwise by late today (Tuesday), Pacific Time, I will > update the comment and pull it into -rcu. If you guys have some other > route to mainline in mind, you have my Reviewed-by. Either way, just > let me know. I'm currently testing the recursion fix and will push that to Linus when done. But you can take the comment update through your tree. Peter, are you OK if Paul changes your comment to what I suggested? -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] rcu,ftrace: Fix ftrace recursion 2020-09-29 17:04 ` Steven Rostedt @ 2020-10-05 0:18 ` Paul E. McKenney 0 siblings, 0 replies; 11+ messages in thread From: Paul E. McKenney @ 2020-10-05 0:18 UTC (permalink / raw) To: Steven Rostedt Cc: Peter Zijlstra, linux-kernel, Thomas Gleixner, Ingo Molnar, kim.phillips On Tue, Sep 29, 2020 at 01:04:49PM -0400, Steven Rostedt wrote: > On Tue, 29 Sep 2020 09:56:40 -0700 > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > Well, I think we should actually apply both, but the comment needs to be > > > updated, as it will no longer be outside recursion. And the comment is > > > wrong now as well, as its only outside recursion protection for the > > > assist_func(). > > > > > > But it does prevent it from being always called for perf. > > > > > > * Make notrace because it can be called by the internal functions of > > > * ftrace, and making this notrace removes unnecessary recursion calls. > > > > Fair enough. ;-) > > > > If I don't hear otherwise by late today (Tuesday), Pacific Time, I will > > update the comment and pull it into -rcu. If you guys have some other > > route to mainline in mind, you have my Reviewed-by. Either way, just > > let me know. > > I'm currently testing the recursion fix and will push that to Linus when > done. But you can take the comment update through your tree. > > Peter, are you OK if Paul changes your comment to what I suggested? Hearing no objections, I have queued the patch with the comment updated as suggested by Steven. Thank you all! Thanx, Paul ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] rcu,ftrace: Fix ftrace recursion 2020-09-29 14:36 ` Steven Rostedt 2020-09-29 14:41 ` Paul E. McKenney @ 2020-09-29 14:49 ` Peter Zijlstra 2020-09-29 15:19 ` Steven Rostedt 1 sibling, 1 reply; 11+ messages in thread From: Peter Zijlstra @ 2020-09-29 14:49 UTC (permalink / raw) To: Steven Rostedt Cc: Paul McKenney, linux-kernel, Thomas Gleixner, Ingo Molnar, kim.phillips On Tue, Sep 29, 2020 at 10:36:20AM -0400, Steven Rostedt wrote: > > +notrace bool rcu_is_watching(void) > > { > > bool ret; > > > > I think the patch I suggested is more suitable. Both, with only your patch we'd still take the pointless mcount call, which is then pure overhead. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] rcu,ftrace: Fix ftrace recursion 2020-09-29 14:49 ` Peter Zijlstra @ 2020-09-29 15:19 ` Steven Rostedt 0 siblings, 0 replies; 11+ messages in thread From: Steven Rostedt @ 2020-09-29 15:19 UTC (permalink / raw) To: Peter Zijlstra Cc: Paul McKenney, linux-kernel, Thomas Gleixner, Ingo Molnar, kim.phillips On Tue, 29 Sep 2020 16:49:53 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Tue, Sep 29, 2020 at 10:36:20AM -0400, Steven Rostedt wrote: > > > > +notrace bool rcu_is_watching(void) > > > { > > > bool ret; > > > > > > > I think the patch I suggested is more suitable. > > Both, with only your patch we'd still take the pointless mcount call, > which is then pure overhead. Yep, and I stated the same thing to Paul ;-) -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] rcu,ftrace: Fix ftrace recursion 2020-09-29 11:33 [PATCH] rcu,ftrace: Fix ftrace recursion Peter Zijlstra 2020-09-29 14:31 ` Paul E. McKenney 2020-09-29 14:36 ` Steven Rostedt @ 2020-09-29 14:43 ` Kim Phillips 2 siblings, 0 replies; 11+ messages in thread From: Kim Phillips @ 2020-09-29 14:43 UTC (permalink / raw) To: Peter Zijlstra, Paul McKenney Cc: linux-kernel, Steven Rostedt, Thomas Gleixner, Ingo Molnar On 9/29/20 6:33 AM, Peter Zijlstra wrote: > > Kim reported that perf-ftrace made his box unhappy. It turns out that > commit: > > ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") > > removed one too many notrace. Probably due to there not being a helpful > comment. > > Reinstate the notrace and add a comment to avoid loosing it again. > > Fixes: ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") > Reported-by: Kim Phillips <kim.phillips@amd.com> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- Tested-by: Kim Phillips <kim.phillips@amd.com> Thanks, Kim ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-10-05 0:18 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-29 11:33 [PATCH] rcu,ftrace: Fix ftrace recursion Peter Zijlstra 2020-09-29 14:31 ` Paul E. McKenney 2020-09-29 14:36 ` Steven Rostedt 2020-09-29 14:41 ` Paul E. McKenney 2020-09-29 14:54 ` Steven Rostedt 2020-09-29 16:56 ` Paul E. McKenney 2020-09-29 17:04 ` Steven Rostedt 2020-10-05 0:18 ` Paul E. McKenney 2020-09-29 14:49 ` Peter Zijlstra 2020-09-29 15:19 ` Steven Rostedt 2020-09-29 14:43 ` Kim Phillips
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).