| Message ID | 20220908075826.37094-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] mm: hugetlb: eliminate memory-less nodes handling | expand |
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>
> On Sep 8, 2022, at 20:21, David Hildenbrand <david@redhat.com> 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@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 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@redhat.com> Thanks David. > > > -- > Thanks, > > David / dhildenb >
diff --git a/drivers/base/node.c b/drivers/base/node.c index ed391cb09999..80b1e91b9608 100644 --- a/drivers/base/node.c +++ b/drivers/base/node.c @@ -20,6 +20,7 @@ #include <linux/pm_runtime.h> #include <linux/swap.h> #include <linux/slab.h> +#include <linux/hugetlb.h> static struct bus_type node_subsys = { .name = "node", @@ -608,10 +609,12 @@ static int register_node(struct node *node, int num) node->dev.groups = node_dev_groups; error = device_register(&node->dev); - if (error) + if (error) { put_device(&node->dev); - else + } else { + hugetlb_register_node(node); compaction_register_node(node); + } return error; } @@ -625,6 +628,7 @@ static int register_node(struct node *node, int num) */ void unregister_node(struct node *node) { + hugetlb_unregister_node(node); compaction_unregister_node(node); node_remove_accesses(node); node_remove_caches(node); diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h index 852f911d676e..2d98d0ac4a48 100644 --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -16,6 +16,7 @@ struct ctl_table; struct user_struct; struct mmu_gather; +struct node; #ifndef is_hugepd typedef struct { unsigned long pd; } hugepd_t; @@ -967,6 +968,11 @@ static inline void huge_ptep_modify_prot_commit(struct vm_area_struct *vma, } #endif +#ifdef CONFIG_NUMA +void hugetlb_register_node(struct node *node); +void hugetlb_unregister_node(struct node *node); +#endif + #else /* CONFIG_HUGETLB_PAGE */ struct hstate {}; @@ -1141,6 +1147,14 @@ static inline void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep, pte_t pte) { } + +static inline void hugetlb_register_node(struct node *node) +{ +} + +static inline void hugetlb_unregister_node(struct node *node) +{ +} #endif /* CONFIG_HUGETLB_PAGE */ static inline spinlock_t *huge_pte_lock(struct hstate *h, diff --git a/mm/hugetlb.c b/mm/hugetlb.c index d0617d64d718..06ac469582da 100644 --- 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; } -#ifdef CONFIG_NUMA /* * node_hstate/s - associate per node hstate attributes, via their kobjects, @@ -3953,7 +3943,7 @@ static struct hstate *kobj_to_node_hstate(struct kobject *kobj, int *nidp) * Unregister hstate attributes from a single node device. * No-op if no hstate attributes attached. */ -static void hugetlb_unregister_node(struct node *node) +void hugetlb_unregister_node(struct node *node) { struct hstate *h; struct node_hstate *nhs = &node_hstates[node->dev.id]; @@ -3983,19 +3973,22 @@ static void hugetlb_unregister_node(struct node *node) * Register hstate attributes for a single node device. * No-op if attributes already registered. */ -static int hugetlb_register_node(struct node *node) +void hugetlb_register_node(struct node *node) { struct hstate *h; struct node_hstate *nhs = &node_hstates[node->dev.id]; int err; + if (!hugetlb_sysfs_initialized) + return; + if (nhs->hugepages_kobj) - return 0; /* already allocated */ + return; /* already allocated */ nhs->hugepages_kobj = kobject_create_and_add("hugepages", &node->dev.kobj); if (!nhs->hugepages_kobj) - return -ENOMEM; + return; for_each_hstate(h) { err = hugetlb_sysfs_add_hstate(h, nhs->hugepages_kobj, @@ -4005,28 +3998,9 @@ static int hugetlb_register_node(struct node *node) pr_err("HugeTLB: Unable to add hstate %s for node %d\n", h->name, node->dev.id); hugetlb_unregister_node(node); - return -ENOMEM; + break; } } - return 0; -} - -static int __meminit hugetlb_memory_callback(struct notifier_block *self, - unsigned long action, void *arg) -{ - int ret = 0; - struct memory_notify *mnb = arg; - int nid = mnb->status_change_nid; - - if (nid == NUMA_NO_NODE) - return NOTIFY_DONE; - - if (action == MEM_GOING_ONLINE) - ret = hugetlb_register_node(node_devices[nid]); - else if (action == MEM_CANCEL_ONLINE || action == MEM_OFFLINE) - hugetlb_unregister_node(node_devices[nid]); - - return notifier_from_errno(ret); } /* @@ -4038,11 +4012,8 @@ static void __init hugetlb_register_all_nodes(void) { int nid; - get_online_mems(); - hotplug_memory_notifier(hugetlb_memory_callback, 0); - for_each_node_state(nid, N_MEMORY) + for_each_online_node(nid) hugetlb_register_node(node_devices[nid]); - put_online_mems(); } #else /* !CONFIG_NUMA */ @@ -4056,8 +4027,31 @@ static struct hstate *kobj_to_node_hstate(struct kobject *kobj, int *nidp) static void hugetlb_register_all_nodes(void) { } +static inline void hugetlb_mark_sysfs_initialized(void) +{ +} #endif +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; + + 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); + } + + hugetlb_mark_sysfs_initialized(); + hugetlb_register_all_nodes(); +} + static int __init hugetlb_init(void) { int i; @@ -4112,7 +4106,6 @@ static int __init hugetlb_init(void) report_hugepages(); hugetlb_sysfs_init(); - hugetlb_register_all_nodes(); hugetlb_cgroup_file_init(); #ifdef CONFIG_SMP
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(-)