Messages in this thread | | | Date | Tue, 17 Jan 2023 23:31:13 +0900 | From | Masami Hiramatsu (Google) <> | Subject | Re: [PATCH v4] trace,hardirq: No moar _rcuidle() tracing |
| |
On Tue, 17 Jan 2023 10:17:16 +0100 Ingo Molnar <mingo@kernel.org> wrote:
> > * Peter Zijlstra <peterz@infradead.org> wrote: > > > On Tue, Jan 17, 2023 at 01:24:46PM +0900, Masami Hiramatsu wrote: > > > Hi Peter, > > > > > > On Thu, 12 Jan 2023 20:43:49 +0100 > > > Peter Zijlstra <peterz@infradead.org> wrote: > > > > > > > Robot reported that trace_hardirqs_{on,off}() tickle the forbidden > > > > _rcuidle() tracepoint through local_irq_{en,dis}able(). > > > > > > > > For 'sane' configs, these calls will only happen with RCU enabled and > > > > as such can use the regular tracepoint. This also means it's possible > > > > to trace them from NMI context again. > > > > > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > > > > > The code looks good to me. I just have a question about comment. > > > > > > > --- > > > > kernel/trace/trace_preemptirq.c | 21 +++++++++++++-------- > > > > 1 file changed, 13 insertions(+), 8 deletions(-) > > > > > > > > --- a/kernel/trace/trace_preemptirq.c > > > > +++ b/kernel/trace/trace_preemptirq.c > > > > @@ -20,6 +20,15 @@ > > > > static DEFINE_PER_CPU(int, tracing_irq_cpu); > > > > > > > > /* > > > > + * ... > > > > > > Is this intended? Wouldn't you leave any comment here? > > > > I indeed forgot to write the comment before posting, my bad :/ Ingo fixed > > it up when he applied. > > For completeness, I've attached the final commit, which has this comment > included: > > +/* > + * Use regular trace points on architectures that implement noinstr > + * tooling: these calls will only happen with RCU enabled, which can > + * use a regular tracepoint. > + * > + * On older architectures, use the rcuidle tracing methods (which > + * aren't NMI-safe - so exclude NMI contexts): > + */
Thanks! This looks good to me.
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > Thanks, > > Ingo > > ================> > From: Peter Zijlstra <peterz@infradead.org> > Date: Thu, 12 Jan 2023 20:43:49 +0100 > Subject: [PATCH] tracing, hardirq: No moar _rcuidle() tracing > > Robot reported that trace_hardirqs_{on,off}() tickle the forbidden > _rcuidle() tracepoint through local_irq_{en,dis}able(). > > For 'sane' configs, these calls will only happen with RCU enabled and > as such can use the regular tracepoint. This also means it's possible > to trace them from NMI context again. > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > Signed-off-by: Ingo Molnar <mingo@kernel.org> > Link: https://lore.kernel.org/r/20230112195541.477416709@infradead.org > --- > kernel/trace/trace_preemptirq.c | 26 ++++++++++++++++++-------- > 1 file changed, 18 insertions(+), 8 deletions(-) > > diff --git a/kernel/trace/trace_preemptirq.c b/kernel/trace/trace_preemptirq.c > index 629f2854e12b..f992444a0b1f 100644 > --- a/kernel/trace/trace_preemptirq.c > +++ b/kernel/trace/trace_preemptirq.c > @@ -19,6 +19,20 @@ > /* Per-cpu variable to prevent redundant calls when IRQs already off */ > static DEFINE_PER_CPU(int, tracing_irq_cpu); > > +/* > + * Use regular trace points on architectures that implement noinstr > + * tooling: these calls will only happen with RCU enabled, which can > + * use a regular tracepoint. > + * > + * On older architectures, use the rcuidle tracing methods (which > + * aren't NMI-safe - so exclude NMI contexts): > + */ > +#ifdef CONFIG_ARCH_WANTS_NO_INSTR > +#define trace(point) trace_##point > +#else > +#define trace(point) if (!in_nmi()) trace_##point##_rcuidle > +#endif > + > /* > * Like trace_hardirqs_on() but without the lockdep invocation. This is > * used in the low level entry code where the ordering vs. RCU is important > @@ -28,8 +42,7 @@ static DEFINE_PER_CPU(int, tracing_irq_cpu); > void trace_hardirqs_on_prepare(void) > { > if (this_cpu_read(tracing_irq_cpu)) { > - if (!in_nmi()) > - trace_irq_enable(CALLER_ADDR0, CALLER_ADDR1); > + trace(irq_enable)(CALLER_ADDR0, CALLER_ADDR1); > tracer_hardirqs_on(CALLER_ADDR0, CALLER_ADDR1); > this_cpu_write(tracing_irq_cpu, 0); > } > @@ -40,8 +53,7 @@ NOKPROBE_SYMBOL(trace_hardirqs_on_prepare); > void trace_hardirqs_on(void) > { > if (this_cpu_read(tracing_irq_cpu)) { > - if (!in_nmi()) > - trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); > + trace(irq_enable)(CALLER_ADDR0, CALLER_ADDR1); > tracer_hardirqs_on(CALLER_ADDR0, CALLER_ADDR1); > this_cpu_write(tracing_irq_cpu, 0); > } > @@ -63,8 +75,7 @@ void trace_hardirqs_off_finish(void) > if (!this_cpu_read(tracing_irq_cpu)) { > this_cpu_write(tracing_irq_cpu, 1); > tracer_hardirqs_off(CALLER_ADDR0, CALLER_ADDR1); > - if (!in_nmi()) > - trace_irq_disable(CALLER_ADDR0, CALLER_ADDR1); > + trace(irq_disable)(CALLER_ADDR0, CALLER_ADDR1); > } > > } > @@ -78,8 +89,7 @@ void trace_hardirqs_off(void) > if (!this_cpu_read(tracing_irq_cpu)) { > this_cpu_write(tracing_irq_cpu, 1); > tracer_hardirqs_off(CALLER_ADDR0, CALLER_ADDR1); > - if (!in_nmi()) > - trace_irq_disable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); > + trace(irq_disable)(CALLER_ADDR0, CALLER_ADDR1); > } > } > EXPORT_SYMBOL(trace_hardirqs_off);
-- Masami Hiramatsu (Google) <mhiramat@kernel.org>
| |