Hi,
When I "halt -p", lockdep warnings triggered as following (hand copy):
WARNING : at kernel/lockdep.c: 700 lookup_lock_class()
<snip>

lock_acquire

cleanup_workqueue_thread

workqueue_cpu_callback

notifier_call_chain

__raw_notifier_call_chain

raw_notifier_call_chain

__cpu_down

disable_nonboot_cpus

kernel_power_off

sys_reboot

<snip>
Regards

dave

--

My first guess would be a __create_workqueue_key() where a key is

re-used with a different workqueue name.
--

Hm, can you elaborate how you got there from the trace above? I don't

think I'm following.
johannes

The warning that triggered (lockdep.c:700) means that one class (key)

was used with more than one name.
Looking at cleanup_workqueue_thread(), the lock_acquire() there works on

wq->lockdep_map, and that is only initialized at one spot:

__create_workqueue_key(), thus it stands to reason that that was

mis-used.
--

Oh ok, yes, makes sense. Maybe something is generating a workqueue with

a name that's passed in but the key is statically from that place. I'll

try to find it.
johannes

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.) ...
[ message continues ]

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

Oh I should have explained this, missed it earlier. No, the

single-threadedness doesn't really matter for the problem, but the

warning Dave got couldn't possibly have triggered for a single-threaded

workqueue because it was from the CPU up/down code that spawns/stops the

new workqueue thread on a new/dying CPU.
johannes

True, but I don't see a good way to avoid that. Similar things also

happen with
	mutex_init(&priv->mtx);
Except, how could I do that though? Keys are required to be static, so I

can't have the object as the key. In any case, I don't think it matters

much because the workqueues are per-hardware but all have similar users,
It doesn't seem to have so far ;) I don't think it should. If some code

allocates a per-instance workqueue that's much like having an inode lock

or so.
The scenario to get into trouble with this would require having a

per-instance lock and a per-instance workqueue and flushing the

workqueue (that can contain functions taking the lock of instance A) of

instance A under the lock of instance B, but unless that is nested in a

way that it cannot happen in order BA as well it's actually a possible
Alright, I'll write a patch description and send it in a minute.
johannes

Yeah, it happens, I tend to 'fix' them when I encounter it though,

sometimes by just slightly altering the expression. It helps when
Yeah, I think so too, but never underestimate the creativity of driver
We had to split up the inode lock to per filesystem classes, just
Great, thanks for the effort.
--

i.e. filesystems can legally have different locking rules wrt. i_lock. I

dont really like it (we should have as simple locking rules as possible)

but it is the VFS status quo :)
	Ingo

--

Dave Young reported warnings from lockdep that the workqueue API

can sometimes try to register lockdep classes with the same key

but different names. This is not permitted in lockdep.
Unfortunately, I was unaware of that restriction when I wrote

the code to debug workqueue problems with lockdep and used the

workqueue name as the lockdep class name. This can obviously

lead to the problem if the workqueue name is dynamic.
This patch solves the problem by always using a constant name

for the workqueue's lockdep class, namely either the constant

name that was passed in or a string consisting of the variable

name.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---

Please be careful with this patch, I haven't been able to test it so far

because my powerbook doesn't have lockdep.
 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), NUL...
[ message continues ]

Hi,

Just for confirm, the warnings didn't trigger after applied your patch, thanks.
Regards

dave

--

I add some debug printk and found the names :
block_osm/exec_osm
in drivers/message/i2o
maybe this helps.
Regards

--

Not sure right or not, the following patch fixed the problem:
diff -upr linux/include/linux/workqueue.h linux.new/include/linux/workqueue.h

--- linux/include/linux/workqueue.h	2008-01-15 08:49:08.000000000 +0800

+++ linux.new/include/linux/workqueue.h	2008-01-15 08:49:42.000000000 +0800

@@ -154,7 +154,7 @@ __create_workqueue_key(const char *name,

 #ifdef CONFIG_LOCKDEP

 #define __create_workqueue(name, singlethread, freezeable)	\

 ({								\

-	static struct lock_class_key __key;			\

+	struct lock_class_key __key;			\

 								\

 	__create_workqueue_key((name), (singlethread),		\

--

That didn't get you:
  INFO: trying to register non-static key.
Msgs?
But it sure looks like __create_workqueue() is asking for trouble, if

there is a __create_workqueue() instance that takes a non constant name

we're in trouble.
--