[RFC PATCH 0/4] mm/hotplug: make hotplug memory_block alligned

[RFC PATCH 0/4] mm/hotplug: make hotplug memory_block alligned

[Wei Yang]

Michal & all

Previously we found the hotplug range is mem_section aligned instead of
memory_block.

Here is several draft patches to fix that. To make sure I am getting your
point correctly, I post it here before further investigation.

Willing to see your comments. :-)

Wei Yang (4):
  mm/hotplug: aligne the hotplugable range with memory_block
  mm/hotplug: walk_memroy_range on memory_block uit
  mm/hotplug: make __add_pages() iterate on memory_block and split
    __add_section()
  base/memory: pass start_section_nr to init_memory_block()

 drivers/base/memory.c  | 34 ++++++++++++----------------------
 include/linux/memory.h |  4 +++-
 mm/memory_hotplug.c    | 48 +++++++++++++++++++++++++-----------------------
 3 files changed, 40 insertions(+), 46 deletions(-)

-- 
2.11.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC PATCH 1/4] mm/hotplug: aligne the hotplugable range with memory_block

[Wei Yang]

memory hotplug is memory block aligned instead of section aligned.

This patch fix the range check during hotplug.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 drivers/base/memory.c  | 3 ++-
 include/linux/memory.h | 2 ++
 mm/memory_hotplug.c    | 9 +++++----
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index c7c4e0325cdb..b54cfe9cd98b 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -31,7 +31,8 @@ static DEFINE_MUTEX(mem_sysfs_mutex);
 
 #define to_memory_block(dev) container_of(dev, struct memory_block, dev)
 
-static int sections_per_block;
+int sections_per_block;
+EXPORT_SYMBOL(sections_per_block);
 
 static inline int base_memory_block_id(int section_nr)
 {
diff --git a/include/linux/memory.h b/include/linux/memory.h
index b723a686fc10..51a6355aa56d 100644
--- a/include/linux/memory.h
+++ b/include/linux/memory.h
@@ -142,4 +142,6 @@ extern struct memory_block *find_memory_block(struct mem_section *);
  */
 extern struct mutex text_mutex;
 
+extern int sections_per_block;
+
 #endif /* _LINUX_MEMORY_H_ */
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 387ca386142c..f5d06afc8645 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1183,11 +1183,12 @@ static int check_hotplug_memory_range(u64 start, u64 size)
 {
 	u64 start_pfn = PFN_DOWN(start);
 	u64 nr_pages = size >> PAGE_SHIFT;
+	u64 page_per_block = sections_per_block * PAGES_PER_SECTION;
 
-	/* Memory range must be aligned with section */
-	if ((start_pfn & ~PAGE_SECTION_MASK) ||
-	    (nr_pages % PAGES_PER_SECTION) || (!nr_pages)) {
-		pr_err("Section-unaligned hotplug range: start 0x%llx, size 0x%llx\n",
+	/* Memory range must be aligned with memory_block */
+	if ((start_pfn & (page_per_block - 1)) ||
+	    (nr_pages % page_per_block) || (!nr_pages)) {
+		pr_err("Memory_block-unaligned hotplug range: start 0x%llx, size 0x%llx\n",
 				(unsigned long long)start,
 				(unsigned long long)size);
 		return -EINVAL;
-- 
2.11.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC PATCH 2/4] mm/hotplug: walk_memroy_range on memory_block uit

[Wei Yang]

hotplug memory range is memory_block aligned and walk_memroy_range guarded
with check_hotplug_memory_range(). This is save to iterate on the
memory_block base.

This patch adjust the iteration unit and assume there is not hole in
hotplug memory range.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 mm/memory_hotplug.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index f5d06afc8645..a79a83ec965f 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1858,17 +1858,11 @@ int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,
 	unsigned long pfn, section_nr;
 	int ret;
 
-	for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
+	for (pfn = start_pfn; pfn < end_pfn;
+		pfn += PAGES_PER_SECTION * sections_per_block) {
 		section_nr = pfn_to_section_nr(pfn);
-		if (!present_section_nr(section_nr))
-			continue;
 
 		section = __nr_to_section(section_nr);
-		/* same memblock? */
-		if (mem)
-			if ((section_nr >= mem->start_section_nr) &&
-			    (section_nr <= mem->end_section_nr))
-				continue;
 
 		mem = find_memory_block_hinted(section, mem);
 		if (!mem)
-- 
2.11.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC PATCH 3/4] mm/hotplug: make __add_pages() iterate on memory_block and split __add_section()

[Wei Yang]

Memory hotplug unit is memory_block which contains one or several
mem_section. The current logic is iterating on each mem_section and add or
adjust the memory_block every time.

This patch makes the __add_pages() iterate on memory_block and split
__add_section() to two functions: __add_section() and __add_memory_block().

The first one would take care of each section data and the second one would
register the memory_block at once, which makes the function more clear and
natural.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 drivers/base/memory.c | 17 +++++------------
 mm/memory_hotplug.c   | 27 +++++++++++++++++----------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index b54cfe9cd98b..468e5ad1bc87 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -705,19 +705,12 @@ int register_new_memory(int nid, struct mem_section *section)
 
 	mutex_lock(&mem_sysfs_mutex);
 
-	mem = find_memory_block(section);
-	if (mem) {
-		mem->section_count++;
-		put_device(&mem->dev);
-	} else {
-		ret = init_memory_block(&mem, section, MEM_OFFLINE);
-		if (ret)
-			goto out;
-		mem->section_count++;
-	}
+	ret = init_memory_block(&mem, section, MEM_OFFLINE);
+	if (ret)
+		goto out;
+	mem->section_count = sections_per_block;
 
-	if (mem->section_count == sections_per_block)
-		ret = register_mem_sect_under_node(mem, nid);
+	ret = register_mem_sect_under_node(mem, nid);
 out:
 	mutex_unlock(&mem_sysfs_mutex);
 	return ret;
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a79a83ec965f..14a08b980b59 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -302,8 +302,7 @@ 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,
-		bool want_memblock)
+static int __meminit __add_section(int nid, unsigned long phys_start_pfn)
 {
 	int ret;
 	int i;
@@ -332,6 +331,18 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
 		SetPageReserved(page);
 	}
 
+	return 0;
+}
+
+static int __meminit __add_memory_block(int nid, unsigned long phys_start_pfn,
+		bool want_memblock)
+{
+	int ret;
+
+	ret = __add_section(nid, phys_start_pfn);
+	if (ret)
+		return ret;
+
 	if (!want_memblock)
 		return 0;
 
@@ -347,15 +358,10 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
 int __ref __add_pages(int nid, unsigned long phys_start_pfn,
 			unsigned long nr_pages, bool want_memblock)
 {
-	unsigned long i;
+	unsigned long pfn;
 	int err = 0;
-	int start_sec, end_sec;
 	struct vmem_altmap *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);
-
 	altmap = to_vmem_altmap((unsigned long) pfn_to_page(phys_start_pfn));
 	if (altmap) {
 		/*
@@ -370,8 +376,9 @@ int __ref __add_pages(int nid, unsigned long phys_start_pfn,
 		altmap->alloc = 0;
 	}
 
-	for (i = start_sec; i <= end_sec; i++) {
-		err = __add_section(nid, section_nr_to_pfn(i), want_memblock);
+	for (pfn; pfn < phys_start_pfn + nr_pages;
+			pfn += sections_per_block * PAGES_PER_SECTION) {
+		err = __add_memory_block(nid, pfn, want_memblock);
 
 		/*
 		 * EEXIST is finally dealt with by ioresource collision
-- 
2.11.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC PATCH 4/4] base/memory: pass start_section_nr to init_memory_block()

[Wei Yang]

The second parameter of init_memory_block() is to calculate the
start_section_nr for the memory_block. While current implementation dose
some unnecessary transform between mem_sectioni and section_nr.

This patch simplifies the function by just passing the start_section_nr to
it. By doing so, we can also simplify add_memory_block() too.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 drivers/base/memory.c  | 16 ++++++----------
 include/linux/memory.h |  2 +-
 mm/memory_hotplug.c    |  2 +-
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 468e5ad1bc87..43783dbb1d5e 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -645,7 +645,7 @@ int register_memory(struct memory_block *memory)
 }
 
 static int init_memory_block(struct memory_block **memory,
-			     struct mem_section *section, unsigned long state)
+			     int start_section_nr, unsigned long state)
 {
 	struct memory_block *mem;
 	unsigned long start_pfn;
@@ -656,9 +656,7 @@ static int init_memory_block(struct memory_block **memory,
 	if (!mem)
 		return -ENOMEM;
 
-	scn_nr = __section_nr(section);
-	mem->start_section_nr =
-			base_memory_block_id(scn_nr) * sections_per_block;
+	mem->start_section_nr = start_section_nr;
 	mem->end_section_nr = mem->start_section_nr + sections_per_block - 1;
 	mem->state = state;
 	start_pfn = section_nr_to_pfn(mem->start_section_nr);
@@ -673,21 +671,19 @@ static int init_memory_block(struct memory_block **memory,
 static int add_memory_block(int base_section_nr)
 {
 	struct memory_block *mem;
-	int i, ret, section_count = 0, section_nr;
+	int i, ret, section_count = 0;
 
 	for (i = base_section_nr;
 	     (i < base_section_nr + sections_per_block) && i < NR_MEM_SECTIONS;
 	     i++) {
 		if (!present_section_nr(i))
 			continue;
-		if (section_count == 0)
-			section_nr = i;
 		section_count++;
 	}
 
 	if (section_count == 0)
 		return 0;
-	ret = init_memory_block(&mem, __nr_to_section(section_nr), MEM_ONLINE);
+	ret = init_memory_block(&mem, base_section_nr, MEM_ONLINE);
 	if (ret)
 		return ret;
 	mem->section_count = section_count;
@@ -698,14 +694,14 @@ static int add_memory_block(int base_section_nr)
  * need an interface for the VM to add new memory regions,
  * but without onlining it.
  */
-int register_new_memory(int nid, struct mem_section *section)
+int register_new_memory(int nid, int start_section_nr)
 {
 	int ret = 0;
 	struct memory_block *mem;
 
 	mutex_lock(&mem_sysfs_mutex);
 
-	ret = init_memory_block(&mem, section, MEM_OFFLINE);
+	ret = init_memory_block(&mem, start_section_nr, MEM_OFFLINE);
 	if (ret)
 		goto out;
 	mem->section_count = sections_per_block;
diff --git a/include/linux/memory.h b/include/linux/memory.h
index 51a6355aa56d..0cbde14f7cea 100644
--- a/include/linux/memory.h
+++ b/include/linux/memory.h
@@ -108,7 +108,7 @@ extern int register_memory_notifier(struct notifier_block *nb);
 extern void unregister_memory_notifier(struct notifier_block *nb);
 extern int register_memory_isolate_notifier(struct notifier_block *nb);
 extern void unregister_memory_isolate_notifier(struct notifier_block *nb);
-extern int register_new_memory(int, struct mem_section *);
+extern int register_new_memory(int, int);
 #ifdef CONFIG_MEMORY_HOTREMOVE
 extern int unregister_memory_section(struct mem_section *);
 #endif
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 14a08b980b59..fc198847dd5b 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -346,7 +346,7 @@ static int __meminit __add_memory_block(int nid, unsigned long phys_start_pfn,
 	if (!want_memblock)
 		return 0;
 
-	return register_new_memory(nid, __pfn_to_section(phys_start_pfn));
+	return register_new_memory(nid, pfn_to_section_nr(phys_start_pfn));
 }
 
 /*
-- 
2.11.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 1/4] mm/hotplug: aligne the hotplugable range with memory_block

[John Hubbard]

On 06/24/2017 07:52 PM, Wei Yang wrote:
> memory hotplug is memory block aligned instead of section aligned.
> 
> This patch fix the range check during hotplug.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  drivers/base/memory.c  | 3 ++-
>  include/linux/memory.h | 2 ++
>  mm/memory_hotplug.c    | 9 +++++----
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index c7c4e0325cdb..b54cfe9cd98b 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -31,7 +31,8 @@ static DEFINE_MUTEX(mem_sysfs_mutex);
>  
>  #define to_memory_block(dev) container_of(dev, struct memory_block, dev)
>  
> -static int sections_per_block;
> +int sections_per_block;
> +EXPORT_SYMBOL(sections_per_block);

Hi Wei,

Is sections_per_block ever assigned a value? I am not seeing that happen,
either in this patch, or in the larger patchset.


>  
>  static inline int base_memory_block_id(int section_nr)
>  {
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index b723a686fc10..51a6355aa56d 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -142,4 +142,6 @@ extern struct memory_block *find_memory_block(struct mem_section *);
>   */
>  extern struct mutex text_mutex;
>  
> +extern int sections_per_block;
> +
>  #endif /* _LINUX_MEMORY_H_ */
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 387ca386142c..f5d06afc8645 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1183,11 +1183,12 @@ static int check_hotplug_memory_range(u64 start, u64 size)
>  {
>  	u64 start_pfn = PFN_DOWN(start);
>  	u64 nr_pages = size >> PAGE_SHIFT;
> +	u64 page_per_block = sections_per_block * PAGES_PER_SECTION;

"pages_per_block" would be a little better.

Also, in the first line of the commit, s/aligne/align/.

thanks,
john h

>  
> -	/* Memory range must be aligned with section */
> -	if ((start_pfn & ~PAGE_SECTION_MASK) ||
> -	    (nr_pages % PAGES_PER_SECTION) || (!nr_pages)) {
> -		pr_err("Section-unaligned hotplug range: start 0x%llx, size 0x%llx\n",
> +	/* Memory range must be aligned with memory_block */
> +	if ((start_pfn & (page_per_block - 1)) ||
> +	    (nr_pages % page_per_block) || (!nr_pages)) {
> +		pr_err("Memory_block-unaligned hotplug range: start 0x%llx, size 0x%llx\n",
>  				(unsigned long long)start,
>  				(unsigned long long)size);
>  		return -EINVAL;
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 1/4] mm/hotplug: aligne the hotplugable range with memory_block

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]

On Sat, Jun 24, 2017 at 08:31:20PM -0700, John Hubbard wrote:
>On 06/24/2017 07:52 PM, Wei Yang wrote:
>> memory hotplug is memory block aligned instead of section aligned.
>> 
>> This patch fix the range check during hotplug.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> ---
>>  drivers/base/memory.c  | 3 ++-
>>  include/linux/memory.h | 2 ++
>>  mm/memory_hotplug.c    | 9 +++++----
>>  3 files changed, 9 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index c7c4e0325cdb..b54cfe9cd98b 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -31,7 +31,8 @@ static DEFINE_MUTEX(mem_sysfs_mutex);
>>  
>>  #define to_memory_block(dev) container_of(dev, struct memory_block, dev)
>>  
>> -static int sections_per_block;
>> +int sections_per_block;
>> +EXPORT_SYMBOL(sections_per_block);
>
>Hi Wei,
>
>Is sections_per_block ever assigned a value? I am not seeing that happen,
>either in this patch, or in the larger patchset.
>

This is assigned in memory_dev_init(). Not in my patch.

>
>>  
>>  static inline int base_memory_block_id(int section_nr)
>>  {
>> diff --git a/include/linux/memory.h b/include/linux/memory.h
>> index b723a686fc10..51a6355aa56d 100644
>> --- a/include/linux/memory.h
>> +++ b/include/linux/memory.h
>> @@ -142,4 +142,6 @@ extern struct memory_block *find_memory_block(struct mem_section *);
>>   */
>>  extern struct mutex text_mutex;
>>  
>> +extern int sections_per_block;
>> +
>>  #endif /* _LINUX_MEMORY_H_ */
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 387ca386142c..f5d06afc8645 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1183,11 +1183,12 @@ static int check_hotplug_memory_range(u64 start, u64 size)
>>  {
>>  	u64 start_pfn = PFN_DOWN(start);
>>  	u64 nr_pages = size >> PAGE_SHIFT;
>> +	u64 page_per_block = sections_per_block * PAGES_PER_SECTION;
>
>"pages_per_block" would be a little better.
>
>Also, in the first line of the commit, s/aligne/align/.

Good, thanks.

>
>thanks,
>john h
>
>>  
>> -	/* Memory range must be aligned with section */
>> -	if ((start_pfn & ~PAGE_SECTION_MASK) ||
>> -	    (nr_pages % PAGES_PER_SECTION) || (!nr_pages)) {
>> -		pr_err("Section-unaligned hotplug range: start 0x%llx, size 0x%llx\n",
>> +	/* Memory range must be aligned with memory_block */
>> +	if ((start_pfn & (page_per_block - 1)) ||
>> +	    (nr_pages % page_per_block) || (!nr_pages)) {
>> +		pr_err("Memory_block-unaligned hotplug range: start 0x%llx, size 0x%llx\n",
>>  				(unsigned long long)start,
>>  				(unsigned long long)size);
>>  		return -EINVAL;
>> 

-- 
Wei Yang
Help you, Help me

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH 1/4] mm/hotplug: aligne the hotplugable range with memory_block

[John Hubbard]

On 06/25/2017 05:20 PM, Wei Yang wrote:
> * PGP Signed by an unknown key
> 
> On Sat, Jun 24, 2017 at 08:31:20PM -0700, John Hubbard wrote:
>> On 06/24/2017 07:52 PM, Wei Yang wrote:
>>> memory hotplug is memory block aligned instead of section aligned.
>>>
>>> This patch fix the range check during hotplug.
>>>
>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>>> ---
>>>  drivers/base/memory.c  | 3 ++-
>>>  include/linux/memory.h | 2 ++
>>>  mm/memory_hotplug.c    | 9 +++++----
>>>  3 files changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>>> index c7c4e0325cdb..b54cfe9cd98b 100644
>>> --- a/drivers/base/memory.c
>>> +++ b/drivers/base/memory.c
>>> @@ -31,7 +31,8 @@ static DEFINE_MUTEX(mem_sysfs_mutex);
>>>  
>>>  #define to_memory_block(dev) container_of(dev, struct memory_block, dev)
>>>  
>>> -static int sections_per_block;
>>> +int sections_per_block;
>>> +EXPORT_SYMBOL(sections_per_block);
>>
>> Hi Wei,
>>
>> Is sections_per_block ever assigned a value? I am not seeing that happen,
>> either in this patch, or in the larger patchset.
>>
> 
> This is assigned in memory_dev_init(). Not in my patch.
> 

ah, there it is, thanks. (I misread the diff slightly and thought you were adding that
variable, but I see it's actually been there forever.)  

thanks
john h

>>
>>>  
>>>  static inline int base_memory_block_id(int section_nr)
>>>  {
>>> diff --git a/include/linux/memory.h b/include/linux/memory.h
>>> index b723a686fc10..51a6355aa56d 100644
>>> --- a/include/linux/memory.h
>>> +++ b/include/linux/memory.h
>>> @@ -142,4 +142,6 @@ extern struct memory_block *find_memory_block(struct mem_section *);
>>>   */
>>>  extern struct mutex text_mutex;
>>>  
>>> +extern int sections_per_block;
>>> +
>>>  #endif /* _LINUX_MEMORY_H_ */
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index 387ca386142c..f5d06afc8645 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1183,11 +1183,12 @@ static int check_hotplug_memory_range(u64 start, u64 size)
>>>  {
>>>  	u64 start_pfn = PFN_DOWN(start);
>>>  	u64 nr_pages = size >> PAGE_SHIFT;
>>> +	u64 page_per_block = sections_per_block * PAGES_PER_SECTION;
>>
>> "pages_per_block" would be a little better.
>>
>> Also, in the first line of the commit, s/aligne/align/.
> 
> Good, thanks.
> 
>>
>> thanks,
>> john h
>>
>>>  
>>> -	/* Memory range must be aligned with section */
>>> -	if ((start_pfn & ~PAGE_SECTION_MASK) ||
>>> -	    (nr_pages % PAGES_PER_SECTION) || (!nr_pages)) {
>>> -		pr_err("Section-unaligned hotplug range: start 0x%llx, size 0x%llx\n",
>>> +	/* Memory range must be aligned with memory_block */
>>> +	if ((start_pfn & (page_per_block - 1)) ||
>>> +	    (nr_pages % page_per_block) || (!nr_pages)) {
>>> +		pr_err("Memory_block-unaligned hotplug range: start 0x%llx, size 0x%llx\n",
>>>  				(unsigned long long)start,
>>>  				(unsigned long long)size);
>>>  		return -EINVAL;
>>>
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 2/4] mm/hotplug: walk_memroy_range on memory_block uit

[John Hubbard]

On 06/24/2017 07:52 PM, Wei Yang wrote:
> hotplug memory range is memory_block aligned and walk_memroy_range guarded
> with check_hotplug_memory_range(). This is save to iterate on the
> memory_block base.> 
> This patch adjust the iteration unit and assume there is not hole in
> hotplug memory range.

Hi Wei,

In the patch subject, s/memroy/memory/ , and s/uit/unit/, and
s/save/safe.

Actually, I still have a tough time with it, so maybe the 
description above could instead be worded approximately
like this:

Given that a hotpluggable memory range is now block-aligned,
it is safe for walk_memory_range to iterate by blocks.

Change walk_memory_range() so that it iterates at block
boundaries, rather than section boundaries. Also, skip the check
for whether pages are present in the section, and assume 
that there are no holes in the range. (<Insert reason why 
that is safe, here>)


> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  mm/memory_hotplug.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index f5d06afc8645..a79a83ec965f 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1858,17 +1858,11 @@ int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,
>  	unsigned long pfn, section_nr;
>  	int ret;
>  
> -	for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
> +	for (pfn = start_pfn; pfn < end_pfn;
> +		pfn += PAGES_PER_SECTION * sections_per_block) {

Here, and in one or two other spots in the patch, it would be nice
to repeat your approach from patch 0001, where you introduced a
pages_per_block variable. That definitely helps when reading the code.

>  		section_nr = pfn_to_section_nr(pfn);
> -		if (!present_section_nr(section_nr))
> -			continue;

Why is it safe to assume no holes in the memory range? (Maybe Michal's 
patch already covered this and I haven't got that far yet?)

The documentation for this routine says that it walks through all
present memory sections in the range, so it seems like this patch
breaks that.

>  
>  		section = __nr_to_section(section_nr);
> -		/* same memblock? */
> -		if (mem)
> -			if ((section_nr >= mem->start_section_nr) &&
> -			    (section_nr <= mem->end_section_nr))
> -				continue;

Yes, that deletion looks good.

thanks,
john h

>  
>  		mem = find_memory_block_hinted(section, mem);
>  		if (!mem)
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 0/4] mm/hotplug: make hotplug memory_block alligned

[Michal Hocko]

On Sun 25-06-17 10:52:23, Wei Yang wrote:
> Michal & all
> 
> Previously we found the hotplug range is mem_section aligned instead of
> memory_block.
> 
> Here is several draft patches to fix that. To make sure I am getting your
> point correctly, I post it here before further investigation.

This description doesn't explain what the problem is and why do we want
to fix it. Before diving into the code and review changes it would help
a lot to give a short introduction and explain your intention and your
assumptions you base your changes on.

So please start with a highlevel description first.

> Willing to see your comments. :-)
> 
> Wei Yang (4):
>   mm/hotplug: aligne the hotplugable range with memory_block
>   mm/hotplug: walk_memroy_range on memory_block uit
>   mm/hotplug: make __add_pages() iterate on memory_block and split
>     __add_section()
>   base/memory: pass start_section_nr to init_memory_block()
> 
>  drivers/base/memory.c  | 34 ++++++++++++----------------------
>  include/linux/memory.h |  4 +++-
>  mm/memory_hotplug.c    | 48 +++++++++++++++++++++++++-----------------------
>  3 files changed, 40 insertions(+), 46 deletions(-)
> 
> -- 
> 2.11.0
> 

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 3/4] mm/hotplug: make __add_pages() iterate on memory_block and split __add_section()

[John Hubbard]

On 06/24/2017 07:52 PM, Wei Yang wrote:
> Memory hotplug unit is memory_block which contains one or several
> mem_section. The current logic is iterating on each mem_section and add or
> adjust the memory_block every time.
> 
> This patch makes the __add_pages() iterate on memory_block and split
> __add_section() to two functions: __add_section() and __add_memory_block().
> 
> The first one would take care of each section data and the second one would
> register the memory_block at once, which makes the function more clear and
> natural.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  drivers/base/memory.c | 17 +++++------------
>  mm/memory_hotplug.c   | 27 +++++++++++++++++----------
>  2 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index b54cfe9cd98b..468e5ad1bc87 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -705,19 +705,12 @@ int register_new_memory(int nid, struct mem_section *section)
>  
>  	mutex_lock(&mem_sysfs_mutex);
>  
> -	mem = find_memory_block(section);
> -	if (mem) {
> -		mem->section_count++;
> -		put_device(&mem->dev);
> -	} else {
> -		ret = init_memory_block(&mem, section, MEM_OFFLINE);
> -		if (ret)
> -			goto out;
> -		mem->section_count++;
> -	}
> +	ret = init_memory_block(&mem, section, MEM_OFFLINE);
> +	if (ret)
> +		goto out;
> +	mem->section_count = sections_per_block;

Hi Wei,

Things have changed...the register_new_memory() routine is accepting a single section,
but instead of registering just that section, it is registering a containing block.
(That works, because apparently the approach is to make sections_per_block == 1,
and eventually kill sections, if I am reading all this correctly.)

So, how about this: let's add a line to the function comment: 

* Register an entire memory_block.

That makes it clearer that we're dealing in blocks, even though the memsection*
argument is passed in.

>  
> -	if (mem->section_count == sections_per_block)
> -		ret = register_mem_sect_under_node(mem, nid);
> +	ret = register_mem_sect_under_node(mem, nid);
>  out:
>  	mutex_unlock(&mem_sysfs_mutex);
>  	return ret;
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index a79a83ec965f..14a08b980b59 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -302,8 +302,7 @@ 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,
> -		bool want_memblock)
> +static int __meminit __add_section(int nid, unsigned long phys_start_pfn)
>  {
>  	int ret;
>  	int i;
> @@ -332,6 +331,18 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
>  		SetPageReserved(page);
>  	}
>  
> +	return 0;
> +}
> +
> +static int __meminit __add_memory_block(int nid, unsigned long phys_start_pfn,
> +		bool want_memblock)
> +{
> +	int ret;
> +
> +	ret = __add_section(nid, phys_start_pfn);
> +	if (ret)
> +		return ret;
> +
>  	if (!want_memblock)
>  		return 0;
>  
> @@ -347,15 +358,10 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
>  int __ref __add_pages(int nid, unsigned long phys_start_pfn,
>  			unsigned long nr_pages, bool want_memblock)
>  {
> -	unsigned long i;
> +	unsigned long pfn;
>  	int err = 0;
> -	int start_sec, end_sec;
>  	struct vmem_altmap *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);
> -
>  	altmap = to_vmem_altmap((unsigned long) pfn_to_page(phys_start_pfn));
>  	if (altmap) {
>  		/*
> @@ -370,8 +376,9 @@ int __ref __add_pages(int nid, unsigned long phys_start_pfn,
>  		altmap->alloc = 0;
>  	}
>  
> -	for (i = start_sec; i <= end_sec; i++) {
> -		err = __add_section(nid, section_nr_to_pfn(i), want_memblock);
> +	for (pfn; pfn < phys_start_pfn + nr_pages;
> +			pfn += sections_per_block * PAGES_PER_SECTION) {

A pages_per_block variable would be nice here, too.

thanks,
john h

> +		err = __add_memory_block(nid, pfn, want_memblock);
>  
>  		/*
>  		 * EEXIST is finally dealt with by ioresource collision
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 1/4] mm/hotplug: aligne the hotplugable range with memory_block

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

On Sun, Jun 25, 2017 at 11:49:21PM -0700, John Hubbard wrote:
>>>
>>> Hi Wei,
>>>
>>> Is sections_per_block ever assigned a value? I am not seeing that happen,
>>> either in this patch, or in the larger patchset.
>>>
>> 
>> This is assigned in memory_dev_init(). Not in my patch.
>> 
>
>ah, there it is, thanks. (I misread the diff slightly and thought you were adding that
>variable, but I see it's actually been there forever.)  
>

:-)

Welcome your comments.

>thanks
>john h
>

-- 
Wei Yang
Help you, Help me

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH 2/4] mm/hotplug: walk_memroy_range on memory_block uit

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 3474 bytes --]

On Mon, Jun 26, 2017 at 12:32:40AM -0700, John Hubbard wrote:
>On 06/24/2017 07:52 PM, Wei Yang wrote:
>> hotplug memory range is memory_block aligned and walk_memroy_range guarded
>> with check_hotplug_memory_range(). This is save to iterate on the
>> memory_block base.> 
>> This patch adjust the iteration unit and assume there is not hole in
>> hotplug memory range.
>
>Hi Wei,
>
>In the patch subject, s/memroy/memory/ , and s/uit/unit/, and
>s/save/safe.
>

Nice.

>Actually, I still have a tough time with it, so maybe the 
>description above could instead be worded approximately
>like this:
>
>Given that a hotpluggable memory range is now block-aligned,
>it is safe for walk_memory_range to iterate by blocks.
>
>Change walk_memory_range() so that it iterates at block
>boundaries, rather than section boundaries. Also, skip the check
>for whether pages are present in the section, and assume 
>that there are no holes in the range. (<Insert reason why 
>that is safe, here>)
>

Sounds better than mine :-)

>
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> ---
>>  mm/memory_hotplug.c | 10 ++--------
>>  1 file changed, 2 insertions(+), 8 deletions(-)
>> 
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index f5d06afc8645..a79a83ec965f 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1858,17 +1858,11 @@ int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,
>>  	unsigned long pfn, section_nr;
>>  	int ret;
>>  
>> -	for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
>> +	for (pfn = start_pfn; pfn < end_pfn;
>> +		pfn += PAGES_PER_SECTION * sections_per_block) {
>
>Here, and in one or two other spots in the patch, it would be nice
>to repeat your approach from patch 0001, where you introduced a
>pages_per_block variable. That definitely helps when reading the code.
>

Sounds nice, let me try to introduce the pages_per_block.

>>  		section_nr = pfn_to_section_nr(pfn);
>> -		if (!present_section_nr(section_nr))
>> -			continue;
>
>Why is it safe to assume no holes in the memory range? (Maybe Michal's 
>patch already covered this and I haven't got that far yet?)
>
>The documentation for this routine says that it walks through all
>present memory sections in the range, so it seems like this patch
>breaks that.
>

Hmm... it is a little bit hard to describe.

First the documentation of the function is a little misleading. When you look
at the code, it call the "func" only once for a memory_block, not for every
present mem_section as it says. So have some memory in the memory_block would
meet the requirement.

Second, after the check in patch 1, it is for sure the range is memory_block
aligned, which means it must have some memory in that memory_block. It would
be strange if someone claim to add a memory range but with no real memory.

This is why I remove the check here.


>>  
>>  		section = __nr_to_section(section_nr);
>> -		/* same memblock? */
>> -		if (mem)
>> -			if ((section_nr >= mem->start_section_nr) &&
>> -			    (section_nr <= mem->end_section_nr))
>> -				continue;
>
>Yes, that deletion looks good.
>

From this we can see, if there IS some memory, the function will be invoked
and only invoked once.

>thanks,
>john h
>
>>  
>>  		mem = find_memory_block_hinted(section, mem);
>>  		if (!mem)
>> 

-- 
Wei Yang
Help you, Help me

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH 3/4] mm/hotplug: make __add_pages() iterate on memory_block and split __add_section()

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 5602 bytes --]

On Mon, Jun 26, 2017 at 12:50:14AM -0700, John Hubbard wrote:
>On 06/24/2017 07:52 PM, Wei Yang wrote:
>> Memory hotplug unit is memory_block which contains one or several
>> mem_section. The current logic is iterating on each mem_section and add or
>> adjust the memory_block every time.
>> 
>> This patch makes the __add_pages() iterate on memory_block and split
>> __add_section() to two functions: __add_section() and __add_memory_block().
>> 
>> The first one would take care of each section data and the second one would
>> register the memory_block at once, which makes the function more clear and
>> natural.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> ---
>>  drivers/base/memory.c | 17 +++++------------
>>  mm/memory_hotplug.c   | 27 +++++++++++++++++----------
>>  2 files changed, 22 insertions(+), 22 deletions(-)
>> 
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index b54cfe9cd98b..468e5ad1bc87 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -705,19 +705,12 @@ int register_new_memory(int nid, struct mem_section *section)
>>  
>>  	mutex_lock(&mem_sysfs_mutex);
>>  
>> -	mem = find_memory_block(section);
>> -	if (mem) {
>> -		mem->section_count++;
>> -		put_device(&mem->dev);
>> -	} else {
>> -		ret = init_memory_block(&mem, section, MEM_OFFLINE);
>> -		if (ret)
>> -			goto out;
>> -		mem->section_count++;
>> -	}
>> +	ret = init_memory_block(&mem, section, MEM_OFFLINE);
>> +	if (ret)
>> +		goto out;
>> +	mem->section_count = sections_per_block;
>
>Hi Wei,
>
>Things have changed...the register_new_memory() routine is accepting a single section,
>but instead of registering just that section, it is registering a containing block.
>(That works, because apparently the approach is to make sections_per_block == 1,
>and eventually kill sections, if I am reading all this correctly.)
>

The original function is a little confusing. Actually it tries to register a
memory_block while it register it for several times, on each present
mem_section actually.

This change here will register the whole memory_block at once.

You would see in next patch it will accept the start section number instead of
a section, while maybe more easy to understand it.

BTW, I don't get your point on kill sections when sections_per_block == The
original function is a little confusing. Actually it tries to register a
memory_block while it register it for several times, on each present
mem_section actually.

This change here will register the whole memory_block at once.

You would see in next patch it will accept the start section number instead of
a section, while maybe more easy to understand it.

BTW, I don't get your point on kill sections when sections_per_block == 1.
Would you rephrase this?

>So, how about this: let's add a line to the function comment: 
>
>* Register an entire memory_block.
>

May look good, let me have a try.

>That makes it clearer that we're dealing in blocks, even though the memsection*
>argument is passed in.
>
>>  
>> -	if (mem->section_count == sections_per_block)
>> -		ret = register_mem_sect_under_node(mem, nid);
>> +	ret = register_mem_sect_under_node(mem, nid);
>>  out:
>>  	mutex_unlock(&mem_sysfs_mutex);
>>  	return ret;
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index a79a83ec965f..14a08b980b59 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -302,8 +302,7 @@ 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,
>> -		bool want_memblock)
>> +static int __meminit __add_section(int nid, unsigned long phys_start_pfn)
>>  {
>>  	int ret;
>>  	int i;
>> @@ -332,6 +331,18 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
>>  		SetPageReserved(page);
>>  	}
>>  
>> +	return 0;
>> +}
>> +
>> +static int __meminit __add_memory_block(int nid, unsigned long phys_start_pfn,
>> +		bool want_memblock)
>> +{
>> +	int ret;
>> +
>> +	ret = __add_section(nid, phys_start_pfn);
>> +	if (ret)
>> +		return ret;
>> +
>>  	if (!want_memblock)
>>  		return 0;
>>  
>> @@ -347,15 +358,10 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
>>  int __ref __add_pages(int nid, unsigned long phys_start_pfn,
>>  			unsigned long nr_pages, bool want_memblock)
>>  {
>> -	unsigned long i;
>> +	unsigned long pfn;
>>  	int err = 0;
>> -	int start_sec, end_sec;
>>  	struct vmem_altmap *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);
>> -
>>  	altmap = to_vmem_altmap((unsigned long) pfn_to_page(phys_start_pfn));
>>  	if (altmap) {
>>  		/*
>> @@ -370,8 +376,9 @@ int __ref __add_pages(int nid, unsigned long phys_start_pfn,
>>  		altmap->alloc = 0;
>>  	}
>>  
>> -	for (i = start_sec; i <= end_sec; i++) {
>> -		err = __add_section(nid, section_nr_to_pfn(i), want_memblock);
>> +	for (pfn; pfn < phys_start_pfn + nr_pages;
>> +			pfn += sections_per_block * PAGES_PER_SECTION) {
>

yep

>A pages_per_block variable would be nice here, too.
>
>thanks,
>john h
>
>> +		err = __add_memory_block(nid, pfn, want_memblock);
>>  
>>  		/*
>>  		 * EEXIST is finally dealt with by ioresource collision
>> 

-- 
Wei Yang
Help you, Help me

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH 0/4] mm/hotplug: make hotplug memory_block alligned

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

On Mon, Jun 26, 2017 at 09:46:35AM +0200, Michal Hocko wrote:
>On Sun 25-06-17 10:52:23, Wei Yang wrote:
>> Michal & all
>> 
>> Previously we found the hotplug range is mem_section aligned instead of
>> memory_block.
>> 
>> Here is several draft patches to fix that. To make sure I am getting your
>> point correctly, I post it here before further investigation.
>
>This description doesn't explain what the problem is and why do we want
>to fix it. Before diving into the code and review changes it would help
>a lot to give a short introduction and explain your intention and your
>assumptions you base your changes on.
>
>So please start with a highlevel description first.
>

Here is the high level description in my mind, glad to see your comment.


The minimum unit of memory hotplug is memory_block instead of mem_section.
While in current implementation, we see several concept misunderstanding.

For example:
1. The alignment check is based on mem_section instead of memory_block
2. Online memory range on section base instead of memory_block base

Even memory_block and mem_section are close related, they are two concepts. It
is possible to initialize and register them respectively.

For example:
1. In __add_section(), it tries to register these two in one place.

This patch generally does the following:
1. Aligned the range with memory_block
2. Online rage with memory_block base
3. Split the registration of memory_block and mem_section



-- 
Wei Yang
Help you, Help me

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH 3/4] mm/hotplug: make __add_pages() iterate on memory_block and split __add_section()

[John Hubbard]

On 06/26/2017 04:53 PM, Wei Yang wrote:
> On Mon, Jun 26, 2017 at 12:50:14AM -0700, John Hubbard wrote:
>> On 06/24/2017 07:52 PM, Wei Yang wrote:
[...]
>>
>> Things have changed...the register_new_memory() routine is accepting a single section,
>> but instead of registering just that section, it is registering a containing block.
>> (That works, because apparently the approach is to make sections_per_block == 1,
>> and eventually kill sections, if I am reading all this correctly.)
>>
> 
> The original function is a little confusing. Actually it tries to register a
> memory_block while it register it for several times, on each present
> mem_section actually.
> 
> This change here will register the whole memory_block at once.
> 
> You would see in next patch it will accept the start section number instead of
> a section, while maybe more easy to understand it.

Yes I saw that, and it does help, but even after that, I still thought
we should add that "* Register an entire memory_block."  line.

> 
> BTW, I don't get your point on kill sections when sections_per_block == The
> original function is a little confusing. Actually it tries to register a
> memory_block while it register it for several times, on each present
> mem_section actually.
> 
> This change here will register the whole memory_block at once.
> 
> You would see in next patch it will accept the start section number instead of
> a section, while maybe more easy to understand it.
> 
> BTW, I don't get your point on kill sections when sections_per_block == 1.
> Would you rephrase this?
> 

I was just trying to say, "if I understand correctly, your plan is
to:

   Step 1: have one section per block, and then eventually

   Step 2: get rid of sections (that's what "kill" meant) entirely."

No big deal, I'm just saying it out loud, to be sure I've got it right.

thanks,
john h

>> So, how about this: let's add a line to the function comment: 
>>
>> * Register an entire memory_block.
>>
> 
> May look good, let me have a try.
> 
>> That makes it clearer that we're dealing in blocks, even though the memsection*
>> argument is passed in.
>>
>>>  
>>> -	if (mem->section_count == sections_per_block)
>>> -		ret = register_mem_sect_under_node(mem, nid);
>>> +	ret = register_mem_sect_under_node(mem, nid);
>>>  out:
>>>  	mutex_unlock(&mem_sysfs_mutex);
>>>  	return ret;
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index a79a83ec965f..14a08b980b59 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -302,8 +302,7 @@ 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,
>>> -		bool want_memblock)
>>> +static int __meminit __add_section(int nid, unsigned long phys_start_pfn)
>>>  {
>>>  	int ret;
>>>  	int i;
>>> @@ -332,6 +331,18 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
>>>  		SetPageReserved(page);
>>>  	}
>>>  
>>> +	return 0;
>>> +}
>>> +
>>> +static int __meminit __add_memory_block(int nid, unsigned long phys_start_pfn,
>>> +		bool want_memblock)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = __add_section(nid, phys_start_pfn);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>  	if (!want_memblock)
>>>  		return 0;
>>>  
>>> @@ -347,15 +358,10 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
>>>  int __ref __add_pages(int nid, unsigned long phys_start_pfn,
>>>  			unsigned long nr_pages, bool want_memblock)
>>>  {
>>> -	unsigned long i;
>>> +	unsigned long pfn;
>>>  	int err = 0;
>>> -	int start_sec, end_sec;
>>>  	struct vmem_altmap *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);
>>> -
>>>  	altmap = to_vmem_altmap((unsigned long) pfn_to_page(phys_start_pfn));
>>>  	if (altmap) {
>>>  		/*
>>> @@ -370,8 +376,9 @@ int __ref __add_pages(int nid, unsigned long phys_start_pfn,
>>>  		altmap->alloc = 0;
>>>  	}
>>>  
>>> -	for (i = start_sec; i <= end_sec; i++) {
>>> -		err = __add_section(nid, section_nr_to_pfn(i), want_memblock);
>>> +	for (pfn; pfn < phys_start_pfn + nr_pages;
>>> +			pfn += sections_per_block * PAGES_PER_SECTION) {
>>
> 
> yep
> 
>> A pages_per_block variable would be nice here, too.
>>
>> thanks,
>> john h
>>
>>> +		err = __add_memory_block(nid, pfn, want_memblock);
>>>  
>>>  		/*
>>>  		 * EEXIST is finally dealt with by ioresource collision
>>>
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 2/4] mm/hotplug: walk_memroy_range on memory_block uit

[John Hubbard]

On 06/26/2017 04:40 PM, Wei Yang wrote:
> On Mon, Jun 26, 2017 at 12:32:40AM -0700, John Hubbard wrote:
>> On 06/24/2017 07:52 PM, Wei Yang wrote:
[...]
>>
>> Why is it safe to assume no holes in the memory range? (Maybe Michal's 
>> patch already covered this and I haven't got that far yet?)
>>
>> The documentation for this routine says that it walks through all
>> present memory sections in the range, so it seems like this patch
>> breaks that.
>>
> 
> Hmm... it is a little bit hard to describe.
> 
> First the documentation of the function is a little misleading. When you look
> at the code, it call the "func" only once for a memory_block, not for every
> present mem_section as it says. So have some memory in the memory_block would
> meet the requirement.
> 
> Second, after the check in patch 1, it is for sure the range is memory_block
> aligned, which means it must have some memory in that memory_block. It would
> be strange if someone claim to add a memory range but with no real memory.
> 
> This is why I remove the check here.

OK. In that case, it seems like we should update the function documentation
to match. Something like this, maybe? :

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index bdaafcf46f49..d36b2f4eaf39 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1872,14 +1872,14 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
 /**
- * walk_memory_range - walks through all mem sections in [start_pfn, end_pfn)
+ * walk_memory_range - walks through all mem blocks in [start_pfn, end_pfn)
  * @start_pfn: start pfn of the memory range
  * @end_pfn: end pfn of the memory range
  * @arg: argument passed to func
- * @func: callback for each memory section walked
+ * @func: callback for each memory block walked
  *
- * This function walks through all present mem sections in range
- * [start_pfn, end_pfn) and call func on each mem section.
+ * This function walks through all mem blocks in the range
+ * [start_pfn, end_pfn) and calls func on each mem block.
  *
  * Returns the return value of func.
  */


thanks,
john h

> 
> 
>>>  
>>>  		section = __nr_to_section(section_nr);
>>> -		/* same memblock? */
>>> -		if (mem)
>>> -			if ((section_nr >= mem->start_section_nr) &&
>>> -			    (section_nr <= mem->end_section_nr))
>>> -				continue;
>>
>> Yes, that deletion looks good.
>>
> 
> From this we can see, if there IS some memory, the function will be invoked
> and only invoked once.
> 
>> thanks,
>> john h
>>
>>>  
>>>  		mem = find_memory_block_hinted(section, mem);
>>>  		if (!mem)
>>>
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 4/4] base/memory: pass start_section_nr to init_memory_block()

[John Hubbard]

On 06/24/2017 07:52 PM, Wei Yang wrote:
> The second parameter of init_memory_block() is to calculate the
> start_section_nr for the memory_block. While current implementation dose
> some unnecessary transform between mem_sectioni and section_nr.

Hi Wei,

I am unable to find anything wrong with this patch (except of course
that your top-level description in the "[PATCH 0/4" thread will need
to be added somewhere).

Here's a slight typo/grammar improvement for the patch
description above, if you like:

"The current implementation does some unnecessary conversions
between mem_section and section_nr."

thanks,
john h

> 
> This patch simplifies the function by just passing the start_section_nr to
> it. By doing so, we can also simplify add_memory_block() too.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  drivers/base/memory.c  | 16 ++++++----------
>  include/linux/memory.h |  2 +-
>  mm/memory_hotplug.c    |  2 +-
>  3 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 468e5ad1bc87..43783dbb1d5e 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -645,7 +645,7 @@ int register_memory(struct memory_block *memory)
>  }
>  
>  static int init_memory_block(struct memory_block **memory,
> -			     struct mem_section *section, unsigned long state)
> +			     int start_section_nr, unsigned long state)
>  {
>  	struct memory_block *mem;
>  	unsigned long start_pfn;
> @@ -656,9 +656,7 @@ static int init_memory_block(struct memory_block **memory,
>  	if (!mem)
>  		return -ENOMEM;
>  
> -	scn_nr = __section_nr(section);
> -	mem->start_section_nr =
> -			base_memory_block_id(scn_nr) * sections_per_block;
> +	mem->start_section_nr = start_section_nr;
>  	mem->end_section_nr = mem->start_section_nr + sections_per_block - 1;
>  	mem->state = state;
>  	start_pfn = section_nr_to_pfn(mem->start_section_nr);
> @@ -673,21 +671,19 @@ static int init_memory_block(struct memory_block **memory,
>  static int add_memory_block(int base_section_nr)
>  {
>  	struct memory_block *mem;
> -	int i, ret, section_count = 0, section_nr;
> +	int i, ret, section_count = 0;
>  
>  	for (i = base_section_nr;
>  	     (i < base_section_nr + sections_per_block) && i < NR_MEM_SECTIONS;
>  	     i++) {
>  		if (!present_section_nr(i))
>  			continue;
> -		if (section_count == 0)
> -			section_nr = i;
>  		section_count++;
>  	}
>  
>  	if (section_count == 0)
>  		return 0;
> -	ret = init_memory_block(&mem, __nr_to_section(section_nr), MEM_ONLINE);
> +	ret = init_memory_block(&mem, base_section_nr, MEM_ONLINE);
>  	if (ret)
>  		return ret;
>  	mem->section_count = section_count;
> @@ -698,14 +694,14 @@ static int add_memory_block(int base_section_nr)
>   * need an interface for the VM to add new memory regions,
>   * but without onlining it.
>   */
> -int register_new_memory(int nid, struct mem_section *section)
> +int register_new_memory(int nid, int start_section_nr)
>  {
>  	int ret = 0;
>  	struct memory_block *mem;
>  
>  	mutex_lock(&mem_sysfs_mutex);
>  
> -	ret = init_memory_block(&mem, section, MEM_OFFLINE);
> +	ret = init_memory_block(&mem, start_section_nr, MEM_OFFLINE);
>  	if (ret)
>  		goto out;
>  	mem->section_count = sections_per_block;
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index 51a6355aa56d..0cbde14f7cea 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -108,7 +108,7 @@ extern int register_memory_notifier(struct notifier_block *nb);
>  extern void unregister_memory_notifier(struct notifier_block *nb);
>  extern int register_memory_isolate_notifier(struct notifier_block *nb);
>  extern void unregister_memory_isolate_notifier(struct notifier_block *nb);
> -extern int register_new_memory(int, struct mem_section *);
> +extern int register_new_memory(int, int);
>  #ifdef CONFIG_MEMORY_HOTREMOVE
>  extern int unregister_memory_section(struct mem_section *);
>  #endif
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 14a08b980b59..fc198847dd5b 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -346,7 +346,7 @@ static int __meminit __add_memory_block(int nid, unsigned long phys_start_pfn,
>  	if (!want_memblock)
>  		return 0;
>  
> -	return register_new_memory(nid, __pfn_to_section(phys_start_pfn));
> +	return register_new_memory(nid, pfn_to_section_nr(phys_start_pfn));
>  }
>  
>  /*
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC PATCH 2/4] mm/hotplug: walk_memroy_range on memory_block uit

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 2393 bytes --]

On Mon, Jun 26, 2017 at 11:59:52PM -0700, John Hubbard wrote:
>On 06/26/2017 04:40 PM, Wei Yang wrote:
>> On Mon, Jun 26, 2017 at 12:32:40AM -0700, John Hubbard wrote:
>>> On 06/24/2017 07:52 PM, Wei Yang wrote:
>[...]
>>>
>>> Why is it safe to assume no holes in the memory range? (Maybe Michal's 
>>> patch already covered this and I haven't got that far yet?)
>>>
>>> The documentation for this routine says that it walks through all
>>> present memory sections in the range, so it seems like this patch
>>> breaks that.
>>>
>> 
>> Hmm... it is a little bit hard to describe.
>> 
>> First the documentation of the function is a little misleading. When you look
>> at the code, it call the "func" only once for a memory_block, not for every
>> present mem_section as it says. So have some memory in the memory_block would
>> meet the requirement.
>> 
>> Second, after the check in patch 1, it is for sure the range is memory_block
>> aligned, which means it must have some memory in that memory_block. It would
>> be strange if someone claim to add a memory range but with no real memory.
>> 
>> This is why I remove the check here.
>
>OK. In that case, it seems like we should update the function documentation
>to match. Something like this, maybe? :
>
>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index bdaafcf46f49..d36b2f4eaf39 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -1872,14 +1872,14 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
> #endif /* CONFIG_MEMORY_HOTREMOVE */
> 
> /**
>- * walk_memory_range - walks through all mem sections in [start_pfn, end_pfn)
>+ * walk_memory_range - walks through all mem blocks in [start_pfn, end_pfn)
>  * @start_pfn: start pfn of the memory range
>  * @end_pfn: end pfn of the memory range
>  * @arg: argument passed to func
>- * @func: callback for each memory section walked
>+ * @func: callback for each memory block walked
>  *
>- * This function walks through all present mem sections in range
>- * [start_pfn, end_pfn) and call func on each mem section.
>+ * This function walks through all mem blocks in the range
>+ * [start_pfn, end_pfn) and calls func on each mem block.
>  *
>  * Returns the return value of func.
>  */
>

Yes, I have changed this in my repo.

>
>thanks,
>john h

-- 
Wei Yang
Help you, Help me

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH 3/4] mm/hotplug: make __add_pages() iterate on memory_block and split __add_section()

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

On Mon, Jun 26, 2017 at 11:47:38PM -0700, John Hubbard wrote:
>On 06/26/2017 04:53 PM, Wei Yang wrote:
>> On Mon, Jun 26, 2017 at 12:50:14AM -0700, John Hubbard wrote:
>>> On 06/24/2017 07:52 PM, Wei Yang wrote:
>[...]
>>>
>>> Things have changed...the register_new_memory() routine is accepting a single section,
>>> but instead of registering just that section, it is registering a containing block.
>>> (That works, because apparently the approach is to make sections_per_block == 1,
>>> and eventually kill sections, if I am reading all this correctly.)
>>>
>> 
>> The original function is a little confusing. Actually it tries to register a
>> memory_block while it register it for several times, on each present
>> mem_section actually.
>> 
>> This change here will register the whole memory_block at once.
>> 
>> You would see in next patch it will accept the start section number instead of
>> a section, while maybe more easy to understand it.
>
>Yes I saw that, and it does help, but even after that, I still thought
>we should add that "* Register an entire memory_block."  line.
>

Well, it is fine to me.


-- 
Wei Yang
Help you, Help me

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH 4/4] base/memory: pass start_section_nr to init_memory_block()

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

On Tue, Jun 27, 2017 at 12:11:52AM -0700, John Hubbard wrote:
>On 06/24/2017 07:52 PM, Wei Yang wrote:
>> The second parameter of init_memory_block() is to calculate the
>> start_section_nr for the memory_block. While current implementation dose
>> some unnecessary transform between mem_sectioni and section_nr.
>
>Hi Wei,
>
>I am unable to find anything wrong with this patch (except of course
>that your top-level description in the "[PATCH 0/4" thread will need
>to be added somewhere).
>
>Here's a slight typo/grammar improvement for the patch
>description above, if you like:
>
>"The current implementation does some unnecessary conversions
>between mem_section and section_nr."
>

Thanks :-)

>thanks,
>john h
>

-- 
Wei Yang
Help you, Help me

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH 3/4] mm/hotplug: make __add_pages() iterate on memory_block and split __add_section()

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index a79a83ec965f..14a08b980b59 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -302,8 +302,7 @@ 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,
>-		bool want_memblock)
>+static int __meminit __add_section(int nid, unsigned long phys_start_pfn)
> {
> 	int ret;
> 	int i;
>@@ -332,6 +331,18 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
> 		SetPageReserved(page);
> 	}
> 
>+	return 0;
>+}
>+
>+static int __meminit __add_memory_block(int nid, unsigned long phys_start_pfn,
>+		bool want_memblock)
>+{
>+	int ret;
>+
>+	ret = __add_section(nid, phys_start_pfn);
>+	if (ret)
>+		return ret;
>+

One error here.

I forget to iterate on each section in the memory_block.
Fixed in my repo.


-- 
Wei Yang
Help you, Help me

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH 0/4] mm/hotplug: make hotplug memory_block alligned

[Michal Hocko]

On Tue 27-06-17 10:13:35, Wei Yang wrote:
> On Mon, Jun 26, 2017 at 09:46:35AM +0200, Michal Hocko wrote:
> >On Sun 25-06-17 10:52:23, Wei Yang wrote:
> >> Michal & all
> >> 
> >> Previously we found the hotplug range is mem_section aligned instead of
> >> memory_block.
> >> 
> >> Here is several draft patches to fix that. To make sure I am getting your
> >> point correctly, I post it here before further investigation.
> >
> >This description doesn't explain what the problem is and why do we want
> >to fix it. Before diving into the code and review changes it would help
> >a lot to give a short introduction and explain your intention and your
> >assumptions you base your changes on.
> >
> >So please start with a highlevel description first.
> >
> 
> Here is the high level description in my mind, glad to see your comment.
> 
> 
> The minimum unit of memory hotplug is memory_block instead of mem_section.
> While in current implementation, we see several concept misunderstanding.
> 
> For example:
> 1. The alignment check is based on mem_section instead of memory_block
> 2. Online memory range on section base instead of memory_block base
> 
> Even memory_block and mem_section are close related, they are two concepts. It
> is possible to initialize and register them respectively.
> 
> For example:
> 1. In __add_section(), it tries to register these two in one place.
> 
> This patch generally does the following:
> 1. Aligned the range with memory_block
> 2. Online rage with memory_block base
> 3. Split the registration of memory_block and mem_section

OK this is slightly better but from a quick glance over cumulative diff
(sorry I didn't have time to look at separate patches yet) you only go
half way through there. E.g. register_new_memory still uses section_nr.
Basically would I would love to see is to make section implementation
details and get it out of any hotplug APIs. Sections are a sparse
concept and they should stay there and in few hotplug functions which
talk to sparse.

Also you have surely noticed that this area is full of subtle code so it
will take a lot of testing to uncover subtle dependencies. I remember
hitting many of them while touching this area. I would encourage you to
read previous discussions for the rework to see which different setups
broke and why.

Thanks!
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>