2009-07-22 14:57:08

by Thomas Gleixner

[permalink] [raw]
Subject: [GIT pull] genirq fixes for 2.6.31

Linus,

Please pull the latest irq-fixes-for-linus git tree from:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git irq-fixes-for-linus

The affinity problem went unnoticed in reviews and testing and just
exploded nicely in the preempt-rt tree where we force thread all
interrupts.

Thanks,

tglx

------------------>
Thomas Gleixner (1):
genirq: Delegate irq affinity setting to the irq thread


include/linux/interrupt.h | 2 +
kernel/irq/internals.h | 3 +-
kernel/irq/manage.c | 50 +++++++++++++++++++++++++++++++++++++++-----
kernel/irq/migration.c | 2 +-
4 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 2721f07..88b056a 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -64,11 +64,13 @@
* 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
*/
enum {
IRQTF_RUNTHREAD,
IRQTF_DIED,
IRQTF_WARNED,
+ IRQTF_AFFINITY,
};

typedef irqreturn_t (*irq_handler_t)(int, void *);
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 7346825..e70ed55 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -42,8 +42,7 @@ static inline void unregister_handler_proc(unsigned int irq,

extern int irq_select_affinity_usr(unsigned int irq);

-extern void
-irq_set_thread_affinity(struct irq_desc *desc, const struct cpumask *cpumask);
+extern void irq_set_thread_affinity(struct irq_desc *desc);

/*
* Debugging printout:
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 50da676..f0de36f 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -80,14 +80,22 @@ int irq_can_set_affinity(unsigned int irq)
return 1;
}

-void
-irq_set_thread_affinity(struct irq_desc *desc, const struct cpumask *cpumask)
+/**
+ * irq_set_thread_affinity - Notify irq threads to adjust affinity
+ * @desc: irq descriptor which has affitnity changed
+ *
+ * We just set IRQTF_AFFINITY and delegate the affinity setting
+ * to the interrupt thread itself. We can not call
+ * set_cpus_allowed_ptr() here as we hold desc->lock and this
+ * code can be called from hard interrupt context.
+ */
+void irq_set_thread_affinity(struct irq_desc *desc)
{
struct irqaction *action = desc->action;

while (action) {
if (action->thread)
- set_cpus_allowed_ptr(action->thread, cpumask);
+ set_bit(IRQTF_AFFINITY, &action->thread_flags);
action = action->next;
}
}
@@ -112,7 +120,7 @@ int irq_set_affinity(unsigned int irq, const struct cpumask *cpumask)
if (desc->status & IRQ_MOVE_PCNTXT) {
if (!desc->chip->set_affinity(irq, cpumask)) {
cpumask_copy(desc->affinity, cpumask);
- irq_set_thread_affinity(desc, cpumask);
+ irq_set_thread_affinity(desc);
}
}
else {
@@ -122,7 +130,7 @@ int irq_set_affinity(unsigned int irq, const struct cpumask *cpumask)
#else
if (!desc->chip->set_affinity(irq, cpumask)) {
cpumask_copy(desc->affinity, cpumask);
- irq_set_thread_affinity(desc, cpumask);
+ irq_set_thread_affinity(desc);
}
#endif
desc->status |= IRQ_AFFINITY_SET;
@@ -176,7 +184,7 @@ int irq_select_affinity_usr(unsigned int irq)
spin_lock_irqsave(&desc->lock, flags);
ret = setup_affinity(irq, desc);
if (!ret)
- irq_set_thread_affinity(desc, desc->affinity);
+ irq_set_thread_affinity(desc);
spin_unlock_irqrestore(&desc->lock, flags);

return ret;
@@ -444,6 +452,34 @@ static int irq_wait_for_interrupt(struct irqaction *action)
}

/*
+ * Check whether we need to change the affinity of the interrupt thread.
+ */
+static void
+irq_thread_check_affinity(struct irq_desc *desc, struct irqaction *action)
+{
+ cpumask_var_t mask;
+
+ if (!test_and_clear_bit(IRQTF_AFFINITY, &action->thread_flags))
+ return;
+
+ /*
+ * In case we are out of memory we set IRQTF_AFFINITY again and
+ * try again next time
+ */
+ if (!alloc_cpumask_var(&mask, GFP_KERNEL)) {
+ set_bit(IRQTF_AFFINITY, &action->thread_flags);
+ return;
+ }
+
+ spin_lock_irq(&desc->lock);
+ cpumask_copy(mask, desc->affinity);
+ spin_unlock_irq(&desc->lock);
+
+ set_cpus_allowed_ptr(current, mask);
+ free_cpumask_var(mask);
+}
+
+/*
* Interrupt handler thread
*/
static int irq_thread(void *data)
@@ -458,6 +494,8 @@ static int irq_thread(void *data)

while (!irq_wait_for_interrupt(action)) {

+ irq_thread_check_affinity(desc, action);
+
atomic_inc(&desc->threads_active);

spin_lock_irq(&desc->lock);
diff --git a/kernel/irq/migration.c b/kernel/irq/migration.c
index cfe767c..fcb6c96 100644
--- a/kernel/irq/migration.c
+++ b/kernel/irq/migration.c
@@ -45,7 +45,7 @@ void move_masked_irq(int irq)
< nr_cpu_ids))
if (!desc->chip->set_affinity(irq, desc->pending_mask)) {
cpumask_copy(desc->affinity, desc->pending_mask);
- irq_set_thread_affinity(desc, desc->pending_mask);
+ irq_set_thread_affinity(desc);
}

cpumask_clear(desc->pending_mask);


2009-07-22 23:28:39

by Kevin Winchester

[permalink] [raw]
Subject: Re: [GIT pull] genirq fixes for 2.6.31

Thomas Gleixner wrote:
> +/*
> * Interrupt handler thread
> */
> static int irq_thread(void *data)
> @@ -458,6 +494,8 @@ static int irq_thread(void *data)
>
> while (!irq_wait_for_interrupt(action)) {
>
> + irq_thread_check_affinity(desc, action);
> +
> atomic_inc(&desc->threads_active);
>
> spin_lock_irq(&desc->lock);

Any chance we could do this in a way that doesn't break the build for CONFIG_SMP=n? :)

Should this call simply be wrapped in an #ifdef, or should the function be defined for !SMP?

--
Kevin Winchester

2009-07-22 23:33:42

by Kevin Winchester

[permalink] [raw]
Subject: Re: [GIT pull] genirq fixes for 2.6.31

Kevin Winchester wrote:
> Thomas Gleixner wrote:
>> +/*
>> * Interrupt handler thread
>> */
>> static int irq_thread(void *data)
>> @@ -458,6 +494,8 @@ static int irq_thread(void *data)
>>
>> while (!irq_wait_for_interrupt(action)) {
>>
>> + irq_thread_check_affinity(desc, action);
>> +
>> atomic_inc(&desc->threads_active);
>>
>> spin_lock_irq(&desc->lock);
>
> Any chance we could do this in a way that doesn't break the build for CONFIG_SMP=n? :)
>
> Should this call simply be wrapped in an #ifdef, or should the function be defined for !SMP?
>

Actually, I guess the function is defined always, but it references desc->affinity unconditionally.

Perhaps something like the following (likely whitespace damaged) patch:

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f0de36f..353e335 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -451,6 +451,8 @@ static int irq_wait_for_interrupt(struct irqaction *action)
return -1;
}

+#ifdef CONFIG_SMP
+
/*
* Check whether we need to change the affinity of the interrupt thread.
*/
@@ -479,6 +481,15 @@ irq_thread_check_affinity(struct irq_desc *desc, struct irqaction *action)
free_cpumask_var(mask);
}

+#else
+
+static void
+irq_thread_check_affinity(struct irq_desc *desc, struct irqaction *action)
+{
+}
+
+#endif
+
/*
* Interrupt handler thread
*/

2009-07-23 00:01:13

by Kevin Winchester

[permalink] [raw]
Subject: [PATCH] irq: Add compiling definition of irq_thread_check_affinity for !CONFIG_SMP


The addition and use of irq_thread_check_affinity neglected the fact
that struct irq_desc does not have an affinity member when CONFIG_SMP=n.

Fix this by adding an empty definition of the method for !SMP.

Signed-off-by: Kevin Winchester <[email protected]>
---

I'll try not to be lazy and send a real patch. Please let me know if
I've done it wrong.

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f0de36f..82f01c8 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -190,11 +190,46 @@ int irq_select_affinity_usr(unsigned int irq)
return ret;
}

+/*
+ * Check whether we need to change the affinity of the interrupt thread.
+ */
+static void
+irq_thread_check_affinity(struct irq_desc *desc, struct irqaction *action)
+{
+ cpumask_var_t mask;
+
+ if (!test_and_clear_bit(IRQTF_AFFINITY, &action->thread_flags))
+ return;
+
+ /*
+ * In case we are out of memory we set IRQTF_AFFINITY again and
+ * try again next time
+ */
+ if (!alloc_cpumask_var(&mask, GFP_KERNEL)) {
+ set_bit(IRQTF_AFFINITY, &action->thread_flags);
+ return;
+ }
+
+ spin_lock_irq(&desc->lock);
+ cpumask_copy(mask, desc->affinity);
+ spin_unlock_irq(&desc->lock);
+
+ set_cpus_allowed_ptr(current, mask);
+ free_cpumask_var(mask);
+}
+
#else
+
static inline int setup_affinity(unsigned int irq, struct irq_desc *desc)
{
return 0;
}
+
+static void
+irq_thread_check_affinity(struct irq_desc *desc, struct irqaction *action)
+{
+}
+
#endif

void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
@@ -452,34 +487,6 @@ static int irq_wait_for_interrupt(struct irqaction *action)
}

/*
- * Check whether we need to change the affinity of the interrupt thread.
- */
-static void
-irq_thread_check_affinity(struct irq_desc *desc, struct irqaction *action)
-{
- cpumask_var_t mask;
-
- if (!test_and_clear_bit(IRQTF_AFFINITY, &action->thread_flags))
- return;
-
- /*
- * In case we are out of memory we set IRQTF_AFFINITY again and
- * try again next time
- */
- if (!alloc_cpumask_var(&mask, GFP_KERNEL)) {
- set_bit(IRQTF_AFFINITY, &action->thread_flags);
- return;
- }
-
- spin_lock_irq(&desc->lock);
- cpumask_copy(mask, desc->affinity);
- spin_unlock_irq(&desc->lock);
-
- set_cpus_allowed_ptr(current, mask);
- free_cpumask_var(mask);
-}
-
-/*
* Interrupt handler thread
*/
static int irq_thread(void *data)