* Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues
2013-01-02 21:52 ` [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues David Decotigny
@ 2013-01-02 21:54 ` Ben Hutchings
2013-01-02 22:05 ` Eric Dumazet
` (4 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Ben Hutchings @ 2013-01-02 21:54 UTC (permalink / raw)
To: David Decotigny
Cc: linux-kernel, David S. Miller, Or Gerlitz, Amir Vadai,
Paul E. McKenney, Thomas Gleixner, Andrew Morton, Josh Triplett,
David Howells, Paul Gortmaker
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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues
2013-01-02 21:52 ` [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues David Decotigny
2013-01-02 21:54 ` Ben Hutchings
@ 2013-01-02 22:05 ` Eric Dumazet
2013-01-02 22:20 ` Josh Triplett
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Eric Dumazet @ 2013-01-02 22:05 UTC (permalink / raw)
To: David Decotigny
Cc: linux-kernel, Ben Hutchings, David S. Miller, Or Gerlitz,
Amir Vadai, Paul E. McKenney, Thomas Gleixner, Andrew Morton,
Josh Triplett, David Howells, Paul Gortmaker
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>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues
2013-01-02 21:52 ` [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues David Decotigny
2013-01-02 21:54 ` Ben Hutchings
2013-01-02 22:05 ` Eric Dumazet
@ 2013-01-02 22:20 ` Josh Triplett
2013-01-02 23:12 ` Andrew Morton
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Josh Triplett @ 2013-01-02 22:20 UTC (permalink / raw)
To: David Decotigny
Cc: linux-kernel, Ben Hutchings, David S. Miller, Or Gerlitz,
Amir Vadai, Paul E. McKenney, Thomas Gleixner, Andrew Morton,
David Howells, Paul Gortmaker
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues
2013-01-02 21:52 ` [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues David Decotigny
` (2 preceding siblings ...)
2013-01-02 22:20 ` Josh Triplett
@ 2013-01-02 23:12 ` Andrew Morton
2013-01-02 23:46 ` Ben Hutchings
2013-01-03 2:35 ` David Decotigny
2013-01-07 13:00 ` Amir Vadai
2013-01-07 13:01 ` Amir Vadai
5 siblings, 2 replies; 13+ messages in thread
From: Andrew Morton @ 2013-01-02 23:12 UTC (permalink / raw)
To: David Decotigny
Cc: linux-kernel, Ben Hutchings, David S. Miller, Or Gerlitz,
Amir Vadai, Paul E. McKenney, Thomas Gleixner, Josh Triplett,
David Howells, Paul Gortmaker
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.
> */
>
> ...
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues
2013-01-02 23:12 ` Andrew Morton
@ 2013-01-02 23:46 ` Ben Hutchings
2013-01-03 21:58 ` Andrew Morton
2013-01-03 2:35 ` David Decotigny
1 sibling, 1 reply; 13+ messages in thread
From: Ben Hutchings @ 2013-01-02 23:46 UTC (permalink / raw)
To: Andrew Morton
Cc: David Decotigny, linux-kernel, David S. Miller, Or Gerlitz,
Amir Vadai, Paul E. McKenney, Thomas Gleixner, Josh Triplett,
David Howells, Paul Gortmaker
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues
2013-01-02 23:46 ` Ben Hutchings
@ 2013-01-03 21:58 ` Andrew Morton
2013-01-08 16:05 ` Ben Hutchings
0 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2013-01-03 21:58 UTC (permalink / raw)
To: Ben Hutchings
Cc: David Decotigny, linux-kernel, David S. Miller, Or Gerlitz,
Amir Vadai, Paul E. McKenney, Thomas Gleixner, Josh Triplett,
David Howells, Paul Gortmaker
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues
2013-01-03 21:58 ` Andrew Morton
@ 2013-01-08 16:05 ` Ben Hutchings
0 siblings, 0 replies; 13+ messages in thread
From: Ben Hutchings @ 2013-01-08 16:05 UTC (permalink / raw)
To: Andrew Morton
Cc: David Decotigny, linux-kernel, David S. Miller, Or Gerlitz,
Amir Vadai, Paul E. McKenney, Thomas Gleixner, Josh Triplett,
David Howells, Paul Gortmaker
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues
2013-01-02 23:12 ` Andrew Morton
2013-01-02 23:46 ` Ben Hutchings
@ 2013-01-03 2:35 ` David Decotigny
2013-01-03 21:47 ` Andrew Morton
1 sibling, 1 reply; 13+ messages in thread
From: David Decotigny @ 2013-01-03 2:35 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel@vger.kernel.org, Ben Hutchings, David S. Miller,
Or Gerlitz, Amir Vadai, Paul E. McKenney, Thomas Gleixner,
Josh Triplett, David Howells, Paul Gortmaker
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.
>> */
>>
>> ...
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues
2013-01-03 2:35 ` David Decotigny
@ 2013-01-03 21:47 ` Andrew Morton
2013-01-03 22:26 ` Ben Hutchings
0 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2013-01-03 21:47 UTC (permalink / raw)
To: David Decotigny
Cc: linux-kernel@vger.kernel.org, Ben Hutchings, David S. Miller,
Or Gerlitz, Amir Vadai, Paul E. McKenney, Thomas Gleixner,
Josh Triplett, David Howells, Paul Gortmaker
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.
_
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues
2013-01-03 21:47 ` Andrew Morton
@ 2013-01-03 22:26 ` Ben Hutchings
0 siblings, 0 replies; 13+ messages in thread
From: Ben Hutchings @ 2013-01-03 22:26 UTC (permalink / raw)
To: Andrew Morton
Cc: David Decotigny, linux-kernel@vger.kernel.org, David S. Miller,
Or Gerlitz, Amir Vadai, Paul E. McKenney, Thomas Gleixner,
Josh Triplett, David Howells, Paul Gortmaker
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues
2013-01-02 21:52 ` [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues David Decotigny
` (3 preceding siblings ...)
2013-01-02 23:12 ` Andrew Morton
@ 2013-01-07 13:00 ` Amir Vadai
2013-01-07 13:01 ` Amir Vadai
5 siblings, 0 replies; 13+ messages in thread
From: Amir Vadai @ 2013-01-07 13:00 UTC (permalink / raw)
To: David Decotigny
Cc: linux-kernel, Ben Hutchings, David S. Miller, Or Gerlitz,
Paul E. McKenney, Thomas Gleixner, Andrew Morton, Josh Triplett,
David Howells, Paul Gortmaker
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>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues
2013-01-02 21:52 ` [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues David Decotigny
` (4 preceding siblings ...)
2013-01-07 13:00 ` Amir Vadai
@ 2013-01-07 13:01 ` Amir Vadai
5 siblings, 0 replies; 13+ messages in thread
From: Amir Vadai @ 2013-01-07 13:01 UTC (permalink / raw)
To: David Decotigny
Cc: linux-kernel, Ben Hutchings, David S. Miller, Or Gerlitz,
Paul E. McKenney, Thomas Gleixner, Andrew Morton, Josh Triplett,
David Howells, Paul Gortmaker
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>
^ permalink raw reply [flat|nested] 13+ messages in thread