On Fri, 27 Apr 2012, Tejun Heo wrote:
> > Would it be better to release just the lockdep annotation while
> > continuing to hold the actual lock, or to really drop the lock?
>
> Just the lockdep annotation, I think.
This is turning out to be harder than it looked.
In order to release the lockdep annotation, I need the lockdep_map
which is stored in the sysfs_dirent structure. But when the attribute
method is called, all it is given is a pointer to the attribute itself
(which contains the lockdep_class_key but not the lockdep_map) and a
pointer to the corresponding kobject.
Is there any reasonable way to get from the kobject and the attribute
to the appropriate sysfs_dirent? Search through all the groups
attached to the kobject? Restrict the new interface so that it can be
used only by attributes at the kobject's top level (i.e., not in a
named group)?
Any suggestions?
Alan Stern
On Thu, May 03, 2012 at 05:30:30PM -0400, Alan Stern wrote:
> On Fri, 27 Apr 2012, Tejun Heo wrote:
>
> > > Would it be better to release just the lockdep annotation while
> > > continuing to hold the actual lock, or to really drop the lock?
> >
> > Just the lockdep annotation, I think.
>
> This is turning out to be harder than it looked.
>
> In order to release the lockdep annotation, I need the lockdep_map
> which is stored in the sysfs_dirent structure. But when the attribute
> method is called, all it is given is a pointer to the attribute itself
> (which contains the lockdep_class_key but not the lockdep_map) and a
> pointer to the corresponding kobject.
>
> Is there any reasonable way to get from the kobject and the attribute
> to the appropriate sysfs_dirent? Search through all the groups
> attached to the kobject? Restrict the new interface so that it can be
> used only by attributes at the kobject's top level (i.e., not in a
> named group)?
>
> Any suggestions?
Urgh... I can't think of anything pretty. How about just marking the
attr as "I'm special" and let sysfs code override lockdep annotation?
Thanks.
--
tejun
On Fri, 4 May 2012, Tejun Heo wrote:
> > Is there any reasonable way to get from the kobject and the attribute
> > to the appropriate sysfs_dirent? Search through all the groups
> > attached to the kobject? Restrict the new interface so that it can be
> > used only by attributes at the kobject's top level (i.e., not in a
> > named group)?
> >
> > Any suggestions?
>
> Urgh... I can't think of anything pretty. How about just marking the
> attr as "I'm special" and let sysfs code override lockdep annotation?
Never mind, I figured it out. The caller knows what the group is (if
there's a group), so it can simply pass that information along.
To make things clearer, I broke the interface into two parts: one to
find the sysfs_dirent and the other to handle the lockdep annotations.
Here's the patch -- it works. What do you think?
Alan Stern
P.S.: I'm not sure about the namespace arguments to sysfs_get_dirent().
Is NULL appropriate? Or should I use sysfs_ns_type(kobj->sd)?
Index: usb-3.4/include/linux/sysfs.h
===================================================================
--- usb-3.4.orig/include/linux/sysfs.h
+++ usb-3.4/include/linux/sysfs.h
@@ -121,6 +121,10 @@ struct sysfs_dirent;
int sysfs_schedule_callback(struct kobject *kobj, void (*func)(void *),
void *data, struct module *owner);
+struct sysfs_dirent *sysfs_find_dirent_for_attr(struct kobject *kobj,
+ const struct attribute *attr, const char *group);
+void sysfs_unannotate_readlock(struct sysfs_dirent *sd);
+void sysfs_reannotate_readlock(struct sysfs_dirent *sd);
int __must_check sysfs_create_dir(struct kobject *kobj);
void sysfs_remove_dir(struct kobject *kobj);
@@ -188,6 +192,21 @@ static inline int sysfs_schedule_callbac
return -ENOSYS;
}
+static inline struct sysfs_dirent *sysfs_find_dirent_for_attr(
+ struct kobject *kobj,
+ const struct attribute *attr, const char *group)
+{
+ return NULL;
+}
+
+static inline void sysfs_unannotate_readlock(struct sysfs_dirent *sd)
+{
+}
+
+static inline void sysfs_reannotate_readlock(struct sysfs_dirent *sd)
+{
+}
+
static inline int sysfs_create_dir(struct kobject *kobj)
{
return 0;
Index: usb-3.4/include/linux/device.h
===================================================================
--- usb-3.4.orig/include/linux/device.h
+++ usb-3.4/include/linux/device.h
@@ -514,6 +514,11 @@ extern void device_remove_bin_file(struc
const struct bin_attribute *attr);
extern int device_schedule_callback_owner(struct device *dev,
void (*func)(struct device *dev), struct module *owner);
+extern void *device_start_attribute_infanticide(
+ struct device *dev, const struct device_attribute *attr,
+ const char *group);
+extern void device_end_attribute_infanticide(void *cookie);
+
/* This is a macro to avoid include problems with THIS_MODULE */
#define device_schedule_callback(dev, func) \
Index: usb-3.4/fs/sysfs/dir.c
===================================================================
--- usb-3.4.orig/fs/sysfs/dir.c
+++ usb-3.4/fs/sysfs/dir.c
@@ -133,6 +133,74 @@ static void sysfs_unlink_sibling(struct
}
/**
+ * sysfs_find_dirent_for_attr - find sysfs_dirent for a given kobject and attribute
+ * @kobj: kobject the attribute is attached to
+ * @attr: attribute whose sysfs_dirent should be found
+ * @group: name of the group containing @attr, or NULL
+ *
+ * Looks up and returns a pointer to the sysfs_dirent structure
+ * corresponding to the instance of @attr in @group attached to @kobj.
+ * Returns NULL if the sysfs_dirent cannot be found.
+ *
+ * Retains a reference to the returned sysfs_dirent. The caller must
+ * release this reference, as done by sysfs_reannotate_readlock()
+ * below.
+ */
+struct sysfs_dirent *sysfs_find_dirent_for_attr(struct kobject *kobj,
+ const struct attribute *attr, const char *group)
+{
+ struct sysfs_dirent *dir_sd;
+ struct sysfs_dirent *sd;
+
+ if (group)
+ dir_sd = sysfs_get_dirent(kobj->sd, NULL, group);
+ else
+ dir_sd = sysfs_get(kobj->sd);
+ if (!dir_sd)
+ return NULL;
+
+ sd = sysfs_get_dirent(dir_sd, NULL, attr->name);
+ sysfs_put(dir_sd);
+ return sd;
+}
+
+/**
+ * sysfs_unannotate_readlock - drop the lockdep annotation for an attribute
+ * @sd: sysfs_dirent for the attribute whose lock annotation is dropped
+ *
+ * Lockdep is not able to cope with the tree-structured locks used
+ * by the device core. It considers all the s_active lock associated
+ * a particular attribute to be the same. This causes a false
+ * positive report when a method for an attribute attached to a device
+ * wants to remove the same attribute attached to a child device.
+ *
+ * To avoid such false positives, the method can call this routine to
+ * drop the lockdep annotation indicating that the attribute's dirent is
+ * currently locked, remove the child's attribute, and then reacquire
+ * the annotation.
+ */
+void sysfs_unannotate_readlock(struct sysfs_dirent *sd)
+{
+ rwsem_release(&sd->dep_map, 1, _RET_IP_);
+}
+
+/**
+ * sysfs_reannotate_readlock - reacquire the lockdep annotation for an attribute
+ * @sd: sysfs_dirent for the attribute whose lock annotation is reacquired
+ *
+ * After an attribute method has called sysfs_unannotate_readlock()
+ * and removed the attributes from various child devices, it should
+ * call this routine to reacquire the lockdep annotation (and also to
+ * drop the reference to @sd obtained by
+ * sysfs_get_dirent_for_attribute()) before returning.
+ */
+void sysfs_reannotate_readlock(struct sysfs_dirent *sd)
+{
+ rwsem_acquire_read(&sd->dep_map, 0, 1, _RET_IP_);
+ sysfs_put(sd);
+}
+
+/**
* sysfs_get_active - get an active reference to sysfs_dirent
* @sd: sysfs_dirent to get an active reference to
*
Index: usb-3.4/drivers/base/core.c
===================================================================
--- usb-3.4.orig/drivers/base/core.c
+++ usb-3.4/drivers/base/core.c
@@ -609,6 +609,48 @@ int device_schedule_callback_owner(struc
}
EXPORT_SYMBOL_GPL(device_schedule_callback_owner);
+/**
+ * device_start_attribute_infanticide - prepare to remove child attributes
+ * @dev: device passed to the calling method
+ * @attr: attribute passed to the calling method
+ * @group: name of the group containing @attr, or NULL
+ *
+ * To avoid false positive reports from lockdep, this routine should be
+ * called by an attribute method that wants to remove the same attribute
+ * from a descendant of the device for which it was called (an
+ * "infanticidal" attribute).
+ *
+ * The returned value should be treated as an opaque cookie and passed to
+ * device_end_attribute_infanticide() below after the descendant
+ * attributes have been removed.
+ */
+void *device_start_attribute_infanticide(
+ struct device *dev, const struct device_attribute *attr,
+ const char *group)
+{
+ struct sysfs_dirent *sd;
+
+ sd = sysfs_find_dirent_for_attr(&dev->kobj, &attr->attr, group);
+ if (sd)
+ sysfs_unannotate_readlock(sd);
+ return sd;
+}
+EXPORT_SYMBOL_GPL(device_start_attribute_infanticide);
+
+/**
+ * device_end_attribute_infanticide - finish removing child attributes
+ * @cookie: value returned by device_start_attribute_infanticide() above
+ *
+ * This routine should be called by attribute methods after they have
+ * finished removing the corresponding attributes from descendant devices.
+ */
+void device_end_attribute_infanticide(void *cookie)
+{
+ if (cookie)
+ sysfs_reannotate_readlock(cookie);
+}
+EXPORT_SYMBOL_GPL(device_end_attribute_infanticide);
+
static void klist_children_get(struct klist_node *n)
{
struct device_private *p = to_device_private_parent(n);
Index: usb-3.4/drivers/usb/core/sysfs.c
===================================================================
--- usb-3.4.orig/drivers/usb/core/sysfs.c
+++ usb-3.4/drivers/usb/core/sysfs.c
@@ -64,11 +64,14 @@ set_bConfigurationValue(struct device *d
{
struct usb_device *udev = to_usb_device(dev);
int config, value;
+ void *cookie;
if (sscanf(buf, "%d", &config) != 1 || config < -1 || config > 255)
return -EINVAL;
usb_lock_device(udev);
+ cookie = device_start_attribute_infanticide(dev, attr, NULL);
value = usb_set_configuration(udev, config);
+ device_end_attribute_infanticide(cookie);
usb_unlock_device(udev);
return (value < 0) ? value : count;
}
@@ -588,10 +591,15 @@ static ssize_t usb_dev_authorized_store(
result = sscanf(buf, "%u\n", &val);
if (result != 1)
result = -EINVAL;
- else if (val == 0)
+ else if (val == 0) {
+ void *cookie;
+
+ cookie = device_start_attribute_infanticide(dev, attr, NULL);
result = usb_deauthorize_device(usb_dev);
- else
+ device_end_attribute_infanticide(cookie);
+ } else {
result = usb_authorize_device(usb_dev);
+ }
return result < 0? result : size;
}
@@ -605,12 +613,15 @@ static ssize_t usb_remove_store(struct d
{
struct usb_device *udev = to_usb_device(dev);
int rc = 0;
+ void *cookie;
usb_lock_device(udev);
if (udev->state != USB_STATE_NOTATTACHED) {
/* To avoid races, first unconfigure and then remove */
+ cookie = device_start_attribute_infanticide(dev, attr, NULL);
usb_set_configuration(udev, -1);
+ device_end_attribute_infanticide(cookie);
rc = usb_remove_device(udev);
}
if (rc == 0)
Hello, Alan.
On Fri, May 04, 2012 at 03:08:53PM -0400, Alan Stern wrote:
> @@ -588,10 +591,15 @@ static ssize_t usb_dev_authorized_store(
> result = sscanf(buf, "%u\n", &val);
> if (result != 1)
> result = -EINVAL;
> - else if (val == 0)
> + else if (val == 0) {
> + void *cookie;
> +
> + cookie = device_start_attribute_infanticide(dev, attr, NULL);
> result = usb_deauthorize_device(usb_dev);
> - else
> + device_end_attribute_infanticide(cookie);
> + } else {
> result = usb_authorize_device(usb_dev);
> + }
I *think* it looks way too huge as lockdep workaround. We're adding a
whole separate lookup interface for this. If looking up afterwards is
difficult, can't we get away with adding a field in struct attribute?
Thanks.
--
tejun
On Mon, 7 May 2012, Tejun Heo wrote:
> Hello, Alan.
Hi.
> On Fri, May 04, 2012 at 03:08:53PM -0400, Alan Stern wrote:
> > @@ -588,10 +591,15 @@ static ssize_t usb_dev_authorized_store(
> > result = sscanf(buf, "%u\n", &val);
> > if (result != 1)
> > result = -EINVAL;
> > - else if (val == 0)
> > + else if (val == 0) {
> > + void *cookie;
> > +
> > + cookie = device_start_attribute_infanticide(dev, attr, NULL);
> > result = usb_deauthorize_device(usb_dev);
> > - else
> > + device_end_attribute_infanticide(cookie);
> > + } else {
> > result = usb_authorize_device(usb_dev);
> > + }
>
> I *think* it looks way too huge as lockdep workaround. We're adding a
Well, it's not _terribly_ huge. As far as the driver is concerned,
it's just a start and an end call. The lookup interface isn't even
EXPORTed, because its only user is the device core.
> whole separate lookup interface for this. If looking up afterwards is
> difficult, can't we get away with adding a field in struct attribute?
You mean, an "ignore this attribute for lockdep purposes" flag? Yes,
that would work just as well.
I guess in the end it's a question of balance. Which has more
overhead, adding a few function calls here and there, or adding a new
flags field to every struct attribute?
Alan Stern
On Mon, May 07, 2012 at 05:51:52PM -0400, Alan Stern wrote:
> I guess in the end it's a question of balance. Which has more
> overhead, adding a few function calls here and there, or adding a new
> flags field to every struct attribute?
Yes, and there are different types of overheads. I'm happy to trade
some runtime memory overhead under debugging mode for lower code
complexity. Lock proving is pretty expensive anyway. I don't think
there's much point in trying to optimize some bytes from struct
attributes.
Thanks.
--
tejun
On Mon, 7 May 2012, Tejun Heo wrote:
> On Mon, May 07, 2012 at 05:51:52PM -0400, Alan Stern wrote:
> > I guess in the end it's a question of balance. Which has more
> > overhead, adding a few function calls here and there, or adding a new
> > flags field to every struct attribute?
>
> Yes, and there are different types of overheads. I'm happy to trade
> some runtime memory overhead under debugging mode for lower code
> complexity. Lock proving is pretty expensive anyway. I don't think
> there's much point in trying to optimize some bytes from struct
> attributes.
Okay, then what do you think about this approach? It does seem smaller
and simpler than the previous attempt.
And I did try to avoid unnecessary bloat; if lockdep isn't being used
then the extra attribute flag isn't present.
Alan Stern
Index: usb-3.4/include/linux/sysfs.h
===================================================================
--- usb-3.4.orig/include/linux/sysfs.h
+++ usb-3.4/include/linux/sysfs.h
@@ -27,6 +27,7 @@ struct attribute {
const char *name;
umode_t mode;
#ifdef CONFIG_DEBUG_LOCK_ALLOC
+ bool ignore_lockdep:1;
struct lock_class_key *key;
struct lock_class_key skey;
#endif
@@ -80,6 +81,17 @@ struct attribute_group {
#define __ATTR_NULL { .attr = { .name = NULL } }
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+#define __ATTR_IGNORE_LOCKDEP(_name,_mode,_show,_store) { \
+ .attr = {.name = __stringify(_name), .mode = _mode, \
+ .ignore_lockdep = true }, \
+ .show = _show, \
+ .store = _store, \
+}
+#else
+#define __ATTR_IGNORE_LOCKDEP __ATTR
+#endif
+
#define attr_name(_attr) (_attr).attr.name
struct file;
Index: usb-3.4/fs/sysfs/dir.c
===================================================================
--- usb-3.4.orig/fs/sysfs/dir.c
+++ usb-3.4/fs/sysfs/dir.c
@@ -132,6 +132,24 @@ static void sysfs_unlink_sibling(struct
rb_erase(&sd->s_rb, &sd->s_parent->s_dir.children);
}
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+
+/* Test for attributes that want to ignore lockdep for read-locking */
+static bool ignore_lockdep(struct sysfs_dirent *sd)
+{
+ return sysfs_type(sd) == SYSFS_KOBJ_ATTR &&
+ sd->s_attr.attr->ignore_lockdep;
+}
+
+#else
+
+static inline bool ignore_lockdep(struct sysfs_dirent *sd)
+{
+ return true;
+}
+
+#endif
+
/**
* sysfs_get_active - get an active reference to sysfs_dirent
* @sd: sysfs_dirent to get an active reference to
@@ -155,15 +173,17 @@ struct sysfs_dirent *sysfs_get_active(st
return NULL;
t = atomic_cmpxchg(&sd->s_active, v, v + 1);
- if (likely(t == v)) {
- rwsem_acquire_read(&sd->dep_map, 0, 1, _RET_IP_);
- return sd;
- }
+ if (likely(t == v))
+ break;
if (t < 0)
return NULL;
cpu_relax();
}
+
+ if (likely(!ignore_lockdep(sd)))
+ rwsem_acquire_read(&sd->dep_map, 0, 1, _RET_IP_);
+ return sd;
}
/**
@@ -180,7 +200,8 @@ void sysfs_put_active(struct sysfs_diren
if (unlikely(!sd))
return;
- rwsem_release(&sd->dep_map, 1, _RET_IP_);
+ if (likely(!ignore_lockdep(sd)))
+ rwsem_release(&sd->dep_map, 1, _RET_IP_);
v = atomic_dec_return(&sd->s_active);
if (likely(v != SD_DEACTIVATED_BIAS))
return;
Index: usb-3.4/include/linux/device.h
===================================================================
--- usb-3.4.orig/include/linux/device.h
+++ usb-3.4/include/linux/device.h
@@ -503,6 +503,9 @@ ssize_t device_store_int(struct device *
#define DEVICE_INT_ATTR(_name, _mode, _var) \
struct dev_ext_attribute dev_attr_##_name = \
{ __ATTR(_name, _mode, device_show_ulong, device_store_ulong), &(_var) }
+#define DEVICE_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
+ struct device_attribute dev_attr_##_name = \
+ __ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
extern int device_create_file(struct device *device,
const struct device_attribute *entry);
Index: usb-3.4/drivers/usb/core/sysfs.c
===================================================================
--- usb-3.4.orig/drivers/usb/core/sysfs.c
+++ usb-3.4/drivers/usb/core/sysfs.c
@@ -73,7 +73,7 @@ set_bConfigurationValue(struct device *d
return (value < 0) ? value : count;
}
-static DEVICE_ATTR(bConfigurationValue, S_IRUGO | S_IWUSR,
+static DEVICE_ATTR_IGNORE_LOCKDEP(bConfigurationValue, S_IRUGO | S_IWUSR,
show_bConfigurationValue, set_bConfigurationValue);
/* String fields */
@@ -595,7 +595,7 @@ static ssize_t usb_dev_authorized_store(
return result < 0? result : size;
}
-static DEVICE_ATTR(authorized, 0644,
+static DEVICE_ATTR_IGNORE_LOCKDEP(authorized, 0644,
usb_dev_authorized_show, usb_dev_authorized_store);
/* "Safely remove a device" */
@@ -618,7 +618,7 @@ static ssize_t usb_remove_store(struct d
usb_unlock_device(udev);
return rc;
}
-static DEVICE_ATTR(remove, 0200, NULL, usb_remove_store);
+static DEVICE_ATTR_IGNORE_LOCKDEP(remove, 0200, NULL, usb_remove_store);
static struct attribute *dev_attrs[] = {
Hello,
On Tue, May 08, 2012 at 02:53:11PM -0400, Alan Stern wrote:
> On Mon, 7 May 2012, Tejun Heo wrote:
>
> > On Mon, May 07, 2012 at 05:51:52PM -0400, Alan Stern wrote:
> > > I guess in the end it's a question of balance. Which has more
> > > overhead, adding a few function calls here and there, or adding a new
> > > flags field to every struct attribute?
> >
> > Yes, and there are different types of overheads. I'm happy to trade
> > some runtime memory overhead under debugging mode for lower code
> > complexity. Lock proving is pretty expensive anyway. I don't think
> > there's much point in trying to optimize some bytes from struct
> > attributes.
>
> Okay, then what do you think about this approach? It does seem smaller
> and simpler than the previous attempt.
>
> And I did try to avoid unnecessary bloat; if lockdep isn't being used
> then the extra attribute flag isn't present.
Yeap, looks good to me.
Thanks.
--
tejun
On Wed, 9 May 2012, Tejun Heo wrote:
> Hello,
>
> On Tue, May 08, 2012 at 02:53:11PM -0400, Alan Stern wrote:
> > On Mon, 7 May 2012, Tejun Heo wrote:
> >
> > > On Mon, May 07, 2012 at 05:51:52PM -0400, Alan Stern wrote:
> > > > I guess in the end it's a question of balance. Which has more
> > > > overhead, adding a few function calls here and there, or adding a new
> > > > flags field to every struct attribute?
> > >
> > > Yes, and there are different types of overheads. I'm happy to trade
> > > some runtime memory overhead under debugging mode for lower code
> > > complexity. Lock proving is pretty expensive anyway. I don't think
> > > there's much point in trying to optimize some bytes from struct
> > > attributes.
> >
> > Okay, then what do you think about this approach? It does seem smaller
> > and simpler than the previous attempt.
> >
> > And I did try to avoid unnecessary bloat; if lockdep isn't being used
> > then the extra attribute flag isn't present.
>
> Yeap, looks good to me.
Unless there are any objections from Eric or Peter in the next few
days, I'll submit it. Can I add your Acked-by?
Alan Stern
On Wed, May 09, 2012 at 01:47:34PM -0400, Alan Stern wrote:
> On Wed, 9 May 2012, Tejun Heo wrote:
>
> > Hello,
> >
> > On Tue, May 08, 2012 at 02:53:11PM -0400, Alan Stern wrote:
> > > On Mon, 7 May 2012, Tejun Heo wrote:
> > >
> > > > On Mon, May 07, 2012 at 05:51:52PM -0400, Alan Stern wrote:
> > > > > I guess in the end it's a question of balance. Which has more
> > > > > overhead, adding a few function calls here and there, or adding a new
> > > > > flags field to every struct attribute?
> > > >
> > > > Yes, and there are different types of overheads. I'm happy to trade
> > > > some runtime memory overhead under debugging mode for lower code
> > > > complexity. Lock proving is pretty expensive anyway. I don't think
> > > > there's much point in trying to optimize some bytes from struct
> > > > attributes.
> > >
> > > Okay, then what do you think about this approach? It does seem smaller
> > > and simpler than the previous attempt.
> > >
> > > And I did try to avoid unnecessary bloat; if lockdep isn't being used
> > > then the extra attribute flag isn't present.
> >
> > Yeap, looks good to me.
>
> Unless there are any objections from Eric or Peter in the next few
> days, I'll submit it. Can I add your Acked-by?
Sure.
Thanks for the persistence.
--
tejun