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
Let's avoid this indirection and just call the function offline_pages().
Signed-off-by: David Hildenbrand <[email protected]>
---
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
Let's try to minimze the noise.
Signed-off-by: David Hildenbrand <[email protected]>
---
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
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
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
On 16.08.2018 13:44, Stephen Rothwell wrote:
> Hi David,
>
> On Thu, 16 Aug 2018 12:06:24 +0200 David Hildenbrand <[email protected]> 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
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 <[email protected]>
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 <[email protected]>
[1] https://patchwork.kernel.org/patch/10567277/
Thanks
--
Oscar Salvador
SUSE L3
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Hi David,
On Thu, 16 Aug 2018 12:06:24 +0200 David Hildenbrand <[email protected]> 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
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
> 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
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
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
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
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
On 16.08.2018 12:06, David Hildenbrand wrote:
> Let's try to minimze the noise.
>
> Signed-off-by: David Hildenbrand <[email protected]>
> ---
> 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
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
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 <[email protected]>
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 <[email protected]>
> ---
> 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 */
>
> /**
>
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 <[email protected]>>
> 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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> 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. .*/
>
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
LGTM
Reviewed-by: Pavel Tatashin <[email protected]>
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 <[email protected]>
> ---
> 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;
>
>
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>> ---
>> 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