Skip to contentSkip to conversation

genirq: Forced threaded interrupt handlers

PATCH In review Thomas Gleixner 3 participants 14 messages Feb 23, 2011 – Feb 26, 2011

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover genirq: Forced threaded interrupt handlers

Some time ago when the threaded interrupt handlers infrastructure was about to be merged, Andrew asked me where that command line switch was which magically runs all interrupt handlers and the softirqs in threads.

While we were doing that brute force in preempt-rt for quite a while it took some time to come up with a reasonable non intrusive implementation for mainline. We also had to find a solution which fits Linus' recently issued "palatable Trojan horse" requirement (see:
https://lwn.net/Articles/370998/).

The gift of this patch series is the ability to add "threadirqs" to the kernel command line and magically (almost) all interrupt handlers
- except those which are explicitely marked IRQF_NO_THREAD - are confined into threads along with all soft interrupts.

That allows to enhance the debugability of the kernel as a bug in an interrupt handler is not necessarily taking the whole machine down. It's just the particular irq thread which goes into nirwana. Bad luck if that's the one which is crucial to retrieve the bug report, but in most cases - yes, I analysed quite a lot of bugzilla reports - it will be helpful for reporters not to be forced to transcribe the bug from the screen.

An architecture has to enable that feature in Kconfig via a selectable option which says: Yes, we marked all interrupts which never can be threaded - like IPIs etc. - as IRQF_NO_THREAD. All interrupts marked IRQF_TIMER or IRQF_PER_CPU are automatically excluded from threading.

A side effect of this is, that the long standing request of supporting oneshot threaded handlers on shared interrupt lines (given that all drivers agree) is now possible. I know, that this is nuts, but unfortunately common sense and basic understanding of the problem seems to be not required when HW folks are set out to save a gate.

Thanks,

tglx

---
 Documentation/kernel-parameters.txt |    4 +
 include/linux/interrupt.h           |   13 +++
 include/linux/irqdesc.h             |    2 
 kernel/irq/Kconfig                  |    3 
 kernel/irq/handle.c                 |   34 +++++----
 kernel/irq/internals.h              |    2 
 kernel/irq/manage.c                 |  128 +++++++++++++++++++++++++++++++-----
 kernel/sched.c                      |    5 +
 kernel/softirq.c                    |   16 +++-
 9 files changed, 175 insertions(+), 32 deletions(-)
#1 lore raw

PATCH 1/5 genirq: Prepare the handling of shared oneshot interrupts

For level type interrupts we need to track how many threads are on flight to avoid useless interrupt storms when not all thread handlers have finished yet. Keep track of the woken threads and only unmask when there are no more threads in flight.

Yes, I'm lazy and using a bitfield. But not only because I'm lazy, the main reason is that it's way simpler than using a refcount. A refcount based solution would need to keep track of various things like crashing the irq thread, spurious interrupts coming in, disables/enables, free_irq() and some more. The bitfield keeps the tracking simple and makes things just work. It's also nicely confined to the thread code pathes and does not require additional checks all over the place.

---
 include/linux/interrupt.h |    2 +
 include/linux/irqdesc.h   |    2 +
 kernel/irq/handle.c       |   34 ++++++++++++++++++-----------
 kernel/irq/manage.c       |   53 ++++++++++++++++++++++++++++++++++++++++------
 4 files changed, 72 insertions(+), 19 deletions(-)
include/linux/interrupt.h +2 -0
Index: linux-2.6-tip/include/linux/interrupt.h
===================================================================
@@ -99,6 +99,7 @@ typedef irqreturn_t (*irq_handler_t)(int
  * @thread_fn:	interupt handler function for threaded interrupts
  * @thread:	thread pointer for threaded interrupts
  * @thread_flags:	flags related to @thread
+ * @thread_mask:	bitmask for keeping track of @thread activity
  */
 struct irqaction {
 	irq_handler_t handler;
@@ -109,6 +110,7 @@ struct irqaction {
 	irq_handler_t thread_fn;
 	struct task_struct *thread;
 	unsigned long thread_flags;
+	unsigned long thread_mask;
 	const char *name;
 	struct proc_dir_entry *dir;
 } ____cacheline_internodealigned_in_smp;
include/linux/irqdesc.h +2 -0
Index: linux-2.6-tip/include/linux/irqdesc.h
===================================================================
@@ -28,6 +28,7 @@ struct timer_rand_state;
  * @lock:		locking for SMP
  * @affinity_notify:	context for notification of affinity changes
  * @pending_mask:	pending rebalanced interrupts
+ * @threads_oneshot:	bitfield to handle shared oneshot threads
  * @threads_active:	number of irqaction threads currently running
  * @wait_for_threads:	wait queue for sync_irq to wait for threaded handlers
  * @dir:		/proc/irq/ procfs entry
@@ -86,6 +87,7 @@ struct irq_desc {
 	cpumask_var_t		pending_mask;
 #endif
 #endif
+	unsigned long		threads_oneshot;
 	atomic_t		threads_active;
 	wait_queue_head_t       wait_for_threads;
 #ifdef CONFIG_PROC_FS
kernel/irq/handle.c +21 -13
Index: linux-2.6-tip/kernel/irq/handle.c
===================================================================
@@ -51,6 +51,26 @@ static void warn_no_thread(unsigned int 
 	       "but no thread function available.", irq, action->name);
 }
 
+static void irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
+{
+	/*
+	 * Wake up the handler thread for this action. In case the
+	 * thread crashed and was killed we just pretend that we
+	 * handled the interrupt. The hardirq handler has disabled the
+	 * device interrupt, so no irq storm is lurking.
+	 */
+	if (!test_bit(IRQTF_DIED, &action->thread_flags) &&
+	    !test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags)) {
+		/*
+		 * We or the mask lockless. Safe because the code
+		 * which clears the mask is serialized
+		 * vs. IRQ_INPROGRESS.
+		 */
+		desc->threads_oneshot |= action->thread_mask;
+		wake_up_process(action->thread);
+	}
+}
+
 irqreturn_t
 handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
 {
@@ -84,19 +104,7 @@ handle_irq_event_percpu(struct irq_desc 
 				break;
 			}
 
-			/*
-			 * Wake up the handler thread for this
-			 * action. In case the thread crashed and was
-			 * killed we just pretend that we handled the
-			 * interrupt. The hardirq handler above has
-			 * disabled the device interrupt, so no irq
-			 * storm is lurking.
-			 */
-			if (likely(!test_bit(IRQTF_DIED,
-					     &action->thread_flags))) {
-				set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
-				wake_up_process(action->thread);
-			}
+			irq_wake_thread(desc, action);
 
 			/* Fall through to add to randomness */
 		case IRQ_HANDLED:
kernel/irq/manage.c +47 -6

124 lines not on this page · raw message

#2 lore raw

Re: [patch 1/5] genirq: Prepare the handling of shared oneshot interrupts

Quoted 8 lines · Thomas Gleixner

+               /*
+                * We or the mask lockless. Safe because the code
+                * which clears the mask is serialized
+                * vs. IRQ_INPROGRESS.
+                */
+               desc->threads_oneshot |= action->thread_mask;
+               wake_up_process(action->thread);

That comment makes no sense.

What has "code which clears the mask" to do with anything?

Even if another CPU _sets_ a bit, doing so in parallel will lose bits if it's not locked. One CPU will read the old value, set its bit, and store the new value: with the race, only one of the bits will be set.

So maybe the code is safe, but the comment is still totally wrong. You had better be safe not just against people clearing bits, you need to be safe against ANY OTHER WRITER (clear or set), including very much OTHER BITS in the same word.

So if it really is safe, comment on ALL the cases.

Linus

#3 lore raw

Re: [patch 1/5] genirq: Prepare the handling of shared oneshot interrupts

Quoted 18 lines · Linus Torvalds
Quoted 8 lines · Thomas Gleixner

+               /*
+                * We or the mask lockless. Safe because the code
+                * which clears the mask is serialized
+                * vs. IRQ_INPROGRESS.
+                */
+               desc->threads_oneshot |= action->thread_mask;
+               wake_up_process(action->thread);

That comment makes no sense.

What has "code which clears the mask" to do with anything?

Even if another CPU _sets_ a bit, doing so in parallel will lose bits if it's not locked. One CPU will read the old value, set its bit, and store the new value: with the race, only one of the bits will be set.

Quoted 6 lines

So maybe the code is safe, but the comment is still totally wrong. You had better be safe not just against people clearing bits, you need to be safe against ANY OTHER WRITER (clear or set), including very much OTHER BITS in the same word.

So if it really is safe, comment on ALL the cases.

Fair enough, that comment really sucks if you are not familiar with the gory details of that code.

What about the following replacement, which might be overly detailed, but that's exactly how it works.

Thanks,

tglx

kernel/irq/handle.c +53 -11
Index: linux-2.6-tip/kernel/irq/handle.c
===================================================================
@@ -57,18 +57,60 @@ static void irq_wake_thread(struct irq_d
 	 * Wake up the handler thread for this action. In case the
 	 * thread crashed and was killed we just pretend that we
 	 * handled the interrupt. The hardirq handler has disabled the
-	 * device interrupt, so no irq storm is lurking.
+	 * device interrupt, so no irq storm is lurking. If the
+	 * RUNTHREAD bit is already set, nothing to do.
 	 */
-	if (!test_bit(IRQTF_DIED, &action->thread_flags) &&
-	    !test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags)) {
-		/*
-		 * We or the mask lockless. Safe because the code
-		 * which clears the mask is serialized
-		 * vs. IRQ_INPROGRESS.
-		 */
-		desc->threads_oneshot |= action->thread_mask;
-		wake_up_process(action->thread);
-	}
+	if (test_bit(IRQTF_DIED, &action->thread_flags) ||
+	    test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags))
+		return;
+
+	/*
+	 * It's safe to OR the mask lockless here. We have only two
+	 * places which write to threads_oneshot: This code and the
+	 * irq thread.
+	 *
+	 * This code is the hard irq context and can never run on two
+	 * cpus in parallel. If it ever does we have more serious
+	 * problems than this bitmask.
+	 *
+	 * The irq threads of this irq which clear their "running" bit
+	 * in threads_oneshot are serialized via desc->lock against
+	 * each other and they are serialized against this code by
+	 * IRQS_INPROGRESS.
+	 *
+	 * Hard irq handler:
+	 *
+	 *	spin_lock(desc->lock);
+	 *	desc->state |= IRQS_INPROGRESS;
+	 *	spin_unlock(desc->lock);
+	 *	set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
+	 *	desc->threads_oneshot |= mask;
+	 *	spin_unlock(desc->lock);
+	 *	desc->state &= ~IRQS_INPROGRESS;
+	 *	spin_unlock(desc->lock);
+	 *
+	 * irq thread:
+	 *
+	 * again:
+	 *	spin_lock(desc->lock);
+	 *	if (desc->state & IRQS_INPROGRESS) {
+	 *		spin_unlock(desc->lock);
+	 *		while(desc->state & IRQS_INPROGRESS)
+	 *			cpu_relax();
+	 *		goto again;
+	 *	}
+	 *	if (!test_bit(IRQTF_RUNTHREAD, &action->thread_flags))
+	 *		desc->threads_oneshot &= ~mask;
+	 *	spin_unlock(desc->lock);
+	 *
+	 * So either the thread waits for us to clear IRQS_INPROGRESS
+	 * or we are waiting in the flow handler for desc->lock to be
+	 * released before we reach this point. The thread also checks
+	 * IRQTF_RUNTHREAD under desc->lock. If set it leaves
+	 * threads_oneshot untouched and runs the thread another time.
+	 */
+	desc->threads_oneshot |= action->thread_mask;
+	wake_up_process(action->thread);
 }
 
 irqreturn_t
kernel/irq/manage.c +2 -1
Index: linux-2.6-tip/kernel/irq/manage.c
===================================================================
@@ -637,7 +637,8 @@ again:
 	 *
 	 * This also serializes the state of shared oneshot handlers
 	 * versus "desc->threads_onehsot |= action->thread_mask;" in
-	 * handle_irq_event().
+	 * irq_wake_thread(). See the comment there which explains the
+	 * serialization.
 	 */
 	if (unlikely(desc->istate & IRQS_INPROGRESS)) {
 		raw_spin_unlock_irq(&desc->lock);
#4 lore raw

[tip:irq/core] genirq: Prepare the handling of shared oneshot interrupts

In reply to Thomas Gleixner
Commit-ID:  b5faba21a6805c33b40e258d36f57997ee1de131
Gitweb:     http://git.kernel.org/tip/b5faba21a6805c33b40e258d36f57997ee1de131
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Wed, 23 Feb 2011 23:52:13 +0000
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Fri, 25 Feb 2011 20:24:21 +0100

genirq: Prepare the handling of shared oneshot interrupts

For level type interrupts we need to track how many threads are on flight to avoid useless interrupt storms when not all thread handlers have finished yet. Keep track of the woken threads and only unmask when there are no more threads in flight.

Yes, I'm lazy and using a bitfield. But not only because I'm lazy, the main reason is that it's way simpler than using a refcount. A refcount based solution would need to keep track of various things like crashing the irq thread, spurious interrupts coming in, disables/enables, free_irq() and some more. The bitfield keeps the tracking simple and makes things just work. It's also nicely confined to the thread code pathes and does not require additional checks all over the place.

LKML-Reference: <[email protected]>

---
 include/linux/interrupt.h |    2 +
 include/linux/irqdesc.h   |    2 +
 kernel/irq/handle.c       |   76 +++++++++++++++++++++++++++++++++++++--------
 kernel/irq/manage.c       |   54 ++++++++++++++++++++++++++++---
 4 files changed, 115 insertions(+), 19 deletions(-)
include/linux/interrupt.h +2 -0
index 8da6643..e116fef 100644
@@ -99,6 +99,7 @@ typedef irqreturn_t (*irq_handler_t)(int, void *);
  * @thread_fn:	interupt handler function for threaded interrupts
  * @thread:	thread pointer for threaded interrupts
  * @thread_flags:	flags related to @thread
+ * @thread_mask:	bitmask for keeping track of @thread activity
  */
 struct irqaction {
 	irq_handler_t handler;
@@ -109,6 +110,7 @@ struct irqaction {
 	irq_handler_t thread_fn;
 	struct task_struct *thread;
 	unsigned long thread_flags;
+	unsigned long thread_mask;
 	const char *name;
 	struct proc_dir_entry *dir;
 } ____cacheline_internodealigned_in_smp;
include/linux/irqdesc.h +2 -0
index 2f87d64..9eb9cd3 100644
@@ -28,6 +28,7 @@ struct timer_rand_state;
  * @lock:		locking for SMP
  * @affinity_notify:	context for notification of affinity changes
  * @pending_mask:	pending rebalanced interrupts
+ * @threads_oneshot:	bitfield to handle shared oneshot threads
  * @threads_active:	number of irqaction threads currently running
  * @wait_for_threads:	wait queue for sync_irq to wait for threaded handlers
  * @dir:		/proc/irq/ procfs entry
@@ -86,6 +87,7 @@ struct irq_desc {
 	cpumask_var_t		pending_mask;
 #endif
 #endif
+	unsigned long		threads_oneshot;
 	atomic_t		threads_active;
 	wait_queue_head_t       wait_for_threads;
 #ifdef CONFIG_PROC_FS
kernel/irq/handle.c +63 -13

88 lines not on this page · raw message

kernel/irq/manage.c +48 -6

125 lines not on this page · raw message

#5 lore raw

PATCH 2/5 genirq: Allow shared oneshot interrupts

In reply to Thomas Gleixner

Support ONESHOT on shared interrupts, if all drivers agree on it.

---
 kernel/irq/manage.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
kernel/irq/manage.c +4 -6
Index: linux-2.6-tip/kernel/irq/manage.c
===================================================================
@@ -823,10 +823,6 @@ __setup_irq(unsigned int irq, struct irq
 		rand_initialize_irq(irq);
 	}
 
-	/* Oneshot interrupts are not allowed with shared */
-	if ((new->flags & IRQF_ONESHOT) && (new->flags & IRQF_SHARED))
-		return -EINVAL;
-
 	/*
 	 * Check whether the interrupt nests into another interrupt
 	 * thread.
@@ -880,10 +876,12 @@ __setup_irq(unsigned int irq, struct irq
 		 * Can't share interrupts unless both agree to and are
 		 * the same type (level, edge, polarity). So both flag
 		 * fields must have IRQF_SHARED set and the bits which
-		 * set the trigger type must match.
+		 * set the trigger type must match. Also all must
+		 * agree on ONESHOT.
 		 */
 		if (!((old->flags & new->flags) & IRQF_SHARED) ||
-		    ((old->flags ^ new->flags) & IRQF_TRIGGER_MASK)) {
+		    ((old->flags ^ new->flags) & IRQF_TRIGGER_MASK) ||
+		    ((old->flags ^ new->flags) & IRQF_ONESHOT)) {
 			old_name = old->name;
 			goto mismatch;
 		}
#6 lore raw

[tip:irq/core] genirq: Allow shared oneshot interrupts

Commit-ID:  9d591edd02a245305b1b9379e4c5571bad4d2774
Gitweb:     http://git.kernel.org/tip/9d591edd02a245305b1b9379e4c5571bad4d2774
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Wed, 23 Feb 2011 23:52:16 +0000
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Fri, 25 Feb 2011 20:24:21 +0100

genirq: Allow shared oneshot interrupts

Support ONESHOT on shared interrupts, if all drivers agree on it.

LKML-Reference: <[email protected]>

---
 kernel/irq/manage.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
kernel/irq/manage.c +4 -6
index 2301de1..58c8613 100644
@@ -824,10 +824,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		rand_initialize_irq(irq);
 	}
 
-	/* Oneshot interrupts are not allowed with shared */
-	if ((new->flags & IRQF_ONESHOT) && (new->flags & IRQF_SHARED))
-		return -EINVAL;
-
 	/*
 	 * Check whether the interrupt nests into another interrupt
 	 * thread.
@@ -881,10 +877,12 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		 * Can't share interrupts unless both agree to and are
 		 * the same type (level, edge, polarity). So both flag
 		 * fields must have IRQF_SHARED set and the bits which
-		 * set the trigger type must match.
+		 * set the trigger type must match. Also all must
+		 * agree on ONESHOT.
 		 */
 		if (!((old->flags & new->flags) & IRQF_SHARED) ||
-		    ((old->flags ^ new->flags) & IRQF_TRIGGER_MASK)) {
+		    ((old->flags ^ new->flags) & IRQF_TRIGGER_MASK) ||
+		    ((old->flags ^ new->flags) & IRQF_ONESHOT)) {
 			old_name = old->name;
 			goto mismatch;
 		}
#7 lore raw

PATCH 3/5 genirq: Add IRQF_NO_THREAD

In reply to Thomas Gleixner

Some low level interrupts cannot be threaded even when we force thread all interrupt handlers. Add a flag to annotate such interrupts. Add all timer interrupts to this category by default.

---
 include/linux/interrupt.h |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
include/linux/interrupt.h +3 -1
Index: linux-2.6-tip/include/linux/interrupt.h
===================================================================
@@ -58,6 +58,7 @@
  *                irq line disabled until the threaded handler has been run.
  * IRQF_NO_SUSPEND - Do not disable this IRQ during suspend
  * IRQF_FORCE_RESUME - Force enable it on resume even if IRQF_NO_SUSPEND is set
+ * IRQF_NO_THREAD - Interrupt cannot be threaded
  */
 #define IRQF_DISABLED		0x00000020
 #define IRQF_SAMPLE_RANDOM	0x00000040
@@ -70,8 +71,9 @@
 #define IRQF_ONESHOT		0x00002000
 #define IRQF_NO_SUSPEND		0x00004000
 #define IRQF_FORCE_RESUME	0x00008000
+#define IRQF_NO_THREAD		0x00010000
 
-#define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND)
+#define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
 
 /*
  * These values can be returned by request_any_context_irq() and
#8 lore raw

[tip:irq/core] genirq: Add IRQF_NO_THREAD

Commit-ID:  0c4602ff88d6d6ef0ee6d228ee9acaa6448ff6f5
Gitweb:     http://git.kernel.org/tip/0c4602ff88d6d6ef0ee6d228ee9acaa6448ff6f5
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Wed, 23 Feb 2011 23:52:18 +0000
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Fri, 25 Feb 2011 20:24:22 +0100

genirq: Add IRQF_NO_THREAD

Some low level interrupts cannot be threaded even when we force thread all interrupt handlers. Add a flag to annotate such interrupts. Add all timer interrupts to this category by default.

LKML-Reference: <[email protected]>

---
 include/linux/interrupt.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
include/linux/interrupt.h +3 -1
index e116fef..0fc3eb9 100644
@@ -58,6 +58,7 @@
  *                irq line disabled until the threaded handler has been run.
  * IRQF_NO_SUSPEND - Do not disable this IRQ during suspend
  * IRQF_FORCE_RESUME - Force enable it on resume even if IRQF_NO_SUSPEND is set
+ * IRQF_NO_THREAD - Interrupt cannot be threaded
  */
 #define IRQF_DISABLED		0x00000020
 #define IRQF_SAMPLE_RANDOM	0x00000040
@@ -70,8 +71,9 @@
 #define IRQF_ONESHOT		0x00002000
 #define IRQF_NO_SUSPEND		0x00004000
 #define IRQF_FORCE_RESUME	0x00008000
+#define IRQF_NO_THREAD		0x00010000
 
-#define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND)
+#define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
 
 /*
  * These values can be returned by request_any_context_irq() and
#9 lore raw

PATCH 4/5 sched: Switch wait_task_inactive to schedule_hrtimeout()

In reply to Thomas Gleixner

When we force thread hard and soft interrupts the startup of ksoftirqd would hang in kthread_bind() when wait_task_inactive() calls schedule_timeout_uninterruptible() because there is no softirq yet which will wake us up.

---
 kernel/sched.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
kernel/sched.c +4 -1
Index: linux-2.6-tip/kernel/sched.c
===================================================================
@@ -2224,7 +2224,10 @@ unsigned long wait_task_inactive(struct 
 		 * yield - it could be a while.
 		 */
 		if (unlikely(on_rq)) {
-			schedule_timeout_uninterruptible(1);
+			ktime_t to = ktime_set(0, NSEC_PER_SEC/HZ);
+
+			set_current_state(TASK_UNINTERRUPTIBLE);
+			schedule_hrtimeout(&to, HRTIMER_MODE_REL);
 			continue;
 		}
 
#10 lore raw

[tip:irq/core] sched: Switch wait_task_inactive to schedule_hrtimeout()

Commit-ID:  8eb90c30e0e815a1308828352eabd03ca04229dd
Gitweb:     http://git.kernel.org/tip/8eb90c30e0e815a1308828352eabd03ca04229dd
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Wed, 23 Feb 2011 23:52:21 +0000
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Fri, 25 Feb 2011 20:24:22 +0100

sched: Switch wait_task_inactive to schedule_hrtimeout()

When we force thread hard and soft interrupts the startup of ksoftirqd would hang in kthread_bind() when wait_task_inactive() calls schedule_timeout_uninterruptible() because there is no softirq yet which will wake us up.

LKML-Reference: <[email protected]>

---
 kernel/sched.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
kernel/sched.c +4 -1
index 18d38e4..66ca5d9 100644
@@ -2224,7 +2224,10 @@ unsigned long wait_task_inactive(struct task_struct *p, long match_state)
 		 * yield - it could be a while.
 		 */
 		if (unlikely(on_rq)) {
-			schedule_timeout_uninterruptible(1);
+			ktime_t to = ktime_set(0, NSEC_PER_SEC/HZ);
+
+			set_current_state(TASK_UNINTERRUPTIBLE);
+			schedule_hrtimeout(&to, HRTIMER_MODE_REL);
 			continue;
 		}
 
#11 lore raw

PATCH 5/5 genirq: Provide forced interrupt threading

In reply to Thomas Gleixner

Add a commandline parameter "threadirqs" which forces all interrupts except those marked IRQF_NO_THREAD to run threaded. That's mostly a debug option to allow retrieving better debug data from crashing interrupt handlers. If "threadirqs" is not enabled on the kernel command line, then there is no impact in the interrupt hotpath.

Architecture code needs to select CONFIG_IRQ_FORCED_THREADING after marking the interrupts which cant be threaded IRQF_NO_THREAD. All interrupts which have IRQF_TIMER set are implict marked IRQF_NO_THREAD. Also all PER_CPU interrupts are excluded.

---
 Documentation/kernel-parameters.txt |    4 ++
 include/linux/interrupt.h           |    7 +++
 kernel/irq/Kconfig                  |    3 +
 kernel/irq/internals.h              |    2 +
 kernel/irq/manage.c                 |   67 +++++++++++++++++++++++++++++++++---
 kernel/softirq.c                    |   16 +++++++-
 6 files changed, 93 insertions(+), 6 deletions(-)
Documentation/kernel-parameters.txt +4 -0
Index: linux-2.6-tip/Documentation/kernel-parameters.txt
===================================================================
@@ -2436,6 +2436,10 @@ and is between 256 and 4096 characters. 
 			<deci-seconds>: poll all this frequency
 			0: no polling (default)
 
+	threadirqs	[KNL]
+			Force threading of all interrupt handlers except those
+			marked explicitely IRQF_NO_THREAD.
+
 	topology=	[S390]
 			Format: {off | on}
 			Specify if the kernel should make use of the cpu
include/linux/interrupt.h +7 -0
Index: linux-2.6-tip/include/linux/interrupt.h
===================================================================
@@ -383,6 +383,13 @@ static inline int disable_irq_wake(unsig
 }
 #endif /* CONFIG_GENERIC_HARDIRQS */
 
+
+#ifdef CONFIG_IRQ_FORCED_THREADING
+extern bool force_irqthreads;
+#else
+#define force_irqthreads	(0)
+#endif
+
 #ifndef __ARCH_SET_SOFTIRQ_PENDING
 #define set_softirq_pending(x) (local_softirq_pending() = (x))
 #define or_softirq_pending(x)  (local_softirq_pending() |= (x))
kernel/irq/Kconfig +3 -0
Index: linux-2.6-tip/kernel/irq/Kconfig
===================================================================
@@ -38,6 +38,9 @@ config HARDIRQS_SW_RESEND
 config IRQ_PREFLOW_FASTEOI
        bool
 
+config IRQ_FORCED_THREADING
+       bool
+
 config SPARSE_IRQ
 	bool "Support sparse irq numbering"
 	depends on HAVE_SPARSE_IRQ
kernel/irq/internals.h +2 -0
Index: linux-2.6-tip/kernel/irq/internals.h
===================================================================
@@ -27,12 +27,14 @@ extern int noirqdebug;
  * IRQTF_DIED      - handler thread died
  * IRQTF_WARNED    - warning "IRQ_WAKE_THREAD w/o thread_fn" has been printed
  * IRQTF_AFFINITY  - irq thread is requested to adjust affinity
+ * IRQTF_FORCED_THREAD  - irq action is force threaded
  */
 enum {
 	IRQTF_RUNTHREAD,
 	IRQTF_DIED,
 	IRQTF_WARNED,
 	IRQTF_AFFINITY,
+	IRQTF_FORCED_THREAD,
 };
 
 /*
kernel/irq/manage.c +63 -4

105 lines not on this page · raw message

kernel/softirq.c +14 -2
Index: linux-2.6-tip/kernel/softirq.c
===================================================================
@@ -311,9 +311,21 @@ void irq_enter(void)
 }
 
 #ifdef __ARCH_IRQ_EXIT_IRQS_DISABLED
-# define invoke_softirq()	__do_softirq()
+static inline void invoke_softirq(void)
+{
+	if (!force_irqthreads)
+		__do_softirq();
+	else
+		wakeup_softirqd();
+}
 #else
-# define invoke_softirq()	do_softirq()
+static inline void invoke_softirq(void)
+{
+	if (!force_irqthreads)
+		do_softirq();
+	else
+		wakeup_softirqd();
+}
 #endif
 
 /*
#12 lore raw

[tip:irq/core] genirq: Provide forced interrupt threading

Commit-ID:  8d32a307e4faa8b123dc8a9cd56d1a7525f69ad3
Gitweb:     http://git.kernel.org/tip/8d32a307e4faa8b123dc8a9cd56d1a7525f69ad3
Author:     Thomas Gleixner <[email protected]>
AuthorDate: Wed, 23 Feb 2011 23:52:23 +0000
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Sat, 26 Feb 2011 11:57:18 +0100

genirq: Provide forced interrupt threading

Add a commandline parameter "threadirqs" which forces all interrupts except those marked IRQF_NO_THREAD to run threaded. That's mostly a debug option to allow retrieving better debug data from crashing interrupt handlers. If "threadirqs" is not enabled on the kernel command line, then there is no impact in the interrupt hotpath.

Architecture code needs to select CONFIG_IRQ_FORCED_THREADING after marking the interrupts which cant be threaded IRQF_NO_THREAD. All interrupts which have IRQF_TIMER set are implict marked IRQF_NO_THREAD. Also all PER_CPU interrupts are excluded.

Forced threading hard interrupts also forces all soft interrupt handling into thread context.

When enabled it might slow down things a bit, but for debugging problems in interrupt code it's a reasonable penalty as it does not immediately crash and burn the machine when an interrupt handler is buggy.

Some test results on a Core2Duo machine:

Cache cold run of:

 # time git grep irq_desc
      non-threaded       threaded
 real 1m18.741s          1m19.061s
 user 0m1.874s           0m1.757s
 sys  0m5.843s           0m5.427s
 # iperf -c server
non-threaded
[  3]  0.0-10.0 sec  1.09 GBytes   933 Mbits/sec
[  3]  0.0-10.0 sec  1.09 GBytes   934 Mbits/sec
[  3]  0.0-10.0 sec  1.09 GBytes   933 Mbits/sec
threaded
[  3]  0.0-10.0 sec  1.09 GBytes   939 Mbits/sec
[  3]  0.0-10.0 sec  1.09 GBytes   934 Mbits/sec
[  3]  0.0-10.0 sec  1.09 GBytes   937 Mbits/sec

LKML-Reference: <[email protected]>

---
 Documentation/kernel-parameters.txt |    4 ++
 include/linux/interrupt.h           |    7 ++++
 kernel/irq/Kconfig                  |    3 ++
 kernel/irq/internals.h              |    2 +
 kernel/irq/manage.c                 |   67 ++++++++++++++++++++++++++++++++--
 kernel/softirq.c                    |   16 +++++++-
 6 files changed, 93 insertions(+), 6 deletions(-)
Documentation/kernel-parameters.txt +4 -0
index 89835a4..cac6cf9 100644
@@ -2436,6 +2436,10 @@ and is between 256 and 4096 characters. It is defined in the file
 			<deci-seconds>: poll all this frequency
 			0: no polling (default)
 
+	threadirqs	[KNL]
+			Force threading of all interrupt handlers except those
+			marked explicitely IRQF_NO_THREAD.
+
 	topology=	[S390]
 			Format: {off | on}
 			Specify if the kernel should make use of the cpu
include/linux/interrupt.h +7 -0
index 0fc3eb9..f8a8af1 100644
@@ -383,6 +383,13 @@ static inline int disable_irq_wake(unsigned int irq)
 }
 #endif /* CONFIG_GENERIC_HARDIRQS */
 
+
+#ifdef CONFIG_IRQ_FORCED_THREADING
+extern bool force_irqthreads;
+#else
+#define force_irqthreads	(0)
+#endif
+
 #ifndef __ARCH_SET_SOFTIRQ_PENDING
 #define set_softirq_pending(x) (local_softirq_pending() = (x))
 #define or_softirq_pending(x)  (local_softirq_pending() |= (x))
kernel/irq/Kconfig +3 -0
index 355b8c7..144db9d 100644
@@ -38,6 +38,9 @@ config HARDIRQS_SW_RESEND
 config IRQ_PREFLOW_FASTEOI
        bool
 
+config IRQ_FORCED_THREADING
+       bool
+
 config SPARSE_IRQ
 	bool "Support sparse irq numbering"
 	depends on HAVE_SPARSE_IRQ
kernel/irq/internals.h +2 -0
index 935bec4..6c6ec9a 100644
@@ -27,12 +27,14 @@ extern int noirqdebug;
  * IRQTF_DIED      - handler thread died
  * IRQTF_WARNED    - warning "IRQ_WAKE_THREAD w/o thread_fn" has been printed
  * IRQTF_AFFINITY  - irq thread is requested to adjust affinity
+ * IRQTF_FORCED_THREAD  - irq action is force threaded
  */
 enum {
 	IRQTF_RUNTHREAD,
 	IRQTF_DIED,
 	IRQTF_WARNED,
 	IRQTF_AFFINITY,
+	IRQTF_FORCED_THREAD,
 };
 
 /*
kernel/irq/manage.c +63 -4

105 lines not on this page · raw message

kernel/softirq.c +14 -2
index c049046..a33fb29 100644
@@ -311,9 +311,21 @@ void irq_enter(void)
 }
 
 #ifdef __ARCH_IRQ_EXIT_IRQS_DISABLED
-# define invoke_softirq()	__do_softirq()
+static inline void invoke_softirq(void)
+{
+	if (!force_irqthreads)
+		__do_softirq();
+	else
+		wakeup_softirqd();
+}
 #else
-# define invoke_softirq()	do_softirq()
+static inline void invoke_softirq(void)
+{
+	if (!force_irqthreads)
+		do_softirq();
+	else
+		wakeup_softirqd();
+}
 #endif
 
 /*
#13 lore raw
In reply to Thomas Gleixner
Quoted 16 lines · Thomas Gleixner

Some time ago when the threaded interrupt handlers infrastructure was about to be merged, Andrew asked me where that command line switch was which magically runs all interrupt handlers and the softirqs in threads.

While we were doing that brute force in preempt-rt for quite a while it took some time to come up with a reasonable non intrusive implementation for mainline. We also had to find a solution which fits Linus' recently issued "palatable Trojan horse" requirement (see:
https://lwn.net/Articles/370998/).

The gift of this patch series is the ability to add "threadirqs" to the kernel command line and magically (almost) all interrupt handlers
- except those which are explicitely marked IRQF_NO_THREAD - are confined into threads along with all soft interrupts.

Quoted 7 lines

That allows to enhance the debugability of the kernel as a bug in an interrupt handler is not necessarily taking the whole machine down. It's just the particular irq thread which goes into nirwana. Bad luck if that's the one which is crucial to retrieve the bug report, but in most cases - yes, I analysed quite a lot of bugzilla reports - it will be helpful for reporters not to be forced to transcribe the bug from the screen.

Just a quick bike shed painting suggestion: could we please name it anything but 'forced' threaded irqs? Something like 'irqthread debugging' or 'full irqthreads'?

Thanks,

Ingo