Date:   Thu, 4 Apr 2019 17:02:45 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Oscar Salvador <osalvador@...e.de>, akpm@...ux-foundation.org
Cc:     mhocko@...e.com, dan.j.williams@...el.com,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH 2/2] mm, memory_hotplug: provide a more generic
 restrictions for memory hotplug

On 04.04.19 16:57, David Hildenbrand wrote:
> On 04.04.19 14:59, Oscar Salvador wrote:
>> From: Michal Hocko <mhocko@...e.com>
>>
>> arch_add_memory, __add_pages take a want_memblock which controls whether
>> the newly added memory should get the sysfs memblock user API (e.g.
>> ZONE_DEVICE users do not want/need this interface). Some callers even
>> want to control where do we allocate the memmap from by configuring
>> altmap.
>>
>> Add a more generic hotplug context for arch_add_memory and __add_pages.
>> struct mhp_restrictions contains flags which contains additional
>> features to be enabled by the memory hotplug (MHP_MEMBLOCK_API
>> currently) and altmap for alternative memmap allocator.
>>
>> This patch shouldn't introduce any functional change.
>>
>> Signed-off-by: Michal Hocko <mhocko@...e.com>
>> Signed-off-by: Oscar Salvador <osalvador@...e.de>
>> ---
>>  arch/arm64/mm/mmu.c            |  6 +++---
>>  arch/ia64/mm/init.c            |  6 +++---
>>  arch/powerpc/mm/mem.c          |  6 +++---
>>  arch/s390/mm/init.c            |  6 +++---
>>  arch/sh/mm/init.c              |  6 +++---
>>  arch/x86/mm/init_32.c          |  6 +++---
>>  arch/x86/mm/init_64.c          | 10 +++++-----
>>  include/linux/memory_hotplug.h | 29 +++++++++++++++++++++++------
>>  kernel/memremap.c              | 10 +++++++---
>>  mm/memory_hotplug.c            | 10 ++++++----
>>  10 files changed, 59 insertions(+), 36 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index e6acfa7be4c7..db509550329d 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1046,8 +1046,8 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr)
>>  }
>>  
>>  #ifdef CONFIG_MEMORY_HOTPLUG
>> -int arch_add_memory(int nid, u64 start, u64 size, struct vmem_altmap *altmap,
>> -		    bool want_memblock)
>> +int arch_add_memory(int nid, u64 start, u64 size,
>> +			struct mhp_restrictions *restrictions)
> 
> Should the restrictions be marked const?
> 
>>  {
>>  	int flags = 0;
>>  
>> @@ -1058,6 +1058,6 @@ int arch_add_memory(int nid, u64 start, u64 size, struct vmem_altmap *altmap,
>>  			     size, PAGE_KERNEL, pgd_pgtable_alloc, flags);
>>  
>>  	return __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
>> -			   altmap, want_memblock);
>> +							restrictions);
> 
> Again, some strange alignment thingies going on here :)
> 
>>  }
>>  #endif
>> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
>> index e49200e31750..379eb1f9adc9 100644
>> --- a/arch/ia64/mm/init.c
>> +++ b/arch/ia64/mm/init.c
>> @@ -666,14 +666,14 @@ mem_init (void)
>>  }
>>  
>>  #ifdef CONFIG_MEMORY_HOTPLUG
>> -int arch_add_memory(int nid, u64 start, u64 size, struct vmem_altmap *altmap,
>> -		bool want_memblock)
>> +int arch_add_memory(int nid, u64 start, u64 size,
>> +			struct mhp_restrictions *restrictions)
>>  {
>>  	unsigned long start_pfn = start >> PAGE_SHIFT;
>>  	unsigned long nr_pages = size >> PAGE_SHIFT;
>>  	int ret;
>>  
>> -	ret = __add_pages(nid, start_pfn, nr_pages, altmap, want_memblock);
>> +	ret = __add_pages(nid, start_pfn, nr_pages, restrictions);
>>  	if (ret)
>>  		printk("%s: Problem encountered in __add_pages() as ret=%d\n",
>>  		       __func__,  ret);
>> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
>> index 1aa27aac73c5..76deaa8525db 100644
>> --- a/arch/powerpc/mm/mem.c
>> +++ b/arch/powerpc/mm/mem.c
>> @@ -109,8 +109,8 @@ int __weak remove_section_mapping(unsigned long start, unsigned long end)
>>  	return -ENODEV;
>>  }
>>  
>> -int __meminit arch_add_memory(int nid, u64 start, u64 size, struct vmem_altmap *altmap,
>> -		bool want_memblock)
>> +int __meminit arch_add_memory(int nid, u64 start, u64 size,
>> +			struct mhp_restrictions *restrictions)
>>  {
>>  	unsigned long start_pfn = start >> PAGE_SHIFT;
>>  	unsigned long nr_pages = size >> PAGE_SHIFT;
>> @@ -127,7 +127,7 @@ int __meminit arch_add_memory(int nid, u64 start, u64 size, struct vmem_altmap *
>>  	}
>>  	flush_inval_dcache_range(start, start + size);
>>  
>> -	return __add_pages(nid, start_pfn, nr_pages, altmap, want_memblock);
>> +	return __add_pages(nid, start_pfn, nr_pages, restrictions);
>>  }
>>  
>>  #ifdef CONFIG_MEMORY_HOTREMOVE
>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>> index 25e3113091ea..f5db961ad792 100644
>> --- a/arch/s390/mm/init.c
>> +++ b/arch/s390/mm/init.c
>> @@ -216,8 +216,8 @@ device_initcall(s390_cma_mem_init);
>>  
>>  #endif /* CONFIG_CMA */
>>  
>> -int arch_add_memory(int nid, u64 start, u64 size, struct vmem_altmap *altmap,
>> -		bool want_memblock)
>> +int arch_add_memory(int nid, u64 start, u64 size,
>> +		struct mhp_restrictions *restrictions)
>>  {
>>  	unsigned long start_pfn = PFN_DOWN(start);
>>  	unsigned long size_pages = PFN_DOWN(size);
>> @@ -227,7 +227,7 @@ int arch_add_memory(int nid, u64 start, u64 size, struct vmem_altmap *altmap,
>>  	if (rc)
>>  		return rc;
>>  
>> -	rc = __add_pages(nid, start_pfn, size_pages, altmap, want_memblock);
>> +	rc = __add_pages(nid, start_pfn, size_pages, restrictions);
>>  	if (rc)
>>  		vmem_remove_mapping(start, size);
>>  	return rc;
>> diff --git a/arch/sh/mm/init.c b/arch/sh/mm/init.c
>> index 8e004b2f1a6a..168d3a6b9358 100644
>> --- a/arch/sh/mm/init.c
>> +++ b/arch/sh/mm/init.c
>> @@ -404,15 +404,15 @@ void __init mem_init(void)
>>  }
>>  
>>  #ifdef CONFIG_MEMORY_HOTPLUG
>> -int arch_add_memory(int nid, u64 start, u64 size, struct vmem_altmap *altmap,
>> -		bool want_memblock)
>> +int arch_add_memory(int nid, u64 start, u64 size,
>> +			struct mhp_restrictions *restrictions)
>>  {
>>  	unsigned long start_pfn = PFN_DOWN(start);
>>  	unsigned long nr_pages = size >> PAGE_SHIFT;
>>  	int ret;
>>  
>>  	/* We only have ZONE_NORMAL, so this is easy.. */
>> -	ret = __add_pages(nid, start_pfn, nr_pages, altmap, want_memblock);
>> +	ret = __add_pages(nid, start_pfn, nr_pages, restrictions);
>>  	if (unlikely(ret))
>>  		printk("%s: Failed, __add_pages() == %d\n", __func__, ret);
>>  
>> diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
>> index 85c94f9a87f8..755dbed85531 100644
>> --- a/arch/x86/mm/init_32.c
>> +++ b/arch/x86/mm/init_32.c
>> @@ -850,13 +850,13 @@ void __init mem_init(void)
>>  }
>>  
>>  #ifdef CONFIG_MEMORY_HOTPLUG
>> -int arch_add_memory(int nid, u64 start, u64 size, struct vmem_altmap *altmap,
>> -		bool want_memblock)
>> +int arch_add_memory(int nid, u64 start, u64 size,
>> +			struct mhp_restrictions *restrictions)
>>  {
>>  	unsigned long start_pfn = start >> PAGE_SHIFT;
>>  	unsigned long nr_pages = size >> PAGE_SHIFT;
>>  
>> -	return __add_pages(nid, start_pfn, nr_pages, altmap, want_memblock);
>> +	return __add_pages(nid, start_pfn, nr_pages, restrictions);
>>  }
>>  
>>  #ifdef CONFIG_MEMORY_HOTREMOVE
>> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
>> index bccff68e3267..db42c11b48fb 100644
>> --- a/arch/x86/mm/init_64.c
>> +++ b/arch/x86/mm/init_64.c
>> @@ -777,11 +777,11 @@ static void update_end_of_memory_vars(u64 start, u64 size)
>>  }
>>  
>>  int add_pages(int nid, unsigned long start_pfn, unsigned long nr_pages,
>> -		struct vmem_altmap *altmap, bool want_memblock)
>> +				struct mhp_restrictions *restrictions)
>>  {
>>  	int ret;
>>  
>> -	ret = __add_pages(nid, start_pfn, nr_pages, altmap, want_memblock);
>> +	ret = __add_pages(nid, start_pfn, nr_pages, restrictions);
>>  	WARN_ON_ONCE(ret);
>>  
>>  	/* update max_pfn, max_low_pfn and high_memory */
>> @@ -791,15 +791,15 @@ int add_pages(int nid, unsigned long start_pfn, unsigned long nr_pages,
>>  	return ret;
>>  }
>>  
>> -int arch_add_memory(int nid, u64 start, u64 size, struct vmem_altmap *altmap,
>> -		bool want_memblock)
>> +int arch_add_memory(int nid, u64 start, u64 size,
>> +			struct mhp_restrictions *restrictions)
>>  {
>>  	unsigned long start_pfn = start >> PAGE_SHIFT;
>>  	unsigned long nr_pages = size >> PAGE_SHIFT;
>>  
>>  	init_memory_mapping(start, start + size);
>>  
>> -	return add_pages(nid, start_pfn, nr_pages, altmap, want_memblock);
>> +	return add_pages(nid, start_pfn, nr_pages, restrictions);
>>  }
>>  
>>  #define PAGE_INUSE 0xFD
>> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>> index 3c8cf347804c..5bd4b56f639c 100644
>> --- a/include/linux/memory_hotplug.h
>> +++ b/include/linux/memory_hotplug.h
>> @@ -118,20 +118,37 @@ extern int __remove_pages(struct zone *zone, unsigned long start_pfn,
>>  	unsigned long nr_pages, struct vmem_altmap *altmap);
>>  #endif /* CONFIG_MEMORY_HOTREMOVE */
>>  
>> +/*
>> + * Do we want sysfs memblock files created. This will allow userspace to online
>> + * and offline memory explicitly. Lack of this bit means that the caller has to
>> + * call move_pfn_range_to_zone to finish the initialization.
>> + */
> 
> I think you can be more precise here.
> 
> "Create memory block devices for added pages. This is usually the case
> for all system ram (and only system ram), as only this way memory can be
> onlined/offlined by user space and kdump to correctly detect the new
> memory using udev events."
> 
> Maybe we should even go a step further and call this
> 
> MHP_SYSTEM_RAM
> 
> Because that is what it is right now.
> 
>> +
>> +#define MHP_MEMBLOCK_API               (1<<0)
>> +
>> +/*
>> + * Restrictions for the memory hotplug:
>> + * flags:  MHP_ flags
>> + * altmap: alternative allocator for memmap array
>> + */
>> +struct mhp_restrictions {
>> +	unsigned long flags;
>> +	struct vmem_altmap *altmap;
>> +};
>> +
>>  /* reasonably generic interface to expand the physical pages */
>>  extern int __add_pages(int nid, unsigned long start_pfn, unsigned long nr_pages,
>> -		struct vmem_altmap *altmap, bool want_memblock);
>> +					struct mhp_restrictions *restrictions);
>>  
>>  #ifndef CONFIG_ARCH_HAS_ADD_PAGES
>>  static inline int add_pages(int nid, unsigned long start_pfn,
>> -		unsigned long nr_pages, struct vmem_altmap *altmap,
>> -		bool want_memblock)
>> +		unsigned long nr_pages, struct mhp_restrictions *restrictions)
>>  {
>> -	return __add_pages(nid, start_pfn, nr_pages, altmap, want_memblock);
>> +	return __add_pages(nid, start_pfn, nr_pages, restrictions);
>>  }
>>  #else /* ARCH_HAS_ADD_PAGES */
>>  int add_pages(int nid, unsigned long start_pfn, unsigned long nr_pages,
>> -		struct vmem_altmap *altmap, bool want_memblock);
>> +				struct mhp_restrictions *restrictions);
> 
> dito alignment. You have tabs configured to 8 characters, right?
> 
>>  #endif /* ARCH_HAS_ADD_PAGES */
>>  
>>  #ifdef CONFIG_NUMA
>> @@ -333,7 +350,7 @@ extern int __add_memory(int nid, u64 start, u64 size);
>>  extern int add_memory(int nid, u64 start, u64 size);
>>  extern int add_memory_resource(int nid, struct resource *resource);
>>  extern int arch_add_memory(int nid, u64 start, u64 size,
>> -		struct vmem_altmap *altmap, bool want_memblock);
>> +			struct mhp_restrictions *restrictions);
>>  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);
>> diff --git a/kernel/memremap.c b/kernel/memremap.c
>> index a856cb5ff192..cc5e3e34417d 100644
>> --- a/kernel/memremap.c
>> +++ b/kernel/memremap.c
>> @@ -149,6 +149,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>>  	struct resource *res = &pgmap->res;
>>  	struct dev_pagemap *conflict_pgmap;
>>  	pgprot_t pgprot = PAGE_KERNEL;
>> +	struct mhp_restrictions restrictions = {};
>>  	int error, nid, is_ram;
>>  
>>  	if (!pgmap->ref || !pgmap->kill)
>> @@ -199,6 +200,9 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>>  	if (error)
>>  		goto err_pfn_remap;
>>  
>> +	/* We do not want any optional features only our own memmap */
>> +	restrictions.altmap = altmap;
>> +>  	mem_hotplug_begin();
>>  
>>  	/*
>> @@ -214,7 +218,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>>  	 */
>>  	if (pgmap->type == MEMORY_DEVICE_PRIVATE) {
>>  		error = add_pages(nid, align_start >> PAGE_SHIFT,
>> -				align_size >> PAGE_SHIFT, NULL, false);
>> +				align_size >> PAGE_SHIFT, &restrictions);
>>  	} else {
>>  		error = kasan_add_zero_shadow(__va(align_start), align_size);
>>  		if (error) {
>> @@ -222,8 +226,8 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>>  			goto err_kasan;
>>  		}
>>  
>> -		error = arch_add_memory(nid, align_start, align_size, altmap,
>> -				false);
>> +		error = arch_add_memory(nid, align_start, align_size,
>> +							&restrictions);
> 
> dito alignment
> 
>>  	}
>>  
>>  	if (!error) {
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index d8a3e9554aec..50f77e059457 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -274,12 +274,12 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
>>   * add the new pages.
>>   */
>>  int __ref __add_pages(int nid, unsigned long phys_start_pfn,
>> -		unsigned long nr_pages, struct vmem_altmap *altmap,
>> -		bool want_memblock)
>> +		unsigned long nr_pages, struct mhp_restrictions *restrictions)
>>  {
>>  	unsigned long i;
>>  	int err = 0;
>>  	int start_sec, end_sec;
>> +	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);
>> @@ -300,7 +300,7 @@ int __ref __add_pages(int nid, unsigned long phys_start_pfn,
>>  
>>  	for (i = start_sec; i <= end_sec; i++) {
>>  		err = __add_section(nid, section_nr_to_pfn(i), altmap,
>> -				want_memblock);
>> +				restrictions->flags & MHP_MEMBLOCK_API);
>>  
>>  		/*
>>  		 * EEXIST is finally dealt with by ioresource collision
>> @@ -1102,6 +1102,7 @@ int __ref add_memory_resource(int nid, struct resource *res)
>>  	u64 start, size;
>>  	bool new_node = false;
>>  	int ret;
>> +	struct mhp_restrictions restrictions = {};
> 
> I'd make this the very first variable.
> 
> Also eventually
> 
> struct mhp_restrictions restrictions = {
> 	.restrictions = MHP_MEMBLOCK_API
> };
> 
>>  
>>  	start = res->start;
>>  	size = resource_size(res);
>> @@ -1126,7 +1127,8 @@ int __ref add_memory_resource(int nid, struct resource *res)
>>  	new_node = ret;
>>  
>>  	/* call arch's memory hotadd */
>> -	ret = arch_add_memory(nid, start, size, NULL, true);
>> +	restrictions.flags = MHP_MEMBLOCK_API;
>> +	ret = arch_add_memory(nid, start, size, &restrictions);
>>  	if (ret < 0)
>>  		goto error;
>>  
>>
> 
> 

s/alignment/indentation/

I think I should take a nap :)

-- 

Thanks,

David / dhildenb

Powered by blists - more mailing lists