[PATCH v1] mm/memory_hotplug: Easier calculation to get pages to next section boundary

[PATCH v1] mm/memory_hotplug: Easier calculation to get pages to next section boundary

[David Hildenbrand]

Let's use a calculation that's easier to understand and calculates the
same result. Reusing existing macros makes this look nicer.

We always want to have the number of pages (> 0) to the next section
boundary, starting from the current pfn.

Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: Baoquan He <bhe@redhat.com>
Cc: Wei Yang <richardw.yang@linux.intel.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 mm/memory_hotplug.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 0a54ffac8c68..c30191183c04 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -528,7 +528,8 @@ void __remove_pages(unsigned long pfn, unsigned long nr_pages,
 	for (; pfn < end_pfn; pfn += cur_nr_pages) {
 		cond_resched();
 		/* Select all remaining pages up to the next section boundary */
-		cur_nr_pages = min(end_pfn - pfn, -(pfn | PAGE_SECTION_MASK));
+		cur_nr_pages = min(end_pfn - pfn,
+				   SECTION_ALIGN_UP(pfn + 1) - pfn);
 		__remove_section(pfn, cur_nr_pages, map_offset, altmap);
 		map_offset = 0;
 	}
-- 
2.24.1

Re: [PATCH v1] mm/memory_hotplug: Easier calculation to get pages to next section boundary

[Wei Yang]

On Wed, Feb 05, 2020 at 02:52:51PM +0100, David Hildenbrand wrote:
>Let's use a calculation that's easier to understand and calculates the
>same result. Reusing existing macros makes this look nicer.
>
>We always want to have the number of pages (> 0) to the next section
>boundary, starting from the current pfn.
>
>Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Cc: Michal Hocko <mhocko@kernel.org>
>Cc: Oscar Salvador <osalvador@suse.de>
>Cc: Baoquan He <bhe@redhat.com>
>Cc: Wei Yang <richardw.yang@linux.intel.com>
>Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>

BTW, I got one question about hotplug size requirement.

I thought the hotplug range should be section size aligned, while taking a
look into current code function check_hotplug_memory_range() guard the range.

This function says the range should be block_size aligned. And if I am
correct, block size on x86 should be in the range

    [MIN_MEMORY_BLOCK_SIZE, MEM_SIZE_FOR_LARGE_BLOCK]
    
And MIN_MEMORY_BLOCK_SIZE is section size.

Seems currently we support subsection hotplug? Then how a subsection range got
hotplug? Or this patch is a pre-requisite?

>---
> mm/memory_hotplug.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index 0a54ffac8c68..c30191183c04 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -528,7 +528,8 @@ void __remove_pages(unsigned long pfn, unsigned long nr_pages,
> 	for (; pfn < end_pfn; pfn += cur_nr_pages) {
> 		cond_resched();
> 		/* Select all remaining pages up to the next section boundary */
>-		cur_nr_pages = min(end_pfn - pfn, -(pfn | PAGE_SECTION_MASK));
>+		cur_nr_pages = min(end_pfn - pfn,
>+				   SECTION_ALIGN_UP(pfn + 1) - pfn);
> 		__remove_section(pfn, cur_nr_pages, map_offset, altmap);
> 		map_offset = 0;
> 	}
>-- 
>2.24.1

-- 
Wei Yang
Help you, Help me

Re: [PATCH v1] mm/memory_hotplug: Easier calculation to get pages to next section boundary

[Wei Yang]

On Thu, Feb 06, 2020 at 07:19:45AM +0800, Wei Yang wrote:
>On Wed, Feb 05, 2020 at 02:52:51PM +0100, David Hildenbrand wrote:
>>Let's use a calculation that's easier to understand and calculates the
>>same result. Reusing existing macros makes this look nicer.
>>
>>We always want to have the number of pages (> 0) to the next section
>>boundary, starting from the current pfn.
>>
>>Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
>>Cc: Andrew Morton <akpm@linux-foundation.org>
>>Cc: Michal Hocko <mhocko@kernel.org>
>>Cc: Oscar Salvador <osalvador@suse.de>
>>Cc: Baoquan He <bhe@redhat.com>
>>Cc: Wei Yang <richardw.yang@linux.intel.com>
>>Signed-off-by: David Hildenbrand <david@redhat.com>
>
>Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>
>
>BTW, I got one question about hotplug size requirement.
>
>I thought the hotplug range should be section size aligned, while taking a
>look into current code function check_hotplug_memory_range() guard the range.
>
>This function says the range should be block_size aligned. And if I am
>correct, block size on x86 should be in the range
>
>    [MIN_MEMORY_BLOCK_SIZE, MEM_SIZE_FOR_LARGE_BLOCK]
>    
>And MIN_MEMORY_BLOCK_SIZE is section size.
>
>Seems currently we support subsection hotplug? Then how a subsection range got
>hotplug? Or this patch is a pre-requisite?
>

One more question is we support hot-add subsection memory but not support
hot-online subsection memory.

Is my understanding correct?

-- 
Wei Yang
Help you, Help me

Re: [PATCH v1] mm/memory_hotplug: Easier calculation to get pages to next section boundary

[Baoquan He]

On 02/06/20 at 07:50am, Wei Yang wrote:
> On Thu, Feb 06, 2020 at 07:19:45AM +0800, Wei Yang wrote:
> >On Wed, Feb 05, 2020 at 02:52:51PM +0100, David Hildenbrand wrote:
> >>Let's use a calculation that's easier to understand and calculates the
> >>same result. Reusing existing macros makes this look nicer.
> >>
> >>We always want to have the number of pages (> 0) to the next section
> >>boundary, starting from the current pfn.
> >>
> >>Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
> >>Cc: Andrew Morton <akpm@linux-foundation.org>
> >>Cc: Michal Hocko <mhocko@kernel.org>
> >>Cc: Oscar Salvador <osalvador@suse.de>
> >>Cc: Baoquan He <bhe@redhat.com>
> >>Cc: Wei Yang <richardw.yang@linux.intel.com>
> >>Signed-off-by: David Hildenbrand <david@redhat.com>
> >
> >Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>
> >
> >BTW, I got one question about hotplug size requirement.
> >
> >I thought the hotplug range should be section size aligned, while taking a
> >look into current code function check_hotplug_memory_range() guard the range.

A good question. The current code should be block size aligned. I
remember in some places we assume each block comprise all the sections.
Can't imagine one or some of them are half section filled.

It truly has a risk that system ram is very huge to make the block
size is 2G, someone try to insert a 1G memory board. However, it should
only exist in experiment environment, e.g build a guest with enough ram,
then hot add 1G DIMM. In reality, we don't need to worry about it, at
least what I saw is 512G order of magnitude.

> >
> >This function says the range should be block_size aligned. And if I am
> >correct, block size on x86 should be in the range
> >
> >    [MIN_MEMORY_BLOCK_SIZE, MEM_SIZE_FOR_LARGE_BLOCK]
> >    
> >And MIN_MEMORY_BLOCK_SIZE is section size.

No, if I got it right, the range on x86 is
[MIN_MEMORY_BLOCK_SIZE, MAX_BLOCK_SIZE].

MEM_SIZE_FOR_LARGE_BLOCK is the starting point from which block size can
be adjusted. Otherwise it's MIN_MEMORY_BLOCK_SIZE.

/* Amount of ram needed to start using large blocks */                                                                                            
#define MEM_SIZE_FOR_LARGE_BLOCK (64UL << 30)

> >
> >Seems currently we support subsection hotplug? Then how a subsection range got
> >hotplug? Or this patch is a pre-requisite?

The sub-section hotplug feature was added by your colleague Dan
Williams. It intends to fix a nvdimms issue that nvdimms device could be
mapped into a non section size aligned starting address. And nvdimms
makes use of the existing memory hotplug mechanism to manage pages.
Not sure if we are saying the same thing.

> >
> 
> One more question is we support hot-add subsection memory but not support
> hot-online subsection memory.
> 
> Is my understanding correct?
> 
> -- 
> Wei Yang
> Help you, Help me
> 

Re: [PATCH v1] mm/memory_hotplug: Easier calculation to get pages to next section boundary

[Baoquan He]

On 02/06/20 at 08:13am, Baoquan He wrote:
> On 02/06/20 at 07:50am, Wei Yang wrote:
> > On Thu, Feb 06, 2020 at 07:19:45AM +0800, Wei Yang wrote:
> > >On Wed, Feb 05, 2020 at 02:52:51PM +0100, David Hildenbrand wrote:
> > >>Let's use a calculation that's easier to understand and calculates the
> > >>same result. Reusing existing macros makes this look nicer.
> > >>
> > >>We always want to have the number of pages (> 0) to the next section
> > >>boundary, starting from the current pfn.
> > >>
> > >>Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
> > >>Cc: Andrew Morton <akpm@linux-foundation.org>
> > >>Cc: Michal Hocko <mhocko@kernel.org>
> > >>Cc: Oscar Salvador <osalvador@suse.de>
> > >>Cc: Baoquan He <bhe@redhat.com>
> > >>Cc: Wei Yang <richardw.yang@linux.intel.com>
> > >>Signed-off-by: David Hildenbrand <david@redhat.com>
> > >
> > >Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>
> > >
> > >BTW, I got one question about hotplug size requirement.
> > >
> > >I thought the hotplug range should be section size aligned, while taking a
> > >look into current code function check_hotplug_memory_range() guard the range.
> 
> A good question. The current code should be block size aligned. I
> remember in some places we assume each block comprise all the sections.
> Can't imagine one or some of them are half section filled.

I could be wrong, half filled block may not cause problem. 

> 
> It truly has a risk that system ram is very huge to make the block
> size is 2G, someone try to insert a 1G memory board. However, it should
> only exist in experiment environment, e.g build a guest with enough ram,
> then hot add 1G DIMM. In reality, we don't need to worry about it, at
> least what I saw is 512G order of magnitude.
> 
> > >
> > >This function says the range should be block_size aligned. And if I am
> > >correct, block size on x86 should be in the range
> > >
> > >    [MIN_MEMORY_BLOCK_SIZE, MEM_SIZE_FOR_LARGE_BLOCK]
> > >    
> > >And MIN_MEMORY_BLOCK_SIZE is section size.
> 
> No, if I got it right, the range on x86 is
> [MIN_MEMORY_BLOCK_SIZE, MAX_BLOCK_SIZE].
> 
> MEM_SIZE_FOR_LARGE_BLOCK is the starting point from which block size can
> be adjusted. Otherwise it's MIN_MEMORY_BLOCK_SIZE.
> 
> /* Amount of ram needed to start using large blocks */                                                                                            
> #define MEM_SIZE_FOR_LARGE_BLOCK (64UL << 30)
> 
> > >
> > >Seems currently we support subsection hotplug? Then how a subsection range got
> > >hotplug? Or this patch is a pre-requisite?
> 
> The sub-section hotplug feature was added by your colleague Dan
> Williams. It intends to fix a nvdimms issue that nvdimms device could be
> mapped into a non section size aligned starting address. And nvdimms
> makes use of the existing memory hotplug mechanism to manage pages.
> Not sure if we are saying the same thing.
> 
> > >
> > 
> > One more question is we support hot-add subsection memory but not support
> > hot-online subsection memory.
> > 
> > Is my understanding correct?
> > 
> > -- 
> > Wei Yang
> > Help you, Help me
> > 

Re: [PATCH v1] mm/memory_hotplug: Easier calculation to get pages to next section boundary

[Baoquan He]

On 02/05/20 at 02:52pm, David Hildenbrand wrote:
> Let's use a calculation that's easier to understand and calculates the
> same result. Reusing existing macros makes this look nicer.
> 
> We always want to have the number of pages (> 0) to the next section
> boundary, starting from the current pfn.
> 
> Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Wei Yang <richardw.yang@linux.intel.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

LGTM.

Reviewed-by: Baoquan He <bhe@redhat.com>

> ---
>  mm/memory_hotplug.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 0a54ffac8c68..c30191183c04 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -528,7 +528,8 @@ void __remove_pages(unsigned long pfn, unsigned long nr_pages,
>  	for (; pfn < end_pfn; pfn += cur_nr_pages) {
>  		cond_resched();
>  		/* Select all remaining pages up to the next section boundary */
> -		cur_nr_pages = min(end_pfn - pfn, -(pfn | PAGE_SECTION_MASK));
> +		cur_nr_pages = min(end_pfn - pfn,
> +				   SECTION_ALIGN_UP(pfn + 1) - pfn);
>  		__remove_section(pfn, cur_nr_pages, map_offset, altmap);
>  		map_offset = 0;
>  	}
> -- 
> 2.24.1
> 

Re: [PATCH v1] mm/memory_hotplug: Easier calculation to get pages to next section boundary

[Wei Yang]

On Thu, Feb 06, 2020 at 08:37:36AM +0800, Baoquan He wrote:
>On 02/06/20 at 08:13am, Baoquan He wrote:
>> On 02/06/20 at 07:50am, Wei Yang wrote:
>> > On Thu, Feb 06, 2020 at 07:19:45AM +0800, Wei Yang wrote:
>> > >On Wed, Feb 05, 2020 at 02:52:51PM +0100, David Hildenbrand wrote:
>> > >>Let's use a calculation that's easier to understand and calculates the
>> > >>same result. Reusing existing macros makes this look nicer.
>> > >>
>> > >>We always want to have the number of pages (> 0) to the next section
>> > >>boundary, starting from the current pfn.
>> > >>
>> > >>Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
>> > >>Cc: Andrew Morton <akpm@linux-foundation.org>
>> > >>Cc: Michal Hocko <mhocko@kernel.org>
>> > >>Cc: Oscar Salvador <osalvador@suse.de>
>> > >>Cc: Baoquan He <bhe@redhat.com>
>> > >>Cc: Wei Yang <richardw.yang@linux.intel.com>
>> > >>Signed-off-by: David Hildenbrand <david@redhat.com>
>> > >
>> > >Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>
>> > >
>> > >BTW, I got one question about hotplug size requirement.
>> > >
>> > >I thought the hotplug range should be section size aligned, while taking a
>> > >look into current code function check_hotplug_memory_range() guard the range.
>> 
>> A good question. The current code should be block size aligned. I
>> remember in some places we assume each block comprise all the sections.
>> Can't imagine one or some of them are half section filled.
>
>I could be wrong, half filled block may not cause problem. 
>

David must be angry about our flooding the mail list :-)

Check the code again, there are two memory range check:

  * check_hotplug_memory_range(), block/section aligned
  * check_pfn_span(), subsection aligned

The second check, check_pfn_span() in __add_pages(), enable the capability to
add a memory range with subsection size.

This means hotplug still keeps section alignment.

BTW, __add_pages() share the same logic as __remove_pages(). Why not change it
too? Do I miss something or I don't have the latest source code?

-- 
Wei Yang
Help you, Help me

Re: [PATCH v1] mm/memory_hotplug: Easier calculation to get pages to next section boundary

[Baoquan He]

On 02/06/20 at 02:26am, Wei Yang wrote:
> On Thu, Feb 06, 2020 at 08:37:36AM +0800, Baoquan He wrote:
> >On 02/06/20 at 08:13am, Baoquan He wrote:
> >> On 02/06/20 at 07:50am, Wei Yang wrote:
> >> > On Thu, Feb 06, 2020 at 07:19:45AM +0800, Wei Yang wrote:
> >> > >On Wed, Feb 05, 2020 at 02:52:51PM +0100, David Hildenbrand wrote:
> >> > >>Let's use a calculation that's easier to understand and calculates the
> >> > >>same result. Reusing existing macros makes this look nicer.
> >> > >>
> >> > >>We always want to have the number of pages (> 0) to the next section
> >> > >>boundary, starting from the current pfn.
> >> > >>
> >> > >>Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
> >> > >>Cc: Andrew Morton <akpm@linux-foundation.org>
> >> > >>Cc: Michal Hocko <mhocko@kernel.org>
> >> > >>Cc: Oscar Salvador <osalvador@suse.de>
> >> > >>Cc: Baoquan He <bhe@redhat.com>
> >> > >>Cc: Wei Yang <richardw.yang@linux.intel.com>
> >> > >>Signed-off-by: David Hildenbrand <david@redhat.com>
> >> > >
> >> > >Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>
> >> > >
> >> > >BTW, I got one question about hotplug size requirement.
> >> > >
> >> > >I thought the hotplug range should be section size aligned, while taking a
> >> > >look into current code function check_hotplug_memory_range() guard the range.
> >> 
> >> A good question. The current code should be block size aligned. I
> >> remember in some places we assume each block comprise all the sections.
> >> Can't imagine one or some of them are half section filled.
> >
> >I could be wrong, half filled block may not cause problem. 
> >
> 
> David must be angry about our flooding the mail list :-)

Believe he won't, :-) If you like, we can talk off line.

> 
> Check the code again, there are two memory range check:
> 
>   * check_hotplug_memory_range(), block/section aligned
>   * check_pfn_span(), subsection aligned
> 
> The second check, check_pfn_span() in __add_pages(), enable the capability to
> add a memory range with subsection size.
> 
> This means hotplug still keeps section alignment.

memremap_pages() also call add_pages(), it doesn't have the
check_hotplug_memory_range() invocation. check_pfn_span() is made for
it specifically.

> 
> BTW, __add_pages() share the same logic as __remove_pages(). Why not change it
> too? Do I miss something or I don't have the latest source code?

Good question, and I think it need. Just David is refactoring/cleaning
up the remove_pages() code path, this is found out by Segher from patch
reviewing.

Re: [PATCH v1] mm/memory_hotplug: Easier calculation to get pages to next section boundary

[Wei Yang]

On Thu, Feb 06, 2020 at 10:48:16AM +0800, Baoquan He wrote:
>On 02/06/20 at 02:26am, Wei Yang wrote:
>> On Thu, Feb 06, 2020 at 08:37:36AM +0800, Baoquan He wrote:
>> >On 02/06/20 at 08:13am, Baoquan He wrote:
>> >> On 02/06/20 at 07:50am, Wei Yang wrote:
>> >> > On Thu, Feb 06, 2020 at 07:19:45AM +0800, Wei Yang wrote:
>> >> > >On Wed, Feb 05, 2020 at 02:52:51PM +0100, David Hildenbrand wrote:
>> >> > >>Let's use a calculation that's easier to understand and calculates the
>> >> > >>same result. Reusing existing macros makes this look nicer.
>> >> > >>
>> >> > >>We always want to have the number of pages (> 0) to the next section
>> >> > >>boundary, starting from the current pfn.
>> >> > >>
>> >> > >>Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
>> >> > >>Cc: Andrew Morton <akpm@linux-foundation.org>
>> >> > >>Cc: Michal Hocko <mhocko@kernel.org>
>> >> > >>Cc: Oscar Salvador <osalvador@suse.de>
>> >> > >>Cc: Baoquan He <bhe@redhat.com>
>> >> > >>Cc: Wei Yang <richardw.yang@linux.intel.com>
>> >> > >>Signed-off-by: David Hildenbrand <david@redhat.com>
>> >> > >
>> >> > >Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>
>> >> > >
>> >> > >BTW, I got one question about hotplug size requirement.
>> >> > >
>> >> > >I thought the hotplug range should be section size aligned, while taking a
>> >> > >look into current code function check_hotplug_memory_range() guard the range.
>> >> 
>> >> A good question. The current code should be block size aligned. I
>> >> remember in some places we assume each block comprise all the sections.
>> >> Can't imagine one or some of them are half section filled.
>> >
>> >I could be wrong, half filled block may not cause problem. 
>> >
>> 
>> David must be angry about our flooding the mail list :-)
>
>Believe he won't, :-) If you like, we can talk off line.
>
>> 
>> Check the code again, there are two memory range check:
>> 
>>   * check_hotplug_memory_range(), block/section aligned
>>   * check_pfn_span(), subsection aligned
>> 
>> The second check, check_pfn_span() in __add_pages(), enable the capability to
>> add a memory range with subsection size.
>> 
>> This means hotplug still keeps section alignment.
>
>memremap_pages() also call add_pages(), it doesn't have the
>check_hotplug_memory_range() invocation. check_pfn_span() is made for
>it specifically.
>

If my understanding is correct, memremap_pages() is used to add some dev
memory to system. This is the use case which Dan want to enable for
sub-section. Since memremap_pages() is not called in mem-hotplug path, this
doesn't affect the hotplug range alignment.

>> 
>> BTW, __add_pages() share the same logic as __remove_pages(). Why not change it
>> too? Do I miss something or I don't have the latest source code?
>
>Good question, and I think it need. Just David is refactoring/cleaning
>up the remove_pages() code path, this is found out by Segher from patch
>reviewing.

Ah, we may need a following cleanup :-)

-- 
Wei Yang
Help you, Help me

Re: [PATCH v1] mm/memory_hotplug: Easier calculation to get pages to next section boundary

[Baoquan He]

On 02/06/20 at 04:34am, Wei Yang wrote:
> On Thu, Feb 06, 2020 at 10:48:16AM +0800, Baoquan He wrote:
> >On 02/06/20 at 02:26am, Wei Yang wrote:
> >> On Thu, Feb 06, 2020 at 08:37:36AM +0800, Baoquan He wrote:
> >> >On 02/06/20 at 08:13am, Baoquan He wrote:
> >> >> On 02/06/20 at 07:50am, Wei Yang wrote:
> >> >> > On Thu, Feb 06, 2020 at 07:19:45AM +0800, Wei Yang wrote:
> >> >> > >On Wed, Feb 05, 2020 at 02:52:51PM +0100, David Hildenbrand wrote:
> >> >> > >>Let's use a calculation that's easier to understand and calculates the
> >> >> > >>same result. Reusing existing macros makes this look nicer.
> >> >> > >>
> >> >> > >>We always want to have the number of pages (> 0) to the next section
> >> >> > >>boundary, starting from the current pfn.
> >> >> > >>
> >> >> > >>Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
> >> >> > >>Cc: Andrew Morton <akpm@linux-foundation.org>
> >> >> > >>Cc: Michal Hocko <mhocko@kernel.org>
> >> >> > >>Cc: Oscar Salvador <osalvador@suse.de>
> >> >> > >>Cc: Baoquan He <bhe@redhat.com>
> >> >> > >>Cc: Wei Yang <richardw.yang@linux.intel.com>
> >> >> > >>Signed-off-by: David Hildenbrand <david@redhat.com>
> >> >> > >
> >> >> > >Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>
> >> >> > >
> >> >> > >BTW, I got one question about hotplug size requirement.
> >> >> > >
> >> >> > >I thought the hotplug range should be section size aligned, while taking a
> >> >> > >look into current code function check_hotplug_memory_range() guard the range.
> >> >> 
> >> >> A good question. The current code should be block size aligned. I
> >> >> remember in some places we assume each block comprise all the sections.
> >> >> Can't imagine one or some of them are half section filled.
> >> >
> >> >I could be wrong, half filled block may not cause problem. 
> >> >
> >> 
> >> David must be angry about our flooding the mail list :-)
> >
> >Believe he won't, :-) If you like, we can talk off line.
> >
> >> 
> >> Check the code again, there are two memory range check:
> >> 
> >>   * check_hotplug_memory_range(), block/section aligned
> >>   * check_pfn_span(), subsection aligned
> >> 
> >> The second check, check_pfn_span() in __add_pages(), enable the capability to
> >> add a memory range with subsection size.
> >> 
> >> This means hotplug still keeps section alignment.
> >
> >memremap_pages() also call add_pages(), it doesn't have the
> >check_hotplug_memory_range() invocation. check_pfn_span() is made for
> >it specifically.
> >
> 
> If my understanding is correct, memremap_pages() is used to add some dev
> memory to system. This is the use case which Dan want to enable for
> sub-section. Since memremap_pages() is not called in mem-hotplug path, this
> doesn't affect the hotplug range alignment.

Yeah, we are on the same page.

> 
> >> 
> >> BTW, __add_pages() share the same logic as __remove_pages(). Why not change it
> >> too? Do I miss something or I don't have the latest source code?
> >
> >Good question, and I think it need. Just David is refactoring/cleaning
> >up the remove_pages() code path, this is found out by Segher from patch
> >reviewing.
> 
> Ah, we may need a following cleanup :-)

Agree. See what David will say. Fold it into this patch, or anyone post
a new one.

Re: [PATCH v1] mm/memory_hotplug: Easier calculation to get pages to next section boundary

[David Hildenbrand]

On 06.02.20 05:39, Baoquan He wrote:
> On 02/06/20 at 04:34am, Wei Yang wrote:
>> On Thu, Feb 06, 2020 at 10:48:16AM +0800, Baoquan He wrote:
>>> On 02/06/20 at 02:26am, Wei Yang wrote:
>>>> On Thu, Feb 06, 2020 at 08:37:36AM +0800, Baoquan He wrote:
>>>>> On 02/06/20 at 08:13am, Baoquan He wrote:
>>>>>> On 02/06/20 at 07:50am, Wei Yang wrote:
>>>>>>> On Thu, Feb 06, 2020 at 07:19:45AM +0800, Wei Yang wrote:
>>>>>>>> On Wed, Feb 05, 2020 at 02:52:51PM +0100, David Hildenbrand wrote:
>>>>>>>>> Let's use a calculation that's easier to understand and calculates the
>>>>>>>>> same result. Reusing existing macros makes this look nicer.
>>>>>>>>>
>>>>>>>>> We always want to have the number of pages (> 0) to the next section
>>>>>>>>> boundary, starting from the current pfn.
>>>>>>>>>
>>>>>>>>> Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
>>>>>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>>>>>> Cc: Michal Hocko <mhocko@kernel.org>
>>>>>>>>> Cc: Oscar Salvador <osalvador@suse.de>
>>>>>>>>> Cc: Baoquan He <bhe@redhat.com>
>>>>>>>>> Cc: Wei Yang <richardw.yang@linux.intel.com>
>>>>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>>>>
>>>>>>>> Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>
>>>>>>>>
>>>>>>>> BTW, I got one question about hotplug size requirement.
>>>>>>>>
>>>>>>>> I thought the hotplug range should be section size aligned, while taking a
>>>>>>>> look into current code function check_hotplug_memory_range() guard the range.
>>>>>>
>>>>>> A good question. The current code should be block size aligned. I
>>>>>> remember in some places we assume each block comprise all the sections.
>>>>>> Can't imagine one or some of them are half section filled.
>>>>>
>>>>> I could be wrong, half filled block may not cause problem. 
>>>>>
>>>>
>>>> David must be angry about our flooding the mail list :-)
>>>
>>> Believe he won't, :-) If you like, we can talk off line.
>>>
>>>>
>>>> Check the code again, there are two memory range check:
>>>>
>>>>   * check_hotplug_memory_range(), block/section aligned
>>>>   * check_pfn_span(), subsection aligned
>>>>
>>>> The second check, check_pfn_span() in __add_pages(), enable the capability to
>>>> add a memory range with subsection size.
>>>>
>>>> This means hotplug still keeps section alignment.
>>>
>>> memremap_pages() also call add_pages(), it doesn't have the
>>> check_hotplug_memory_range() invocation. check_pfn_span() is made for
>>> it specifically.
>>>
>>
>> If my understanding is correct, memremap_pages() is used to add some dev
>> memory to system. This is the use case which Dan want to enable for
>> sub-section. Since memremap_pages() is not called in mem-hotplug path, this
>> doesn't affect the hotplug range alignment.
> 
> Yeah, we are on the same page.

We allow sub-section hoy-add only for device memory/hmm. BIOS often
align PMEM devices to sub-sections, and not supporting this makes life
difficult to support these devices. (You cannot simply cut off something
of a disk :) )

System memory can only be hotplugged in memory block granularity, the
same granularity onlining/offlining from user space will happen. Boot
memory, however, can be sub-section aligned, but can never be offlined
(as it contains holes) and therefore never be removed.

>>
>>>>
>>>> BTW, __add_pages() share the same logic as __remove_pages(). Why not change it
>>>> too? Do I miss something or I don't have the latest source code?
>>>
>>> Good question, and I think it need. Just David is refactoring/cleaning
>>> up the remove_pages() code path, this is found out by Segher from patch
>>> reviewing.
>>
>> Ah, we may need a following cleanup :-)
> 
> Agree. See what David will say. Fold it into this patch, or anyone post
> a new one.
> 

Yes, I only cleaned up __remove_pages() for now. I can send a similar
cleanup for __add_pages().

Will resend this patch, also taking care of __add_pages(). Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH v1] mm/memory_hotplug: Easier calculation to get pages to next section boundary

[Wei Yang]

On Thu, Feb 06, 2020 at 10:01:21AM +0100, David Hildenbrand wrote:
>On 06.02.20 05:39, Baoquan He wrote:
>> On 02/06/20 at 04:34am, Wei Yang wrote:
>>> On Thu, Feb 06, 2020 at 10:48:16AM +0800, Baoquan He wrote:
>>>> On 02/06/20 at 02:26am, Wei Yang wrote:
>>>>> On Thu, Feb 06, 2020 at 08:37:36AM +0800, Baoquan He wrote:
>>>>>> On 02/06/20 at 08:13am, Baoquan He wrote:
>>>>>>> On 02/06/20 at 07:50am, Wei Yang wrote:
>>>>>>>> On Thu, Feb 06, 2020 at 07:19:45AM +0800, Wei Yang wrote:
>>>>>>>>> On Wed, Feb 05, 2020 at 02:52:51PM +0100, David Hildenbrand wrote:
>>>>>>>>>> Let's use a calculation that's easier to understand and calculates the
>>>>>>>>>> same result. Reusing existing macros makes this look nicer.
>>>>>>>>>>
>>>>>>>>>> We always want to have the number of pages (> 0) to the next section
>>>>>>>>>> boundary, starting from the current pfn.
>>>>>>>>>>
>>>>>>>>>> Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
>>>>>>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>>>>>>> Cc: Michal Hocko <mhocko@kernel.org>
>>>>>>>>>> Cc: Oscar Salvador <osalvador@suse.de>
>>>>>>>>>> Cc: Baoquan He <bhe@redhat.com>
>>>>>>>>>> Cc: Wei Yang <richardw.yang@linux.intel.com>
>>>>>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>>>>>
>>>>>>>>> Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>
>>>>>>>>>
>>>>>>>>> BTW, I got one question about hotplug size requirement.
>>>>>>>>>
>>>>>>>>> I thought the hotplug range should be section size aligned, while taking a
>>>>>>>>> look into current code function check_hotplug_memory_range() guard the range.
>>>>>>>
>>>>>>> A good question. The current code should be block size aligned. I
>>>>>>> remember in some places we assume each block comprise all the sections.
>>>>>>> Can't imagine one or some of them are half section filled.
>>>>>>
>>>>>> I could be wrong, half filled block may not cause problem. 
>>>>>>
>>>>>
>>>>> David must be angry about our flooding the mail list :-)
>>>>
>>>> Believe he won't, :-) If you like, we can talk off line.
>>>>
>>>>>
>>>>> Check the code again, there are two memory range check:
>>>>>
>>>>>   * check_hotplug_memory_range(), block/section aligned
>>>>>   * check_pfn_span(), subsection aligned
>>>>>
>>>>> The second check, check_pfn_span() in __add_pages(), enable the capability to
>>>>> add a memory range with subsection size.
>>>>>
>>>>> This means hotplug still keeps section alignment.
>>>>
>>>> memremap_pages() also call add_pages(), it doesn't have the
>>>> check_hotplug_memory_range() invocation. check_pfn_span() is made for
>>>> it specifically.
>>>>
>>>
>>> If my understanding is correct, memremap_pages() is used to add some dev
>>> memory to system. This is the use case which Dan want to enable for
>>> sub-section. Since memremap_pages() is not called in mem-hotplug path, this
>>> doesn't affect the hotplug range alignment.
>> 
>> Yeah, we are on the same page.
>
>We allow sub-section hoy-add only for device memory/hmm. BIOS often
>align PMEM devices to sub-sections, and not supporting this makes life
>difficult to support these devices. (You cannot simply cut off something
>of a disk :) )
>
>System memory can only be hotplugged in memory block granularity, the
>same granularity onlining/offlining from user space will happen. Boot
>memory, however, can be sub-section aligned, but can never be offlined
>(as it contains holes) and therefore never be removed.
>

This makes life easier.

Thanks for your explanation.

-- 
Wei Yang
Help you, Help me