Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b8d2fe17-93e6-743a-73c4-e8b22964704b@redhat.com>
Date:   Thu, 8 Sep 2022 14:21:34 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.0
Content-Language: en-US
To:     Muchun Song <songmuchun@bytedance.com>, gregkh@linuxfoundation.org,
        rafael@kernel.org, mike.kravetz@oracle.com,
        akpm@linux-foundation.org, osalvador@suse.de, ying.huang@intel.com,
        aneesh.kumar@linux.ibm.com, rientjes@google.com
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        muchun.song@linux.dev
References: <20220908075826.37094-1-songmuchun@bytedance.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH v3] mm: hugetlb: eliminate memory-less nodes handling
In-Reply-To: <20220908075826.37094-1-songmuchun@bytedance.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 08.09.22 09:58, Muchun Song wrote:
> The memory-notify-based approach aims to handle meory-less nodes, however, it just adds
> the complexity of code as pointed by David in thread [1].  The handling of memory-less
> nodes is introduced by commit 4faf8d950ec4 ("hugetlb: handle memory hot-plug events").
>  From its commit message, we cannot find any necessity of handling this case. So, we can
> simply register/unregister sysfs entries in register_node/unregister_node to simlify the
> code.
> 
> BTW, hotplug callback added because in hugetlb_register_all_nodes() we register sysfs
> nodes only for N_MEMORY nodes, seeing commit 9b5e5d0fdc91, which said it was a preparation
> for handling memory-less nodes via memory hotplug. Since we want to remove memory hotplug,
> so make sure we only register per-node sysfs for online (N_ONLINE) nodes in
> hugetlb_register_all_nodes().
> 
> https://lore.kernel.org/linux-mm/60933ffc-b850-976c-78a0-0ee6e0ea9ef0@redhat.com/ [1]
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
> v3:
>   - Fix 'struct node' is not declared reported by LTP.
> 
> v2:
>   - Move declaration of function related to hugetlb to hugetlb.h (David).
>   - Introduce hugetlb_sysfs_initialized() and call it from hugetlb_sysfs_init() (David).
>   - Move hugetlb_register_all_nodes() into hugetlb_sysfs_init() (David).
>   - Fix implicit-function-declaration reported by LKP.
>   - Register per-node sysfs for online (N_ONLINE) nodes instead of N_MEMORY (Aneesh).
> 
>   drivers/base/node.c     |  8 +++--
>   include/linux/hugetlb.h | 14 +++++++++
>   mm/hugetlb.c            | 81 ++++++++++++++++++++++---------------------------
>   3 files changed, 57 insertions(+), 46 deletions(-)


> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3880,24 +3880,14 @@ static int hugetlb_sysfs_add_hstate(struct hstate *h, struct kobject *parent,
>   	return 0;
>   }
>   
> -static void __init hugetlb_sysfs_init(void)
> -{
> -	struct hstate *h;
> -	int err;
> -
> -	hugepages_kobj = kobject_create_and_add("hugepages", mm_kobj);
> -	if (!hugepages_kobj)
> -		return;
> +#ifdef CONFIG_NUMA
> +static bool hugetlb_sysfs_initialized __ro_after_init;
>   
> -	for_each_hstate(h) {
> -		err = hugetlb_sysfs_add_hstate(h, hugepages_kobj,
> -					 hstate_kobjs, &hstate_attr_group);
> -		if (err)
> -			pr_err("HugeTLB: Unable to add hstate %s", h->name);
> -	}
> +static inline void hugetlb_mark_sysfs_initialized(void)
> +{
> +	hugetlb_sysfs_initialized = true;
>   }

Do we really need a separate function for this? Why not simply always 
set that from hugetlb_sysfs_init() ?

I'm also not sure if we really want to optimize out one variable for 
!CONFIG_NUMA.


Anyhow, in general

Acked-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb