[PATCH 0/3] hugetlb: Dynamic pool resize improvements

Adam Litke

author

2008-02-25 22:00 UTC #0 lore raw

cover hugetlb: Dynamic pool resize improvements

This series of patches contains fixes for a few issues found with the new dynamically resizing hugetlb pool while stress testing. The first patch corrects the page count for surplus huge pages when they are first allocated. This avoids a BUG when CONFIG_DEBUG_VM is enabled. The second patch closes a difficult to trigger race when setting up a reservation involving surplus pages which could lead to reservations not being honored. The third patch is a minor performance optimization in gather_surplus_huge_pages(). Patches 1 and 2 are candidates for -stable.

These patches were generated against 2.6.24

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

Adam Litke

author

2008-02-25 22:01 UTC #1 lore raw

PATCH 1/3 hugetlb: Correct page count for surplus huge pages

Free pages in the hugetlb pool are free and as such have a reference count of zero. Regular allocations into the pool from the buddy are "freed" into the pool which results in their page_count dropping to zero. However, surplus pages are directly utilized by the caller without first being freed so an explicit reset of the reference count is needed.

This hasn't effected end users because the bad page count is reset before the page is handed off. However, under CONFIG_DEBUG_VM this triggers a BUG when the page count is validated.

Thanks go to Mel for first spotting this issue and providing an initial fix.

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

Cc Mel Gorman <[email protected]>

---

 mm/hugetlb.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

mm/hugetlb.c +9 -3

index db861d8..026e5ee 100644
@@ -267,6 +267,11 @@ static struct page *alloc_buddy_huge_page(struct vm_area_struct *vma,
 
 	spin_lock(&hugetlb_lock);
 	if (page) {
+		/*
+		 * This page is now managed by the hugetlb allocator and has
+		 * no current users -- reset its reference count.
+		 */
+		set_page_count(page, 0);
 		nid = page_to_nid(page);
 		set_compound_page_dtor(page, free_huge_page);
 		/*

@@ -345,13 +350,14 @@ free:
 			enqueue_huge_page(page);
 		else {
 			/*
-			 * Decrement the refcount and free the page using its
-			 * destructor.  This must be done with hugetlb_lock
+			 * The page has a reference count of zero already, so
+			 * call free_huge_page directly instead of using
+			 * put_page.  This must be done with hugetlb_lock
 			 * unlocked which is safe because free_huge_page takes
 			 * hugetlb_lock before deciding how to free the page.
 			 */
 			spin_unlock(&hugetlb_lock);
-			put_page(page);
+			free_huge_page(page);
 			spin_lock(&hugetlb_lock);
 		}
 	}

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

2008-02-25 22:26 UTC #2 lore raw

Re: [PATCH 1/3] hugetlb: Correct page count for surplus huge pages

mm/hugetlb.c static struct page *alloc_buddy_huge_page(struct vm_area_struct *vma, · line 274

Quoted 8 lines · Adam Litke

        spin_lock(&hugetlb_lock);
        if (page) {

mm/hugetlb.c +5 -0

+               /*
+                * This page is now managed by the hugetlb allocator and has
+                * no current users -- reset its reference count.
+                */
+               set_page_count(page, 0);

So, they come out of the allocator and have a refcount of 1, and you want them to be consistent with the other huge pages that have a count of 0?

I'd feel a lot better about this if you did a __put_page() then a atomic_read() or equivalent to double-check what's going on. (I basically suggested the same thing to Jon Tollefson on the ginormous page stuff). It just forces the thing to be more consistent.

It also seems a bit goofy to me to zero the refcount here, then reset it to one later on in update_and_free_page().

I dunno. It just seems like every time something in here gets touched, three other things break. Makes me nervous. :(

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

Adam Litke

author

2008-02-25 22:57 UTC #3 lore raw

Re: [PATCH 1/3] hugetlb: Correct page count for surplus huge pages

Quoted 10 lines · Dave Hansen

mm/hugetlb.c static struct page *alloc_buddy_huge_page(struct vm_area_struct *vma, · line 274

Quoted 8 lines · Adam Litke

        spin_lock(&hugetlb_lock);
        if (page) {

mm/hugetlb.c +5 -0

+               /*
+                * This page is now managed by the hugetlb allocator and has
+                * no current users -- reset its reference count.
+                */
+               set_page_count(page, 0);

Quoted 3 lines

So, they come out of the allocator and have a refcount of 1, and you want them to be consistent with the other huge pages that have a count of 0?

Yep all pages come out of the buddy allocator in this state. What is different in this case is that we are choosing not to enqueue it into the hugetlb pool right away since it might be immediately needed by the caller.

Quoted 4 lines

I'd feel a lot better about this if you did a __put_page() then a atomic_read() or equivalent to double-check what's going on. (I basically suggested the same thing to Jon Tollefson on the ginormous page stuff). It just forces the thing to be more consistent.

I could agree to this.

Quoted 2 lines

It also seems a bit goofy to me to zero the refcount here, then reset it to one later on in update_and_free_page().

Yeah, it is a special case -- and commented accordingly. Do you have any ideas how to avoid it without the wasted time of an enqueue_huge_page()/dequeue_huge_page() cycle?

Quoted 2 lines

I dunno. It just seems like every time something in here gets touched, three other things break. Makes me nervous. :(

Now c'mon, that's not fair. I'd expect that sort of statement from the Hillary Clinton campaign, not you Dave :)

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>

Dave Hansen

2008-02-25 23:11 UTC #4 lore raw

Re: [PATCH 1/3] hugetlb: Correct page count for surplus huge pages

Quoted 6 lines · Adam Litke

Quoted 2 lines
It also seems a bit goofy to me to zero the refcount here, then reset it to one later on in update_and_free_page().
Yeah, it is a special case -- and commented accordingly. Do you have any ideas how to avoid it without the wasted time of an enqueue_huge_page()/dequeue_huge_page() cycle?

There are a couple of steps here, right?

1. alloc from the buddy list
2. initialize to set ->dtor, page->_count, etc...
3. enqueue_huge_page()
4. somebody does dequeue_huge_page() and gets it

I wonder if it might get simpler if you just make the pages on the freelists "virgin buddy pages". Basically don't touch pages much until after they're dequeued. Flip flop (a la John Kerry) the order around a bit:

1. alloc from the buddy list
2. enqueue_huge_page()
3. somebody does dequeue_huge_page() and before it returns, we:
4. initialize to set ->dtor, page->_count, etc...

This has the disadvantage of shifting some work from a "once per alloc from the buddy list" to "once per en/dequeue". Basically, just try and re-think when you turn pages from plain buddy pages into hugetlb-flavored pages.

-- Dave

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

Adam Litke

author

2008-02-26 16:45 UTC #5 lore raw

Re: [PATCH 1/3] hugetlb: Correct page count for surplus huge pages

On Mon, 2008-02-25 at 15:11 -0800, Dave Hansen wrote:
<snip>

Quoted 10 lines

I wonder if it might get simpler if you just make the pages on the freelists "virgin buddy pages". Basically don't touch pages much until after they're dequeued. Flip flop (a la John Kerry) the order around a bit:
1. alloc from the buddy list
2. enqueue_huge_page()
3. somebody does dequeue_huge_page() and before it returns, we:
4. initialize to set ->dtor, page->_count, etc...

Quoted 4 lines

This has the disadvantage of shifting some work from a "once per alloc from the buddy list" to "once per en/dequeue". Basically, just try and re-think when you turn pages from plain buddy pages into hugetlb-flavored pages.

This is an interesting idea and I will think about it some more. However, switching this around will introduce more of the churn that makes people nervous. So I would appeal that we put forth my original idea (with your suggested modification) because it is a simple and verifiable bug fix. Amended patch follows:

commit 635ff936930f5c56be23feffd06764554f88f8ad

Author: Adam Litke <[email protected]>
Date:   Tue Feb 26 08:42:33 2008 -0800

hugetlb: Correct page count for surplus huge pages

Free pages in the hugetlb pool are free and as such have a reference count of zero. Regular allocations into the pool from the buddy are "freed" into the pool which results in their page_count dropping to zero. However, surplus pages are directly utilized by the caller without first being freed so an explicit reset of the reference count is needed.

This hasn't effected end users because the bad page count is reset before the page is handed off. However, under CONFIG_DEBUG_VM this triggers a BUG when the page count is validated.

Thanks go to Mel for first spotting this issue and providing an initial fix.

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

Cc Mel Gorman <[email protected]>

mm/hugetlb.c +9 -3

index db861d8..5afcacb 100644
@@ -267,6 +267,11 @@ static struct page *alloc_buddy_huge_page(struct vm_area_struct *vma,
 
 	spin_lock(&hugetlb_lock);
 	if (page) {
+		/*
+		 * This page is now managed by the hugetlb allocator and has
+		 * no current users -- reset its reference count.
+		 */
+		BUG_ON(!put_page_testzero(page));
 		nid = page_to_nid(page);
 		set_compound_page_dtor(page, free_huge_page);
 		/*

@@ -345,13 +350,14 @@ free:
 			enqueue_huge_page(page);
 		else {
 			/*
-			 * Decrement the refcount and free the page using its
-			 * destructor.  This must be done with hugetlb_lock
+			 * The page has a reference count of zero already, so
+			 * call free_huge_page directly instead of using
+			 * put_page.  This must be done with hugetlb_lock
 			 * unlocked which is safe because free_huge_page takes
 			 * hugetlb_lock before deciding how to free the page.
 			 */
 			spin_unlock(&hugetlb_lock);
-			put_page(page);
+			free_huge_page(page);
 			spin_lock(&hugetlb_lock);
 		}
 	}

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>

Dave Hansen

2008-02-26 16:48 UTC #6 lore raw

Re: [PATCH 1/3] hugetlb: Correct page count for surplus huge pages

Quoted 3 lines · Adam Litke

This is an interesting idea and I will think about it some more. However, switching this around will introduce more of the churn that makes people nervous.

Touche!

Quoted 3 lines

So I would appeal that we put forth my original
idea (with your suggested modification) because it is a simple and verifiable bug fix.

Looks good, thanks Adam.

-- Dave

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

Adam Litke

author

2008-02-25 22:01 UTC #7 lore raw

PATCH 2/3 hugetlb: Close a difficult to trigger reservation race

In reply to Adam Litke

If the following occurs in threads A and B:
A) Allocates some surplus pages to satisfy a reservation
B) Frees some huge pages
A) A notices the extra free pages and drops hugetlb_lock to free some of its surplus pages back to the buddy allocator.
B) Allocates some huge pages
A) Reacquires hugetlb_lock and returns from gather_surplus_huge_pages()

This series of events can result in a "short" reservation and subsequent application failure.

Avoid this by commiting the reservation after pages have been allocated but before dropping the lock to free excess pages. For parity, release the reservation in return_unused_surplus_pages().

Thanks to Andy Whitcroft for discovering this.

This does make the functions gather_surplus_huge_pages() and return_unused_surplus_pages() responsible for a bit more than their names imply. Does anyone support function name changes to create_reservation() and destroy_reservation() respectively?

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

Cc Mel Gorman <[email protected]>

Cc Andy Whitcroft <[email protected]>

---

 mm/hugetlb.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

mm/hugetlb.c +13 -4

index 026e5ee..8296431 100644
@@ -300,8 +300,10 @@ static int gather_surplus_pages(int delta)
 	int needed, allocated;
 
 	needed = (resv_huge_pages + delta) - free_huge_pages;
-	if (needed <= 0)
+	if (needed <= 0) {
+		resv_huge_pages += delta;
 		return 0;
+	}
 
 	allocated = 0;
 	INIT_LIST_HEAD(&surplus_list);

@@ -339,9 +341,12 @@ retry:
 	 * The surplus_list now contains _at_least_ the number of extra pages
 	 * needed to accomodate the reservation.  Add the appropriate number
 	 * of pages to the hugetlb pool and free the extras back to the buddy
-	 * allocator.
+	 * allocator.  Commit the entire reservation here to prevent another
+	 * process from stealing the pages as they are added to the pool but
+	 * before they are reserved.
 	 */
 	needed += allocated;
+	resv_huge_pages += delta;
 	ret = 0;
 free:
 	list_for_each_entry_safe(page, tmp, &surplus_list, lru) {

@@ -376,6 +381,9 @@ static void return_unused_surplus_pages(unsigned long unused_resv_pages)
 	struct page *page;
 	unsigned long nr_pages;
 
+	/* Uncommit the reservation */
+	resv_huge_pages -= unused_resv_pages;
+
 	nr_pages = min(unused_resv_pages, surplus_huge_pages);
 
 	while (nr_pages) {

@@ -1197,12 +1205,13 @@ static int hugetlb_acct_memory(long delta)
 		if (gather_surplus_pages(delta) < 0)
 			goto out;
 
-		if (delta > cpuset_mems_nr(free_huge_pages_node))
+		if (delta > cpuset_mems_nr(free_huge_pages_node)) {
+			return_unused_surplus_pages(delta);
 			goto out;
+		}
 	}
 
 	ret = 0;
-	resv_huge_pages += delta;
 	if (delta < 0)
 		return_unused_surplus_pages((unsigned long) -delta);

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

Adam Litke

author

2008-02-25 22:01 UTC #8 lore raw

PATCH 3/3 hugetlb: Decrease hugetlb_lock cycling in gather_surplus_huge_pages

In reply to Adam Litke

To reduce the number of times we acquire and release hugetlb_lock when freeing excess huge pages, loop through the page list twice: once to siphon pages into the hugetlb pool, and again to free the excess pages back to the buddy allocator. This removes the lock/unlock around free_huge_page().

Note that we still visit each page only once since a page is always removed from the list when it is visited.

Thanks Mel Gorman for this improvement.

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

Cc Mel Gorman <[email protected]>

---

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

mm/hugetlb.c +12 -5

index 8296431..306d762 100644
@@ -349,11 +349,19 @@ retry:
 	resv_huge_pages += delta;
 	ret = 0;
 free:
+	/* Free the needed pages to the hugetlb pool */
 	list_for_each_entry_safe(page, tmp, &surplus_list, lru) {
+		if ((--needed) < 0)
+			break;
 		list_del(&page->lru);
-		if ((--needed) >= 0)
-			enqueue_huge_page(page);
-		else {
+		enqueue_huge_page(page);
+	}
+
+	/* Free unnecessary surplus pages to the buddy allocator */
+	if (!list_empty(&surplus_list)) {
+		spin_unlock(&hugetlb_lock);
+		list_for_each_entry_safe(page, tmp, &surplus_list, lru) {
+			list_del(&page->lru);
 			/*
 			 * The page has a reference count of zero already, so
 			 * call free_huge_page directly instead of using

@@ -361,10 +369,9 @@ free:
 			 * unlocked which is safe because free_huge_page takes
 			 * hugetlb_lock before deciding how to free the page.
 			 */
-			spin_unlock(&hugetlb_lock);
 			free_huge_page(page);
-			spin_lock(&hugetlb_lock);
 		}
+		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

2008-02-25 22:31 UTC #9 lore raw

Re: [PATCH 3/3] hugetlb: Decrease hugetlb_lock cycling in gather_surplus_huge_pages

mm/hugetlb.c retry: · line 364

Quoted 5 lines · Adam Litke

mm/hugetlb.c +5 -0

+       /* Free unnecessary surplus pages to the buddy allocator */
+       if (!list_empty(&surplus_list)) {
+               spin_unlock(&hugetlb_lock);
+               list_for_each_entry_safe(page, tmp, &surplus_list, lru) {
+                       list_del(&page->lru);

What is the surplus_list protected by?

-- Dave

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

Adam Litke

author

2008-02-25 22:44 UTC #10 lore raw

Re: [PATCH 3/3] hugetlb: Decrease hugetlb_lock cycling in gather_surplus_huge_pages

Quoted 8 lines · Dave Hansen

mm/hugetlb.c retry: · line 364

Quoted 5 lines · Adam Litke

mm/hugetlb.c +5 -0

+       /* Free unnecessary surplus pages to the buddy allocator */
+       if (!list_empty(&surplus_list)) {
+               spin_unlock(&hugetlb_lock);
+               list_for_each_entry_safe(page, tmp, &surplus_list, lru) {
+                       list_del(&page->lru);

What is the surplus_list protected by?

It's a local variable on the stack.

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>