* [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
* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.