[PATCH v2 0/3] Refactoring for remove_memory_section/unregister_mem_sect_under_nodes

[PATCH v2 0/3] Refactoring for remove_memory_section/unregister_mem_sect_under_nodes

[osalvador]

From: Oscar Salvador <osalvador@suse.de>

This patchset does some cleanups and refactoring in the memory-hotplug code.

The first and the second patch are pretty straightforward, as they
only remove unused arguments/checks.

The third one refactors unregister_mem_sect_under_nodes.
This is needed to have a proper fallback in case we could not allocate
memory. (details can be seen in patch3).

Oscar Salvador (3):
  mm/memory-hotplug: Drop unused args from remove_memory_section
  mm/memory_hotplug: Drop mem_blk check from
    unregister_mem_sect_under_nodes
  mm/memory_hotplug: Refactor unregister_mem_sect_under_nodes

 drivers/base/memory.c |  5 ++---
 drivers/base/node.c   | 30 +++++++++++++++---------------
 include/linux/node.h  |  5 ++---
 3 files changed, 19 insertions(+), 21 deletions(-)

-- 
2.13.6

[PATCH v2 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

Re: [PATCH v2 1/3] mm/memory-hotplug: Drop unused args from remove_memory_section

[David Hildenbrand]

On 13.08.2018 17:46, osalvador@techadventures.net wrote:
> 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 */
>  
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

[PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk 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

Re: [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes

[David Hildenbrand]

On 13.08.2018 17:46, osalvador@techadventures.net wrote:
> 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);
> 

While it is correct in current code, I wonder if this sanity check
should stay. I would completely agree if it would be a static function.

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes

[Oscar Salvador]

On Tue, Aug 14, 2018 at 11:30:51AM +0200, David Hildenbrand wrote:

> 
> While it is correct in current code, I wonder if this sanity check
> should stay. I would completely agree if it would be a static function.

Hi David,

Well, unregister_mem_sect_under_nodes() __only__ gets called from remove_memory_section().
But remove_memory_section() only calls unregister_mem_sect_under_nodes() IFF mem_blk
is not NULL:

static int remove_memory_section
{
	...
	mem = find_memory_block(section);
	if (!mem)
		goto out_unlock;

	unregister_mem_sect_under_nodes(mem, __section_nr(section));
	...
}

So, to me keeping the check is redundant, as we already check for it before calling in.

Thanks
-- 
Oscar Salvador
SUSE L3

Re: [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes

[David Hildenbrand]

On 14.08.2018 11:36, Oscar Salvador wrote:
> On Tue, Aug 14, 2018 at 11:30:51AM +0200, David Hildenbrand wrote:
> 
>>
>> While it is correct in current code, I wonder if this sanity check
>> should stay. I would completely agree if it would be a static function.
> 
> Hi David,
> 
> Well, unregister_mem_sect_under_nodes() __only__ gets called from remove_memory_section().
> But remove_memory_section() only calls unregister_mem_sect_under_nodes() IFF mem_blk
> is not NULL:
> 
> static int remove_memory_section
> {
> 	...
> 	mem = find_memory_block(section);
> 	if (!mem)
> 		goto out_unlock;
> 
> 	unregister_mem_sect_under_nodes(mem, __section_nr(section));
> 	...
> }

Yes I know, as I said, if it would be local to a file I would not care.
Making this functions never return an error is nice, though (and as you
noted, the return value is never checked).

I am a friend of stating which conditions a function expects to hold if
a function can be called from other parts of the system. Usually I
prefer to use BUG_ONs for that (whoever decides to call it can directly
see what he as to check before calling) or comments. But comments tend
to become obsolete.

> 
> So, to me keeping the check is redundant, as we already check for it before calling in.
> 
> Thanks
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes

[Oscar Salvador]

On Tue, Aug 14, 2018 at 11:44:50AM +0200, David Hildenbrand wrote:
> 
> Yes I know, as I said, if it would be local to a file I would not care.
> Making this functions never return an error is nice, though (and as you
> noted, the return value is never checked).
> 
> I am a friend of stating which conditions a function expects to hold if
> a function can be called from other parts of the system. Usually I
> prefer to use BUG_ONs for that (whoever decides to call it can directly
> see what he as to check before calling) or comments. But comments tend
> to become obsolete.

Uhm, I think a BUG_ON is too much here.
We could replace the check with a WARN_ON, just in case
a new function decides to call unregister_mem_sect_under_nodes() in the future.

Something like:

WARN_ON(!mem_blk)
	return;

In that case, we should get a nice splat in the logs that should tell us
who is calling it with an invalid mem_blk.

-- 
Oscar Salvador
SUSE L3

Re: [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes

[David Hildenbrand]

On 14.08.2018 12:06, Oscar Salvador wrote:
> On Tue, Aug 14, 2018 at 11:44:50AM +0200, David Hildenbrand wrote:
>>
>> Yes I know, as I said, if it would be local to a file I would not care.
>> Making this functions never return an error is nice, though (and as you
>> noted, the return value is never checked).
>>
>> I am a friend of stating which conditions a function expects to hold if
>> a function can be called from other parts of the system. Usually I
>> prefer to use BUG_ONs for that (whoever decides to call it can directly
>> see what he as to check before calling) or comments. But comments tend
>> to become obsolete.
> 
> Uhm, I think a BUG_ON is too much here.
> We could replace the check with a WARN_ON, just in case
> a new function decides to call unregister_mem_sect_under_nodes() in the future.
> 
> Something like:
> 
> WARN_ON(!mem_blk)
> 	return;
> 
> In that case, we should get a nice splat in the logs that should tell us
> who is calling it with an invalid mem_blk.
> 

Whatever you think is best. I have no idea what the general rules in MM
code are. Maybe dropping this check is totally fine.

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes

[Oscar Salvador]

On Tue, Aug 14, 2018 at 12:09:14PM +0200, David Hildenbrand wrote:
> 
> Whatever you think is best. I have no idea what the general rules in MM
> code are. Maybe dropping this check is totally fine.

Well, if you ask me, callers should care for validating mem_blk before calling in.
But a WARN_ON is not harmful either.

Let us just wait to hear more from others.
 
Thanks
-- 
Oscar Salvador
SUSE L3

[PATCH v2 3/3] mm/memory_hotplug: Refactor unregister_mem_sect_under_nodes

[osalvador]

From: Oscar Salvador <osalvador@suse.de>

unregister_mem_sect_under_nodes() tries to allocate a nodemask_t
in order to check whithin the loop which nodes have already been unlinked,
so we do not repeat the operation on them.

NODEMASK_ALLOC calls kmalloc() if NODES_SHIFT > 8, otherwise
it just declares a nodemask_t variable whithin the stack.

Since kamlloc() can fail, we actually check whether NODEMASK_ALLOC failed or
not, and we return -ENOMEM accordingly.
remove_memory_section() does not check for the return value though.

The problem with this is that if we return -ENOMEM, it means that
unregister_mem_sect_under_nodes will not be able to remove the symlinks,
but since we do not check the return value, we go ahead and we call unregister_memory(),
which will remove all the mem_blks directories.

This will leave us with dangled symlinks.

The easiest way to overcome this is to fallback by calling sysfs_remove_link()
unconditionally in case NODEMASK_ALLOC failed.
This means that we will call sysfs_remove_link on nodes that have been already unlinked,
but nothing wrong happens as sysfs_remove_link() backs off somewhere down the chain in case
the link has already been removed.

I think that this is better than

a) dangled symlinks
b) having to recovery from such error in remove_memory_section

Since from now on we will not need to take care about return values, we can make the function void.

While at it, we can also drop the node_online() check, as a node can only be
offline if all the memory/cpus associated with it have been removed.

As we have a safe fallback, one thing that could also be done is to add __GFP_NORETRY
in the flags when calling NODEMASK_ALLOC, so we do not retry.

Signed-off-by: Oscar Salvador <osalvador@suse.de>
---
 drivers/base/node.c  | 26 +++++++++++++++-----------
 include/linux/node.h |  5 ++---
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index dd3bdab230b2..0a3ca62687ea 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -449,35 +449,39 @@ int register_mem_sect_under_node(struct memory_block *mem_blk, void *arg)
 }
 
 /* unregister memory section under all nodes that it spans */
-int unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
+void 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);
+	if (unlinked_nodes)
+		nodes_clear(*unlinked_nodes);
 
 	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 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))
+		/*
+		 * It is possible that NODEMASK_ALLOC fails due to memory pressure.
+		 * If that happens, we fallback to call sysfs_remove_link unconditionally.
+		 * Nothing wrong will happen as sysfs_remove_link will back off
+		 * somewhere down the chain in case the link has already been removed.
+		 */
+		if (unlinked_nodes && 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));
 	}
-	NODEMASK_FREE(unlinked_nodes);
-	return 0;
+
+	if (unlinked_nodes)
+		NODEMASK_FREE(unlinked_nodes);
 }
 
 int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn)
diff --git a/include/linux/node.h b/include/linux/node.h
index 257bb3d6d014..1203378e596a 100644
--- a/include/linux/node.h
+++ b/include/linux/node.h
@@ -72,7 +72,7 @@ extern int register_cpu_under_node(unsigned int cpu, unsigned int nid);
 extern int unregister_cpu_under_node(unsigned int cpu, unsigned int nid);
 extern int register_mem_sect_under_node(struct memory_block *mem_blk,
 						void *arg);
-extern int unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
+extern void unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
 					   unsigned long phys_index);
 
 #ifdef CONFIG_HUGETLBFS
@@ -105,10 +105,9 @@ static inline int register_mem_sect_under_node(struct memory_block *mem_blk,
 {
 	return 0;
 }
-static inline int unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
+static inline void unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
 						  unsigned long phys_index)
 {
-	return 0;
 }
 
 static inline void register_hugetlbfs_with_node(node_registration_func_t reg,
-- 
2.13.6

Re: [PATCH v2 3/3] mm/memory_hotplug: Refactor unregister_mem_sect_under_nodes

[David Hildenbrand]

On 13.08.2018 17:46, osalvador@techadventures.net wrote:
> From: Oscar Salvador <osalvador@suse.de>
> 
> unregister_mem_sect_under_nodes() tries to allocate a nodemask_t
> in order to check whithin the loop which nodes have already been unlinked,
> so we do not repeat the operation on them.
> 
> NODEMASK_ALLOC calls kmalloc() if NODES_SHIFT > 8, otherwise
> it just declares a nodemask_t variable whithin the stack.
> 
> Since kamlloc() can fail, we actually check whether NODEMASK_ALLOC failed or
> not, and we return -ENOMEM accordingly.
> remove_memory_section() does not check for the return value though.
> 
> The problem with this is that if we return -ENOMEM, it means that
> unregister_mem_sect_under_nodes will not be able to remove the symlinks,
> but since we do not check the return value, we go ahead and we call unregister_memory(),
> which will remove all the mem_blks directories.
> 
> This will leave us with dangled symlinks.
> 
> The easiest way to overcome this is to fallback by calling sysfs_remove_link()
> unconditionally in case NODEMASK_ALLOC failed.
> This means that we will call sysfs_remove_link on nodes that have been already unlinked,
> but nothing wrong happens as sysfs_remove_link() backs off somewhere down the chain in case
> the link has already been removed.
> 
> I think that this is better than
> 
> a) dangled symlinks
> b) having to recovery from such error in remove_memory_section
> 
> Since from now on we will not need to take care about return values, we can make the function void.
> 
> While at it, we can also drop the node_online() check, as a node can only be
> offline if all the memory/cpus associated with it have been removed.

I would prefer splitting this change out into a separate patch.

> 
> As we have a safe fallback, one thing that could also be done is to add __GFP_NORETRY
> in the flags when calling NODEMASK_ALLOC, so we do not retry.
> 
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
> ---
>  drivers/base/node.c  | 26 +++++++++++++++-----------
>  include/linux/node.h |  5 ++---
>  2 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index dd3bdab230b2..0a3ca62687ea 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -449,35 +449,39 @@ int register_mem_sect_under_node(struct memory_block *mem_blk, void *arg)
>  }
>  
>  /* unregister memory section under all nodes that it spans */
> -int unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
> +void 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);
> +	if (unlinked_nodes)
> +		nodes_clear(*unlinked_nodes);
>  
>  	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 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))
> +		/*
> +		 * It is possible that NODEMASK_ALLOC fails due to memory pressure.
> +		 * If that happens, we fallback to call sysfs_remove_link unconditionally.
> +		 * Nothing wrong will happen as sysfs_remove_link will back off
> +		 * somewhere down the chain in case the link has already been removed.
> +		 */
> +		if (unlinked_nodes && 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));
>  	}
> -	NODEMASK_FREE(unlinked_nodes);
> -	return 0;
> +
> +	if (unlinked_nodes)
> +		NODEMASK_FREE(unlinked_nodes);

NODEMASK_FEEE/kfree can deal with NULL pointers.

>  }
>  
>  int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn)
> diff --git a/include/linux/node.h b/include/linux/node.h
> index 257bb3d6d014..1203378e596a 100644
> --- a/include/linux/node.h
> +++ b/include/linux/node.h
> @@ -72,7 +72,7 @@ extern int register_cpu_under_node(unsigned int cpu, unsigned int nid);
>  extern int unregister_cpu_under_node(unsigned int cpu, unsigned int nid);
>  extern int register_mem_sect_under_node(struct memory_block *mem_blk,
>  						void *arg);
> -extern int unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
> +extern void unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
>  					   unsigned long phys_index);
>  
>  #ifdef CONFIG_HUGETLBFS
> @@ -105,10 +105,9 @@ static inline int register_mem_sect_under_node(struct memory_block *mem_blk,
>  {
>  	return 0;
>  }
> -static inline int unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
> +static inline void unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
>  						  unsigned long phys_index)
>  {
> -	return 0;
>  }
>  
>  static inline void register_hugetlbfs_with_node(node_registration_func_t reg,
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v2 3/3] mm/memory_hotplug: Refactor unregister_mem_sect_under_nodes

[Oscar Salvador]

On Tue, Aug 14, 2018 at 11:39:34AM +0200, David Hildenbrand wrote:
> On 13.08.2018 17:46, osalvador@techadventures.net wrote:
> > From: Oscar Salvador <osalvador@suse.de>
> > While at it, we can also drop the node_online() check, as a node can only be
> > offline if all the memory/cpus associated with it have been removed.
> 
> I would prefer splitting this change out into a separate patch.

Yes, I guess it is better as it is not really related to the changes in this patch.
I will wait for more feedback and I will split it up in v3.

> > +
> > +	if (unlinked_nodes)
> > +		NODEMASK_FREE(unlinked_nodes);
> 
> NODEMASK_FEEE/kfree can deal with NULL pointers.

Good point, I missed that.
I will fix it up in v3.

Thanks for reviewing.
-- 
Oscar Salvador
SUSE L3