* [PATCH] tracing/irqtrace: only call trace_hardirqs_on/off when state changes
@ 2017-11-16 16:15 Nicholas Piggin
2018-05-01 18:46 ` Steven Rostedt
0 siblings, 1 reply; 10+ messages in thread
From: Nicholas Piggin @ 2017-11-16 16:15 UTC (permalink / raw)
Cc: Nicholas Piggin, Steven Rostedt, Ingo Molnar, linux-kernel
In local_irq_save and local_irq_restore, only call irq tracing when
the flag state acutally changes. It is not unexpected for the state
to go disable->disable.
This allows the irq tracing code to better track superfluous
enables and disables, and in future could issue warnings. For the
most part they are harmless, but they can indicate that the caller
has lost track of its irq state.
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
I wonder what you think of this patch? After a small fix to
init/main.c and some powerpc fixes, I was able to use this patch
and a change to warn in the lockdep code in case of redundant ops,
to verify there were no local_irq_enable() when enabled, or
local_irq_disable() when disabled (which was implicated in a bug).
Lots of code to verify so we can't do this immediately, but I
think it's cleaner to enforce the rule?
Thanks,
Nick
include/linux/irqflags.h | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 46cb57d5eb13..82287e1c6c52 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -110,7 +110,8 @@ do { \
#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)
@@ -118,9 +119,11 @@ do { \
do { \
if (raw_irqs_disabled_flags(flags)) { \
raw_local_irq_restore(flags); \
- trace_hardirqs_off(); \
+ if (!irqs_disabled()) \
+ trace_hardirqs_off(); \
} else { \
- trace_hardirqs_on(); \
+ if (irqs_disabled()) \
+ trace_hardirqs_on(); \
raw_local_irq_restore(flags); \
} \
} while (0)
--
2.15.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] tracing/irqtrace: only call trace_hardirqs_on/off when state changes
2017-11-16 16:15 [PATCH] tracing/irqtrace: only call trace_hardirqs_on/off when state changes Nicholas Piggin
@ 2018-05-01 18:46 ` Steven Rostedt
2018-05-01 19:19 ` Peter Zijlstra
0 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2018-05-01 18:46 UTC (permalink / raw)
To: Nicholas Piggin, Peter Zijlstra; +Cc: Ingo Molnar, linux-kernel
On Fri, 17 Nov 2017 02:15:06 +1000
Nicholas Piggin <npiggin@gmail.com> wrote:
> In local_irq_save and local_irq_restore, only call irq tracing when
> the flag state acutally changes. It is not unexpected for the state
> to go disable->disable.
>
> This allows the irq tracing code to better track superfluous
> enables and disables, and in future could issue warnings. For the
> most part they are harmless, but they can indicate that the caller
> has lost track of its irq state.
I missed this before (that was a busy time, I missed a lot of emails
then :-/ ).
Anyway, this makes sense.
Peter?
-- Steve
>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> I wonder what you think of this patch? After a small fix to
> init/main.c and some powerpc fixes, I was able to use this patch
> and a change to warn in the lockdep code in case of redundant ops,
> to verify there were no local_irq_enable() when enabled, or
> local_irq_disable() when disabled (which was implicated in a bug).
>
> Lots of code to verify so we can't do this immediately, but I
> think it's cleaner to enforce the rule?
>
> Thanks,
> Nick
>
> include/linux/irqflags.h | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
> index 46cb57d5eb13..82287e1c6c52 100644
> --- a/include/linux/irqflags.h
> +++ b/include/linux/irqflags.h
> @@ -110,7 +110,8 @@ do { \
> #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)
>
>
> @@ -118,9 +119,11 @@ do { \
> do { \
> if (raw_irqs_disabled_flags(flags)) { \
> raw_local_irq_restore(flags); \
> - trace_hardirqs_off(); \
> + if (!irqs_disabled()) \
> + trace_hardirqs_off(); \
> } else { \
> - trace_hardirqs_on(); \
> + if (irqs_disabled()) \
> + trace_hardirqs_on(); \
> raw_local_irq_restore(flags); \
> } \
> } while (0)
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] tracing/irqtrace: only call trace_hardirqs_on/off when state changes
2018-05-01 18:46 ` Steven Rostedt
@ 2018-05-01 19:19 ` Peter Zijlstra
2018-05-01 19:38 ` Steven Rostedt
0 siblings, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2018-05-01 19:19 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Nicholas Piggin, Ingo Molnar, linux-kernel
On Tue, May 01, 2018 at 02:46:20PM -0400, Steven Rostedt wrote:
> On Fri, 17 Nov 2017 02:15:06 +1000
> Nicholas Piggin <npiggin@gmail.com> wrote:
>
> > In local_irq_save and local_irq_restore, only call irq tracing when
> > the flag state acutally changes. It is not unexpected for the state
> > to go disable->disable.
> >
> > This allows the irq tracing code to better track superfluous
> > enables and disables, and in future could issue warnings. For the
> > most part they are harmless, but they can indicate that the caller
> > has lost track of its irq state.
>
> I missed this before (that was a busy time, I missed a lot of emails
> then :-/ ).
>
> Anyway, this makes sense.
>
> Peter?
I'm confused. The patch calls the trace hooks less often, so how can it
then better track superfluous calls?
> > @@ -110,7 +110,8 @@ do { \
> > #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)
Here we only call the trace hook when we actually did an ON->OFF change
and loose the call on OFF->OFF.
> > @@ -118,9 +119,11 @@ do { \
> > do { \
> > if (raw_irqs_disabled_flags(flags)) { \
> > raw_local_irq_restore(flags); \
> > - trace_hardirqs_off(); \
> > + if (!irqs_disabled()) \
> > + trace_hardirqs_off(); \
Only call on ON->OFF, ignore OFF->OFF.
> > } else { \
> > - trace_hardirqs_on(); \
> > + if (irqs_disabled()) \
> > + trace_hardirqs_on(); \
> > raw_local_irq_restore(flags); \
> > } \
> > } while (0)
Only call on OFF->ON, ignore ON->ON.
Now, lockdep only minimally tracks these otherwise redundant operations;
see redundant_hardirqs_{on,off} counters, and loosing that doesn't seen
like a big issue.
But I'm confused how this helps track superfluous things, it looks like
it explicitly tracks _less_ superfluous transitions.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] tracing/irqtrace: only call trace_hardirqs_on/off when state changes
2018-05-01 19:19 ` Peter Zijlstra
@ 2018-05-01 19:38 ` Steven Rostedt
2018-05-01 19:48 ` Peter Zijlstra
0 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2018-05-01 19:38 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Nicholas Piggin, Ingo Molnar, linux-kernel
On Tue, 1 May 2018 21:19:51 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, May 01, 2018 at 02:46:20PM -0400, Steven Rostedt wrote:
> > On Fri, 17 Nov 2017 02:15:06 +1000
> > Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > > In local_irq_save and local_irq_restore, only call irq tracing when
> > > the flag state acutally changes. It is not unexpected for the state
> > > to go disable->disable.
> > >
> > > This allows the irq tracing code to better track superfluous
> > > enables and disables, and in future could issue warnings. For the
> > > most part they are harmless, but they can indicate that the caller
> > > has lost track of its irq state.
> >
> > I missed this before (that was a busy time, I missed a lot of emails
> > then :-/ ).
> >
> > Anyway, this makes sense.
> >
> > Peter?
>
> I'm confused. The patch calls the trace hooks less often, so how can it
> then better track superfluous calls?
>
> > > @@ -110,7 +110,8 @@ do { \
> > > #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)
>
> Here we only call the trace hook when we actually did an ON->OFF change
> and loose the call on OFF->OFF.
>
> > > @@ -118,9 +119,11 @@ do { \
> > > do { \
> > > if (raw_irqs_disabled_flags(flags)) { \
> > > raw_local_irq_restore(flags); \
> > > - trace_hardirqs_off(); \
> > > + if (!irqs_disabled()) \
> > > + trace_hardirqs_off(); \
>
> Only call on ON->OFF, ignore OFF->OFF.
>
> > > } else { \
> > > - trace_hardirqs_on(); \
> > > + if (irqs_disabled()) \
> > > + trace_hardirqs_on(); \
> > > raw_local_irq_restore(flags); \
> > > } \
> > > } while (0)
>
> Only call on OFF->ON, ignore ON->ON.
>
>
> Now, lockdep only minimally tracks these otherwise redundant operations;
> see redundant_hardirqs_{on,off} counters, and loosing that doesn't seen
> like a big issue.
>
> But I'm confused how this helps track superfluous things, it looks like
> it explicitly tracks _less_ superfluous transitions.
I think it is about triggering on OFF->OFF a warning, as that would
only happen if we have:
local_irq_save(flags);
[..]
local_irq_disable();
-- Steve
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] tracing/irqtrace: only call trace_hardirqs_on/off when state changes
2018-05-01 19:38 ` Steven Rostedt
@ 2018-05-01 19:48 ` Peter Zijlstra
2018-05-01 20:00 ` Steven Rostedt
0 siblings, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2018-05-01 19:48 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Nicholas Piggin, Ingo Molnar, linux-kernel
On Tue, May 01, 2018 at 03:38:40PM -0400, Steven Rostedt wrote:
> On Tue, 1 May 2018 21:19:51 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> > Now, lockdep only minimally tracks these otherwise redundant operations;
> > see redundant_hardirqs_{on,off} counters, and loosing that doesn't seen
> > like a big issue.
> >
> > But I'm confused how this helps track superfluous things, it looks like
> > it explicitly tracks _less_ superfluous transitions.
>
> I think it is about triggering on OFF->OFF a warning, as that would
> only happen if we have:
>
> local_irq_save(flags);
> [..]
> local_irq_disable();
>
Ahh, ok. Yes, that is easier to do with these changes. The alternative
is to add more information to the tracehooks such that we can do the
same internally, but whatever.
Yeah, I'm fine with the proposed change, but maybe improve the Changelog
a little for slow people like me :-)
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] tracing/irqtrace: only call trace_hardirqs_on/off when state changes
2018-05-01 19:48 ` Peter Zijlstra
@ 2018-05-01 20:00 ` Steven Rostedt
2018-05-01 21:15 ` Joel Fernandes
0 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2018-05-01 20:00 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Nicholas Piggin, Ingo Molnar, linux-kernel
On Tue, 1 May 2018 21:48:38 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, May 01, 2018 at 03:38:40PM -0400, Steven Rostedt wrote:
> > On Tue, 1 May 2018 21:19:51 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
>
> > > Now, lockdep only minimally tracks these otherwise redundant operations;
> > > see redundant_hardirqs_{on,off} counters, and loosing that doesn't seen
> > > like a big issue.
> > >
> > > But I'm confused how this helps track superfluous things, it looks like
> > > it explicitly tracks _less_ superfluous transitions.
> >
> > I think it is about triggering on OFF->OFF a warning, as that would
> > only happen if we have:
> >
> > local_irq_save(flags);
> > [..]
> > local_irq_disable();
> >
>
> Ahh, ok. Yes, that is easier to do with these changes. The alternative
> is to add more information to the tracehooks such that we can do the
> same internally, but whatever.
>
> Yeah, I'm fine with the proposed change, but maybe improve the Changelog
> a little for slow people like me :-)
Great!
Nicholas,
I know this is an old patch (from last November), but want to send it
again with a proper change log and signed off by?
-- Steve
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] tracing/irqtrace: only call trace_hardirqs_on/off when state changes
2018-05-01 20:00 ` Steven Rostedt
@ 2018-05-01 21:15 ` Joel Fernandes
2018-05-02 0:12 ` Nicholas Piggin
0 siblings, 1 reply; 10+ messages in thread
From: Joel Fernandes @ 2018-05-01 21:15 UTC (permalink / raw)
To: Steven Rostedt
Cc: Peter Zijlstra, Nicholas Piggin, Ingo Molnar, Linux Kernel Mailing List
On Tue, May 1, 2018 at 1:00 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 1 May 2018 21:48:38 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
>
>> On Tue, May 01, 2018 at 03:38:40PM -0400, Steven Rostedt wrote:
>> > On Tue, 1 May 2018 21:19:51 +0200
>> > Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> > > Now, lockdep only minimally tracks these otherwise redundant operations;
>> > > see redundant_hardirqs_{on,off} counters, and loosing that doesn't seen
>> > > like a big issue.
>> > >
>> > > But I'm confused how this helps track superfluous things, it looks like
>> > > it explicitly tracks _less_ superfluous transitions.
>> >
>> > I think it is about triggering on OFF->OFF a warning, as that would
>> > only happen if we have:
>> >
>> > local_irq_save(flags);
>> > [..]
>> > local_irq_disable();
>> >
>>
>> Ahh, ok. Yes, that is easier to do with these changes. The alternative
>> is to add more information to the tracehooks such that we can do the
>> same internally, but whatever.
>>
>> Yeah, I'm fine with the proposed change, but maybe improve the Changelog
>> a little for slow people like me :-)
>
> Great!
>
> Nicholas,
>
> I know this is an old patch (from last November), but want to send it
> again with a proper change log and signed off by?
I actually wrote the exact same patch yesterday with changes Matsami
suggested. However I decided not to send it, since it didn't have any
performance improvement (which was the reason I wrote it).
Also with my recent set, I don't think it will help detect repeated
calls to trace_hardirqs_off because we are handling that recursive
case by using per-cpu variable and bailing out if there is a
recursion, before even calling into lockdep.
I have mixed feelings about this patch, I am Ok with this patch but I
suggest its sent with the follow-up patch that shows its use of this.
And also appreciate if such a follow-up patch is rebased onto the IRQ
tracepoint work: https://patchwork.kernel.org/patch/10373129/
What do you think?
thanks,
- Joel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] tracing/irqtrace: only call trace_hardirqs_on/off when state changes
2018-05-01 21:15 ` Joel Fernandes
@ 2018-05-02 0:12 ` Nicholas Piggin
2018-07-11 1:44 ` Steven Rostedt
0 siblings, 1 reply; 10+ messages in thread
From: Nicholas Piggin @ 2018-05-02 0:12 UTC (permalink / raw)
To: Joel Fernandes
Cc: Steven Rostedt, Peter Zijlstra, Ingo Molnar, Linux Kernel Mailing List
On Tue, 1 May 2018 14:15:14 -0700
Joel Fernandes <joel.opensrc@gmail.com> wrote:
> On Tue, May 1, 2018 at 1:00 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Tue, 1 May 2018 21:48:38 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> >
> >> On Tue, May 01, 2018 at 03:38:40PM -0400, Steven Rostedt wrote:
> >> > On Tue, 1 May 2018 21:19:51 +0200
> >> > Peter Zijlstra <peterz@infradead.org> wrote:
> >>
> >> > > Now, lockdep only minimally tracks these otherwise redundant operations;
> >> > > see redundant_hardirqs_{on,off} counters, and loosing that doesn't seen
> >> > > like a big issue.
> >> > >
> >> > > But I'm confused how this helps track superfluous things, it looks like
> >> > > it explicitly tracks _less_ superfluous transitions.
> >> >
> >> > I think it is about triggering on OFF->OFF a warning, as that would
> >> > only happen if we have:
> >> >
> >> > local_irq_save(flags);
> >> > [..]
> >> > local_irq_disable();
> >> >
> >>
> >> Ahh, ok. Yes, that is easier to do with these changes. The alternative
> >> is to add more information to the tracehooks such that we can do the
> >> same internally, but whatever.
> >>
> >> Yeah, I'm fine with the proposed change, but maybe improve the Changelog
> >> a little for slow people like me :-)
> >
> > Great!
> >
> > Nicholas,
> >
> > I know this is an old patch (from last November), but want to send it
> > again with a proper change log and signed off by?
>
> I actually wrote the exact same patch yesterday with changes Matsami
> suggested. However I decided not to send it, since it didn't have any
> performance improvement (which was the reason I wrote it).
>
> Also with my recent set, I don't think it will help detect repeated
> calls to trace_hardirqs_off because we are handling that recursive
> case by using per-cpu variable and bailing out if there is a
> recursion, before even calling into lockdep.
>
> I have mixed feelings about this patch, I am Ok with this patch but I
> suggest its sent with the follow-up patch that shows its use of this.
> And also appreciate if such a follow-up patch is rebased onto the IRQ
> tracepoint work: https://patchwork.kernel.org/patch/10373129/
>
> What do you think?
I'll try to dig it up and resend. Thanks for the feedback on it.
Thanks,
Nick
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] tracing/irqtrace: only call trace_hardirqs_on/off when state changes
2018-05-02 0:12 ` Nicholas Piggin
@ 2018-07-11 1:44 ` Steven Rostedt
2018-07-11 5:58 ` Joel Fernandes
0 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2018-07-11 1:44 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Joel Fernandes, Peter Zijlstra, Ingo Molnar, Linux Kernel Mailing List
On Wed, 2 May 2018 10:12:14 +1000
Nicholas Piggin <npiggin@gmail.com> wrote:
> > I have mixed feelings about this patch, I am Ok with this patch but I
> > suggest its sent with the follow-up patch that shows its use of this.
> > And also appreciate if such a follow-up patch is rebased onto the IRQ
> > tracepoint work: https://patchwork.kernel.org/patch/10373129/
> >
> > What do you think?
>
> I'll try to dig it up and resend. Thanks for the feedback on it.
Joel,
With your latest patches, is this obsolete?
-- Steve
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tracing/irqtrace: only call trace_hardirqs_on/off when state changes
2018-07-11 1:44 ` Steven Rostedt
@ 2018-07-11 5:58 ` Joel Fernandes
0 siblings, 0 replies; 10+ messages in thread
From: Joel Fernandes @ 2018-07-11 5:58 UTC (permalink / raw)
To: Steven Rostedt
Cc: Nicholas Piggin, Joel Fernandes, Peter Zijlstra, Ingo Molnar,
Linux Kernel Mailing List
On Tue, Jul 10, 2018 at 09:44:57PM -0400, Steven Rostedt wrote:
> On Wed, 2 May 2018 10:12:14 +1000
> Nicholas Piggin <npiggin@gmail.com> wrote:
>
> > > I have mixed feelings about this patch, I am Ok with this patch but I
> > > suggest its sent with the follow-up patch that shows its use of this.
> > > And also appreciate if such a follow-up patch is rebased onto the IRQ
> > > tracepoint work: https://patchwork.kernel.org/patch/10373129/
> > >
> > > What do you think?
> >
> > I'll try to dig it up and resend. Thanks for the feedback on it.
>
> Joel,
>
> With your latest patches, is this obsolete?
Yes, that's right. This patch isn't needed for what I was doing (improving
the performance of the irq disable/enable path). I didn't see any performance
improvement with this patch. Instead, the patches in my series use SRCU to
improve the performance of the tracepoints.
thanks,
- Joel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-07-11 5:58 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-16 16:15 [PATCH] tracing/irqtrace: only call trace_hardirqs_on/off when state changes Nicholas Piggin
2018-05-01 18:46 ` Steven Rostedt
2018-05-01 19:19 ` Peter Zijlstra
2018-05-01 19:38 ` Steven Rostedt
2018-05-01 19:48 ` Peter Zijlstra
2018-05-01 20:00 ` Steven Rostedt
2018-05-01 21:15 ` Joel Fernandes
2018-05-02 0:12 ` Nicholas Piggin
2018-07-11 1:44 ` Steven Rostedt
2018-07-11 5:58 ` Joel Fernandes
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).