[RFC] genirq: Flush the irq thread on synchronization

[RFC] genirq: Flush the irq thread on synchronization

[Ido Yariv]

The current implementation does not always flush the threaded handler
when disabling the irq. In case the irq handler was called, but the
threaded handler hasn't started running yet, the interrupt will be
flagged as pending, and the handler will not run. This implementation
has some issues:

First, if the interrupt is a wake source and flagged as pending, the
system will not be able to suspend.

Second, when quickly disabling and re-enabling the irq, the threaded
handler might continue to run after the irq is re-enabled without the
irq handler being called first. This might be an unexpected behavior.

In addition, it might be counter-intuitive that the threaded handler
will not be called even though the irq handler was called and returned
IRQ_WAKE_THREAD.

Fix this by always waiting for the threaded handler to complete in
synchronize_irq().

Signed-off-by: Ido Yariv <ido@wizery.com>
---
 kernel/irq/handle.c |   10 ++++++++++
 kernel/irq/manage.c |   48 ++++++++++++++++++++++--------------------------
 2 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 470d08c..28f54d1 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -110,6 +110,16 @@ static void irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
 	 * threads_oneshot untouched and runs the thread another time.
 	 */
 	desc->threads_oneshot |= action->thread_mask;
+
+	/*
+	 * If threads_active is increased by the thread, synchronize_irq()
+	 * might be called after the thread has been woken up, but before it
+	 * has had a chance to increase threads_active. In this case,
+	 * synchronize_irq() will return even though the thread is still
+	 * running. Thus, increase threads_active here.
+	 */
+	atomic_inc(&desc->threads_active);
+
 	wake_up_process(action->thread);
 }
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 1da999f..896658f 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -759,6 +759,16 @@ static irqreturn_t irq_thread_fn(struct irq_desc *desc,
 	return ret;
 }
 
+static void wake_threads_waitq(struct irq_desc *desc)
+{
+	int wake;
+
+	wake = atomic_dec_and_test(&desc->threads_active);
+
+	if (wake && waitqueue_active(&desc->wait_for_threads))
+		wake_up(&desc->wait_for_threads);
+}
+
 /*
  * Interrupt handler thread
  */
@@ -771,7 +781,6 @@ static int irq_thread(void *data)
 	struct irq_desc *desc = irq_to_desc(action->irq);
 	irqreturn_t (*handler_fn)(struct irq_desc *desc,
 			struct irqaction *action);
-	int wake;
 
 	if (force_irqthreads & test_bit(IRQTF_FORCED_THREAD,
 					&action->thread_flags))
@@ -783,37 +792,24 @@ static int irq_thread(void *data)
 	current->irqaction = action;
 
 	while (!irq_wait_for_interrupt(action)) {
+		irqreturn_t action_ret;
 
 		irq_thread_check_affinity(desc, action);
 
-		atomic_inc(&desc->threads_active);
-
-		raw_spin_lock_irq(&desc->lock);
-		if (unlikely(irqd_irq_disabled(&desc->irq_data))) {
-			/*
-			 * CHECKME: We might need a dedicated
-			 * IRQ_THREAD_PENDING flag here, which
-			 * retriggers the thread in check_irq_resend()
-			 * but AFAICT IRQS_PENDING should be fine as it
-			 * retriggers the interrupt itself --- tglx
-			 */
-			desc->istate |= IRQS_PENDING;
-			raw_spin_unlock_irq(&desc->lock);
-		} else {
-			irqreturn_t action_ret;
-
-			raw_spin_unlock_irq(&desc->lock);
-			action_ret = handler_fn(desc, action);
-			if (!noirqdebug)
-				note_interrupt(action->irq, desc, action_ret);
-		}
-
-		wake = atomic_dec_and_test(&desc->threads_active);
+		action_ret = handler_fn(desc, action);
+		if (!noirqdebug)
+			note_interrupt(action->irq, desc, action_ret);
 
-		if (wake && waitqueue_active(&desc->wait_for_threads))
-			wake_up(&desc->wait_for_threads);
+		wake_threads_waitq(desc);
 	}
 
+	/*
+	 * In case the thread was woken up, threads_active is already
+	 * increased. Decrease it and wake up any waiting tasks.
+	 */
+	if (test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags))
+		wake_threads_waitq(desc);
+
 	/* Prevent a stale desc->threads_oneshot */
 	irq_finalize_oneshot(desc, action, true);
 
-- 
1.7.7.3

Re: [RFC] genirq: Flush the irq thread on synchronization

[Thomas Gleixner]

On Fri, 2 Dec 2011, Ido Yariv wrote:

> The current implementation does not always flush the threaded handler
> when disabling the irq. In case the irq handler was called, but the
> threaded handler hasn't started running yet, the interrupt will be
> flagged as pending, and the handler will not run. This implementation
> has some issues:
> 
> First, if the interrupt is a wake source and flagged as pending, the
> system will not be able to suspend.
> 
> Second, when quickly disabling and re-enabling the irq, the threaded
> handler might continue to run after the irq is re-enabled without the
> irq handler being called first. This might be an unexpected behavior.

I'd wish people would stop calling disable/enable_irq() in loops and
circles for no reason.

> In addition, it might be counter-intuitive that the threaded handler
> will not be called even though the irq handler was called and returned
> IRQ_WAKE_THREAD.
> 
> Fix this by always waiting for the threaded handler to complete in
> synchronize_irq().

I can see your problem, but this might lead to threads_active leaks
under certain conditions. desc->threads_active was only meant to deal
with shared interrupts.

We explicitely allow a design where the primary handler can leave the
device interrupt enabled and allow further interrupts to occur while
the handler is running. We only have a single bit to note that the
thread should run, but your wakeup would up the threads_active count
in that scenario several times w/o a counterpart which decrements it.

The solution for this is to keep the current threads_active semantics
and make the wait function different. Instead of waiting for
threads_active to become 0 it should wait for threads_active == 0 and
the IRQTF_RUNTHREAD for all actions to be cleared. To avoid looping
over the actions, we can take a similar approach as we take with the
desc->threads_oneshot bitfield.

Thanks,

	tglx

Re: [RFC] genirq: Flush the irq thread on synchronization

[Ido Yariv]

Hi Thomas,

On Sat, Dec 03, 2011 at 12:21:46AM +0100, Thomas Gleixner wrote:
> I can see your problem, but this might lead to threads_active leaks
> under certain conditions. desc->threads_active was only meant to deal
> with shared interrupts.
> 
> We explicitely allow a design where the primary handler can leave the
> device interrupt enabled and allow further interrupts to occur while
> the handler is running. We only have a single bit to note that the
> thread should run, but your wakeup would up the threads_active count
> in that scenario several times w/o a counterpart which decrements it.
> 
> The solution for this is to keep the current threads_active semantics
> and make the wait function different. Instead of waiting for
> threads_active to become 0 it should wait for threads_active == 0 and
> the IRQTF_RUNTHREAD for all actions to be cleared. To avoid looping
> over the actions, we can take a similar approach as we take with the
> desc->threads_oneshot bitfield.

Thanks for reviewing this.

I might be missing something, but I don't see any potential
threads_active leaks in this approach. We wont increase threads_active
if IRQTF_RUNTHREAD was already set beforehand (as test_and_set_bit()
will return 1).

If irq_wake_thread is called multiple times before irq_thread has had a
chance to run, threads_active will only be increased once and decreased
back when IRQTF_RUNTHREAD is cleared.

Am I missing something? If not, do you see any other issues with this
implementation?

Thanks,
Ido.

Re: [RFC] genirq: Flush the irq thread on synchronization

[Ido Yariv]

Hi Thomas,

On Sun, Dec 04, 2011 at 09:09:32PM +0200, Ido Yariv wrote:
> Hi Thomas,
> 
> On Sat, Dec 03, 2011 at 12:21:46AM +0100, Thomas Gleixner wrote:
> > I can see your problem, but this might lead to threads_active leaks
> > under certain conditions. desc->threads_active was only meant to deal
> > with shared interrupts.
> > 
> > We explicitely allow a design where the primary handler can leave the
> > device interrupt enabled and allow further interrupts to occur while
> > the handler is running. We only have a single bit to note that the
> > thread should run, but your wakeup would up the threads_active count
> > in that scenario several times w/o a counterpart which decrements it.
> > 
> > The solution for this is to keep the current threads_active semantics
> > and make the wait function different. Instead of waiting for
> > threads_active to become 0 it should wait for threads_active == 0 and
> > the IRQTF_RUNTHREAD for all actions to be cleared. To avoid looping
> > over the actions, we can take a similar approach as we take with the
> > desc->threads_oneshot bitfield.
> 
> Thanks for reviewing this.
> 
> I might be missing something, but I don't see any potential
> threads_active leaks in this approach. We wont increase threads_active
> if IRQTF_RUNTHREAD was already set beforehand (as test_and_set_bit()
> will return 1).
> 
> If irq_wake_thread is called multiple times before irq_thread has had a
> chance to run, threads_active will only be increased once and decreased
> back when IRQTF_RUNTHREAD is cleared.
> 
> Am I missing something? If not, do you see any other issues with this
> implementation?

Any thoughts on this?

Thanks,
Ido.

Re: [RFC] genirq: Flush the irq thread on synchronization

[Ido Yariv]

Hi Thomas,

On Sun, Dec 04, 2011 at 09:09:32PM +0200, Ido Yariv wrote:
> Hi Thomas,
> 
> On Sat, Dec 03, 2011 at 12:21:46AM +0100, Thomas Gleixner wrote:
> > I can see your problem, but this might lead to threads_active leaks
> > under certain conditions. desc->threads_active was only meant to deal
> > with shared interrupts.
> > 
> > We explicitely allow a design where the primary handler can leave the
> > device interrupt enabled and allow further interrupts to occur while
> > the handler is running. We only have a single bit to note that the
> > thread should run, but your wakeup would up the threads_active count
> > in that scenario several times w/o a counterpart which decrements it.
> > 
> > The solution for this is to keep the current threads_active semantics
> > and make the wait function different. Instead of waiting for
> > threads_active to become 0 it should wait for threads_active == 0 and
> > the IRQTF_RUNTHREAD for all actions to be cleared. To avoid looping
> > over the actions, we can take a similar approach as we take with the
> > desc->threads_oneshot bitfield.
> 
> Thanks for reviewing this.
> 
> I might be missing something, but I don't see any potential
> threads_active leaks in this approach. We wont increase threads_active
> if IRQTF_RUNTHREAD was already set beforehand (as test_and_set_bit()
> will return 1).
> 
> If irq_wake_thread is called multiple times before irq_thread has had a
> chance to run, threads_active will only be increased once and decreased
> back when IRQTF_RUNTHREAD is cleared.
> 
> Am I missing something? If not, do you see any other issues with this
> implementation?

Any thoughts on this?

Thanks,
Ido.

Re: [RFC] genirq: Flush the irq thread on synchronization

[Thomas Gleixner]

On Mon, 13 Feb 2012, Ido Yariv wrote:

> Hi Thomas,
> 
> On Sun, Dec 04, 2011 at 09:09:32PM +0200, Ido Yariv wrote:
> > Hi Thomas,
> > 
> > On Sat, Dec 03, 2011 at 12:21:46AM +0100, Thomas Gleixner wrote:
> > > I can see your problem, but this might lead to threads_active leaks
> > > under certain conditions. desc->threads_active was only meant to deal
> > > with shared interrupts.
> > > 
> > > We explicitely allow a design where the primary handler can leave the
> > > device interrupt enabled and allow further interrupts to occur while
> > > the handler is running. We only have a single bit to note that the
> > > thread should run, but your wakeup would up the threads_active count
> > > in that scenario several times w/o a counterpart which decrements it.
> > > 
> > > The solution for this is to keep the current threads_active semantics
> > > and make the wait function different. Instead of waiting for
> > > threads_active to become 0 it should wait for threads_active == 0 and
> > > the IRQTF_RUNTHREAD for all actions to be cleared. To avoid looping
> > > over the actions, we can take a similar approach as we take with the
> > > desc->threads_oneshot bitfield.
> > 
> > Thanks for reviewing this.
> > 
> > I might be missing something, but I don't see any potential
> > threads_active leaks in this approach. We wont increase threads_active
> > if IRQTF_RUNTHREAD was already set beforehand (as test_and_set_bit()
> > will return 1).
> > 
> > If irq_wake_thread is called multiple times before irq_thread has had a
> > chance to run, threads_active will only be increased once and decreased
> > back when IRQTF_RUNTHREAD is cleared.
> > 
> > Am I missing something? If not, do you see any other issues with this
> > implementation?
> 
> Any thoughts on this?

Sorry, went of my radar. Will have a look later today.

Thanks,

	tglx

Re: [RFC] genirq: Flush the irq thread on synchronization

[Ido Yariv]

Hi Thomas,

On Wed, Feb 15, 2012 at 03:34:11PM +0100, Thomas Gleixner wrote:
> On Mon, 13 Feb 2012, Ido Yariv wrote:
> 
> > Hi Thomas,
> > 
> > On Sun, Dec 04, 2011 at 09:09:32PM +0200, Ido Yariv wrote:
> > > Hi Thomas,
> > > 
> > > On Sat, Dec 03, 2011 at 12:21:46AM +0100, Thomas Gleixner wrote:
> > > > I can see your problem, but this might lead to threads_active leaks
> > > > under certain conditions. desc->threads_active was only meant to deal
> > > > with shared interrupts.
> > > > 
> > > > We explicitely allow a design where the primary handler can leave the
> > > > device interrupt enabled and allow further interrupts to occur while
> > > > the handler is running. We only have a single bit to note that the
> > > > thread should run, but your wakeup would up the threads_active count
> > > > in that scenario several times w/o a counterpart which decrements it.
> > > > 
> > > > The solution for this is to keep the current threads_active semantics
> > > > and make the wait function different. Instead of waiting for
> > > > threads_active to become 0 it should wait for threads_active == 0 and
> > > > the IRQTF_RUNTHREAD for all actions to be cleared. To avoid looping
> > > > over the actions, we can take a similar approach as we take with the
> > > > desc->threads_oneshot bitfield.
> > > 
> > > Thanks for reviewing this.
> > > 
> > > I might be missing something, but I don't see any potential
> > > threads_active leaks in this approach. We wont increase threads_active
> > > if IRQTF_RUNTHREAD was already set beforehand (as test_and_set_bit()
> > > will return 1).
> > > 
> > > If irq_wake_thread is called multiple times before irq_thread has had a
> > > chance to run, threads_active will only be increased once and decreased
> > > back when IRQTF_RUNTHREAD is cleared.
> > > 
> > > Am I missing something? If not, do you see any other issues with this
> > > implementation?
> > 
> > Any thoughts on this?
> 
> Sorry, went of my radar. Will have a look later today.

Have you had a chance to look into this?

Thanks,
Ido.

Re: [RFC] genirq: Flush the irq thread on synchronization

[Thomas Gleixner]

On Sat, 3 Dec 2011, Thomas Gleixner wrote:
> On Fri, 2 Dec 2011, Ido Yariv wrote:
> 
> > The current implementation does not always flush the threaded handler
> > when disabling the irq. In case the irq handler was called, but the
> > threaded handler hasn't started running yet, the interrupt will be
> > flagged as pending, and the handler will not run. This implementation
> > has some issues:
> > 
> > First, if the interrupt is a wake source and flagged as pending, the
> > system will not be able to suspend.
> > 
> > Second, when quickly disabling and re-enabling the irq, the threaded
> > handler might continue to run after the irq is re-enabled without the
> > irq handler being called first. This might be an unexpected behavior.
> 
> I'd wish people would stop calling disable/enable_irq() in loops and
> circles for no reason.
> 
> > In addition, it might be counter-intuitive that the threaded handler
> > will not be called even though the irq handler was called and returned
> > IRQ_WAKE_THREAD.
> > 
> > Fix this by always waiting for the threaded handler to complete in
> > synchronize_irq().
> 
> I can see your problem, but this might lead to threads_active leaks
> under certain conditions. desc->threads_active was only meant to deal
> with shared interrupts.
> 
> We explicitely allow a design where the primary handler can leave the
> device interrupt enabled and allow further interrupts to occur while
> the handler is running. We only have a single bit to note that the
> thread should run, but your wakeup would up the threads_active count
> in that scenario several times w/o a counterpart which decrements it.
> 
> The solution for this is to keep the current threads_active semantics
> and make the wait function different. Instead of waiting for
> threads_active to become 0 it should wait for threads_active == 0 and
> the IRQTF_RUNTHREAD for all actions to be cleared. To avoid looping
> over the actions, we can take a similar approach as we take with the
> desc->threads_oneshot bitfield.

Does the following (untested) patch solve your issues?

Thanks,

	tglx

Index: tip/kernel/irq/manage.c
===================================================================
--- tip.orig/kernel/irq/manage.c
+++ tip/kernel/irq/manage.c
@@ -28,6 +28,18 @@ static int __init setup_forced_irqthread
 early_param("threadirqs", setup_forced_irqthreads);
 #endif
 
+static bool irq_threads_stopped(struct irq_desc *desc)
+{
+	unsigned long flags;
+	bool res;
+
+	raw_spin_lock_irqsave(&desc->lock, flags);
+	res = !atomic_read(&desc->threads_active) &&
+		!desc->threads_oneshot;
+	raw_spin_unlock_irqrestore(&desc->lock, flags);
+	return res;
+}
+
 /**
  *	synchronize_irq - wait for pending IRQ handlers (on other CPUs)
  *	@irq: interrupt number to wait for
@@ -68,7 +80,7 @@ void synchronize_irq(unsigned int irq)
 	 * We made sure that no hardirq handler is running. Now verify
 	 * that no threaded handlers are active.
 	 */
-	wait_event(desc->wait_for_threads, !atomic_read(&desc->threads_active));
+	wait_event(desc->wait_for_threads, irq_threads_stopped(desc));
 }
 EXPORT_SYMBOL(synchronize_irq);
 
@@ -639,13 +651,11 @@ static int irq_wait_for_interrupt(struct
 /*
  * Oneshot interrupts keep the irq line masked until the threaded
  * handler finished. unmask if the interrupt has not been disabled and
- * is marked MASKED.
+ * is marked MASKED. We also track that way that all threads are done.
  */
 static void irq_finalize_oneshot(struct irq_desc *desc,
 				 struct irqaction *action, bool force)
 {
-	if (!(desc->istate & IRQS_ONESHOT))
-		return;
 again:
 	chip_bus_lock(desc);
 	raw_spin_lock_irq(&desc->lock);
@@ -681,6 +691,9 @@ again:
 
 	desc->threads_oneshot &= ~action->thread_mask;
 
+	if (!(desc->istate & IRQS_ONESHOT))
+		goto out_unlock;
+
 	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
 	    irqd_irq_masked(&desc->irq_data))
 		unmask_irq(desc);
@@ -780,30 +793,15 @@ static int irq_thread(void *data)
 	current->irqaction = action;
 
 	while (!irq_wait_for_interrupt(action)) {
+		irqreturn_t action_ret;
 
 		irq_thread_check_affinity(desc, action);
 
 		atomic_inc(&desc->threads_active);
 
-		raw_spin_lock_irq(&desc->lock);
-		if (unlikely(irqd_irq_disabled(&desc->irq_data))) {
-			/*
-			 * CHECKME: We might need a dedicated
-			 * IRQ_THREAD_PENDING flag here, which
-			 * retriggers the thread in check_irq_resend()
-			 * but AFAICT IRQS_PENDING should be fine as it
-			 * retriggers the interrupt itself --- tglx
-			 */
-			desc->istate |= IRQS_PENDING;
-			raw_spin_unlock_irq(&desc->lock);
-		} else {
-			irqreturn_t action_ret;
-
-			raw_spin_unlock_irq(&desc->lock);
-			action_ret = handler_fn(desc, action);
-			if (!noirqdebug)
-				note_interrupt(action->irq, desc, action_ret);
-		}
+		action_ret = handler_fn(desc, action);
+		if (!noirqdebug)
+			note_interrupt(action->irq, desc, action_ret);
 
 		wake = atomic_dec_and_test(&desc->threads_active);
 
@@ -993,7 +991,7 @@ __setup_irq(unsigned int irq, struct irq
 	 * Setup the thread mask for this irqaction. Unlikely to have
 	 * 32 resp 64 irqs sharing one line, but who knows.
 	 */
-	if (new->flags & IRQF_ONESHOT && thread_mask == ~0UL) {
+	if (thread_mask == ~0UL) {
 		ret = -EBUSY;
 		goto out_mask;
 	}

Re: [RFC] genirq: Flush the irq thread on synchronization

[Ido Yariv]

Hi Thomas,

On Mon, Dec 05, 2011 at 10:55:01PM +0100, Thomas Gleixner wrote:
> Does the following (untested) patch solve your issues?

I briefly tested this patch and it appears to also solve the issues I
mentioned.

I think both solutions would work. The only drawbacks I see in using
this patch over the other one are that this patch involves a bit more
locking in synchronize_irq() and irq_finalize_oneshot() (for non-oneshot
irqs), and that the oneshot mechanism is used for non-oneshot threads.

Thanks,
Ido.

Re: [RFC] genirq: Flush the irq thread on synchronization

[Thomas Gleixner]

On Wed, 7 Dec 2011, Ido Yariv wrote:

> Hi Thomas,
> 
> On Mon, Dec 05, 2011 at 10:55:01PM +0100, Thomas Gleixner wrote:
> > Does the following (untested) patch solve your issues?
> 
> I briefly tested this patch and it appears to also solve the issues I
> mentioned.
> 
> I think both solutions would work. The only drawbacks I see in using
> this patch over the other one are that this patch involves a bit more
> locking in synchronize_irq() and irq_finalize_oneshot() (for non-oneshot
> irqs), and that the oneshot mechanism is used for non-oneshot threads.

I don't care about synchronize_irq() at all. It's a slow path anyway.

The extra burden in irq_finalize_oneshot() is somewhat of a concern,
but that locks should be completely uncontended for real threaded irq
users, so in practice it's a non issue.

It's the only solution which actually solves the thread_active leak,
though I just discovered a very narrow race window between the
test_and_clear_bit and the active increment, which is easy to
fix. I'll send out another version tomorrow

Thanks,

	tglx

Re: [RFC] genirq: Flush the irq thread on synchronization

[Ido Yariv]

Hi Thomas,

On Wed, Dec 07, 2011 at 01:48:30AM +0100, Thomas Gleixner wrote:
> It's the only solution which actually solves the thread_active leak,

I think a thread regarding this issue was missed:
https://lkml.org/lkml/2011/12/4/113

Thanks,
Ido.

[tip:irq/core] genirq: Flush the irq thread on synchronization

[tip-bot for Ido Yariv]

Commit-ID:  7140ea1980f2fae9c7aaeac5f6b35317e1389ee6
Gitweb:     http://git.kernel.org/tip/7140ea1980f2fae9c7aaeac5f6b35317e1389ee6
Author:     Ido Yariv <ido@wizery.com>
AuthorDate: Fri, 2 Dec 2011 18:24:12 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Mar 2012 11:56:20 +0100

genirq: Flush the irq thread on synchronization

The current implementation does not always flush the threaded handler
when disabling the irq. In case the irq handler was called, but the
threaded handler hasn't started running yet, the interrupt will be
flagged as pending, and the handler will not run. This implementation
has some issues:

First, if the interrupt is a wake source and flagged as pending, the
system will not be able to suspend.

Second, when quickly disabling and re-enabling the irq, the threaded
handler might continue to run after the irq is re-enabled without the
irq handler being called first. This might be an unexpected behavior.

In addition, it might be counter-intuitive that the threaded handler
will not be called even though the irq handler was called and returned
IRQ_WAKE_THREAD.

Fix this by always waiting for the threaded handler to complete in
synchronize_irq().

[ tglx: Massaged comments, added WARN_ONs and the missing
  	IRQTF_RUNTHREAD check in exit_irq_thread() ]

Signed-off-by: Ido Yariv <ido@wizery.com>
Link: http://lkml.kernel.org/r/1322843052-7166-1-git-send-email-ido@wizery.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/irq/handle.c |   12 ++++++++++
 kernel/irq/manage.c |   60 +++++++++++++++++++++++++++-----------------------
 2 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 500aaf6..6ff84e6 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -110,6 +110,18 @@ static void irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
 	 * threads_oneshot untouched and runs the thread another time.
 	 */
 	desc->threads_oneshot |= action->thread_mask;
+
+	/*
+	 * We increment the threads_active counter in case we wake up
+	 * the irq thread. The irq thread decrements the counter when
+	 * it returns from the handler or in the exit path and wakes
+	 * up waiters which are stuck in synchronize_irq() when the
+	 * active count becomes zero. synchronize_irq() is serialized
+	 * against this code (hard irq handler) via IRQS_INPROGRESS
+	 * like the finalize_oneshot() code. See comment above.
+	 */
+	atomic_inc(&desc->threads_active);
+
 	wake_up_process(action->thread);
 }
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 1786cf7..453feed 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -759,6 +759,13 @@ static irqreturn_t irq_thread_fn(struct irq_desc *desc,
 	return ret;
 }
 
+static void wake_threads_waitq(struct irq_desc *desc)
+{
+	if (atomic_dec_and_test(&desc->threads_active) &&
+	    waitqueue_active(&desc->wait_for_threads))
+		wake_up(&desc->wait_for_threads);
+}
+
 /*
  * Interrupt handler thread
  */
@@ -771,7 +778,6 @@ static int irq_thread(void *data)
 	struct irq_desc *desc = irq_to_desc(action->irq);
 	irqreturn_t (*handler_fn)(struct irq_desc *desc,
 			struct irqaction *action);
-	int wake;
 
 	if (force_irqthreads && test_bit(IRQTF_FORCED_THREAD,
 					&action->thread_flags))
@@ -783,39 +789,30 @@ static int irq_thread(void *data)
 	current->irq_thread = 1;
 
 	while (!irq_wait_for_interrupt(action)) {
+		irqreturn_t action_ret;
 
 		irq_thread_check_affinity(desc, action);
 
-		atomic_inc(&desc->threads_active);
+		action_ret = handler_fn(desc, action);
+		if (!noirqdebug)
+			note_interrupt(action->irq, desc, action_ret);
 
-		raw_spin_lock_irq(&desc->lock);
-		if (unlikely(irqd_irq_disabled(&desc->irq_data))) {
-			/*
-			 * CHECKME: We might need a dedicated
-			 * IRQ_THREAD_PENDING flag here, which
-			 * retriggers the thread in check_irq_resend()
-			 * but AFAICT IRQS_PENDING should be fine as it
-			 * retriggers the interrupt itself --- tglx
-			 */
-			desc->istate |= IRQS_PENDING;
-			raw_spin_unlock_irq(&desc->lock);
-		} else {
-			irqreturn_t action_ret;
-
-			raw_spin_unlock_irq(&desc->lock);
-			action_ret = handler_fn(desc, action);
-			if (!noirqdebug)
-				note_interrupt(action->irq, desc, action_ret);
-		}
-
-		wake = atomic_dec_and_test(&desc->threads_active);
-
-		if (wake && waitqueue_active(&desc->wait_for_threads))
-			wake_up(&desc->wait_for_threads);
+		wake_threads_waitq(desc);
 	}
 
-	/* Prevent a stale desc->threads_oneshot */
-	irq_finalize_oneshot(desc, action, true);
+	/*
+	 * This is the regular exit path. __free_irq() is stopping the
+	 * thread via kthread_stop() after calling
+	 * synchronize_irq(). So neither IRQTF_RUNTHREAD nor the
+	 * oneshot mask bit should be set.
+	 *
+	 * Verify that this is true.
+	 */
+	if (WARN_ON(test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags)))
+		wake_threads_waitq(desc);
+
+	if (WARN_ON(desc->threads_oneshot & action->thread_mask))
+		irq_finalize_oneshot(desc, action, true);
 
 	/*
 	 * Clear irq_thread. Otherwise exit_irq_thread() would make
@@ -845,6 +842,13 @@ void exit_irq_thread(void)
 
 	desc = irq_to_desc(action->irq);
 
+	/*
+	 * If IRQTF_RUNTHREAD is set, we need to decrement
+	 * desc->threads_active and wake possible waiters.
+	 */
+	if (test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags))
+		wake_threads_waitq(desc);
+
 	/* Prevent a stale desc->threads_oneshot */
 	irq_finalize_oneshot(desc, action, true);
 }

Re: [tip:irq/core] genirq: Flush the irq thread on synchronization

[Alexander Gordeev]

On Wed, Mar 14, 2012 at 04:07:30AM -0700, tip-bot for Ido Yariv wrote:

Ido, Thomas,

Sorry for being late into the discussion.
Have couple of concerns regarding this hunk..

> @@ -783,39 +789,30 @@ static int irq_thread(void *data)
>  	current->irq_thread = 1;
>  
>  	while (!irq_wait_for_interrupt(action)) {
> +		irqreturn_t action_ret;
>  
>  		irq_thread_check_affinity(desc, action);
>  
> -		atomic_inc(&desc->threads_active);
> +		action_ret = handler_fn(desc, action);
> +		if (!noirqdebug)
> +			note_interrupt(action->irq, desc, action_ret);
>  
> -		raw_spin_lock_irq(&desc->lock);
> -		if (unlikely(irqd_irq_disabled(&desc->irq_data))) {
> -			/*
> -			 * CHECKME: We might need a dedicated
> -			 * IRQ_THREAD_PENDING flag here, which
> -			 * retriggers the thread in check_irq_resend()
> -			 * but AFAICT IRQS_PENDING should be fine as it
> -			 * retriggers the interrupt itself --- tglx
> -			 */
> -			desc->istate |= IRQS_PENDING;
> -			raw_spin_unlock_irq(&desc->lock);
> -		} else {
> -			irqreturn_t action_ret;
> -
> -			raw_spin_unlock_irq(&desc->lock);
> -			action_ret = handler_fn(desc, action);
> -			if (!noirqdebug)
> -				note_interrupt(action->irq, desc, action_ret);
> -		}
> -
> -		wake = atomic_dec_and_test(&desc->threads_active);
> -
> -		if (wake && waitqueue_active(&desc->wait_for_threads))
> -			wake_up(&desc->wait_for_threads);
> +		wake_threads_waitq(desc);
>  	}
>  
> -	/* Prevent a stale desc->threads_oneshot */
> -	irq_finalize_oneshot(desc, action, true);
> +	/*
> +	 * This is the regular exit path. __free_irq() is stopping the
> +	 * thread via kthread_stop() after calling
> +	 * synchronize_irq(). So neither IRQTF_RUNTHREAD nor the
> +	 * oneshot mask bit should be set.
> +	 *
> +	 * Verify that this is true.
> +	 */
> +	if (WARN_ON(test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags)))
> +		wake_threads_waitq(desc);

If we hit this warning we do not know if this IRQTF_RUNTHREAD bit's count in
desc->threads_active was decremented or not.

Nevertheless, wake_threads_waitq() gets called and desc->threads_active gets
decremented. As result, if desc->threads_active initially was decremented, we
might wrongly wake up the queue while some threaded handler is still running.

By contrast, if we choose not to wake up here, we might stuck in
synchronize_irq(). Which is probably better than a fooling synchronize_irq().

> +
> +	if (WARN_ON(desc->threads_oneshot & action->thread_mask))
> +		irq_finalize_oneshot(desc, action, true);

This check is called when the action is already removed in __free_irq() and no
desc->lock is held. Hence, a concurrent __setup_irq() could reallocate the very
same bit in the meantime. So neither irq_finalize_oneshot() nor the warning
are legitimate here.

>  
>  	/*
>  	 * Clear irq_thread. Otherwise exit_irq_thread() would make

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [tip:irq/core] genirq: Flush the irq thread on synchronization

[Thomas Gleixner]

On Thu, 15 Mar 2012, Alexander Gordeev wrote:
> On Wed, Mar 14, 2012 at 04:07:30AM -0700, tip-bot for Ido Yariv wrote:
> > +
> > +	if (WARN_ON(desc->threads_oneshot & action->thread_mask))
> > +		irq_finalize_oneshot(desc, action, true);
> 
> This check is called when the action is already removed in __free_irq() and no
> desc->lock is held. Hence, a concurrent __setup_irq() could reallocate the very
> same bit in the meantime. So neither irq_finalize_oneshot() nor the warning
> are legitimate here.

I put the WARN_ONs there for paranoia reasons, but yes, you are right
they are more confusing than helpful. We really know that everything
has been cleaned up.

Thanks,

	tglx

Re: [tip:irq/core] genirq: Flush the irq thread on synchronization

[Ido Yariv]

Hi Alexander,

On Thu, Mar 15, 2012 at 08:07:56PM +0100, Alexander Gordeev wrote:
> > -	/* Prevent a stale desc->threads_oneshot */
> > -	irq_finalize_oneshot(desc, action, true);
> > +	/*
> > +	 * This is the regular exit path. __free_irq() is stopping the
> > +	 * thread via kthread_stop() after calling
> > +	 * synchronize_irq(). So neither IRQTF_RUNTHREAD nor the
> > +	 * oneshot mask bit should be set.
> > +	 *
> > +	 * Verify that this is true.
> > +	 */
> > +	if (WARN_ON(test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags)))
> > +		wake_threads_waitq(desc);
> 
> If we hit this warning we do not know if this IRQTF_RUNTHREAD bit's count in
> desc->threads_active was decremented or not.
> 
> Nevertheless, wake_threads_waitq() gets called and desc->threads_active gets
> decremented. As result, if desc->threads_active initially was decremented, we
> might wrongly wake up the queue while some threaded handler is still running.
> 
> By contrast, if we choose not to wake up here, we might stuck in
> synchronize_irq(). Which is probably better than a fooling synchronize_irq().

AFAICT, IRQTF_RUNTHREAD and the desc->threads_active are always modified
together:
desc->threads_active is incremented if and only if IRQTF_RUNTHREAD is
set after being cleared (in irq_wake_thread()).
desc->threads_active is decremented in wake_threads_waitq(), which is
only called when IRQTF_RUNTHREAD is cleared.

It seems that if we get to this point, either IRQTF_RUNTHREAD is set and
desc->threads_active was not decremented, or it is not set.

Do you see any case where the two will be out of sync?


> > +
> > +	if (WARN_ON(desc->threads_oneshot & action->thread_mask))
> > +		irq_finalize_oneshot(desc, action, true);
> 
> This check is called when the action is already removed in __free_irq() and no
> desc->lock is held. Hence, a concurrent __setup_irq() could reallocate the very
> same bit in the meantime. So neither irq_finalize_oneshot() nor the warning
> are legitimate here.

That's interesting. However, it doesn't seem to be a regression that's
caused by this patch (the irq_finalize_oneshot() was there before), so
it might be a good idea to fix this separately.

Thanks,
Ido.

Re: [tip:irq/core] genirq: Flush the irq thread on synchronization

[Thomas Gleixner]

On Fri, 16 Mar 2012, Ido Yariv wrote:
> On Thu, Mar 15, 2012 at 08:07:56PM +0100, Alexander Gordeev wrote:
> > Nevertheless, wake_threads_waitq() gets called and desc->threads_active gets
> > decremented. As result, if desc->threads_active initially was decremented, we
> > might wrongly wake up the queue while some threaded handler is still running.
> > 
> > By contrast, if we choose not to wake up here, we might stuck in
> > synchronize_irq(). Which is probably better than a fooling synchronize_irq().
> 
> AFAICT, IRQTF_RUNTHREAD and the desc->threads_active are always modified
> together:
> desc->threads_active is incremented if and only if IRQTF_RUNTHREAD is
> set after being cleared (in irq_wake_thread()).
> desc->threads_active is decremented in wake_threads_waitq(), which is
> only called when IRQTF_RUNTHREAD is cleared.
> 
> It seems that if we get to this point, either IRQTF_RUNTHREAD is set and
> desc->threads_active was not decremented, or it is not set.
> 
> Do you see any case where the two will be out of sync?

There is really no way, that the flag is set when we come out this
regular exit path.

__free_irq()
	remove action
	synchronize_irq()
	kthread_stop()

So after the action is removed and synchronize_irq() made sure that
the thread has finished IRQTF_RUNTHREAD cannot be set. Neither can we
have a stale threads_oneshot bit set from this action.

That's a leftover of the old code and I really did not think it
through fully and kept the stuff along with paranoid warnings.

I'm going to remove all of it.

Thanks,

	tglx

[tip:irq/core] genirq: Remove paranoid warnons and bogus fixups

[tip-bot for Thomas Gleixner]

Commit-ID:  e04268b0effc0ceea366c50b3107baad9edadafa
Gitweb:     http://git.kernel.org/tip/e04268b0effc0ceea366c50b3107baad9edadafa
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 15 Mar 2012 22:55:21 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 16 Mar 2012 11:27:28 +0100

genirq: Remove paranoid warnons and bogus fixups

Alexander pointed out that the warnons in the regular exit path are
bogus and the thread_mask one actually could be triggered when
__setup_irq() hands out that thread_mask again after __free_irq()
dropped irq_desc->lock.

Thinking more about it, neither IRQTF_RUNTHREAD nor the bit in
thread_mask can be set as this is the regular exit path. We come here
due to:
	__free_irq()
	   remove action from desc
	   synchronize_irq()
	   kthread_stop()

So synchronize_irq() makes sure that the thread finished running and
cleaned up both the thread_active count and thread_mask. After that
point nothing can set IRQTF_RUNTHREAD on this action. So the warnons
and the cleanups are pointless.

Reported-by: Alexander Gordeev <agordeev@redhat.com>
Cc: Ido Yariv <ido@wizery.com>
Link: http://lkml.kernel.org/r/20120315190755.GA6732@dhcp-26-207.brq.redhat.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/irq/manage.c |   14 ++++----------
 1 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 453feed..b0ccd1a 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -804,17 +804,11 @@ static int irq_thread(void *data)
 	 * This is the regular exit path. __free_irq() is stopping the
 	 * thread via kthread_stop() after calling
 	 * synchronize_irq(). So neither IRQTF_RUNTHREAD nor the
-	 * oneshot mask bit should be set.
+	 * oneshot mask bit can be set. We cannot verify that as we
+	 * cannot touch the oneshot mask at this point anymore as
+	 * __setup_irq() might have given out currents thread_mask
+	 * again.
 	 *
-	 * Verify that this is true.
-	 */
-	if (WARN_ON(test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags)))
-		wake_threads_waitq(desc);
-
-	if (WARN_ON(desc->threads_oneshot & action->thread_mask))
-		irq_finalize_oneshot(desc, action, true);
-
-	/*
 	 * Clear irq_thread. Otherwise exit_irq_thread() would make
 	 * fuzz about an active irq thread going into nirvana.
 	 */