Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1640640 > unrolled thread

[RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace

Started bySteven Rostedt <rostedt@goodmis.org>
First post2017-05-12 19:30 +0200
Last post2017-05-17 18:50 +0200
Articles 7 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace Steven Rostedt <rostedt@goodmis.org> - 2017-05-12 19:30 +0200
    Re: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before  calling a stack trace "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-12 20:30 +0200
      Re: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before  calling a stack trace Steven Rostedt <rostedt@goodmis.org> - 2017-05-12 20:40 +0200
        Re: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before  calling a stack trace "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-12 21:00 +0200
          Re: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before  calling a stack trace Steven Rostedt <rostedt@goodmis.org> - 2017-05-12 22:10 +0200
            Re: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before  calling a stack trace "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-12 22:40 +0200
              Re: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before  calling a stack trace Steven Rostedt <rostedt@goodmis.org> - 2017-05-17 18:50 +0200

#1640640 — [RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-12 19:30 +0200
Subject[RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace
Message-ID<tGmy6-7Uc-35@gated-at.bofh.it>
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

As stack tracing now requires "rcu watching", force RCU to be watching when
recording a stack trace.

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

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index c4536c449021..a4208cebb42b 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2568,7 +2568,12 @@ static inline void ftrace_trace_stack(struct trace_array *tr,
 void __trace_stack(struct trace_array *tr, unsigned long flags, int skip,
 		   int pc)
 {
+	if (unlikely(rcu_irq_enter_disabled()))
+		return;
+
+	rcu_irq_enter();
 	__ftrace_trace_stack(tr->trace_buffer.buffer, flags, skip, pc, NULL);
+	rcu_irq_exit();
 }
 
 /**
-- 
2.10.2

[toc] | [next] | [standalone]

#1640675 — Re: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-05-12 20:30 +0200
SubjectRe: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace
Message-ID<tGnu9-7Y-9@gated-at.bofh.it>
In reply to#1640640
On Fri, May 12, 2017 at 01:15:45PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> As stack tracing now requires "rcu watching", force RCU to be watching when
> recording a stack trace.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Assuming that you never get to __trace_stack() if in an NMI handler,
this looks good to me!

In contrast, if if __trace_stack() ever is called from an NMI handler,
invoking rcu_irq_enter() can be fatal.

							Thanx, Paul

> ---
>  kernel/trace/trace.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index c4536c449021..a4208cebb42b 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2568,7 +2568,12 @@ static inline void ftrace_trace_stack(struct trace_array *tr,
>  void __trace_stack(struct trace_array *tr, unsigned long flags, int skip,
>  		   int pc)
>  {
> +	if (unlikely(rcu_irq_enter_disabled()))
> +		return;
> +
> +	rcu_irq_enter();
>  	__ftrace_trace_stack(tr->trace_buffer.buffer, flags, skip, pc, NULL);
> +	rcu_irq_exit();
>  }
> 
>  /**
> -- 
> 2.10.2
> 
>

[toc] | [prev] | [next] | [standalone]

#1640679 — Re: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-12 20:40 +0200
SubjectRe: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace
Message-ID<tGnDQ-cz-17@gated-at.bofh.it>
In reply to#1640675
On Fri, 12 May 2017 11:25:35 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> On Fri, May 12, 2017 at 01:15:45PM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > 
> > As stack tracing now requires "rcu watching", force RCU to be watching when
> > recording a stack trace.
> > 
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>  
> 
> Assuming that you never get to __trace_stack() if in an NMI handler,
> this looks good to me!
> 
> In contrast, if if __trace_stack() ever is called from an NMI handler,
> invoking rcu_irq_enter() can be fatal.

Then someone may die.

OK, what's the case of running this in nmi? How does perf do it?

Do we just skip the check if it is in an nmi?

	if (!in_nmi()) {
		if (unlikely(rcu_irq_enter_disabled()))
			return;
		rcu_irq_enter();
	}

	__ftrace_trace_stack();

	if (!in_nmi())
		rcu_irq_exit();

?

-- Steve

[toc] | [prev] | [next] | [standalone]

#1640691 — Re: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-05-12 21:00 +0200
SubjectRe: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace
Message-ID<tGnXb-lv-7@gated-at.bofh.it>
In reply to#1640679
On Fri, May 12, 2017 at 02:36:19PM -0400, Steven Rostedt wrote:
> On Fri, 12 May 2017 11:25:35 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > On Fri, May 12, 2017 at 01:15:45PM -0400, Steven Rostedt wrote:
> > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > > 
> > > As stack tracing now requires "rcu watching", force RCU to be watching when
> > > recording a stack trace.
> > > 
> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>  
> > 
> > Assuming that you never get to __trace_stack() if in an NMI handler,
> > this looks good to me!
> > 
> > In contrast, if if __trace_stack() ever is called from an NMI handler,
> > invoking rcu_irq_enter() can be fatal.
> 
> Then someone may die.
> 
> OK, what's the case of running this in nmi? How does perf do it?

I have no idea.  If it cannot happen, then it cannot happen and all
is well, RCU is happy, and I am happy.  ;-)

> Do we just skip the check if it is in an nmi?
> 
> 	if (!in_nmi()) {
> 		if (unlikely(rcu_irq_enter_disabled()))
> 			return;
> 		rcu_irq_enter();
> 	}
> 
> 	__ftrace_trace_stack();
> 
> 	if (!in_nmi())
> 		rcu_irq_exit();
> 
> ?

If it -can- happen, bail out of the function without doing the
__ftrace_trace_stack()?  Or does that just cause other problems further
down the road?  Or BUG_ON(in_nmi())?

But again if it cannot happen, no problem and no need for extra code.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1640722 — Re: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-12 22:10 +0200
SubjectRe: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace
Message-ID<tGp2V-1mH-5@gated-at.bofh.it>
In reply to#1640691
On Fri, 12 May 2017 11:50:03 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> On Fri, May 12, 2017 at 02:36:19PM -0400, Steven Rostedt wrote:
> > On Fri, 12 May 2017 11:25:35 -0700
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> >   
> > > On Fri, May 12, 2017 at 01:15:45PM -0400, Steven Rostedt wrote:  
> > > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > > > 
> > > > As stack tracing now requires "rcu watching", force RCU to be watching when
> > > > recording a stack trace.
> > > > 
> > > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>    
> > > 
> > > Assuming that you never get to __trace_stack() if in an NMI handler,
> > > this looks good to me!
> > > 
> > > In contrast, if if __trace_stack() ever is called from an NMI handler,
> > > invoking rcu_irq_enter() can be fatal.  
> > 
> > Then someone may die.
> > 
> > OK, what's the case of running this in nmi? How does perf do it?  
> 
> I have no idea.  If it cannot happen, then it cannot happen and all
> is well, RCU is happy, and I am happy.  ;-)
> 
> > Do we just skip the check if it is in an nmi?
> > 
> > 	if (!in_nmi()) {
> > 		if (unlikely(rcu_irq_enter_disabled()))
> > 			return;
> > 		rcu_irq_enter();
> > 	}
> > 
> > 	__ftrace_trace_stack();
> > 
> > 	if (!in_nmi())
> > 		rcu_irq_exit();
> > 
> > ?  
> 
> If it -can- happen, bail out of the function without doing the

Why?

> __ftrace_trace_stack()?  Or does that just cause other problems further
> down the road?  Or BUG_ON(in_nmi())?

Why?

> 
> But again if it cannot happen, no problem and no need for extra code.
> 

We can't call stack trace from nmi anymore? It calls rcu_read_lock()
which is why we need to make sure rcu is watching, otherwise lockdep
complains.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1640734 — Re: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-05-12 22:40 +0200
SubjectRe: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace
Message-ID<tGpvY-1zo-5@gated-at.bofh.it>
In reply to#1640722
On Fri, May 12, 2017 at 04:05:32PM -0400, Steven Rostedt wrote:
> On Fri, 12 May 2017 11:50:03 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > On Fri, May 12, 2017 at 02:36:19PM -0400, Steven Rostedt wrote:
> > > On Fri, 12 May 2017 11:25:35 -0700
> > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > >   
> > > > On Fri, May 12, 2017 at 01:15:45PM -0400, Steven Rostedt wrote:  
> > > > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > > > > 
> > > > > As stack tracing now requires "rcu watching", force RCU to be watching when
> > > > > recording a stack trace.
> > > > > 
> > > > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>    
> > > > 
> > > > Assuming that you never get to __trace_stack() if in an NMI handler,
> > > > this looks good to me!
> > > > 
> > > > In contrast, if if __trace_stack() ever is called from an NMI handler,
> > > > invoking rcu_irq_enter() can be fatal.  
> > > 
> > > Then someone may die.
> > > 
> > > OK, what's the case of running this in nmi? How does perf do it?  
> > 
> > I have no idea.  If it cannot happen, then it cannot happen and all
> > is well, RCU is happy, and I am happy.  ;-)
> > 
> > > Do we just skip the check if it is in an nmi?
> > > 
> > > 	if (!in_nmi()) {
> > > 		if (unlikely(rcu_irq_enter_disabled()))
> > > 			return;
> > > 		rcu_irq_enter();
> > > 	}
> > > 
> > > 	__ftrace_trace_stack();
> > > 
> > > 	if (!in_nmi())
> > > 		rcu_irq_exit();
> > > 
> > > ?  
> > 
> > If it -can- happen, bail out of the function without doing the
> 
> Why?
> 
> > __ftrace_trace_stack()?  Or does that just cause other problems further
> > down the road?  Or BUG_ON(in_nmi())?
> 
> Why?
> 
> > But again if it cannot happen, no problem and no need for extra code.
> 
> We can't call stack trace from nmi anymore? It calls rcu_read_lock()
> which is why we need to make sure rcu is watching, otherwise lockdep
> complains.

Ah, finally got it!  If we are in_nmi(), you are relying on the
NMI handler's call to rcu_nmi_enter(), which works.  The piece I was
forgetting was that you also recently said in an unrelated LKML thread
that all the functions called at the very beginings and ends of NMI
handlers (which can see !in_nmi()) are marked notrace, so that should
be covered as well.

So never mind!  (And thank you for the explanation.)

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1643510 — Re: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-17 18:50 +0200
SubjectRe: [RFC][PATCH 1/5] tracing: Make sure RCU is watching before calling a stack trace
Message-ID<tIaj8-5vV-17@gated-at.bofh.it>
In reply to#1640734
On Fri, 12 May 2017 13:31:45 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> On Fri, May 12, 2017 at 04:05:32PM -0400, Steven Rostedt wrote:
> > On Fri, 12 May 2017 11:50:03 -0700
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> >   
> > > On Fri, May 12, 2017 at 02:36:19PM -0400, Steven Rostedt wrote:  
> > > > On Fri, 12 May 2017 11:25:35 -0700
> > > > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > > >     
> > > > > On Fri, May 12, 2017 at 01:15:45PM -0400, Steven Rostedt wrote:    
> > > > > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > > > > > 
> > > > > > As stack tracing now requires "rcu watching", force RCU to be watching when
> > > > > > recording a stack trace.
> > > > > > 
> > > > > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>      
> > > > > 
> > > > > Assuming that you never get to __trace_stack() if in an NMI handler,
> > > > > this looks good to me!
> > > > > 
> > > > > In contrast, if if __trace_stack() ever is called from an NMI handler,
> > > > > invoking rcu_irq_enter() can be fatal.    
> > > > 
> > > > Then someone may die.
> > > > 
> > > > OK, what's the case of running this in nmi? How does perf do it?    
> > > 
> > > I have no idea.  If it cannot happen, then it cannot happen and all
> > > is well, RCU is happy, and I am happy.  ;-)
> > >   
> > > > Do we just skip the check if it is in an nmi?
> > > > 
> > > > 	if (!in_nmi()) {
> > > > 		if (unlikely(rcu_irq_enter_disabled()))
> > > > 			return;
> > > > 		rcu_irq_enter();
> > > > 	}
> > > > 
> > > > 	__ftrace_trace_stack();
> > > > 
> > > > 	if (!in_nmi())
> > > > 		rcu_irq_exit();
> > > > 
> > > > ?    
> > > 
> > > If it -can- happen, bail out of the function without doing the  
> > 
> > Why?
> >   
> > > __ftrace_trace_stack()?  Or does that just cause other problems further
> > > down the road?  Or BUG_ON(in_nmi())?  
> > 
> > Why?
> >   
> > > But again if it cannot happen, no problem and no need for extra code.  
> > 
> > We can't call stack trace from nmi anymore? It calls rcu_read_lock()
> > which is why we need to make sure rcu is watching, otherwise lockdep
> > complains.  
> 
> Ah, finally got it!  If we are in_nmi(), you are relying on the
> NMI handler's call to rcu_nmi_enter(), which works.  The piece I was
> forgetting was that you also recently said in an unrelated LKML thread
> that all the functions called at the very beginings and ends of NMI
> handlers (which can see !in_nmi()) are marked notrace, so that should
> be covered as well.
> 
> So never mind!  (And thank you for the explanation.)

Is this an Acked-by?

-- Steve

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web