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

Groups > linux.kernel > #1618172 > unrolled thread

[PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions

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

  [PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions Steven Rostedt <rostedt@goodmis.org> - 2017-04-06 18:50 +0200
    Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-06 20:20 +0200
      Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable()  functions Steven Rostedt <rostedt@goodmis.org> - 2017-04-06 20:50 +0200
        Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-06 22:30 +0200
          Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable()  functions Steven Rostedt <rostedt@goodmis.org> - 2017-04-06 23:30 +0200
            Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-07 00:20 +0200
              Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable()  functions Steven Rostedt <rostedt@goodmis.org> - 2017-04-07 02:00 +0200

#1618172 — [PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

There are certain parts of the kernel that can not let stack tracing
proceed (namely in RCU), because the stack tracer uses RCU, and parts of RCU
internals can not handle having RCU read side locks taken.

Add stack_tracer_disable() and stack_tracer_enable() functions to let RCU
stop stack tracing on the current CPU as it is in those critical sections.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/ftrace.h     |  6 ++++++
 kernel/trace/trace_stack.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index ef7123219f14..40afee35565a 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -286,6 +286,12 @@ int
 stack_trace_sysctl(struct ctl_table *table, int write,
 		   void __user *buffer, size_t *lenp,
 		   loff_t *ppos);
+
+void stack_tracer_disable(void);
+void stack_tracer_enable(void);
+#else
+static inline void stack_tracer_disable(void) { }
+static inline void stack_tracer_enabe(void) { }
 #endif
 
 struct ftrace_func_command {
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 05ad2b86461e..5adbb73ec2ec 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -41,6 +41,34 @@ static DEFINE_MUTEX(stack_sysctl_mutex);
 int stack_tracer_enabled;
 static int last_stack_tracer_enabled;
 
+/**
+ * stack_tracer_disable - temporarily disable the stack tracer
+ *
+ * There's a few locations (namely in RCU) where stack tracing
+ * can not be executed. This function is used to disable stack
+ * tracing during those critical sections.
+ *
+ * This function will disable preemption. stack_tracer_enable()
+ * must be called shortly after this is called.
+ */
+void stack_tracer_disable(void)
+{
+	preempt_disable_notrace();
+	this_cpu_inc(trace_active);
+}
+
+/**
+ * stack_tracer_enable - re-enable the stack tracer
+ *
+ * After stack_tracer_disable() is called, stack_tracer_enable()
+ * must shortly be called afterward.
+ */
+void stack_tracer_enable(void)
+{
+	this_cpu_dec(trace_active);
+	preempt_enable_notrace();
+}
+
 void stack_trace_print(void)
 {
 	long i;
-- 
2.10.2

[toc] | [next] | [standalone]

#1618249

On Thu, Apr 06, 2017 at 12:42:40PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> There are certain parts of the kernel that can not let stack tracing
> proceed (namely in RCU), because the stack tracer uses RCU, and parts of RCU
> internals can not handle having RCU read side locks taken.
> 
> Add stack_tracer_disable() and stack_tracer_enable() functions to let RCU
> stop stack tracing on the current CPU as it is in those critical sections.

s/as it is in/when it is in/?

> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

One quibble above, one objection below.

							Thanx, Paul

> ---
>  include/linux/ftrace.h     |  6 ++++++
>  kernel/trace/trace_stack.c | 28 ++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index ef7123219f14..40afee35565a 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -286,6 +286,12 @@ int
>  stack_trace_sysctl(struct ctl_table *table, int write,
>  		   void __user *buffer, size_t *lenp,
>  		   loff_t *ppos);
> +
> +void stack_tracer_disable(void);
> +void stack_tracer_enable(void);
> +#else
> +static inline void stack_tracer_disable(void) { }
> +static inline void stack_tracer_enabe(void) { }
>  #endif
> 
>  struct ftrace_func_command {
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index 05ad2b86461e..5adbb73ec2ec 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -41,6 +41,34 @@ static DEFINE_MUTEX(stack_sysctl_mutex);
>  int stack_tracer_enabled;
>  static int last_stack_tracer_enabled;
> 
> +/**
> + * stack_tracer_disable - temporarily disable the stack tracer
> + *
> + * There's a few locations (namely in RCU) where stack tracing
> + * can not be executed. This function is used to disable stack
> + * tracing during those critical sections.
> + *
> + * This function will disable preemption. stack_tracer_enable()
> + * must be called shortly after this is called.
> + */
> +void stack_tracer_disable(void)
> +{
> +	preempt_disable_notrace();

Interrupts are disabled in all current call points, so you don't really
need to disable preemption.  I would normally not worry, given the
ease-of-use improvements, but some people get annoyed about even slight
increases in idle-entry overhead.

> +	this_cpu_inc(trace_active);
> +}
> +
> +/**
> + * stack_tracer_enable - re-enable the stack tracer
> + *
> + * After stack_tracer_disable() is called, stack_tracer_enable()
> + * must shortly be called afterward.
> + */
> +void stack_tracer_enable(void)
> +{
> +	this_cpu_dec(trace_active);
> +	preempt_enable_notrace();

Ditto...

> +}
> +
>  void stack_trace_print(void)
>  {
>  	long i;
> -- 
> 2.10.2
> 
> 

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

#1618266 — Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions

On Thu, 6 Apr 2017 11:12:22 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> On Thu, Apr 06, 2017 at 12:42:40PM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > 
> > There are certain parts of the kernel that can not let stack tracing
> > proceed (namely in RCU), because the stack tracer uses RCU, and parts of RCU
> > internals can not handle having RCU read side locks taken.
> > 
> > Add stack_tracer_disable() and stack_tracer_enable() functions to let RCU
> > stop stack tracing on the current CPU as it is in those critical sections.  
> 
> s/as it is in/when it is in/?
> 
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>  
> 
> One quibble above, one objection below.
> 
> 							Thanx, Paul
> 
> > ---
> >  include/linux/ftrace.h     |  6 ++++++
> >  kernel/trace/trace_stack.c | 28 ++++++++++++++++++++++++++++
> >  2 files changed, 34 insertions(+)
> > 
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index ef7123219f14..40afee35565a 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -286,6 +286,12 @@ int
> >  stack_trace_sysctl(struct ctl_table *table, int write,
> >  		   void __user *buffer, size_t *lenp,
> >  		   loff_t *ppos);
> > +
> > +void stack_tracer_disable(void);
> > +void stack_tracer_enable(void);
> > +#else
> > +static inline void stack_tracer_disable(void) { }
> > +static inline void stack_tracer_enabe(void) { }
> >  #endif
> > 
> >  struct ftrace_func_command {
> > diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> > index 05ad2b86461e..5adbb73ec2ec 100644
> > --- a/kernel/trace/trace_stack.c
> > +++ b/kernel/trace/trace_stack.c
> > @@ -41,6 +41,34 @@ static DEFINE_MUTEX(stack_sysctl_mutex);
> >  int stack_tracer_enabled;
> >  static int last_stack_tracer_enabled;
> > 
> > +/**
> > + * stack_tracer_disable - temporarily disable the stack tracer
> > + *
> > + * There's a few locations (namely in RCU) where stack tracing
> > + * can not be executed. This function is used to disable stack
> > + * tracing during those critical sections.
> > + *
> > + * This function will disable preemption. stack_tracer_enable()
> > + * must be called shortly after this is called.
> > + */
> > +void stack_tracer_disable(void)
> > +{
> > +	preempt_disable_notrace();  
> 
> Interrupts are disabled in all current call points, so you don't really
> need to disable preemption.  I would normally not worry, given the
> ease-of-use improvements, but some people get annoyed about even slight
> increases in idle-entry overhead.

My worry is that we add another caller that doesn't disable interrupts
or preemption.

I could add a __stack_trace_disable() that skips the disabling of
preemption, as the "__" usually denotes the call is "special".

-- Steve

> 
> > +	this_cpu_inc(trace_active);
> > +}
> > +
> > +/**
> > + * stack_tracer_enable - re-enable the stack tracer
> > + *
> > + * After stack_tracer_disable() is called, stack_tracer_enable()
> > + * must shortly be called afterward.
> > + */
> > +void stack_tracer_enable(void)
> > +{
> > +	this_cpu_dec(trace_active);
> > +	preempt_enable_notrace();  
> 
> Ditto...
> 
> > +}
> > +
> >  void stack_trace_print(void)
> >  {
> >  	long i;
> > -- 
> > 2.10.2
> > 
> >   

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

#1618333

On Thu, Apr 06, 2017 at 02:48:03PM -0400, Steven Rostedt wrote:
> On Thu, 6 Apr 2017 11:12:22 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > On Thu, Apr 06, 2017 at 12:42:40PM -0400, Steven Rostedt wrote:
> > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > > 
> > > There are certain parts of the kernel that can not let stack tracing
> > > proceed (namely in RCU), because the stack tracer uses RCU, and parts of RCU
> > > internals can not handle having RCU read side locks taken.
> > > 
> > > Add stack_tracer_disable() and stack_tracer_enable() functions to let RCU
> > > stop stack tracing on the current CPU as it is in those critical sections.  
> > 
> > s/as it is in/when it is in/?
> > 
> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>  
> > 
> > One quibble above, one objection below.
> > 
> > 							Thanx, Paul
> > 
> > > ---
> > >  include/linux/ftrace.h     |  6 ++++++
> > >  kernel/trace/trace_stack.c | 28 ++++++++++++++++++++++++++++
> > >  2 files changed, 34 insertions(+)
> > > 
> > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > > index ef7123219f14..40afee35565a 100644
> > > --- a/include/linux/ftrace.h
> > > +++ b/include/linux/ftrace.h
> > > @@ -286,6 +286,12 @@ int
> > >  stack_trace_sysctl(struct ctl_table *table, int write,
> > >  		   void __user *buffer, size_t *lenp,
> > >  		   loff_t *ppos);
> > > +
> > > +void stack_tracer_disable(void);
> > > +void stack_tracer_enable(void);
> > > +#else
> > > +static inline void stack_tracer_disable(void) { }
> > > +static inline void stack_tracer_enabe(void) { }
> > >  #endif
> > > 
> > >  struct ftrace_func_command {
> > > diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> > > index 05ad2b86461e..5adbb73ec2ec 100644
> > > --- a/kernel/trace/trace_stack.c
> > > +++ b/kernel/trace/trace_stack.c
> > > @@ -41,6 +41,34 @@ static DEFINE_MUTEX(stack_sysctl_mutex);
> > >  int stack_tracer_enabled;
> > >  static int last_stack_tracer_enabled;
> > > 
> > > +/**
> > > + * stack_tracer_disable - temporarily disable the stack tracer
> > > + *
> > > + * There's a few locations (namely in RCU) where stack tracing
> > > + * can not be executed. This function is used to disable stack
> > > + * tracing during those critical sections.
> > > + *
> > > + * This function will disable preemption. stack_tracer_enable()
> > > + * must be called shortly after this is called.
> > > + */
> > > +void stack_tracer_disable(void)
> > > +{
> > > +	preempt_disable_notrace();  
> > 
> > Interrupts are disabled in all current call points, so you don't really
> > need to disable preemption.  I would normally not worry, given the
> > ease-of-use improvements, but some people get annoyed about even slight
> > increases in idle-entry overhead.
> 
> My worry is that we add another caller that doesn't disable interrupts
> or preemption.
> 
> I could add a __stack_trace_disable() that skips the disabling of
> preemption, as the "__" usually denotes the call is "special".

Given that interrupts are disabled at that point, and given also that
NMI skips stack tracing if growth is required, could we just leave
out the stack_tracer_disable() and stack_tracer_enable()?

							Thanx, Paul

> -- Steve
> 
> > 
> > > +	this_cpu_inc(trace_active);
> > > +}
> > > +
> > > +/**
> > > + * stack_tracer_enable - re-enable the stack tracer
> > > + *
> > > + * After stack_tracer_disable() is called, stack_tracer_enable()
> > > + * must shortly be called afterward.
> > > + */
> > > +void stack_tracer_enable(void)
> > > +{
> > > +	this_cpu_dec(trace_active);
> > > +	preempt_enable_notrace();  
> > 
> > Ditto...
> > 
> > > +}
> > > +
> > >  void stack_trace_print(void)
> > >  {
> > >  	long i;
> > > -- 
> > > 2.10.2
> > > 
> > >   
> 

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

#1618358 — Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions

On Thu, 6 Apr 2017 13:21:17 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> > My worry is that we add another caller that doesn't disable interrupts
> > or preemption.
> > 
> > I could add a __stack_trace_disable() that skips the disabling of
> > preemption, as the "__" usually denotes the call is "special".  
> 
> Given that interrupts are disabled at that point, and given also that
> NMI skips stack tracing if growth is required, could we just leave
> out the stack_tracer_disable() and stack_tracer_enable()?

There may be other use cases. Hmm, maybe I'll just have it do a check
to make sure preemption is disabled. Something like:

	if (IS_ENABLED(CONFIG_DEBUG_PREEMPT))
		WARN_ON_ONCE(!preempt_count());

-- Steve

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

#1618376

On Thu, Apr 06, 2017 at 05:23:48PM -0400, Steven Rostedt wrote:
> On Thu, 6 Apr 2017 13:21:17 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > > My worry is that we add another caller that doesn't disable interrupts
> > > or preemption.
> > > 
> > > I could add a __stack_trace_disable() that skips the disabling of
> > > preemption, as the "__" usually denotes the call is "special".  
> > 
> > Given that interrupts are disabled at that point, and given also that
> > NMI skips stack tracing if growth is required, could we just leave
> > out the stack_tracer_disable() and stack_tracer_enable()?
> 
> There may be other use cases. Hmm, maybe I'll just have it do a check
> to make sure preemption is disabled. Something like:
> 
> 	if (IS_ENABLED(CONFIG_DEBUG_PREEMPT))
> 		WARN_ON_ONCE(!preempt_count());

This in the include/linux/ftrace.h file so that it can be inlined?
That makes sense to me.

							Thanx, Paul

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

#1618412 — Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions

On Thu, 6 Apr 2017 15:08:21 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> On Thu, Apr 06, 2017 at 05:23:48PM -0400, Steven Rostedt wrote:
> > On Thu, 6 Apr 2017 13:21:17 -0700
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> >   
> > > > My worry is that we add another caller that doesn't disable interrupts
> > > > or preemption.
> > > > 
> > > > I could add a __stack_trace_disable() that skips the disabling of
> > > > preemption, as the "__" usually denotes the call is "special".    
> > > 
> > > Given that interrupts are disabled at that point, and given also that
> > > NMI skips stack tracing if growth is required, could we just leave
> > > out the stack_tracer_disable() and stack_tracer_enable()?  
> > 
> > There may be other use cases. Hmm, maybe I'll just have it do a check
> > to make sure preemption is disabled. Something like:
> > 
> > 	if (IS_ENABLED(CONFIG_DEBUG_PREEMPT))
> > 		WARN_ON_ONCE(!preempt_count());  
> 
> This in the include/linux/ftrace.h file so that it can be inlined?
> That makes sense to me.
> 

Hah, I already had that part (inlining) written. It's a separate patch
though. I'll post another series tomorrow.

-- Steve

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web