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 6AB71C3DA59 for ; Mon, 22 Jul 2024 07:36:57 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0B2106B0098; Mon, 22 Jul 2024 03:36:57 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 062386B0099; Mon, 22 Jul 2024 03:36:56 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E458D6B009A; Mon, 22 Jul 2024 03:36:56 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id C4C936B0098 for ; Mon, 22 Jul 2024 03:36:56 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 70D5714159E for ; Mon, 22 Jul 2024 07:36:56 +0000 (UTC) X-FDA: 82366582032.28.F3E28E4 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf22.hostedemail.com (Postfix) with ESMTP id AE7C5C0014 for ; Mon, 22 Jul 2024 07:36:54 +0000 (UTC) Authentication-Results: imf22.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf22.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1721633770; 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; bh=lDYyU2JZhPNZviXj0DDh7L3TQj4/on/gPzfW01nM8q4=; b=wqIFOOkvGw3z9ZJaqASZkna2nBHF6ErUIql1aFigpIDHHCU7BumsV2KqRN1CPWWgrhaFNv 8t0YiPBDosY2q32bQMnE4gF3qF7VQrbvFiYdKzqo9BFoyEm4E8xsb2YUmFQfihnx1gK4wH kmZyfHsbZGrrA+YyDj79RNqnGgbsfPk= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721633770; a=rsa-sha256; cv=none; b=pm7z9HRHIuWBolrrM/VYPrvMU6LjbQUXVXDt+YMJ0HRfUkCfeTSsdqxqw+NGB0UhvIwfXK +9E1WZZWNmWJMr4sfGBt/oEwZEHuWM6e4BdJg3QhRUrw4/5bhSHeqOq016h2raMLBDjVfq FsxoVoBssao5qbbo4XkQsJuo9TOblog= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf22.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 64599FEC; Mon, 22 Jul 2024 00:37:19 -0700 (PDT) Received: from [10.57.77.243] (unknown [10.57.77.243]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8ADB73F766; Mon, 22 Jul 2024 00:36:52 -0700 (PDT) Message-ID: <2109a518-8c5e-41c8-8bd4-9c3499c29fd0@arm.com> Date: Mon, 22 Jul 2024 08:36:51 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1 2/2] mm: mTHP stats for pagecache folio allocations Content-Language: en-GB To: Baolin Wang , 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> <26e84b0f-68ed-4e98-925e-5799a2ae1164@linux.alibaba.com> From: Ryan Roberts In-Reply-To: <26e84b0f-68ed-4e98-925e-5799a2ae1164@linux.alibaba.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: AE7C5C0014 X-Stat-Signature: dnnqyijfdipxnp8jxkew7owukjb1jp5t X-Rspamd-Server: rspam09 X-Rspam-User: X-HE-Tag: 1721633814-523323 X-HE-Meta: U2FsdGVkX19DMebNOcqbhfiZe4DnDPLCfhXZX1a/vDPsThLEORXgcZ16OnxUyeKHKv5lH5IXQtCuSMJnXtbSO+tSThWtWqhxEpUeDk3lWu93cu6p6ntr1l9QCJKBEy/uycU758oLqYliuZf1ONvW4mTSDoS86H/uRdWfxxnr5eurlTz4fZJRs7bwYstrsdH1RiutG9+iRczod8hKX9Ukn7G0dARoNXmC1xP54m3sylZrAHgxZ8+hL/51OZ6gNZR23bvQE+pBNKOoOUVLHtsKNzUhnHr1h3L34bA3McWnTJ2gFQn40dYcOmnHMlPhpA0/rWFVHcSLKrNxcPBj9QcVoQrkw4WHNWXfICiAhCMhpDnjmZALBR0ZqUef4B6CBCStoWstR4FI+0WbY99cd54ghBTDUiiepfy0TRtPYUYDvAcMO3BsYBQbP8wKKykw6ffK4JAOWi+le1UfoTEwrTIXLgaFBUUar7VMx4kZ/OUsHmDruYHQyadehtEBo+NmTHNWFh6L67wvLcFr3tXEmkn5lN1D8hv0CO6qTRx8/sb9HPocG51psBFwr7/p1gIMYJ03J5PvFC4rgrOtEzuf/aLwA8J7158NNHISTXKvNQYNcnCY4/UuuMFnf6CfBNJb2EYg2E9pPePY6bcWDHcERMpLBXnrJawF4pTGgUAwOUw/B0VkkH3NeRk8b8SeQR4sCUhYmR3qpMIeJ6YLT+jw1jnpM5Mbe+rq5FPctlNqAwoYxAg07rqVqtHPGzBD85VETlObzoRuOmqMWLTvOd++PXMhiUiBMl270nxI4yr3zKspHCi5XNkleyphCZA+xegUdVhRMrTBsaSI+/GJ/oEVOzhvaTIHVNeorfamil83B0+Bz5mSIkF/C3eXrLvbT/9kSw9hM10w9N/h6KMreqq48qMnh6nWU4+GPJQ2LdC3PgWMHvLC+kgQ1iT3Aa3U00SAS8cSbPJcgJ8t8InSmcTit9z 7hwd3VWR cJyDh5yXJy/y5kAc0cZi2tpj42GLeWgTZWLdLgK9oDU4wTnzlCcfw+gIFDvRCaMQjKp4UEmctDc8HxjHu+AC4i87SJOz3P7zGcCEKH2yXLN/aa9oLsrG21gn1lNm0XGmAjEH58G6ree1GPK2gengjgryZ2m65BrNIRpHQ+6G/xKGztl8= 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 22/07/2024 04:52, Baolin Wang wrote: > > > 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. No problem, thanks for raising anyway; TBH, I wasn't completely sure when I wrote it initially. So good to have clear resolution.