lib: cpu_rmap: avoid flushing all workqueues

In some cases, free_irq_cpu_rmap() is called while holding a lock (eg. rtnl). This can lead to deadlocks, because it invokes flush_scheduled_work() which ends up waiting for whole system workqueue to flush, but some pending works might try to acquire the lock we are already holding.

This commit uses reference-counting to replace
irq_run_affinity_notifiers(). It also removes
irq_run_affinity_notifiers() altogether.

Signed-off-by David Decotigny <[email protected]>

 include/linux/cpu_rmap.h  |   13 ++++--------
 include/linux/interrupt.h |    5 -----
 lib/cpu_rmap.c            |   48 +++++++++++++++++++++++++++++++++++++++------
 3 files changed, 46 insertions(+), 20 deletions(-)

include/linux/cpu_rmap.h +4 -9

index ac3bbb5..3be2813 100644
@@ -13,9 +13,11 @@
 #include <linux/cpumask.h>
 #include <linux/gfp.h>
 #include <linux/slab.h>
+#include <linux/kref.h>
 
 /**
  * struct cpu_rmap - CPU affinity reverse-map
+ * @refcount: kref for object
  * @size: Number of objects to be reverse-mapped
  * @used: Number of objects added
  * @obj: Pointer to array of object pointers

@@ -23,6 +25,7 @@
  *      based on affinity masks
  */
 struct cpu_rmap {
+	struct kref	refcount;
 	u16		size, used;
 	void		**obj;
 	struct {

@@ -33,15 +36,7 @@ struct cpu_rmap {
 #define CPU_RMAP_DIST_INF 0xffff
 
 extern struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags);
-
-/**
- * free_cpu_rmap - free CPU affinity reverse-map
- * @rmap: Reverse-map allocated with alloc_cpu_rmap(), or %NULL
- */
-static inline void free_cpu_rmap(struct cpu_rmap *rmap)
-{
-	kfree(rmap);
-}
+extern void free_cpu_rmap(struct cpu_rmap *rmap);
 
 extern int cpu_rmap_add(struct cpu_rmap *rmap, void *obj);
 extern int cpu_rmap_update(struct cpu_rmap *rmap, u16 index,

include/linux/interrupt.h +0 -5

index 5e4e617..5fa5afe 100644
@@ -268,11 +268,6 @@ struct irq_affinity_notify {
 extern int
 irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify);
 
-static inline void irq_run_affinity_notifiers(void)
-{
-	flush_scheduled_work();
-}
-
 #else /* CONFIG_SMP */
 
 static inline int irq_set_affinity(unsigned int irq, const struct cpumask *m)

lib/cpu_rmap.c +22 -2

index 145dec5..295cbf8 100644
@@ -45,6 +45,7 @@ struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags)
 	if (!rmap)
 		return NULL;
 
+	kref_init(&rmap->refcount);
 	rmap->obj = (void **)((char *)rmap + obj_offset);
 
 	/* Initially assign CPUs to objects on a rota, since we have

@@ -63,6 +64,26 @@ struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags)
 }
 EXPORT_SYMBOL(alloc_cpu_rmap);
 
+/**
+ * reclaim_cpu_rmap - internal reclaiming helper called from kref_put
+ * @ref: kref to struct cpu_rmap
+ */
+static void reclaim_cpu_rmap(struct kref *ref)
+{
+	struct cpu_rmap *rmap = container_of(ref, struct cpu_rmap, refcount);
+	kfree(rmap);
+}
+
+/**
+ * free_cpu_rmap - free CPU affinity reverse-map
+ * @rmap: Reverse-map allocated with alloc_cpu_rmap(), or %NULL
+ */
+void free_cpu_rmap(struct cpu_rmap *rmap)
+{
+	kref_put(&rmap->refcount, reclaim_cpu_rmap);
+}
+EXPORT_SYMBOL(free_cpu_rmap);
+
 /* Reevaluate nearest object for given CPU, comparing with the given
  * neighbours at the given distance.
  */

@@ -197,8 +218,7 @@ struct irq_glue {
  * free_irq_cpu_rmap - free a CPU affinity reverse-map used for IRQs
  * @rmap: Reverse-map allocated with alloc_irq_cpu_map(), or %NULL
  *
- * Must be called in process context, before freeing the IRQs, and
- * without holding any locks required by global workqueue items.
+ * Must be called in process context, before freeing the IRQs.
  */
 void free_irq_cpu_rmap(struct cpu_rmap *rmap)
 {

lib/cpu_rmap.c +9 -0

 }
 EXPORT_SYMBOL(free_irq_cpu_rmap);
 
+/**
+ * irq_cpu_rmap_notify - callback for IRQ subsystem when IRQ affinity updated
+ * @notify: struct irq_affinity_notify passed by irq/manage.c
+ * @mask: cpu mask for new SMP affinity
+ *
+ * This is executed in workqueue context.
+ */
 static void
 irq_cpu_rmap_notify(struct irq_affinity_notify *notify, const cpumask_t *mask)
 {

@@ -230,16 +256,23 @@ irq_cpu_rmap_notify(struct irq_affinity_notify *notify, const cpumask_t *mask)
 		pr_warning("irq_cpu_rmap_notify: update failed: %d\n", rc);
 }
 
+/**
+ * irq_cpu_rmap_release - reclaiming callback for IRQ subsystem

lib/cpu_rmap.c +1 -1

 }
 
 /**
  * irq_cpu_rmap_add - add an IRQ to a CPU affinity reverse-map
- * @rmap: The reverse-map
+ * @rmap: The per-IRQ reverse-map
  * @irq: The IRQ number
  *
  * This adds an IRQ affinity notifier that will update the reverse-map

lib/cpu_rmap.c +1 -0

+	}
 	return rc;
 }
 EXPORT_SYMBOL(irq_cpu_rmap_add);

Signature · 1 line

1.7.10.2.5.g20d7bc9

Quoted 11 lines · David Decotigny

In some cases, free_irq_cpu_rmap() is called while holding a lock (eg. rtnl). This can lead to deadlocks, because it invokes flush_scheduled_work() which ends up waiting for whole system workqueue to flush, but some pending works might try to acquire the lock we are already holding.

This commit uses reference-counting to replace
irq_run_affinity_notifiers(). It also removes
irq_run_affinity_notifiers() altogether.

Signed-off-by David Decotigny <[email protected]>

A couple of comments below; with those addressed,

Reviewed-by Josh Triplett <[email protected]>

Quoted 117 lines

 include/linux/cpu_rmap.h  |   13 ++++--------
 include/linux/interrupt.h |    5 -----
 lib/cpu_rmap.c            |   48 +++++++++++++++++++++++++++++++++++++++------
 3 files changed, 46 insertions(+), 20 deletions(-)

include/linux/cpu_rmap.h +4 -9

index ac3bbb5..3be2813 100644
@@ -13,9 +13,11 @@
 #include <linux/cpumask.h>
 #include <linux/gfp.h>
 #include <linux/slab.h>
+#include <linux/kref.h>
 
 /**
  * struct cpu_rmap - CPU affinity reverse-map
+ * @refcount: kref for object
  * @size: Number of objects to be reverse-mapped
  * @used: Number of objects added
  * @obj: Pointer to array of object pointers

@@ -23,6 +25,7 @@
  *      based on affinity masks
  */
 struct cpu_rmap {
+     struct kref     refcount;
      u16             size, used;
      void            **obj;
      struct {

@@ -33,15 +36,7 @@ struct cpu_rmap {
 #define CPU_RMAP_DIST_INF 0xffff
 
 extern struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags);
-
-/**
- * free_cpu_rmap - free CPU affinity reverse-map
- * @rmap: Reverse-map allocated with alloc_cpu_rmap(), or %NULL
- */
-static inline void free_cpu_rmap(struct cpu_rmap *rmap)
-{
-     kfree(rmap);
-}
+extern void free_cpu_rmap(struct cpu_rmap *rmap);
 
 extern int cpu_rmap_add(struct cpu_rmap *rmap, void *obj);
 extern int cpu_rmap_update(struct cpu_rmap *rmap, u16 index,

include/linux/interrupt.h +0 -5

index 5e4e617..5fa5afe 100644
@@ -268,11 +268,6 @@ struct irq_affinity_notify {
 extern int
 irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify);
 
-static inline void irq_run_affinity_notifiers(void)
-{
-     flush_scheduled_work();
-}
-
 #else /* CONFIG_SMP */
 
 static inline int irq_set_affinity(unsigned int irq, const struct cpumask *m)

lib/cpu_rmap.c +22 -3

index 145dec5..295cbf8 100644
@@ -45,6 +45,7 @@ struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags)
      if (!rmap)
              return NULL;
 
+     kref_init(&rmap->refcount);
      rmap->obj = (void **)((char *)rmap + obj_offset);
 
      /* Initially assign CPUs to objects on a rota, since we have

@@ -63,6 +64,26 @@ struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags)
 }
 EXPORT_SYMBOL(alloc_cpu_rmap);
 
+/**
+ * reclaim_cpu_rmap - internal reclaiming helper called from kref_put
+ * @ref: kref to struct cpu_rmap
+ */
+static void reclaim_cpu_rmap(struct kref *ref)
+{
+     struct cpu_rmap *rmap = container_of(ref, struct cpu_rmap, refcount);
+     kfree(rmap);
+}
+
+/**
+ * free_cpu_rmap - free CPU affinity reverse-map
+ * @rmap: Reverse-map allocated with alloc_cpu_rmap(), or %NULL
+ */
+void free_cpu_rmap(struct cpu_rmap *rmap)
+{
+     kref_put(&rmap->refcount, reclaim_cpu_rmap);
+}
+EXPORT_SYMBOL(free_cpu_rmap);
+
 /* Reevaluate nearest object for given CPU, comparing with the given
  * neighbours at the given distance.
  */

@@ -197,8 +218,7 @@ struct irq_glue {
  * free_irq_cpu_rmap - free a CPU affinity reverse-map used for IRQs
  * @rmap: Reverse-map allocated with alloc_irq_cpu_map(), or %NULL
  *
- * Must be called in process context, before freeing the IRQs, and
- * without holding any locks required by global workqueue items.
+ * Must be called in process context, before freeing the IRQs.
  */
 void free_irq_cpu_rmap(struct cpu_rmap *rmap)
 {

@@ -212,12 +232,18 @@ void free_irq_cpu_rmap(struct cpu_rmap *rmap)
              glue = rmap->obj[index];
              irq_set_affinity_notifier(glue->notify.irq, NULL);
      }
-     irq_run_affinity_notifiers();
 

lib/cpu_rmap.c void free_irq_cpu_rmap(struct cpu_rmap *rmap) · line 236

Quoted 2 lines

lib/cpu_rmap.c +1 -1

-     kfree(rmap);
+     kref_put(&rmap->refcount, reclaim_cpu_rmap);

This should call free_cpu_rmap, rather than duplicating its body.

Quoted 18 lines

lib/cpu_rmap.c +7 -0

 }
 EXPORT_SYMBOL(free_irq_cpu_rmap);
 
+/**
+ * irq_cpu_rmap_notify - callback for IRQ subsystem when IRQ affinity updated
+ * @notify: struct irq_affinity_notify passed by irq/manage.c
+ * @mask: cpu mask for new SMP affinity
+ *
+ * This is executed in workqueue context.
+ */
 static void
 irq_cpu_rmap_notify(struct irq_affinity_notify *notify, const cpumask_t *mask)
 {

@@ -230,16 +256,23 @@ irq_cpu_rmap_notify(struct irq_affinity_notify *notify, const cpumask_t *mask)
              pr_warning("irq_cpu_rmap_notify: update failed: %d\n", rc);
 }
 

lib/cpu_rmap.c irq_cpu_rmap_notify(struct irq_affinity_notify *notify, const cpumask_t *mask) · line 270

Quoted 12 lines

lib/cpu_rmap.c +7 -0

+/**
+ * irq_cpu_rmap_release - reclaiming callback for IRQ subsystem
+ * @ref: kref to struct irq_affinity_notify passed by irq/manage.c
+ */
 static void irq_cpu_rmap_release(struct kref *ref)
 {
      struct irq_glue *glue =
              container_of(ref, struct irq_glue, notify.kref);
+     struct cpu_rmap *rmap = glue->rmap;
+
      kfree(glue);
+     kref_put(&rmap->refcount, reclaim_cpu_rmap);

Likewise, but also, why not call free_cpu_rmap(glue->rmap) before kfree(glue) so you don't need the local copy?

Quoted 14 lines

lib/cpu_rmap.c +1 -1

 }
 
 /**
  * irq_cpu_rmap_add - add an IRQ to a CPU affinity reverse-map
- * @rmap: The reverse-map
+ * @rmap: The per-IRQ reverse-map
  * @irq: The IRQ number
  *
  * This adds an IRQ affinity notifier that will update the reverse-map

@@ -259,9 +292,12 @@ int irq_cpu_rmap_add(struct cpu_rmap *rmap, int irq)
      glue->notify.release = irq_cpu_rmap_release;
      glue->rmap = rmap;
      glue->index = cpu_rmap_add(rmap, glue);

lib/cpu_rmap.c int irq_cpu_rmap_add(struct cpu_rmap *rmap, int irq) · line 299

Quoted 6 lines

lib/cpu_rmap.c +3 -1

+     kref_get(&rmap->refcount);
      rc = irq_set_affinity_notifier(irq, &glue->notify);
-     if (rc)
+     if (rc) {
              kfree(glue);
+             kref_put(&rmap->refcount, reclaim_cpu_rmap);

Likewise.

Quoted 8 lines

lib/cpu_rmap.c +1 -1

+     }
      return rc;
 }
 EXPORT_SYMBOL(irq_cpu_rmap_add);
--

1.7.10.2.5.g20d7bc9

Thank you, Josh,

A few comments below, and the revised version shortly.

Quoted 22 lines · David Decotigny

Quoted 11 lines

In some cases, free_irq_cpu_rmap() is called while holding a lock (eg. rtnl). This can lead to deadlocks, because it invokes flush_scheduled_work() which ends up waiting for whole system workqueue to flush, but some pending works might try to acquire the lock we are already holding.

This commit uses reference-counting to replace
irq_run_affinity_notifiers(). It also removes
irq_run_affinity_notifiers() altogether.

Signed-off-by David Decotigny <[email protected]>

A couple of comments below; with those addressed,

Reviewed-by Josh Triplett <[email protected]>

Quoted 6 lines

lib/cpu_rmap.c +0 -0

@@ -230,16 +256,23 @@ irq_cpu_rmap_notify(struct irq_affinity_notify *notify, const cpumask_t *mask)
              pr_warning("irq_cpu_rmap_notify: update failed: %d\n", rc);
 }
 

Quoted 15 lines

lib/cpu_rmap.c irq_cpu_rmap_notify(struct irq_affinity_notify *notify, const cpumask_t *mask) · line 270

Quoted 12 lines

lib/cpu_rmap.c +7 -0

+/**
+ * irq_cpu_rmap_release - reclaiming callback for IRQ subsystem
+ * @ref: kref to struct irq_affinity_notify passed by irq/manage.c
+ */
 static void irq_cpu_rmap_release(struct kref *ref)
 {
      struct irq_glue *glue =
              container_of(ref, struct irq_glue, notify.kref);
+     struct cpu_rmap *rmap = glue->rmap;
+
      kfree(glue);
+     kref_put(&rmap->refcount, reclaim_cpu_rmap);

Likewise, but also, why not call free_cpu_rmap(glue->rmap) before kfree(glue) so you don't need the local copy?

I prefer to keep this kref_put here. I believe that calling something named "free_cpu_rmap" here might be misleading. It's code sharing vs. what we actually need to do, even though both are equivalent... for now.

For the order, it was deliberate, to have some kind of symmetry with kfree/kref_put in the error path we have in next function (irq_cpu_rmap_add). I reversed the order in that next function to avoid this unneeded local variable here. New ordering makes more sense anyways.

Quoted 13 lines

Quoted 13 lines

lib/cpu_rmap.c +1 -1

 }
 
 /**
  * irq_cpu_rmap_add - add an IRQ to a CPU affinity reverse-map
- * @rmap: The reverse-map
+ * @rmap: The per-IRQ reverse-map
  * @irq: The IRQ number
  *
  * This adds an IRQ affinity notifier that will update the reverse-map

@@ -259,9 +292,12 @@ int irq_cpu_rmap_add(struct cpu_rmap *rmap, int irq)
      glue->notify.release = irq_cpu_rmap_release;
      glue->rmap = rmap;
      glue->index = cpu_rmap_add(rmap, glue);

Quoted 8 lines

lib/cpu_rmap.c int irq_cpu_rmap_add(struct cpu_rmap *rmap, int irq) · line 299

Quoted 6 lines

lib/cpu_rmap.c +3 -1

+     kref_get(&rmap->refcount);
      rc = irq_set_affinity_notifier(irq, &glue->notify);
-     if (rc)
+     if (rc) {
              kfree(glue);
+             kref_put(&rmap->refcount, reclaim_cpu_rmap);

Likewise.

I prefer to leave the explicit kref_put here too.

Next version soon, after some re-testing.