Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751545AbYAOMHE (ORCPT ); Tue, 15 Jan 2008 07:07:04 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751140AbYAOMGw (ORCPT ); Tue, 15 Jan 2008 07:06:52 -0500 Received: from crystal.sipsolutions.net ([195.210.38.204]:36083 "EHLO sipsolutions.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751134AbYAOMGv (ORCPT ); Tue, 15 Jan 2008 07:06:51 -0500 Subject: Re: 2.6.24-rc7 lockdep warning when poweroff From: Johannes Berg To: Peter Zijlstra Cc: Dave Young , Linus Torvalds , Linux Kernel Mailing List , Ingo Molnar , Oleg Nesterov In-Reply-To: <1200307896.5887.42.camel@johannes.berg> References: <1200302644.7415.6.camel@twins> <1200306902.5887.39.camel@johannes.berg> <1200307288.7415.11.camel@twins> <1200307896.5887.42.camel@johannes.berg> Content-Type: text/plain Date: Tue, 15 Jan 2008 11:41:25 +0100 Message-Id: <1200393685.5887.113.camel@johannes.berg> Mime-Version: 1.0 X-Mailer: Evolution 2.12.2 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4608 Lines: 126 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): 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 different key - different name - OK Correct? 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. 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. 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). 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.) Who should I send it to in that case? 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/