Skip to contentSkip to conversation

genirq: support multiple IRQ notifier.

PATCH In review Weongyo Jeong 4 participants 8 messages Mar 25, 2016 – Mar 28, 2016

Subsystems (5)
SCSI Library code InfiniBand IRQ +1 more subsystems
lore
Share & tools
lore

Weongyo Jeong

author
#0 lore raw

cover genirq: support multiple IRQ notifier.

Each irq_desc only supports one IRQ affinity notifier at current implementation so when we try to register another notifier, it silently unregister previous entry and register new one.

However the problem is that if CONFIG_RFS_ACCEL is set, at current implementation no way to set additional IRQ affinity notifier for some NIC cards RFS enabled because it already used for RFS. With this patch we can register multiple IRQ affinity notifiers.

Weongyo Jeong (2):
genirq: clean up for irq_set_affinity_notifier().
genirq: support multiple IRQ notifier.

 drivers/infiniband/hw/qib/qib_iba7322.c | 10 ++--
 drivers/scsi/qla2xxx/qla_isr.c          |  4 +-
 include/linux/interrupt.h               | 13 +++++-
 include/linux/irqdesc.h                 |  2 +-
 kernel/irq/irqdesc.c                    |  1 +
 kernel/irq/manage.c                     | 82 ++++++++++++++++++++++-----------
 lib/cpu_rmap.c                          |  4 +-
 7 files changed, 78 insertions(+), 38 deletions(-)
Signature · 6 lines
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#1 lore raw

PATCH 1/2 genirq: clean up for irq_set_affinity_notifier().

At current irq_set_affinity_notifier() implementation, it has two meaning; set and clear notify. To be more clear, separate it into two; irq_set_affinity_notifier() and irq_del_affinity_notifier().

At irq_set_affinity_notifier() function, no longer might_sleep() function is called unnecessarily and -EEXIST code is returned.

Signed-off-by Weongyo Jeong <[email protected]>
---
 drivers/infiniband/hw/qib/qib_iba7322.c |  8 ++---
 drivers/scsi/qla2xxx/qla_isr.c          |  2 +-
 include/linux/interrupt.h               |  2 ++
 kernel/irq/manage.c                     | 60 ++++++++++++++++++++++++---------
 lib/cpu_rmap.c                          |  2 +-
 5 files changed, 52 insertions(+), 22 deletions(-)
drivers/infiniband/hw/qib/qib_iba7322.c +4 -4
index 6c8ff10..58c482a 100644
@@ -3355,10 +3355,10 @@ static void reset_dca_notifier(struct qib_devdata *dd, struct qib_msix_entry *m)
 		"Disabling notifier on HCA %d irq %d\n",
 		dd->unit,
 		m->msix.vector);
-	irq_set_affinity_notifier(
-		m->msix.vector,
-		NULL);
-	m->notifier = NULL;
+	if (m->notifier != NULL) {
+		irq_del_affinity_notifier(&m->notifier->notify);
+		m->notifier = NULL;
+	}
 }
 
 static void setup_dca_notifier(struct qib_devdata *dd, struct qib_msix_entry *m)
drivers/scsi/qla2xxx/qla_isr.c +1 -1
index 5649c20..0a652fa 100644
@@ -3013,7 +3013,7 @@ qla24xx_disable_msix(struct qla_hw_data *ha)
 		qentry = &ha->msix_entries[i];
 		if (qentry->have_irq) {
 			/* un-register irq cpu affinity notification */
-			irq_set_affinity_notifier(qentry->vector, NULL);
+			irq_del_affinity_notifier(&qentry->irq_notify);
 			free_irq(qentry->vector, qentry->rsp);
 		}
 	}
include/linux/interrupt.h +2 -0
index 358076e..fc54356 100644
@@ -277,6 +277,8 @@ extern int irq_set_affinity_hint(unsigned int irq, const struct cpumask *m);
 
 extern int
 irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify);
+extern int
+irq_del_affinity_notifier(struct irq_affinity_notify *notify);
 
 #else /* CONFIG_SMP */
kernel/irq/manage.c +44 -16

90 lines not on this page · raw message

lib/cpu_rmap.c +1 -1
index f610b2a..0412a51 100644
@@ -235,7 +235,7 @@ void free_irq_cpu_rmap(struct cpu_rmap *rmap)
 
 	for (index = 0; index < rmap->used; index++) {
 		glue = rmap->obj[index];
-		irq_set_affinity_notifier(glue->notify.irq, NULL);
+		irq_del_affinity_notifier(&glue->notify);
 	}
 
 	cpu_rmap_put(rmap);
Signature · 6 lines
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#2 lore raw

PATCH 2/2 genirq: support multiple IRQ notifier.

In reply to Weongyo Jeong

At current implementation, it only supports one IRQ notifier for irq_desc. When if we try to register another IRQ notifier, it automatically un-register previous one and register new one. With this patch, multiple IRQ notifier is supported.

Signed-off-by Weongyo Jeong <[email protected]>
---
 drivers/infiniband/hw/qib/qib_iba7322.c |  2 +-
 drivers/scsi/qla2xxx/qla_isr.c          |  2 +-
 include/linux/interrupt.h               | 11 ++++++++--
 include/linux/irqdesc.h                 |  2 +-
 kernel/irq/irqdesc.c                    |  1 +
 kernel/irq/manage.c                     | 38 +++++++++++++++++----------------
 lib/cpu_rmap.c                          |  2 +-
 7 files changed, 34 insertions(+), 24 deletions(-)
drivers/infiniband/hw/qib/qib_iba7322.c +1 -1
index 58c482a..5376592 100644
@@ -3380,7 +3380,7 @@ static void setup_dca_notifier(struct qib_devdata *dd, struct qib_msix_entry *m)
 		qib_devinfo(dd->pcidev,
 			"set notifier irq %d rcv %d notify %p\n",
 			n->notify.irq, n->rcv, &n->notify);
-		ret = irq_set_affinity_notifier(
+		ret = irq_add_affinity_notifier(
 				n->notify.irq,
 				&n->notify);
 		if (ret) {
drivers/scsi/qla2xxx/qla_isr.c +1 -1
index 0a652fa..13318c0 100644
@@ -3101,7 +3101,7 @@ qla24xx_enable_msix(struct qla_hw_data *ha, struct rsp_que *rsp)
 		rsp->msix = qentry;
 
 		/* Register for CPU affinity notification. */
-		irq_set_affinity_notifier(qentry->vector, &qentry->irq_notify);
+		irq_add_affinity_notifier(qentry->vector, &qentry->irq_notify);
 
 		/* Schedule work (ie. trigger a notification) to read cpu
 		 * mask for this specific irq.
include/linux/interrupt.h +9 -2
index fc54356..d7368ab 100644
@@ -230,6 +230,7 @@ struct irq_affinity_notify {
 	struct work_struct work;
 	void (*notify)(struct irq_affinity_notify *, const cpumask_t *mask);
 	void (*release)(struct kref *ref);
+	struct list_head list;
 };
 
 #if defined(CONFIG_SMP)

@@ -276,7 +277,7 @@ extern int irq_select_affinity(unsigned int irq);
 extern int irq_set_affinity_hint(unsigned int irq, const struct cpumask *m);
 
 extern int
-irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify);
+irq_add_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify);
 extern int
 irq_del_affinity_notifier(struct irq_affinity_notify *notify);
 

@@ -306,7 +307,13 @@ static inline int irq_set_affinity_hint(unsigned int irq,
 }
 
 static inline int
-irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify)
+irq_add_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify)
+{
+	return 0;
+}
+
+static inline int
+irq_del_affinity_notifier(struct irq_affinity_notify *notify)
 {
 	return 0;
 }
include/linux/irqdesc.h +1 -1
index dcca77c..51cc163 100644
@@ -68,7 +68,7 @@ struct irq_desc {
 	struct cpumask		*percpu_enabled;
 #ifdef CONFIG_SMP
 	const struct cpumask	*affinity_hint;
-	struct irq_affinity_notify *affinity_notify;
+	struct list_head	affinity_notify_list;
 #ifdef CONFIG_GENERIC_PENDING_IRQ
 	cpumask_var_t		pending_mask;
 #endif
kernel/irq/irqdesc.c +1 -0
index 0ccd028..4f99fb3 100644
@@ -70,6 +70,7 @@ static int alloc_masks(struct irq_desc *desc, gfp_t gfp, int node)
 
 static void desc_smp_init(struct irq_desc *desc, int node)
 {
+	INIT_LIST_HEAD(&desc->affinity_notify_list);
 	cpumask_copy(desc->irq_common_data.affinity, irq_default_affinity);
 #ifdef CONFIG_GENERIC_PENDING_IRQ
 	cpumask_clear(desc->pending_mask);
kernel/irq/manage.c +20 -18
index 6fb1414..bfd7673 100644
@@ -202,6 +202,7 @@ int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
 int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
 			    bool force)
 {
+	struct irq_affinity_notify *notify;
 	struct irq_chip *chip = irq_data_get_irq_chip(data);
 	struct irq_desc *desc = irq_data_to_desc(data);
 	int ret = 0;

@@ -216,9 +217,9 @@ int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
 		irq_copy_pending(desc, mask);
 	}
 
-	if (desc->affinity_notify) {
-		kref_get(&desc->affinity_notify->kref);
-		schedule_work(&desc->affinity_notify->work);
+	list_for_each_entry(notify, &desc->affinity_notify_list, list) {
+		kref_get(&notify->kref);
+		schedule_work(&notify->work);
 	}
 	irqd_set(data, IRQD_AFFINITY_SET);
 

@@ -282,7 +283,7 @@ out:
 }
 
 /**
- *	irq_set_affinity_notifier - set notification of IRQ affinity changes
+ *	irq_add_affinity_notifier - set notification of IRQ affinity changes
  *	@irq:		Interrupt for which to enable notification
  *	@notify:	Context for notification.
  *			Function pointers must be initialised;

@@ -291,7 +292,7 @@ out:
  *	Notification may only be enabled after the IRQ is allocated.
  */
 int
-irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify)
+irq_add_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
 	unsigned long flags;

@@ -302,20 +303,16 @@ irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify)
 		return -EINVAL;
 
 	raw_spin_lock_irqsave(&desc->lock, flags);
-	if (desc->affinity_notify != NULL) {
-		raw_spin_unlock_irqrestore(&desc->lock, flags);
-		return -EEXIST;
-	}
 	notify->irq = irq;
 	kref_init(&notify->kref);
 	INIT_WORK(&notify->work, irq_affinity_notify);
 
-	desc->affinity_notify = notify;
+	list_add(&notify->list, &desc->affinity_notify_list);
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(irq_set_affinity_notifier);
+EXPORT_SYMBOL_GPL(irq_add_affinity_notifier);
 
 /**
  *	irq_del_affinity_notifier - delete notification of IRQ affinity changes

@@ -328,7 +325,6 @@ int
 irq_del_affinity_notifier(struct irq_affinity_notify *notify)
 {
 	struct irq_desc *desc;
-	struct irq_affinity_notify *old_notify;
 	unsigned long flags;
 
 	/* The release function is promised process context */

@@ -341,12 +337,10 @@ irq_del_affinity_notifier(struct irq_affinity_notify *notify)
 		return -EINVAL;
 
 	raw_spin_lock_irqsave(&desc->lock, flags);
-	old_notify = desc->affinity_notify;
-	desc->affinity_notify = NULL;
+	list_del(&notify->list);
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 
-	if (old_notify)
-		kref_put(&old_notify->kref, old_notify->release);
+	kref_put(&notify->kref, notify->release);
 
 	return 0;
 }

@@ -1571,14 +1565,22 @@ EXPORT_SYMBOL_GPL(remove_irq);
  */
 void free_irq(unsigned int irq, void *dev_id)
 {
+#ifdef CONFIG_SMP
+	struct irq_affinity_notify *notify, *notifytmp;
+#endif
 	struct irq_desc *desc = irq_to_desc(irq);
 
 	if (!desc || WARN_ON(irq_settings_is_per_cpu_devid(desc)))
 		return;
 
 #ifdef CONFIG_SMP
-	if (WARN_ON(desc->affinity_notify))
-		desc->affinity_notify = NULL;
+	list_for_each_entry_safe(notify, notifytmp, &desc->affinity_notify_list,
+				 list) {
+		if (notify->irq != irq)
+			continue;
+		__WARN();
+		list_del(&notify->list);
+	}
 #endif
 
 	kfree(__free_irq(irq, dev_id));
lib/cpu_rmap.c +1 -1
index 0412a51..fb3a8e4 100644
@@ -297,7 +297,7 @@ int irq_cpu_rmap_add(struct cpu_rmap *rmap, int irq)
 	glue->rmap = rmap;
 	cpu_rmap_get(rmap);
 	glue->index = cpu_rmap_add(rmap, glue);
-	rc = irq_set_affinity_notifier(irq, &glue->notify);
+	rc = irq_add_affinity_notifier(irq, &glue->notify);
 	if (rc) {
 		cpu_rmap_put(glue->rmap);
 		kfree(glue);
Signature · 1 line
2.1.3
build bot
#3 lore raw
url: https://github.com/0day-ci/linux/commits/Weongyo-Jeong/genirq-support-multiple-IRQ-notifier/20160325-235708 reproduce: make htmldocs

Hi Weongyo,

[auto build test WARNING on tip/irq/core]
[also build test WARNING on v4.5 next-20160324]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url: https://github.com/0day-ci/linux/commits/Weongyo-Jeong/genirq-support-multiple-IRQ-notifier/20160325-235708
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

>> include/linux/interrupt.h:224: warning: No description found for parameter 'list'

kernel/irq/handle.c:1: warning: no structured comments found

vim +/list +224 include/linux/interrupt.h

f0ba3d05c Eyal Perry         2014-05-20  208   * @kref:		Reference count, for internal use
f0ba3d05c Eyal Perry         2014-05-20  209   * @work:		Work item, for internal use
f0ba3d05c Eyal Perry         2014-05-20  210   * @notify:		Function to be called on change.  This will be
f0ba3d05c Eyal Perry         2014-05-20  211   *			called in process context.
f0ba3d05c Eyal Perry         2014-05-20  212   * @release:		Function to be called on release.  This will be
f0ba3d05c Eyal Perry         2014-05-20  213   *			called in process context.  Once registered, the
f0ba3d05c Eyal Perry         2014-05-20  214   *			structure must only be freed when this function is
f0ba3d05c Eyal Perry         2014-05-20  215   *			called or later.
f0ba3d05c Eyal Perry         2014-05-20  216   */
f0ba3d05c Eyal Perry         2014-05-20  217  struct irq_affinity_notify {
f0ba3d05c Eyal Perry         2014-05-20  218  	unsigned int irq;
f0ba3d05c Eyal Perry         2014-05-20  219  	struct kref kref;
f0ba3d05c Eyal Perry         2014-05-20  220  	struct work_struct work;
f0ba3d05c Eyal Perry         2014-05-20  221  	void (*notify)(struct irq_affinity_notify *, const cpumask_t *mask);
f0ba3d05c Eyal Perry         2014-05-20  222  	void (*release)(struct kref *ref);
3fb671e48 Weongyo Jeong      2016-03-25  223  	struct list_head list;
f0ba3d05c Eyal Perry         2014-05-20 @224  };
f0ba3d05c Eyal Perry         2014-05-20  225  
0244ad004 Martin Schwidefsky 2013-08-30  226  #if defined(CONFIG_SMP)
d7b906897 Russell King       2008-04-17  227  
d036e67b4 Rusty Russell      2009-01-01  228  extern cpumask_var_t irq_default_affinity;
184047567 Max Krasnyansky    2008-05-29  229  
01f8fa4f0 Thomas Gleixner    2014-04-16  230  /* Internal implementation. Use the helpers below */
01f8fa4f0 Thomas Gleixner    2014-04-16  231  extern int __irq_set_affinity(unsigned int irq, const struct cpumask *cpumask,
01f8fa4f0 Thomas Gleixner    2014-04-16  232  			      bool force);

:::::: The code at line 224 was first introduced by commit
:::::: f0ba3d05c9c647ab42ed6a0dbdfdeae42bfbd6de genirq: Provide !SMP stub for irq_set_affinity_notifier()

:::::: TO: Eyal Perry <[email protected]>
:::::: CC: Thomas Gleixner <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

build bot
#4 lore raw
In reply to Weongyo Jeong
reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree…

Hi Weongyo,

[auto build test ERROR on tip/irq/core]
[also build test ERROR on v4.5 next-20160324]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Weongyo-Jeong/genirq-support-multiple-IRQ-notifier/20160325-235708
config: arm64-allnoconfig (attached as .config)

reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64

All errors (new ones prefixed by >>):

kernel/irq/manage.c: In function 'free_irq':

>> kernel/irq/manage.c:1581:3: error: implicit declaration of function '__WARN' [-Werror=implicit-function-declaration]
      __WARN();
      ^

cc1: some warnings being treated as errors

vim +/__WARN +1581 kernel/irq/manage.c

1575
1576 #ifdef CONFIG_SMP
1577 list_for_each_entry_safe(notify, notifytmp, &desc->affinity_notify_list,
1578 list) {
1579 if (notify->irq != irq)
1580 continue;

> 1581			__WARN();

1582 list_del(&notify->list);
1583 }
1584 #endif

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

#5 lore raw
In reply to Weongyo Jeong
Quoted 8 lines · Weongyo Jeong

Each irq_desc only supports one IRQ affinity notifier at current implementation so when we try to register another notifier, it silently unregister previous entry and register new one.

However the problem is that if CONFIG_RFS_ACCEL is set, at current implementation no way to set additional IRQ affinity notifier for some NIC cards RFS enabled because it already used for RFS. With this patch we can register multiple IRQ affinity notifiers.

The whole concept of these irq affinity notifiers seems wrong to me.

If a device supports MSI-X it should simply request per-cpu or per-node vectors and we should prevent affinity changes for them.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#6 lore raw
Quoted 10 lines · Christoph Hellwig
Quoted 8 lines · Weongyo Jeong

Each irq_desc only supports one IRQ affinity notifier at current implementation so when we try to register another notifier, it silently unregister previous entry and register new one.

However the problem is that if CONFIG_RFS_ACCEL is set, at current implementation no way to set additional IRQ affinity notifier for some NIC cards RFS enabled because it already used for RFS. With this patch we can register multiple IRQ affinity notifiers.

Quoted 4 lines

The whole concept of these irq affinity notifiers seems wrong to me.

If a device supports MSI-X it should simply request per-cpu or per-node vectors and we should prevent affinity changes for them.

You beat me to it. I'm having a half baken prototype to implement this at the core level. Will post next week.

Thanks,

tglx

author
#7 lore raw
In reply to Christoph Hellwig
Quoted 10 lines · Christoph Hellwig
Quoted 8 lines · Weongyo Jeong

Each irq_desc only supports one IRQ affinity notifier at current implementation so when we try to register another notifier, it silently unregister previous entry and register new one.

However the problem is that if CONFIG_RFS_ACCEL is set, at current implementation no way to set additional IRQ affinity notifier for some NIC cards RFS enabled because it already used for RFS. With this patch we can register multiple IRQ affinity notifiers.

Quoted 4 lines

The whole concept of these irq affinity notifiers seems wrong to me.

If a device supports MSI-X it should simply request per-cpu or per-node vectors and we should prevent affinity changes for them.

This could be a silly question. Are you meaning that we should remove feature of IRQ affinity notifiers and device writer should explicitly set CPU affinity with masking when it requests IRQ?

And then some CPU affinity of IRQ are still changable via /proc/irq/<n>/smp_affinity and some aren't for device drivers, right?

Regards,
Weongyo Jeong
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html