Skip to contentSkip to conversation

lib: cpu_rmap: avoid flushing all workqueues

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

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

David Decotigny

author
#1 lore raw

PATCH v3 1/1 lib: cpu_rmap: avoid flushing all workqueues

In reply to a message not in the archive

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            |   56 ++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 54 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 +50 -6

107 lines not on this page · raw message

Signature · 1 line
1.7.10.2.5.g20d7bc9
#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]>

You might consider adding a cpu_rmap_get to parallel cpu_rmap_put.

Also, why keep free_cpu_rmap around at this point? As far as I can tell, it has no callers.

Otherwise, this looks good to me.

- Josh Triplett

Reviewed-by on v3 1/1
#6 lore raw
In reply to David Decotigny
Quoted 2 lines · David Decotigny

In some cases, free_irq_cpu_rmap() is called while holding a lock (eg. rtnl).

I made fairly sure that it didn't get called while holding the RTNL lock. However it looks like some mlx4_en ethtool ops now call it (indirectly).

Quoted 4 lines

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.

Which is why the kernel-doc says not to do that. But realistically a device driver may not easily be able to avoid holding either its underlying device's lock or a susbsytem global lock... and presumably there may sometimes be a work item that needs the device lock.

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

[...]

Quoted 2 lines
lib/cpu_rmap.c +0 -0

[...]

Quoted 4 lines

/**
* irq_cpu_rmap_add - add an IRQ to a CPU affinity reverse-map
- * @rmap: The reverse-map
+ * @rmap: The per-IRQ reverse-map

[...]

Please drop this comment change; the only 'per-IRQ' thing is the IRQ notifier structure which is private.

With that, you can add:

Reviewed-by Ben Hutchings <[email protected]>

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.
#3 lore raw
In reply to Josh Triplett
Quoted 11 lines · Josh Triplett
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 8 lines
Quoted 1 line
Signed-off-by David Decotigny <[email protected]>

You might consider adding a cpu_rmap_get to parallel cpu_rmap_put.

Also, why keep free_cpu_rmap around at this point? As far as I can tell, it has no callers.

Otherwise, this looks good to me.

I intended to make cpu_rmap usable independently of IRQ notification, although as you note there have been no other users so far. free_cpu_rmap() is now effectively an alias for cpu_rmap_put(), and the latter *does* have a caller. So perhaps cpu_rmap_put() should be extern and the alias dropped.

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.
author
#4 lore raw

Thanks,

I appreciate your review and clarification, as I was afraid there could be something I missed. I will send v4 of this patch: an update with the cpu_rmap_get helper that Josh suggested and the "free_cpu_rmap" alias removed.

Regards,

--
David Decotigny

Quoted 34 lines · Ben Hutchings
Quoted 11 lines · Josh Triplett
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 8 lines
Quoted 1 line
Signed-off-by David Decotigny <[email protected]>

You might consider adding a cpu_rmap_get to parallel cpu_rmap_put.

Also, why keep free_cpu_rmap around at this point? As far as I can tell, it has no callers.

Otherwise, this looks good to me.

I intended to make cpu_rmap usable independently of IRQ notification, although as you note there have been no other users so far. free_cpu_rmap() is now effectively an alias for cpu_rmap_put(), and the latter *does* have a caller. So perhaps cpu_rmap_put() should be extern and the alias dropped.

Ben.

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

author
#5 lore raw

Revising my position: v4 will only have comments update (Ben) and new cpu_rmap_get inetrnal helper (Josh). IMHO, further API rework discussed with Ben and Eric ought to be in a later patch, as they affect drivers.

--
David Decotigny

Quoted 48 lines · David Decotigny

Thanks,

I appreciate your review and clarification, as I was afraid there could be something I missed. I will send v4 of this patch: an update with the cpu_rmap_get helper that Josh suggested and the "free_cpu_rmap" alias removed.

Regards,

--
David Decotigny

Quoted 34 lines · Ben Hutchings
Quoted 11 lines · Josh Triplett
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 8 lines
Quoted 1 line
Signed-off-by David Decotigny <[email protected]>

You might consider adding a cpu_rmap_get to parallel cpu_rmap_put.

Also, why keep free_cpu_rmap around at this point? As far as I can tell, it has no callers.

Otherwise, this looks good to me.

I intended to make cpu_rmap usable independently of IRQ notification, although as you note there have been no other users so far. free_cpu_rmap() is now effectively an alias for cpu_rmap_put(), and the latter *does* have a caller. So perhaps cpu_rmap_put() should be extern and the alias dropped.

Ben.

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