hugetlb: fix hugepage allocation with memoryless nodes

PATCH In review Nishanth Aravamudan 2 participants 5 messages Oct 9, 2007 – Oct 11, 2007

Subsystems (2)

Memory management HugeTLB

lore

Share & tools

lore

Nishanth Aravamudan

author

2007-10-09 01:27 UTC #0 lore raw

PATCH 1/1 hugetlb: fix hugepage allocation with memoryless nodes

Anton found a problem with the hugetlb pool allocation when some nodes have no memory (http://marc.info/?l=linux-mm&m=118133042025995&w=2). Lee worked on versions that tried to fix it, but none were accepted. Christoph has created a set of patches which allow for GFP_THISNODE allocations to fail if the node has no memory.

Currently, alloc_fresh_huge_page() returns NULL when it is not able to allocate a huge page on the current node, as specified by its custom interleave variable. The callers of this function, though, assume that a failure in alloc_fresh_huge_page() indicates no hugepages can be allocated on the system period. This might not be the case, for instance, if we have an uneven NUMA system, and we happen to try to allocate a hugepage on a node with less memory and fail, while there is still plenty of free memory on the other nodes.

To correct this, make alloc_fresh_huge_page() search through all online nodes before deciding no hugepages can be allocated. Add a helper function for actually allocating the hugepage.

Note: we expect particular semantics for __GFP_THISNODE, which are now enforced even for memoryless nodes. That is, there is should be no fallback to other nodes. Therefore, we rely on the nid passed into alloc_pages_node() to be the nid the page comes from. If this is incorrect, accounting will break.

Tested on x86 !NUMA, x86 NUMA, x86_64 NUMA and ppc64 NUMA (with 2 memoryless nodes).

Before on the ppc64 box:
Trying to clear the hugetlb pool
Done. 0 free
Trying to resize the pool to 100

Node 0 HugePages_Free:     25
Node 1 HugePages_Free:     75
Node 2 HugePages_Free:      0
Node 3 HugePages_Free:      0
Done. Initially     100 free
Trying to resize the pool to 200
Node 0 HugePages_Free:     50
Node 1 HugePages_Free:    150
Node 2 HugePages_Free:      0
Node 3 HugePages_Free:      0

Done. 200 free

After:
Trying to clear the hugetlb pool
Done. 0 free
Trying to resize the pool to 100

Node 0 HugePages_Free:     50
Node 1 HugePages_Free:     50
Node 2 HugePages_Free:      0
Node 3 HugePages_Free:      0
Done. Initially     100 free
Trying to resize the pool to 200
Node 0 HugePages_Free:    100
Node 1 HugePages_Free:    100
Node 2 HugePages_Free:      0
Node 3 HugePages_Free:      0

Done. 200 free

Signed-off-by Nishanth Aravamudan <[email protected]>

Acked-by Christoph Lameter <[email protected]>

Cc Anton Blanchard <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Lee Schermerhorn <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Adam Litke <[email protected]>

Cc William Lee Irwin III <[email protected]>

---
 mm/hugetlb.c |   63 ++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 20 deletions(-)

Andrew, this patch alone suffices to fix the hugetlb pool allocation with memoryless nodes and is rebased on 2.6.23-rc8-mm2 + Adam's 5 patches. What I had been sending as patch 2/2 (which pretty much just did sed -i s/node_online_map/node_states[N_HIGH_MEMORY]/ mm/hugetlb.c) will need to be extended/tested for the new loops in hugetlb.c, but that is an efficiency concern, not a correctness one. I'd like to see if we can get this patch in 2.6.24, so if you could pick it up for the next -mm, I'd greatly appreciate it.

Also, I wonder (and have suggested this before): would it make sense to put a VM_BUG_ON (VM_BUG_ON_ONCE, if it exists) for GFP_THISNODE allocations coming from a node other than the one specified in the core VM under DEBUG_VM or something? That would have caught the case that Mel's one-zonelist stack created, I think.

mm/hugetlb.c +43 ~~-20~~

index 3d1b111..098e608 100644
@@ -32,6 +32,7 @@ static unsigned int surplus_huge_pages_node[MAX_NUMNODES];
 static gfp_t htlb_alloc_mask = GFP_HIGHUSER;
 unsigned long hugepages_treat_as_movable;
 int hugetlb_dynamic_pool;
+static int last_allocated_nid;
 
 /*
  * Protects updates to hugepage_freelists, nr_huge_pages, and free_huge_pages

@@ -165,36 +166,56 @@ static int adjust_pool_surplus(int delta)
 	return ret;
 }
 
-static int alloc_fresh_huge_page(void)
+static struct page *alloc_fresh_huge_page_node(int nid)
 {
-	static int prev_nid;
 	struct page *page;
-	int nid;
-
-	/*
-	 * Copy static prev_nid to local nid, work on that, then copy it
-	 * back to prev_nid afterwards: otherwise there's a window in which
-	 * a racer might pass invalid nid MAX_NUMNODES to alloc_pages_node.
-	 * But we don't need to use a spin_lock here: it really doesn't
-	 * matter if occasionally a racer chooses the same nid as we do.
-	 */
-	nid = next_node(prev_nid, node_online_map);
-	if (nid == MAX_NUMNODES)
-		nid = first_node(node_online_map);
-	prev_nid = nid;
 
-	page = alloc_pages_node(nid, htlb_alloc_mask|__GFP_COMP|__GFP_NOWARN,
-					HUGETLB_PAGE_ORDER);
+	page = alloc_pages_node(nid,
+		htlb_alloc_mask|__GFP_COMP|__GFP_THISNODE|__GFP_NOWARN,
+		HUGETLB_PAGE_ORDER);
 	if (page) {
 		set_compound_page_dtor(page, free_huge_page);
 		spin_lock(&hugetlb_lock);
 		nr_huge_pages++;
-		nr_huge_pages_node[page_to_nid(page)]++;
+		nr_huge_pages_node[nid]++;
 		spin_unlock(&hugetlb_lock);
 		put_page(page); /* free it into the hugepage allocator */
-		return 1;
 	}
-	return 0;
+
+	return page;
+}
+
+static int alloc_fresh_huge_page(void)
+{
+	struct page *page;
+	int start_nid;
+	int next_nid;
+	int ret = 0;
+
+	start_nid = last_allocated_nid;
+
+	do {
+		page = alloc_fresh_huge_page_node(last_allocated_nid);
+		if (page)
+			ret = 1;
+		/*
+		 * Use a helper variable to find the next node and then
+		 * copy it back to last_allocated_nid afterwards:
+		 * otherwise there's a window in which a racer might
+		 * pass invalid nid MAX_NUMNODES to alloc_pages_node.
+		 * But we don't need to use a spin_lock here: it really
+		 * doesn't matter if occasionally a racer chooses the
+		 * same nid as we do.  Move nid forward in the mask even
+		 * if we just successfully allocated a hugepage so that
+		 * the next caller gets hugepages on the next node.
+		 */
+		next_nid = next_node(last_allocated_nid, node_online_map);
+		if (next_nid == MAX_NUMNODES)
+			next_nid = first_node(node_online_map);
+		last_allocated_nid = next_nid;
+	} while (!page && last_allocated_nid != start_nid);
+
+	return ret;
 }
 
 static struct page *alloc_buddy_huge_page(struct vm_area_struct *vma,

@@ -365,6 +386,8 @@ static int __init hugetlb_init(void)
 	for (i = 0; i < MAX_NUMNODES; ++i)
 		INIT_LIST_HEAD(&hugepage_freelists[i]);
 
+	last_allocated_nid = first_node(node_online_map);
+
 	for (i = 0; i < max_huge_pages; ++i) {
 		if (!alloc_fresh_huge_page())
 			break;

Signature · 8 lines

Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center

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

Nishanth Aravamudan

author

2007-10-11 04:11 UTC #1 lore raw

Quoted 1 line · Nishanth Aravamudan

hugetlb: fix hugepage allocation with memoryless nodes

<snip>

Quoted 24 lines

---
 mm/hugetlb.c |   63 ++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 20 deletions(-)
Andrew, this patch alone suffices to fix the hugetlb pool allocation with memoryless nodes and is rebased on 2.6.23-rc8-mm2 + Adam's 5 patches. What I had been sending as patch 2/2 (which pretty much just did sed -i s/node_online_map/node_states[N_HIGH_MEMORY]/ mm/hugetlb.c) will need to be extended/tested for the new loops in hugetlb.c, but that is an efficiency concern, not a correctness one. I'd like to see if we can get this patch in 2.6.24, so if you could pick it up for the next -mm, I'd greatly appreciate it.
Also, I wonder (and have suggested this before): would it make sense to put a VM_BUG_ON (VM_BUG_ON_ONCE, if it exists) for GFP_THISNODE allocations coming from a node other than the one specified in the core VM under DEBUG_VM or something? That would have caught the case that Mel's one-zonelist stack created, I think.
mm/hugetlb.c +0 -0
index 3d1b111..098e608 100644

mm/hugetlb.c static unsigned int surplus_huge_pages_node[MAX_NUMNODES]; · line 35

Quoted 4 lines

mm/hugetlb.c +1 -0

@@ -32,6 +32,7 @@ static unsigned int surplus_huge_pages_node[MAX_NUMNODES];
 static gfp_t htlb_alloc_mask = GFP_HIGHUSER;
 unsigned long hugepages_treat_as_movable;
 int hugetlb_dynamic_pool;
+static int last_allocated_nid;

While reworking patch 2/2 to incorporate the current state of hugetlb.c after Adam's stack is applied, I realized that this is not a very good name. It actually is the *current* nid to try to allocate hugepages on.

Christoph, since you proposed the name, do you think

hugetlb_current_nid

is ok, too? If so I'll change the name throughout the patch (no functional change).

Thanks,
Nish

Signature · 8 lines

Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center

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

Christoph Lameter

2007-10-11 04:14 UTC #2 lore raw

Quoted 11 lines · Nishanth Aravamudan

mm/hugetlb.c static unsigned int surplus_huge_pages_node[MAX_NUMNODES]; · line 35
Quoted 6 lines
+++ b/mm/hugetlb.c
mm/hugetlb.c +1 -0
@@ -32,6 +32,7 @@ static unsigned int surplus_huge_pages_node[MAX_NUMNODES];
 static gfp_t htlb_alloc_mask = GFP_HIGHUSER;
 unsigned long hugepages_treat_as_movable;
 int hugetlb_dynamic_pool;
+static int last_allocated_nid;
While reworking patch 2/2 to incorporate the current state of hugetlb.c after Adam's stack is applied, I realized that this is not a very good name. It actually is the *current* nid to try to allocate hugepages on.

Quoted 6 lines

Christoph, since you proposed the name, do you think
hugetlb_current_nid
is ok, too? If so I'll change the name throughout the patch (no functional change).

Sure. However, current is bit ambiguous. Is it the node we used last or the one to use next? Call it next_hugetlb_nid? Either way is fine with me though.

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

Nishanth Aravamudan

author

2007-10-11 04:29 UTC #3 lore raw

Quoted 15 lines · Christoph Lameter

Quoted 13 lines · Nishanth Aravamudan
mm/hugetlb.c static unsigned int surplus_huge_pages_node[MAX_NUMNODES]; · line 35
Quoted 6 lines
+++ b/mm/hugetlb.c
mm/hugetlb.c +1 -0
@@ -32,6 +32,7 @@ static unsigned int surplus_huge_pages_node[MAX_NUMNODES];
 static gfp_t htlb_alloc_mask = GFP_HIGHUSER;
 unsigned long hugepages_treat_as_movable;
 int hugetlb_dynamic_pool;
+static int last_allocated_nid;
While reworking patch 2/2 to incorporate the current state of hugetlb.c after Adam's stack is applied, I realized that this is not a very good name. It actually is the *current* nid to try to allocate hugepages on.
Christoph, since you proposed the name, do you think

Quoted 8 lines

Quoted 4 lines
hugetlb_current_nid
is ok, too? If so I'll change the name throughout the patch (no functional change).
Sure. However, current is bit ambiguous. Is it the node we used last or the one to use next? Call it next_hugetlb_nid? Either way is fine with me though.

Good point. Given the way the code is laid out now, it always represents the nid to allocate on next (so is the first one to try).

next_hugetlb_nid or hugetlb_next_nid it is.

Thanks,
Nish

Signature · 8 lines

Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center

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

Nishanth Aravamudan

author

2007-10-11 05:19 UTC #4 lore raw

PATCH 1/1 hugetlb: fix hugepage allocation with memoryless nodes

Quoted 21 lines · Nishanth Aravamudan

Quoted 20 lines · Christoph Lameter
Quoted 17 lines · Nishanth Aravamudan
Review static unsigned int surplus_huge_pages_node[MAX_NUMNODES]; · line 35
Quoted 6 lines
+++ b/mm/hugetlb.c
(quoted diff) +1 -0
@@ -32,6 +32,7 @@ static unsigned int surplus_huge_pages_node[MAX_NUMNODES];
 static gfp_t htlb_alloc_mask = GFP_HIGHUSER;
 unsigned long hugepages_treat_as_movable;
 int hugetlb_dynamic_pool;
+static int last_allocated_nid;
While reworking patch 2/2 to incorporate the current state of hugetlb.c after Adam's stack is applied, I realized that this is not a very good name. It actually is the *current* nid to try to allocate hugepages on.
Christoph, since you proposed the name, do you think
hugetlb_current_nid
is ok, too? If so I'll change the name throughout the patch (no functional change).

Quoted 8 lines

Quoted 3 lines
Sure. However, current is bit ambiguous. Is it the node we used last or the one to use next? Call it next_hugetlb_nid? Either way is fine with me though.
Good point. Given the way the code is laid out now, it always represents the nid to allocate on next (so is the first one to try).
next_hugetlb_nid or hugetlb_next_nid it is.

hugetlb: fix hugepage allocation with memoryless nodes

To correct this, make alloc_fresh_huge_page() search through all online nodes before deciding no hugepages can be allocated. Add a helper function for actually allocating the hugepage. Use a new global nid iterator to control which nid to allocate on.

Tested on x86 !NUMA, x86 NUMA, x86_64 NUMA and ppc64 NUMA (with 2 memoryless nodes).

Before on the ppc64 box:
Trying to clear the hugetlb pool
Done. 0 free
Trying to resize the pool to 100

Node 0 HugePages_Free:     25
Node 1 HugePages_Free:     75
Node 2 HugePages_Free:      0
Node 3 HugePages_Free:      0
Done. Initially     100 free
Trying to resize the pool to 200
Node 0 HugePages_Free:     50
Node 1 HugePages_Free:    150
Node 2 HugePages_Free:      0
Node 3 HugePages_Free:      0

Done. 200 free

After:
Trying to clear the hugetlb pool
Done. 0 free
Trying to resize the pool to 100

Node 0 HugePages_Free:     50
Node 1 HugePages_Free:     50
Node 2 HugePages_Free:      0
Node 3 HugePages_Free:      0
Done. Initially     100 free
Trying to resize the pool to 200
Node 0 HugePages_Free:    100
Node 1 HugePages_Free:    100
Node 2 HugePages_Free:      0
Node 3 HugePages_Free:      0

Done. 200 free

Signed-off-by Nishanth Aravamudan <[email protected]>

Acked-by Christoph Lameter <[email protected]>

mm/hugetlb.c +43 ~~-20~~

index 68e8391..f246aee 100644
@@ -32,6 +32,7 @@ static unsigned int surplus_huge_pages_node[MAX_NUMNODES];
 static gfp_t htlb_alloc_mask = GFP_HIGHUSER;
 unsigned long hugepages_treat_as_movable;
 int hugetlb_dynamic_pool;
+static int hugetlb_next_nid;
 
 /*
  * Protects updates to hugepage_freelists, nr_huge_pages, and free_huge_pages

@@ -166,36 +167,56 @@ static int adjust_pool_surplus(int delta)
 	return ret;
 }
 
-static int alloc_fresh_huge_page(void)
+static struct page *alloc_fresh_huge_page_node(int nid)
 {
-	static int prev_nid;
 	struct page *page;
-	int nid;
-
-	/*
-	 * Copy static prev_nid to local nid, work on that, then copy it
-	 * back to prev_nid afterwards: otherwise there's a window in which
-	 * a racer might pass invalid nid MAX_NUMNODES to alloc_pages_node.
-	 * But we don't need to use a spin_lock here: it really doesn't
-	 * matter if occasionally a racer chooses the same nid as we do.
-	 */
-	nid = next_node(prev_nid, node_online_map);
-	if (nid == MAX_NUMNODES)
-		nid = first_node(node_online_map);
-	prev_nid = nid;
 
-	page = alloc_pages_node(nid, htlb_alloc_mask|__GFP_COMP|__GFP_NOWARN,
-					HUGETLB_PAGE_ORDER);
+	page = alloc_pages_node(nid,
+		htlb_alloc_mask|__GFP_COMP|__GFP_THISNODE|__GFP_NOWARN,
+		HUGETLB_PAGE_ORDER);
 	if (page) {
 		set_compound_page_dtor(page, free_huge_page);
 		spin_lock(&hugetlb_lock);
 		nr_huge_pages++;
-		nr_huge_pages_node[page_to_nid(page)]++;
+		nr_huge_pages_node[nid]++;
 		spin_unlock(&hugetlb_lock);
 		put_page(page); /* free it into the hugepage allocator */
-		return 1;
 	}
-	return 0;
+
+	return page;
+}
+
+static int alloc_fresh_huge_page(void)
+{
+	struct page *page;
+	int start_nid;
+	int next_nid;
+	int ret = 0;
+
+	start_nid = hugetlb_next_nid;
+
+	do {
+		page = alloc_fresh_huge_page_node(hugetlb_next_nid);
+		if (page)
+			ret = 1;
+		/*
+		 * Use a helper variable to find the next node and then
+		 * copy it back to hugetlb_next_nid afterwards:
+		 * otherwise there's a window in which a racer might
+		 * pass invalid nid MAX_NUMNODES to alloc_pages_node.
+		 * But we don't need to use a spin_lock here: it really
+		 * doesn't matter if occasionally a racer chooses the
+		 * same nid as we do.  Move nid forward in the mask even
+		 * if we just successfully allocated a hugepage so that
+		 * the next caller gets hugepages on the next node.
+		 */
+		next_nid = next_node(hugetlb_next_nid, node_online_map);
+		if (next_nid == MAX_NUMNODES)
+			next_nid = first_node(node_online_map);
+		hugetlb_next_nid = next_nid;
+	} while (!page && hugetlb_next_nid != start_nid);
+
+	return ret;
 }
 
 static struct page *alloc_buddy_huge_page(struct vm_area_struct *vma,

@@ -366,6 +387,8 @@ static int __init hugetlb_init(void)
 	for (i = 0; i < MAX_NUMNODES; ++i)
 		INIT_LIST_HEAD(&hugepage_freelists[i]);
 
+	hugetlb_next_nid = first_node(node_online_map);
+
 	for (i = 0; i < max_huge_pages; ++i) {
 		if (!alloc_fresh_huge_page())
 			break;

Signature · 8 lines

Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center

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