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

From: Joel Fernandes
Date: Mon Oct 02 2017 - 15:33:37 EST


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