Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Mon, 2018-07-02 at 19:15 -0700, Linus Torvalds wrote:
> On Mon, Jul 2, 2018 at 5:57 PM Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > 
> > I'll have a look see if I can find a way to check that the sysfs dir is
> > empty without adding that childcount, that would at least alleviate
> > some of your objection, and would probably make the patch
> > smaller/simpler.
> 
> That would definitely make me happier.
> 
> Right now we already remove the actual device node sysfs associations
> synchronously with "kobject_del()" (even if it still then stays around
> as a kobject), and that does the remove for the object itself - just
> not the glue directory.
> 
> If we then just did a "if glue dir is empty, delete the glue dir too"
> in cleanup_glue_dir(), at least ther patch would be prettier.
> 
> I *think* that should be easy. Maybe. Something almost, but not quite,
> entirely unlike the below UNTESTED hack?

Yes, something like that. I have a bunch of other things I need to
attend to today, so I might not come up with a patch before some time
tomorrow but I'll have a look.
 
> It's whitespace-damaged on purpose. It's probably broken shit. DO NOT
> USE UNDER ANY CIRCUMSTANCES.  Think of it more as a "something like
> this might work, but probably doesn't". Maybe it gives you an idea,
> although that idea might be "Linus has finally lost it".
> 
> I suspect it's broken because kernfs has this notion of inactive
> nodes, so just testing for the rbtree being empty is probably wrong. I
> think maybe it needs to test for there not being any active nodes, but
> then you'd need the kernfs_mutex etc, so that would make things much
> worse.

There's also a "subdirs" in kernfs_node which we could use. That's a
bit ugly because it only account directories but we happen to know that
the gluedirs only contain directories, so it would work fine in
practice for that case.

> I really haven't touched the kernfs code much, thus the "this may be
> complete and utter garbage" warning.
> 
> Adding Tejun to the participants, he should know. Or Greg, who has
> been silent, presumably because he's still getting over his crazy
> travels.

Cheers,
Ben.

>                 Linus
> 
> ---
> 
>     --- a/drivers/base/core.c
>    +++ b/drivers/base/core.c
>    @@ -1585,6 +1585,12 @@ static inline struct kobject
> *get_glue_dir(struct device *dev)
>         return dev->kobj.parent;
>     }
> 
>    +static inline bool has_children(struct kobject *dir)
>    +{
>    +    struct kernfs_node *kn = dir->sd;
>    +    return kn && kn->dir.children.rb_node;
>    +}
>    +
>     /*
>      * make sure cleaning up dir as the last step, we need to make
>      * sure .release handler of kobject is run with holding the
>    @@ -1597,6 +1603,10 @@ static void cleanup_glue_dir(struct device
> *dev, struct kobject *glue_dir)
>                 return;
> 
>         mutex_lock(&gdp_mutex);
>    -    kobject_put(glue_dir);
>    +    if (has_children(glue_dir))
>    +            kobject_put(glue_dir);
>    +    else
>    +            kobject_del(glue_dir);
>         mutex_unlock(&gdp_mutex);
>     }