The previous commit introduced the CONFIG_TRACE_IRQFLAGS_TOGGLE symbol.
This patch implements the actual infrastructure to allow tracing
irq_disable and irq_enable events without pulling in the heavy
CONFIG_TRACE_IRQFLAGS dependencies like lockdep or the irqsoff tracer.
The implementation hooks into the local_irq_* macros in irqflags.h.
Instead of using the heavy trace_hardirqs_on/off calls, it uses
lightweight tracepoint_enabled() checks. If the tracepoint is enabled,
it calls into specific wrapper functions in trace_preemptirq.c.
These wrappers check the raw hardware state via raw_irqs_disabled() to
filter out redundant events, such as disabling interrupts when they
are already disabled. This approach is simpler than the full
TRACE_IRQFLAGS method which requires maintaining a per-cpu software
state variable.
To support this, the tracepoint definitions are exposed under the new
configuration. Additionally, a circular header dependency involving
irqflags.h, tracepoint-defs.h, and atomic.h is resolved by moving the
atomic.h inclusion to tracepoint.h, allowing irqflags.h to include
tracepoint-defs.h safely.
Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
include/linux/irqflags.h | 62 ++++++++++++++++++++++++++++++-
include/linux/tracepoint-defs.h | 1 -
include/linux/tracepoint.h | 1 +
include/trace/events/preemptirq.h | 2 +-
kernel/trace/trace_preemptirq.c | 49 ++++++++++++++++++++++++
5 files changed, 112 insertions(+), 3 deletions(-)
diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 57b074e0cfbbb..f40557bebd325 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -18,6 +18,19 @@
#include <asm/irqflags.h>
#include <asm/percpu.h>
+/*
+ * Avoid the circular dependency
+ * irqflags.h <-----------------+
+ * tracepoint_defs.h |
+ * static_key.h |
+ * jump_label.h |
+ * atomic.h |
+ * cmpxchg.h ---------+
+ */
+#ifdef CONFIG_TRACE_IRQFLAGS_TOGGLE
+#include <linux/tracepoint-defs.h>
+#endif
+
struct task_struct;
/* Currently lockdep_softirqs_on/off is used only by lockdep */
@@ -232,7 +245,54 @@ extern void warn_bogus_irq_restore(void);
} while (0)
-#else /* !CONFIG_TRACE_IRQFLAGS */
+#elif defined(CONFIG_TRACE_IRQFLAGS_TOGGLE) /* !CONFIG_TRACE_IRQFLAGS */
+
+DECLARE_TRACEPOINT(irq_enable);
+DECLARE_TRACEPOINT(irq_disable);
+
+void trace_local_irq_enable(void);
+void trace_local_irq_disable(void);
+void trace_local_irq_save(unsigned long flags);
+void trace_local_irq_restore(unsigned long flags);
+void trace_safe_halt(void);
+
+#define local_irq_enable() \
+ do { \
+ if (tracepoint_enabled(irq_enable)) \
+ trace_local_irq_enable(); \
+ raw_local_irq_enable(); \
+ } while (0)
+
+#define local_irq_disable() \
+ do { \
+ if (tracepoint_enabled(irq_disable)) \
+ trace_local_irq_disable(); \
+ else \
+ raw_local_irq_disable(); \
+ } while (0)
+
+#define local_irq_save(flags) \
+ do { \
+ raw_local_irq_save(flags); \
+ if (tracepoint_enabled(irq_disable)) \
+ trace_local_irq_save(flags); \
+ } while (0)
+
+#define local_irq_restore(flags) \
+ do { \
+ if (tracepoint_enabled(irq_enable)) \
+ trace_local_irq_restore(flags); \
+ raw_local_irq_restore(flags); \
+ } while (0)
+
+#define safe_halt() \
+ do { \
+ if (tracepoint_enabled(irq_enable)) \
+ trace_safe_halt(); \
+ raw_safe_halt(); \
+ } while (0)
+
+#else /* !CONFIG_TRACE_IRQFLAGS_TOGGLE */
#define local_irq_enable() do { raw_local_irq_enable(); } while (0)
#define local_irq_disable() do { raw_local_irq_disable(); } while (0)
diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
index aebf0571c736e..cb1f15a4e43f0 100644
--- a/include/linux/tracepoint-defs.h
+++ b/include/linux/tracepoint-defs.h
@@ -8,7 +8,6 @@
* trace_print_flags{_u64}. Otherwise linux/tracepoint.h should be used.
*/
-#include <linux/atomic.h>
#include <linux/static_key.h>
struct static_call_key;
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 22ca1c8b54f32..e7d8c5ca00c79 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -20,6 +20,7 @@
#include <linux/rcupdate_trace.h>
#include <linux/tracepoint-defs.h>
#include <linux/static_call.h>
+#include <linux/atomic.h>
struct module;
struct tracepoint;
diff --git a/include/trace/events/preemptirq.h b/include/trace/events/preemptirq.h
index f99562d2b496b..a607a6f4e29ca 100644
--- a/include/trace/events/preemptirq.h
+++ b/include/trace/events/preemptirq.h
@@ -32,7 +32,7 @@ DECLARE_EVENT_CLASS(preemptirq_template,
(void *)((unsigned long)(_stext) + __entry->parent_offs))
);
-#ifdef CONFIG_TRACE_IRQFLAGS
+#if defined(CONFIG_TRACE_IRQFLAGS) || defined(CONFIG_TRACE_IRQFLAGS_TOGGLE)
DEFINE_EVENT(preemptirq_template, irq_disable,
TP_PROTO(unsigned long ip, unsigned long parent_ip),
TP_ARGS(ip, parent_ip));
diff --git a/kernel/trace/trace_preemptirq.c b/kernel/trace/trace_preemptirq.c
index 9f098fcb28012..0f32da96d2f01 100644
--- a/kernel/trace/trace_preemptirq.c
+++ b/kernel/trace/trace_preemptirq.c
@@ -111,8 +111,57 @@ void trace_hardirqs_off(void)
}
EXPORT_SYMBOL(trace_hardirqs_off);
NOKPROBE_SYMBOL(trace_hardirqs_off);
+
#endif /* CONFIG_TRACE_IRQFLAGS */
+#ifdef CONFIG_TRACE_IRQFLAGS_TOGGLE
+EXPORT_TRACEPOINT_SYMBOL(irq_disable);
+EXPORT_TRACEPOINT_SYMBOL(irq_enable);
+
+void trace_local_irq_enable(void)
+{
+ if (raw_irqs_disabled())
+ trace(irq_enable, TP_ARGS(CALLER_ADDR0, CALLER_ADDR1));
+}
+EXPORT_SYMBOL(trace_local_irq_enable);
+NOKPROBE_SYMBOL(trace_local_irq_enable);
+
+void trace_local_irq_disable(void)
+{
+ const bool was_disabled = raw_irqs_disabled();
+
+ raw_local_irq_disable();
+ if (!was_disabled)
+ trace(irq_disable, TP_ARGS(CALLER_ADDR0, CALLER_ADDR1));
+}
+EXPORT_SYMBOL(trace_local_irq_disable);
+NOKPROBE_SYMBOL(trace_local_irq_disable);
+
+void trace_local_irq_save(unsigned long flags)
+{
+ if (!raw_irqs_disabled_flags(flags))
+ trace(irq_disable, TP_ARGS(CALLER_ADDR0, CALLER_ADDR1));
+}
+EXPORT_SYMBOL(trace_local_irq_save);
+NOKPROBE_SYMBOL(trace_local_irq_save);
+
+void trace_local_irq_restore(unsigned long flags)
+{
+ if (!raw_irqs_disabled_flags(flags) && raw_irqs_disabled())
+ trace(irq_enable, TP_ARGS(CALLER_ADDR0, CALLER_ADDR1));
+}
+EXPORT_SYMBOL(trace_local_irq_restore);
+NOKPROBE_SYMBOL(trace_local_irq_restore);
+
+void trace_safe_halt(void)
+{
+ if (raw_irqs_disabled())
+ trace(irq_enable, TP_ARGS(CALLER_ADDR0, CALLER_ADDR1));
+}
+EXPORT_SYMBOL(trace_safe_halt);
+NOKPROBE_SYMBOL(trace_safe_halt);
+#endif /* CONFIG_TRACE_IRQFLAGS_TOGGLE */
+
#ifdef CONFIG_TRACE_PREEMPT_TOGGLE
#if !defined(CONFIG_DEBUG_PREEMPT) && !defined(CONFIG_PREEMPT_TRACER)
--
2.53.0On Wed, Mar 11, 2026 at 09:50:18AM -0300, Wander Lairson Costa wrote:
> +#define local_irq_enable() \
> + do { \
> + if (tracepoint_enabled(irq_enable)) \
> + trace_local_irq_enable(); \
I'm thinking you didn't even look at the assembly generated :/
Otherwise you would have written this like:
if (tracepoint_enabled(irq_enable))
__do_trace_local_irq_enable();
> + raw_local_irq_enable(); \
> + } while (0)
Again, this was one instruction, and you clearly didn't bother looking
at the mess you've generated :/
On Wed, Mar 11, 2026 at 08:43:05PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 11, 2026 at 09:50:18AM -0300, Wander Lairson Costa wrote:
> > +#define local_irq_enable() \
> > + do { \
> > + if (tracepoint_enabled(irq_enable)) \
> > + trace_local_irq_enable(); \
>
> I'm thinking you didn't even look at the assembly generated :/
Yes, I did. But I was more concerned with the hot path, making sure it
only adds the NOP instruction generated by the static_key machinery.
>
> Otherwise you would have written this like:
>
> if (tracepoint_enabled(irq_enable))
> __do_trace_local_irq_enable();
>
> > + raw_local_irq_enable(); \
> > + } while (0)
>
Steve already opposed to using internal functions. When trace invoke
public API is available, we can always come back and replace the call.
> Again, this was one instruction, and you clearly didn't bother looking
> at the mess you've generated :/
>
I might have missed something (as was the case for preempt_enable), but
I spent quite some time looking at the assembly code. I did my best, but
there lots of people far more skilled than me. And patch submission is
the way to connect to these people.
On Wed, 11 Mar 2026 20:43:05 +0100
Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Mar 11, 2026 at 09:50:18AM -0300, Wander Lairson Costa wrote:
> > +#define local_irq_enable() \
> > + do { \
> > + if (tracepoint_enabled(irq_enable)) \
> > + trace_local_irq_enable(); \
>
> I'm thinking you didn't even look at the assembly generated :/
>
> Otherwise you would have written this like:
>
> if (tracepoint_enabled(irq_enable))
> __do_trace_local_irq_enable();
Please don't use the internal functions outside of the tracepoint.h
Vineeth is currently working on a patch set to properly do that. It's going
to introduce:
trace_invoke_<event>()
Which basically is just __do_trace_<event>(), but as a wrapper that can
handle updates that may be needed, but supplies a proper API where thing
wont randomly break when __do_trace_<event>() changes.
-- Steve
>
> > + raw_local_irq_enable(); \
> > + } while (0)
>
> Again, this was one instruction, and you clearly didn't bother looking
> at the mess you've generated :/
On Wed, Mar 11, 2026 at 03:48:42PM -0400, Steven Rostedt wrote:
> On Wed, 11 Mar 2026 20:43:05 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
>
> > On Wed, Mar 11, 2026 at 09:50:18AM -0300, Wander Lairson Costa wrote:
> > > +#define local_irq_enable() \
> > > + do { \
> > > + if (tracepoint_enabled(irq_enable)) \
> > > + trace_local_irq_enable(); \
> >
> > I'm thinking you didn't even look at the assembly generated :/
> >
> > Otherwise you would have written this like:
> >
> > if (tracepoint_enabled(irq_enable))
> > __do_trace_local_irq_enable();
>
> Please don't use the internal functions outside of the tracepoint.h
>
> Vineeth is currently working on a patch set to properly do that. It's going
> to introduce:
>
> trace_invoke_<event>()
>
> Which basically is just __do_trace_<event>(), but as a wrapper that can
> handle updates that may be needed, but supplies a proper API where thing
> wont randomly break when __do_trace_<event>() changes.
That's like a 3 line patch, hardly worth the effort. Its not like it'll
be hard to find and fix any users if you do ever change that.
On Wed, 11 Mar 2026 20:53:10 +0100
Peter Zijlstra <peterz@infradead.org> wrote:
> > Which basically is just __do_trace_<event>(), but as a wrapper that can
> > handle updates that may be needed, but supplies a proper API where thing
> > wont randomly break when __do_trace_<event>() changes.
>
> That's like a 3 line patch, hardly worth the effort. Its not like it'll
> be hard to find and fix any users if you do ever change that.
No, but I prefer clean code, and not hacks that use internal functions with
underscores in their names. Not to mention, it properly handles different
cases:
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 22ca1c8b54f3..07219316a8e1 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -294,6 +294,10 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
WARN_ONCE(!rcu_is_watching(), \
"RCU not watching for tracepoint"); \
} \
+ } \
+ static inline void trace_invoke_##name(proto) \
+ { \
+ __do_trace_##name(args); \
}
#define __DECLARE_TRACE_SYSCALL(name, proto, args, data_proto) \
@@ -313,6 +317,11 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
WARN_ONCE(!rcu_is_watching(), \
"RCU not watching for tracepoint"); \
} \
+ } \
+ static inline void trace_invoke_##name(proto) \
+ { \
+ might_fault(); \
+ __do_trace_##name(args); \
}
Then it goes through and updates every location that has a:
if (trace_<event>_enabled()) {
[..]
trace_<event>();
}
With the new proper API.
-- Steve
On Wed, Mar 11, 2026 at 04:07:14PM -0400, Steven Rostedt wrote:
> On Wed, 11 Mar 2026 20:53:10 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
>
> > > Which basically is just __do_trace_<event>(), but as a wrapper that can
> > > handle updates that may be needed, but supplies a proper API where thing
> > > wont randomly break when __do_trace_<event>() changes.
> >
> > That's like a 3 line patch, hardly worth the effort. Its not like it'll
> > be hard to find and fix any users if you do ever change that.
>
> No, but I prefer clean code, and not hacks that use internal functions with
> underscores in their names. Not to mention, it properly handles different
> cases:
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 22ca1c8b54f3..07219316a8e1 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -294,6 +294,10 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> WARN_ONCE(!rcu_is_watching(), \
> "RCU not watching for tracepoint"); \
> } \
> + } \
> + static inline void trace_invoke_##name(proto) \
> + { \
> + __do_trace_##name(args); \
> }
>
> #define __DECLARE_TRACE_SYSCALL(name, proto, args, data_proto) \
> @@ -313,6 +317,11 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> WARN_ONCE(!rcu_is_watching(), \
> "RCU not watching for tracepoint"); \
> } \
> + } \
> + static inline void trace_invoke_##name(proto) \
> + { \
> + might_fault(); \
> + __do_trace_##name(args); \
> }
>
>
> Then it goes through and updates every location that has a:
>
> if (trace_<event>_enabled()) {
> [..]
> trace_<event>();
> }
We have Cocinelle for that :-), and while I absolutely suck at writing
Cocinelle, I had some limited success using Gemini to write some for me
the other day.
On Wed, 11 Mar 2026 21:46:07 +0100
Peter Zijlstra <peterz@infradead.org> wrote:
> > Then it goes through and updates every location that has a:
> >
> > if (trace_<event>_enabled()) {
> > [..]
> > trace_<event>();
> > }
>
> We have Cocinelle for that :-), and while I absolutely suck at writing
> Cocinelle, I had some limited success using Gemini to write some for me
> the other day.
Heh, I believe Vineeth used claude ;-)
-- Steve
© 2016 - 2026 Red Hat, Inc.