[PATCH][-mm] reclassify sg_sysfs_class for lockdep

[PATCH][-mm] reclassify sg_sysfs_class for lockdep

[Dave Young]

As register sg_interface, the sg_add will be called, which then will
add device to sg_sysfs_class. This will cause lockdep warning,
please see following email

In this case the locks are from diffrent classi, one is sdev_class,
another is sg_sysfs_class 

Here reclassify the sg_sysfs_class for lockdep

Date: Wed, 14 May 2008 08:56:39 -0700
From: Greg KH <greg@kroah.com>
To: Andrew Morton <akpm@linux-foundation.org>,
	Dave Young <hidave.darkstar@gmail.com>
Cc: linux-scsi@vger.kernel.org, Kay Sievers <kay.sievers@vrfy.org>
Subject: Re: lockdep whine in 2.6.26-rc2-mm1
Message-ID: <20080514155639.GB28594@kroah.com>
References: <20080514000933.56adc131.akpm@linux-foundation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20080514000933.56adc131.akpm@linux-foundation.org>
User-Agent: Mutt/1.5.16 (2007-06-09)


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

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

---
drivers/scsi/sg.c |    6 ++++++
1 file changed, 6 insertions(+)

diff -upr linux/drivers/scsi/sg.c linux.new/drivers/scsi/sg.c
--- linux/drivers/scsi/sg.c	2008-05-27 17:09:42.000000000 +0800
+++ linux.new/drivers/scsi/sg.c	2008-05-27 17:09:51.000000000 +0800
@@ -49,6 +49,7 @@ static int sg_version_num = 30534;	/* 2 
 #include <linux/delay.h>
 #include <linux/scatterlist.h>
 #include <linux/blktrace_api.h>
+#include <linux/lockdep.h>
 
 #include "scsi.h"
 #include <scsi/scsi_dbg.h>
@@ -67,6 +68,8 @@ static int sg_proc_init(void);
 static void sg_proc_cleanup(void);
 #endif
 
+static struct lock_class_key sg_class_key;
+
 #define SG_ALLOW_DIO_DEF 0
 #define SG_ALLOW_DIO_CODE /* compile out by commenting this define */
 
@@ -1579,6 +1582,9 @@ init_sg(void)
 		rc = PTR_ERR(sg_sysfs_class);
 		goto err_out;
         }
+
+	lockdep_set_class_and_name(&sg_sysfs_class->mutex, &sg_class_key,
+						sg_sysfs_class->name);
 	sg_sysfs_valid = 1;
 	rc = scsi_register_interface(&sg_interface);
 	if (0 == rc) {

Re: [PATCH][-mm] reclassify sg_sysfs_class for lockdep

[James Bottomley]

On Tue, 2008-05-27 at 18:10 +0800, Dave Young wrote:
> As register sg_interface, the sg_add will be called, which then will
> add device to sg_sysfs_class. This will cause lockdep warning,
> please see following email
> 
> In this case the locks are from diffrent classi, one is sdev_class,
> another is sg_sysfs_class 
> 
> Here reclassify the sg_sysfs_class for lockdep

This isn't really a generic solution, is it?  It only works because we
currently only have two users of the interface functions, so if we
reclassify one they look separate to lockdep.  It will fall over again
if we ever get another one.

Surely the correct fix is to initialise lockdep for the mutex the same
way we did for the semaphore in class_register() (which does exactly the
same locking without triggering lockdep)?  That way we'll also fix the
problem for other conversions of semaphore->mutex.

James

Re: [PATCH][-mm] reclassify sg_sysfs_class for lockdep

[Matthew Wilcox]

On Tue, May 27, 2008 at 06:10:09PM +0800, Dave Young wrote:
> As register sg_interface, the sg_add will be called, which then will
> add device to sg_sysfs_class. This will cause lockdep warning,
> please see following email
> 
> In this case the locks are from diffrent classi, one is sdev_class,
> another is sg_sysfs_class 
> 
> Here reclassify the sg_sysfs_class for lockdep

Look, you've been told how to do this properly.  Send me the patch that
does the mutex conversion and I'll do a patch on top of that.  I have no
inclination to go wading in the cesspool of patches that is mm trying to
find yours.

-- 
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: [PATCH][-mm] reclassify sg_sysfs_class for lockdep

[Andrew Morton]

On Wed, 28 May 2008 08:49:43 -0600
Matthew Wilcox <matthew@wil.cx> wrote:

> On Tue, May 27, 2008 at 06:10:09PM +0800, Dave Young wrote:
> > As register sg_interface, the sg_add will be called, which then will
> > add device to sg_sysfs_class. This will cause lockdep warning,
> > please see following email
> > 
> > In this case the locks are from diffrent classi, one is sdev_class,
> > another is sg_sysfs_class 
> > 
> > Here reclassify the sg_sysfs_class for lockdep
> 
> Look, you've been told how to do this properly.  Send me the patch that
> does the mutex conversion and I'll do a patch on top of that.

Below.

>  I have no
> inclination to go wading in the cesspool of patches that is mm trying to
> find yours.

Feel free to identify the cessful patches so they can be fixed or dropped.


From: Dave Young <hidave.darkstar@gmail.com>

The class_device is already removed, so do the class->sem to mutex conversion.

Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
Cc: Greg KH <greg@kroah.com>
Cc: Kay Sievers <kay.sievers@vrfy.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/base/class.c   |   22 +++++++++++-----------
 drivers/base/core.c    |    9 ++++-----
 include/linux/device.h |    3 ++-
 3 files changed, 17 insertions(+), 17 deletions(-)

diff -puN drivers/base/class.c~struct-class-sem-to-mutex-converting drivers/base/class.c
--- a/drivers/base/class.c~struct-class-sem-to-mutex-converting
+++ a/drivers/base/class.c
@@ -143,7 +143,7 @@ int class_register(struct class *cls)
 	INIT_LIST_HEAD(&cls->devices);
 	INIT_LIST_HEAD(&cls->interfaces);
 	kset_init(&cls->class_dirs);
-	init_MUTEX(&cls->sem);
+	mutex_init(&cls->mutex);
 	error = kobject_set_name(&cls->subsys.kobj, "%s", cls->name);
 	if (error)
 		return error;
@@ -268,7 +268,7 @@ char *make_class_name(const char *name, 
  * We check the return of @fn each time. If it returns anything
  * other than 0, we break out and return that value.
  *
- * Note, we hold class->sem in this function, so it can not be
+ * Note, we hold class->mutex in this function, so it can not be
  * re-acquired in @fn, otherwise it will self-deadlocking. For
  * example, calls to add or remove class members would be verboten.
  */
@@ -280,7 +280,7 @@ int class_for_each_device(struct class *
 
 	if (!class)
 		return -EINVAL;
-	down(&class->sem);
+	mutex_lock(&class->mutex);
 	list_for_each_entry(dev, &class->devices, node) {
 		if (start) {
 			if (start == dev)
@@ -293,7 +293,7 @@ int class_for_each_device(struct class *
 		if (error)
 			break;
 	}
-	up(&class->sem);
+	mutex_unlock(&class->mutex);
 
 	return error;
 }
@@ -316,7 +316,7 @@ EXPORT_SYMBOL_GPL(class_for_each_device)
  *
  * Note, you will need to drop the reference with put_device() after use.
  *
- * We hold class->sem in this function, so it can not be
+ * We hold class->mutex in this function, so it can not be
  * re-acquired in @match, otherwise it will self-deadlocking. For
  * example, calls to add or remove class members would be verboten.
  */
@@ -330,7 +330,7 @@ struct device *class_find_device(struct 
 	if (!class)
 		return NULL;
 
-	down(&class->sem);
+	mutex_lock(&class->mutex);
 	list_for_each_entry(dev, &class->devices, node) {
 		if (start) {
 			if (start == dev)
@@ -344,7 +344,7 @@ struct device *class_find_device(struct 
 		} else
 			put_device(dev);
 	}
-	up(&class->sem);
+	mutex_unlock(&class->mutex);
 
 	return found ? dev : NULL;
 }
@@ -362,13 +362,13 @@ int class_interface_register(struct clas
 	if (!parent)
 		return -EINVAL;
 
-	down(&parent->sem);
+	mutex_lock(&parent->mutex);
 	list_add_tail(&class_intf->node, &parent->interfaces);
 	if (class_intf->add_dev) {
 		list_for_each_entry(dev, &parent->devices, node)
 			class_intf->add_dev(dev, class_intf);
 	}
-	up(&parent->sem);
+	mutex_unlock(&parent->mutex);
 
 	return 0;
 }
@@ -381,13 +381,13 @@ void class_interface_unregister(struct c
 	if (!parent)
 		return;
 
-	down(&parent->sem);
+	mutex_lock(&parent->mutex);
 	list_del_init(&class_intf->node);
 	if (class_intf->remove_dev) {
 		list_for_each_entry(dev, &parent->devices, node)
 			class_intf->remove_dev(dev, class_intf);
 	}
-	up(&parent->sem);
+	mutex_unlock(&parent->mutex);
 
 	class_put(parent);
 }
diff -puN drivers/base/core.c~struct-class-sem-to-mutex-converting drivers/base/core.c
--- a/drivers/base/core.c~struct-class-sem-to-mutex-converting
+++ a/drivers/base/core.c
@@ -20,7 +20,6 @@
 #include <linux/notifier.h>
 #include <linux/genhd.h>
 #include <linux/kallsyms.h>
-#include <linux/semaphore.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -889,7 +888,7 @@ int device_add(struct device *dev)
 		klist_add_tail(&dev->knode_parent, &parent->klist_children);
 
 	if (dev->class) {
-		down(&dev->class->sem);
+		mutex_lock(&dev->class->mutex);
 		/* tie the class to the device */
 		list_add_tail(&dev->node, &dev->class->devices);
 
@@ -897,7 +896,7 @@ int device_add(struct device *dev)
 		list_for_each_entry(class_intf, &dev->class->interfaces, node)
 			if (class_intf->add_dev)
 				class_intf->add_dev(dev, class_intf);
-		up(&dev->class->sem);
+		mutex_unlock(&dev->class->mutex);
 	}
  Done:
 	put_device(dev);
@@ -998,14 +997,14 @@ void device_del(struct device *dev)
 	if (dev->class) {
 		device_remove_class_symlinks(dev);
 
-		down(&dev->class->sem);
+		mutex_lock(&dev->class->mutex);
 		/* notify any interfaces that the device is now gone */
 		list_for_each_entry(class_intf, &dev->class->interfaces, node)
 			if (class_intf->remove_dev)
 				class_intf->remove_dev(dev, class_intf);
 		/* remove the device from the class list */
 		list_del_init(&dev->node);
-		up(&dev->class->sem);
+		mutex_unlock(&dev->class->mutex);
 	}
 	device_remove_file(dev, &uevent_attr);
 	device_remove_attrs(dev);
diff -puN include/linux/device.h~struct-class-sem-to-mutex-converting include/linux/device.h
--- a/include/linux/device.h~struct-class-sem-to-mutex-converting
+++ a/include/linux/device.h
@@ -20,6 +20,7 @@
 #include <linux/types.h>
 #include <linux/module.h>
 #include <linux/pm.h>
+#include <linux/mutex.h>
 #include <linux/semaphore.h>
 #include <asm/atomic.h>
 #include <asm/device.h>
@@ -185,7 +186,7 @@ struct class {
 	struct list_head	devices;
 	struct list_head	interfaces;
 	struct kset		class_dirs;
-	struct semaphore	sem; /* locks children, devices, interfaces */
+	struct mutex		mutex; /* locks devices, interfaces */
 	struct class_attribute		*class_attrs;
 	struct device_attribute		*dev_attrs;
 	struct kobject			*dev_kobj;
_

Re: [PATCH][-mm] reclassify sg_sysfs_class for lockdep

[Dave Young]

On Wed, May 28, 2008 at 10:40 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Tue, 2008-05-27 at 18:10 +0800, Dave Young wrote:
>> As register sg_interface, the sg_add will be called, which then will
>> add device to sg_sysfs_class. This will cause lockdep warning,
>> please see following email
>>
>> In this case the locks are from diffrent classi, one is sdev_class,
>> another is sg_sysfs_class
>>
>> Here reclassify the sg_sysfs_class for lockdep
>
> This isn't really a generic solution, is it?  It only works because we
> currently only have two users of the interface functions, so if we
> reclassify one they look separate to lockdep.  It will fall over again
> if we ever get another one.
>
> Surely the correct fix is to initialise lockdep for the mutex the same
> way we did for the semaphore in class_register() (which does exactly the
> same locking without triggering lockdep)?  That way we'll also fix the
> problem for other conversions of semaphore->mutex.

Matthew & greg did the work already.

>From my original idea I don't want to do this for all classes, and I
would think it as a rare case.

Regards
dave

Re: [PATCH][-mm] reclassify sg_sysfs_class for lockdep

[James Bottomley]

On Thu, 2008-05-29 at 08:45 +0800, Dave Young wrote:
> > This isn't really a generic solution, is it?  It only works because we
> > currently only have two users of the interface functions, so if we
> > reclassify one they look separate to lockdep.  It will fall over again
> > if we ever get another one.
> >
> > Surely the correct fix is to initialise lockdep for the mutex the same
> > way we did for the semaphore in class_register() (which does exactly the
> > same locking without triggering lockdep)?  That way we'll also fix the
> > problem for other conversions of semaphore->mutex.
> 
> Matthew & greg did the work already.

Yes, that was nice of them ...

> >From my original idea I don't want to do this for all classes, and I
> would think it as a rare case.

Hardly ... the fact that it showed up in two different cases indicated
the problem to be more generic.

The root cause of the problem was that mutexes are part of the lockdep
infrastructure whereas semaphores aren't and the dynamic initialisation
of mutexes becomes a lockdep problem unless they're keyed and
initiallised correctly. If you want to do more semaphore->mutex
conversions, it would really be useful for you to understand what was
done and why it was necessary ... because it's going to become necessary
again in others of them.

James