[PATCH v2 0/3] mm: Cleanup & allow modules to hotplug memory

[PATCH v2 0/3] mm: Cleanup & allow modules to hotplug memory

[Alastair D'Silva]

From: Alastair D'Silva <alastair@d-silva.org>

This series addresses some minor issues found when developing a
persistent memory driver.

Changelog:
V2:
  - Drop mm/hotplug: export try_online_node
        (not necessary)
  - Return errors from __section_nr
  - Remove errant whitespace change in
        mm: don't hide potentially null memmap pointer
  - Rework mm: don't hide potentially null memmap pointer
    to use a start & count
  - Drop mm/hotplug: Avoid RCU stalls when removing large amounts of memory
        (similar patch already went in)

Alastair D'Silva (3):
  mm: Trigger bug on if a section is not found in __section_nr
  mm: don't hide potentially null memmap pointer in
    sparse_remove_one_section
  mm: Don't manually decrement num_poisoned_pages

 drivers/base/memory.c | 18 +++++++++++++++---
 mm/sparse.c           | 21 +++++++++++++++------
 2 files changed, 30 insertions(+), 9 deletions(-)

-- 
2.21.0

[PATCH v2 1/3] mm: Trigger bug on if a section is not found in __section_nr

[Alastair D'Silva]

From: Alastair D'Silva <alastair@d-silva.org>

If a memory section comes in where the physical address is greater than
that which is managed by the kernel, this function would not trigger the
bug and instead return a bogus section number.

This patch tracks whether the section was actually found, and triggers the
bug if not.

Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
 drivers/base/memory.c | 18 +++++++++++++++---
 mm/sparse.c           |  7 ++++++-
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index f180427e48f4..9244c122abf1 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -585,13 +585,21 @@ int __weak arch_get_memory_phys_device(unsigned long start_pfn)
 struct memory_block *find_memory_block_hinted(struct mem_section *section,
 					      struct memory_block *hint)
 {
-	int block_id = base_memory_block_id(__section_nr(section));
+	int block_id, section_nr;
 	struct device *hintdev = hint ? &hint->dev : NULL;
 	struct device *dev;
 
+	section_nr = __section_nr(section);
+	if (section_nr < 0) {
+		if (hintdev)
+			put_device(hintdev);
+		return NULL;
+	}
+
+	block_id = base_memory_block_id(section_nr);
 	dev = subsys_find_device_by_id(&memory_subsys, block_id, hintdev);
-	if (hint)
-		put_device(&hint->dev);
+	if (hintdev)
+		put_device(hintdev);
 	if (!dev)
 		return NULL;
 	return to_memory_block(dev);
@@ -664,6 +672,10 @@ static int init_memory_block(struct memory_block **memory,
 		return -ENOMEM;
 
 	scn_nr = __section_nr(section);
+
+	if (scn_nr < 0)
+		return scn_nr;
+
 	mem->start_section_nr =
 			base_memory_block_id(scn_nr) * sections_per_block;
 	mem->end_section_nr = mem->start_section_nr + sections_per_block - 1;
diff --git a/mm/sparse.c b/mm/sparse.c
index fd13166949b5..57a1a3d9c1cf 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -113,10 +113,15 @@ int __section_nr(struct mem_section* ms)
 			continue;
 
 		if ((ms >= root) && (ms < (root + SECTIONS_PER_ROOT)))
-		     break;
+			break;
 	}
 
 	VM_BUG_ON(!root);
+	if (root_nr == NR_SECTION_ROOTS) {
+		VM_BUG_ON(true);
+
+		return -EINVAL;
+	}
 
 	return (root_nr * SECTIONS_PER_ROOT) + (ms - root);
 }
-- 
2.21.0

[PATCH v2 2/3] mm: don't hide potentially null memmap pointer in sparse_remove_one_section

[Alastair D'Silva]

From: Alastair D'Silva <alastair@d-silva.org>

By adding offset to memmap before passing it in to clear_hwpoisoned_pages,
we hide a potentially null memmap from the null check inside
clear_hwpoisoned_pages.

This patch passes the offset to clear_hwpoisoned_pages instead, allowing
memmap to successfully peform it's null check.

Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
 mm/sparse.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 57a1a3d9c1cf..1ec32aef5590 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -753,7 +753,8 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 
 #ifdef CONFIG_MEMORY_HOTREMOVE
 #ifdef CONFIG_MEMORY_FAILURE
-static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
+static void clear_hwpoisoned_pages(struct page *memmap,
+		unsigned long start, unsigned long count)
 {
 	int i;
 
@@ -769,7 +770,7 @@ static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
 	if (atomic_long_read(&num_poisoned_pages) == 0)
 		return;
 
-	for (i = 0; i < nr_pages; i++) {
+	for (i = start; i < start + count; i++) {
 		if (PageHWPoison(&memmap[i])) {
 			atomic_long_sub(1, &num_poisoned_pages);
 			ClearPageHWPoison(&memmap[i]);
@@ -777,7 +778,8 @@ static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
 	}
 }
 #else
-static inline void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
+static inline void clear_hwpoisoned_pages(struct page *memmap,
+		unsigned long start, unsigned long count)
 {
 }
 #endif
@@ -824,7 +826,7 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		ms->pageblock_flags = NULL;
 	}
 
-	clear_hwpoisoned_pages(memmap + map_offset,
+	clear_hwpoisoned_pages(memmap, map_offset,
 			PAGES_PER_SECTION - map_offset);
 	free_section_usemap(memmap, usemap, altmap);
 }
-- 
2.21.0

[PATCH v2 3/3] mm: Don't manually decrement num_poisoned_pages

[Alastair D'Silva]

From: Alastair D'Silva <alastair@d-silva.org>

Use the function written to do it instead.

Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
 mm/sparse.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 1ec32aef5590..d9b3625bfdf0 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -11,6 +11,8 @@
 #include <linux/export.h>
 #include <linux/spinlock.h>
 #include <linux/vmalloc.h>
+#include <linux/swap.h>
+#include <linux/swapops.h>
 
 #include "internal.h"
 #include <asm/dma.h>
@@ -772,7 +774,7 @@ static void clear_hwpoisoned_pages(struct page *memmap,
 
 	for (i = start; i < start + count; i++) {
 		if (PageHWPoison(&memmap[i])) {
-			atomic_long_sub(1, &num_poisoned_pages);
+			num_poisoned_pages_dec();
 			ClearPageHWPoison(&memmap[i]);
 		}
 	}
-- 
2.21.0

Re: [PATCH v2 1/3] mm: Trigger bug on if a section is not found in __section_nr

[Michal Hocko]

On Wed 26-06-19 16:11:21, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> If a memory section comes in where the physical address is greater than
> that which is managed by the kernel, this function would not trigger the
> bug and instead return a bogus section number.
> 
> This patch tracks whether the section was actually found, and triggers the
> bug if not.

Why do we want/need that? In other words the changelog should contina
WHY and WHAT. This one contains only the later one.
 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
>  drivers/base/memory.c | 18 +++++++++++++++---
>  mm/sparse.c           |  7 ++++++-
>  2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index f180427e48f4..9244c122abf1 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -585,13 +585,21 @@ int __weak arch_get_memory_phys_device(unsigned long start_pfn)
>  struct memory_block *find_memory_block_hinted(struct mem_section *section,
>  					      struct memory_block *hint)
>  {
> -	int block_id = base_memory_block_id(__section_nr(section));
> +	int block_id, section_nr;
>  	struct device *hintdev = hint ? &hint->dev : NULL;
>  	struct device *dev;
>  
> +	section_nr = __section_nr(section);
> +	if (section_nr < 0) {
> +		if (hintdev)
> +			put_device(hintdev);
> +		return NULL;
> +	}
> +
> +	block_id = base_memory_block_id(section_nr);
>  	dev = subsys_find_device_by_id(&memory_subsys, block_id, hintdev);
> -	if (hint)
> -		put_device(&hint->dev);
> +	if (hintdev)
> +		put_device(hintdev);
>  	if (!dev)
>  		return NULL;
>  	return to_memory_block(dev);
> @@ -664,6 +672,10 @@ static int init_memory_block(struct memory_block **memory,
>  		return -ENOMEM;
>  
>  	scn_nr = __section_nr(section);
> +
> +	if (scn_nr < 0)
> +		return scn_nr;
> +
>  	mem->start_section_nr =
>  			base_memory_block_id(scn_nr) * sections_per_block;
>  	mem->end_section_nr = mem->start_section_nr + sections_per_block - 1;
> diff --git a/mm/sparse.c b/mm/sparse.c
> index fd13166949b5..57a1a3d9c1cf 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -113,10 +113,15 @@ int __section_nr(struct mem_section* ms)
>  			continue;
>  
>  		if ((ms >= root) && (ms < (root + SECTIONS_PER_ROOT)))
> -		     break;
> +			break;
>  	}
>  
>  	VM_BUG_ON(!root);
> +	if (root_nr == NR_SECTION_ROOTS) {
> +		VM_BUG_ON(true);
> +
> +		return -EINVAL;
> +	}
>  
>  	return (root_nr * SECTIONS_PER_ROOT) + (ms - root);
>  }
> -- 
> 2.21.0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 2/3] mm: don't hide potentially null memmap pointer in sparse_remove_one_section

[Michal Hocko]

On Wed 26-06-19 16:11:22, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> By adding offset to memmap before passing it in to clear_hwpoisoned_pages,
> we hide a potentially null memmap from the null check inside
> clear_hwpoisoned_pages.
> 
> This patch passes the offset to clear_hwpoisoned_pages instead, allowing
> memmap to successfully peform it's null check.

Same issue with the changelog as the previous patch (missing WHY).

> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
>  mm/sparse.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 57a1a3d9c1cf..1ec32aef5590 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -753,7 +753,8 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  
>  #ifdef CONFIG_MEMORY_HOTREMOVE
>  #ifdef CONFIG_MEMORY_FAILURE
> -static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
> +static void clear_hwpoisoned_pages(struct page *memmap,
> +		unsigned long start, unsigned long count)
>  {
>  	int i;
>  
> @@ -769,7 +770,7 @@ static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
>  	if (atomic_long_read(&num_poisoned_pages) == 0)
>  		return;
>  
> -	for (i = 0; i < nr_pages; i++) {
> +	for (i = start; i < start + count; i++) {
>  		if (PageHWPoison(&memmap[i])) {
>  			atomic_long_sub(1, &num_poisoned_pages);
>  			ClearPageHWPoison(&memmap[i]);
> @@ -777,7 +778,8 @@ static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
>  	}
>  }
>  #else
> -static inline void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
> +static inline void clear_hwpoisoned_pages(struct page *memmap,
> +		unsigned long start, unsigned long count)
>  {
>  }
>  #endif
> @@ -824,7 +826,7 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		ms->pageblock_flags = NULL;
>  	}
>  
> -	clear_hwpoisoned_pages(memmap + map_offset,
> +	clear_hwpoisoned_pages(memmap, map_offset,
>  			PAGES_PER_SECTION - map_offset);
>  	free_section_usemap(memmap, usemap, altmap);
>  }
> -- 
> 2.21.0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 3/3] mm: Don't manually decrement num_poisoned_pages

[Michal Hocko]

On Wed 26-06-19 16:11:23, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> Use the function written to do it instead.

I am not sure a single line helper is a great win but this makes the
code consistent at least.

> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/sparse.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 1ec32aef5590..d9b3625bfdf0 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -11,6 +11,8 @@
>  #include <linux/export.h>
>  #include <linux/spinlock.h>
>  #include <linux/vmalloc.h>
> +#include <linux/swap.h>
> +#include <linux/swapops.h>
>  
>  #include "internal.h"
>  #include <asm/dma.h>
> @@ -772,7 +774,7 @@ static void clear_hwpoisoned_pages(struct page *memmap,
>  
>  	for (i = start; i < start + count; i++) {
>  		if (PageHWPoison(&memmap[i])) {
> -			atomic_long_sub(1, &num_poisoned_pages);
> +			num_poisoned_pages_dec();
>  			ClearPageHWPoison(&memmap[i]);
>  		}
>  	}
> -- 
> 2.21.0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 1/3] mm: Trigger bug on if a section is not found in __section_nr

[Alastair D'Silva]

On Wed, 2019-06-26 at 08:21 +0200, Michal Hocko wrote:
> On Wed 26-06-19 16:11:21, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> > 
> > If a memory section comes in where the physical address is greater
> > than
> > that which is managed by the kernel, this function would not
> > trigger the
> > bug and instead return a bogus section number.
> > 
> > This patch tracks whether the section was actually found, and
> > triggers the
> > bug if not.
> 
> Why do we want/need that? In other words the changelog should contina
> WHY and WHAT. This one contains only the later one.
>  

Thanks, I'll update the comment.

During driver development, I tried adding peristent memory at a memory
address that exceeded the maximum permissable address for the platform.

This caused __section_nr to silently return bogus section numbers,
rather than complaining.

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva    
Twitter: @EvilDeece
blog: http://alastair.d-silva.org

Re: [PATCH v2 2/3] mm: don't hide potentially null memmap pointer in sparse_remove_one_section

[Alastair D'Silva]

On Wed, 2019-06-26 at 08:23 +0200, Michal Hocko wrote:
> On Wed 26-06-19 16:11:22, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> > 
> > By adding offset to memmap before passing it in to
> > clear_hwpoisoned_pages,
> > we hide a potentially null memmap from the null check inside
> > clear_hwpoisoned_pages.
> > 
> > This patch passes the offset to clear_hwpoisoned_pages instead,
> > allowing
> > memmap to successfully peform it's null check.
> 
> Same issue with the changelog as the previous patch (missing WHY).
> 

The first paragraph explains what the problem is with the existing code
(same applies to 1/3 too).

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva    
Twitter: @EvilDeece
blog: http://alastair.d-silva.org

Re: [PATCH v2 1/3] mm: Trigger bug on if a section is not found in __section_nr

[Michal Hocko]

On Wed 26-06-19 16:27:30, Alastair D'Silva wrote:
> On Wed, 2019-06-26 at 08:21 +0200, Michal Hocko wrote:
> > On Wed 26-06-19 16:11:21, Alastair D'Silva wrote:
> > > From: Alastair D'Silva <alastair@d-silva.org>
> > > 
> > > If a memory section comes in where the physical address is greater
> > > than
> > > that which is managed by the kernel, this function would not
> > > trigger the
> > > bug and instead return a bogus section number.
> > > 
> > > This patch tracks whether the section was actually found, and
> > > triggers the
> > > bug if not.
> > 
> > Why do we want/need that? In other words the changelog should contina
> > WHY and WHAT. This one contains only the later one.
> >  
> 
> Thanks, I'll update the comment.
> 
> During driver development, I tried adding peristent memory at a memory
> address that exceeded the maximum permissable address for the platform.
> 
> This caused __section_nr to silently return bogus section numbers,
> rather than complaining.

OK, I see, but is an additional code worth it for the non-development
case? I mean why should we be testing for something that shouldn't
happen normally? Is it too easy to get things wrong or what is the
underlying reason to change it now?

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 2/3] mm: don't hide potentially null memmap pointer in sparse_remove_one_section

[Michal Hocko]

On Wed 26-06-19 16:30:55, Alastair D'Silva wrote:
> On Wed, 2019-06-26 at 08:23 +0200, Michal Hocko wrote:
> > On Wed 26-06-19 16:11:22, Alastair D'Silva wrote:
> > > From: Alastair D'Silva <alastair@d-silva.org>
> > > 
> > > By adding offset to memmap before passing it in to
> > > clear_hwpoisoned_pages,
> > > we hide a potentially null memmap from the null check inside
> > > clear_hwpoisoned_pages.
> > > 
> > > This patch passes the offset to clear_hwpoisoned_pages instead,
> > > allowing
> > > memmap to successfully peform it's null check.
> > 
> > Same issue with the changelog as the previous patch (missing WHY).
> > 
> 
> The first paragraph explains what the problem is with the existing code
> (same applies to 1/3 too).

Under what conditions that happens? Is this a theoretical problem or can
you hit this by a (buggy) code? Please be much more specific.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 0/3] mm: Cleanup & allow modules to hotplug memory

[Christoph Hellwig]

On Wed, Jun 26, 2019 at 04:11:20PM +1000, Alastair D'Silva wrote:
>   - Drop mm/hotplug: export try_online_node
>         (not necessary)

With this the subject line of the cover letter seems incorrect now :)

Re: [PATCH v2 1/3] mm: Trigger bug on if a section is not found in __section_nr

[Alastair D'Silva]

On Wed, 2019-06-26 at 08:57 +0200, Michal Hocko wrote:
> On Wed 26-06-19 16:27:30, Alastair D'Silva wrote:
> > On Wed, 2019-06-26 at 08:21 +0200, Michal Hocko wrote:
> > > On Wed 26-06-19 16:11:21, Alastair D'Silva wrote:
> > > > From: Alastair D'Silva <alastair@d-silva.org>
> > > > 
> > > > If a memory section comes in where the physical address is
> > > > greater
> > > > than
> > > > that which is managed by the kernel, this function would not
> > > > trigger the
> > > > bug and instead return a bogus section number.
> > > > 
> > > > This patch tracks whether the section was actually found, and
> > > > triggers the
> > > > bug if not.
> > > 
> > > Why do we want/need that? In other words the changelog should
> > > contina
> > > WHY and WHAT. This one contains only the later one.
> > >  
> > 
> > Thanks, I'll update the comment.
> > 
> > During driver development, I tried adding peristent memory at a
> > memory
> > address that exceeded the maximum permissable address for the
> > platform.
> > 
> > This caused __section_nr to silently return bogus section numbers,
> > rather than complaining.
> 
> OK, I see, but is an additional code worth it for the non-development
> case? I mean why should we be testing for something that shouldn't
> happen normally? Is it too easy to get things wrong or what is the
> underlying reason to change it now?
> 

It took me a while to identify what the problem was - having the BUG_ON
would have saved me a few hours.

I'm happy to just have the BUG_ON 'nd drop the new error return (I
added that in response to Mike Rapoport's comment that the original
patch would still return a bogus section number).


-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva    
Twitter: @EvilDeece
blog: http://alastair.d-silva.org

Re: [PATCH v2 0/3] mm: Cleanup & allow modules to hotplug memory

[Alastair D'Silva]

On Wed, 2019-06-26 at 00:57 -0700, Christoph Hellwig wrote:
> On Wed, Jun 26, 2019 at 04:11:20PM +1000, Alastair D'Silva wrote:
> >   - Drop mm/hotplug: export try_online_node
> >         (not necessary)
> 
> With this the subject line of the cover letter seems incorrect now :)
> 

Indeed :)

I left it as I was unsure whether changing the series title would make
it harder to track revisions.


-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva    
Twitter: @EvilDeece
blog: http://alastair.d-silva.org

Re: [PATCH v2 1/3] mm: Trigger bug on if a section is not found in __section_nr

[Michal Hocko]

On Thu 27-06-19 10:50:57, Alastair D'Silva wrote:
> On Wed, 2019-06-26 at 08:57 +0200, Michal Hocko wrote:
> > On Wed 26-06-19 16:27:30, Alastair D'Silva wrote:
> > > On Wed, 2019-06-26 at 08:21 +0200, Michal Hocko wrote:
> > > > On Wed 26-06-19 16:11:21, Alastair D'Silva wrote:
> > > > > From: Alastair D'Silva <alastair@d-silva.org>
> > > > > 
> > > > > If a memory section comes in where the physical address is
> > > > > greater
> > > > > than
> > > > > that which is managed by the kernel, this function would not
> > > > > trigger the
> > > > > bug and instead return a bogus section number.
> > > > > 
> > > > > This patch tracks whether the section was actually found, and
> > > > > triggers the
> > > > > bug if not.
> > > > 
> > > > Why do we want/need that? In other words the changelog should
> > > > contina
> > > > WHY and WHAT. This one contains only the later one.
> > > >  
> > > 
> > > Thanks, I'll update the comment.
> > > 
> > > During driver development, I tried adding peristent memory at a
> > > memory
> > > address that exceeded the maximum permissable address for the
> > > platform.
> > > 
> > > This caused __section_nr to silently return bogus section numbers,
> > > rather than complaining.
> > 
> > OK, I see, but is an additional code worth it for the non-development
> > case? I mean why should we be testing for something that shouldn't
> > happen normally? Is it too easy to get things wrong or what is the
> > underlying reason to change it now?
> > 
> 
> It took me a while to identify what the problem was - having the BUG_ON
> would have saved me a few hours.
> 
> I'm happy to just have the BUG_ON 'nd drop the new error return (I
> added that in response to Mike Rapoport's comment that the original
> patch would still return a bogus section number).

Well, BUG_ON is about the worst way to handle an incorrect input. You
really do not want to put a production environment down just because
there is a bug in a driver, right? There are still many {VM_}BUG_ONs
in the tree and there is a general trend to get rid of many of them
rather than adding new ones.

Now back to your patch. You are adding an error handling where we simply
do not expect invalid data. This is often the case for the core kernel
functionality because we do expect consumers of the code to do the right
thing. E.g. __section_nr already takes a pointer to struct section which
assumes that this core data structure is already valid. Adding a check
here adds an unnecessary overhead so this doesn't really sound like a
good idea to me.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 1/3] mm: Trigger bug on if a section is not found in __section_nr

[Alastair D'Silva]

On Thu, 2019-06-27 at 10:10 +0200, Michal Hocko wrote:
> On Thu 27-06-19 10:50:57, Alastair D'Silva wrote:
> > On Wed, 2019-06-26 at 08:57 +0200, Michal Hocko wrote:
> > > On Wed 26-06-19 16:27:30, Alastair D'Silva wrote:
> > > > On Wed, 2019-06-26 at 08:21 +0200, Michal Hocko wrote:
> > > > > On Wed 26-06-19 16:11:21, Alastair D'Silva wrote:
> > > > > > From: Alastair D'Silva <alastair@d-silva.org>
> > > > > > 
> > > > > > If a memory section comes in where the physical address is
> > > > > > greater
> > > > > > than
> > > > > > that which is managed by the kernel, this function would
> > > > > > not
> > > > > > trigger the
> > > > > > bug and instead return a bogus section number.
> > > > > > 
> > > > > > This patch tracks whether the section was actually found,
> > > > > > and
> > > > > > triggers the
> > > > > > bug if not.
> > > > > 
> > > > > Why do we want/need that? In other words the changelog should
> > > > > contina
> > > > > WHY and WHAT. This one contains only the later one.
> > > > >  
> > > > 
> > > > Thanks, I'll update the comment.
> > > > 
> > > > During driver development, I tried adding peristent memory at a
> > > > memory
> > > > address that exceeded the maximum permissable address for the
> > > > platform.
> > > > 
> > > > This caused __section_nr to silently return bogus section
> > > > numbers,
> > > > rather than complaining.
> > > 
> > > OK, I see, but is an additional code worth it for the non-
> > > development
> > > case? I mean why should we be testing for something that
> > > shouldn't
> > > happen normally? Is it too easy to get things wrong or what is
> > > the
> > > underlying reason to change it now?
> > > 
> > 
> > It took me a while to identify what the problem was - having the
> > BUG_ON
> > would have saved me a few hours.
> > 
> > I'm happy to just have the BUG_ON 'nd drop the new error return (I
> > added that in response to Mike Rapoport's comment that the original
> > patch would still return a bogus section number).
> 
> Well, BUG_ON is about the worst way to handle an incorrect input. You
> really do not want to put a production environment down just because
> there is a bug in a driver, right? There are still many {VM_}BUG_ONs
> in the tree and there is a general trend to get rid of many of them
> rather than adding new ones.
> 
> Now back to your patch. You are adding an error handling where we
> simply
> do not expect invalid data. This is often the case for the core
> kernel
> functionality because we do expect consumers of the code to do the
> right
> thing. E.g. __section_nr already takes a pointer to struct section
> which
> assumes that this core data structure is already valid. Adding a
> check
> here adds an unnecessary overhead so this doesn't really sound like a
> good idea to me.


Thanks for providing a good explanation.

In this situation, since we return a bogus section number, we get
crashes elsewhere in the kernel if the code rumbles on.

Given that there is already a VM_BUG_ON in the code, how do you feel
about broadening the scope from 'VM_BUG_ON(!root)' to 'VM_BUG_ON(!root
|| (root_nr == NR_SECTION_ROOTS))'?

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva    
Twitter: @EvilDeece
blog: http://alastair.d-silva.org

Re: [PATCH v2 2/3] mm: don't hide potentially null memmap pointer in sparse_remove_one_section

[David Hildenbrand]

On 26.06.19 08:11, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> By adding offset to memmap before passing it in to clear_hwpoisoned_pages,
> we hide a potentially null memmap from the null check inside
> clear_hwpoisoned_pages.
> 
> This patch passes the offset to clear_hwpoisoned_pages instead, allowing
> memmap to successfully peform it's null check.
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
>  mm/sparse.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 57a1a3d9c1cf..1ec32aef5590 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -753,7 +753,8 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  
>  #ifdef CONFIG_MEMORY_HOTREMOVE
>  #ifdef CONFIG_MEMORY_FAILURE
> -static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
> +static void clear_hwpoisoned_pages(struct page *memmap,
> +		unsigned long start, unsigned long count)
>  {
>  	int i;
>  
> @@ -769,7 +770,7 @@ static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
>  	if (atomic_long_read(&num_poisoned_pages) == 0)
>  		return;
>  
> -	for (i = 0; i < nr_pages; i++) {
> +	for (i = start; i < start + count; i++) {

start and count are unsigned long's, i is an int.

Besides that

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

>  		if (PageHWPoison(&memmap[i])) {
>  			atomic_long_sub(1, &num_poisoned_pages);
>  			ClearPageHWPoison(&memmap[i]);
> @@ -777,7 +778,8 @@ static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
>  	}
>  }
>  #else
> -static inline void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
> +static inline void clear_hwpoisoned_pages(struct page *memmap,
> +		unsigned long start, unsigned long count)
>  {
>  }
>  #endif
> @@ -824,7 +826,7 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		ms->pageblock_flags = NULL;
>  	}
>  
> -	clear_hwpoisoned_pages(memmap + map_offset,
> +	clear_hwpoisoned_pages(memmap, map_offset,
>  			PAGES_PER_SECTION - map_offset);
>  	free_section_usemap(memmap, usemap, altmap);
>  }
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v2 3/3] mm: Don't manually decrement num_poisoned_pages

[David Hildenbrand]

On 26.06.19 08:11, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> Use the function written to do it instead.
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
>  mm/sparse.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 1ec32aef5590..d9b3625bfdf0 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -11,6 +11,8 @@
>  #include <linux/export.h>
>  #include <linux/spinlock.h>
>  #include <linux/vmalloc.h>
> +#include <linux/swap.h>
> +#include <linux/swapops.h>
>  
>  #include "internal.h"
>  #include <asm/dma.h>
> @@ -772,7 +774,7 @@ static void clear_hwpoisoned_pages(struct page *memmap,
>  
>  	for (i = start; i < start + count; i++) {
>  		if (PageHWPoison(&memmap[i])) {
> -			atomic_long_sub(1, &num_poisoned_pages);
> +			num_poisoned_pages_dec();
>  			ClearPageHWPoison(&memmap[i]);
>  		}
>  	}
> 

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

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 1/3] mm: Trigger bug on if a section is not found in __section_nr

[David Hildenbrand]

On 27.06.19 10:10, Michal Hocko wrote:
> On Thu 27-06-19 10:50:57, Alastair D'Silva wrote:
>> On Wed, 2019-06-26 at 08:57 +0200, Michal Hocko wrote:
>>> On Wed 26-06-19 16:27:30, Alastair D'Silva wrote:
>>>> On Wed, 2019-06-26 at 08:21 +0200, Michal Hocko wrote:
>>>>> On Wed 26-06-19 16:11:21, Alastair D'Silva wrote:
>>>>>> From: Alastair D'Silva <alastair@d-silva.org>
>>>>>>
>>>>>> If a memory section comes in where the physical address is
>>>>>> greater
>>>>>> than
>>>>>> that which is managed by the kernel, this function would not
>>>>>> trigger the
>>>>>> bug and instead return a bogus section number.
>>>>>>
>>>>>> This patch tracks whether the section was actually found, and
>>>>>> triggers the
>>>>>> bug if not.
>>>>>
>>>>> Why do we want/need that? In other words the changelog should
>>>>> contina
>>>>> WHY and WHAT. This one contains only the later one.
>>>>>  
>>>>
>>>> Thanks, I'll update the comment.
>>>>
>>>> During driver development, I tried adding peristent memory at a
>>>> memory
>>>> address that exceeded the maximum permissable address for the
>>>> platform.
>>>>
>>>> This caused __section_nr to silently return bogus section numbers,
>>>> rather than complaining.
>>>
>>> OK, I see, but is an additional code worth it for the non-development
>>> case? I mean why should we be testing for something that shouldn't
>>> happen normally? Is it too easy to get things wrong or what is the
>>> underlying reason to change it now?
>>>
>>
>> It took me a while to identify what the problem was - having the BUG_ON
>> would have saved me a few hours.
>>
>> I'm happy to just have the BUG_ON 'nd drop the new error return (I
>> added that in response to Mike Rapoport's comment that the original
>> patch would still return a bogus section number).
> 
> Well, BUG_ON is about the worst way to handle an incorrect input. You
> really do not want to put a production environment down just because
> there is a bug in a driver, right? There are still many {VM_}BUG_ONs
> in the tree and there is a general trend to get rid of many of them
> rather than adding new ones.

VM_BUG_ON is only really active with CONFIG_DEBUG_VM. On
!CONFIG_DEBUG_VM it translated to BUILD_BUG_ON_INVALID(), which is a
compile-time only check.

Or am I missing something?

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 1/3] mm: Trigger bug on if a section is not found in __section_nr

[Michal Hocko]

On Fri 28-06-19 13:37:07, David Hildenbrand wrote:
> VM_BUG_ON is only really active with CONFIG_DEBUG_VM. On
> !CONFIG_DEBUG_VM it translated to BUILD_BUG_ON_INVALID(), which is a
> compile-time only check.
> 
> Or am I missing something?

You are not missing anything.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 1/3] mm: Trigger bug on if a section is not found in __section_nr

[Michal Hocko]

On Fri 28-06-19 10:46:28, Alastair D'Silva wrote:
[...]
> Given that there is already a VM_BUG_ON in the code, how do you feel
> about broadening the scope from 'VM_BUG_ON(!root)' to 'VM_BUG_ON(!root
> || (root_nr == NR_SECTION_ROOTS))'?

As far as I understand the existing VM_BUG_ON will hit when the
mem_section tree gets corrupted. This is a different situation to an
incorrect section given so I wouldn't really mix those two. And I still
do not see much point to protect from unexpected input parameter as this
is internal function as already pointed out.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 1/3] mm: Trigger bug on if a section is not found in __section_nr

[Alastair D'Silva]

On Mon, 2019-07-01 at 12:46 +0200, Michal Hocko wrote:
> On Fri 28-06-19 10:46:28, Alastair D'Silva wrote:
> [...]
> > Given that there is already a VM_BUG_ON in the code, how do you
> > feel
> > about broadening the scope from 'VM_BUG_ON(!root)' to
> > 'VM_BUG_ON(!root
> > > > (root_nr == NR_SECTION_ROOTS))'?
> 
> As far as I understand the existing VM_BUG_ON will hit when the
> mem_section tree gets corrupted. This is a different situation to an
> incorrect section given so I wouldn't really mix those two. And I
> still
> do not see much point to protect from unexpected input parameter as
> this
> is internal function as already pointed out.
> 

Hi Michael,

I was able to hit this problem as the system firmware had assigned the
prototype pmem device an address range above the 128TB limit that we
originally supported. This has since been lifted to 2PB with patch
4ffe713b7587b14695c9bec26a000fc88ef54895.

As it stands, we cannot move this range lower as the high bits are
dictated by the location the card is connected.

Since the physical address of the memory is not controlled by the
kernel, I believe we should catch (or at least make it easy to debug)
the sitution where external firmware allocates physical addresses
beyond that which the kernel supports.

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva    
Twitter: @EvilDeece
blog: http://alastair.d-silva.org

Re: [PATCH v2 1/3] mm: Trigger bug on if a section is not found in __section_nr

[Michal Hocko]

On Tue 02-07-19 14:13:25, Alastair D'Silva wrote:
> On Mon, 2019-07-01 at 12:46 +0200, Michal Hocko wrote:
> > On Fri 28-06-19 10:46:28, Alastair D'Silva wrote:
> > [...]
> > > Given that there is already a VM_BUG_ON in the code, how do you
> > > feel
> > > about broadening the scope from 'VM_BUG_ON(!root)' to
> > > 'VM_BUG_ON(!root
> > > > > (root_nr == NR_SECTION_ROOTS))'?
> > 
> > As far as I understand the existing VM_BUG_ON will hit when the
> > mem_section tree gets corrupted. This is a different situation to an
> > incorrect section given so I wouldn't really mix those two. And I
> > still
> > do not see much point to protect from unexpected input parameter as
> > this
> > is internal function as already pointed out.
> > 
> 
> Hi Michael,
> 
> I was able to hit this problem as the system firmware had assigned the
> prototype pmem device an address range above the 128TB limit that we
> originally supported. This has since been lifted to 2PB with patch
> 4ffe713b7587b14695c9bec26a000fc88ef54895.
> 
> As it stands, we cannot move this range lower as the high bits are
> dictated by the location the card is connected.
> 
> Since the physical address of the memory is not controlled by the
> kernel, I believe we should catch (or at least make it easy to debug)
> the sitution where external firmware allocates physical addresses
> beyond that which the kernel supports.

Just make it clear, I am not against a sanitization. I am objecting to
put it into __section_nr because this is way too late. As already
explained, you already must have a bogus mem_section object in hand.
Why cannot you add a sanity check right there when the memory is added?
Either when the section is registered or even sooner in arch_add_memory.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 1/3] mm: Trigger bug on if a section is not found in __section_nr

[Alastair D'Silva]

On Tue, 2019-07-02 at 08:13 +0200, Michal Hocko wrote:
> On Tue 02-07-19 14:13:25, Alastair D'Silva wrote:
> > On Mon, 2019-07-01 at 12:46 +0200, Michal Hocko wrote:
> > > On Fri 28-06-19 10:46:28, Alastair D'Silva wrote:
> > > [...]
> > > > Given that there is already a VM_BUG_ON in the code, how do you
> > > > feel
> > > > about broadening the scope from 'VM_BUG_ON(!root)' to
> > > > 'VM_BUG_ON(!root
> > > > > > (root_nr == NR_SECTION_ROOTS))'?
> > > 
> > > As far as I understand the existing VM_BUG_ON will hit when the
> > > mem_section tree gets corrupted. This is a different situation to
> > > an
> > > incorrect section given so I wouldn't really mix those two. And I
> > > still
> > > do not see much point to protect from unexpected input parameter
> > > as
> > > this
> > > is internal function as already pointed out.
> > > 
> > 
> > Hi Michael,
> > 
> > I was able to hit this problem as the system firmware had assigned
> > the
> > prototype pmem device an address range above the 128TB limit that
> > we
> > originally supported. This has since been lifted to 2PB with patch
> > 4ffe713b7587b14695c9bec26a000fc88ef54895.
> > 
> > As it stands, we cannot move this range lower as the high bits are
> > dictated by the location the card is connected.
> > 
> > Since the physical address of the memory is not controlled by the
> > kernel, I believe we should catch (or at least make it easy to
> > debug)
> > the sitution where external firmware allocates physical addresses
> > beyond that which the kernel supports.
> 
> Just make it clear, I am not against a sanitization. I am objecting
> to
> put it into __section_nr because this is way too late. As already
> explained, you already must have a bogus mem_section object in hand.
> Why cannot you add a sanity check right there when the memory is
> added?
> Either when the section is registered or even sooner in
> arch_add_memory.
> 

Good point, I was thinking of a libnvdimm enhancement to check that the
end address is in range, but a more generic solution is better.

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva    
Twitter: @EvilDeece
blog: http://alastair.d-silva.org