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 2 of 4 — ← Prev page 1 [2] 3 4  Next page →

#1614263

On Thu, Mar 30, 2017 at 01:54:48PM +0200, Michal Hocko wrote:
> Patch 5 is the core of the change. In order to make it easier to review
> I have tried it to be as minimalistic as possible and the large code
> removal is moved to patch 6.
> 
> I would appreciate if s390 folks could take a look at patch 4 and the
> arch_add_memory because I am not sure I've grokked what they wanted to
> achieve there completely.

[adding Gerald Schaefer]

This seems to work fine on s390. So for the s390 bits:
Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>

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

#1614938

On Fri 31-03-17 21:19:24, Heiko Carstens wrote:
> On Thu, Mar 30, 2017 at 01:54:48PM +0200, Michal Hocko wrote:
> > Patch 5 is the core of the change. In order to make it easier to review
> > I have tried it to be as minimalistic as possible and the large code
> > removal is moved to patch 6.
> > 
> > I would appreciate if s390 folks could take a look at patch 4 and the
> > arch_add_memory because I am not sure I've grokked what they wanted to
> > achieve there completely.
> 
> [adding Gerald Schaefer]
> 
> This seems to work fine on s390. So for the s390 bits:
> Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>

Thanks a lot!
-- 
Michal Hocko
SUSE Labs

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

#1615136

On Thu 30-03-17 13:54:48, Michal Hocko wrote:
[...]
> Any thoughts, complains, suggestions?

Anyting? I would really appreciate a feedback from IBM and Futjitsu guys
who have shaped this code last few years. Also Igor and Vitaly seem to
be using memory hotplug in virtualized environments. I do not expect
they would see a huge advantage of the rework but I would appreciate
to give it some testing to catch any potential regressions.

I plan to repost the series and would like to prevent from pointless
submission if there are any obvious issues.

Thanks!
-- 
Michal Hocko
SUSE Labs

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

#1615160

On Mon, 3 Apr 2017 13:55:46 +0200
Michal Hocko <mhocko@kernel.org> wrote:

> On Thu 30-03-17 13:54:48, Michal Hocko wrote:
> [...]
> > Any thoughts, complains, suggestions?  
> 
> Anyting? I would really appreciate a feedback from IBM and Futjitsu guys
> who have shaped this code last few years. Also Igor and Vitaly seem to
> be using memory hotplug in virtualized environments. I do not expect
> they would see a huge advantage of the rework but I would appreciate
> to give it some testing to catch any potential regressions.
I really appreciate this rework as it simplifies code a bit and potentially
would allow me/Vitaly to make auto-online work with movable zone as well.

I'll try to test the series within this week.

> 
> I plan to repost the series and would like to prevent from pointless
> submission if there are any obvious issues.
> 
> Thanks!

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

#1615525

On Mon, Apr 03, 2017 at 01:55:46PM +0200, Michal Hocko wrote:
>Anyting? I would really appreciate a feedback from IBM and Futjitsu 
>guys who have shaped this code last few years. Also Igor and Vitaly 
>seem to be using memory hotplug in virtualized environments. I do not 
>expect they would see a huge advantage of the rework but I would 
>appreciate to give it some testing to catch any potential regressions.

Sorry for the delayed reply.

With this set, I'm able to "online_movable" blocks in ascending order.  

However, I am seeing a regression. When adding memory to a memoryless 
node, it shows up in node 0 instead. I'm digging to see if I can help 
narrow down where things go wrong.

-- 
Reza Arbab

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

#1615536

On Mon 03-04-17 14:58:30, Reza Arbab wrote:
> On Mon, Apr 03, 2017 at 01:55:46PM +0200, Michal Hocko wrote:
> >Anyting? I would really appreciate a feedback from IBM and Futjitsu guys
> >who have shaped this code last few years. Also Igor and Vitaly seem to be
> >using memory hotplug in virtualized environments. I do not expect they
> >would see a huge advantage of the rework but I would appreciate to give it
> >some testing to catch any potential regressions.
> 
> Sorry for the delayed reply.
> 
> With this set, I'm able to "online_movable" blocks in ascending order.
> 
> However, I am seeing a regression. When adding memory to a memoryless node,
> it shows up in node 0 instead. I'm digging to see if I can help narrow down
> where things go wrong.

OK, I guess I know what is going on here. online_pages relies on
pfn_to_nid(pfn) to return a proper node. But we are doing
page_to_nid(pfn_to_page(__pfn_to_nid_pfn)) so we rely on the page being
properly initialized. Damn, I should have noticed that. There are two
ways around that. Either the __add_section stores the nid into the
struct page and make page_to_nid reliable or store it somewhere else
(ideally into the memblock). The first is easier (let's do it for now)
but longterm we do not want to rely on the struct page at all I believe.

Does the following help?
---
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index b9dc1c4e26c3..0e21b9f67c9d 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -309,14 +309,19 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn)
 
 	/*
 	 * Make all the pages reserved so that nobody will stumble over half
-	 * initialized state.
+	 * initialized state. 
+	 * FIXME: We also have to associate it with a node because pfn_to_node
+	 * relies on having page with the proper node.
 	 */
 	for (i = 0; i < PAGES_PER_SECTION; i++) {
 		unsigned long pfn = phys_start_pfn + i;
+		struct page *page;
 		if (!pfn_valid(pfn))
 			continue;
 
-		SetPageReserved(pfn_to_page(phys_start_pfn + i));
+		page = pfn_to_page(pfn);
+		set_page_node(page, nid);
+		SetPageReserved(page);
 	}
 
 	return register_new_memory(nid, __pfn_to_section(phys_start_pfn));
-- 
Michal Hocko
SUSE Labs

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

#1615552

On Mon, Apr 03, 2017 at 10:23:38PM +0200, Michal Hocko wrote:
>On Mon 03-04-17 14:58:30, Reza Arbab wrote:
>> However, I am seeing a regression. When adding memory to a memoryless 
>> node, it shows up in node 0 instead. I'm digging to see if I can help 
>> narrow down where things go wrong.
>
>OK, I guess I know what is going on here. online_pages relies on
>pfn_to_nid(pfn) to return a proper node. But we are doing
>page_to_nid(pfn_to_page(__pfn_to_nid_pfn)) so we rely on the page being
>properly initialized. Damn, I should have noticed that. There are two
>ways around that. Either the __add_section stores the nid into the
>struct page and make page_to_nid reliable or store it somewhere else
>(ideally into the memblock). The first is easier (let's do it for now)
>but longterm we do not want to rely on the struct page at all I believe.
>
>Does the following help?
>---
>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index b9dc1c4e26c3..0e21b9f67c9d 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -309,14 +309,19 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn)
>
> 	/*
> 	 * Make all the pages reserved so that nobody will stumble over half
>-	 * initialized state.
>+	 * initialized state.
>+	 * FIXME: We also have to associate it with a node because pfn_to_node
>+	 * relies on having page with the proper node.
> 	 */
> 	for (i = 0; i < PAGES_PER_SECTION; i++) {
> 		unsigned long pfn = phys_start_pfn + i;
>+		struct page *page;
> 		if (!pfn_valid(pfn))
> 			continue;
>
>-		SetPageReserved(pfn_to_page(phys_start_pfn + i));
>+		page = pfn_to_page(pfn);
>+		set_page_node(page, nid);
>+		SetPageReserved(page);
> 	}
>
> 	return register_new_memory(nid, __pfn_to_section(phys_start_pfn));

Almost there. I'm seeing the memory in the correct node now, but the 
/sys/devices/system/node/nodeX/memoryY links are not being created.

I think it's tripping up here, in register_mem_sect_under_node():

		page_nid = get_nid_for_pfn(pfn);
		if (page_nid < 0)
			continue;

-- 
Reza Arbab

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

#1615745

[Let's add Gary who as introduced this code c04fc586c1a48]

On Mon 03-04-17 15:42:13, Reza Arbab wrote:
> On Mon, Apr 03, 2017 at 10:23:38PM +0200, Michal Hocko wrote:
> >On Mon 03-04-17 14:58:30, Reza Arbab wrote:
> >>However, I am seeing a regression. When adding memory to a memoryless
> >>node, it shows up in node 0 instead. I'm digging to see if I can help
> >>narrow down where things go wrong.
> >
> >OK, I guess I know what is going on here. online_pages relies on
> >pfn_to_nid(pfn) to return a proper node. But we are doing
> >page_to_nid(pfn_to_page(__pfn_to_nid_pfn)) so we rely on the page being
> >properly initialized. Damn, I should have noticed that. There are two
> >ways around that. Either the __add_section stores the nid into the
> >struct page and make page_to_nid reliable or store it somewhere else
> >(ideally into the memblock). The first is easier (let's do it for now)
> >but longterm we do not want to rely on the struct page at all I believe.
> >
> >Does the following help?
> >---
> >diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> >index b9dc1c4e26c3..0e21b9f67c9d 100644
> >--- a/mm/memory_hotplug.c
> >+++ b/mm/memory_hotplug.c
> >@@ -309,14 +309,19 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn)
> >
> >	/*
> >	 * Make all the pages reserved so that nobody will stumble over half
> >-	 * initialized state.
> >+	 * initialized state.
> >+	 * FIXME: We also have to associate it with a node because pfn_to_node
> >+	 * relies on having page with the proper node.
> >	 */
> >	for (i = 0; i < PAGES_PER_SECTION; i++) {
> >		unsigned long pfn = phys_start_pfn + i;
> >+		struct page *page;
> >		if (!pfn_valid(pfn))
> >			continue;
> >
> >-		SetPageReserved(pfn_to_page(phys_start_pfn + i));
> >+		page = pfn_to_page(pfn);
> >+		set_page_node(page, nid);
> >+		SetPageReserved(page);
> >	}
> >
> >	return register_new_memory(nid, __pfn_to_section(phys_start_pfn));
> 
> Almost there. I'm seeing the memory in the correct node now, but the
> /sys/devices/system/node/nodeX/memoryY links are not being created.
> 
> I think it's tripping up here, in register_mem_sect_under_node():
> 
> 		page_nid = get_nid_for_pfn(pfn);
> 		if (page_nid < 0)
> 			continue;

Huh, this code is confusing. How can we have a memblock spanning more
nodes? If not then the loop over all sections in the memblock seem
pointless as well.  Also why do we require page_initialized() in
get_nid_for_pfn? The changelog doesn't explain that and there are no
comments that would help either.

Gary, could you clarify this please?
-- 
Michal Hocko
SUSE Labs

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

#1615750

On Tue 04-04-17 09:23:29, Michal Hocko wrote:
> [Let's add Gary who as introduced this code c04fc586c1a48]

OK, so Gary's email doesn't exist anymore. Does anybody can comment on
this? I suspect this code is just-in-case... Mel?
 
> On Mon 03-04-17 15:42:13, Reza Arbab wrote:
[...]
> > Almost there. I'm seeing the memory in the correct node now, but the
> > /sys/devices/system/node/nodeX/memoryY links are not being created.
> > 
> > I think it's tripping up here, in register_mem_sect_under_node():
> > 
> > 		page_nid = get_nid_for_pfn(pfn);
> > 		if (page_nid < 0)
> > 			continue;
> 
> Huh, this code is confusing. How can we have a memblock spanning more
> nodes? If not then the loop over all sections in the memblock seem
> pointless as well.  Also why do we require page_initialized() in
> get_nid_for_pfn? The changelog doesn't explain that and there are no
> comments that would help either.
> 
> Gary, could you clarify this please?
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

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

#1615776

On Tue 04-04-17 09:34:12, Michal Hocko wrote:
> On Tue 04-04-17 09:23:29, Michal Hocko wrote:
> > [Let's add Gary who as introduced this code c04fc586c1a48]
> 
> OK, so Gary's email doesn't exist anymore. Does anybody can comment on
> this? I suspect this code is just-in-case... Mel?
>  
> > On Mon 03-04-17 15:42:13, Reza Arbab wrote:
> [...]
> > > Almost there. I'm seeing the memory in the correct node now, but the
> > > /sys/devices/system/node/nodeX/memoryY links are not being created.
> > > 
> > > I think it's tripping up here, in register_mem_sect_under_node():
> > > 
> > > 		page_nid = get_nid_for_pfn(pfn);
> > > 		if (page_nid < 0)
> > > 			continue;
> > 
> > Huh, this code is confusing. How can we have a memblock spanning more
> > nodes? If not then the loop over all sections in the memblock seem
> > pointless as well.  Also why do we require page_initialized() in
> > get_nid_for_pfn? The changelog doesn't explain that and there are no
> > comments that would help either.

OK, so I've been thinkin about that and I believe that page_initialized
check in get_nid_for_pfn is just bogus. There is nothing to rely on the
page::lru to be already initialized. So I will go with the following as
a separate preparatory patch.

I believe the whole code should be revisited and I have put that on my
ever growing todo list because I suspect that it is more complex than
necessary. I suspect that memblock do not span more nodes and all this
is just-in-case code (e.g. the onlining code assumes a single zone aka
node. But let's do that later.

---
From fd2e3b6eca1cf7766527203d23db6aca5957a3f1 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Tue, 4 Apr 2017 10:05:06 +0200
Subject: [PATCH] mm: drop page_initialized check from get_nid_for_pfn

c04fc586c1a4 ("mm: show node to memory section relationship with
symlinks in sysfs") has added means to export memblock<->node
association into the sysfs. It has also introduced get_nid_for_pfn
which is a rather confusing counterpart of pfn_to_nid which checks also
whether the pfn page is already initialized (page_initialized).  This
is done by checking page::lru != NULL which doesn't make any sense at
all. Nothing in this path really relies on the lru list being used or
initialized. Just remove it

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 drivers/base/node.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index 5548f9686016..ee080a35e869 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -368,8 +368,6 @@ int unregister_cpu_under_node(unsigned int cpu, unsigned int nid)
 }
 
 #ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
-#define page_initialized(page)  (page->lru.next)
-
 static int __ref get_nid_for_pfn(unsigned long pfn)
 {
 	struct page *page;
@@ -380,9 +378,6 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
 	if (system_state == SYSTEM_BOOTING)
 		return early_pfn_to_nid(pfn);
 #endif
-	page = pfn_to_page(pfn);
-	if (!page_initialized(page))
-		return -1;
 	return pfn_to_nid(pfn);
 }
 
-- 
2.11.0

-- 
Michal Hocko
SUSE Labs

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

#1616153

On Tue, Apr 04, 2017 at 10:23:02AM +0200, Michal Hocko wrote:
>OK, so I've been thinkin about that and I believe that page_initialized
>check in get_nid_for_pfn is just bogus. There is nothing to rely on the
>page::lru to be already initialized. So I will go with the following as
>a separate preparatory patch.
>
>I believe the whole code should be revisited and I have put that on my
>ever growing todo list because I suspect that it is more complex than
>necessary. I suspect that memblock do not span more nodes and all this
>is just-in-case code (e.g. the onlining code assumes a single zone aka
>node. But let's do that later.
>
>---
>From fd2e3b6eca1cf7766527203d23db6aca5957a3f1 Mon Sep 17 00:00:00 2001
>From: Michal Hocko <mhocko@suse.com>
>Date: Tue, 4 Apr 2017 10:05:06 +0200
>Subject: [PATCH] mm: drop page_initialized check from get_nid_for_pfn
>
>c04fc586c1a4 ("mm: show node to memory section relationship with
>symlinks in sysfs") has added means to export memblock<->node
>association into the sysfs. It has also introduced get_nid_for_pfn
>which is a rather confusing counterpart of pfn_to_nid which checks also
>whether the pfn page is already initialized (page_initialized).  This
>is done by checking page::lru != NULL which doesn't make any sense at
>all. Nothing in this path really relies on the lru list being used or
>initialized. Just remove it
>
>Signed-off-by: Michal Hocko <mhocko@suse.com>
>---
> drivers/base/node.c | 5 -----
> 1 file changed, 5 deletions(-)
>
>diff --git a/drivers/base/node.c b/drivers/base/node.c
>index 5548f9686016..ee080a35e869 100644
>--- a/drivers/base/node.c
>+++ b/drivers/base/node.c
>@@ -368,8 +368,6 @@ int unregister_cpu_under_node(unsigned int cpu, unsigned int nid)
> }
>
> #ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
>-#define page_initialized(page)  (page->lru.next)
>-
> static int __ref get_nid_for_pfn(unsigned long pfn)
> {
> 	struct page *page;
>@@ -380,9 +378,6 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
> 	if (system_state == SYSTEM_BOOTING)
> 		return early_pfn_to_nid(pfn);
> #endif
>-	page = pfn_to_page(pfn);
>-	if (!page_initialized(page))
>-		return -1;
> 	return pfn_to_nid(pfn);
> }
>

Verified that /sys/devices/system/node/nodeX/memoryY links are there now.

-- 
Reza Arbab

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

#1616166

On Tue, Apr 04, 2017 at 10:23:02AM +0200, Michal Hocko wrote:
>diff --git a/drivers/base/node.c b/drivers/base/node.c
>index 5548f9686016..ee080a35e869 100644
>--- a/drivers/base/node.c
>+++ b/drivers/base/node.c
>@@ -368,8 +368,6 @@ int unregister_cpu_under_node(unsigned int cpu, unsigned int nid)
> }
>
> #ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
>-#define page_initialized(page)  (page->lru.next)
>-
> static int __ref get_nid_for_pfn(unsigned long pfn)
> {
> 	struct page *page;
>@@ -380,9 +378,6 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
> 	if (system_state == SYSTEM_BOOTING)
> 		return early_pfn_to_nid(pfn);
> #endif
>-	page = pfn_to_page(pfn);
>-	if (!page_initialized(page))
>-		return -1;
> 	return pfn_to_nid(pfn);
> }
>

You can get rid of 'page' altogether.

drivers/base/node.c: In function ‘get_nid_for_pfn’:
drivers/base/node.c:373:15: warning: unused variable ‘page’ [-Wunused-variable]

-- 
Reza Arbab

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

#1616202

On Tue 04-04-17 11:02:39, Reza Arbab wrote:
> On Tue, Apr 04, 2017 at 10:23:02AM +0200, Michal Hocko wrote:
> >diff --git a/drivers/base/node.c b/drivers/base/node.c
> >index 5548f9686016..ee080a35e869 100644
> >--- a/drivers/base/node.c
> >+++ b/drivers/base/node.c
> >@@ -368,8 +368,6 @@ int unregister_cpu_under_node(unsigned int cpu, unsigned int nid)
> >}
> >
> >#ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
> >-#define page_initialized(page)  (page->lru.next)
> >-
> >static int __ref get_nid_for_pfn(unsigned long pfn)
> >{
> >	struct page *page;
> >@@ -380,9 +378,6 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
> >	if (system_state == SYSTEM_BOOTING)
> >		return early_pfn_to_nid(pfn);
> >#endif
> >-	page = pfn_to_page(pfn);
> >-	if (!page_initialized(page))
> >-		return -1;
> >	return pfn_to_nid(pfn);
> >}
> >
> 
> You can get rid of 'page' altogether.
> 
> drivers/base/node.c: In function ‘get_nid_for_pfn’:
> drivers/base/node.c:373:15: warning: unused variable ‘page’ [-Wunused-variable]

Right, updated.

Thanks for your testing! This is highly appreciated.
Can I assume your Tested-by?
-- 
Michal Hocko
SUSE Labs

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

#1616308

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. 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

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


-- 
Reza Arbab

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

#1616360

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)

While the flexibility is attractive I do not think it is worth the
additional complexity without any proof of the usecase. Especially when
we consider that this only work when we offline from the start or end of
the zone or whole zone. I guess it would be the best to simply revert
this whole thing. It is quite a lot of code with a dubious use. What
do Futjitsu guys think about it?
---
From 1b08ecef3e8ebcef585fe8f2b23155be54cce335 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Tue, 4 Apr 2017 21:09:00 +0200
Subject: [PATCH] mm, hotplug: get rid of zone/node shrinking

this is basically a revert of 815121d2b5cd ("memory_hotplug: clear zone
when removing the memory"). While the node/zone shrinking sounds
attractive at first sight because it allows to
	online_movable(range)
	[...]
	offline_movable(range)
	[...]
	online_kernel(range)

but this requires that the range is in the beginning or the end of a
zone or operate on the whole zone basis. This code is even broken as
noticed by Reza Arbab. He has triggered an oops when doing the full node
offline
Unable to handle kernel paging request for data at address 0xc000800000f19e78
	__nr_to_section
	__pfn_to_section
	find_biggest_section_pfn
	shrink_pgdat_span
	__remove_zone
	__remove_section
	__remove_pages
	arch_remove_memory
	remove_memory
which is caused by an overflow in find_biggest_section_pfn. This code
simply cannot work on the first section [0, PAGES_PER_SECTION] because
	pfn = end_pfn - 1;
	for (; pfn >= start_pfn; pfn -= PAGES_PER_SECTION) {
pfn would underflow in the unsigned arithmetic. This doesn't happen
usually because the lowest pfns are usually reserved and out of the
hotplug reach.

The changelog of the above commit doesn't mention any such usecase and
sounds more like an nice-to-have and inverse to __add_zone which we are
trying to get rid of in this series. So let's simplify the code and
remove the complication.  We can reintroduce it back along with a valid
usecase description.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/memory_hotplug.c | 207 ----------------------------------------------------
 1 file changed, 207 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a358d7a67651..d48a4456b20d 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -379,212 +379,9 @@ EXPORT_SYMBOL_GPL(__add_pages);
 
 #ifdef CONFIG_MEMORY_HOTREMOVE
 /* find the smallest valid pfn in the range [start_pfn, end_pfn) */
-static int find_smallest_section_pfn(int nid, struct zone *zone,
-				     unsigned long start_pfn,
-				     unsigned long end_pfn)
-{
-	struct mem_section *ms;
-
-	for (; start_pfn < end_pfn; start_pfn += PAGES_PER_SECTION) {
-		ms = __pfn_to_section(start_pfn);
-
-		if (unlikely(!valid_section(ms)))
-			continue;
-
-		if (unlikely(pfn_to_nid(start_pfn) != nid))
-			continue;
-
-		if (zone && zone != page_zone(pfn_to_page(start_pfn)))
-			continue;
-
-		return start_pfn;
-	}
-
-	return 0;
-}
-
-/* find the biggest valid pfn in the range [start_pfn, end_pfn). */
-static int find_biggest_section_pfn(int nid, struct zone *zone,
-				    unsigned long start_pfn,
-				    unsigned long end_pfn)
-{
-	struct mem_section *ms;
-	unsigned long pfn;
-
-	/* pfn is the end pfn of a memory section. */
-	pfn = end_pfn - 1;
-	for (; pfn >= start_pfn; pfn -= PAGES_PER_SECTION) {
-		ms = __pfn_to_section(pfn);
-
-		if (unlikely(!valid_section(ms)))
-			continue;
-
-		if (unlikely(pfn_to_nid(pfn) != nid))
-			continue;
-
-		if (zone && zone != page_zone(pfn_to_page(pfn)))
-			continue;
-
-		return pfn;
-	}
-
-	return 0;
-}
-
-static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
-			     unsigned long end_pfn)
-{
-	unsigned long zone_start_pfn = zone->zone_start_pfn;
-	unsigned long z = zone_end_pfn(zone); /* zone_end_pfn namespace clash */
-	unsigned long zone_end_pfn = z;
-	unsigned long pfn;
-	struct mem_section *ms;
-	int nid = zone_to_nid(zone);
-
-	zone_span_writelock(zone);
-	if (zone_start_pfn == start_pfn) {
-		/*
-		 * If the section is smallest section in the zone, it need
-		 * shrink zone->zone_start_pfn and zone->zone_spanned_pages.
-		 * In this case, we find second smallest valid mem_section
-		 * for shrinking zone.
-		 */
-		pfn = find_smallest_section_pfn(nid, zone, end_pfn,
-						zone_end_pfn);
-		if (pfn) {
-			zone->zone_start_pfn = pfn;
-			zone->spanned_pages = zone_end_pfn - pfn;
-		}
-	} else if (zone_end_pfn == end_pfn) {
-		/*
-		 * If the section is biggest section in the zone, it need
-		 * shrink zone->spanned_pages.
-		 * In this case, we find second biggest valid mem_section for
-		 * shrinking zone.
-		 */
-		pfn = find_biggest_section_pfn(nid, zone, zone_start_pfn,
-					       start_pfn);
-		if (pfn)
-			zone->spanned_pages = pfn - zone_start_pfn + 1;
-	}
-
-	/*
-	 * The section is not biggest or smallest mem_section in the zone, it
-	 * only creates a hole in the zone. So in this case, we need not
-	 * change the zone. But perhaps, the zone has only hole data. Thus
-	 * it check the zone has only hole or not.
-	 */
-	pfn = zone_start_pfn;
-	for (; pfn < zone_end_pfn; pfn += PAGES_PER_SECTION) {
-		ms = __pfn_to_section(pfn);
-
-		if (unlikely(!valid_section(ms)))
-			continue;
-
-		if (page_zone(pfn_to_page(pfn)) != zone)
-			continue;
-
-		 /* If the section is current section, it continues the loop */
-		if (start_pfn == pfn)
-			continue;
-
-		/* If we find valid section, we have nothing to do */
-		zone_span_writeunlock(zone);
-		return;
-	}
-
-	/* The zone has no valid section */
-	zone->zone_start_pfn = 0;
-	zone->spanned_pages = 0;
-	zone_span_writeunlock(zone);
-}
-
-static void shrink_pgdat_span(struct pglist_data *pgdat,
-			      unsigned long start_pfn, unsigned long end_pfn)
-{
-	unsigned long pgdat_start_pfn = pgdat->node_start_pfn;
-	unsigned long p = pgdat_end_pfn(pgdat); /* pgdat_end_pfn namespace clash */
-	unsigned long pgdat_end_pfn = p;
-	unsigned long pfn;
-	struct mem_section *ms;
-	int nid = pgdat->node_id;
-
-	if (pgdat_start_pfn == start_pfn) {
-		/*
-		 * If the section is smallest section in the pgdat, it need
-		 * shrink pgdat->node_start_pfn and pgdat->node_spanned_pages.
-		 * In this case, we find second smallest valid mem_section
-		 * for shrinking zone.
-		 */
-		pfn = find_smallest_section_pfn(nid, NULL, end_pfn,
-						pgdat_end_pfn);
-		if (pfn) {
-			pgdat->node_start_pfn = pfn;
-			pgdat->node_spanned_pages = pgdat_end_pfn - pfn;
-		}
-	} else if (pgdat_end_pfn == end_pfn) {
-		/*
-		 * If the section is biggest section in the pgdat, it need
-		 * shrink pgdat->node_spanned_pages.
-		 * In this case, we find second biggest valid mem_section for
-		 * shrinking zone.
-		 */
-		pfn = find_biggest_section_pfn(nid, NULL, pgdat_start_pfn,
-					       start_pfn);
-		if (pfn)
-			pgdat->node_spanned_pages = pfn - pgdat_start_pfn + 1;
-	}
-
-	/*
-	 * If the section is not biggest or smallest mem_section in the pgdat,
-	 * it only creates a hole in the pgdat. So in this case, we need not
-	 * change the pgdat.
-	 * But perhaps, the pgdat has only hole data. Thus it check the pgdat
-	 * has only hole or not.
-	 */
-	pfn = pgdat_start_pfn;
-	for (; pfn < pgdat_end_pfn; pfn += PAGES_PER_SECTION) {
-		ms = __pfn_to_section(pfn);
-
-		if (unlikely(!valid_section(ms)))
-			continue;
-
-		if (pfn_to_nid(pfn) != nid)
-			continue;
-
-		 /* If the section is current section, it continues the loop */
-		if (start_pfn == pfn)
-			continue;
-
-		/* If we find valid section, we have nothing to do */
-		return;
-	}
-
-	/* The pgdat has no valid section */
-	pgdat->node_start_pfn = 0;
-	pgdat->node_spanned_pages = 0;
-}
-
-static void __remove_zone(struct zone *zone, unsigned long start_pfn)
-{
-	struct pglist_data *pgdat = zone->zone_pgdat;
-	int nr_pages = PAGES_PER_SECTION;
-	int zone_type;
-	unsigned long flags;
-
-	zone_type = zone - pgdat->node_zones;
-
-	pgdat_resize_lock(zone->zone_pgdat, &flags);
-	shrink_zone_span(zone, start_pfn, start_pfn + nr_pages);
-	shrink_pgdat_span(pgdat, start_pfn, start_pfn + nr_pages);
-	pgdat_resize_unlock(zone->zone_pgdat, &flags);
-}
-
 static int __remove_section(struct zone *zone, struct mem_section *ms,
 		unsigned long map_offset)
 {
-	unsigned long start_pfn;
-	int scn_nr;
 	int ret = -EINVAL;
 
 	if (!valid_section(ms))
@@ -594,10 +391,6 @@ static int __remove_section(struct zone *zone, struct mem_section *ms,
 	if (ret)
 		return ret;
 
-	scn_nr = __section_nr(ms);
-	start_pfn = section_nr_to_pfn(scn_nr);
-	__remove_zone(zone, start_pfn);
-
 	sparse_remove_one_section(zone, ms, map_offset);
 	return 0;
 }
-- 
2.11.0


-- 
Michal Hocko
SUSE Labs

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

#1616391

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.

>From 1b08ecef3e8ebcef585fe8f2b23155be54cce335 Mon Sep 17 00:00:00 2001
>From: Michal Hocko <mhocko@suse.com>
>Date: Tue, 4 Apr 2017 21:09:00 +0200
>Subject: [PATCH] mm, hotplug: get rid of zone/node shrinking
>
...%<...
>---
> mm/memory_hotplug.c | 207 ----------------------------------------------------
> 1 file changed, 207 deletions(-)

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.

-- 
Reza Arbab

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

#1616624

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.

This is quite unexpected because the code obviously cannot handle the
first memory section. Could you paste /proc/zoneinfo and
grep . -r /sys/devices/system/memory/auto_online_blocks/memory*, after
onlining for both patched and unpatched kernels?

> >From 1b08ecef3e8ebcef585fe8f2b23155be54cce335 Mon Sep 17 00:00:00 2001
> >From: Michal Hocko <mhocko@suse.com>
> >Date: Tue, 4 Apr 2017 21:09:00 +0200
> >Subject: [PATCH] mm, hotplug: get rid of zone/node shrinking
> >
> ...%<...
> >---
> >mm/memory_hotplug.c | 207 ----------------------------------------------------
> >1 file changed, 207 deletions(-)
> 
> 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?
-- 
Michal Hocko
SUSE Labs

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

#1616759

On Wed 05-04-17 08:42:39, Michal Hocko wrote:
> 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.
> 
> This is quite unexpected because the code obviously cannot handle the
> first memory section. Could you paste /proc/zoneinfo and
> grep . -r /sys/devices/system/memory/auto_online_blocks/memory*, after
> onlining for both patched and unpatched kernels?

Btw. how do you test this? I am really surprised you managed to
hotremove such a low pfn range.
-- 
Michal Hocko
SUSE Labs

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

#1617027

On Wed, Apr 05, 2017 at 11:24:27AM +0200, Michal Hocko wrote:
>On Wed 05-04-17 08:42:39, Michal Hocko wrote:
>> On Tue 04-04-17 16:43:39, Reza Arbab wrote:
>> > It's new. Without this patchset, I can repeatedly
>> > add_memory()->online_movable->offline->remove_memory() all of a node's
>> > memory.
>>
>> This is quite unexpected because the code obviously cannot handle the
>> first memory section. Could you paste /proc/zoneinfo and
>> grep . -r /sys/devices/system/memory/auto_online_blocks/memory*, after
>> onlining for both patched and unpatched kernels?
>
>Btw. how do you test this? I am really surprised you managed to
>hotremove such a low pfn range.

When I boot, I have node 0 (4GB) and node 1 (empty):

Early memory node ranges
  node   0: [mem 0x0000000000000000-0x00000000ffffffff]
Initmem setup node 0 [mem 0x0000000000000000-0x00000000ffffffff]
On node 0 totalpages: 65536
  DMA zone: 64 pages used for memmap
  DMA zone: 0 pages reserved
  DMA zone: 65536 pages, LIFO batch:1
Could not find start_pfn for node 1
Initmem setup node 1 [mem 0x0000000000000000-0x0000000000000000]
On node 1 totalpages: 0

My steps from there:

1. add_memory(1, 0x100000000, 0x100000000)
2. echo online_movable > /sys/devices/system/node/node1/memory[511..256]
3. echo offline > /sys/devices/system/node/node1/memory[256..511]
4. remove_memory(1, 0x100000000, 0x100000000)

After step 2, regardless of kernel:

$ cat /proc/zoneinfo
Node 0, zone      DMA
  per-node stats
      nr_inactive_anon 418
      nr_active_anon 2710
      nr_inactive_file 4895
      nr_active_file 1945
      nr_unevictable 0
      nr_isolated_anon 0
      nr_isolated_file 0
      nr_pages_scanned 0
      workingset_refault 0
      workingset_activate 0
      workingset_nodereclaim 0
      nr_anon_pages 2654
      nr_mapped    739
      nr_file_pages 7314
      nr_dirty     1
      nr_writeback 0
      nr_writeback_temp 0
      nr_shmem     474
      nr_shmem_hugepages 0
      nr_shmem_pmdmapped 0
      nr_anon_transparent_hugepages 0
      nr_unstable  0
      nr_vmscan_write 0
      nr_vmscan_immediate_reclaim 0
      nr_dirtied   3259
      nr_written   460
  pages free     53520
        min      63
        low      128
        high     193
   node_scanned  0
        spanned  65536
        present  65536
        managed  65218
      nr_free_pages 53520
      nr_zone_inactive_anon 418
      nr_zone_active_anon 2710
      nr_zone_inactive_file 4895
      nr_zone_active_file 1945
      nr_zone_unevictable 0
      nr_zone_write_pending 1
      nr_mlock     0
      nr_slab_reclaimable 438
      nr_slab_unreclaimable 808
      nr_page_table_pages 32
      nr_kernel_stack 2080
      nr_bounce    0
      numa_hit     313226
      numa_miss    0
      numa_foreign 0
      numa_interleave 3071
      numa_local   313226
      numa_other   0
      nr_free_cma  0
        protection: (0, 0, 0, 0)
  pagesets
    cpu: 0
              count: 2
              high:  6
              batch: 1
  vm stats threshold: 12
  node_unreclaimable:  0
  start_pfn:           0
  node_inactive_ratio: 0
Node 1, zone  Movable
  per-node stats
      nr_inactive_anon 0
      nr_active_anon 0
      nr_inactive_file 0
      nr_active_file 0
      nr_unevictable 0
      nr_isolated_anon 0
      nr_isolated_file 0
      nr_pages_scanned 0
      workingset_refault 0
      workingset_activate 0
      workingset_nodereclaim 0
      nr_anon_pages 0
      nr_mapped    0
      nr_file_pages 0
      nr_dirty     0
      nr_writeback 0
      nr_writeback_temp 0
      nr_shmem     0
      nr_shmem_hugepages 0
      nr_shmem_pmdmapped 0
      nr_anon_transparent_hugepages 0
      nr_unstable  0
      nr_vmscan_write 0
      nr_vmscan_immediate_reclaim 0
      nr_dirtied   0
      nr_written   0
  pages free     65536
        min      63
        low      128
        high     193
   node_scanned  0
        spanned  65536
        present  65536
        managed  65536
      nr_free_pages 65536
      nr_zone_inactive_anon 0
      nr_zone_active_anon 0
      nr_zone_inactive_file 0
      nr_zone_active_file 0
      nr_zone_unevictable 0
      nr_zone_write_pending 0
      nr_mlock     0
      nr_slab_reclaimable 0
      nr_slab_unreclaimable 0
      nr_page_table_pages 0
      nr_kernel_stack 0
      nr_bounce    0
      numa_hit     0
      numa_miss    0
      numa_foreign 0
      numa_interleave 0
      numa_local   0
      numa_other   0
      nr_free_cma  0
        protection: (0, 0, 0, 0)
  pagesets
    cpu: 0
              count: 0
              high:  6
              batch: 1
  vm stats threshold: 14
  node_unreclaimable:  1
  start_pfn:           65536
  node_inactive_ratio: 0

After step 2, on v4.11-rc5:

$ grep . /sys/devices/system/memory/memory*/valid_zones
/sys/devices/system/memory/memory[0..254]/valid_zones:DMA
/sys/devices/system/memory/memory255/valid_zones:DMA Normal Movable
/sys/devices/system/memory/memory256/valid_zones:Movable Normal
/sys/devices/system/memory/memory[257..511]/valid_zones:Movable

After step 2, on v4.11-rc5 + all the patches from this thread:

$ grep . /sys/devices/system/memory/memory*/valid_zones
/sys/devices/system/memory/memory[0..255]/valid_zones:DMA
/sys/devices/system/memory/memory[256..511]/valid_zones:Movable

On v4.11-rc5, I can do steps 1-4 ad nauseam.
On v4.11-rc5 + all the patches from this thread, I can do things 
repeatedly, but starting on the second iteration, all the

  /sys/devices/system/node/node1/memory*

symlinks are not created. I can still proceed using the actual files,

  /sys/devices/system/memory/memory[256..511]

instead. I think it may be because step 4 does node_set_offline(1). That 
is, the node is not only emptied of memory, it is offlined completely.

I hope this made sense. :/

-- 
Reza Arbab

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

#1617071

On Wed 05-04-17 09:53:05, Reza Arbab wrote:
[...]
> I hope this made sense. :/

yes it certainly helped me to make some picture of your setup. I will
keep thinking about that. 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.

I suspect I broke some hidden expectation that made add_memory vs.
remove_memory more symetric before. I will keep digging.
-- 
Michal Hocko
SUSE Labs

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

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

Back to top | Article view | linux.kernel

csiph-web