Re: [PATCH v9 07/12] mm/sparsemem: Prepare for sub-section ranges

2019-06-14 Thread David Hildenbrand 
On 05.06.19 23:58, Dan Williams wrote:
> Prepare the memory hot-{add,remove} paths for handling sub-section
> ranges by plumbing the starting page frame and number of pages being
> handled through arch_{add,remove}_memory() to
> sparse_{add,remove}_one_section().
> 
> This is simply plumbing, small cleanups, and some identifier renames. No
> intended functional changes.
> 
> Cc: Michal Hocko 
> Cc: Vlastimil Babka 
> Cc: Logan Gunthorpe 
> Cc: Oscar Salvador 
> Reviewed-by: Pavel Tatashin 
> Signed-off-by: Dan Williams 
> ---
>  include/linux/memory_hotplug.h |5 +-
>  mm/memory_hotplug.c|  114 
> +---
>  mm/sparse.c|   15 ++---
>  3 files changed, 81 insertions(+), 53 deletions(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 79e0add6a597..3ab0282b4fe5 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -348,9 +348,10 @@ extern int add_memory_resource(int nid, struct resource 
> *resource);
>  extern void move_pfn_range_to_zone(struct zone *zone, unsigned long 
> start_pfn,
>   unsigned long nr_pages, struct vmem_altmap *altmap);
>  extern bool is_memblock_offlined(struct memory_block *mem);
> -extern int sparse_add_one_section(int nid, unsigned long start_pfn,
> -   struct vmem_altmap *altmap);
> +extern int sparse_add_section(int nid, unsigned long pfn,
> + unsigned long nr_pages, struct vmem_altmap *altmap);
>  extern void sparse_remove_one_section(struct mem_section *ms,
> + unsigned long pfn, unsigned long nr_pages,
>   unsigned long map_offset, struct vmem_altmap *altmap);
>  extern struct page *sparse_decode_mem_map(unsigned long coded_mem_map,
> unsigned long pnum);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 4b882c57781a..399bf78bccc5 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -252,51 +252,84 @@ void __init register_page_bootmem_info_node(struct 
> pglist_data *pgdat)
>  }
>  #endif /* CONFIG_HAVE_BOOTMEM_INFO_NODE */
>  
> -static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
> -struct vmem_altmap *altmap)
> +static int __meminit __add_section(int nid, unsigned long pfn,
> + unsigned long nr_pages, struct vmem_altmap *altmap)
>  {
>   int ret;
>  
> - if (pfn_valid(phys_start_pfn))
> + if (pfn_valid(pfn))
>   return -EEXIST;
>  
> - ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
> + ret = sparse_add_section(nid, pfn, nr_pages, altmap);
>   return ret < 0 ? ret : 0;
>  }
>  
> +static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
> + const char *reason)
> +{
> + /*
> +  * Disallow all operations smaller than a sub-section and only
> +  * allow operations smaller than a section for
> +  * SPARSEMEM_VMEMMAP. Note that check_hotplug_memory_range()
> +  * enforces a larger memory_block_size_bytes() granularity for
> +  * memory that will be marked online, so this check should only
> +  * fire for direct arch_{add,remove}_memory() users outside of
> +  * add_memory_resource().
> +  */
> + unsigned long min_align;
> +
> + if (IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP))
> + min_align = PAGES_PER_SUBSECTION;
> + else
> + min_align = PAGES_PER_SECTION;
> + if (!IS_ALIGNED(pfn, min_align)
> + || !IS_ALIGNED(nr_pages, min_align)) {
> + WARN(1, "Misaligned __%s_pages start: %#lx end: #%lx\n",
> + reason, pfn, pfn + nr_pages - 1);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
>  /*
>   * Reasonably generic function for adding memory.  It is
>   * expected that archs that support memory hotplug will
>   * call this function after deciding the zone to which to
>   * add the new pages.
>   */
> -int __ref __add_pages(int nid, unsigned long phys_start_pfn,
> - unsigned long nr_pages, struct mhp_restrictions *restrictions)
> +int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> + struct mhp_restrictions *restrictions)
>  {
>   unsigned long i;
> - int err = 0;
> - int start_sec, end_sec;
> + int start_sec, end_sec, err;
>   struct vmem_altmap *altmap = restrictions->altmap;
>  
> - /* during initialize mem_map, align hot-added range to section */
> - start_sec = pfn_to_section_nr(phys_start_pfn);
> - end_sec = pfn_to_section_nr(phys_start_pfn + nr_pages - 1);
> -
>   if (altmap) {
>   /*
>* Validate altmap is within bounds of the total request
>*/
> - if (altmap->base_pfn != phys_start_pfn
> + if (altmap->base_pfn != pfn
>   ||

Re: [PATCH v9 07/12] mm/sparsemem: Prepare for sub-section ranges

2019-06-06 Thread Dan Williams 
On Thu, Jun 6, 2019 at 10:21 AM Oscar Salvador  wrote:
>
> On Wed, Jun 05, 2019 at 02:58:37PM -0700, Dan Williams wrote:
> > Prepare the memory hot-{add,remove} paths for handling sub-section
> > ranges by plumbing the starting page frame and number of pages being
> > handled through arch_{add,remove}_memory() to
> > sparse_{add,remove}_one_section().
> >
> > This is simply plumbing, small cleanups, and some identifier renames. No
> > intended functional changes.
> >
> > Cc: Michal Hocko 
> > Cc: Vlastimil Babka 
> > Cc: Logan Gunthorpe 
> > Cc: Oscar Salvador 
> > Reviewed-by: Pavel Tatashin 
> > Signed-off-by: Dan Williams 
> > ---
> >  include/linux/memory_hotplug.h |5 +-
> >  mm/memory_hotplug.c|  114 
> > +---
> >  mm/sparse.c|   15 ++---
> >  3 files changed, 81 insertions(+), 53 deletions(-)
> >
> > diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> > index 79e0add6a597..3ab0282b4fe5 100644
> > --- a/include/linux/memory_hotplug.h
> > +++ b/include/linux/memory_hotplug.h
> > @@ -348,9 +348,10 @@ extern int add_memory_resource(int nid, struct 
> > resource *resource);
> >  extern void move_pfn_range_to_zone(struct zone *zone, unsigned long 
> > start_pfn,
> >   unsigned long nr_pages, struct vmem_altmap *altmap);
> >  extern bool is_memblock_offlined(struct memory_block *mem);
> > -extern int sparse_add_one_section(int nid, unsigned long start_pfn,
> > -   struct vmem_altmap *altmap);
> > +extern int sparse_add_section(int nid, unsigned long pfn,
> > + unsigned long nr_pages, struct vmem_altmap *altmap);
> >  extern void sparse_remove_one_section(struct mem_section *ms,
> > + unsigned long pfn, unsigned long nr_pages,
> >   unsigned long map_offset, struct vmem_altmap *altmap);
> >  extern struct page *sparse_decode_mem_map(unsigned long coded_mem_map,
> > unsigned long pnum);
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index 4b882c57781a..399bf78bccc5 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -252,51 +252,84 @@ void __init register_page_bootmem_info_node(struct 
> > pglist_data *pgdat)
> >  }
> >  #endif /* CONFIG_HAVE_BOOTMEM_INFO_NODE */
> >
> > -static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
> > -struct vmem_altmap *altmap)
> > +static int __meminit __add_section(int nid, unsigned long pfn,
> > + unsigned long nr_pages, struct vmem_altmap *altmap)
> >  {
> >   int ret;
> >
> > - if (pfn_valid(phys_start_pfn))
> > + if (pfn_valid(pfn))
> >   return -EEXIST;
> >
> > - ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
> > + ret = sparse_add_section(nid, pfn, nr_pages, altmap);
> >   return ret < 0 ? ret : 0;
> >  }
> >
> > +static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
> > + const char *reason)
> > +{
> > + /*
> > +  * Disallow all operations smaller than a sub-section and only
> > +  * allow operations smaller than a section for
> > +  * SPARSEMEM_VMEMMAP. Note that check_hotplug_memory_range()
> > +  * enforces a larger memory_block_size_bytes() granularity for
> > +  * memory that will be marked online, so this check should only
> > +  * fire for direct arch_{add,remove}_memory() users outside of
> > +  * add_memory_resource().
> > +  */
> > + unsigned long min_align;
> > +
> > + if (IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP))
> > + min_align = PAGES_PER_SUBSECTION;
> > + else
> > + min_align = PAGES_PER_SECTION;
> > + if (!IS_ALIGNED(pfn, min_align)
> > + || !IS_ALIGNED(nr_pages, min_align)) {
> > + WARN(1, "Misaligned __%s_pages start: %#lx end: #%lx\n",
> > + reason, pfn, pfn + nr_pages - 1);
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
>
>
> This caught my eye.
> Back in patch#4 "Convert kmalloc_section_memmap() to 
> populate_section_memmap()",
> you placed a mis-usage check for !CONFIG_SPARSEMEM_VMEMMAP in
> populate_section_memmap().
>
> populate_section_memmap() gets called from sparse_add_one_section(), which 
> means
> that we should have passed this check, otherwise we cannot go further and call
> __add_section().
>
> So, unless I am missing something it seems to me that the check from patch#4 
> could go?
> And I think the same applies to depopulate_section_memmap()?

Yes, good catch, I can kill those extra checks in favor of this one.

> Besides that, it looks good to me:

Thanks Oscar!

>
> Reviewed-by: Oscar Salvador 
>
> --
> Oscar Salvador
> SUSE L3

Re: [PATCH v9 07/12] mm/sparsemem: Prepare for sub-section ranges

2019-06-06 Thread Oscar Salvador 
On Wed, Jun 05, 2019 at 02:58:37PM -0700, Dan Williams wrote:
> Prepare the memory hot-{add,remove} paths for handling sub-section
> ranges by plumbing the starting page frame and number of pages being
> handled through arch_{add,remove}_memory() to
> sparse_{add,remove}_one_section().
> 
> This is simply plumbing, small cleanups, and some identifier renames. No
> intended functional changes.
> 
> Cc: Michal Hocko 
> Cc: Vlastimil Babka 
> Cc: Logan Gunthorpe 
> Cc: Oscar Salvador 
> Reviewed-by: Pavel Tatashin 
> Signed-off-by: Dan Williams 
> ---
>  include/linux/memory_hotplug.h |5 +-
>  mm/memory_hotplug.c|  114 
> +---
>  mm/sparse.c|   15 ++---
>  3 files changed, 81 insertions(+), 53 deletions(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 79e0add6a597..3ab0282b4fe5 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -348,9 +348,10 @@ extern int add_memory_resource(int nid, struct resource 
> *resource);
>  extern void move_pfn_range_to_zone(struct zone *zone, unsigned long 
> start_pfn,
>   unsigned long nr_pages, struct vmem_altmap *altmap);
>  extern bool is_memblock_offlined(struct memory_block *mem);
> -extern int sparse_add_one_section(int nid, unsigned long start_pfn,
> -   struct vmem_altmap *altmap);
> +extern int sparse_add_section(int nid, unsigned long pfn,
> + unsigned long nr_pages, struct vmem_altmap *altmap);
>  extern void sparse_remove_one_section(struct mem_section *ms,
> + unsigned long pfn, unsigned long nr_pages,
>   unsigned long map_offset, struct vmem_altmap *altmap);
>  extern struct page *sparse_decode_mem_map(unsigned long coded_mem_map,
> unsigned long pnum);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 4b882c57781a..399bf78bccc5 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -252,51 +252,84 @@ void __init register_page_bootmem_info_node(struct 
> pglist_data *pgdat)
>  }
>  #endif /* CONFIG_HAVE_BOOTMEM_INFO_NODE */
>  
> -static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
> -struct vmem_altmap *altmap)
> +static int __meminit __add_section(int nid, unsigned long pfn,
> + unsigned long nr_pages, struct vmem_altmap *altmap)
>  {
>   int ret;
>  
> - if (pfn_valid(phys_start_pfn))
> + if (pfn_valid(pfn))
>   return -EEXIST;
>  
> - ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
> + ret = sparse_add_section(nid, pfn, nr_pages, altmap);
>   return ret < 0 ? ret : 0;
>  }
>  
> +static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
> + const char *reason)
> +{
> + /*
> +  * Disallow all operations smaller than a sub-section and only
> +  * allow operations smaller than a section for
> +  * SPARSEMEM_VMEMMAP. Note that check_hotplug_memory_range()
> +  * enforces a larger memory_block_size_bytes() granularity for
> +  * memory that will be marked online, so this check should only
> +  * fire for direct arch_{add,remove}_memory() users outside of
> +  * add_memory_resource().
> +  */
> + unsigned long min_align;
> +
> + if (IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP))
> + min_align = PAGES_PER_SUBSECTION;
> + else
> + min_align = PAGES_PER_SECTION;
> + if (!IS_ALIGNED(pfn, min_align)
> + || !IS_ALIGNED(nr_pages, min_align)) {
> + WARN(1, "Misaligned __%s_pages start: %#lx end: #%lx\n",
> + reason, pfn, pfn + nr_pages - 1);
> + return -EINVAL;
> + }
> + return 0;
> +}


This caught my eye.
Back in patch#4 "Convert kmalloc_section_memmap() to populate_section_memmap()",
you placed a mis-usage check for !CONFIG_SPARSEMEM_VMEMMAP in
populate_section_memmap().

populate_section_memmap() gets called from sparse_add_one_section(), which means
that we should have passed this check, otherwise we cannot go further and call
__add_section().

So, unless I am missing something it seems to me that the check from patch#4 
could go?
And I think the same applies to depopulate_section_memmap()?

Besides that, it looks good to me:

Reviewed-by: Oscar Salvador 

-- 
Oscar Salvador
SUSE L3

[PATCH v9 07/12] mm/sparsemem: Prepare for sub-section ranges

2019-06-05 Thread Dan Williams 
Prepare the memory hot-{add,remove} paths for handling sub-section
ranges by plumbing the starting page frame and number of pages being
handled through arch_{add,remove}_memory() to
sparse_{add,remove}_one_section().

This is simply plumbing, small cleanups, and some identifier renames. No
intended functional changes.

Cc: Michal Hocko 
Cc: Vlastimil Babka 
Cc: Logan Gunthorpe 
Cc: Oscar Salvador 
Reviewed-by: Pavel Tatashin 
Signed-off-by: Dan Williams 
---
 include/linux/memory_hotplug.h |5 +-
 mm/memory_hotplug.c|  114 +---
 mm/sparse.c|   15 ++---
 3 files changed, 81 insertions(+), 53 deletions(-)

diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 79e0add6a597..3ab0282b4fe5 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -348,9 +348,10 @@ extern int add_memory_resource(int nid, struct resource 
*resource);
 extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
unsigned long nr_pages, struct vmem_altmap *altmap);
 extern bool is_memblock_offlined(struct memory_block *mem);
-extern int sparse_add_one_section(int nid, unsigned long start_pfn,
- struct vmem_altmap *altmap);
+extern int sparse_add_section(int nid, unsigned long pfn,
+   unsigned long nr_pages, struct vmem_altmap *altmap);
 extern void sparse_remove_one_section(struct mem_section *ms,
+   unsigned long pfn, unsigned long nr_pages,
unsigned long map_offset, struct vmem_altmap *altmap);
 extern struct page *sparse_decode_mem_map(unsigned long coded_mem_map,
  unsigned long pnum);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 4b882c57781a..399bf78bccc5 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -252,51 +252,84 @@ void __init register_page_bootmem_info_node(struct 
pglist_data *pgdat)
 }
 #endif /* CONFIG_HAVE_BOOTMEM_INFO_NODE */
 
-static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
-  struct vmem_altmap *altmap)
+static int __meminit __add_section(int nid, unsigned long pfn,
+   unsigned long nr_pages, struct vmem_altmap *altmap)
 {
int ret;
 
-   if (pfn_valid(phys_start_pfn))
+   if (pfn_valid(pfn))
return -EEXIST;
 
-   ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
+   ret = sparse_add_section(nid, pfn, nr_pages, altmap);
return ret < 0 ? ret : 0;
 }
 
+static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
+   const char *reason)
+{
+   /*
+* Disallow all operations smaller than a sub-section and only
+* allow operations smaller than a section for
+* SPARSEMEM_VMEMMAP. Note that check_hotplug_memory_range()
+* enforces a larger memory_block_size_bytes() granularity for
+* memory that will be marked online, so this check should only
+* fire for direct arch_{add,remove}_memory() users outside of
+* add_memory_resource().
+*/
+   unsigned long min_align;
+
+   if (IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP))
+   min_align = PAGES_PER_SUBSECTION;
+   else
+   min_align = PAGES_PER_SECTION;
+   if (!IS_ALIGNED(pfn, min_align)
+   || !IS_ALIGNED(nr_pages, min_align)) {
+   WARN(1, "Misaligned __%s_pages start: %#lx end: #%lx\n",
+   reason, pfn, pfn + nr_pages - 1);
+   return -EINVAL;
+   }
+   return 0;
+}
+
 /*
  * Reasonably generic function for adding memory.  It is
  * expected that archs that support memory hotplug will
  * call this function after deciding the zone to which to
  * add the new pages.
  */
-int __ref __add_pages(int nid, unsigned long phys_start_pfn,
-   unsigned long nr_pages, struct mhp_restrictions *restrictions)
+int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
+   struct mhp_restrictions *restrictions)
 {
unsigned long i;
-   int err = 0;
-   int start_sec, end_sec;
+   int start_sec, end_sec, err;
struct vmem_altmap *altmap = restrictions->altmap;
 
-   /* during initialize mem_map, align hot-added range to section */
-   start_sec = pfn_to_section_nr(phys_start_pfn);
-   end_sec = pfn_to_section_nr(phys_start_pfn + nr_pages - 1);
-
if (altmap) {
/*
 * Validate altmap is within bounds of the total request
 */
-   if (altmap->base_pfn != phys_start_pfn
+   if (altmap->base_pfn != pfn
|| vmem_altmap_offset(altmap) > nr_pages) {
pr_warn_once("memory add fail, invalid altmap\n");
-   err = -EINVAL;
-   goto