> On Sep 8, 2022, at 20:21, David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx/ [1]
>> Suggested-by: David Hildenbrand <david@xxxxxxxxxx>
>> Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
>> ---
>> 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 can remove this helper.
>
> I'm also not sure if we really want to optimize out one variable for !CONFIG_NUMA.
Either is fine to me. I think the optimization does not bring any complexity.
So I’ll keep it the same in the next version unless anyone is against this.
>
> Anyhow, in general
>
> Acked-by: David Hildenbrand <david@xxxxxxxxxx>
Thanks David.
>
>
> --
> Thanks,
>
> David / dhildenb
>
