hugetlb: Fix dynamic pool resize failure case

PATCH In review Adam Litke 2 participants 3 messages Oct 9, 2007 – Oct 10, 2007

Subsystems (2)

Memory management HugeTLB

lore

Share & tools

lore

Adam Litke

author

2007-10-09 15:58 UTC #0 lore raw

PATCH 1/1 hugetlb: Fix dynamic pool resize failure case

When gather_surplus_pages() fails to allocate enough huge pages to satisfy the requested reservation, it frees what it did allocate back to the buddy allocator. put_page() should be called instead of update_and_free_page() to ensure that pool counters are updated as appropriate and the page's refcount is decremented.

Andrew: This should apply cleanly to my patches in the -mm tree.

Signed-off-by Adam Litke <[email protected]>

---

 mm/hugetlb.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

mm/hugetlb.c +5 -2

index 9b3dfac..f349c16 100644
@@ -281,8 +281,11 @@ free:
 		list_del(&page->lru);
 		if ((--needed) >= 0)
 			enqueue_huge_page(page);
-		else
-			update_and_free_page(page);
+		else {
+			spin_unlock(&hugetlb_lock);
+			put_page(page);
+			spin_lock(&hugetlb_lock);
+		}
 	}
 
 	return ret;

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

Dave Hansen

2007-10-09 21:20 UTC #1 lore raw

Quoted 5 lines · Adam Litke

index 9b3dfac..f349c16 100644
mm/hugetlb.c +0 -0
@@ -281,8 +281,11 @@ free:
 		list_del(&page->lru);

mm/hugetlb.c free: · line 289

Quoted 10 lines

mm/hugetlb.c +5 -2

 		if ((--needed) >= 0)
 			enqueue_huge_page(page);
-		else
-			update_and_free_page(page);
+		else {
+			spin_unlock(&hugetlb_lock);
+			put_page(page);
+			spin_lock(&hugetlb_lock);
+		}
 	}

update_and_free_page() does several things:
1. it decrements nr_huge_pages(_node[])
2. it resets the member page flags to some known values
3. clears the compound page destructor
4. clears the page refcount (to 1)
5. actually frees the page back to the allocator

put_page() does several things, too:
1. put_page() hits PageCompound(), then calls put_compound_page()
2. put_compound_page() calls the compound page destructor which is set to free_huge_page() (this was set in alloc_buddy_huge_page())
3. free_huge_page() checks page_count(), takes the hugetlb_lock, and calls enqueue_huge_page()
4. enqueue_huge_page() puts the page back in hugepage_freelists[nid], then _increments_ nr_huge_pages(_node[])

This seems weird to me that you're replacing a function with something that eventually does the opposite. update_and_free_page() also did nothing with the hugepage_freelists[], which enqueue_huge_page() does. Something doesn't quite add up here. Did you realize that the destuctor was going to get called? Or, did I misread it, and the destructor is _not_ called?

I also think it's a crime that alloc_buddy_huge_page() doesn't share code with alloc_fresh_huge_page().

Adam Litke

author

2007-10-10 13:58 UTC #2 lore raw

Quoted 24 lines · Dave Hansen

Quoted 5 lines · Adam Litke
index 9b3dfac..f349c16 100644
mm/hugetlb.c +0 -0
@@ -281,8 +281,11 @@ free:
 		list_del(&page->lru);
mm/hugetlb.c free: · line 289
Quoted 10 lines
mm/hugetlb.c +5 -2
 		if ((--needed) >= 0)
 			enqueue_huge_page(page);
-		else
-			update_and_free_page(page);
+		else {
+			spin_unlock(&hugetlb_lock);
+			put_page(page);
+			spin_lock(&hugetlb_lock);
+		}
 	}
update_and_free_page() does several things:
1. it decrements nr_huge_pages(_node[])
2. it resets the member page flags to some known values
3. clears the compound page destructor
4. clears the page refcount (to 1)
5. actually frees the page back to the allocator

Quoted 6 lines

put_page() does several things, too:
1. put_page() hits PageCompound(), then calls put_compound_page()
2. put_compound_page() calls the compound page destructor which is set to free_huge_page() (this was set in alloc_buddy_huge_page())
3. free_huge_page() checks page_count(), takes the hugetlb_lock, and calls enqueue_huge_page()

Here's where I know your looking at different kernel source than this was meant to patch :)

Quoted 9 lines

4. enqueue_huge_page() puts the page back in hugepage_freelists[nid], then _increments_ nr_huge_pages(_node[])
This seems weird to me that you're replacing a function with something that eventually does the opposite. update_and_free_page() also did nothing with the hugepage_freelists[], which enqueue_huge_page() does. Something doesn't quite add up here. Did you realize that the destuctor was going to get called? Or, did I misread it, and the destructor is _not_ called?

With my patches applied, free_huge_page() deals with both regular and surplus pages. When there is a surplus, the pool needs to gravitate back to its configured size. In that case, pages are freed to the buddy allocator. In the absence of a surplus, pages are dealt with in the way you describe above. So put_page() does _precisely_ what is needed in this scenario.

Quoted 2 lines

I also think it's a crime that alloc_buddy_huge_page() doesn't share code with alloc_fresh_huge_page().

Different issue (unrelated to this patch), but I'll have a look and see if I can consolidate them.

Signature · 8 lines

Adam Litke - (agl at us.ibm.com)
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>