From: Mark Rutland <mark.rutland@arm.com> To: Peter Zijlstra <peterz@infradead.org> Cc: juri.lelli@redhat.com, daniel.lezcano@linaro.org, wanpengli@tencent.com, kvm@vger.kernel.org, rafael@kernel.org, pv-drivers@vmware.com, Frederic Weisbecker <fweisbec@gmail.com>, dave.hansen@linux.intel.com, virtualization@lists.linux-foundation.org, bsegall@google.com, amakhalov@vmware.com, will@kernel.org, vschneid@redhat.com, hpa@zytor.com, x86@kernel.org, mingo@kernel.org, mgorman@suse.de, linux-trace-kernel@vger.kernel.org, Paul McKenney <paulmck@kernel.org>, linux-pm@vger.kernel.org, boqun.feng@gmail.com, Steven Rostedt <rostedt@goodmis.org>, bp@alien8.de, vincent.guittot@linaro.org, boris.ostrovsky@oracle.com, dietmar.eggemann@arm.com, jgross@suse.com, seanjc@google.com, linux-kernel@vger.kernel.org, tglx@linutronix.de, mhiramat@kernel.org, pbonzini@redhat.com, bristot@redhat.com Subject: Re: [PATCH 3/6] ftrace/x86: Warn and ignore graph tracing when RCU is disabled Date: Wed, 25 Jan 2023 11:32:04 +0000 [thread overview] Message-ID: <Y9ETNHyE2NgrPJJL@FVFF77S0Q05N> (raw) In-Reply-To: <Y9EI0Gn/NUJt6GEk@hirez.programming.kicks-ass.net> On Wed, Jan 25, 2023 at 11:47:44AM +0100, Peter Zijlstra wrote: > On Tue, Jan 24, 2023 at 05:12:14PM +0000, Mark Rutland wrote: > > On Tue, Jan 24, 2023 at 03:44:35PM +0100, Peter Zijlstra wrote: > > > On Mon, Jan 23, 2023 at 05:07:53PM -0500, Steven Rostedt wrote: > > > > > > > Actually, perhaps we can just add this, and all you need to do is create > > > > and set CONFIG_NO_RCU_TRACING (or some other name). > > > > > > Elsewhere I've used CONFIG_ARCH_WANTS_NO_INSTR for this. > > > > Yes please; if we use CONFIG_ARCH_WANTS_NO_INSTR then arm64 will get this "for > > free" once we add the missing checks (which I assume we need) in our ftrace_prepare_return(). > > > > > Anyway, I took it for a spin and it .... doesn't seems to do the job. > > > > > > With my patch the first splat is > > > > > > "RCU not on for: cpuidle_poll_time+0x0/0x70" > > > > > > While with yours I seems to get the endless: > > > > > > "WARNING: suspicious RCU usage" > > > > > > thing. Let me see if I can figure out where it goes side-ways. > > > > Hmmm... for WARN_ONCE() don't we need to wake RCU first also? I thought we > > needed that at least for the printk machinery? > > OK, the below seems to work nice for me -- although I'm still on a > hacked up printk, but the recursive RCU not watching fail seems to be > tamed. FWIW, I gave that a spin on arm64 with the ftrace selftests, and I see no splats, so it looks good on that front. Currently arm64's BUG/WARN exception handling does the usual lockdep/rcu/whatever stuff before getting to report_bug(), so that bit should be redundant (and any WARN() or BUG() early in the entry code is likely to lead to a stack overflow and kill the kernel), but it shouldn't be harmful. > Ofc. Paul might have an opinion on this glorious bodge ;-) I'll leave that to Paul. ;) Thanks, Mark. > > --- > > diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h > index c303f7a114e9..d48cd92d2364 100644 > --- a/include/linux/trace_recursion.h > +++ b/include/linux/trace_recursion.h > @@ -135,6 +135,21 @@ extern void ftrace_record_recursion(unsigned long ip, unsigned long parent_ip); > # define do_ftrace_record_recursion(ip, pip) do { } while (0) > #endif > > +#ifdef CONFIG_ARCH_WANTS_NO_INSTR > +# define trace_warn_on_no_rcu(ip) \ > + ({ \ > + bool __ret = !rcu_is_watching(); \ > + if (__ret && !trace_recursion_test(TRACE_RECORD_RECURSION_BIT)) { \ > + trace_recursion_set(TRACE_RECORD_RECURSION_BIT); \ > + WARN_ONCE(true, "RCU not on for: %pS\n", (void *)ip); \ > + trace_recursion_clear(TRACE_RECORD_RECURSION_BIT); \ > + } \ > + __ret; \ > + }) > +#else > +# define trace_warn_on_no_rcu(ip) false > +#endif > + > /* > * Preemption is promised to be disabled when return bit >= 0. > */ > @@ -144,6 +159,9 @@ static __always_inline int trace_test_and_set_recursion(unsigned long ip, unsign > unsigned int val = READ_ONCE(current->trace_recursion); > int bit; > > + if (trace_warn_on_no_rcu(ip)) > + return -1; > + > bit = trace_get_context_bit() + start; > if (unlikely(val & (1 << bit))) { > /* > diff --git a/lib/bug.c b/lib/bug.c > index c223a2575b72..0a10643ea168 100644 > --- a/lib/bug.c > +++ b/lib/bug.c > @@ -47,6 +47,7 @@ > #include <linux/sched.h> > #include <linux/rculist.h> > #include <linux/ftrace.h> > +#include <linux/context_tracking.h> > > extern struct bug_entry __start___bug_table[], __stop___bug_table[]; > > @@ -153,7 +154,7 @@ struct bug_entry *find_bug(unsigned long bugaddr) > return module_find_bug(bugaddr); > } > > -enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs) > +static enum bug_trap_type __report_bug(unsigned long bugaddr, struct pt_regs *regs) > { > struct bug_entry *bug; > const char *file; > @@ -209,6 +210,30 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs) > return BUG_TRAP_TYPE_BUG; > } > > +enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs) > +{ > + enum bug_trap_type ret; > + bool rcu = false; > + > +#ifdef CONFIG_CONTEXT_TRACKING_IDLE > + /* > + * Horrible hack to shut up recursive RCU isn't watching fail since > + * lots of the actual reporting also relies on RCU. > + */ > + if (!rcu_is_watching()) { > + rcu = true; > + ct_state_inc(RCU_DYNTICKS_IDX); > + } > +#endif > + > + ret = __report_bug(bugaddr, regs); > + > + if (rcu) > + ct_state_inc(RCU_DYNTICKS_IDX); > + > + return ret; > +} > + > static void clear_once_table(struct bug_entry *start, struct bug_entry *end) > { > struct bug_entry *bug; _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
WARNING: multiple messages have this Message-ID (diff)
From: Mark Rutland <mark.rutland@arm.com> To: Peter Zijlstra <peterz@infradead.org> Cc: Steven Rostedt <rostedt@goodmis.org>, mingo@kernel.org, will@kernel.org, boqun.feng@gmail.com, tglx@linutronix.de, bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com, seanjc@google.com, pbonzini@redhat.com, jgross@suse.com, srivatsa@csail.mit.edu, amakhalov@vmware.com, pv-drivers@vmware.com, mhiramat@kernel.org, wanpengli@tencent.com, vkuznets@redhat.com, boris.ostrovsky@oracle.com, rafael@kernel.org, daniel.lezcano@linaro.org, juri.lelli@redhat.com, vincent.guittot@linaro.org, dietmar.eggemann@arm.com, bsegall@google.com, mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, virtualization@lists.linux-foundation.org, linux-trace-kernel@vger.kernel.org, linux-pm@vger.kernel.org, Paul McKenney <paulmck@kernel.org>, Frederic Weisbecker <fweisbec@gmail.com> Subject: Re: [PATCH 3/6] ftrace/x86: Warn and ignore graph tracing when RCU is disabled Date: Wed, 25 Jan 2023 11:32:04 +0000 [thread overview] Message-ID: <Y9ETNHyE2NgrPJJL@FVFF77S0Q05N> (raw) In-Reply-To: <Y9EI0Gn/NUJt6GEk@hirez.programming.kicks-ass.net> On Wed, Jan 25, 2023 at 11:47:44AM +0100, Peter Zijlstra wrote: > On Tue, Jan 24, 2023 at 05:12:14PM +0000, Mark Rutland wrote: > > On Tue, Jan 24, 2023 at 03:44:35PM +0100, Peter Zijlstra wrote: > > > On Mon, Jan 23, 2023 at 05:07:53PM -0500, Steven Rostedt wrote: > > > > > > > Actually, perhaps we can just add this, and all you need to do is create > > > > and set CONFIG_NO_RCU_TRACING (or some other name). > > > > > > Elsewhere I've used CONFIG_ARCH_WANTS_NO_INSTR for this. > > > > Yes please; if we use CONFIG_ARCH_WANTS_NO_INSTR then arm64 will get this "for > > free" once we add the missing checks (which I assume we need) in our ftrace_prepare_return(). > > > > > Anyway, I took it for a spin and it .... doesn't seems to do the job. > > > > > > With my patch the first splat is > > > > > > "RCU not on for: cpuidle_poll_time+0x0/0x70" > > > > > > While with yours I seems to get the endless: > > > > > > "WARNING: suspicious RCU usage" > > > > > > thing. Let me see if I can figure out where it goes side-ways. > > > > Hmmm... for WARN_ONCE() don't we need to wake RCU first also? I thought we > > needed that at least for the printk machinery? > > OK, the below seems to work nice for me -- although I'm still on a > hacked up printk, but the recursive RCU not watching fail seems to be > tamed. FWIW, I gave that a spin on arm64 with the ftrace selftests, and I see no splats, so it looks good on that front. Currently arm64's BUG/WARN exception handling does the usual lockdep/rcu/whatever stuff before getting to report_bug(), so that bit should be redundant (and any WARN() or BUG() early in the entry code is likely to lead to a stack overflow and kill the kernel), but it shouldn't be harmful. > Ofc. Paul might have an opinion on this glorious bodge ;-) I'll leave that to Paul. ;) Thanks, Mark. > > --- > > diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h > index c303f7a114e9..d48cd92d2364 100644 > --- a/include/linux/trace_recursion.h > +++ b/include/linux/trace_recursion.h > @@ -135,6 +135,21 @@ extern void ftrace_record_recursion(unsigned long ip, unsigned long parent_ip); > # define do_ftrace_record_recursion(ip, pip) do { } while (0) > #endif > > +#ifdef CONFIG_ARCH_WANTS_NO_INSTR > +# define trace_warn_on_no_rcu(ip) \ > + ({ \ > + bool __ret = !rcu_is_watching(); \ > + if (__ret && !trace_recursion_test(TRACE_RECORD_RECURSION_BIT)) { \ > + trace_recursion_set(TRACE_RECORD_RECURSION_BIT); \ > + WARN_ONCE(true, "RCU not on for: %pS\n", (void *)ip); \ > + trace_recursion_clear(TRACE_RECORD_RECURSION_BIT); \ > + } \ > + __ret; \ > + }) > +#else > +# define trace_warn_on_no_rcu(ip) false > +#endif > + > /* > * Preemption is promised to be disabled when return bit >= 0. > */ > @@ -144,6 +159,9 @@ static __always_inline int trace_test_and_set_recursion(unsigned long ip, unsign > unsigned int val = READ_ONCE(current->trace_recursion); > int bit; > > + if (trace_warn_on_no_rcu(ip)) > + return -1; > + > bit = trace_get_context_bit() + start; > if (unlikely(val & (1 << bit))) { > /* > diff --git a/lib/bug.c b/lib/bug.c > index c223a2575b72..0a10643ea168 100644 > --- a/lib/bug.c > +++ b/lib/bug.c > @@ -47,6 +47,7 @@ > #include <linux/sched.h> > #include <linux/rculist.h> > #include <linux/ftrace.h> > +#include <linux/context_tracking.h> > > extern struct bug_entry __start___bug_table[], __stop___bug_table[]; > > @@ -153,7 +154,7 @@ struct bug_entry *find_bug(unsigned long bugaddr) > return module_find_bug(bugaddr); > } > > -enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs) > +static enum bug_trap_type __report_bug(unsigned long bugaddr, struct pt_regs *regs) > { > struct bug_entry *bug; > const char *file; > @@ -209,6 +210,30 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs) > return BUG_TRAP_TYPE_BUG; > } > > +enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs) > +{ > + enum bug_trap_type ret; > + bool rcu = false; > + > +#ifdef CONFIG_CONTEXT_TRACKING_IDLE > + /* > + * Horrible hack to shut up recursive RCU isn't watching fail since > + * lots of the actual reporting also relies on RCU. > + */ > + if (!rcu_is_watching()) { > + rcu = true; > + ct_state_inc(RCU_DYNTICKS_IDX); > + } > +#endif > + > + ret = __report_bug(bugaddr, regs); > + > + if (rcu) > + ct_state_inc(RCU_DYNTICKS_IDX); > + > + return ret; > +} > + > static void clear_once_table(struct bug_entry *start, struct bug_entry *end) > { > struct bug_entry *bug;
next prev parent reply other threads:[~2023-01-25 11:32 UTC|newest] Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-01-23 20:50 [PATCH 0/6] A few cpuidle vs rcu fixes Peter Zijlstra 2023-01-23 20:50 ` Peter Zijlstra 2023-01-23 20:50 ` [PATCH 1/6] x86: Always inline arch_atomic64 Peter Zijlstra 2023-01-23 20:50 ` Peter Zijlstra 2023-01-23 20:50 ` [PATCH 2/6] x86/pvclock: improve atomic update of last_value in pvclock_clocksource_read Peter Zijlstra 2023-01-23 20:50 ` Peter Zijlstra 2023-01-23 20:50 ` [PATCH 3/6] ftrace/x86: Warn and ignore graph tracing when RCU is disabled Peter Zijlstra 2023-01-23 20:50 ` Peter Zijlstra 2023-01-23 21:53 ` Steven Rostedt 2023-01-23 21:53 ` Steven Rostedt 2023-01-23 22:07 ` Steven Rostedt 2023-01-23 22:07 ` Steven Rostedt 2023-01-24 14:44 ` Peter Zijlstra 2023-01-24 14:44 ` Peter Zijlstra 2023-01-24 17:12 ` Mark Rutland 2023-01-24 17:12 ` Mark Rutland 2023-01-25 9:37 ` Peter Zijlstra 2023-01-25 9:37 ` Peter Zijlstra 2023-01-25 10:47 ` Peter Zijlstra 2023-01-25 10:47 ` Peter Zijlstra 2023-01-25 11:32 ` Mark Rutland [this message] 2023-01-25 11:32 ` Mark Rutland 2023-01-25 18:46 ` Paul E. McKenney 2023-01-26 9:28 ` Peter Zijlstra 2023-01-26 9:28 ` Peter Zijlstra 2023-01-28 19:12 ` Paul E. McKenney 2023-01-23 20:50 ` [PATCH 4/6] x86: Mark sched_clock() noinstr Peter Zijlstra 2023-01-23 20:50 ` Peter Zijlstra 2023-01-23 20:50 ` [PATCH 5/6] sched/clock: Make local_clock() noinstr Peter Zijlstra 2023-01-23 20:50 ` Peter Zijlstra 2023-01-23 20:50 ` [PATCH 6/6] cpuidle: Fix poll_idle() noinstr annotation Peter Zijlstra 2023-01-23 20:50 ` Peter Zijlstra 2023-01-24 14:24 ` Rafael J. Wysocki 2023-01-24 14:24 ` Rafael J. Wysocki 2023-01-24 16:34 ` [PATCH 0/6] A few cpuidle vs rcu fixes Mark Rutland 2023-01-24 16:34 ` Mark Rutland 2023-01-24 17:30 ` Mark Rutland 2023-01-24 17:30 ` Mark Rutland 2023-01-24 18:39 ` Mark Rutland 2023-01-24 18:39 ` Mark Rutland 2023-01-25 9:35 ` Peter Zijlstra 2023-01-25 9:35 ` Peter Zijlstra 2023-01-25 9:40 ` Peter Zijlstra 2023-01-25 9:40 ` Peter Zijlstra 2023-01-25 10:23 ` Mark Rutland 2023-01-25 10:23 ` Mark Rutland 2023-01-31 14:22 ` [tip: sched/core] cpuidle: tracing, preempt: Squash _rcuidle tracing tip-bot2 for Peter Zijlstra 2023-01-25 9:31 ` [PATCH 0/6] A few cpuidle vs rcu fixes Peter Zijlstra 2023-01-25 9:31 ` Peter Zijlstra 2023-01-25 9:36 ` Mark Rutland 2023-01-25 9:36 ` Mark Rutland 2023-01-25 15:20 ` Mark Rutland 2023-01-25 15:20 ` Mark Rutland
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=Y9ETNHyE2NgrPJJL@FVFF77S0Q05N \ --to=mark.rutland@arm.com \ --cc=amakhalov@vmware.com \ --cc=boqun.feng@gmail.com \ --cc=boris.ostrovsky@oracle.com \ --cc=bp@alien8.de \ --cc=bristot@redhat.com \ --cc=bsegall@google.com \ --cc=daniel.lezcano@linaro.org \ --cc=dave.hansen@linux.intel.com \ --cc=dietmar.eggemann@arm.com \ --cc=fweisbec@gmail.com \ --cc=hpa@zytor.com \ --cc=jgross@suse.com \ --cc=juri.lelli@redhat.com \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=linux-trace-kernel@vger.kernel.org \ --cc=mgorman@suse.de \ --cc=mhiramat@kernel.org \ --cc=mingo@kernel.org \ --cc=paulmck@kernel.org \ --cc=pbonzini@redhat.com \ --cc=peterz@infradead.org \ --cc=pv-drivers@vmware.com \ --cc=rafael@kernel.org \ --cc=rostedt@goodmis.org \ --cc=seanjc@google.com \ --cc=tglx@linutronix.de \ --cc=vincent.guittot@linaro.org \ --cc=virtualization@lists.linux-foundation.org \ --cc=vschneid@redhat.com \ --cc=wanpengli@tencent.com \ --cc=will@kernel.org \ --cc=x86@kernel.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.