Re: [PATCH v6 2/2] tracing: Add support for preempt and irq enable/disable events

[Joel Fernandes]

On Mon, Oct 2, 2017 at 12:23 PM, Joel Fernandes <joelaf@xxxxxxxxxx> wrote:
> Hi Steven,
>
> On Sat, Sep 30, 2017 at 2:08 AM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>> On Mon, 25 Sep 2017 17:23:00 -0700
>> Joel Fernandes <joelaf@xxxxxxxxxx> 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