Re: [PATCH v1 2/2] mm: mTHP stats for pagecache folio allocations

[Baolin Wang]

On 2024/7/13 19:00, Ryan Roberts wrote:
> [...]
>
> > > +static int thpsize_create(int order, struct kobject *parent)
> > >    {
> > >        unsigned long size = (PAGE_SIZE << order) / SZ_1K;
> > > +    struct thpsize_child *stats;
> > >        struct thpsize *thpsize;
> > >        int ret;
> > >    +    /*
> > > +     * Each child object (currently only "stats" directory) holds a
> > > +     * reference to the top-level thpsize object, so we can drop our ref to
> > > +     * the top-level once stats is setup. Then we just need to drop a
> > > +     * reference on any children to clean everything up. We can't just use
> > > +     * the attr group name for the stats subdirectory because there may be
> > > +     * multiple attribute groups to populate inside stats and overlaying
> > > +     * using the name property isn't supported in that way; each attr group
> > > +     * name, if provided, must be unique in the parent directory.
> > > +     */
> > > +
> > >        thpsize = kzalloc(sizeof(*thpsize), GFP_KERNEL);
> > > -    if (!thpsize)
> > > -        return ERR_PTR(-ENOMEM);
> > > +    if (!thpsize) {
> > > +        ret = -ENOMEM;
> > > +        goto err;
> > > +    }
> > > +    thpsize->order = order;
> > >          ret = kobject_init_and_add(&thpsize->kobj, &thpsize_ktype, parent,
> > >                       "hugepages-%lukB", size);
> > >        if (ret) {
> > >            kfree(thpsize);
> > > -        return ERR_PTR(ret);
> > > +        goto err;
> > >        }
> > >    -    ret = sysfs_create_group(&thpsize->kobj, &thpsize_attr_group);
> > > -    if (ret) {
> > > +    stats = kzalloc(sizeof(*stats), GFP_KERNEL);
> > > +    if (!stats) {
> > >            kobject_put(&thpsize->kobj);
> > > -        return ERR_PTR(ret);
> > > +        ret = -ENOMEM;
> > > +        goto err;
> > >        }
> > >    -    ret = sysfs_create_group(&thpsize->kobj, &stats_attr_group);
> > > +    ret = kobject_init_and_add(&stats->kobj, &thpsize_child_ktype,
> > > +                   &thpsize->kobj, "stats");
> > > +    kobject_put(&thpsize->kobj);
> > >        if (ret) {
> > > -        kobject_put(&thpsize->kobj);
> > > -        return ERR_PTR(ret);
> > > +        kfree(stats);
> > > +        goto err;
> > >        }
> > >    -    thpsize->order = order;
> > > -    return thpsize;
> > > +    if (BIT(order) & THP_ORDERS_ALL_ANON) {
> > > +        ret = sysfs_create_group(&thpsize->kobj, &thpsize_attr_group);
> > > +        if (ret)
> > > +            goto err_put;
> > > +
> > > +        ret = sysfs_create_group(&stats->kobj, &stats_attr_group);
> > > +        if (ret)
> > > +            goto err_put;
> > > +    }
> > > +
> > > +    if (BIT(order) & PAGECACHE_LARGE_ORDERS) {
> > > +        ret = sysfs_create_group(&stats->kobj, &file_stats_attr_group);
> > > +        if (ret)
> > > +            goto err_put;
> > > +    }
> > > +
> > > +    list_add(&stats->node, &thpsize_child_list);
> > > +    return 0;
> > > +err_put:
> >
> > IIUC, I think you should call 'sysfs_remove_group' to remove the group before
> > putting the kobject.
>
> Are you sure about that? As I understood it, sysfs_create_group() was
> conceptually modifying the state of the kobj, so when the kobj gets destroyed,
> all its state is tidied up. __kobject_del() (called on the last kobject_put())
> calls sysfs_remove_groups() and tidies up the sysfs state as far as I can see?

IIUC, __kobject_del() only removes the ktype defaut groups by 
'sysfs_remove_groups(kobj, ktype->default_groups)', but your created 
groups are not added into the ktype->default_groups. That means you 
should mannuly remove them, or am I miss something?