lockdep whine in 2.6.26-rc2-mm1

lockdep whine in 2.6.26-rc2-mm1

[Andrew Morton]

=============================================
[ INFO: possible recursive locking detected ]
2.6.26-rc2-mm1 #15
---------------------------------------------
modprobe/942 is trying to acquire lock:
 (&cls->mutex){--..}, at: [<ffffffff811b431e>] device_add+0x43d/0x57a

but task is already holding lock:
 (&cls->mutex){--..}, at: [<ffffffff811b6787>] class_interface_register+0x48/0xbd

other info that might help us debug this:
1 lock held by modprobe/942:
 #0:  (&cls->mutex){--..}, at: [<ffffffff811b6787>] class_interface_register+0x48/0xbd

stack backtrace:
Pid: 942, comm: modprobe Not tainted 2.6.26-rc2-mm1 #15

Call Trace:
 [<ffffffff81056be1>] __lock_acquire+0x90d/0xc50
 [<ffffffff8100c50f>] ? restore_args+0x0/0x30
 [<ffffffff811b431e>] ? device_add+0x43d/0x57a
 [<ffffffff81057276>] lock_acquire+0x91/0xb7
 [<ffffffff811b431e>] ? device_add+0x43d/0x57a
 [<ffffffff812b23ab>] mutex_lock_nested+0xf2/0x278
 [<ffffffff811b431e>] ? device_add+0x43d/0x57a
 [<ffffffff812b3acd>] ? _spin_unlock+0x23/0x28
 [<ffffffff811b431e>] device_add+0x43d/0x57a
 [<ffffffff811b4471>] device_register+0x16/0x1b
 [<ffffffff811b4555>] device_create+0xdf/0x112
 [<ffffffff81055fdc>] ? trace_hardirqs_on+0xd/0xf
 [<ffffffff81055fdc>] ? trace_hardirqs_on+0xd/0xf
 [<ffffffff812b1fe1>] ? mutex_unlock+0x9/0xb
 [<ffffffff811b79b7>] ? kobj_map+0x113/0x124
 [<ffffffff810ac0f5>] ? exact_lock+0x0/0x14
 [<ffffffff810abd09>] ? exact_match+0x0/0x9
 [<ffffffffa00ec448>] :sg:sg_add+0x2a3/0x3bd
 [<ffffffff811b67b6>] class_interface_register+0x77/0xbd
 [<ffffffffa005d869>] :scsi_mod:scsi_register_interface+0x11/0x13
 [<ffffffffa00d50a3>] :sg:init_sg+0xa3/0x155
 [<ffffffff8105ea8f>] sys_init_module+0x1823/0x197a
 [<ffffffff810c45bc>] ? seq_release+0x0/0x56
 [<ffffffff8100bebb>] system_call_after_swapgs+0x7b/0x80

Driver 'sr' needs updating - please use bus_type methods
sd 0:0:0:0: Attached scsi generic sg0 type 0
sr0: scsi3-mmc drive: 24x/24x writer dvd-ram cd/rw xa/form2 cdda tray
Uniform CD-ROM driver Revision: 3.20
sr 3:0:0:0: Attached scsi CD-ROM sr0
sr 3:0:0:0: Attached scsi generic sg1 type 5


Full dmesg: http://userweb.kernel.org/~akpm/dmesg-t61p.txt
Config: http://userweb.kernel.org/~akpm/config-t61p.txt

Re: lockdep whine in 2.6.26-rc2-mm1

[Greg KH]

On Wed, May 14, 2008 at 12:09:33AM -0700, Andrew Morton wrote:
> 
> =============================================
> [ INFO: possible recursive locking detected ]
> 2.6.26-rc2-mm1 #15
> ---------------------------------------------
> modprobe/942 is trying to acquire lock:
>  (&cls->mutex){--..}, at: [<ffffffff811b431e>] device_add+0x43d/0x57a
> 
> but task is already holding lock:
>  (&cls->mutex){--..}, at: [<ffffffff811b6787>] class_interface_register+0x48/0xbd
> 
> other info that might help us debug this:
> 1 lock held by modprobe/942:
>  #0:  (&cls->mutex){--..}, at: [<ffffffff811b6787>] class_interface_register+0x48/0xbd
> 
> stack backtrace:
> Pid: 942, comm: modprobe Not tainted 2.6.26-rc2-mm1 #15
> 
> Call Trace:
>  [<ffffffff81056be1>] __lock_acquire+0x90d/0xc50
>  [<ffffffff8100c50f>] ? restore_args+0x0/0x30
>  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
>  [<ffffffff81057276>] lock_acquire+0x91/0xb7
>  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
>  [<ffffffff812b23ab>] mutex_lock_nested+0xf2/0x278
>  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
>  [<ffffffff812b3acd>] ? _spin_unlock+0x23/0x28
>  [<ffffffff811b431e>] device_add+0x43d/0x57a
>  [<ffffffff811b4471>] device_register+0x16/0x1b
>  [<ffffffff811b4555>] device_create+0xdf/0x112
>  [<ffffffff81055fdc>] ? trace_hardirqs_on+0xd/0xf
>  [<ffffffff81055fdc>] ? trace_hardirqs_on+0xd/0xf
>  [<ffffffff812b1fe1>] ? mutex_unlock+0x9/0xb
>  [<ffffffff811b79b7>] ? kobj_map+0x113/0x124
>  [<ffffffff810ac0f5>] ? exact_lock+0x0/0x14
>  [<ffffffff810abd09>] ? exact_match+0x0/0x9
>  [<ffffffffa00ec448>] :sg:sg_add+0x2a3/0x3bd
>  [<ffffffff811b67b6>] class_interface_register+0x77/0xbd
>  [<ffffffffa005d869>] :scsi_mod:scsi_register_interface+0x11/0x13
>  [<ffffffffa00d50a3>] :sg:init_sg+0xa3/0x155
>  [<ffffffff8105ea8f>] sys_init_module+0x1823/0x197a
>  [<ffffffff810c45bc>] ? seq_release+0x0/0x56
>  [<ffffffff8100bebb>] system_call_after_swapgs+0x7b/0x80

I'm guessing that this is due to David's "change the semaphore to a
mutex" patch that you have in your tree, but I refused to take as I was
worried about just this issue :)

David, any thoughts?

> Driver 'sr' needs updating - please use bus_type methods

This is not the same issue, last I looked Kay and James were working on
resolving this.

thanks,

greg k-h

Re: lockdep whine in 2.6.26-rc2-mm1

[Matthew Wilcox]

On Wed, May 14, 2008 at 08:56:39AM -0700, Greg KH wrote:
> On Wed, May 14, 2008 at 12:09:33AM -0700, Andrew Morton wrote:
> > 
> > =============================================
> > [ INFO: possible recursive locking detected ]
> > 2.6.26-rc2-mm1 #15
> > ---------------------------------------------
> > modprobe/942 is trying to acquire lock:
> >  (&cls->mutex){--..}, at: [<ffffffff811b431e>] device_add+0x43d/0x57a
> > 
> > but task is already holding lock:
> >  (&cls->mutex){--..}, at: [<ffffffff811b6787>] class_interface_register+0x48/0xbd
> > 
> > other info that might help us debug this:
> > 1 lock held by modprobe/942:
> >  #0:  (&cls->mutex){--..}, at: [<ffffffff811b6787>] class_interface_register+0x48/0xbd
> > 
> > stack backtrace:
> > Pid: 942, comm: modprobe Not tainted 2.6.26-rc2-mm1 #15
> > 
> > Call Trace:
> >  [<ffffffff81056be1>] __lock_acquire+0x90d/0xc50
> >  [<ffffffff8100c50f>] ? restore_args+0x0/0x30
> >  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
> >  [<ffffffff81057276>] lock_acquire+0x91/0xb7
> >  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
> >  [<ffffffff812b23ab>] mutex_lock_nested+0xf2/0x278
> >  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
> >  [<ffffffff812b3acd>] ? _spin_unlock+0x23/0x28
> >  [<ffffffff811b431e>] device_add+0x43d/0x57a
> >  [<ffffffff811b4471>] device_register+0x16/0x1b
> >  [<ffffffff811b4555>] device_create+0xdf/0x112
> >  [<ffffffff81055fdc>] ? trace_hardirqs_on+0xd/0xf
> >  [<ffffffff81055fdc>] ? trace_hardirqs_on+0xd/0xf
> >  [<ffffffff812b1fe1>] ? mutex_unlock+0x9/0xb
> >  [<ffffffff811b79b7>] ? kobj_map+0x113/0x124
> >  [<ffffffff810ac0f5>] ? exact_lock+0x0/0x14
> >  [<ffffffff810abd09>] ? exact_match+0x0/0x9
> >  [<ffffffffa00ec448>] :sg:sg_add+0x2a3/0x3bd
> >  [<ffffffff811b67b6>] class_interface_register+0x77/0xbd
> >  [<ffffffffa005d869>] :scsi_mod:scsi_register_interface+0x11/0x13
> >  [<ffffffffa00d50a3>] :sg:init_sg+0xa3/0x155
> >  [<ffffffff8105ea8f>] sys_init_module+0x1823/0x197a
> >  [<ffffffff810c45bc>] ? seq_release+0x0/0x56
> >  [<ffffffff8100bebb>] system_call_after_swapgs+0x7b/0x80
> 
> I'm guessing that this is due to David's "change the semaphore to a
> mutex" patch that you have in your tree, but I refused to take as I was
> worried about just this issue :)

Hm, I thought I saw the same patch from Arjan months ago ... anyway.

class_interface_register() (at least in -rc2) holds &parent->sem (ie the
result of calling class_get() on the class) around calling ->add_dev.
The class in this case is the sg_sysfs_class, so it's calling sg_add().
sg_add() calls device_create() which calls device_add() which acquires
the &dev->class->sem.  The class in this case would _also_ seem to be
sg_sysfs_class.

So why doesn't this deadlock today?

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: lockdep whine in 2.6.26-rc2-mm1

[Dave Young]

On Thu, May 15, 2008 at 12:24 AM, Matthew Wilcox <matthew@wil.cx> wrote:
> On Wed, May 14, 2008 at 08:56:39AM -0700, Greg KH wrote:
>> On Wed, May 14, 2008 at 12:09:33AM -0700, Andrew Morton wrote:
>> >
>> > =============================================
>> > [ INFO: possible recursive locking detected ]
>> > 2.6.26-rc2-mm1 #15
>> > ---------------------------------------------
>> > modprobe/942 is trying to acquire lock:
>> >  (&cls->mutex){--..}, at: [<ffffffff811b431e>] device_add+0x43d/0x57a
>> >
>> > but task is already holding lock:
>> >  (&cls->mutex){--..}, at: [<ffffffff811b6787>] class_interface_register+0x48/0xbd
>> >
>> > other info that might help us debug this:
>> > 1 lock held by modprobe/942:
>> >  #0:  (&cls->mutex){--..}, at: [<ffffffff811b6787>] class_interface_register+0x48/0xbd
>> >
>> > stack backtrace:
>> > Pid: 942, comm: modprobe Not tainted 2.6.26-rc2-mm1 #15
>> >
>> > Call Trace:
>> >  [<ffffffff81056be1>] __lock_acquire+0x90d/0xc50
>> >  [<ffffffff8100c50f>] ? restore_args+0x0/0x30
>> >  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
>> >  [<ffffffff81057276>] lock_acquire+0x91/0xb7
>> >  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
>> >  [<ffffffff812b23ab>] mutex_lock_nested+0xf2/0x278
>> >  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
>> >  [<ffffffff812b3acd>] ? _spin_unlock+0x23/0x28
>> >  [<ffffffff811b431e>] device_add+0x43d/0x57a
>> >  [<ffffffff811b4471>] device_register+0x16/0x1b
>> >  [<ffffffff811b4555>] device_create+0xdf/0x112
>> >  [<ffffffff81055fdc>] ? trace_hardirqs_on+0xd/0xf
>> >  [<ffffffff81055fdc>] ? trace_hardirqs_on+0xd/0xf
>> >  [<ffffffff812b1fe1>] ? mutex_unlock+0x9/0xb
>> >  [<ffffffff811b79b7>] ? kobj_map+0x113/0x124
>> >  [<ffffffff810ac0f5>] ? exact_lock+0x0/0x14
>> >  [<ffffffff810abd09>] ? exact_match+0x0/0x9
>> >  [<ffffffffa00ec448>] :sg:sg_add+0x2a3/0x3bd
>> >  [<ffffffff811b67b6>] class_interface_register+0x77/0xbd
>> >  [<ffffffffa005d869>] :scsi_mod:scsi_register_interface+0x11/0x13
>> >  [<ffffffffa00d50a3>] :sg:init_sg+0xa3/0x155
>> >  [<ffffffff8105ea8f>] sys_init_module+0x1823/0x197a
>> >  [<ffffffff810c45bc>] ? seq_release+0x0/0x56
>> >  [<ffffffff8100bebb>] system_call_after_swapgs+0x7b/0x80
>>
>> I'm guessing that this is due to David's "change the semaphore to a
>> mutex" patch that you have in your tree, but I refused to take as I was
>> worried about just this issue :)
>
> Hm, I thought I saw the same patch from Arjan months ago ... anyway.
>
> class_interface_register() (at least in -rc2) holds &parent->sem (ie the
> result of calling class_get() on the class) around calling ->add_dev.
> The class in this case is the sg_sysfs_class, so it's calling sg_add().
> sg_add() calls device_create() which calls device_add() which acquires
> the &dev->class->sem.  The class in this case would _also_ seem to be
> sg_sysfs_class.
>
> So why doesn't this deadlock today?

The classes are different here, first sdev_class, then sg_sysfs_class

Regards
dave

Re: lockdep whine in 2.6.26-rc2-mm1

[Dave Young]

On Thu, May 15, 2008 at 01:47:02PM +0800, Dave Young wrote:
> On Thu, May 15, 2008 at 12:24 AM, Matthew Wilcox <matthew@wil.cx> wrote:
> > On Wed, May 14, 2008 at 08:56:39AM -0700, Greg KH wrote:
> >> On Wed, May 14, 2008 at 12:09:33AM -0700, Andrew Morton wrote:
> >> >
> >> > =============================================
> >> > [ INFO: possible recursive locking detected ]
> >> > 2.6.26-rc2-mm1 #15
> >> > ---------------------------------------------
> >> > modprobe/942 is trying to acquire lock:
> >> >  (&cls->mutex){--..}, at: [<ffffffff811b431e>] device_add+0x43d/0x57a
> >> >
> >> > but task is already holding lock:
> >> >  (&cls->mutex){--..}, at: [<ffffffff811b6787>] class_interface_register+0x48/0xbd
> >> >
> >> > other info that might help us debug this:
> >> > 1 lock held by modprobe/942:
> >> >  #0:  (&cls->mutex){--..}, at: [<ffffffff811b6787>] class_interface_register+0x48/0xbd
> >> >
> >> > stack backtrace:
> >> > Pid: 942, comm: modprobe Not tainted 2.6.26-rc2-mm1 #15
> >> >
> >> > Call Trace:
> >> >  [<ffffffff81056be1>] __lock_acquire+0x90d/0xc50
> >> >  [<ffffffff8100c50f>] ? restore_args+0x0/0x30
> >> >  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
> >> >  [<ffffffff81057276>] lock_acquire+0x91/0xb7
> >> >  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
> >> >  [<ffffffff812b23ab>] mutex_lock_nested+0xf2/0x278
> >> >  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
> >> >  [<ffffffff812b3acd>] ? _spin_unlock+0x23/0x28
> >> >  [<ffffffff811b431e>] device_add+0x43d/0x57a
> >> >  [<ffffffff811b4471>] device_register+0x16/0x1b
> >> >  [<ffffffff811b4555>] device_create+0xdf/0x112
> >> >  [<ffffffff81055fdc>] ? trace_hardirqs_on+0xd/0xf
> >> >  [<ffffffff81055fdc>] ? trace_hardirqs_on+0xd/0xf
> >> >  [<ffffffff812b1fe1>] ? mutex_unlock+0x9/0xb
> >> >  [<ffffffff811b79b7>] ? kobj_map+0x113/0x124
> >> >  [<ffffffff810ac0f5>] ? exact_lock+0x0/0x14
> >> >  [<ffffffff810abd09>] ? exact_match+0x0/0x9
> >> >  [<ffffffffa00ec448>] :sg:sg_add+0x2a3/0x3bd
> >> >  [<ffffffff811b67b6>] class_interface_register+0x77/0xbd
> >> >  [<ffffffffa005d869>] :scsi_mod:scsi_register_interface+0x11/0x13
> >> >  [<ffffffffa00d50a3>] :sg:init_sg+0xa3/0x155
> >> >  [<ffffffff8105ea8f>] sys_init_module+0x1823/0x197a
> >> >  [<ffffffff810c45bc>] ? seq_release+0x0/0x56
> >> >  [<ffffffff8100bebb>] system_call_after_swapgs+0x7b/0x80
> >>
> >> I'm guessing that this is due to David's "change the semaphore to a
> >> mutex" patch that you have in your tree, but I refused to take as I was
> >> worried about just this issue :)
> >
> > Hm, I thought I saw the same patch from Arjan months ago ... anyway.
> >
> > class_interface_register() (at least in -rc2) holds &parent->sem (ie the
> > result of calling class_get() on the class) around calling ->add_dev.
> > The class in this case is the sg_sysfs_class, so it's calling sg_add().
> > sg_add() calls device_create() which calls device_add() which acquires
> > the &dev->class->sem.  The class in this case would _also_ seem to be
> > sg_sysfs_class.
> >
> > So why doesn't this deadlock today?
> 
> The classes are different here, first sdev_class, then sg_sysfs_class

Greg, what about using mutex_lock_nested to silence lockdep? They are
the only usage of class->mutex out of class.c

Signed-off-by: Dave Young <hidave.darkstar@gmail.com>

---
drivers/base/core.c |    4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff -upr linux/drivers/base/core.c linux.new/drivers/base/core.c
--- linux/drivers/base/core.c	2008-05-15 16:48:31.000000000 +0800
+++ linux.new/drivers/base/core.c	2008-05-15 16:47:59.000000000 +0800
@@ -888,7 +888,7 @@ int device_add(struct device *dev)
 		klist_add_tail(&dev->knode_parent, &parent->klist_children);
 
 	if (dev->class) {
-		mutex_lock(&dev->class->mutex);
+		mutex_lock_nested(&dev->class->mutex, SINGLE_DEPTH_NESTING);
 		/* tie the class to the device */
 		list_add_tail(&dev->node, &dev->class->devices);
 
@@ -997,7 +997,7 @@ void device_del(struct device *dev)
 	if (dev->class) {
 		device_remove_class_symlinks(dev);
 
-		mutex_lock(&dev->class->mutex);
+		mutex_lock_nested(&dev->class->mutex, SINGLE_DEPTH_NESTING);
 		/* notify any interfaces that the device is now gone */
 		list_for_each_entry(class_intf, &dev->class->interfaces, node)
 			if (class_intf->remove_dev)

Re: lockdep whine in 2.6.26-rc2-mm1

[Matthew Wilcox]

On Thu, May 15, 2008 at 05:01:01PM +0800, Dave Young wrote:
> > The classes are different here, first sdev_class, then sg_sysfs_class

Oh ... right.  I misread scsi_register_interface as
class_register_interface.

> Greg, what about using mutex_lock_nested to silence lockdep? They are
> the only usage of class->mutex out of class.c

I don't see how we prove that, for example, you can never take the
sg_sysfs_class mutex and then take the sdev_class mutex.

There aren't /that/ many classes in the kernel (my laptop has 33 in
/sys/class).  How about we make each (sysfs) class its own (lockdep)
class?  That way we let lockdep do its job rather than telling it "nah,
you're all right, this is good".

I don't have a copy of the sem->mutex conversion to hand, but I imagine
you want to do something like:

 - Add a struct lock_class_key to struct class
 - In drivers/base/class.c use __mutex_init instead of mutex_init

I think that should be enough ...

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: lockdep whine in 2.6.26-rc2-mm1

[Dave Young]

On 5/15/08, Matthew Wilcox <matthew@wil.cx> wrote:
> On Thu, May 15, 2008 at 05:01:01PM +0800, Dave Young wrote:
>> > The classes are different here, first sdev_class, then sg_sysfs_class
>
> Oh ... right.  I misread scsi_register_interface as
> class_register_interface.
>
>> Greg, what about using mutex_lock_nested to silence lockdep? They are
>> the only usage of class->mutex out of class.c
>
> I don't see how we prove that, for example, you can never take the
> sg_sysfs_class mutex and then take the sdev_class mutex.

Sorry for my delay. AFAIK, there's no this kind of use.
>
> There aren't /that/ many classes in the kernel (my laptop has 33 in
> /sys/class).  How about we make each (sysfs) class its own (lockdep)
> class?  That way we let lockdep do its job rather than telling it "nah,
> you're all right, this is good".
>
> I don't have a copy of the sem->mutex conversion to hand, but I imagine
> you want to do something like:
>
>  - Add a struct lock_class_key to struct class
>  - In drivers/base/class.c use __mutex_init instead of mutex_init
>
> I think that should be enough ...

Yes, it's safer than mutex_lock_nested.  If you all have no objection
I can do the fix like this.

Thanks.

Re: lockdep whine in 2.6.26-rc2-mm1

[Dave Young]

On Sat, May 17, 2008 at 6:14 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> On 5/15/08, Matthew Wilcox <matthew@wil.cx> wrote:
>> On Thu, May 15, 2008 at 05:01:01PM +0800, Dave Young wrote:
>>> > The classes are different here, first sdev_class, then sg_sysfs_class
>>
>> Oh ... right.  I misread scsi_register_interface as
>> class_register_interface.
>>
>>> Greg, what about using mutex_lock_nested to silence lockdep? They are
>>> the only usage of class->mutex out of class.c
>>
>> I don't see how we prove that, for example, you can never take the
>> sg_sysfs_class mutex and then take the sdev_class mutex.
>
> Sorry for my delay. AFAIK, there's no this kind of use.
>>
>> There aren't /that/ many classes in the kernel (my laptop has 33 in
>> /sys/class).  How about we make each (sysfs) class its own (lockdep)
>> class?  That way we let lockdep do its job rather than telling it "nah,
>> you're all right, this is good".
>>
>> I don't have a copy of the sem->mutex conversion to hand, but I imagine
>> you want to do something like:
>>
>>  - Add a struct lock_class_key to struct class
>>  - In drivers/base/class.c use __mutex_init instead of mutex_init
>>
>> I think that should be enough ...
>
> Yes, it's safer than mutex_lock_nested.  If you all have no objection
> I can do the fix like this.
>

I rechecked the class_interface use, the users are scsi and pcmcia.

The two classes could call device_add/del while doing
class_interface_register/unregister is :
sg_sysfs_class
pcmcia_socket_class

So is it possible to reset their lock_class instead of do __mutex_init
for all classes?

Re: lockdep whine in 2.6.26-rc2-mm1

[Dave Young]

On Mon, May 19, 2008 at 1:20 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> On Sat, May 17, 2008 at 6:14 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
>> On 5/15/08, Matthew Wilcox <matthew@wil.cx> wrote:
>>> On Thu, May 15, 2008 at 05:01:01PM +0800, Dave Young wrote:
>>>> > The classes are different here, first sdev_class, then sg_sysfs_class
>>>
>>> Oh ... right.  I misread scsi_register_interface as
>>> class_register_interface.
>>>
>>>> Greg, what about using mutex_lock_nested to silence lockdep? They are
>>>> the only usage of class->mutex out of class.c
>>>
>>> I don't see how we prove that, for example, you can never take the
>>> sg_sysfs_class mutex and then take the sdev_class mutex.
>>
>> Sorry for my delay. AFAIK, there's no this kind of use.
>>>
>>> There aren't /that/ many classes in the kernel (my laptop has 33 in
>>> /sys/class).  How about we make each (sysfs) class its own (lockdep)
>>> class?  That way we let lockdep do its job rather than telling it "nah,
>>> you're all right, this is good".
>>>
>>> I don't have a copy of the sem->mutex conversion to hand, but I imagine
>>> you want to do something like:
>>>
>>>  - Add a struct lock_class_key to struct class
>>>  - In drivers/base/class.c use __mutex_init instead of mutex_init
>>>
>>> I think that should be enough ...
>>
>> Yes, it's safer than mutex_lock_nested.  If you all have no objection
>> I can do the fix like this.
>>
>
> I rechecked the class_interface use, the users are scsi and pcmcia.
>
> The two classes could call device_add/del while doing
> class_interface_register/unregister is :
> sg_sysfs_class
> pcmcia_socket_class
>
> So is it possible to reset their lock_class instead of do __mutex_init
> for all classes?
>

Like this?

1) Add macro :
+#define class_reclassify(class) \
+do { \
+       static struct lock_class_key    class_key; \
+       lockdep_set_class_and_name(&(class)->mutex, &class_key, \
+                                (class)->name); \
+} while (0)

2) Then in sg.c:
 class_reclassify(sg_sysfs_class);

3) And in pcmcia/cs.c
class_reclassify(&pcmcia_socket_class);

I have built and boot tested the patch, but due to connecting gmail
smtp failed so I cant send it via mutt today.

Re: lockdep whine in 2.6.26-rc2-mm1

[Matthew Wilcox]

On Mon, May 19, 2008 at 01:20:33PM +0800, Dave Young wrote:
> On Sat, May 17, 2008 at 6:14 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> > On 5/15/08, Matthew Wilcox <matthew@wil.cx> wrote:
> >> On Thu, May 15, 2008 at 05:01:01PM +0800, Dave Young wrote:
> >>> > The classes are different here, first sdev_class, then sg_sysfs_class
> >>
> >> Oh ... right.  I misread scsi_register_interface as
> >> class_register_interface.
> >>
> >>> Greg, what about using mutex_lock_nested to silence lockdep? They are
> >>> the only usage of class->mutex out of class.c
> >>
> >> I don't see how we prove that, for example, you can never take the
> >> sg_sysfs_class mutex and then take the sdev_class mutex.
> >
> > Sorry for my delay. AFAIK, there's no this kind of use.

The question isn't whether there is or isn't this kind of use right now.
The question is whether there might be this kind of use in the future,
and if there is, whether we'd like lockdep to warn us.

> I rechecked the class_interface use, the users are scsi and pcmcia.
> 
> The two classes could call device_add/del while doing
> class_interface_register/unregister is :
> sg_sysfs_class
> pcmcia_socket_class
> 
> So is it possible to reset their lock_class instead of do __mutex_init
> for all classes?

Again, it's not about current usage, it's about future usage.  Why do
you want all sysfs classes to have the same lockdep class?  There can
be good reasons.  For example, if two locks are conceptually the same,
keeping them in the same class helps you find AB-BA problems earlier.

Is there a reason you don't like my idea?

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: lockdep whine in 2.6.26-rc2-mm1

[Dave Young]

On Mon, May 19, 2008 at 6:23 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> On Mon, May 19, 2008 at 01:20:33PM +0800, Dave Young wrote:
>> On Sat, May 17, 2008 at 6:14 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
>> > On 5/15/08, Matthew Wilcox <matthew@wil.cx> wrote:
>> >> On Thu, May 15, 2008 at 05:01:01PM +0800, Dave Young wrote:
>> >>> > The classes are different here, first sdev_class, then sg_sysfs_class
>> >>
>> >> Oh ... right.  I misread scsi_register_interface as
>> >> class_register_interface.
>> >>
>> >>> Greg, what about using mutex_lock_nested to silence lockdep? They are
>> >>> the only usage of class->mutex out of class.c
>> >>
>> >> I don't see how we prove that, for example, you can never take the
>> >> sg_sysfs_class mutex and then take the sdev_class mutex.
>> >
>> > Sorry for my delay. AFAIK, there's no this kind of use.
>
> The question isn't whether there is or isn't this kind of use right now.
> The question is whether there might be this kind of use in the future,
> and if there is, whether we'd like lockdep to warn us.

In the future, IMHO, the class_interface should go away just as
class_device. If that happened this problem would going away as well.

>
>> I rechecked the class_interface use, the users are scsi and pcmcia.
>>
>> The two classes could call device_add/del while doing
>> class_interface_register/unregister is :
>> sg_sysfs_class
>> pcmcia_socket_class
>>
>> So is it possible to reset their lock_class instead of do __mutex_init
>> for all classes?
>
> Again, it's not about current usage, it's about future usage.  Why do
> you want all sysfs classes to have the same lockdep class?  There can
> be good reasons.  For example, if two locks are conceptually the same,
> keeping them in the same class helps you find AB-BA problems earlier.
>
> Is there a reason you don't like my idea?

Your idea is good. But as I said above, for current situation there's
no potential problems except class_interface usage, and the
class_interface will go away
 in the future.

So IMO It's not necessary to do this for all classes.

Regards
dave

Re: lockdep whine in 2.6.26-rc2-mm1

[Matthew Wilcox]

On Tue, May 20, 2008 at 09:27:59AM +0800, Dave Young wrote:
> > The question isn't whether there is or isn't this kind of use right now.
> > The question is whether there might be this kind of use in the future,
> > and if there is, whether we'd like lockdep to warn us.
> 
> In the future, IMHO, the class_interface should go away just as
> class_device. If that happened this problem would going away as well.

Go away ... and be replaced by something else, right?  I mean, it's not
like we don't have a need to bind two kinds of driver to the same
device.  So we're going to continue to have the situation where you can
register a new kind of driver and want to create new devices, so we'll
still have a nesting of some kind.

> Your idea is good. But as I said above, for current situation there's
> no potential problems except class_interface usage, and the
> class_interface will go away
>  in the future.
> 
> So IMO It's not necessary to do this for all classes.

Assuming your analysis is correct, of course.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: lockdep whine in 2.6.26-rc2-mm1

[Greg KH]

On Tue, May 20, 2008 at 09:27:59AM +0800, Dave Young wrote:
> On Mon, May 19, 2008 at 6:23 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> > On Mon, May 19, 2008 at 01:20:33PM +0800, Dave Young wrote:
> >> On Sat, May 17, 2008 at 6:14 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> >> > On 5/15/08, Matthew Wilcox <matthew@wil.cx> wrote:
> >> >> On Thu, May 15, 2008 at 05:01:01PM +0800, Dave Young wrote:
> >> >>> > The classes are different here, first sdev_class, then sg_sysfs_class
> >> >>
> >> >> Oh ... right.  I misread scsi_register_interface as
> >> >> class_register_interface.
> >> >>
> >> >>> Greg, what about using mutex_lock_nested to silence lockdep? They are
> >> >>> the only usage of class->mutex out of class.c
> >> >>
> >> >> I don't see how we prove that, for example, you can never take the
> >> >> sg_sysfs_class mutex and then take the sdev_class mutex.
> >> >
> >> > Sorry for my delay. AFAIK, there's no this kind of use.
> >
> > The question isn't whether there is or isn't this kind of use right now.
> > The question is whether there might be this kind of use in the future,
> > and if there is, whether we'd like lockdep to warn us.
> 
> In the future, IMHO, the class_interface should go away just as
> class_device. If that happened this problem would going away as well.

Patches gladly accepted to do this, but what you will end up with is
something just called a different name, yet doing the same
functionality, so you are back at square one :(

thanks,

greg k-h

Re: lockdep whine in 2.6.26-rc2-mm1

[Dave Young]

On Wed, May 21, 2008 at 2:07 AM, Greg KH <greg@kroah.com> wrote:
> On Tue, May 20, 2008 at 09:27:59AM +0800, Dave Young wrote:
>> On Mon, May 19, 2008 at 6:23 PM, Matthew Wilcox <matthew@wil.cx> wrote:
>> > On Mon, May 19, 2008 at 01:20:33PM +0800, Dave Young wrote:
>> >> On Sat, May 17, 2008 at 6:14 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
>> >> > On 5/15/08, Matthew Wilcox <matthew@wil.cx> wrote:
>> >> >> On Thu, May 15, 2008 at 05:01:01PM +0800, Dave Young wrote:
>> >> >>> > The classes are different here, first sdev_class, then sg_sysfs_class
>> >> >>
>> >> >> Oh ... right.  I misread scsi_register_interface as
>> >> >> class_register_interface.
>> >> >>
>> >> >>> Greg, what about using mutex_lock_nested to silence lockdep? They are
>> >> >>> the only usage of class->mutex out of class.c
>> >> >>
>> >> >> I don't see how we prove that, for example, you can never take the
>> >> >> sg_sysfs_class mutex and then take the sdev_class mutex.
>> >> >
>> >> > Sorry for my delay. AFAIK, there's no this kind of use.
>> >
>> > The question isn't whether there is or isn't this kind of use right now.
>> > The question is whether there might be this kind of use in the future,
>> > and if there is, whether we'd like lockdep to warn us.
>>
>> In the future, IMHO, the class_interface should go away just as
>> class_device. If that happened this problem would going away as well.
>
> Patches gladly accepted to do this, but what you will end up with is
> something just called a different name, yet doing the same
> functionality, so you are back at square one :(

Greg, do you have proposals about the class_interface removal before
this thread? or ideas?

---
Regards
dave

Re: lockdep whine in 2.6.26-rc2-mm1

[Greg KH]

On Wed, May 21, 2008 at 11:20:32AM +0800, Dave Young wrote:
> On Wed, May 21, 2008 at 2:07 AM, Greg KH <greg@kroah.com> wrote:
> > On Tue, May 20, 2008 at 09:27:59AM +0800, Dave Young wrote:
> >> On Mon, May 19, 2008 at 6:23 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> >> > On Mon, May 19, 2008 at 01:20:33PM +0800, Dave Young wrote:
> >> >> On Sat, May 17, 2008 at 6:14 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> >> >> > On 5/15/08, Matthew Wilcox <matthew@wil.cx> wrote:
> >> >> >> On Thu, May 15, 2008 at 05:01:01PM +0800, Dave Young wrote:
> >> >> >>> > The classes are different here, first sdev_class, then sg_sysfs_class
> >> >> >>
> >> >> >> Oh ... right.  I misread scsi_register_interface as
> >> >> >> class_register_interface.
> >> >> >>
> >> >> >>> Greg, what about using mutex_lock_nested to silence lockdep? They are
> >> >> >>> the only usage of class->mutex out of class.c
> >> >> >>
> >> >> >> I don't see how we prove that, for example, you can never take the
> >> >> >> sg_sysfs_class mutex and then take the sdev_class mutex.
> >> >> >
> >> >> > Sorry for my delay. AFAIK, there's no this kind of use.
> >> >
> >> > The question isn't whether there is or isn't this kind of use right now.
> >> > The question is whether there might be this kind of use in the future,
> >> > and if there is, whether we'd like lockdep to warn us.
> >>
> >> In the future, IMHO, the class_interface should go away just as
> >> class_device. If that happened this problem would going away as well.
> >
> > Patches gladly accepted to do this, but what you will end up with is
> > something just called a different name, yet doing the same
> > functionality, so you are back at square one :(
> 
> Greg, do you have proposals about the class_interface removal before
> this thread? or ideas?

Nope, I'm not doing it :)

But, like I said, you are going to end up with the same functionality,
right?  So you will have the same problem.

thanks,

greg k-h

Re: lockdep whine in 2.6.26-rc2-mm1

[Dave Young]

On Wed, May 14, 2008 at 11:56 PM, Greg KH <greg@kroah.com> wrote:
> On Wed, May 14, 2008 at 12:09:33AM -0700, Andrew Morton wrote:
>>
>> =============================================
>> [ INFO: possible recursive locking detected ]
>> 2.6.26-rc2-mm1 #15
>> ---------------------------------------------
>> modprobe/942 is trying to acquire lock:
>>  (&cls->mutex){--..}, at: [<ffffffff811b431e>] device_add+0x43d/0x57a
>>
>> but task is already holding lock:
>>  (&cls->mutex){--..}, at: [<ffffffff811b6787>] class_interface_register+0x48/0xbd
>>
>> other info that might help us debug this:
>> 1 lock held by modprobe/942:
>>  #0:  (&cls->mutex){--..}, at: [<ffffffff811b6787>] class_interface_register+0x48/0xbd
>>
>> stack backtrace:
>> Pid: 942, comm: modprobe Not tainted 2.6.26-rc2-mm1 #15
>>
>> Call Trace:
>>  [<ffffffff81056be1>] __lock_acquire+0x90d/0xc50
>>  [<ffffffff8100c50f>] ? restore_args+0x0/0x30
>>  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
>>  [<ffffffff81057276>] lock_acquire+0x91/0xb7
>>  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
>>  [<ffffffff812b23ab>] mutex_lock_nested+0xf2/0x278
>>  [<ffffffff811b431e>] ? device_add+0x43d/0x57a
>>  [<ffffffff812b3acd>] ? _spin_unlock+0x23/0x28
>>  [<ffffffff811b431e>] device_add+0x43d/0x57a
>>  [<ffffffff811b4471>] device_register+0x16/0x1b
>>  [<ffffffff811b4555>] device_create+0xdf/0x112
>>  [<ffffffff81055fdc>] ? trace_hardirqs_on+0xd/0xf
>>  [<ffffffff81055fdc>] ? trace_hardirqs_on+0xd/0xf
>>  [<ffffffff812b1fe1>] ? mutex_unlock+0x9/0xb
>>  [<ffffffff811b79b7>] ? kobj_map+0x113/0x124
>>  [<ffffffff810ac0f5>] ? exact_lock+0x0/0x14
>>  [<ffffffff810abd09>] ? exact_match+0x0/0x9
>>  [<ffffffffa00ec448>] :sg:sg_add+0x2a3/0x3bd
>>  [<ffffffff811b67b6>] class_interface_register+0x77/0xbd
>>  [<ffffffffa005d869>] :scsi_mod:scsi_register_interface+0x11/0x13
>>  [<ffffffffa00d50a3>] :sg:init_sg+0xa3/0x155
>>  [<ffffffff8105ea8f>] sys_init_module+0x1823/0x197a
>>  [<ffffffff810c45bc>] ? seq_release+0x0/0x56
>>  [<ffffffff8100bebb>] system_call_after_swapgs+0x7b/0x80
>
> I'm guessing that this is due to David's "change the semaphore to a
> mutex" patch that you have in your tree, but I refused to take as I was
> worried about just this issue :)
>
> David, any thoughts?

Yes, put them into -mm is right.
As matthew said, the class_interface_register call add_dev, then
device_add cause the issue.

Regards
dave