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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <29f0fc5a-c2b7-4925-9bdb-fd2abe5383ae@arm.com>
Date: Sat, 13 Jul 2024 12:00:17 +0100
From: Ryan Roberts <ryan.roberts@....com>
To: Baolin Wang <baolin.wang@...ux.alibaba.com>,
 Andrew Morton <akpm@...ux-foundation.org>, Hugh Dickins <hughd@...gle.com>,
 Jonathan Corbet <corbet@....net>,
 "Matthew Wilcox (Oracle)" <willy@...radead.org>,
 David Hildenbrand <david@...hat.com>, Barry Song <baohua@...nel.org>,
 Lance Yang <ioworker0@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v1 2/2] mm: mTHP stats for pagecache folio allocations

[...]

>> +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?

> 
>> +    kobject_put(&stats->kobj);
>> +err:
>> +    return ret;
>>   }