LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] genirq: Handle force threading of irqs with primary and thread handler
@ 2015-09-21  9:01 Thomas Gleixner
  2015-09-22  6:42 ` Kohji Okuno
  2015-09-22 10:42 ` [tip:irq/core] " tip-bot for Thomas Gleixner
  0 siblings, 2 replies; 6+ messages in thread
From: Thomas Gleixner @ 2015-09-21  9:01 UTC (permalink / raw)
  To: LKML
  Cc: Kohji Okuno, Michal Smucr, Nathan Sullivan, Sebastian Andrzej Siewior


[-- Attachment #1: Type: TEXT/PLAIN, Size: 10140 bytes --]

Force threading of interrupts does not really deal with interrupts
which are requested with a primary and a threaded handler. The current
policy is to leave them alone and let the primary handler run in
interrupt context, but we set the ONESHOT flag for those interrupts as
well.

Kohji Okuno debugged a problem with the SDHCI driver where the
interrupt thread waits for a hardware interrupt to trigger, which can't
work well because the hardware interrupt is masked due to the ONESHOT
flag being set. He proposed to set the ONESHOT flag only if the
interrupt does not provide a thread handler.

Though that does not work either because these interrupts can be
shared. So the other interrupt would rightfully get the ONESHOT flag
set and therefor the same situation would happen again.

To deal with this proper, we need to force thread the primary handler
of such interrupts as well. That means that the primary interrupt
handler is treated as any other primary interrupt handler which is not
marked IRQF_NO_THREAD. The threaded handler becomes a separate thread
so the SDHCI flow logic can be handled gracefully.

The same issue was reported against 4.1-rt.

Reported-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
Reported-By: Michal Šmucr <msmucr@gmail.com>
Reported-by: Nathan Sullivan <nathan.sullivan@ni.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/interrupt.h |    2 
 kernel/irq/manage.c       |  160 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 120 insertions(+), 42 deletions(-)

Index: linux/include/linux/interrupt.h
===================================================================
--- linux.orig/include/linux/interrupt.h
+++ linux/include/linux/interrupt.h
@@ -102,6 +102,7 @@ typedef irqreturn_t (*irq_handler_t)(int
  * @flags:	flags (see IRQF_* above)
  * @thread_fn:	interrupt handler function for threaded interrupts
  * @thread:	thread pointer for threaded interrupts
+ * @secondary:	pointer to secondary irqaction (force threading)
  * @thread_flags:	flags related to @thread
  * @thread_mask:	bitmask for keeping track of @thread activity
  * @dir:	pointer to the proc/irq/NN/name entry
@@ -113,6 +114,7 @@ struct irqaction {
 	struct irqaction	*next;
 	irq_handler_t		thread_fn;
 	struct task_struct	*thread;
+	struct irqaction	*secondary;
 	unsigned int		irq;
 	unsigned int		flags;
 	unsigned long		thread_flags;
Index: linux/kernel/irq/manage.c
===================================================================
--- linux.orig/kernel/irq/manage.c
+++ linux/kernel/irq/manage.c
@@ -730,6 +730,12 @@ static irqreturn_t irq_nested_primary_ha
 	return IRQ_NONE;
 }
 
+static irqreturn_t irq_forced_secondary_handler(int irq, void *dev_id)
+{
+	WARN(1, "Secondary action handler called for irq %d\n", irq);
+	return IRQ_NONE;
+}
+
 static int irq_wait_for_interrupt(struct irqaction *action)
 {
 	set_current_state(TASK_INTERRUPTIBLE);
@@ -756,7 +762,8 @@ static int irq_wait_for_interrupt(struct
 static void irq_finalize_oneshot(struct irq_desc *desc,
 				 struct irqaction *action)
 {
-	if (!(desc->istate & IRQS_ONESHOT))
+	if (!(desc->istate & IRQS_ONESHOT) ||
+	    action->handler == irq_forced_secondary_handler)
 		return;
 again:
 	chip_bus_lock(desc);
@@ -910,6 +917,18 @@ static void irq_thread_dtor(struct callb
 	irq_finalize_oneshot(desc, action);
 }
 
+static void irq_wake_secondary(struct irq_desc *desc, struct irqaction *action)
+{
+	struct irqaction *secondary = action->secondary;
+
+	if (WARN_ON_ONCE(!secondary))
+		return;
+
+	raw_spin_lock_irq(&desc->lock);
+	__irq_wake_thread(desc, secondary);
+	raw_spin_unlock_irq(&desc->lock);
+}
+
 /*
  * Interrupt handler thread
  */
@@ -940,6 +959,8 @@ static int irq_thread(void *data)
 		action_ret = handler_fn(desc, action);
 		if (action_ret == IRQ_HANDLED)
 			atomic_inc(&desc->threads_handled);
+		if (action_ret == IRQ_WAKE_THREAD)
+			irq_wake_secondary(desc, action);
 
 		wake_threads_waitq(desc);
 	}
@@ -984,20 +1005,36 @@ void irq_wake_thread(unsigned int irq, v
 }
 EXPORT_SYMBOL_GPL(irq_wake_thread);
 
-static void irq_setup_forced_threading(struct irqaction *new)
+static int irq_setup_forced_threading(struct irqaction *new)
 {
 	if (!force_irqthreads)
-		return;
+		return 0;
 	if (new->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT))
-		return;
+		return 0;
 
 	new->flags |= IRQF_ONESHOT;
 
-	if (!new->thread_fn) {
-		set_bit(IRQTF_FORCED_THREAD, &new->thread_flags);
-		new->thread_fn = new->handler;
-		new->handler = irq_default_primary_handler;
-	}
+	/*
+	 * Handle the case where we have a real primary handler and a
+	 * thread handler. We force thread them as well by creating a
+	 * secondary action.
+	 */
+	if (new->handler != irq_default_primary_handler && new->thread_fn) {
+		/* Allocate the secondary action */
+		new->secondary = kzalloc(sizeof(struct irqaction), GFP_KERNEL);
+		if (!new->secondary)
+			return -ENOMEM;
+		new->secondary->handler = irq_forced_secondary_handler;
+		new->secondary->thread_fn = new->thread_fn;
+		new->secondary->dev_id = new->dev_id;
+		new->secondary->irq = new->irq;
+		new->secondary->name = new->name;
+	}
+	/* Deal with the primary handler */
+	set_bit(IRQTF_FORCED_THREAD, &new->thread_flags);
+	new->thread_fn = new->handler;
+	new->handler = irq_default_primary_handler;
+	return 0;
 }
 
 static int irq_request_resources(struct irq_desc *desc)
@@ -1017,6 +1054,48 @@ static void irq_release_resources(struct
 		c->irq_release_resources(d);
 }
 
+static int
+setup_irq_thread(struct irqaction *new, unsigned int irq, bool secondary)
+{
+	struct task_struct *t;
+	struct sched_param param = {
+		.sched_priority = MAX_USER_RT_PRIO/2,
+	};
+
+	if (!secondary) {
+		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
+				   new->name);
+	} else {
+		t = kthread_create(irq_thread, new, "irq/%d-s-%s", irq,
+				   new->name);
+		param.sched_priority -= 1;
+	}
+
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+
+	sched_setscheduler_nocheck(t, SCHED_FIFO, &param);
+
+	/*
+	 * We keep the reference to the task struct even if
+	 * the thread dies to avoid that the interrupt code
+	 * references an already freed task_struct.
+	 */
+	get_task_struct(t);
+	new->thread = t;
+	/*
+	 * Tell the thread to set its affinity. This is
+	 * important for shared interrupt handlers as we do
+	 * not invoke setup_affinity() for the secondary
+	 * handlers as everything is already set up. Even for
+	 * interrupts marked with IRQF_NO_BALANCE this is
+	 * correct as we want the thread to move to the cpu(s)
+	 * on which the requesting code placed the interrupt.
+	 */
+	set_bit(IRQTF_AFFINITY, &new->thread_flags);
+	return 0;
+}
+
 /*
  * Internal function to register an irqaction - typically used to
  * allocate special interrupts that are part of the architecture.
@@ -1037,6 +1116,8 @@ __setup_irq(unsigned int irq, struct irq
 	if (!try_module_get(desc->owner))
 		return -ENODEV;
 
+	new->irq = irq;
+
 	/*
 	 * Check whether the interrupt nests into another interrupt
 	 * thread.
@@ -1054,8 +1135,11 @@ __setup_irq(unsigned int irq, struct irq
 		 */
 		new->handler = irq_nested_primary_handler;
 	} else {
-		if (irq_settings_can_thread(desc))
-			irq_setup_forced_threading(new);
+		if (irq_settings_can_thread(desc)) {
+			ret = irq_setup_forced_threading(new);
+			if (ret)
+				goto out_mput;
+		}
 	}
 
 	/*
@@ -1064,37 +1148,14 @@ __setup_irq(unsigned int irq, struct irq
 	 * thread.
 	 */
 	if (new->thread_fn && !nested) {
-		struct task_struct *t;
-		static const struct sched_param param = {
-			.sched_priority = MAX_USER_RT_PRIO/2,
-		};
-
-		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
-				   new->name);
-		if (IS_ERR(t)) {
-			ret = PTR_ERR(t);
+		ret = setup_irq_thread(new, irq, false);
+		if (ret)
 			goto out_mput;
+		if (new->secondary) {
+			ret = setup_irq_thread(new->secondary, irq, true);
+			if (ret)
+				goto out_thread;
 		}
-
-		sched_setscheduler_nocheck(t, SCHED_FIFO, &param);
-
-		/*
-		 * We keep the reference to the task struct even if
-		 * the thread dies to avoid that the interrupt code
-		 * references an already freed task_struct.
-		 */
-		get_task_struct(t);
-		new->thread = t;
-		/*
-		 * Tell the thread to set its affinity. This is
-		 * important for shared interrupt handlers as we do
-		 * not invoke setup_affinity() for the secondary
-		 * handlers as everything is already set up. Even for
-		 * interrupts marked with IRQF_NO_BALANCE this is
-		 * correct as we want the thread to move to the cpu(s)
-		 * on which the requesting code placed the interrupt.
-		 */
-		set_bit(IRQTF_AFFINITY, &new->thread_flags);
 	}
 
 	if (!alloc_cpumask_var(&mask, GFP_KERNEL)) {
@@ -1267,7 +1328,6 @@ __setup_irq(unsigned int irq, struct irq
 				   irq, nmsk, omsk);
 	}
 
-	new->irq = irq;
 	*old_ptr = new;
 
 	irq_pm_install_action(desc, new);
@@ -1293,6 +1353,8 @@ __setup_irq(unsigned int irq, struct irq
 	 */
 	if (new->thread)
 		wake_up_process(new->thread);
+	if (new->secondary)
+		wake_up_process(new->secondary->thread);
 
 	register_irq_proc(irq, desc);
 	new->dir = NULL;
@@ -1323,6 +1385,13 @@ out_thread:
 		kthread_stop(t);
 		put_task_struct(t);
 	}
+	if (new->secondary && new->secondary->thread) {
+		struct task_struct *t = new->secondary->thread;
+
+		new->secondary->thread = NULL;
+		kthread_stop(t);
+		put_task_struct(t);
+	}
 out_mput:
 	module_put(desc->owner);
 	return ret;
@@ -1430,9 +1499,14 @@ static struct irqaction *__free_irq(unsi
 	if (action->thread) {
 		kthread_stop(action->thread);
 		put_task_struct(action->thread);
+		if (action->secondary && action->secondary->thread) {
+			kthread_stop(action->secondary->thread);
+			put_task_struct(action->secondary->thread);
+		}
 	}
 
 	module_put(desc->owner);
+	kfree(action->secondary);
 	return action;
 }
 
@@ -1576,8 +1650,10 @@ int request_threaded_irq(unsigned int ir
 	retval = __setup_irq(irq, desc, action);
 	chip_bus_sync_unlock(desc);
 
-	if (retval)
+	if (retval) {
+		kfree(action->secondary);
 		kfree(action);
+	}
 
 #ifdef CONFIG_DEBUG_SHIRQ_FIXME
 	if (!retval && (irqflags & IRQF_SHARED)) {

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] genirq: Handle force threading of irqs with primary and thread handler
  2015-09-21  9:01 [PATCH] genirq: Handle force threading of irqs with primary and thread handler Thomas Gleixner
@ 2015-09-22  6:42 ` Kohji Okuno
  2015-09-22 10:42 ` [tip:irq/core] " tip-bot for Thomas Gleixner
  1 sibling, 0 replies; 6+ messages in thread
From: Kohji Okuno @ 2015-09-22  6:42 UTC (permalink / raw)
  To: tglx; +Cc: linux-kernel, msmucr, nathan.sullivan, bigeasy, okuno.kohji

From: Thomas Gleixner <tglx@linutronix.de>
Date: Mon, 21 Sep 2015 11:01:10 +0200
> To deal with this proper, we need to force thread the primary handler
> of such interrupts as well. That means that the primary interrupt
> handler is treated as any other primary interrupt handler which is not
> marked IRQF_NO_THREAD. The threaded handler becomes a separate thread
> so the SDHCI flow logic can be handled gracefully.
> 
> The same issue was reported against 4.1-rt.

Hi tglx,
I tried your patch. It worked good for me.

Many thanks,
 Kohji Okuno

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [tip:irq/core] genirq: Handle force threading of irqs with primary and thread handler
  2015-09-21  9:01 [PATCH] genirq: Handle force threading of irqs with primary and thread handler Thomas Gleixner
  2015-09-22  6:42 ` Kohji Okuno
@ 2015-09-22 10:42 ` tip-bot for Thomas Gleixner
  2015-10-06 18:59   ` Felipe Balbi
  1 sibling, 1 reply; 6+ messages in thread
From: tip-bot for Thomas Gleixner @ 2015-09-22 10:42 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: hpa, okuno.kohji, bigeasy, linux-kernel, msmucr, nathan.sullivan,
	mingo, tglx

Commit-ID:  2a1d3ab8986d1b2f598ffc42351d94166fa0f022
Gitweb:     http://git.kernel.org/tip/2a1d3ab8986d1b2f598ffc42351d94166fa0f022
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Mon, 21 Sep 2015 11:01:10 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 22 Sep 2015 12:39:57 +0200

genirq: Handle force threading of irqs with primary and thread handler

Force threading of interrupts does not really deal with interrupts
which are requested with a primary and a threaded handler. The current
policy is to leave them alone and let the primary handler run in
interrupt context, but we set the ONESHOT flag for those interrupts as
well.

Kohji Okuno debugged a problem with the SDHCI driver where the
interrupt thread waits for a hardware interrupt to trigger, which can't
work well because the hardware interrupt is masked due to the ONESHOT
flag being set. He proposed to set the ONESHOT flag only if the
interrupt does not provide a thread handler.

Though that does not work either because these interrupts can be
shared. So the other interrupt would rightfully get the ONESHOT flag
set and therefor the same situation would happen again.

To deal with this proper, we need to force thread the primary handler
of such interrupts as well. That means that the primary interrupt
handler is treated as any other primary interrupt handler which is not
marked IRQF_NO_THREAD. The threaded handler becomes a separate thread
so the SDHCI flow logic can be handled gracefully.

The same issue was reported against 4.1-rt.

Reported-and-tested-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
Reported-By: Michal Smucr <msmucr@gmail.com>
Reported-and-tested-by: Nathan Sullivan <nathan.sullivan@ni.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Link: http://lkml.kernel.org/r/alpine.DEB.2.11.1509211058080.5606@nanos
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/interrupt.h |   2 +
 kernel/irq/manage.c       | 158 ++++++++++++++++++++++++++++++++++------------
 2 files changed, 119 insertions(+), 41 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index be7e75c..ad16809 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -102,6 +102,7 @@ typedef irqreturn_t (*irq_handler_t)(int, void *);
  * @flags:	flags (see IRQF_* above)
  * @thread_fn:	interrupt handler function for threaded interrupts
  * @thread:	thread pointer for threaded interrupts
+ * @secondary:	pointer to secondary irqaction (force threading)
  * @thread_flags:	flags related to @thread
  * @thread_mask:	bitmask for keeping track of @thread activity
  * @dir:	pointer to the proc/irq/NN/name entry
@@ -113,6 +114,7 @@ struct irqaction {
 	struct irqaction	*next;
 	irq_handler_t		thread_fn;
 	struct task_struct	*thread;
+	struct irqaction	*secondary;
 	unsigned int		irq;
 	unsigned int		flags;
 	unsigned long		thread_flags;
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f9a59f6..0a63c2b 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -730,6 +730,12 @@ static irqreturn_t irq_nested_primary_handler(int irq, void *dev_id)
 	return IRQ_NONE;
 }
 
+static irqreturn_t irq_forced_secondary_handler(int irq, void *dev_id)
+{
+	WARN(1, "Secondary action handler called for irq %d\n", irq);
+	return IRQ_NONE;
+}
+
 static int irq_wait_for_interrupt(struct irqaction *action)
 {
 	set_current_state(TASK_INTERRUPTIBLE);
@@ -756,7 +762,8 @@ static int irq_wait_for_interrupt(struct irqaction *action)
 static void irq_finalize_oneshot(struct irq_desc *desc,
 				 struct irqaction *action)
 {
-	if (!(desc->istate & IRQS_ONESHOT))
+	if (!(desc->istate & IRQS_ONESHOT) ||
+	    action->handler == irq_forced_secondary_handler)
 		return;
 again:
 	chip_bus_lock(desc);
@@ -910,6 +917,18 @@ static void irq_thread_dtor(struct callback_head *unused)
 	irq_finalize_oneshot(desc, action);
 }
 
+static void irq_wake_secondary(struct irq_desc *desc, struct irqaction *action)
+{
+	struct irqaction *secondary = action->secondary;
+
+	if (WARN_ON_ONCE(!secondary))
+		return;
+
+	raw_spin_lock_irq(&desc->lock);
+	__irq_wake_thread(desc, secondary);
+	raw_spin_unlock_irq(&desc->lock);
+}
+
 /*
  * Interrupt handler thread
  */
@@ -940,6 +959,8 @@ static int irq_thread(void *data)
 		action_ret = handler_fn(desc, action);
 		if (action_ret == IRQ_HANDLED)
 			atomic_inc(&desc->threads_handled);
+		if (action_ret == IRQ_WAKE_THREAD)
+			irq_wake_secondary(desc, action);
 
 		wake_threads_waitq(desc);
 	}
@@ -984,20 +1005,36 @@ void irq_wake_thread(unsigned int irq, void *dev_id)
 }
 EXPORT_SYMBOL_GPL(irq_wake_thread);
 
-static void irq_setup_forced_threading(struct irqaction *new)
+static int irq_setup_forced_threading(struct irqaction *new)
 {
 	if (!force_irqthreads)
-		return;
+		return 0;
 	if (new->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT))
-		return;
+		return 0;
 
 	new->flags |= IRQF_ONESHOT;
 
-	if (!new->thread_fn) {
-		set_bit(IRQTF_FORCED_THREAD, &new->thread_flags);
-		new->thread_fn = new->handler;
-		new->handler = irq_default_primary_handler;
+	/*
+	 * Handle the case where we have a real primary handler and a
+	 * thread handler. We force thread them as well by creating a
+	 * secondary action.
+	 */
+	if (new->handler != irq_default_primary_handler && new->thread_fn) {
+		/* Allocate the secondary action */
+		new->secondary = kzalloc(sizeof(struct irqaction), GFP_KERNEL);
+		if (!new->secondary)
+			return -ENOMEM;
+		new->secondary->handler = irq_forced_secondary_handler;
+		new->secondary->thread_fn = new->thread_fn;
+		new->secondary->dev_id = new->dev_id;
+		new->secondary->irq = new->irq;
+		new->secondary->name = new->name;
 	}
+	/* Deal with the primary handler */
+	set_bit(IRQTF_FORCED_THREAD, &new->thread_flags);
+	new->thread_fn = new->handler;
+	new->handler = irq_default_primary_handler;
+	return 0;
 }
 
 static int irq_request_resources(struct irq_desc *desc)
@@ -1017,6 +1054,48 @@ static void irq_release_resources(struct irq_desc *desc)
 		c->irq_release_resources(d);
 }
 
+static int
+setup_irq_thread(struct irqaction *new, unsigned int irq, bool secondary)
+{
+	struct task_struct *t;
+	struct sched_param param = {
+		.sched_priority = MAX_USER_RT_PRIO/2,
+	};
+
+	if (!secondary) {
+		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
+				   new->name);
+	} else {
+		t = kthread_create(irq_thread, new, "irq/%d-s-%s", irq,
+				   new->name);
+		param.sched_priority -= 1;
+	}
+
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+
+	sched_setscheduler_nocheck(t, SCHED_FIFO, &param);
+
+	/*
+	 * We keep the reference to the task struct even if
+	 * the thread dies to avoid that the interrupt code
+	 * references an already freed task_struct.
+	 */
+	get_task_struct(t);
+	new->thread = t;
+	/*
+	 * Tell the thread to set its affinity. This is
+	 * important for shared interrupt handlers as we do
+	 * not invoke setup_affinity() for the secondary
+	 * handlers as everything is already set up. Even for
+	 * interrupts marked with IRQF_NO_BALANCE this is
+	 * correct as we want the thread to move to the cpu(s)
+	 * on which the requesting code placed the interrupt.
+	 */
+	set_bit(IRQTF_AFFINITY, &new->thread_flags);
+	return 0;
+}
+
 /*
  * Internal function to register an irqaction - typically used to
  * allocate special interrupts that are part of the architecture.
@@ -1037,6 +1116,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	if (!try_module_get(desc->owner))
 		return -ENODEV;
 
+	new->irq = irq;
+
 	/*
 	 * Check whether the interrupt nests into another interrupt
 	 * thread.
@@ -1054,8 +1135,11 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		 */
 		new->handler = irq_nested_primary_handler;
 	} else {
-		if (irq_settings_can_thread(desc))
-			irq_setup_forced_threading(new);
+		if (irq_settings_can_thread(desc)) {
+			ret = irq_setup_forced_threading(new);
+			if (ret)
+				goto out_mput;
+		}
 	}
 
 	/*
@@ -1064,37 +1148,14 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	 * thread.
 	 */
 	if (new->thread_fn && !nested) {
-		struct task_struct *t;
-		static const struct sched_param param = {
-			.sched_priority = MAX_USER_RT_PRIO/2,
-		};
-
-		t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
-				   new->name);
-		if (IS_ERR(t)) {
-			ret = PTR_ERR(t);
+		ret = setup_irq_thread(new, irq, false);
+		if (ret)
 			goto out_mput;
+		if (new->secondary) {
+			ret = setup_irq_thread(new->secondary, irq, true);
+			if (ret)
+				goto out_thread;
 		}
-
-		sched_setscheduler_nocheck(t, SCHED_FIFO, &param);
-
-		/*
-		 * We keep the reference to the task struct even if
-		 * the thread dies to avoid that the interrupt code
-		 * references an already freed task_struct.
-		 */
-		get_task_struct(t);
-		new->thread = t;
-		/*
-		 * Tell the thread to set its affinity. This is
-		 * important for shared interrupt handlers as we do
-		 * not invoke setup_affinity() for the secondary
-		 * handlers as everything is already set up. Even for
-		 * interrupts marked with IRQF_NO_BALANCE this is
-		 * correct as we want the thread to move to the cpu(s)
-		 * on which the requesting code placed the interrupt.
-		 */
-		set_bit(IRQTF_AFFINITY, &new->thread_flags);
 	}
 
 	if (!alloc_cpumask_var(&mask, GFP_KERNEL)) {
@@ -1267,7 +1328,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 				   irq, nmsk, omsk);
 	}
 
-	new->irq = irq;
 	*old_ptr = new;
 
 	irq_pm_install_action(desc, new);
@@ -1293,6 +1353,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	 */
 	if (new->thread)
 		wake_up_process(new->thread);
+	if (new->secondary)
+		wake_up_process(new->secondary->thread);
 
 	register_irq_proc(irq, desc);
 	new->dir = NULL;
@@ -1323,6 +1385,13 @@ out_thread:
 		kthread_stop(t);
 		put_task_struct(t);
 	}
+	if (new->secondary && new->secondary->thread) {
+		struct task_struct *t = new->secondary->thread;
+
+		new->secondary->thread = NULL;
+		kthread_stop(t);
+		put_task_struct(t);
+	}
 out_mput:
 	module_put(desc->owner);
 	return ret;
@@ -1430,9 +1499,14 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 	if (action->thread) {
 		kthread_stop(action->thread);
 		put_task_struct(action->thread);
+		if (action->secondary && action->secondary->thread) {
+			kthread_stop(action->secondary->thread);
+			put_task_struct(action->secondary->thread);
+		}
 	}
 
 	module_put(desc->owner);
+	kfree(action->secondary);
 	return action;
 }
 
@@ -1576,8 +1650,10 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	retval = __setup_irq(irq, desc, action);
 	chip_bus_sync_unlock(desc);
 
-	if (retval)
+	if (retval) {
+		kfree(action->secondary);
 		kfree(action);
+	}
 
 #ifdef CONFIG_DEBUG_SHIRQ_FIXME
 	if (!retval && (irqflags & IRQF_SHARED)) {

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tip:irq/core] genirq:  Handle force threading of irqs with primary and thread handler
  2015-09-22 10:42 ` [tip:irq/core] " tip-bot for Thomas Gleixner
@ 2015-10-06 18:59   ` Felipe Balbi
  2015-10-09 10:36     ` Thomas Gleixner
  0 siblings, 1 reply; 6+ messages in thread
From: Felipe Balbi @ 2015-10-06 18:59 UTC (permalink / raw)
  To: hpa, okuno.kohji, bigeasy, linux-kernel, msmucr, nathan.sullivan,
	tglx, mingo, linux-tip-commits
  Cc: hpa, okuno.kohji, bigeasy, linux-kernel, msmucr, nathan.sullivan,
	mingo, tglx


[-- Attachment #1: Type: text/plain, Size: 3405 bytes --]


Hi Thomas,

tip-bot for Thomas Gleixner <tipbot@zytor.com> writes:
> Commit-ID:  2a1d3ab8986d1b2f598ffc42351d94166fa0f022
> Gitweb:     http://git.kernel.org/tip/2a1d3ab8986d1b2f598ffc42351d94166fa0f022
> Author:     Thomas Gleixner <tglx@linutronix.de>
> AuthorDate: Mon, 21 Sep 2015 11:01:10 +0200
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Tue, 22 Sep 2015 12:39:57 +0200
>
> genirq: Handle force threading of irqs with primary and thread handler
>
> Force threading of interrupts does not really deal with interrupts
> which are requested with a primary and a threaded handler. The current
> policy is to leave them alone and let the primary handler run in
> interrupt context, but we set the ONESHOT flag for those interrupts as
> well.
>
> Kohji Okuno debugged a problem with the SDHCI driver where the
> interrupt thread waits for a hardware interrupt to trigger, which can't
> work well because the hardware interrupt is masked due to the ONESHOT
> flag being set. He proposed to set the ONESHOT flag only if the
> interrupt does not provide a thread handler.
>
> Though that does not work either because these interrupts can be
> shared. So the other interrupt would rightfully get the ONESHOT flag
> set and therefor the same situation would happen again.
>
> To deal with this proper, we need to force thread the primary handler
> of such interrupts as well. That means that the primary interrupt
> handler is treated as any other primary interrupt handler which is not
> marked IRQF_NO_THREAD. The threaded handler becomes a separate thread
> so the SDHCI flow logic can be handled gracefully.
>
> The same issue was reported against 4.1-rt.
>
> Reported-and-tested-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
> Reported-By: Michal Smucr <msmucr@gmail.com>
> Reported-and-tested-by: Nathan Sullivan <nathan.sullivan@ni.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Link: http://lkml.kernel.org/r/alpine.DEB.2.11.1509211058080.5606@nanos
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

this commit causes a performance regression for the USB driver on
several platforms (anybody using drivers/usb/dwc3, basically).

Here's the USB throughput with linux-next in 3 different scenarios:

1) Linux next without threadirqs cmdline

   test  0: sent     256.00 MB read      33.02 MB/s write      30.01 MB/s

2) Linux next with threadirqs on cmdline

   test  0: sent     256.00 MB read      30.70 MB/s write      27.89 MB/s

3) Linux next with threadirqs on cmdline + revert of $subject

   test  0: sent     256.00 MB read      32.93 MB/s write      29.85 MB/s


Considering this is trying to solve an issue found on the SDHCI driver,
shouldn't that be fixed instead ? Another option would be, of course, to
add IRQF_NO_THREAD to dwc3, but I'd like to avoid that if possible.

The way we try to use dwc3 is rather simple, actually. We use the
primary handle *only* to detect is $this device generated the IRQ and if
did we wake up the thread. We also don't make use of ONESHOT because we
mask $this device IRQs in the primary handler and only unmask after the
thread runs.

It's a bit surprising, to me at least, that simply running everything as
a thread would have such a measurable impact, but it does.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tip:irq/core] genirq: Handle force threading of irqs with primary and thread handler
  2015-10-06 18:59   ` Felipe Balbi
@ 2015-10-09 10:36     ` Thomas Gleixner
  2015-10-09 14:01       ` Felipe Balbi
  0 siblings, 1 reply; 6+ messages in thread
From: Thomas Gleixner @ 2015-10-09 10:36 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: hpa, okuno.kohji, bigeasy, linux-kernel, msmucr, nathan.sullivan,
	mingo, linux-tip-commits

On Tue, 6 Oct 2015, Felipe Balbi wrote:
> this commit causes a performance regression for the USB driver on
> several platforms (anybody using drivers/usb/dwc3, basically).
> 
> Here's the USB throughput with linux-next in 3 different scenarios:
> 
> 1) Linux next without threadirqs cmdline
> 
>    test  0: sent     256.00 MB read      33.02 MB/s write      30.01 MB/s
> 
> 2) Linux next with threadirqs on cmdline
> 
>    test  0: sent     256.00 MB read      30.70 MB/s write      27.89 MB/s
> 
> 3) Linux next with threadirqs on cmdline + revert of $subject
> 
>    test  0: sent     256.00 MB read      32.93 MB/s write      29.85 MB/s
> 
> 
> Considering this is trying to solve an issue found on the SDHCI driver,
> shouldn't that be fixed instead ? Another option would be, of course, to
> add IRQF_NO_THREAD to dwc3, but I'd like to avoid that if possible.

It's not only an issue for SDHCI. It's a general problem with other
drivers as well.
 
> The way we try to use dwc3 is rather simple, actually. We use the
> primary handle *only* to detect is $this device generated the IRQ and if
> did we wake up the thread. We also don't make use of ONESHOT because we
> mask $this device IRQs in the primary handler and only unmask after the
> thread runs.

So in your case IRQF_NO_THREAD is really the solution. It will keep
your primary handler handled in the hard interrupt context. That will
work on RT as well.
 
> It's a bit surprising, to me at least, that simply running everything as
> a thread would have such a measurable impact, but it does.

I'm surprised of the size of the impact as well. I wouldn't have
expected that another kernel thread context switch makes such a
difference.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [tip:irq/core] genirq: Handle force threading of irqs with primary and thread handler
  2015-10-09 10:36     ` Thomas Gleixner
@ 2015-10-09 14:01       ` Felipe Balbi
  0 siblings, 0 replies; 6+ messages in thread
From: Felipe Balbi @ 2015-10-09 14:01 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: hpa, okuno.kohji, bigeasy, linux-kernel, msmucr, nathan.sullivan,
	mingo, linux-tip-commits


[-- Attachment #1: Type: text/plain, Size: 1607 bytes --]


Hi,

Thomas Gleixner <tglx@linutronix.de> writes:
> On Tue, 6 Oct 2015, Felipe Balbi wrote:
>> this commit causes a performance regression for the USB driver on
>> several platforms (anybody using drivers/usb/dwc3, basically).
>> 
>> Here's the USB throughput with linux-next in 3 different scenarios:
>> 
>> 1) Linux next without threadirqs cmdline
>> 
>>    test  0: sent     256.00 MB read      33.02 MB/s write      30.01 MB/s
>> 
>> 2) Linux next with threadirqs on cmdline
>> 
>>    test  0: sent     256.00 MB read      30.70 MB/s write      27.89 MB/s
>> 
>> 3) Linux next with threadirqs on cmdline + revert of $subject
>> 
>>    test  0: sent     256.00 MB read      32.93 MB/s write      29.85 MB/s
>> 
>> 
>> Considering this is trying to solve an issue found on the SDHCI driver,
>> shouldn't that be fixed instead ? Another option would be, of course, to
>> add IRQF_NO_THREAD to dwc3, but I'd like to avoid that if possible.
>
> It's not only an issue for SDHCI. It's a general problem with other
> drivers as well.
>  
>> The way we try to use dwc3 is rather simple, actually. We use the
>> primary handle *only* to detect is $this device generated the IRQ and if
>> did we wake up the thread. We also don't make use of ONESHOT because we
>> mask $this device IRQs in the primary handler and only unmask after the
>> thread runs.
>
> So in your case IRQF_NO_THREAD is really the solution. It will keep
> your primary handler handled in the hard interrupt context. That will
> work on RT as well.

all right. I'll patch that up. Thanks

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-21  9:01 [PATCH] genirq: Handle force threading of irqs with primary and thread handler Thomas Gleixner
2015-09-22  6:42 ` Kohji Okuno
2015-09-22 10:42 ` [tip:irq/core] " tip-bot for Thomas Gleixner
2015-10-06 18:59   ` Felipe Balbi
2015-10-09 10:36     ` Thomas Gleixner
2015-10-09 14:01       ` Felipe Balbi

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git