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

[PATCH 0/5] 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.

As well as cleanup code, it also exports try_online_node so that
it can be called from driver modules that provide access to additional
physical memory.

Alastair D'Silva (5):
  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
  mm/hotplug: Avoid RCU stalls when removing large amounts of memory
  mm/hotplug: export try_online_node

 include/linux/memory_hotplug.h |  4 ++--
 kernel/cpu.c                   |  2 +-
 mm/memory_hotplug.c            | 23 ++++++++++++++++++-----
 mm/sparse.c                    | 28 +++++++++++++++++-----------
 4 files changed, 38 insertions(+), 19 deletions(-)

-- 
2.21.0

[PATCH 1/5] 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>
---
 mm/sparse.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index fd13166949b5..104a79fedd00 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -105,20 +105,23 @@ static inline int sparse_index_init(unsigned long section_nr, int nid)
 int __section_nr(struct mem_section* ms)
 {
 	unsigned long root_nr;
-	struct mem_section *root = NULL;
+	struct mem_section *found = NULL;
+	struct mem_section *root;
 
 	for (root_nr = 0; root_nr < NR_SECTION_ROOTS; root_nr++) {
 		root = __nr_to_section(root_nr * SECTIONS_PER_ROOT);
 		if (!root)
 			continue;
 
-		if ((ms >= root) && (ms < (root + SECTIONS_PER_ROOT)))
-		     break;
+		if ((ms >= root) && (ms < (root + SECTIONS_PER_ROOT))) {
+			found = root;
+			break;
+		}
 	}
 
-	VM_BUG_ON(!root);
+	VM_BUG_ON(!found);
 
-	return (root_nr * SECTIONS_PER_ROOT) + (ms - root);
+	return (root_nr * SECTIONS_PER_ROOT) + (ms - found);
 }
 #else
 int __section_nr(struct mem_section* ms)
-- 
2.21.0

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

[Mike Rapoport]

On Mon, Jun 17, 2019 at 02:36:27PM +1000, 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.
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
>  mm/sparse.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index fd13166949b5..104a79fedd00 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -105,20 +105,23 @@ static inline int sparse_index_init(unsigned long section_nr, int nid)
>  int __section_nr(struct mem_section* ms)
>  {
>  	unsigned long root_nr;
> -	struct mem_section *root = NULL;
> +	struct mem_section *found = NULL;
> +	struct mem_section *root;
> 
>  	for (root_nr = 0; root_nr < NR_SECTION_ROOTS; root_nr++) {
>  		root = __nr_to_section(root_nr * SECTIONS_PER_ROOT);
>  		if (!root)
>  			continue;
> 
> -		if ((ms >= root) && (ms < (root + SECTIONS_PER_ROOT)))
> -		     break;
> +		if ((ms >= root) && (ms < (root + SECTIONS_PER_ROOT))) {
> +			found = root;
> +			break;
> +		}
>  	}
> 
> -	VM_BUG_ON(!root);
> +	VM_BUG_ON(!found);

Isn't it enough to check for root_nr == NR_SECTION_ROOTS?

> 
> -	return (root_nr * SECTIONS_PER_ROOT) + (ms - root);
> +	return (root_nr * SECTIONS_PER_ROOT) + (ms - found);

It'll still return a bogus section number with CONFIG_DEBUG_VM=n

>  }
>  #else
>  int __section_nr(struct mem_section* ms)
> -- 
> 2.21.0
> 

-- 
Sincerely yours,
Mike.

[PATCH 2/5] 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,
is hides 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 | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 104a79fedd00..66a99da9b11b 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -746,12 +746,14 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 		kfree(usemap);
 		__kfree_section_memmap(memmap, altmap);
 	}
+
 	return ret;
 }
 
 #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 map_offset, int nr_pages)
 {
 	int i;
 
@@ -767,7 +769,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 = map_offset; i < nr_pages; i++) {
 		if (PageHWPoison(&memmap[i])) {
 			atomic_long_sub(1, &num_poisoned_pages);
 			ClearPageHWPoison(&memmap[i]);
@@ -775,7 +777,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 map_offset, int nr_pages)
 {
 }
 #endif
@@ -822,8 +825,7 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		ms->pageblock_flags = NULL;
 	}
 
-	clear_hwpoisoned_pages(memmap + map_offset,
-			PAGES_PER_SECTION - map_offset);
+	clear_hwpoisoned_pages(memmap, map_offset, PAGES_PER_SECTION);
 	free_section_usemap(memmap, usemap, altmap);
 }
 #endif /* CONFIG_MEMORY_HOTREMOVE */
-- 
2.21.0

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

[Mike Rapoport]

On Mon, Jun 17, 2019 at 02:36:28PM +1000, 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,
> is hides 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>

One nit below, otherwise

Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>

> ---
>  mm/sparse.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 104a79fedd00..66a99da9b11b 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -746,12 +746,14 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  		kfree(usemap);
>  		__kfree_section_memmap(memmap, altmap);
>  	}
> +

The whitespace change here is not related

>  	return ret;
>  }
> 
>  #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 map_offset, int nr_pages)
>  {
>  	int i;
> 
> @@ -767,7 +769,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 = map_offset; i < nr_pages; i++) {
>  		if (PageHWPoison(&memmap[i])) {
>  			atomic_long_sub(1, &num_poisoned_pages);
>  			ClearPageHWPoison(&memmap[i]);
> @@ -775,7 +777,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 map_offset, int nr_pages)
>  {
>  }
>  #endif
> @@ -822,8 +825,7 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		ms->pageblock_flags = NULL;
>  	}
> 
> -	clear_hwpoisoned_pages(memmap + map_offset,
> -			PAGES_PER_SECTION - map_offset);
> +	clear_hwpoisoned_pages(memmap, map_offset, PAGES_PER_SECTION);
>  	free_section_usemap(memmap, usemap, altmap);
>  }
>  #endif /* CONFIG_MEMORY_HOTREMOVE */
> -- 
> 2.21.0
> 

-- 
Sincerely yours,
Mike.

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

[David Hildenbrand]

On 17.06.19 06:36, 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,
> is hides 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 | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 104a79fedd00..66a99da9b11b 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -746,12 +746,14 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  		kfree(usemap);
>  		__kfree_section_memmap(memmap, altmap);
>  	}
> +
>  	return ret;
>  }
>  
>  #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 map_offset, int nr_pages)
>  {
>  	int i;
>  
> @@ -767,7 +769,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 = map_offset; i < nr_pages; i++) {
>  		if (PageHWPoison(&memmap[i])) {
>  			atomic_long_sub(1, &num_poisoned_pages);
>  			ClearPageHWPoison(&memmap[i]);
> @@ -775,7 +777,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 map_offset, int nr_pages)

I somewhat dislike that map_offset modifies nr_pages internally.

I would prefer decoupling both and passing the actual number of pages to
clear instead:

clear_hwpoisoned_pages(memmap, map_offset,
		       PAGES_PER_SECTION - map_offset);


>  {
>  }
>  #endif
> @@ -822,8 +825,7 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		ms->pageblock_flags = NULL;
>  	}
>  
> -	clear_hwpoisoned_pages(memmap + map_offset,
> -			PAGES_PER_SECTION - map_offset);
> +	clear_hwpoisoned_pages(memmap, map_offset, PAGES_PER_SECTION);
>  	free_section_usemap(memmap, usemap, altmap);
>  }
>  #endif /* CONFIG_MEMORY_HOTREMOVE */
> 


-- 

Thanks,

David / dhildenb

[PATCH 3/5] 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 66a99da9b11b..e2402937efe4 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>
@@ -771,7 +773,7 @@ static void clear_hwpoisoned_pages(struct page *memmap,
 
 	for (i = map_offset; i < nr_pages; i++) {
 		if (PageHWPoison(&memmap[i])) {
-			atomic_long_sub(1, &num_poisoned_pages);
+			num_poisoned_pages_dec();
 			ClearPageHWPoison(&memmap[i]);
 		}
 	}
-- 
2.21.0

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

[Mike Rapoport]

On Mon, Jun 17, 2019 at 02:36:29PM +1000, 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>

Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>

> ---
>  mm/sparse.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 66a99da9b11b..e2402937efe4 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>
> @@ -771,7 +773,7 @@ static void clear_hwpoisoned_pages(struct page *memmap,
> 
>  	for (i = map_offset; i < nr_pages; i++) {
>  		if (PageHWPoison(&memmap[i])) {
> -			atomic_long_sub(1, &num_poisoned_pages);
> +			num_poisoned_pages_dec();
>  			ClearPageHWPoison(&memmap[i]);
>  		}
>  	}
> -- 
> 2.21.0
> 

-- 
Sincerely yours,
Mike.

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

[David Hildenbrand]

On 17.06.19 06:36, 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 66a99da9b11b..e2402937efe4 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>
> @@ -771,7 +773,7 @@ static void clear_hwpoisoned_pages(struct page *memmap,
>  
>  	for (i = map_offset; i < nr_pages; 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

[PATCH 4/5] mm/hotplug: Avoid RCU stalls when removing large amounts of memory

[Alastair D'Silva]

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

When removing sufficiently large amounts of memory, we trigger RCU stall
detection. By periodically calling cond_resched(), we avoid bogus stall
warnings.

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

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index e096c987d261..382b3a0c9333 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -578,6 +578,9 @@ void __remove_pages(struct zone *zone, unsigned long phys_start_pfn,
 		__remove_section(zone, __pfn_to_section(pfn), map_offset,
 				 altmap);
 		map_offset = 0;
+
+		if (!(i & 0x0FFF))
+			cond_resched();
 	}
 
 	set_zone_contiguous(zone);
-- 
2.21.0

Re: [PATCH 4/5] mm/hotplug: Avoid RCU stalls when removing large amounts of memory

[Mike Rapoport]

On Mon, Jun 17, 2019 at 02:36:30PM +1000, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> When removing sufficiently large amounts of memory, we trigger RCU stall
> detection. By periodically calling cond_resched(), we avoid bogus stall
> warnings.
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
>  mm/memory_hotplug.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index e096c987d261..382b3a0c9333 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -578,6 +578,9 @@ void __remove_pages(struct zone *zone, unsigned long phys_start_pfn,
>  		__remove_section(zone, __pfn_to_section(pfn), map_offset,
>  				 altmap);
>  		map_offset = 0;
> +
> +		if (!(i & 0x0FFF))

No magic numbers please. And a comment would be appreciated.

> +			cond_resched();
>  	}
> 
>  	set_zone_contiguous(zone);
> -- 
> 2.21.0
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH 4/5] mm/hotplug: Avoid RCU stalls when removing large amounts of memory

[Alastair D'Silva]

On Mon, 2019-06-17 at 09:53 +0300, Mike Rapoport wrote:
> On Mon, Jun 17, 2019 at 02:36:30PM +1000, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> > 
> > When removing sufficiently large amounts of memory, we trigger RCU
> > stall
> > detection. By periodically calling cond_resched(), we avoid bogus
> > stall
> > warnings.
> > 
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> >  mm/memory_hotplug.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index e096c987d261..382b3a0c9333 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -578,6 +578,9 @@ void __remove_pages(struct zone *zone, unsigned
> > long phys_start_pfn,
> >  		__remove_section(zone, __pfn_to_section(pfn),
> > map_offset,
> >  				 altmap);
> >  		map_offset = 0;
> > +
> > +		if (!(i & 0x0FFF))
> 
> No magic numbers please. And a comment would be appreciated.
> 

Agreed, thanks for the review.

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

Re: [PATCH 4/5] mm/hotplug: Avoid RCU stalls when removing large amounts of memory

[Michal Hocko]

On Mon 17-06-19 14:36:30,  Alastair D'Silva  wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> When removing sufficiently large amounts of memory, we trigger RCU stall
> detection. By periodically calling cond_resched(), we avoid bogus stall
> warnings.
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
>  mm/memory_hotplug.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index e096c987d261..382b3a0c9333 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -578,6 +578,9 @@ void __remove_pages(struct zone *zone, unsigned long phys_start_pfn,
>  		__remove_section(zone, __pfn_to_section(pfn), map_offset,
>  				 altmap);
>  		map_offset = 0;
> +
> +		if (!(i & 0x0FFF))
> +			cond_resched();

We already do have cond_resched before __remove_section. Why is an
additional needed?

>  	}
>  
>  	set_zone_contiguous(zone);
> -- 
> 2.21.0
> 

-- 
Michal Hocko
SUSE Labs

RE: [PATCH 4/5] mm/hotplug: Avoid RCU stalls when removing large amounts of memory

[Alastair D'Silva]

> -----Original Message-----
> From: Michal Hocko <mhocko@kernel.org>
> Sent: Monday, 17 June 2019 5:47 PM
> To: Alastair D'Silva <alastair@au1.ibm.com>
> Cc: alastair@d-silva.org; Arun KS <arunks@codeaurora.org>; Mukesh Ojha
> <mojha@codeaurora.org>; Logan Gunthorpe <logang@deltatee.com>; Wei
> Yang <richard.weiyang@gmail.com>; Peter Zijlstra <peterz@infradead.org>;
> Ingo Molnar <mingo@kernel.org>; linux-mm@kvack.org; Qian Cai
> <cai@lca.pw>; Thomas Gleixner <tglx@linutronix.de>; Andrew Morton
> <akpm@linux-foundation.org>; Mike Rapoport <rppt@linux.vnet.ibm.com>;
> Baoquan He <bhe@redhat.com>; David Hildenbrand <david@redhat.com>;
> Josh Poimboeuf <jpoimboe@redhat.com>; Pavel Tatashin
> <pasha.tatashin@soleen.com>; Juergen Gross <jgross@suse.com>; Oscar
> Salvador <osalvador@suse.com>; Jiri Kosina <jkosina@suse.cz>; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 4/5] mm/hotplug: Avoid RCU stalls when removing large
> amounts of memory
> 
> On Mon 17-06-19 14:36:30,  Alastair D'Silva  wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > When removing sufficiently large amounts of memory, we trigger RCU
> > stall detection. By periodically calling cond_resched(), we avoid
> > bogus stall warnings.
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> >  mm/memory_hotplug.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index
> > e096c987d261..382b3a0c9333 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -578,6 +578,9 @@ void __remove_pages(struct zone *zone, unsigned
> long phys_start_pfn,
> >  		__remove_section(zone, __pfn_to_section(pfn),
> map_offset,
> >  				 altmap);
> >  		map_offset = 0;
> > +
> > +		if (!(i & 0x0FFF))
> > +			cond_resched();
> 
> We already do have cond_resched before __remove_section. Why is an
> additional needed?

I was getting stalls when removing ~1TB of memory.


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

Re: [PATCH 4/5] mm/hotplug: Avoid RCU stalls when removing large amounts of memory

[Michal Hocko]

On Mon 17-06-19 17:57:16, Alastair D'Silva wrote:
> > -----Original Message-----
> > From: Michal Hocko <mhocko@kernel.org>
> > Sent: Monday, 17 June 2019 5:47 PM
> > To: Alastair D'Silva <alastair@au1.ibm.com>
> > Cc: alastair@d-silva.org; Arun KS <arunks@codeaurora.org>; Mukesh Ojha
> > <mojha@codeaurora.org>; Logan Gunthorpe <logang@deltatee.com>; Wei
> > Yang <richard.weiyang@gmail.com>; Peter Zijlstra <peterz@infradead.org>;
> > Ingo Molnar <mingo@kernel.org>; linux-mm@kvack.org; Qian Cai
> > <cai@lca.pw>; Thomas Gleixner <tglx@linutronix.de>; Andrew Morton
> > <akpm@linux-foundation.org>; Mike Rapoport <rppt@linux.vnet.ibm.com>;
> > Baoquan He <bhe@redhat.com>; David Hildenbrand <david@redhat.com>;
> > Josh Poimboeuf <jpoimboe@redhat.com>; Pavel Tatashin
> > <pasha.tatashin@soleen.com>; Juergen Gross <jgross@suse.com>; Oscar
> > Salvador <osalvador@suse.com>; Jiri Kosina <jkosina@suse.cz>; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH 4/5] mm/hotplug: Avoid RCU stalls when removing large
> > amounts of memory
> > 
> > On Mon 17-06-19 14:36:30,  Alastair D'Silva  wrote:
> > > From: Alastair D'Silva <alastair@d-silva.org>
> > >
> > > When removing sufficiently large amounts of memory, we trigger RCU
> > > stall detection. By periodically calling cond_resched(), we avoid
> > > bogus stall warnings.
> > >
> > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > > ---
> > >  mm/memory_hotplug.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index
> > > e096c987d261..382b3a0c9333 100644
> > > --- a/mm/memory_hotplug.c
> > > +++ b/mm/memory_hotplug.c
> > > @@ -578,6 +578,9 @@ void __remove_pages(struct zone *zone, unsigned
> > long phys_start_pfn,
> > >  		__remove_section(zone, __pfn_to_section(pfn),
> > map_offset,
> > >  				 altmap);
> > >  		map_offset = 0;
> > > +
> > > +		if (!(i & 0x0FFF))
> > > +			cond_resched();
> > 
> > We already do have cond_resched before __remove_section. Why is an
> > additional needed?
> 
> I was getting stalls when removing ~1TB of memory.

Have debugged what is the source of the stall? We do cond_resched once a
memory section which should be a constant unit of work regardless of the
total amount of memory to be removed.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/5] mm/hotplug: Avoid RCU stalls when removing large amounts of memory

[Oscar Salvador]

On Mon, Jun 17, 2019 at 05:57:16PM +1000, Alastair D'Silva wrote:
> I was getting stalls when removing ~1TB of memory.

Would you mind sharing one of those stalls-splats?
I am bit spectic here because as I Michal pointed out, we do cond_resched
once per section removed.

-- 
Oscar Salvador
SUSE L3

[PATCH 5/5] mm/hotplug: export try_online_node

[Alastair D'Silva]

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

If an external driver module supplies physical memory and needs to expose
the memory on a specific NUMA node, it needs to be able to call
try_online_node to allocate the data structures for the node.

The previous assertion that all callers want to online the node, and that
the provided memory address starts at 0 is no longer true, so these
parameters must alse be exposed.

Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
 include/linux/memory_hotplug.h |  4 ++--
 kernel/cpu.c                   |  2 +-
 mm/memory_hotplug.c            | 20 +++++++++++++++-----
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index ae892eef8b82..9272e7955541 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -109,7 +109,7 @@ extern void __online_page_set_limits(struct page *page);
 extern void __online_page_increment_counters(struct page *page);
 extern void __online_page_free(struct page *page);
 
-extern int try_online_node(int nid);
+int try_online_node(int nid, u64 start, bool set_node_online);
 
 extern int arch_add_memory(int nid, u64 start, u64 size,
 			struct mhp_restrictions *restrictions);
@@ -274,7 +274,7 @@ static inline void register_page_bootmem_info_node(struct pglist_data *pgdat)
 {
 }
 
-static inline int try_online_node(int nid)
+static inline int try_online_node(int nid, u64 start, bool set_node_online)
 {
 	return 0;
 }
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 077fde6fb953..ffe5f7239a5c 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1167,7 +1167,7 @@ static int do_cpu_up(unsigned int cpu, enum cpuhp_state target)
 		return -EINVAL;
 	}
 
-	err = try_online_node(cpu_to_node(cpu));
+	err = try_online_node(cpu_to_node(cpu), 0, true);
 	if (err)
 		return err;
 
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 382b3a0c9333..9c2784f89e60 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1004,7 +1004,7 @@ static void rollback_node_hotadd(int nid)
 
 
 /**
- * try_online_node - online a node if offlined
+ * __try_online_node - online a node if offlined
  * @nid: the node ID
  * @start: start addr of the node
  * @set_node_online: Whether we want to online the node
@@ -1039,18 +1039,28 @@ static int __try_online_node(int nid, u64 start, bool set_node_online)
 	return ret;
 }
 
-/*
- * Users of this function always want to online/register the node
+/**
+ * try_online_node - online a node if offlined
+ * @nid: the node ID
+ * @start: start addr of the node
+ * @set_node_online: Whether we want to online the node
+ * called by cpu_up() to online a node without onlined memory.
+ *
+ * Returns:
+ * 1 -> a new node has been allocated
+ * 0 -> the node is already online
+ * -ENOMEM -> the node could not be allocated
  */
-int try_online_node(int nid)
+int try_online_node(int nid, u64 start, bool set_node_online)
 {
 	int ret;
 
 	mem_hotplug_begin();
-	ret =  __try_online_node(nid, 0, true);
+	ret =  __try_online_node(nid, start, set_node_online);
 	mem_hotplug_done();
 	return ret;
 }
+EXPORT_SYMBOL_GPL(try_online_node);
 
 static int check_hotplug_memory_range(u64 start, u64 size)
 {
-- 
2.21.0

Re: [PATCH 5/5] mm/hotplug: export try_online_node

[Peter Zijlstra]

On Mon, Jun 17, 2019 at 02:36:31PM +1000, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> If an external driver module supplies physical memory and needs to expose

Why would you ever want to allow a module to do such a thing?

Re: [PATCH 5/5] mm/hotplug: export try_online_node

[Alastair D'Silva]

On Mon, 2019-06-17 at 08:59 +0200, Peter Zijlstra wrote:
> On Mon, Jun 17, 2019 at 02:36:31PM +1000, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> > 
> > If an external driver module supplies physical memory and needs to
> > expose
> 
> Why would you ever want to allow a module to do such a thing?
> 

I'm working on a driver for Storage Class Memory, connected via an
OpenCAPI link.

The memory is only usable once the card says it's OK to access it.

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

Re: [PATCH 5/5] mm/hotplug: export try_online_node

[Christoph Hellwig]

On Mon, Jun 17, 2019 at 05:05:30PM +1000, Alastair D'Silva wrote:
> On Mon, 2019-06-17 at 08:59 +0200, Peter Zijlstra wrote:
> > On Mon, Jun 17, 2019 at 02:36:31PM +1000, Alastair D'Silva wrote:
> > > From: Alastair D'Silva <alastair@d-silva.org>
> > > 
> > > If an external driver module supplies physical memory and needs to
> > > expose
> > 
> > Why would you ever want to allow a module to do such a thing?
> > 
> 
> I'm working on a driver for Storage Class Memory, connected via an
> OpenCAPI link.
> 
> The memory is only usable once the card says it's OK to access it.

And all that should go through our pmem APIs, not not directly
poke into mm internals.  And if you still need core patches send them
along with the actual driver.

RE: [PATCH 5/5] mm/hotplug: export try_online_node

[Alastair D'Silva]

> -----Original Message-----
> From: Christoph Hellwig <hch@infradead.org>
> Sent: Monday, 17 June 2019 5:15 PM
> To: Alastair D'Silva <alastair@d-silva.org>
> Cc: Peter Zijlstra <peterz@infradead.org>; Andrew Morton <akpm@linux-
> foundation.org>; David Hildenbrand <david@redhat.com>; Oscar Salvador
> <osalvador@suse.com>; Michal Hocko <mhocko@suse.com>; Pavel Tatashin
> <pasha.tatashin@soleen.com>; Wei Yang <richard.weiyang@gmail.com>;
> Arun KS <arunks@codeaurora.org>; Qian Cai <cai@lca.pw>; Thomas Gleixner
> <tglx@linutronix.de>; Ingo Molnar <mingo@kernel.org>; Josh Poimboeuf
> <jpoimboe@redhat.com>; Jiri Kosina <jkosina@suse.cz>; Mukesh Ojha
> <mojha@codeaurora.org>; Mike Rapoport <rppt@linux.vnet.ibm.com>;
> Baoquan He <bhe@redhat.com>; Logan Gunthorpe
> <logang@deltatee.com>; linux-mm@kvack.org; linux-
> kernel@vger.kernel.org; linux-nvdimm@lists.01.org
> Subject: Re: [PATCH 5/5] mm/hotplug: export try_online_node
> 
> On Mon, Jun 17, 2019 at 05:05:30PM +1000, Alastair D'Silva wrote:
> > On Mon, 2019-06-17 at 08:59 +0200, Peter Zijlstra wrote:
> > > On Mon, Jun 17, 2019 at 02:36:31PM +1000, Alastair D'Silva wrote:
> > > > From: Alastair D'Silva <alastair@d-silva.org>
> > > >
> > > > If an external driver module supplies physical memory and needs to
> > > > expose
> > >
> > > Why would you ever want to allow a module to do such a thing?
> > >
> >
> > I'm working on a driver for Storage Class Memory, connected via an
> > OpenCAPI link.
> >
> > The memory is only usable once the card says it's OK to access it.
> 
> And all that should go through our pmem APIs, not not directly poke into
mm
> internals.  And if you still need core patches send them along with the
actual
> driver.

I tried that, but I was getting crashes as the NUMA data structures for that
node were not initialised.

Calling this was required to prevent uninitialized accesses in the pmem
library.

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

Re: [PATCH 5/5] mm/hotplug: export try_online_node

['Christoph Hellwig']

On Mon, Jun 17, 2019 at 06:00:00PM +1000, Alastair D'Silva wrote:
> > And all that should go through our pmem APIs, not not directly poke into
> mm
> > internals.  And if you still need core patches send them along with the
> actual
> > driver.
> 
> I tried that, but I was getting crashes as the NUMA data structures for that
> node were not initialised.
> 
> Calling this was required to prevent uninitialized accesses in the pmem
> library.

Please send your driver to the linux-nvdimm and linux-mm lists so that
it can be carefully reviewed.

Re: [PATCH 5/5] mm/hotplug: export try_online_node

[Michal Hocko]

[Cc Jerome - email thread starts
http://lkml.kernel.org/r/20190617043635.13201-1-alastair@au1.ibm.com]

On Mon 17-06-19 17:05:30,  Alastair D'Silva  wrote:
> On Mon, 2019-06-17 at 08:59 +0200, Peter Zijlstra wrote:
> > On Mon, Jun 17, 2019 at 02:36:31PM +1000, Alastair D'Silva wrote:
> > > From: Alastair D'Silva <alastair@d-silva.org>
> > > 
> > > If an external driver module supplies physical memory and needs to
> > > expose
> > 
> > Why would you ever want to allow a module to do such a thing?
> > 
> 
> I'm working on a driver for Storage Class Memory, connected via an
> OpenCAPI link.
> 
> The memory is only usable once the card says it's OK to access it.

Isn't this what HMM is aiming for? Could you give a more precise
description of what the actual storage is, how it is going to be used
etc... In other words describe the usecase?

-- 
Michal Hocko
SUSE Labs