Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1612985 > unrolled thread

[PATCH 0/6] mm: make movable onlining suck less

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-03-30 14:00 +0200
    [PATCH 1/6] mm: get rid of zone_is_initialized Michal Hocko <mhocko@kernel.org> - 2017-03-30 14:00 +0200
      Re: [PATCH 1/6] mm: get rid of zone_is_initialized "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2017-03-31 05:50 +0200
        Re: [PATCH 1/6] mm: get rid of zone_is_initialized Michal Hocko <mhocko@kernel.org> - 2017-03-31 08:50 +0200
          Re: [PATCH 1/6] mm: get rid of zone_is_initialized Michal Hocko <mhocko@kernel.org> - 2017-03-31 08:50 +0200
      [PATCH v1 1/6] mm: get rid of zone_is_initialized Michal Hocko <mhocko@kernel.org> - 2017-03-31 09:50 +0200
        Re: [PATCH v1 1/6] mm: get rid of zone_is_initialized Michal Hocko <mhocko@kernel.org> - 2017-04-05 10:20 +0200
          Re: [PATCH v1 1/6] mm: get rid of zone_is_initialized Michal Hocko <mhocko@kernel.org> - 2017-04-05 11:30 +0200
          Re: [PATCH v1 1/6] mm: get rid of zone_is_initialized Igor Mammedov <imammedo@redhat.com> - 2017-04-05 11:50 +0200
    [PATCH 2/6] mm, tile: drop arch_{add,remove}_memory Michal Hocko <mhocko@kernel.org> - 2017-03-30 14:00 +0200
    [PATCH 3/6] mm: remove return value from init_currently_empty_zone Michal Hocko <mhocko@kernel.org> - 2017-03-30 14:00 +0200
      Re: [PATCH 3/6] mm: remove return value from init_currently_empty_zone "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2017-03-31 06:00 +0200
        Re: [PATCH 3/6] mm: remove return value from  init_currently_empty_zone Michal Hocko <mhocko@kernel.org> - 2017-03-31 08:50 +0200
          Re: [PATCH 3/6] mm: remove return value from init_currently_empty_zone "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2017-03-31 09:10 +0200
            Re: [PATCH 3/6] mm: remove return value from  init_currently_empty_zone Michal Hocko <mhocko@kernel.org> - 2017-03-31 09:20 +0200
      Re: [PATCH 3/6] mm: remove return value from  init_currently_empty_zone Michal Hocko <mhocko@kernel.org> - 2017-03-31 09:50 +0200
      Re: [PATCH 3/6] mm: remove return value from  init_currently_empty_zone Reza Arbab <arbab@linux.vnet.ibm.com> - 2017-04-03 23:30 +0200
        Re: [PATCH 3/6] mm: remove return value from  init_currently_empty_zone Michal Hocko <mhocko@kernel.org> - 2017-04-04 09:40 +0200
    [PATCH 6/6] mm, memory_hotplug: remove unused cruft after memory hotplug rework Michal Hocko <mhocko@kernel.org> - 2017-03-30 14:00 +0200
      Re: [PATCH 6/6] mm, memory_hotplug: remove unused cruft after memory  hotplug rework Michal Hocko <mhocko@kernel.org> - 2017-03-31 09:50 +0200
    Re: [PATCH 0/6] mm: make movable onlining suck less Heiko Carstens <heiko.carstens@de.ibm.com> - 2017-03-31 21:20 +0200
      Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-03 09:40 +0200
    Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-03 14:00 +0200
      Re: [PATCH 0/6] mm: make movable onlining suck less Igor Mammedov <imammedo@redhat.com> - 2017-04-03 14:30 +0200
      Re: [PATCH 0/6] mm: make movable onlining suck less Reza Arbab <arbab@linux.vnet.ibm.com> - 2017-04-03 22:00 +0200
        Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-03 22:30 +0200
          Re: [PATCH 0/6] mm: make movable onlining suck less Reza Arbab <arbab@linux.vnet.ibm.com> - 2017-04-03 22:50 +0200
            Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-04 09:30 +0200
              Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-04 09:40 +0200
                Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-04 10:30 +0200
                  Re: [PATCH 0/6] mm: make movable onlining suck less Reza Arbab <arbab@linux.vnet.ibm.com> - 2017-04-04 18:00 +0200
                  Re: [PATCH 0/6] mm: make movable onlining suck less Reza Arbab <arbab@linux.vnet.ibm.com> - 2017-04-04 18:10 +0200
                    Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-04 18:50 +0200
                      Re: [PATCH 0/6] mm: make movable onlining suck less Reza Arbab <arbab@linux.vnet.ibm.com> - 2017-04-04 20:40 +0200
                        Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-04 21:50 +0200
                          Re: [PATCH 0/6] mm: make movable onlining suck less Reza Arbab <arbab@linux.vnet.ibm.com> - 2017-04-04 23:50 +0200
                            Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-05 08:50 +0200
                              Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-05 11:30 +0200
                                Re: [PATCH 0/6] mm: make movable onlining suck less Reza Arbab <arbab@linux.vnet.ibm.com> - 2017-04-05 17:00 +0200
                                  Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-05 17:50 +0200
                                    Re: [PATCH 0/6] mm: make movable onlining suck less Reza Arbab <arbab@linux.vnet.ibm.com> - 2017-04-05 19:40 +0200
                                      Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-05 20:20 +0200
                                        Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-05 21:50 +0200
                                        Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-05 23:10 +0200
                                          Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-06 13:10 +0200
                              Re: [PATCH 0/6] mm: make movable onlining suck less Reza Arbab <arbab@linux.vnet.ibm.com> - 2017-04-05 18:00 +0200
                                Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-05 18:40 +0200
                                  Re: [PATCH 0/6] mm: make movable onlining suck less Reza Arbab <arbab@linux.vnet.ibm.com> - 2017-04-05 23:00 +0200
                                  Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-06 11:30 +0200
                            Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-05 16:00 +0200
                              Re: [PATCH 0/6] mm: make movable onlining suck less Reza Arbab <arbab@linux.vnet.ibm.com> - 2017-04-05 17:30 +0200
                          Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-05 08:40 +0200
    Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-06 15:10 +0200
      Re: [PATCH 0/6] mm: make movable onlining suck less Reza Arbab <arbab@linux.vnet.ibm.com> - 2017-04-06 17:30 +0200
        Re: [PATCH 0/6] mm: make movable onlining suck less Reza Arbab <arbab@linux.vnet.ibm.com> - 2017-04-06 17:50 +0200
          Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-06 18:30 +0200
            Re: [PATCH 0/6] mm: make movable onlining suck less Mel Gorman <mgorman@suse.de> - 2017-04-06 18:30 +0200
            Re: [PATCH 0/6] mm: make movable onlining suck less Mel Gorman <mgorman@suse.de> - 2017-04-06 19:00 +0200
              Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-06 19:20 +0200
                Re: [PATCH 0/6] mm: make movable onlining suck less Mel Gorman <mgorman@suse.de> - 2017-04-06 19:50 +0200
        Re: [PATCH 0/6] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-06 17:50 +0200

Page 3 of 4 — ← Prev page 1 2 [3] 4  Next page →

#1617231

On Wed, Apr 05, 2017 at 05:42:59PM +0200, Michal Hocko wrote:
>But one thing that is really bugging me is how could you see low pfns 
>in the previous oops. Please drop the last patch and sprinkle printks 
>down the remove_memory path to see where this all go south. I believe 
>that there is something in the initialization code lurking in my code. 
>Please also scratch the pfn_valid check in online_pages diff. It will 
>not help here.

Got it. 

shrink_pgdat_span: start_pfn=0x10000, end_pfn=0x10100, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000

The problem is that pgdat_start_pfn here should be 0x10000. As you 
suspected, it never got set. This fixes things for me.

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 623507f..37c1b63 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -884,7 +884,7 @@ static void __meminit resize_pgdat_range(struct pglist_data *pgdat, unsigned lon
 {
 	unsigned long old_end_pfn = pgdat_end_pfn(pgdat);
 
-	if (start_pfn < pgdat->node_start_pfn)
+	if (!pgdat->node_spanned_pages || start_pfn < pgdat->node_start_pfn)
 		pgdat->node_start_pfn = start_pfn;
 
 	pgdat->node_spanned_pages = max(start_pfn + nr_pages, old_end_pfn) - pgdat->node_start_pfn;
---

Along these lines, maybe we should also do

-	if (start_pfn < zone->zone_start_pfn)
+	if (zone_is_empty(zone) || start_pfn < zone->zone_start_pfn)

in resize_zone_range()?

-- 
Reza Arbab

[toc] | [prev] | [next] | [standalone]

#1617267

On Wed 05-04-17 12:32:49, Reza Arbab wrote:
> On Wed, Apr 05, 2017 at 05:42:59PM +0200, Michal Hocko wrote:
> >But one thing that is really bugging me is how could you see low pfns in
> >the previous oops. Please drop the last patch and sprinkle printks down
> >the remove_memory path to see where this all go south. I believe that
> >there is something in the initialization code lurking in my code. Please
> >also scratch the pfn_valid check in online_pages diff. It will not help
> >here.
> 
> Got it.
> 
> shrink_pgdat_span: start_pfn=0x10000, end_pfn=0x10100, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000
> 
> The problem is that pgdat_start_pfn here should be 0x10000. As you
> suspected, it never got set. This fixes things for me.
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 623507f..37c1b63 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -884,7 +884,7 @@ static void __meminit resize_pgdat_range(struct pglist_data *pgdat, unsigned lon
> {
> 	unsigned long old_end_pfn = pgdat_end_pfn(pgdat);
> 
> -	if (start_pfn < pgdat->node_start_pfn)
> +	if (!pgdat->node_spanned_pages || start_pfn < pgdat->node_start_pfn)
> 		pgdat->node_start_pfn = start_pfn;

Dang! You are absolutely right. This explains the issue during the
remove_memory. I still fail to see how this makes any difference for the
sysfs file registration though. If anything the pgdat will be larger and
so try_offline_node would check also unrelated node0 but the code will
handle that and eventually offline the node1 anyway. /me still confused.
 
> 	pgdat->node_spanned_pages = max(start_pfn + nr_pages, old_end_pfn) - pgdat->node_start_pfn;
> ---
> 
> Along these lines, maybe we should also do
> 
> -	if (start_pfn < zone->zone_start_pfn)
> +	if (zone_is_empty(zone) || start_pfn < zone->zone_start_pfn)

yes we should.

Thanks a lot!
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1617315

On Wed 05-04-17 20:15:02, Michal Hocko wrote:
> On Wed 05-04-17 12:32:49, Reza Arbab wrote:
> > On Wed, Apr 05, 2017 at 05:42:59PM +0200, Michal Hocko wrote:
> > >But one thing that is really bugging me is how could you see low pfns in
> > >the previous oops. Please drop the last patch and sprinkle printks down
> > >the remove_memory path to see where this all go south. I believe that
> > >there is something in the initialization code lurking in my code. Please
> > >also scratch the pfn_valid check in online_pages diff. It will not help
> > >here.
> > 
> > Got it.
> > 
> > shrink_pgdat_span: start_pfn=0x10000, end_pfn=0x10100, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000
> > 
> > The problem is that pgdat_start_pfn here should be 0x10000. As you
> > suspected, it never got set. This fixes things for me.
> > 
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index 623507f..37c1b63 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -884,7 +884,7 @@ static void __meminit resize_pgdat_range(struct pglist_data *pgdat, unsigned lon
> > {
> > 	unsigned long old_end_pfn = pgdat_end_pfn(pgdat);
> > 
> > -	if (start_pfn < pgdat->node_start_pfn)
> > +	if (!pgdat->node_spanned_pages || start_pfn < pgdat->node_start_pfn)
> > 		pgdat->node_start_pfn = start_pfn;
> 
> Dang! You are absolutely right. This explains the issue during the
> remove_memory. I still fail to see how this makes any difference for the
> sysfs file registration though. If anything the pgdat will be larger and
> so try_offline_node would check also unrelated node0 but the code will
> handle that and eventually offline the node1 anyway. /me still confused.

OK, so I've managed to convince my kvm setup to create a node without
any memory initially but I cannot seem to be able to reach
node_set_offline in try_offline_node because check_and_unmap_cpu_on_node
fails for me even when I offline cpus bound to the node because we are
using for_each_present_cpu in check_cpu_on_node so I would have to start
with a cpuless node or find a way how to hotremove those cpus.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1617381

On Wed 05-04-17 20:15:02, Michal Hocko wrote:
> On Wed 05-04-17 12:32:49, Reza Arbab wrote:
> > On Wed, Apr 05, 2017 at 05:42:59PM +0200, Michal Hocko wrote:
> > >But one thing that is really bugging me is how could you see low pfns in
> > >the previous oops. Please drop the last patch and sprinkle printks down
> > >the remove_memory path to see where this all go south. I believe that
> > >there is something in the initialization code lurking in my code. Please
> > >also scratch the pfn_valid check in online_pages diff. It will not help
> > >here.
> > 
> > Got it.
> > 
> > shrink_pgdat_span: start_pfn=0x10000, end_pfn=0x10100, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000
> > 
> > The problem is that pgdat_start_pfn here should be 0x10000. As you
> > suspected, it never got set. This fixes things for me.
> > 
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index 623507f..37c1b63 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -884,7 +884,7 @@ static void __meminit resize_pgdat_range(struct pglist_data *pgdat, unsigned lon
> > {
> > 	unsigned long old_end_pfn = pgdat_end_pfn(pgdat);
> > 
> > -	if (start_pfn < pgdat->node_start_pfn)
> > +	if (!pgdat->node_spanned_pages || start_pfn < pgdat->node_start_pfn)
> > 		pgdat->node_start_pfn = start_pfn;
> 
> Dang! You are absolutely right. This explains the issue during the
> remove_memory. I still fail to see how this makes any difference for the
> sysfs file registration though. If anything the pgdat will be larger and
> so try_offline_node would check also unrelated node0 but the code will
> handle that and eventually offline the node1 anyway. /me still confused.

OK, I was staring into the code and I guess I finally understand what is
going on here. Looking at arch_add_memory->...->register_mem_sect_under_node
was just misleading. I am still not 100% sure why but we try to do the
same thing later from register_one_node->link_mem_sections for nodes
which were offline. I should have noticed this path before. And here
is the difference from the previous code. We are past arch_add_memory
and that path used to do __add_zone which among other things will also
resize node boundaries. I am not doing that anymore because I postpone
that to the onlining phase. Jeez this code is so convoluted my head
spins.

I am not really sure how to fix this. I suspect register_mem_sect_under_node
should just ignore the online state of the node. But I wouldn't
be all that surprised if this had some subtle reason as well. An
alternative would be to actually move register_mem_sect_under_node out
of register_new_memory and move it up the call stack, most probably to
add_memory_resource. We have the range and can map it to the memblock
and so will not rely on the node range. I will sleep over it and
hopefully come up with something tomorrow.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1617882

On Wed 05-04-17 23:02:14, Michal Hocko wrote:
[...]
> OK, I was staring into the code and I guess I finally understand what is
> going on here. Looking at arch_add_memory->...->register_mem_sect_under_node
> was just misleading. I am still not 100% sure why but we try to do the
> same thing later from register_one_node->link_mem_sections for nodes
> which were offline. I should have noticed this path before. And here
> is the difference from the previous code. We are past arch_add_memory
> and that path used to do __add_zone which among other things will also
> resize node boundaries. I am not doing that anymore because I postpone
> that to the onlining phase. Jeez this code is so convoluted my head
> spins.
> 
> I am not really sure how to fix this. I suspect register_mem_sect_under_node
> should just ignore the online state of the node. But I wouldn't
> be all that surprised if this had some subtle reason as well. An
> alternative would be to actually move register_mem_sect_under_node out
> of register_new_memory and move it up the call stack, most probably to
> add_memory_resource. We have the range and can map it to the memblock
> and so will not rely on the node range. I will sleep over it and
> hopefully come up with something tomorrow.

OK, so this is the most sensible way I was able to come up with. I
didn't get to test it yet but from the above analysis it should work.
---
From 6c99a3284ea70262e3f25cbe71826a57aeaa7ffd Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Thu, 6 Apr 2017 11:59:37 +0200
Subject: [PATCH] mm, memory_hotplug: split up register_one_node

Memory hotplug (add_memory_resource) has to reinitialize node
infrastructure if the node is offline (one which went through the
complete add_memory(); remove_memory() cycle). That involves node
registration to the kobj infrastructure (register_node), the proper
association with cpus (register_cpu_under_node) and finally creation of
node<->memblock symlinks (link_mem_sections).

The last part requires to know node_start_pfn and node_spanned_pages
which we currently have but a leter patch will postpone this
initialization to the onlining phase which happens later. In fact
we do not need to rely on the early initialization even now because
we know which range is currently hot added.

Split register_one_node into core which does all the common work for
the boot time NUMA initialization and the hotplug (__register_one_node).
register_one_node keeps the full initialization while hotplug calls
__register_one_node and manually calls link_mem_sections for the proper
range.

This shouldn't introduce any functional change.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 drivers/base/node.c  | 51 ++++++++++++++++++++-------------------------------
 include/linux/node.h | 35 ++++++++++++++++++++++++++++++++++-
 mm/memory_hotplug.c  | 17 ++++++++++++++++-
 3 files changed, 70 insertions(+), 33 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index 06294d69779b..dff5b53f7905 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -461,10 +461,9 @@ int unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
 	return 0;
 }
 
-static int link_mem_sections(int nid)
+int link_mem_sections(int nid, unsigned long start_pfn, unsigned long nr_pages)
 {
-	unsigned long start_pfn = NODE_DATA(nid)->node_start_pfn;
-	unsigned long end_pfn = start_pfn + NODE_DATA(nid)->node_spanned_pages;
+	unsigned long end_pfn = start_pfn + nr_pages;
 	unsigned long pfn;
 	struct memory_block *mem_blk = NULL;
 	int err = 0;
@@ -552,10 +551,7 @@ static int node_memory_callback(struct notifier_block *self,
 	return NOTIFY_OK;
 }
 #endif	/* CONFIG_HUGETLBFS */
-#else	/* !CONFIG_MEMORY_HOTPLUG_SPARSE */
-
-static int link_mem_sections(int nid) { return 0; }
-#endif	/* CONFIG_MEMORY_HOTPLUG_SPARSE */
+#endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
 
 #if !defined(CONFIG_MEMORY_HOTPLUG_SPARSE) || \
     !defined(CONFIG_HUGETLBFS)
@@ -569,39 +565,32 @@ static void init_node_hugetlb_work(int nid) { }
 
 #endif
 
-int register_one_node(int nid)
+int __register_one_node(int nid)
 {
-	int error = 0;
+	int p_node = parent_node(nid);
+	struct node *parent = NULL;
+	int error;
 	int cpu;
 
-	if (node_online(nid)) {
-		int p_node = parent_node(nid);
-		struct node *parent = NULL;
-
-		if (p_node != nid)
-			parent = node_devices[p_node];
-
-		node_devices[nid] = kzalloc(sizeof(struct node), GFP_KERNEL);
-		if (!node_devices[nid])
-			return -ENOMEM;
-
-		error = register_node(node_devices[nid], nid, parent);
+	if (p_node != nid)
+		parent = node_devices[p_node];
 
-		/* link cpu under this node */
-		for_each_present_cpu(cpu) {
-			if (cpu_to_node(cpu) == nid)
-				register_cpu_under_node(cpu, nid);
-		}
+	node_devices[nid] = kzalloc(sizeof(struct node), GFP_KERNEL);
+	if (!node_devices[nid])
+		return -ENOMEM;
 
-		/* link memory sections under this node */
-		error = link_mem_sections(nid);
+	error = register_node(node_devices[nid], nid, parent);
 
-		/* initialize work queue for memory hot plug */
-		init_node_hugetlb_work(nid);
+	/* link cpu under this node */
+	for_each_present_cpu(cpu) {
+		if (cpu_to_node(cpu) == nid)
+			register_cpu_under_node(cpu, nid);
 	}
 
-	return error;
+	/* initialize work queue for memory hot plug */
+	init_node_hugetlb_work(nid);
 
+	return error;
 }
 
 void unregister_one_node(int nid)
diff --git a/include/linux/node.h b/include/linux/node.h
index 2115ad5d6f19..2baa640d0b92 100644
--- a/include/linux/node.h
+++ b/include/linux/node.h
@@ -30,9 +30,38 @@ struct memory_block;
 extern struct node *node_devices[];
 typedef  void (*node_registration_func_t)(struct node *);
 
+#ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
+extern int link_mem_sections(int nid, unsigned long start_pfn, unsigned long nr_pages);
+#else
+static int link_mem_sections(int nid, unsigned long start_pfn, unsigned long nr_pages)
+{
+	return 0;
+}
+#endif
+
 extern void unregister_node(struct node *node);
 #ifdef CONFIG_NUMA
-extern int register_one_node(int nid);
+/* Core of the node registration - only memory hotplug should use this */
+extern int __register_one_node(int nid);
+
+/* Registers an online node */
+static inline int register_one_node(int nid)
+{
+	int error = 0;
+
+	if (node_online(nid)) {
+		struct pglist_data *pgdat = NODE_DATA(nid);
+
+		error = __register_one_node(nid);
+		if (error)
+			return error;
+		/* link memory sections under this node */
+		error = link_mem_sections(nid, pgdat->node_start_pfn, pgdat->node_spanned_pages);
+	}
+
+	return error;
+}
+
 extern void unregister_one_node(int nid);
 extern int register_cpu_under_node(unsigned int cpu, unsigned int nid);
 extern int unregister_cpu_under_node(unsigned int cpu, unsigned int nid);
@@ -46,6 +75,10 @@ extern void register_hugetlbfs_with_node(node_registration_func_t doregister,
 					 node_registration_func_t unregister);
 #endif
 #else
+static inline int __register_one_node(int nid)
+{
+	return 0;
+}
 static inline int register_one_node(int nid)
 {
 	return 0;
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index c2b018c808b7..2c731bdfa845 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1220,7 +1220,22 @@ int __ref add_memory_resource(int nid, struct resource *res, bool online)
 	node_set_online(nid);
 
 	if (new_node) {
-		ret = register_one_node(nid);
+		unsigned long start_pfn = start >> PAGE_SHIFT;
+		unsigned long nr_pages = size >> PAGE_SHIFT;
+
+		ret = __register_one_node(nid);
+		if (ret)
+			goto register_fail;
+
+		/*
+		 * link memory sections under this node. This is already
+		 * done when creatig memory section in register_new_memory
+		 * but that depends to have the node registered so offline
+		 * nodes have to go through register_node.
+		 * TODO clean up this mess.
+		 */
+		ret = link_mem_sections(nid, start_pfn, nr_pages);
+register_fail:
 		/*
 		 * If sysfs file of new node can't create, cpu on the node
 		 * can't be hot-added. There is no rollback way now.
-- 
2.11.0

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1617080

On Wed, Apr 05, 2017 at 08:42:39AM +0200, Michal Hocko wrote:
>On Tue 04-04-17 16:43:39, Reza Arbab wrote:
>> Okay, getting further. With this I can again repeatedly add and 
>> remove, but now I'm seeing a weird variation of that earlier issue:
>>
>> 1. add_memory(), online_movable
>>   /sys/devices/system/node/nodeX/memoryY symlinks are created.
>>
>> 2. offline, remove_memory()
>>   The node is offlined, since all memory has been removed, so all of
>>   /sys/devices/system/node/nodeX is gone. This is normal.
>>
>> 3. add_memory(), online_movable
>>   The node is onlined, so /sys/devices/system/node/nodeX is recreated,
>>   and the memory is added, but just like earlier in this email thread,
>>   the memoryY links are not there.
>
>Could you add some printks to see why the sysfs creation failed please?

Ah, simple enough. It's this, right at the top of 
register_mem_sect_under_node():

	if (!node_online(nid))
		return 0;

That being the case, I really don't understand why your patches make any 
difference. Is node_set_online() being called later than before somehow?

-- 
Reza Arbab

[toc] | [prev] | [next] | [standalone]

#1617131

On Wed 05-04-17 10:48:52, Reza Arbab wrote:
> On Wed, Apr 05, 2017 at 08:42:39AM +0200, Michal Hocko wrote:
> >On Tue 04-04-17 16:43:39, Reza Arbab wrote:
> >>Okay, getting further. With this I can again repeatedly add and remove,
> >>but now I'm seeing a weird variation of that earlier issue:
> >>
> >>1. add_memory(), online_movable
> >>  /sys/devices/system/node/nodeX/memoryY symlinks are created.
> >>
> >>2. offline, remove_memory()
> >>  The node is offlined, since all memory has been removed, so all of
> >>  /sys/devices/system/node/nodeX is gone. This is normal.
> >>
> >>3. add_memory(), online_movable
> >>  The node is onlined, so /sys/devices/system/node/nodeX is recreated,
> >>  and the memory is added, but just like earlier in this email thread,
> >>  the memoryY links are not there.
> >
> >Could you add some printks to see why the sysfs creation failed please?
> 
> Ah, simple enough. It's this, right at the top of
> register_mem_sect_under_node():
> 
> 	if (!node_online(nid))
> 		return 0;
> 
> That being the case, I really don't understand why your patches make any
> difference. Is node_set_online() being called later than before somehow?

This is really interesting. Because add_memory_resource does the
following
	/* call arch's memory hotadd */
	ret = arch_add_memory(nid, start, size);

	if (ret < 0)
		goto error;

	/* we online node here. we can't roll back from here. */
	node_set_online(nid);

so we are setting the node online _after_ arch_add_memory but the code
which adds those sysfs file is called from

arch_add_memory
  __add_pages
    __add_section
      register_new_memory
        register_mem_sect_under_node
          node_online check

I haven't touched this part. What is the point of this check anyway? We
have already associated all the pages with a node (and with a zone prior
to my patches) so we _know_ how to create those links. The check goes
back to the initial submissions. Gary is not available anymore so we
cannot ask. But I completely fail to see how my changes could have made
any difference.

I assume that things start working after you remove that check? Btw. if
you put printk to the original kernel does it see the node online? I
would be also interested whether you see try_offline_node setting the
node offline in the original code.

Thanks!
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1617376

On Wed, Apr 05, 2017 at 06:34:39PM +0200, Michal Hocko wrote:
>This is really interesting. Because add_memory_resource does the
>following
>	/* call arch's memory hotadd */
>	ret = arch_add_memory(nid, start, size);
>
>	if (ret < 0)
>		goto error;
>
>	/* we online node here. we can't roll back from here. */
>	node_set_online(nid);
>
>so we are setting the node online _after_ arch_add_memory but the code
>which adds those sysfs file is called from
>
>arch_add_memory
>  __add_pages
>    __add_section
>      register_new_memory
>        register_mem_sect_under_node
>          node_online check

Okay, so it turns out the original code ends up creating the sysfs links 
not here, but just a little bit afterwards.

add_memory
  add_memory_resource
    arch_add_memory
      [your quoted stack trace above]
    ...
    set_node_online
    ...
    register_one_node
      link_mem_sections
	register_mem_sect_under_node

The reason they're not getting created now is because 
NODE_DATA(nid)->node_spanned_pages = 0 at this point.

link_mem_sections: nid=1, start_pfn=0x10000, end_pfn=0x10000

This is another uninitialized situation, like the one with 
node_start_pfn which caused my removal crash. Except here I'm not sure 
the correct place to splice in and set it.

-- 
Reza Arbab

[toc] | [prev] | [next] | [standalone]

#1617766

On Wed 05-04-17 18:34:39, Michal Hocko wrote:
> On Wed 05-04-17 10:48:52, Reza Arbab wrote:
> > On Wed, Apr 05, 2017 at 08:42:39AM +0200, Michal Hocko wrote:
> > >On Tue 04-04-17 16:43:39, Reza Arbab wrote:
> > >>Okay, getting further. With this I can again repeatedly add and remove,
> > >>but now I'm seeing a weird variation of that earlier issue:
> > >>
> > >>1. add_memory(), online_movable
> > >>  /sys/devices/system/node/nodeX/memoryY symlinks are created.
> > >>
> > >>2. offline, remove_memory()
> > >>  The node is offlined, since all memory has been removed, so all of
> > >>  /sys/devices/system/node/nodeX is gone. This is normal.
> > >>
> > >>3. add_memory(), online_movable
> > >>  The node is onlined, so /sys/devices/system/node/nodeX is recreated,
> > >>  and the memory is added, but just like earlier in this email thread,
> > >>  the memoryY links are not there.
> > >
> > >Could you add some printks to see why the sysfs creation failed please?
> > 
> > Ah, simple enough. It's this, right at the top of
> > register_mem_sect_under_node():
> > 
> > 	if (!node_online(nid))
> > 		return 0;
> > 
> > That being the case, I really don't understand why your patches make any
> > difference. Is node_set_online() being called later than before somehow?
> 
> This is really interesting. Because add_memory_resource does the
> following
> 	/* call arch's memory hotadd */
> 	ret = arch_add_memory(nid, start, size);
> 
> 	if (ret < 0)
> 		goto error;
> 
> 	/* we online node here. we can't roll back from here. */
> 	node_set_online(nid);
> 
> so we are setting the node online _after_ arch_add_memory but the code
> which adds those sysfs file is called from
> 
> arch_add_memory
>   __add_pages
>     __add_section
>       register_new_memory
>         register_mem_sect_under_node
>           node_online check
> 
> I haven't touched this part. What is the point of this check anyway? We
> have already associated all the pages with a node (and with a zone prior
> to my patches) so we _know_ how to create those links. The check goes
> back to the initial submissions. Gary is not available anymore so we
> cannot ask. But I completely fail to see how my changes could have made
> any difference.

I wasn't able to undestand that from the code so I've just tried to
remove the check and it blown up
	BUG: unable to handle kernel NULL pointer dereference at
	0000000000000040
	IP: sysfs_create_link_nowarn+0x13/0x32

	if (!kobj)
		parent = sysfs_root_kn;
	else
		parent = kobj->sd;
		^^^^^^^^^^^^^^^^^^

when creating the link
register_mem_sect_under_node:
		ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
					&mem_blk->dev.kobj,
					kobject_name(&mem_blk->dev.kobj));

which means that node_devices[nid]->dev.kobj is NULL. This happens later
in register_one_node->register_node. This really _screems_ for a clean up!
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1616958

On Tue 04-04-17 16:43:39, Reza Arbab wrote:
> On Tue, Apr 04, 2017 at 09:41:22PM +0200, Michal Hocko wrote:
> >On Tue 04-04-17 13:30:13, Reza Arbab wrote:
> >>I think I found another edge case.  You
> >>get an oops when removing all of a node's memory:
> >>
> >>__nr_to_section
> >>__pfn_to_section
> >>find_biggest_section_pfn
> >>shrink_pgdat_span
> >>__remove_zone
> >>__remove_section
> >>__remove_pages
> >>arch_remove_memory
> >>remove_memory
> >
> >Is this something new or an old issue? I believe the state after the
> >online should be the same as before. So if you onlined the full node
> >then there shouldn't be any difference. Let me have a look...
> 
> It's new. Without this patchset, I can repeatedly
> add_memory()->online_movable->offline->remove_memory() all of a node's
> memory.

OK, I know what is going on here.
shrink_pgdat_span: start_pfn=0x1ff00, end_pfn=0x20000, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000
[...]
find_biggest_section_pfn loop: pfn=0xff, sec_nr = 0x0

so the node starts at pfn 0 while we are trying to remove range starting
from pfn=255 (1MB). Rather than going with find_smallest_section_pfn we
go with the other branch and that underflows as already mentioned. I
seriously doubt that the node really starts at pfn 0. I am not sure
which arch you are testing on but I believe we reserve the lowest
address pfn range on all aches. The previous code presumably handled
that properly because the original node/zone has started at the lowest
possible address and the zone shifting then preserves that.

My code doesn't do that though. So I guess I have to sanitize. Does this
help? Please drop the "mm, memory_hotplug: get rid of zone/node
shrinking" patch.
---
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index acf2b5eb5ecb..2c5613d19eb6 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -750,6 +750,15 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	int ret;
 	struct memory_notify arg;
 
+	do {
+		if (pfn_valid(pfn))
+			break;
+		pfn++;
+	} while (--nr_pages > 0);
+
+	if (!nr_pages)
+		return -EINVAL;
+
 	nid = pfn_to_nid(pfn);
 	if (!allow_online_pfn_range(nid, pfn, nr_pages, online_type))
 		return -EINVAL;
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1617048

On Wed, Apr 05, 2017 at 03:52:49PM +0200, Michal Hocko wrote:
>My code doesn't do that though. So I guess I have to sanitize. Does 
>this help? Please drop the "mm, memory_hotplug: get rid of zone/node
>shrinking" patch.
>---
>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index acf2b5eb5ecb..2c5613d19eb6 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -750,6 +750,15 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
> 	int ret;
> 	struct memory_notify arg;
>
>+	do {
>+		if (pfn_valid(pfn))
>+			break;
>+		pfn++;
>+	} while (--nr_pages > 0);
>+
>+	if (!nr_pages)
>+		return -EINVAL;
>+
> 	nid = pfn_to_nid(pfn);
> 	if (!allow_online_pfn_range(nid, pfn, nr_pages, online_type))
> 		return -EINVAL;

Sorry, no change. Back to the oops in find_biggest_section_pfn().

-- 
Reza Arbab

[toc] | [prev] | [next] | [standalone]

#1616613

On Tue 04-04-17 21:41:22, Michal Hocko wrote:
> On Tue 04-04-17 13:30:13, Reza Arbab wrote:
> > On Tue, Apr 04, 2017 at 06:44:53PM +0200, Michal Hocko wrote:
> > >Thanks for your testing! This is highly appreciated.
> > >Can I assume your Tested-by?
> > 
> > Of course! Not quite done, though. 
> 
> Ohh, I didn't mean to rush you to that!
> 
> > I think I found another edge case.  You
> > get an oops when removing all of a node's memory:
> > 
> > __nr_to_section
> > __pfn_to_section
> > find_biggest_section_pfn
> > shrink_pgdat_span
> > __remove_zone
> > __remove_section
> > __remove_pages
> > arch_remove_memory
> > remove_memory
> 
> Is this something new or an old issue? I believe the state after the
> online should be the same as before. So if you onlined the full node
> then there shouldn't be any difference. Let me have a look...
> 
> > I stuck some debugging prints in, for context:
> > 
> > shrink_pgdat_span: start_pfn=0x10000, end_pfn=0x10100, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000
> > shrink_pgdat_span: start_pfn=0x10100, end_pfn=0x10200, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000
> > ...%<...
> > shrink_pgdat_span: start_pfn=0x1fe00, end_pfn=0x1ff00, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000
> > shrink_pgdat_span: start_pfn=0x1ff00, end_pfn=0x20000, pgdat_start_pfn=0x0, pgdat_end_pfn=0x20000
> > find_biggest_section_pfn: start_pfn=0x0, end_pfn=0x1ff00
> > find_biggest_section_pfn loop: pfn=0x1feff, sec_nr = 0x1fe
> > find_biggest_section_pfn loop: pfn=0x1fdff, sec_nr = 0x1fd
> > ...%<...
> > find_biggest_section_pfn loop: pfn=0x1ff, sec_nr = 0x1
> > find_biggest_section_pfn loop: pfn=0xff, sec_nr = 0x0
> > find_biggest_section_pfn loop: pfn=0xffffffffffffffff, sec_nr = 0xffffffffffffff
> > Unable to handle kernel paging request for data at address 0xc000800000f19e78
> 
> ...this looks like a straight underflow and it is clear that the code
> is just broken. Have a look at the loop
> 	pfn = end_pfn - 1;
> 	for (; pfn >= start_pfn; pfn -= PAGES_PER_SECTION) {
> 
> assume that end_pfn is properly PAGES_PER_SECTION aligned (start_pfn
> would be 0 obviously). This is unsigned arithmetic and so it cannot work
> for the first section. So the code is broken and has been broken since
> it has been introduced. Nobody has noticed because the low pfns are
> usually reserved and out of the hotplug reach. We could tweak it but I
> am not even sure we really want/need this behavior. It complicates the
> code and am not really sure we need to support
> 	online_movable(range)
> 	offline_movable(range)
> 	online_kernel(range)

OK, so I managed to confuse myself. This is not about offlining. This is
about arch_remove_memory path which means this is about memory
hotremove. So we are talking about hotremove(N1, range1) and hotadd(N2,
range2) where range1 and range2 have a non-empty intersection. Do we
need to supporst this usecase?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1617994

OK, so after recent change mostly driven by testing from Reza Arbab
(thanks again) I believe I am getting to a working state finally. All I
currently have is
in git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git tree
attempts/rewrite-mem_hotplug-WIP branch. I will highly appreciate more
testing of course and if there are no new issues found I will repost the
series for the review.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1618109

On Thu, Apr 06, 2017 at 03:08:46PM +0200, Michal Hocko wrote:
>OK, so after recent change mostly driven by testing from Reza Arbab
>(thanks again) I believe I am getting to a working state finally. All I
>currently have is
>in git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git tree
>attempts/rewrite-mem_hotplug-WIP branch. I will highly appreciate more
>testing of course and if there are no new issues found I will repost the
>series for the review.

Looking good! I can do my add/remove/repeat test and things seem fine.

One thing--starting on the second iteration, I am seeing the WARN in 
free_area_init_node();

add_memory
  add_memory_resource
    hotadd_new_pgdat
      free_area_init_node
	WARN_ON(pgdat->nr_zones || pgdat->kswapd_classzone_idx);

-- 
Reza Arbab

[toc] | [prev] | [next] | [standalone]

#1618121

On Thu, Apr 06, 2017 at 05:41:28PM +0200, Michal Hocko wrote:
>On Thu 06-04-17 10:24:49, Reza Arbab wrote:
>> On Thu, Apr 06, 2017 at 03:08:46PM +0200, Michal Hocko wrote:
>> >OK, so after recent change mostly driven by testing from Reza Arbab
>> >(thanks again) I believe I am getting to a working state finally. All I
>> >currently have is
>> >in git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git tree
>> >attempts/rewrite-mem_hotplug-WIP branch. I will highly appreciate more
>> >testing of course and if there are no new issues found I will repost the
>> >series for the review.
>>
>> Looking good! I can do my add/remove/repeat test and things seem fine.
>>
>> One thing--starting on the second iteration, I am seeing the WARN in
>> free_area_init_node();
>>
>> add_memory
>>  add_memory_resource
>>    hotadd_new_pgdat
>>      free_area_init_node
>> 	WARN_ON(pgdat->nr_zones || pgdat->kswapd_classzone_idx);
>
>Have you tested with my attempts/rewrite-mem_hotplug-WIP mentioned
>elsewhere? Because I suspect that "mm: get rid of zone_is_initialized"
>might cause this.

This was my first time using your git branch instead of applying the 
patches from this thread to v4.11-rc5 myself.

-- 
Reza Arbab

[toc] | [prev] | [next] | [standalone]

#1618162

On Thu 06-04-17 10:46:53, Reza Arbab wrote:
> On Thu, Apr 06, 2017 at 05:41:28PM +0200, Michal Hocko wrote:
> >On Thu 06-04-17 10:24:49, Reza Arbab wrote:
> >>On Thu, Apr 06, 2017 at 03:08:46PM +0200, Michal Hocko wrote:
> >>>OK, so after recent change mostly driven by testing from Reza Arbab
> >>>(thanks again) I believe I am getting to a working state finally. All I
> >>>currently have is
> >>>in git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git tree
> >>>attempts/rewrite-mem_hotplug-WIP branch. I will highly appreciate more
> >>>testing of course and if there are no new issues found I will repost the
> >>>series for the review.
> >>
> >>Looking good! I can do my add/remove/repeat test and things seem fine.
> >>
> >>One thing--starting on the second iteration, I am seeing the WARN in
> >>free_area_init_node();
> >>
> >>add_memory
> >> add_memory_resource
> >>   hotadd_new_pgdat
> >>     free_area_init_node
> >>	WARN_ON(pgdat->nr_zones || pgdat->kswapd_classzone_idx);
> >
> >Have you tested with my attempts/rewrite-mem_hotplug-WIP mentioned
> >elsewhere? Because I suspect that "mm: get rid of zone_is_initialized"
> >might cause this.
> 
> This was my first time using your git branch instead of applying the patches
> from this thread to v4.11-rc5 myself.

OK, so this looks like another thing to resolve. I have seen this
warning as well but I didn't consider it relevant because I had to tweak
the code make the node go offline (removed check_and_unmap_cpu_on_node
from try_offline_node) so I thought it was a fallout from there. 

But let's have a look. hotadd_new_pgdat does for an existing pgdat
		/* Reset the nr_zones, order and classzone_idx before reuse */
		pgdat->nr_zones = 0;
		pgdat->kswapd_order = 0;
		pgdat->kswapd_classzone_idx = MAX_NR_ZONES;

so free_area_init_node absolutely has to hit this warning. This is not
in the Linus tree because it is still in Andrew's mmotm coming from
http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-vmscan-prevent-kswapd-sleeping-prematurely-due-to-mismatched-classzone_idx.patch

So yay, finally that doesn't come from me. Mel, I guess that either
hotadd_new_pgdat should keep its kswapd_classzone_idx = 0 or the warning
should be updated.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1618165

On Thu, Apr 06, 2017 at 06:21:55PM +0200, Michal Hocko wrote:
> On Thu 06-04-17 10:46:53, Reza Arbab wrote:
> > On Thu, Apr 06, 2017 at 05:41:28PM +0200, Michal Hocko wrote:
> > >On Thu 06-04-17 10:24:49, Reza Arbab wrote:
> > >>On Thu, Apr 06, 2017 at 03:08:46PM +0200, Michal Hocko wrote:
> > >>>OK, so after recent change mostly driven by testing from Reza Arbab
> > >>>(thanks again) I believe I am getting to a working state finally. All I
> > >>>currently have is
> > >>>in git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git tree
> > >>>attempts/rewrite-mem_hotplug-WIP branch. I will highly appreciate more
> > >>>testing of course and if there are no new issues found I will repost the
> > >>>series for the review.
> > >>
> > >>Looking good! I can do my add/remove/repeat test and things seem fine.
> > >>
> > >>One thing--starting on the second iteration, I am seeing the WARN in
> > >>free_area_init_node();
> > >>
> > >>add_memory
> > >> add_memory_resource
> > >>   hotadd_new_pgdat
> > >>     free_area_init_node
> > >>	WARN_ON(pgdat->nr_zones || pgdat->kswapd_classzone_idx);
> > >
> > >Have you tested with my attempts/rewrite-mem_hotplug-WIP mentioned
> > >elsewhere? Because I suspect that "mm: get rid of zone_is_initialized"
> > >might cause this.
> > 
> > This was my first time using your git branch instead of applying the patches
> > from this thread to v4.11-rc5 myself.
> 
> OK, so this looks like another thing to resolve. I have seen this
> warning as well but I didn't consider it relevant because I had to tweak
> the code make the node go offline (removed check_and_unmap_cpu_on_node
> from try_offline_node) so I thought it was a fallout from there. 
> 
> But let's have a look. hotadd_new_pgdat does for an existing pgdat
> 		/* Reset the nr_zones, order and classzone_idx before reuse */
> 		pgdat->nr_zones = 0;
> 		pgdat->kswapd_order = 0;
> 		pgdat->kswapd_classzone_idx = MAX_NR_ZONES;
> 
> so free_area_init_node absolutely has to hit this warning. This is not
> in the Linus tree because it is still in Andrew's mmotm coming from
> http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-vmscan-prevent-kswapd-sleeping-prematurely-due-to-mismatched-classzone_idx.patch
> 
> So yay, finally that doesn't come from me. Mel, I guess that either
> hotadd_new_pgdat should keep its kswapd_classzone_idx = 0 or the warning
> should be updated.
> 

Almost certainly the case that the warning should be updated.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1618187

On Thu, Apr 06, 2017 at 06:21:55PM +0200, Michal Hocko wrote:
> > This was my first time using your git branch instead of applying the patches
> > from this thread to v4.11-rc5 myself.
> 
> OK, so this looks like another thing to resolve. I have seen this
> warning as well but I didn't consider it relevant because I had to tweak
> the code make the node go offline (removed check_and_unmap_cpu_on_node
> from try_offline_node) so I thought it was a fallout from there. 
> 
> But let's have a look. hotadd_new_pgdat does for an existing pgdat
> 		/* Reset the nr_zones, order and classzone_idx before reuse */
> 		pgdat->nr_zones = 0;
> 		pgdat->kswapd_order = 0;
> 		pgdat->kswapd_classzone_idx = MAX_NR_ZONES;
> 
> so free_area_init_node absolutely has to hit this warning. This is not
> in the Linus tree because it is still in Andrew's mmotm coming from
> http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-vmscan-prevent-kswapd-sleeping-prematurely-due-to-mismatched-classzone_idx.patch
> 
> So yay, finally that doesn't come from me. Mel, I guess that either
> hotadd_new_pgdat should keep its kswapd_classzone_idx = 0 or the warning
> should be updated.
> 

Actually, it's obvious very quickly when I started the fix that updating
the warning would then trigger on normal boot. It's more appropriate to
let a hotadd of a new pgdat defer the initialisation of that field to
kswapd starting for the new node.

Can you try this? It's build/boot tested only, no hotplug testing.

---8<---
mm, vmscan: prevent kswapd sleeping prematurely due to mismatched classzone_idx -fix

The patch "mm, vmscan: prevent kswapd sleeping prematurely due to mismatched
classzone_idx" has different initial starting conditions when kswapd
is asleep. kswapd initialises it properly when it starts but the patch
initialises kswapd_classzone_idx early and trips on a warning in
free_area_init_node. This patch leaves the kswapd_classzone_idx as zero
and defers to kswapd to initialise it properly when it starts.

This is a fix to the mmotm patch
mm-vmscan-prevent-kswapd-sleeping-prematurely-due-to-mismatched-classzone_idx.patch

Signed-off-by: Mel Gorman <mgorman@suse.de>

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 2309a7fbec93..76d4745513ee 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1214,10 +1214,14 @@ static pg_data_t __ref *hotadd_new_pgdat(int nid, u64 start)
 
 		arch_refresh_nodedata(nid, pgdat);
 	} else {
-		/* Reset the nr_zones, order and classzone_idx before reuse */
+		/*
+		 * Reset the nr_zones, order and classzone_idx before reuse.
+		 * Note that kswapd will init kswapd_classzone_idx properly
+		 * when it starts in the near future.
+		 */
 		pgdat->nr_zones = 0;
 		pgdat->kswapd_order = 0;
-		pgdat->kswapd_classzone_idx = MAX_NR_ZONES;
+		pgdat->kswapd_classzone_idx = 0;
 	}
 
 	/* we can use NODE_DATA(nid) from here */

[toc] | [prev] | [next] | [standalone]

#1618203

On Thu 06-04-17 17:55:20, Mel Gorman wrote:
> On Thu, Apr 06, 2017 at 06:21:55PM +0200, Michal Hocko wrote:
> > > This was my first time using your git branch instead of applying the patches
> > > from this thread to v4.11-rc5 myself.
> > 
> > OK, so this looks like another thing to resolve. I have seen this
> > warning as well but I didn't consider it relevant because I had to tweak
> > the code make the node go offline (removed check_and_unmap_cpu_on_node
> > from try_offline_node) so I thought it was a fallout from there. 
> > 
> > But let's have a look. hotadd_new_pgdat does for an existing pgdat
> > 		/* Reset the nr_zones, order and classzone_idx before reuse */
> > 		pgdat->nr_zones = 0;
> > 		pgdat->kswapd_order = 0;
> > 		pgdat->kswapd_classzone_idx = MAX_NR_ZONES;
> > 
> > so free_area_init_node absolutely has to hit this warning. This is not
> > in the Linus tree because it is still in Andrew's mmotm coming from
> > http://www.ozlabs.org/~akpm/mmotm/broken-out/mm-vmscan-prevent-kswapd-sleeping-prematurely-due-to-mismatched-classzone_idx.patch
> > 
> > So yay, finally that doesn't come from me. Mel, I guess that either
> > hotadd_new_pgdat should keep its kswapd_classzone_idx = 0 or the warning
> > should be updated.
> > 
> 
> Actually, it's obvious very quickly when I started the fix that updating
> the warning would then trigger on normal boot. It's more appropriate to
> let a hotadd of a new pgdat defer the initialisation of that field to
> kswapd starting for the new node.
> 
> Can you try this? It's build/boot tested only, no hotplug testing.
> 
> ---8<---
> mm, vmscan: prevent kswapd sleeping prematurely due to mismatched classzone_idx -fix
> 
> The patch "mm, vmscan: prevent kswapd sleeping prematurely due to mismatched
> classzone_idx" has different initial starting conditions when kswapd
> is asleep. kswapd initialises it properly when it starts but the patch
> initialises kswapd_classzone_idx early and trips on a warning in
> free_area_init_node. This patch leaves the kswapd_classzone_idx as zero
> and defers to kswapd to initialise it properly when it starts.

It will start during the online phase which is later than this physical
memory hotadd.

> This is a fix to the mmotm patch
> mm-vmscan-prevent-kswapd-sleeping-prematurely-due-to-mismatched-classzone_idx.patch
> 
> Signed-off-by: Mel Gorman <mgorman@suse.de>

Yes, that is what I would expect. Feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>

if this is routed as a separate patch. Although I expect Andrew will
fold it into the original patch.
 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 2309a7fbec93..76d4745513ee 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1214,10 +1214,14 @@ static pg_data_t __ref *hotadd_new_pgdat(int nid, u64 start)
>  
>  		arch_refresh_nodedata(nid, pgdat);
>  	} else {
> -		/* Reset the nr_zones, order and classzone_idx before reuse */
> +		/*
> +		 * Reset the nr_zones, order and classzone_idx before reuse.
> +		 * Note that kswapd will init kswapd_classzone_idx properly
> +		 * when it starts in the near future.
> +		 */
>  		pgdat->nr_zones = 0;
>  		pgdat->kswapd_order = 0;
> -		pgdat->kswapd_classzone_idx = MAX_NR_ZONES;
> +		pgdat->kswapd_classzone_idx = 0;
>  	}
>  
>  	/* we can use NODE_DATA(nid) from here */
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1618223

On Thu, Apr 06, 2017 at 07:12:42PM +0200, Michal Hocko wrote:
> > ---8<---
> > mm, vmscan: prevent kswapd sleeping prematurely due to mismatched classzone_idx -fix
> > 
> > The patch "mm, vmscan: prevent kswapd sleeping prematurely due to mismatched
> > classzone_idx" has different initial starting conditions when kswapd
> > is asleep. kswapd initialises it properly when it starts but the patch
> > initialises kswapd_classzone_idx early and trips on a warning in
> > free_area_init_node. This patch leaves the kswapd_classzone_idx as zero
> > and defers to kswapd to initialise it properly when it starts.
> 
> It will start during the online phase which is later than this physical
> memory hotadd.
> 

Good, that's what appeared to be happening at least. It would be
somewhat insane if kswapd was running before zones were initialised.

> > This is a fix to the mmotm patch
> > mm-vmscan-prevent-kswapd-sleeping-prematurely-due-to-mismatched-classzone_idx.patch
> > 
> > Signed-off-by: Mel Gorman <mgorman@suse.de>
> 
> Yes, that is what I would expect. Feel free to add
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> if this is routed as a separate patch. Although I expect Andrew will
> fold it into the original patch.
> 

I added the ack anyway and resent the patch so it doesn't get lost in
the middle of a thread. Thanks.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

Page 3 of 4 — ← Prev page 1 2 [3] 4  Next page →

Back to top | Article view | linux.kernel

csiph-web