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 <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
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;
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 <[email protected]>
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 <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> 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;
>
On Tue, 29 Sep 2020 13:33:40 +0200
Peter Zijlstra <[email protected]> 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 <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> 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
On Tue, Sep 29, 2020 at 10:36:20AM -0400, Steven Rostedt wrote:
> On Tue, 29 Sep 2020 13:33:40 +0200
> Peter Zijlstra <[email protected]> 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 <[email protected]>
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > ---
> > 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
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 <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
Tested-by: Kim Phillips <[email protected]>
Thanks,
Kim
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.
On Tue, 29 Sep 2020 07:41:06 -0700
"Paul E. McKenney" <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > > ---
> > > 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
On Tue, 29 Sep 2020 16:49:53 +0200
Peter Zijlstra <[email protected]> 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
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" <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > > > ---
> > > > 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
On Tue, 29 Sep 2020 09:56:40 -0700
"Paul E. McKenney" <[email protected]> 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
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" <[email protected]> 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