On Mon, Oct 2, 2017 at 12:23 PM, Joel Fernandes <[email protected]> wrote: > Hi Steven, > > On Sat, Sep 30, 2017 at 2:08 AM, Steven Rostedt <[email protected]> wrote: >> On Mon, 25 Sep 2017 17:23:00 -0700 >> Joel Fernandes <[email protected]> wrote: >> >>> The trace_hardirqs_off API can be called even when IRQs are already >>> off. This is unlike the trace_hardirqs_on which checks if IRQs are off >>> (atleast from some callsites), here are the definitions just for >>> reference [1]. I guess we could modify local_irq_disable and >>> local_irq_save to check what the HW flags was before calling >>> raw_local_irq_save and only then call trace_hardirqs_off if they were >>> indeed on and now being turned off, but that adds complexity to it - >>> also we have to then modify all the callsites from assembly code to >>> conditionally call trace_hardirqs_on/off :(. >>> >> >>> [1] >>> http://elixir.free-electrons.com/linux/latest/source/include/linux/irqflags.h#L89 >> >> Yeah, I think the issue for the recursion is basically this: >> >> #define local_irq_restore(flags) \ >> do { \ >> if (raw_irqs_disabled_flags(flags)) { \ >> raw_local_irq_restore(flags); \ >> trace_hardirqs_off(); \ >> } else { \ >> trace_hardirqs_on(); \ >> raw_local_irq_restore(flags); \ >> } \ >> } while (0) >> >> >> Peter, >> >> Is there any place where we would call local_irq_restore() when >> interrupts are enabled? > > Actually local_irq_restore doesn't have the problem I describe since > we do check for 'flags' and based on the condition and *only* then > call the trace_hardirqs_* functions right?
I take that back a bit, to Steven's point - local_irq_restore does have the same "issue" but modifying Steven's point a little bit, local_irq_restore can do the a second unnecessary trace_harirqs_off call *even* in cases where IRQs were already off before the flags were saved. The following patch should correct that however, there are other cases where this can still happen as I described in [1] so I suggest we just use the per-CPU variable as I'm doing in [2]. [1] https://lkml.org/lkml/2017/10/2/484 [2] https://patchwork.kernel.org/patch/9978705/ diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h index 5dd1272d1ab2..2a1af0dd9cc4 100644 --- a/include/linux/irqflags.h +++ b/include/linux/irqflags.h @@ -93,7 +93,9 @@ #define local_irq_save(flags) \ do { \ raw_local_irq_save(flags); \ - trace_hardirqs_off(); \ + if (!raw_irqs_disabled_flags(flags)) { \ + trace_hardirqs_off(); \ + } \ } while (0) @@ -101,7 +103,6 @@ do { \ if (raw_irqs_disabled_flags(flags)) { \ raw_local_irq_restore(flags); \ - trace_hardirqs_off(); \ } else { \ trace_hardirqs_on(); \ raw_local_irq_restore(flags); \ -- 2.14.2.822.g60be5d43e6-goog
