Re: [PATCH] rcu,ftrace: Fix ftrace recursion
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" 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
Re: [PATCH] rcu,ftrace: Fix ftrace recursion
On Tue, 29 Sep 2020 09:56:40 -0700 "Paul E. McKenney" 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
Re: [PATCH] rcu,ftrace: Fix ftrace recursion
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" 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 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 > > > > Signed-off-by: Peter Zijlstra (Intel) > > > > --- > > > > 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
Re: [PATCH] rcu,ftrace: Fix ftrace recursion
On Tue, 29 Sep 2020 16:49:53 +0200 Peter Zijlstra 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
Re: [PATCH] rcu,ftrace: Fix ftrace recursion
On Tue, 29 Sep 2020 07:41:06 -0700 "Paul E. McKenney" 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 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 > > > Signed-off-by: Peter Zijlstra (Intel) > > > --- > > > 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
Re: [PATCH] rcu,ftrace: Fix ftrace recursion
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.
Re: [PATCH] rcu,ftrace: Fix ftrace recursion
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 > Signed-off-by: Peter Zijlstra (Intel) > --- Tested-by: Kim Phillips Thanks, Kim
Re: [PATCH] rcu,ftrace: Fix ftrace recursion
On Tue, Sep 29, 2020 at 10:36:20AM -0400, Steven Rostedt wrote: > On Tue, 29 Sep 2020 13:33:40 +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. > > > > Fixes: ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") > > Reported-by: Kim Phillips > > Signed-off-by: Peter Zijlstra (Intel) > > --- > > 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
Re: [PATCH] rcu,ftrace: Fix ftrace recursion
On Tue, 29 Sep 2020 13:33:40 +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. > > Fixes: ff5c4f5cad33 ("rcu/tree: Mark the idle relevant functions noinstr") > Reported-by: Kim Phillips > Signed-off-by: Peter Zijlstra (Intel) > --- > 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
Re: [PATCH] rcu,ftrace: Fix ftrace recursion
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 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 > Signed-off-by: Peter Zijlstra (Intel) > --- > 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; >
[PATCH] rcu,ftrace: Fix ftrace recursion
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 Signed-off-by: Peter Zijlstra (Intel) --- 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;