[PATCH] hugetlb: Fix dynamic pool resize failure case

[PATCH] hugetlb: Fix dynamic pool resize failure case

[Adam Litke]

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 <agl@us.ibm.com>
---

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

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 9b3dfac..f349c16 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -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 majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] hugetlb: Fix dynamic pool resize failure case

[Dave Hansen]

On Tue, 2007-10-09 at 08:58 -0700, Adam Litke wrote:
> index 9b3dfac..f349c16 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -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);
> +		}
>  	}

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().  

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

Re: [PATCH] hugetlb: Fix dynamic pool resize failure case

[Adam Litke]

On Tue, 2007-10-09 at 14:20 -0700, Dave Hansen wrote:
> On Tue, 2007-10-09 at 08:58 -0700, Adam Litke wrote:
> > index 9b3dfac..f349c16 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -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);
> > +		}
> >  	}
> 
> 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()

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

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

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

-- 
Adam Litke - (agl at us.ibm.com)
IBM Linux Technology Center

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

[PATCH] hugetlb: Fix dynamic pool resize failure case

[Adam Litke]

Changes since V1
	Added a comment explaining the free logic in gather_surplus_pages.

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 <agl@us.ibm.com>
---

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

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 9b3dfac..ce66c72 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -281,8 +281,17 @@ free:
 		list_del(&page->lru);
 		if ((--needed) >= 0)
 			enqueue_huge_page(page);
-		else
-			update_and_free_page(page);
+		else {
+			/*
+			 * Decrement the refcount and free the page using its
+			 * destructor.  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);
+			spin_lock(&hugetlb_lock);
+		}
 	}
 
 	return ret;

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

Re: [PATCH] hugetlb: Fix dynamic pool resize failure case

[Dave Hansen]

On Fri, 2007-10-12 at 12:15 -0700, Adam Litke wrote:
> 
> Changes since V1
>         Added a comment explaining the free logic in gather_surplus_pages.
> 
> 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.

The comment looks good.  It's much more obvious what it's doing now and
certainly answers all the questions I asked before.

I do think we need to consider some of the wider implications before the
series that this applies on top of goes to mainline, but this patch by
itself is just fine with me.

Acked-by: Dave Hansen <haveblue@us.ibm.com>

-- Dave

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