[patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2

[patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2

[Thomas Gleixner]

This patch series implements support for threaded irq handlers for the
generic IRQ layer.

Changes vs. V1:
	- review comments addressed
	- irq affinity setting for handler threads

Threaded interrupt handlers are not only interesting in the preempt-rt
context. Threaded interrupt handlers can help to address common
problems in the interrupt handling code:

 - move long running handlers out of the hard interrupt context

 - avoid complex hardirq -> tasklet/softirq interaction and locking
   problems by integration of this functionality into the threaded
   handler code

 - improved debugability of the kernel: faulty handlers do not take
   down the system.

 - allows prioritizing of the handlers which share an interrupt line

The implementation provides an opt-in mechanism to convert drivers to
the threaded interrupt handler model contrary to the preempt-rt patch
where the threaded handlers are enabled by a brute force switch. The
brute force switch is suboptimal as it does not change the interrupt
handler -> tasklet/softirq interaction problems, but was the only way
which was possible for the limited man power of the preempt-rt
developers.

Converting an interrupt to threaded makes only sense when the handler
code takes advantage of it by integrating tasklet/softirq
functionality and simplifying the locking.

When a driver wants to use threaded interrupt handlers it needs to
provide a quick check handler function, which checks whether the
interrupt was originated from the device or not.

In case it was originated from the device the quick check handler must
disable the interrupt at the device level and return
IRQ_WAKE_THREAD. The generic interrupt handling core then sets the
IRQF_RUNTHREAD in the irqaction of the handler and wakes the
associated thread.

The irqaction is referenced in the threads task_struct so handlers can
check for newly arrived interrupts in action flags. Aside of that the
reference is used to prevent further referencing of the thread in the
interrupt code in the case of segfault to make sure that the system
(minus the now dead interrupt handler) survives and debug information
can be retrieved. In the best case the driver can be restarted, but
dont expect that as a given.

The code was tested with a converted USB EHCI driver. The EHCI shares
an interrupt line with another device and both the threaded and the
non threaded handlers coexist nicely.

Thanks,

	tglx

[patch 1/4] genirq: make irqreturn_t an enum

[Thomas Gleixner]

[-- Attachment #1: genirq-makeirq-return-t-an-enum.patch --]
[-- Type: text/plain, Size: 2249 bytes --]

Impact: cleanup

Remove the 2.4 compabiliy cruft

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Peter Zijlstra <peterz@infradead.org>

---
 include/linux/irq.h       |    4 ++--
 include/linux/irqreturn.h |   28 ++++++++++------------------
 2 files changed, 12 insertions(+), 20 deletions(-)

Index: linux-2.6-tip/include/linux/irq.h
===================================================================
--- linux-2.6-tip.orig/include/linux/irq.h
+++ linux-2.6-tip/include/linux/irq.h
@@ -272,7 +272,7 @@ static inline int irq_balancing_disabled
 }
 
 /* Handle irq action chains: */
-extern int handle_IRQ_event(unsigned int irq, struct irqaction *action);
+extern irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action);
 
 /*
  * Built-in IRQ handlers for various IRQ types,
@@ -317,7 +317,7 @@ static inline void generic_handle_irq(un
 
 /* Handling of unhandled and spurious interrupts: */
 extern void note_interrupt(unsigned int irq, struct irq_desc *desc,
-			   int action_ret);
+			   irqreturn_t action_ret);
 
 /* Resending of interrupts :*/
 void check_irq_resend(struct irq_desc *desc, unsigned int irq);
Index: linux-2.6-tip/include/linux/irqreturn.h
===================================================================
--- linux-2.6-tip.orig/include/linux/irqreturn.h
+++ linux-2.6-tip/include/linux/irqreturn.h
@@ -1,25 +1,17 @@
-/* irqreturn.h */
 #ifndef _LINUX_IRQRETURN_H
 #define _LINUX_IRQRETURN_H
 
-/*
- * For 2.4.x compatibility, 2.4.x can use
- *
- *	typedef void irqreturn_t;
- *	#define IRQ_NONE
- *	#define IRQ_HANDLED
- *	#define IRQ_RETVAL(x)
- *
- * To mix old-style and new-style irq handler returns.
- *
- * IRQ_NONE means we didn't handle it.
- * IRQ_HANDLED means that we did have a valid interrupt and handled it.
- * IRQ_RETVAL(x) selects on the two depending on x being non-zero (for handled)
+/**
+ * enum irqreturn
+ * @IRQ_NONE		interrupt was not from this device
+ * @IRQ_HANDLED		interrupt was handled by this device
  */
-typedef int irqreturn_t;
+enum irqreturn {
+	IRQ_NONE,
+	IRQ_HANDLED,
+};
 
-#define IRQ_NONE	(0)
-#define IRQ_HANDLED	(1)
-#define IRQ_RETVAL(x)	((x) != 0)
+typedef enum irqreturn irqreturn_t;
+#define IRQ_RETVAL(x)	((x) != IRQ_NONE)
 
 #endif

Re: [patch 1/4] genirq: make irqreturn_t an enum

[Geert Uytterhoeven]

On Thu, Feb 26, 2009 at 14:28, Thomas Gleixner <tglx@linutronix.de> wrote:
> Impact: cleanup
>
> Remove the 2.4 compabiliy cruft

> --- linux-2.6-tip.orig/include/linux/irqreturn.h
> +++ linux-2.6-tip/include/linux/irqreturn.h
> @@ -1,25 +1,17 @@
> -/* irqreturn.h */
>  #ifndef _LINUX_IRQRETURN_H
>  #define _LINUX_IRQRETURN_H
>
> -/*
> - * For 2.4.x compatibility, 2.4.x can use
> - *
> - *     typedef void irqreturn_t;
> - *     #define IRQ_NONE
> - *     #define IRQ_HANDLED
> - *     #define IRQ_RETVAL(x)
> - *
> - * To mix old-style and new-style irq handler returns.
> - *
> - * IRQ_NONE means we didn't handle it.
> - * IRQ_HANDLED means that we did have a valid interrupt and handled it.
> - * IRQ_RETVAL(x) selects on the two depending on x being non-zero (for handled)
> +/**
> + * enum irqreturn
> + * @IRQ_NONE           interrupt was not from this device
> + * @IRQ_HANDLED                interrupt was handled by this device
>  */
> -typedef int irqreturn_t;
> +enum irqreturn {
> +       IRQ_NONE,
> +       IRQ_HANDLED,
> +};
>
> -#define IRQ_NONE       (0)
> -#define IRQ_HANDLED    (1)
> -#define IRQ_RETVAL(x)  ((x) != 0)
> +typedef enum irqreturn irqreturn_t;
> +#define IRQ_RETVAL(x)  ((x) != IRQ_NONE)

JFYI, this causes the following warning in linux-next on m68k:

| arch/m68k/kernel/ints.c:231: warning: assignment from incompatible
pointer type

(cfr. http://kisskb.ellerman.id.au/kisskb/buildresult/271602/)

as struct irq_node in arch/m68k/include/asm/irq_mm.h still has a
`int (*handler)(int, void *);'.

I'll cook a patch, but there may be other users hidden somewhere in
arch-specific code...

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[patch 2/4] genirq: use kzalloc instead of explicit zero initialization

[Thomas Gleixner]

[-- Attachment #1: genirq-kzalloc-irq-action.patch --]
[-- Type: text/plain, Size: 863 bytes --]

Impact: simplification

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Peter Zijlstra <peterz@infradead.org>
---
 kernel/irq/manage.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Index: linux-2.6-tip/kernel/irq/manage.c
===================================================================
--- linux-2.6-tip.orig/kernel/irq/manage.c
+++ linux-2.6-tip/kernel/irq/manage.c
@@ -714,15 +714,13 @@ int request_irq(unsigned int irq, irq_ha
 	if (!handler)
 		return -EINVAL;
 
-	action = kmalloc(sizeof(struct irqaction), GFP_KERNEL);
+	action = kzalloc(sizeof(struct irqaction), GFP_KERNEL);
 	if (!action)
 		return -ENOMEM;
 
 	action->handler = handler;
 	action->flags = irqflags;
-	cpus_clear(action->mask);
 	action->name = devname;
-	action->next = NULL;
 	action->dev_id = dev_id;
 
 	retval = __setup_irq(irq, desc, action);

[patch 3/4] genirq: add a quick check handler

[Thomas Gleixner]

[-- Attachment #1: genirq-add-quickcheckhandler.patch --]
[-- Type: text/plain, Size: 4864 bytes --]

Preparatory patch for threaded interrupt handlers.

Adds a quick check handler which is called before the real handler.
The quick check handler can decide whether the interrupt was originated
from the device or not. It can also declare the interrupt as handled
and subsequently avoid that the real handler is called.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Peter Zijlstra <peterz@infradead.org>

---
 include/linux/interrupt.h |   16 ++++++++++++++--
 include/linux/irqreturn.h |    2 ++
 kernel/irq/handle.c       |   18 +++++++++++++++---
 kernel/irq/manage.c       |   17 ++++++++++++-----
 4 files changed, 43 insertions(+), 10 deletions(-)

Index: linux-2.6-tip/include/linux/interrupt.h
===================================================================
--- linux-2.6-tip.orig/include/linux/interrupt.h
+++ linux-2.6-tip/include/linux/interrupt.h
@@ -62,6 +62,7 @@
 typedef irqreturn_t (*irq_handler_t)(int, void *);
 
 struct irqaction {
+	irq_handler_t quick_check_handler;
 	irq_handler_t handler;
 	unsigned long flags;
 	cpumask_t mask;
@@ -73,8 +74,19 @@ struct irqaction {
 };
 
 extern irqreturn_t no_action(int cpl, void *dev_id);
-extern int __must_check request_irq(unsigned int, irq_handler_t handler,
-		       unsigned long, const char *, void *);
+
+extern int __must_check
+request_irq_quickcheck(unsigned int irq, irq_handler_t handler,
+		       irq_handler_t quick_check_handler,
+		       unsigned long flags, const char *name, void *dev);
+
+static inline int __must_check
+request_irq(unsigned int irq, irq_handler_t handler, unsigned long flags,
+	    const char *name, void *dev)
+{
+	return request_irq_quickcheck(irq, handler, NULL, flags, name, dev);
+}
+
 extern void free_irq(unsigned int, void *);
 
 struct device;
Index: linux-2.6-tip/include/linux/irqreturn.h
===================================================================
--- linux-2.6-tip.orig/include/linux/irqreturn.h
+++ linux-2.6-tip/include/linux/irqreturn.h
@@ -5,10 +5,12 @@
  * enum irqreturn
  * @IRQ_NONE		interrupt was not from this device
  * @IRQ_HANDLED		interrupt was handled by this device
+ * @IRQ_NEEDS_HANDLING	quick check handler requests to run real handler
  */
 enum irqreturn {
 	IRQ_NONE,
 	IRQ_HANDLED,
+	IRQ_NEEDS_HANDLING,
 };
 
 typedef enum irqreturn irqreturn_t;
Index: linux-2.6-tip/kernel/irq/handle.c
===================================================================
--- linux-2.6-tip.orig/kernel/irq/handle.c
+++ linux-2.6-tip/kernel/irq/handle.c
@@ -354,9 +354,21 @@ irqreturn_t handle_IRQ_event(unsigned in
 		local_irq_enable_in_hardirq();
 
 	do {
-		ret = action->handler(irq, action->dev_id);
-		if (ret == IRQ_HANDLED)
-			status |= action->flags;
+		if (action->quick_check_handler)
+			ret = action->quick_check_handler(irq, action->dev_id);
+		else
+			ret = IRQ_NEEDS_HANDLING;
+
+		switch (ret) {
+		default:
+			break;
+
+		case IRQ_NEEDS_HANDLING:
+			ret = action->handler(irq, action->dev_id);
+			if (ret == IRQ_HANDLED)
+				status |= action->flags;
+			break;
+		}
 		retval |= ret;
 		action = action->next;
 	} while (action);
Index: linux-2.6-tip/kernel/irq/manage.c
===================================================================
--- linux-2.6-tip.orig/kernel/irq/manage.c
+++ linux-2.6-tip/kernel/irq/manage.c
@@ -641,9 +641,13 @@ void free_irq(unsigned int irq, void *de
 EXPORT_SYMBOL(free_irq);
 
 /**
- *	request_irq - allocate an interrupt line
+ *	request_irq_quickcheck - allocate an interrupt line
  *	@irq: Interrupt line to allocate
- *	@handler: Function to be called when the IRQ occurs
+ *	@handler: Function to be called when the IRQ occurs.
+ *		  Primary handler for threaded interrupts
+ *      @quick_check_handler: Function called before the real interrupt
+ *			handler. It checks if the interrupt originated
+ *			from the device. This can be NULL.
  *	@irqflags: Interrupt type flags
  *	@devname: An ascii name for the claiming device
  *	@dev_id: A cookie passed back to the handler function
@@ -670,8 +674,10 @@ EXPORT_SYMBOL(free_irq);
  *	IRQF_TRIGGER_*		Specify active edge(s) or level
  *
  */
-int request_irq(unsigned int irq, irq_handler_t handler,
-		unsigned long irqflags, const char *devname, void *dev_id)
+int request_irq_quickcheck(unsigned int irq, irq_handler_t handler,
+			   irq_handler_t quick_check_handler,
+			   unsigned long irqflags, const char *devname,
+			   void *dev_id)
 {
 	struct irqaction *action;
 	struct irq_desc *desc;
@@ -718,6 +724,7 @@ int request_irq(unsigned int irq, irq_ha
 	if (!action)
 		return -ENOMEM;
 
+	action->quick_check_handler = quick_check_handler;
 	action->handler = handler;
 	action->flags = irqflags;
 	action->name = devname;
@@ -748,4 +755,4 @@ int request_irq(unsigned int irq, irq_ha
 #endif
 	return retval;
 }
-EXPORT_SYMBOL(request_irq);
+EXPORT_SYMBOL(request_irq_quickcheck);

Re: [patch 3/4] genirq: add a quick check handler

[Andrew Morton]

On Thu, 26 Feb 2009 13:28:18 -0000
Thomas Gleixner <tglx@linutronix.de> wrote:

> Preparatory patch for threaded interrupt handlers.
> 
> Adds a quick check handler which is called before the real handler.
> The quick check handler can decide whether the interrupt was originated
> from the device or not. It can also declare the interrupt as handled
> and subsequently avoid that the real handler is called.
> 
> ...
> 
> Index: linux-2.6-tip/include/linux/interrupt.h
> ===================================================================
> --- linux-2.6-tip.orig/include/linux/interrupt.h
> +++ linux-2.6-tip/include/linux/interrupt.h
> @@ -62,6 +62,7 @@
>  typedef irqreturn_t (*irq_handler_t)(int, void *);
>  
>  struct irqaction {
> +	irq_handler_t quick_check_handler;
>  	irq_handler_t handler;
>  	unsigned long flags;
>  	cpumask_t mask;
> @@ -73,8 +74,19 @@ struct irqaction {
>  };
>  
>  extern irqreturn_t no_action(int cpl, void *dev_id);
> -extern int __must_check request_irq(unsigned int, irq_handler_t handler,
> -		       unsigned long, const char *, void *);
> +
> +extern int __must_check
> +request_irq_quickcheck(unsigned int irq, irq_handler_t handler,
> +		       irq_handler_t quick_check_handler,
> +		       unsigned long flags, const char *name, void *dev);
> +
> +static inline int __must_check
> +request_irq(unsigned int irq, irq_handler_t handler, unsigned long flags,
> +	    const char *name, void *dev)
> +{
> +	return request_irq_quickcheck(irq, handler, NULL, flags, name, dev);
> +}
> +
>  extern void free_irq(unsigned int, void *);
>  
>  struct device;
> Index: linux-2.6-tip/include/linux/irqreturn.h
> ===================================================================
> --- linux-2.6-tip.orig/include/linux/irqreturn.h
> +++ linux-2.6-tip/include/linux/irqreturn.h
> @@ -5,10 +5,12 @@
>   * enum irqreturn
>   * @IRQ_NONE		interrupt was not from this device
>   * @IRQ_HANDLED		interrupt was handled by this device
> + * @IRQ_NEEDS_HANDLING	quick check handler requests to run real handler
>   */
>  enum irqreturn {
>  	IRQ_NONE,
>  	IRQ_HANDLED,
> +	IRQ_NEEDS_HANDLING,
>  };

The enquiring mind is wondering which of these values the quickcheck
handler can return.  IRQ_NEEDS_HANDLING or IRQ_NONE?  Or can it
legitimately return IRQ_HANDLED?  If so, what would that semantically
mean?

I mean, an IRQ handler could easily have a super-fast-path and a slow
path.  It could decide to do the super-fast operation in hard irq
context and return IRQ_HANDLED, and return IRQ_NEEDS_HANDLING if
slow-path handling is needed?

It's all a bit unclear and deserves documenting and thinking about.

>  typedef enum irqreturn irqreturn_t;
> Index: linux-2.6-tip/kernel/irq/handle.c
> ===================================================================
> --- linux-2.6-tip.orig/kernel/irq/handle.c
> +++ linux-2.6-tip/kernel/irq/handle.c
> @@ -354,9 +354,21 @@ irqreturn_t handle_IRQ_event(unsigned in
>  		local_irq_enable_in_hardirq();
>  
>  	do {
> -		ret = action->handler(irq, action->dev_id);
> -		if (ret == IRQ_HANDLED)
> -			status |= action->flags;
> +		if (action->quick_check_handler)
> +			ret = action->quick_check_handler(irq, action->dev_id);
> +		else
> +			ret = IRQ_NEEDS_HANDLING;
> +
> +		switch (ret) {
> +		default:
> +			break;
> +
> +		case IRQ_NEEDS_HANDLING:
> +			ret = action->handler(irq, action->dev_id);
> +			if (ret == IRQ_HANDLED)
> +				status |= action->flags;
> +			break;
> +		}
>  		retval |= ret;
>  		action = action->next;
>  	} while (action);
> Index: linux-2.6-tip/kernel/irq/manage.c
> ===================================================================
> --- linux-2.6-tip.orig/kernel/irq/manage.c
> +++ linux-2.6-tip/kernel/irq/manage.c
> @@ -641,9 +641,13 @@ void free_irq(unsigned int irq, void *de
>  EXPORT_SYMBOL(free_irq);
>  
>  /**
> - *	request_irq - allocate an interrupt line
> + *	request_irq_quickcheck - allocate an interrupt line
>   *	@irq: Interrupt line to allocate
> - *	@handler: Function to be called when the IRQ occurs
> + *	@handler: Function to be called when the IRQ occurs.
> + *		  Primary handler for threaded interrupts
> + *      @quick_check_handler: Function called before the real interrupt
> + *			handler. It checks if the interrupt originated
> + *			from the device. This can be NULL.

So what semantics are implemented if this pointer is NULL?  We just
assume IRQ_NEEDS_HANDLING?


>   *	@irqflags: Interrupt type flags
>   *	@devname: An ascii name for the claiming device
>   *	@dev_id: A cookie passed back to the handler function
> @@ -670,8 +674,10 @@ EXPORT_SYMBOL(free_irq);
>   *	IRQF_TRIGGER_*		Specify active edge(s) or level
>   *
>   */
> -int request_irq(unsigned int irq, irq_handler_t handler,
> -		unsigned long irqflags, const char *devname, void *dev_id)
> +int request_irq_quickcheck(unsigned int irq, irq_handler_t handler,
> +			   irq_handler_t quick_check_handler,
> +			   unsigned long irqflags, const char *devname,
> +			   void *dev_id)
>  {
>  	struct irqaction *action;
>  	struct irq_desc *desc;
> @@ -718,6 +724,7 @@ int request_irq(unsigned int irq, irq_ha
>  	if (!action)
>  		return -ENOMEM;
>  
> +	action->quick_check_handler = quick_check_handler;
>  	action->handler = handler;
>  	action->flags = irqflags;
>  	action->name = devname;
> @@ -748,4 +755,4 @@ int request_irq(unsigned int irq, irq_ha
>  #endif
>  	return retval;
>  }
> -EXPORT_SYMBOL(request_irq);
> +EXPORT_SYMBOL(request_irq_quickcheck);
> 

Re: [patch 3/4] genirq: add a quick check handler

[Thomas Gleixner]

On Thu, 26 Feb 2009, Andrew Morton wrote:
> >  enum irqreturn {
> >  	IRQ_NONE,
> >  	IRQ_HANDLED,
> > +	IRQ_NEEDS_HANDLING,
> >  };
> 
> The enquiring mind is wondering which of these values the quickcheck
> handler can return.  IRQ_NEEDS_HANDLING or IRQ_NONE?  Or can it
> legitimately return IRQ_HANDLED?  If so, what would that semantically
> mean?

Yes it can return IRQ_HANDLED as well. That means no further action
(calling the slow path handler) is needed.
 
> I mean, an IRQ handler could easily have a super-fast-path and a slow
> path.  It could decide to do the super-fast operation in hard irq
> context and return IRQ_HANDLED, and return IRQ_NEEDS_HANDLING if
> slow-path handling is needed?
> 
> It's all a bit unclear and deserves documenting and thinking about.

Yep, I need to sit down and write up documentation.
 
> > + *      @quick_check_handler: Function called before the real interrupt
> > + *			handler. It checks if the interrupt originated
> > + *			from the device. This can be NULL.
> 
> So what semantics are implemented if this pointer is NULL?  We just
> assume IRQ_NEEDS_HANDLING?

Correct.

	tglx

Re: [patch 3/4] genirq: add a quick check handler

[Christoph Hellwig]

I really disagree with the notation of the pre-handler.  Instead of
adding an additional pre handler method you should add a new threadfn
method.  The handler could just as now handle/not handle the interrupt,
or as a third option defer it to the thread.  That makes the different
semantics a lot clearer, and means ->handler and ->threadfn both have
very well defined contexts, instead of sometimes calling ->handler
sometimes from irq and sometimes from thread context.  This also
makes it much easier for complex hardware that might have simple and
fast interrupts that it may want to handle directly from hardirq context
in just a couple of cycles or complex interrupts that might be deferred
to process context.

In that model that main loop in handle_IRQ_event would look something
like this:


	do {
		ret = action->handler(irq, action->dev_id);
		switch (ret) {
		case IRQ_HANDLED:
			status |= action->flags;
			break;
		case IRQ_WAKE_THREAD:
			if (likely(!test_bit(IRQTF_DIED,
					     &action->thread_flags))) {
				set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
				wake_up_process(action->thread);
			}
			/*
			 * Set it to handled so the spurious check
			 * does not trigger.
			 */
			ret = IRQ_HANDLED;
			break;
		}
		retval |= ret;
		action = action->next;
	 } while (action);

Re: [patch 3/4] genirq: add a quick check handler

[Thomas Gleixner]

On Sat, 28 Feb 2009, Christoph Hellwig wrote:
> I really disagree with the notation of the pre-handler.  Instead of
> adding an additional pre handler method you should add a new threadfn
> method.  The handler could just as now handle/not handle the interrupt,
> or as a third option defer it to the thread.  That makes the different
> semantics a lot clearer, and means ->handler and ->threadfn both have
> very well defined contexts, instead of sometimes calling ->handler
> sometimes from irq and sometimes from thread context.  This also
> makes it much easier for complex hardware that might have simple and
> fast interrupts that it may want to handle directly from hardirq context
> in just a couple of cycles or complex interrupts that might be deferred
> to process context.
> 
> In that model that main loop in handle_IRQ_event would look something
> like this:
> 
> 
> 	do {
> 		ret = action->handler(irq, action->dev_id);
> 		switch (ret) {
> 		case IRQ_HANDLED:
> 			status |= action->flags;
> 			break;
> 		case IRQ_WAKE_THREAD:
> 			if (likely(!test_bit(IRQTF_DIED,
> 					     &action->thread_flags))) {
> 				set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
> 				wake_up_process(action->thread);
> 			}
> 			/*
> 			 * Set it to handled so the spurious check
> 			 * does not trigger.
> 			 */
> 			ret = IRQ_HANDLED;
> 			break;
> 		}
> 		retval |= ret;
> 		action = action->next;
> 	 } while (action);

Makes a lot of sense.

Thanks,

	tglx
 

Re: [patch 3/4] genirq: add a quick check handler

[Sven-Thorsten Dietrich]

On Sat, 2009-02-28 at 17:24 -0500, Christoph Hellwig wrote:
> I really disagree with the notation of the pre-handler.  Instead of
> adding an additional pre handler method you should add a new threadfn
> method.  The handler could just as now handle/not handle the interrupt,
> or as a third option defer it to the thread.  That makes the different
> semantics a lot clearer, and means ->handler and ->threadfn both have
> very well defined contexts, instead of sometimes calling ->handler
> sometimes from irq and sometimes from thread context.  This also
> makes it much easier for complex hardware that might have simple and
> fast interrupts that it may want to handle directly from hardirq context
> in just a couple of cycles or complex interrupts that might be deferred
> to process context.
> 

Most of the IRQ handler, whether run in a thread or IRQ context, will be
the same code - so what you are proposing would have to eliminate code
duplication as well as heavy runtime branching overhead.

Ultimately, no matter how its done, the concept of disabling IRQ assert
at the device level, rather than the apic level, is the optimal
"correct" implementation.

Formulating that into the code, as Thomas proposed with the quickcheck,
supplies structural demarcation for semi as well as software design.

Things just get quite ugly - as-is- when you try to add runtime thread
enable and disable.

Sven

Re: [patch 3/4] genirq: add a quick check handler

[Christoph Hellwig]

On Thu, Mar 05, 2009 at 11:59:09AM -0800, Sven-Thorsten Dietrich wrote:
> Most of the IRQ handler, whether run in a thread or IRQ context, will be
> the same code - so what you are proposing would have to eliminate code
> duplication as well as heavy runtime branching overhead.
> 
> Ultimately, no matter how its done, the concept of disabling IRQ assert
> at the device level, rather than the apic level, is the optimal
> "correct" implementation.
> 
> Formulating that into the code, as Thomas proposed with the quickcheck,
> supplies structural demarcation for semi as well as software design.


Umm, the code will be look more or less the same either way.  I just
think overloading the current handler to mean two different things is
a bad idea.  For a driver using a quick disable handler and a long slow
threaded one the only difference is naming the two functions
differently.

I wonder if you're still thinking in the way of a -RT like setup where
threaded interrupts can be enabled and disabled globally?  I don't think
we should ever do that for mainline.

Re: [patch 3/4] genirq: add a quick check handler

[Steven Rostedt]

On Tue, 17 Mar 2009, Christoph Hellwig wrote:
> 
> I wonder if you're still thinking in the way of a -RT like setup where
> threaded interrupts can be enabled and disabled globally?  I don't think
> we should ever do that for mainline.

I agree with this statement. You either want the interrupts as threads or 
you don't. Honestly, I've never used the enable/disable threaded 
interrupts feature in -rt except to test if it worked. But we recommend 
against using it.

-- Steve

[patch 4/4] genirq: add support for threaded interrupt handlers

[Thomas Gleixner]

[-- Attachment #1: genirq-threaded-irq-handler-support.patch --]
[-- Type: text/plain, Size: 13656 bytes --]

Add suppport for threaded interrupt handlers. This is a more strict
separation of the primary interrupt handler, which runs in hard
interrupt context, and the real interrupt handler, which handles the
real device functionality, than the existing hardirq/softirq
separation.

The primary hard interrupt context handler solely checks whether the
interrupt originates from the device or not. In case the interrupt is
asserted by the device the handler disables the interrupt on the
device level. This must be the only functionality of the primary
handler and this restriction has to be carefully monitored to avoid
unresolvable locking scenarios with a fully preemptible kernel.

The threaded handler allows to integrate hardware related
functionality and softirq/tasklet functions in the handler
thread.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Peter Zijlstra <peterz@infradead.org>

---
 include/linux/interrupt.h |   33 +++++++++++
 include/linux/irqreturn.h |    2 
 include/linux/sched.h     |    3 +
 kernel/exit.c             |    2 
 kernel/irq/handle.c       |   43 ++++++++++++--
 kernel/irq/manage.c       |  136 +++++++++++++++++++++++++++++++++++++++++++---
 6 files changed, 208 insertions(+), 11 deletions(-)

Index: linux-2.6-tip/include/linux/interrupt.h
===================================================================
--- linux-2.6-tip.orig/include/linux/interrupt.h
+++ linux-2.6-tip/include/linux/interrupt.h
@@ -49,6 +49,7 @@
  * IRQF_IRQPOLL - Interrupt is used for polling (only the interrupt that is
  *                registered first in an shared interrupt is considered for
  *                performance reasons)
+ * IRQF_THREADED - request threaded interrupt handler
  */
 #define IRQF_DISABLED		0x00000020
 #define IRQF_SAMPLE_RANDOM	0x00000040
@@ -58,6 +59,20 @@
 #define IRQF_PERCPU		0x00000400
 #define IRQF_NOBALANCING	0x00000800
 #define IRQF_IRQPOLL		0x00001000
+#define IRQF_THREADED		0x00002000
+
+/*
+ * Bits used by threaded handlers:
+ * IRQTF_RUNTHREAD - signals that the interrupt handler thread should run
+ * IRQTF_WARNED    - warning rate limit when a threaded handler is
+ *		     requested to run in hard interrupt context
+ * IRQTF_DIED      - handler thread died
+ */
+enum {
+	IRQTF_RUNTHREAD,
+	IRQTF_WARNED,
+	IRQTF_DIED,
+};
 
 typedef irqreturn_t (*irq_handler_t)(int, void *);
 
@@ -71,6 +86,8 @@ struct irqaction {
 	struct irqaction *next;
 	int irq;
 	struct proc_dir_entry *dir;
+	struct task_struct *thread;
+	unsigned long thread_flags;
 };
 
 extern irqreturn_t no_action(int cpl, void *dev_id);
@@ -87,6 +104,15 @@ request_irq(unsigned int irq, irq_handle
 	return request_irq_quickcheck(irq, handler, NULL, flags, name, dev);
 }
 
+static inline int __must_check
+request_threaded_irq(unsigned int irq, irq_handler_t handler,
+		     irq_handler_t quick_check_handler,
+		     unsigned long flags, const char *name, void *dev)
+{
+	return request_irq_quickcheck(irq, handler, quick_check_handler,
+				    flags | IRQF_THREADED, name, dev);
+}
+
 extern void free_irq(unsigned int, void *);
 
 struct device;
@@ -96,6 +122,13 @@ extern int __must_check devm_request_irq
 			    const char *devname, void *dev_id);
 extern void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id);
 
+static inline int irq_thread_should_run(struct irqaction *action)
+{
+	return test_and_clear_bit(IRQTF_RUNTHREAD, &action->flags);
+}
+
+extern void exit_irq_thread(struct task_struct *tsk);
+
 /*
  * On lockdep we dont want to enable hardirqs in hardirq
  * context. Use local_irq_enable_in_hardirq() to annotate
Index: linux-2.6-tip/include/linux/irqreturn.h
===================================================================
--- linux-2.6-tip.orig/include/linux/irqreturn.h
+++ linux-2.6-tip/include/linux/irqreturn.h
@@ -6,11 +6,13 @@
  * @IRQ_NONE		interrupt was not from this device
  * @IRQ_HANDLED		interrupt was handled by this device
  * @IRQ_NEEDS_HANDLING	quick check handler requests to run real handler
+ * @IRQ_WAKE_THREAD	quick check handler requests to wake the handler thread
  */
 enum irqreturn {
 	IRQ_NONE,
 	IRQ_HANDLED,
 	IRQ_NEEDS_HANDLING,
+	IRQ_WAKE_THREAD,
 };
 
 typedef enum irqreturn irqreturn_t;
Index: linux-2.6-tip/include/linux/sched.h
===================================================================
--- linux-2.6-tip.orig/include/linux/sched.h
+++ linux-2.6-tip/include/linux/sched.h
@@ -1307,6 +1307,9 @@ struct task_struct {
 /* Protection of (de-)allocation: mm, files, fs, tty, keyrings */
 	spinlock_t alloc_lock;
 
+	/* IRQ handler threads */
+	struct irqaction *irqaction;
+
 	/* Protection of the PI data structures: */
 	spinlock_t pi_lock;
 
Index: linux-2.6-tip/kernel/exit.c
===================================================================
--- linux-2.6-tip.orig/kernel/exit.c
+++ linux-2.6-tip/kernel/exit.c
@@ -1040,6 +1040,8 @@ NORET_TYPE void do_exit(long code)
 		schedule();
 	}
 
+	exit_irq_thread(tsk);
+
 	exit_signals(tsk);  /* sets PF_EXITING */
 	/*
 	 * tsk->flags are checked in the futex code to protect against
Index: linux-2.6-tip/kernel/irq/handle.c
===================================================================
--- linux-2.6-tip.orig/kernel/irq/handle.c
+++ linux-2.6-tip/kernel/irq/handle.c
@@ -360,13 +360,46 @@ irqreturn_t handle_IRQ_event(unsigned in
 			ret = IRQ_NEEDS_HANDLING;
 
 		switch (ret) {
-		default:
+		case IRQ_NEEDS_HANDLING:
+			if (!(action->flags & IRQF_THREADED)) {
+				ret = action->handler(irq, action->dev_id);
+				if (ret == IRQ_HANDLED)
+					status |= action->flags;
+				break;
+			}
+			/*
+			 * Warn once when a quick check handler asked
+			 * for invoking the threaded (main) handler
+			 * directly
+			 */
+			WARN(!test_and_set_bit(IRQTF_WARNED,
+					       &action->thread_flags),
+			     "IRQ %d requested to run threaded handler "
+			     "in hard interrupt context\n", irq);
+
+			/* Fall through and wake the thread */
+		case IRQ_WAKE_THREAD:
+			/*
+			 * In case the thread crashed and was killed
+			 * we just pretend that we handled the
+			 * interrupt. The quick check handler 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);
+			}
+
+			/*
+			 * Set it to handled so the spurious check
+			 * does not trigger.
+			 */
+			ret = IRQ_HANDLED;
 			break;
 
-		case IRQ_NEEDS_HANDLING:
-			ret = action->handler(irq, action->dev_id);
-			if (ret == IRQ_HANDLED)
-				status |= action->flags;
+		default:
 			break;
 		}
 		retval |= ret;
Index: linux-2.6-tip/kernel/irq/manage.c
===================================================================
--- linux-2.6-tip.orig/kernel/irq/manage.c
+++ linux-2.6-tip/kernel/irq/manage.c
@@ -8,10 +8,12 @@
  */
 
 #include <linux/irq.h>
+#include <linux/kthread.h>
 #include <linux/module.h>
 #include <linux/random.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
+#include <linux/sched.h>
 
 #include "internals.h"
 
@@ -27,6 +29,12 @@ cpumask_var_t irq_default_affinity;
  *	holding a resource the IRQ handler may need you will deadlock.
  *
  *	This function may be called - with care - from IRQ context.
+ *
+ *	Note: with threaded interrupt handlers synchronize_irq does
+ *	_NOT_ guarantee that the handler thread is in a quiescent
+ *	state. There is no general solution for this as threaded
+ *	handlers can sleep and have other fancy states which are not
+ *	that simple to track as the hardirq handler.
  */
 void synchronize_irq(unsigned int irq)
 {
@@ -72,6 +80,18 @@ int irq_can_set_affinity(unsigned int ir
 	return 1;
 }
 
+static void
+irq_set_thread_affinity(struct irq_desc *desc, const struct cpumask *cpumask)
+{
+	struct irqaction *action = desc->action;
+
+	while (action) {
+		if (action->thread)
+			set_cpus_allowed_ptr(action->thread, cpumask);
+		action = action->next;
+	}
+}
+
 /**
  *	irq_set_affinity - Set the irq affinity of a given irq
  *	@irq:		Interrupt to set affinity
@@ -100,6 +120,7 @@ int irq_set_affinity(unsigned int irq, c
 	cpumask_copy(desc->affinity, cpumask);
 	desc->chip->set_affinity(irq, cpumask);
 #endif
+	irq_set_thread_affinity(desc, cpumask);
 	desc->status |= IRQ_AFFINITY_SET;
 	spin_unlock_irqrestore(&desc->lock, flags);
 	return 0;
@@ -150,6 +171,8 @@ int irq_select_affinity_usr(unsigned int
 
 	spin_lock_irqsave(&desc->lock, flags);
 	ret = setup_affinity(irq, desc);
+	if (!ret)
+		irq_set_thread_affinity(desc, &desc->affinity);
 	spin_unlock_irqrestore(&desc->lock, flags);
 
 	return ret;
@@ -385,6 +408,58 @@ int __irq_set_trigger(struct irq_desc *d
 }
 
 /*
+ * Interrupt handler thread
+ */
+static int irq_thread(void *data)
+{
+	struct irqaction *action = data;
+	struct sched_param param = { 0, };
+
+	/*
+	 * Set irq thread priority to SCHED_FIFO prio 50:
+	 */
+	param.sched_priority = MAX_USER_RT_PRIO/2;
+	sched_setscheduler(current, SCHED_FIFO, &param);
+
+	current->irqaction = action;
+
+	while (!kthread_should_stop()) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (irq_thread_should_run(action) && current->irqaction) {
+			__set_current_state(TASK_RUNNING);
+			action->handler(action->irq, action->dev_id);
+		} else
+			schedule();
+	}
+	/*
+	 * Clear irqaction. Otherwise exit_irq_thread() would make
+	 * fuzz about an active irq thread going into nirvana.
+	 */
+	current->irqaction = NULL;
+	return 0;
+}
+
+/*
+ * Called from do_exit()
+ */
+void exit_irq_thread(struct task_struct *tsk)
+{
+	if (!tsk->irqaction)
+		return;
+
+	printk(KERN_ERR
+	       "exiting task \"%s\" (%d) is an active IRQ thread (irq %d)\n",
+	       tsk->comm ? tsk->comm : "", tsk->pid, tsk->irqaction->irq);
+
+	/*
+	 * Set the THREAD DIED flag to prevent further wakeups of the
+	 * soon to be gone threaded handler.
+	 */
+	set_bit(IRQTF_DIED, &tsk->irqaction->flags);
+	tsk->irqaction = NULL;
+}
+
+/*
  * Internal function to register an irqaction - typically used to
  * allocate special interrupts that are part of the architecture.
  */
@@ -402,6 +477,11 @@ __setup_irq(unsigned int irq, struct irq
 
 	if (desc->chip == &no_irq_chip)
 		return -ENOSYS;
+
+	/* Threaded interrupts need a quickcheck handler */
+	if ((new->flags & IRQF_THREADED) && !new->quick_check_handler)
+		return -EINVAL;
+
 	/*
 	 * Some drivers like serial.c use request_irq() heavily,
 	 * so we have to be careful not to interfere with a
@@ -420,6 +500,26 @@ __setup_irq(unsigned int irq, struct irq
 	}
 
 	/*
+	 * Threaded handler ?
+	 */
+	if (new->flags & IRQF_THREADED) {
+		struct task_struct *t;
+
+		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
+				   new->name);
+		if (IS_ERR(t))
+			return PTR_ERR(t);
+		/*
+		 * We keep the reference to the task struct even if
+		 * the thread dies to avoid that the interrupt code
+		 * references an already gone task_struct.
+		 */
+		get_task_struct(t);
+		new->thread = t;
+		wake_up_process(t);
+	}
+
+	/*
 	 * The following block of code has to be executed atomically
 	 */
 	spin_lock_irqsave(&desc->lock, flags);
@@ -461,10 +561,8 @@ __setup_irq(unsigned int irq, struct irq
 			ret = __irq_set_trigger(desc, irq,
 					new->flags & IRQF_TRIGGER_MASK);
 
-			if (ret) {
-				spin_unlock_irqrestore(&desc->lock, flags);
-				return ret;
-			}
+			if (ret)
+				goto out_thread;
 		} else
 			compat_irq_chip_set_default_handler(desc);
 #if defined(CONFIG_IRQ_PER_CPU)
@@ -532,8 +630,19 @@ mismatch:
 		dump_stack();
 	}
 #endif
+	ret = -EBUSY;
+
+out_thread:
 	spin_unlock_irqrestore(&desc->lock, flags);
-	return -EBUSY;
+	if (new->thread) {
+		struct task_struct *t = new->thread;
+
+		new->thread = NULL;
+		if (likely(!test_bit(IRQTF_DIED, &new->thread_flags)))
+			kthread_stop(t);
+		put_task_struct(t);
+	}
+	return ret;
 }
 
 /**
@@ -568,6 +677,7 @@ void free_irq(unsigned int irq, void *de
 {
 	struct irq_desc *desc = irq_to_desc(irq);
 	struct irqaction *action, **action_ptr;
+	struct task_struct *irqthread;
 	unsigned long flags;
 
 	WARN(in_interrupt(), "Trying to free IRQ %d from IRQ context!\n", irq);
@@ -614,6 +724,12 @@ void free_irq(unsigned int irq, void *de
 		else
 			desc->chip->disable(irq);
 	}
+
+	irqthread = action->thread;
+	action->thread = NULL;
+	if (irqthread)
+		irqthread->irqaction = NULL;
+
 	spin_unlock_irqrestore(&desc->lock, flags);
 
 	unregister_handler_proc(irq, action);
@@ -621,6 +737,12 @@ void free_irq(unsigned int irq, void *de
 	/* Make sure it's not being used on another CPU: */
 	synchronize_irq(irq);
 
+	if (irqthread) {
+		if (!test_bit(IRQTF_DIED, &action->thread_flags))
+			kthread_stop(irqthread);
+		put_task_struct(irqthread);
+	}
+
 #ifdef CONFIG_DEBUG_SHIRQ
 	/*
 	 * It's a shared IRQ -- the driver ought to be prepared for an IRQ
@@ -647,7 +769,8 @@ EXPORT_SYMBOL(free_irq);
  *		  Primary handler for threaded interrupts
  *      @quick_check_handler: Function called before the real interrupt
  *			handler. It checks if the interrupt originated
- *			from the device. This can be NULL.
+ *			from the device. This can be NULL, but is mandatory
+ *			for threaded interrupt handlers.
  *	@irqflags: Interrupt type flags
  *	@devname: An ascii name for the claiming device
  *	@dev_id: A cookie passed back to the handler function
@@ -672,6 +795,7 @@ EXPORT_SYMBOL(free_irq);
  *	IRQF_DISABLED	Disable local interrupts while processing
  *	IRQF_SAMPLE_RANDOM	The interrupt can be used for entropy
  *	IRQF_TRIGGER_*		Specify active edge(s) or level
+ *      IRQF_THREADED           Interrupt is threaded
  *
  */
 int request_irq_quickcheck(unsigned int irq, irq_handler_t handler,

Re: [patch 4/4] genirq: add support for threaded interrupt handlers

[Andrew Morton]

On Thu, 26 Feb 2009 13:28:23 -0000
Thomas Gleixner <tglx@linutronix.de> wrote:

> Add suppport for threaded interrupt handlers. This is a more strict
> separation of the primary interrupt handler, which runs in hard
> interrupt context, and the real interrupt handler, which handles the
> real device functionality, than the existing hardirq/softirq
> separation.
> 
> The primary hard interrupt context handler solely checks whether the
> interrupt originates from the device or not. In case the interrupt is
> asserted by the device the handler disables the interrupt on the
> device level. This must be the only functionality of the primary
> handler and this restriction has to be carefully monitored to avoid
> unresolvable locking scenarios with a fully preemptible kernel.
> 
> The threaded handler allows to integrate hardware related
> functionality and softirq/tasklet functions in the handler
> thread.
> 

A typical device driver will do:

some_function(...)
{
	spin_lock_irqsave(&dev->lock);
}

irq_handler(...)
{
	spin_lock(&dev->lock);
}

and this does not deadlock, because the driver "knows" that the IRQ is
disabled during the execution of the IRQ handler.

But how is this optimisation supported with IRQ threads?  Do we leave
the IRQ disabled during the thread execution?  Or does the driver need
to be changed?

Bearing in mind that the driver might choose to split the IRQ handling
between hard-irq context fastpath and process-context slowpath (I
hope), and that each path might want to take the same lock.

>
> ...
>
> @@ -96,6 +122,13 @@ extern int __must_check devm_request_irq
>  			    const char *devname, void *dev_id);
>  extern void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id);
>  
> +static inline int irq_thread_should_run(struct irqaction *action)
> +{
> +	return test_and_clear_bit(IRQTF_RUNTHREAD, &action->flags);
> +}

I don't think this needs to be placed in a header file?

> +extern void exit_irq_thread(struct task_struct *tsk);
> +
>  /*
>   * On lockdep we dont want to enable hardirqs in hardirq
>   * context. Use local_irq_enable_in_hardirq() to annotate
> Index: linux-2.6-tip/include/linux/irqreturn.h
> ===================================================================
> --- linux-2.6-tip.orig/include/linux/irqreturn.h
> +++ linux-2.6-tip/include/linux/irqreturn.h
> @@ -6,11 +6,13 @@
>   * @IRQ_NONE		interrupt was not from this device
>   * @IRQ_HANDLED		interrupt was handled by this device
>   * @IRQ_NEEDS_HANDLING	quick check handler requests to run real handler
> + * @IRQ_WAKE_THREAD	quick check handler requests to wake the handler thread
>   */
>  enum irqreturn {
>  	IRQ_NONE,
>  	IRQ_HANDLED,
>  	IRQ_NEEDS_HANDLING,
> +	IRQ_WAKE_THREAD,
>  };
>  
>  typedef enum irqreturn irqreturn_t;
> Index: linux-2.6-tip/include/linux/sched.h
> ===================================================================
> --- linux-2.6-tip.orig/include/linux/sched.h
> +++ linux-2.6-tip/include/linux/sched.h
> @@ -1307,6 +1307,9 @@ struct task_struct {
>  /* Protection of (de-)allocation: mm, files, fs, tty, keyrings */
>  	spinlock_t alloc_lock;
>  
> +	/* IRQ handler threads */
> +	struct irqaction *irqaction;
> +

If this field always used?  Perhaps CONFIG_GENERIC_HARDIRQS=n kernels
could save some space?


>  	/* Protection of the PI data structures: */
>  	spinlock_t pi_lock;
>  
> Index: linux-2.6-tip/kernel/exit.c
> ===================================================================
> --- linux-2.6-tip.orig/kernel/exit.c
> +++ linux-2.6-tip/kernel/exit.c
> @@ -1040,6 +1040,8 @@ NORET_TYPE void do_exit(long code)
>  		schedule();
>  	}
>  
> +	exit_irq_thread(tsk);
> +

Did we just break CONFIG_GENERIC_HARDIRQS=n builds?

>  	exit_signals(tsk);  /* sets PF_EXITING */
>  	/*
>  	 * tsk->flags are checked in the futex code to protect against
> Index: linux-2.6-tip/kernel/irq/handle.c
> ===================================================================
> --- linux-2.6-tip.orig/kernel/irq/handle.c
> +++ linux-2.6-tip/kernel/irq/handle.c
> @@ -360,13 +360,46 @@ irqreturn_t handle_IRQ_event(unsigned in
>  			ret = IRQ_NEEDS_HANDLING;
>  
>  		switch (ret) {
> -		default:
> +		case IRQ_NEEDS_HANDLING:
> +			if (!(action->flags & IRQF_THREADED)) {
> +				ret = action->handler(irq, action->dev_id);
> +				if (ret == IRQ_HANDLED)
> +					status |= action->flags;
> +				break;
> +			}
> +			/*
> +			 * Warn once when a quick check handler asked

s/quick check/quickcheck/

> +			 * for invoking the threaded (main) handler
> +			 * directly
> +			 */

It would be nice if the comment were to explain why this is considered
bad.

> +			WARN(!test_and_set_bit(IRQTF_WARNED,
> +					       &action->thread_flags),
> +			     "IRQ %d requested to run threaded handler "
> +			     "in hard interrupt context\n", irq);
> +
> +			/* Fall through and wake the thread */
> +		case IRQ_WAKE_THREAD:
> +			/*
> +			 * In case the thread crashed and was killed
> +			 * we just pretend that we handled the
> +			 * interrupt. The quick check handler has

s/quick check/quickcheck/.  Given that "quickcheck" now enters the
kernel nomenclature, let us use it consistently.

> +			 * disabled the device interrupt, so no irq
> +			 * storm is lurking.
> +			 */

The quickcheck handler has disabled the device interrupt?  Where did
this factoid come from?  What linkage is there between the crashing of
a kernel thread and a quickcheck handler knowing to disable the
interrupt source?  How is a driver writer to know that his quickcheck
handler is to disable the interrupt, and under what circustances?

etc.  Head is spinning a bit.


> +			if (likely(!test_bit(IRQTF_DIED,
> +					     &action->thread_flags))) {
> +				set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
> +				wake_up_process(action->thread);
> +			}
> +
> +			/*
> +			 * Set it to handled so the spurious check
> +			 * does not trigger.
> +			 */
> +			ret = IRQ_HANDLED;
>  			break;
>  
> -		case IRQ_NEEDS_HANDLING:
> -			ret = action->handler(irq, action->dev_id);
> -			if (ret == IRQ_HANDLED)
> -				status |= action->flags;
> +		default:
>  			break;
>
> ...
>
> +static void
> +irq_set_thread_affinity(struct irq_desc *desc, const struct cpumask *cpumask)
> +{
> +	struct irqaction *action = desc->action;
> +
> +	while (action) {
> +		if (action->thread)
> +			set_cpus_allowed_ptr(action->thread, cpumask);
> +		action = action->next;
> +	}
> +}

Is all this code cpu-hotplug-friendly?

>  /**
>   *	irq_set_affinity - Set the irq affinity of a given irq
>   *	@irq:		Interrupt to set affinity
> @@ -100,6 +120,7 @@ int irq_set_affinity(unsigned int irq, c
>  	cpumask_copy(desc->affinity, cpumask);
>  	desc->chip->set_affinity(irq, cpumask);
>  #endif
> +	irq_set_thread_affinity(desc, cpumask);
>  	desc->status |= IRQ_AFFINITY_SET;
>  	spin_unlock_irqrestore(&desc->lock, flags);
>  	return 0;
> @@ -150,6 +171,8 @@ int irq_select_affinity_usr(unsigned int
>  
>  	spin_lock_irqsave(&desc->lock, flags);
>  	ret = setup_affinity(irq, desc);
> +	if (!ret)
> +		irq_set_thread_affinity(desc, &desc->affinity);
>  	spin_unlock_irqrestore(&desc->lock, flags);
>  
>  	return ret;
> @@ -385,6 +408,58 @@ int __irq_set_trigger(struct irq_desc *d
>  }
>  
>  /*
> + * Interrupt handler thread
> + */
> +static int irq_thread(void *data)
> +{
> +	struct irqaction *action = data;
> +	struct sched_param param = { 0, };
> +
> +	/*
> +	 * Set irq thread priority to SCHED_FIFO prio 50:
> +	 */
> +	param.sched_priority = MAX_USER_RT_PRIO/2;

It might be a wee bit more efficient to do

	struct sched_param param = {
		.sched_priority = MAX_USER_RT_PRIO/2,
	};

It would be even more efficient to build this struct at compile time,
but sched_setscheduler() forgot the `const'.

> +	sched_setscheduler(current, SCHED_FIFO, &param);
> +
> +	current->irqaction = action;
> +
> +	while (!kthread_should_stop()) {
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		if (irq_thread_should_run(action) && current->irqaction) {
> +			__set_current_state(TASK_RUNNING);
> +			action->handler(action->irq, action->dev_id);
> +		} else
> +			schedule();
> +	}
> +	/*
> +	 * Clear irqaction. Otherwise exit_irq_thread() would make
> +	 * fuzz about an active irq thread going into nirvana.
> +	 */
> +	current->irqaction = NULL;
> +	return 0;
> +}
> +
> +/*
> + * Called from do_exit()
> + */
> +void exit_irq_thread(struct task_struct *tsk)
> +{
> +	if (!tsk->irqaction)
> +		return;
> +
> +	printk(KERN_ERR
> +	       "exiting task \"%s\" (%d) is an active IRQ thread (irq %d)\n",
> +	       tsk->comm ? tsk->comm : "", tsk->pid, tsk->irqaction->irq);

task_struct.comm is an array, and testing it for NULL makes no sense.

The `tsk' argument is a holdover from the old days when accessing
`current' was considered to be slow.  It would be better if this
fuction were to take no argumetns and were to operate on `current'.

If we _really_ want this function to be able to operate on tasks other
than `current' then we have a whole buncha races to fix first.

> +	/*
> +	 * Set the THREAD DIED flag to prevent further wakeups of the
> +	 * soon to be gone threaded handler.
> +	 */
> +	set_bit(IRQTF_DIED, &tsk->irqaction->flags);
> +	tsk->irqaction = NULL;

Why do we need to do both?  It would be clearer if irq_thread() were to
test IRQTF_DIED.

> +}
> +
> +/*
>   * Internal function to register an irqaction - typically used to
>   * allocate special interrupts that are part of the architecture.
>   */
> @@ -402,6 +477,11 @@ __setup_irq(unsigned int irq, struct irq
>  
>  	if (desc->chip == &no_irq_chip)
>  		return -ENOSYS;
> +
> +	/* Threaded interrupts need a quickcheck handler */
> +	if ((new->flags & IRQF_THREADED) && !new->quick_check_handler)
> +		return -EINVAL;

Seems redundant.  (new->flags & IRQF_THREADED) and
!!new->quick_check_handler both contain the same information?

>  	/*
>  	 * Some drivers like serial.c use request_irq() heavily,
>  	 * so we have to be careful not to interfere with a
> @@ -420,6 +500,26 @@ __setup_irq(unsigned int irq, struct irq
>  	}
>  
>  	/*
> +	 * Threaded handler ?
> +	 */
> +	if (new->flags & IRQF_THREADED) {
> +		struct task_struct *t;
> +
> +		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
> +				   new->name);

Seems there's a big risk of overrunning task_struct.comm[] here.

> +		if (IS_ERR(t))
> +			return PTR_ERR(t);
> +		/*
> +		 * We keep the reference to the task struct even if
> +		 * the thread dies to avoid that the interrupt code
> +		 * references an already gone task_struct.

Sentence needs help.  "to prevent the interrupt code from referencing a
freed task_struct"?

> +		 */
> +		get_task_struct(t);
> +		new->thread = t;
> +		wake_up_process(t);
> +	}
> +
> +	/*
>  	 * The following block of code has to be executed atomically
>  	 */
>  	spin_lock_irqsave(&desc->lock, flags);
>
> ...
>

Re: [patch 4/4] genirq: add support for threaded interrupt handlers

[Arjan van de Ven]

On Thu, 26 Feb 2009 15:32:16 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 26 Feb 2009 13:28:23 -0000
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > Add suppport for threaded interrupt handlers. This is a more strict
> > separation of the primary interrupt handler, which runs in hard
> > interrupt context, and the real interrupt handler, which handles the
> > real device functionality, than the existing hardirq/softirq
> > separation.
> > 
> > The primary hard interrupt context handler solely checks whether the
> > interrupt originates from the device or not. In case the interrupt
> > is asserted by the device the handler disables the interrupt on the
> > device level. This must be the only functionality of the primary
> > handler and this restriction has to be carefully monitored to avoid
> > unresolvable locking scenarios with a fully preemptible kernel.
> > 
> > The threaded handler allows to integrate hardware related
> > functionality and softirq/tasklet functions in the handler
> > thread.
> > 
> 
> A typical device driver will do:
> 
> some_function(...)
> {
> 	spin_lock_irqsave(&dev->lock);
> }
> 
> irq_handler(...)
> {
> 	spin_lock(&dev->lock);
> }
> 
> and this does not deadlock, because the driver "knows" that the IRQ is
> disabled during the execution of the IRQ handler.
> 
> But how is this optimisation supported with IRQ threads?  Do we leave
> the IRQ disabled during the thread execution?  Or does the driver need
> to be changed?
> 
> Bearing in mind that the driver might choose to split the IRQ handling
> between hard-irq context fastpath and process-context slowpath (I
> hope), and that each path might want to take the same lock.
> 


Realistically, for the "we go threaded interrupts" case (which is
opt-in), I think the only sane option is
* the quickhandler runs with irqs off
* the "slow" threaded handler runs with irqs on
And we guarantee both of these conditions from the core, to the point
that I think we should not allow any other combination.

This also should be fine for basically all cases; the quick handler
really needs to be quick so irq off makes sense, and the slow handler
can, worst case, turn off interrupts by itself, but normally is
preemptable etc etc.


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch 4/4] genirq: add support for threaded interrupt handlers

[Andrew Morton]

On Thu, 26 Feb 2009 21:27:52 -0800 Arjan van de Ven <arjan@infradead.org> wrote:

> > 
> > Bearing in mind that the driver might choose to split the IRQ handling
> > between hard-irq context fastpath and process-context slowpath (I
> > hope), and that each path might want to take the same lock.
> > 
> 
> 
> Realistically, for the "we go threaded interrupts" case (which is
> opt-in), I think the only sane option is
> * the quickhandler runs with irqs off
> * the "slow" threaded handler runs with irqs on
> And we guarantee both of these conditions from the core, to the point
> that I think we should not allow any other combination.
> 
> This also should be fine for basically all cases; the quick handler
> really needs to be quick so irq off makes sense, and the slow handler
> can, worst case, turn off interrupts by itself, but normally is
> preemptable etc etc.

I was actually kinda surprised by the patch - it needs moderate changes
to each driver.  I'd have thought that it would be possible to arrange for
_all_ drivers to have their interrupt handlers automagically called from
process context with no driver changes.

Did anyone ever try that?  I think they did...

Re: [patch 4/4] genirq: add support for threaded interrupt handlers

[Peter Zijlstra]

On Thu, 2009-02-26 at 21:45 -0800, Andrew Morton wrote:
> On Thu, 26 Feb 2009 21:27:52 -0800 Arjan van de Ven <arjan@infradead.org> wrote:
> 
> > > 
> > > Bearing in mind that the driver might choose to split the IRQ handling
> > > between hard-irq context fastpath and process-context slowpath (I
> > > hope), and that each path might want to take the same lock.
> > > 
> > 
> > 
> > Realistically, for the "we go threaded interrupts" case (which is
> > opt-in), I think the only sane option is
> > * the quickhandler runs with irqs off
> > * the "slow" threaded handler runs with irqs on
> > And we guarantee both of these conditions from the core, to the point
> > that I think we should not allow any other combination.
> > 
> > This also should be fine for basically all cases; the quick handler
> > really needs to be quick so irq off makes sense, and the slow handler
> > can, worst case, turn off interrupts by itself, but normally is
> > preemptable etc etc.
> 
> I was actually kinda surprised by the patch - it needs moderate changes
> to each driver.  I'd have thought that it would be possible to arrange for
> _all_ drivers to have their interrupt handlers automagically called from
> process context with no driver changes.
> 
> Did anyone ever try that?  I think they did...

Yes, current preempt-rt does exactly that, as mentioned in the
changelog. That same changelog also mentions why this isn't such a hot
idea:


"The implementation provides an opt-in mechanism to convert drivers to
the threaded interrupt handler model contrary to the preempt-rt patch
where the threaded handlers are enabled by a brute force switch. The
brute force switch is suboptimal as it does not change the interrupt
handler -> tasklet/softirq interaction problems, but was the only way
which was possible for the limited man power of the preempt-rt
developers."


So the idea is that we want people to re-think and change their
interrupt routines to take advantage of the benefits of threaded
interrupts and avoid the endless context switching between threaded
interrupts handler, softirq/tasklet/workqueue contexts, which preempt-rt
now has.

The only way to avoid that is by pushing all softirq/tasklet/worklet
code into the threaded handler, and the only way to do that is by
reworking the drivers.

Since there are too many drivers to count on our few hands, we need an
opt-in, so that we can gradually migrate towards this scheme.

Re: [patch 4/4] genirq: add support for threaded interrupt handlers

[Andrew Morton]

On Fri, 27 Feb 2009 08:18:33 +0100 Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, 2009-02-26 at 21:45 -0800, Andrew Morton wrote:
> > On Thu, 26 Feb 2009 21:27:52 -0800 Arjan van de Ven <arjan@infradead.org> wrote:
> > 
> > > > 
> > > > Bearing in mind that the driver might choose to split the IRQ handling
> > > > between hard-irq context fastpath and process-context slowpath (I
> > > > hope), and that each path might want to take the same lock.
> > > > 
> > > 
> > > 
> > > Realistically, for the "we go threaded interrupts" case (which is
> > > opt-in), I think the only sane option is
> > > * the quickhandler runs with irqs off
> > > * the "slow" threaded handler runs with irqs on
> > > And we guarantee both of these conditions from the core, to the point
> > > that I think we should not allow any other combination.
> > > 
> > > This also should be fine for basically all cases; the quick handler
> > > really needs to be quick so irq off makes sense, and the slow handler
> > > can, worst case, turn off interrupts by itself, but normally is
> > > preemptable etc etc.
> > 
> > I was actually kinda surprised by the patch - it needs moderate changes
> > to each driver.  I'd have thought that it would be possible to arrange for
> > _all_ drivers to have their interrupt handlers automagically called from
> > process context with no driver changes.
> > 
> > Did anyone ever try that?  I think they did...
> 
> Yes, current preempt-rt does exactly that, as mentioned in the
> changelog. That same changelog also mentions why this isn't such a hot
> idea:
> 
> 
> "The implementation provides an opt-in mechanism to convert drivers to
> the threaded interrupt handler model contrary to the preempt-rt patch
> where the threaded handlers are enabled by a brute force switch. The
> brute force switch is suboptimal as it does not change the interrupt
> handler -> tasklet/softirq interaction problems, but was the only way
> which was possible for the limited man power of the preempt-rt
> developers."
> 
> 
> So the idea is that we want people to re-think and change their
> interrupt routines to take advantage of the benefits of threaded
> interrupts and avoid the endless context switching between threaded
> interrupts handler, softirq/tasklet/workqueue contexts, which preempt-rt
> now has.
> 
> The only way to avoid that is by pushing all softirq/tasklet/worklet
> code into the threaded handler, and the only way to do that is by
> reworking the drivers.
> 
> Since there are too many drivers to count on our few hands, we need an
> opt-in, so that we can gradually migrate towards this scheme.

OK, fair enough, good decision.

What is the plan (if any) for integrating threaded interrupt handlers
with softirqs?  I guess things will "just work" at present, and
threaded softirqs happen in a later patch?

I'd have thought that the softirq latency could get quite a bit worse
with this proposed threaded-irq patch?

I suppose we could just run the softirq handlers directly after running
the irq handler, from the IRQ thread.  Rather than having to poke
softirqd all the time?

Thwap me if this was all in whatever-changelog-that-was as well ;)


Also...

Given that this threaded-irq code appears to be new and not very tested
in -rt, I think it would be a good idea to convert some popular drivers
(e1000/e1000e) so that the core code gets a good thrashing before we
merge it.

Re: [patch 4/4] genirq: add support for threaded interrupt handlers

[Peter Zijlstra]

On Thu, 2009-02-26 at 23:48 -0800, Andrew Morton wrote:

> > > I was actually kinda surprised by the patch - it needs moderate changes
> > > to each driver.  I'd have thought that it would be possible to arrange for
> > > _all_ drivers to have their interrupt handlers automagically called from
> > > process context with no driver changes.
> > > 
> > > Did anyone ever try that?  I think they did...
> > 
> > Yes, current preempt-rt does exactly that, as mentioned in the
> > changelog. That same changelog also mentions why this isn't such a hot
> > idea:
> > 
> > 
> > "The implementation provides an opt-in mechanism to convert drivers to
> > the threaded interrupt handler model contrary to the preempt-rt patch
> > where the threaded handlers are enabled by a brute force switch. The
> > brute force switch is suboptimal as it does not change the interrupt
> > handler -> tasklet/softirq interaction problems, but was the only way
> > which was possible for the limited man power of the preempt-rt
> > developers."
> > 
> > 
> > So the idea is that we want people to re-think and change their
> > interrupt routines to take advantage of the benefits of threaded
> > interrupts and avoid the endless context switching between threaded
> > interrupts handler, softirq/tasklet/workqueue contexts, which preempt-rt
> > now has.
> > 
> > The only way to avoid that is by pushing all softirq/tasklet/worklet
> > code into the threaded handler, and the only way to do that is by
> > reworking the drivers.
> > 
> > Since there are too many drivers to count on our few hands, we need an
> > opt-in, so that we can gradually migrate towards this scheme.
> 
> OK, fair enough, good decision.
> 
> What is the plan (if any) for integrating threaded interrupt handlers
> with softirqs?  I guess things will "just work" at present, and
> threaded softirqs happen in a later patch?

Thing is, stuff that now needs softirq could be directly done in the
threaded handler, ergo softirq usage should decline (and hopefully
disappear all together - famous last words).

We only use softirq/workqueues/tasklets because we need a preemptible
environment, which the traditional irq handler didn't provide. With
threaded interrupts we do have that.

> I'd have thought that the softirq latency could get quite a bit worse
> with this proposed threaded-irq patch?

Due to the propagation of wakeups? irq wakes up thread, thread wakes up
softirq, etc?

Yes it would, another good reason to simply use the threaded handler to
do whatever the softirq used to do, no?

> I suppose we could just run the softirq handlers directly after running
> the irq handler, from the IRQ thread.  Rather than having to poke
> softirqd all the time?

One could I suppose, except that softirqs are per-cpu and threaded
interrupts are not, so they don't map that well. We played around with
this on preempt-rt for a while, but it kept on breaking stuff.

Its all a lot more tracktable when you get to change the driver, as you
have more information.

> Thwap me if this was all in whatever-changelog-that-was as well ;)

Hehe, no you got some good points this time around ;-)

> Also...
> 
> Given that this threaded-irq code appears to be new and not very tested
> in -rt, I think it would be a good idea to convert some popular drivers
> (e1000/e1000e) so that the core code gets a good thrashing before we
> merge it.

Right, Thomas did the EHCI usb driver, the network driver you propose is
a tad hard since it relies on the whole network stack softirq stuff.
Re-working the whole net-stack to make use of threaded handlers is a
massive undertaking -- although I seem to remember someone doing it a
few years back and seeing a general performance improvement, Thomas
still got a link to that work?

But yes, it would be prudent to convert a few frequently used driver to
this model before merging I suppose.

Re: [patch 4/4] genirq: add support for threaded interrupt handlers

[Andrew Morton]

On Fri, 27 Feb 2009 09:05:10 +0100 Peter Zijlstra <peterz@infradead.org> wrote:

> > What is the plan (if any) for integrating threaded interrupt handlers
> > with softirqs?  I guess things will "just work" at present, and
> > threaded softirqs happen in a later patch?
> 
> Thing is, stuff that now needs softirq could be directly done in the
> threaded handler, ergo softirq usage should decline (and hopefully
> disappear all together - famous last words).
> 
> We only use softirq/workqueues/tasklets because we need a preemptible
> environment, which the traditional irq handler didn't provide. With
> threaded interrupts we do have that.

ah.  I was specifically thinking of net/core/dev.c.  That's our
heaviest user of interrupts and softirqs, I expect?

> > I'd have thought that the softirq latency could get quite a bit worse
> > with this proposed threaded-irq patch?
> 
> Due to the propagation of wakeups? irq wakes up thread, thread wakes up
> softirq, etc?
> 
> Yes it would, another good reason to simply use the threaded handler to
> do whatever the softirq used to do, no?
> 
> > I suppose we could just run the softirq handlers directly after running
> > the irq handler, from the IRQ thread.  Rather than having to poke
> > softirqd all the time?
> 
> One could I suppose, except that softirqs are per-cpu and threaded
> interrupts are not, so they don't map that well. We played around with
> this on preempt-rt for a while, but it kept on breaking stuff.
> 
> Its all a lot more tracktable when you get to change the driver, as you
> have more information.
> 
> > Thwap me if this was all in whatever-changelog-that-was as well ;)
> 
> Hehe, no you got some good points this time around ;-)
> 
> > Also...
> > 
> > Given that this threaded-irq code appears to be new and not very tested
> > in -rt, I think it would be a good idea to convert some popular drivers
> > (e1000/e1000e) so that the core code gets a good thrashing before we
> > merge it.
> 
> Right, Thomas did the EHCI usb driver, the network driver you propose is
> a tad hard since it relies on the whole network stack softirq stuff.
> Re-working the whole net-stack to make use of threaded handlers is a
> massive undertaking

oh.  That rather changes the perspective on the whole exercise.  hrm.

> -- although I seem to remember someone doing it a
> few years back and seeing a general performance improvement, Thomas
> still got a link to that work?
> 
> But yes, it would be prudent to convert a few frequently used driver to
> this model before merging I suppose.

Re: [patch 4/4] genirq: add support for threaded interrupt handlers

[Arjan van de Ven]

On Fri, 27 Feb 2009 09:05:10 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> 
> Right, Thomas did the EHCI usb driver, the network driver you propose
> is a tad hard since it relies on the whole network stack softirq
> stuff. Re-working the whole net-stack to make use of threaded
> handlers is a massive undertaking -- although I seem to remember
> someone doing it a few years back and seeing a general performance
> improvement, Thomas still got a link to that work?


we shouldn't have to; just running the softirq handler from the irq
thread should work, and not lose real performance.
Sure you're not going to get a performance gain that you could get
but it still works.
or am I missing something?

(and esp for things like wireless drivers, that should be just fine; I
can understand hesitation to do 10g ethernet drivers right away ;-)


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch 4/4] genirq: add support for threaded interrupt handlers

[Steven Rostedt]

On Fri, 27 Feb 2009, Arjan van de Ven wrote:

> On Fri, 27 Feb 2009 09:05:10 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > 
> > Right, Thomas did the EHCI usb driver, the network driver you propose
> > is a tad hard since it relies on the whole network stack softirq
> > stuff. Re-working the whole net-stack to make use of threaded
> > handlers is a massive undertaking -- although I seem to remember
> > someone doing it a few years back and seeing a general performance
> > improvement, Thomas still got a link to that work?
> 
> 
> we shouldn't have to; just running the softirq handler from the irq
> thread should work, and not lose real performance.
> Sure you're not going to get a performance gain that you could get
> but it still works.
> or am I missing something?

The way softirqs are currently designed, you can not run them from
the irq thread. We discovered this (the hard way) in the preempt-rt 
kernel. The issue is that each softirq (threaded in preempt-rt, with a 
thread for every CPU. ie. net-rx/0 net-rx/1 net-rx/2 net-rx/3 for a 4 CPU 
box) is bound to a specific CPU. The design is to modify per cpu data. If 
a IRQ thread were to run the softirq code and migrate to another CPU, you 
will have very hard to debug crashes on your hands.

We also tried to bind the IRQ thread to the CPU if it were to run a 
softirq. But that too had issues. If the IRQ thread was bound to a CPU and 
a higher priority process preempted it, that IRQ handler could not migrate 
to another CPU to finish the work. Now the IRQ handler would need to wait 
for that high priority process to give up the CPU in order to continue.

The solution is to redesign the softirq code to handle migration.

-- Steve



> 
> (and esp for things like wireless drivers, that should be just fine; I
> can understand hesitation to do 10g ethernet drivers right away ;-)
> 
> 
> -- 
> Arjan van de Ven 	Intel Open Source Technology Centre
> For development, discussion and tips for power savings, 
> visit http://www.lesswatts.org
> 

Re: [patch 4/4] genirq: add support for threaded interrupt handlers

[Stefan Richter]

Steven Rostedt wrote:
> The way softirqs are currently designed, you can not run them from
> the irq thread. We discovered this (the hard way) in the preempt-rt 
> kernel. The issue is that each softirq (threaded in preempt-rt, with a 
> thread for every CPU. ie. net-rx/0 net-rx/1 net-rx/2 net-rx/3 for a 4 CPU 
> box) is bound to a specific CPU. The design is to modify per cpu data. If 
> a IRQ thread were to run the softirq code and migrate to another CPU, you 
> will have very hard to debug crashes on your hands.

I presume that a lot of tasklet users rely on the fact that tasklets are
executed on the CPU which scheduled them.  (E.g. the firewire stack's
low level currently does.)

> We also tried to bind the IRQ thread to the CPU if it were to run a 
> softirq. But that too had issues. If the IRQ thread was bound to a CPU and 
> a higher priority process preempted it, that IRQ handler could not migrate 
> to another CPU to finish the work. Now the IRQ handler would need to wait 
> for that high priority process to give up the CPU in order to continue.
> 
> The solution is to redesign the softirq code to handle migration.

Do you mean "redesign the softirq framework" or "redesign the softirq
framework users"?

(In the particular case of firewire, the latter should be just fine.)
-- 
Stefan Richter
-=====-==--= --=- ===--
http://arcgraph.de/sr/

Re: [patch 4/4] genirq: add support for threaded interrupt handlers

[Peter Zijlstra]

On Sat, 2009-02-28 at 14:46 +0100, Stefan Richter wrote:
> > We also tried to bind the IRQ thread to the CPU if it were to run a 
> > softirq. But that too had issues. If the IRQ thread was bound to a CPU and 
> > a higher priority process preempted it, that IRQ handler could not migrate 
> > to another CPU to finish the work. Now the IRQ handler would need to wait 
> > for that high priority process to give up the CPU in order to continue.
> > 
> > The solution is to redesign the softirq code to handle migration.
> 
> Do you mean "redesign the softirq framework" or "redesign the softirq
> framework users"?
> 
> (In the particular case of firewire, the latter should be just fine.)

The latter.

Re: [patch 4/4] genirq: add support for threaded interrupt handlers

[Andi Kleen]

Andrew Morton <akpm@linux-foundation.org> writes:
>
> Given that this threaded-irq code appears to be new and not very tested
> in -rt, I think it would be a good idea to convert some popular drivers
> (e1000/e1000e) so that the core code gets a good thrashing before we
> merge it.

It's questionable it's a good idea for these kind of network drivers,
because the majority of work should be done in NAPI context anyways.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [patch 4/4] genirq: add support for threaded interrupt handlers

[Thomas Gleixner]

On Thu, 26 Feb 2009, Andrew Morton wrote:
> > The threaded handler allows to integrate hardware related
> > functionality and softirq/tasklet functions in the handler
> > thread.
> > 
> 
> A typical device driver will do:
> 
> some_function(...)
> {
> 	spin_lock_irqsave(&dev->lock);
> }
> 
> irq_handler(...)
> {
> 	spin_lock(&dev->lock);
> }
> 
> and this does not deadlock, because the driver "knows" that the IRQ is
> disabled during the execution of the IRQ handler.
> 
> But how is this optimisation supported with IRQ threads?  Do we leave
> the IRQ disabled during the thread execution?  Or does the driver need
> to be changed?
> 
> Bearing in mind that the driver might choose to split the IRQ handling
> between hard-irq context fastpath and process-context slowpath (I
> hope), and that each path might want to take the same lock.

The hard-irq fastpath still can do spin_lock(), but the process
context slowpath needs to disable irqs when it locks something which
is shared with the fast path handler.
 
> >
> > ...
> >
> > @@ -96,6 +122,13 @@ extern int __must_check devm_request_irq
> >  			    const char *devname, void *dev_id);
> >  extern void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id);
> >  
> > +static inline int irq_thread_should_run(struct irqaction *action)
> > +{
> > +	return test_and_clear_bit(IRQTF_RUNTHREAD, &action->flags);
> > +}
> 
> I don't think this needs to be placed in a header file?

This is not only for the generic irq code, also threaded handlers need
it to figure out whether there is further work to do.
 
> >  
> > +	/* IRQ handler threads */
> > +	struct irqaction *irqaction;
> > +
> 
> If this field always used?  Perhaps CONFIG_GENERIC_HARDIRQS=n kernels
> could save some space?

Forgot, that CONFIG_GENERIC_HARDIRQS=n archs still exist :)
 
> 
> >  	/* Protection of the PI data structures: */
> >  	spinlock_t pi_lock;
> >  
> > Index: linux-2.6-tip/kernel/exit.c
> > ===================================================================
> > --- linux-2.6-tip.orig/kernel/exit.c
> > +++ linux-2.6-tip/kernel/exit.c
> > @@ -1040,6 +1040,8 @@ NORET_TYPE void do_exit(long code)
> >  		schedule();
> >  	}
> >  
> > +	exit_irq_thread(tsk);
> > +
> 
> Did we just break CONFIG_GENERIC_HARDIRQS=n builds?

Yup, will fix.

> >  	exit_signals(tsk);  /* sets PF_EXITING */
> >  	/*
> >  	 * tsk->flags are checked in the futex code to protect against
> > Index: linux-2.6-tip/kernel/irq/handle.c
> > ===================================================================
> > --- linux-2.6-tip.orig/kernel/irq/handle.c
> > +++ linux-2.6-tip/kernel/irq/handle.c
> > @@ -360,13 +360,46 @@ irqreturn_t handle_IRQ_event(unsigned in
> >  			ret = IRQ_NEEDS_HANDLING;
> >  
> >  		switch (ret) {
> > -		default:
> > +		case IRQ_NEEDS_HANDLING:
> > +			if (!(action->flags & IRQF_THREADED)) {
> > +				ret = action->handler(irq, action->dev_id);
> > +				if (ret == IRQ_HANDLED)
> > +					status |= action->flags;
> > +				break;
> > +			}
> > +			/*
> > +			 * Warn once when a quick check handler asked
> 
> s/quick check/quickcheck/
> 
> > +			 * for invoking the threaded (main) handler
> > +			 * directly
> > +			 */
> 
> It would be nice if the comment were to explain why this is considered
> bad.

Fair enough.
 
> > +			 * disabled the device interrupt, so no irq
> > +			 * storm is lurking.
> > +			 */
> 
> The quickcheck handler has disabled the device interrupt?  Where did
> this factoid come from?  What linkage is there between the crashing of
> a kernel thread and a quickcheck handler knowing to disable the
> interrupt source?  How is a driver writer to know that his quickcheck
> handler is to disable the interrupt, and under what circustances?
> 
> etc.  Head is spinning a bit.

If we want the slow path handler run threaded then we need to make
sure in the quickcheck handler that interrupts are disabled on the
device level. Otherwise we could reeenter the hardirq
immediately. I'll whip up some docu for that.
 
> > +	while (action) {
> > +		if (action->thread)
> > +			set_cpus_allowed_ptr(action->thread, cpumask);
> > +		action = action->next;
> > +	}
> > +}
> 
> Is all this code cpu-hotplug-friendly?

It should and I did not see any problems when I offlined/onlined CPUs
during testing.
 
> > +	printk(KERN_ERR
> > +	       "exiting task \"%s\" (%d) is an active IRQ thread (irq %d)\n",
> > +	       tsk->comm ? tsk->comm : "", tsk->pid, tsk->irqaction->irq);
> 
> task_struct.comm is an array, and testing it for NULL makes no sense.
> 
> The `tsk' argument is a holdover from the old days when accessing
> `current' was considered to be slow.  It would be better if this
> fuction were to take no argumetns and were to operate on `current'.

tsk should always be current. I just followed the other exit_xxx() ones.
 
> If we _really_ want this function to be able to operate on tasks other
> than `current' then we have a whole buncha races to fix first.
> 
> > +	/*
> > +	 * Set the THREAD DIED flag to prevent further wakeups of the
> > +	 * soon to be gone threaded handler.
> > +	 */
> > +	set_bit(IRQTF_DIED, &tsk->irqaction->flags);
> > +	tsk->irqaction = NULL;
> 
> Why do we need to do both?  It would be clearer if irq_thread() were to
> test IRQTF_DIED.

irq_thread does not test IRQTF_DIED, irq_thread is the one which
died. tsk->irqaction does not need to be cleared.
 
> > +/*
> >   * Internal function to register an irqaction - typically used to
> >   * allocate special interrupts that are part of the architecture.
> >   */
> > @@ -402,6 +477,11 @@ __setup_irq(unsigned int irq, struct irq
> >  
> >  	if (desc->chip == &no_irq_chip)
> >  		return -ENOSYS;
> > +
> > +	/* Threaded interrupts need a quickcheck handler */
> > +	if ((new->flags & IRQF_THREADED) && !new->quick_check_handler)
> > +		return -EINVAL;
> 
> Seems redundant.  (new->flags & IRQF_THREADED) and
> !!new->quick_check_handler both contain the same information?

The idea was to allow driver developers a two stage approach for threaded:

1) split out the quickcheck handler and make it work, while still
calling the real handler right after that from hardirq context

2) make the real handler threaded
 
> >  	/*
> >  	 * Some drivers like serial.c use request_irq() heavily,
> >  	 * so we have to be careful not to interfere with a
> > @@ -420,6 +500,26 @@ __setup_irq(unsigned int irq, struct irq
> >  	}
> >  
> >  	/*
> > +	 * Threaded handler ?
> > +	 */
> > +	if (new->flags & IRQF_THREADED) {
> > +		struct task_struct *t;
> > +
> > +		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
> > +				   new->name);
> 
> Seems there's a big risk of overrunning task_struct.comm[] here.

good point.
 
> > +		if (IS_ERR(t))
> > +			return PTR_ERR(t);
> > +		/*
> > +		 * We keep the reference to the task struct even if
> > +		 * the thread dies to avoid that the interrupt code
> > +		 * references an already gone task_struct.
> 
> Sentence needs help.  "to prevent the interrupt code from referencing a
> freed task_struct"?

I know that my english sucks.

Thanks,

	tglx

Re: [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2

[Jon Masters]

On Thu, 2009-02-26 at 13:28 +0000, Thomas Gleixner wrote:
> This patch series implements support for threaded irq handlers for the
> generic IRQ layer.

> The code was tested with a converted USB EHCI driver. The EHCI shares
> an interrupt line with another device and both the threaded and the
> non threaded handlers coexist nicely.

Cool. Thanks Thomas. The previous patchset works here aside from some
issues with Sven's USB patch - I'll pull this new set in and see which
drivers I can help convert over. Are you pulling this into the RT tree
already?

Jon.

Re: [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2

[Sven-Thorsten Dietrich]

On Thu, 2009-02-26 at 10:26 -0500, Jon Masters wrote:
> On Thu, 2009-02-26 at 13:28 +0000, Thomas Gleixner wrote:
> > This patch series implements support for threaded irq handlers for the
> > generic IRQ layer.
> 
> > The code was tested with a converted USB EHCI driver. The EHCI shares
> > an interrupt line with another device and both the threaded and the
> > non threaded handlers coexist nicely.
> 
> Cool. Thanks Thomas. The previous patchset works here aside from some
> issues with Sven's USB patch - I'll pull this new set in and see which
> drivers I can help convert over. Are you pulling this into the RT tree
> already?

Hi Jon,


Were you still using the USB threading version from October? 

There was at least a missing spin_unlock there at one point. We fixed
that at LCA.

I do take bug reports directly, btw. and might even see them if you can
just mail them to LKML.

But "some issues" is rather vague.

Thanks

Sven

> 
> Jon.
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2

[Christoph Hellwig]

On Thu, Feb 26, 2009 at 01:28:02PM -0000, Thomas Gleixner wrote:
> This patch series implements support for threaded irq handlers for the
> generic IRQ layer.

What tree is this again?  Patch number two fails to apply again
mainline.

Re: [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2

[Thomas Gleixner]

On Sat, 28 Feb 2009, Christoph Hellwig wrote:

> On Thu, Feb 26, 2009 at 01:28:02PM -0000, Thomas Gleixner wrote:
> > This patch series implements support for threaded irq handlers for the
> > generic IRQ layer.
> 
> What tree is this again?  Patch number two fails to apply again
> mainline.

http://tglx.de/~tglx/patches.tar.bz2

is against mainline.

   tglx
 

[ANNOUNCE] USB genirq infrastructure for threaded interrupt handlers V2

[Sven-Thorsten Dietrich]

On Thu, 2009-02-26 at 13:28 +0000, Thomas Gleixner wrote:
> This patch series implements support for threaded irq handlers for the
> generic IRQ layer.
> 
> Changes vs. V1:
> 	- review comments addressed
> 	- irq affinity setting for handler threads
> 

I have forward-ported Thomas's patch set to 2.6.29-rc7, and added the
USB irq implementation for ohci, ehci and uhci.

A tar ball of the entire series is here:

http://www.thebigcorporation.com/Sven/genirq-usb/genirq-usb-2.6.29-rc7-v0.1.tar.bz2


After spending quite a bit of time looking at this implementation (and
at RT's):

The primary difference between the RT IRQ threading implementation, and
this one, is that the former implements mandatory per-LINE IRQ
threading, while the latter implements opt-in per-DEVICE IRQ threading.

The RFC "per-DEVICE" design can side-step the boot-IRQ quirks issue that
has plagued RT. But the opt-in design requires, that EVERY driver that
is to support IRQ threading must be modified.

Currently per-DEVICE design cannot cleanly support switching IRQs in and
out of thread context.

Last but not least, not all devices would be able to support IRQ
threading in this design. (RT has effectively no fall-out - practically
every piece of hardware works in that implementation)

The per-LINE design in RT brings with it a host of problems and
performance compromises, see the boot IRQ quirks work merged into
2.6.28. 

But the IRQ quirks issue can't be addressed for all chipsets, and
continues to be a problem on some hardware. This implementation would
also not completely eliminate that problem, depending on the device
palette presented and their IRQ arrangement / sharing.

The advantage of the RT per-line design, is that drivers modifications
are minimal, and just about every driver and every architecture works
with the existing implementation.

But per-line performance is abysmal under high IRQ loads when IRQs are
shared, and this has potential to boost throughput for RT. In some
cases, (multi-core) it might even boost throughput for the other PREEMPT
configurations. TBd.


The ideal solution IMO, would be one where you can switch IRQs in and
out of thread context. 

This would increase the tuning and debugging options, and not compromise
throughput for those who don't care for what threading can provide.

SO with those general comments, following is the punch list against my
USB implementation:

1. IRQ threading will impact performance, and simply forcing it on, as
in the USB case, may not be an ideal solution. This design does not
allow the ability to turn on and off IRQ threading as the RT per-line
IRQ implementation does. 

I'll run some benchmarks with what hardware I have here tomorrow.

2. Status tracking. The contents of the status register should be stored
in the IRQ (quickcheck handler, and used in the threaded portion). I
have added some debugging, and I can see the USB status registers
changing between running the quickcheck handler in IRQ context, and
executing the thread. 

This would obviously happen if a USB device is plugged in, between IRQ
and thread execution, for example. But any USB activity causes status
register changes.

USB drivers seem to be graceful enough to "catch up" on the changes.

Other hardware may not be as forgiving. What do you do - queue up a list
of status register snapshots, or just process the device as-is, when the
thread runs? What events may be missed? This is probably hardware
dependent, and possibly further driver-code mods may be needed to
accommodate case-by-case.

3. Locking. USB has 3 primary implementations. Each use a different
degree of locking strictness, and this implementation requires adding
locks to be compatible with any possible RT implementation (because in
RT, the IRQ-context portion locks must be raw locks, while the thread
side locks must be mutexes to avoid all sorts of latency and
scheduling-while-atomic fiascos).

In general, there has to be at least some locking around the caching of
status registers, so that the quickcheck does not stomp of status
register bits being written by a yet incomplete thread-level execution.

I.e. the assumption is, that device level IRQs are not re-enabled until
the end of thread execution. This is probably not consistent with a lot
of driver implementations, which may assume that IRQ-context execution
is non-reentrant. 

This is the very terse version, just to put down some of my thoughts.


Overall, I would be curious about feedback on in implementation of this
on top of the RT tree. 

IMO, a hybrid solution, where per-device as well as per-line IRQ
threading, and NO IRQ threading, are all possible to co-exist, would be
ideal both for variants.

Regards,

Sven