[PATCH v2 0/9] tracing vs rcu vs nmi

[PATCH v2 0/9] tracing vs rcu vs nmi

[Peter Zijlstra]

Hi all,

These here patches are the result of Mathieu and Steve trying to get commit
865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle
tracepoints") reverted again.

One of the things discovered is that tracing MUST NOT happen before nmi_enter()
or after nmi_exit(). Audit results of the previous version are still valid.

Changes since -v1:

 - Added tags
 - Changed #4; changed nmi_enter() to use __preempt_count_add() vs
   marking preempt_count_add() notrace.
 - Changed #5; confusion on which functions are notrace due to Makefile
 - Added #9; remove limitation on the perf-function-trace coupling

[PATCH v2 1/9] rcu: Rename rcu_irq_{enter,exit}_irqson()

[Peter Zijlstra]

The functions do in fact use local_irq_{save,restore}() and can
therefore be used when IRQs are in fact disabled. Worse, they are
already used in places where IRQs are disabled, leading to great
confusion when reading the code.

Rename them to fix this confusion.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/rcupdate.h   |    4 ++--
 include/linux/rcutiny.h    |    4 ++--
 include/linux/rcutree.h    |    4 ++--
 include/linux/tracepoint.h |    4 ++--
 kernel/cpu_pm.c            |    4 ++--
 kernel/rcu/tree.c          |    8 ++++----
 kernel/trace/trace.c       |    4 ++--
 7 files changed, 16 insertions(+), 16 deletions(-)

--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -120,9 +120,9 @@ static inline void rcu_init_nohz(void) {
  */
 #define RCU_NONIDLE(a) \
 	do { \
-		rcu_irq_enter_irqson(); \
+		rcu_irq_enter_irqsave(); \
 		do { a; } while (0); \
-		rcu_irq_exit_irqson(); \
+		rcu_irq_exit_irqsave(); \
 	} while (0)
 
 /*
--- a/include/linux/rcutiny.h
+++ b/include/linux/rcutiny.h
@@ -68,8 +68,8 @@ static inline int rcu_jiffies_till_stall
 static inline void rcu_idle_enter(void) { }
 static inline void rcu_idle_exit(void) { }
 static inline void rcu_irq_enter(void) { }
-static inline void rcu_irq_exit_irqson(void) { }
-static inline void rcu_irq_enter_irqson(void) { }
+static inline void rcu_irq_exit_irqsave(void) { }
+static inline void rcu_irq_enter_irqsave(void) { }
 static inline void rcu_irq_exit(void) { }
 static inline void exit_rcu(void) { }
 static inline bool rcu_preempt_need_deferred_qs(struct task_struct *t)
--- a/include/linux/rcutree.h
+++ b/include/linux/rcutree.h
@@ -46,8 +46,8 @@ void rcu_idle_enter(void);
 void rcu_idle_exit(void);
 void rcu_irq_enter(void);
 void rcu_irq_exit(void);
-void rcu_irq_enter_irqson(void);
-void rcu_irq_exit_irqson(void);
+void rcu_irq_enter_irqsave(void);
+void rcu_irq_exit_irqsave(void);
 
 void exit_rcu(void);
 
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -181,7 +181,7 @@ static inline struct tracepoint *tracepo
 		 */							\
 		if (rcuidle) {						\
 			__idx = srcu_read_lock_notrace(&tracepoint_srcu);\
-			rcu_irq_enter_irqson();				\
+			rcu_irq_enter_irqsave();			\
 		}							\
 									\
 		it_func_ptr = rcu_dereference_raw((tp)->funcs);		\
@@ -195,7 +195,7 @@ static inline struct tracepoint *tracepo
 		}							\
 									\
 		if (rcuidle) {						\
-			rcu_irq_exit_irqson();				\
+			rcu_irq_exit_irqsave();				\
 			srcu_read_unlock_notrace(&tracepoint_srcu, __idx);\
 		}							\
 									\
--- a/kernel/cpu_pm.c
+++ b/kernel/cpu_pm.c
@@ -24,10 +24,10 @@ static int cpu_pm_notify(enum cpu_pm_eve
 	 * could be disfunctional in cpu idle. Copy RCU_NONIDLE code to let
 	 * RCU know this.
 	 */
-	rcu_irq_enter_irqson();
+	rcu_irq_enter_irqsave();
 	ret = __atomic_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL,
 		nr_to_call, nr_calls);
-	rcu_irq_exit_irqson();
+	rcu_irq_exit_irqsave();
 
 	return notifier_to_errno(ret);
 }
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -699,10 +699,10 @@ void rcu_irq_exit(void)
 /*
  * Wrapper for rcu_irq_exit() where interrupts are enabled.
  *
- * If you add or remove a call to rcu_irq_exit_irqson(), be sure to test
+ * If you add or remove a call to rcu_irq_exit_irqsave(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-void rcu_irq_exit_irqson(void)
+void rcu_irq_exit_irqsave(void)
 {
 	unsigned long flags;
 
@@ -875,10 +875,10 @@ void rcu_irq_enter(void)
 /*
  * Wrapper for rcu_irq_enter() where interrupts are enabled.
  *
- * If you add or remove a call to rcu_irq_enter_irqson(), be sure to test
+ * If you add or remove a call to rcu_irq_enter_irqsave(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-void rcu_irq_enter_irqson(void)
+void rcu_irq_enter_irqsave(void)
 {
 	unsigned long flags;
 
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3004,9 +3004,9 @@ void __trace_stack(struct trace_array *t
 	if (unlikely(in_nmi()))
 		return;
 
-	rcu_irq_enter_irqson();
+	rcu_irq_enter_irqsave();
 	__ftrace_trace_stack(buffer, flags, skip, pc, NULL);
-	rcu_irq_exit_irqson();
+	rcu_irq_exit_irqsave();
 }
 
 /**

Re: [PATCH v2 1/9] rcu: Rename rcu_irq_{enter,exit}_irqson()

[Joel Fernandes]

On Wed, Feb 12, 2020 at 10:01:40PM +0100, Peter Zijlstra wrote:
> The functions do in fact use local_irq_{save,restore}() and can
> therefore be used when IRQs are in fact disabled. Worse, they are
> already used in places where IRQs are disabled, leading to great
> confusion when reading the code.
> 
> Rename them to fix this confusion.


Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel

> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/rcupdate.h   |    4 ++--
>  include/linux/rcutiny.h    |    4 ++--
>  include/linux/rcutree.h    |    4 ++--
>  include/linux/tracepoint.h |    4 ++--
>  kernel/cpu_pm.c            |    4 ++--
>  kernel/rcu/tree.c          |    8 ++++----
>  kernel/trace/trace.c       |    4 ++--
>  7 files changed, 16 insertions(+), 16 deletions(-)
> 
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -120,9 +120,9 @@ static inline void rcu_init_nohz(void) {
>   */
>  #define RCU_NONIDLE(a) \
>  	do { \
> -		rcu_irq_enter_irqson(); \
> +		rcu_irq_enter_irqsave(); \
>  		do { a; } while (0); \
> -		rcu_irq_exit_irqson(); \
> +		rcu_irq_exit_irqsave(); \
>  	} while (0)
>  
>  /*
> --- a/include/linux/rcutiny.h
> +++ b/include/linux/rcutiny.h
> @@ -68,8 +68,8 @@ static inline int rcu_jiffies_till_stall
>  static inline void rcu_idle_enter(void) { }
>  static inline void rcu_idle_exit(void) { }
>  static inline void rcu_irq_enter(void) { }
> -static inline void rcu_irq_exit_irqson(void) { }
> -static inline void rcu_irq_enter_irqson(void) { }
> +static inline void rcu_irq_exit_irqsave(void) { }
> +static inline void rcu_irq_enter_irqsave(void) { }
>  static inline void rcu_irq_exit(void) { }
>  static inline void exit_rcu(void) { }
>  static inline bool rcu_preempt_need_deferred_qs(struct task_struct *t)
> --- a/include/linux/rcutree.h
> +++ b/include/linux/rcutree.h
> @@ -46,8 +46,8 @@ void rcu_idle_enter(void);
>  void rcu_idle_exit(void);
>  void rcu_irq_enter(void);
>  void rcu_irq_exit(void);
> -void rcu_irq_enter_irqson(void);
> -void rcu_irq_exit_irqson(void);
> +void rcu_irq_enter_irqsave(void);
> +void rcu_irq_exit_irqsave(void);
>  
>  void exit_rcu(void);
>  
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -181,7 +181,7 @@ static inline struct tracepoint *tracepo
>  		 */							\
>  		if (rcuidle) {						\
>  			__idx = srcu_read_lock_notrace(&tracepoint_srcu);\
> -			rcu_irq_enter_irqson();				\
> +			rcu_irq_enter_irqsave();			\
>  		}							\
>  									\
>  		it_func_ptr = rcu_dereference_raw((tp)->funcs);		\
> @@ -195,7 +195,7 @@ static inline struct tracepoint *tracepo
>  		}							\
>  									\
>  		if (rcuidle) {						\
> -			rcu_irq_exit_irqson();				\
> +			rcu_irq_exit_irqsave();				\
>  			srcu_read_unlock_notrace(&tracepoint_srcu, __idx);\
>  		}							\
>  									\
> --- a/kernel/cpu_pm.c
> +++ b/kernel/cpu_pm.c
> @@ -24,10 +24,10 @@ static int cpu_pm_notify(enum cpu_pm_eve
>  	 * could be disfunctional in cpu idle. Copy RCU_NONIDLE code to let
>  	 * RCU know this.
>  	 */
> -	rcu_irq_enter_irqson();
> +	rcu_irq_enter_irqsave();
>  	ret = __atomic_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL,
>  		nr_to_call, nr_calls);
> -	rcu_irq_exit_irqson();
> +	rcu_irq_exit_irqsave();
>  
>  	return notifier_to_errno(ret);
>  }
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -699,10 +699,10 @@ void rcu_irq_exit(void)
>  /*
>   * Wrapper for rcu_irq_exit() where interrupts are enabled.
>   *
> - * If you add or remove a call to rcu_irq_exit_irqson(), be sure to test
> + * If you add or remove a call to rcu_irq_exit_irqsave(), be sure to test
>   * with CONFIG_RCU_EQS_DEBUG=y.
>   */
> -void rcu_irq_exit_irqson(void)
> +void rcu_irq_exit_irqsave(void)
>  {
>  	unsigned long flags;
>  
> @@ -875,10 +875,10 @@ void rcu_irq_enter(void)
>  /*
>   * Wrapper for rcu_irq_enter() where interrupts are enabled.
>   *
> - * If you add or remove a call to rcu_irq_enter_irqson(), be sure to test
> + * If you add or remove a call to rcu_irq_enter_irqsave(), be sure to test
>   * with CONFIG_RCU_EQS_DEBUG=y.
>   */
> -void rcu_irq_enter_irqson(void)
> +void rcu_irq_enter_irqsave(void)
>  {
>  	unsigned long flags;
>  
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3004,9 +3004,9 @@ void __trace_stack(struct trace_array *t
>  	if (unlikely(in_nmi()))
>  		return;
>  
> -	rcu_irq_enter_irqson();
> +	rcu_irq_enter_irqsave();
>  	__ftrace_trace_stack(buffer, flags, skip, pc, NULL);
> -	rcu_irq_exit_irqson();
> +	rcu_irq_exit_irqsave();
>  }
>  
>  /**
> 
> 

[PATCH v2 2/9] rcu: Mark rcu_dynticks_curr_cpu_in_eqs() inline

[Peter Zijlstra]

Since rcu_is_watching() is notrace (and needs to be, as it can be
called from the tracers), make sure everything it in turn calls is
notrace too.

To that effect, mark rcu_dynticks_curr_cpu_in_eqs() inline, which
implies notrace, as the function is tiny.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/rcu/tree.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -294,7 +294,7 @@ static void rcu_dynticks_eqs_online(void
  *
  * No ordering, as we are sampling CPU-local information.
  */
-static bool rcu_dynticks_curr_cpu_in_eqs(void)
+static inline bool rcu_dynticks_curr_cpu_in_eqs(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 

Re: [PATCH v2 2/9] rcu: Mark rcu_dynticks_curr_cpu_in_eqs() inline

[Joel Fernandes]

On Wed, Feb 12, 2020 at 10:01:41PM +0100, Peter Zijlstra wrote:
> Since rcu_is_watching() is notrace (and needs to be, as it can be
> called from the tracers), make sure everything it in turn calls is
> notrace too.
> 
> To that effect, mark rcu_dynticks_curr_cpu_in_eqs() inline, which
> implies notrace, as the function is tiny.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/rcu/tree.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -294,7 +294,7 @@ static void rcu_dynticks_eqs_online(void
>   *
>   * No ordering, as we are sampling CPU-local information.
>   */
> -static bool rcu_dynticks_curr_cpu_in_eqs(void)
> +static inline bool rcu_dynticks_curr_cpu_in_eqs(void)

I think there are ways to turn off function inlining, such as gcc's:
-fkeep-inline-functions

And just to be sure weird compilers (clang *cough*) don't screw this up,
could we make it static inline notrace?

Build tested it on the tip tree on top of your patch:

---8<-----------------------

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index f3cb824fe5bbf..078d56951c8e7 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -294,7 +294,7 @@ static void rcu_dynticks_eqs_online(void)
  *
  * No ordering, as we are sampling CPU-local information.
  */
-static inline bool rcu_dynticks_curr_cpu_in_eqs(void)
+static inline notrace bool rcu_dynticks_curr_cpu_in_eqs(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 

Re: [PATCH v2 2/9] rcu: Mark rcu_dynticks_curr_cpu_in_eqs() inline

[Steven Rostedt]

On Wed, 12 Feb 2020 17:38:18 -0500
Joel Fernandes <joel@joelfernandes.org> wrote:

> I think there are ways to turn off function inlining, such as gcc's:
> -fkeep-inline-functions
> 
> And just to be sure weird compilers (clang *cough*) don't screw this up,
> could we make it static inline notrace?

inline is defined as notrace, so not needed.

I did that because of surprises when functions marked as inline
suddenly became non inlined and traced, which caused issues with
function tracing (before I finally got recursion protection working).
But even then, I figured, if something is inlined and gcc actually
inlines it, it wont be traced. For consistency, if something is marked
inline, it should not be traced.

-- Steve

Re: [PATCH v2 2/9] rcu: Mark rcu_dynticks_curr_cpu_in_eqs() inline

[Joel Fernandes]

On Wed, Feb 12, 2020 at 8:41 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 12 Feb 2020 17:38:18 -0500
> Joel Fernandes <joel@joelfernandes.org> wrote:
>
> > I think there are ways to turn off function inlining, such as gcc's:
> > -fkeep-inline-functions
> >
> > And just to be sure weird compilers (clang *cough*) don't screw this up,
> > could we make it static inline notrace?
>
> inline is defined as notrace, so not needed.
>
> I did that because of surprises when functions marked as inline
> suddenly became non inlined and traced, which caused issues with
> function tracing (before I finally got recursion protection working).
> But even then, I figured, if something is inlined and gcc actually
> inlines it, it wont be traced. For consistency, if something is marked
> inline, it should not be traced.

Ah I see it, thanks for the clarification Steve! That looks like a
good idea. I withdraw my previous comment.

- Joel

[PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Peter Zijlstra]

To facilitate tracers that need RCU, add some helpers to wrap the
magic required.

The problem is that we can call into tracers (trace events and
function tracing) while RCU isn't watching and this can happen from
any context, including NMI.

It is this latter that is causing most of the trouble; we must make
sure in_nmi() returns true before we land in anything tracing,
otherwise we cannot recover.

These helpers are macros because of header-hell; they're placed here
because of the proximity to nmi_{enter,exit{().

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/hardirq.h |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -89,4 +89,52 @@ extern void irq_exit(void);
 		arch_nmi_exit();				\
 	} while (0)
 
+/*
+ * Tracing vs RCU
+ * --------------
+ *
+ * tracepoints and function-tracing can happen when RCU isn't watching (idle,
+ * or early IRQ/NMI entry).
+ *
+ * When it happens during idle or early during IRQ entry, tracing will have
+ * to inform RCU that it ought to pay attention, this is done by calling
+ * rcu_irq_enter_irqsave().
+ *
+ * On NMI entry, we must be very careful that tracing only happens after we've
+ * incremented preempt_count(), otherwise we cannot tell we're in NMI and take
+ * the special path.
+ */
+
+#define __TR_IRQ	1
+#define __TR_NMI	2
+
+#define trace_rcu_enter()					\
+({								\
+	unsigned long state = 0;				\
+	if (!rcu_is_watching())	{				\
+		if (in_nmi()) {					\
+			state = __TR_NMI;			\
+			rcu_nmi_enter();			\
+		} else {					\
+			state = __TR_IRQ;			\
+			rcu_irq_enter_irqsave();		\
+		}						\
+	}							\
+	state;							\
+})
+
+#define trace_rcu_exit(state)					\
+do {								\
+	switch (state) {					\
+	case __TR_IRQ:						\
+		rcu_irq_exit_irqsave();				\
+		break;						\
+	case __TR_NMI:						\
+		rcu_nmi_exit();					\
+		break;						\
+	default:						\
+		break;						\
+	}							\
+} while (0)
+
 #endif /* LINUX_HARDIRQ_H */

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Joel Fernandes]

On Wed, Feb 12, 2020 at 10:01:42PM +0100, Peter Zijlstra wrote:
> To facilitate tracers that need RCU, add some helpers to wrap the
> magic required.
> 
> The problem is that we can call into tracers (trace events and
> function tracing) while RCU isn't watching and this can happen from
> any context, including NMI.
> 
> It is this latter that is causing most of the trouble; we must make
> sure in_nmi() returns true before we land in anything tracing,
> otherwise we cannot recover.
> 
> These helpers are macros because of header-hell; they're placed here
> because of the proximity to nmi_{enter,exit{().
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/hardirq.h |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> --- a/include/linux/hardirq.h
> +++ b/include/linux/hardirq.h
> @@ -89,4 +89,52 @@ extern void irq_exit(void);
>  		arch_nmi_exit();				\
>  	} while (0)
>  
> +/*
> + * Tracing vs RCU
> + * --------------
> + *
> + * tracepoints and function-tracing can happen when RCU isn't watching (idle,
> + * or early IRQ/NMI entry).
> + *
> + * When it happens during idle or early during IRQ entry, tracing will have
> + * to inform RCU that it ought to pay attention, this is done by calling
> + * rcu_irq_enter_irqsave().
> + *
> + * On NMI entry, we must be very careful that tracing only happens after we've
> + * incremented preempt_count(), otherwise we cannot tell we're in NMI and take
> + * the special path.
> + */
> +
> +#define __TR_IRQ	1
> +#define __TR_NMI	2
> +
> +#define trace_rcu_enter()					\
> +({								\
> +	unsigned long state = 0;				\
> +	if (!rcu_is_watching())	{				\
> +		if (in_nmi()) {					\
> +			state = __TR_NMI;			\
> +			rcu_nmi_enter();			\
> +		} else {					\
> +			state = __TR_IRQ;			\
> +			rcu_irq_enter_irqsave();		\

I think this can be simplified. You don't need to rely on in_nmi() here. I
believe for NMI's, you can just call rcu_irq_enter_irqsave() and that should
be sufficient to get RCU watching. Paul can correct me if I'm wrong, but I am
pretty sure that would work.

In fact, I think a better naming for rcu_irq_enter_irqsave() pair could be
(in the first patch):

rcu_ensure_watching_begin();
rcu_ensure_watching_end();

thanks,

 - Joel



> +		}						\
> +	}							\
> +	state;							\
> +})
> +
> +#define trace_rcu_exit(state)					\
> +do {								\
> +	switch (state) {					\
> +	case __TR_IRQ:						\
> +		rcu_irq_exit_irqsave();				\
> +		break;						\
> +	case __TR_NMI:						\
> +		rcu_nmi_exit();					\
> +		break;						\
> +	default:						\
> +		break;						\
> +	}							\
> +} while (0)
> +
>  #endif /* LINUX_HARDIRQ_H */
> 
> 

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Peter Zijlstra]

On Wed, Feb 12, 2020 at 06:20:05PM -0500, Joel Fernandes wrote:
> On Wed, Feb 12, 2020 at 10:01:42PM +0100, Peter Zijlstra wrote:

> > +#define trace_rcu_enter()					\
> > +({								\
> > +	unsigned long state = 0;				\
> > +	if (!rcu_is_watching())	{				\
> > +		if (in_nmi()) {					\
> > +			state = __TR_NMI;			\
> > +			rcu_nmi_enter();			\
> > +		} else {					\
> > +			state = __TR_IRQ;			\
> > +			rcu_irq_enter_irqsave();		\
> 
> I think this can be simplified. You don't need to rely on in_nmi() here. I
> believe for NMI's, you can just call rcu_irq_enter_irqsave() and that should
> be sufficient to get RCU watching. Paul can correct me if I'm wrong, but I am
> pretty sure that would work.
> 
> In fact, I think a better naming for rcu_irq_enter_irqsave() pair could be
> (in the first patch):
> 
> rcu_ensure_watching_begin();
> rcu_ensure_watching_end();

So I hadn't looked deeply into rcu_irq_enter(), it seems to call
rcu_nmi_enter_common(), but with @irq=true.

What exactly is the purpose of that @irq argument, and how much will it
hurt to lie there? Will it come apart if we have @irq != !in_nmi()
for example?

There is a comment in there that says ->dynticks_nmi_nesting ought to be
odd only if we're in NMI. The only place that seems to care is
rcu_nmi_exit_common(), and that does indeed do something different for
IRQs vs NMIs.

So I don't think we can blindly unify this. But perhaps Paul sees a way?

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Joel Fernandes]

On Thu, Feb 13, 2020 at 09:27:16AM +0100, Peter Zijlstra wrote:
> On Wed, Feb 12, 2020 at 06:20:05PM -0500, Joel Fernandes wrote:
> > On Wed, Feb 12, 2020 at 10:01:42PM +0100, Peter Zijlstra wrote:
> 
> > > +#define trace_rcu_enter()					\
> > > +({								\
> > > +	unsigned long state = 0;				\
> > > +	if (!rcu_is_watching())	{				\
> > > +		if (in_nmi()) {					\
> > > +			state = __TR_NMI;			\
> > > +			rcu_nmi_enter();			\
> > > +		} else {					\
> > > +			state = __TR_IRQ;			\
> > > +			rcu_irq_enter_irqsave();		\
> > 
> > I think this can be simplified. You don't need to rely on in_nmi() here. I
> > believe for NMI's, you can just call rcu_irq_enter_irqsave() and that should
> > be sufficient to get RCU watching. Paul can correct me if I'm wrong, but I am
> > pretty sure that would work.
> > 
> > In fact, I think a better naming for rcu_irq_enter_irqsave() pair could be
> > (in the first patch):
> > 
> > rcu_ensure_watching_begin();
> > rcu_ensure_watching_end();
> 
> So I hadn't looked deeply into rcu_irq_enter(), it seems to call
> rcu_nmi_enter_common(), but with @irq=true.
> 
> What exactly is the purpose of that @irq argument, and how much will it
> hurt to lie there? Will it come apart if we have @irq != !in_nmi()
> for example?

At least rcu_dynticks_task_exit() and rcu_cleanup_after_idle() seem to be
safe regardless of IRQ or NMI. If they are safe either way, we should
probably get look into removing @irq. But I'm not fully sure and looking
forward to Paul's thought on that.. I would love for us to simplify that as
well if possible.

> There is a comment in there that says ->dynticks_nmi_nesting ought to be
> odd only if we're in NMI. The only place that seems to care is
> rcu_nmi_exit_common(), and that does indeed do something different for
> IRQs vs NMIs.

I know a bit about the counters. I had previously unified it and it passed
RCU torture testing etc. (Didn't get merge as Paul wanted it done slightly
done differently) : https://lore.kernel.org/patchwork/patch/1120022/ . I am
planning to get back to finishing that soon.

About the comment on dynticks_nmi_nesting and counters, you mean this comment
right? The odd value of '1' is just to catch the outer most handler. We need
to enter/exit the EQS (extended QS) state only from the outermost handler. I
believe that would work regardless whether it is NMI and IRQ that are
nesting. If IRQs could nest within other IRQs in Linux, then that could also
very well have used the odd/ even trick.

	/*
	 * If idle from RCU viewpoint, atomically increment ->dynticks
	 * to mark non-idle and increment ->dynticks_nmi_nesting by one.
	 * Otherwise, increment ->dynticks_nmi_nesting by two.  This means
	 * if ->dynticks_nmi_nesting is equal to one, we are guaranteed
	 * to be in the outermost NMI handler that interrupted an RCU-idle
	 * period (observation due to Andy Lutomirski).
	 */

thanks,

 - Joel

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Paul E. McKenney]

On Thu, Feb 13, 2020 at 09:27:16AM +0100, Peter Zijlstra wrote:
> On Wed, Feb 12, 2020 at 06:20:05PM -0500, Joel Fernandes wrote:
> > On Wed, Feb 12, 2020 at 10:01:42PM +0100, Peter Zijlstra wrote:
> 
> > > +#define trace_rcu_enter()					\
> > > +({								\
> > > +	unsigned long state = 0;				\
> > > +	if (!rcu_is_watching())	{				\
> > > +		if (in_nmi()) {					\
> > > +			state = __TR_NMI;			\
> > > +			rcu_nmi_enter();			\
> > > +		} else {					\
> > > +			state = __TR_IRQ;			\
> > > +			rcu_irq_enter_irqsave();		\
> > 
> > I think this can be simplified. You don't need to rely on in_nmi() here. I
> > believe for NMI's, you can just call rcu_irq_enter_irqsave() and that should
> > be sufficient to get RCU watching. Paul can correct me if I'm wrong, but I am
> > pretty sure that would work.
> > 
> > In fact, I think a better naming for rcu_irq_enter_irqsave() pair could be
> > (in the first patch):
> > 
> > rcu_ensure_watching_begin();
> > rcu_ensure_watching_end();
> 
> So I hadn't looked deeply into rcu_irq_enter(), it seems to call
> rcu_nmi_enter_common(), but with @irq=true.
> 
> What exactly is the purpose of that @irq argument, and how much will it
> hurt to lie there? Will it come apart if we have @irq != !in_nmi()
> for example?
> 
> There is a comment in there that says ->dynticks_nmi_nesting ought to be
> odd only if we're in NMI. The only place that seems to care is
> rcu_nmi_exit_common(), and that does indeed do something different for
> IRQs vs NMIs.
> 
> So I don't think we can blindly unify this. But perhaps Paul sees a way?

The reason for the irq argument is to avoid invoking
rcu_prepare_for_idle() and rcu_dynticks_task_enter() from NMI context
from rcu_nmi_exit_common().  Similarly, we need to avoid invoking
rcu_dynticks_task_exit() and rcu_cleanup_after_idle() from NMI context
from rcu_nmi_enter_common().

It might well be that I could make these functions be NMI-safe, but
rcu_prepare_for_idle() in particular would be a bit ugly at best.
So, before looking into that, I have a question.  Given these proposed
changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
to just use in_nmi()?

							Thanx, Paul

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Peter Zijlstra]

On Thu, Feb 13, 2020 at 05:51:38AM -0800, Paul E. McKenney wrote:

> The reason for the irq argument is to avoid invoking
> rcu_prepare_for_idle() and rcu_dynticks_task_enter() from NMI context
> from rcu_nmi_exit_common().  Similarly, we need to avoid invoking
> rcu_dynticks_task_exit() and rcu_cleanup_after_idle() from NMI context
> from rcu_nmi_enter_common().

Aaah, I see. I didn't grep hard enough earlier today (I only found
stubs). Yes, those take locks, we mustn't call them from NMI context.

> It might well be that I could make these functions be NMI-safe, but
> rcu_prepare_for_idle() in particular would be a bit ugly at best.
> So, before looking into that, I have a question.  Given these proposed
> changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> to just use in_nmi()?

That _should_ already be the case today. That is, if we end up in a
tracer and in_nmi() is unreliable we're already screwed anyway.

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Paul E. McKenney]

On Thu, Feb 13, 2020 at 05:40:31PM +0100, Peter Zijlstra wrote:
> On Thu, Feb 13, 2020 at 05:51:38AM -0800, Paul E. McKenney wrote:
> 
> > The reason for the irq argument is to avoid invoking
> > rcu_prepare_for_idle() and rcu_dynticks_task_enter() from NMI context
> > from rcu_nmi_exit_common().  Similarly, we need to avoid invoking
> > rcu_dynticks_task_exit() and rcu_cleanup_after_idle() from NMI context
> > from rcu_nmi_enter_common().
> 
> Aaah, I see. I didn't grep hard enough earlier today (I only found
> stubs). Yes, those take locks, we mustn't call them from NMI context.

Been there, done that...

> > It might well be that I could make these functions be NMI-safe, but
> > rcu_prepare_for_idle() in particular would be a bit ugly at best.
> > So, before looking into that, I have a question.  Given these proposed
> > changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> > to just use in_nmi()?
> 
> That _should_ already be the case today. That is, if we end up in a
> tracer and in_nmi() is unreliable we're already screwed anyway.

So something like this, then?  This is untested, probably doesn't even
build, and could use some careful review from both Peter and Steve,
at least.  As in the below is the second version of the patch, the first
having been missing a couple of important "!" characters.

							Thanx, Paul

------------------------------------------------------------------------

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 1f5fdf7..f783572 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -623,16 +623,18 @@ void rcu_user_enter(void)
 }
 #endif /* CONFIG_NO_HZ_FULL */
 
-/*
+/**
+ * rcu_nmi_exit - inform RCU of exit from NMI context
+ *
  * If we are returning from the outermost NMI handler that interrupted an
  * RCU-idle period, update rdp->dynticks and rdp->dynticks_nmi_nesting
  * to let the RCU grace-period handling know that the CPU is back to
  * being RCU-idle.
  *
- * If you add or remove a call to rcu_nmi_exit_common(), be sure to test
+ * If you add or remove a call to rcu_nmi_exit(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-static __always_inline void rcu_nmi_exit_common(bool irq)
+static __always_inline void rcu_nmi_exit(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 
@@ -660,27 +662,16 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
 	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
 	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
 
-	if (irq)
+	if (!in_nmi())
 		rcu_prepare_for_idle();
 
 	rcu_dynticks_eqs_enter();
 
-	if (irq)
+	if (!in_nmi())
 		rcu_dynticks_task_enter();
 }
 
 /**
- * rcu_nmi_exit - inform RCU of exit from NMI context
- *
- * If you add or remove a call to rcu_nmi_exit(), be sure to test
- * with CONFIG_RCU_EQS_DEBUG=y.
- */
-void rcu_nmi_exit(void)
-{
-	rcu_nmi_exit_common(false);
-}
-
-/**
  * rcu_irq_exit - inform RCU that current CPU is exiting irq towards idle
  *
  * Exit from an interrupt handler, which might possibly result in entering
@@ -702,7 +693,7 @@ void rcu_nmi_exit(void)
 void rcu_irq_exit(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_nmi_exit_common(true);
+	rcu_nmi_exit();
 }
 
 /*
@@ -786,7 +777,7 @@ void rcu_user_exit(void)
 #endif /* CONFIG_NO_HZ_FULL */
 
 /**
- * rcu_nmi_enter_common - inform RCU of entry to NMI context
+ * rcu_nmi_enter - inform RCU of entry to NMI context
  * @irq: Is this call from rcu_irq_enter?
  *
  * If the CPU was idle from RCU's viewpoint, update rdp->dynticks and
@@ -795,10 +786,10 @@ void rcu_user_exit(void)
  * long as the nesting level does not overflow an int.  (You will probably
  * run out of stack space first.)
  *
- * If you add or remove a call to rcu_nmi_enter_common(), be sure to test
+ * If you add or remove a call to rcu_nmi_enter(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-static __always_inline void rcu_nmi_enter_common(bool irq)
+static __always_inline void rcu_nmi_enter(void)
 {
 	long incby = 2;
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
@@ -816,12 +807,12 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
 	 */
 	if (rcu_dynticks_curr_cpu_in_eqs()) {
 
-		if (irq)
+		if (!in_nmi())
 			rcu_dynticks_task_exit();
 
 		rcu_dynticks_eqs_exit();
 
-		if (irq)
+		if (!in_nmi())
 			rcu_cleanup_after_idle();
 
 		incby = 1;
@@ -844,14 +835,6 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
 		   rdp->dynticks_nmi_nesting + incby);
 	barrier();
 }
-
-/**
- * rcu_nmi_enter - inform RCU of entry to NMI context
- */
-void rcu_nmi_enter(void)
-{
-	rcu_nmi_enter_common(false);
-}
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**
@@ -879,7 +862,7 @@ NOKPROBE_SYMBOL(rcu_nmi_enter);
 void rcu_irq_enter(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_nmi_enter_common(true);
+	rcu_nmi_enter();
 }
 
 /*

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Joel Fernandes]

On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
[...] 
> > > It might well be that I could make these functions be NMI-safe, but
> > > rcu_prepare_for_idle() in particular would be a bit ugly at best.
> > > So, before looking into that, I have a question.  Given these proposed
> > > changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> > > to just use in_nmi()?
> > 
> > That _should_ already be the case today. That is, if we end up in a
> > tracer and in_nmi() is unreliable we're already screwed anyway.
> 
> So something like this, then?  This is untested, probably doesn't even
> build, and could use some careful review from both Peter and Steve,
> at least.  As in the below is the second version of the patch, the first
> having been missing a couple of important "!" characters.

I removed the static from rcu_nmi_enter()/exit() as it is called from
outside, that makes it build now. Updated below is Paul's diff. I also added
NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
asymmetric.

---8<-----------------------

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index d91c9156fab2e..bbcc7767f18ee 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -614,16 +614,18 @@ void rcu_user_enter(void)
 }
 #endif /* CONFIG_NO_HZ_FULL */
 
-/*
+/**
+ * rcu_nmi_exit - inform RCU of exit from NMI context
+ *
  * If we are returning from the outermost NMI handler that interrupted an
  * RCU-idle period, update rdp->dynticks and rdp->dynticks_nmi_nesting
  * to let the RCU grace-period handling know that the CPU is back to
  * being RCU-idle.
  *
- * If you add or remove a call to rcu_nmi_exit_common(), be sure to test
+ * If you add or remove a call to rcu_nmi_exit(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-static __always_inline void rcu_nmi_exit_common(bool irq)
+__always_inline void rcu_nmi_exit(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 
@@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
 	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
 	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
 
-	if (irq)
+	if (!in_nmi())
 		rcu_prepare_for_idle();
 
 	rcu_dynticks_eqs_enter();
 
-	if (irq)
+	if (!in_nmi())
 		rcu_dynticks_task_enter();
 }
-
-/**
- * rcu_nmi_exit - inform RCU of exit from NMI context
- *
- * If you add or remove a call to rcu_nmi_exit(), be sure to test
- * with CONFIG_RCU_EQS_DEBUG=y.
- */
-void rcu_nmi_exit(void)
-{
-	rcu_nmi_exit_common(false);
-}
+NOKPROBE_SYMBOL(rcu_nmi_exit);
 
 /**
  * rcu_irq_exit - inform RCU that current CPU is exiting irq towards idle
@@ -693,7 +685,7 @@ void rcu_nmi_exit(void)
 void rcu_irq_exit(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_nmi_exit_common(true);
+	rcu_nmi_exit();
 }
 
 /*
@@ -777,7 +769,7 @@ void rcu_user_exit(void)
 #endif /* CONFIG_NO_HZ_FULL */
 
 /**
- * rcu_nmi_enter_common - inform RCU of entry to NMI context
+ * rcu_nmi_enter - inform RCU of entry to NMI context
  * @irq: Is this call from rcu_irq_enter?
  *
  * If the CPU was idle from RCU's viewpoint, update rdp->dynticks and
@@ -786,10 +778,10 @@ void rcu_user_exit(void)
  * long as the nesting level does not overflow an int.  (You will probably
  * run out of stack space first.)
  *
- * If you add or remove a call to rcu_nmi_enter_common(), be sure to test
+ * If you add or remove a call to rcu_nmi_enter(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-static __always_inline void rcu_nmi_enter_common(bool irq)
+__always_inline void rcu_nmi_enter(void)
 {
 	long incby = 2;
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
@@ -807,12 +799,12 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
 	 */
 	if (rcu_dynticks_curr_cpu_in_eqs()) {
 
-		if (irq)
+		if (!in_nmi())
 			rcu_dynticks_task_exit();
 
 		rcu_dynticks_eqs_exit();
 
-		if (irq)
+		if (!in_nmi())
 			rcu_cleanup_after_idle();
 
 		incby = 1;
@@ -834,14 +826,6 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
 		   rdp->dynticks_nmi_nesting + incby);
 	barrier();
 }
-
-/**
- * rcu_nmi_enter - inform RCU of entry to NMI context
- */
-void rcu_nmi_enter(void)
-{
-	rcu_nmi_enter_common(false);
-}
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**
@@ -869,7 +853,7 @@ NOKPROBE_SYMBOL(rcu_nmi_enter);
 void rcu_irq_enter(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_nmi_enter_common(true);
+	rcu_nmi_enter();
 }
 
 /*

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Paul E. McKenney]

On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:
> On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
> [...] 
> > > > It might well be that I could make these functions be NMI-safe, but
> > > > rcu_prepare_for_idle() in particular would be a bit ugly at best.
> > > > So, before looking into that, I have a question.  Given these proposed
> > > > changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> > > > to just use in_nmi()?
> > > 
> > > That _should_ already be the case today. That is, if we end up in a
> > > tracer and in_nmi() is unreliable we're already screwed anyway.
> > 
> > So something like this, then?  This is untested, probably doesn't even
> > build, and could use some careful review from both Peter and Steve,
> > at least.  As in the below is the second version of the patch, the first
> > having been missing a couple of important "!" characters.
> 
> I removed the static from rcu_nmi_enter()/exit() as it is called from
> outside, that makes it build now. Updated below is Paul's diff. I also added
> NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> asymmetric.

My compiler complained about the static and the __always_inline, so I
fixed those.  But please help me out on adding the NOKPROBE_SYMBOL()
to rcu_nmi_exit().  What bad thing happens if we leave this on only
rcu_nmi_enter()?

							Thanx, Paul

> ---8<-----------------------
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index d91c9156fab2e..bbcc7767f18ee 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -614,16 +614,18 @@ void rcu_user_enter(void)
>  }
>  #endif /* CONFIG_NO_HZ_FULL */
>  
> -/*
> +/**
> + * rcu_nmi_exit - inform RCU of exit from NMI context
> + *
>   * If we are returning from the outermost NMI handler that interrupted an
>   * RCU-idle period, update rdp->dynticks and rdp->dynticks_nmi_nesting
>   * to let the RCU grace-period handling know that the CPU is back to
>   * being RCU-idle.
>   *
> - * If you add or remove a call to rcu_nmi_exit_common(), be sure to test
> + * If you add or remove a call to rcu_nmi_exit(), be sure to test
>   * with CONFIG_RCU_EQS_DEBUG=y.
>   */
> -static __always_inline void rcu_nmi_exit_common(bool irq)
> +__always_inline void rcu_nmi_exit(void)
>  {
>  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>  
> @@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
>  	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
>  	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
>  
> -	if (irq)
> +	if (!in_nmi())
>  		rcu_prepare_for_idle();
>  
>  	rcu_dynticks_eqs_enter();
>  
> -	if (irq)
> +	if (!in_nmi())
>  		rcu_dynticks_task_enter();
>  }
> -
> -/**
> - * rcu_nmi_exit - inform RCU of exit from NMI context
> - *
> - * If you add or remove a call to rcu_nmi_exit(), be sure to test
> - * with CONFIG_RCU_EQS_DEBUG=y.
> - */
> -void rcu_nmi_exit(void)
> -{
> -	rcu_nmi_exit_common(false);
> -}
> +NOKPROBE_SYMBOL(rcu_nmi_exit);
>  
>  /**
>   * rcu_irq_exit - inform RCU that current CPU is exiting irq towards idle
> @@ -693,7 +685,7 @@ void rcu_nmi_exit(void)
>  void rcu_irq_exit(void)
>  {
>  	lockdep_assert_irqs_disabled();
> -	rcu_nmi_exit_common(true);
> +	rcu_nmi_exit();
>  }
>  
>  /*
> @@ -777,7 +769,7 @@ void rcu_user_exit(void)
>  #endif /* CONFIG_NO_HZ_FULL */
>  
>  /**
> - * rcu_nmi_enter_common - inform RCU of entry to NMI context
> + * rcu_nmi_enter - inform RCU of entry to NMI context
>   * @irq: Is this call from rcu_irq_enter?
>   *
>   * If the CPU was idle from RCU's viewpoint, update rdp->dynticks and
> @@ -786,10 +778,10 @@ void rcu_user_exit(void)
>   * long as the nesting level does not overflow an int.  (You will probably
>   * run out of stack space first.)
>   *
> - * If you add or remove a call to rcu_nmi_enter_common(), be sure to test
> + * If you add or remove a call to rcu_nmi_enter(), be sure to test
>   * with CONFIG_RCU_EQS_DEBUG=y.
>   */
> -static __always_inline void rcu_nmi_enter_common(bool irq)
> +__always_inline void rcu_nmi_enter(void)
>  {
>  	long incby = 2;
>  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> @@ -807,12 +799,12 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
>  	 */
>  	if (rcu_dynticks_curr_cpu_in_eqs()) {
>  
> -		if (irq)
> +		if (!in_nmi())
>  			rcu_dynticks_task_exit();
>  
>  		rcu_dynticks_eqs_exit();
>  
> -		if (irq)
> +		if (!in_nmi())
>  			rcu_cleanup_after_idle();
>  
>  		incby = 1;
> @@ -834,14 +826,6 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
>  		   rdp->dynticks_nmi_nesting + incby);
>  	barrier();
>  }
> -
> -/**
> - * rcu_nmi_enter - inform RCU of entry to NMI context
> - */
> -void rcu_nmi_enter(void)
> -{
> -	rcu_nmi_enter_common(false);
> -}
>  NOKPROBE_SYMBOL(rcu_nmi_enter);
>  
>  /**
> @@ -869,7 +853,7 @@ NOKPROBE_SYMBOL(rcu_nmi_enter);
>  void rcu_irq_enter(void)
>  {
>  	lockdep_assert_irqs_disabled();
> -	rcu_nmi_enter_common(true);
> +	rcu_nmi_enter();
>  }
>  
>  /*

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Joel Fernandes]

On Thu, Feb 13, 2020 at 12:54:42PM -0800, Paul E. McKenney wrote:
> On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:
> > On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
> > [...] 
> > > > > It might well be that I could make these functions be NMI-safe, but
> > > > > rcu_prepare_for_idle() in particular would be a bit ugly at best.
> > > > > So, before looking into that, I have a question.  Given these proposed
> > > > > changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> > > > > to just use in_nmi()?
> > > > 
> > > > That _should_ already be the case today. That is, if we end up in a
> > > > tracer and in_nmi() is unreliable we're already screwed anyway.
> > > 
> > > So something like this, then?  This is untested, probably doesn't even
> > > build, and could use some careful review from both Peter and Steve,
> > > at least.  As in the below is the second version of the patch, the first
> > > having been missing a couple of important "!" characters.
> > 
> > I removed the static from rcu_nmi_enter()/exit() as it is called from
> > outside, that makes it build now. Updated below is Paul's diff. I also added
> > NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> > asymmetric.
> 
> My compiler complained about the static and the __always_inline, so I
> fixed those.  But please help me out on adding the NOKPROBE_SYMBOL()
> to rcu_nmi_exit().  What bad thing happens if we leave this on only
> rcu_nmi_enter()?

It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but
allowing it on exit (from a code reading standpoint) so my reaction was to
add it to both, but we could probably keep that as a separate
patch/discussion since it is slightly unrelated to the patch.. Sorry to
confuse the topic.

thanks,

 - Joel


> 							Thanx, Paul
> 
> > ---8<-----------------------
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index d91c9156fab2e..bbcc7767f18ee 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -614,16 +614,18 @@ void rcu_user_enter(void)
> >  }
> >  #endif /* CONFIG_NO_HZ_FULL */
> >  
> > -/*
> > +/**
> > + * rcu_nmi_exit - inform RCU of exit from NMI context
> > + *
> >   * If we are returning from the outermost NMI handler that interrupted an
> >   * RCU-idle period, update rdp->dynticks and rdp->dynticks_nmi_nesting
> >   * to let the RCU grace-period handling know that the CPU is back to
> >   * being RCU-idle.
> >   *
> > - * If you add or remove a call to rcu_nmi_exit_common(), be sure to test
> > + * If you add or remove a call to rcu_nmi_exit(), be sure to test
> >   * with CONFIG_RCU_EQS_DEBUG=y.
> >   */
> > -static __always_inline void rcu_nmi_exit_common(bool irq)
> > +__always_inline void rcu_nmi_exit(void)
> >  {
> >  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> >  
> > @@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
> >  	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
> >  	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
> >  
> > -	if (irq)
> > +	if (!in_nmi())
> >  		rcu_prepare_for_idle();
> >  
> >  	rcu_dynticks_eqs_enter();
> >  
> > -	if (irq)
> > +	if (!in_nmi())
> >  		rcu_dynticks_task_enter();
> >  }
> > -
> > -/**
> > - * rcu_nmi_exit - inform RCU of exit from NMI context
> > - *
> > - * If you add or remove a call to rcu_nmi_exit(), be sure to test
> > - * with CONFIG_RCU_EQS_DEBUG=y.
> > - */
> > -void rcu_nmi_exit(void)
> > -{
> > -	rcu_nmi_exit_common(false);
> > -}
> > +NOKPROBE_SYMBOL(rcu_nmi_exit);
> >  
> >  /**
> >   * rcu_irq_exit - inform RCU that current CPU is exiting irq towards idle
> > @@ -693,7 +685,7 @@ void rcu_nmi_exit(void)
> >  void rcu_irq_exit(void)
> >  {
> >  	lockdep_assert_irqs_disabled();
> > -	rcu_nmi_exit_common(true);
> > +	rcu_nmi_exit();
> >  }
> >  
> >  /*
> > @@ -777,7 +769,7 @@ void rcu_user_exit(void)
> >  #endif /* CONFIG_NO_HZ_FULL */
> >  
> >  /**
> > - * rcu_nmi_enter_common - inform RCU of entry to NMI context
> > + * rcu_nmi_enter - inform RCU of entry to NMI context
> >   * @irq: Is this call from rcu_irq_enter?
> >   *
> >   * If the CPU was idle from RCU's viewpoint, update rdp->dynticks and
> > @@ -786,10 +778,10 @@ void rcu_user_exit(void)
> >   * long as the nesting level does not overflow an int.  (You will probably
> >   * run out of stack space first.)
> >   *
> > - * If you add or remove a call to rcu_nmi_enter_common(), be sure to test
> > + * If you add or remove a call to rcu_nmi_enter(), be sure to test
> >   * with CONFIG_RCU_EQS_DEBUG=y.
> >   */
> > -static __always_inline void rcu_nmi_enter_common(bool irq)
> > +__always_inline void rcu_nmi_enter(void)
> >  {
> >  	long incby = 2;
> >  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > @@ -807,12 +799,12 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
> >  	 */
> >  	if (rcu_dynticks_curr_cpu_in_eqs()) {
> >  
> > -		if (irq)
> > +		if (!in_nmi())
> >  			rcu_dynticks_task_exit();
> >  
> >  		rcu_dynticks_eqs_exit();
> >  
> > -		if (irq)
> > +		if (!in_nmi())
> >  			rcu_cleanup_after_idle();
> >  
> >  		incby = 1;
> > @@ -834,14 +826,6 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
> >  		   rdp->dynticks_nmi_nesting + incby);
> >  	barrier();
> >  }
> > -
> > -/**
> > - * rcu_nmi_enter - inform RCU of entry to NMI context
> > - */
> > -void rcu_nmi_enter(void)
> > -{
> > -	rcu_nmi_enter_common(false);
> > -}
> >  NOKPROBE_SYMBOL(rcu_nmi_enter);
> >  
> >  /**
> > @@ -869,7 +853,7 @@ NOKPROBE_SYMBOL(rcu_nmi_enter);
> >  void rcu_irq_enter(void)
> >  {
> >  	lockdep_assert_irqs_disabled();
> > -	rcu_nmi_enter_common(true);
> > +	rcu_nmi_enter();
> >  }
> >  
> >  /*

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Steven Rostedt]

[ Added Masami ]

On Thu, 13 Feb 2020 16:19:30 -0500
Joel Fernandes <joel@joelfernandes.org> wrote:

> On Thu, Feb 13, 2020 at 12:54:42PM -0800, Paul E. McKenney wrote:
> > On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:  
> > > On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
> > > [...]   
> > > > > > It might well be that I could make these functions be NMI-safe, but
> > > > > > rcu_prepare_for_idle() in particular would be a bit ugly at best.
> > > > > > So, before looking into that, I have a question.  Given these proposed
> > > > > > changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> > > > > > to just use in_nmi()?  
> > > > > 
> > > > > That _should_ already be the case today. That is, if we end up in a
> > > > > tracer and in_nmi() is unreliable we're already screwed anyway.  
> > > > 
> > > > So something like this, then?  This is untested, probably doesn't even
> > > > build, and could use some careful review from both Peter and Steve,
> > > > at least.  As in the below is the second version of the patch, the first
> > > > having been missing a couple of important "!" characters.  
> > > 
> > > I removed the static from rcu_nmi_enter()/exit() as it is called from
> > > outside, that makes it build now. Updated below is Paul's diff. I also added
> > > NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> > > asymmetric.  
> > 
> > My compiler complained about the static and the __always_inline, so I
> > fixed those.  But please help me out on adding the NOKPROBE_SYMBOL()
> > to rcu_nmi_exit().  What bad thing happens if we leave this on only
> > rcu_nmi_enter()?  
> 
> It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but
> allowing it on exit (from a code reading standpoint) so my reaction was to
> add it to both, but we could probably keep that as a separate
> patch/discussion since it is slightly unrelated to the patch.. Sorry to
> confuse the topic.
>

rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit
c13324a505c77 ("x86/kprobes: Prohibit probing on functions before
kprobe_int3_handler()")

The issue was that we must not allow anything in do_int3() call kprobe
code before kprobe_int3_handler() is called. Because ist_enter() (in
do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had
nothing to do with it being RCU nor NMI, but because it was simply
called in do_int3().

Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont
to why rcu_nmi_enter() would probably be useful, like below:

-- Steve

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 1694a6b57ad8..e2c9e3e2f480 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -846,6 +846,12 @@ void rcu_nmi_enter(void)
 {
 	rcu_nmi_enter_common(false);
 }
+/*
+ * On x86, All functions in do_int3() must be marked NOKPROBE before
+ * kprobe_int3_handler() is called. ist_enter() which is called in do_int3()
+ * before kprobe_int3_handle() happens to call rcu_nmi_enter() in which case
+ * rcu_nmi_enter() must be marked NOKRPOBE.
+ */
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Paul E. McKenney]

On Thu, Feb 13, 2020 at 04:38:25PM -0500, Steven Rostedt wrote:
> [ Added Masami ]
> 
> On Thu, 13 Feb 2020 16:19:30 -0500
> Joel Fernandes <joel@joelfernandes.org> wrote:
> 
> > On Thu, Feb 13, 2020 at 12:54:42PM -0800, Paul E. McKenney wrote:
> > > On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:  
> > > > On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
> > > > [...]   
> > > > > > > It might well be that I could make these functions be NMI-safe, but
> > > > > > > rcu_prepare_for_idle() in particular would be a bit ugly at best.
> > > > > > > So, before looking into that, I have a question.  Given these proposed
> > > > > > > changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> > > > > > > to just use in_nmi()?  
> > > > > > 
> > > > > > That _should_ already be the case today. That is, if we end up in a
> > > > > > tracer and in_nmi() is unreliable we're already screwed anyway.  
> > > > > 
> > > > > So something like this, then?  This is untested, probably doesn't even
> > > > > build, and could use some careful review from both Peter and Steve,
> > > > > at least.  As in the below is the second version of the patch, the first
> > > > > having been missing a couple of important "!" characters.  
> > > > 
> > > > I removed the static from rcu_nmi_enter()/exit() as it is called from
> > > > outside, that makes it build now. Updated below is Paul's diff. I also added
> > > > NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> > > > asymmetric.  
> > > 
> > > My compiler complained about the static and the __always_inline, so I
> > > fixed those.  But please help me out on adding the NOKPROBE_SYMBOL()
> > > to rcu_nmi_exit().  What bad thing happens if we leave this on only
> > > rcu_nmi_enter()?  
> > 
> > It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but
> > allowing it on exit (from a code reading standpoint) so my reaction was to
> > add it to both, but we could probably keep that as a separate
> > patch/discussion since it is slightly unrelated to the patch.. Sorry to
> > confuse the topic.
> >
> 
> rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit
> c13324a505c77 ("x86/kprobes: Prohibit probing on functions before
> kprobe_int3_handler()")
> 
> The issue was that we must not allow anything in do_int3() call kprobe
> code before kprobe_int3_handler() is called. Because ist_enter() (in
> do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had
> nothing to do with it being RCU nor NMI, but because it was simply
> called in do_int3().
> 
> Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont
> to why rcu_nmi_enter() would probably be useful, like below:

Thank you, Steve!  Could I please have your Signed-off-by for this?

							Thanx, Paul

> -- Steve
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 1694a6b57ad8..e2c9e3e2f480 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -846,6 +846,12 @@ void rcu_nmi_enter(void)
>  {
>  	rcu_nmi_enter_common(false);
>  }
> +/*
> + * On x86, All functions in do_int3() must be marked NOKPROBE before
> + * kprobe_int3_handler() is called. ist_enter() which is called in do_int3()
> + * before kprobe_int3_handle() happens to call rcu_nmi_enter() in which case
> + * rcu_nmi_enter() must be marked NOKRPOBE.
> + */
>  NOKPROBE_SYMBOL(rcu_nmi_enter);
>  
>  /**

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Steven Rostedt]

On Thu, 13 Feb 2020 13:50:04 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> On Thu, Feb 13, 2020 at 04:38:25PM -0500, Steven Rostedt wrote:
> > [ Added Masami ]
> > 
> > On Thu, 13 Feb 2020 16:19:30 -0500
> > Joel Fernandes <joel@joelfernandes.org> wrote:
> >   
> > > On Thu, Feb 13, 2020 at 12:54:42PM -0800, Paul E. McKenney wrote:  
> > > > On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:    
> > > > > On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
> > > > > [...]     
> > > > > > > > It might well be that I could make these functions be NMI-safe, but
> > > > > > > > rcu_prepare_for_idle() in particular would be a bit ugly at best.
> > > > > > > > So, before looking into that, I have a question.  Given these proposed
> > > > > > > > changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> > > > > > > > to just use in_nmi()?    
> > > > > > > 
> > > > > > > That _should_ already be the case today. That is, if we end up in a
> > > > > > > tracer and in_nmi() is unreliable we're already screwed anyway.    
> > > > > > 
> > > > > > So something like this, then?  This is untested, probably doesn't even
> > > > > > build, and could use some careful review from both Peter and Steve,
> > > > > > at least.  As in the below is the second version of the patch, the first
> > > > > > having been missing a couple of important "!" characters.    
> > > > > 
> > > > > I removed the static from rcu_nmi_enter()/exit() as it is called from
> > > > > outside, that makes it build now. Updated below is Paul's diff. I also added
> > > > > NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> > > > > asymmetric.    
> > > > 
> > > > My compiler complained about the static and the __always_inline, so I
> > > > fixed those.  But please help me out on adding the NOKPROBE_SYMBOL()
> > > > to rcu_nmi_exit().  What bad thing happens if we leave this on only
> > > > rcu_nmi_enter()?    
> > > 
> > > It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but
> > > allowing it on exit (from a code reading standpoint) so my reaction was to
> > > add it to both, but we could probably keep that as a separate
> > > patch/discussion since it is slightly unrelated to the patch.. Sorry to
> > > confuse the topic.
> > >  
> > 
> > rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit
> > c13324a505c77 ("x86/kprobes: Prohibit probing on functions before
> > kprobe_int3_handler()")
> > 
> > The issue was that we must not allow anything in do_int3() call kprobe
> > code before kprobe_int3_handler() is called. Because ist_enter() (in
> > do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had
> > nothing to do with it being RCU nor NMI, but because it was simply
> > called in do_int3().
> > 
> > Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont
> > to why rcu_nmi_enter() would probably be useful, like below:  
> 
> Thank you, Steve!  Could I please have your Signed-off-by for this?

Sure, but it was untested ;-)

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

I'd like a Reviewed-by from Masami though.

-- Steve

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Paul E. McKenney]

On Thu, Feb 13, 2020 at 05:04:51PM -0500, Steven Rostedt wrote:
> On Thu, 13 Feb 2020 13:50:04 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > On Thu, Feb 13, 2020 at 04:38:25PM -0500, Steven Rostedt wrote:
> > > [ Added Masami ]
> > > 
> > > On Thu, 13 Feb 2020 16:19:30 -0500
> > > Joel Fernandes <joel@joelfernandes.org> wrote:
> > >   
> > > > On Thu, Feb 13, 2020 at 12:54:42PM -0800, Paul E. McKenney wrote:  
> > > > > On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:    
> > > > > > On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
> > > > > > [...]     
> > > > > > > > > It might well be that I could make these functions be NMI-safe, but
> > > > > > > > > rcu_prepare_for_idle() in particular would be a bit ugly at best.
> > > > > > > > > So, before looking into that, I have a question.  Given these proposed
> > > > > > > > > changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> > > > > > > > > to just use in_nmi()?    
> > > > > > > > 
> > > > > > > > That _should_ already be the case today. That is, if we end up in a
> > > > > > > > tracer and in_nmi() is unreliable we're already screwed anyway.    
> > > > > > > 
> > > > > > > So something like this, then?  This is untested, probably doesn't even
> > > > > > > build, and could use some careful review from both Peter and Steve,
> > > > > > > at least.  As in the below is the second version of the patch, the first
> > > > > > > having been missing a couple of important "!" characters.    
> > > > > > 
> > > > > > I removed the static from rcu_nmi_enter()/exit() as it is called from
> > > > > > outside, that makes it build now. Updated below is Paul's diff. I also added
> > > > > > NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> > > > > > asymmetric.    
> > > > > 
> > > > > My compiler complained about the static and the __always_inline, so I
> > > > > fixed those.  But please help me out on adding the NOKPROBE_SYMBOL()
> > > > > to rcu_nmi_exit().  What bad thing happens if we leave this on only
> > > > > rcu_nmi_enter()?    
> > > > 
> > > > It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but
> > > > allowing it on exit (from a code reading standpoint) so my reaction was to
> > > > add it to both, but we could probably keep that as a separate
> > > > patch/discussion since it is slightly unrelated to the patch.. Sorry to
> > > > confuse the topic.
> > > >  
> > > 
> > > rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit
> > > c13324a505c77 ("x86/kprobes: Prohibit probing on functions before
> > > kprobe_int3_handler()")
> > > 
> > > The issue was that we must not allow anything in do_int3() call kprobe
> > > code before kprobe_int3_handler() is called. Because ist_enter() (in
> > > do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had
> > > nothing to do with it being RCU nor NMI, but because it was simply
> > > called in do_int3().
> > > 
> > > Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont
> > > to why rcu_nmi_enter() would probably be useful, like below:  
> > 
> > Thank you, Steve!  Could I please have your Signed-off-by for this?
> 
> Sure, but it was untested ;-)

No problem!  I will fire up rcutorture on it.  ;-)

But experience indicates that you cannot even make a joke around here.
There is probably already someone out there somewhere building a
comment-checker based on deep semantic analysis and machine learning.  :-/

> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> I'd like a Reviewed-by from Masami though.

Sounds good!  Masami, would you be willing to review?

							Thanx, Paul

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Masami Hiramatsu]

On Thu, 13 Feb 2020 14:39:18 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> On Thu, Feb 13, 2020 at 05:04:51PM -0500, Steven Rostedt wrote:
> > On Thu, 13 Feb 2020 13:50:04 -0800
> > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > 
> > > On Thu, Feb 13, 2020 at 04:38:25PM -0500, Steven Rostedt wrote:
> > > > [ Added Masami ]
> > > > 
> > > > On Thu, 13 Feb 2020 16:19:30 -0500
> > > > Joel Fernandes <joel@joelfernandes.org> wrote:
> > > >   
> > > > > On Thu, Feb 13, 2020 at 12:54:42PM -0800, Paul E. McKenney wrote:  
> > > > > > On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:    
> > > > > > > On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
> > > > > > > [...]     
> > > > > > > > > > It might well be that I could make these functions be NMI-safe, but
> > > > > > > > > > rcu_prepare_for_idle() in particular would be a bit ugly at best.
> > > > > > > > > > So, before looking into that, I have a question.  Given these proposed
> > > > > > > > > > changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> > > > > > > > > > to just use in_nmi()?    
> > > > > > > > > 
> > > > > > > > > That _should_ already be the case today. That is, if we end up in a
> > > > > > > > > tracer and in_nmi() is unreliable we're already screwed anyway.    
> > > > > > > > 
> > > > > > > > So something like this, then?  This is untested, probably doesn't even
> > > > > > > > build, and could use some careful review from both Peter and Steve,
> > > > > > > > at least.  As in the below is the second version of the patch, the first
> > > > > > > > having been missing a couple of important "!" characters.    
> > > > > > > 
> > > > > > > I removed the static from rcu_nmi_enter()/exit() as it is called from
> > > > > > > outside, that makes it build now. Updated below is Paul's diff. I also added
> > > > > > > NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> > > > > > > asymmetric.    
> > > > > > 
> > > > > > My compiler complained about the static and the __always_inline, so I
> > > > > > fixed those.  But please help me out on adding the NOKPROBE_SYMBOL()
> > > > > > to rcu_nmi_exit().  What bad thing happens if we leave this on only
> > > > > > rcu_nmi_enter()?    
> > > > > 
> > > > > It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but
> > > > > allowing it on exit (from a code reading standpoint) so my reaction was to
> > > > > add it to both, but we could probably keep that as a separate
> > > > > patch/discussion since it is slightly unrelated to the patch.. Sorry to
> > > > > confuse the topic.
> > > > >  
> > > > 
> > > > rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit
> > > > c13324a505c77 ("x86/kprobes: Prohibit probing on functions before
> > > > kprobe_int3_handler()")
> > > > 
> > > > The issue was that we must not allow anything in do_int3() call kprobe
> > > > code before kprobe_int3_handler() is called. Because ist_enter() (in
> > > > do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had
> > > > nothing to do with it being RCU nor NMI, but because it was simply
> > > > called in do_int3().
> > > > 
> > > > Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont
> > > > to why rcu_nmi_enter() would probably be useful, like below:  
> > > 
> > > Thank you, Steve!  Could I please have your Signed-off-by for this?
> > 
> > Sure, but it was untested ;-)
> 
> No problem!  I will fire up rcutorture on it.  ;-)
> 
> But experience indicates that you cannot even make a joke around here.
> There is probably already someone out there somewhere building a
> comment-checker based on deep semantic analysis and machine learning.  :-/
> 
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > 
> > I'd like a Reviewed-by from Masami though.
> 
> Sounds good!  Masami, would you be willing to review?

Yes, the functions before calling kprobe_int3_handler() must not
be kprobed. It can cause an infinite recursive int3 trapping.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you!

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Paul E. McKenney]

On Fri, Feb 14, 2020 at 03:19:06PM +0900, Masami Hiramatsu wrote:
> On Thu, 13 Feb 2020 14:39:18 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > On Thu, Feb 13, 2020 at 05:04:51PM -0500, Steven Rostedt wrote:
> > > On Thu, 13 Feb 2020 13:50:04 -0800
> > > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > 
> > > > On Thu, Feb 13, 2020 at 04:38:25PM -0500, Steven Rostedt wrote:
> > > > > [ Added Masami ]
> > > > > 
> > > > > On Thu, 13 Feb 2020 16:19:30 -0500
> > > > > Joel Fernandes <joel@joelfernandes.org> wrote:
> > > > >   
> > > > > > On Thu, Feb 13, 2020 at 12:54:42PM -0800, Paul E. McKenney wrote:  
> > > > > > > On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:    
> > > > > > > > On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
> > > > > > > > [...]     
> > > > > > > > > > > It might well be that I could make these functions be NMI-safe, but
> > > > > > > > > > > rcu_prepare_for_idle() in particular would be a bit ugly at best.
> > > > > > > > > > > So, before looking into that, I have a question.  Given these proposed
> > > > > > > > > > > changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> > > > > > > > > > > to just use in_nmi()?    
> > > > > > > > > > 
> > > > > > > > > > That _should_ already be the case today. That is, if we end up in a
> > > > > > > > > > tracer and in_nmi() is unreliable we're already screwed anyway.    
> > > > > > > > > 
> > > > > > > > > So something like this, then?  This is untested, probably doesn't even
> > > > > > > > > build, and could use some careful review from both Peter and Steve,
> > > > > > > > > at least.  As in the below is the second version of the patch, the first
> > > > > > > > > having been missing a couple of important "!" characters.    
> > > > > > > > 
> > > > > > > > I removed the static from rcu_nmi_enter()/exit() as it is called from
> > > > > > > > outside, that makes it build now. Updated below is Paul's diff. I also added
> > > > > > > > NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> > > > > > > > asymmetric.    
> > > > > > > 
> > > > > > > My compiler complained about the static and the __always_inline, so I
> > > > > > > fixed those.  But please help me out on adding the NOKPROBE_SYMBOL()
> > > > > > > to rcu_nmi_exit().  What bad thing happens if we leave this on only
> > > > > > > rcu_nmi_enter()?    
> > > > > > 
> > > > > > It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but
> > > > > > allowing it on exit (from a code reading standpoint) so my reaction was to
> > > > > > add it to both, but we could probably keep that as a separate
> > > > > > patch/discussion since it is slightly unrelated to the patch.. Sorry to
> > > > > > confuse the topic.
> > > > > >  
> > > > > 
> > > > > rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit
> > > > > c13324a505c77 ("x86/kprobes: Prohibit probing on functions before
> > > > > kprobe_int3_handler()")
> > > > > 
> > > > > The issue was that we must not allow anything in do_int3() call kprobe
> > > > > code before kprobe_int3_handler() is called. Because ist_enter() (in
> > > > > do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had
> > > > > nothing to do with it being RCU nor NMI, but because it was simply
> > > > > called in do_int3().
> > > > > 
> > > > > Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont
> > > > > to why rcu_nmi_enter() would probably be useful, like below:  
> > > > 
> > > > Thank you, Steve!  Could I please have your Signed-off-by for this?
> > > 
> > > Sure, but it was untested ;-)
> > 
> > No problem!  I will fire up rcutorture on it.  ;-)
> > 
> > But experience indicates that you cannot even make a joke around here.
> > There is probably already someone out there somewhere building a
> > comment-checker based on deep semantic analysis and machine learning.  :-/
> > 
> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > 
> > > I'd like a Reviewed-by from Masami though.
> > 
> > Sounds good!  Masami, would you be willing to review?
> 
> Yes, the functions before calling kprobe_int3_handler() must not
> be kprobed. It can cause an infinite recursive int3 trapping.
> 
> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you both!

Like this?

							Thanx, Paul

------------------------------------------------------------------------

commit 1817fdc8f4e4bd18c76305c9b937fb0dccbb1583
Author: Steven Rostedt <rostedt@goodmis.org>
Date:   Sat Feb 15 06:54:50 2020 -0800

    rcu: Provide comment for NOKPROBE() on rcu_nmi_enter()
    
    The rcu_nmi_enter() function was marked NOKPROBE() by commit
    c13324a505c77 ("x86/kprobes: Prohibit probing on functions before
    kprobe_int3_handler()") because the do_int3() call kprobe code must
    not be invoked before kprobe_int3_handler() is called.  It turns out
    that ist_enter() (in do_int3()) calls rcu_nmi_enter(), hence the
    marking NOKPROBE() being added to rcu_nmi_enter().
    
    This commit therefore adds a comment documenting this line of reasoning.
    
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
    Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 132b53e..4a885af 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -835,6 +835,12 @@ void rcu_nmi_enter(void)
 		   rdp->dynticks_nmi_nesting + incby);
 	barrier();
 }
+/*
+ * On x86, All functions in do_int3() must be marked NOKPROBE before
+ * kprobe_int3_handler() is called. ist_enter() which is called in do_int3()
+ * before kprobe_int3_handle() happens to call rcu_nmi_enter() which means
+ * that rcu_nmi_enter() must be marked NOKRPOBE.
+ */
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Masami Hiramatsu]

On Sat, 15 Feb 2020 06:59:34 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> On Fri, Feb 14, 2020 at 03:19:06PM +0900, Masami Hiramatsu wrote:
> > On Thu, 13 Feb 2020 14:39:18 -0800
> > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > 
> > > On Thu, Feb 13, 2020 at 05:04:51PM -0500, Steven Rostedt wrote:
> > > > On Thu, 13 Feb 2020 13:50:04 -0800
> > > > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > > 
> > > > > On Thu, Feb 13, 2020 at 04:38:25PM -0500, Steven Rostedt wrote:
> > > > > > [ Added Masami ]
> > > > > > 
> > > > > > On Thu, 13 Feb 2020 16:19:30 -0500
> > > > > > Joel Fernandes <joel@joelfernandes.org> wrote:
> > > > > >   
> > > > > > > On Thu, Feb 13, 2020 at 12:54:42PM -0800, Paul E. McKenney wrote:  
> > > > > > > > On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:    
> > > > > > > > > On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
> > > > > > > > > [...]     
> > > > > > > > > > > > It might well be that I could make these functions be NMI-safe, but
> > > > > > > > > > > > rcu_prepare_for_idle() in particular would be a bit ugly at best.
> > > > > > > > > > > > So, before looking into that, I have a question.  Given these proposed
> > > > > > > > > > > > changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> > > > > > > > > > > > to just use in_nmi()?    
> > > > > > > > > > > 
> > > > > > > > > > > That _should_ already be the case today. That is, if we end up in a
> > > > > > > > > > > tracer and in_nmi() is unreliable we're already screwed anyway.    
> > > > > > > > > > 
> > > > > > > > > > So something like this, then?  This is untested, probably doesn't even
> > > > > > > > > > build, and could use some careful review from both Peter and Steve,
> > > > > > > > > > at least.  As in the below is the second version of the patch, the first
> > > > > > > > > > having been missing a couple of important "!" characters.    
> > > > > > > > > 
> > > > > > > > > I removed the static from rcu_nmi_enter()/exit() as it is called from
> > > > > > > > > outside, that makes it build now. Updated below is Paul's diff. I also added
> > > > > > > > > NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> > > > > > > > > asymmetric.    
> > > > > > > > 
> > > > > > > > My compiler complained about the static and the __always_inline, so I
> > > > > > > > fixed those.  But please help me out on adding the NOKPROBE_SYMBOL()
> > > > > > > > to rcu_nmi_exit().  What bad thing happens if we leave this on only
> > > > > > > > rcu_nmi_enter()?    
> > > > > > > 
> > > > > > > It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but
> > > > > > > allowing it on exit (from a code reading standpoint) so my reaction was to
> > > > > > > add it to both, but we could probably keep that as a separate
> > > > > > > patch/discussion since it is slightly unrelated to the patch.. Sorry to
> > > > > > > confuse the topic.
> > > > > > >  
> > > > > > 
> > > > > > rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit
> > > > > > c13324a505c77 ("x86/kprobes: Prohibit probing on functions before
> > > > > > kprobe_int3_handler()")
> > > > > > 
> > > > > > The issue was that we must not allow anything in do_int3() call kprobe
> > > > > > code before kprobe_int3_handler() is called. Because ist_enter() (in
> > > > > > do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had
> > > > > > nothing to do with it being RCU nor NMI, but because it was simply
> > > > > > called in do_int3().
> > > > > > 
> > > > > > Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont
> > > > > > to why rcu_nmi_enter() would probably be useful, like below:  
> > > > > 
> > > > > Thank you, Steve!  Could I please have your Signed-off-by for this?
> > > > 
> > > > Sure, but it was untested ;-)
> > > 
> > > No problem!  I will fire up rcutorture on it.  ;-)
> > > 
> > > But experience indicates that you cannot even make a joke around here.
> > > There is probably already someone out there somewhere building a
> > > comment-checker based on deep semantic analysis and machine learning.  :-/
> > > 
> > > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > > 
> > > > I'd like a Reviewed-by from Masami though.
> > > 
> > > Sounds good!  Masami, would you be willing to review?
> > 
> > Yes, the functions before calling kprobe_int3_handler() must not
> > be kprobed. It can cause an infinite recursive int3 trapping.
> > 
> > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> 
> Thank you both!
> 
> Like this?
> 
> 							Thanx, Paul
> 

This is good to me.
BTW, if you consider the x86 specific code is in the generic file,
we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c.
(Sorry, I've hit this idea right now)

Thank you,

> ------------------------------------------------------------------------
> 
> commit 1817fdc8f4e4bd18c76305c9b937fb0dccbb1583
> Author: Steven Rostedt <rostedt@goodmis.org>
> Date:   Sat Feb 15 06:54:50 2020 -0800
> 
>     rcu: Provide comment for NOKPROBE() on rcu_nmi_enter()
>     
>     The rcu_nmi_enter() function was marked NOKPROBE() by commit
>     c13324a505c77 ("x86/kprobes: Prohibit probing on functions before
>     kprobe_int3_handler()") because the do_int3() call kprobe code must
>     not be invoked before kprobe_int3_handler() is called.  It turns out
>     that ist_enter() (in do_int3()) calls rcu_nmi_enter(), hence the
>     marking NOKPROBE() being added to rcu_nmi_enter().
>     
>     This commit therefore adds a comment documenting this line of reasoning.
>     
>     Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
>     Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
>     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 132b53e..4a885af 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -835,6 +835,12 @@ void rcu_nmi_enter(void)
>  		   rdp->dynticks_nmi_nesting + incby);
>  	barrier();
>  }
> +/*
> + * On x86, All functions in do_int3() must be marked NOKPROBE before
> + * kprobe_int3_handler() is called. ist_enter() which is called in do_int3()
> + * before kprobe_int3_handle() happens to call rcu_nmi_enter() which means
> + * that rcu_nmi_enter() must be marked NOKRPOBE.
> + */
>  NOKPROBE_SYMBOL(rcu_nmi_enter);
>  
>  /**


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Paul E. McKenney]

On Mon, Feb 17, 2020 at 05:55:19PM +0900, Masami Hiramatsu wrote:
> On Sat, 15 Feb 2020 06:59:34 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > On Fri, Feb 14, 2020 at 03:19:06PM +0900, Masami Hiramatsu wrote:
> > > On Thu, 13 Feb 2020 14:39:18 -0800
> > > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > 
> > > > On Thu, Feb 13, 2020 at 05:04:51PM -0500, Steven Rostedt wrote:
> > > > > On Thu, 13 Feb 2020 13:50:04 -0800
> > > > > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > > > 
> > > > > > On Thu, Feb 13, 2020 at 04:38:25PM -0500, Steven Rostedt wrote:
> > > > > > > [ Added Masami ]
> > > > > > > 
> > > > > > > On Thu, 13 Feb 2020 16:19:30 -0500
> > > > > > > Joel Fernandes <joel@joelfernandes.org> wrote:
> > > > > > >   
> > > > > > > > On Thu, Feb 13, 2020 at 12:54:42PM -0800, Paul E. McKenney wrote:  
> > > > > > > > > On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:    
> > > > > > > > > > On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
> > > > > > > > > > [...]     
> > > > > > > > > > > > > It might well be that I could make these functions be NMI-safe, but
> > > > > > > > > > > > > rcu_prepare_for_idle() in particular would be a bit ugly at best.
> > > > > > > > > > > > > So, before looking into that, I have a question.  Given these proposed
> > > > > > > > > > > > > changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> > > > > > > > > > > > > to just use in_nmi()?    
> > > > > > > > > > > > 
> > > > > > > > > > > > That _should_ already be the case today. That is, if we end up in a
> > > > > > > > > > > > tracer and in_nmi() is unreliable we're already screwed anyway.    
> > > > > > > > > > > 
> > > > > > > > > > > So something like this, then?  This is untested, probably doesn't even
> > > > > > > > > > > build, and could use some careful review from both Peter and Steve,
> > > > > > > > > > > at least.  As in the below is the second version of the patch, the first
> > > > > > > > > > > having been missing a couple of important "!" characters.    
> > > > > > > > > > 
> > > > > > > > > > I removed the static from rcu_nmi_enter()/exit() as it is called from
> > > > > > > > > > outside, that makes it build now. Updated below is Paul's diff. I also added
> > > > > > > > > > NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> > > > > > > > > > asymmetric.    
> > > > > > > > > 
> > > > > > > > > My compiler complained about the static and the __always_inline, so I
> > > > > > > > > fixed those.  But please help me out on adding the NOKPROBE_SYMBOL()
> > > > > > > > > to rcu_nmi_exit().  What bad thing happens if we leave this on only
> > > > > > > > > rcu_nmi_enter()?    
> > > > > > > > 
> > > > > > > > It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but
> > > > > > > > allowing it on exit (from a code reading standpoint) so my reaction was to
> > > > > > > > add it to both, but we could probably keep that as a separate
> > > > > > > > patch/discussion since it is slightly unrelated to the patch.. Sorry to
> > > > > > > > confuse the topic.
> > > > > > > >  
> > > > > > > 
> > > > > > > rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit
> > > > > > > c13324a505c77 ("x86/kprobes: Prohibit probing on functions before
> > > > > > > kprobe_int3_handler()")
> > > > > > > 
> > > > > > > The issue was that we must not allow anything in do_int3() call kprobe
> > > > > > > code before kprobe_int3_handler() is called. Because ist_enter() (in
> > > > > > > do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had
> > > > > > > nothing to do with it being RCU nor NMI, but because it was simply
> > > > > > > called in do_int3().
> > > > > > > 
> > > > > > > Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont
> > > > > > > to why rcu_nmi_enter() would probably be useful, like below:  
> > > > > > 
> > > > > > Thank you, Steve!  Could I please have your Signed-off-by for this?
> > > > > 
> > > > > Sure, but it was untested ;-)
> > > > 
> > > > No problem!  I will fire up rcutorture on it.  ;-)
> > > > 
> > > > But experience indicates that you cannot even make a joke around here.
> > > > There is probably already someone out there somewhere building a
> > > > comment-checker based on deep semantic analysis and machine learning.  :-/
> > > > 
> > > > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > > > 
> > > > > I'd like a Reviewed-by from Masami though.
> > > > 
> > > > Sounds good!  Masami, would you be willing to review?
> > > 
> > > Yes, the functions before calling kprobe_int3_handler() must not
> > > be kprobed. It can cause an infinite recursive int3 trapping.
> > > 
> > > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> > 
> > Thank you both!
> > 
> > Like this?
> > 
> > 							Thanx, Paul
> > 
> 
> This is good to me.

Thank you for looking it over!  (I already have your

> BTW, if you consider the x86 specific code is in the generic file,
> we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c.
> (Sorry, I've hit this idea right now)

Might this affect other architectures with NMIs and probe-like things?
If so, it might make sense to leave it where it is.

							Thanx, Paul

> Thank you,
> 
> > ------------------------------------------------------------------------
> > 
> > commit 1817fdc8f4e4bd18c76305c9b937fb0dccbb1583
> > Author: Steven Rostedt <rostedt@goodmis.org>
> > Date:   Sat Feb 15 06:54:50 2020 -0800
> > 
> >     rcu: Provide comment for NOKPROBE() on rcu_nmi_enter()
> >     
> >     The rcu_nmi_enter() function was marked NOKPROBE() by commit
> >     c13324a505c77 ("x86/kprobes: Prohibit probing on functions before
> >     kprobe_int3_handler()") because the do_int3() call kprobe code must
> >     not be invoked before kprobe_int3_handler() is called.  It turns out
> >     that ist_enter() (in do_int3()) calls rcu_nmi_enter(), hence the
> >     marking NOKPROBE() being added to rcu_nmi_enter().
> >     
> >     This commit therefore adds a comment documenting this line of reasoning.
> >     
> >     Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> >     Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 132b53e..4a885af 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -835,6 +835,12 @@ void rcu_nmi_enter(void)
> >  		   rdp->dynticks_nmi_nesting + incby);
> >  	barrier();
> >  }
> > +/*
> > + * On x86, All functions in do_int3() must be marked NOKPROBE before
> > + * kprobe_int3_handler() is called. ist_enter() which is called in do_int3()
> > + * before kprobe_int3_handle() happens to call rcu_nmi_enter() which means
> > + * that rcu_nmi_enter() must be marked NOKRPOBE.
> > + */
> >  NOKPROBE_SYMBOL(rcu_nmi_enter);
> >  
> >  /**
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Masami Hiramatsu]

On Mon, 17 Feb 2020 08:31:12 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > BTW, if you consider the x86 specific code is in the generic file,
> > we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c.
> > (Sorry, I've hit this idea right now)
> 
> Might this affect other architectures with NMIs and probe-like things?
> If so, it might make sense to leave it where it is.

Yes, git grep shows that arm64 is using rcu_nmi_enter() in
debug_exception_enter().
OK, let's keep it, but maybe it is good to update the comment for
arm64 too. What about following?

+/*
+ * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
+ * marked NOKPROBE before kprobes handler is called.
+ * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
+ * before kprobes handle happens to call rcu_nmi_enter() which means
+ * that rcu_nmi_enter() must be marked NOKRPOBE.
+ */

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Paul E. McKenney]

On Tue, Feb 18, 2020 at 01:33:35PM +0900, Masami Hiramatsu wrote:
> On Mon, 17 Feb 2020 08:31:12 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > 
> > > BTW, if you consider the x86 specific code is in the generic file,
> > > we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c.
> > > (Sorry, I've hit this idea right now)
> > 
> > Might this affect other architectures with NMIs and probe-like things?
> > If so, it might make sense to leave it where it is.
> 
> Yes, git grep shows that arm64 is using rcu_nmi_enter() in
> debug_exception_enter().
> OK, let's keep it, but maybe it is good to update the comment for
> arm64 too. What about following?
> 
> +/*
> + * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
> + * marked NOKPROBE before kprobes handler is called.
> + * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
> + * before kprobes handle happens to call rcu_nmi_enter() which means
> + * that rcu_nmi_enter() must be marked NOKRPOBE.
> + */

Would it work to describe the general problem, then give x86 details
as a specific example, as follows?

/*
 * On some architectures, certain exceptions prohibit use of kprobes until
 * the exception code path reaches a certain point.  For example, on x86 all
 * functions called by do_int3() must be marked NOKPROBE.  However, once
 * kprobe_int3_handler() is called, kprobing is permitted.  Specifically,
 * ist_enter() is called in do_int3() before kprobe_int3_handle().
 * Furthermore, ist_enter() calls rcu_nmi_enter(), which means that
 * rcu_nmi_enter() must be marked NOKRPOBE.
 */

That way, I don't feel like I need to update the commment each time
a new architecture adds this capability.  ;-)

							Thanx, Paul

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Mathieu Desnoyers]

----- On Feb 18, 2020, at 11:12 AM, paulmck paulmck@kernel.org wrote:

> On Tue, Feb 18, 2020 at 01:33:35PM +0900, Masami Hiramatsu wrote:
>> On Mon, 17 Feb 2020 08:31:12 -0800
>> "Paul E. McKenney" <paulmck@kernel.org> wrote:
>> > 
>> > > BTW, if you consider the x86 specific code is in the generic file,
>> > > we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c.
>> > > (Sorry, I've hit this idea right now)
>> > 
>> > Might this affect other architectures with NMIs and probe-like things?
>> > If so, it might make sense to leave it where it is.
>> 
>> Yes, git grep shows that arm64 is using rcu_nmi_enter() in
>> debug_exception_enter().
>> OK, let's keep it, but maybe it is good to update the comment for
>> arm64 too. What about following?
>> 
>> +/*
>> + * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
>> + * marked NOKPROBE before kprobes handler is called.
>> + * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
>> + * before kprobes handle happens to call rcu_nmi_enter() which means
>> + * that rcu_nmi_enter() must be marked NOKRPOBE.
>> + */
> 
> Would it work to describe the general problem, then give x86 details
> as a specific example, as follows?
> 
> /*
> * On some architectures, certain exceptions prohibit use of kprobes until
> * the exception code path reaches a certain point.  For example, on x86 all
> * functions called by do_int3() must be marked NOKPROBE.  However, once
> * kprobe_int3_handler() is called, kprobing is permitted.  Specifically,
> * ist_enter() is called in do_int3() before kprobe_int3_handle().
> * Furthermore, ist_enter() calls rcu_nmi_enter(), which means that
> * rcu_nmi_enter() must be marked NOKRPOBE.
> */
> 
> That way, I don't feel like I need to update the commment each time
> a new architecture adds this capability.  ;-)

I suspect this kind of comment would belong in a new
Documentation/features/kprobes/annotations.txt or something
similar. You might want to look at other "features" files to see
the expected layout.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Steven Rostedt]

On Tue, 18 Feb 2020 08:12:31 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> Would it work to describe the general problem, then give x86 details
> as a specific example, as follows?
> 
> /*
>  * On some architectures, certain exceptions prohibit use of kprobes until
>  * the exception code path reaches a certain point.  For example, on x86 all
>  * functions called by do_int3() must be marked NOKPROBE.  However, once
>  * kprobe_int3_handler() is called, kprobing is permitted.  Specifically,
>  * ist_enter() is called in do_int3() before kprobe_int3_handle().
>  * Furthermore, ist_enter() calls rcu_nmi_enter(), which means that
>  * rcu_nmi_enter() must be marked NOKRPOBE.
>  */
> 
> That way, I don't feel like I need to update the commment each time
> a new architecture adds this capability.  ;-)

I don't think this is going to be an issue for other archs, as they
don't have an IST.

-- Steve

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Steven Rostedt]

On Tue, 18 Feb 2020 13:33:35 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Mon, 17 Feb 2020 08:31:12 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> >   
> > > BTW, if you consider the x86 specific code is in the generic file,
> > > we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c.
> > > (Sorry, I've hit this idea right now)  
> > 
> > Might this affect other architectures with NMIs and probe-like things?
> > If so, it might make sense to leave it where it is.  
> 
> Yes, git grep shows that arm64 is using rcu_nmi_enter() in
> debug_exception_enter().
> OK, let's keep it, but maybe it is good to update the comment for
> arm64 too. What about following?
> 
> +/*
> + * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
> + * marked NOKPROBE before kprobes handler is called.
> + * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
> + * before kprobes handle happens to call rcu_nmi_enter() which means
> + * that rcu_nmi_enter() must be marked NOKRPOBE.
> + */
> 

Ah, why don't we just say...

/*
 * All functions called in the breakpoint trap handler (e.g. do_int3()
 * on x86), must not allow kprobes until the kprobe breakpoint handler
 * is called, otherwise it can cause an infinite recursion.
 * On some archs, rcu_nmi_enter() is called in the breakpoint handler
 * before the kprobe breakpoint handler is called, thus it must be
 * marked as NOKPROBE.
 */

And that way we don't make this an arch specific comment.

-- Steve

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Paul E. McKenney]

On Tue, Feb 18, 2020 at 12:46:09PM -0500, Steven Rostedt wrote:
> On Tue, 18 Feb 2020 13:33:35 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > On Mon, 17 Feb 2020 08:31:12 -0800
> > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > >   
> > > > BTW, if you consider the x86 specific code is in the generic file,
> > > > we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c.
> > > > (Sorry, I've hit this idea right now)  
> > > 
> > > Might this affect other architectures with NMIs and probe-like things?
> > > If so, it might make sense to leave it where it is.  
> > 
> > Yes, git grep shows that arm64 is using rcu_nmi_enter() in
> > debug_exception_enter().
> > OK, let's keep it, but maybe it is good to update the comment for
> > arm64 too. What about following?
> > 
> > +/*
> > + * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
> > + * marked NOKPROBE before kprobes handler is called.
> > + * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
> > + * before kprobes handle happens to call rcu_nmi_enter() which means
> > + * that rcu_nmi_enter() must be marked NOKRPOBE.
> > + */
> > 
> 
> Ah, why don't we just say...
> 
> /*
>  * All functions called in the breakpoint trap handler (e.g. do_int3()
>  * on x86), must not allow kprobes until the kprobe breakpoint handler
>  * is called, otherwise it can cause an infinite recursion.
>  * On some archs, rcu_nmi_enter() is called in the breakpoint handler
>  * before the kprobe breakpoint handler is called, thus it must be
>  * marked as NOKPROBE.
>  */
> 
> And that way we don't make this an arch specific comment.

That looks good to me.  Masami, does this work for you?

							Thanx, Paul

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Masami Hiramatsu]

On Tue, 18 Feb 2020 12:18:06 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> On Tue, Feb 18, 2020 at 12:46:09PM -0500, Steven Rostedt wrote:
> > On Tue, 18 Feb 2020 13:33:35 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > > On Mon, 17 Feb 2020 08:31:12 -0800
> > > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > >   
> > > > > BTW, if you consider the x86 specific code is in the generic file,
> > > > > we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c.
> > > > > (Sorry, I've hit this idea right now)  
> > > > 
> > > > Might this affect other architectures with NMIs and probe-like things?
> > > > If so, it might make sense to leave it where it is.  
> > > 
> > > Yes, git grep shows that arm64 is using rcu_nmi_enter() in
> > > debug_exception_enter().
> > > OK, let's keep it, but maybe it is good to update the comment for
> > > arm64 too. What about following?
> > > 
> > > +/*
> > > + * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
> > > + * marked NOKPROBE before kprobes handler is called.
> > > + * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
> > > + * before kprobes handle happens to call rcu_nmi_enter() which means
> > > + * that rcu_nmi_enter() must be marked NOKRPOBE.
> > > + */
> > > 
> > 
> > Ah, why don't we just say...
> > 
> > /*
> >  * All functions called in the breakpoint trap handler (e.g. do_int3()
> >  * on x86), must not allow kprobes until the kprobe breakpoint handler
> >  * is called, otherwise it can cause an infinite recursion.
> >  * On some archs, rcu_nmi_enter() is called in the breakpoint handler
> >  * before the kprobe breakpoint handler is called, thus it must be
> >  * marked as NOKPROBE.
> >  */
> > 
> > And that way we don't make this an arch specific comment.
> 
> That looks good to me.  Masami, does this work for you?

Yes, that looks good to me too :)

Thank you!


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Masami Hiramatsu]

Hi,

On Wed, 19 Feb 2020 11:45:10 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Tue, 18 Feb 2020 12:18:06 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > On Tue, Feb 18, 2020 at 12:46:09PM -0500, Steven Rostedt wrote:
> > > On Tue, 18 Feb 2020 13:33:35 +0900
> > > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > 
> > > > On Mon, 17 Feb 2020 08:31:12 -0800
> > > > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > > >   
> > > > > > BTW, if you consider the x86 specific code is in the generic file,
> > > > > > we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c.
> > > > > > (Sorry, I've hit this idea right now)  
> > > > > 
> > > > > Might this affect other architectures with NMIs and probe-like things?
> > > > > If so, it might make sense to leave it where it is.  
> > > > 
> > > > Yes, git grep shows that arm64 is using rcu_nmi_enter() in
> > > > debug_exception_enter().
> > > > OK, let's keep it, but maybe it is good to update the comment for
> > > > arm64 too. What about following?
> > > > 
> > > > +/*
> > > > + * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
> > > > + * marked NOKPROBE before kprobes handler is called.
> > > > + * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
> > > > + * before kprobes handle happens to call rcu_nmi_enter() which means
> > > > + * that rcu_nmi_enter() must be marked NOKRPOBE.
> > > > + */
> > > > 
> > > 
> > > Ah, why don't we just say...
> > > 
> > > /*
> > >  * All functions called in the breakpoint trap handler (e.g. do_int3()
> > >  * on x86), must not allow kprobes until the kprobe breakpoint handler
> > >  * is called, otherwise it can cause an infinite recursion.
> > >  * On some archs, rcu_nmi_enter() is called in the breakpoint handler
> > >  * before the kprobe breakpoint handler is called, thus it must be
> > >  * marked as NOKPROBE.
> > >  */
> > > 
> > > And that way we don't make this an arch specific comment.
> > 
> > That looks good to me.  Masami, does this work for you?
> 
> Yes, that looks good to me too :)

Oops, I'm guilty!
Sorry *rcu_nmi_exit()* also must be NOKPROBE, since even if we could catch
a recursive kprobe call, we can only skip the kprobe handler, but we must
exit from do_int3() and hit rcu_nmi_exit() again!

[45235.497591] Unrecoverable kprobe detected.
[45235.501400] Dumping kprobe:
[45235.502433] Name: (null)
[45235.502433] Offset: 0
[45235.502433] Address: rcu_nmi_exit+0x0/0x290
[45235.504044] ------------[ cut here ]------------
[45235.504855] kernel BUG at arch/x86/kernel/kprobes/core.c:646!
[45235.505816] invalid opcode: 0000 [#1] PREEMPT SMP PTI
[45235.506615] CPU: 7 PID: 143 Comm: sh Not tainted 5.6.0-rc3+ #143
[45235.507662] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
[45235.509764] RIP: 0010:reenter_kprobe.cold+0x14/0x16
[45235.510630] Code: 48 8b 75 10 48 c7 c7 f0 70 0e 82 48 8b 56 28 e8 22 91 08 00 0f 0b 48 c7 c7 20 71 0e 82 e8 14 91 08 00 48 89 ef e8 23 ee 0f 00 <0f> 0b 48 89 ee 48 c7 c7 48 71 0e 82 e8 fb 90 08 00 e9 c3 fc ff ff
[45235.513948] RSP: 0018:ffffc90000347bf8 EFLAGS: 00010046
[45235.514906] RAX: 0000000000000036 RBX: 0000000000017f20 RCX: 0000000000000000
[45235.516109] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000001
[45235.517278] RBP: ffff88807c9820c0 R08: 0000000000000000 R09: 0000000000000001
[45235.518415] R10: 0000000000000000 R11: ffff88807c9d1f18 R12: ffff88807d9d7f20
[45235.519609] R13: ffffc90000347c68 R14: ffffffff810e8a60 R15: ffffffff810e8a61
[45235.520787] FS:  0000000001d9a8c0(0000) GS:ffff88807d9c0000(0000) knlGS:0000000000000000
[45235.522198] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[45235.523172] CR2: 0000000001da9000 CR3: 000000007a880000 CR4: 00000000000006a0
[45235.524288] Call Trace:
[45235.524825]  kprobe_int3_handler+0x74/0x150
[45235.525627]  do_int3+0x36/0xf0
[45235.526244]  int3+0x42/0x50
[45235.526767] RIP: 0010:rcu_nmi_exit+0x1/0x290
[45235.527551] Code: a2 0d 82 be c2 01 00 00 48 c7 c7 d5 44 0f 82 c6 05 e7 ac 24 01 01 e8 1f ba fd ff eb b8 66 66 2e 0f 1f 84 00 00 00 00 00 90 cc <57> 41 56 41 55 41 54 55 48 c7 c5 40 c2 02 00 53 48 89 eb e8 77 75
[45235.530898] RSP: 0018:ffffc90000347d40 EFLAGS: 00000046
[45235.531816] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[45235.533001] RDX: 0000000000000001 RSI: ffffffff8101e1fe RDI: ffffffff8101e1fe
[45235.534252] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
[45235.535516] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[45235.536759] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[45235.537945]  ? ist_exit+0xe/0x20
[45235.538593]  ? ist_exit+0xe/0x20
[45235.539239]  ? rcu_nmi_exit+0x1/0x290
[45235.541182]  int3+0x42/0x50
[45235.541687] RIP: 0010:0xffffffffa000005a
[45235.542363] Code: 2e 16 13 e1 00 00 00 00 00 00 00 00 89 f8 e9 1f 16 13 e1 00 00 00 00 00 00 00 00 89 f8 e9 20 16 13 e1 00 00 00 00 00 00 00 00 <41> 57 e9 01 8a 0e e1 00 00 00 00 00 00 00 00 41 57 e9 f2 22 26 e1
[45235.545628] RSP: 0018:ffffc90000347e20 EFLAGS: 00000146
[45235.546596] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[45235.547989] RDX: 0000000000000001 RSI: ffffffff8101e1fe RDI: ffffffff8101e1fe
[45235.550183] RBP: 0000000000000000 R08: 0000000000000001 R09: ffff88807d2aa000
[45235.551591] R10: 0000000000000a4c R11: ffff88807bfec600 R12: 0000000000000000
[45235.552893] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[45235.554633]  ? ist_exit+0xe/0x20
[45235.555537]  ? ist_exit+0xe/0x20
[45235.556565]  ? rcu_nmi_exit+0x1/0x290
[45235.557909]  ? int3+0x42/0x50
[45235.559156]  ? 0xffffffffa0000069
[45235.560547]  ? vfs_read+0x1/0x150
[45235.561522]  ? ksys_read+0x60/0xe0
[45235.562458]  ? do_syscall_64+0x4b/0x1e0
[45235.563404]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
[45235.564705] Modules linked in:
[45235.565556] ---[ end trace 870af8724dba9ac8 ]---

So all functions called from do_int3() must be NOKPROBE.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Joel Fernandes]

On Sat, Mar 07, 2020 at 03:01:49AM +0900, Masami Hiramatsu wrote:
> Hi,
> 
> On Wed, 19 Feb 2020 11:45:10 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > On Tue, 18 Feb 2020 12:18:06 -0800
> > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > 
> > > On Tue, Feb 18, 2020 at 12:46:09PM -0500, Steven Rostedt wrote:
> > > > On Tue, 18 Feb 2020 13:33:35 +0900
> > > > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > > 
> > > > > On Mon, 17 Feb 2020 08:31:12 -0800
> > > > > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > > > >   
> > > > > > > BTW, if you consider the x86 specific code is in the generic file,
> > > > > > > we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c.
> > > > > > > (Sorry, I've hit this idea right now)  
> > > > > > 
> > > > > > Might this affect other architectures with NMIs and probe-like things?
> > > > > > If so, it might make sense to leave it where it is.  
> > > > > 
> > > > > Yes, git grep shows that arm64 is using rcu_nmi_enter() in
> > > > > debug_exception_enter().
> > > > > OK, let's keep it, but maybe it is good to update the comment for
> > > > > arm64 too. What about following?
> > > > > 
> > > > > +/*
> > > > > + * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
> > > > > + * marked NOKPROBE before kprobes handler is called.
> > > > > + * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
> > > > > + * before kprobes handle happens to call rcu_nmi_enter() which means
> > > > > + * that rcu_nmi_enter() must be marked NOKRPOBE.
> > > > > + */
> > > > > 
> > > > 
> > > > Ah, why don't we just say...
> > > > 
> > > > /*
> > > >  * All functions called in the breakpoint trap handler (e.g. do_int3()
> > > >  * on x86), must not allow kprobes until the kprobe breakpoint handler
> > > >  * is called, otherwise it can cause an infinite recursion.
> > > >  * On some archs, rcu_nmi_enter() is called in the breakpoint handler
> > > >  * before the kprobe breakpoint handler is called, thus it must be
> > > >  * marked as NOKPROBE.
> > > >  */
> > > > 
> > > > And that way we don't make this an arch specific comment.
> > > 
> > > That looks good to me.  Masami, does this work for you?
> > 
> > Yes, that looks good to me too :)

Aha! So then I'm glad I brought it up ;-) OCDs pay off these days :-D

thanks,

 - Joel


> Oops, I'm guilty!
> Sorry *rcu_nmi_exit()* also must be NOKPROBE, since even if we could catch
> a recursive kprobe call, we can only skip the kprobe handler, but we must
> exit from do_int3() and hit rcu_nmi_exit() again!
> 
> [45235.497591] Unrecoverable kprobe detected.
> [45235.501400] Dumping kprobe:
> [45235.502433] Name: (null)
> [45235.502433] Offset: 0
> [45235.502433] Address: rcu_nmi_exit+0x0/0x290
> [45235.504044] ------------[ cut here ]------------
> [45235.504855] kernel BUG at arch/x86/kernel/kprobes/core.c:646!
> [45235.505816] invalid opcode: 0000 [#1] PREEMPT SMP PTI
> [45235.506615] CPU: 7 PID: 143 Comm: sh Not tainted 5.6.0-rc3+ #143
> [45235.507662] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
> [45235.509764] RIP: 0010:reenter_kprobe.cold+0x14/0x16
> [45235.510630] Code: 48 8b 75 10 48 c7 c7 f0 70 0e 82 48 8b 56 28 e8 22 91 08 00 0f 0b 48 c7 c7 20 71 0e 82 e8 14 91 08 00 48 89 ef e8 23 ee 0f 00 <0f> 0b 48 89 ee 48 c7 c7 48 71 0e 82 e8 fb 90 08 00 e9 c3 fc ff ff
> [45235.513948] RSP: 0018:ffffc90000347bf8 EFLAGS: 00010046
> [45235.514906] RAX: 0000000000000036 RBX: 0000000000017f20 RCX: 0000000000000000
> [45235.516109] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000001
> [45235.517278] RBP: ffff88807c9820c0 R08: 0000000000000000 R09: 0000000000000001
> [45235.518415] R10: 0000000000000000 R11: ffff88807c9d1f18 R12: ffff88807d9d7f20
> [45235.519609] R13: ffffc90000347c68 R14: ffffffff810e8a60 R15: ffffffff810e8a61
> [45235.520787] FS:  0000000001d9a8c0(0000) GS:ffff88807d9c0000(0000) knlGS:0000000000000000
> [45235.522198] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [45235.523172] CR2: 0000000001da9000 CR3: 000000007a880000 CR4: 00000000000006a0
> [45235.524288] Call Trace:
> [45235.524825]  kprobe_int3_handler+0x74/0x150
> [45235.525627]  do_int3+0x36/0xf0
> [45235.526244]  int3+0x42/0x50
> [45235.526767] RIP: 0010:rcu_nmi_exit+0x1/0x290
> [45235.527551] Code: a2 0d 82 be c2 01 00 00 48 c7 c7 d5 44 0f 82 c6 05 e7 ac 24 01 01 e8 1f ba fd ff eb b8 66 66 2e 0f 1f 84 00 00 00 00 00 90 cc <57> 41 56 41 55 41 54 55 48 c7 c5 40 c2 02 00 53 48 89 eb e8 77 75
> [45235.530898] RSP: 0018:ffffc90000347d40 EFLAGS: 00000046
> [45235.531816] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> [45235.533001] RDX: 0000000000000001 RSI: ffffffff8101e1fe RDI: ffffffff8101e1fe
> [45235.534252] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
> [45235.535516] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> [45235.536759] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> [45235.537945]  ? ist_exit+0xe/0x20
> [45235.538593]  ? ist_exit+0xe/0x20
> [45235.539239]  ? rcu_nmi_exit+0x1/0x290
> [45235.541182]  int3+0x42/0x50
> [45235.541687] RIP: 0010:0xffffffffa000005a
> [45235.542363] Code: 2e 16 13 e1 00 00 00 00 00 00 00 00 89 f8 e9 1f 16 13 e1 00 00 00 00 00 00 00 00 89 f8 e9 20 16 13 e1 00 00 00 00 00 00 00 00 <41> 57 e9 01 8a 0e e1 00 00 00 00 00 00 00 00 41 57 e9 f2 22 26 e1
> [45235.545628] RSP: 0018:ffffc90000347e20 EFLAGS: 00000146
> [45235.546596] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> [45235.547989] RDX: 0000000000000001 RSI: ffffffff8101e1fe RDI: ffffffff8101e1fe
> [45235.550183] RBP: 0000000000000000 R08: 0000000000000001 R09: ffff88807d2aa000
> [45235.551591] R10: 0000000000000a4c R11: ffff88807bfec600 R12: 0000000000000000
> [45235.552893] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> [45235.554633]  ? ist_exit+0xe/0x20
> [45235.555537]  ? ist_exit+0xe/0x20
> [45235.556565]  ? rcu_nmi_exit+0x1/0x290
> [45235.557909]  ? int3+0x42/0x50
> [45235.559156]  ? 0xffffffffa0000069
> [45235.560547]  ? vfs_read+0x1/0x150
> [45235.561522]  ? ksys_read+0x60/0xe0
> [45235.562458]  ? do_syscall_64+0x4b/0x1e0
> [45235.563404]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [45235.564705] Modules linked in:
> [45235.565556] ---[ end trace 870af8724dba9ac8 ]---
> 
> So all functions called from do_int3() must be NOKPROBE.
> 
> Thank you,
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Joel Fernandes]

On Sat, Mar 07, 2020 at 03:01:49AM +0900, Masami Hiramatsu wrote:
> Hi,
> 
> On Wed, 19 Feb 2020 11:45:10 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > On Tue, 18 Feb 2020 12:18:06 -0800
> > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > 
> > > On Tue, Feb 18, 2020 at 12:46:09PM -0500, Steven Rostedt wrote:
> > > > On Tue, 18 Feb 2020 13:33:35 +0900
> > > > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > > 
> > > > > On Mon, 17 Feb 2020 08:31:12 -0800
> > > > > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > > > >   
> > > > > > > BTW, if you consider the x86 specific code is in the generic file,
> > > > > > > we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c.
> > > > > > > (Sorry, I've hit this idea right now)  
> > > > > > 
> > > > > > Might this affect other architectures with NMIs and probe-like things?
> > > > > > If so, it might make sense to leave it where it is.  
> > > > > 
> > > > > Yes, git grep shows that arm64 is using rcu_nmi_enter() in
> > > > > debug_exception_enter().
> > > > > OK, let's keep it, but maybe it is good to update the comment for
> > > > > arm64 too. What about following?
> > > > > 
> > > > > +/*
> > > > > + * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
> > > > > + * marked NOKPROBE before kprobes handler is called.
> > > > > + * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
> > > > > + * before kprobes handle happens to call rcu_nmi_enter() which means
> > > > > + * that rcu_nmi_enter() must be marked NOKRPOBE.
> > > > > + */
> > > > > 
> > > > 
> > > > Ah, why don't we just say...
> > > > 
> > > > /*
> > > >  * All functions called in the breakpoint trap handler (e.g. do_int3()
> > > >  * on x86), must not allow kprobes until the kprobe breakpoint handler
> > > >  * is called, otherwise it can cause an infinite recursion.
> > > >  * On some archs, rcu_nmi_enter() is called in the breakpoint handler
> > > >  * before the kprobe breakpoint handler is called, thus it must be
> > > >  * marked as NOKPROBE.
> > > >  */
> > > > 
> > > > And that way we don't make this an arch specific comment.
> > > 
> > > That looks good to me.  Masami, does this work for you?
> > 
> > Yes, that looks good to me too :)
> 
> Oops, I'm guilty!
> Sorry *rcu_nmi_exit()* also must be NOKPROBE, since even if we could catch
> a recursive kprobe call, we can only skip the kprobe handler, but we must
> exit from do_int3() and hit rcu_nmi_exit() again!
> 
> [45235.497591] Unrecoverable kprobe detected.
> [45235.501400] Dumping kprobe:
> [45235.502433] Name: (null)
> [45235.502433] Offset: 0
> [45235.502433] Address: rcu_nmi_exit+0x0/0x290
> [45235.504044] ------------[ cut here ]------------
> [45235.504855] kernel BUG at arch/x86/kernel/kprobes/core.c:646!
> [45235.505816] invalid opcode: 0000 [#1] PREEMPT SMP PTI
> [45235.506615] CPU: 7 PID: 143 Comm: sh Not tainted 5.6.0-rc3+ #143
> [45235.507662] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
> [45235.509764] RIP: 0010:reenter_kprobe.cold+0x14/0x16
> [45235.510630] Code: 48 8b 75 10 48 c7 c7 f0 70 0e 82 48 8b 56 28 e8 22 91 08 00 0f 0b 48 c7 c7 20 71 0e 82 e8 14 91 08 00 48 89 ef e8 23 ee 0f 00 <0f> 0b 48 89 ee 48 c7 c7 48 71 0e 82 e8 fb 90 08 00 e9 c3 fc ff ff
> [45235.513948] RSP: 0018:ffffc90000347bf8 EFLAGS: 00010046
> [45235.514906] RAX: 0000000000000036 RBX: 0000000000017f20 RCX: 0000000000000000
> [45235.516109] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000001
> [45235.517278] RBP: ffff88807c9820c0 R08: 0000000000000000 R09: 0000000000000001
> [45235.518415] R10: 0000000000000000 R11: ffff88807c9d1f18 R12: ffff88807d9d7f20
> [45235.519609] R13: ffffc90000347c68 R14: ffffffff810e8a60 R15: ffffffff810e8a61
> [45235.520787] FS:  0000000001d9a8c0(0000) GS:ffff88807d9c0000(0000) knlGS:0000000000000000
> [45235.522198] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [45235.523172] CR2: 0000000001da9000 CR3: 000000007a880000 CR4: 00000000000006a0
> [45235.524288] Call Trace:
> [45235.524825]  kprobe_int3_handler+0x74/0x150
> [45235.525627]  do_int3+0x36/0xf0
> [45235.526244]  int3+0x42/0x50
> [45235.526767] RIP: 0010:rcu_nmi_exit+0x1/0x290
> [45235.527551] Code: a2 0d 82 be c2 01 00 00 48 c7 c7 d5 44 0f 82 c6 05 e7 ac 24 01 01 e8 1f ba fd ff eb b8 66 66 2e 0f 1f 84 00 00 00 00 00 90 cc <57> 41 56 41 55 41 54 55 48 c7 c5 40 c2 02 00 53 48 89 eb e8 77 75
> [45235.530898] RSP: 0018:ffffc90000347d40 EFLAGS: 00000046
> [45235.531816] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> [45235.533001] RDX: 0000000000000001 RSI: ffffffff8101e1fe RDI: ffffffff8101e1fe
> [45235.534252] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
> [45235.535516] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> [45235.536759] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> [45235.537945]  ? ist_exit+0xe/0x20
> [45235.538593]  ? ist_exit+0xe/0x20
> [45235.539239]  ? rcu_nmi_exit+0x1/0x290
> [45235.541182]  int3+0x42/0x50
> [45235.541687] RIP: 0010:0xffffffffa000005a
> [45235.542363] Code: 2e 16 13 e1 00 00 00 00 00 00 00 00 89 f8 e9 1f 16 13 e1 00 00 00 00 00 00 00 00 89 f8 e9 20 16 13 e1 00 00 00 00 00 00 00 00 <41> 57 e9 01 8a 0e e1 00 00 00 00 00 00 00 00 41 57 e9 f2 22 26 e1
> [45235.545628] RSP: 0018:ffffc90000347e20 EFLAGS: 00000146
> [45235.546596] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> [45235.547989] RDX: 0000000000000001 RSI: ffffffff8101e1fe RDI: ffffffff8101e1fe
> [45235.550183] RBP: 0000000000000000 R08: 0000000000000001 R09: ffff88807d2aa000
> [45235.551591] R10: 0000000000000a4c R11: ffff88807bfec600 R12: 0000000000000000
> [45235.552893] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> [45235.554633]  ? ist_exit+0xe/0x20
> [45235.555537]  ? ist_exit+0xe/0x20
> [45235.556565]  ? rcu_nmi_exit+0x1/0x290
> [45235.557909]  ? int3+0x42/0x50
> [45235.559156]  ? 0xffffffffa0000069
> [45235.560547]  ? vfs_read+0x1/0x150
> [45235.561522]  ? ksys_read+0x60/0xe0
> [45235.562458]  ? do_syscall_64+0x4b/0x1e0
> [45235.563404]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [45235.564705] Modules linked in:
> [45235.565556] ---[ end trace 870af8724dba9ac8 ]---
> 
> So all functions called from do_int3() must be NOKPROBE.

Makes sense, I am curious though why you thought before that the breakpoint
recursion was not possible *after* kprobe_int3_handler(). In the below thread
you said it is to be marked only for functions *before*
kprobe_int3_handler(). Was there a reason?
https://lore.kernel.org/lkml/154998793571.31052.11301258949601150994.stgit@devbox/

thanks,

 - Joel


> 
> Thank you,
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Masami Hiramatsu]

On Fri, 6 Mar 2020 14:11:51 -0500
Joel Fernandes <joel@joelfernandes.org> wrote:

> On Sat, Mar 07, 2020 at 03:01:49AM +0900, Masami Hiramatsu wrote:
> > Hi,
> > 
> > On Wed, 19 Feb 2020 11:45:10 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > > On Tue, 18 Feb 2020 12:18:06 -0800
> > > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > 
> > > > On Tue, Feb 18, 2020 at 12:46:09PM -0500, Steven Rostedt wrote:
> > > > > On Tue, 18 Feb 2020 13:33:35 +0900
> > > > > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > > > 
> > > > > > On Mon, 17 Feb 2020 08:31:12 -0800
> > > > > > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > > > > >   
> > > > > > > > BTW, if you consider the x86 specific code is in the generic file,
> > > > > > > > we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c.
> > > > > > > > (Sorry, I've hit this idea right now)  
> > > > > > > 
> > > > > > > Might this affect other architectures with NMIs and probe-like things?
> > > > > > > If so, it might make sense to leave it where it is.  
> > > > > > 
> > > > > > Yes, git grep shows that arm64 is using rcu_nmi_enter() in
> > > > > > debug_exception_enter().
> > > > > > OK, let's keep it, but maybe it is good to update the comment for
> > > > > > arm64 too. What about following?
> > > > > > 
> > > > > > +/*
> > > > > > + * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
> > > > > > + * marked NOKPROBE before kprobes handler is called.
> > > > > > + * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
> > > > > > + * before kprobes handle happens to call rcu_nmi_enter() which means
> > > > > > + * that rcu_nmi_enter() must be marked NOKRPOBE.
> > > > > > + */
> > > > > > 
> > > > > 
> > > > > Ah, why don't we just say...
> > > > > 
> > > > > /*
> > > > >  * All functions called in the breakpoint trap handler (e.g. do_int3()
> > > > >  * on x86), must not allow kprobes until the kprobe breakpoint handler
> > > > >  * is called, otherwise it can cause an infinite recursion.
> > > > >  * On some archs, rcu_nmi_enter() is called in the breakpoint handler
> > > > >  * before the kprobe breakpoint handler is called, thus it must be
> > > > >  * marked as NOKPROBE.
> > > > >  */
> > > > > 
> > > > > And that way we don't make this an arch specific comment.
> > > > 
> > > > That looks good to me.  Masami, does this work for you?
> > > 
> > > Yes, that looks good to me too :)
> > 
> > Oops, I'm guilty!
> > Sorry *rcu_nmi_exit()* also must be NOKPROBE, since even if we could catch
> > a recursive kprobe call, we can only skip the kprobe handler, but we must
> > exit from do_int3() and hit rcu_nmi_exit() again!
> > 
> > [45235.497591] Unrecoverable kprobe detected.
> > [45235.501400] Dumping kprobe:
> > [45235.502433] Name: (null)
> > [45235.502433] Offset: 0
> > [45235.502433] Address: rcu_nmi_exit+0x0/0x290
> > [45235.504044] ------------[ cut here ]------------
> > [45235.504855] kernel BUG at arch/x86/kernel/kprobes/core.c:646!
> > [45235.505816] invalid opcode: 0000 [#1] PREEMPT SMP PTI
> > [45235.506615] CPU: 7 PID: 143 Comm: sh Not tainted 5.6.0-rc3+ #143
> > [45235.507662] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
> > [45235.509764] RIP: 0010:reenter_kprobe.cold+0x14/0x16
> > [45235.510630] Code: 48 8b 75 10 48 c7 c7 f0 70 0e 82 48 8b 56 28 e8 22 91 08 00 0f 0b 48 c7 c7 20 71 0e 82 e8 14 91 08 00 48 89 ef e8 23 ee 0f 00 <0f> 0b 48 89 ee 48 c7 c7 48 71 0e 82 e8 fb 90 08 00 e9 c3 fc ff ff
> > [45235.513948] RSP: 0018:ffffc90000347bf8 EFLAGS: 00010046
> > [45235.514906] RAX: 0000000000000036 RBX: 0000000000017f20 RCX: 0000000000000000
> > [45235.516109] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000001
> > [45235.517278] RBP: ffff88807c9820c0 R08: 0000000000000000 R09: 0000000000000001
> > [45235.518415] R10: 0000000000000000 R11: ffff88807c9d1f18 R12: ffff88807d9d7f20
> > [45235.519609] R13: ffffc90000347c68 R14: ffffffff810e8a60 R15: ffffffff810e8a61
> > [45235.520787] FS:  0000000001d9a8c0(0000) GS:ffff88807d9c0000(0000) knlGS:0000000000000000
> > [45235.522198] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [45235.523172] CR2: 0000000001da9000 CR3: 000000007a880000 CR4: 00000000000006a0
> > [45235.524288] Call Trace:
> > [45235.524825]  kprobe_int3_handler+0x74/0x150
> > [45235.525627]  do_int3+0x36/0xf0
> > [45235.526244]  int3+0x42/0x50
> > [45235.526767] RIP: 0010:rcu_nmi_exit+0x1/0x290
> > [45235.527551] Code: a2 0d 82 be c2 01 00 00 48 c7 c7 d5 44 0f 82 c6 05 e7 ac 24 01 01 e8 1f ba fd ff eb b8 66 66 2e 0f 1f 84 00 00 00 00 00 90 cc <57> 41 56 41 55 41 54 55 48 c7 c5 40 c2 02 00 53 48 89 eb e8 77 75
> > [45235.530898] RSP: 0018:ffffc90000347d40 EFLAGS: 00000046
> > [45235.531816] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> > [45235.533001] RDX: 0000000000000001 RSI: ffffffff8101e1fe RDI: ffffffff8101e1fe
> > [45235.534252] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
> > [45235.535516] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> > [45235.536759] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> > [45235.537945]  ? ist_exit+0xe/0x20
> > [45235.538593]  ? ist_exit+0xe/0x20
> > [45235.539239]  ? rcu_nmi_exit+0x1/0x290
> > [45235.541182]  int3+0x42/0x50
> > [45235.541687] RIP: 0010:0xffffffffa000005a
> > [45235.542363] Code: 2e 16 13 e1 00 00 00 00 00 00 00 00 89 f8 e9 1f 16 13 e1 00 00 00 00 00 00 00 00 89 f8 e9 20 16 13 e1 00 00 00 00 00 00 00 00 <41> 57 e9 01 8a 0e e1 00 00 00 00 00 00 00 00 41 57 e9 f2 22 26 e1
> > [45235.545628] RSP: 0018:ffffc90000347e20 EFLAGS: 00000146
> > [45235.546596] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> > [45235.547989] RDX: 0000000000000001 RSI: ffffffff8101e1fe RDI: ffffffff8101e1fe
> > [45235.550183] RBP: 0000000000000000 R08: 0000000000000001 R09: ffff88807d2aa000
> > [45235.551591] R10: 0000000000000a4c R11: ffff88807bfec600 R12: 0000000000000000
> > [45235.552893] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> > [45235.554633]  ? ist_exit+0xe/0x20
> > [45235.555537]  ? ist_exit+0xe/0x20
> > [45235.556565]  ? rcu_nmi_exit+0x1/0x290
> > [45235.557909]  ? int3+0x42/0x50
> > [45235.559156]  ? 0xffffffffa0000069
> > [45235.560547]  ? vfs_read+0x1/0x150
> > [45235.561522]  ? ksys_read+0x60/0xe0
> > [45235.562458]  ? do_syscall_64+0x4b/0x1e0
> > [45235.563404]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [45235.564705] Modules linked in:
> > [45235.565556] ---[ end trace 870af8724dba9ac8 ]---
> > 
> > So all functions called from do_int3() must be NOKPROBE.
> 
> Makes sense, I am curious though why you thought before that the breakpoint
> recursion was not possible *after* kprobe_int3_handler(). In the below thread
> you said it is to be marked only for functions *before*
> kprobe_int3_handler(). Was there a reason?
> https://lore.kernel.org/lkml/154998793571.31052.11301258949601150994.stgit@devbox/

Sorry, that was my simple mistake. When I tested the rcu_nmi_exit(), I forgot
to disable optprobe. So whole the do_int3() was not hit... (I had to think
deeper why the hit count of the probe was not incremented at that point.)

This time, I set 0 to the /proc/sys/debug/kprobes_optimization, and confirmed
this bug.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Thomas Gleixner]

Steven Rostedt <rostedt@goodmis.org> writes:
> rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit

Very well said: OR other reasons. I assume you meant 'for' but ...

> c13324a505c77 ("x86/kprobes: Prohibit probing on functions before
> kprobe_int3_handler()")
>
> The issue was that we must not allow anything in do_int3() call kprobe
> code before kprobe_int3_handler() is called. Because ist_enter() (in
> do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had
> nothing to do with it being RCU nor NMI, but because it was simply
> called in do_int3().
>
> Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont
> to why rcu_nmi_enter() would probably be useful, like below:

... this is really wrong.

While the int3 issue was the reason why it was marked NOKPROBE, fact is
that aside of int3 problem (which is probably true for any other
architecture using breakpoints for patching) any probe _before_ RCU is
watching and _after_ RCU stopped watching is broken. Same applies for
BPF and tracepoints which call into BPF or other nonsense.

Can we please stop claiming that instrumentation can touch anything it
wants and just admit that anything outside RCU covered regions is
off-limits for instrumentation? Same applies for entry code and critical
exceptions/traps.

There is a reason why the tracer can't trace itself and there are very
valid reasons to limit the instrumentation ability in other places.

It's nice to be able to see into 'everything' but for 99.9999% of the
cases the coverage of these things is absolutely irrelevant.

Yes I know, "Correctness first" is this old school thing for grumpy old
greybeards who are still stuck in the 90's. "Features first" is the new
mantra. I deal with that every day by mopping up the mess it creates.

Thanks,

        tglx

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Paul E. McKenney]

On Thu, Feb 13, 2020 at 04:19:30PM -0500, Joel Fernandes wrote:
> On Thu, Feb 13, 2020 at 12:54:42PM -0800, Paul E. McKenney wrote:
> > On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:
> > > On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
> > > [...] 
> > > > > > It might well be that I could make these functions be NMI-safe, but
> > > > > > rcu_prepare_for_idle() in particular would be a bit ugly at best.
> > > > > > So, before looking into that, I have a question.  Given these proposed
> > > > > > changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> > > > > > to just use in_nmi()?
> > > > > 
> > > > > That _should_ already be the case today. That is, if we end up in a
> > > > > tracer and in_nmi() is unreliable we're already screwed anyway.
> > > > 
> > > > So something like this, then?  This is untested, probably doesn't even
> > > > build, and could use some careful review from both Peter and Steve,
> > > > at least.  As in the below is the second version of the patch, the first
> > > > having been missing a couple of important "!" characters.
> > > 
> > > I removed the static from rcu_nmi_enter()/exit() as it is called from
> > > outside, that makes it build now. Updated below is Paul's diff. I also added
> > > NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> > > asymmetric.
> > 
> > My compiler complained about the static and the __always_inline, so I
> > fixed those.  But please help me out on adding the NOKPROBE_SYMBOL()
> > to rcu_nmi_exit().  What bad thing happens if we leave this on only
> > rcu_nmi_enter()?
> 
> It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but
> allowing it on exit (from a code reading standpoint) so my reaction was to
> add it to both, but we could probably keep that as a separate
> patch/discussion since it is slightly unrelated to the patch.. Sorry to
> confuse the topic.

Actually and perhaps unusually, I was not being sarcastic, but was instead
asking a serious question.  Is the current code correct?  Should the
current NOKPROBE_SYMBOL() be removed?  Should the other NOKPROBE_SYMBOL()
be added?  Something else?  And either way, why?

							Thanx, Paul

> thanks,
> 
>  - Joel
> 
> 
> > 							Thanx, Paul
> > 
> > > ---8<-----------------------
> > > 
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index d91c9156fab2e..bbcc7767f18ee 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -614,16 +614,18 @@ void rcu_user_enter(void)
> > >  }
> > >  #endif /* CONFIG_NO_HZ_FULL */
> > >  
> > > -/*
> > > +/**
> > > + * rcu_nmi_exit - inform RCU of exit from NMI context
> > > + *
> > >   * If we are returning from the outermost NMI handler that interrupted an
> > >   * RCU-idle period, update rdp->dynticks and rdp->dynticks_nmi_nesting
> > >   * to let the RCU grace-period handling know that the CPU is back to
> > >   * being RCU-idle.
> > >   *
> > > - * If you add or remove a call to rcu_nmi_exit_common(), be sure to test
> > > + * If you add or remove a call to rcu_nmi_exit(), be sure to test
> > >   * with CONFIG_RCU_EQS_DEBUG=y.
> > >   */
> > > -static __always_inline void rcu_nmi_exit_common(bool irq)
> > > +__always_inline void rcu_nmi_exit(void)
> > >  {
> > >  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > >  
> > > @@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
> > >  	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
> > >  	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
> > >  
> > > -	if (irq)
> > > +	if (!in_nmi())
> > >  		rcu_prepare_for_idle();
> > >  
> > >  	rcu_dynticks_eqs_enter();
> > >  
> > > -	if (irq)
> > > +	if (!in_nmi())
> > >  		rcu_dynticks_task_enter();
> > >  }
> > > -
> > > -/**
> > > - * rcu_nmi_exit - inform RCU of exit from NMI context
> > > - *
> > > - * If you add or remove a call to rcu_nmi_exit(), be sure to test
> > > - * with CONFIG_RCU_EQS_DEBUG=y.
> > > - */
> > > -void rcu_nmi_exit(void)
> > > -{
> > > -	rcu_nmi_exit_common(false);
> > > -}
> > > +NOKPROBE_SYMBOL(rcu_nmi_exit);
> > >  
> > >  /**
> > >   * rcu_irq_exit - inform RCU that current CPU is exiting irq towards idle
> > > @@ -693,7 +685,7 @@ void rcu_nmi_exit(void)
> > >  void rcu_irq_exit(void)
> > >  {
> > >  	lockdep_assert_irqs_disabled();
> > > -	rcu_nmi_exit_common(true);
> > > +	rcu_nmi_exit();
> > >  }
> > >  
> > >  /*
> > > @@ -777,7 +769,7 @@ void rcu_user_exit(void)
> > >  #endif /* CONFIG_NO_HZ_FULL */
> > >  
> > >  /**
> > > - * rcu_nmi_enter_common - inform RCU of entry to NMI context
> > > + * rcu_nmi_enter - inform RCU of entry to NMI context
> > >   * @irq: Is this call from rcu_irq_enter?
> > >   *
> > >   * If the CPU was idle from RCU's viewpoint, update rdp->dynticks and
> > > @@ -786,10 +778,10 @@ void rcu_user_exit(void)
> > >   * long as the nesting level does not overflow an int.  (You will probably
> > >   * run out of stack space first.)
> > >   *
> > > - * If you add or remove a call to rcu_nmi_enter_common(), be sure to test
> > > + * If you add or remove a call to rcu_nmi_enter(), be sure to test
> > >   * with CONFIG_RCU_EQS_DEBUG=y.
> > >   */
> > > -static __always_inline void rcu_nmi_enter_common(bool irq)
> > > +__always_inline void rcu_nmi_enter(void)
> > >  {
> > >  	long incby = 2;
> > >  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > @@ -807,12 +799,12 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
> > >  	 */
> > >  	if (rcu_dynticks_curr_cpu_in_eqs()) {
> > >  
> > > -		if (irq)
> > > +		if (!in_nmi())
> > >  			rcu_dynticks_task_exit();
> > >  
> > >  		rcu_dynticks_eqs_exit();
> > >  
> > > -		if (irq)
> > > +		if (!in_nmi())
> > >  			rcu_cleanup_after_idle();
> > >  
> > >  		incby = 1;
> > > @@ -834,14 +826,6 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
> > >  		   rdp->dynticks_nmi_nesting + incby);
> > >  	barrier();
> > >  }
> > > -
> > > -/**
> > > - * rcu_nmi_enter - inform RCU of entry to NMI context
> > > - */
> > > -void rcu_nmi_enter(void)
> > > -{
> > > -	rcu_nmi_enter_common(false);
> > > -}
> > >  NOKPROBE_SYMBOL(rcu_nmi_enter);
> > >  
> > >  /**
> > > @@ -869,7 +853,7 @@ NOKPROBE_SYMBOL(rcu_nmi_enter);
> > >  void rcu_irq_enter(void)
> > >  {
> > >  	lockdep_assert_irqs_disabled();
> > > -	rcu_nmi_enter_common(true);
> > > +	rcu_nmi_enter();
> > >  }
> > >  
> > >  /*

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Joel Fernandes]

On Thu, Feb 13, 2020 at 01:48:59PM -0800, Paul E. McKenney wrote:
> On Thu, Feb 13, 2020 at 04:19:30PM -0500, Joel Fernandes wrote:
> > On Thu, Feb 13, 2020 at 12:54:42PM -0800, Paul E. McKenney wrote:
> > > On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:
> > > > On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
> > > > [...] 
> > > > > > > It might well be that I could make these functions be NMI-safe, but
> > > > > > > rcu_prepare_for_idle() in particular would be a bit ugly at best.
> > > > > > > So, before looking into that, I have a question.  Given these proposed
> > > > > > > changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
> > > > > > > to just use in_nmi()?
> > > > > > 
> > > > > > That _should_ already be the case today. That is, if we end up in a
> > > > > > tracer and in_nmi() is unreliable we're already screwed anyway.
> > > > > 
> > > > > So something like this, then?  This is untested, probably doesn't even
> > > > > build, and could use some careful review from both Peter and Steve,
> > > > > at least.  As in the below is the second version of the patch, the first
> > > > > having been missing a couple of important "!" characters.
> > > > 
> > > > I removed the static from rcu_nmi_enter()/exit() as it is called from
> > > > outside, that makes it build now. Updated below is Paul's diff. I also added
> > > > NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> > > > asymmetric.
> > > 
> > > My compiler complained about the static and the __always_inline, so I
> > > fixed those.  But please help me out on adding the NOKPROBE_SYMBOL()
> > > to rcu_nmi_exit().  What bad thing happens if we leave this on only
> > > rcu_nmi_enter()?
> > 
> > It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but
> > allowing it on exit (from a code reading standpoint) so my reaction was to
> > add it to both, but we could probably keep that as a separate
> > patch/discussion since it is slightly unrelated to the patch.. Sorry to
> > confuse the topic.
> 
> Actually and perhaps unusually, I was not being sarcastic, but was instead
> asking a serious question.  Is the current code correct?  Should the
> current NOKPROBE_SYMBOL() be removed?  Should the other NOKPROBE_SYMBOL()
> be added?  Something else?  And either way, why?

Oh ok, it was a fair question. Seems Steve nailed it, only the
rcu_nmi_enter() needs NOKPROBE, although as you mentioned in the other
thread, it would be good to get Masami's eyes on it since he introduced the
NOKPROBE.

thanks,

 - Joel


> 							Thanx, Paul
> 
> > thanks,
> > 
> >  - Joel
> > 
> > 
> > > 							Thanx, Paul
> > > 
> > > > ---8<-----------------------
> > > > 
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index d91c9156fab2e..bbcc7767f18ee 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -614,16 +614,18 @@ void rcu_user_enter(void)
> > > >  }
> > > >  #endif /* CONFIG_NO_HZ_FULL */
> > > >  
> > > > -/*
> > > > +/**
> > > > + * rcu_nmi_exit - inform RCU of exit from NMI context
> > > > + *
> > > >   * If we are returning from the outermost NMI handler that interrupted an
> > > >   * RCU-idle period, update rdp->dynticks and rdp->dynticks_nmi_nesting
> > > >   * to let the RCU grace-period handling know that the CPU is back to
> > > >   * being RCU-idle.
> > > >   *
> > > > - * If you add or remove a call to rcu_nmi_exit_common(), be sure to test
> > > > + * If you add or remove a call to rcu_nmi_exit(), be sure to test
> > > >   * with CONFIG_RCU_EQS_DEBUG=y.
> > > >   */
> > > > -static __always_inline void rcu_nmi_exit_common(bool irq)
> > > > +__always_inline void rcu_nmi_exit(void)
> > > >  {
> > > >  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > >  
> > > > @@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
> > > >  	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
> > > >  	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
> > > >  
> > > > -	if (irq)
> > > > +	if (!in_nmi())
> > > >  		rcu_prepare_for_idle();
> > > >  
> > > >  	rcu_dynticks_eqs_enter();
> > > >  
> > > > -	if (irq)
> > > > +	if (!in_nmi())
> > > >  		rcu_dynticks_task_enter();
> > > >  }
> > > > -
> > > > -/**
> > > > - * rcu_nmi_exit - inform RCU of exit from NMI context
> > > > - *
> > > > - * If you add or remove a call to rcu_nmi_exit(), be sure to test
> > > > - * with CONFIG_RCU_EQS_DEBUG=y.
> > > > - */
> > > > -void rcu_nmi_exit(void)
> > > > -{
> > > > -	rcu_nmi_exit_common(false);
> > > > -}
> > > > +NOKPROBE_SYMBOL(rcu_nmi_exit);
> > > >  
> > > >  /**
> > > >   * rcu_irq_exit - inform RCU that current CPU is exiting irq towards idle
> > > > @@ -693,7 +685,7 @@ void rcu_nmi_exit(void)
> > > >  void rcu_irq_exit(void)
> > > >  {
> > > >  	lockdep_assert_irqs_disabled();
> > > > -	rcu_nmi_exit_common(true);
> > > > +	rcu_nmi_exit();
> > > >  }
> > > >  
> > > >  /*
> > > > @@ -777,7 +769,7 @@ void rcu_user_exit(void)
> > > >  #endif /* CONFIG_NO_HZ_FULL */
> > > >  
> > > >  /**
> > > > - * rcu_nmi_enter_common - inform RCU of entry to NMI context
> > > > + * rcu_nmi_enter - inform RCU of entry to NMI context
> > > >   * @irq: Is this call from rcu_irq_enter?
> > > >   *
> > > >   * If the CPU was idle from RCU's viewpoint, update rdp->dynticks and
> > > > @@ -786,10 +778,10 @@ void rcu_user_exit(void)
> > > >   * long as the nesting level does not overflow an int.  (You will probably
> > > >   * run out of stack space first.)
> > > >   *
> > > > - * If you add or remove a call to rcu_nmi_enter_common(), be sure to test
> > > > + * If you add or remove a call to rcu_nmi_enter(), be sure to test
> > > >   * with CONFIG_RCU_EQS_DEBUG=y.
> > > >   */
> > > > -static __always_inline void rcu_nmi_enter_common(bool irq)
> > > > +__always_inline void rcu_nmi_enter(void)
> > > >  {
> > > >  	long incby = 2;
> > > >  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > > @@ -807,12 +799,12 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
> > > >  	 */
> > > >  	if (rcu_dynticks_curr_cpu_in_eqs()) {
> > > >  
> > > > -		if (irq)
> > > > +		if (!in_nmi())
> > > >  			rcu_dynticks_task_exit();
> > > >  
> > > >  		rcu_dynticks_eqs_exit();
> > > >  
> > > > -		if (irq)
> > > > +		if (!in_nmi())
> > > >  			rcu_cleanup_after_idle();
> > > >  
> > > >  		incby = 1;
> > > > @@ -834,14 +826,6 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
> > > >  		   rdp->dynticks_nmi_nesting + incby);
> > > >  	barrier();
> > > >  }
> > > > -
> > > > -/**
> > > > - * rcu_nmi_enter - inform RCU of entry to NMI context
> > > > - */
> > > > -void rcu_nmi_enter(void)
> > > > -{
> > > > -	rcu_nmi_enter_common(false);
> > > > -}
> > > >  NOKPROBE_SYMBOL(rcu_nmi_enter);
> > > >  
> > > >  /**
> > > > @@ -869,7 +853,7 @@ NOKPROBE_SYMBOL(rcu_nmi_enter);
> > > >  void rcu_irq_enter(void)
> > > >  {
> > > >  	lockdep_assert_irqs_disabled();
> > > > -	rcu_nmi_enter_common(true);
> > > > +	rcu_nmi_enter();
> > > >  }
> > > >  
> > > >  /*

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Steven Rostedt]

On Thu, 13 Feb 2020 17:58:53 -0500
Joel Fernandes <joel@joelfernandes.org> wrote:

> Oh ok, it was a fair question. Seems Steve nailed it, only the
> rcu_nmi_enter() needs NOKPROBE, although as you mentioned in the other
> thread, it would be good to get Masami's eyes on it since he introduced the
> NOKPROBE.

Note, I did go and verify that the issue still exists, and the NOKPROBE
looks to still be needed. But as you stated, because Masami added the
NOKPROBE I'd feel more comfortable with him looking over my explanation.

-- Steve

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Peter Zijlstra]

On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:

> > > That _should_ already be the case today. That is, if we end up in a
> > > tracer and in_nmi() is unreliable we're already screwed anyway.

> I removed the static from rcu_nmi_enter()/exit() as it is called from
> outside, that makes it build now. Updated below is Paul's diff. I also added
> NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> asymmetric.

> +__always_inline void rcu_nmi_exit(void)
>  {
>  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>  
> @@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
>  	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
>  	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
>  
> -	if (irq)
> +	if (!in_nmi())
>  		rcu_prepare_for_idle();
>  
>  	rcu_dynticks_eqs_enter();
>  
> -	if (irq)
> +	if (!in_nmi())
>  		rcu_dynticks_task_enter();
>  }

Boris and me have been going over the #MC code (and finding loads of
'interesting' code) and ran into ist_enter(), whish has the following
code:

                /*
                 * We might have interrupted pretty much anything.  In
                 * fact, if we're a machine check, we can even interrupt
                 * NMI processing.  We don't want in_nmi() to return true,
                 * but we need to notify RCU.
                 */
                rcu_nmi_enter();


Which, to me, sounds all sorts of broken. The IST (be it #DB or #MC) can
happen while we're holding all sorts of locks. This must be an NMI-like
context.

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Paul E. McKenney]

On Tue, Feb 18, 2020 at 08:58:31PM +0100, Peter Zijlstra wrote:
> On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:
> 
> > > > That _should_ already be the case today. That is, if we end up in a
> > > > tracer and in_nmi() is unreliable we're already screwed anyway.
> 
> > I removed the static from rcu_nmi_enter()/exit() as it is called from
> > outside, that makes it build now. Updated below is Paul's diff. I also added
> > NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> > asymmetric.
> 
> > +__always_inline void rcu_nmi_exit(void)
> >  {
> >  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> >  
> > @@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
> >  	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
> >  	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
> >  
> > -	if (irq)
> > +	if (!in_nmi())
> >  		rcu_prepare_for_idle();
> >  
> >  	rcu_dynticks_eqs_enter();
> >  
> > -	if (irq)
> > +	if (!in_nmi())
> >  		rcu_dynticks_task_enter();
> >  }
> 
> Boris and me have been going over the #MC code (and finding loads of
> 'interesting' code) and ran into ist_enter(), whish has the following
> code:
> 
>                 /*
>                  * We might have interrupted pretty much anything.  In
>                  * fact, if we're a machine check, we can even interrupt
>                  * NMI processing.  We don't want in_nmi() to return true,
>                  * but we need to notify RCU.
>                  */
>                 rcu_nmi_enter();
> 
> 
> Which, to me, sounds all sorts of broken. The IST (be it #DB or #MC) can
> happen while we're holding all sorts of locks. This must be an NMI-like
> context.

Ouch!  Looks like I need to hold off on getting rid of the "irq"
parameters if in_nmi() isn't going to be accurate.

							Thanx, Paul

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Peter Zijlstra]

On Tue, Feb 18, 2020 at 12:17:28PM -0800, Paul E. McKenney wrote:
> On Tue, Feb 18, 2020 at 08:58:31PM +0100, Peter Zijlstra wrote:
> > On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:
> > 
> > > > > That _should_ already be the case today. That is, if we end up in a
> > > > > tracer and in_nmi() is unreliable we're already screwed anyway.
> > 
> > > I removed the static from rcu_nmi_enter()/exit() as it is called from
> > > outside, that makes it build now. Updated below is Paul's diff. I also added
> > > NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> > > asymmetric.
> > 
> > > +__always_inline void rcu_nmi_exit(void)
> > >  {
> > >  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > >  
> > > @@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
> > >  	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
> > >  	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
> > >  
> > > -	if (irq)
> > > +	if (!in_nmi())
> > >  		rcu_prepare_for_idle();
> > >  
> > >  	rcu_dynticks_eqs_enter();
> > >  
> > > -	if (irq)
> > > +	if (!in_nmi())
> > >  		rcu_dynticks_task_enter();
> > >  }
> > 
> > Boris and me have been going over the #MC code (and finding loads of
> > 'interesting' code) and ran into ist_enter(), whish has the following
> > code:
> > 
> >                 /*
> >                  * We might have interrupted pretty much anything.  In
> >                  * fact, if we're a machine check, we can even interrupt
> >                  * NMI processing.  We don't want in_nmi() to return true,
> >                  * but we need to notify RCU.
> >                  */
> >                 rcu_nmi_enter();
> > 
> > 
> > Which, to me, sounds all sorts of broken. The IST (be it #DB or #MC) can
> > happen while we're holding all sorts of locks. This must be an NMI-like
> > context.
> 
> Ouch!  Looks like I need to hold off on getting rid of the "irq"
> parameters if in_nmi() isn't going to be accurate.

I'm currently trying to twist my brain around all this, because I
suspect it's all completely broken one way or another.

But yes, we definitely need to fix this before your patch goes in.

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Paul E. McKenney]

On Tue, Feb 18, 2020 at 09:40:21PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 18, 2020 at 12:17:28PM -0800, Paul E. McKenney wrote:
> > On Tue, Feb 18, 2020 at 08:58:31PM +0100, Peter Zijlstra wrote:
> > > On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:
> > > 
> > > > > > That _should_ already be the case today. That is, if we end up in a
> > > > > > tracer and in_nmi() is unreliable we're already screwed anyway.
> > > 
> > > > I removed the static from rcu_nmi_enter()/exit() as it is called from
> > > > outside, that makes it build now. Updated below is Paul's diff. I also added
> > > > NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
> > > > asymmetric.
> > > 
> > > > +__always_inline void rcu_nmi_exit(void)
> > > >  {
> > > >  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > >  
> > > > @@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
> > > >  	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
> > > >  	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
> > > >  
> > > > -	if (irq)
> > > > +	if (!in_nmi())
> > > >  		rcu_prepare_for_idle();
> > > >  
> > > >  	rcu_dynticks_eqs_enter();
> > > >  
> > > > -	if (irq)
> > > > +	if (!in_nmi())
> > > >  		rcu_dynticks_task_enter();
> > > >  }
> > > 
> > > Boris and me have been going over the #MC code (and finding loads of
> > > 'interesting' code) and ran into ist_enter(), whish has the following
> > > code:
> > > 
> > >                 /*
> > >                  * We might have interrupted pretty much anything.  In
> > >                  * fact, if we're a machine check, we can even interrupt
> > >                  * NMI processing.  We don't want in_nmi() to return true,
> > >                  * but we need to notify RCU.
> > >                  */
> > >                 rcu_nmi_enter();
> > > 
> > > 
> > > Which, to me, sounds all sorts of broken. The IST (be it #DB or #MC) can
> > > happen while we're holding all sorts of locks. This must be an NMI-like
> > > context.
> > 
> > Ouch!  Looks like I need to hold off on getting rid of the "irq"
> > parameters if in_nmi() isn't going to be accurate.
> 
> I'm currently trying to twist my brain around all this, because I
> suspect it's all completely broken one way or another.
> 
> But yes, we definitely need to fix this before your patch goes in.

OK.  I will drop it later today, but leave tag in_nmi.2020.02.18a
pointing at it for future reference.

							Thanx, Paul

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Peter Zijlstra]

On Tue, Feb 18, 2020 at 01:39:25PM -0800, Paul E. McKenney wrote:
> OK.  I will drop it later today, but leave tag in_nmi.2020.02.18a
> pointing at it for future reference.

Thanks, I've picked it up in my series.

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Paul E. McKenney]

On Wed, Feb 19, 2020 at 10:57:43AM +0100, Peter Zijlstra wrote:
> On Tue, Feb 18, 2020 at 01:39:25PM -0800, Paul E. McKenney wrote:
> > OK.  I will drop it later today, but leave tag in_nmi.2020.02.18a
> > pointing at it for future reference.
> 
> Thanks, I've picked it up in my series.

Very good, thank you!

							Thanx, Paul

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Joel Fernandes]

On Wed, Feb 12, 2020 at 10:01:42PM +0100, Peter Zijlstra wrote:
> To facilitate tracers that need RCU, add some helpers to wrap the
> magic required.
> 
> The problem is that we can call into tracers (trace events and
> function tracing) while RCU isn't watching and this can happen from
> any context, including NMI.
> 
> It is this latter that is causing most of the trouble; we must make
> sure in_nmi() returns true before we land in anything tracing,
> otherwise we cannot recover.
> 
> These helpers are macros because of header-hell; they're placed here
> because of the proximity to nmi_{enter,exit{().
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/hardirq.h |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> --- a/include/linux/hardirq.h
> +++ b/include/linux/hardirq.h
> @@ -89,4 +89,52 @@ extern void irq_exit(void);
>  		arch_nmi_exit();				\
>  	} while (0)
>  
> +/*
> + * Tracing vs RCU
> + * --------------
> + *
> + * tracepoints and function-tracing can happen when RCU isn't watching (idle,
> + * or early IRQ/NMI entry).
> + *
> + * When it happens during idle or early during IRQ entry, tracing will have
> + * to inform RCU that it ought to pay attention, this is done by calling
> + * rcu_irq_enter_irqsave().
> + *
> + * On NMI entry, we must be very careful that tracing only happens after we've
> + * incremented preempt_count(), otherwise we cannot tell we're in NMI and take
> + * the special path.
> + */
> +
> +#define __TR_IRQ	1
> +#define __TR_NMI	2
> +
> +#define trace_rcu_enter()					\
> +({								\
> +	unsigned long state = 0;				\
> +	if (!rcu_is_watching())	{				\
> +		if (in_nmi()) {					\
> +			state = __TR_NMI;			\
> +			rcu_nmi_enter();			\
> +		} else {					\
> +			state = __TR_IRQ;			\
> +			rcu_irq_enter_irqsave();		\

Since rcu_irq_enter_irqsave can be called from a tracer context, should those
be marked with notrace as well? AFAICS, there's no notrace marking on them.

thanks,

 - Joel


> +		}						\
> +	}							\
> +	state;							\
> +})
> +
> +#define trace_rcu_exit(state)					\
> +do {								\
> +	switch (state) {					\
> +	case __TR_IRQ:						\
> +		rcu_irq_exit_irqsave();				\
> +		break;						\
> +	case __TR_NMI:						\
> +		rcu_nmi_exit();					\
> +		break;						\
> +	default:						\
> +		break;						\
> +	}							\
> +} while (0)
> +
>  #endif /* LINUX_HARDIRQ_H */
> 
> 

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Peter Zijlstra]

On Wed, Feb 12, 2020 at 06:27:02PM -0500, Joel Fernandes wrote:
> On Wed, Feb 12, 2020 at 10:01:42PM +0100, Peter Zijlstra wrote:

> > +#define trace_rcu_enter()					\
> > +({								\
> > +	unsigned long state = 0;				\
> > +	if (!rcu_is_watching())	{				\
> > +		if (in_nmi()) {					\
> > +			state = __TR_NMI;			\
> > +			rcu_nmi_enter();			\
> > +		} else {					\
> > +			state = __TR_IRQ;			\
> > +			rcu_irq_enter_irqsave();		\
> 
> Since rcu_irq_enter_irqsave can be called from a tracer context, should those
> be marked with notrace as well? AFAICS, there's no notrace marking on them.

It should work, these functions are re-entrant (as are IRQs / NMIs) and
Steve wants to be able to trace RCU itself.

Re: [PATCH v2 3/9] rcu,tracing: Create trace_rcu_{enter,exit}()

[Joel Fernandes]

On Thu, Feb 13, 2020 at 09:28:14AM +0100, Peter Zijlstra wrote:
> On Wed, Feb 12, 2020 at 06:27:02PM -0500, Joel Fernandes wrote:
> > On Wed, Feb 12, 2020 at 10:01:42PM +0100, Peter Zijlstra wrote:
> 
> > > +#define trace_rcu_enter()					\
> > > +({								\
> > > +	unsigned long state = 0;				\
> > > +	if (!rcu_is_watching())	{				\
> > > +		if (in_nmi()) {					\
> > > +			state = __TR_NMI;			\
> > > +			rcu_nmi_enter();			\
> > > +		} else {					\
> > > +			state = __TR_IRQ;			\
> > > +			rcu_irq_enter_irqsave();		\
> > 
> > Since rcu_irq_enter_irqsave can be called from a tracer context, should those
> > be marked with notrace as well? AFAICS, there's no notrace marking on them.
> 
> It should work, these functions are re-entrant (as are IRQs / NMIs) and
> Steve wants to be able to trace RCU itself.

Hoping there are no recursion scenarios possible, but that sounds good to me. thanks,

 - Joel

[PATCH v2 4/9] sched,rcu,tracing: Avoid tracing before in_nmi() is correct

[Peter Zijlstra]

If we call into a tracer before in_nmi() becomes true, the tracer can
no longer detect it is called from NMI context and behave correctly.

Therefore change nmi_{enter,exit}() to use __preempt_count_{add,sub}()
as the normal preempt_count_{add,sub}() have a (desired) function
trace entry.

This fixes a potential issue with current code; AFAICT when the
function-tracer has stack-tracing enabled __trace_stack() will
malfunction when it hits the preempt_count_add() function entry from
NMI context.

Suggested-by: Steven Rostedt (VMware) <rosted@goodmis.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/hardirq.h |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -65,6 +65,15 @@ extern void irq_exit(void);
 #define arch_nmi_exit()		do { } while (0)
 #endif
 
+/*
+ * NMI vs Tracing
+ * --------------
+ *
+ * We must not land in a tracer until (or after) we've changed preempt_count
+ * such that in_nmi() becomes true. To that effect all NMI C entry points must
+ * be marked 'notrace' and call nmi_enter() as soon as possible.
+ */
+
 #define nmi_enter()						\
 	do {							\
 		arch_nmi_enter();				\
@@ -72,7 +81,7 @@ extern void irq_exit(void);
 		lockdep_off();					\
 		ftrace_nmi_enter();				\
 		BUG_ON(in_nmi());				\
-		preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET);	\
+		__preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET); \
 		rcu_nmi_enter();				\
 		trace_hardirq_enter();				\
 	} while (0)
@@ -82,7 +91,7 @@ extern void irq_exit(void);
 		trace_hardirq_exit();				\
 		rcu_nmi_exit();					\
 		BUG_ON(!in_nmi());				\
-		preempt_count_sub(NMI_OFFSET + HARDIRQ_OFFSET);	\
+		__preempt_count_sub(NMI_OFFSET + HARDIRQ_OFFSET); \
 		ftrace_nmi_exit();				\
 		lockdep_on();					\
 		printk_nmi_exit();				\

[PATCH v2 5/9] x86,tracing: Add comments to do_nmi()

[Peter Zijlstra]

Add a few comments to do_nmi() as a result of the audit.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/nmi.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -529,11 +529,14 @@ do_nmi(struct pt_regs *regs, long error_
 	 * continue to use the NMI stack.
 	 */
 	if (unlikely(is_debug_stack(regs->sp))) {
-		debug_stack_set_zero();
+		debug_stack_set_zero(); /* notrace due to Makefile */
 		this_cpu_write(update_debug_stack, 1);
 	}
 #endif
 
+	/*
+	 * It is important that no tracing happens before nmi_enter()!
+	 */
 	nmi_enter();
 
 	inc_irq_stat(__nmi_count);
@@ -542,10 +545,13 @@ do_nmi(struct pt_regs *regs, long error_
 		default_do_nmi(regs);
 
 	nmi_exit();
+	/*
+	 * No tracing after nmi_exit()!
+	 */
 
 #ifdef CONFIG_X86_64
 	if (unlikely(this_cpu_read(update_debug_stack))) {
-		debug_stack_reset();
+		debug_stack_reset(); /* notrace due to Makefile */
 		this_cpu_write(update_debug_stack, 0);
 	}
 #endif

[PATCH v2 6/9] perf,tracing: Prepare the perf-trace interface for RCU changes

[Peter Zijlstra]

The tracepoint interface will stop providing regular RCU context; make
sure we do it ourselves, since perf makes use of regular RCU protected
data.

Suggested-by: Steven Rostedt (VMware) <rosted@goodmis.org>
Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |    5 +++++
 1 file changed, 5 insertions(+)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8950,6 +8950,7 @@ void perf_tp_event(u16 event_type, u64 c
 {
 	struct perf_sample_data data;
 	struct perf_event *event;
+	unsigned long rcu_flags;
 
 	struct perf_raw_record raw = {
 		.frag = {
@@ -8961,6 +8962,8 @@ void perf_tp_event(u16 event_type, u64 c
 	perf_sample_data_init(&data, 0, 0);
 	data.raw = &raw;
 
+	rcu_flags = trace_rcu_enter();
+
 	perf_trace_buf_update(record, event_type);
 
 	hlist_for_each_entry_rcu(event, head, hlist_entry) {
@@ -8996,6 +8999,8 @@ void perf_tp_event(u16 event_type, u64 c
 	}
 
 	perf_swevent_put_recursion_context(rctx);
+
+	trace_rcu_exit(rcu_flags);
 }
 EXPORT_SYMBOL_GPL(perf_tp_event);
 

Re: [PATCH v2 6/9] perf,tracing: Prepare the perf-trace interface for RCU changes

[Joel Fernandes]

On Wed, Feb 12, 2020 at 10:01:45PM +0100, Peter Zijlstra wrote:
> The tracepoint interface will stop providing regular RCU context; make
> sure we do it ourselves, since perf makes use of regular RCU protected
> data.
> 
> Suggested-by: Steven Rostedt (VMware) <rosted@goodmis.org>
> Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/events/core.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8950,6 +8950,7 @@ void perf_tp_event(u16 event_type, u64 c
>  {
>  	struct perf_sample_data data;
>  	struct perf_event *event;
> +	unsigned long rcu_flags;

The flags are not needed I guess, if you agree on not using in_nmi() in
trace_rcu_enter().

thanks,

 - Joel


>  	struct perf_raw_record raw = {
>  		.frag = {
> @@ -8961,6 +8962,8 @@ void perf_tp_event(u16 event_type, u64 c
>  	perf_sample_data_init(&data, 0, 0);
>  	data.raw = &raw;
>  
> +	rcu_flags = trace_rcu_enter();
> +
>  	perf_trace_buf_update(record, event_type);
>  
>  	hlist_for_each_entry_rcu(event, head, hlist_entry) {
> @@ -8996,6 +8999,8 @@ void perf_tp_event(u16 event_type, u64 c
>  	}
>  
>  	perf_swevent_put_recursion_context(rctx);
> +
> +	trace_rcu_exit(rcu_flags);
>  }
>  EXPORT_SYMBOL_GPL(perf_tp_event);
>  
> 
> 

Re: [PATCH v2 6/9] perf,tracing: Prepare the perf-trace interface for RCU changes

[Peter Zijlstra]

On Wed, Feb 12, 2020 at 06:28:30PM -0500, Joel Fernandes wrote:
> On Wed, Feb 12, 2020 at 10:01:45PM +0100, Peter Zijlstra wrote:
> > The tracepoint interface will stop providing regular RCU context; make
> > sure we do it ourselves, since perf makes use of regular RCU protected
> > data.
> > 
> > Suggested-by: Steven Rostedt (VMware) <rosted@goodmis.org>
> > Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  kernel/events/core.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -8950,6 +8950,7 @@ void perf_tp_event(u16 event_type, u64 c
> >  {
> >  	struct perf_sample_data data;
> >  	struct perf_event *event;
> > +	unsigned long rcu_flags;
> 
> The flags are not needed I guess, if you agree on not using in_nmi() in
> trace_rcu_enter().

Even then we need to store the state: 'didn't do nothing' vs 'did call
rcu_needs_to_wake_up_and_pay_attention_noaw'. That is, we only need to
do something (expensive!) when !rcu_is_watching().

Re: [PATCH v2 6/9] perf,tracing: Prepare the perf-trace interface for RCU changes

[Joel Fernandes]

On Thu, Feb 13, 2020 at 09:29:51AM +0100, Peter Zijlstra wrote:
> On Wed, Feb 12, 2020 at 06:28:30PM -0500, Joel Fernandes wrote:
> > On Wed, Feb 12, 2020 at 10:01:45PM +0100, Peter Zijlstra wrote:
> > > The tracepoint interface will stop providing regular RCU context; make
> > > sure we do it ourselves, since perf makes use of regular RCU protected
> > > data.
> > > 
> > > Suggested-by: Steven Rostedt (VMware) <rosted@goodmis.org>
> > > Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > >  kernel/events/core.c |    5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > --- a/kernel/events/core.c
> > > +++ b/kernel/events/core.c
> > > @@ -8950,6 +8950,7 @@ void perf_tp_event(u16 event_type, u64 c
> > >  {
> > >  	struct perf_sample_data data;
> > >  	struct perf_event *event;
> > > +	unsigned long rcu_flags;
> > 
> > The flags are not needed I guess, if you agree on not using in_nmi() in
> > trace_rcu_enter().
> 
> Even then we need to store the state: 'didn't do nothing' vs 'did call
> rcu_needs_to_wake_up_and_pay_attention_noaw'. That is, we only need to
> do something (expensive!) when !rcu_is_watching().

You are right, that sounds good. I was talking to Paul and we chatted that if
in_nmi() is safe (which I believe it is as we are not calling RCU before you
update the preempt counts), then in RCU we can replace the @irq with
!in_nmi() and simplify that code.  Then we can simplify this bit as well
(keep rcu_flags but only call rcu_irq_enter_irqsave() instead of
rcu_nmi_enter(). May be you can do the RCU internal bits in your v3 or should
those be separate patches? Whatever Paul and you want to do.

thanks,

 - Joel

 - Joel

[PATCH v2 7/9] tracing: Employ trace_rcu_{enter,exit}()

[Peter Zijlstra]

Replace the opencoded (and incomplete) RCU manipulations with the new
helpers to ensure a regular RCU context when calling into
__ftrace_trace_stack().

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/trace/trace.c |   19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2989,24 +2989,11 @@ void __trace_stack(struct trace_array *t
 		   int pc)
 {
 	struct trace_buffer *buffer = tr->array_buffer.buffer;
+	unsigned long rcu_flags;
 
-	if (rcu_is_watching()) {
-		__ftrace_trace_stack(buffer, flags, skip, pc, NULL);
-		return;
-	}
-
-	/*
-	 * When an NMI triggers, RCU is enabled via rcu_nmi_enter(),
-	 * but if the above rcu_is_watching() failed, then the NMI
-	 * triggered someplace critical, and rcu_irq_enter() should
-	 * not be called from NMI.
-	 */
-	if (unlikely(in_nmi()))
-		return;
-
-	rcu_irq_enter_irqsave();
+	rcu_flags = trace_rcu_enter();
 	__ftrace_trace_stack(buffer, flags, skip, pc, NULL);
-	rcu_irq_exit_irqsave();
+	trace_rcu_exit(rcu_flags);
 }
 
 /**

[PATCH v2 8/9] tracing: Remove regular RCU context for _rcuidle tracepoints (again)

[Peter Zijlstra]

Effectively revert commit 865e63b04e9b2 ("tracing: Add back in
rcu_irq_enter/exit_irqson() for rcuidle tracepoints") now that we've
taught perf how to deal with not having an RCU context provided.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/tracepoint.h |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -179,10 +179,8 @@ static inline struct tracepoint *tracepo
 		 * For rcuidle callers, use srcu since sched-rcu	\
 		 * doesn't work from the idle path.			\
 		 */							\
-		if (rcuidle) {						\
+		if (rcuidle)						\
 			__idx = srcu_read_lock_notrace(&tracepoint_srcu);\
-			rcu_irq_enter_irqsave();			\
-		}							\
 									\
 		it_func_ptr = rcu_dereference_raw((tp)->funcs);		\
 									\
@@ -194,10 +192,8 @@ static inline struct tracepoint *tracepo
 			} while ((++it_func_ptr)->func);		\
 		}							\
 									\
-		if (rcuidle) {						\
-			rcu_irq_exit_irqsave();				\
+		if (rcuidle)						\
 			srcu_read_unlock_notrace(&tracepoint_srcu, __idx);\
-		}							\
 									\
 		preempt_enable_notrace();				\
 	} while (0)

[PATCH v2 9/9] perf,tracing: Allow function tracing when !RCU

[Peter Zijlstra]

Since perf is now able to deal with !rcu_is_watching() contexts,
remove the restraint.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/trace/trace_event_perf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -477,7 +477,7 @@ static int perf_ftrace_function_register
 {
 	struct ftrace_ops *ops = &event->ftrace_ops;
 
-	ops->flags   = FTRACE_OPS_FL_RCU;
+	ops->flags   = 0;
 	ops->func    = perf_ftrace_function_call;
 	ops->private = (void *)(unsigned long)nr_cpu_ids;
 

Re: [PATCH v2 9/9] perf,tracing: Allow function tracing when !RCU

[Sergey Senozhatsky]

On (20/02/12 22:01), Peter Zijlstra wrote:
> Since perf is now able to deal with !rcu_is_watching() contexts,
> remove the restraint.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/trace/trace_event_perf.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -477,7 +477,7 @@ static int perf_ftrace_function_register
>  {
>  	struct ftrace_ops *ops = &event->ftrace_ops;
>  
> -	ops->flags   = FTRACE_OPS_FL_RCU;
> +	ops->flags   = 0;

FTRACE_OPS_FL_ENABLED?

	-ss

Re: [PATCH v2 9/9] perf,tracing: Allow function tracing when !RCU

[Sergey Senozhatsky]

On (20/02/14 11:28), Sergey Senozhatsky wrote:
> On (20/02/12 22:01), Peter Zijlstra wrote:
> > Since perf is now able to deal with !rcu_is_watching() contexts,
> > remove the restraint.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  kernel/trace/trace_event_perf.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- a/kernel/trace/trace_event_perf.c
> > +++ b/kernel/trace/trace_event_perf.c
> > @@ -477,7 +477,7 @@ static int perf_ftrace_function_register
> >  {
> >  	struct ftrace_ops *ops = &event->ftrace_ops;
> >  
> > -	ops->flags   = FTRACE_OPS_FL_RCU;
> > +	ops->flags   = 0;
> 
> FTRACE_OPS_FL_ENABLED?

No, never mind.

	-ss

Re: [PATCH v2 9/9] perf,tracing: Allow function tracing when !RCU

[Steven Rostedt]

On Fri, 14 Feb 2020 11:42:45 +0900
Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:

> > > +++ b/kernel/trace/trace_event_perf.c
> > > @@ -477,7 +477,7 @@ static int perf_ftrace_function_register
> > >  {
> > >  	struct ftrace_ops *ops = &event->ftrace_ops;
> > >  
> > > -	ops->flags   = FTRACE_OPS_FL_RCU;
> > > +	ops->flags   = 0;  
> > 
> > FTRACE_OPS_FL_ENABLED?  
> 
> No, never mind.

:-)

-- Steve

Re: [PATCH v2 9/9] perf,tracing: Allow function tracing when !RCU

[Kim Phillips]

On 2/12/20 3:01 PM, Peter Zijlstra wrote:
> Since perf is now able to deal with !rcu_is_watching() contexts,
> remove the restraint.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/trace/trace_event_perf.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -477,7 +477,7 @@ static int perf_ftrace_function_register
>  {
>  	struct ftrace_ops *ops = &event->ftrace_ops;
>  
> -	ops->flags   = FTRACE_OPS_FL_RCU;
> +	ops->flags   = 0;
>  	ops->func    = perf_ftrace_function_call;
>  	ops->private = (void *)(unsigned long)nr_cpu_ids;

If this is the last user of the flag, should all remaining
FTRACE_OPS_FL_RCU references be removed, too?

Thanks,

Kim

Re: [PATCH v2 9/9] perf,tracing: Allow function tracing when !RCU

[Steven Rostedt]

On Fri, 14 Feb 2020 14:38:14 -0600
Kim Phillips <kim.phillips@amd.com> wrote:

> On 2/12/20 3:01 PM, Peter Zijlstra wrote:
> > Since perf is now able to deal with !rcu_is_watching() contexts,
> > remove the restraint.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  kernel/trace/trace_event_perf.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- a/kernel/trace/trace_event_perf.c
> > +++ b/kernel/trace/trace_event_perf.c
> > @@ -477,7 +477,7 @@ static int perf_ftrace_function_register
> >  {
> >  	struct ftrace_ops *ops = &event->ftrace_ops;
> >  
> > -	ops->flags   = FTRACE_OPS_FL_RCU;
> > +	ops->flags   = 0;
> >  	ops->func    = perf_ftrace_function_call;
> >  	ops->private = (void *)(unsigned long)nr_cpu_ids;  
> 
> If this is the last user of the flag, should all remaining
> FTRACE_OPS_FL_RCU references be removed, too?

Let's wait till Peter's patches goes through a merge cycle or two. I
want to make sure there's no other RCU issues that pop up before we
remove this infrastructure.

-- Steve