[PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

[PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

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

diff --git a/include/linux/cpu_rmap.h b/include/linux/cpu_rmap.h
index ac3bbb5..3be2813 100644
--- a/include/linux/cpu_rmap.h
+++ b/include/linux/cpu_rmap.h
@@ -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,
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 5e4e617..5fa5afe 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -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)
diff --git a/lib/cpu_rmap.c b/lib/cpu_rmap.c
index 145dec5..bcd1f0e 100644
--- a/lib/cpu_rmap.c
+++ b/lib/cpu_rmap.c
@@ -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);
-- 
1.7.10.2.5.g20d7bc9

Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

[Ben Hutchings]

On Wed, 2013-01-02 at 13:52 -0800, David Decotigny 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.
> 
> Signed-off-by: David Decotigny <decot@googlers.com>
[...]

Reviewed-by: Ben Hutchings <bhutchings@solarflare.com>

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

Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

[Eric Dumazet]

On Wed, 2013-01-02 at 13:52 -0800, David Decotigny 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.
> 
> Signed-off-by: David Decotigny <decot@googlers.com>
> ---

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

[Josh Triplett]

On Wed, Jan 02, 2013 at 01:52:25PM -0800, David Decotigny 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.
> 
> Signed-off-by: David Decotigny <decot@googlers.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> ---
>  include/linux/cpu_rmap.h  |   13 +++-------
>  include/linux/interrupt.h |    5 ----
>  lib/cpu_rmap.c            |   63 +++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 62 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/cpu_rmap.h b/include/linux/cpu_rmap.h
> index ac3bbb5..3be2813 100644
> --- a/include/linux/cpu_rmap.h
> +++ b/include/linux/cpu_rmap.h
> @@ -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,
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 5e4e617..5fa5afe 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -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)
> diff --git a/lib/cpu_rmap.c b/lib/cpu_rmap.c
> index 145dec5..bcd1f0e 100644
> --- a/lib/cpu_rmap.c
> +++ b/lib/cpu_rmap.c
> @@ -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);
> -- 
> 1.7.10.2.5.g20d7bc9
> 

Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

[Andrew Morton]

On Wed,  2 Jan 2013 13:52:25 -0800
David Decotigny <decot@googlers.com> 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?

> --- a/include/linux/cpu_rmap.h
> +++ b/include/linux/cpu_rmap.h
>
> ...
>
> @@ -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);

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


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

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

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

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

Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

[Ben Hutchings]

On Wed, 2013-01-02 at 15:12 -0800, Andrew Morton wrote:
> On Wed,  2 Jan 2013 13:52:25 -0800
> David Decotigny <decot@googlers.com> 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()

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

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

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

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

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

Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

[David Decotigny]

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

On Wed, Jan 2, 2013 at 3:12 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed,  2 Jan 2013 13:52:25 -0800
> David Decotigny <decot@googlers.com> 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?
>
>> --- a/include/linux/cpu_rmap.h
>> +++ b/include/linux/cpu_rmap.h
>>
>> ...
>>
>> @@ -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);
>
> 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().
>
>
>>  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);
>>
>> +/**
>> + * 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.
>
>>
>> ...
>>
>> +/**
>> + * 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.
>
>> +/**
>> + * 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.
>
>>  /* Reevaluate nearest object for given CPU, comparing with the given
>>   * neighbours at the given distance.
>>   */
>>
>> ...
>>
>

Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

[Andrew Morton]

On Wed, 2 Jan 2013 18:35:00 -0800
David Decotigny <decot@googlers.com> wrote:
>

(please don't top-post)

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

^^ all this pertains to the existing code.

> 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 <akpm@linux-foundation.org>
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" <davem@davemloft.net>
Cc: Amir Vadai <amirv@mellanox.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
Cc: David Decotigny <decot@googlers.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

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

diff -puN include/linux/cpu_rmap.h~lib-cpu_rmap-avoid-flushing-all-workqueues-fix include/linux/cpu_rmap.h
--- a/include/linux/cpu_rmap.h~lib-cpu_rmap-avoid-flushing-all-workqueues-fix
+++ a/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
diff -puN lib/cpu_rmap.c~lib-cpu_rmap-avoid-flushing-all-workqueues-fix lib/cpu_rmap.c
--- a/lib/cpu_rmap.c~lib-cpu_rmap-avoid-flushing-all-workqueues-fix
+++ a/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.
_

Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

[Andrew Morton]

On Wed, 2 Jan 2013 23:46:46 +0000
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Wed, 2013-01-02 at 15:12 -0800, Andrew Morton wrote:
> > On Wed,  2 Jan 2013 13:52:25 -0800
> > David Decotigny <decot@googlers.com> 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()
> 
> 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.

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

> And IRQ setup and teardown need to be properly serialised in the driver
> already.
> 
> >   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.

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

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

Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

[Ben Hutchings]

On Thu, 2013-01-03 at 13:47 -0800, Andrew Morton wrote:
> On Wed, 2 Jan 2013 18:35:00 -0800
> David Decotigny <decot@googlers.com> wrote:
> >
> 
> (please don't top-post)
> 
> > 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.
> 
> ^^ all this pertains to the existing code.
> 
> > 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.

It removes the notifiers.

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

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

Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

[Amir Vadai]

On 02/01/2013 23:52, David Decotigny 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.
>
> Signed-off-by: David Decotigny<decot@googlers.com>
> ---
>   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 <amirv@mellano.com>

Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

[Amir Vadai]

On 02/01/2013 23:52, David Decotigny 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.
>
> Signed-off-by: David Decotigny<decot@googlers.com>
> ---
>   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 <amirv@mellanox.com>

Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

[Ben Hutchings]

On Thu, 2013-01-03 at 13:58 -0800, Andrew Morton wrote:
> On Wed, 2 Jan 2013 23:46:46 +0000
> Ben Hutchings <bhutchings@solarflare.com> wrote:
[...]
> > >   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.
> 
> 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.

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