From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id B2908C3DA59 for ; Mon, 22 Jul 2024 03:52:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 178A86B0082; Sun, 21 Jul 2024 23:52:18 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 100696B0083; Sun, 21 Jul 2024 23:52:18 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EE32B6B0085; Sun, 21 Jul 2024 23:52:17 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id CE0126B0082 for ; Sun, 21 Jul 2024 23:52:17 -0400 (EDT) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 4A047814A7 for ; Mon, 22 Jul 2024 03:52:17 +0000 (UTC) X-FDA: 82366015914.02.5F41223 Received: from out30-133.freemail.mail.aliyun.com (out30-133.freemail.mail.aliyun.com [115.124.30.133]) by imf19.hostedemail.com (Postfix) with ESMTP id 7AAA51A0015 for ; Mon, 22 Jul 2024 03:52:14 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=linux.alibaba.com header.s=default header.b=d+P1NTNQ; dmarc=pass (policy=none) header.from=linux.alibaba.com; spf=pass (imf19.hostedemail.com: domain of baolin.wang@linux.alibaba.com designates 115.124.30.133 as permitted sender) smtp.mailfrom=baolin.wang@linux.alibaba.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721620299; a=rsa-sha256; cv=none; b=Prl2B5YtDgSKEi4So1Edzn4dMmb1BO/6VRlBshOYTcVhYQPEV9/15PmiQnNiJ5+RurKWYy OWJzrJGydwcMdYO938KtRKzpEfgDFQYMtlsFsdZ3+ogJ8JR3U1Wu5Jhc9eQsddBQwgp0/n DPBKXc6O9MDFOTWyNOtVcz/ZqEjg4zo= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=linux.alibaba.com header.s=default header.b=d+P1NTNQ; dmarc=pass (policy=none) header.from=linux.alibaba.com; spf=pass (imf19.hostedemail.com: domain of baolin.wang@linux.alibaba.com designates 115.124.30.133 as permitted sender) smtp.mailfrom=baolin.wang@linux.alibaba.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1721620299; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=ZRZsaWjHik4fJ4+vcINy/iv5wgmnKjA60/N2NkOyV/A=; b=L+pD8SG1hTReYL0/2m2ZU0YoerGzBiSKX83oJLhxttZUhTcslg8SrWWqJcVWYqEQ7mPSR/ vBZUUwXOWPn00WosXK1tOX1Aa6HU72PgRD+lBpjXJ7UNlCDkQZsGffCqaoKpgFe2z2IqWp Ao6tYYT+rA5KAH5fmNYWCSW35BMLuLs= DKIM-Signature:v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.alibaba.com; s=default; t=1721620331; h=Message-ID:Date:MIME-Version:Subject:To:From:Content-Type; bh=ZRZsaWjHik4fJ4+vcINy/iv5wgmnKjA60/N2NkOyV/A=; b=d+P1NTNQaAZTctFgfrhcKvXVhlIIIqB56KBCk2xd2jdUn96XqpZhuWHcy0CjmMdrlEZyil+PYi/PIGxGbjdUNkJaRJ1ZdgzLEldBBBtsuBhgx5/Clt5PmJtv33CCXsq1nDEcdHrajHeafWtCU+JdQkPnTpS9GpYXQwgGEejT/e4= X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R381e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=maildocker-contentspam033037067113;MF=baolin.wang@linux.alibaba.com;NM=1;PH=DS;RN=10;SR=0;TI=SMTPD_---0WAyfmNo_1721620329; Received: from 30.97.56.74(mailfrom:baolin.wang@linux.alibaba.com fp:SMTPD_---0WAyfmNo_1721620329) by smtp.aliyun-inc.com; Mon, 22 Jul 2024 11:52:10 +0800 Message-ID: <26e84b0f-68ed-4e98-925e-5799a2ae1164@linux.alibaba.com> Date: Mon, 22 Jul 2024 11:52:09 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1 2/2] mm: mTHP stats for pagecache folio allocations To: Ryan Roberts , Andrew Morton , Hugh Dickins , Jonathan Corbet , "Matthew Wilcox (Oracle)" , David Hildenbrand , Barry Song , Lance Yang Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org References: <20240711072929.3590000-1-ryan.roberts@arm.com> <20240711072929.3590000-3-ryan.roberts@arm.com> <9e0d84e5-2319-4425-9760-2c6bb23fc390@linux.alibaba.com> <29f0fc5a-c2b7-4925-9bdb-fd2abe5383ae@arm.com> From: Baolin Wang In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 7AAA51A0015 X-Stat-Signature: zx8zodf89mofr5g3es53kogmx71j8mfa X-Rspam-User: X-HE-Tag: 1721620334-582064 X-HE-Meta: U2FsdGVkX198oO4jnuseWuRu8gA6b6Yygy5tyll0661DCaXaTTmu4uQC/pxGKLqq6IFAjbLeNV0gySNgfTARu6AICUNng5FZcdIXZ7H4E6RgUb1VN9M7PmMKSuxWgNa1iPxQPCSKEBsA4Q4N4FWBI4KYzqttroTrbWlTtQYdwdjZwubH6g/UCLYuu2pgaWOwOhix043ifo3Ddc75iDJFPux2SRnBwjVTJua/6Nt5BNgCbr5sgFahZENkdTCK1j/CtGlS5IZ967/4Nge+TOdq9fd65oZ4wDHusTuXh3T9wMy+/opbBrNxtcN3+vglJkkKxH4V6T5zbBv3Fj8r78YBL0cwnmXVEwPOu4VitYXYjbqE8lWxDy0dfRaiWeKjqfXoT4ULeWfiLr+Qf7PWmUS7qXUgrDChAzsafqCRQvENTO0twnTIp0L/frrAZwkYsK0mPMYYElngO4OMT54m9ST2vx23qnLAED3I4Q2Kc5PAR9csFACIITduLDyEwPgOUlUC3KZs42useyrn1ZkNY2R9LaZhqjV2opqYu5XWF4+XHIPk1S9QvyKex5loW2CiJThX0mhPe2Yg2VUyaoyxNi4Y6r3/Q34dBXi75J8K6tvur12WRAU5677jafHvIPYSjvMUrMSXeiqtotucuGhFo8QWvxdXRmmFq7wX60p2bvYpN71Qmib1bcdBIrCp52Yssj25ZOXxMOxa3IsXA0jgsq5q2BSczrjNtshRyk9GDqaa9rjDFwrcOBUcpi21oVsXRB4iPluaugpoP6Jzi/G5Rjf6PiYbPnrEEbcUNldW5OsaqPiBQ+G3WLeTZ0CeYw4Y7FDa3cwXz5125pKk9EyDUfQXzI1TvjpCRrulgtYqDyMrRXwqDpFV3vWEkzdY6LSfAVwfgME5vt0cl+DVxsrSf0BLR+zWKuQ2k18ttyXHnxN/qHqOTh/65HPxQ0vJwB/9uyglsZHRbpzvsvTHNWlmm06 8t77zK2K dB9y8OWio65+CN2I2Ii7Rfv/RZgIuhlwP4ZaKTFWHarXWsUwGJ5yDyNRBqo12OMghXHlSS4C6bPmxsomnmYjWvs/nwrzQRhBImLVekn1w8ToyBiwgTsqhu9OBEPO/XeCb/5MyzI7J+JNUJfmbcFmZnTKidnuqkkpfqmmJPgFxzyw1vZkoZxBbgCuCRIJNmoemhdMDXeXH+aokpp3sGOShc9A2WXiA7w7lMdpMR2of3L5DG82G66Fs6H6sEi5Yeb0g12nc X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 2024/7/14 17:05, Ryan Roberts wrote: > On 13/07/2024 13:54, Baolin Wang wrote: >> >> >> 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? > > That was also putting doubt in my mind. But the sample at > samples/kobject/kobject-example.c does not call sysfs_remove_group(). It just > calls sysfs_create_group() in example_init() and calls kobject_put() in > example_exit(). So I think that's the correct pattern. > > Looking at the code more closely, sysfs_create_group() just creates files for > each of the attributes in the group. __kobject_del() calls sysfs_remove_dir(), > who's comment states "we remove any files in the directory before we remove the > directory" so I'm pretty sure sysfs_remove_group() is not required. Thanks for the explanation, and I think you are right after checking the code again. Sorry for the noise.