* [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, ¶m); + + /* + * 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, ¶m); - - /* - * 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, ¶m); + + /* + * 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, ¶m); - - /* - * 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 related [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, other threads:[~2015-10-09 14:01 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).