| Message ID | 20220307150725.6810-4-osalvador@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A minor hotplug refactoring | expand |
On 07.03.22 16:07, Oscar Salvador wrote: > Since we pre-allocate all nodes now, hotadd_init_pgdat() does > not need to return a pgdat struct, as that was meant for > __try_online_node() to check whether the node was succesfully > allocated. > > Also get rid of the __ref as all functions hotadd_init_pgdat() > calls fall within the same section. > > Also try to make more clear the return codes from __try_online_node(). > __try_online_node() can return either 0, 1 or -errno (the latter not really > as the BUG_ON() would catch it before we return) but depending on the caller > that has different meanings. > For add_memory_resource(), when __try_online_node() returns non-zero, > it means that the node was already allocated and it does not need to bring > it up. It is fine not to check for -errno values because we do not > get to call register_one_node() when !set_node_online. > For those who call try_online_node(), so set_node_online is true, a value > other than zero means a failure (e.g: cpu_up() or find_and_online_cpu_nid()). > > Signed-off-by: Oscar Salvador <osalvador@suse.de> > --- > mm/memory_hotplug.c | 35 ++++++++++++++--------------------- > 1 file changed, 14 insertions(+), 21 deletions(-) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index 07cece9e22e4..5c92ac81a399 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -1161,8 +1161,7 @@ static void reset_node_present_pages(pg_data_t *pgdat) > pgdat->node_present_pages = 0; > } > > -/* we are OK calling __meminit stuff here - we have CONFIG_MEMORY_HOTPLUG */ > -static pg_data_t __ref *hotadd_init_pgdat(int nid) > +static void hotadd_init_pgdat(int nid) > { > struct pglist_data *pgdat = NODE_DATA(nid); > > @@ -1182,8 +1181,6 @@ static pg_data_t __ref *hotadd_init_pgdat(int nid) > * to access not-initialized zonelist, build here. > */ > build_all_zonelists(pgdat); > - > - return pgdat; > } > > /* > @@ -1193,31 +1190,27 @@ static pg_data_t __ref *hotadd_init_pgdat(int nid) > * called by cpu_up() to online a node without onlined memory. > * > * Returns: > - * 1 -> a new node has been allocated > - * 0 -> the node is already online > - * -ENOMEM -> the node could not be allocated > + * 1 -> The node has been initialized. > + * 0 -> Either the node was already online, or we succesfully registered a new > + * one. > + * -errno -> register_one_node() failed. > */ > static int __try_online_node(int nid, bool set_node_online) > { > - pg_data_t *pgdat; > - int ret = 1; > + int ret; > > if (node_online(nid)) > return 0; > > - pgdat = hotadd_init_pgdat(nid); > - if (!pgdat) { > - pr_err("Cannot online node %d due to NULL pgdat\n", nid); > - ret = -ENOMEM; > - goto out; > - } > + hotadd_init_pgdat(nid); > + > + if (!set_node_online) > + return 1; > + > + node_set_online(nid); > + ret = register_one_node(nid); > + BUG_ON(ret); > > - if (set_node_online) { > - node_set_online(nid); > - ret = register_one_node(nid); > - BUG_ON(ret); > - } > -out: > return ret; BUG_ON(ret); return ret; hm? This will never return :) So either leave the old code flow or "return 1;" I'd actually prefer the old code flow to then "return 1;" here: if (set_node_online) { node_set_online(nid); BUG_ON(register_one_node(nid)); } return 1; We can then let go of "ret".
On Thu, Apr 28, 2022 at 02:35:11PM +0200, David Hildenbrand wrote: > BUG_ON(ret); > return ret; > > hm? This will never return :) Yes, this looks like a brainfart. It will not return __only__ in the case where register_node_node() fails, which is what we do right now, but then I do not know why added "errno -> register_one_node() failed." in the comment. I might have managed to confuse myself. Thanks
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index 07cece9e22e4..5c92ac81a399 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -1161,8 +1161,7 @@ static void reset_node_present_pages(pg_data_t *pgdat) pgdat->node_present_pages = 0; } -/* we are OK calling __meminit stuff here - we have CONFIG_MEMORY_HOTPLUG */ -static pg_data_t __ref *hotadd_init_pgdat(int nid) +static void hotadd_init_pgdat(int nid) { struct pglist_data *pgdat = NODE_DATA(nid); @@ -1182,8 +1181,6 @@ static pg_data_t __ref *hotadd_init_pgdat(int nid) * to access not-initialized zonelist, build here. */ build_all_zonelists(pgdat); - - return pgdat; } /* @@ -1193,31 +1190,27 @@ static pg_data_t __ref *hotadd_init_pgdat(int nid) * called by cpu_up() to online a node without onlined memory. * * Returns: - * 1 -> a new node has been allocated - * 0 -> the node is already online - * -ENOMEM -> the node could not be allocated + * 1 -> The node has been initialized. + * 0 -> Either the node was already online, or we succesfully registered a new + * one. + * -errno -> register_one_node() failed. */ static int __try_online_node(int nid, bool set_node_online) { - pg_data_t *pgdat; - int ret = 1; + int ret; if (node_online(nid)) return 0; - pgdat = hotadd_init_pgdat(nid); - if (!pgdat) { - pr_err("Cannot online node %d due to NULL pgdat\n", nid); - ret = -ENOMEM; - goto out; - } + hotadd_init_pgdat(nid); + + if (!set_node_online) + return 1; + + node_set_online(nid); + ret = register_one_node(nid); + BUG_ON(ret); - if (set_node_online) { - node_set_online(nid); - ret = register_one_node(nid); - BUG_ON(ret); - } -out: return ret; }
Since we pre-allocate all nodes now, hotadd_init_pgdat() does not need to return a pgdat struct, as that was meant for __try_online_node() to check whether the node was succesfully allocated. Also get rid of the __ref as all functions hotadd_init_pgdat() calls fall within the same section. Also try to make more clear the return codes from __try_online_node(). __try_online_node() can return either 0, 1 or -errno (the latter not really as the BUG_ON() would catch it before we return) but depending on the caller that has different meanings. For add_memory_resource(), when __try_online_node() returns non-zero, it means that the node was already allocated and it does not need to bring it up. It is fine not to check for -errno values because we do not get to call register_one_node() when !set_node_online. For those who call try_online_node(), so set_node_online is true, a value other than zero means a failure (e.g: cpu_up() or find_and_online_cpu_nid()). Signed-off-by: Oscar Salvador <osalvador@suse.de> --- mm/memory_hotplug.c | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-)