From: Oscar Salvador <[email protected]>
This patchset refactors/clean ups node_states_check_changes_online/offline
functions together with node_states_set/clear_node.
The main reason behind this patchset is that currently, these
functions are suboptimal and confusing.
For example, they contain wrong statements like:
if (N_MEMORY == N_NORMAL_MEMORY)
if (N_MEMORY =! N_NORMAL_MEMORY)
if (N_MEMORY != N_HIGH_MEMORY)
if (N_MEMORY == N_HIGH_MEMORY)
These comparasions are wrong, as N_MEMORY will never be equal
to either N_NORMAL_MEMORY or N_HIGH_MEMORY.
Although the statements do not "affect" the flow because in the way
they are placed, they are completely wrong and confusing.
I caught another misuse of this in [1].
Another thing that this patchset addresses is the fact that
some functions get called twice, or even unconditionally, without
any need.
Examples of this are:
- node_states_set_node()->node_set_state(node, N_MEMORY)
* node_states_set_node() gets called whenever we online pages,
so we end up calling node_set_state(node, N_MEMORY) everytime.
To avoid this, we should check if the node is already in
node_state[N_MEMORY].
- node_states_set_node()->node_set_state(node, N_HIGH_MEMORY)
* On !CONFIG_HIGH_MEMORY, N_HIGH_MEMORY == N_NORMAL_MEMORY,
but the current code sets:
status_change_nid_high = status_change_nid_normal
This means that we will call node_set_state(node, N_NORMAL_MEMORY) twice.
The fix here is to set status_change_nid_normal = -1 on such systems,
so we skip the second call.
[1] https://patchwork.kernel.org/patch/10579155/
Oscar Salvador (5):
mm/memory_hotplug: Spare unnecessary calls to node_set_state
mm/memory_hotplug: Avoid node_set/clear_state(N_HIGH_MEMORY) when
!CONFIG_HIGHMEM
mm/memory_hotplug: Tidy up node_states_clear_node
mm/memory_hotplug: Simplify node_states_check_changes_online
mm/memory_hotplug: Clean up node_states_check_changes_offline
mm/memory_hotplug.c | 153 +++++++++++++++++++++-------------------------------
1 file changed, 60 insertions(+), 93 deletions(-)
--
2.13.6
From: Oscar Salvador <[email protected]>
This patch, as the previous one, gets rid of the wrong if statements.
While at it, I realized that the comments are sometimes very confusing,
to say the least, and wrong.
For example:
---
zone_last = ZONE_MOVABLE;
/*
* check whether node_states[N_HIGH_MEMORY] will be changed
* If we try to offline the last present @nr_pages from the node,
* we can determind we will need to clear the node from
* node_states[N_HIGH_MEMORY].
*/
for (; zt <= zone_last; zt++)
present_pages += pgdat->node_zones[zt].present_pages;
if (nr_pages >= present_pages)
arg->status_change_nid = zone_to_nid(zone);
else
arg->status_change_nid = -1;
---
In case the node gets empry, it must be removed from N_MEMORY.
We already check N_HIGH_MEMORY a bit above within the CONFIG_HIGHMEM
ifdef code.
Not to say that status_change_nid is for N_MEMORY, and not for
N_HIGH_MEMORY.
So I re-wrote some of the comments to what I think is better.
Signed-off-by: Oscar Salvador <[email protected]>
---
mm/memory_hotplug.c | 71 +++++++++++++++++++++--------------------------------
1 file changed, 28 insertions(+), 43 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index ab3c1de18c5d..15ecf3d7a554 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1485,51 +1485,36 @@ static void node_states_check_changes_offline(unsigned long nr_pages,
{
struct pglist_data *pgdat = zone->zone_pgdat;
unsigned long present_pages = 0;
- enum zone_type zt, zone_last = ZONE_NORMAL;
+ enum zone_type zt;
/*
- * If we have HIGHMEM or movable node, node_states[N_NORMAL_MEMORY]
- * contains nodes which have zones of 0...ZONE_NORMAL,
- * set zone_last to ZONE_NORMAL.
- *
- * If we don't have HIGHMEM nor movable node,
- * node_states[N_NORMAL_MEMORY] contains nodes which have zones of
- * 0...ZONE_MOVABLE, set zone_last to ZONE_MOVABLE.
+ * Check whether node_states[N_NORMAL_MEMORY] will be changed.
+ * If the memory to be offline is within the range
+ * [0..ZONE_NORMAL], and it is the last present memory there,
+ * the zones in that range will become empty after the offlining,
+ * thus we can determine that we need to clear the node from
+ * node_states[N_NORMAL_MEMORY].
*/
- if (N_MEMORY == N_NORMAL_MEMORY)
- zone_last = ZONE_MOVABLE;
-
- /*
- * check whether node_states[N_NORMAL_MEMORY] will be changed.
- * If the memory to be offline is in a zone of 0...zone_last,
- * and it is the last present memory, 0...zone_last will
- * become empty after offline , thus we can determind we will
- * need to clear the node from node_states[N_NORMAL_MEMORY].
- */
- for (zt = 0; zt <= zone_last; zt++)
+ for (zt = 0; zt <= ZONE_NORMAL; zt++)
present_pages += pgdat->node_zones[zt].present_pages;
- if (zone_idx(zone) <= zone_last && nr_pages >= present_pages)
+ if (zone_idx(zone) <= ZONE_NORMAL && nr_pages >= present_pages)
arg->status_change_nid_normal = zone_to_nid(zone);
else
arg->status_change_nid_normal = -1;
#ifdef CONFIG_HIGHMEM
/*
- * If we have movable node, node_states[N_HIGH_MEMORY]
- * contains nodes which have zones of 0...ZONE_HIGHMEM,
- * set zone_last to ZONE_HIGHMEM.
- *
- * If we don't have movable node, node_states[N_NORMAL_MEMORY]
- * contains nodes which have zones of 0...ZONE_MOVABLE,
- * set zone_last to ZONE_MOVABLE.
+ * node_states[N_HIGH_MEMORY] contains nodes which
+ * have normal memory or high memory.
+ * Here we add the present_pages belonging to ZONE_HIGHMEM.
+ * If the zone is within the range of [0..ZONE_HIGHMEM), and
+ * we determine that the zones in that range become empty,
+ * we need to clear the node for N_HIGH_MEMORY.
*/
- zone_last = ZONE_HIGHMEM;
- if (N_MEMORY == N_HIGH_MEMORY)
- zone_last = ZONE_MOVABLE;
+ zt = ZONE_HIGHMEM;
+ present_pages += pgdat->node_zones[zt].present_pages;
- for (; zt <= zone_last; zt++)
- present_pages += pgdat->node_zones[zt].present_pages;
- if (zone_idx(zone) <= zone_last && nr_pages >= present_pages)
+ if (zone_idx(zone) <= zt && nr_pages >= present_pages)
arg->status_change_nid_high = zone_to_nid(zone);
else
arg->status_change_nid_high = -1;
@@ -1542,18 +1527,18 @@ static void node_states_check_changes_offline(unsigned long nr_pages,
#endif
/*
- * node_states[N_HIGH_MEMORY] contains nodes which have 0...ZONE_MOVABLE
+ * We have accounted the pages from [0..ZONE_NORMAL), and
+ * in case of CONFIG_HIGHMEM the pages from ZONE_HIGHMEM
+ * as well.
+ * Here we count the possible pages from ZONE_MOVABLE.
+ * If after having accounted all the pages, we see that the nr_pages
+ * to be offlined is over or equal to the accounted pages,
+ * we know that the node will become empty, and so, we can clear
+ * it for N_MEMORY as well.
*/
- zone_last = ZONE_MOVABLE;
+ zt = ZONE_MOVABLE;
+ present_pages += pgdat->node_zones[zt].present_pages;
- /*
- * check whether node_states[N_HIGH_MEMORY] will be changed
- * If we try to offline the last present @nr_pages from the node,
- * we can determind we will need to clear the node from
- * node_states[N_HIGH_MEMORY].
- */
- for (; zt <= zone_last; zt++)
- present_pages += pgdat->node_zones[zt].present_pages;
if (nr_pages >= present_pages)
arg->status_change_nid = zone_to_nid(zone);
else
--
2.13.6
From: Oscar Salvador <[email protected]>
node_states_clear has the following if statements:
if ((N_MEMORY != N_NORMAL_MEMORY) &&
(arg->status_change_nid_high >= 0))
...
if ((N_MEMORY != N_HIGH_MEMORY) &&
(arg->status_change_nid >= 0))
...
N_MEMORY can never be equal to neither N_NORMAL_MEMORY nor
N_HIGH_MEMORY.
Similar problem was found in [1].
Since this is wrong, let us get rid of it.
[1] https://patchwork.kernel.org/patch/10579155/
Signed-off-by: Oscar Salvador <[email protected]>
---
mm/memory_hotplug.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index c2c7359bd0a7..131c08106d54 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1590,12 +1590,10 @@ static void node_states_clear_node(int node, struct memory_notify *arg)
if (arg->status_change_nid_normal >= 0)
node_clear_state(node, N_NORMAL_MEMORY);
- if ((N_MEMORY != N_NORMAL_MEMORY) &&
- (arg->status_change_nid_high >= 0))
+ if (arg->status_change_nid_high >= 0)
node_clear_state(node, N_HIGH_MEMORY);
- if ((N_MEMORY != N_HIGH_MEMORY) &&
- (arg->status_change_nid >= 0))
+ if (arg->status_change_nid >= 0)
node_clear_state(node, N_MEMORY);
}
--
2.13.6
From: Oscar Salvador <[email protected]>
While looking at node_states_check_changes_online, I stumbled
upon some confusing things.
Right after entering the function, we find this:
if (N_MEMORY == N_NORMAL_MEMORY)
zone_last = ZONE_MOVABLE;
This is wrong.
N_MEMORY cannot really be equal to N_NORMAL_MEMORY.
My guess is that this wanted to be something like:
if (N_NORMAL_MEMORY == N_HIGH_MEMORY)
to check if we have CONFIG_HIGHMEM.
Later on, in the CONFIG_HIGHMEM block, we have:
if (N_MEMORY == N_HIGH_MEMORY)
zone_last = ZONE_MOVABLE;
Again, this is wrong, and will never be evaluated to true.
Besides removing these wrong if statements, I simplified
the function a bit.
Signed-off-by: Oscar Salvador <[email protected]>
---
mm/memory_hotplug.c | 71 +++++++++++++++++------------------------------------
1 file changed, 23 insertions(+), 48 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 131c08106d54..ab3c1de18c5d 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -687,61 +687,36 @@ static void node_states_check_changes_online(unsigned long nr_pages,
struct zone *zone, struct memory_notify *arg)
{
int nid = zone_to_nid(zone);
- enum zone_type zone_last = ZONE_NORMAL;
/*
- * If we have HIGHMEM or movable node, node_states[N_NORMAL_MEMORY]
- * contains nodes which have zones of 0...ZONE_NORMAL,
- * set zone_last to ZONE_NORMAL.
- *
- * If we don't have HIGHMEM nor movable node,
- * node_states[N_NORMAL_MEMORY] contains nodes which have zones of
- * 0...ZONE_MOVABLE, set zone_last to ZONE_MOVABLE.
+ * zone_for_pfn_range() can only return a zone within
+ * (0..ZONE_NORMAL] or ZONE_MOVABLE.
+ * If the zone is within the range (0..ZONE_NORMAL],
+ * we need to check if:
+ * 1) We need to set the node for N_NORMAL_MEMORY
+ * 2) On CONFIG_HIGHMEM systems, we need to also set
+ * the node for N_HIGH_MEMORY.
+ * 3) On !CONFIG_HIGHMEM, we can disregard N_HIGH_MEMORY,
+ * as N_HIGH_MEMORY falls back to N_NORMAL_MEMORY.
*/
- if (N_MEMORY == N_NORMAL_MEMORY)
- zone_last = ZONE_MOVABLE;
- /*
- * if the memory to be online is in a zone of 0...zone_last, and
- * the zones of 0...zone_last don't have memory before online, we will
- * need to set the node to node_states[N_NORMAL_MEMORY] after
- * the memory is online.
- */
- if (zone_idx(zone) <= zone_last && !node_state(nid, N_NORMAL_MEMORY))
- arg->status_change_nid_normal = nid;
- else
- arg->status_change_nid_normal = -1;
-
-#ifdef CONFIG_HIGHMEM
- /*
- * If we have movable node, node_states[N_HIGH_MEMORY]
- * contains nodes which have zones of 0...ZONE_HIGHMEM,
- * set zone_last to ZONE_HIGHMEM.
- *
- * If we don't have movable node, node_states[N_NORMAL_MEMORY]
- * contains nodes which have zones of 0...ZONE_MOVABLE,
- * set zone_last to ZONE_MOVABLE.
- */
- zone_last = ZONE_HIGHMEM;
- if (N_MEMORY == N_HIGH_MEMORY)
- zone_last = ZONE_MOVABLE;
+ if (zone_idx(zone) <= ZONE_NORMAL) {
+ if (!node_state(nid, N_NORMAL_MEMORY))
+ arg->status_change_nid_normal = nid;
+ else
+ arg->status_change_nid_normal = -1;
- if (zone_idx(zone) <= zone_last && !node_state(nid, N_HIGH_MEMORY))
- arg->status_change_nid_high = nid;
- else
- arg->status_change_nid_high = -1;
-#else
- /*
- * When !CONFIG_HIGHMEM, N_HIGH_MEMORY equals N_NORMAL_MEMORY
- * so setting the node for N_NORMAL_MEMORY is enough.
- */
- arg->status_change_nid_high = -1;
-#endif
+ if (IS_ENABLED(CONFIG_HIGHMEM)) {
+ if (!node_state(nid, N_HIGH_MEMORY))
+ arg->status_change_nid_high = nid;
+ } else
+ arg->status_change_nid_high = -1;
+ }
/*
- * if the node don't have memory befor online, we will need to
- * set the node to node_states[N_MEMORY] after the memory
- * is online.
+ * if the node doesn't have memory before onlining it, we will need
+ * to set the node to node_states[N_MEMORY] after the memory
+ * gets onlined.
*/
if (!node_state(nid, N_MEMORY))
arg->status_change_nid = nid;
--
2.13.6
From: Oscar Salvador <[email protected]>
In node_states_check_changes_online, we check if the node will
have to be set for any of the N_*_MEMORY states after the pages
have been onlined.
Later on, we perform the activation in node_states_set_node.
Currently, in node_states_set_node we set the node to N_MEMORY
unconditionally.
This means that we call node_set_state for N_MEMORY every time
pages go online, but we only need to do it if the node has not yet been
set for N_MEMORY.
Fix this by checking status_change_nid.
Signed-off-by: Oscar Salvador <[email protected]>
---
mm/memory_hotplug.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 38d94b703e9d..63facfc57224 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -753,7 +753,8 @@ static void node_states_set_node(int node, struct memory_notify *arg)
if (arg->status_change_nid_high >= 0)
node_set_state(node, N_HIGH_MEMORY);
- node_set_state(node, N_MEMORY);
+ if (arg->status_change_nid >= 0)
+ node_set_state(node, N_MEMORY);
}
static void __meminit resize_zone_range(struct zone *zone, unsigned long start_pfn,
--
2.13.6
From: Oscar Salvador <[email protected]>
Currently, when !CONFIG_HIGHMEM, status_change_nid_high is being set
to status_change_nid_normal, but on such systems N_HIGH_MEMORY falls
back to N_NORMAL_MEMORY.
That means that if status_change_nid_normal is not -1,
we will perform two calls to node_set_state for the same memory type.
Set status_change_nid_high to -1 for !CONFIG_HIGHMEM, so we skip the
double call in node_states_set_node.
The same goes for node_clear_state.
Signed-off-by: Oscar Salvador <[email protected]>
---
mm/memory_hotplug.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 63facfc57224..c2c7359bd0a7 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -731,7 +731,11 @@ static void node_states_check_changes_online(unsigned long nr_pages,
else
arg->status_change_nid_high = -1;
#else
- arg->status_change_nid_high = arg->status_change_nid_normal;
+ /*
+ * When !CONFIG_HIGHMEM, N_HIGH_MEMORY equals N_NORMAL_MEMORY
+ * so setting the node for N_NORMAL_MEMORY is enough.
+ */
+ arg->status_change_nid_high = -1;
#endif
/*
@@ -1555,7 +1559,11 @@ static void node_states_check_changes_offline(unsigned long nr_pages,
else
arg->status_change_nid_high = -1;
#else
- arg->status_change_nid_high = arg->status_change_nid_normal;
+ /*
+ * When !CONFIG_HIGHMEM, N_HIGH_MEMORY equals N_NORMAL_MEMORY
+ * so clearing the node for N_NORMAL_MEMORY is enough.
+ */
+ arg->status_change_nid_high = -1;
#endif
/*
--
2.13.6
On 9/19/18 6:08 AM, Oscar Salvador wrote:
> From: Oscar Salvador <[email protected]>
>
> In node_states_check_changes_online, we check if the node will
> have to be set for any of the N_*_MEMORY states after the pages
> have been onlined.
>
> Later on, we perform the activation in node_states_set_node.
> Currently, in node_states_set_node we set the node to N_MEMORY
> unconditionally.
> This means that we call node_set_state for N_MEMORY every time
> pages go online, but we only need to do it if the node has not yet been
> set for N_MEMORY.
>
> Fix this by checking status_change_nid.
>
> Signed-off-by: Oscar Salvador <[email protected]>
Reviewed-by: Pavel Tatashin <[email protected]>
> ---
> mm/memory_hotplug.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 38d94b703e9d..63facfc57224 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -753,7 +753,8 @@ static void node_states_set_node(int node, struct memory_notify *arg)
> if (arg->status_change_nid_high >= 0)
> node_set_state(node, N_HIGH_MEMORY);
>
> - node_set_state(node, N_MEMORY);
> + if (arg->status_change_nid >= 0)
> + node_set_state(node, N_MEMORY);
> }
>
> static void __meminit resize_zone_range(struct zone *zone, unsigned long start_pfn,
>
On 9/19/18 6:08 AM, Oscar Salvador wrote:
> From: Oscar Salvador <[email protected]>
>
> Currently, when !CONFIG_HIGHMEM, status_change_nid_high is being set
> to status_change_nid_normal, but on such systems N_HIGH_MEMORY falls
> back to N_NORMAL_MEMORY.
> That means that if status_change_nid_normal is not -1,
> we will perform two calls to node_set_state for the same memory type.
>
> Set status_change_nid_high to -1 for !CONFIG_HIGHMEM, so we skip the
> double call in node_states_set_node.
>
> The same goes for node_clear_state.
>
> Signed-off-by: Oscar Salvador <[email protected]>
Reviewed-by: Pavel Tatashin <[email protected]>
This is a rare case where I think comments are unnecessary as the code
is self explanatory. So, I would remove the comments before:
> + arg->status_change_nid_high = -1;
Pavel
> ---
> mm/memory_hotplug.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 63facfc57224..c2c7359bd0a7 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -731,7 +731,11 @@ static void node_states_check_changes_online(unsigned long nr_pages,
> else
> arg->status_change_nid_high = -1;
> #else
> - arg->status_change_nid_high = arg->status_change_nid_normal;
> + /*
> + * When !CONFIG_HIGHMEM, N_HIGH_MEMORY equals N_NORMAL_MEMORY
> + * so setting the node for N_NORMAL_MEMORY is enough.
> + */
> + arg->status_change_nid_high = -1;
> #endif
>
> /*
> @@ -1555,7 +1559,11 @@ static void node_states_check_changes_offline(unsigned long nr_pages,
> else
> arg->status_change_nid_high = -1;
> #else
> - arg->status_change_nid_high = arg->status_change_nid_normal;
> + /*
> + * When !CONFIG_HIGHMEM, N_HIGH_MEMORY equals N_NORMAL_MEMORY
> + * so clearing the node for N_NORMAL_MEMORY is enough.
> + */
> + arg->status_change_nid_high = -1;
> #endif
>
> /*
>
On 9/19/18 6:08 AM, Oscar Salvador wrote:
> From: Oscar Salvador <[email protected]>
>
> node_states_clear has the following if statements:
>
> if ((N_MEMORY != N_NORMAL_MEMORY) &&
> (arg->status_change_nid_high >= 0))
> ...
>
> if ((N_MEMORY != N_HIGH_MEMORY) &&
> (arg->status_change_nid >= 0))
> ...
>
> N_MEMORY can never be equal to neither N_NORMAL_MEMORY nor
> N_HIGH_MEMORY.
>
> Similar problem was found in [1].
> Since this is wrong, let us get rid of it.
>
> [1] https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F10579155%2F&data=02%7C01%7CPavel.Tatashin%40microsoft.com%7C1e31e6a5c8754abe0b4608d61e17e01c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636729485241367584&sdata=ztkPNyRIv2c0j5lrujwGM%2FrD5in6G7AvvdqxVXCzwGs%3D&reserved=0
>
> Signed-off-by: Oscar Salvador <[email protected]>
Reviewed-by: Pavel Tatashin <[email protected]>
> ---
> mm/memory_hotplug.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c2c7359bd0a7..131c08106d54 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1590,12 +1590,10 @@ static void node_states_clear_node(int node, struct memory_notify *arg)
> if (arg->status_change_nid_normal >= 0)
> node_clear_state(node, N_NORMAL_MEMORY);
>
> - if ((N_MEMORY != N_NORMAL_MEMORY) &&
> - (arg->status_change_nid_high >= 0))
> + if (arg->status_change_nid_high >= 0)
> node_clear_state(node, N_HIGH_MEMORY);
>
> - if ((N_MEMORY != N_HIGH_MEMORY) &&
> - (arg->status_change_nid >= 0))
> + if (arg->status_change_nid >= 0)
> node_clear_state(node, N_MEMORY);
> }
>
>
On 9/19/18 6:08 AM, Oscar Salvador wrote:
> From: Oscar Salvador <[email protected]>
>
> While looking at node_states_check_changes_online, I stumbled
> upon some confusing things.
>
> Right after entering the function, we find this:
>
> if (N_MEMORY == N_NORMAL_MEMORY)
> zone_last = ZONE_MOVABLE;
>
> This is wrong.
> N_MEMORY cannot really be equal to N_NORMAL_MEMORY.
> My guess is that this wanted to be something like:
>
> if (N_NORMAL_MEMORY == N_HIGH_MEMORY)
>
> to check if we have CONFIG_HIGHMEM.
>
> Later on, in the CONFIG_HIGHMEM block, we have:
>
> if (N_MEMORY == N_HIGH_MEMORY)
> zone_last = ZONE_MOVABLE;
>
> Again, this is wrong, and will never be evaluated to true.
>
> Besides removing these wrong if statements, I simplified
> the function a bit.
>
> Signed-off-by: Oscar Salvador <[email protected]>
> ---
> mm/memory_hotplug.c | 71 +++++++++++++++++------------------------------------
> 1 file changed, 23 insertions(+), 48 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 131c08106d54..ab3c1de18c5d 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -687,61 +687,36 @@ static void node_states_check_changes_online(unsigned long nr_pages,
> struct zone *zone, struct memory_notify *arg)
> {
> int nid = zone_to_nid(zone);
> - enum zone_type zone_last = ZONE_NORMAL;
>
> /*
> - * If we have HIGHMEM or movable node, node_states[N_NORMAL_MEMORY]
> - * contains nodes which have zones of 0...ZONE_NORMAL,
> - * set zone_last to ZONE_NORMAL.
> - *
> - * If we don't have HIGHMEM nor movable node,
> - * node_states[N_NORMAL_MEMORY] contains nodes which have zones of
> - * 0...ZONE_MOVABLE, set zone_last to ZONE_MOVABLE.
> + * zone_for_pfn_range() can only return a zone within
> + * (0..ZONE_NORMAL] or ZONE_MOVABLE.
But what if that changes, will this function need to change as well?
> + * If the zone is within the range (0..ZONE_NORMAL],
> + * we need to check if:
> + * 1) We need to set the node for N_NORMAL_MEMORY
> + * 2) On CONFIG_HIGHMEM systems, we need to also set
> + * the node for N_HIGH_MEMORY.
> + * 3) On !CONFIG_HIGHMEM, we can disregard N_HIGH_MEMORY,
> + * as N_HIGH_MEMORY falls back to N_NORMAL_MEMORY.
> */
> - if (N_MEMORY == N_NORMAL_MEMORY)
> - zone_last = ZONE_MOVABLE;
>
> - /*
> - * if the memory to be online is in a zone of 0...zone_last, and
> - * the zones of 0...zone_last don't have memory before online, we will
> - * need to set the node to node_states[N_NORMAL_MEMORY] after
> - * the memory is online.
> - */
> - if (zone_idx(zone) <= zone_last && !node_state(nid, N_NORMAL_MEMORY))
> - arg->status_change_nid_normal = nid;
> - else
> - arg->status_change_nid_normal = -1;
> -
> -#ifdef CONFIG_HIGHMEM
> - /*
> - * If we have movable node, node_states[N_HIGH_MEMORY]
> - * contains nodes which have zones of 0...ZONE_HIGHMEM,
> - * set zone_last to ZONE_HIGHMEM.
> - *
> - * If we don't have movable node, node_states[N_NORMAL_MEMORY]
> - * contains nodes which have zones of 0...ZONE_MOVABLE,
> - * set zone_last to ZONE_MOVABLE.
> - */
> - zone_last = ZONE_HIGHMEM;
> - if (N_MEMORY == N_HIGH_MEMORY)
> - zone_last = ZONE_MOVABLE;
> + if (zone_idx(zone) <= ZONE_NORMAL) {
> + if (!node_state(nid, N_NORMAL_MEMORY))
> + arg->status_change_nid_normal = nid;
> + else
> + arg->status_change_nid_normal = -1;
>
> - if (zone_idx(zone) <= zone_last && !node_state(nid, N_HIGH_MEMORY))
> - arg->status_change_nid_high = nid;
> - else
> - arg->status_change_nid_high = -1;
> -#else
> - /*
> - * When !CONFIG_HIGHMEM, N_HIGH_MEMORY equals N_NORMAL_MEMORY
> - * so setting the node for N_NORMAL_MEMORY is enough.
> - */
> - arg->status_change_nid_high = -1;
> -#endif
> + if (IS_ENABLED(CONFIG_HIGHMEM)) {
> + if (!node_state(nid, N_HIGH_MEMORY))
> + arg->status_change_nid_high = nid;
Should not we have:
else
arg->status_change_nid_high = -1; ?
> + } else
> + arg->status_change_nid_high = -1;
I prefer to have braces in else part as well when if has braces.
> + }
>
> /*
> - * if the node don't have memory befor online, we will need to
> - * set the node to node_states[N_MEMORY] after the memory
> - * is online.
> + * if the node doesn't have memory before onlining it, we will need
> + * to set the node to node_states[N_MEMORY] after the memory
> + * gets onlined.
> */
> if (!node_state(nid, N_MEMORY))
> arg->status_change_nid = nid;
>
I think it is simpler to have something like this:
int nid = zone_to_nid(zone);
arg->status_change_nid_high = -1;
arg->status_change_nid = -1;
arg->status_change_nid = -1;
if (!node_state(nid, N_MEMORY))
arg->status_change_nid = nid;
if (zone_idx(zone) <= ZONE_NORMAL && !node_state(nid, N_NORMAL_MEMORY))
arg->status_change_nid_normal = nid;
#ifdef CONFIG_HIGHMEM
if (zone_idx(zone) <= N_HIGH_MEMORY && !node_state(nid, N_HIGH_MEMORY))
arg->status_change_nid_high = nid;
#endif
Pavel
On 9/19/18 6:08 AM, Oscar Salvador wrote:
> From: Oscar Salvador <[email protected]>
>
> This patch, as the previous one, gets rid of the wrong if statements.
> While at it, I realized that the comments are sometimes very confusing,
> to say the least, and wrong.
> For example:
>
> ---
> zone_last = ZONE_MOVABLE;
>
> /*
> * check whether node_states[N_HIGH_MEMORY] will be changed
> * If we try to offline the last present @nr_pages from the node,
> * we can determind we will need to clear the node from
> * node_states[N_HIGH_MEMORY].
> */
>
> for (; zt <= zone_last; zt++)
> present_pages += pgdat->node_zones[zt].present_pages;
> if (nr_pages >= present_pages)
> arg->status_change_nid = zone_to_nid(zone);
> else
> arg->status_change_nid = -1;
> ---
>
> In case the node gets empry, it must be removed from N_MEMORY.
> We already check N_HIGH_MEMORY a bit above within the CONFIG_HIGHMEM
> ifdef code.
> Not to say that status_change_nid is for N_MEMORY, and not for
> N_HIGH_MEMORY.
>
> So I re-wrote some of the comments to what I think is better.
>
> Signed-off-by: Oscar Salvador <[email protected]>
> ---
> mm/memory_hotplug.c | 71 +++++++++++++++++++++--------------------------------
> 1 file changed, 28 insertions(+), 43 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index ab3c1de18c5d..15ecf3d7a554 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1485,51 +1485,36 @@ static void node_states_check_changes_offline(unsigned long nr_pages,
> {
> struct pglist_data *pgdat = zone->zone_pgdat;
> unsigned long present_pages = 0;
> - enum zone_type zt, zone_last = ZONE_NORMAL;
> + enum zone_type zt;
>
> /*
> - * If we have HIGHMEM or movable node, node_states[N_NORMAL_MEMORY]
> - * contains nodes which have zones of 0...ZONE_NORMAL,
> - * set zone_last to ZONE_NORMAL.
> - *
> - * If we don't have HIGHMEM nor movable node,
> - * node_states[N_NORMAL_MEMORY] contains nodes which have zones of
> - * 0...ZONE_MOVABLE, set zone_last to ZONE_MOVABLE.
> + * Check whether node_states[N_NORMAL_MEMORY] will be changed.
> + * If the memory to be offline is within the range
> + * [0..ZONE_NORMAL], and it is the last present memory there,
> + * the zones in that range will become empty after the offlining,
> + * thus we can determine that we need to clear the node from
> + * node_states[N_NORMAL_MEMORY].
> */
> - if (N_MEMORY == N_NORMAL_MEMORY)
> - zone_last = ZONE_MOVABLE;
> -
> - /*
> - * check whether node_states[N_NORMAL_MEMORY] will be changed.
> - * If the memory to be offline is in a zone of 0...zone_last,
> - * and it is the last present memory, 0...zone_last will
> - * become empty after offline , thus we can determind we will
> - * need to clear the node from node_states[N_NORMAL_MEMORY].
> - */
> - for (zt = 0; zt <= zone_last; zt++)
> + for (zt = 0; zt <= ZONE_NORMAL; zt++)
> present_pages += pgdat->node_zones[zt].present_pages;
> - if (zone_idx(zone) <= zone_last && nr_pages >= present_pages)
> + if (zone_idx(zone) <= ZONE_NORMAL && nr_pages >= present_pages)
> arg->status_change_nid_normal = zone_to_nid(zone);
> else
> arg->status_change_nid_normal = -1;
>
> #ifdef CONFIG_HIGHMEM
> /*
> - * If we have movable node, node_states[N_HIGH_MEMORY]
> - * contains nodes which have zones of 0...ZONE_HIGHMEM,
> - * set zone_last to ZONE_HIGHMEM.
> - *
> - * If we don't have movable node, node_states[N_NORMAL_MEMORY]
> - * contains nodes which have zones of 0...ZONE_MOVABLE,
> - * set zone_last to ZONE_MOVABLE.
> + * node_states[N_HIGH_MEMORY] contains nodes which
> + * have normal memory or high memory.
> + * Here we add the present_pages belonging to ZONE_HIGHMEM.
> + * If the zone is within the range of [0..ZONE_HIGHMEM), and
> + * we determine that the zones in that range become empty,
> + * we need to clear the node for N_HIGH_MEMORY.
> */
> - zone_last = ZONE_HIGHMEM;
> - if (N_MEMORY == N_HIGH_MEMORY)
> - zone_last = ZONE_MOVABLE;
> + zt = ZONE_HIGHMEM;
> + present_pages += pgdat->node_zones[zt].present_pages;
>
> - for (; zt <= zone_last; zt++)
> - present_pages += pgdat->node_zones[zt].present_pages;
> - if (zone_idx(zone) <= zone_last && nr_pages >= present_pages)
> + if (zone_idx(zone) <= zt && nr_pages >= present_pages)
> arg->status_change_nid_high = zone_to_nid(zone);
> else
> arg->status_change_nid_high = -1;
> @@ -1542,18 +1527,18 @@ static void node_states_check_changes_offline(unsigned long nr_pages,
> #endif
>
> /*
> - * node_states[N_HIGH_MEMORY] contains nodes which have 0...ZONE_MOVABLE
> + * We have accounted the pages from [0..ZONE_NORMAL), and
> + * in case of CONFIG_HIGHMEM the pages from ZONE_HIGHMEM
> + * as well.
> + * Here we count the possible pages from ZONE_MOVABLE.
> + * If after having accounted all the pages, we see that the nr_pages
> + * to be offlined is over or equal to the accounted pages,
> + * we know that the node will become empty, and so, we can clear
> + * it for N_MEMORY as well.
> */
> - zone_last = ZONE_MOVABLE;
> + zt = ZONE_MOVABLE;
> + present_pages += pgdat->node_zones[zt].present_pages;
>
> - /*
> - * check whether node_states[N_HIGH_MEMORY] will be changed
> - * If we try to offline the last present @nr_pages from the node,
> - * we can determind we will need to clear the node from
> - * node_states[N_HIGH_MEMORY].
> - */
> - for (; zt <= zone_last; zt++)
> - present_pages += pgdat->node_zones[zt].present_pages;
> if (nr_pages >= present_pages)
> arg->status_change_nid = zone_to_nid(zone);
> else
>
A couple nits:
I would simplify this a little, initialize all fields at the beginning
arg->status_change_nid_normal = -1;
arg->status_change_nid_high = -1;
arg->status_change_nid = -1;
And remove all the elses, including ifdef else.
And remove:
zt = ZONE_HIGHMEM;
zt = ZONE_MOVABLE;
Use macros directly, lines are are still going to be under 80 chars.
Otherwise looks good.
Reviewed-by: Pavel Tatashin <[email protected]>
Pavel
On Fri, Sep 21, 2018 at 12:15:53AM +0000, Pasha Tatashin wrote:
Hi Pavel,
> But what if that changes, will this function need to change as well?
That's true.
> Should not we have:
> else
> arg->status_change_nid_high = -1; ?
>
> > + } else
> > + arg->status_change_nid_high = -1;
Yes, I forgot about that else.
> I think it is simpler to have something like this:
>
> int nid = zone_to_nid(zone);
>
> arg->status_change_nid_high = -1;
> arg->status_change_nid = -1;
> arg->status_change_nid = -1;
>
> if (!node_state(nid, N_MEMORY))
> arg->status_change_nid = nid;
> if (zone_idx(zone) <= ZONE_NORMAL && !node_state(nid, N_NORMAL_MEMORY))
> arg->status_change_nid_normal = nid;
> #ifdef CONFIG_HIGHMEM
> if (zone_idx(zone) <= N_HIGH_MEMORY && !node_state(nid, N_HIGH_MEMORY))
> arg->status_change_nid_high = nid;
> #endif
I can write it that way, I also like it more.
I will send it in V2.
Thanks for reviewing it!
--
Oscar Salvador
SUSE L3
On Thu, Sep 20, 2018 at 08:59:18PM +0000, Pasha Tatashin wrote:
> This is a rare case where I think comments are unnecessary as the code
> is self explanatory. So, I would remove the comments before:
Fair enough.
I just wanted to make clear why it was not needed.
I will remove it in the next version.
Thanks
--
Oscar Salvador
SUSE L3