fix might sleep oops in irq affinity callback hook

Fix might_sleep oops in the irq affinity notifier logic.

Bug was due to the generic support function irq_set_affinity() invoking schedule_work() underneath a raw spin lock. Our workaround is to move the schedule_work() down a few lines to where the lock has just been dropped. This may not be ideal, as the resultant code is a bit ugly.

This bug was discovered when the sfc networking driver was loaded. Perusal of the kernel source shows that the mellanox and infiniband drivers are also likely to suffer from this problem.

(applies to 3.6.11.6-rt38, appears to be applicable to all later rt's and I expect it is valid for all earlier rt's as well).

Index: b/kernel/irq/manage.c
===================================================================
@@ -163,7 +163,7 @@ int irq_do_set_affinity(struct irq_data 
 	return ret;
 }
 
-int __irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask)
+static int __irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask)
 {
 	struct irq_chip *chip = irq_data_get_irq_chip(data);
 	struct irq_desc *desc = irq_data_to_desc(data);

@@ -179,10 +179,6 @@ int __irq_set_affinity_locked(struct irq
 		irq_copy_pending(desc, mask);
 	}
 
-	if (desc->affinity_notify) {
-		kref_get(&desc->affinity_notify->kref);
-		schedule_work(&desc->affinity_notify->work);
-	}
 	irqd_set(data, IRQD_AFFINITY_SET);
 
 	return ret;

@@ -205,7 +201,14 @@ int irq_set_affinity(unsigned int irq, c
 
 	raw_spin_lock_irqsave(&desc->lock, flags);
 	ret =  __irq_set_affinity_locked(irq_desc_get_irq_data(desc), mask);
-	raw_spin_unlock_irqrestore(&desc->lock, flags);
+
+	if (!ret && desc->affinity_notify) {
+		kref_get(&desc->affinity_notify->kref);
+		raw_spin_unlock_irqrestore(&desc->lock, flags);
+		schedule_work(&desc->affinity_notify->work);
+	} else
+		raw_spin_unlock_irqrestore(&desc->lock, flags);
+
 	return ret;
 }
 

Index: b/include/linux/irq.h
===================================================================
@@ -381,7 +381,6 @@ extern void remove_percpu_irq(unsigned i
 
 extern void irq_cpu_online(void);
 extern void irq_cpu_offline(void);
-extern int __irq_set_affinity_locked(struct irq_data *data,  const struct cpumask *cpumask);
 
 #ifdef CONFIG_GENERIC_HARDIRQS
 

Joe could you please test this one? In 3.10 we have one user of __irq_set_affinity_locked() and we might get more.

Steven: in the longterm we might want a helper for this sort of things? I remember the MCE had the same problem.

---
 include/linux/interrupt.h |  1 +
 kernel/irq/manage.c       | 79 +++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 77 insertions(+), 3 deletions(-)

index 11bdb1e..838d4bd 100644
@@ -263,6 +263,7 @@ struct irq_affinity_notify {
 	unsigned int irq;
 	struct kref kref;
 	struct work_struct work;
+	struct list_head list;
 	void (*notify)(struct irq_affinity_notify *, const cpumask_t *mask);
 	void (*release)(struct kref *ref);
 };

1.8.4.rc3

Hi Sebastian,
I backported the patch to 3.6.11.6-rt38 and added a few printk's so that when daemon started I would see that and when each affinity hook was executed I would see that too.

The system came up and the printk for daemon start showed up near where the sfc driver started, but no affinity action printk appeared. Thus the patch as written does not seem to work.

Joe

Hi Sebastian,
Today I intrumented both the nort and rt paths through the affinity hook logic, and built and booted both nort and rt kernels. Here is what I found:

For the nort kernel, each time I change the affinity of one of the SFC driver IRQs listed in /proc/interrupts, I get a notice in dmesg that the SFC callback was invoked.

For the rt kernel, I get the message only for the first time the affinity for some SFC IRQ is changed. Subsequent changes of the _same_ IRQ do not have the callback invoked. If I go and change some other SFC IRQ that will also invoke the callback the first time only.

finny# echo 1 >/proc/irq/46/smp_affinity; dmesg | fgrep genirq
genirq: set_affinity_thread STARTED
genirq: NOTIFIER CALLED ...
finny# echo 1 >/proc/irq/46/smp_affinity; dmesg | fgrep genirq
genirq: set_affinity_thread STARTED
genirq: NOTIFIER CALLED ...
finny# echo 1 >/proc/irq/46/smp_affinity; dmesg | fgrep genirq
genirq: set_affinity_thread STARTED
genirq: NOTIFIER CALLED ...
finny# echo 2 >/proc/irq/46/smp_affinity; dmesg | fgrep genirq
genirq: set_affinity_thread STARTED
genirq: NOTIFIER CALLED ...
finny# echo 2 >/proc/irq/47/smp_affinity; dmesg | fgrep genirq
genirq: set_affinity_thread STARTED
genirq: NOTIFIER CALLED ...
genirq: NOTIFIER CALLED ...
finny# echo 2 >/proc/irq/47/smp_affinity; dmesg | fgrep genirq
genirq: set_affinity_thread STARTED
genirq: NOTIFIER CALLED ...
genirq: NOTIFIER CALLED ...
finny# echo 1 >/proc/irq/47/smp_affinity; dmesg | fgrep genirq
genirq: set_affinity_thread STARTED
genirq: NOTIFIER CALLED ...
genirq: NOTIFIER CALLED ...

Regards,
Joe

Hi Joe,

it seems the thread does not get woken up second invocation. I try to check this somehow here.

Sebastian

Hi Joe,

I forget to remove that item from the list so your observing was correct. Thanks for testing.
I'm going to tae that one, as it seems to work:

@@ -261,6 +261,7 @@ struct irq_affinity_notify {
 	unsigned int irq;
 	struct kref kref;
 	struct work_struct work;
+	struct list_head list;
 	void (*notify)(struct irq_affinity_notify *, const cpumask_t *mask);
 	void (*release)(struct kref *ref);
 };

Sebastian

Hi Sebastian,
I backported the new version to 3.6.11.6-rt38, it works perfectly. Thanks!

Joe