Re: [PATCH v3 4/7] mm/sparse.c: only use subsection map in VMEMMAP case

[David Hildenbrand <david@redhat.com>]

On 07.03.20 09:42, Baoquan He wrote:
> Currently, to support subsection aligned memory region adding for pmem,
> subsection map is added to track which subsection is present.
> 
> However, config ZONE_DEVICE depends on SPARSEMEM_VMEMMAP. It means
> subsection map only makes sense when SPARSEMEM_VMEMMAP enabled. For the
> classic sparse, subsection map is meaningless and confusing.
> 
> About the classic sparse which doesn't support subsection hotplug, Dan
> said it's more because the effort and maintenance burden outweighs the
> benefit. Besides, the current 64 bit ARCHes all enable
> SPARSEMEM_VMEMMAP_ENABLE by default.
> 
> Combining the above reasons, no need to provide subsection map and the
> relevant handling for the classic sparse. Handle it with this patch.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  include/linux/mmzone.h |  2 ++
>  mm/sparse.c            | 25 +++++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 42b77d3b68e8..f3f264826423 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -1143,7 +1143,9 @@ static inline unsigned long section_nr_to_pfn(unsigned long sec)
>  #define SUBSECTION_ALIGN_DOWN(pfn) ((pfn) & PAGE_SUBSECTION_MASK)
>  
>  struct mem_section_usage {
> +#ifdef CONFIG_SPARSEMEM_VMEMMAP
>  	DECLARE_BITMAP(subsection_map, SUBSECTIONS_PER_SECTION);
> +#endif
>  	/* See declaration of similar field in struct zone */
>  	unsigned long pageblock_flags[0];
>  };
> diff --git a/mm/sparse.c b/mm/sparse.c
> index d9dcd58d5c1d..2142045ab5c5 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -209,6 +209,7 @@ static inline unsigned long first_present_section_nr(void)
>  	return next_present_section_nr(-1);
>  }
>  
> +#ifdef CONFIG_SPARSEMEM_VMEMMAP
>  static void subsection_mask_set(unsigned long *map, unsigned long pfn,
>  		unsigned long nr_pages)
>  {
> @@ -243,6 +244,11 @@ void __init subsection_map_init(unsigned long pfn, unsigned long nr_pages)
>  		nr_pages -= pfns;
>  	}
>  }
> +#else
> +void __init subsection_map_init(unsigned long pfn, unsigned long nr_pages)
> +{
> +}
> +#endif
>  
>  /* Record a memory area against a node. */
>  void __init memory_present(int nid, unsigned long start, unsigned long end)
> @@ -726,6 +732,7 @@ static void free_map_bootmem(struct page *memmap)
>  }
>  #endif /* CONFIG_SPARSEMEM_VMEMMAP */
>  
> +#ifdef CONFIG_SPARSEMEM_VMEMMAP
>  static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
>  {
>  	DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 };
> @@ -753,6 +760,17 @@ static bool is_subsection_map_empty(struct mem_section *ms)
>  	return bitmap_empty(&ms->usage->subsection_map[0],
>  			    SUBSECTIONS_PER_SECTION);
>  }
> +#else
> +static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages)
> +{
> +	return 0;
> +}
> +
> +static bool is_subsection_map_empty(struct mem_section *ms)
> +{
> +	return true;
> +}
> +#endif
>  
>  static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
>  		struct vmem_altmap *altmap)
> @@ -809,6 +827,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
>  		ms->section_mem_map = (unsigned long)NULL;
>  }
>  
> +#ifdef CONFIG_SPARSEMEM_VMEMMAP
>  static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
>  {
>  	struct mem_section *ms = __pfn_to_section(pfn);
> @@ -830,6 +849,12 @@ static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
>  
>  	return rc;
>  }
> +#else
> +static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages)
> +{
> +	return 0;
> +}
> +#endif
>  
>  static struct page * __meminit section_activate(int nid, unsigned long pfn,
>  		unsigned long nr_pages, struct vmem_altmap *altmap)
> 

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

-- 
Thanks,

David / dhildenb