* [PATCH 4/4] genirq: Get rid of unnecessary IRQTF_DIED flag
@ 2012-03-09 13:59 Alexander Gordeev
2012-03-09 15:06 ` Oleg Nesterov
2012-03-09 21:13 ` [tip:irq/core] " tip-bot for Alexander Gordeev
0 siblings, 2 replies; 6+ messages in thread
From: Alexander Gordeev @ 2012-03-09 13:59 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: Oleg Nesterov, linux-kernel
Currently IRQTF_DIED flag is set when a IRQ thread handler calls do_exit()
But also PF_EXITING per process flag gets set when a thread exits. This
fix eliminates the duplicate by using PF_EXITING flag.
Also, there is a race condition in exit_irq_thread(). In case a thread's
bit is cleared in desc->threads_oneshot (and the IRQ line gets unmasked),
but before IRQTF_DIED flag is set, a new interrupt might come in and set
just cleared bit again, this time forever. This fix throws IRQTF_DIED flag
away, eliminating the race as a result.
Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
kernel/exit.c | 4 ++--
kernel/irq/handle.c | 4 ++--
kernel/irq/internals.h | 2 --
kernel/irq/manage.c | 11 +----------
4 files changed, 5 insertions(+), 16 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index 4b4042f..752d2c0 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -935,8 +935,6 @@ void do_exit(long code)
schedule();
}
- exit_irq_thread();
-
exit_signals(tsk); /* sets PF_EXITING */
/*
* tsk->flags are checked in the futex code to protect against
@@ -945,6 +943,8 @@ void do_exit(long code)
smp_mb();
raw_spin_unlock_wait(&tsk->pi_lock);
+ exit_irq_thread();
+
if (unlikely(in_atomic()))
printk(KERN_INFO "note: %s[%d] exited with preempt_count %d\n",
current->comm, task_pid_nr(current),
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 470d08c..42a32ec 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -60,8 +60,8 @@ static void irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
* device interrupt, so no irq storm is lurking. If the
* RUNTHREAD bit is already set, nothing to do.
*/
- if (test_bit(IRQTF_DIED, &action->thread_flags) ||
- test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags))
+ if (test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags) ||
+ (action->thread->flags & PF_EXITING))
return;
/*
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index b795231..5c233e0 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -20,14 +20,12 @@ extern bool noirqdebug;
/*
* Bits used by threaded handlers:
* IRQTF_RUNTHREAD - signals that the interrupt handler thread should run
- * IRQTF_DIED - handler thread died
* IRQTF_WARNED - warning "IRQ_WAKE_THREAD w/o thread_fn" has been printed
* IRQTF_AFFINITY - irq thread is requested to adjust affinity
* IRQTF_FORCED_THREAD - irq action is force threaded
*/
enum {
IRQTF_RUNTHREAD,
- IRQTF_DIED,
IRQTF_WARNED,
IRQTF_AFFINITY,
IRQTF_FORCED_THREAD,
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 3feab4a..a94466d 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -845,17 +845,8 @@ void exit_irq_thread(void)
desc = irq_to_desc(action->irq);
- /*
- * Prevent a stale desc->threads_oneshot. Must be called
- * before setting the IRQTF_DIED flag.
- */
+ /* Prevent a stale desc->threads_oneshot */
irq_finalize_oneshot(desc, action, true);
-
- /*
- * Set the THREAD DIED flag to prevent further wakeups of the
- * soon to be gone threaded handler.
- */
- set_bit(IRQTF_DIED, &action->flags);
}
static void irq_setup_forced_threading(struct irqaction *new)
--
1.7.7.6
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 4/4] genirq: Get rid of unnecessary IRQTF_DIED flag
2012-03-09 13:59 [PATCH 4/4] genirq: Get rid of unnecessary IRQTF_DIED flag Alexander Gordeev
@ 2012-03-09 15:06 ` Oleg Nesterov
2012-03-09 16:17 ` Thomas Gleixner
2012-03-09 21:13 ` [tip:irq/core] " tip-bot for Alexander Gordeev
1 sibling, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2012-03-09 15:06 UTC (permalink / raw)
To: Alexander Gordeev; +Cc: Thomas Gleixner, linux-kernel
Of course I can't ack this, but afaics the whole series looks fine.
Only one minor nit,
On 03/09, Alexander Gordeev wrote:
>
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -60,8 +60,8 @@ static void irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
> * device interrupt, so no irq storm is lurking. If the
> * RUNTHREAD bit is already set, nothing to do.
> */
> - if (test_bit(IRQTF_DIED, &action->thread_flags) ||
> - test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags))
> + if (test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags) ||
> + (action->thread->flags & PF_EXITING))
> return;
perhaps it makes sense to check PF_EXITING first, we do not want
to set IRQTF_RUNTHREAD in this case. I think this doesn't really
matter (and the check is obviously racy anyway), just looks a bit
confusing.
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/4] genirq: Get rid of unnecessary IRQTF_DIED flag
2012-03-09 15:06 ` Oleg Nesterov
@ 2012-03-09 16:17 ` Thomas Gleixner
2012-03-12 11:38 ` Alexander Gordeev
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Gleixner @ 2012-03-09 16:17 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Alexander Gordeev, linux-kernel
On Fri, 9 Mar 2012, Oleg Nesterov wrote:
> Of course I can't ack this, but afaics the whole series looks fine.
>
> Only one minor nit,
>
> On 03/09, Alexander Gordeev wrote:
> >
> > --- a/kernel/irq/handle.c
> > +++ b/kernel/irq/handle.c
> > @@ -60,8 +60,8 @@ static void irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
> > * device interrupt, so no irq storm is lurking. If the
> > * RUNTHREAD bit is already set, nothing to do.
> > */
> > - if (test_bit(IRQTF_DIED, &action->thread_flags) ||
> > - test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags))
> > + if (test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags) ||
> > + (action->thread->flags & PF_EXITING))
> > return;
>
> perhaps it makes sense to check PF_EXITING first, we do not want
> to set IRQTF_RUNTHREAD in this case. I think this doesn't really
> matter (and the check is obviously racy anyway), just looks a bit
> confusing.
It does not matter, the thread cleans it up in the exit path. So it's
mostly cosmetic, but you are right, it reads better :)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/4] genirq: Get rid of unnecessary IRQTF_DIED flag
2012-03-09 16:17 ` Thomas Gleixner
@ 2012-03-12 11:38 ` Alexander Gordeev
2012-03-12 15:10 ` Alexander Gordeev
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Gordeev @ 2012-03-12 11:38 UTC (permalink / raw)
To: Thomas Gleixner, Oleg Nesterov; +Cc: linux-kernel
On Fri, Mar 09, 2012 at 05:17:16PM +0100, Thomas Gleixner wrote:
> On Fri, 9 Mar 2012, Oleg Nesterov wrote:
>
> > Of course I can't ack this, but afaics the whole series looks fine.
> >
> > Only one minor nit,
> >
> > On 03/09, Alexander Gordeev wrote:
> > >
> > > --- a/kernel/irq/handle.c
> > > +++ b/kernel/irq/handle.c
> > > @@ -60,8 +60,8 @@ static void irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
> > > * device interrupt, so no irq storm is lurking. If the
> > > * RUNTHREAD bit is already set, nothing to do.
> > > */
> > > - if (test_bit(IRQTF_DIED, &action->thread_flags) ||
> > > - test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags))
> > > + if (test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags) ||
> > > + (action->thread->flags & PF_EXITING))
> > > return;
> >
> > perhaps it makes sense to check PF_EXITING first, we do not want
> > to set IRQTF_RUNTHREAD in this case. I think this doesn't really
> > matter (and the check is obviously racy anyway), just looks a bit
> > confusing.
>
> It does not matter, the thread cleans it up in the exit path. So it's
> mostly cosmetic, but you are right, it reads better :)
Oleg, Thomas,
I swapped the checks because I wanted to avoid this scenario:
CPU1 CPU2
do_exit()
exit_signals(tsk); /* sets PF_EXITING */
smp_mb(); <--------------------------------+
exit_irq_thread(); |
irq_finalize_oneshot(); |
desc->threads_oneshot &= ~action->thread_mask;
unmask_irq(desc); |
|
/* Once the irq is unmasked new interrupt can come... */ |
|
irq_wake_thread() |
|
/* To notice PF_EXITING has changed a call to smp_rmb() should be here |
* to pair with smp_mb() in do_exit() ---------------------------------+
* But it is not, and the condition might not fulfill.
* Hence the thread might not return, although it should.
*/
if ((action->thread->flags & PF_EXITING) ||
test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags))
return;
...
/* And would be stalled bit will be mistakenly set */
desc->threads_oneshot |= action->thread_mask;
Given that PF_EXITING check almost never fulfills but always gets executed and
test_and_set_bit() is a smp general barrier I thought swapping of PF_EXITING vs
IRQTF_RUNTHREAD checks is better than putting explicit smp_rmb().
If my considerations are flawed I will repost the patch with the original
order as it indeed reads better :)
--
Regards,
Alexander Gordeev
agordeev@redhat.com
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 4/4] genirq: Get rid of unnecessary IRQTF_DIED flag
2012-03-12 11:38 ` Alexander Gordeev
@ 2012-03-12 15:10 ` Alexander Gordeev
0 siblings, 0 replies; 6+ messages in thread
From: Alexander Gordeev @ 2012-03-12 15:10 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: Oleg Nesterov, linux-kernel
On Mon, Mar 12, 2012 at 12:38:30PM +0100, Alexander Gordeev wrote:
> If my considerations are flawed I will repost the patch with the original
> order as it indeed reads better :)
Thomas, please nevermind my last note.
--
Regards,
Alexander Gordeev
agordeev@redhat.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:irq/core] genirq: Get rid of unnecessary IRQTF_DIED flag
2012-03-09 13:59 [PATCH 4/4] genirq: Get rid of unnecessary IRQTF_DIED flag Alexander Gordeev
2012-03-09 15:06 ` Oleg Nesterov
@ 2012-03-09 21:13 ` tip-bot for Alexander Gordeev
1 sibling, 0 replies; 6+ messages in thread
From: tip-bot for Alexander Gordeev @ 2012-03-09 21:13 UTC (permalink / raw)
To: linux-tip-commits; +Cc: linux-kernel, agordeev, hpa, mingo, oleg, tglx
Commit-ID: 5234ffb9f74cfa8993d174782bc861dd9b7b5bfb
Gitweb: http://git.kernel.org/tip/5234ffb9f74cfa8993d174782bc861dd9b7b5bfb
Author: Alexander Gordeev <agordeev@redhat.com>
AuthorDate: Fri, 9 Mar 2012 14:59:59 +0100
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 9 Mar 2012 17:19:09 +0100
genirq: Get rid of unnecessary IRQTF_DIED flag
Currently IRQTF_DIED flag is set when a IRQ thread handler calls do_exit()
But also PF_EXITING per process flag gets set when a thread exits. This
fix eliminates the duplicate by using PF_EXITING flag.
Also, there is a race condition in exit_irq_thread(). In case a thread's
bit is cleared in desc->threads_oneshot (and the IRQ line gets unmasked),
but before IRQTF_DIED flag is set, a new interrupt might come in and set
just cleared bit again, this time forever. This fix throws IRQTF_DIED flag
away, eliminating the race as a result.
[ tglx: Test THREAD_EXITING first as suggested by Oleg ]
Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Link: http://lkml.kernel.org/r/20120309135958.GD2114@dhcp-26-207.brq.redhat.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
kernel/exit.c | 4 ++--
kernel/irq/handle.c | 2 +-
kernel/irq/internals.h | 2 --
kernel/irq/manage.c | 11 +----------
4 files changed, 4 insertions(+), 15 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index 4b4042f..752d2c0 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -935,8 +935,6 @@ void do_exit(long code)
schedule();
}
- exit_irq_thread();
-
exit_signals(tsk); /* sets PF_EXITING */
/*
* tsk->flags are checked in the futex code to protect against
@@ -945,6 +943,8 @@ void do_exit(long code)
smp_mb();
raw_spin_unlock_wait(&tsk->pi_lock);
+ exit_irq_thread();
+
if (unlikely(in_atomic()))
printk(KERN_INFO "note: %s[%d] exited with preempt_count %d\n",
current->comm, task_pid_nr(current),
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 470d08c..500aaf6 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -60,7 +60,7 @@ static void irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
* device interrupt, so no irq storm is lurking. If the
* RUNTHREAD bit is already set, nothing to do.
*/
- if (test_bit(IRQTF_DIED, &action->thread_flags) ||
+ if ((action->thread->flags & PF_EXITING) ||
test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags))
return;
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index b795231..5c233e0 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -20,14 +20,12 @@ extern bool noirqdebug;
/*
* Bits used by threaded handlers:
* IRQTF_RUNTHREAD - signals that the interrupt handler thread should run
- * IRQTF_DIED - handler thread died
* IRQTF_WARNED - warning "IRQ_WAKE_THREAD w/o thread_fn" has been printed
* IRQTF_AFFINITY - irq thread is requested to adjust affinity
* IRQTF_FORCED_THREAD - irq action is force threaded
*/
enum {
IRQTF_RUNTHREAD,
- IRQTF_DIED,
IRQTF_WARNED,
IRQTF_AFFINITY,
IRQTF_FORCED_THREAD,
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 3feab4a..a94466d 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -845,17 +845,8 @@ void exit_irq_thread(void)
desc = irq_to_desc(action->irq);
- /*
- * Prevent a stale desc->threads_oneshot. Must be called
- * before setting the IRQTF_DIED flag.
- */
+ /* Prevent a stale desc->threads_oneshot */
irq_finalize_oneshot(desc, action, true);
-
- /*
- * Set the THREAD DIED flag to prevent further wakeups of the
- * soon to be gone threaded handler.
- */
- set_bit(IRQTF_DIED, &action->flags);
}
static void irq_setup_forced_threading(struct irqaction *new)
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-03-12 15:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-09 13:59 [PATCH 4/4] genirq: Get rid of unnecessary IRQTF_DIED flag Alexander Gordeev
2012-03-09 15:06 ` Oleg Nesterov
2012-03-09 16:17 ` Thomas Gleixner
2012-03-12 11:38 ` Alexander Gordeev
2012-03-12 15:10 ` Alexander Gordeev
2012-03-09 21:13 ` [tip:irq/core] " tip-bot for Alexander Gordeev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox