Messages in this thread | | | From | Joel Fernandes <> | Date | Mon, 2 Oct 2017 12:23:51 -0700 | Subject | Re: [PATCH v6 2/2] tracing: Add support for preempt and irq enable/disable events |
| |
Hi Steven,
On Sat, Sep 30, 2017 at 2:08 AM, Steven Rostedt <rostedt@goodmis.org> wrote: > On Mon, 25 Sep 2017 17:23:00 -0700 > Joel Fernandes <joelaf@google.com> 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?
The problem I describe is the following case where trace_hardirqs_off can be called twice, this is just one example: The APIC timer interrupt is received and trace_hardirqs_off_caller is called. So we register that irqs just got turned off. Then, some time in the interrupt exit path, it calls local_irq_disable again, this can happen in this path:
irq_exit -> local_irq_disable -> trace_hardirqs_off
This can happen if __ARCH_IRQ_EXIT_IRQS_DISABLED != 1.
The other path this can happen from is: ret_from_intr -> TRACE_IRQS_OFF while interrupts are already off.
For now, can we just use the per-CPU variable as I did in my latest patchset to avoid the double calling for the the IRQ disable cases? Do you have or Peter have any issues with that? https://patchwork.kernel.org/patch/9978705/ I documented the need for the per-CPU variable in that patch as Peter wanted. Please let me know if the patch I linked above looks Ok, thanks a lot.
thanks,
- Joel
| |