On Tue, 2008-01-15 at 11:41 +0100, Johannes Berg wrote:
> Ok, I checked all users of the create*workqueue() API now.
>
> Turns out that there are many users that give a dynamic string as the
> workqueue name (only the first three are relevant for the problem at
> hand because the others are single-threaded):
I'm not sure the single threadedness of a workqueue matters here.
> drivers/connector/cn_queue.c
> drivers/media/video/ivtv/ivtv-driver.c
> drivers/message/i2o/driver.c
>
> drivers/i2c/chips/m41t00.c drivers/infiniband/core/mad.c
> drivers/message/fusion/mptfc.c drivers/net/qla3xxx.c
> drivers/scsi/hosts.c drivers/scsi/qla4xxx/ql4_os.c
> drivers/scsi/scsi_transport_fc.c drivers/spi/mpc52xx_psc_spi.c
> drivers/spi/omap2_mcspi.c drivers/spi/spi_bitbang.c
> drivers/spi/spi_txx9.c drivers/spi/spi_imx.c drivers/spi/pxa2xx_spi.c
> drivers/spi/spi_bfin5xx.c drivers/power/ds2760_battery.c
> net/mac80211/ieee80211.c
>
>
> That's not really the problem though.
>
> TBH, when writing the workqueue deadlock detection code I wasn't aware
> that it is not allowed to use the same key with different names.
>
> To make sure now:
> same key - different name - BAD
> same key - same name - OK
> different key - same name - OK
Strictly speaking one can do that, although I would recommend against it
- it leads to confusion as to which lock got into trouble when looking
at lockdep/stat output.
> different key - different name - OK
>
> Correct?
Yeah.
> The root problem here seems to be that I use the same name as for the
> workqueue for the lockdep_map and other code uses a non-static workqueue
> name. Using the workqueue name for the lock is good for knowing which
> workqueue ran into trouble though.
Indeed, and also using a different key allows the workqueue to have
different lock dependencies as well. The trouble is, lockdep works at
the class level, a class with multiple names just doesn't make sense,
and reporting will get it wrong (although it may appear to work
correctly in the trivial cases).
> mac80211 for example wants to allocate a (single-threaded) workqueue for
> each hardware that is plugged in and wants to call it by the hardware
> name.
Right, that would require a new key for each instance.
> Anyway, the patch below should help. I hope the patch compiles, I don't
> have a lockdep-enabled system at hand right now (irqtrace is still not
> supported on powerpc and my 64-bit powerpc isn't running a kernel with
> my irqtrace support patch at the moment).
Tssk :-)
> If you think the patch is a correct way to solve the problem I'll submit
> it formally and it should then be included in 2.6.24 to avoid
> regressions with the workqueue API (the workqueue lockup detection was
> merged early in 2.6.24.)
The patch looks ok, one important thing to note is that it means that
all workqueues instantiated by the same __create_workqueue() call-site
share lock dependency chains - I'm unsure if that might get us into
trouble or not.
> Who should I send it to in that case?
Me and Ingo :-)
> Dave, do you know if you had connector, ivtv or i2o in the kernel (just
> to make sure my analysis was correct)? And can you reproduce the
> problem, and if so, can you try if this patch helps?
>
> johannes
>
>
> ---
> include/linux/workqueue.h | 14 +++++++++++---
> kernel/workqueue.c | 5 +++--
> 2 files changed, 14 insertions(+), 5 deletions(-)
>
> --- everything.orig/include/linux/workqueue.h 2008-01-15 02:10:55.098131131
> +0100
> +++ everything/include/linux/workqueue.h 2008-01-15 02:26:37.428130426
> +0100
> @@ -149,19 +149,27 @@ struct execute_work {
>
> extern struct workqueue_struct *
> __create_workqueue_key(const char *name, int singlethread,
> - int freezeable, struct lock_class_key *key);
> + int freezeable, struct lock_class_key *key,
> + const char *lock_name);
>
> #ifdef CONFIG_LOCKDEP
> #define __create_workqueue(name, singlethread, freezeable) \
> ({ \
> static struct lock_class_key __key; \
> + const char *__lock_name; \
> + \
> + if (__builtin_constant_p(name)) \
> + __lock_name = (name); \
> + else \
> + __lock_name = #name; \
> \
> __create_workqueue_key((name), (singlethread), \
> - (freezeable), &__key); \
> + (freezeable), &__key, \
> + __lock_name); \
> })
> #else
> #define __create_workqueue(name, singlethread, freezeable) \
> - __create_workqueue_key((name), (singlethread), (freezeable), NULL)
> + __create_workqueue_key((name), (singlethread), (freezeable), NULL, NULL)
> #endif
>
> #define create_workqueue(name) __create_workqueue((name), 0, 0)
> --- everything.orig/kernel/workqueue.c 2008-01-15 02:15:13.578132867
> +0100
> +++ everything/kernel/workqueue.c 2008-01-15 02:18:40.518138455 +0100
> @@ -722,7 +722,8 @@ static void start_workqueue_thread(struc
> struct workqueue_struct *__create_workqueue_key(const char *name,
> int singlethread,
> int freezeable,
> - struct lock_class_key *key)
> + struct lock_class_key *key,
> + const char *lock_name)
> {
> struct workqueue_struct *wq;
> struct cpu_workqueue_struct *cwq;
> @@ -739,7 +740,7 @@ struct workqueue_struct *__create_workqu
> }
>
> wq->name = name;
> - lockdep_init_map(&wq->lockdep_map, name, key, 0);
> + lockdep_init_map(&wq->lockdep_map, lock_name, key, 0);
> wq->singlethread = singlethread;
> wq->freezeable = freezeable;
> INIT_LIST_HEAD(&wq->list);
>
>
--
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/
