[PATCH v1 0/5] mm/memory_hotplug: online/offline_pages refactorings

[PATCH v1 0/5] mm/memory_hotplug: online/offline_pages refactorings

[David Hildenbrand]

While looking into onlining/offlining of subsections, I noticed that
online/offlining code can in its current form only deal with whole sections
and that onlining/offlining of sections that are already online/offline is
problematic. So let's add some additional checks (that also serve as
implicit documentation) and do some cleanups.

David Hildenbrand (5):
  mm/memory_hotplug: drop intermediate __offline_pages
  mm/memory_hotplug: enforce section alignment when onlining/offlining
  mm/memory_hotplug: check if sections are already online/offline
  mm/memory_hotplug: onlining pages can only fail due to notifiers
  mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()

 include/linux/mmzone.h |  2 ++
 mm/memory_hotplug.c    | 43 ++++++++++++++++++++++--------------------
 mm/sparse.c            | 28 +++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH v1 1/5] mm/memory_hotplug: drop intermediate __offline_pages

[David Hildenbrand]

Let's avoid this indirection and just call the function offline_pages().

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 mm/memory_hotplug.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 6a2726920ed2..090cf474de87 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1589,10 +1589,10 @@ static void node_states_clear_node(int node, struct memory_notify *arg)
 		node_clear_state(node, N_MEMORY);
 }
 
-static int __ref __offline_pages(unsigned long start_pfn,
-		  unsigned long end_pfn)
+/* Must be protected by mem_hotplug_begin() or a device_lock */
+int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 {
-	unsigned long pfn, nr_pages;
+	unsigned long pfn, end_pfn = start_pfn + nr_pages;
 	long offlined_pages;
 	int ret, node;
 	unsigned long flags;
@@ -1612,7 +1612,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
 
 	zone = page_zone(pfn_to_page(valid_start));
 	node = zone_to_nid(zone);
-	nr_pages = end_pfn - start_pfn;
 
 	/* set above range as isolated */
 	ret = start_isolate_page_range(start_pfn, end_pfn,
@@ -1700,12 +1699,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
 	return ret;
 }
-
-/* Must be protected by mem_hotplug_begin() or a device_lock */
-int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
-{
-	return __offline_pages(start_pfn, start_pfn + nr_pages);
-}
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
 /**
-- 
2.17.1

[PATCH v1 2/5] mm/memory_hotplug: enforce section alignment when onlining/offlining

[David Hildenbrand]

onlining/offlining code works on whole sections, so let's enforce that.
Existing code only allows to add memory in memory block size. And only
whole memory blocks can be onlined/offlined. Memory blocks are always
aligned to sections, so this should not break anything.

online_pages/offline_pages will implicitly mark whole sections
online/offline, so the code really can only handle such granularities.

(especially offlining code cannot deal with pageblock_nr_pages but
 theoretically only MAX_ORDER-1)

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 mm/memory_hotplug.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 090cf474de87..30d2fa42b0bb 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -897,6 +897,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	struct memory_notify arg;
 	struct memory_block *mem;
 
+	if (!IS_ALIGNED(pfn, PAGES_PER_SECTION))
+		return -EINVAL;
+	if (!IS_ALIGNED(nr_pages, PAGES_PER_SECTION))
+		return -EINVAL;
+
 	/*
 	 * We can't use pfn_to_nid() because nid might be stored in struct page
 	 * which is not yet initialized. Instead, we find nid from memory block.
@@ -1600,10 +1605,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 	struct zone *zone;
 	struct memory_notify arg;
 
-	/* at least, alignment against pageblock is necessary */
-	if (!IS_ALIGNED(start_pfn, pageblock_nr_pages))
+	if (!IS_ALIGNED(start_pfn, PAGES_PER_SECTION))
 		return -EINVAL;
-	if (!IS_ALIGNED(end_pfn, pageblock_nr_pages))
+	if (!IS_ALIGNED(nr_pages, PAGES_PER_SECTION))
 		return -EINVAL;
 	/* This makes hotplug much easier...and readable.
 	   we assume this for now. .*/
-- 
2.17.1

[PATCH v1 3/5] mm/memory_hotplug: check if sections are already online/offline

[David Hildenbrand]

Let's add some more sanity checking now that onlining/offlining code
works completely on section basis. This will make sure that we will
never try to online/offline sections that are already (or partially) in
the desired state.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/linux/mmzone.h |  2 ++
 mm/memory_hotplug.c    |  5 +++++
 mm/sparse.c            | 28 ++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 0859130e4db8..addfa41c047a 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -1228,6 +1228,8 @@ static inline int online_section_nr(unsigned long nr)
 }
 
 #ifdef CONFIG_MEMORY_HOTPLUG
+bool mem_sections_online(unsigned long pfn, unsigned long end_pfn);
+bool mem_sections_offline(unsigned long pfn, unsigned long end_pfn);
 void online_mem_sections(unsigned long start_pfn, unsigned long end_pfn);
 #ifdef CONFIG_MEMORY_HOTREMOVE
 void offline_mem_sections(unsigned long start_pfn, unsigned long end_pfn);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 30d2fa42b0bb..3dc6d2a309c2 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -901,6 +901,8 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 		return -EINVAL;
 	if (!IS_ALIGNED(nr_pages, PAGES_PER_SECTION))
 		return -EINVAL;
+	if (!mem_sections_offline(pfn, pfn + nr_pages))
+		return -EINVAL;
 
 	/*
 	 * We can't use pfn_to_nid() because nid might be stored in struct page
@@ -1609,6 +1611,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 		return -EINVAL;
 	if (!IS_ALIGNED(nr_pages, PAGES_PER_SECTION))
 		return -EINVAL;
+	if (!mem_sections_online(start_pfn, end_pfn))
+		return -EINVAL;
+
 	/* This makes hotplug much easier...and readable.
 	   we assume this for now. .*/
 	if (!test_pages_in_a_zone(start_pfn, end_pfn, &valid_start, &valid_end))
diff --git a/mm/sparse.c b/mm/sparse.c
index 10b07eea9a6e..44693cf38ca9 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -520,6 +520,34 @@ void __init sparse_init(void)
 
 #ifdef CONFIG_MEMORY_HOTPLUG
 
+/* check if all mem sections are online */
+bool mem_sections_online(unsigned long pfn, unsigned long end_pfn)
+{
+	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
+		unsigned long section_nr = pfn_to_section_nr(pfn);
+
+		if (WARN_ON(!valid_section_nr(section_nr)))
+			continue;
+		if (!online_section_nr(section_nr))
+			return false;
+	}
+	return true;
+}
+
+/* check if all mem sections are offline */
+bool mem_sections_offline(unsigned long pfn, unsigned long end_pfn)
+{
+	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
+		unsigned long section_nr = pfn_to_section_nr(pfn);
+
+		if (WARN_ON(!valid_section_nr(section_nr)))
+			continue;
+		if (online_section_nr(section_nr))
+			return false;
+	}
+	return true;
+}
+
 /* Mark all memory sections within the pfn range as online */
 void online_mem_sections(unsigned long start_pfn, unsigned long end_pfn)
 {
-- 
2.17.1

[PATCH v1 4/5] mm/memory_hotplug: onlining pages can only fail due to notifiers

[David Hildenbrand]

Onlining pages can only fail if a notifier reported a problem (e.g. -ENOMEM).
online_pages_range() can never fail.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 mm/memory_hotplug.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 3dc6d2a309c2..bbbd16f9d877 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -933,13 +933,8 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 		setup_zone_pageset(zone);
 	}
 
-	ret = walk_system_ram_range(pfn, nr_pages, &onlined_pages,
-		online_pages_range);
-	if (ret) {
-		if (need_zonelists_rebuild)
-			zone_pcp_reset(zone);
-		goto failed_addition;
-	}
+	walk_system_ram_range(pfn, nr_pages, &onlined_pages,
+			      online_pages_range);
 
 	zone->present_pages += onlined_pages;
 
-- 
2.17.1

[PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()

[David Hildenbrand]

Let's try to minimze the noise.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 mm/memory_hotplug.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index bbbd16f9d877..6fec2dc6a73d 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -966,9 +966,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	return 0;
 
 failed_addition:
+#ifdef CONFIG_DEBUG_VM
 	pr_debug("online_pages [mem %#010llx-%#010llx] failed\n",
 		 (unsigned long long) pfn << PAGE_SHIFT,
 		 (((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
+#endif
 	memory_notify(MEM_CANCEL_ONLINE, &arg);
 	return ret;
 }
@@ -1660,7 +1662,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 	offlined_pages = check_pages_isolated(start_pfn, end_pfn);
 	if (offlined_pages < 0)
 		goto repeat;
+#ifdef CONFIG_DEBUG_VM
 	pr_info("Offlined Pages %ld\n", offlined_pages);
+#endif
 	/* Ok, all of our target is isolated.
 	   We cannot do rollback at this point. */
 	offline_isolated_pages(start_pfn, end_pfn);
@@ -1695,9 +1699,11 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 	return 0;
 
 failed_removal:
+#ifdef CONFIG_DEBUG_VM
 	pr_debug("memory offlining [mem %#010llx-%#010llx] failed\n",
 		 (unsigned long long) start_pfn << PAGE_SHIFT,
 		 ((unsigned long long) end_pfn << PAGE_SHIFT) - 1);
+#endif
 	memory_notify(MEM_CANCEL_OFFLINE, &arg);
 	/* pushback to free area */
 	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
-- 
2.17.1

Re: [PATCH v1 3/5] mm/memory_hotplug: check if sections are already online/offline

[Oscar Salvador]

On Thu, Aug 16, 2018 at 12:06:26PM +0200, David Hildenbrand wrote:

> +
> +/* check if all mem sections are offline */
> +bool mem_sections_offline(unsigned long pfn, unsigned long end_pfn)
> +{
> +	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
> +		unsigned long section_nr = pfn_to_section_nr(pfn);
> +
> +		if (WARN_ON(!valid_section_nr(section_nr)))
> +			continue;
> +		if (online_section_nr(section_nr))
> +			return false;
> +	}
> +	return true;
> +}

AFAICS pages_correctly_probed will catch this first.
pages_correctly_probed checks for the section to be:

- present
- valid
- !online

Maybe it makes sense to rename it, and write another pages_correctly_probed routine
for the offline case.

So all checks would stay in memory_block_action level, and we would not need
the mem_sections_offline/online stuff.

Thanks
-- 
Oscar Salvador
SUSE L3

Re: [PATCH v1 3/5] mm/memory_hotplug: check if sections are already online/offline

[David Hildenbrand]

On 16.08.2018 12:47, Oscar Salvador wrote:
> On Thu, Aug 16, 2018 at 12:06:26PM +0200, David Hildenbrand wrote:
> 
>> +
>> +/* check if all mem sections are offline */
>> +bool mem_sections_offline(unsigned long pfn, unsigned long end_pfn)
>> +{
>> +	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
>> +		unsigned long section_nr = pfn_to_section_nr(pfn);
>> +
>> +		if (WARN_ON(!valid_section_nr(section_nr)))
>> +			continue;
>> +		if (online_section_nr(section_nr))
>> +			return false;
>> +	}
>> +	return true;
>> +}
> 
> AFAICS pages_correctly_probed will catch this first.
> pages_correctly_probed checks for the section to be:
> 
> - present
> - valid
> - !online

Right, I missed that function.

> 
> Maybe it makes sense to rename it, and write another pages_correctly_probed routine
> for the offline case.
> 
> So all checks would stay in memory_block_action level, and we would not need
> the mem_sections_offline/online stuff.

I guess I would rather have it all moved into
online_pages/offline_pages, so we have a clean interface.

> 
> Thanks
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v1 1/5] mm/memory_hotplug: drop intermediate __offline_pages

[Stephen Rothwell]

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

Hi David,

On Thu, 16 Aug 2018 12:06:24 +0200 David Hildenbrand <david@redhat.com> wrote:
>
> -static int __ref __offline_pages(unsigned long start_pfn,
> -		  unsigned long end_pfn)
> +/* Must be protected by mem_hotplug_begin() or a device_lock */
> +int offline_pages(unsigned long start_pfn, unsigned long nr_pages)

You lose the __ref marking.  Does this introduce warnings since
offline_pages() calls (at least) zone_pcp_update() which is marked
__meminit.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/5] mm/memory_hotplug: drop intermediate __offline_pages

[David Hildenbrand]

On 16.08.2018 13:44, Stephen Rothwell wrote:
> Hi David,
> 
> On Thu, 16 Aug 2018 12:06:24 +0200 David Hildenbrand <david@redhat.com> wrote:
>>
>> -static int __ref __offline_pages(unsigned long start_pfn,
>> -		  unsigned long end_pfn)
>> +/* Must be protected by mem_hotplug_begin() or a device_lock */
>> +int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
> 
> You lose the __ref marking.  Does this introduce warnings since
> offline_pages() calls (at least) zone_pcp_update() which is marked
> __meminit.
> 

Good point, I'll recompile and in case there is a warning, keep the
__ref. Thanks!

-- 

Thanks,

David / dhildenb

Re: [PATCH v1 2/5] mm/memory_hotplug: enforce section alignment when onlining/offlining

[Oscar Salvador]

On Thu, Aug 16, 2018 at 12:06:25PM +0200, David Hildenbrand wrote:
> onlining/offlining code works on whole sections, so let's enforce that.
> Existing code only allows to add memory in memory block size. And only
> whole memory blocks can be onlined/offlined. Memory blocks are always
> aligned to sections, so this should not break anything.
> 
> online_pages/offline_pages will implicitly mark whole sections
> online/offline, so the code really can only handle such granularities.
> 
> (especially offlining code cannot deal with pageblock_nr_pages but
>  theoretically only MAX_ORDER-1)
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Hi David,

If you are really willing to move the checks from this patch[1] to
online/offline_pages, you might consider to put that in as well.
So we have a function that checks for everything, and not multiple checks.

Another thing is that I would have prefered to take the checks up to
memory_block_action, but offline_pages gets also called from ppc-memtrace code.

Other than that, 

Reviewed-by: Oscar Salvador <osalvador@suse.de>


[1] https://patchwork.kernel.org/patch/10567277/

Thanks
-- 
Oscar Salvador
SUSE L3

Re: [PATCH v1 1/5] mm/memory_hotplug: drop intermediate __offline_pages

[kbuild test robot]

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

Hi David,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.18 next-20180816]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/David-Hildenbrand/mm-memory_hotplug-online-offline_pages-refactorings/20180817-002307
config: i386-randconfig-a0-201832 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

>> WARNING: vmlinux.o(.text+0x175eea): Section mismatch in reference from the function offline_pages() to the function .meminit.text:init_per_zone_wmark_min()
   The function offline_pages() references
   the function __meminit init_per_zone_wmark_min().
   This is often because offline_pages lacks a __meminit
   annotation or the annotation of init_per_zone_wmark_min is wrong.
--
>> WARNING: vmlinux.o(.text+0x17624f): Section mismatch in reference from the function offline_pages() to the function .meminit.text:zone_pcp_update()
   The function offline_pages() references
   the function __meminit zone_pcp_update().
   This is often because offline_pages lacks a __meminit
   annotation or the annotation of zone_pcp_update is wrong.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32364 bytes --]

Re: [PATCH v1 1/5] mm/memory_hotplug: drop intermediate __offline_pages

[kbuild test robot]

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

Hi David,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.18 next-20180816]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/David-Hildenbrand/mm-memory_hotplug-online-offline_pages-refactorings/20180817-002307
config: i386-randconfig-a0-201832 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

>> WARNING: vmlinux.o(.text+0x175eea): Section mismatch in reference from the function offline_pages() to the function .meminit.text:init_per_zone_wmark_min()
   The function offline_pages() references
   the function __meminit init_per_zone_wmark_min().
   This is often because offline_pages lacks a __meminit
   annotation or the annotation of init_per_zone_wmark_min is wrong.
--
>> WARNING: vmlinux.o(.text+0x17624f): Section mismatch in reference from the function offline_pages() to the function .meminit.text:zone_pcp_update()
   The function offline_pages() references
   the function __meminit zone_pcp_update().
   This is often because offline_pages lacks a __meminit
   annotation or the annotation of zone_pcp_update is wrong.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32364 bytes --]

Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()

[Oscar Salvador]

>  failed_addition:
> +#ifdef CONFIG_DEBUG_VM
>  	pr_debug("online_pages [mem %#010llx-%#010llx] failed\n",
>  		 (unsigned long long) pfn << PAGE_SHIFT,
>  		 (((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
> +#endif

I have never been sure about this.
IMO, if I fail to online pages, I want to know I failed.
I think that pr_err would be better than pr_debug and without CONFIG_DEBUG_VM.

But at least, if not, envolve it with a CONFIG_DEBUG_VM, but change pr_debug to pr_info.

> +#ifdef CONFIG_DEBUG_VM
>  	pr_debug("memory offlining [mem %#010llx-%#010llx] failed\n",
>  		 (unsigned long long) start_pfn << PAGE_SHIFT,
>  		 ((unsigned long long) end_pfn << PAGE_SHIFT) - 1);
> +#endif

Same goes here.

Thanks
-- 
Oscar Salvador
SUSE L3

Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()

[Wei Yang]

On Fri, Aug 17, 2018 at 10:18:53AM +0200, Oscar Salvador wrote:
>>  failed_addition:
>> +#ifdef CONFIG_DEBUG_VM
>>  	pr_debug("online_pages [mem %#010llx-%#010llx] failed\n",
>>  		 (unsigned long long) pfn << PAGE_SHIFT,
>>  		 (((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
>> +#endif
>
>I have never been sure about this.
>IMO, if I fail to online pages, I want to know I failed.
>I think that pr_err would be better than pr_debug and without CONFIG_DEBUG_VM.
>
>But at least, if not, envolve it with a CONFIG_DEBUG_VM, but change pr_debug to pr_info.
>

I don't have a clear rule about these debug macro neither.

While when you look at the page related logs in calculate_node_totalpages(),
it is KERNEL_DEBUG level and without any config macro.

Maybe we should leave them at the same state?

>> +#ifdef CONFIG_DEBUG_VM
>>  	pr_debug("memory offlining [mem %#010llx-%#010llx] failed\n",
>>  		 (unsigned long long) start_pfn << PAGE_SHIFT,
>>  		 ((unsigned long long) end_pfn << PAGE_SHIFT) - 1);
>> +#endif
>
>Same goes here.
>
>Thanks
>-- 
>Oscar Salvador
>SUSE L3

-- 
Wei Yang
Help you, Help me

Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()

[David Hildenbrand]

On 17.08.2018 10:18, Oscar Salvador wrote:
>>  failed_addition:
>> +#ifdef CONFIG_DEBUG_VM
>>  	pr_debug("online_pages [mem %#010llx-%#010llx] failed\n",
>>  		 (unsigned long long) pfn << PAGE_SHIFT,
>>  		 (((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
>> +#endif
> 
> I have never been sure about this.
> IMO, if I fail to online pages, I want to know I failed.
> I think that pr_err would be better than pr_debug and without CONFIG_DEBUG_VM.

I consider both error messages only partially useful, as

1. They only catch a subset of actual failures the function handles.
   E.g. onlining will not report an error message if the memory notifier
   failed.
2. Onlining/Offlining is usually (with exceptions - e.g. onlining during
   add_memory) triggered from user space, where we present an error
   code. At any times, the actual state of the memory blocks can be
   observed by querying the state.

I would even vote for dropping the two error case messages completely.
At least I don't consider them very useful.

> 
> But at least, if not, envolve it with a CONFIG_DEBUG_VM, but change pr_debug to pr_info.
> 
>> +#ifdef CONFIG_DEBUG_VM
>>  	pr_debug("memory offlining [mem %#010llx-%#010llx] failed\n",
>>  		 (unsigned long long) start_pfn << PAGE_SHIFT,
>>  		 ((unsigned long long) end_pfn << PAGE_SHIFT) - 1);
>> +#endif
> 
> Same goes here.
> 
> Thanks
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()

[David Hildenbrand]

On 20.08.2018 11:45, David Hildenbrand wrote:
> On 17.08.2018 10:18, Oscar Salvador wrote:
>>>  failed_addition:
>>> +#ifdef CONFIG_DEBUG_VM
>>>  	pr_debug("online_pages [mem %#010llx-%#010llx] failed\n",
>>>  		 (unsigned long long) pfn << PAGE_SHIFT,
>>>  		 (((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
>>> +#endif
>>
>> I have never been sure about this.
>> IMO, if I fail to online pages, I want to know I failed.
>> I think that pr_err would be better than pr_debug and without CONFIG_DEBUG_VM.
> 
> I consider both error messages only partially useful, as
> 
> 1. They only catch a subset of actual failures the function handles.
>    E.g. onlining will not report an error message if the memory notifier
>    failed.

That statement was wrong. It is rather in offline_pages, errors from
start_isolate_page_range() are ignored.

> 2. Onlining/Offlining is usually (with exceptions - e.g. onlining during
>    add_memory) triggered from user space, where we present an error
>    code. At any times, the actual state of the memory blocks can be
>    observed by querying the state.
> 
> I would even vote for dropping the two error case messages completely.
> At least I don't consider them very useful.
> 
>>
>> But at least, if not, envolve it with a CONFIG_DEBUG_VM, but change pr_debug to pr_info.
>>
>>> +#ifdef CONFIG_DEBUG_VM
>>>  	pr_debug("memory offlining [mem %#010llx-%#010llx] failed\n",
>>>  		 (unsigned long long) start_pfn << PAGE_SHIFT,
>>>  		 ((unsigned long long) end_pfn << PAGE_SHIFT) - 1);
>>> +#endif
>>
>> Same goes here.
>>
>> Thanks
>>
> 
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()

[David Hildenbrand]

On 19.08.2018 14:34, Wei Yang wrote:
> On Fri, Aug 17, 2018 at 10:18:53AM +0200, Oscar Salvador wrote:
>>>  failed_addition:
>>> +#ifdef CONFIG_DEBUG_VM
>>>  	pr_debug("online_pages [mem %#010llx-%#010llx] failed\n",
>>>  		 (unsigned long long) pfn << PAGE_SHIFT,
>>>  		 (((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
>>> +#endif
>>
>> I have never been sure about this.
>> IMO, if I fail to online pages, I want to know I failed.
>> I think that pr_err would be better than pr_debug and without CONFIG_DEBUG_VM.
>>
>> But at least, if not, envolve it with a CONFIG_DEBUG_VM, but change pr_debug to pr_info.
>>
> 
> I don't have a clear rule about these debug macro neither.
> 
> While when you look at the page related logs in calculate_node_totalpages(),
> it is KERNEL_DEBUG level and without any config macro.
> 
> Maybe we should leave them at the same state?

I guess we can do that for the to debug messages.

When offlining memory right now:

:/# echo 0 > /sys/devices/system/memory/memory9/online
[   24.476207] Offlined Pages 32768
[   24.477200] remove from free list 48000 1024 50000
[   24.477896] remove from free list 48400 1024 50000
[   24.478584] remove from free list 48800 1024 50000
[   24.479454] remove from free list 48c00 1024 50000
[   24.480192] remove from free list 49000 1024 50000
[   24.480957] remove from free list 49400 1024 50000
[   24.481752] remove from free list 49800 1024 50000
[   24.482578] remove from free list 49c00 1024 50000
[   24.483302] remove from free list 4a000 1024 50000
[   24.484300] remove from free list 4a400 1024 50000
[   24.484902] remove from free list 4a800 1024 50000
[   24.485462] remove from free list 4ac00 1024 50000
[   24.486381] remove from free list 4b000 1024 50000
[   24.487108] remove from free list 4b400 1024 50000
[   24.487842] remove from free list 4b800 1024 50000
[   24.488610] remove from free list 4bc00 1024 50000
[   24.489548] remove from free list 4c000 1024 50000
[   24.490392] remove from free list 4c400 1024 50000
[   24.491224] remove from free list 4c800 1024 50000
...

While "remove from free list" is pr_info under CONFIG_DEBUG_VM,
"Offlined Pages ..." is pr_info without CONFIG_DEBUG_VM.

-- 

Thanks,

David / dhildenb

Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()

[David Hildenbrand]

On 16.08.2018 12:06, David Hildenbrand wrote:
> Let's try to minimze the noise.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  mm/memory_hotplug.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index bbbd16f9d877..6fec2dc6a73d 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -966,9 +966,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>  	return 0;
>  
>  failed_addition:
> +#ifdef CONFIG_DEBUG_VM
>  	pr_debug("online_pages [mem %#010llx-%#010llx] failed\n",
>  		 (unsigned long long) pfn << PAGE_SHIFT,
>  		 (((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
> +#endif
>  	memory_notify(MEM_CANCEL_ONLINE, &arg);
>  	return ret;
>  }
> @@ -1660,7 +1662,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
>  	offlined_pages = check_pages_isolated(start_pfn, end_pfn);
>  	if (offlined_pages < 0)
>  		goto repeat;
> +#ifdef CONFIG_DEBUG_VM
>  	pr_info("Offlined Pages %ld\n", offlined_pages);
> +#endif
>  	/* Ok, all of our target is isolated.
>  	   We cannot do rollback at this point. */
>  	offline_isolated_pages(start_pfn, end_pfn);
> @@ -1695,9 +1699,11 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
>  	return 0;
>  
>  failed_removal:
> +#ifdef CONFIG_DEBUG_VM
>  	pr_debug("memory offlining [mem %#010llx-%#010llx] failed\n",
>  		 (unsigned long long) start_pfn << PAGE_SHIFT,
>  		 ((unsigned long long) end_pfn << PAGE_SHIFT) - 1);
> +#endif
>  	memory_notify(MEM_CANCEL_OFFLINE, &arg);
>  	/* pushback to free area */
>  	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
> 

I'll drop this patch for now, maybe the error messages are actually
useful when debugging a crashdump of a system without CONFIG_DEBUG_VM.

-- 

Thanks,

David / dhildenb

Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()

[Wei Yang]

On Mon, Aug 20, 2018 at 11:57:04AM +0200, David Hildenbrand wrote:
>On 19.08.2018 14:34, Wei Yang wrote:
>> On Fri, Aug 17, 2018 at 10:18:53AM +0200, Oscar Salvador wrote:
>>>>  failed_addition:
>>>> +#ifdef CONFIG_DEBUG_VM
>>>>  	pr_debug("online_pages [mem %#010llx-%#010llx] failed\n",
>>>>  		 (unsigned long long) pfn << PAGE_SHIFT,
>>>>  		 (((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
>>>> +#endif
>>>
>>> I have never been sure about this.
>>> IMO, if I fail to online pages, I want to know I failed.
>>> I think that pr_err would be better than pr_debug and without CONFIG_DEBUG_VM.
>>>
>>> But at least, if not, envolve it with a CONFIG_DEBUG_VM, but change pr_debug to pr_info.
>>>
>> 
>> I don't have a clear rule about these debug macro neither.
>> 
>> While when you look at the page related logs in calculate_node_totalpages(),
>> it is KERNEL_DEBUG level and without any config macro.
>> 
>> Maybe we should leave them at the same state?
>
>I guess we can do that for the to debug messages.
>
>When offlining memory right now:
>
>:/# echo 0 > /sys/devices/system/memory/memory9/online
>[   24.476207] Offlined Pages 32768
>[   24.477200] remove from free list 48000 1024 50000
>[   24.477896] remove from free list 48400 1024 50000
>[   24.478584] remove from free list 48800 1024 50000
>[   24.479454] remove from free list 48c00 1024 50000
>[   24.480192] remove from free list 49000 1024 50000
>[   24.480957] remove from free list 49400 1024 50000
>[   24.481752] remove from free list 49800 1024 50000
>[   24.482578] remove from free list 49c00 1024 50000
>[   24.483302] remove from free list 4a000 1024 50000
>[   24.484300] remove from free list 4a400 1024 50000
>[   24.484902] remove from free list 4a800 1024 50000
>[   24.485462] remove from free list 4ac00 1024 50000
>[   24.486381] remove from free list 4b000 1024 50000
>[   24.487108] remove from free list 4b400 1024 50000
>[   24.487842] remove from free list 4b800 1024 50000
>[   24.488610] remove from free list 4bc00 1024 50000
>[   24.489548] remove from free list 4c000 1024 50000
>[   24.490392] remove from free list 4c400 1024 50000
>[   24.491224] remove from free list 4c800 1024 50000
>...
>
>While "remove from free list" is pr_info under CONFIG_DEBUG_VM,
>"Offlined Pages ..." is pr_info without CONFIG_DEBUG_VM.

Hmm... yes, don't see the logic between them.

>
>-- 
>
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

Re: [PATCH v1 1/5] mm/memory_hotplug: drop intermediate __offline_pages

[Pasha Tatashin]

I guess the wrap was done because of __ref, but no reason to have this
wrap. So looks good to me.

Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com>

On 8/16/18 6:06 AM, David Hildenbrand wrote:
> Let's avoid this indirection and just call the function offline_pages().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  mm/memory_hotplug.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 6a2726920ed2..090cf474de87 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1589,10 +1589,10 @@ static void node_states_clear_node(int node, struct memory_notify *arg)
>  		node_clear_state(node, N_MEMORY);
>  }
>  
> -static int __ref __offline_pages(unsigned long start_pfn,
> -		  unsigned long end_pfn)
> +/* Must be protected by mem_hotplug_begin() or a device_lock */
> +int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
>  {
> -	unsigned long pfn, nr_pages;
> +	unsigned long pfn, end_pfn = start_pfn + nr_pages;
>  	long offlined_pages;
>  	int ret, node;
>  	unsigned long flags;
> @@ -1612,7 +1612,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
>  
>  	zone = page_zone(pfn_to_page(valid_start));
>  	node = zone_to_nid(zone);
> -	nr_pages = end_pfn - start_pfn;
>  
>  	/* set above range as isolated */
>  	ret = start_isolate_page_range(start_pfn, end_pfn,
> @@ -1700,12 +1699,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
>  	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
>  	return ret;
>  }
> -
> -/* Must be protected by mem_hotplug_begin() or a device_lock */
> -int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
> -{
> -	return __offline_pages(start_pfn, start_pfn + nr_pages);
> -}
>  #endif /* CONFIG_MEMORY_HOTREMOVE */
>  
>  /**
> 

Re: [PATCH v1 1/5] mm/memory_hotplug: drop intermediate __offline_pages

[Pasha Tatashin]

On 8/30/18 4:17 PM, Pasha Tatashin wrote:
> I guess the wrap was done because of __ref, but no reason to have this
> wrap. So looks good to me.
> 
> Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com>>
> On 8/16/18 6:06 AM, David Hildenbrand wrote:
>> Let's avoid this indirection and just call the function offline_pages().
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  mm/memory_hotplug.c | 13 +++----------
>>  1 file changed, 3 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 6a2726920ed2..090cf474de87 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1589,10 +1589,10 @@ static void node_states_clear_node(int node, struct memory_notify *arg)
>>  		node_clear_state(node, N_MEMORY);
>>  }
>>  
>> -static int __ref __offline_pages(unsigned long start_pfn,
>> -		  unsigned long end_pfn)
>> +/* Must be protected by mem_hotplug_begin() or a device_lock */
>> +int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
      ^^^
I meant to say keep the __ref, otherwise looks good.

Thank you,
Pavel

Re: [PATCH v1 2/5] mm/memory_hotplug: enforce section alignment when onlining/offlining

[Pasha Tatashin]

Hi David,

I am not sure this is needed, because we already have a stricter checker:

check_hotplug_memory_range()

You could call it from online_pages(), if you think there is a reason to
do it, but other than that it is done from add_memory_resource() and
from remove_memory().

Thank you,
Pavel

On 8/16/18 6:06 AM, David Hildenbrand wrote:
> onlining/offlining code works on whole sections, so let's enforce that.
> Existing code only allows to add memory in memory block size. And only
> whole memory blocks can be onlined/offlined. Memory blocks are always
> aligned to sections, so this should not break anything.
> 
> online_pages/offline_pages will implicitly mark whole sections
> online/offline, so the code really can only handle such granularities.
> 
> (especially offlining code cannot deal with pageblock_nr_pages but
>  theoretically only MAX_ORDER-1)
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  mm/memory_hotplug.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 090cf474de87..30d2fa42b0bb 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -897,6 +897,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>  	struct memory_notify arg;
>  	struct memory_block *mem;
>  
> +	if (!IS_ALIGNED(pfn, PAGES_PER_SECTION))
> +		return -EINVAL;
> +	if (!IS_ALIGNED(nr_pages, PAGES_PER_SECTION))
> +		return -EINVAL;
> +
>  	/*
>  	 * We can't use pfn_to_nid() because nid might be stored in struct page
>  	 * which is not yet initialized. Instead, we find nid from memory block.
> @@ -1600,10 +1605,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
>  	struct zone *zone;
>  	struct memory_notify arg;
>  
> -	/* at least, alignment against pageblock is necessary */
> -	if (!IS_ALIGNED(start_pfn, pageblock_nr_pages))
> +	if (!IS_ALIGNED(start_pfn, PAGES_PER_SECTION))
>  		return -EINVAL;
> -	if (!IS_ALIGNED(end_pfn, pageblock_nr_pages))
> +	if (!IS_ALIGNED(nr_pages, PAGES_PER_SECTION))
>  		return -EINVAL;
>  	/* This makes hotplug much easier...and readable.
>  	   we assume this for now. .*/
> 

Re: [PATCH v1 3/5] mm/memory_hotplug: check if sections are already online/offline

[Pasha Tatashin]

On 8/16/18 7:00 AM, David Hildenbrand wrote:
> On 16.08.2018 12:47, Oscar Salvador wrote:
>> On Thu, Aug 16, 2018 at 12:06:26PM +0200, David Hildenbrand wrote:
>>
>>> +
>>> +/* check if all mem sections are offline */
>>> +bool mem_sections_offline(unsigned long pfn, unsigned long end_pfn)
>>> +{
>>> +	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
>>> +		unsigned long section_nr = pfn_to_section_nr(pfn);
>>> +
>>> +		if (WARN_ON(!valid_section_nr(section_nr)))
>>> +			continue;
>>> +		if (online_section_nr(section_nr))
>>> +			return false;
>>> +	}
>>> +	return true;
>>> +}
>>
>> AFAICS pages_correctly_probed will catch this first.
>> pages_correctly_probed checks for the section to be:
>>
>> - present
>> - valid
>> - !online
> 
> Right, I missed that function.
> 
>>
>> Maybe it makes sense to rename it, and write another pages_correctly_probed routine
>> for the offline case.
>>
>> So all checks would stay in memory_block_action level, and we would not need
>> the mem_sections_offline/online stuff.

I am OK with that, but will wait for a patch to review.

Pavel

Re: [PATCH v1 4/5] mm/memory_hotplug: onlining pages can only fail due to notifiers

[Pasha Tatashin]

LGTM

Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com>

On 8/16/18 6:06 AM, David Hildenbrand wrote:
> Onlining pages can only fail if a notifier reported a problem (e.g. -ENOMEM).
> online_pages_range() can never fail.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  mm/memory_hotplug.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 3dc6d2a309c2..bbbd16f9d877 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -933,13 +933,8 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>  		setup_zone_pageset(zone);
>  	}
>  
> -	ret = walk_system_ram_range(pfn, nr_pages, &onlined_pages,
> -		online_pages_range);
> -	if (ret) {
> -		if (need_zonelists_rebuild)
> -			zone_pcp_reset(zone);
> -		goto failed_addition;
> -	}
> +	walk_system_ram_range(pfn, nr_pages, &onlined_pages,
> +			      online_pages_range);
>  
>  	zone->present_pages += onlined_pages;
>  
> 

Re: [PATCH v1 5/5] mm/memory_hotplug: print only with DEBUG_VM in online/offline_pages()

[Pasha Tatashin]

On 8/20/18 6:46 AM, David Hildenbrand wrote:
> On 16.08.2018 12:06, David Hildenbrand wrote:
>> Let's try to minimze the noise.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  mm/memory_hotplug.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index bbbd16f9d877..6fec2dc6a73d 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -966,9 +966,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>>  	return 0;
>>  
>>  failed_addition:
>> +#ifdef CONFIG_DEBUG_VM
>>  	pr_debug("online_pages [mem %#010llx-%#010llx] failed\n",
>>  		 (unsigned long long) pfn << PAGE_SHIFT,
>>  		 (((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
>> +#endif
>>  	memory_notify(MEM_CANCEL_ONLINE, &arg);
>>  	return ret;
>>  }
>> @@ -1660,7 +1662,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
>>  	offlined_pages = check_pages_isolated(start_pfn, end_pfn);
>>  	if (offlined_pages < 0)
>>  		goto repeat;
>> +#ifdef CONFIG_DEBUG_VM
>>  	pr_info("Offlined Pages %ld\n", offlined_pages);
>> +#endif
>>  	/* Ok, all of our target is isolated.
>>  	   We cannot do rollback at this point. */
>>  	offline_isolated_pages(start_pfn, end_pfn);
>> @@ -1695,9 +1699,11 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
>>  	return 0;
>>  
>>  failed_removal:
>> +#ifdef CONFIG_DEBUG_VM
>>  	pr_debug("memory offlining [mem %#010llx-%#010llx] failed\n",
>>  		 (unsigned long long) start_pfn << PAGE_SHIFT,
>>  		 ((unsigned long long) end_pfn << PAGE_SHIFT) - 1);
>> +#endif
>>  	memory_notify(MEM_CANCEL_OFFLINE, &arg);
>>  	/* pushback to free area */
>>  	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
>>
> 
> I'll drop this patch for now, maybe the error messages are actually
> useful when debugging a crashdump of a system without CONFIG_DEBUG_VM.
> 

Yes, please drop it, no reason to reduce amount of these messages. They
are useful.

Thank you,
Pavel

Re: [PATCH v1 2/5] mm/memory_hotplug: enforce section alignment when onlining/offlining

[David Hildenbrand]

On 31.08.2018 00:14, Pasha Tatashin wrote:
> Hi David,
> 
> I am not sure this is needed, because we already have a stricter checker:
> 
> check_hotplug_memory_range()
> 
> You could call it from online_pages(), if you think there is a reason to
> do it, but other than that it is done from add_memory_resource() and
> from remove_memory().

Hi,

As offline_pages() is called from a different location for ppc (and I
understand why but don't consider this clean) and I used both functions
in a prototype, believing they would work with pageblock_nr_pages, I
really think that we should at least drop the misleading check from
offline_pages() and better also add checks for check_hotplug_memory_range().

Thanks for having a look Pavel!

> 
> Thank you,
> Pavel
> 
> On 8/16/18 6:06 AM, David Hildenbrand wrote:
>> onlining/offlining code works on whole sections, so let's enforce that.
>> Existing code only allows to add memory in memory block size. And only
>> whole memory blocks can be onlined/offlined. Memory blocks are always
>> aligned to sections, so this should not break anything.
>>
>> online_pages/offline_pages will implicitly mark whole sections
>> online/offline, so the code really can only handle such granularities.
>>
>> (especially offlining code cannot deal with pageblock_nr_pages but
>>  theoretically only MAX_ORDER-1)
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  mm/memory_hotplug.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 090cf474de87..30d2fa42b0bb 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -897,6 +897,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>>  	struct memory_notify arg;
>>  	struct memory_block *mem;
>>  
>> +	if (!IS_ALIGNED(pfn, PAGES_PER_SECTION))
>> +		return -EINVAL;
>> +	if (!IS_ALIGNED(nr_pages, PAGES_PER_SECTION))
>> +		return -EINVAL;
>> +
>>  	/*
>>  	 * We can't use pfn_to_nid() because nid might be stored in struct page
>>  	 * which is not yet initialized. Instead, we find nid from memory block.
>> @@ -1600,10 +1605,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
>>  	struct zone *zone;
>>  	struct memory_notify arg;
>>  
>> -	/* at least, alignment against pageblock is necessary */
>> -	if (!IS_ALIGNED(start_pfn, pageblock_nr_pages))
>> +	if (!IS_ALIGNED(start_pfn, PAGES_PER_SECTION))
>>  		return -EINVAL;
>> -	if (!IS_ALIGNED(end_pfn, pageblock_nr_pages))
>> +	if (!IS_ALIGNED(nr_pages, PAGES_PER_SECTION))
>>  		return -EINVAL;
>>  	/* This makes hotplug much easier...and readable.
>>  	   we assume this for now. .*/


-- 

Thanks,

David / dhildenb