* [PATCH v2 0/3] Refactoring for remove_memory_section/unregister_mem_sect_under_nodes
@ 2018-08-13 15:46 osalvador
2018-08-13 15:46 ` [PATCH v2 1/3] mm/memory-hotplug: Drop unused args from remove_memory_section osalvador
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: osalvador @ 2018-08-13 15:46 UTC (permalink / raw)
To: akpm
Cc: mhocko, dan.j.williams, jglisse, rafael, david, yasu.isimatu,
logang, dave.jiang, Jonathan.Cameron, vbabka, linux-mm,
linux-kernel, Oscar Salvador
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/3] mm/memory-hotplug: Drop unused args from remove_memory_section
2018-08-13 15:46 [PATCH v2 0/3] Refactoring for remove_memory_section/unregister_mem_sect_under_nodes osalvador
@ 2018-08-13 15:46 ` osalvador
2018-08-14 9:29 ` David Hildenbrand
2018-08-13 15:46 ` [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes osalvador
2018-08-13 15:46 ` [PATCH v2 3/3] mm/memory_hotplug: Refactor unregister_mem_sect_under_nodes osalvador
2 siblings, 1 reply; 13+ messages in thread
From: osalvador @ 2018-08-13 15:46 UTC (permalink / raw)
To: akpm
Cc: mhocko, dan.j.williams, jglisse, rafael, david, yasu.isimatu,
logang, dave.jiang, Jonathan.Cameron, vbabka, linux-mm,
linux-kernel, Oscar Salvador
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes
2018-08-13 15:46 [PATCH v2 0/3] Refactoring for remove_memory_section/unregister_mem_sect_under_nodes osalvador
2018-08-13 15:46 ` [PATCH v2 1/3] mm/memory-hotplug: Drop unused args from remove_memory_section osalvador
@ 2018-08-13 15:46 ` osalvador
2018-08-14 9:30 ` David Hildenbrand
2018-08-13 15:46 ` [PATCH v2 3/3] mm/memory_hotplug: Refactor unregister_mem_sect_under_nodes osalvador
2 siblings, 1 reply; 13+ messages in thread
From: osalvador @ 2018-08-13 15:46 UTC (permalink / raw)
To: akpm
Cc: mhocko, dan.j.williams, jglisse, rafael, david, yasu.isimatu,
logang, dave.jiang, Jonathan.Cameron, vbabka, linux-mm,
linux-kernel, Oscar Salvador
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] mm/memory_hotplug: Refactor unregister_mem_sect_under_nodes
2018-08-13 15:46 [PATCH v2 0/3] Refactoring for remove_memory_section/unregister_mem_sect_under_nodes osalvador
2018-08-13 15:46 ` [PATCH v2 1/3] mm/memory-hotplug: Drop unused args from remove_memory_section osalvador
2018-08-13 15:46 ` [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes osalvador
@ 2018-08-13 15:46 ` osalvador
2018-08-14 9:39 ` David Hildenbrand
2 siblings, 1 reply; 13+ messages in thread
From: osalvador @ 2018-08-13 15:46 UTC (permalink / raw)
To: akpm
Cc: mhocko, dan.j.williams, jglisse, rafael, david, yasu.isimatu,
logang, dave.jiang, Jonathan.Cameron, vbabka, linux-mm,
linux-kernel, Oscar Salvador
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] mm/memory-hotplug: Drop unused args from remove_memory_section
2018-08-13 15:46 ` [PATCH v2 1/3] mm/memory-hotplug: Drop unused args from remove_memory_section osalvador
@ 2018-08-14 9:29 ` David Hildenbrand
0 siblings, 0 replies; 13+ messages in thread
From: David Hildenbrand @ 2018-08-14 9:29 UTC (permalink / raw)
To: osalvador, akpm
Cc: mhocko, dan.j.williams, jglisse, rafael, yasu.isimatu, logang,
dave.jiang, Jonathan.Cameron, vbabka, linux-mm, linux-kernel,
Oscar Salvador
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes
2018-08-13 15:46 ` [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes osalvador
@ 2018-08-14 9:30 ` David Hildenbrand
2018-08-14 9:36 ` Oscar Salvador
0 siblings, 1 reply; 13+ messages in thread
From: David Hildenbrand @ 2018-08-14 9:30 UTC (permalink / raw)
To: osalvador, akpm
Cc: mhocko, dan.j.williams, jglisse, rafael, yasu.isimatu, logang,
dave.jiang, Jonathan.Cameron, vbabka, linux-mm, linux-kernel,
Oscar Salvador
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes
2018-08-14 9:30 ` David Hildenbrand
@ 2018-08-14 9:36 ` Oscar Salvador
2018-08-14 9:44 ` David Hildenbrand
0 siblings, 1 reply; 13+ messages in thread
From: Oscar Salvador @ 2018-08-14 9:36 UTC (permalink / raw)
To: David Hildenbrand
Cc: akpm, mhocko, dan.j.williams, jglisse, rafael, yasu.isimatu,
logang, dave.jiang, Jonathan.Cameron, vbabka, linux-mm,
linux-kernel, 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] mm/memory_hotplug: Refactor unregister_mem_sect_under_nodes
2018-08-13 15:46 ` [PATCH v2 3/3] mm/memory_hotplug: Refactor unregister_mem_sect_under_nodes osalvador
@ 2018-08-14 9:39 ` David Hildenbrand
2018-08-14 9:55 ` Oscar Salvador
0 siblings, 1 reply; 13+ messages in thread
From: David Hildenbrand @ 2018-08-14 9:39 UTC (permalink / raw)
To: osalvador, akpm
Cc: mhocko, dan.j.williams, jglisse, rafael, yasu.isimatu, logang,
dave.jiang, Jonathan.Cameron, vbabka, linux-mm, linux-kernel,
Oscar Salvador
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes
2018-08-14 9:36 ` Oscar Salvador
@ 2018-08-14 9:44 ` David Hildenbrand
2018-08-14 10:06 ` Oscar Salvador
0 siblings, 1 reply; 13+ messages in thread
From: David Hildenbrand @ 2018-08-14 9:44 UTC (permalink / raw)
To: Oscar Salvador
Cc: akpm, mhocko, dan.j.williams, jglisse, rafael, yasu.isimatu,
logang, dave.jiang, Jonathan.Cameron, vbabka, linux-mm,
linux-kernel, Oscar Salvador
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] mm/memory_hotplug: Refactor unregister_mem_sect_under_nodes
2018-08-14 9:39 ` David Hildenbrand
@ 2018-08-14 9:55 ` Oscar Salvador
0 siblings, 0 replies; 13+ messages in thread
From: Oscar Salvador @ 2018-08-14 9:55 UTC (permalink / raw)
To: David Hildenbrand
Cc: akpm, mhocko, dan.j.williams, jglisse, rafael, yasu.isimatu,
logang, dave.jiang, Jonathan.Cameron, vbabka, linux-mm,
linux-kernel, 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes
2018-08-14 9:44 ` David Hildenbrand
@ 2018-08-14 10:06 ` Oscar Salvador
2018-08-14 10:09 ` David Hildenbrand
0 siblings, 1 reply; 13+ messages in thread
From: Oscar Salvador @ 2018-08-14 10:06 UTC (permalink / raw)
To: David Hildenbrand
Cc: akpm, mhocko, dan.j.williams, jglisse, rafael, yasu.isimatu,
logang, dave.jiang, Jonathan.Cameron, vbabka, linux-mm,
linux-kernel, 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes
2018-08-14 10:06 ` Oscar Salvador
@ 2018-08-14 10:09 ` David Hildenbrand
2018-08-14 12:36 ` Oscar Salvador
0 siblings, 1 reply; 13+ messages in thread
From: David Hildenbrand @ 2018-08-14 10:09 UTC (permalink / raw)
To: Oscar Salvador
Cc: akpm, mhocko, dan.j.williams, jglisse, rafael, yasu.isimatu,
logang, dave.jiang, Jonathan.Cameron, vbabka, linux-mm,
linux-kernel, Oscar Salvador
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes
2018-08-14 10:09 ` David Hildenbrand
@ 2018-08-14 12:36 ` Oscar Salvador
0 siblings, 0 replies; 13+ messages in thread
From: Oscar Salvador @ 2018-08-14 12:36 UTC (permalink / raw)
To: David Hildenbrand
Cc: akpm, mhocko, dan.j.williams, jglisse, rafael, yasu.isimatu,
logang, dave.jiang, Jonathan.Cameron, vbabka, linux-mm,
linux-kernel, 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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-08-14 12:36 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-13 15:46 [PATCH v2 0/3] Refactoring for remove_memory_section/unregister_mem_sect_under_nodes osalvador
2018-08-13 15:46 ` [PATCH v2 1/3] mm/memory-hotplug: Drop unused args from remove_memory_section osalvador
2018-08-14 9:29 ` David Hildenbrand
2018-08-13 15:46 ` [PATCH v2 2/3] mm/memory_hotplug: Drop mem_blk check from unregister_mem_sect_under_nodes osalvador
2018-08-14 9:30 ` David Hildenbrand
2018-08-14 9:36 ` Oscar Salvador
2018-08-14 9:44 ` David Hildenbrand
2018-08-14 10:06 ` Oscar Salvador
2018-08-14 10:09 ` David Hildenbrand
2018-08-14 12:36 ` Oscar Salvador
2018-08-13 15:46 ` [PATCH v2 3/3] mm/memory_hotplug: Refactor unregister_mem_sect_under_nodes osalvador
2018-08-14 9:39 ` David Hildenbrand
2018-08-14 9:55 ` Oscar Salvador
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).