diff mbox

[1/2] kobject: remove kset from sysfs immediately in kset_unregister()

Message ID 20131203224143.GA19962@google.com
State Not Applicable
Headers show

Commit Message

Bjorn Helgaas Dec. 3, 2013, 10:41 p.m. UTC 
[+cc Matt, Tejun]

On Tue, Dec 03, 2013 at 10:04:25AM -0800, Greg Kroah-Hartman wrote:
> On Tue, Oct 08, 2013 at 02:20:16PM -0600, Bjorn Helgaas wrote:
> > There's no explicit "unlink from sysfs" interface for ksets, so I think
> > callers of kset_unregister() expect the kset to be removed from sysfs
> > immediately, without waiting for the last reference to be released.
> > 
> > This patch makes the sysfs removal happen immediately, so the caller may
> > create a new kset with the same name as soon as kset_unregister() returns.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> With the PCI MSI attribute change, this patch is no longer needed,
> right?

Well, yes and no.  The attached patch extends Russell's delayed kobject
release to make the delay somewhat random.  I *think* that should be
valid, but correct me if I'm wrong.

With the random delay patch, it's easy to trigger the "sysfs: cannot
create duplicate filename" error, e.g., by unloading and reloading the
edd module, because the module unload only waits for the /sys/module/edd
object to be released.  Other objects, such as the /sys/firmware/edd
kset, may be released later.

Modules like edd could be changed to explicitly call kobject_del() on
the kset kobject.  Maybe that's a better approach, so we consistently
use kobject_del() to remove things from sysfs.  But I don't know whether
it's really useful to allow the kset to hang around in sysfs after
kset_unregister(), and it's sort of subtle to track down problems like
this.

Several other kset_unregister() callers have this situation, so if we
don't change it to call kobject_del() internally, maybe we should add a
note to Documentation/kobject.txt about calling kobject_del() first.
The existing hint only talks about using it when you need a two-stage
delete, which isn't really the case here.

If we *do* decide to change kset_unregister(), I have an updated
documentation patch that makes it more clear that the release may
happen after kset_unregister() returns.

Bjorn

> > ---
> >  Documentation/kobject.txt |    3 ++-
> >  lib/kobject.c             |    1 +
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/kobject.txt b/Documentation/kobject.txt
> > index c5182bb..8e8b501 100644
> > --- a/Documentation/kobject.txt
> > +++ b/Documentation/kobject.txt
> > @@ -342,7 +342,8 @@ kset use:
> >  
> >  When you are finished with the kset, call:
> >    void kset_unregister(struct kset *kset);
> > -to destroy it.
> > +to destroy it.  This removes the kset from sysfs and, after the kset
> > +reference count goes to zero, releases it.
> >  
> >  An example of using a kset can be seen in the
> >  samples/kobject/kset-example.c file in the kernel tree.
> > diff --git a/lib/kobject.c b/lib/kobject.c
> > index 9621751..9098992 100644
> > --- a/lib/kobject.c
> > +++ b/lib/kobject.c
> > @@ -753,6 +753,7 @@ void kset_unregister(struct kset *k)
> >  {
> >  	if (!k)
> >  		return;
> > +	kobject_del(&k->kobj);
> >  	kobject_put(&k->kobj);
> >  }
> >  



kobject: delay kobject release for random time

From: Bjorn Helgaas <bhelgaas@google.com>

This delays kobject release functions for a random time between 1 and 8
seconds, which effectively changes the order in which they're called.
---
 lib/kobject.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Greg KH Dec. 6, 2013, 12:01 a.m. UTC | #1 
On Tue, Dec 03, 2013 at 03:41:43PM -0700, Bjorn Helgaas wrote:
> [+cc Matt, Tejun]
> 
> On Tue, Dec 03, 2013 at 10:04:25AM -0800, Greg Kroah-Hartman wrote:
> > On Tue, Oct 08, 2013 at 02:20:16PM -0600, Bjorn Helgaas wrote:
> > > There's no explicit "unlink from sysfs" interface for ksets, so I think
> > > callers of kset_unregister() expect the kset to be removed from sysfs
> > > immediately, without waiting for the last reference to be released.
> > > 
> > > This patch makes the sysfs removal happen immediately, so the caller may
> > > create a new kset with the same name as soon as kset_unregister() returns.
> > > 
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > With the PCI MSI attribute change, this patch is no longer needed,
> > right?
> 
> Well, yes and no.  The attached patch extends Russell's delayed kobject
> release to make the delay somewhat random.  I *think* that should be
> valid, but correct me if I'm wrong.

Heh, I like it, mind if I queue it up?

> With the random delay patch, it's easy to trigger the "sysfs: cannot
> create duplicate filename" error, e.g., by unloading and reloading the
> edd module, because the module unload only waits for the /sys/module/edd
> object to be released.  Other objects, such as the /sys/firmware/edd
> kset, may be released later.
> 
> Modules like edd could be changed to explicitly call kobject_del() on
> the kset kobject.  Maybe that's a better approach, so we consistently
> use kobject_del() to remove things from sysfs.  But I don't know whether
> it's really useful to allow the kset to hang around in sysfs after
> kset_unregister(), and it's sort of subtle to track down problems like
> this.
> 
> Several other kset_unregister() callers have this situation, so if we
> don't change it to call kobject_del() internally, maybe we should add a
> note to Documentation/kobject.txt about calling kobject_del() first.
> The existing hint only talks about using it when you need a two-stage
> delete, which isn't really the case here.
> 
> If we *do* decide to change kset_unregister(), I have an updated
> documentation patch that makes it more clear that the release may
> happen after kset_unregister() returns.

Ok, I'll take this patch, it makes sense on the module unload path for
example.  Thanks for the explanation.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Dec. 6, 2013, 12:34 a.m. UTC | #2 
On Thu, Dec 5, 2013 at 5:01 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Dec 03, 2013 at 03:41:43PM -0700, Bjorn Helgaas wrote:
>> [+cc Matt, Tejun]
>>
>> On Tue, Dec 03, 2013 at 10:04:25AM -0800, Greg Kroah-Hartman wrote:
>> > On Tue, Oct 08, 2013 at 02:20:16PM -0600, Bjorn Helgaas wrote:
>> > > There's no explicit "unlink from sysfs" interface for ksets, so I think
>> > > callers of kset_unregister() expect the kset to be removed from sysfs
>> > > immediately, without waiting for the last reference to be released.
>> > >
>> > > This patch makes the sysfs removal happen immediately, so the caller may
>> > > create a new kset with the same name as soon as kset_unregister() returns.
>> > >
>> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> >
>> > With the PCI MSI attribute change, this patch is no longer needed,
>> > right?
>>
>> Well, yes and no.  The attached patch extends Russell's delayed kobject
>> release to make the delay somewhat random.  I *think* that should be
>> valid, but correct me if I'm wrong.
>
> Heh, I like it, mind if I queue it up?

Sure, that'd be fine.  I'll resend these two patches with my sign-off
and updated changelogs.

>> With the random delay patch, it's easy to trigger the "sysfs: cannot
>> create duplicate filename" error, e.g., by unloading and reloading the
>> edd module, because the module unload only waits for the /sys/module/edd
>> object to be released.  Other objects, such as the /sys/firmware/edd
>> kset, may be released later.
>>
>> Modules like edd could be changed to explicitly call kobject_del() on
>> the kset kobject.  Maybe that's a better approach, so we consistently
>> use kobject_del() to remove things from sysfs.  But I don't know whether
>> it's really useful to allow the kset to hang around in sysfs after
>> kset_unregister(), and it's sort of subtle to track down problems like
>> this.
>>
>> Several other kset_unregister() callers have this situation, so if we
>> don't change it to call kobject_del() internally, maybe we should add a
>> note to Documentation/kobject.txt about calling kobject_del() first.
>> The existing hint only talks about using it when you need a two-stage
>> delete, which isn't really the case here.
>>
>> If we *do* decide to change kset_unregister(), I have an updated
>> documentation patch that makes it more clear that the release may
>> happen after kset_unregister() returns.
>
> Ok, I'll take this patch, it makes sense on the module unload path for
> example.  Thanks for the explanation.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/lib/kobject.c b/lib/kobject.c
index 5b4b8886435e..c87df2f7dbff 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -18,6 +18,7 @@ 
 #include <linux/export.h>
 #include <linux/stat.h>
 #include <linux/slab.h>
+#include <linux/random.h>
 
 /**
  * kobject_namespace - return @kobj's namespace tag
@@ -625,10 +626,13 @@  static void kobject_release(struct kref *kref)
 {
 	struct kobject *kobj = container_of(kref, struct kobject, kref);
 #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
-	pr_info("kobject: '%s' (%p): %s, parent %p (delayed)\n",
-		 kobject_name(kobj), kobj, __func__, kobj->parent);
+	unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
+
+	pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
+		 kobject_name(kobj), kobj, __func__, kobj->parent, delay);
 	INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
-	schedule_delayed_work(&kobj->release, HZ);
+
+	schedule_delayed_work(&kobj->release, delay);
 #else
 	kobject_cleanup(kobj);
 #endif