[PATCH] mm: hugetlb: defer freeing pages when gathering surplus pages

[PATCH] mm: hugetlb: defer freeing pages when gathering surplus pages

[Hillf Danton]

When gathering surplus pages, the number of needed pages is recomputed after
reacquiring hugetlb lock to catch changes in resv_huge_pages and
free_huge_pages. Plus it is recomputed with the number of newly allocated
pages involved.

Thus freeing pages could be deferred a bit to see if the final page request is
satisfied, though pages could be allocated less than needed.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/mm/hugetlb.c	Tue Feb 14 20:10:46 2012
+++ b/mm/hugetlb.c	Tue Feb 14 20:19:42 2012
@@ -852,6 +852,7 @@ static int gather_surplus_pages(struct h
 	struct page *page, *tmp;
 	int ret, i;
 	int needed, allocated;
+	bool alloc_ok = true;

 	needed = (h->resv_huge_pages + delta) - h->free_huge_pages;
 	if (needed <= 0) {
@@ -867,17 +868,13 @@ retry:
 	spin_unlock(&hugetlb_lock);
 	for (i = 0; i < needed; i++) {
 		page = alloc_buddy_huge_page(h, NUMA_NO_NODE);
-		if (!page)
-			/*
-			 * We were not able to allocate enough pages to
-			 * satisfy the entire reservation so we free what
-			 * we've allocated so far.
-			 */
-			goto free;
-
+		if (!page) {
+			alloc_ok = false;
+			break;
+		}
 		list_add(&page->lru, &surplus_list);
 	}
-	allocated += needed;
+	allocated += i;

 	/*
 	 * After retaking hugetlb_lock, we need to recalculate 'needed'
@@ -886,9 +883,16 @@ retry:
 	spin_lock(&hugetlb_lock);
 	needed = (h->resv_huge_pages + delta) -
 			(h->free_huge_pages + allocated);
-	if (needed > 0)
-		goto retry;
-
+	if (needed > 0) {
+		if (alloc_ok)
+			goto retry;
+		/*
+		 * We were not able to allocate enough pages to
+		 * satisfy the entire reservation so we free what
+		 * we've allocated so far.
+		 */
+		goto free;
+	}
 	/*
 	 * The surplus_list now contains _at_least_ the number of extra pages
 	 * needed to accommodate the reservation.  Add the appropriate number
@@ -914,10 +918,10 @@ retry:
 		VM_BUG_ON(page_count(page));
 		enqueue_huge_page(h, page);
 	}
+free:
 	spin_unlock(&hugetlb_lock);

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

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: hugetlb: defer freeing pages when gathering surplus pages

[Michal Hocko]

[Sorry for the late reply but I was away from email for quite sometime]

On Tue 14-02-12 20:53:51, Hillf Danton wrote:
> When gathering surplus pages, the number of needed pages is recomputed after
> reacquiring hugetlb lock to catch changes in resv_huge_pages and
> free_huge_pages. Plus it is recomputed with the number of newly allocated
> pages involved.
> 
> Thus freeing pages could be deferred a bit to see if the final page request is
> satisfied, though pages could be allocated less than needed.

The patch looks OK but I am missing a word why we need it. I guess
your primary motivation is that we want to reduce false positives when
we fail to allocate surplus pages while somebody freed some in the
background.
What is the workload that you observed such a behavior? Or is this just
from the code review?

> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
> 
> --- a/mm/hugetlb.c	Tue Feb 14 20:10:46 2012
> +++ b/mm/hugetlb.c	Tue Feb 14 20:19:42 2012
> @@ -852,6 +852,7 @@ static int gather_surplus_pages(struct h
>  	struct page *page, *tmp;
>  	int ret, i;
>  	int needed, allocated;
> +	bool alloc_ok = true;
> 
>  	needed = (h->resv_huge_pages + delta) - h->free_huge_pages;
>  	if (needed <= 0) {
> @@ -867,17 +868,13 @@ retry:
>  	spin_unlock(&hugetlb_lock);
>  	for (i = 0; i < needed; i++) {
>  		page = alloc_buddy_huge_page(h, NUMA_NO_NODE);
> -		if (!page)
> -			/*
> -			 * We were not able to allocate enough pages to
> -			 * satisfy the entire reservation so we free what
> -			 * we've allocated so far.
> -			 */
> -			goto free;
> -
> +		if (!page) {
> +			alloc_ok = false;
> +			break;
> +		}
>  		list_add(&page->lru, &surplus_list);
>  	}
> -	allocated += needed;
> +	allocated += i;
> 
>  	/*
>  	 * After retaking hugetlb_lock, we need to recalculate 'needed'
> @@ -886,9 +883,16 @@ retry:
>  	spin_lock(&hugetlb_lock);
>  	needed = (h->resv_huge_pages + delta) -
>  			(h->free_huge_pages + allocated);
> -	if (needed > 0)
> -		goto retry;
> -
> +	if (needed > 0) {
> +		if (alloc_ok)
> +			goto retry;
> +		/*
> +		 * We were not able to allocate enough pages to
> +		 * satisfy the entire reservation so we free what
> +		 * we've allocated so far.
> +		 */
> +		goto free;
> +	}
>  	/*
>  	 * The surplus_list now contains _at_least_ the number of extra pages
>  	 * needed to accommodate the reservation.  Add the appropriate number
> @@ -914,10 +918,10 @@ retry:
>  		VM_BUG_ON(page_count(page));
>  		enqueue_huge_page(h, page);
>  	}
> +free:
>  	spin_unlock(&hugetlb_lock);
> 
>  	/* Free unnecessary surplus pages to the buddy allocator */
> -free:
>  	if (!list_empty(&surplus_list)) {
>  		list_for_each_entry_safe(page, tmp, &surplus_list, lru) {
>  			list_del(&page->lru);
> --
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: hugetlb: defer freeing pages when gathering surplus pages

[Hillf Danton]

On Wed, Mar 14, 2012 at 7:13 PM, Michal Hocko <mhocko@suse.cz> wrote:
> [Sorry for the late reply but I was away from email for quite sometime]
>

Nice to see you back:)

> On Tue 14-02-12 20:53:51, Hillf Danton wrote:
>> When gathering surplus pages, the number of needed pages is recomputed after
>> reacquiring hugetlb lock to catch changes in resv_huge_pages and
>> free_huge_pages. Plus it is recomputed with the number of newly allocated
>> pages involved.
>>
>> Thus freeing pages could be deferred a bit to see if the final page request is
>> satisfied, though pages could be allocated less than needed.
>
> The patch looks OK but I am missing a word why we need it. I guess

False negative is removed as it should be.

> your primary motivation is that we want to reduce false positives when
> we fail to allocate surplus pages while somebody freed some in the
> background.
> What is the workload that you observed such a behavior? Or is this just
> from the code review?
>
The second.

-hd

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: hugetlb: defer freeing pages when gathering surplus pages

[Michal Hocko]

On Wed 14-03-12 21:08:13, Hillf Danton wrote:
> On Wed, Mar 14, 2012 at 7:13 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > [Sorry for the late reply but I was away from email for quite sometime]
> >
> 
> Nice to see you back:)

Thanks

> 
> > On Tue 14-02-12 20:53:51, Hillf Danton wrote:
> >> When gathering surplus pages, the number of needed pages is recomputed after
> >> reacquiring hugetlb lock to catch changes in resv_huge_pages and
> >> free_huge_pages. Plus it is recomputed with the number of newly allocated
> >> pages involved.
> >>
> >> Thus freeing pages could be deferred a bit to see if the final page request is
> >> satisfied, though pages could be allocated less than needed.
> >
> > The patch looks OK but I am missing a word why we need it. I guess
> 
> False negative is removed as it should be.

Right, I meant false negative. Would be nice to have it in the
changelog... Anyway, Andrew has already picked up the patch I guess.
Just in case

Reviewed-by: Michal Hocko <mhocko@suse.cz>

> > your primary motivation is that we want to reduce false positives when
> > we fail to allocate surplus pages while somebody freed some in the
> > background.
> > What is the workload that you observed such a behavior? Or is this just
> > from the code review?
> >
> The second.

OK

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>