[PATCH 0/3] Refactor/cleanup for remove_memory_section/unregister_mem_sect_under_nodes

[PATCH 0/3] Refactor/cleanup for remove_memory_section/unregister_mem_sect_under_nodes

[osalvador]

From: Oscar Salvador <osalvador@suse.de>

This patchset is about cleaning up/refactoring a few functions
from the memory-hotplug code.

The first and the second patch are pretty straightforward, as they
only remove unused arguments/checks.
The third one change the layout of the unregister_mem_sect_under_nodes a bit.

Oscar Salvador (3):
  mm/memory_hotplug: Drop unused args from remove_memory_section
  mm/memory_hotplug: Drop unneeded check from
    unregister_mem_sect_under_nodes
  mm/memory_hotplug: Cleanup unregister_mem_sect_under_nodes

 drivers/base/memory.c |  5 ++---
 drivers/base/node.c   | 34 +++++++++++-----------------------
 2 files changed, 13 insertions(+), 26 deletions(-)

-- 
2.13.6

[PATCH 1/3] mm/memory_hotplug: Drop unused args from remove_memory_section

[osalvador]

From: Oscar Salvador <osalvador@suse.de>

unregister_memory_section() calls remove_memory_section()
with three arguments:

* node_id
* section
* phys_device

Neither node_id nor phys_device are used.
Let us drop them from the function.

Signed-off-by: Oscar Salvador <osalvador@suse.de>
---
 drivers/base/memory.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index c8a1cb0b6136..2c622a9a7490 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -752,8 +752,7 @@ unregister_memory(struct memory_block *memory)
 	device_unregister(&memory->dev);
 }
 
-static int remove_memory_section(unsigned long node_id,
-			       struct mem_section *section, int phys_device)
+static int remove_memory_section(struct mem_section *section)
 {
 	struct memory_block *mem;
 
@@ -785,7 +784,7 @@ int unregister_memory_section(struct mem_section *section)
 	if (!present_section(section))
 		return -EINVAL;
 
-	return remove_memory_section(0, section, 0);
+	return remove_memory_section(section);
 }
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
-- 
2.13.6

[PATCH 2/3] mm/memory_hotplug: Drop unneeded check from unregister_mem_sect_under_nodes

[osalvador]

From: Oscar Salvador <osalvador@suse.de>

Before calling to unregister_mem_sect_under_nodes(),
remove_memory_section() already checks if we got a valid
memory_block.

No need to check that again in unregister_mem_sect_under_nodes().

Signed-off-by: Oscar Salvador <osalvador@suse.de>
---
 drivers/base/node.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index 1ac4c36e13bb..dd3bdab230b2 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -455,10 +455,6 @@ int unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
 	NODEMASK_ALLOC(nodemask_t, unlinked_nodes, GFP_KERNEL);
 	unsigned long pfn, sect_start_pfn, sect_end_pfn;
 
-	if (!mem_blk) {
-		NODEMASK_FREE(unlinked_nodes);
-		return -EFAULT;
-	}
 	if (!unlinked_nodes)
 		return -ENOMEM;
 	nodes_clear(*unlinked_nodes);
-- 
2.13.6

[PATCH 3/3] mm/memory_hotplug: Cleanup unregister_mem_sect_under_nodes

[osalvador]

From: Oscar Salvador <osalvador@suse.de>

With the assumption that the relationship between
memory_block <-> node is 1:1, we can refactor this function a bit.

This assumption is being taken from register_mem_sect_under_node()
code.

register_mem_sect_under_node() takes the mem_blk's nid, and compares it
to the pfn's nid we are checking.
If they match, we go ahead and link both objects.
Once done, we just return.

So, the relationship between memory_block <-> node seems to stand.

Currently, unregister_mem_sect_under_nodes() defines a nodemask_t
which is being checked in the loop to see if we have already unliked certain node.
But since a memory_block can only belong to a node, we can drop the nodemask
and the check within the loop.

If we find a match between the mem_block->nid and the nid of the
pfn we are checking, we unlink the objects and return, as unlink the objects
once is enough.

Signed-off-by: Oscar Salvador <osalvador@suse.de>
---
 drivers/base/node.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index dd3bdab230b2..0657ed70bddd 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -448,35 +448,27 @@ int register_mem_sect_under_node(struct memory_block *mem_blk, void *arg)
 	return 0;
 }
 
-/* unregister memory section under all nodes that it spans */
+/* unregister memory section from the node it belongs to */
 int unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
 				    unsigned long phys_index)
 {
-	NODEMASK_ALLOC(nodemask_t, unlinked_nodes, GFP_KERNEL);
 	unsigned long pfn, sect_start_pfn, sect_end_pfn;
-
-	if (!unlinked_nodes)
-		return -ENOMEM;
-	nodes_clear(*unlinked_nodes);
+	int nid = mem_blk->nid;
 
 	sect_start_pfn = section_nr_to_pfn(phys_index);
 	sect_end_pfn = sect_start_pfn + PAGES_PER_SECTION - 1;
 	for (pfn = sect_start_pfn; pfn <= sect_end_pfn; pfn++) {
-		int nid;
+		int page_nid = get_nid_for_pfn(pfn);
 
-		nid = get_nid_for_pfn(pfn);
-		if (nid < 0)
-			continue;
-		if (!node_online(nid))
-			continue;
-		if (node_test_and_set(nid, *unlinked_nodes))
-			continue;
-		sysfs_remove_link(&node_devices[nid]->dev.kobj,
-			 kobject_name(&mem_blk->dev.kobj));
-		sysfs_remove_link(&mem_blk->dev.kobj,
-			 kobject_name(&node_devices[nid]->dev.kobj));
+		if (page_nid >= 0 && page_nid == nid) {
+			sysfs_remove_link(&node_devices[nid]->dev.kobj,
+				 kobject_name(&mem_blk->dev.kobj));
+			sysfs_remove_link(&mem_blk->dev.kobj,
+				 kobject_name(&node_devices[nid]->dev.kobj));
+			break;
+		}
 	}
-	NODEMASK_FREE(unlinked_nodes);
+
 	return 0;
 }
 
-- 
2.13.6

Re: [PATCH 3/3] mm/memory_hotplug: Cleanup unregister_mem_sect_under_nodes

[Andrew Morton]

On Fri, 10 Aug 2018 17:29:31 +0200 osalvador@techadventures.net wrote:

> From: Oscar Salvador <osalvador@suse.de>
> 
> With the assumption that the relationship between
> memory_block <-> node is 1:1, we can refactor this function a bit.
> 
> This assumption is being taken from register_mem_sect_under_node()
> code.
> 
> register_mem_sect_under_node() takes the mem_blk's nid, and compares it
> to the pfn's nid we are checking.
> If they match, we go ahead and link both objects.
> Once done, we just return.
> 
> So, the relationship between memory_block <-> node seems to stand.
> 
> Currently, unregister_mem_sect_under_nodes() defines a nodemask_t
> which is being checked in the loop to see if we have already unliked certain node.

"unlinked a certain node"

> But since a memory_block can only belong to a node, we can drop the nodemask

"to a single node"?

> and the check within the loop.
> 
> If we find a match between the mem_block->nid and the nid of the
> pfn we are checking, we unlink the objects and return, as unlink the objects

"unlinking"

> once is enough.
> 
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -448,35 +448,27 @@ int register_mem_sect_under_node(struct memory_block *mem_blk, void *arg)
>  	return 0;
>  }
>  
> -/* unregister memory section under all nodes that it spans */
> +/* unregister memory section from the node it belongs to */
>  int unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
>  				    unsigned long phys_index)
>  {
> -	NODEMASK_ALLOC(nodemask_t, unlinked_nodes, GFP_KERNEL);
>  	unsigned long pfn, sect_start_pfn, sect_end_pfn;
> -
> -	if (!unlinked_nodes)
> -		return -ENOMEM;
> -	nodes_clear(*unlinked_nodes);
> +	int nid = mem_blk->nid;
>  
>  	sect_start_pfn = section_nr_to_pfn(phys_index);
>  	sect_end_pfn = sect_start_pfn + PAGES_PER_SECTION - 1;
>  	for (pfn = sect_start_pfn; pfn <= sect_end_pfn; pfn++) {
> -		int nid;
> +		int page_nid = get_nid_for_pfn(pfn);
>  
> -		nid = get_nid_for_pfn(pfn);
> -		if (nid < 0)
> -			continue;
> -		if (!node_online(nid))
> -			continue;
> -		if (node_test_and_set(nid, *unlinked_nodes))
> -			continue;
> -		sysfs_remove_link(&node_devices[nid]->dev.kobj,
> -			 kobject_name(&mem_blk->dev.kobj));
> -		sysfs_remove_link(&mem_blk->dev.kobj,
> -			 kobject_name(&node_devices[nid]->dev.kobj));
> +		if (page_nid >= 0 && page_nid == nid) {
> +			sysfs_remove_link(&node_devices[nid]->dev.kobj,
> +				 kobject_name(&mem_blk->dev.kobj));
> +			sysfs_remove_link(&mem_blk->dev.kobj,
> +				 kobject_name(&node_devices[nid]->dev.kobj));
> +			break;
> +		}
>  	}
> -	NODEMASK_FREE(unlinked_nodes);
> +
>  	return 0;
>  }

I guess so.  But the node_online() check was silently removed?

Re: [PATCH 3/3] mm/memory_hotplug: Cleanup unregister_mem_sect_under_nodes

[Oscar Salvador]

On Fri, Aug 10, 2018 at 03:37:27PM -0700, Andrew Morton wrote:
> I guess so.  But the node_online() check was silently removed?

A node can only get offline if all the memory and CPUs associated
with it are removed.

This is being checked in remove_memory()->try_offline_node().
There we check whether the node has still valid sections or not,
and if there are still CPUs associated to it.

In the case that either we still have valid sections or that we have
CPUs linked to this node, we do not offline it.

So we cannot really be removing a memory from a node that is offline,
that is why it is safe to drop the check.

It was my mistake not to explain that properly in the changelog though.
I will send a V2 fixing up all you pointed out and explaining
why it is safe to drop the check.

Thanks
-- 
Oscar Salvador
SUSE L3

Re: [PATCH 3/3] mm/memory_hotplug: Cleanup unregister_mem_sect_under_nodes

[Oscar Salvador]

On Fri, Aug 10, 2018 at 05:29:31PM +0200, osalvador@techadventures.net wrote:
> From: Oscar Salvador <osalvador@suse.de>
> 
> With the assumption that the relationship between
> memory_block <-> node is 1:1, we can refactor this function a bit.
> 
> This assumption is being taken from register_mem_sect_under_node()
> code.

Doh, this assumption is wrong for boot case when a mem_blk can have
multiple sections.

Nevertheless, I think that unregister_mem_sect_under_nodes can be polished a bit.
I am working on that

-- 
Oscar Salvador
SUSE L3