Re: [PATCH v3 3/5] mm,sparse: Add SECTION_USE_VMEMMAP flag

[David Hildenbrand]

On 25.07.19 18:02, Oscar Salvador wrote:
> When hot-removing memory, we need to be careful about two things:
> 
> 1) Memory range must be memory_block aligned. This is what
>    check_hotplug_memory_range() checks for.
> 
> 2) If a range was hot-added using MHP_MEMMAP_ON_MEMORY, we need to check
>    whether the caller is removing memory with the same granularity that
>    it was added.

The second step does only apply to MMAP_ON_MEMORY and is not universally
true.

> 
> So to check against case 2), we mark all sections used by vmemmap
> (not only the ones containing vmemmap pages, but all sections spanning
> the memory range) with SECTION_USE_VMEMMAP.

SECTION_USE_VMEMAP is misleding.

Rather SECTION_MMAP_ON_MEMORY (TBD). Please *really* add a description
(these sections)

> 
> This will allow us to do some sanity checks when in hot-remove stage.
> 

One idea: lookup the struct page of the lowest memory address you are
removing and test if it lies on a PageVmemmap(). Then, from the stored
info along the vmemmap page (start + length) you can test if all memory
the vmemmap is responsible for is removed.

This should work or am I missing something?

> Signed-off-by: Oscar Salvador <osalvador@xxxxxxx>
> ---
>  include/linux/memory_hotplug.h | 3 ++-
>  include/linux/mmzone.h         | 8 +++++++-
>  mm/memory_hotplug.c            | 2 +-
>  mm/sparse.c                    | 9 +++++++--
>  4 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 45dece922d7c..6b20008d9297 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -366,7 +366,8 @@ 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_section(int nid, unsigned long pfn,
> -		unsigned long nr_pages, struct vmem_altmap *altmap);
> +		unsigned long nr_pages, struct vmem_altmap *altmap,
> +		bool vmemmap_section);
>  extern void sparse_remove_section(struct mem_section *ms,
>  		unsigned long pfn, unsigned long nr_pages,
>  		unsigned long map_offset, struct vmem_altmap *altmap);
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index d77d717c620c..259c326962f5 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -1254,7 +1254,8 @@ extern size_t mem_section_usage_size(void);
>  #define SECTION_HAS_MEM_MAP	(1UL<<1)
>  #define SECTION_IS_ONLINE	(1UL<<2)
>  #define SECTION_IS_EARLY	(1UL<<3)
> -#define SECTION_MAP_LAST_BIT	(1UL<<4)
> +#define SECTION_USE_VMEMMAP	(1UL<<4)
> +#define SECTION_MAP_LAST_BIT	(1UL<<5)
>  #define SECTION_MAP_MASK	(~(SECTION_MAP_LAST_BIT-1))
>  #define SECTION_NID_SHIFT	3
>  
> @@ -1265,6 +1266,11 @@ static inline struct page *__section_mem_map_addr(struct mem_section *section)
>  	return (struct page *)map;
>  }
>  
> +static inline int vmemmap_section(struct mem_section *section)
> +{
> +	return (section && (section->section_mem_map & SECTION_USE_VMEMMAP));
> +}
> +
>  static inline int present_section(struct mem_section *section)
>  {
>  	return (section && (section->section_mem_map & SECTION_MARKED_PRESENT));
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 3d97c3711333..c2338703ce80 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -314,7 +314,7 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>  
>  		pfns = min(nr_pages, PAGES_PER_SECTION
>  				- (pfn & ~PAGE_SECTION_MASK));
> -		err = sparse_add_section(nid, pfn, pfns, altmap);
> +		err = sparse_add_section(nid, pfn, pfns, altmap, 0);
>  		if (err)
>  			break;
>  		pfn += pfns;
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 79355a86064f..09cac39e39d9 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -856,13 +856,18 @@ static struct page * __meminit section_activate(int nid, unsigned long pfn,
>   * * -ENOMEM	- Out of memory.
>   */
>  int __meminit sparse_add_section(int nid, unsigned long start_pfn,
> -		unsigned long nr_pages, struct vmem_altmap *altmap)
> +		unsigned long nr_pages, struct vmem_altmap *altmap,
> +		bool vmemmap_section)
>  {
>  	unsigned long section_nr = pfn_to_section_nr(start_pfn);
> +	unsigned long flags = 0;
>  	struct mem_section *ms;
>  	struct page *memmap;
>  	int ret;
>  
> +	if (vmemmap_section)
> +		flags = SECTION_USE_VMEMMAP;
> +
>  	ret = sparse_index_init(section_nr, nid);
>  	if (ret < 0)
>  		return ret;
> @@ -884,7 +889,7 @@ int __meminit sparse_add_section(int nid, unsigned long start_pfn,
>  	/* Align memmap to section boundary in the subsection case */
>  	if (section_nr_to_pfn(section_nr) != start_pfn)
>  		memmap = pfn_to_kaddr(section_nr_to_pfn(section_nr));
> -	sparse_init_one_section(ms, section_nr, memmap, ms->usage, 0);
> +	sparse_init_one_section(ms, section_nr, memmap, ms->usage, flags);
>  
>  	return 0;
>  }
> 


-- 

Thanks,

David / dhildenb