linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Joel Fernandes <joelaf@google.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Joel Fernandes <joel@joelfernandes.org>,
	LKML <linux-kernel@vger.kernel.org>,
	"Cc: Android Kernel" <kernel-team@android.com>,
	Boqun Feng <boqun.feng@gmail.com>,
	Byungchul Park <byungchul.park@lge.com>,
	Ingo Molnar <mingo@redhat.com>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Paul McKenney <paulmck@linux.vnet.ibm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Glexiner <tglx@linutronix.de>,
	Tom Zanussi <tom.zanussi@linux.intel.com>
Subject: Re: [PATCH v12 3/3] tracing: Centralize preemptirq tracepoints and unify their usage
Date: Tue, 7 Aug 2018 18:17:42 -0700	[thread overview]
Message-ID: <CAJWu+oqBOEJicepLaUG=JpmtDTgxjRdMyJAgOEUkdk4NzX0esw@mail.gmail.com> (raw)
In-Reply-To: <20180807204820.50b83c6d@vmware.local.home>

[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]

On Tue, Aug 7, 2018 at 5:48 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 07 Aug 2018 19:54:13 -0400
> Joel Fernandes <joelaf@google.com> wrote:
>
>
>> >OK, I hit this bug, but it's not because of the partial revert. This
>> >bug seems it needs to be another partial revert. I like you movement of
>> >the code, but I'm starting to doubt that we can use a trace event as a
>> >hook for critical areas yet. Well, not until we can use srcu in NMI.
>> >
>>
>> I sent a patch to use srcu for all tracepoints including nmi. That
>> patch also removes this warning and fixes the one other issue masami
>> reported (hot plug causing a warning).
>
> Is it one I can take, or does Paul have it? I'll need it to continue
> with this as is, because I can't pass my tests without triggering that
> warning (and that fails the test).

I think you could take it if you're Ok with it, its purely in the
tracing code and I tested it yesterday morning. Its attached to this
email. I tested that it fixes the mmio trace (hotplug related) issue..

>>
>> If Paul and Mathieu can confirm SRCU works on offline CPUs that would
>> be great.
>
> Yes please.
>

BTW I found this in Paul's docs RCU Requirements docs, "SRCU is insensitive
to whether or not a CPU is online" so I think it will work.

The paragraph was..
Also unlike other RCU flavors, SRCU's callbacks-wait function
srcu_barrier() may be invoked from CPU-hotplug notifiers,
though this is not necessarily a good idea.
The reason that this is possible is that SRCU is insensitive
to whether or not a CPU is online, which means that srcu_barrier()
need not exclude CPU-hotplug operations.

>>
>> It's just this one warning or anything else that makes you feel
>> tracepoints for critical paths isn't feasible?
>
> Currently I'm down to this, but I can't get past my first test in my
> ktest suite, because it fails here.

Ok hopefully this will get you past that. Sorry for the frustration.

[-- Attachment #2: 0001-tracepoint-Run-tracepoints-even-after-CPU-is-offline.patch --]
[-- Type: text/x-patch, Size: 6127 bytes --]

From 6986af946ceb04fc9ddc6d5b45fc559b6807e465 Mon Sep 17 00:00:00 2001
From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Date: Sun, 5 Aug 2018 20:17:41 -0700
Subject: [PATCH] tracepoint: Run tracepoints even after CPU is offline

Commit f37755490fe9 ("tracepoints: Do not trace when cpu is offline")
causes a problem for lockdep using tracepoint code. Once a CPU is
offline, tracepoints donot get called, however this causes a big problem
for lockdep probes that need to run so that IRQ annotations are marked
correctly.

A race is possible where while the CPU is going offline, an interrupt
can come in and then a lockdep assert causes an annotation warning:

[  106.551354] IRQs not enabled as expected
[  106.551785] WARNING: CPU: 1 PID: 0 at kernel/time/tick-sched.c:982
                                         tick_nohz_idle_enter+0x99/0xb0
[  106.552964] Modules linked in:
[  106.553299] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G        W

We need tracepoints to run as late as possible. This commit fixes the
issue by removing the cpu_online check in tracepoint code that was
introduced in the mentioned commit, however we now switch to using SRCU
for all tracepoints and special handle calling tracepoints from NMI so
that we don't run into issues that result from using sched-RCU when the
CPUs are marked to be offline.

Fixes: c3bc8fd637a9 ("tracing: Centralize preemptirq tracepoints and
                      unify their usage")
Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 include/linux/tracepoint.h | 27 +++++++++++----------------
 kernel/tracepoint.c        | 10 ++++++----
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index d9a084c72541..5733502bb3ce 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -35,6 +35,7 @@ struct trace_eval_map {
 #define TRACEPOINT_DEFAULT_PRIO	10
 
 extern struct srcu_struct tracepoint_srcu;
+extern struct srcu_struct tracepoint_srcu_nmi;
 
 extern int
 tracepoint_probe_register(struct tracepoint *tp, void *probe, void *data);
@@ -144,13 +145,11 @@ extern void syscall_unregfunc(void);
 		void *it_func;						\
 		void *__data;						\
 		int __maybe_unused idx = 0;				\
+		struct srcu_struct *ss;					\
 									\
 		if (!(cond))						\
 			return;						\
 									\
-		/* srcu can't be used from NMI */			\
-		WARN_ON_ONCE(rcuidle && in_nmi());			\
-									\
 		/* keep srcu and sched-rcu usage consistent */		\
 		preempt_disable_notrace();				\
 									\
@@ -159,7 +158,11 @@ extern void syscall_unregfunc(void);
 		 * doesn't work from the idle path.			\
 		 */							\
 		if (rcuidle)						\
-			idx = srcu_read_lock_notrace(&tracepoint_srcu);	\
+			ss = &tracepoint_srcu_nmi;			\
+		else							\
+			ss = &tracepoint_srcu;				\
+									\
+		idx = srcu_read_lock_notrace(ss);			\
 									\
 		it_func_ptr = rcu_dereference_raw((tp)->funcs);		\
 									\
@@ -171,8 +174,7 @@ extern void syscall_unregfunc(void);
 			} while ((++it_func_ptr)->func);		\
 		}							\
 									\
-		if (rcuidle)						\
-			srcu_read_unlock_notrace(&tracepoint_srcu, idx);\
+		srcu_read_unlock_notrace(ss, idx);			\
 									\
 		preempt_enable_notrace();				\
 	} while (0)
@@ -212,11 +214,6 @@ extern void syscall_unregfunc(void);
 				TP_PROTO(data_proto),			\
 				TP_ARGS(data_args),			\
 				TP_CONDITION(cond), 0);			\
-		if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {		\
-			rcu_read_lock_sched_notrace();			\
-			rcu_dereference_sched(__tracepoint_##name.funcs);\
-			rcu_read_unlock_sched_notrace();		\
-		}							\
 	}								\
 	__DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args),		\
 		PARAMS(cond), PARAMS(data_proto), PARAMS(data_args))	\
@@ -365,19 +362,17 @@ extern void syscall_unregfunc(void);
  * "void *__data, proto" as the callback prototype.
  */
 #define DECLARE_TRACE_NOARGS(name)					\
-	__DECLARE_TRACE(name, void, ,					\
-			cpu_online(raw_smp_processor_id()),		\
+	__DECLARE_TRACE(name, void, , 1,				\
 			void *__data, __data)
 
 #define DECLARE_TRACE(name, proto, args)				\
-	__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),		\
-			cpu_online(raw_smp_processor_id()),		\
+	__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args), 1,		\
 			PARAMS(void *__data, proto),			\
 			PARAMS(__data, args))
 
 #define DECLARE_TRACE_CONDITION(name, proto, args, cond)		\
 	__DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),		\
-			cpu_online(raw_smp_processor_id()) && (PARAMS(cond)), \
+			PARAMS(cond),					\
 			PARAMS(void *__data, proto),			\
 			PARAMS(__data, args))
 
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 955148d91b74..769d74b2f90e 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -32,7 +32,9 @@ extern struct tracepoint * const __start___tracepoints_ptrs[];
 extern struct tracepoint * const __stop___tracepoints_ptrs[];
 
 DEFINE_SRCU(tracepoint_srcu);
+DEFINE_SRCU(tracepoint_srcu_nmi);
 EXPORT_SYMBOL_GPL(tracepoint_srcu);
+EXPORT_SYMBOL_GPL(tracepoint_srcu_nmi);
 
 /* Set to 1 to enable tracepoint debug output */
 static const int tracepoint_debug;
@@ -70,14 +72,14 @@ static inline void *allocate_probes(int count)
 	return p == NULL ? NULL : p->probes;
 }
 
-static void srcu_free_old_probes(struct rcu_head *head)
+static void srcu_free_old_probes_nmi(struct rcu_head *head)
 {
 	kfree(container_of(head, struct tp_probes, rcu));
 }
 
-static void rcu_free_old_probes(struct rcu_head *head)
+static void srcu_free_old_probes(struct rcu_head *head)
 {
-	call_srcu(&tracepoint_srcu, head, srcu_free_old_probes);
+	call_srcu(&tracepoint_srcu_nmi, head, srcu_free_old_probes_nmi);
 }
 
 static inline void release_probes(struct tracepoint_func *old)
@@ -91,7 +93,7 @@ static inline void release_probes(struct tracepoint_func *old)
 		 * cover both cases. So let us chain the SRCU and sched RCU
 		 * callbacks to wait for both grace periods.
 		 */
-		call_rcu_sched(&tp_probes->rcu, rcu_free_old_probes);
+		call_srcu(&tracepoint_srcu, &tp_probes->rcu, srcu_free_old_probes);
 	}
 }
 
-- 
2.18.0.597.ga71716f1ad-goog


  reply	other threads:[~2018-08-08  1:17 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-30 22:24 [PATCH v12 0/3] tracing: Centralize preemptirq tracepoints and unify their usage Joel Fernandes
2018-07-30 22:24 ` [PATCH v12 1/3] lockdep: use this_cpu_ptr instead of get_cpu_var stats Joel Fernandes
2018-07-30 22:24 ` [PATCH v12 2/3] tracepoint: Make rcuidle tracepoint callers use SRCU Joel Fernandes
2018-07-30 23:10   ` Steven Rostedt
2018-08-10 15:35   ` Steven Rostedt
2018-08-10 16:32     ` Steven Rostedt
2018-07-30 22:24 ` [PATCH v12 3/3] tracing: Centralize preemptirq tracepoints and unify their usage Joel Fernandes
2018-08-06 19:50   ` Steven Rostedt
2018-08-07  0:43     ` Joel Fernandes
2018-08-07  1:43       ` Steven Rostedt
2018-08-07 13:33         ` Joel Fernandes
2018-08-07 13:49           ` Steven Rostedt
2018-08-07 14:10             ` Joel Fernandes
2018-08-07 14:34               ` Steven Rostedt
2018-08-07 14:48                 ` Joel Fernandes
2018-08-07 15:09                   ` Steven Rostedt
2018-08-07 15:24                     ` Joel Fernandes
2018-08-07 23:45                       ` Steven Rostedt
2018-08-07 23:54                         ` Joel Fernandes
2018-08-08  0:48                           ` Steven Rostedt
2018-08-08  1:17                             ` Joel Fernandes [this message]
2018-08-08  1:55                               ` Steven Rostedt
2018-08-08  2:13                                 ` Joel Fernandes
2018-08-08  2:28                                   ` Steven Rostedt
2018-08-08  3:44                                     ` Joel Fernandes
2018-08-08  3:53                                       ` Joel Fernandes
2018-08-08  5:06                                         ` Joel Fernandes
2018-08-08 12:46                                         ` Steven Rostedt
2018-08-08 13:03                                           ` Paul E. McKenney
2018-08-08 13:07                                             ` Steven Rostedt
2018-08-08 14:33                                               ` Paul E. McKenney
2018-08-08 14:49                                                 ` Steven Rostedt
2018-08-08 15:05                                                   ` Paul E. McKenney
2018-08-08 15:23                                                     ` Steven Rostedt
2018-08-08 16:02                                                       ` Paul E. McKenney
2018-08-08 16:24                                                         ` Steven Rostedt
2018-08-08 17:21                                                           ` Paul E. McKenney
2018-08-08 13:00                                         ` Paul E. McKenney
2018-08-08 14:10                                           ` Joel Fernandes
2018-08-08 14:49                                             ` Paul E. McKenney
2018-08-08 19:24                                               ` Joel Fernandes
2018-08-08 20:18                                                 ` Paul E. McKenney
2018-08-08 22:15                                                   ` Joel Fernandes
2018-08-08 22:47                                                     ` Paul E. McKenney
2018-08-09 12:18                                                       ` joel
2018-08-08 14:27                                           ` Steven Rostedt
2018-08-08 14:42                                             ` Paul E. McKenney
2018-08-08 15:27                                               ` Steven Rostedt
2018-08-08 16:03                                                 ` Paul E. McKenney
2018-08-02 14:55 ` [PATCH v12 0/3] " Masami Hiramatsu
2018-08-03  2:57   ` Joel Fernandes
2018-08-03  7:23     ` Masami Hiramatsu
2018-08-04  4:51       ` Joel Fernandes
2018-08-05 16:46         ` Joel Fernandes
2018-08-06  2:07           ` Masami Hiramatsu
2018-08-06 15:24             ` Joel Fernandes
2018-08-03  7:34     ` Masami Hiramatsu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAJWu+oqBOEJicepLaUG=JpmtDTgxjRdMyJAgOEUkdk4NzX0esw@mail.gmail.com' \
    --to=joelaf@google.com \
    --cc=boqun.feng@gmail.com \
    --cc=byungchul.park@lge.com \
    --cc=joel@joelfernandes.org \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mhiramat@kernel.org \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=tom.zanussi@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).