On Thu, Jun 6, 2019 at 10:21 AM Oscar Salvador <osalvador@xxxxxxx> 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 <mhocko@xxxxxxxx>
> > Cc: Vlastimil Babka <vbabka@xxxxxxx>
> > Cc: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> > Cc: Oscar Salvador <osalvador@xxxxxxx>
> > Reviewed-by: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > ---
> > 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 <osalvador@xxxxxxx>
>
> --
> Oscar Salvador
> SUSE L3
