hugetlb: search harder for memory in alloc_fresh_huge_page()

hugetlb: search harder for memory in alloc_fresh_huge_page()

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.

While there are interleave interfaces that could be exported from the mempolicy layer, that seems like an inappropriate design decision. Work is needed on a subsystem-level interleaving interface, but I'm still not quite sure how that should look. Hence the custom interleaving here.

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

---
I split up patch 1/5 into two bits, as they are really two logical changes. Does this look better, Christoph?

mm/hugetlb.c +1 -16

index d7ca59d..17a377e 100644
@@ -101,36 +101,59 @@ static void free_huge_page(struct page *page)
 	spin_unlock(&hugetlb_lock);
 }
 
-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,

mm/hugetlb.c +31 -2

 		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)
+{
+	static int nid = -1;
+	struct page *page;
+	int start_nid;
+	int next_nid;
+	int ret = 0;
+
+	if (nid < 0)
+		nid = first_node(node_online_map);
+	start_nid = nid;
+
+	do {
+		page = alloc_fresh_huge_page_node(nid);
+		if (page)
+			ret = 1;
+		/*
+		 * Use a helper variable to find the next node and then
+		 * copy it back to nid 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.
+		 */

Quoted 22 lines · Nishanth Aravamudan

hugetlb: search harder for memory in alloc_fresh_huge_page()

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.

While there are interleave interfaces that could be exported from the mempolicy layer, that seems like an inappropriate design decision. Work is needed on a subsystem-level interleaving interface, but I'm still not quite sure how that should look. Hence the custom interleaving here.

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

Quoted 8 lines

(quoted diff) +3 -2

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

Review

Quoted 3 lines

(quoted diff) +1 -1

 		nr_huge_pages++;
-		nr_huge_pages_node[page_to_nid(page)]++;
+		nr_huge_pages_node[nid]++;

Not that I don't trust __GFP_THISNODE, but may I suggest a "VM_BUG_ON(page_to_nid(page) != nid)" -- up above the spin_lock(), of course. Better yet, add the assertion and drop this one line change? This isn't a hot path, I think.

Quoted 4 lines

Not that I don't trust __GFP_THISNODE, but may I suggest a "VM_BUG_ON(page_to_nid(page) != nid)" -- up above the spin_lock(), of course. Better yet, add the assertion and drop this one line change?

Quoted 4 lines · Nishanth Aravamudan

Quoted 4 lines

Not that I don't trust __GFP_THISNODE, but may I suggest a "VM_BUG_ON(page_to_nid(page) != nid)" -- up above the spin_lock(), of course. Better yet, add the assertion and drop this one line change?

Dont do this change.

Which change? Using nid without a VM_BUG_ON (as in the original patch) or adding a VM_BUG_ON and using page_to_nid()?

Quoted 2 lines · Nishanth Aravamudan

Which change? Using nid without a VM_BUG_ON (as in the original patch) or adding a VM_BUG_ON and using page_to_nid()?

Adding VM_BUG_ON. If page_alloc does not work then something basic is broken.

Quoted 5 lines · Christoph Lameter

Quoted 2 lines · Nishanth Aravamudan

Which change? Using nid without a VM_BUG_ON (as in the original patch) or adding a VM_BUG_ON and using page_to_nid()?

Quoted 2 lines

Adding VM_BUG_ON. If page_alloc does not work then something basic is broken.

I agree. So perhaps there needs to be a VM_BUG_ON_ONCE() or something somewhere in the core code for the case of a __GFP_THISNODE allocation going off node?

Quoted 4 lines · Nishanth Aravamudan

Quoted 4 lines

Not that I don't trust __GFP_THISNODE, but may I suggest a "VM_BUG_ON(page_to_nid(page) != nid)" -- up above the spin_lock(), of course. Better yet, add the assertion and drop this one line change?

Dont do this change.

Quoted 23 lines · Lee Schermerhorn

Quoted 22 lines · Nishanth Aravamudan

hugetlb: search harder for memory in alloc_fresh_huge_page()

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.

While there are interleave interfaces that could be exported from the mempolicy layer, that seems like an inappropriate design decision. Work is needed on a subsystem-level interleaving interface, but I'm still not quite sure how that should look. Hence the custom interleaving here.

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

<snip>

Quoted 8 lines

Quoted 8 lines

(quoted diff) +3 -2

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

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +1 -1

 		nr_huge_pages++;
-		nr_huge_pages_node[page_to_nid(page)]++;
+		nr_huge_pages_node[nid]++;

Not that I don't trust __GFP_THISNODE, but may I suggest a "VM_BUG_ON(page_to_nid(page) != nid)" -- up above the spin_lock(), of course. Better yet, add the assertion and drop this one line change? This isn't a hot path, I think.

Hrm, I think if it's really a concern then the VM_BUG_ON should be in alloc_pages_node() itself? Or somewhere lower level, I mean, it's a bug everywhere, not just in hugetlb.c. And, more importantly, if __GFP_THISNODE doesn't work, it pretty much defeats the purpose of my sysfs attribute patch. Echo'ing a value for node 0 and getting hugepages on node 1 would be bad :)

But here's the patch respun, as requested:

hugetlb: search harder for memory in alloc_fresh_huge_page()

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. Also, since we expect particular semantics for __GFP_THISNODE, which are newly enforced, add a VM_BUG_ON when allocations occur off the requested node.

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

mm/hugetlb.c +1 -17

index d7ca59d..e7b103d 100644
@@ -101,36 +101,60 @@ static void free_huge_page(struct page *page)
 	spin_unlock(&hugetlb_lock);
 }
 
-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);