lib: cpu_rmap: avoid flushing all workqueues

PATCH In review David Decotigny 6 participants 13 messages Jan 2, 2013 – Jan 8, 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

2013-01-02 21:52 UTC #1 lore raw

PATCH v4 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            |   63 +++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 62 insertions(+), 19 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 +58 -5

110 lines not on this page · raw message

Signature · 1 line

1.7.10.2.5.g20d7bc9

Ben Hutchings

Reviewed-by on v4 1/1

2013-01-02 21:54 UTC #2 lore raw

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]>

[...]

Reviewed-by Ben Hutchings <[email protected]>

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.

Eric Dumazet

Acked-by on v4 1/1

2013-01-02 22:05 UTC #3 lore raw

In reply to David Decotigny

Quoted 6 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.

Quoted 6 lines

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]>
---

Acked-by Eric Dumazet <[email protected]>

Josh Triplett

Reviewed-by on v4 1/1

2013-01-02 22:20 UTC #4 lore raw

In reply to David Decotigny

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]>

Reviewed-by Josh Triplett <[email protected]>

Quoted 186 lines

---
 include/linux/cpu_rmap.h  |   13 +++-------
 include/linux/interrupt.h |    5 ----
 lib/cpu_rmap.c            |   63 +++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 62 insertions(+), 19 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 +58 -5

index 145dec5..bcd1f0e 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,44 @@ struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags)
 }
 EXPORT_SYMBOL(alloc_cpu_rmap);
 
+/**
+ * cpu_rmap_reclaim - internal reclaiming helper called from kref_put
+ * @ref: kref to struct cpu_rmap
+ */
+static void cpu_rmap_reclaim(struct kref *ref)
+{
+	struct cpu_rmap *rmap = container_of(ref, struct cpu_rmap, refcount);
+	kfree(rmap);
+}
+
+/**
+ * cpu_rmap_get - internal helper to get new ref on a cpu_rmap
+ * @rmap: reverse-map allocated with alloc_cpu_rmap()
+ */
+static inline void cpu_rmap_get(struct cpu_rmap *rmap)
+{
+	kref_get(&rmap->refcount);
+}
+
+/**
+ * cpu_rmap_put - internal helper to release ref on a cpu_rmap
+ * @rmap: reverse-map allocated with alloc_cpu_rmap()
+ */
+static inline void cpu_rmap_put(struct cpu_rmap *rmap)
+{
+	kref_put(&rmap->refcount, cpu_rmap_reclaim);
+}
+
+/**
+ * free_cpu_rmap - free CPU affinity reverse-map
+ * @rmap: Reverse-map allocated with alloc_cpu_rmap()
+ */
+void free_cpu_rmap(struct cpu_rmap *rmap)
+{
+	cpu_rmap_put(rmap);
+}
+EXPORT_SYMBOL(free_cpu_rmap);
+
 /* Reevaluate nearest object for given CPU, comparing with the given
  * neighbours at the given distance.
  */

@@ -197,8 +236,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 +250,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);
+	cpu_rmap_put(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,10 +274,16 @@ 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
+ * @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);
+
+	cpu_rmap_put(glue->rmap);
 	kfree(glue);
 }

@@ -258,10 +308,13 @@ int irq_cpu_rmap_add(struct cpu_rmap *rmap, int irq)
 	glue->notify.notify = irq_cpu_rmap_notify;
 	glue->notify.release = irq_cpu_rmap_release;
 	glue->rmap = rmap;
+	cpu_rmap_get(rmap);
 	glue->index = cpu_rmap_add(rmap, glue);
 	rc = irq_set_affinity_notifier(irq, &glue->notify);
-	if (rc)
+	if (rc) {
+		cpu_rmap_put(glue->rmap);
 		kfree(glue);
+	}
 	return rc;
 }
 EXPORT_SYMBOL(irq_cpu_rmap_add);

Signature · 1 line

1.7.10.2.5.g20d7bc9

Andrew Morton

maintainer

2013-01-02 23:12 UTC #5 lore raw

In reply to David Decotigny

Quoted 9 lines · On Wed, 2 Jan 2013 13:52:25 -0800 David Decotigny <[email protected]> wrote:

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.

I can't say that I've ever noticed cpu_rmap.c before :( Is is too late to review it?

- The naming is chaotic. At least these:

	EXPORT_SYMBOL(alloc_cpu_rmap);
	EXPORT_SYMBOL(free_cpu_rmap);
	EXPORT_SYMBOL(cpu_rmap_add);
	EXPORT_SYMBOL(cpu_rmap_update);
	EXPORT_SYMBOL(free_irq_cpu_rmap);
	EXPORT_SYMBOL(irq_cpu_rmap_add);

should be consistently named cpu_rmap_foo()

- What's the locking model? It appears to be caller-provided, but it is undocumented.

drivers/net/ethernet/mellanox/mlx4/ appears to be using msix_ctl.pool_lock for exclusion, but I didn't check for coverage.

drivers/net/ethernet/sfc/efx.c seems to not need locking because all its cpu_rmap operations are at module_init() time.

The cpu_rmap code would be less of a hand grenade if each of its interface functions documented the caller's locking requirements.

As for this patch: there's no cc:stable here but it does appear that the problem is sufficiently serious to justify a backport, agree?

Quoted 9 lines

include/linux/cpu_rmap.h +0 -0

...

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

include/linux/cpu_rmap.h struct cpu_rmap { · line 39

Quoted 10 lines

include/linux/cpu_rmap.h +1 -9

-
-/**
- * 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);

Can we do away with free_cpu_rmap() altogether? It is a misleading name - it is a put() function, not a free() function. It would be clearer (not to mention faster and smaller) to change all call sites to directly call cpu_rmap_put().

Quoted 8 lines

include/linux/cpu_rmap.h +0 -0

 extern int cpu_rmap_add(struct cpu_rmap *rmap, void *obj);
 extern int cpu_rmap_update(struct cpu_rmap *rmap, u16 index,
 
...

@@ -63,6 +64,44 @@ struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags)
 }
 EXPORT_SYMBOL(alloc_cpu_rmap);

include/linux/cpu_rmap.h struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags) · line 75

Quoted 10 lines

include/linux/cpu_rmap.h +9 -0

 
+/**
+ * cpu_rmap_reclaim - internal reclaiming helper called from kref_put
+ * @ref: kref to struct cpu_rmap
+ */
+static void cpu_rmap_reclaim(struct kref *ref)
+{
+	struct cpu_rmap *rmap = container_of(ref, struct cpu_rmap, refcount);
+	kfree(rmap);
+}

I suggest this be renamed to cpu_rmap_release(). As "release" is the conventional term for a kref release handler.

include/linux/cpu_rmap.h

Quoted 11 lines

...

include/linux/cpu_rmap.h +8 -0

+/**
+ * cpu_rmap_put - internal helper to release ref on a cpu_rmap
+ * @rmap: reverse-map allocated with alloc_cpu_rmap()
+ */
+static inline void cpu_rmap_put(struct cpu_rmap *rmap)
+{
+	kref_put(&rmap->refcount, cpu_rmap_reclaim);
+}

As mentioned, I suggest this become the public interface. And I suppose it should propagate kref_put()'s return value, in case someone is interested.

include/linux/cpu_rmap.h

Quoted 9 lines

include/linux/cpu_rmap.h +9 -0

+/**
+ * free_cpu_rmap - free CPU affinity reverse-map
+ * @rmap: Reverse-map allocated with alloc_cpu_rmap()
+ */
+void free_cpu_rmap(struct cpu_rmap *rmap)
+{
+	cpu_rmap_put(rmap);
+}
+EXPORT_SYMBOL(free_cpu_rmap);

zap.

Quoted 6 lines

/* Reevaluate nearest object for given CPU, comparing with the given
* neighbours at the given distance.
*/
...

Ben Hutchings

2013-01-02 23:46 UTC #6 lore raw

Quoted 18 lines · Andrew Morton

Quoted 9 lines · On Wed, 2 Jan 2013 13:52:25 -0800 David Decotigny <[email protected]> wrote:
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.
I can't say that I've ever noticed cpu_rmap.c before :( Is is too late to review it?
- The naming is chaotic. At least these:

Quoted 8 lines

	EXPORT_SYMBOL(alloc_cpu_rmap);
	EXPORT_SYMBOL(free_cpu_rmap);
	EXPORT_SYMBOL(cpu_rmap_add);
	EXPORT_SYMBOL(cpu_rmap_update);
	EXPORT_SYMBOL(free_irq_cpu_rmap);
	EXPORT_SYMBOL(irq_cpu_rmap_add);

should be consistently named cpu_rmap_foo()

There is a common practice of defining alloc_foo() and free_foo() alongside foo_do_this() and foo_do_that(). I deliberately chose to follow that. If this is deprecated then it should be documented somewhere.

There is also a separation between functions that are specific to IRQ affinity (last 2) and those that are not (first 4).

Quoted 2 lines

- What's the locking model? It appears to be caller-provided, but it is undocumented.

I think caller-provided can be assumed as the default for library code. And IRQ setup and teardown need to be properly serialised in the driver already.

Quoted 8 lines

drivers/net/ethernet/mellanox/mlx4/ appears to be using msix_ctl.pool_lock for exclusion, but I didn't check for coverage.
drivers/net/ethernet/sfc/efx.c seems to not need locking because all its cpu_rmap operations are at module_init() time.
The cpu_rmap code would be less of a hand grenade if each of its interface functions documented the caller's locking requirements.

This particular 'hand grenade' *was* documented. So I don't think documentation is the problem.

Quoted 2 lines

As for this patch: there's no cc:stable here but it does appear that the problem is sufficiently serious to justify a backport, agree?

[...]

Not sure. So far as I can see, nothing called free_irq_cpu_rmap() while holding the RTNL lock before v3.8-rc1. If there can be work items on a global workqueue that lock a PCI device (perhaps EEH?) then stable versions may also be affected.

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.

Andrew Morton

maintainer

2013-01-03 21:58 UTC #7 lore raw

Quoted 26 lines · On Wed, 2 Jan 2013 23:46:46 +0000 Ben Hutchings <[email protected]>…

Quoted 25 lines · Andrew Morton
Quoted 9 lines · On Wed, 2 Jan 2013 13:52:25 -0800 David Decotigny <[email protected]> wrote:
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.
I can't say that I've ever noticed cpu_rmap.c before :( Is is too late to review it?
- The naming is chaotic. At least these:
	EXPORT_SYMBOL(alloc_cpu_rmap);
	EXPORT_SYMBOL(free_cpu_rmap);
	EXPORT_SYMBOL(cpu_rmap_add);
	EXPORT_SYMBOL(cpu_rmap_update);
	EXPORT_SYMBOL(free_irq_cpu_rmap);
	EXPORT_SYMBOL(irq_cpu_rmap_add);

Quoted 6 lines

Quoted 1 line
should be consistently named cpu_rmap_foo()
There is a common practice of defining alloc_foo() and free_foo() alongside foo_do_this() and foo_do_that(). I deliberately chose to follow that. If this is deprecated then it should be documented somewhere.

I don't think anyone has thought about it to that extent. I always recommend that the exported identifiers be named as subsysid_functionname() and cannot think of any reason for special-casing alloc and free.

Quoted 4 lines

Quoted 2 lines
- What's the locking model? It appears to be caller-provided, but it is undocumented.
I think caller-provided can be assumed as the default for library code.

Nope, a lot of library code does internal locking. And boy, does that cause problems! Experience tells us that caller-provided locking is better. But to avoid nasty problems, the library should clearly document its locking requirements!

Bear in mind that spinlocks and mutexes aren't the only form of locks. A caller may wish to use an rwsem or rwlock, either to get parallelism in cpu_rmap_lookup_index() and cpu_rmap_lookup_obj(), or because they were already using such a lock. The cpu_rmap() locking documentation should describe which interface calls are OK with read-side locking. Not only to instruct users, but also to act as a constraint upon future developers of the cpu_rmap code. It becomes a contract saying "if you use read_lock() for this, we won't later break your stuff".

Quoted 6 lines

And IRQ setup and teardown need to be properly serialised in the driver already.
Quoted 3 lines
drivers/net/ethernet/mellanox/mlx4/ appears to be using msix_ctl.pool_lock for exclusion, but I didn't check for coverage.

Quoted 8 lines

Quoted 5 lines
drivers/net/ethernet/sfc/efx.c seems to not need locking because all its cpu_rmap operations are at module_init() time.
The cpu_rmap code would be less of a hand grenade if each of its interface functions documented the caller's locking requirements.
This particular 'hand grenade' *was* documented. So I don't think documentation is the problem.

Dunno what you're referring to here. There is no cpu_rmap() locking documentation.

Quoted 8 lines

Quoted 2 lines
As for this patch: there's no cc:stable here but it does appear that the problem is sufficiently serious to justify a backport, agree?
[...]
Not sure. So far as I can see, nothing called free_irq_cpu_rmap() while holding the RTNL lock before v3.8-rc1. If there can be work items on a global workqueue that lock a PCI device (perhaps EEH?) then stable versions may also be affected.

OK. The patch is rather non-trivial so I guess we aim for 3.8-only for now.

Ben Hutchings

2013-01-08 16:05 UTC #8 lore raw

Quoted 2 lines · Andrew Morton

On Wed, 2 Jan 2013 23:46:46 +0000
Ben Hutchings <[email protected]> wrote:

[...]

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
drivers/net/ethernet/mellanox/mlx4/ appears to be using msix_ctl.pool_lock for exclusion, but I didn't check for coverage.
drivers/net/ethernet/sfc/efx.c seems to not need locking because all its cpu_rmap operations are at module_init() time.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
The cpu_rmap code would be less of a hand grenade if each of its interface functions documented the caller's locking requirements.
This particular 'hand grenade' *was* documented. So I don't think documentation is the problem.
Dunno what you're referring to here. There is no cpu_rmap() locking documentation.

[...]

/**
* 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. */

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.

David Decotigny

author

2013-01-03 02:35 UTC #9 lore raw

In reply to Andrew Morton

Thanks. It is not too late to review this code. But I'd prefer not to address refactoring issues with this patch, which is supposed to fix a deadlock with some drivers. So I'd rather not to add function renaming, suppressions, etc. that have an effect outside cpu_rmap's code. Instead, I'd like to propose another patch later, which will be a little more intrusive in that respect, if that's ok with you.

I believe Ben answered your other concerns, I consider him as the expert as to whether there should be finer-grained locking implemented in this subsystem. Let me just add that I second him in saying that the deadlock risk was clearly identified and mentioned in the doc. Unfortunately, initial implementation makes this risk hard to work-around for some drivers, which is what this patch proposes to address.

So, for now, I'd like to keep v4 as the current version. And some refactoring will be done in a later patch.

Regards,

--
David Decotigny

Quoted 126 lines · Andrew Morton

Quoted 9 lines · On Wed, 2 Jan 2013 13:52:25 -0800 David Decotigny <[email protected]> wrote:
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.
I can't say that I've ever noticed cpu_rmap.c before :( Is is too late to review it?
- The naming is chaotic. At least these:
        EXPORT_SYMBOL(alloc_cpu_rmap);
        EXPORT_SYMBOL(free_cpu_rmap);
        EXPORT_SYMBOL(cpu_rmap_add);
        EXPORT_SYMBOL(cpu_rmap_update);
        EXPORT_SYMBOL(free_irq_cpu_rmap);
        EXPORT_SYMBOL(irq_cpu_rmap_add);
should be consistently named cpu_rmap_foo()
- What's the locking model? It appears to be caller-provided, but it is undocumented.
drivers/net/ethernet/mellanox/mlx4/ appears to be using msix_ctl.pool_lock for exclusion, but I didn't check for coverage.
drivers/net/ethernet/sfc/efx.c seems to not need locking because all its cpu_rmap operations are at module_init() time.
The cpu_rmap code would be less of a hand grenade if each of its interface functions documented the caller's locking requirements.
As for this patch: there's no cc:stable here but it does appear that the problem is sufficiently serious to justify a backport, agree?
Quoted 9 lines
include/linux/cpu_rmap.h +0 -0
 
...
@@ -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);
include/linux/cpu_rmap.h struct cpu_rmap { · line 39
Quoted 10 lines
include/linux/cpu_rmap.h +1 -9
-
-/**
- * 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);
Can we do away with free_cpu_rmap() altogether? It is a misleading name - it is a put() function, not a free() function. It would be clearer (not to mention faster and smaller) to change all call sites to directly call cpu_rmap_put().
Quoted 8 lines
include/linux/cpu_rmap.h +0 -0
 extern int cpu_rmap_add(struct cpu_rmap *rmap, void *obj);
 extern int cpu_rmap_update(struct cpu_rmap *rmap, u16 index,
 
...
@@ -63,6 +64,44 @@ struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags)
 }
 EXPORT_SYMBOL(alloc_cpu_rmap);
include/linux/cpu_rmap.h struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags) · line 75
Quoted 10 lines
include/linux/cpu_rmap.h +9 -0
 
+/**
+ * cpu_rmap_reclaim - internal reclaiming helper called from kref_put
+ * @ref: kref to struct cpu_rmap
+ */
+static void cpu_rmap_reclaim(struct kref *ref)
+{
+     struct cpu_rmap *rmap = container_of(ref, struct cpu_rmap, refcount);
+     kfree(rmap);
+}
I suggest this be renamed to cpu_rmap_release(). As "release" is the conventional term for a kref release handler.
include/linux/cpu_rmap.h
Quoted 11 lines
...
include/linux/cpu_rmap.h +8 -0
+/**
+ * cpu_rmap_put - internal helper to release ref on a cpu_rmap
+ * @rmap: reverse-map allocated with alloc_cpu_rmap()
+ */
+static inline void cpu_rmap_put(struct cpu_rmap *rmap)
+{
+     kref_put(&rmap->refcount, cpu_rmap_reclaim);
+}
As mentioned, I suggest this become the public interface. And I suppose it should propagate kref_put()'s return value, in case someone is interested.
include/linux/cpu_rmap.h
Quoted 9 lines
include/linux/cpu_rmap.h +9 -0
+/**
+ * free_cpu_rmap - free CPU affinity reverse-map
+ * @rmap: Reverse-map allocated with alloc_cpu_rmap()
+ */
+void free_cpu_rmap(struct cpu_rmap *rmap)
+{
+     cpu_rmap_put(rmap);
+}
+EXPORT_SYMBOL(free_cpu_rmap);
zap.
Quoted 6 lines
/* Reevaluate nearest object for given CPU, comparing with the given
* neighbours at the given distance.
*/
...

Andrew Morton

maintainer

2013-01-03 21:47 UTC #10 lore raw

Quoted 1 line · On Wed, 2 Jan 2013 18:35:00 -0800 David Decotigny <[email protected]> wrote:

(please don't top-post)

Quoted 7 lines

Thanks. It is not too late to review this code. But I'd prefer not to address refactoring issues with this patch, which is supposed to fix a deadlock with some drivers. So I'd rather not to add function renaming, suppressions, etc. that have an effect outside cpu_rmap's code. Instead, I'd like to propose another patch later, which will be a little more intrusive in that respect, if that's ok with you.

Quoted 7 lines

I believe Ben answered your other concerns, I consider him as the expert as to whether there should be finer-grained locking implemented in this subsystem. Let me just add that I second him in saying that the deadlock risk was clearly identified and mentioned in the doc. Unfortunately, initial implementation makes this risk hard to work-around for some drivers, which is what this patch proposes to address.

^^ all this pertains to the existing code.

Quoted 2 lines

So, for now, I'd like to keep v4 as the current version. And some refactoring will be done in a later patch.

^^ this pertains to the current patch. And no reason for ignoring my review comments was provided.

So I did it myself. It's very simple, as free_cpu_rmap() has no callers.

Also, free_irq_cpu_rmap() is now distinctly fishy - it runs all the notifiers every time it is called. Surely it should only do that when the refcount falls to zero?

From: Andrew Morton <[email protected]>
Subject: lib-cpu_rmap-avoid-flushing-all-workqueues-fix

eliminate free_cpu_rmap, rename cpu_rmap_reclaim() to cpu_rmap_release(), propagate kref_put() retval from cpu_rmap_put()

Cc "David S. Miller" <[email protected]>

Cc Amir Vadai <[email protected]>

Cc Ben Hutchings <[email protected]>

Cc David Decotigny <[email protected]>

Cc Eric Dumazet <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Or Gerlitz <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 include/linux/cpu_rmap.h |    2 +-
 lib/cpu_rmap.c           |   21 ++++++---------------
 2 files changed, 7 insertions(+), 16 deletions(-)

include/linux/cpu_rmap.h renamed from include/linux/cpu_rmap.h~lib-cpu_rmap-avoid-flushing-all-workqueues-fix +1 -1

diff -puN include/linux/cpu_rmap.h~lib-cpu_rmap-avoid-flushing-all-workqueues-fix include/linux/cpu_rmap.h
@@ -36,7 +36,7 @@ struct cpu_rmap {
 #define CPU_RMAP_DIST_INF 0xffff
 
 extern struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t flags);
-extern void free_cpu_rmap(struct cpu_rmap *rmap);
+extern int cpu_rmap_put(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,

diff -puN include/linux/interrupt.h~lib-cpu_rmap-avoid-flushing-all-workqueues-fix include/linux/interrupt.h

lib/cpu_rmap.c renamed from lib/cpu_rmap.c~lib-cpu_rmap-avoid-flushing-all-workqueues-fix +6 ~~-15~~

diff -puN lib/cpu_rmap.c~lib-cpu_rmap-avoid-flushing-all-workqueues-fix lib/cpu_rmap.c
@@ -65,10 +65,10 @@ struct cpu_rmap *alloc_cpu_rmap(unsigned
 EXPORT_SYMBOL(alloc_cpu_rmap);
 
 /**
- * cpu_rmap_reclaim - internal reclaiming helper called from kref_put
+ * cpu_rmap_release - internal reclaiming helper called from kref_put
  * @ref: kref to struct cpu_rmap
  */
-static void cpu_rmap_reclaim(struct kref *ref)
+static void cpu_rmap_release(struct kref *ref)
 {
 	struct cpu_rmap *rmap = container_of(ref, struct cpu_rmap, refcount);
 	kfree(rmap);

@@ -84,23 +84,14 @@ static inline void cpu_rmap_get(struct c
 }
 
 /**
- * cpu_rmap_put - internal helper to release ref on a cpu_rmap
+ * cpu_rmap_put - release ref on a cpu_rmap
  * @rmap: reverse-map allocated with alloc_cpu_rmap()
  */
-static inline void cpu_rmap_put(struct cpu_rmap *rmap)
+int cpu_rmap_put(struct cpu_rmap *rmap)
 {
-	kref_put(&rmap->refcount, cpu_rmap_reclaim);
+	return kref_put(&rmap->refcount, cpu_rmap_release);
 }
-
-/**
- * free_cpu_rmap - free CPU affinity reverse-map
- * @rmap: Reverse-map allocated with alloc_cpu_rmap()
- */
-void free_cpu_rmap(struct cpu_rmap *rmap)
-{
-	cpu_rmap_put(rmap);
-}
-EXPORT_SYMBOL(free_cpu_rmap);
+EXPORT_SYMBOL(cpu_rmap_put);
 
 /* Reevaluate nearest object for given CPU, comparing with the given
  * neighbours at the given distance.

Ben Hutchings

2013-01-03 22:27 UTC #11 lore raw

Quoted 26 lines · Andrew Morton

Quoted 1 line · On Wed, 2 Jan 2013 18:35:00 -0800 David Decotigny <[email protected]> wrote:
(please don't top-post)
Quoted 7 lines
Thanks. It is not too late to review this code. But I'd prefer not to address refactoring issues with this patch, which is supposed to fix a deadlock with some drivers. So I'd rather not to add function renaming, suppressions, etc. that have an effect outside cpu_rmap's code. Instead, I'd like to propose another patch later, which will be a little more intrusive in that respect, if that's ok with you.
Quoted 7 lines
I believe Ben answered your other concerns, I consider him as the expert as to whether there should be finer-grained locking implemented in this subsystem. Let me just add that I second him in saying that the deadlock risk was clearly identified and mentioned in the doc. Unfortunately, initial implementation makes this risk hard to work-around for some drivers, which is what this patch proposes to address.
^^ all this pertains to the existing code.
Quoted 2 lines
So, for now, I'd like to keep v4 as the current version. And some refactoring will be done in a later patch.

Quoted 7 lines

^^ this pertains to the current patch. And no reason for ignoring my review comments was provided.
So I did it myself. It's very simple, as free_cpu_rmap() has no callers.
Also, free_irq_cpu_rmap() is now distinctly fishy - it runs all the notifiers every time it is called.

It removes the notifiers.

Quoted 1 line

Surely it should only do that when the refcount falls to zero?

[...]

No, absolutely not. An IRQ cpu_rmap may have multiple references to it now, but it only has one owner: the driver that allocates it and the associated IRQs.

As soon as the driver frees those IRQs, they may be allocated by some other driver, so we require that notifiers are removed from them first (see the WARN_ON in free_irq()). Also, if free_irq_cpu_rmap() did not remove the notifiers, more notifications could be scheduled and keep the cpu_rmap alive indefinitely.

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.

Amir Vadai

Acked-by on v4 1/1

2013-01-07 13:00 UTC #12 lore raw

In reply to David Decotigny

Quoted 10 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.

Quoted 7 lines

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

---
  include/linux/cpu_rmap.h  |   13 +++-------
  include/linux/interrupt.h |    5 ----
  lib/cpu_rmap.c            |   63 +++++++++++++++++++++++++++++++++++++++++----
  3 files changed, 62 insertions(+), 19 deletions(-)

Acked-by Amir Vadai <[email protected]>

Amir Vadai

Acked-by on v4 1/1

2013-01-07 13:01 UTC #13 lore raw

In reply to David Decotigny

Quoted 10 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.

Quoted 7 lines

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

---
  include/linux/cpu_rmap.h  |   13 +++-------
  include/linux/interrupt.h |    5 ----
  lib/cpu_rmap.c            |   63 +++++++++++++++++++++++++++++++++++++++++----
  3 files changed, 62 insertions(+), 19 deletions(-)

Acked-by Amir Vadai <[email protected]>