ring-buffer: Mark irq_work as HARD_IRQ to prevent deadlocks

ftrace may trigger rb_wakeups while holding pi_lock which will also be requested via trace_...->...->ring_buffer_unlock_commit->...-> irq_work_queue->raise_softirq->try_to_wake_up. This quickly causes deadlocks when trying to use ftrace under -rt.

Resolve this by marking the ring buffer's irq_work as HARD_IRQ.

Signed-off-by Jan Kiszka <[email protected]>

---

I'm not yet sure if this doesn't push work into hard-irq context that is better not done there on -rt.

I'm also not sure if there aren't more such cases, given that -rt turns the default irq_work wakeup policy around. But maybe we are lucky.

 kernel/trace/ring_buffer.c | 1 +
 1 file changed, 1 insertion(+)

kernel/trace/ring_buffer.c +1 -0

index f4fbbfc..6a1939e 100644
@@ -1252,6 +1252,7 @@ rb_allocate_cpu_buffer(struct ring_buffer *buffer, int nr_pages, int cpu)
 	init_irq_work(&cpu_buffer->irq_work.work, rb_wake_up_waiters);
 	init_waitqueue_head(&cpu_buffer->irq_work.waiters);
 	init_waitqueue_head(&cpu_buffer->irq_work.full_waiters);
+	cpu_buffer->irq_work.work.flags = IRQ_WORK_HARD_IRQ;
 
 	bpage = kzalloc_node(ALIGN(sizeof(*bpage), cache_line_size()),
 			    GFP_KERNEL, cpu_to_node(cpu));

ftrace may trigger rb_wakeups while holding pi_lock which will also be requested via trace_...->...->ring_buffer_unlock_commit->...-> irq_work_queue->raise_softirq->try_to_wake_up. This quickly causes deadlocks when trying to use ftrace under -rt.

Resolve this by marking the ring buffer's irq_work as HARD_IRQ.

Signed-off-by Jan Kiszka <[email protected]>

---

I'm not yet sure if this doesn't push work into hard-irq context that is better not done there on -rt.

I'm also not sure if there aren't more such cases, given that -rt turns the default irq_work wakeup policy around. But maybe we are lucky.

 kernel/trace/ring_buffer.c | 1 +
 1 file changed, 1 insertion(+)

kernel/trace/ring_buffer.c +0 -0

index f4fbbfc..6a1939e 100644

Quoted 5 lines · Jan Kiszka

ftrace may trigger rb_wakeups while holding pi_lock which will also be requested via trace_...->...->ring_buffer_unlock_commit->...-> irq_work_queue->raise_softirq->try_to_wake_up. This quickly causes deadlocks when trying to use ftrace under -rt.

Quoted 7 lines

Resolve this by marking the ring buffer's irq_work as HARD_IRQ.

Signed-off-by Jan Kiszka <[email protected]>

---

I'm not yet sure if this doesn't push work into hard-irq context that is better not done there on -rt.

everything should be done in the soft-irq.

Quoted 3 lines

I'm also not sure if there aren't more such cases, given that -rt turns the default irq_work wakeup policy around. But maybe we are lucky.

The only thing that is getting done in the hardirq is the FULL_NO_HZ thingy. I would be _very_ glad if we could keep it that way.

Quoted 10 lines

 kernel/trace/ring_buffer.c | 1 +
 1 file changed, 1 insertion(+)

kernel/trace/ring_buffer.c +0 -0

index f4fbbfc..6a1939e 100644

kernel/trace/ring_buffer.c rb_allocate_cpu_buffer(struct ring_buffer *buffer, int nr_pages, int cpu) · line 1258

Quoted 7 lines

kernel/trace/ring_buffer.c +1 -0

@@ -1252,6 +1252,7 @@ rb_allocate_cpu_buffer(struct ring_buffer *buffer, int nr_pages, int cpu)
 	init_irq_work(&cpu_buffer->irq_work.work, rb_wake_up_waiters);
 	init_waitqueue_head(&cpu_buffer->irq_work.waiters);
 	init_waitqueue_head(&cpu_buffer->irq_work.full_waiters);
+	cpu_buffer->irq_work.work.flags = IRQ_WORK_HARD_IRQ;
 
 	bpage = kzalloc_node(ALIGN(sizeof(*bpage), cache_line_size()),
 			    GFP_KERNEL, cpu_to_node(cpu));

Sebastian

Quoted 20 lines · Sebastian Andrzej Siewior

Quoted 5 lines · Jan Kiszka

ftrace may trigger rb_wakeups while holding pi_lock which will also be requested via trace_...->...->ring_buffer_unlock_commit->...-> irq_work_queue->raise_softirq->try_to_wake_up. This quickly causes deadlocks when trying to use ftrace under -rt.

Quoted 7 lines

Resolve this by marking the ring buffer's irq_work as HARD_IRQ.

Signed-off-by Jan Kiszka <[email protected]>

---

I'm not yet sure if this doesn't push work into hard-irq context that is better not done there on -rt.

everything should be done in the soft-irq.

Quoted 3 lines

I'm also not sure if there aren't more such cases, given that -rt turns the default irq_work wakeup policy around. But maybe we are lucky.

Quoted 2 lines

The only thing that is getting done in the hardirq is the FULL_NO_HZ thingy. I would be _very_ glad if we could keep it that way.

Then - to my current understanding - we need an NMI-safe trigger for soft-irq work. Is there anything like this existing already? Or can we still use the IPI-based kick without actually doing the work in hard-irq context?

Jan

Quoted 24 lines · Jan Kiszka

Quoted 22 lines · Sebastian Andrzej Siewior

Quoted 5 lines · Jan Kiszka

ftrace may trigger rb_wakeups while holding pi_lock which will also be requested via trace_...->...->ring_buffer_unlock_commit->...-> irq_work_queue->raise_softirq->try_to_wake_up. This quickly causes deadlocks when trying to use ftrace under -rt.

Quoted 7 lines

Resolve this by marking the ring buffer's irq_work as HARD_IRQ.

Signed-off-by Jan Kiszka <[email protected]>

---

I'm not yet sure if this doesn't push work into hard-irq context that is better not done there on -rt.

everything should be done in the soft-irq.

Quoted 3 lines

I'm also not sure if there aren't more such cases, given that -rt turns the default irq_work wakeup policy around. But maybe we are lucky.

The only thing that is getting done in the hardirq is the FULL_NO_HZ thingy. I would be _very_ glad if we could keep it that way.

Quoted 4 lines

Then - to my current understanding - we need an NMI-safe trigger for soft-irq work. Is there anything like this existing already? Or can we still use the IPI-based kick without actually doing the work in hard-irq context?

But if you trigger it via IPI it will still run in hardirq context, right? Can you describe how run into this and try to think about it in a quiet moment. It it just enabling the function tracer and running it?

Quoted 14 lines · Sebastian Andrzej Siewior

Quoted 12 lines · Jan Kiszka

ftrace may trigger rb_wakeups while holding pi_lock which will also be requested via trace_...->...->ring_buffer_unlock_commit->...-> irq_work_queue->raise_softirq->try_to_wake_up. This quickly causes deadlocks when trying to use ftrace under -rt.

Resolve this by marking the ring buffer's irq_work as HARD_IRQ.

Signed-off-by Jan Kiszka <[email protected]>

---

I'm not yet sure if this doesn't push work into hard-irq context that is better not done there on -rt.

Quoted 8 lines

everything should be done in the soft-irq.

Quoted 3 lines

I'm also not sure if there aren't more such cases, given that -rt turns the default irq_work wakeup policy around. But maybe we are lucky.

The only thing that is getting done in the hardirq is the FULL_NO_HZ thingy. I would be _very_ glad if we could keep it that way.

Then - to my current understanding - we need an NMI-safe trigger for soft-irq work. Is there anything like this existing already? Or can we still use the IPI-based kick without actually doing the work in hard-irq context?

Quoted 15 lines · On Thu, 16 Apr 2015 16:28:58 +0200 Jan Kiszka <[email protected]> wrote:

Quoted 14 lines · Sebastian Andrzej Siewior

Quoted 12 lines · Jan Kiszka

ftrace may trigger rb_wakeups while holding pi_lock which will also be requested via trace_...->...->ring_buffer_unlock_commit->...-> irq_work_queue->raise_softirq->try_to_wake_up. This quickly causes deadlocks when trying to use ftrace under -rt.

Resolve this by marking the ring buffer's irq_work as HARD_IRQ.

Signed-off-by Jan Kiszka <[email protected]>

---

I'm not yet sure if this doesn't push work into hard-irq context that is better not done there on -rt.

Quoted 8 lines

Quoted 8 lines

everything should be done in the soft-irq.

Quoted 3 lines

I'm also not sure if there aren't more such cases, given that -rt turns the default irq_work wakeup policy around. But maybe we are lucky.

The only thing that is getting done in the hardirq is the FULL_NO_HZ thingy. I would be _very_ glad if we could keep it that way.

tracing is special, even more so than NO_HZ_FULL, as it also traces that as well (and even RCU). Tracing the kernel is like a debugger. Ideally, it would not be part of the kernel, but just an external observer. Without special hardware that is not the case, so we try to be outside the main system as much as possible.

Quoted 6 lines

Then - to my current understanding - we need an NMI-safe trigger for soft-irq work. Is there anything like this existing already? Or can we still use the IPI-based kick without actually doing the work in hard-irq context?

The reason why it uses irq_work() is because a simple wakeup can deadlock the system if called by the tracing infrastructure (as we see raise_softirq() does too).

But yeah, there's no real need to have the ring buffer irq work handler run from hardirq context. The only requirement is that you can not do the raise from the irq_work_queue call. If you want to have the hardirq work handle do the raise softirq, that's fine. Perhaps that's the solution? Have all irq_work_queue() always trigger the hard irq, but the hard irq may just raise a softirq or it will call the handler directly if IRQ_WORK_HARD_IRQ is set.

My patch is definitely not OK. It causes

[  380.372579] BUG: scheduling while atomic: trace-cmd/2149/0x00010004
...
[  380.372604] Call Trace:
[  380.372610]  <IRQ>  [<ffffffff81607694>] dump_stack+0x50/0x9f
[  380.372613]  [<ffffffff8160413c>] __schedule_bug+0x59/0x69
[  380.372615]  [<ffffffff8160a1d5>] __schedule+0x675/0x800
[  380.372617]  [<ffffffff8160a394>] schedule+0x34/0xa0
[  380.372619]  [<ffffffff8160bf7d>] rt_spin_lock_slowlock+0xcd/0x290
[  380.372621]  [<ffffffff8160d8b5>] rt_spin_lock+0x25/0x30
[  380.372623]  [<ffffffff8108fe39>] __wake_up+0x29/0x60

Instead of turning all irq_work requests into lazy ones on -rt, just move their execution from hard into soft-irq context.

This resolves deadlocks of ftrace which will queue work from arbitrary
contexts, including those that have locks held that are needed for raising a soft-irq.

Signed-off-by Jan Kiszka <[email protected]>

---

Second try, looks much better so far. And it also removes my concerns regarding other potential cases besides ftrace.

 kernel/irq_work.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

kernel/irq_work.c +1 -4

index 9dda38a..3f6ffcd 100644
@@ -85,12 +85,9 @@ bool irq_work_queue_on(struct irq_work *work, int 
 cpu)
              raise_irqwork = llist_add(&work->llnode,
                                        &per_cpu(hirq_work_list, cpu));
      else
-             raise_irqwork = llist_add(&work->llnode,
-                                       &per_cpu(lazy_list, cpu));
-#else
+#endif
              raise_irqwork = llist_add(&work->llnode,
                                        &per_cpu(raised_list, cpu));
-#endif
 
      if (raise_irqwork)

              arch_send_call_function_single_ipi(cpu);

kernel/irq_work.c +9 -12

@@ -114,21 +111,20 @@ bool irq_work_queue(struct irq_work *work)
      if (work->flags & IRQ_WORK_HARD_IRQ) {
              if (llist_add(&work->llnode, this_cpu_ptr(&hirq_work_list)))
                      arch_irq_work_raise();
-     } else {
+     } else
+#endif
+     if (work->flags & IRQ_WORK_LAZY) {
              if (llist_add(&work->llnode, this_cpu_ptr(&lazy_list)) &&
                  tick_nohz_tick_stopped())
+#ifdef CONFIG_PREEMPT_RT_FULL
                      raise_softirq(TIMER_SOFTIRQ);
-     }
 #else
-     if (work->flags & IRQ_WORK_LAZY) {
-             if (llist_add(&work->llnode, this_cpu_ptr(&lazy_list)) &&
-                 tick_nohz_tick_stopped())
                      arch_irq_work_raise();
+#endif
      } else {
              if (llist_add(&work->llnode, this_cpu_ptr(&raised_list)))
                      arch_irq_work_raise();
      }
-#endif
 
      preempt_enable();
 

@@ -202,6 +198,8 @@ void irq_work_run(void)
 {
 #ifdef CONFIG_PREEMPT_RT_FULL
      irq_work_run_list(this_cpu_ptr(&hirq_work_list));
+     if (!llist_empty(this_cpu_ptr(&raised_list)))
+             raise_softirq(TIMER_SOFTIRQ);
 #else
      irq_work_run_list(this_cpu_ptr(&raised_list));
      irq_work_run_list(this_cpu_ptr(&lazy_list));

@@ -211,15 +209,11 @@ EXPORT_SYMBOL_GPL(irq_work_run);
 
 void irq_work_tick(void)
 {
-#ifdef CONFIG_PREEMPT_RT_FULL
-     irq_work_run_list(this_cpu_ptr(&lazy_list));
-#else
-     struct llist_head *raised = &__get_cpu_var(raised_list);
+     struct llist_head *raised = this_cpu_ptr(&raised_list);
 
      if (!llist_empty(raised) && !arch_irq_work_has_interrupt())
              irq_work_run_list(raised);
-     irq_work_run_list(&__get_cpu_var(lazy_list));
-#endif
+     irq_work_run_list(this_cpu_ptr(&lazy_list));
 }
 
 /*

Quoted 8 lines · Jan Kiszka

Instead of turning all irq_work requests into lazy ones on -rt, just
move their execution from hard into soft-irq context.

This resolves deadlocks of ftrace which will queue work from arbitrary
contexts, including those that have locks held that are needed for raising a soft-irq.

Yup, trace-cmd record -> dead-box fully repeatable, and now fixed.

Quoted 11 lines · Mike Galbraith

Quoted 8 lines · Jan Kiszka

Instead of turning all irq_work requests into lazy ones on -rt, just
move their execution from hard into soft-irq context.

This resolves deadlocks of ftrace which will queue work from arbitrary
contexts, including those that have locks held that are needed for raising a soft-irq.

Yup, trace-cmd record -> dead-box fully repeatable, and now fixed.

Except you kinda forgot to run the raised list. The reformatted (which saved two whole lines;) patch below adds that to irq_work_tick(), which fixes the livelock both powertop and perf top otherwise meet.

Subject: [PATCH RT 3.18] irq_work: Provide a soft-irq based queue
Date:   Thu, 16 Apr 2015 18:28:16 +0200
From:   Jan Kiszka <[email protected]>

Instead of turning all irq_work requests into lazy ones on -rt, just move their execution from hard into soft-irq context.

This resolves deadlocks of ftrace which will queue work from arbitrary contexts, including those that have locks held that are needed for raising a soft-irq.

Signed-off-by Jan Kiszka <[email protected]>

---

Second try, looks much better so far. And it also removes my concerns regarding other potential cases besides ftrace.

 kernel/irq_work.c |   84 ++++++++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 43 deletions(-)

kernel/irq_work.c +37 -36

@@ -80,17 +80,12 @@ bool irq_work_queue_on(struct irq_work *
        if (!irq_work_claim(work))
                return false;
 
-#ifdef CONFIG_PREEMPT_RT_FULL
-       if (work->flags & IRQ_WORK_HARD_IRQ)
+       if (IS_ENABLED(CONFIG_PREEMPT_RT_FULL) && (work->flags & IRQ_WORK_HARD_IRQ))
                raise_irqwork = llist_add(&work->llnode,
                                          &per_cpu(hirq_work_list, cpu));
        else
                raise_irqwork = llist_add(&work->llnode,
-                                         &per_cpu(lazy_list, cpu));
-#else
-               raise_irqwork = llist_add(&work->llnode,
                                          &per_cpu(raised_list, cpu));
-#endif
 
        if (raise_irqwork)
                arch_send_call_function_single_ipi(cpu);

@@ -103,6 +98,9 @@ EXPORT_SYMBOL_GPL(irq_work_queue_on);
 /* Enqueue the irq work @work on the current CPU */
 bool irq_work_queue(struct irq_work *work)
 {
+       bool realtime = IS_ENABLED(CONFIG_PREEMPT_RT_FULL);
+       bool raise = false;
+
        /* Only queue if not already pending */
        if (!irq_work_claim(work))
                return false;

@@ -110,25 +108,22 @@ bool irq_work_queue(struct irq_work *wor
        /* Queue the entry and raise the IPI if needed. */
        preempt_disable();
 
-#ifdef CONFIG_PREEMPT_RT_FULL
-       if (work->flags & IRQ_WORK_HARD_IRQ) {
+       if (realtime && (work->flags & IRQ_WORK_HARD_IRQ)) {
                if (llist_add(&work->llnode, this_cpu_ptr(&hirq_work_list)))
-                       arch_irq_work_raise();
-       } else {
-               if (llist_add(&work->llnode, this_cpu_ptr(&lazy_list)) &&
-                   tick_nohz_tick_stopped())
-                       raise_softirq(TIMER_SOFTIRQ);
-       }
-#else
-       if (work->flags & IRQ_WORK_LAZY) {
+                       raise = 1;
+       } else if (work->flags & IRQ_WORK_LAZY) {
                if (llist_add(&work->llnode, this_cpu_ptr(&lazy_list)) &&
-                   tick_nohz_tick_stopped())
-                       arch_irq_work_raise();
-       } else {
-               if (llist_add(&work->llnode, this_cpu_ptr(&raised_list)))
-                       arch_irq_work_raise();
-       }
-#endif
+                   tick_nohz_tick_stopped()) {
+                       if (realtime)
+                               raise_softirq(TIMER_SOFTIRQ);
+                       else
+                               raise = true;
+               }
+       } else if (llist_add(&work->llnode, this_cpu_ptr(&raised_list)))
+               raise = true;
+
+       if (raise)
+               arch_irq_work_raise();
 
        preempt_enable();
 

@@ -143,12 +138,13 @@ bool irq_work_needs_cpu(void)
        raised = this_cpu_ptr(&raised_list);
        lazy = this_cpu_ptr(&lazy_list);
 
-       if (llist_empty(raised))
-               if (llist_empty(lazy))
-#ifdef CONFIG_PREEMPT_RT_FULL
+       if (llist_empty(raised) && llist_empty(lazy)) {
+               if (IS_ENABLED(CONFIG_PREEMPT_RT_FULL)) {
                        if (llist_empty(this_cpu_ptr(&hirq_work_list)))
-#endif
                                return false;
+               } else
+                       return false;
+       }
 
        /* All work should have been flushed before going offline */
        WARN_ON_ONCE(cpu_is_offline(smp_processor_id()));

@@ -162,9 +158,7 @@ static void irq_work_run_list(struct lli
        struct irq_work *work;
        struct llist_node *llnode;
 
-#ifndef CONFIG_PREEMPT_RT_FULL
-       BUG_ON(!irqs_disabled());
-#endif
+       BUG_ON(!IS_ENABLED(CONFIG_PREEMPT_RT_FULL) && !irqs_disabled());
 
        if (llist_empty(list))
                return;

@@ -200,26 +194,30 @@ static void irq_work_run_list(struct lli
  */
 void irq_work_run(void)
 {
-#ifdef CONFIG_PREEMPT_RT_FULL
-       irq_work_run_list(this_cpu_ptr(&hirq_work_list));
-#else
-       irq_work_run_list(this_cpu_ptr(&raised_list));
-       irq_work_run_list(this_cpu_ptr(&lazy_list));
-#endif
+       if (IS_ENABLED(CONFIG_PREEMPT_RT_FULL)) {
+               irq_work_run_list(this_cpu_ptr(&hirq_work_list));
+               /*
+                * NOTE: we raise softirq via IPI for safety,
+                * and execute in irq_work_tick() to move the
+                * overhead from hard to soft irq context.
+                */
+               if (!llist_empty(this_cpu_ptr(&raised_list)))
+                       raise_softirq(TIMER_SOFTIRQ);
+       } else {
+               irq_work_run_list(this_cpu_ptr(&raised_list));
+               irq_work_run_list(this_cpu_ptr(&lazy_list));
+       }
 }
 EXPORT_SYMBOL_GPL(irq_work_run);
 
 void irq_work_tick(void)

Quoted 9 lines

void irq_work_tick(void)
{

(quoted diff) +1 -4

-#ifdef CONFIG_PREEMPT_RT_FULL
-       irq_work_run_list(this_cpu_ptr(&lazy_list));
-#else
-       struct llist_head *raised = &__get_cpu_var(raised_list);
+       struct llist_head *raised = this_cpu_ptr(&raised_list);

Quoted 4 lines

(quoted diff) +1 -1

-       if (!llist_empty(raised) && !arch_irq_work_has_interrupt())
+       if (!llist_empty(raised) && 

(!arch_irq_work_has_interrupt() ||
+ IS_ENABLED(CONFIG_PREEMPT_RT_FULL)))

OK, that additional condition is addressing archs that don't have irq_work support and fall back to the timer, right?

Quoted 9 lines · Jan Kiszka

Quoted 9 lines

void irq_work_tick(void)
{

kernel/irq_work.c +1 -4

-#ifdef CONFIG_PREEMPT_RT_FULL
-       irq_work_run_list(this_cpu_ptr(&lazy_list));
-#else
-       struct llist_head *raised = &__get_cpu_var(raised_list);
+       struct llist_head *raised = this_cpu_ptr(&raised_list);

Quoted 7 lines

Quoted 4 lines

(quoted diff) +1 -1

-       if (!llist_empty(raised) && !arch_irq_work_has_interrupt())
+       if (!llist_empty(raised) && 

(!arch_irq_work_has_interrupt() ||
+ IS_ENABLED(CONFIG_PREEMPT_RT_FULL)))

OK, that additional condition is addressing archs that don't have irq_work support and fall back to the timer, right?

How will ever run if it is not run in either irq_work_run() or irq_work_tick()? There are two choices, we better pick one.

kernel/irq_work.c +3 -0

@@ -103,6 +98,9 @@ EXPORT_SYMBOL_GPL(irq_work_queue_on);
 /* Enqueue the irq work @work on the current CPU */
 bool irq_work_queue(struct irq_work *work)
 {
+       bool realtime = IS_ENABLED(CONFIG_PREEMPT_RT_FULL);
+       bool raise = false;
+
        /* Only queue if not already pending */
        if (!irq_work_claim(work))
                return false;

Quoted 11 lines · Mike Galbraith

kernel/irq_work.c +3 -0

@@ -103,6 +98,9 @@ EXPORT_SYMBOL_GPL(irq_work_queue_on);
 /* Enqueue the irq work @work on the current CPU */
 bool irq_work_queue(struct irq_work *work)
 {
+       bool realtime = IS_ENABLED(CONFIG_PREEMPT_RT_FULL);
+       bool raise = false;
+
        /* Only queue if not already pending */
        if (!irq_work_claim(work))
                return false;

kernel/irq_work.c bool irq_work_queue(struct irq_work *wor · line 113

Quoted 8 lines

kernel/irq_work.c +1 -2

@@ -110,25 +108,22 @@ bool irq_work_queue(struct irq_work *wor
        /* Queue the entry and raise the IPI if needed. */
        preempt_disable();
 
-#ifdef CONFIG_PREEMPT_RT_FULL
-       if (work->flags & IRQ_WORK_HARD_IRQ) {
+       if (realtime && (work->flags & IRQ_WORK_HARD_IRQ)) {
                if (llist_add(&work->llnode, 
 this_cpu_ptr(&hirq_work_list)))

This boils down to

#ifdef CONFIG_X
some_type x;
#endif
...

if (IS_ENABLED(CONFIG_X) && ...)

      use(x);

And here we even have an indirection for IS_ENABLED via that local bool
variable. Is that pattern OK for Linux? Does it compile in all supported
optimization levels of all supported compilers?

Quoted 31 lines · Jan Kiszka

Quoted 11 lines · Mike Galbraith

kernel/irq_work.c +3 -0

@@ -103,6 +98,9 @@ EXPORT_SYMBOL_GPL(irq_work_queue_on);
 /* Enqueue the irq work @work on the current CPU */
 bool irq_work_queue(struct irq_work *work)
 {
+       bool realtime = IS_ENABLED(CONFIG_PREEMPT_RT_FULL);
+       bool raise = false;
+
        /* Only queue if not already pending */
        if (!irq_work_claim(work))
                return false;

kernel/irq_work.c bool irq_work_queue(struct irq_work *wor · line 113

Quoted 8 lines

kernel/irq_work.c +1 -2

@@ -110,25 +108,22 @@ bool irq_work_queue(struct irq_work *wor
        /* Queue the entry and raise the IPI if needed. */
        preempt_disable();
 
-#ifdef CONFIG_PREEMPT_RT_FULL
-       if (work->flags & IRQ_WORK_HARD_IRQ) {
+       if (realtime && (work->flags & IRQ_WORK_HARD_IRQ)) {
                if (llist_add(&work->llnode, 
 this_cpu_ptr(&hirq_work_list)))

This boils down to

#ifdef CONFIG_X
some_type x;
#endif
...

if (IS_ENABLED(CONFIG_X) && ...)

      use(x);

Quoted 5 lines

And here we even have an indirection for IS_ENABLED via that local bool
variable. Is that pattern OK for Linux? Does it compile in all supported
optimization levels of all supported compilers?

I hope it all goes away, that being what IS_ENABLED() is there for.

Quoted 32 lines · Mike Galbraith

Quoted 31 lines · Jan Kiszka

Quoted 11 lines · Mike Galbraith

kernel/irq_work.c +3 -0

@@ -103,6 +98,9 @@ EXPORT_SYMBOL_GPL(irq_work_queue_on);
 /* Enqueue the irq work @work on the current CPU */
 bool irq_work_queue(struct irq_work *work)
 {
+       bool realtime = IS_ENABLED(CONFIG_PREEMPT_RT_FULL);
+       bool raise = false;
+
        /* Only queue if not already pending */
        if (!irq_work_claim(work))
                return false;

kernel/irq_work.c bool irq_work_queue(struct irq_work *wor · line 113

Quoted 8 lines

kernel/irq_work.c +1 -2

@@ -110,25 +108,22 @@ bool irq_work_queue(struct irq_work *wor
        /* Queue the entry and raise the IPI if needed. */
        preempt_disable();
 
-#ifdef CONFIG_PREEMPT_RT_FULL
-       if (work->flags & IRQ_WORK_HARD_IRQ) {
+       if (realtime && (work->flags & IRQ_WORK_HARD_IRQ)) {
                if (llist_add(&work->llnode, 
 this_cpu_ptr(&hirq_work_list)))

This boils down to

#ifdef CONFIG_X
some_type x;
#endif
...

if (IS_ENABLED(CONFIG_X) && ...)

      use(x);

Quoted 8 lines

Quoted 6 lines

And here we even have an indirection for IS_ENABLED via that local
bool
variable. Is that pattern OK for Linux? Does it compile in all supported
optimization levels of all supported compilers?

I hope it all goes away, that being what IS_ENABLED() is there for.

Hope is good - but not enough here: it breaks the build under !CONFIG_X, even the case without the bool var.

CC kernel/irq_work.o
In file included from ../include/asm-generic/percpu.h:6:0,

                 from ../arch/x86/include/asm/percpu.h:522,
                 from ../arch/x86/include/asm/current.h:5,
                 from ../arch/x86/include/asm/processor.h:15,
                 from ../arch/x86/include/asm/irq_work.h:4,
                 from ../include/linux/irq_work.h:47,
                 from ../kernel/irq_work.c:11:
../kernel/irq_work.c: In function ‘irq_work_queue_on’:
../kernel/irq_work.c:85:17: error: ‘hirq_work_list’ undeclared 
(first use in this function)
        &per_cpu(hirq_work_list, cpu));

Quoted 12 lines

CC kernel/irq_work.o
In file included from ../include/asm-generic/percpu.h:6:0,

                 from ../arch/x86/include/asm/percpu.h:522,
                 from ../arch/x86/include/asm/current.h:5,
                 from ../arch/x86/include/asm/processor.h:15,
                 from ../arch/x86/include/asm/irq_work.h:4,
                 from ../include/linux/irq_work.h:47,
                 from ../kernel/irq_work.c:11:
../kernel/irq_work.c: In function ‘irq_work_queue_on’:
../kernel/irq_work.c:85:17: error: ‘hirq_work_list’ undeclared 
(first use in this function)
        &per_cpu(hirq_work_list, cpu));

Aw poo, so that's just what I _thought_ it was for.

Quoted 13 lines · On Thu, 23 Apr 2015 09:19:26 +0200 Mike Galbraith <[email protected]>…

Quoted 12 lines

CC kernel/irq_work.o
In file included from ../include/asm-generic/percpu.h:6:0,

                 from ../arch/x86/include/asm/percpu.h:522,
                 from ../arch/x86/include/asm/current.h:5,
                 from ../arch/x86/include/asm/processor.h:15,
                 from ../arch/x86/include/asm/irq_work.h:4,
                 from ../include/linux/irq_work.h:47,
                 from ../kernel/irq_work.c:11:
../kernel/irq_work.c: In function ‘irq_work_queue_on’:
../kernel/irq_work.c:85:17: error: ‘hirq_work_list’ undeclared 
(first use in this function)
        &per_cpu(hirq_work_list, cpu));

Quoted 1 line

Aw poo, so that's just what I _thought_ it was for.

It helps optimization but does nothing for undefined symbols.

That said, why don't we clean up that irq_work code and at least declare both lists, and get rid of all the #ifdefs. I wonder if gcc is smart enough to not allocate a static variable if it happens to be optimized out?

Quoted 16 lines · Steven Rostedt

Quoted 13 lines · On Thu, 23 Apr 2015 09:19:26 +0200 Mike Galbraith <[email protected]>…

Quoted 12 lines

CC kernel/irq_work.o
In file included from ../include/asm-generic/percpu.h:6:0,

                 from ../arch/x86/include/asm/percpu.h:522,
                 from ../arch/x86/include/asm/current.h:5,
                 from ../arch/x86/include/asm/processor.h:15,
                 from ../arch/x86/include/asm/irq_work.h:4,
                 from ../include/linux/irq_work.h:47,
                 from ../kernel/irq_work.c:11:
../kernel/irq_work.c: In function ‘irq_work_queue_on’:
../kernel/irq_work.c:85:17: error: ‘hirq_work_list’ undeclared 
(first use in this function)
        &per_cpu(hirq_work_list, cpu));

Quoted 8 lines

Quoted 1 line

Aw poo, so that's just what I _thought_ it was for.

It helps optimization but does nothing for undefined symbols.

That said, why don't we clean up that irq_work code and at least declare both lists, and get rid of all the #ifdefs. I wonder if gcc is smart enough to not allocate a static variable if it happens to be optimized out?

Nope, it didn't notice a thing.

This is a stab at that cleanup. Usable as is with Jan's ok, or as fodder for your bitmaster-9000 patch shredder, or whatever. Box works and it makes line count shrink...

I downgraded evolution v3.16->v3.12 to restore its ability to read it's own fscking "Preformatted" switch, so whitespace should be fine.

Oh, btw, if anyone (else) makes a 4.1-rt, your rt push work will want one of those nifty hirq tags lest box make boom due to trying to do that not only way late, but with irqs enabled which pisses sched all off.

Subject: [PATCH RT 3.18] irq_work: Provide a soft-irq based queue
Date: Thu, 16 Apr 2015 18:28:16 +0200
From: Jan Kiszka <[email protected]>

Instead of turning all irq_work requests into lazy ones on -rt, just move their execution from hard into soft-irq context.

This resolves deadlocks of ftrace which will queue work from arbitrary contexts, including those that have locks held that are needed for raising a soft-irq.

Mike: cleanup ifdef mess and kill hirq_work_list. We need two lists, and already have them, merely need to select according to work type. In -rt all work not tagged for hirq execution is queued to the lazy list and runs via irq_work_tick(). Raising SOFTIRQ_TIMER is always done via IPI for deadlock safety, if the work item is not a lazy work or the tick is stopped, fire IPI immediately, otherwise let it wait. IOW, lazy work is lazy in -rt only until someone queues immediate work.

The approach looks good to me, but the commit log deserves a rework now.

The approach looks good to me, but the commit log deserves a rework now.

Quoted 1 line · Jan Kiszka

The approach looks good to me, but the commit log deserves a rework now.

Ok, we agree on the approach, and that the changelog wants a bit of attention, so either you're gonna rewrite it to suit you, do a pretty changelog, and I ack, or I take the blame for the posted form, scribble something that I hope is a better log, and you ack. Either will work.

Here's my changelog+blame-taking, if you're ok with it, ack, and we can call it a day, otherwise onward to plan B.

irq_work: Delegate non-immediate irq work to ksoftirqd

Based on a patch from Jan Kiszka.

Jan reported that ftrace queueing work from arbitrary contexts can and does lead to deadlock. trace-cmd -e sched:* deadlocked in fact.

Resolve the problem by delegating all non-immediate work to ksoftirqd.

We need two lists to do this, one for hard irq, one for soft, so we can use the two existing lists, eliminating the -rt specific list and all of the ifdefery while we're at it.

Strategy: Queue work tagged for hirq invocation to the raised_list, invoke via IPI as usual. If a work item being queued to lazy_list, which becomes our all others list, is not a lazy work item, or the tick is stopped, fire an IPI to raise SOFTIRQ_TIMER immediately, otherwise let ksofirqd find it when the tick comes along. Raising SOFTIRQ_TIMER via IPI even when queueing local ensures delegation.

Signed-off-by Mike Galbraith <[email protected]>

---
 kernel/irq_work.c |   85 ++++++++++++++++++++----------------------------------
 1 file changed, 33 insertions(+), 52 deletions(-)

kernel/irq_work.c +31 -50

@@ -23,9 +23,7 @@
 
 static DEFINE_PER_CPU(struct llist_head, raised_list);
 static DEFINE_PER_CPU(struct llist_head, lazy_list);
-#ifdef CONFIG_PREEMPT_RT_FULL
-static DEFINE_PER_CPU(struct llist_head, hirq_work_list);
-#endif
+
 /*
  * Claim the entry so that no one else will poke at it.
  */

@@ -68,7 +66,7 @@ void __weak arch_irq_work_raise(void)
  */
 bool irq_work_queue_on(struct irq_work *work, int cpu)
 {
-	bool raise_irqwork;
+	struct llist_head *list;
 
 	/* All work should have been flushed before going offline */
 	WARN_ON_ONCE(cpu_is_offline(cpu));

@@ -80,19 +78,12 @@ bool irq_work_queue_on(struct irq_work *
 	if (!irq_work_claim(work))
 		return false;
 
-#ifdef CONFIG_PREEMPT_RT_FULL
-	if (work->flags & IRQ_WORK_HARD_IRQ)
-		raise_irqwork = llist_add(&work->llnode,
-					  &per_cpu(hirq_work_list, cpu));
+	if (IS_ENABLED(CONFIG_PREEMPT_RT_FULL) && !(work->flags & IRQ_WORK_HARD_IRQ))
+		list = &per_cpu(lazy_list, cpu);
 	else
-		raise_irqwork = llist_add(&work->llnode,
-					  &per_cpu(lazy_list, cpu));
-#else
-		raise_irqwork = llist_add(&work->llnode,
-					  &per_cpu(raised_list, cpu));
-#endif
+		list = &per_cpu(raised_list, cpu);
 
-	if (raise_irqwork)
+	if (llist_add(&work->llnode, list))
 		arch_send_call_function_single_ipi(cpu);
 
 	return true;

@@ -103,6 +94,9 @@ EXPORT_SYMBOL_GPL(irq_work_queue_on);
 /* Enqueue the irq work @work on the current CPU */
 bool irq_work_queue(struct irq_work *work)
 {
+	struct llist_head *list;
+	bool lazy_work, realtime = IS_ENABLED(CONFIG_PREEMPT_RT_FULL);
+
 	/* Only queue if not already pending */
 	if (!irq_work_claim(work))
 		return false;

@@ -110,25 +104,17 @@ bool irq_work_queue(struct irq_work *wor
 	/* Queue the entry and raise the IPI if needed. */
 	preempt_disable();
 
-#ifdef CONFIG_PREEMPT_RT_FULL
-	if (work->flags & IRQ_WORK_HARD_IRQ) {
-		if (llist_add(&work->llnode, this_cpu_ptr(&hirq_work_list)))
-			arch_irq_work_raise();
-	} else {
-		if (llist_add(&work->llnode, this_cpu_ptr(&lazy_list)) &&
-		    tick_nohz_tick_stopped())
-			raise_softirq(TIMER_SOFTIRQ);
-	}
-#else
-	if (work->flags & IRQ_WORK_LAZY) {
-		if (llist_add(&work->llnode, this_cpu_ptr(&lazy_list)) &&
-		    tick_nohz_tick_stopped())
-			arch_irq_work_raise();
-	} else {
-		if (llist_add(&work->llnode, this_cpu_ptr(&raised_list)))
+	lazy_work = work->flags & IRQ_WORK_LAZY;
+
+	if (lazy_work || (realtime && !(work->flags & IRQ_WORK_HARD_IRQ)))
+		list = this_cpu_ptr(&lazy_list);
+	else
+		list = this_cpu_ptr(&raised_list);
+
+	if (llist_add(&work->llnode, list)) {
+		if (!lazy_work || tick_nohz_tick_stopped())
 			arch_irq_work_raise();
 	}
-#endif
 
 	preempt_enable();
 

@@ -143,12 +129,8 @@ bool irq_work_needs_cpu(void)
 	raised = this_cpu_ptr(&raised_list);
 	lazy = this_cpu_ptr(&lazy_list);
 
-	if (llist_empty(raised))
-		if (llist_empty(lazy))
-#ifdef CONFIG_PREEMPT_RT_FULL
-			if (llist_empty(this_cpu_ptr(&hirq_work_list)))
-#endif
-				return false;
+	if (llist_empty(raised) && llist_empty(lazy))
+		return false;
 
 	/* All work should have been flushed before going offline */
 	WARN_ON_ONCE(cpu_is_offline(smp_processor_id()));

@@ -162,9 +144,7 @@ static void irq_work_run_list(struct lli
 	struct irq_work *work;
 	struct llist_node *llnode;
 
-#ifndef CONFIG_PREEMPT_RT_FULL
-	BUG_ON(!irqs_disabled());
-#endif
+	BUG_ON(!IS_ENABLED(CONFIG_PREEMPT_RT_FULL) && !irqs_disabled());
 
 	if (llist_empty(list))
 		return;

@@ -200,26 +180,27 @@ static void irq_work_run_list(struct lli
  */
 void irq_work_run(void)
 {
-#ifdef CONFIG_PREEMPT_RT_FULL
-	irq_work_run_list(this_cpu_ptr(&hirq_work_list));
-#else
 	irq_work_run_list(this_cpu_ptr(&raised_list));
-	irq_work_run_list(this_cpu_ptr(&lazy_list));
-#endif
+	if (IS_ENABLED(CONFIG_PREEMPT_RT_FULL)) {
+		/*
+		 * NOTE: we raise softirq via IPI for safety,
+		 * and execute in irq_work_tick() to move the
+		 * overhead from hard to soft irq context.
+		 */
+		if (!llist_empty(this_cpu_ptr(&lazy_list)))
+			raise_softirq(TIMER_SOFTIRQ);
+	} else
+		irq_work_run_list(this_cpu_ptr(&lazy_list));
 }
 EXPORT_SYMBOL_GPL(irq_work_run);
 
 void irq_work_tick(void)
 {
-#ifdef CONFIG_PREEMPT_RT_FULL
-	irq_work_run_list(this_cpu_ptr(&lazy_list));
-#else
-	struct llist_head *raised = &__get_cpu_var(raised_list);

Acked-by Jan Kiszka <[email protected]>

This way around makes more sense as you changed the patch significantly.

Thanks,
Jan