mem-hotplug: Don't clear the only node in new_node_page()

PATCH Applied Li Zhong 6 participants 17 messages Sep 5, 2016 – Sep 21, 2016

Subsystems (3)

Memory management Page allocator Memory hot(un)plug

lore

Share & tools

lore

Li Zhong

author

2016-09-05 03:00 UTC #0 lore raw

PATCH 1/1 mem-hotplug: Don't clear the only node in new_node_page()

Commit 394e31d2c introduced new_node_page() for memory hotplug.

In new_node_page(), the nid is cleared before calling __alloc_pages_nodemask(). But if it is the only node of the system, and the first round allocation fails, it will not be able to get memory from an empty nodemask, and trigger oom.

The patch checks whether it is the last node on the system, and if it is, then don't clear the nid in the nodemask.

Reported-by John Allen <[email protected]>

Signed-off-by Li Zhong <[email protected]>

---
 mm/memory_hotplug.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

mm/memory_hotplug.c +3 -1

index 41266dc..b58906b 100644
@@ -1567,7 +1567,9 @@ static struct page *new_node_page(struct page *page, unsigned long private,
 		return alloc_huge_page_node(page_hstate(compound_head(page)),
 					next_node_in(nid, nmask));
 
-	node_clear(nid, nmask);
+	if (nid != next_node_in(nid, nmask))
+		node_clear(nid, nmask);
+
 	if (PageHighMem(page)
 	    || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
 		gfp_mask |= __GFP_HIGHMEM;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Vlastimil Babka

Acked-by on v1 1/1

2016-09-05 14:18 UTC #1 lore raw

Quoted 4 lines · Li Zhong

Commit 394e31d2c introduced new_node_page() for memory hotplug.
In new_node_page(), the nid is cleared before calling __alloc_pages_nodemask(). But if it is the only node of the system,

So the use case is that we are partially offlining the only online node?

Quoted 2 lines

and the first round allocation fails,
it will not be able to get memory from an empty nodemask, and trigger oom.

Hmm triggering OOM due to empty nodemask sounds like a wrong thing to do. CCing some OOM experts for insight. Also OOM is skipped for __GFP_THISNODE allocations, so we might also consider the same for nodemask-constrained allocations?

Quoted 2 lines

The patch checks whether it is the last node on the system, and if it is, then don't clear the nid in the nodemask.

I'd rather see the allocation not OOM, and rely on the fallback in new_node_page() that doesn't have nodemask. But I suspect it might also make sense to treat empty nodemask as something unexpected and put some WARN_ON (instead of OOM) in the allocator.

Quoted 2 lines

Reported-by John Allen <[email protected]>
Signed-off-by Li Zhong <[email protected]>

Acked-by Vlastimil Babka <[email protected]>

Fixes 394e31d2ceb4 ("mem-hotplug: alloc new page from a nearest neighbor node when mem-offline")

Quoted 12 lines

---
 mm/memory_hotplug.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

mm/memory_hotplug.c +0 -0

index 41266dc..b58906b 100644

mm/memory_hotplug.c static struct page *new_node_page(struct page *page, unsigned long private, · line 1575

Quoted 10 lines

mm/memory_hotplug.c +3 -1

@@ -1567,7 +1567,9 @@ static struct page *new_node_page(struct page *page, unsigned long private,
 		return alloc_huge_page_node(page_hstate(compound_head(page)),
 					next_node_in(nid, nmask));
 
-	node_clear(nid, nmask);
+	if (nid != next_node_in(nid, nmask))
+		node_clear(nid, nmask);
+
 	if (PageHighMem(page)
 	    || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
 		gfp_mask |= __GFP_HIGHMEM;

Li Zhong

author

2016-09-06 08:15 UTC #2 lore raw

Quoted 10 lines · Vlastimil Babka

Quoted 4 lines · Li Zhong
Commit 394e31d2c introduced new_node_page() for memory hotplug.
In new_node_page(), the nid is cleared before calling __alloc_pages_nodemask(). But if it is the only node of the system,
So the use case is that we are partially offlining the only online node?

Yes.

Quoted 10 lines

Quoted 3 lines
and the first round allocation fails,
it will not be able to get memory from an empty nodemask, and trigger oom.
Hmm triggering OOM due to empty nodemask sounds like a wrong thing to do. CCing some OOM experts for insight. Also OOM is skipped for __GFP_THISNODE allocations, so we might also consider the same for nodemask-constrained allocations?
Quoted 2 lines
The patch checks whether it is the last node on the system, and if it is, then don't clear the nid in the nodemask.
I'd rather see the allocation not OOM, and rely on the fallback in new_node_page() that doesn't have nodemask. But I suspect it might also make sense to treat empty nodemask as something unexpected and put some WARN_ON (instead of OOM) in the allocator.

I think it would be much easier to understand these kind of empty nodemask allocation failure with this WARN_ON(), how about something like this?

===

mm/page_alloc.c +5 -0

index a2214c6..57edf18 100644
@@ -3629,6 +3629,11 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
                .migratetype = gfpflags_to_migratetype(gfp_mask),
        };
 
+       if (nodemask && nodes_empty(*nodemask)) {
+               WARN_ON(1);
+               return NULL;
+       }
+
        if (cpusets_enabled()) {
                alloc_mask |= __GFP_HARDWALL;
                alloc_flags |= ALLOC_CPUSET;

===

If that’s ok, maybe I can send a separate patch for this?

Thanks, Zhong

Quoted 19 lines

Quoted 3 lines
Reported-by John Allen <[email protected]>
Signed-off-by Li Zhong <[email protected]>
Acked-by Vlastimil Babka <[email protected]>
Fixes 394e31d2ceb4 ("mem-hotplug: alloc new page from a nearest neighbor node when mem-offline")
Quoted 12 lines
---
mm/memory_hotplug.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
mm/memory_hotplug.c +0 -0
index 41266dc..b58906b 100644
@@ -1567,7 +1567,9 @@ static struct page *new_node_page(struct page *page, unsigned long private,
 		return alloc_huge_page_node(page_hstate(compound_head(page)),
 					next_node_in(nid, nmask));
 

Quoted 11 lines

mm/memory_hotplug.c static struct page *new_node_page(struct page *page, unsigned long private, · line 1575
Quoted 10 lines
mm/memory_hotplug.c +3 -1
-	node_clear(nid, nmask);
+	if (nid != next_node_in(nid, nmask))
+		node_clear(nid, nmask);
+
 	if (PageHighMem(page)
 	    || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
 		gfp_mask |= __GFP_HIGHMEM;

Vlastimil Babka

2016-09-06 14:12 UTC #3 lore raw

Quoted 44 lines · Li Zhong

Quoted 10 lines · Vlastimil Babka
Quoted 4 lines · Li Zhong
Commit 394e31d2c introduced new_node_page() for memory hotplug.
In new_node_page(), the nid is cleared before calling __alloc_pages_nodemask(). But if it is the only node of the system,
So the use case is that we are partially offlining the only online node?
Yes.
Quoted 10 lines
Quoted 3 lines
and the first round allocation fails,
it will not be able to get memory from an empty nodemask, and trigger oom.
Hmm triggering OOM due to empty nodemask sounds like a wrong thing to do. CCing some OOM experts for insight. Also OOM is skipped for __GFP_THISNODE allocations, so we might also consider the same for nodemask-constrained allocations?
Quoted 2 lines
The patch checks whether it is the last node on the system, and if it is, then don't clear the nid in the nodemask.
I'd rather see the allocation not OOM, and rely on the fallback in new_node_page() that doesn't have nodemask. But I suspect it might also make sense to treat empty nodemask as something unexpected and put some WARN_ON (instead of OOM) in the allocator.
I think it would be much easier to understand these kind of empty nodemask allocation failure with this WARN_ON(), how about something like this?
===
mm/page_alloc.c +5 -0
index a2214c6..57edf18 100644
@@ -3629,6 +3629,11 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
                .migratetype = gfpflags_to_migratetype(gfp_mask),
        };
 
+       if (nodemask && nodes_empty(*nodemask)) {
+               WARN_ON(1);
+               return NULL;
+       }
+
        if (cpusets_enabled()) {
                alloc_mask |= __GFP_HARDWALL;
                alloc_flags |= ALLOC_CPUSET;
===

Quoted 1 line

If thata??s ok, maybe I can send a separate patch for this?

Something like that, but please not in the hotpath. I think the earliest suitable place is in __alloc_pages_slowpath() after the get_page_from_freelist() fails. And probably the best way would be to do something like pr_warn("nodemask is empty") and then clear __GFP_NOWARN from gfp_mask and goto nopage.

Thanks, Vlastimil

Quoted 33 lines

Thanks, Zhong
Quoted 30 lines
Quoted 3 lines
Reported-by John Allen <[email protected]>
Signed-off-by Li Zhong <[email protected]>
Acked-by Vlastimil Babka <[email protected]>
Fixes 394e31d2ceb4 ("mem-hotplug: alloc new page from a nearest neighbor node when mem-offline")
Quoted 22 lines
---
mm/memory_hotplug.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
mm/memory_hotplug.c +3 -1
index 41266dc..b58906b 100644
@@ -1567,7 +1567,9 @@ static struct page *new_node_page(struct page *page, unsigned long private,
 		return alloc_huge_page_node(page_hstate(compound_head(page)),
 					next_node_in(nid, nmask));
 
-	node_clear(nid, nmask);
+	if (nid != next_node_in(nid, nmask))
+		node_clear(nid, nmask);
+
 	if (PageHighMem(page)
 	    || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
 		gfp_mask |= __GFP_HIGHMEM;

Li Zhong

author

2016-09-07 07:36 UTC #4 lore raw

PATCH 1/1 mm, page_alloc: warn about empty nodemask

Warn about allocating with an empty nodemask, it would be easier to understand than oom messages. The check is added in the slow path.

Signed-off-by Li Zhong <[email protected]>

---
mm/page_alloc.c | 6 ++++++

 1 file changed, 6 insertions(+)

mm/page_alloc.c +6 -0

index a2214c6..d624ff3 100644
@@ -3448,6 +3448,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (page)
 		goto got_pg;
 
+	if (ac->nodemask && nodes_empty(*ac->nodemask)) {
+		pr_warn("nodemask is empty\n");
+		gfp_mask &= ~__GFP_NOWARN;
+		goto nopage;
+	}
+
 	/*
 	 * For costly allocations, try direct compaction first, as it's likely
 	 * that we have enough base pages and don't need to reclaim. Don't try

Andrew Morton

maintainer

2016-09-08 23:26 UTC #5 lore raw

Re: [PATCH] mm, page_alloc: warn about empty nodemask

Quoted 14 lines · On Wed, 07 Sep 2016 08:41:26 +0800 Li Zhong <[email protected]> wrote:

Warn about allocating with an empty nodemask, it would be easier to understand than oom messages. The check is added in the slow path.
Suggested-by Vlastimil Babka <[email protected]>
Signed-off-by Li Zhong <[email protected]>
---
mm/page_alloc.c | 6 ++++++
 1 file changed, 6 insertions(+)
mm/page_alloc.c +0 -0
index a2214c6..d624ff3 100644

mm/page_alloc.c __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order, · line 3456

Quoted 9 lines

mm/page_alloc.c +6 -0

@@ -3448,6 +3448,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (page)
 		goto got_pg;
 
+	if (ac->nodemask && nodes_empty(*ac->nodemask)) {
+		pr_warn("nodemask is empty\n");
+		gfp_mask &= ~__GFP_NOWARN;
+		goto nopage;
+	}
+

Wouldn't it be better to do

	if (WARN_ON(ac->nodemask && nodes_empty(*ac->nodemask)) {
		...

so we can identify the misbehaving call site?

Li Zhong

author

2016-09-09 04:06 UTC #6 lore raw

Re: [PATCH] mm, page_alloc: warn about empty nodemask

Quoted 31 lines · Andrew Morton

Quoted 14 lines · On Wed, 07 Sep 2016 08:41:26 +0800 Li Zhong <[email protected]> wrote:
Warn about allocating with an empty nodemask, it would be easier to understand than oom messages. The check is added in the slow path.
Suggested-by Vlastimil Babka <[email protected]>
Signed-off-by Li Zhong <[email protected]>
---
mm/page_alloc.c | 6 ++++++
1 file changed, 6 insertions(+)
mm/page_alloc.c +0 -0
index a2214c6..d624ff3 100644
mm/page_alloc.c __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order, · line 3456
Quoted 9 lines
mm/page_alloc.c +6 -0
@@ -3448,6 +3448,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (page)
 		goto got_pg;
 
+	if (ac->nodemask && nodes_empty(*ac->nodemask)) {
+		pr_warn("nodemask is empty\n");
+		gfp_mask &= ~__GFP_NOWARN;
+		goto nopage;
+	}
+
Wouldn't it be better to do

Quoted 4 lines

	if (WARN_ON(ac->nodemask && nodes_empty(*ac->nodemask)) {
		...
so we can identify the misbehaving call site?

I think with __GFP_NOWARN cleared, we could know the call site from warn_alloc_failed(). And the message “nodemask is empty” makes the error obvious without going to the source.

Thanks, Zhong

Vlastimil Babka

Acked-by

2016-09-20 08:27 UTC #7 lore raw

Re: [PATCH] mm, page_alloc: warn about empty nodemask

Quoted 18 lines · Li Zhong

Quoted 18 lines
Quoted 6 lines
mm/page_alloc.c +0 -0
index a2214c6..d624ff3 100644
mm/page_alloc.c __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order, · line 3456
Quoted 9 lines
mm/page_alloc.c +6 -0
@@ -3448,6 +3448,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (page)
 		goto got_pg;
 
+	if (ac->nodemask && nodes_empty(*ac->nodemask)) {
+		pr_warn("nodemask is empty\n");
+		gfp_mask &= ~__GFP_NOWARN;
+		goto nopage;
+	}
+
Wouldn't it be better to do

Quoted 7 lines

Quoted 4 lines
	if (WARN_ON(ac->nodemask && nodes_empty(*ac->nodemask)) {
		...
so we can identify the misbehaving call site?
I think with __GFP_NOWARN cleared, we could know the call site from warn_alloc_failed(). And the message a??nodemask is emptya?? makes the error obvious without going to the source.

Yes, that was my suggestion. It uses the generic warn_alloc_failed() this way. With a WARN_ON we would either have to "return NULL" (and get only the WARN_ON without the extra warn_alloc_failed() stuff) or "goto nopage" and thus get two backtraces. But this should be really rare occurence, so I don't have a particularly strong preference.

Anyway, since I suggested it in the first place:

Acked-by Vlastimil Babka <[email protected]>

Quoted 2 lines

Thanks, Zhong

Michal Hocko

2016-09-12 09:18 UTC #8 lore raw

In reply to Vlastimil Babka

Quoted 6 lines · Vlastimil Babka

Quoted 4 lines · Li Zhong
Commit 394e31d2c introduced new_node_page() for memory hotplug.
In new_node_page(), the nid is cleared before calling __alloc_pages_nodemask(). But if it is the only node of the system,

Quoted 7 lines

So the use case is that we are partially offlining the only online node?
Quoted 2 lines
and the first round allocation fails,
it will not be able to get memory from an empty nodemask, and trigger oom.
Hmm triggering OOM due to empty nodemask sounds like a wrong thing to do. CCing some OOM experts for insight.

Hmm, to be honest I think that using an empty nodemask is just a bug in the code. I do not see any reasonable scenario when this would make a sense. I agree that triggering an OOM killer for that is bad as well but do we actually want to allow for such a case at all? How can this happen?

Quoted 11 lines

Also OOM is skipped for __GFP_THISNODE
allocations, so we might also consider the same for nodemask-constrained allocations?
Quoted 2 lines
The patch checks whether it is the last node on the system, and if it is, then don't clear the nid in the nodemask.
I'd rather see the allocation not OOM, and rely on the fallback in new_node_page() that doesn't have nodemask. But I suspect it might also make sense to treat empty nodemask as something unexpected and put some WARN_ON (instead of OOM) in the allocator.

To be honest I am really not all that happy about 394e31d2ceb4 ("mem-hotplug: alloc new page from a nearest neighbor node when mem-offline") and find it a bit fishy. I would rather re-iterate that patch rather than build new hacks on top.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Vlastimil Babka

2016-09-20 08:31 UTC #9 lore raw

Quoted 14 lines · Michal Hocko

Quoted 11 lines · Vlastimil Babka
Also OOM is skipped for __GFP_THISNODE
allocations, so we might also consider the same for nodemask-constrained allocations?
Quoted 2 lines
The patch checks whether it is the last node on the system, and if it is, then don't clear the nid in the nodemask.
I'd rather see the allocation not OOM, and rely on the fallback in new_node_page() that doesn't have nodemask. But I suspect it might also make sense to treat empty nodemask as something unexpected and put some WARN_ON (instead of OOM) in the allocator.

Quoted 4 lines

To be honest I am really not all that happy about 394e31d2ceb4 ("mem-hotplug: alloc new page from a nearest neighbor node when mem-offline") and find it a bit fishy. I would rather re-iterate that patch rather than build new hacks on top.

OK, IIRC I suggested the main idea of clearing the current node from nodemask and relying on nodelist to get us the other nodes sorted by their distance. Which I thought was an easy way to get to the theoretically optimal result. How would you rewrite it then? (but note that the fix is already mainline).

David Rientjes

2016-09-20 21:53 UTC #10 lore raw

Quoted 22 lines · Vlastimil Babka

Quoted 16 lines · Michal Hocko
Quoted 5 lines · Vlastimil Babka
Also OOM is skipped for __GFP_THISNODE
allocations, so we might also consider the same for nodemask-constrained allocations?
Quoted 1 line
The patch checks whether it is the last node on the system, and if it
Quoted 8 lines
is, then
Quoted 1 line
don't clear the nid in the nodemask.
I'd rather see the allocation not OOM, and rely on the fallback in new_node_page() that doesn't have nodemask. But I suspect it might also make
sense to treat empty nodemask as something unexpected and put some WARN_ON (instead of OOM) in the allocator.
Quoted 4 lines
To be honest I am really not all that happy about 394e31d2ceb4 ("mem-hotplug: alloc new page from a nearest neighbor node when mem-offline") and find it a bit fishy. I would rather re-iterate that patch rather than build new hacks on top.

Quoted 5 lines

OK, IIRC I suggested the main idea of clearing the current node from nodemask and relying on nodelist to get us the other nodes sorted by their distance. Which I thought was an easy way to get to the theoretically optimal result. How would you rewrite it then? (but note that the fix is already mainline).

This is a mess. Commit 9bb627be47a5 ("mem-hotplug: don't clear the only node in new_node_page()") is wrong because it's clearing nid when the next node in node_online_map doesn't match. node_online_map is wrong because it includes memoryless nodes. (Nodes with closest NUMA distance also do not need to have adjacent node ids.)

This is all protected by mem_hotplug_begin() and the zonelists will be stable. The solution is to rewrite new_node_page() to work correctly. Use node_states[N_MEMORY] as mask, clear page_to_nid(page). If mask is not empty, do

__alloc_pages_nodemask(gfp_mask, 0, node_zonelist(page_to_nid(page), gfp_mask), &mask)

and fallback to alloc_page(gfp_mask), which should also be used if the mask is empty -- do not try to allocate memory from the empty set of nodes.

mm-page_alloc-warn-about-empty-nodemask.patch is a rather ridiculous warning to need. The largest user of a page allocator nodemask is mempolicies which makes sure it doesn't pass an empty set. If it's really required, it should at least be unlikely() since the vast majority of callers will have ac->nodemask == NULL.

Li Zhong

author

2016-09-21 02:13 UTC #11 lore raw

Quoted 33 lines · David Rientjes

Quoted 22 lines · Vlastimil Babka
Quoted 16 lines · Michal Hocko
Quoted 5 lines · Vlastimil Babka
Also OOM is skipped for __GFP_THISNODE
allocations, so we might also consider the same for nodemask-constrained allocations?
Quoted 1 line
The patch checks whether it is the last node on the system, and if it
Quoted 8 lines
is, then
Quoted 1 line
don't clear the nid in the nodemask.
I'd rather see the allocation not OOM, and rely on the fallback in new_node_page() that doesn't have nodemask. But I suspect it might also make
sense to treat empty nodemask as something unexpected and put some WARN_ON (instead of OOM) in the allocator.
Quoted 4 lines
To be honest I am really not all that happy about 394e31d2ceb4 ("mem-hotplug: alloc new page from a nearest neighbor node when mem-offline") and find it a bit fishy. I would rather re-iterate that patch rather than build new hacks on top.
Quoted 5 lines
OK, IIRC I suggested the main idea of clearing the current node from nodemask and relying on nodelist to get us the other nodes sorted by their distance. Which I thought was an easy way to get to the theoretically optimal result. How would you rewrite it then? (but note that the fix is already mainline).

Quoted 5 lines

This is a mess. Commit 9bb627be47a5 ("mem-hotplug: don't clear the only node in new_node_page()") is wrong because it's clearing nid when the next node in node_online_map doesn't match. node_online_map is wrong because it includes memoryless nodes. (Nodes with closest NUMA distance also do not need to have adjacent node ids.)

Thanks for pointing out that, so it is still possible that we are allocating from one or more memoryless nodes, which is the same as from an empty mask.

I will try to fix it as you suggested below, test and send it soon.

Quoted 13 lines

This is all protected by mem_hotplug_begin() and the zonelists will be stable. The solution is to rewrite new_node_page() to work correctly. Use node_states[N_MEMORY] as mask, clear page_to_nid(page). If mask is not empty, do
__alloc_pages_nodemask(gfp_mask, 0, node_zonelist(page_to_nid(page), gfp_mask), &mask)
and fallback to alloc_page(gfp_mask), which should also be used if the mask is empty -- do not try to allocate memory from the empty set of nodes.

Quoted 6 lines

mm-page_alloc-warn-about-empty-nodemask.patch is a rather ridiculous warning to need. The largest user of a page allocator nodemask is mempolicies which makes sure it doesn't pass an empty set. If it's really required, it should at least be unlikely() since the vast majority of callers will have ac->nodemask == NULL.

OK, I’ll send a new version adding unlikely().

Thanks, Zhong

Li Zhong

author

2016-09-21 08:38 UTC #12 lore raw

PATCH 1/1 mem-hotplug: Use nodes that contain memory as mask in new_node_page()

In reply to David Rientjes

Commit 9bb627be47a5 ("mem-hotplug: don't clear the only node in new_node_page()") prevents allocating from an empty nodemask, but as David points out, it is still wrong. As node_online_map may include memoryless nodes, only allocating from these nodes is meaningless.

This patch uses node_states[N_MEMORY] mask to prevent the above case.

Signed-off-by Li Zhong <[email protected]>

---
 mm/memory_hotplug.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

mm/memory_hotplug.c +5 -5

index b58906b..9d29ba0 100644
@@ -1555,8 +1555,8 @@ static struct page *new_node_page(struct page *page, unsigned long private,
 {
 	gfp_t gfp_mask = GFP_USER | __GFP_MOVABLE;
 	int nid = page_to_nid(page);
-	nodemask_t nmask = node_online_map;
-	struct page *new_page;
+	nodemask_t nmask = node_states[N_MEMORY];
+	struct page *new_page = NULL;
 
 	/*
 	 * TODO: allocate a destination hugepage from a nearest neighbor node,

@@ -1567,14 +1567,14 @@ static struct page *new_node_page(struct page *page, unsigned long private,
 		return alloc_huge_page_node(page_hstate(compound_head(page)),
 					next_node_in(nid, nmask));
 
-	if (nid != next_node_in(nid, nmask))
-		node_clear(nid, nmask);
+	node_clear(nid, nmask);
 
 	if (PageHighMem(page)
 	    || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
 		gfp_mask |= __GFP_HIGHMEM;
 
-	new_page = __alloc_pages_nodemask(gfp_mask, 0,
+	if (!nodes_empty(nmask))
+		new_page = __alloc_pages_nodemask(gfp_mask, 0,
 					node_zonelist(nid, gfp_mask), &nmask);
 	if (!new_page)
 		new_page = __alloc_pages(gfp_mask, 0,

Vlastimil Babka

Acked-by on v1 1/1

2016-09-21 09:34 UTC #13 lore raw

Re: [PATCH] mem-hotplug: Use nodes that contain memory as mask in new_node_page()

Quoted 4 lines · Li Zhong

Commit 9bb627be47a5 ("mem-hotplug: don't clear the only node in new_node_page()") prevents allocating from an empty nodemask, but as David points out, it is still wrong. As node_online_map may include memoryless nodes, only allocating from these nodes is meaningless.

Right, those pesky memoryless nodes...

Quoted 1 line

This patch uses node_states[N_MEMORY] mask to prevent the above case.

Quoted 1 line

Signed-off-by Li Zhong <[email protected]>

Acked-by Vlastimil Babka <[email protected]>

Quoted 29 lines

---
 mm/memory_hotplug.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

mm/memory_hotplug.c +3 -4

index b58906b..9d29ba0 100644
@@ -1555,8 +1555,8 @@ static struct page *new_node_page(struct page *page, unsigned long private,
 {
 	gfp_t gfp_mask = GFP_USER | __GFP_MOVABLE;
 	int nid = page_to_nid(page);
-	nodemask_t nmask = node_online_map;
-	struct page *new_page;
+	nodemask_t nmask = node_states[N_MEMORY];
+	struct page *new_page = NULL;
 
 	/*
 	 * TODO: allocate a destination hugepage from a nearest neighbor node,

@@ -1567,14 +1567,14 @@ static struct page *new_node_page(struct page *page, unsigned long private,
 		return alloc_huge_page_node(page_hstate(compound_head(page)),
 					next_node_in(nid, nmask));
 
-	if (nid != next_node_in(nid, nmask))
-		node_clear(nid, nmask);
+	node_clear(nid, nmask);

mm/memory_hotplug.c static struct page *new_node_page(struct page *page, unsigned long private, · line 1580

Quoted 10 lines

mm/memory_hotplug.c +2 -1

 	if (PageHighMem(page)
 	    || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
 		gfp_mask |= __GFP_HIGHMEM;
 
-	new_page = __alloc_pages_nodemask(gfp_mask, 0,
+	if (!nodes_empty(nmask))
+		new_page = __alloc_pages_nodemask(gfp_mask, 0,
 					node_zonelist(nid, gfp_mask), &nmask);
 	if (!new_page)
 		new_page = __alloc_pages(gfp_mask, 0,

Michal Hocko

2016-09-21 18:15 UTC #14 lore raw

Re: [PATCH] mem-hotplug: Use nodes that contain memory as mask in new_node_page()

In reply to Li Zhong

Quoted 7 lines · Li Zhong

Commit 9bb627be47a5 ("mem-hotplug: don't clear the only node in new_node_page()") prevents allocating from an empty nodemask, but as David points out, it is still wrong. As node_online_map may include memoryless nodes, only allocating from these nodes is meaningless.
This patch uses node_states[N_MEMORY] mask to prevent the above case.

OK, this is much better

I believe this should have

Fixes 9bb627be47a5 ("mem-hotplug: don't clear the only node in new_node_page()")

Fixes 394e31d2ceb4 ("mem-hotplug: alloc new page from a nearest neighbor node when mem-offline")

Fortunatelly everything is 4.8 so we do not need any backporting to stable. Anyway this just shows that the hotplug patches need much better review. E.g. 394e31d2ceb4 didn't have a single ack or r-b. Sigh

Quoted 39 lines

Signed-off-by Li Zhong <[email protected]>

---
 mm/memory_hotplug.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

mm/memory_hotplug.c +5 -5

index b58906b..9d29ba0 100644
@@ -1555,8 +1555,8 @@ static struct page *new_node_page(struct page *page, unsigned long private,
 {
 	gfp_t gfp_mask = GFP_USER | __GFP_MOVABLE;
 	int nid = page_to_nid(page);
-	nodemask_t nmask = node_online_map;
-	struct page *new_page;
+	nodemask_t nmask = node_states[N_MEMORY];
+	struct page *new_page = NULL;
 
 	/*
 	 * TODO: allocate a destination hugepage from a nearest neighbor node,

@@ -1567,14 +1567,14 @@ static struct page *new_node_page(struct page *page, unsigned long private,
 		return alloc_huge_page_node(page_hstate(compound_head(page)),
 					next_node_in(nid, nmask));
 
-	if (nid != next_node_in(nid, nmask))
-		node_clear(nid, nmask);
+	node_clear(nid, nmask);
 
 	if (PageHighMem(page)
 	    || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
 		gfp_mask |= __GFP_HIGHMEM;
 
-	new_page = __alloc_pages_nodemask(gfp_mask, 0,
+	if (!nodes_empty(nmask))
+		new_page = __alloc_pages_nodemask(gfp_mask, 0,
 					node_zonelist(nid, gfp_mask), &nmask);
 	if (!new_page)
 		new_page = __alloc_pages(gfp_mask, 0,

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michal Hocko

2016-09-21 18:08 UTC #15 lore raw

In reply to David Rientjes

Quoted 48 lines · David Rientjes

Quoted 22 lines · Vlastimil Babka
Quoted 16 lines · Michal Hocko
Quoted 5 lines · Vlastimil Babka
Also OOM is skipped for __GFP_THISNODE
allocations, so we might also consider the same for nodemask-constrained allocations?
Quoted 1 line
The patch checks whether it is the last node on the system, and if it
Quoted 8 lines
is, then
Quoted 1 line
don't clear the nid in the nodemask.
I'd rather see the allocation not OOM, and rely on the fallback in new_node_page() that doesn't have nodemask. But I suspect it might also make
sense to treat empty nodemask as something unexpected and put some WARN_ON (instead of OOM) in the allocator.
Quoted 4 lines
To be honest I am really not all that happy about 394e31d2ceb4 ("mem-hotplug: alloc new page from a nearest neighbor node when mem-offline") and find it a bit fishy. I would rather re-iterate that patch rather than build new hacks on top.
Quoted 5 lines
OK, IIRC I suggested the main idea of clearing the current node from nodemask and relying on nodelist to get us the other nodes sorted by their distance. Which I thought was an easy way to get to the theoretically optimal result. How would you rewrite it then? (but note that the fix is already mainline).
This is a mess. Commit 9bb627be47a5 ("mem-hotplug: don't clear the only node in new_node_page()") is wrong because it's clearing nid when the next node in node_online_map doesn't match. node_online_map is wrong because it includes memoryless nodes. (Nodes with closest NUMA distance also do not need to have adjacent node ids.)
This is all protected by mem_hotplug_begin() and the zonelists will be stable. The solution is to rewrite new_node_page() to work correctly. Use node_states[N_MEMORY] as mask, clear page_to_nid(page). If mask is not empty, do
__alloc_pages_nodemask(gfp_mask, 0, node_zonelist(page_to_nid(page), gfp_mask), &mask)
and fallback to alloc_page(gfp_mask), which should also be used if the mask is empty -- do not try to allocate memory from the empty set of nodes.

Quoted 5 lines

mm-page_alloc-warn-about-empty-nodemask.patch is a rather ridiculous warning to need. The largest user of a page allocator nodemask is mempolicies which makes sure it doesn't pass an empty set. If it's really required, it should at least be unlikely() since the vast majority of callers will have ac->nodemask == NULL.

Sorry to respond late, I was too busy with other thigns but I completely agree with the above. This is the way we should go forward!

Thanks!

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Xishi Qiu

2016-09-06 13:20 UTC #16 lore raw

In reply to Li Zhong

Quoted 6 lines · Li Zhong

Commit 394e31d2c introduced new_node_page() for memory hotplug.
In new_node_page(), the nid is cleared before calling __alloc_pages_nodemask(). But if it is the only node of the system, and the first round allocation fails, it will not be able to get memory from an empty nodemask, and trigger oom.

Hi,

Yes, I missed this case, thanks for your fix.

Thanks,
Xishi Qiu

Quoted 5 lines

The patch checks whether it is the last node on the system, and if it is, then don't clear the nid in the nodemask.
Reported-by John Allen <[email protected]>
Signed-off-by Li Zhong <[email protected]>