x86/entry: Consolidation - Part II

Subject: [patch 1/8] x86/entry/64: Trace irqflags unconditionally on when returing to user space

User space cannot longer disable interrupts so trace return to user space

unconditionally as IRQS_ON.

Signed-off-by Thomas Gleixner <[email protected]>

User space cannot longer disable interrupts so trace return to user space unconditionally as IRQS_ON.

Signed-off-by Thomas Gleixner <[email protected]>

User space cannot longer disable interrupts so trace return to user space unconditionally as IRQS_ON.

Signed-off-by Thomas Gleixner <[email protected]>

---
  arch/x86/entry/entry_64.S |    4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

All syscall entry points call enter_from_user_mode() and local_irq_enable(). Move that into an inline helper so the interrupt tracing can be moved into that helper later instead of sprinkling it all over the place.

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/entry/common.c |   48 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 15 deletions(-)

All syscall entry points call enter_from_user_mode() and local_irq_enable(). Move that into an inline helper so the interrupt tracing can be moved into that helper later instead of sprinkling it all over the place.

Signed-off-by Thomas Gleixner <[email protected]>

All syscall entry points call enter_from_user_mode() and local_irq_enable(). Move that into an inline helper so the interrupt tracing can be moved into that helper later instead of sprinkling it all over the place.

Signed-off-by Thomas Gleixner <[email protected]>

---
  arch/x86/entry/common.c |   48 +++++++++++++++++++++++++++++++++---------------
  1 file changed, 33 insertions(+), 15 deletions(-)

Anything before enter_from_user_mode() is not safe to be traced or probed.

Anything before enter_from_user_mode() is not safe to be traced or probed.

Signed-off-by Thomas Gleixner <[email protected]>

---
  arch/x86/entry/common.c |    9 ++++++---
  1 file changed, 6 insertions(+), 3 deletions(-)

Now that the C entry points are safe, move the irq flags tracing code into the entry helper.

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/entry/common.c          |    5 +++++
 arch/x86/entry/entry_32.S        |   12 ------------
 arch/x86/entry/entry_64.S        |    2 --
 arch/x86/entry/entry_64_compat.S |   18 ------------------
 4 files changed, 5 insertions(+), 32 deletions(-)

arch/x86/entry/common.c +0 -0

 	enter_from_user_mode();
 	local_irq_enable();
 }

Quoted 2 lines · Thomas Gleixner

Now that the C entry points are safe, move the irq flags tracing code into the entry helper.

I'm so confused.

Quoted 12 lines

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/entry/common.c          |    5 +++++
 arch/x86/entry/entry_32.S        |   12 ------------
 arch/x86/entry/entry_64.S        |    2 --
 arch/x86/entry/entry_64_compat.S |   18 ------------------
 4 files changed, 5 insertions(+), 32 deletions(-)

arch/x86/entry/common.c static inline void enter_from_user_mode( · line 64

Quoted 8 lines

arch/x86/entry/common.c +5 -0

@@ -57,6 +57,11 @@ static inline void enter_from_user_mode(
  */
 static __always_inline void syscall_entry_fixups(void)
 {
+	/*
+	 * Usermode is traced as interrupts enabled, but the syscall entry
+	 * mechanisms disable interrupts. Tell the tracer.
+	 */
+	trace_hardirqs_off();

Your earlier patches suggest quite strongly that tracing isn't safe until enter_from_user_mode(). But trace_hardirqs_off() calls trace_irq_disable_rcuidle(), which looks [0] like a tracepoint.

Did you perhaps mean to do this *after* enter_from_user_mode()?

Quoted 31 lines · Andy Lutomirski

Quoted 3 lines · Thomas Gleixner

Now that the C entry points are safe, move the irq flags tracing code into the entry helper.

I'm so confused.

Quoted 12 lines

Signed-off-by Thomas Gleixner <[email protected]>

---

arch/x86/entry/common.c          |    5 +++++
arch/x86/entry/entry_32.S        |   12 ------------
arch/x86/entry/entry_64.S        |    2 --
arch/x86/entry/entry_64_compat.S |   18 ------------------
4 files changed, 5 insertions(+), 32 deletions(-)

arch/x86/entry/common.c static inline void enter_from_user_mode( · line 64

Quoted 8 lines

arch/x86/entry/common.c +5 -0

@@ -57,6 +57,11 @@ static inline void enter_from_user_mode(
 */
 static __always_inline void syscall_entry_fixups(void)
 {
+    /*
+     * Usermode is traced as interrupts enabled, but the syscall entry
+     * mechanisms disable interrupts. Tell the tracer.
+     */
+    trace_hardirqs_off();

Your earlier patches suggest quite strongly that tracing isn't safe until enter_from_user_mode(). But trace_hardirqs_off() calls trace_irq_disable_rcuidle(), which looks [0] like a tracepoint.

Quoted 1 line

Did you perhaps mean to do this *after* enter_from_user_mode()?

aside from the fact that enter_from_user_mode() itself also has a tracepoint, the crucial detail is that we must not trace/kprobe the function calling this.

Specifically for #PF, because we need read_cr2() before this. See later patches.

Quoted 12 lines · Peter Zijlstra

Quoted 5 lines · Andy Lutomirski

Your earlier patches suggest quite strongly that tracing isn't safe until enter_from_user_mode(). But trace_hardirqs_off() calls trace_irq_disable_rcuidle(), which looks [0] like a tracepoint.

Did you perhaps mean to do this *after* enter_from_user_mode()?

aside from the fact that enter_from_user_mode() itself also has a tracepoint, the crucial detail is that we must not trace/kprobe the function calling this.

Quoted 2 lines

Specifically for #PF, because we need read_cr2() before this. See later patches.

Indeed. I’m fine with this patch, but I still don’t understand what the changelog is about.

And I’m still rather baffled by most of the notrace annotations in the series.

Quoted 12 lines · Andy Lutomirski

Quoted 12 lines · Peter Zijlstra

Quoted 5 lines · Andy Lutomirski

Your earlier patches suggest quite strongly that tracing isn't safe until enter_from_user_mode(). But trace_hardirqs_off() calls trace_irq_disable_rcuidle(), which looks [0] like a tracepoint.

Did you perhaps mean to do this *after* enter_from_user_mode()?

aside from the fact that enter_from_user_mode() itself also has a tracepoint, the crucial detail is that we must not trace/kprobe the function calling this.

Quoted 5 lines

Quoted 2 lines

Specifically for #PF, because we need read_cr2() before this. See later patches.

Indeed. I’m fine with this patch, but I still don’t understand what the changelog is about.

Yeah, the changelog is not really helpful. Let me fix that.

Quoted 2 lines

And I’m still rather baffled by most of the notrace annotations in the series.

As discussed on IRC, this might be too broad, but then I rather have the actual C-entry points neither traceable nor probable in general and relax this by calling functions which can be traced and probed.

My rationale for this decision was that enter_from_user_mode() is marked notrace/noprobe as well, so I kept the protection scope the same as we had in the ASM maze which is marked noprobe already.

Quoted 24 lines · Thomas Gleixner

Quoted 12 lines · Andy Lutomirski

Quoted 12 lines

Quoted 12 lines · Peter Zijlstra

Quoted 5 lines · Andy Lutomirski

Your earlier patches suggest quite strongly that tracing isn't safe until enter_from_user_mode(). But trace_hardirqs_off() calls trace_irq_disable_rcuidle(), which looks [0] like a tracepoint.

Did you perhaps mean to do this *after* enter_from_user_mode()?

aside from the fact that enter_from_user_mode() itself also has a tracepoint, the crucial detail is that we must not trace/kprobe the function calling this.

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Specifically for #PF, because we need read_cr2() before this. See later patches.

Indeed. I’m fine with this patch, but I still don’t understand what the changelog is about.

Yeah, the changelog is not really helpful. Let me fix that.

Quoted 2 lines

And I’m still rather baffled by most of the notrace annotations in the series.

Quoted 7 lines

As discussed on IRC, this might be too broad, but then I rather have the actual C-entry points neither traceable nor probable in general and relax this by calling functions which can be traced and probed.

My rationale for this decision was that enter_from_user_mode() is marked notrace/noprobe as well, so I kept the protection scope the same as we had in the ASM maze which is marked noprobe already.

I have second thoughts vs. tracing in this context.

While the tracer itself seems to handle this correctly, what about things like BPF programs which can be attached to tracepoints and function trace entries?

Is that really safe _before_ context tracking has updated RCU state?

Thanks,

tglx

Quoted 3 lines

While the tracer itself seems to handle this correctly, what about things like BPF programs which can be attached to tracepoints and function trace entries?

I think that everything using the tracing code, including BPF, should either do its own rcuidle stuff or explicitly not execute if we’re not in CONTEXT_KERNEL. That is, we probably need to patch BPF.

Quoted 4 lines · On Sat, 29 Feb 2020 11:25:24 -0800 Andy Lutomirski <[email protected]> wrote:

Quoted 3 lines

While the tracer itself seems to handle this correctly, what about things like BPF programs which can be attached to tracepoints and function trace entries?

Quoted 3 lines

I think that everything using the tracing code, including BPF, should either do its own rcuidle stuff or explicitly not execute if we’re not in CONTEXT_KERNEL. That is, we probably need to patch BPF.

That's basically the route we are taking.

Quoted 11 lines · Steven Rostedt

Quoted 7 lines · On Sat, 29 Feb 2020 11:25:24 -0800 Andy Lutomirski <[email protected]> wrote:

Quoted 3 lines

While the tracer itself seems to handle this correctly, what about things like BPF programs which can be attached to tracepoints and function trace entries?

I think that everything using the tracing code, including BPF, should either do its own rcuidle stuff or explicitly not execute if we’re not in CONTEXT_KERNEL. That is, we probably need to patch BPF.

Quoted 1 line

That's basically the route we are taking.

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Thanks,

tglx

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Quoted 5 lines · Andy Lutomirski

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Quoted 2 lines

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Those are not tracepoints and not going through the macro maze. See kernel/trace/trace_preemptirq.c

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Those are not tracepoints and not going through the macro maze. See kernel/trace/trace_preemptirq.c

That has:

void trace_hardirqs_on(void)

{
        if (this_cpu_read(tracing_irq_cpu)) {
                if (!in_nmi())
                        trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1);
                tracer_hardirqs_on(CALLER_ADDR0, CALLER_ADDR1);
                this_cpu_write(tracing_irq_cpu, 0);
        }

        lockdep_hardirqs_on(CALLER_ADDR0);
}

EXPORT_SYMBOL(trace_hardirqs_on);
NOKPROBE_SYMBOL(trace_hardirqs_on);

But this calls trace_irq_enable_rcuidle(), and that's the part of the macro maze I got lost in. I found:

#ifdef CONFIG_TRACE_IRQFLAGS DEFINE_EVENT(preemptirq_template, irq_disable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));

DEFINE_EVENT(preemptirq_template, irq_enable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));
#else
#define trace_irq_enable(...)
#define trace_irq_disable(...)
#define trace_irq_enable_rcuidle(...)
#define trace_irq_disable_rcuidle(...)
#endif

But the DEFINE_EVENT doesn't have the "_rcuidle" part. And that's where I got lost in the macro maze. I looked at the gcc asm output, and there is, indeed:

But I also don't see why this is any different from any other tracepoint.

Quoted 47 lines · Andy Lutomirski

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Those are not tracepoints and not going through the macro maze. See kernel/trace/trace_preemptirq.c

That has:

void trace_hardirqs_on(void)

{
        if (this_cpu_read(tracing_irq_cpu)) {
                if (!in_nmi())
                        trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1);
                tracer_hardirqs_on(CALLER_ADDR0, CALLER_ADDR1);
                this_cpu_write(tracing_irq_cpu, 0);
        }

        lockdep_hardirqs_on(CALLER_ADDR0);
}

EXPORT_SYMBOL(trace_hardirqs_on);
NOKPROBE_SYMBOL(trace_hardirqs_on);

But this calls trace_irq_enable_rcuidle(), and that's the part of the macro maze I got lost in. I found:

#ifdef CONFIG_TRACE_IRQFLAGS DEFINE_EVENT(preemptirq_template, irq_disable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));

DEFINE_EVENT(preemptirq_template, irq_enable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));
#else
#define trace_irq_enable(...)
#define trace_irq_disable(...)
#define trace_irq_enable_rcuidle(...)
#define trace_irq_disable_rcuidle(...)
#endif

Quoted 3 lines

But the DEFINE_EVENT doesn't have the "_rcuidle" part. And that's where I got lost in the macro maze. I looked at the gcc asm output, and there is, indeed:

DEFINE_EVENT
DECLARE_TRACE

    __DECLARE_TRACE
       __DECLARE_TRACE_RCU
         static inline void trace_##name##_rcuidle(proto)
            __DO_TRACE
               if (rcuidle)
                  ....

Quoted 1 line

But I also don't see why this is any different from any other tracepoint.

Indeed. I took a wrong turn at some point in the macro jungle :)

So tracing itself is fine, but then if you have probes or bpf programs attached to a tracepoint these use rcu_read_lock()/unlock() which is obviosly wrong in rcuidle context.

Quoted 61 lines · Thomas Gleixner

Quoted 47 lines · Andy Lutomirski

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Those are not tracepoints and not going through the macro maze. See kernel/trace/trace_preemptirq.c

That has:

void trace_hardirqs_on(void)

{
        if (this_cpu_read(tracing_irq_cpu)) {
                if (!in_nmi())
                        trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1);
                tracer_hardirqs_on(CALLER_ADDR0, CALLER_ADDR1);
                this_cpu_write(tracing_irq_cpu, 0);
        }

        lockdep_hardirqs_on(CALLER_ADDR0);
}

EXPORT_SYMBOL(trace_hardirqs_on);
NOKPROBE_SYMBOL(trace_hardirqs_on);

But this calls trace_irq_enable_rcuidle(), and that's the part of the macro maze I got lost in. I found:

#ifdef CONFIG_TRACE_IRQFLAGS DEFINE_EVENT(preemptirq_template, irq_disable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));

DEFINE_EVENT(preemptirq_template, irq_enable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));
#else
#define trace_irq_enable(...)
#define trace_irq_disable(...)
#define trace_irq_enable_rcuidle(...)
#define trace_irq_disable_rcuidle(...)
#endif

Quoted 3 lines

But the DEFINE_EVENT doesn't have the "_rcuidle" part. And that's where I got lost in the macro maze. I looked at the gcc asm output, and there is, indeed:

DEFINE_EVENT
DECLARE_TRACE

    __DECLARE_TRACE
       __DECLARE_TRACE_RCU
         static inline void trace_##name##_rcuidle(proto)
            __DO_TRACE
               if (rcuidle)
                  ....

Quoted 7 lines

Quoted 1 line

But I also don't see why this is any different from any other tracepoint.

Indeed. I took a wrong turn at some point in the macro jungle :)

So tracing itself is fine, but then if you have probes or bpf programs attached to a tracepoint these use rcu_read_lock()/unlock() which is obviosly wrong in rcuidle context.

Definitely, any such code needs to use tricks similar to that of the tracing code. Or instead use something like SRCU, which is OK with readers from idle. Or use something like Steve Rostedt's workqueue-based approach, though please be very careful with this latter, lest the battery-powered embedded guys come after you for waking up idle CPUs too often. ;-)

Quoted 71 lines · Paul E. McKenney

Quoted 61 lines · Thomas Gleixner

Quoted 47 lines · Andy Lutomirski

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Those are not tracepoints and not going through the macro maze. See kernel/trace/trace_preemptirq.c

That has:

void trace_hardirqs_on(void)

{
        if (this_cpu_read(tracing_irq_cpu)) {
                if (!in_nmi())
                        trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1);
                tracer_hardirqs_on(CALLER_ADDR0, CALLER_ADDR1);
                this_cpu_write(tracing_irq_cpu, 0);
        }

        lockdep_hardirqs_on(CALLER_ADDR0);
}

EXPORT_SYMBOL(trace_hardirqs_on);
NOKPROBE_SYMBOL(trace_hardirqs_on);

But this calls trace_irq_enable_rcuidle(), and that's the part of the macro maze I got lost in. I found:

#ifdef CONFIG_TRACE_IRQFLAGS DEFINE_EVENT(preemptirq_template, irq_disable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));

DEFINE_EVENT(preemptirq_template, irq_enable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));
#else
#define trace_irq_enable(...)
#define trace_irq_disable(...)
#define trace_irq_enable_rcuidle(...)
#define trace_irq_disable_rcuidle(...)
#endif

Quoted 3 lines

But the DEFINE_EVENT doesn't have the "_rcuidle" part. And that's where I got lost in the macro maze. I looked at the gcc asm output, and there is, indeed:

DEFINE_EVENT
DECLARE_TRACE

    __DECLARE_TRACE
       __DECLARE_TRACE_RCU
         static inline void trace_##name##_rcuidle(proto)
            __DO_TRACE
               if (rcuidle)
                  ....

Quoted 7 lines

Quoted 1 line

But I also don't see why this is any different from any other tracepoint.

Indeed. I took a wrong turn at some point in the macro jungle :)

So tracing itself is fine, but then if you have probes or bpf programs attached to a tracepoint these use rcu_read_lock()/unlock() which is obviosly wrong in rcuidle context.

Quoted 6 lines

Definitely, any such code needs to use tricks similar to that of the tracing code. Or instead use something like SRCU, which is OK with readers from idle. Or use something like Steve Rostedt's workqueue-based approach, though please be very careful with this latter, lest the battery-powered embedded guys come after you for waking up idle CPUs too often. ;-)

Are we okay if we somehow ensure that all the entry code before enter_from_user_mode() only does rcuidle tracing variants and has kprobes off? Including for BPF use cases?

It would be *really* nice if we could statically verify this, as has been mentioned elsewhere in the thread. It would also probably be good enough if we could do it at runtime. Maybe with lockdep on, we verify rcu state in tracepoints even if the tracepoint isn't active? And we could plausibly have some widget that could inject something into *every* kprobeable function to check rcu state.

Quoted 79 lines · Andy Lutomirski

Quoted 72 lines · Paul E. McKenney

Quoted 61 lines · Thomas Gleixner

Quoted 47 lines · Andy Lutomirski

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Those are not tracepoints and not going through the macro maze. See kernel/trace/trace_preemptirq.c

That has:

void trace_hardirqs_on(void)

{
       if (this_cpu_read(tracing_irq_cpu)) {
               if (!in_nmi())
                       trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1);
               tracer_hardirqs_on(CALLER_ADDR0, CALLER_ADDR1);
               this_cpu_write(tracing_irq_cpu, 0);
       }

       lockdep_hardirqs_on(CALLER_ADDR0);
}

EXPORT_SYMBOL(trace_hardirqs_on);
NOKPROBE_SYMBOL(trace_hardirqs_on);

But this calls trace_irq_enable_rcuidle(), and that's the part of the macro maze I got lost in. I found:

#ifdef CONFIG_TRACE_IRQFLAGS DEFINE_EVENT(preemptirq_template, irq_disable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));

DEFINE_EVENT(preemptirq_template, irq_enable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));
#else
#define trace_irq_enable(...)
#define trace_irq_disable(...)
#define trace_irq_enable_rcuidle(...)
#define trace_irq_disable_rcuidle(...)
#endif

Quoted 3 lines

But the DEFINE_EVENT doesn't have the "_rcuidle" part. And that's where I got lost in the macro maze. I looked at the gcc asm output, and there is, indeed:

DEFINE_EVENT
DECLARE_TRACE
__DECLARE_TRACE

      __DECLARE_TRACE_RCU
        static inline void trace_##name##_rcuidle(proto)
           __DO_TRACE
              if (rcuidle)
                 ....

Quoted 7 lines

Quoted 1 line

But I also don't see why this is any different from any other tracepoint.

Indeed. I took a wrong turn at some point in the macro jungle :)

So tracing itself is fine, but then if you have probes or bpf programs attached to a tracepoint these use rcu_read_lock()/unlock() which is obviosly wrong in rcuidle context.

Quoted 6 lines

Definitely, any such code needs to use tricks similar to that of the tracing code. Or instead use something like SRCU, which is OK with readers from idle. Or use something like Steve Rostedt's workqueue-based approach, though please be very careful with this latter, lest the battery-powered embedded guys come after you for waking up idle CPUs too often. ;-)

Quoted 3 lines

Are we okay if we somehow ensure that all the entry code before enter_from_user_mode() only does rcuidle tracing variants and has kprobes off? Including for BPF use cases?

That would work, though if BPF used SRCU instead of RCU, this would be unnecessary. Sadly, SRCU has full memory barriers in each of srcu_read_lock() and srcu_read_unlock(), but we are working on it. (As always, no promises!)

Quoted 6 lines

It would be *really* nice if we could statically verify this, as has been mentioned elsewhere in the thread. It would also probably be good enough if we could do it at runtime. Maybe with lockdep on, we verify rcu state in tracepoints even if the tracepoint isn't active? And we could plausibly have some widget that could inject something into *every* kprobeable function to check rcu state.

Or just have at least one testing step that activates all tracepoints, but with lockdep enabled?

Quoted 100 lines · Paul E. McKenney

Quoted 79 lines · Andy Lutomirski

Quoted 72 lines · Paul E. McKenney

Quoted 61 lines · Thomas Gleixner

Quoted 47 lines · Andy Lutomirski

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Those are not tracepoints and not going through the macro maze. See kernel/trace/trace_preemptirq.c

That has:

void trace_hardirqs_on(void)

{
       if (this_cpu_read(tracing_irq_cpu)) {
               if (!in_nmi())
                       trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1);
               tracer_hardirqs_on(CALLER_ADDR0, CALLER_ADDR1);
               this_cpu_write(tracing_irq_cpu, 0);
       }

       lockdep_hardirqs_on(CALLER_ADDR0);
}

EXPORT_SYMBOL(trace_hardirqs_on);
NOKPROBE_SYMBOL(trace_hardirqs_on);

But this calls trace_irq_enable_rcuidle(), and that's the part of the macro maze I got lost in. I found:

#ifdef CONFIG_TRACE_IRQFLAGS DEFINE_EVENT(preemptirq_template, irq_disable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));

DEFINE_EVENT(preemptirq_template, irq_enable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));
#else
#define trace_irq_enable(...)
#define trace_irq_disable(...)
#define trace_irq_enable_rcuidle(...)
#define trace_irq_disable_rcuidle(...)
#endif

Quoted 3 lines

But the DEFINE_EVENT doesn't have the "_rcuidle" part. And that's where I got lost in the macro maze. I looked at the gcc asm output, and there is, indeed:

DEFINE_EVENT
DECLARE_TRACE
__DECLARE_TRACE

      __DECLARE_TRACE_RCU
        static inline void trace_##name##_rcuidle(proto)
           __DO_TRACE
              if (rcuidle)
                 ....

Quoted 7 lines

Quoted 1 line

But I also don't see why this is any different from any other tracepoint.

Indeed. I took a wrong turn at some point in the macro jungle :)

So tracing itself is fine, but then if you have probes or bpf programs attached to a tracepoint these use rcu_read_lock()/unlock() which is obviosly wrong in rcuidle context.

Quoted 6 lines

Definitely, any such code needs to use tricks similar to that of the tracing code. Or instead use something like SRCU, which is OK with readers from idle. Or use something like Steve Rostedt's workqueue-based approach, though please be very careful with this latter, lest the battery-powered embedded guys come after you for waking up idle CPUs too often. ;-)

Quoted 3 lines

Are we okay if we somehow ensure that all the entry code before enter_from_user_mode() only does rcuidle tracing variants and has kprobes off? Including for BPF use cases?

That would work, though if BPF used SRCU instead of RCU, this would be unnecessary. Sadly, SRCU has full memory barriers in each of srcu_read_lock() and srcu_read_unlock(), but we are working on it. (As always, no promises!)

Quoted 6 lines

It would be *really* nice if we could statically verify this, as has been mentioned elsewhere in the thread. It would also probably be good enough if we could do it at runtime. Maybe with lockdep on, we verify rcu state in tracepoints even if the tracepoint isn't active? And we could plausibly have some widget that could inject something into *every* kprobeable function to check rcu state.

Quoted 2 lines

Or just have at least one testing step that activates all tracepoints, but with lockdep enabled?

Also kprobe.

I don’t suppose we could make notrace imply nokprobe. Then all kprobeable functions would also have entry/exit tracepoints, right?

Quoted 98 lines · Paul E. McKenney

Quoted 79 lines · Andy Lutomirski

Quoted 72 lines · Paul E. McKenney

Quoted 61 lines · Thomas Gleixner

Quoted 47 lines · Andy Lutomirski

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Those are not tracepoints and not going through the macro maze. See kernel/trace/trace_preemptirq.c

That has:

void trace_hardirqs_on(void)

{
       if (this_cpu_read(tracing_irq_cpu)) {
               if (!in_nmi())
                       trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1);
               tracer_hardirqs_on(CALLER_ADDR0, CALLER_ADDR1);
               this_cpu_write(tracing_irq_cpu, 0);
       }

       lockdep_hardirqs_on(CALLER_ADDR0);
}

EXPORT_SYMBOL(trace_hardirqs_on);
NOKPROBE_SYMBOL(trace_hardirqs_on);

But this calls trace_irq_enable_rcuidle(), and that's the part of the macro maze I got lost in. I found:

#ifdef CONFIG_TRACE_IRQFLAGS DEFINE_EVENT(preemptirq_template, irq_disable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));

DEFINE_EVENT(preemptirq_template, irq_enable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));
#else
#define trace_irq_enable(...)
#define trace_irq_disable(...)
#define trace_irq_enable_rcuidle(...)
#define trace_irq_disable_rcuidle(...)
#endif

Quoted 3 lines

But the DEFINE_EVENT doesn't have the "_rcuidle" part. And that's where I got lost in the macro maze. I looked at the gcc asm output, and there is, indeed:

DEFINE_EVENT
DECLARE_TRACE
__DECLARE_TRACE

      __DECLARE_TRACE_RCU
        static inline void trace_##name##_rcuidle(proto)
           __DO_TRACE
              if (rcuidle)
                 ....

Quoted 7 lines

Quoted 1 line

But I also don't see why this is any different from any other tracepoint.

Indeed. I took a wrong turn at some point in the macro jungle :)

So tracing itself is fine, but then if you have probes or bpf programs attached to a tracepoint these use rcu_read_lock()/unlock() which is obviosly wrong in rcuidle context.

Quoted 6 lines

Definitely, any such code needs to use tricks similar to that of the tracing code. Or instead use something like SRCU, which is OK with readers from idle. Or use something like Steve Rostedt's workqueue-based approach, though please be very careful with this latter, lest the battery-powered embedded guys come after you for waking up idle CPUs too often. ;-)

Quoted 3 lines

Are we okay if we somehow ensure that all the entry code before enter_from_user_mode() only does rcuidle tracing variants and has kprobes off? Including for BPF use cases?

That would work, though if BPF used SRCU instead of RCU, this would be unnecessary. Sadly, SRCU has full memory barriers in each of srcu_read_lock() and srcu_read_unlock(), but we are working on it. (As always, no promises!)

Quoted 6 lines

It would be *really* nice if we could statically verify this, as has been mentioned elsewhere in the thread. It would also probably be good enough if we could do it at runtime. Maybe with lockdep on, we verify rcu state in tracepoints even if the tracepoint isn't active? And we could plausibly have some widget that could inject something into *every* kprobeable function to check rcu state.

Quoted 2 lines

Or just have at least one testing step that activates all tracepoints, but with lockdep enabled?

Also kprobe.

I don’t suppose we could make notrace imply nokprobe. Then all kprobeable functions would also have entry/exit tracepoints, right?

Quoted 91 lines · On Sun, 1 Mar 2020 11:39:42 -0800 Andy Lutomirski <[email protected]> wrote:

Quoted 91 lines · Paul E. McKenney

Quoted 79 lines · Andy Lutomirski

Quoted 72 lines · Paul E. McKenney

Quoted 61 lines · Thomas Gleixner

Quoted 47 lines · Andy Lutomirski

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Those are not tracepoints and not going through the macro maze. See kernel/trace/trace_preemptirq.c

That has:

void trace_hardirqs_on(void)

{
       if (this_cpu_read(tracing_irq_cpu)) {
               if (!in_nmi())
                       trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1);
               tracer_hardirqs_on(CALLER_ADDR0, CALLER_ADDR1);
               this_cpu_write(tracing_irq_cpu, 0);
       }

       lockdep_hardirqs_on(CALLER_ADDR0);
}

EXPORT_SYMBOL(trace_hardirqs_on);
NOKPROBE_SYMBOL(trace_hardirqs_on);

But this calls trace_irq_enable_rcuidle(), and that's the part of the macro maze I got lost in. I found:

#ifdef CONFIG_TRACE_IRQFLAGS DEFINE_EVENT(preemptirq_template, irq_disable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));

DEFINE_EVENT(preemptirq_template, irq_enable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));
#else
#define trace_irq_enable(...)
#define trace_irq_disable(...)
#define trace_irq_enable_rcuidle(...)
#define trace_irq_disable_rcuidle(...)
#endif

Quoted 3 lines

But the DEFINE_EVENT doesn't have the "_rcuidle" part. And that's where I got lost in the macro maze. I looked at the gcc asm output, and there is, indeed:

DEFINE_EVENT
DECLARE_TRACE
__DECLARE_TRACE

      __DECLARE_TRACE_RCU
        static inline void trace_##name##_rcuidle(proto)
           __DO_TRACE
              if (rcuidle)
                 ....

Quoted 7 lines

Quoted 1 line

But I also don't see why this is any different from any other tracepoint.

Indeed. I took a wrong turn at some point in the macro jungle :)

So tracing itself is fine, but then if you have probes or bpf programs attached to a tracepoint these use rcu_read_lock()/unlock() which is obviosly wrong in rcuidle context.

Quoted 6 lines

Definitely, any such code needs to use tricks similar to that of the tracing code. Or instead use something like SRCU, which is OK with readers from idle. Or use something like Steve Rostedt's workqueue-based approach, though please be very careful with this latter, lest the battery-powered embedded guys come after you for waking up idle CPUs too often. ;-)

Quoted 3 lines

Are we okay if we somehow ensure that all the entry code before enter_from_user_mode() only does rcuidle tracing variants and has kprobes off? Including for BPF use cases?

That would work, though if BPF used SRCU instead of RCU, this would be unnecessary. Sadly, SRCU has full memory barriers in each of srcu_read_lock() and srcu_read_unlock(), but we are working on it. (As always, no promises!)

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

It would be *really* nice if we could statically verify this, as has been mentioned elsewhere in the thread. It would also probably be good enough if we could do it at runtime. Maybe with lockdep on, we verify rcu state in tracepoints even if the tracepoint isn't active? And we could plausibly have some widget that could inject something into *every* kprobeable function to check rcu state.

Quoted 6 lines

Quoted 3 lines

Or just have at least one testing step that activates all tracepoints, but with lockdep enabled?

Also kprobe.

Quoted 2 lines

I don’t suppose we could make notrace imply nokprobe. Then all kprobeable functions would also have entry/exit tracepoints, right?

There was some code before that prevented a kprobe from being allowed in something that was not in the ftrace mcount table (which would make this happen). But I think that was changed because it was too restrictive.

Quoted 71 lines · Paul E. McKenney

Quoted 61 lines · Thomas Gleixner

Quoted 47 lines · Andy Lutomirski

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Those are not tracepoints and not going through the macro maze. See kernel/trace/trace_preemptirq.c

That has:

void trace_hardirqs_on(void)

{
        if (this_cpu_read(tracing_irq_cpu)) {
                if (!in_nmi())
                        trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1);
                tracer_hardirqs_on(CALLER_ADDR0, CALLER_ADDR1);
                this_cpu_write(tracing_irq_cpu, 0);
        }

        lockdep_hardirqs_on(CALLER_ADDR0);
}

EXPORT_SYMBOL(trace_hardirqs_on);
NOKPROBE_SYMBOL(trace_hardirqs_on);

But this calls trace_irq_enable_rcuidle(), and that's the part of the macro maze I got lost in. I found:

#ifdef CONFIG_TRACE_IRQFLAGS DEFINE_EVENT(preemptirq_template, irq_disable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));

DEFINE_EVENT(preemptirq_template, irq_enable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));
#else
#define trace_irq_enable(...)
#define trace_irq_disable(...)
#define trace_irq_enable_rcuidle(...)
#define trace_irq_disable_rcuidle(...)
#endif

Quoted 3 lines

But the DEFINE_EVENT doesn't have the "_rcuidle" part. And that's where I got lost in the macro maze. I looked at the gcc asm output, and there is, indeed:

DEFINE_EVENT
DECLARE_TRACE

    __DECLARE_TRACE
       __DECLARE_TRACE_RCU
         static inline void trace_##name##_rcuidle(proto)
            __DO_TRACE
               if (rcuidle)
                  ....

Quoted 7 lines

Quoted 1 line

But I also don't see why this is any different from any other tracepoint.

Indeed. I took a wrong turn at some point in the macro jungle :)

So tracing itself is fine, but then if you have probes or bpf programs attached to a tracepoint these use rcu_read_lock()/unlock() which is obviosly wrong in rcuidle context.

Quoted 7 lines

Definitely, any such code needs to use tricks similar to that of the tracing code. Or instead use something like SRCU, which is OK with readers from idle. Or use something like Steve Rostedt's workqueue-based approach, though please be very careful with this latter, lest the battery-powered embedded guys come after you for waking up idle CPUs too often. ;-)

Are we okay if we somehow ensure that all the entry code before enter_from_user_mode() only does rcuidle tracing variants and has kprobes off? Including for BPF use cases?

It would be *really* nice if we could statically verify this, as has been mentioned elsewhere in the thread. It would also probably be good enough if we could do it at runtime. Maybe with lockdep on, we verify rcu state in tracepoints even if the tracepoint isn't active? And we could plausibly have some widget that could inject something into *every* kprobeable function to check rcu state.

Quoted 84 lines · Andy Lutomirski

Quoted 72 lines · Paul E. McKenney

Quoted 61 lines · Thomas Gleixner

Quoted 47 lines · Andy Lutomirski

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Those are not tracepoints and not going through the macro maze. See kernel/trace/trace_preemptirq.c

That has:

void trace_hardirqs_on(void)

{
       if (this_cpu_read(tracing_irq_cpu)) {
               if (!in_nmi())
                       trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1);
               tracer_hardirqs_on(CALLER_ADDR0, CALLER_ADDR1);
               this_cpu_write(tracing_irq_cpu, 0);
       }

       lockdep_hardirqs_on(CALLER_ADDR0);
}

EXPORT_SYMBOL(trace_hardirqs_on);
NOKPROBE_SYMBOL(trace_hardirqs_on);

But this calls trace_irq_enable_rcuidle(), and that's the part of the macro maze I got lost in. I found:

#ifdef CONFIG_TRACE_IRQFLAGS DEFINE_EVENT(preemptirq_template, irq_disable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));

DEFINE_EVENT(preemptirq_template, irq_enable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));
#else
#define trace_irq_enable(...)
#define trace_irq_disable(...)
#define trace_irq_enable_rcuidle(...)
#define trace_irq_disable_rcuidle(...)
#endif

Quoted 3 lines

But the DEFINE_EVENT doesn't have the "_rcuidle" part. And that's where I got lost in the macro maze. I looked at the gcc asm output, and there is, indeed:

DEFINE_EVENT
DECLARE_TRACE
__DECLARE_TRACE

      __DECLARE_TRACE_RCU
        static inline void trace_##name##_rcuidle(proto)
           __DO_TRACE
              if (rcuidle)
                 ....

Quoted 7 lines

Quoted 1 line

But I also don't see why this is any different from any other tracepoint.

Indeed. I took a wrong turn at some point in the macro jungle :)

So tracing itself is fine, but then if you have probes or bpf programs attached to a tracepoint these use rcu_read_lock()/unlock() which is obviosly wrong in rcuidle context.

Quoted 7 lines

Definitely, any such code needs to use tricks similar to that of the tracing code. Or instead use something like SRCU, which is OK with readers from idle. Or use something like Steve Rostedt's workqueue-based approach, though please be very careful with this latter, lest the battery-powered embedded guys come after you for waking up idle CPUs too often. ;-)

Are we okay if we somehow ensure that all the entry code before enter_from_user_mode() only does rcuidle tracing variants and has kprobes off? Including for BPF use cases?

Quoted 6 lines

It would be *really* nice if we could statically verify this, as has been mentioned elsewhere in the thread. It would also probably be good enough if we could do it at runtime. Maybe with lockdep on, we verify rcu state in tracepoints even if the tracepoint isn't active? And we could plausibly have some widget that could inject something into *every* kprobeable function to check rcu state.

You are talking about verifying that a non-rcuidle tracepoint is not called into when RCU is not watching right? I think that's fine, though I feel lockdep kernels should not be slowed down any more than they already are. I feel over time if we add too many checks to lockdep enabled kernels, then it becomes too slow even for "debug" kernels. May be it is time for a CONFIG_LOCKDEP_SLOW or some such? And then anyone who wants to go crazy on runtime checking can do so. I myself want to add a few.

Note that the checking is being added into "non rcu-idle" tracepoints many of which are probably always called when RCU is watching, making such checking useless for those tracepoints (and slowing them down however less).

Also another note would be that the whole reason we are getting rid of the "make RCU watch when rcuidle" logic in DO_TRACE is because it is slow for tracepoints that are frequently called into. Another reason to do it is because tracepoint callbacks are expected to know what they are doing and turn on RCU watching as appropriate (as consensus on the matter suggests).

thanks,

- Joel

Quoted 104 lines · Joel Fernandes

Quoted 84 lines · Andy Lutomirski

Quoted 72 lines · Paul E. McKenney

Quoted 61 lines · Thomas Gleixner

Quoted 47 lines · Andy Lutomirski

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Those are not tracepoints and not going through the macro maze. See kernel/trace/trace_preemptirq.c

That has:

void trace_hardirqs_on(void)

{
       if (this_cpu_read(tracing_irq_cpu)) {
               if (!in_nmi())
                       trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1);
               tracer_hardirqs_on(CALLER_ADDR0, CALLER_ADDR1);
               this_cpu_write(tracing_irq_cpu, 0);
       }

       lockdep_hardirqs_on(CALLER_ADDR0);
}

EXPORT_SYMBOL(trace_hardirqs_on);
NOKPROBE_SYMBOL(trace_hardirqs_on);

But this calls trace_irq_enable_rcuidle(), and that's the part of the macro maze I got lost in. I found:

#ifdef CONFIG_TRACE_IRQFLAGS DEFINE_EVENT(preemptirq_template, irq_disable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));

DEFINE_EVENT(preemptirq_template, irq_enable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));
#else
#define trace_irq_enable(...)
#define trace_irq_disable(...)
#define trace_irq_enable_rcuidle(...)
#define trace_irq_disable_rcuidle(...)
#endif

Quoted 3 lines

But the DEFINE_EVENT doesn't have the "_rcuidle" part. And that's where I got lost in the macro maze. I looked at the gcc asm output, and there is, indeed:

DEFINE_EVENT
DECLARE_TRACE
__DECLARE_TRACE

      __DECLARE_TRACE_RCU
        static inline void trace_##name##_rcuidle(proto)
           __DO_TRACE
              if (rcuidle)
                 ....

Quoted 7 lines

Quoted 1 line

But I also don't see why this is any different from any other tracepoint.

Indeed. I took a wrong turn at some point in the macro jungle :)

So tracing itself is fine, but then if you have probes or bpf programs attached to a tracepoint these use rcu_read_lock()/unlock() which is obviosly wrong in rcuidle context.

Quoted 7 lines

Definitely, any such code needs to use tricks similar to that of the tracing code. Or instead use something like SRCU, which is OK with readers from idle. Or use something like Steve Rostedt's workqueue-based approach, though please be very careful with this latter, lest the battery-powered embedded guys come after you for waking up idle CPUs too often. ;-)

Are we okay if we somehow ensure that all the entry code before enter_from_user_mode() only does rcuidle tracing variants and has kprobes off? Including for BPF use cases?

Quoted 6 lines

It would be *really* nice if we could statically verify this, as has been mentioned elsewhere in the thread. It would also probably be good enough if we could do it at runtime. Maybe with lockdep on, we verify rcu state in tracepoints even if the tracepoint isn't active? And we could plausibly have some widget that could inject something into *every* kprobeable function to check rcu state.

You are talking about verifying that a non-rcuidle tracepoint is not called into when RCU is not watching right? I think that's fine, though I feel lockdep kernels should not be slowed down any more than they already are. I feel over time if we add too many checks to lockdep enabled kernels, then it becomes too slow even for "debug" kernels. May be it is time for a CONFIG_LOCKDEP_SLOW or some such? And then anyone who wants to go crazy on runtime checking can do so. I myself want to add a few.

Quoted 4 lines

Note that the checking is being added into "non rcu-idle" tracepoints many of which are probably always called when RCU is watching, making such checking useless for those tracepoints (and slowing them down however less).

Indeed. Static analysis would help a lot here.

Quoted 5 lines

Also another note would be that the whole reason we are getting rid of the "make RCU watch when rcuidle" logic in DO_TRACE is because it is slow for tracepoints that are frequently called into. Another reason to do it is because tracepoint callbacks are expected to know what they are doing and turn on RCU watching as appropriate (as consensus on the matter suggests).

Whoa there. We arch people need crystal clear rules as to what tracepoints can be called in what contexts and what is the responsibility of the callbacks.

Quoted 117 lines · Andy Lutomirski

Quoted 104 lines

Quoted 3 lines · Joel Fernandes

Quoted 84 lines · Andy Lutomirski

Quoted 72 lines · Paul E. McKenney

Quoted 61 lines · Thomas Gleixner

Quoted 47 lines · Andy Lutomirski

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Those are not tracepoints and not going through the macro maze. See kernel/trace/trace_preemptirq.c

That has:

void trace_hardirqs_on(void)

{
      if (this_cpu_read(tracing_irq_cpu)) {
              if (!in_nmi())
                      trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1);
              tracer_hardirqs_on(CALLER_ADDR0, CALLER_ADDR1);
              this_cpu_write(tracing_irq_cpu, 0);
      }

      lockdep_hardirqs_on(CALLER_ADDR0);
}

EXPORT_SYMBOL(trace_hardirqs_on);
NOKPROBE_SYMBOL(trace_hardirqs_on);

But this calls trace_irq_enable_rcuidle(), and that's the part of the macro maze I got lost in. I found:

#ifdef CONFIG_TRACE_IRQFLAGS DEFINE_EVENT(preemptirq_template, irq_disable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));

DEFINE_EVENT(preemptirq_template, irq_enable,
TP_PROTO(unsigned long ip, unsigned long parent_ip), TP_ARGS(ip, parent_ip));
#else
#define trace_irq_enable(...)
#define trace_irq_disable(...)
#define trace_irq_enable_rcuidle(...)
#define trace_irq_disable_rcuidle(...)
#endif

Quoted 3 lines

But the DEFINE_EVENT doesn't have the "_rcuidle" part. And that's where I got lost in the macro maze. I looked at the gcc asm output, and there is, indeed:

DEFINE_EVENT
DECLARE_TRACE
__DECLARE_TRACE

     __DECLARE_TRACE_RCU
       static inline void trace_##name##_rcuidle(proto)
          __DO_TRACE
             if (rcuidle)
                ....

Quoted 7 lines

Quoted 1 line

But I also don't see why this is any different from any other tracepoint.

Indeed. I took a wrong turn at some point in the macro jungle :)

So tracing itself is fine, but then if you have probes or bpf programs attached to a tracepoint these use rcu_read_lock()/unlock() which is obviosly wrong in rcuidle context.

Quoted 7 lines

Definitely, any such code needs to use tricks similar to that of the tracing code. Or instead use something like SRCU, which is OK with readers from idle. Or use something like Steve Rostedt's workqueue-based approach, though please be very careful with this latter, lest the battery-powered embedded guys come after you for waking up idle CPUs too often. ;-)

Are we okay if we somehow ensure that all the entry code before enter_from_user_mode() only does rcuidle tracing variants and has kprobes off? Including for BPF use cases?

Quoted 6 lines

It would be *really* nice if we could statically verify this, as has been mentioned elsewhere in the thread. It would also probably be good enough if we could do it at runtime. Maybe with lockdep on, we verify rcu state in tracepoints even if the tracepoint isn't active? And we could plausibly have some widget that could inject something into *every* kprobeable function to check rcu state.

You are talking about verifying that a non-rcuidle tracepoint is not called into when RCU is not watching right? I think that's fine, though I feel lockdep kernels should not be slowed down any more than they already are. I feel over time if we add too many checks to lockdep enabled kernels, then it becomes too slow even for "debug" kernels. May be it is time for a CONFIG_LOCKDEP_SLOW or some such? And then anyone who wants to go crazy on runtime checking can do so. I myself want to add a few.

Quoted 4 lines

Note that the checking is being added into "non rcu-idle" tracepoints many of which are probably always called when RCU is watching, making such checking useless for those tracepoints (and slowing them down however less).

Indeed. Static analysis would help a lot here.

Quoted 5 lines

Also another note would be that the whole reason we are getting rid of the "make RCU watch when rcuidle" logic in DO_TRACE is because it is slow for tracepoints that are frequently called into. Another reason to do it is because tracepoint callbacks are expected to know what they are doing and turn on RCU watching as appropriate (as consensus on the matter suggests).

Quoted 4 lines

Whoa there. We arch people need crystal clear rules as to what tracepoints can be called in what contexts and what is the responsibility of the callbacks.

The direction that Peter, Mathieu and Steve are going is that callbacks registered on "rcu idle" tracepoints need to turn on "RCU watching" themselves. Such as perf. Turning on "RCU watching" is non-free as I tested in 2017 and we removed it back then, but it was added right back when perf started splatting. Now it is being removed again, and the turning on of RCU's eyes happens in the perf callback itself since perf uses RCU.

If you are calling trace_.._rcuidle(), can you not ensure that RCU is watching by calling the appropriate RCU APIs in your callbacks? Or did I miss the point?

thanks,

- Joel

Quoted 11 lines · Paul E. McKenney

Quoted 3 lines

So tracing itself is fine, but then if you have probes or bpf programs attached to a tracepoint these use rcu_read_lock()/unlock() which is obviosly wrong in rcuidle context.

Definitely, any such code needs to use tricks similar to that of the tracing code. Or instead use something like SRCU, which is OK with readers from idle. Or use something like Steve Rostedt's workqueue-based approach, though please be very careful with this latter, lest the battery-powered embedded guys come after you for waking up idle CPUs too often. ;-)

Are we okay if we somehow ensure that all the entry code before enter_from_user_mode() only does rcuidle tracing variants and has kprobes off? Including for BPF use cases?

It would be *really* nice if we could statically verify this, as has been mentioned elsewhere in the thread. It would also probably be good enough if we could do it at runtime. Maybe with lockdep on, we verify rcu state in tracepoints even if the tracepoint isn't active? And we could plausibly have some widget that could inject something into *every* kprobeable function to check rcu state.

But the DEFINE_EVENT doesn't have the "_rcuidle" part. And that's where I got lost in the macro maze. I looked at the gcc asm output, and there is, indeed:

# ./include/trace/events/preemptirq.h:40:
DEFINE_EVENT(preemptirq_template, irq_enable,

with a bunch of asm magic that looks like it's probably a tracepoint. I still don't quite see where the "_rcuidle" went.

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Thomas Gleixner

Ok, but for the time being anything before/after CONTEXT_KERNEL is unsafe except trace_hardirq_off/on() as those trace functions do not allow to attach anything AFAICT.

Can you point to whatever makes those particular functions special? I failed to follow the macro maze.

Those are not tracepoints and not going through the macro maze. See kernel/trace/trace_preemptirq.c

Now that the C entry points are safe, move the irq flags tracing code into the entry helper.

Signed-off-by Thomas Gleixner <[email protected]>

Now that the C entry points are safe, move the irq flags tracing code into the entry helper.

Signed-off-by Thomas Gleixner <[email protected]>

---
  arch/x86/entry/common.c          |    5 +++++
  arch/x86/entry/entry_32.S        |   12 ------------
  arch/x86/entry/entry_64.S        |    2 --
  arch/x86/entry/entry_64_compat.S |   18 ------------------
  4 files changed, 5 insertions(+), 32 deletions(-)

Split prepare_enter_to_user_mode() and mark it notrace/noprobe so the irq flags tracing on return can be put into it.

Quoted 2 lines · Thomas Gleixner

Split prepare_enter_to_user_mode() and mark it notrace/noprobe so the irq flags tracing on return can be put into it.

Is our tooling clever enough for thsi to do anything? You have a static inline that is only called in one place, and the caller is notrace and NOKPROBE. Does this actually allow tracing in the static inline callee?

Split prepare_enter_to_user_mode() and mark it notrace/noprobe so the irq flags tracing on return can be put into it.

Signed-off-by Thomas Gleixner <[email protected]>

---
  arch/x86/entry/common.c |   15 +++++++++++----
  1 file changed, 11 insertions(+), 4 deletions(-)

arch/x86/entry/common.c +1 -1

@@ -196,7 +196,7 @@ static void exit_to_usermode_loop(struct
  }
 
  /* Called with IRQs disabled. */
-__visible inline void prepare_exit_to_usermode(struct pt_regs *regs)
+static inline void __prepare_exit_to_usermode(struct pt_regs *regs)
  {
  	struct thread_info *ti = current_thread_info();
  	u32 cached_flags;

arch/x86/entry/common.c +4 -3

  #define SYSCALL_EXIT_WORK_FLAGS				\
  	(_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT |	\
  	 _TIF_SINGLESTEP | _TIF_SYSCALL_TRACEPOINT)

@@ -271,7 +277,7 @@ static void syscall_slow_exit_work(struc
   * Called with IRQs on and fully valid regs.  Returns with IRQs off in a
   * state such that we can immediately switch to user mode.
   */
-__visible inline void syscall_return_slowpath(struct pt_regs *regs)
+__visible inline notrace void syscall_return_slowpath(struct pt_regs *regs)
  {
  	struct thread_info *ti = current_thread_info();
  	u32 cached_flags = READ_ONCE(ti->flags);

@@ -292,8 +298,9 @@ static void syscall_slow_exit_work(struc
  		syscall_slow_exit_work(regs, cached_flags);
 
  	local_irq_disable();
-	prepare_exit_to_usermode(regs);
+	__prepare_exit_to_usermode(regs);
  }
+NOKPROBE_SYMBOL(syscall_return_slowpath);
 
  #ifdef CONFIG_X86_64
  static __always_inline

@@ -417,7 +424,7 @@ static __always_inline long do_fast_sysc
  		/* User code screwed up. */
  		local_irq_disable();
  		regs->ax = -EFAULT;
-		prepare_exit_to_usermode(regs);
+		__prepare_exit_to_usermode(regs);
  		return 0;	/* Keep it simple: use IRET. */
  	}
 

Quoted 2 lines · Thomas Gleixner

Split prepare_enter_to_user_mode() and mark it notrace/noprobe so the irq flags tracing on return can be put into it.

This splits prepare_exit_to_usermode() not prepare_enter_to_user_mode().

Quoted 7 lines

/* Called with IRQs disabled. */

arch/x86/entry/common.c +1 -1

-__visible inline void prepare_exit_to_usermode(struct pt_regs *regs)
+static inline void __prepare_exit_to_usermode(struct pt_regs *regs)
  {
  	struct thread_info *ti = current_thread_info();
  	u32 cached_flags;

arch/x86/entry/common.c static void exit_to_usermode_loop(struct · line 248

Quoted 8 lines

arch/x86/entry/common.c +5 -0

@@ -241,6 +241,12 @@ static void exit_to_usermode_loop(struct
  	mds_user_clear_cpu_buffers();
  }
 
+__visible inline notrace void prepare_exit_to_usermode(struct pt_regs *regs)
+{
+	__prepare_exit_to_usermode(regs);
+}
+NOKPROBE_SYMBOL(prepare_exit_to_usermode);

Would it be worth grouping local_irq_disable() and prepare_exit_to_usermode() (similarly to what was done entry with syscall_entry_fixups()) and then put trace_hardirqs_on() there too. For example:

static __always_inline void syscall_exit_fixups(struct pt_regs *regs)

{
         local_irq_disable();
         prepare_exit_to_usermode(regs);
         trace_hardirqs_on();
}

Or is this planned once prepare_exit_from_usermode() is not used by idtentry anymore?

which removes the ASM IRQ tracepoints.

which removes the ASM IRQ tracepoints.

Signed-off-by Thomas Gleixner <[email protected]>

---
  arch/x86/entry/common.c   |    2 ++
  arch/x86/entry/entry_32.S |    3 ++-
  arch/x86/entry/entry_64.S |    3 ---
  3 files changed, 4 insertions(+), 4 deletions(-)

arch/x86/entry/common.c +2 -0

@@ -299,6 +299,8 @@ static void syscall_slow_exit_work(struc
 
  	local_irq_disable();
  	__prepare_exit_to_usermode(regs);
+	/* Return to user space enables interrupts */
+	trace_hardirqs_on();
  }
  NOKPROBE_SYMBOL(syscall_return_slowpath);
 

arch/x86/entry/entry_32.S +2 -1

@@ -812,7 +812,7 @@ SYM_CODE_START(ret_from_fork)
  	movl    %esp, %eax
  	call    syscall_return_slowpath
  	STACKLEAK_ERASE
-	jmp     restore_all
+	jmp     restore_all_switch_stack
 
  	/* kernel thread */
  1:	movl	%edi, %eax

@@ -1077,6 +1077,7 @@ SYM_FUNC_START(entry_INT80_32)
 
  restore_all:
  	TRACE_IRQS_IRET
+restore_all_switch_stack:
  	SWITCH_TO_ENTRY_STACK
  	CHECK_AND_APPLY_ESPFIX
  .Lrestore_nocheck:

arch/x86/entry/entry_64.S +0 -3

@@ -172,8 +172,6 @@ SYM_INNER_LABEL(entry_SYSCALL_64_after_h
  	movq	%rsp, %rsi
  	call	do_syscall_64		/* returns with IRQs disabled */
 
-	TRACE_IRQS_ON			/* return enables interrupts */
-
  	/*
  	 * Try to use SYSRET instead of IRET if we're returning to
  	 * a completely clean 64-bit userspace context.  If we're not,

@@ -340,7 +338,6 @@ SYM_CODE_START(ret_from_fork)
  	UNWIND_HINT_REGS
  	movq	%rsp, %rdi
  	call	syscall_return_slowpath	/* returns with IRQs disabled */
-	TRACE_IRQS_ON			/* user mode is traced as IRQS on */
  	jmp	swapgs_restore_regs_and_return_to_usermode
 
  1:

which again gets it out of the ASM code.

Quoted 1 line · Thomas Gleixner

which again gets it out of the ASM code.

Why is this better than just sticking the tracing in __prepare_exit_from_usermode() or just not splitting it in the first place?

Quoted 3 lines · Andy Lutomirski

Quoted 1 line · Thomas Gleixner

which again gets it out of the ASM code.

Quoted 3 lines

Why is this better than just sticking the tracing in __prepare_exit_from_usermode() or just not splitting it in the first place?

The split is there because prepare_exit_from_usermode() is used from the idtentry maze as well. Once that stuff is converted in the later series the split goes away again.

which cleans up the ASM maze.

Signed-off-by Thomas Gleixner <[email protected]>

---
  arch/x86/entry/common.c          |    8 +++++++-
  arch/x86/entry/entry_32.S        |    9 ++-------
  arch/x86/entry/entry_64_compat.S |   14 +++++---------
  3 files changed, 14 insertions(+), 17 deletions(-)

arch/x86/entry/common.c +0 -0

  NOKPROBE_SYMBOL(do_int80_syscall_32);
 

@@ -468,8 +470,12 @@ static __always_inline long do_fast_sysc
  /* Returns 0 to return using IRET or 1 to return using SYSEXIT/SYSRETL. */
  __visible notrace long do_fast_syscall_32(struct pt_regs *regs)

arch/x86/entry/entry_32.S +2 -7

@@ -811,8 +811,7 @@ SYM_CODE_START(ret_from_fork)
  	/* When we fork, we trace the syscall return in the child, too. */
  	movl    %esp, %eax
  	call    syscall_return_slowpath
-	STACKLEAK_ERASE
-	jmp     restore_all_switch_stack
+	jmp     .Lsyscall_32_done
 
  	/* kernel thread */
  1:	movl	%edi, %eax

@@ -968,8 +967,7 @@ SYM_FUNC_START(entry_SYSENTER_32)
 
  	STACKLEAK_ERASE
 
-/* Opportunistic SYSEXIT */
-	TRACE_IRQS_ON			/* User mode traces as IRQs on. */
+	/* Opportunistic SYSEXIT */
 
  	/*
  	 * Setup entry stack - we keep the pointer in %eax and do the

@@ -1072,11 +1070,8 @@ SYM_FUNC_START(entry_INT80_32)
  	movl	%esp, %eax
  	call	do_int80_syscall_32
  .Lsyscall_32_done:
-
  	STACKLEAK_ERASE
 
-restore_all:
-	TRACE_IRQS_IRET
  restore_all_switch_stack:
  	SWITCH_TO_ENTRY_STACK
  	CHECK_AND_APPLY_ESPFIX

arch/x86/entry/entry_64_compat.S +5 -9

@@ -132,8 +132,8 @@ SYM_FUNC_START(entry_SYSENTER_compat)
  	movq	%rsp, %rdi
  	call	do_fast_syscall_32
  	/* XEN PV guests always use IRET path */
-	ALTERNATIVE "testl %eax, %eax; jz .Lsyscall_32_done", \
-		    "jmp .Lsyscall_32_done", X86_FEATURE_XENPV
+	ALTERNATIVE "testl %eax, %eax; jz swapgs_restore_regs_and_return_to_usermode", \
+		    "jmp swapgs_restore_regs_and_return_to_usermode", X86_FEATURE_XENPV
  	jmp	sysret32_from_system_call
 
  .Lsysenter_fix_flags:

@@ -244,8 +244,8 @@ SYM_INNER_LABEL(entry_SYSCALL_compat_aft
  	movq	%rsp, %rdi
  	call	do_fast_syscall_32
  	/* XEN PV guests always use IRET path */
-	ALTERNATIVE "testl %eax, %eax; jz .Lsyscall_32_done", \
-		    "jmp .Lsyscall_32_done", X86_FEATURE_XENPV
+	ALTERNATIVE "testl %eax, %eax; jz swapgs_restore_regs_and_return_to_usermode", \
+		    "jmp swapgs_restore_regs_and_return_to_usermode", X86_FEATURE_XENPV
 
  	/* Opportunistic SYSRET */
  sysret32_from_system_call:

@@ -254,7 +254,7 @@ SYM_INNER_LABEL(entry_SYSCALL_compat_aft
  	 * stack. So let's erase the thread stack right now.
  	 */
  	STACKLEAK_ERASE
-	TRACE_IRQS_ON			/* User mode traces as IRQs on. */
+
  	movq	RBX(%rsp), %rbx		/* pt_regs->rbx */
  	movq	RBP(%rsp), %rbp		/* pt_regs->rbp */
  	movq	EFLAGS(%rsp), %r11	/* pt_regs->flags (in r11) */

@@ -393,9 +393,5 @@ SYM_CODE_START(entry_INT80_compat)
 
  	movq	%rsp, %rdi
  	call	do_int80_syscall_32
-.Lsyscall_32_done:
-
-	/* Go back to user mode. */
-	TRACE_IRQS_ON
  	jmp	swapgs_restore_regs_and_return_to_usermode
  SYM_CODE_END(entry_INT80_compat)

Quoted 12 lines · Thomas Gleixner

which cleans up the ASM maze.

Signed-off-by Thomas Gleixner <[email protected]>

---
  arch/x86/entry/common.c          |    8 +++++++-
  arch/x86/entry/entry_32.S        |    9 ++-------
  arch/x86/entry/entry_64_compat.S |   14 +++++---------
  3 files changed, 14 insertions(+), 17 deletions(-)

arch/x86/entry/common.c void do_syscall_64_irqs_on(unsigned long · line 337

Quoted 5 lines

arch/x86/entry/common.c +1 -0

@@ -333,6 +333,7 @@ void do_syscall_64_irqs_on(unsigned long
  {
  	syscall_entry_fixups();
  	do_syscall_64_irqs_on(nr, regs);
+	trace_hardirqs_on();
  }

trace_hardirqs_on() is already called through syscall_return_slowpath() (from the previous patch):

do_syscall_64()
-> do_syscall_64_irqs_on()

     -> syscall_return_slowpath()
       -> trace_hardirqs_on()