lib: cpu_rmap: avoid flushing all workqueues
A message that is not in the archive (sent off-list or not yet received): [email protected]
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 replaceirq_run_affinity_notifiers(). It also removesirq_run_affinity_notifiers() altogether.
--- 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
@@ -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 0xffffextern 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
@@ -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
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 invokesflush_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 removesirq_run_affinity_notifiers()altogether.Signed-off-by David Decotigny <[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.
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 invokesflush_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 removesirq_run_affinity_notifiers()altogether.Signed-off-by David Decotigny <[email protected]>---
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 invokesflush_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 removesirq_run_affinity_notifiers()altogether.Signed-off-by David Decotigny <[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@@ -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 0xffffextern 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@@ -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@@ -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
maintainerQuoted 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 invokesflush_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 removesirq_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 0xffffextern 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
-0extern 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.
*/...
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 invokesflush_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 removesirq_run_affinity_notifiers()altogether.I can't say that I've ever noticed
cpu_rmap.cbefore :( 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.cseems to not need locking because all itscpu_rmapoperations are atmodule_init()time.The
cpu_rmapcode 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
maintainerQuoted 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 invokesflush_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 removesirq_run_affinity_notifiers()altogether.I can't say that I've ever noticed
cpu_rmap.cbefore :( 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()andfree_foo()alongsidefoo_do_this()andfoo_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.cseems to not need locking because all itscpu_rmapoperations are atmodule_init()time.The
cpu_rmapcode 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.
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.cseems to not need locking because all itscpu_rmapoperations are atmodule_init()time.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
The
cpu_rmapcode 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.
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 invokesflush_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 removesirq_run_affinity_notifiers()altogether.I can't say that I've ever noticed
cpu_rmap.cbefore :( 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.cseems to not need locking because all itscpu_rmapoperations are atmodule_init()time.The
cpu_rmapcode 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 0xffffextern 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 aput()function, not afree()function. It would be clearer (not to mention faster and smaller) to change all call sites to directly callcpu_rmap_put().Quoted 8 lines
include/linux/cpu_rmap.h +0
-0extern 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
maintainerQuoted 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()
---
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
@@ -36,7 +36,7 @@ struct cpu_rmap {#define CPU_RMAP_DIST_INF 0xffffextern 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
@@ -65,10 +65,10 @@ struct cpu_rmap *alloc_cpu_rmap(unsignedEXPORT_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.
_
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.
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 invokesflush_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 removesirq_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(-)
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 invokesflush_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 removesirq_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(-)