Thanks, Ok for the cpu_rmap_put helper. Will do this in v3 of this patch.
Your comments suggest more refactoring, which might be better in the form of 1 or 2 additional patches that: - rename alloc_cpu_rmap & co according to new conventions (cpu_rmap_*) - remove the cpu_rmap sub-API altogether, keeping only irq_cpu_rmap (controversial?) I'd rather current patch is integrated in its own as it fixes an actual bug. I will send the other patch(es) separately later. v3 for this patch coming soon. On Fri, Dec 28, 2012 at 1:14 PM, Eric Dumazet <[email protected]> wrote: > On Fri, 2012-12-28 at 11:03 -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 <[email protected]> >> --- >> include/linux/cpu_rmap.h | 13 ++++--------- >> include/linux/interrupt.h | 5 ----- >> lib/cpu_rmap.c | 47 >> +++++++++++++++++++++++++++++++++++++++------ >> 3 files changed, 45 insertions(+), 20 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..bb5d0af 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,26 @@ struct cpu_rmap *alloc_cpu_rmap(unsigned int size, gfp_t >> flags) >> } >> EXPORT_SYMBOL(alloc_cpu_rmap); >> >> +/** >> + * reclaim_cpu_rmap - internal reclaiming helper called from kref_put >> + * @ref: kref to struct cpu_rmap >> + */ >> +static void reclaim_cpu_rmap(struct kref *ref) > > Could be named cpu_rmap_free() > > (and alloc_cpu_rmap() should be renamed as cpu_rmap_alloc() > >> +{ >> + struct cpu_rmap *rmap = container_of(ref, struct cpu_rmap, refcount); >> + kfree(rmap); >> +} >> + >> +/** >> + * free_cpu_rmap - free CPU affinity reverse-map >> + * @rmap: Reverse-map allocated with alloc_cpu_rmap(), or %NULL >> + */ >> +void free_cpu_rmap(struct cpu_rmap *rmap) > > This could be named : cpu_rmap_put() > >> +{ >> + kref_put(&rmap->refcount, reclaim_cpu_rmap); >> +} >> +EXPORT_SYMBOL(free_cpu_rmap); > > It seems this function could be static, and not exported. > >> + >> /* Reevaluate nearest object for given CPU, comparing with the given >> * neighbours at the given distance. >> */ >> @@ -197,8 +218,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 +232,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); >> + free_cpu_rmap(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,16 +256,22 @@ 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); >> + >> + kref_put(&glue->rmap->refcount, reclaim_cpu_rmap); > > cpu_rmap_put(glue->rmap); > >> kfree(glue); >> } >> >> /** >> * irq_cpu_rmap_add - add an IRQ to a CPU affinity reverse-map >> - * @rmap: The reverse-map >> + * @rmap: The per-IRQ reverse-map >> * @irq: The IRQ number >> * >> * This adds an IRQ affinity notifier that will update the reverse-map >> @@ -259,9 +291,12 @@ int irq_cpu_rmap_add(struct cpu_rmap *rmap, int irq) >> glue->notify.release = irq_cpu_rmap_release; >> glue->rmap = rmap; >> glue->index = cpu_rmap_add(rmap, glue); >> + kref_get(&rmap->refcount); >> rc = irq_set_affinity_notifier(irq, &glue->notify); >> - if (rc) >> + if (rc) { >> + kref_put(&rmap->refcount, reclaim_cpu_rmap); > > cpu_rmap_put(rmap); > >> kfree(glue); >> + } >> return rc; >> } >> EXPORT_SYMBOL(irq_cpu_rmap_add); > > I personally don't like kref abstraction, especially when used without > wrapper : > > kref_put(&rmap->refcount, reclaim_cpu_rmap); > > Its cleaner to have instead : > > cpu_rmap_put(rmap); > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
