[PATCH 0/5] Refactor node_states_check_changes_online/offline

[PATCH 0/5] Refactor node_states_check_changes_online/offline

[Oscar Salvador]

From: Oscar Salvador <osalvador@suse.de>

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

[PATCH 1/5] mm/memory_hotplug: Spare unnecessary calls to node_set_state

[Oscar Salvador]

From: Oscar Salvador <osalvador@suse.de>

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 <osalvador@suse.de>
---
 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

[PATCH 2/5] mm/memory_hotplug: Avoid node_set/clear_state(N_HIGH_MEMORY) when !CONFIG_HIGHMEM

[Oscar Salvador]

From: Oscar Salvador <osalvador@suse.de>

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 <osalvador@suse.de>
---
 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

[PATCH 3/5] mm/memory_hotplug: Tidy up node_states_clear_node

[Oscar Salvador]

From: Oscar Salvador <osalvador@suse.de>

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 <osalvador@suse.de>
---
 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

[PATCH 4/5] mm/memory_hotplug: Simplify node_states_check_changes_online

[Oscar Salvador]

From: Oscar Salvador <osalvador@suse.de>

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 <osalvador@suse.de>
---
 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

[PATCH 5/5] mm/memory_hotplug: Clean up node_states_check_changes_offline

[Oscar Salvador]

From: Oscar Salvador <osalvador@suse.de>

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 <osalvador@suse.de>
---
 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

Re: [PATCH 1/5] mm/memory_hotplug: Spare unnecessary calls to node_set_state

[Pasha Tatashin]

On 9/19/18 6:08 AM, Oscar Salvador wrote:
> From: Oscar Salvador <osalvador@suse.de>
> 
> 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 <osalvador@suse.de>

Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com>

> ---
>  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,
> 

Re: [PATCH 2/5] mm/memory_hotplug: Avoid node_set/clear_state(N_HIGH_MEMORY) when !CONFIG_HIGHMEM

[Pasha Tatashin]

On 9/19/18 6:08 AM, Oscar Salvador wrote:
> From: Oscar Salvador <osalvador@suse.de>
> 
> 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 <osalvador@suse.de>

Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com>

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
>  
>  	/*
> 

Re: [PATCH 3/5] mm/memory_hotplug: Tidy up node_states_clear_node

[Pasha Tatashin]

On 9/19/18 6:08 AM, Oscar Salvador wrote:
> From: Oscar Salvador <osalvador@suse.de>
> 
> 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&amp;data=02%7C01%7CPavel.Tatashin%40microsoft.com%7C1e31e6a5c8754abe0b4608d61e17e01c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636729485241367584&amp;sdata=ztkPNyRIv2c0j5lrujwGM%2FrD5in6G7AvvdqxVXCzwGs%3D&amp;reserved=0
> 
> Signed-off-by: Oscar Salvador <osalvador@suse.de>

Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com>

> ---
>  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);
>  }
>  
> 

Re: [PATCH 4/5] mm/memory_hotplug: Simplify node_states_check_changes_online

[Pasha Tatashin]

On 9/19/18 6:08 AM, Oscar Salvador wrote:
> From: Oscar Salvador <osalvador@suse.de>
> 
> 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 <osalvador@suse.de>
> ---
>  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

Re: [PATCH 5/5] mm/memory_hotplug: Clean up node_states_check_changes_offline

[Pasha Tatashin]

On 9/19/18 6:08 AM, Oscar Salvador wrote:
> From: Oscar Salvador <osalvador@suse.de>
> 
> 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 <osalvador@suse.de>
> ---
>  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 <pavel.tatashin@microsoft.com>

Pavel

Re: [PATCH 4/5] mm/memory_hotplug: Simplify node_states_check_changes_online

[Oscar Salvador]

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

Re: [PATCH 2/5] mm/memory_hotplug: Avoid node_set/clear_state(N_HIGH_MEMORY) when !CONFIG_HIGHMEM

[Oscar Salvador]

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