tracing/irqtrace: only call trace_hardirqs_on/off when state changes

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 <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc [email protected]

Signed-off-by Nicholas Piggin <[email protected]>

---
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(-)

include/linux/irqflags.h +6 -3

index 46cb57d5eb13..82287e1c6c52 100644
@@ -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)

Quoted 8 lines · On Fri, 17 Nov 2017 02:15:06 +1000 Nicholas Piggin <[email protected]> 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?

include/linux/irqflags.h do { \ · line 115

Quoted 8 lines

include/linux/irqflags.h +2 -1

@@ -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)

include/linux/irqflags.h do { \ · line 123

Quoted 7 lines

include/linux/irqflags.h +2 -1

@@ -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();	\

include/linux/irqflags.h do { \ · line 129

Quoted 7 lines

include/linux/irqflags.h +2 -1

 		} else {				\
-			trace_hardirqs_on();		\
+			if (irqs_disabled())		\
+				trace_hardirqs_on();	\
 			raw_local_irq_restore(flags);	\
 		}					\
 	} while (0)

Quoted 12 lines · Steven Rostedt

Quoted 8 lines · On Fri, 17 Nov 2017 02:15:06 +1000 Nicholas Piggin <[email protected]> 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.

Quoted 6 lines

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?

Quoted 8 lines

include/linux/irqflags.h do { \ · line 115

Quoted 8 lines

include/linux/irqflags.h +2 -1

@@ -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.

Quoted 7 lines

include/linux/irqflags.h do { \ · line 123

Quoted 7 lines

include/linux/irqflags.h +2 -1

@@ -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.

Quoted 7 lines

include/linux/irqflags.h do { \ · line 129

Quoted 7 lines

include/linux/irqflags.h +2 -1

 		} 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.

Quoted 6 lines · On Tue, 1 May 2018 21:19:51 +0200 Peter Zijlstra <[email protected]> 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();

Quoted 10 lines · Steven Rostedt

Quoted 6 lines · On Tue, 1 May 2018 21:19:51 +0200 Peter Zijlstra <[email protected]> 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.

Quoted 7 lines

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 :-)

Quoted 19 lines · On Tue, 1 May 2018 21:48:38 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 10 lines · Steven Rostedt

Quoted 6 lines · On Tue, 1 May 2018 21:19:51 +0200 Peter Zijlstra <[email protected]> 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.

Quoted 7 lines

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();

Quoted 6 lines

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?

Quoted 29 lines · Steven Rostedt

Quoted 19 lines · On Tue, 1 May 2018 21:48:38 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 10 lines · Steven Rostedt

Quoted 6 lines · On Tue, 1 May 2018 21:19:51 +0200 Peter Zijlstra <[email protected]> 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.

Quoted 7 lines

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();

Quoted 6 lines

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 :-)

Quoted 6 lines

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?

Quoted 6 lines

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.

Quoted 5 lines · On Wed, 2 May 2018 10:12:14 +1000 Nicholas Piggin <[email protected]> wrote:

Quoted 5 lines

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/

Quoted 3 lines

Quoted 1 line

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?