Hi Peter,
On Thu, 12 Jan 2023 20:43:49 +0100
Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
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?
Thank you,
> + */
> +#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
> * and lockdep uses a staged approach which splits the lockdep hardirq
> @@ -28,8 +37,7 @@ static DEFINE_PER_CPU(int, tracing_irq_c
> 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 +48,7 @@ NOKPROBE_SYMBOL(trace_hardirqs_on_prepar
> 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 +70,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 +84,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@xxxxxxxxxx>
