lib: cpu_rmap: avoid flushing all workqueues

PATCH Superseded David Decotigny 3 participants 5 messages Dec 28, 2012 – Jan 2, 2013 1 message not in the archive Library code

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

David Decotigny

author

2012-12-28 19:03 UTC #1 lore raw

PATCH v2 1/1 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            |   47 +++++++++++++++++++++++++++++++++++++++------
 3 files changed, 45 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 +41 -6

98 lines not on this page · raw message

Signature · 1 line

1.7.10.2.5.g20d7bc9

Eric Dumazet

2012-12-28 21:14 UTC #2 lore raw

Quoted 87 lines · David Decotigny

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            |   47 +++++++++++++++++++++++++++++++++++++++------
 3 files changed, 45 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 +1 -0

index 145dec5..bb5d0af 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

lib/cpu_rmap.c struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags) · line 71

Quoted 8 lines

lib/cpu_rmap.c +5 -0

@@ -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)

Could be named cpu_rmap_free()

(and alloc_cpu_rmap() should be renamed as cpu_rmap_alloc()

lib/cpu_rmap.c struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags) · line 81

Quoted 10 lines

lib/cpu_rmap.c +10 -0

+{
+	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)

This could be named : cpu_rmap_put()

lib/cpu_rmap.c struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags) · line 85

Quoted 4 lines

lib/cpu_rmap.c +4 -0

+{
+	kref_put(&rmap->refcount, reclaim_cpu_rmap);
+}
+EXPORT_SYMBOL(free_cpu_rmap);

It seems this function could be static, and not exported.

Quoted 39 lines

lib/cpu_rmap.c +10 -4

+
 /* 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();
 
-	kfree(rmap);
+	free_cpu_rmap(rmap);
 }
 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,22 @@ 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 268

Quoted 10 lines

lib/cpu_rmap.c +6 -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);
+
+	kref_put(&glue->rmap->refcount, reclaim_cpu_rmap);

	cpu_rmap_put(glue->rmap);

Quoted 11 lines

lib/cpu_rmap.c +1 -1

 	kfree(glue);
 }
 
 /**
  * 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 int irq_cpu_rmap_add(struct cpu_rmap *rmap, int irq) · line 297

Quoted 8 lines

lib/cpu_rmap.c +3 -1

@@ -259,9 +291,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);
+	kref_get(&rmap->refcount);
 	rc = irq_set_affinity_notifier(irq, &glue->notify);
-	if (rc)
+	if (rc) {
+		kref_put(&rmap->refcount, reclaim_cpu_rmap);

		cpu_rmap_put(rmap);

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

Quoted 5 lines

lib/cpu_rmap.c +1 -0

 		kfree(glue);
+	}
 	return rc;
 }
 EXPORT_SYMBOL(irq_cpu_rmap_add);

I personally don't like kref abstraction, especially when used without wrapper :

kref_put(&rmap->refcount, reclaim_cpu_rmap);

Its cleaner to have instead :

cpu_rmap_put(rmap);

David Decotigny

author

2012-12-28 21:44 UTC #3 lore raw

Thanks,

Ok for the cpu_rmap_put helper. Will do this in v3 of this patch.

Your comments suggest more refactoring, which might be better in the form of 1 or 2 additional patches that:
- rename alloc_cpu_rmap & co according to new conventions (cpu_rmap_*)
- remove the cpu_rmap sub-API altogether, keeping only irq_cpu_rmap
(controversial?)

I'd rather current patch is integrated in its own as it fixes an actual bug. I will send the other patch(es) separately later.

v3 for this patch coming soon.

Quoted 213 lines · Eric Dumazet

Quoted 87 lines · David Decotigny

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            |   47 +++++++++++++++++++++++++++++++++++++++------
 3 files changed, 45 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 +1 -0

index 145dec5..bb5d0af 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

lib/cpu_rmap.c struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags) · line 71

Quoted 8 lines

lib/cpu_rmap.c +5 -0

@@ -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)

Could be named cpu_rmap_free()

(and alloc_cpu_rmap() should be renamed as cpu_rmap_alloc()

lib/cpu_rmap.c struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags) · line 81

Quoted 10 lines

lib/cpu_rmap.c +10 -0

+{
+     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)

This could be named : cpu_rmap_put()

lib/cpu_rmap.c struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags) · line 85

Quoted 4 lines

lib/cpu_rmap.c +4 -0

+{
+     kref_put(&rmap->refcount, reclaim_cpu_rmap);
+}
+EXPORT_SYMBOL(free_cpu_rmap);

It seems this function could be static, and not exported.

Quoted 39 lines

lib/cpu_rmap.c +10 -4

+
 /* 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();
 
-     kfree(rmap);
+     free_cpu_rmap(rmap);
 }
 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,22 @@ 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 268

Quoted 10 lines

lib/cpu_rmap.c +6 -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);
+
+     kref_put(&glue->rmap->refcount, reclaim_cpu_rmap);

        cpu_rmap_put(glue->rmap);

Quoted 11 lines

lib/cpu_rmap.c +1 -1

      kfree(glue);
 }
 
 /**
  * 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 int irq_cpu_rmap_add(struct cpu_rmap *rmap, int irq) · line 297

Quoted 8 lines

lib/cpu_rmap.c +3 -1

@@ -259,9 +291,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);
+     kref_get(&rmap->refcount);
      rc = irq_set_affinity_notifier(irq, &glue->notify);
-     if (rc)
+     if (rc) {
+             kref_put(&rmap->refcount, reclaim_cpu_rmap);

                cpu_rmap_put(rmap);

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

Quoted 5 lines

lib/cpu_rmap.c +1 -0

              kfree(glue);
+     }
      return rc;
 }
 EXPORT_SYMBOL(irq_cpu_rmap_add);

I personally don't like kref abstraction, especially when used without wrapper :

kref_put(&rmap->refcount, reclaim_cpu_rmap);

Its cleaner to have instead :

cpu_rmap_put(rmap);

Eric Dumazet

2012-12-28 21:52 UTC #4 lore raw

Quoted 4 lines · David Decotigny

Thanks,
Ok for the cpu_rmap_put helper. Will do this in v3 of this patch.

Quoted 8 lines

Your comments suggest more refactoring, which might be better in the form of 1 or 2 additional patches that:
- rename alloc_cpu_rmap & co according to new conventions (cpu_rmap_*)
- remove the cpu_rmap sub-API altogether, keeping only irq_cpu_rmap
(controversial?)
I'd rather current patch is integrated in its own as it fixes an actual bug. I will send the other patch(es) separately later.

This looks fine to me !

Ben Hutchings

2013-01-02 20:33 UTC #5 lore raw

In reply to Eric Dumazet

On Fri, 2012-12-28 at 13:14 -0800, Eric Dumazet wrote:
[...]

Quoted 9 lines

lib/cpu_rmap.c struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags) · line 71
Quoted 5 lines
lib/cpu_rmap.c +5 -0
+/**
+ * reclaim_cpu_rmap - internal reclaiming helper called from kref_put
+ * @ref: kref to struct cpu_rmap
+ */
+static void reclaim_cpu_rmap(struct kref *ref)
Could be named cpu_rmap_free()
(and alloc_cpu_rmap() should be renamed as cpu_rmap_alloc()

[...]

There are many other APIs that include alloc_foo() and free_foo(). I'm not aware of any convention that this should be avoided in new APIs (not that this is new any more).

Ben.

Signature · 3 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.