[PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Steven Rostedt]

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

Commit e6753f23d961d ("tracepoint: Make rcuidle tracepoint callers use
SRCU") removed the calls to rcu_irq_enter/exit_irqson() and replaced it with
srcu callbacks as that much faster for the rcuidle cases. But this caused an
issue with perf, because perf only uses rcu to synchronize its trace point
callback routines.

The issue was that if perf traced one of the "rcuidle" paths, that path no
longer enabled RCU if it was not watching, and this caused lockdep to
complain when the perf code used rcu_read_lock() and RCU was not "watching".

Commit 865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for
rcuidle tracepoints") added back the rcu_irq_enter/exit_irqson() code, but
this made the srcu changes no longer applicable.

As perf is the only callback that needs the heavier weight
"rcu_irq_enter/exit_irqson()" calls, move it to the perf specific code and
not bog down those that do not require it.

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

  - Moved the rcu_is_watching logic to perf_tp_event() and remove the
    exporting of rcu_irq_enter/exit_irqson().

 include/linux/tracepoint.h |  8 ++------
 kernel/events/core.c       | 17 ++++++++++++++++-
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 1fb11daa5c53..a83fd076a312 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -179,10 +179,8 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 		 * 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_irqson();				\
-		}							\
 									\
 		it_func_ptr = rcu_dereference_raw((tp)->funcs);		\
 									\
@@ -194,10 +192,8 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 			} while ((++it_func_ptr)->func);		\
 		}							\
 									\
-		if (rcuidle) {						\
-			rcu_irq_exit_irqson();				\
+		if (rcuidle)						\
 			srcu_read_unlock_notrace(&tracepoint_srcu, __idx);\
-		}							\
 									\
 		preempt_enable_notrace();				\
 	} while (0)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 455451d24b4a..0abbf5e2ee62 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8941,6 +8941,7 @@ void perf_tp_event(u16 event_type, u64 count, void *record, int entry_size,
 {
 	struct perf_sample_data data;
 	struct perf_event *event;
+	bool rcu_watching = rcu_is_watching();
 
 	struct perf_raw_record raw = {
 		.frag = {
@@ -8949,6 +8950,17 @@ void perf_tp_event(u16 event_type, u64 count, void *record, int entry_size,
 		},
 	};
 
+	if (!rcu_watching) {
+		/*
+		 * If nmi_enter() is traced, it is possible that
+		 * RCU may not be watching "yet", and this is called.
+		 * We can not call rcu_irq_enter_irqson() in this case.
+		 */
+		if (unlikely(in_nmi()))
+			goto out;
+		rcu_irq_enter_irqson();
+	}
+
 	perf_sample_data_init(&data, 0, 0);
 	data.raw = &raw;
 
@@ -8985,8 +8997,11 @@ void perf_tp_event(u16 event_type, u64 count, void *record, int entry_size,
 unlock:
 		rcu_read_unlock();
 	}
-
+	if (!rcu_watching)
+		rcu_irq_exit_irqson();
+out:
 	perf_swevent_put_recursion_context(rctx);
+
 }
 EXPORT_SYMBOL_GPL(perf_tp_event);
 
-- 
2.20.1

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Mathieu Desnoyers]

----- On Feb 11, 2020, at 9:50 AM, rostedt rostedt@goodmis.org wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Commit e6753f23d961d ("tracepoint: Make rcuidle tracepoint callers use
> SRCU") removed the calls to rcu_irq_enter/exit_irqson() and replaced it with
> srcu callbacks as that much faster for the rcuidle cases. But this caused an
> issue with perf, because perf only uses rcu to synchronize its trace point
> callback routines.
> 
> The issue was that if perf traced one of the "rcuidle" paths, that path no
> longer enabled RCU if it was not watching, and this caused lockdep to
> complain when the perf code used rcu_read_lock() and RCU was not "watching".
> 
> Commit 865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for
> rcuidle tracepoints") added back the rcu_irq_enter/exit_irqson() code, but
> this made the srcu changes no longer applicable.
> 
> As perf is the only callback that needs the heavier weight
> "rcu_irq_enter/exit_irqson()" calls, move it to the perf specific code and
> not bog down those that do not require it.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> Changes since v1:
> 
>  - Moved the rcu_is_watching logic to perf_tp_event() and remove the
>    exporting of rcu_irq_enter/exit_irqson().
> 
> include/linux/tracepoint.h |  8 ++------
> kernel/events/core.c       | 17 ++++++++++++++++-
> 2 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 1fb11daa5c53..a83fd076a312 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -179,10 +179,8 @@ static inline struct tracepoint
> *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> 		 * 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_irqson();				\
> -		}							\
> 									\
> 		it_func_ptr = rcu_dereference_raw((tp)->funcs);		\
> 									\
> @@ -194,10 +192,8 @@ static inline struct tracepoint
> *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> 			} while ((++it_func_ptr)->func);		\
> 		}							\
> 									\
> -		if (rcuidle) {						\
> -			rcu_irq_exit_irqson();				\
> +		if (rcuidle)						\
> 			srcu_read_unlock_notrace(&tracepoint_srcu, __idx);\
> -		}							\
> 									\
> 		preempt_enable_notrace();				\
> 	} while (0)
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 455451d24b4a..0abbf5e2ee62 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8941,6 +8941,7 @@ void perf_tp_event(u16 event_type, u64 count, void
> *record, int entry_size,
> {
> 	struct perf_sample_data data;
> 	struct perf_event *event;
> +	bool rcu_watching = rcu_is_watching();
> 
> 	struct perf_raw_record raw = {
> 		.frag = {
> @@ -8949,6 +8950,17 @@ void perf_tp_event(u16 event_type, u64 count, void
> *record, int entry_size,
> 		},
> 	};
> 
> +	if (!rcu_watching) {
> +		/*
> +		 * If nmi_enter() is traced, it is possible that
> +		 * RCU may not be watching "yet", and this is called.
> +		 * We can not call rcu_irq_enter_irqson() in this case.
> +		 */
> +		if (unlikely(in_nmi()))
> +			goto out;
> +		rcu_irq_enter_irqson();
> +	}
> +
> 	perf_sample_data_init(&data, 0, 0);
> 	data.raw = &raw;

I'm puzzled by this function. It does:

perf_tp_event(...)
{
     hlist_for_each_entry_rcu(event, head, hlist_entry) {
         ...
     }
     if (task && task != current) {
         rcu_read_lock();
         ... = rcu_dereference();
         list_for_each_entry_rcu(...) {
             ....
         }
         rcu_read_unlock();
     }
}

What is the purpose of the rcu_read_lock/unlock within the if (),
considering that there is already an hlist rcu iteration just before ?
It seems to assume that a RCU read-side of some kind of already
ongoing.

Thanks,

Mathieu


> 
> @@ -8985,8 +8997,11 @@ void perf_tp_event(u16 event_type, u64 count, void
> *record, int entry_size,
> unlock:
> 		rcu_read_unlock();
> 	}
> -
> +	if (!rcu_watching)
> +		rcu_irq_exit_irqson();
> +out:
> 	perf_swevent_put_recursion_context(rctx);
> +
> }
> EXPORT_SYMBOL_GPL(perf_tp_event);
> 
> --
> 2.20.1

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

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Peter Zijlstra]

On Tue, Feb 11, 2020 at 09:50:47AM -0500, Steven Rostedt wrote:
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 455451d24b4a..0abbf5e2ee62 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8941,6 +8941,7 @@ void perf_tp_event(u16 event_type, u64 count, void *record, int entry_size,
>  {
>  	struct perf_sample_data data;
>  	struct perf_event *event;
> +	bool rcu_watching = rcu_is_watching();
>  
>  	struct perf_raw_record raw = {
>  		.frag = {
> @@ -8949,6 +8950,17 @@ void perf_tp_event(u16 event_type, u64 count, void *record, int entry_size,
>  		},
>  	};
>  
> +	if (!rcu_watching) {
> +		/*
> +		 * If nmi_enter() is traced, it is possible that
> +		 * RCU may not be watching "yet", and this is called.
> +		 * We can not call rcu_irq_enter_irqson() in this case.
> +		 */
> +		if (unlikely(in_nmi()))
> +			goto out;

unless I'm mistaken, we can simply do rcu_nmi_enter() in this case, and
rcu_nmi_exit() on the other end.

> +		rcu_irq_enter_irqson();
> +	}
> +
>  	perf_sample_data_init(&data, 0, 0);
>  	data.raw = &raw;
>  
> @@ -8985,8 +8997,11 @@ void perf_tp_event(u16 event_type, u64 count, void *record, int entry_size,
>  unlock:
>  		rcu_read_unlock();
>  	}
> -
> +	if (!rcu_watching)
> +		rcu_irq_exit_irqson();
> +out:
>  	perf_swevent_put_recursion_context(rctx);
> +
>  }

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Peter Zijlstra]

On Tue, Feb 11, 2020 at 10:34:38AM -0500, Mathieu Desnoyers wrote:
> 
> I'm puzzled by this function. It does:
> 
> perf_tp_event(...)
> {
>      hlist_for_each_entry_rcu(event, head, hlist_entry) {
>          ...
>      }
>      if (task && task != current) {
>          rcu_read_lock();
>          ... = rcu_dereference();
>          list_for_each_entry_rcu(...) {
>              ....
>          }
>          rcu_read_unlock();
>      }
> }
> 
> What is the purpose of the rcu_read_lock/unlock within the if (),
> considering that there is already an hlist rcu iteration just before ?
> It seems to assume that a RCU read-side of some kind of already
> ongoing.

IIRC the hlist_for_each_entry_rcu() uses the RCU stuff from the
tracepoint API, while the stuff inside the if() uses regular RCU.

Them were note the same one -- tracepoints used rcu-sched, perf used
rcu.

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Mathieu Desnoyers]

----- On Feb 11, 2020, at 10:46 AM, Peter Zijlstra peterz@infradead.org wrote:

> On Tue, Feb 11, 2020 at 10:34:38AM -0500, Mathieu Desnoyers wrote:
>> 
>> I'm puzzled by this function. It does:
>> 
>> perf_tp_event(...)
>> {
>>      hlist_for_each_entry_rcu(event, head, hlist_entry) {
>>          ...
>>      }
>>      if (task && task != current) {
>>          rcu_read_lock();
>>          ... = rcu_dereference();
>>          list_for_each_entry_rcu(...) {
>>              ....
>>          }
>>          rcu_read_unlock();
>>      }
>> }
>> 
>> What is the purpose of the rcu_read_lock/unlock within the if (),
>> considering that there is already an hlist rcu iteration just before ?
>> It seems to assume that a RCU read-side of some kind of already
>> ongoing.
> 
> IIRC the hlist_for_each_entry_rcu() uses the RCU stuff from the
> tracepoint API, while the stuff inside the if() uses regular RCU.
> 
> Them were note the same one -- tracepoints used rcu-sched, perf used
> rcu.

Indeed, there is a call to tracepoint_synchronize_unregister() within
perf_trace_event_unreg(), which provides the required grace period
before freeing the perf event.

That tracepoint_synchronize_unregister() was initially doing a synchronize_sched()
as you point out. It then moved to synchronize_rcu() with the RCU flavors
consolidation, and we've added the synchronize_srcu(&tracepoint_srcu) as well,
which handles the rcuidle cases.

Adding a comment in perf_tp_event() detailing how each RCU use is synchronized
might help readability, e.g.:

At top of function:

/*
 * Synchronization of the perf event RCU hlist is performed by the tracepoint API.
 * Synchronization of the perf event context and perf event context event list
 * is performed through explicit use of RCU.
 */

Thanks,

Mathieu

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

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Steven Rostedt]

On Tue, 11 Feb 2020 16:34:52 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> > +		if (unlikely(in_nmi()))
> > +			goto out;  
> 
> unless I'm mistaken, we can simply do rcu_nmi_enter() in this case, and
> rcu_nmi_exit() on the other end.
> 
> > +		rcu_irq_enter_irqson();

The thing is, I don't think this can ever happen. We've had in the
tracepoint.h:

		/* srcu can't be used from NMI */			\
		WARN_ON_ONCE(rcuidle && in_nmi());			\

And this has yet to trigger.

-- Steve

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Mathieu Desnoyers]

----- On Feb 11, 2020, at 11:18 AM, rostedt rostedt@goodmis.org wrote:

> On Tue, 11 Feb 2020 16:34:52 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
>> > +		if (unlikely(in_nmi()))
>> > +			goto out;
>> 
>> unless I'm mistaken, we can simply do rcu_nmi_enter() in this case, and
>> rcu_nmi_exit() on the other end.
>> 
>> > +		rcu_irq_enter_irqson();
> 
> The thing is, I don't think this can ever happen. We've had in the
> tracepoint.h:
> 
>		/* srcu can't be used from NMI */			\
>		WARN_ON_ONCE(rcuidle && in_nmi());			\
> 
> And this has yet to trigger.

But that "rcuidle" state is defined on a per-tracepoint basis, whereas
"!rcu_is_watching()" is a state which depends on the current execution
context. I don't follow how the fact that this WARN_ON_ONCE() never
triggered allows us to infer anything about (!rcu_is_watching() && in_nmi()).

Thanks,

Mathieu

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

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Steven Rostedt]

On Tue, 11 Feb 2020 11:27:36 -0500 (EST)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> ----- On Feb 11, 2020, at 11:18 AM, rostedt rostedt@goodmis.org wrote:
> 
> > On Tue, 11 Feb 2020 16:34:52 +0100
> > Peter Zijlstra <peterz@infradead.org> wrote:
> >   
> >> > +		if (unlikely(in_nmi()))
> >> > +			goto out;  
> >> 
> >> unless I'm mistaken, we can simply do rcu_nmi_enter() in this case, and
> >> rcu_nmi_exit() on the other end.
> >>   
> >> > +		rcu_irq_enter_irqson();  
> > 
> > The thing is, I don't think this can ever happen. We've had in the
> > tracepoint.h:
> > 
> >		/* srcu can't be used from NMI */			\
> >		WARN_ON_ONCE(rcuidle && in_nmi());			\
> > 
> > And this has yet to trigger.  
> 
> But that "rcuidle" state is defined on a per-tracepoint basis, whereas
> "!rcu_is_watching()" is a state which depends on the current execution
> context. I don't follow how the fact that this WARN_ON_ONCE() never
> triggered allows us to infer anything about (!rcu_is_watching() && in_nmi()).
>

The "_rcuidle()" version of the tracepoint was to be used in places
that RCU may not be watching, otherwise you would get a lockdep splat.

As that "rcuidle" variable is a hardcoded constant, it would be
compiled out when rcuidle is zero. But, in all purposes, rcuidle is
basically equivalent to rcu_is_watching(), because if it wasn't you
would have lockdep splats.

-- Steve

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Peter Zijlstra]

On Tue, Feb 11, 2020 at 11:18:28AM -0500, Steven Rostedt wrote:
> On Tue, 11 Feb 2020 16:34:52 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > +		if (unlikely(in_nmi()))
> > > +			goto out;  
> > 
> > unless I'm mistaken, we can simply do rcu_nmi_enter() in this case, and
> > rcu_nmi_exit() on the other end.
> > 
> > > +		rcu_irq_enter_irqson();
> 
> The thing is, I don't think this can ever happen. 

Per your own argument it should be true in the trace_preempt_off()
tracepoint from nmi_enter():

  <idle>
    // rcu_is_watching() := false
    <NMI>
      nmi_enter()
        preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET)
	  __preempt_count_add()
	  // in_nmi() := true
	  preempt_latency_start()
	    // .oO -- we'll never hit this tracepoint because idle has
	    // preempt_count() == 1, so we'll have:
	    //   preempt_count() != val

	    // .oO-2 we should be able to this this when the NMI
	    // hits userspace and we have NOHZ_FULL on.
	rcu_nmi_enter() // function tracer


But the point is, if we ever do hit it, rcu_nmi_enter() is the right
thing to call when '!rcu_is_watching() && in_nmi()', because, as per the
rcu_nmi_enter() comments, that function fully supports nested NMIs.

How about something like so? And then you get to use the same in
__trace_stack() or something, and anywhere else you're doing this dance.

---

diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
index da0af631ded5..e987236abf95 100644
--- 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_is_watching()
+ * ----------------------------
+ *
+ * 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_irqon().
+ *
+ * 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_irqson();			\
+		}						\
+	}							\
+	state;							\
+})
+
+#define trace_rcu_exit(state)					\
+do {								\
+	switch (state) {					\
+	case __TR_IRQ:						\
+		rcu_irq_exit_irqson();				\
+		break;						\
+	case __IRQ_NMI:						\
+		rcu_nmi_exit();					\
+		break;						\
+	default:						\
+		break;						\
+	}							\
+} while (0)
+
 #endif /* LINUX_HARDIRQ_H */
diff --git a/kernel/events/core.c b/kernel/events/core.c
index e453589da97c..8f34592a1355 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8950,6 +8950,7 @@ void perf_tp_event(u16 event_type, u64 count, void *record, int entry_size,
 {
 	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 count, void *record, int entry_size,
 	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 count, void *record, int entry_size,
 	}
 
 	perf_swevent_put_recursion_context(rctx);
+
+	trace_rcu_exit(rcu_flags);
 }
 EXPORT_SYMBOL_GPL(perf_tp_event);
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 45f79bcc3146..62f87807bbe6 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3781,7 +3781,7 @@ static inline void preempt_latency_start(int val)
 	}
 }
 
-void preempt_count_add(int val)
+void notrace preempt_count_add(int val)
 {
 #ifdef CONFIG_DEBUG_PREEMPT
 	/*
@@ -3813,7 +3813,7 @@ static inline void preempt_latency_stop(int val)
 		trace_preempt_on(CALLER_ADDR0, get_lock_parent_ip());
 }
 
-void preempt_count_sub(int val)
+void notrace preempt_count_sub(int val)
 {
 #ifdef CONFIG_DEBUG_PREEMPT
 	/*

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Peter Zijlstra]

On Tue, Feb 11, 2020 at 06:29:52PM +0100, Peter Zijlstra wrote:
> +#define trace_rcu_enter()					\
> +({								\
> +	unsigned long state = 0;				\
> +	if (!rcu_is_watching())	{				\

Also, afaict rcu_is_watching() itself needs more love, the functio has
notrace, but calls other stuff that does not have notrace or inline.

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Mathieu Desnoyers]

----- On Feb 11, 2020, at 12:29 PM, Peter Zijlstra peterz@infradead.org wrote:

> On Tue, Feb 11, 2020 at 11:18:28AM -0500, Steven Rostedt wrote:
>> On Tue, 11 Feb 2020 16:34:52 +0100
>> Peter Zijlstra <peterz@infradead.org> wrote:
>> 
>> > > +		if (unlikely(in_nmi()))
>> > > +			goto out;
>> > 
>> > unless I'm mistaken, we can simply do rcu_nmi_enter() in this case, and
>> > rcu_nmi_exit() on the other end.
>> > 
>> > > +		rcu_irq_enter_irqson();
>> 
>> The thing is, I don't think this can ever happen.
> 
> Per your own argument it should be true in the trace_preempt_off()
> tracepoint from nmi_enter():
> 
>  <idle>
>    // rcu_is_watching() := false
>    <NMI>
>      nmi_enter()
>        preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET)
>	  __preempt_count_add()
>	  // in_nmi() := true
>	  preempt_latency_start()
>	    // .oO -- we'll never hit this tracepoint because idle has
>	    // preempt_count() == 1, so we'll have:
>	    //   preempt_count() != val
> 
>	    // .oO-2 we should be able to this this when the NMI
>	    // hits userspace and we have NOHZ_FULL on.
>	rcu_nmi_enter() // function tracer
> 
> 
> But the point is, if we ever do hit it, rcu_nmi_enter() is the right
> thing to call when '!rcu_is_watching() && in_nmi()', because, as per the
> rcu_nmi_enter() comments, that function fully supports nested NMIs.
> 
> How about something like so? And then you get to use the same in
> __trace_stack() or something, and anywhere else you're doing this dance.
> 
> ---
> 
> diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
> index da0af631ded5..e987236abf95 100644
> --- 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_is_watching()
> + * ----------------------------
> + *
> + * 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_irqon().
> + *
> + * 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

Minor nits:

Why not make these an enum ?

> +
> +#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_irqson();			\
> +		}						\
> +	}							\
> +	state;							\
> +})
> +
> +#define trace_rcu_exit(state)					\
> +do {								\
> +	switch (state) {					\
> +	case __TR_IRQ:						\
> +		rcu_irq_exit_irqson();				\
> +		break;						\
> +	case __IRQ_NMI:						\
> +		rcu_nmi_exit();					\
> +		break;						\
> +	default:						\
> +		break;						\
> +	}							\
> +} while (0)

And convert these into static inline functions ?

Thanks,

Mathieu

> +
> #endif /* LINUX_HARDIRQ_H */
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index e453589da97c..8f34592a1355 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8950,6 +8950,7 @@ void perf_tp_event(u16 event_type, u64 count, void
> *record, int entry_size,
> {
> 	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 count, void
> *record, int entry_size,
> 	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 count, void
> *record, int entry_size,
> 	}
> 
> 	perf_swevent_put_recursion_context(rctx);
> +
> +	trace_rcu_exit(rcu_flags);
> }
> EXPORT_SYMBOL_GPL(perf_tp_event);
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 45f79bcc3146..62f87807bbe6 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3781,7 +3781,7 @@ static inline void preempt_latency_start(int val)
> 	}
> }
> 
> -void preempt_count_add(int val)
> +void notrace preempt_count_add(int val)
> {
> #ifdef CONFIG_DEBUG_PREEMPT
> 	/*
> @@ -3813,7 +3813,7 @@ static inline void preempt_latency_stop(int val)
> 		trace_preempt_on(CALLER_ADDR0, get_lock_parent_ip());
> }
> 
> -void preempt_count_sub(int val)
> +void notrace preempt_count_sub(int val)
> {
> #ifdef CONFIG_DEBUG_PREEMPT
>  	/*

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

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Paul E. McKenney]

On Tue, Feb 11, 2020 at 06:32:13PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 11, 2020 at 06:29:52PM +0100, Peter Zijlstra wrote:
> > +#define trace_rcu_enter()					\
> > +({								\
> > +	unsigned long state = 0;				\
> > +	if (!rcu_is_watching())	{				\
> 
> Also, afaict rcu_is_watching() itself needs more love, the functio has
> notrace, but calls other stuff that does not have notrace or inline.

Good catch!  Like this?

							Thanx, Paul

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

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 1f5fdf7..51616e72 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -295,7 +295,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 bool notrace rcu_dynticks_curr_cpu_in_eqs(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Peter Zijlstra]

On Tue, Feb 11, 2020 at 12:35:21PM -0500, Mathieu Desnoyers wrote:

> Minor nits:
> 
> Why not make these an enum ?
> 
> > +
> > +#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_irqson();			\
> > +		}						\
> > +	}							\
> > +	state;							\
> > +})
> > +
> > +#define trace_rcu_exit(state)					\
> > +do {								\
> > +	switch (state) {					\
> > +	case __TR_IRQ:						\
> > +		rcu_irq_exit_irqson();				\
> > +		break;						\
> > +	case __IRQ_NMI:						\
> > +		rcu_nmi_exit();					\
> > +		break;						\
> > +	default:						\
> > +		break;						\
> > +	}							\
> > +} while (0)
> 
> And convert these into static inline functions ?

Probably the same reason the rest of the file is macros; header hell.

Now, I could probably make these inlines, but then I'd have to add more
RCU function declariations to this file (which is outside of
rcupdate.h). Do we want to do that?

The reason these were in this file is because I want to keep the comment
and implementation near to nmi_enter/exit.

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Peter Zijlstra]

On Tue, Feb 11, 2020 at 10:54:57AM -0800, Paul E. McKenney wrote:
> On Tue, Feb 11, 2020 at 06:32:13PM +0100, Peter Zijlstra wrote:
> > On Tue, Feb 11, 2020 at 06:29:52PM +0100, Peter Zijlstra wrote:
> > > +#define trace_rcu_enter()					\
> > > +({								\
> > > +	unsigned long state = 0;				\
> > > +	if (!rcu_is_watching())	{				\
> > 
> > Also, afaict rcu_is_watching() itself needs more love, the functio has
> > notrace, but calls other stuff that does not have notrace or inline.
> 
> Good catch!  Like this?
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 1f5fdf7..51616e72 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -295,7 +295,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 bool notrace rcu_dynticks_curr_cpu_in_eqs(void)

Right, except that, given the size of the thing, I'd opt for 'inline'
over 'notrace'.

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Paul E. McKenney]

On Wed, Feb 12, 2020 at 09:05:22AM +0100, Peter Zijlstra wrote:
> On Tue, Feb 11, 2020 at 10:54:57AM -0800, Paul E. McKenney wrote:
> > On Tue, Feb 11, 2020 at 06:32:13PM +0100, Peter Zijlstra wrote:
> > > On Tue, Feb 11, 2020 at 06:29:52PM +0100, Peter Zijlstra wrote:
> > > > +#define trace_rcu_enter()					\
> > > > +({								\
> > > > +	unsigned long state = 0;				\
> > > > +	if (!rcu_is_watching())	{				\
> > > 
> > > Also, afaict rcu_is_watching() itself needs more love, the functio has
> > > notrace, but calls other stuff that does not have notrace or inline.
> > 
> > Good catch!  Like this?
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 1f5fdf7..51616e72 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -295,7 +295,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 bool notrace rcu_dynticks_curr_cpu_in_eqs(void)
> 
> Right, except that, given the size of the thing, I'd opt for 'inline'
> over 'notrace'.

Like this, then?

							Thanx, Paul

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

commit 164a7e5204cf32d22d0e444efd97ac7f2243b8d2
Author: Paul E. McKenney <paulmck@kernel.org>
Date:   Tue Feb 11 11:10:00 2020 -0800

    rcu: Make rcu_dynticks_curr_cpu_in_eqs() be inline
    
    The rcu_is_watching() function is and must be notrace in order to allow
    it to be used by the tracing infrastructure.  However, it also invokes
    rcu_dynticks_curr_cpu_in_eqs(), which therefore needs to be either
    notrace or inline.  But isn't.
    
    This commit therefore adds "inline" to the rcu_dynticks_curr_cpu_in_eqs()
    definition.
    
    Reported-by: Peter Zijlstra <peterz@infradead.org>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 1f5fdf7..16fd113 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -295,7 +295,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 bool inline rcu_dynticks_curr_cpu_in_eqs(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 

Re: [PATCH v2] tracing/perf: Move rcu_irq_enter/exit_irqson() to perf trace point hook

[Mathieu Desnoyers]

----- On Feb 12, 2020, at 3:02 AM, Peter Zijlstra peterz@infradead.org wrote:

> On Tue, Feb 11, 2020 at 12:35:21PM -0500, Mathieu Desnoyers wrote:
> 
>> Minor nits:
>> 
>> Why not make these an enum ?
>> 
>> > +
>> > +#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_irqson();			\
>> > +		}						\
>> > +	}							\
>> > +	state;							\
>> > +})
>> > +
>> > +#define trace_rcu_exit(state)					\
>> > +do {								\
>> > +	switch (state) {					\
>> > +	case __TR_IRQ:						\
>> > +		rcu_irq_exit_irqson();				\
>> > +		break;						\
>> > +	case __IRQ_NMI:						\
>> > +		rcu_nmi_exit();					\
>> > +		break;						\
>> > +	default:						\
>> > +		break;						\
>> > +	}							\
>> > +} while (0)
>> 
>> And convert these into static inline functions ?
> 
> Probably the same reason the rest of the file is macros; header hell.
> 
> Now, I could probably make these inlines, but then I'd have to add more
> RCU function declariations to this file (which is outside of
> rcupdate.h). Do we want to do that?

Probably not :) I was just wondering why.

Thanks,

Mathieu

> 
> The reason these were in this file is because I want to keep the comment
> and implementation near to nmi_enter/exit.

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