[RFC 0/3] hugetlb: constrain allocation/free based on task mempolicy

[RFC 0/3] hugetlb: constrain allocation/free based on task mempolicy

[Lee Schermerhorn]

RFC 0/3 hugetlb: constrain allocation/free based on task mempolicy

Against:  25jun09 mmotm atop the "hugetlb:  balance freeing..."
series

This is V1 of a series of patches to constrain the allocation and
freeing of persistent huge pages using the task NUMA mempolicy of
the task modifying "nr_hugepages".  This series is based on Mel
Gorman's suggestion to use task mempolicy.

I have some concerns about a subtle change in behavior [see patch
2/3 and the updated documentation] and the fact that
this mechanism ignores some of the semantics of the mempolicy
mode [again, see the doc].   However, this method seems to work
fairly well.  And, IMO, the resulting code doesn't look all that
bad.

A couple of limitations in this version:

1) I haven't implemented a boot time parameter to constrain the
   boot time allocation of huge pages.  This can be added if
   anyone feels strongly that it is required.

2) I have not implemented a per node nr_overcommit_hugepages as
   David Rientjes and I discussed earlier.  Again, this can be
   added and specific nodes can be addressed using the mempolicy
   as this series does for allocation and free.

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

[RFC 1/3] hugetlb: add nodemask arg to huge page alloc, free and surplus adjust fcns

[Lee Schermerhorn]

[RFC 1/3] hugetlb:  add nodemask arg to huge page alloc, free and surplus adjust fcns

Against: 25jun09 mmotm atop the "hugetlb: balance freeing..." series

In preparation for constraining huge page allocation and freeing by the
controlling task's numa mempolicy, add a "nodes_allowed" nodemask pointer
to the allocate, free and surplus adjustment functions.  For now, pass
NULL to indicate default behavior--i.e., use node_online_map.  A
subsqeuent patch will derive a non-default mask from the controlling 
task's numa mempolicy.

Note the "cleanup" in alloc_bootmem_huge_page(): always advance next nid,
even if allocation succeeds.  I believe that this is correct behavior,
and I'll replace it in the next patch which assumes this behavior.
However, perhaps the current code is correct:  we only want to advance
bootmem huge page allocation to the next node when we've exhausted all
huge pages on the current hstate "next_node_to_alloc".  Any who understands
the rationale for this:  please advise.

Signed-off-by: Lee Schermerhorn <lee.schermerhorn@hp.com>

 mm/hugetlb.c |   51 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 20 deletions(-)

Index: linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c
===================================================================
--- linux-2.6.31-rc1-mmotm-090625-1549.orig/mm/hugetlb.c	2009-06-29 17:35:11.000000000 -0400
+++ linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c	2009-06-29 23:01:01.000000000 -0400
@@ -631,17 +631,22 @@ static struct page *alloc_fresh_huge_pag
  * if we just successfully allocated a hugepage so that
  * the next caller gets hugepages on the next node.
  */
-static int hstate_next_node_to_alloc(struct hstate *h)
+static int hstate_next_node_to_alloc(struct hstate *h,
+					nodemask_t *nodes_allowed)
 {
 	int next_nid;
-	next_nid = next_node(h->next_nid_to_alloc, node_online_map);
+
+	if (!nodes_allowed)
+		nodes_allowed = &node_online_map;
+
+	next_nid = next_node(h->next_nid_to_alloc, *nodes_allowed);
 	if (next_nid == MAX_NUMNODES)
-		next_nid = first_node(node_online_map);
+		next_nid = first_node(*nodes_allowed);
 	h->next_nid_to_alloc = next_nid;
 	return next_nid;
 }
 
-static int alloc_fresh_huge_page(struct hstate *h)
+static int alloc_fresh_huge_page(struct hstate *h, nodemask_t *nodes_allowed)
 {
 	struct page *page;
 	int start_nid;
@@ -655,7 +660,7 @@ static int alloc_fresh_huge_page(struct 
 		page = alloc_fresh_huge_page_node(h, next_nid);
 		if (page)
 			ret = 1;
-		next_nid = hstate_next_node_to_alloc(h);
+		next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
 	} while (!page && next_nid != start_nid);
 
 	if (ret)
@@ -670,12 +675,16 @@ static int alloc_fresh_huge_page(struct 
  * helper for free_pool_huge_page() - find next node
  * from which to free a huge page
  */
-static int hstate_next_node_to_free(struct hstate *h)
+static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
 {
 	int next_nid;
-	next_nid = next_node(h->next_nid_to_free, node_online_map);
+
+	if (!nodes_allowed)
+		nodes_allowed = &node_online_map;
+
+	next_nid = next_node(h->next_nid_to_free, *nodes_allowed);
 	if (next_nid == MAX_NUMNODES)
-		next_nid = first_node(node_online_map);
+		next_nid = first_node(*nodes_allowed);
 	h->next_nid_to_free = next_nid;
 	return next_nid;
 }
@@ -686,7 +695,8 @@ static int hstate_next_node_to_free(stru
  * balanced over allowed nodes.
  * Called with hugetlb_lock locked.
  */
-static int free_pool_huge_page(struct hstate *h, bool acct_surplus)
+static int free_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
+							 bool acct_surplus)
 {
 	int start_nid;
 	int next_nid;
@@ -717,7 +727,7 @@ static int free_pool_huge_page(struct hs
 			update_and_free_page(h, page);
 			ret = 1;
 		}
-		next_nid = hstate_next_node_to_free(h);
+ 		next_nid = hstate_next_node_to_free(h, nodes_allowed);
 	} while (!ret && next_nid != start_nid);
 
 	return ret;
@@ -919,7 +929,7 @@ static void return_unused_surplus_pages(
 	 * on-line nodes for us and will handle the hstate accounting.
 	 */
 	while (nr_pages--) {
-		if (!free_pool_huge_page(h, 1))
+		if (!free_pool_huge_page(h, NULL, 1))
 			break;
 	}
 }
@@ -1032,6 +1042,7 @@ int __weak alloc_bootmem_huge_page(struc
 				NODE_DATA(h->next_nid_to_alloc),
 				huge_page_size(h), huge_page_size(h), 0);
 
+		hstate_next_node_to_alloc(h, NULL); /* always advance nid */
 		if (addr) {
 			/*
 			 * Use the beginning of the huge page to store the
@@ -1041,7 +1052,6 @@ int __weak alloc_bootmem_huge_page(struc
 			m = addr;
 			goto found;
 		}
-		hstate_next_node_to_alloc(h);
 		nr_nodes--;
 	}
 	return 0;
@@ -1085,7 +1095,7 @@ static void __init hugetlb_hstate_alloc_
 		if (h->order >= MAX_ORDER) {
 			if (!alloc_bootmem_huge_page(h))
 				break;
-		} else if (!alloc_fresh_huge_page(h))
+		} else if (!alloc_fresh_huge_page(h, NULL))
 			break;
 	}
 	h->max_huge_pages = i;
@@ -1160,7 +1170,8 @@ static inline void try_to_free_low(struc
  * balanced by operating on them in a round-robin fashion.
  * Returns 1 if an adjustment was made.
  */
-static int adjust_pool_surplus(struct hstate *h, int delta)
+static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,
+				int delta)
 {
 	int start_nid, next_nid;
 	int ret = 0;
@@ -1176,7 +1187,7 @@ static int adjust_pool_surplus(struct hs
 	do {
 		int nid = next_nid;
 		if (delta < 0)  {
-			next_nid = hstate_next_node_to_alloc(h);
+			next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
 			/*
 			 * To shrink on this node, there must be a surplus page
 			 */
@@ -1184,7 +1195,7 @@ static int adjust_pool_surplus(struct hs
 				continue;
 		}
 		if (delta > 0) {
-			next_nid = hstate_next_node_to_free(h);
+			next_nid = hstate_next_node_to_free(h, nodes_allowed);
 			/*
 			 * Surplus cannot exceed the total number of pages
 			 */
@@ -1223,7 +1234,7 @@ static unsigned long set_max_huge_pages(
 	 */
 	spin_lock(&hugetlb_lock);
 	while (h->surplus_huge_pages && count > persistent_huge_pages(h)) {
-		if (!adjust_pool_surplus(h, -1))
+		if (!adjust_pool_surplus(h, NULL, -1))
 			break;
 	}
 
@@ -1234,7 +1245,7 @@ static unsigned long set_max_huge_pages(
 		 * and reducing the surplus.
 		 */
 		spin_unlock(&hugetlb_lock);
-		ret = alloc_fresh_huge_page(h);
+		ret = alloc_fresh_huge_page(h, NULL);
 		spin_lock(&hugetlb_lock);
 		if (!ret)
 			goto out;
@@ -1260,11 +1271,11 @@ static unsigned long set_max_huge_pages(
 	min_count = max(count, min_count);
 	try_to_free_low(h, min_count);
 	while (min_count < persistent_huge_pages(h)) {
-		if (!free_pool_huge_page(h, 0))
+		if (!free_pool_huge_page(h, NULL, 0))
 			break;
 	}
 	while (count < persistent_huge_pages(h)) {
-		if (!adjust_pool_surplus(h, 1))
+		if (!adjust_pool_surplus(h, NULL, 1))
 			break;
 	}
 out:

--
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: [RFC 1/3] hugetlb: add nodemask arg to huge page alloc, free and surplus adjust fcns

[Mel Gorman]

On Tue, Jun 30, 2009 at 11:47:24AM -0400, Lee Schermerhorn wrote:
> [RFC 1/3] hugetlb:  add nodemask arg to huge page alloc, free and surplus adjust fcns
> 
> Against: 25jun09 mmotm atop the "hugetlb: balance freeing..." series
> 
> In preparation for constraining huge page allocation and freeing by the
> controlling task's numa mempolicy, add a "nodes_allowed" nodemask pointer
> to the allocate, free and surplus adjustment functions.  For now, pass
> NULL to indicate default behavior--i.e., use node_online_map.  A
> subsqeuent patch will derive a non-default mask from the controlling 
> task's numa mempolicy.
> 
> Note the "cleanup" in alloc_bootmem_huge_page(): always advance next nid,
> even if allocation succeeds.  I believe that this is correct behavior,
> and I'll replace it in the next patch which assumes this behavior.
> However, perhaps the current code is correct:  we only want to advance
> bootmem huge page allocation to the next node when we've exhausted all
> huge pages on the current hstate "next_node_to_alloc".  Any who understands
> the rationale for this:  please advise.
> 
> Signed-off-by: Lee Schermerhorn <lee.schermerhorn@hp.com>
> 
>  mm/hugetlb.c |   51 +++++++++++++++++++++++++++++++--------------------
>  1 file changed, 31 insertions(+), 20 deletions(-)
> 
> Index: linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c
> ===================================================================
> --- linux-2.6.31-rc1-mmotm-090625-1549.orig/mm/hugetlb.c	2009-06-29 17:35:11.000000000 -0400
> +++ linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c	2009-06-29 23:01:01.000000000 -0400
> @@ -631,17 +631,22 @@ static struct page *alloc_fresh_huge_pag
>   * if we just successfully allocated a hugepage so that
>   * the next caller gets hugepages on the next node.
>   */
> -static int hstate_next_node_to_alloc(struct hstate *h)
> +static int hstate_next_node_to_alloc(struct hstate *h,
> +					nodemask_t *nodes_allowed)
>  {
>  	int next_nid;
> -	next_nid = next_node(h->next_nid_to_alloc, node_online_map);
> +
> +	if (!nodes_allowed)
> +		nodes_allowed = &node_online_map;
> +
> +	next_nid = next_node(h->next_nid_to_alloc, *nodes_allowed);
>  	if (next_nid == MAX_NUMNODES)
> -		next_nid = first_node(node_online_map);
> +		next_nid = first_node(*nodes_allowed);
>  	h->next_nid_to_alloc = next_nid;
>  	return next_nid;
>  }
>  
> -static int alloc_fresh_huge_page(struct hstate *h)
> +static int alloc_fresh_huge_page(struct hstate *h, nodemask_t *nodes_allowed)
>  {
>  	struct page *page;
>  	int start_nid;
> @@ -655,7 +660,7 @@ static int alloc_fresh_huge_page(struct 
>  		page = alloc_fresh_huge_page_node(h, next_nid);
>  		if (page)
>  			ret = 1;
> -		next_nid = hstate_next_node_to_alloc(h);
> +		next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
>  	} while (!page && next_nid != start_nid);
>  
>  	if (ret)
> @@ -670,12 +675,16 @@ static int alloc_fresh_huge_page(struct 
>   * helper for free_pool_huge_page() - find next node
>   * from which to free a huge page
>   */
> -static int hstate_next_node_to_free(struct hstate *h)
> +static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
>  {
>  	int next_nid;
> -	next_nid = next_node(h->next_nid_to_free, node_online_map);
> +
> +	if (!nodes_allowed)
> +		nodes_allowed = &node_online_map;
> +
> +	next_nid = next_node(h->next_nid_to_free, *nodes_allowed);
>  	if (next_nid == MAX_NUMNODES)
> -		next_nid = first_node(node_online_map);
> +		next_nid = first_node(*nodes_allowed);
>  	h->next_nid_to_free = next_nid;
>  	return next_nid;
>  }
> @@ -686,7 +695,8 @@ static int hstate_next_node_to_free(stru
>   * balanced over allowed nodes.
>   * Called with hugetlb_lock locked.
>   */
> -static int free_pool_huge_page(struct hstate *h, bool acct_surplus)
> +static int free_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
> +							 bool acct_surplus)
>  {
>  	int start_nid;
>  	int next_nid;
> @@ -717,7 +727,7 @@ static int free_pool_huge_page(struct hs
>  			update_and_free_page(h, page);
>  			ret = 1;
>  		}
> -		next_nid = hstate_next_node_to_free(h);
> + 		next_nid = hstate_next_node_to_free(h, nodes_allowed);
>  	} while (!ret && next_nid != start_nid);
>  
>  	return ret;
> @@ -919,7 +929,7 @@ static void return_unused_surplus_pages(
>  	 * on-line nodes for us and will handle the hstate accounting.
>  	 */
>  	while (nr_pages--) {
> -		if (!free_pool_huge_page(h, 1))
> +		if (!free_pool_huge_page(h, NULL, 1))
>  			break;
>  	}
>  }
> @@ -1032,6 +1042,7 @@ int __weak alloc_bootmem_huge_page(struc
>  				NODE_DATA(h->next_nid_to_alloc),
>  				huge_page_size(h), huge_page_size(h), 0);
>  
> +		hstate_next_node_to_alloc(h, NULL); /* always advance nid */
>  		if (addr) {
>  			/*
>  			 * Use the beginning of the huge page to store the
> @@ -1041,7 +1052,6 @@ int __weak alloc_bootmem_huge_page(struc
>  			m = addr;
>  			goto found;
>  		}
> -		hstate_next_node_to_alloc(h);
>  		nr_nodes--;

I strongly suspect that the same node being used until allocation
failure instead of round-robin is an oversight and not deliberate at
all. I can't think of a good reason for boot-allocation to behave
significantly different to runtime-allocation.

This could be a seperate patch just for consideration in isolation but I
for one have no problem with it. If you split out this patch, feel free
to add an Ack from me.

>  	}
>  	return 0;
> @@ -1085,7 +1095,7 @@ static void __init hugetlb_hstate_alloc_
>  		if (h->order >= MAX_ORDER) {
>  			if (!alloc_bootmem_huge_page(h))
>  				break;
> -		} else if (!alloc_fresh_huge_page(h))
> +		} else if (!alloc_fresh_huge_page(h, NULL))
>  			break;
>  	}
>  	h->max_huge_pages = i;
> @@ -1160,7 +1170,8 @@ static inline void try_to_free_low(struc
>   * balanced by operating on them in a round-robin fashion.
>   * Returns 1 if an adjustment was made.
>   */
> -static int adjust_pool_surplus(struct hstate *h, int delta)
> +static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,
> +				int delta)
>  {
>  	int start_nid, next_nid;
>  	int ret = 0;
> @@ -1176,7 +1187,7 @@ static int adjust_pool_surplus(struct hs
>  	do {
>  		int nid = next_nid;
>  		if (delta < 0)  {
> -			next_nid = hstate_next_node_to_alloc(h);
> +			next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
>  			/*
>  			 * To shrink on this node, there must be a surplus page
>  			 */
> @@ -1184,7 +1195,7 @@ static int adjust_pool_surplus(struct hs
>  				continue;
>  		}
>  		if (delta > 0) {
> -			next_nid = hstate_next_node_to_free(h);
> +			next_nid = hstate_next_node_to_free(h, nodes_allowed);
>  			/*
>  			 * Surplus cannot exceed the total number of pages
>  			 */
> @@ -1223,7 +1234,7 @@ static unsigned long set_max_huge_pages(
>  	 */
>  	spin_lock(&hugetlb_lock);
>  	while (h->surplus_huge_pages && count > persistent_huge_pages(h)) {
> -		if (!adjust_pool_surplus(h, -1))
> +		if (!adjust_pool_surplus(h, NULL, -1))
>  			break;
>  	}
>  
> @@ -1234,7 +1245,7 @@ static unsigned long set_max_huge_pages(
>  		 * and reducing the surplus.
>  		 */
>  		spin_unlock(&hugetlb_lock);
> -		ret = alloc_fresh_huge_page(h);
> +		ret = alloc_fresh_huge_page(h, NULL);
>  		spin_lock(&hugetlb_lock);
>  		if (!ret)
>  			goto out;
> @@ -1260,11 +1271,11 @@ static unsigned long set_max_huge_pages(
>  	min_count = max(count, min_count);
>  	try_to_free_low(h, min_count);
>  	while (min_count < persistent_huge_pages(h)) {
> -		if (!free_pool_huge_page(h, 0))
> +		if (!free_pool_huge_page(h, NULL, 0))
>  			break;
>  	}
>  	while (count < persistent_huge_pages(h)) {
> -		if (!adjust_pool_surplus(h, 1))
> +		if (!adjust_pool_surplus(h, NULL, 1))
>  			break;
>  	}
>  out:
> 

Otherwise, this looks like a relatively straight-forward conversion to
having nodemask awareness. I'd expect no behavioural changes from this
patch at all so

Reviewed-by: Mel Gorman <mel@csn.ul.ie>

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
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: [RFC 1/3] hugetlb: add nodemask arg to huge page alloc, free and surplus adjust fcns

[Lee Schermerhorn]

On Wed, 2009-07-01 at 13:38 +0100, Mel Gorman wrote:
> On Tue, Jun 30, 2009 at 11:47:24AM -0400, Lee Schermerhorn wrote:
> > [RFC 1/3] hugetlb:  add nodemask arg to huge page alloc, free and surplus adjust fcns
> > 
> > Against: 25jun09 mmotm atop the "hugetlb: balance freeing..." series
> > 
> > In preparation for constraining huge page allocation and freeing by the
> > controlling task's numa mempolicy, add a "nodes_allowed" nodemask pointer
> > to the allocate, free and surplus adjustment functions.  For now, pass
> > NULL to indicate default behavior--i.e., use node_online_map.  A
> > subsqeuent patch will derive a non-default mask from the controlling 
> > task's numa mempolicy.
> > 
> > Note the "cleanup" in alloc_bootmem_huge_page(): always advance next nid,
> > even if allocation succeeds.  I believe that this is correct behavior,
> > and I'll replace it in the next patch which assumes this behavior.
> > However, perhaps the current code is correct:  we only want to advance
> > bootmem huge page allocation to the next node when we've exhausted all
> > huge pages on the current hstate "next_node_to_alloc".  Any who understands
> > the rationale for this:  please advise.
> > 
> > Signed-off-by: Lee Schermerhorn <lee.schermerhorn@hp.com>
> > 
> >  mm/hugetlb.c |   51 +++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 31 insertions(+), 20 deletions(-)
> > 
> > Index: linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c
> > ===================================================================
> > --- linux-2.6.31-rc1-mmotm-090625-1549.orig/mm/hugetlb.c	2009-06-29 17:35:11.000000000 -0400
> > +++ linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c	2009-06-29 23:01:01.000000000 -0400
> > @@ -631,17 +631,22 @@ static struct page *alloc_fresh_huge_pag
> >   * if we just successfully allocated a hugepage so that
> >   * the next caller gets hugepages on the next node.
> >   */
> > -static int hstate_next_node_to_alloc(struct hstate *h)
> > +static int hstate_next_node_to_alloc(struct hstate *h,
> > +					nodemask_t *nodes_allowed)
> >  {
> >  	int next_nid;
> > -	next_nid = next_node(h->next_nid_to_alloc, node_online_map);
> > +
> > +	if (!nodes_allowed)
> > +		nodes_allowed = &node_online_map;
> > +
> > +	next_nid = next_node(h->next_nid_to_alloc, *nodes_allowed);
> >  	if (next_nid == MAX_NUMNODES)
> > -		next_nid = first_node(node_online_map);
> > +		next_nid = first_node(*nodes_allowed);
> >  	h->next_nid_to_alloc = next_nid;
> >  	return next_nid;
> >  }
> >  
> > -static int alloc_fresh_huge_page(struct hstate *h)
> > +static int alloc_fresh_huge_page(struct hstate *h, nodemask_t *nodes_allowed)
> >  {
> >  	struct page *page;
> >  	int start_nid;
> > @@ -655,7 +660,7 @@ static int alloc_fresh_huge_page(struct 
> >  		page = alloc_fresh_huge_page_node(h, next_nid);
> >  		if (page)
> >  			ret = 1;
> > -		next_nid = hstate_next_node_to_alloc(h);
> > +		next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
> >  	} while (!page && next_nid != start_nid);
> >  
> >  	if (ret)
> > @@ -670,12 +675,16 @@ static int alloc_fresh_huge_page(struct 
> >   * helper for free_pool_huge_page() - find next node
> >   * from which to free a huge page
> >   */
> > -static int hstate_next_node_to_free(struct hstate *h)
> > +static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
> >  {
> >  	int next_nid;
> > -	next_nid = next_node(h->next_nid_to_free, node_online_map);
> > +
> > +	if (!nodes_allowed)
> > +		nodes_allowed = &node_online_map;
> > +
> > +	next_nid = next_node(h->next_nid_to_free, *nodes_allowed);
> >  	if (next_nid == MAX_NUMNODES)
> > -		next_nid = first_node(node_online_map);
> > +		next_nid = first_node(*nodes_allowed);
> >  	h->next_nid_to_free = next_nid;
> >  	return next_nid;
> >  }
> > @@ -686,7 +695,8 @@ static int hstate_next_node_to_free(stru
> >   * balanced over allowed nodes.
> >   * Called with hugetlb_lock locked.
> >   */
> > -static int free_pool_huge_page(struct hstate *h, bool acct_surplus)
> > +static int free_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
> > +							 bool acct_surplus)
> >  {
> >  	int start_nid;
> >  	int next_nid;
> > @@ -717,7 +727,7 @@ static int free_pool_huge_page(struct hs
> >  			update_and_free_page(h, page);
> >  			ret = 1;
> >  		}
> > -		next_nid = hstate_next_node_to_free(h);
> > + 		next_nid = hstate_next_node_to_free(h, nodes_allowed);
> >  	} while (!ret && next_nid != start_nid);
> >  
> >  	return ret;
> > @@ -919,7 +929,7 @@ static void return_unused_surplus_pages(
> >  	 * on-line nodes for us and will handle the hstate accounting.
> >  	 */
> >  	while (nr_pages--) {
> > -		if (!free_pool_huge_page(h, 1))
> > +		if (!free_pool_huge_page(h, NULL, 1))
> >  			break;
> >  	}
> >  }
> > @@ -1032,6 +1042,7 @@ int __weak alloc_bootmem_huge_page(struc
> >  				NODE_DATA(h->next_nid_to_alloc),
> >  				huge_page_size(h), huge_page_size(h), 0);
> >  
> > +		hstate_next_node_to_alloc(h, NULL); /* always advance nid */
> >  		if (addr) {
> >  			/*
> >  			 * Use the beginning of the huge page to store the
> > @@ -1041,7 +1052,6 @@ int __weak alloc_bootmem_huge_page(struc
> >  			m = addr;
> >  			goto found;
> >  		}
> > -		hstate_next_node_to_alloc(h);
> >  		nr_nodes--;
> 
> I strongly suspect that the same node being used until allocation
> failure instead of round-robin is an oversight and not deliberate at
> all. I can't think of a good reason for boot-allocation to behave
> significantly different to runtime-allocation.
> 
> This could be a seperate patch just for consideration in isolation but I
> for one have no problem with it. If you split out this patch, feel free
> to add an Ack from me.

OK.  If this series doesn't fly--e.g., we take another approach--I will
definitely split this out.  If Andrew wants it separate for the change
log, I can do that as well.

> 
> >  	}
> >  	return 0;
> > @@ -1085,7 +1095,7 @@ static void __init hugetlb_hstate_alloc_
> >  		if (h->order >= MAX_ORDER) {
> >  			if (!alloc_bootmem_huge_page(h))
> >  				break;
> > -		} else if (!alloc_fresh_huge_page(h))
> > +		} else if (!alloc_fresh_huge_page(h, NULL))
> >  			break;
> >  	}
> >  	h->max_huge_pages = i;
> > @@ -1160,7 +1170,8 @@ static inline void try_to_free_low(struc
> >   * balanced by operating on them in a round-robin fashion.
> >   * Returns 1 if an adjustment was made.
> >   */
> > -static int adjust_pool_surplus(struct hstate *h, int delta)
> > +static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,
> > +				int delta)
> >  {
> >  	int start_nid, next_nid;
> >  	int ret = 0;
> > @@ -1176,7 +1187,7 @@ static int adjust_pool_surplus(struct hs
> >  	do {
> >  		int nid = next_nid;
> >  		if (delta < 0)  {
> > -			next_nid = hstate_next_node_to_alloc(h);
> > +			next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
> >  			/*
> >  			 * To shrink on this node, there must be a surplus page
> >  			 */
> > @@ -1184,7 +1195,7 @@ static int adjust_pool_surplus(struct hs
> >  				continue;
> >  		}
> >  		if (delta > 0) {
> > -			next_nid = hstate_next_node_to_free(h);
> > +			next_nid = hstate_next_node_to_free(h, nodes_allowed);
> >  			/*
> >  			 * Surplus cannot exceed the total number of pages
> >  			 */
> > @@ -1223,7 +1234,7 @@ static unsigned long set_max_huge_pages(
> >  	 */
> >  	spin_lock(&hugetlb_lock);
> >  	while (h->surplus_huge_pages && count > persistent_huge_pages(h)) {
> > -		if (!adjust_pool_surplus(h, -1))
> > +		if (!adjust_pool_surplus(h, NULL, -1))
> >  			break;
> >  	}
> >  
> > @@ -1234,7 +1245,7 @@ static unsigned long set_max_huge_pages(
> >  		 * and reducing the surplus.
> >  		 */
> >  		spin_unlock(&hugetlb_lock);
> > -		ret = alloc_fresh_huge_page(h);
> > +		ret = alloc_fresh_huge_page(h, NULL);
> >  		spin_lock(&hugetlb_lock);
> >  		if (!ret)
> >  			goto out;
> > @@ -1260,11 +1271,11 @@ static unsigned long set_max_huge_pages(
> >  	min_count = max(count, min_count);
> >  	try_to_free_low(h, min_count);
> >  	while (min_count < persistent_huge_pages(h)) {
> > -		if (!free_pool_huge_page(h, 0))
> > +		if (!free_pool_huge_page(h, NULL, 0))
> >  			break;
> >  	}
> >  	while (count < persistent_huge_pages(h)) {
> > -		if (!adjust_pool_surplus(h, 1))
> > +		if (!adjust_pool_surplus(h, NULL, 1))
> >  			break;
> >  	}
> >  out:
> > 
> 
> Otherwise, this looks like a relatively straight-forward conversion to
> having nodemask awareness. I'd expect no behavioural changes from this
> patch at all so

Yeah, that was the idea.  This one should preserve the existing
behavior, but prep for the next one.

> 
> Reviewed-by: Mel Gorman <mel@csn.ul.ie>
> 

--
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: [RFC 1/3] hugetlb: add nodemask arg to huge page alloc, free and surplus adjust fcns

[Andrew Morton]

On Wed, 01 Jul 2009 09:00:38 -0400
Lee Schermerhorn <Lee.Schermerhorn@hp.com> wrote:

> > I strongly suspect that the same node being used until allocation
> > failure instead of round-robin is an oversight and not deliberate at
> > all. I can't think of a good reason for boot-allocation to behave
> > significantly different to runtime-allocation.
> > 
> > This could be a seperate patch just for consideration in isolation but I
> > for one have no problem with it. If you split out this patch, feel free
> > to add an Ack from me.
> 
> OK.  If this series doesn't fly--e.g., we take another approach--I will
> definitely split this out.  If Andrew wants it separate for the change
> log, I can do that as well.

Can't be bothered, really.  If you think the fix might be useful to
someone running a 2.6.31 or earlier kernel then yes, it would be polite
to split out a minimal backportable fix?

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

[RFC 2/3] hugetlb: derive huge pages nodes allowed from task mempolicy

[Lee Schermerhorn]

[RFC 2/3] hugetlb:  derive huge pages nodes allowed from task mempolicy

Against: 25jun09 mmotm atop the "hugetlb: balance freeing..." series

This patch derives a "nodes_allowed" node mask from the numa
mempolicy of the task modifying the number of persistent huge
pages to control the allocation, freeing and adjusting of surplus
huge pages.  This mask is derived as follows:

* For "default" [NULL] task mempolicy, a NULL nodemask_t pointer
  is produced.  This will cause the hugetlb subsystem to use
  node_online_map as the "nodes_allowed".  This preserves the
  behavior before this patch.
* For "preferred" mempolicy, including explicit local allocation,
  a nodemask with the single preferred node will be produced. 
  "local" policy will NOT track any internode migrations of the
  task adjusting nr_hugepages.
* For "bind" and "interleave" policy, the mempolicy's nodemask
  will be used.
* Other than to inform the construction of the nodes_allowed node
  mask, the actual mempolicy mode is ignored.  That is, all modes
  behave like interleave over the resulting nodes_allowed mask
  with no "fallback".

Because we may have allocated or freed a huge page with a 
different policy/nodes_allowed previously, we always need to
check that the next_node_to_{alloc|free} exists in the current
nodes_allowed mask.  To avoid duplication of code, this is done
in the hstate_next_node_to_{alloc|free}() functions.  So,
these functions have been modified to allow them to be called
to obtain the "start_nid".  Then, whereas prior to this patch
we unconditionally called hstate_next_node_to_{alloc|free}(),
whether or not we successfully allocated/freed a huge page on
the node, now we only call these functions on failure to alloc/free.

Notes:

1) This patch introduces a subtle change in behavior:  huge page
   allocation and freeing will be constrained by any mempolicy
   that the task adjusting the huge page pool inherits from its
   parent.  This policy could come from a distant ancestor.  The
   adminstrator adjusting the huge page pool without explicitly
   specifying a mempolicy via numactl might be surprised by this.
   Additionaly, any mempolicy specified by numactl will be
   constrained by the cpuset in which numactl is invoked.

2) Hugepages allocated at boot time use the node_online_map.
   An additional patch could implement a temporary boot time
   huge pages nodes_allowed command line parameter.

See the updated documentation [next patch] for more information
about the implications of this patch.

Signed-off-by: Lee Schermerhorn <lee.schermerhorn@hp.com>

 include/linux/mempolicy.h |    3 +
 mm/hugetlb.c              |   99 +++++++++++++++++++++++++++++++---------------
 mm/mempolicy.c            |   54 +++++++++++++++++++++++++
 3 files changed, 124 insertions(+), 32 deletions(-)

Index: linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c
===================================================================
--- linux-2.6.31-rc1-mmotm-090625-1549.orig/mm/hugetlb.c	2009-06-29 23:01:01.000000000 -0400
+++ linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c	2009-06-30 11:29:49.000000000 -0400
@@ -621,29 +621,52 @@ static struct page *alloc_fresh_huge_pag
 }
 
 /*
+ * common helper functions for hstate_next_node_to_{alloc|free}.
+ * We may have allocated or freed a huge pages based on a different
+ * nodes_allowed, previously, so h->next_node_to_{alloc|free} might
+ * be outside of *nodes_allowed.  Ensure that we use the next
+ * allowed node for alloc or free.
+ */
+static int next_node_allowed(int nid, nodemask_t *nodes_allowed)
+{
+	nid = next_node(nid, *nodes_allowed);
+	if (nid == MAX_NUMNODES)
+		nid = first_node(*nodes_allowed); /* handle "wrap" */
+	return nid;
+}
+
+static int this_node_allowed(int nid, nodemask_t *nodes_allowed)
+{
+	if (!node_isset(nid, *nodes_allowed))
+		nid = next_node_allowed(nid, nodes_allowed);
+	return nid;
+}
+
+/*
  * Use a helper variable to find the next node and then
  * copy it back to next_nid_to_alloc afterwards:
  * otherwise there's a window in which a racer might
  * pass invalid nid MAX_NUMNODES to alloc_pages_exact_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.
+ * same nid as we do.  Move nid forward in the mask whether
+ * or not we just successfully allocated a hugepage so that
+ * the next allocation addresses the next node.
  */
 static int hstate_next_node_to_alloc(struct hstate *h,
 					nodemask_t *nodes_allowed)
 {
-	int next_nid;
+	int nid, next_nid;
 
 	if (!nodes_allowed)
 		nodes_allowed = &node_online_map;
 
-	next_nid = next_node(h->next_nid_to_alloc, *nodes_allowed);
-	if (next_nid == MAX_NUMNODES)
-		next_nid = first_node(*nodes_allowed);
+	nid = this_node_allowed(h->next_nid_to_alloc, nodes_allowed);
+
+	next_nid = next_node_allowed(nid, nodes_allowed);
 	h->next_nid_to_alloc = next_nid;
-	return next_nid;
+
+	return nid;
 }
 
 static int alloc_fresh_huge_page(struct hstate *h, nodemask_t *nodes_allowed)
@@ -653,15 +676,17 @@ static int alloc_fresh_huge_page(struct 
 	int next_nid;
 	int ret = 0;
 
-	start_nid = h->next_nid_to_alloc;
+	start_nid = hstate_next_node_to_alloc(h, nodes_allowed);
 	next_nid = start_nid;
 
 	do {
 		page = alloc_fresh_huge_page_node(h, next_nid);
-		if (page)
+		if (page) {
 			ret = 1;
+			break;
+		}
 		next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
-	} while (!page && next_nid != start_nid);
+	} while (next_nid != start_nid);
 
 	if (ret)
 		count_vm_event(HTLB_BUDDY_PGALLOC);
@@ -672,21 +697,23 @@ static int alloc_fresh_huge_page(struct 
 }
 
 /*
- * helper for free_pool_huge_page() - find next node
- * from which to free a huge page
+ * helper for free_pool_huge_page() - return the next node
+ * from which to free a huge page.  Advance the next node id
+ * whether or not we find a free huge page to free so that the
+ * next attempt to free addresses the next node.
  */
 static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
 {
-	int next_nid;
+	int nid, next_nid;
 
 	if (!nodes_allowed)
 		nodes_allowed = &node_online_map;
 
-	next_nid = next_node(h->next_nid_to_free, *nodes_allowed);
-	if (next_nid == MAX_NUMNODES)
-		next_nid = first_node(*nodes_allowed);
+	nid = this_node_allowed(h->next_nid_to_free, nodes_allowed);
+	next_nid = next_node_allowed(nid, nodes_allowed);
 	h->next_nid_to_free = next_nid;
-	return next_nid;
+
+	return nid;
 }
 
 /*
@@ -702,7 +729,7 @@ static int free_pool_huge_page(struct hs
 	int next_nid;
 	int ret = 0;
 
-	start_nid = h->next_nid_to_free;
+	start_nid = hstate_next_node_to_free(h, nodes_allowed);
 	next_nid = start_nid;
 
 	do {
@@ -726,9 +753,10 @@ static int free_pool_huge_page(struct hs
 			}
 			update_and_free_page(h, page);
 			ret = 1;
+			break;
 		}
  		next_nid = hstate_next_node_to_free(h, nodes_allowed);
-	} while (!ret && next_nid != start_nid);
+	} while (next_nid != start_nid);
 
 	return ret;
 }
@@ -1039,10 +1067,9 @@ int __weak alloc_bootmem_huge_page(struc
 		void *addr;
 
 		addr = __alloc_bootmem_node_nopanic(
-				NODE_DATA(h->next_nid_to_alloc),
+				NODE_DATA(hstate_next_node_to_alloc(h, NULL)),
 				huge_page_size(h), huge_page_size(h), 0);
 
-		hstate_next_node_to_alloc(h, NULL); /* always advance nid */
 		if (addr) {
 			/*
 			 * Use the beginning of the huge page to store the
@@ -1179,29 +1206,33 @@ static int adjust_pool_surplus(struct hs
 	VM_BUG_ON(delta != -1 && delta != 1);
 
 	if (delta < 0)
-		start_nid = h->next_nid_to_alloc;
+		start_nid = hstate_next_node_to_alloc(h, nodes_allowed);
 	else
-		start_nid = h->next_nid_to_free;
+		start_nid = hstate_next_node_to_free(h, nodes_allowed);
 	next_nid = start_nid;
 
 	do {
 		int nid = next_nid;
 		if (delta < 0)  {
-			next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
 			/*
 			 * To shrink on this node, there must be a surplus page
 			 */
-			if (!h->surplus_huge_pages_node[nid])
+			if (!h->surplus_huge_pages_node[nid]) {
+				next_nid = hstate_next_node_to_alloc(h,
+								nodes_allowed);
 				continue;
+			}
 		}
 		if (delta > 0) {
-			next_nid = hstate_next_node_to_free(h, nodes_allowed);
 			/*
 			 * Surplus cannot exceed the total number of pages
 			 */
 			if (h->surplus_huge_pages_node[nid] >=
-						h->nr_huge_pages_node[nid])
+						h->nr_huge_pages_node[nid]) {
+				next_nid = hstate_next_node_to_free(h,
+								nodes_allowed);
 				continue;
+			}
 		}
 
 		h->surplus_huge_pages += delta;
@@ -1217,10 +1248,13 @@ static int adjust_pool_surplus(struct hs
 static unsigned long set_max_huge_pages(struct hstate *h, unsigned long count)
 {
 	unsigned long min_count, ret;
+	nodemask_t *nodes_allowed;
 
 	if (h->order >= MAX_ORDER)
 		return h->max_huge_pages;
 
+	nodes_allowed = huge_mpol_nodes_allowed();
+
 	/*
 	 * Increase the pool size
 	 * First take pages out of surplus state.  Then make up the
@@ -1234,7 +1268,7 @@ static unsigned long set_max_huge_pages(
 	 */
 	spin_lock(&hugetlb_lock);
 	while (h->surplus_huge_pages && count > persistent_huge_pages(h)) {
-		if (!adjust_pool_surplus(h, NULL, -1))
+		if (!adjust_pool_surplus(h, nodes_allowed, -1))
 			break;
 	}
 
@@ -1245,7 +1279,7 @@ static unsigned long set_max_huge_pages(
 		 * and reducing the surplus.
 		 */
 		spin_unlock(&hugetlb_lock);
-		ret = alloc_fresh_huge_page(h, NULL);
+		ret = alloc_fresh_huge_page(h, nodes_allowed);
 		spin_lock(&hugetlb_lock);
 		if (!ret)
 			goto out;
@@ -1271,16 +1305,17 @@ static unsigned long set_max_huge_pages(
 	min_count = max(count, min_count);
 	try_to_free_low(h, min_count);
 	while (min_count < persistent_huge_pages(h)) {
-		if (!free_pool_huge_page(h, NULL, 0))
+		if (!free_pool_huge_page(h, nodes_allowed, 0))
 			break;
 	}
 	while (count < persistent_huge_pages(h)) {
-		if (!adjust_pool_surplus(h, NULL, 1))
+		if (!adjust_pool_surplus(h, nodes_allowed, 1))
 			break;
 	}
 out:
 	ret = persistent_huge_pages(h);
 	spin_unlock(&hugetlb_lock);
+	kfree(nodes_allowed);
 	return ret;
 }
 
Index: linux-2.6.31-rc1-mmotm-090625-1549/mm/mempolicy.c
===================================================================
--- linux-2.6.31-rc1-mmotm-090625-1549.orig/mm/mempolicy.c	2009-06-29 12:18:11.000000000 -0400
+++ linux-2.6.31-rc1-mmotm-090625-1549/mm/mempolicy.c	2009-06-29 23:11:35.000000000 -0400
@@ -1544,6 +1544,60 @@ struct zonelist *huge_zonelist(struct vm
 	}
 	return zl;
 }
+
+/**
+ * huge_mpol_nodes_allowed()
+ *
+ * Return a [pointer to a] nodelist for persistent huge page allocation
+ * based on the current task's mempolicy:
+ *
+ * If the task's mempolicy is "default" [NULL], just return NULL for
+ * default behavior.  Otherwise, extract the policy nodemask for 'bind'
+ * or 'interleave' policy or construct a nodemask for 'preferred' or
+ * 'local' policy and return a pointer to a kmalloc()ed nodemask_t.
+ * It is the caller's responsibility to free this nodemask.
+ */
+nodemask_t *huge_mpol_nodes_allowed(void)
+{
+	nodemask_t *nodes_allowed = NULL;
+	struct mempolicy *mempolicy;
+	int nid;
+
+	if (!current || !current->mempolicy)
+		return NULL;
+
+	mpol_get(current->mempolicy);
+	nodes_allowed = kzalloc(sizeof(*nodes_allowed), GFP_KERNEL);
+	if (!nodes_allowed) {
+		printk(KERN_WARNING "Unable to allocate nodes allowed mask "
+			"for huge page allocation\nFalling back to default\n");
+		goto out;
+	}
+
+	mempolicy = current->mempolicy;
+	switch(mempolicy->mode) {
+	case MPOL_PREFERRED:
+		if (mempolicy->flags & MPOL_F_LOCAL)
+			nid = numa_node_id();
+		else
+			nid = mempolicy->v.preferred_node;
+		node_set(nid, *nodes_allowed);
+		break;
+
+	case MPOL_BIND:
+		/* Fall through */
+	case MPOL_INTERLEAVE:
+			*nodes_allowed =  mempolicy->v.nodes;
+		break;
+
+	default:
+		BUG();
+	}
+
+out:
+	mpol_put(current->mempolicy);
+	return nodes_allowed;
+}
 #endif
 
 /* Allocate a page in interleaved policy.
Index: linux-2.6.31-rc1-mmotm-090625-1549/include/linux/mempolicy.h
===================================================================
--- linux-2.6.31-rc1-mmotm-090625-1549.orig/include/linux/mempolicy.h	2009-06-29 12:18:11.000000000 -0400
+++ linux-2.6.31-rc1-mmotm-090625-1549/include/linux/mempolicy.h	2009-06-29 23:06:34.000000000 -0400
@@ -201,6 +201,7 @@ extern void mpol_fix_fork_child_flag(str
 extern struct zonelist *huge_zonelist(struct vm_area_struct *vma,
 				unsigned long addr, gfp_t gfp_flags,
 				struct mempolicy **mpol, nodemask_t **nodemask);
+extern nodemask_t *huge_mpol_nodes_allowed(void);
 extern unsigned slab_node(struct mempolicy *policy);
 
 extern enum zone_type policy_zone;
@@ -328,6 +329,8 @@ static inline struct zonelist *huge_zone
 	return node_zonelist(0, gfp_flags);
 }
 
+static inline nodemask_t *huge_mpol_nodes_allowed(void) { return NULL; }
+
 static inline int do_migrate_pages(struct mm_struct *mm,
 			const nodemask_t *from_nodes,
 			const nodemask_t *to_nodes, int flags)

--
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: [RFC 2/3] hugetlb: derive huge pages nodes allowed from task mempolicy

[Mel Gorman]

On Tue, Jun 30, 2009 at 11:48:18AM -0400, Lee Schermerhorn wrote:
> [RFC 2/3] hugetlb:  derive huge pages nodes allowed from task mempolicy
> 
> Against: 25jun09 mmotm atop the "hugetlb: balance freeing..." series
> 
> This patch derives a "nodes_allowed" node mask from the numa
> mempolicy of the task modifying the number of persistent huge
> pages to control the allocation, freeing and adjusting of surplus
> huge pages.  This mask is derived as follows:
> 
> * For "default" [NULL] task mempolicy, a NULL nodemask_t pointer
>   is produced.  This will cause the hugetlb subsystem to use
>   node_online_map as the "nodes_allowed".  This preserves the
>   behavior before this patch.

Sensible.

> * For "preferred" mempolicy, including explicit local allocation,
>   a nodemask with the single preferred node will be produced. 
>   "local" policy will NOT track any internode migrations of the
>   task adjusting nr_hugepages.

This excludes fallback and could do with an in-code comment. I whinge
about this more later.

> * For "bind" and "interleave" policy, the mempolicy's nodemask
>   will be used.

Sensible.

> * Other than to inform the construction of the nodes_allowed node
>   mask, the actual mempolicy mode is ignored.  That is, all modes
>   behave like interleave over the resulting nodes_allowed mask
>   with no "fallback".
> 

Again, seems sensible. Resizing the hugepage pool is not like a normal
applications behaviour.

> Because we may have allocated or freed a huge page with a 
> different policy/nodes_allowed previously, we always need to
> check that the next_node_to_{alloc|free} exists in the current
> nodes_allowed mask.  To avoid duplication of code, this is done
> in the hstate_next_node_to_{alloc|free}() functions. 

Seems fair. While this means that some nodes could be skipped because there
was a hugepage pool resize with a restricted policy and then no policy, I
see little problem with that as such.  I believe you caught all the direct
users of next_nid_to_[alloc|free] and used the helpers which is good.

> So,
> these functions have been modified to allow them to be called
> to obtain the "start_nid".  Then, whereas prior to this patch
> we unconditionally called hstate_next_node_to_{alloc|free}(),
> whether or not we successfully allocated/freed a huge page on
> the node, now we only call these functions on failure to alloc/free.
> 
> Notes:
> 
> 1) This patch introduces a subtle change in behavior:  huge page
>    allocation and freeing will be constrained by any mempolicy
>    that the task adjusting the huge page pool inherits from its
>    parent.  This policy could come from a distant ancestor.  The
>    adminstrator adjusting the huge page pool without explicitly
>    specifying a mempolicy via numactl might be surprised by this.

I would be trying to encourage administrators to use hugeadm instead of
manually tuning the pools. One possible course of action is for hugeadm
to check if a policy is currently set and output that as an INFO message.
That will show up if they run with hugeadm -v. Alternatively, we could note
as a WARN when any policy is set and print an INFO message on details of
the policy.

>    Additionaly, any mempolicy specified by numactl will be
>    constrained by the cpuset in which numactl is invoked.
> 
> 2) Hugepages allocated at boot time use the node_online_map.
>    An additional patch could implement a temporary boot time
>    huge pages nodes_allowed command line parameter.
> 

I'd want for someone to complain before implementing such a patch. Even
on systems where hugepages must be allocated very early on, an init script
should be more than sufficient without implementing parsing for mempolicies
and hugepages.

> See the updated documentation [next patch] for more information
> about the implications of this patch.
> 

Ideally the change log should show a before and after of using numactl +
hugeadm to resize pools on a subset of available nodes.

> Signed-off-by: Lee Schermerhorn <lee.schermerhorn@hp.com>
> 
>  include/linux/mempolicy.h |    3 +
>  mm/hugetlb.c              |   99 +++++++++++++++++++++++++++++++---------------
>  mm/mempolicy.c            |   54 +++++++++++++++++++++++++
>  3 files changed, 124 insertions(+), 32 deletions(-)
> 
> Index: linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c
> ===================================================================
> --- linux-2.6.31-rc1-mmotm-090625-1549.orig/mm/hugetlb.c	2009-06-29 23:01:01.000000000 -0400
> +++ linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c	2009-06-30 11:29:49.000000000 -0400
> @@ -621,29 +621,52 @@ static struct page *alloc_fresh_huge_pag
>  }
>  
>  /*
> + * common helper functions for hstate_next_node_to_{alloc|free}.
> + * We may have allocated or freed a huge pages based on a different
> + * nodes_allowed, previously, so h->next_node_to_{alloc|free} might
> + * be outside of *nodes_allowed.  Ensure that we use the next
> + * allowed node for alloc or free.
> + */
> +static int next_node_allowed(int nid, nodemask_t *nodes_allowed)
> +{
> +	nid = next_node(nid, *nodes_allowed);
> +	if (nid == MAX_NUMNODES)
> +		nid = first_node(*nodes_allowed); /* handle "wrap" */
> +	return nid;
> +}

The handle warp comment is unnecessary there. This is such a common pattern,
it should be self-evident without placing comments that cause the
CodingStyle Police to issue tickets.

> +
> +static int this_node_allowed(int nid, nodemask_t *nodes_allowed)
> +{
> +	if (!node_isset(nid, *nodes_allowed))
> +		nid = next_node_allowed(nid, nodes_allowed);
> +	return nid;
> +}
> +

What happens if node hot-remove occured and there is now no node that we
are allowed to allocate from?

This thing will end up returning MAX_NUMNODES right? That potentially then
gets passed to alloc_pages_exact_node() triggering a VM_BUG_ON() there.

> +/*
>   * Use a helper variable to find the next node and then
>   * copy it back to next_nid_to_alloc afterwards:
>   * otherwise there's a window in which a racer might
>   * pass invalid nid MAX_NUMNODES to alloc_pages_exact_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.
> + * same nid as we do.  Move nid forward in the mask whether
> + * or not we just successfully allocated a hugepage so that
> + * the next allocation addresses the next node.
>   */
>  static int hstate_next_node_to_alloc(struct hstate *h,
>  					nodemask_t *nodes_allowed)
>  {
> -	int next_nid;
> +	int nid, next_nid;
>  
>  	if (!nodes_allowed)
>  		nodes_allowed = &node_online_map;
>  
> -	next_nid = next_node(h->next_nid_to_alloc, *nodes_allowed);
> -	if (next_nid == MAX_NUMNODES)
> -		next_nid = first_node(*nodes_allowed);
> +	nid = this_node_allowed(h->next_nid_to_alloc, nodes_allowed);
> +
> +	next_nid = next_node_allowed(nid, nodes_allowed);
>  	h->next_nid_to_alloc = next_nid;
> -	return next_nid;
> +
> +	return nid;
>  }
>  

Seems reasonable. I see now what you mean about next_nid_to_alloc being more
like its name in patch series than the last.

>  static int alloc_fresh_huge_page(struct hstate *h, nodemask_t *nodes_allowed)
> @@ -653,15 +676,17 @@ static int alloc_fresh_huge_page(struct 
>  	int next_nid;
>  	int ret = 0;
>  
> -	start_nid = h->next_nid_to_alloc;
> +	start_nid = hstate_next_node_to_alloc(h, nodes_allowed);
>  	next_nid = start_nid;
>  

So, here for example, I think we need to make sure start_nid is not
MAX_NUMNODES and bail out if it is.

>  	do {
>  		page = alloc_fresh_huge_page_node(h, next_nid);
> -		if (page)
> +		if (page) {
>  			ret = 1;
> +			break;
> +		}

Ok, so this break is necessary on allocation success because
hstate_next_node_to_alloc() has already bumped next_nid_to_alloc. Right?

>  		next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
> -	} while (!page && next_nid != start_nid);
> +	} while (next_nid != start_nid);
>  
>  	if (ret)
>  		count_vm_event(HTLB_BUDDY_PGALLOC);
> @@ -672,21 +697,23 @@ static int alloc_fresh_huge_page(struct 
>  }
>  
>  /*
> - * helper for free_pool_huge_page() - find next node
> - * from which to free a huge page
> + * helper for free_pool_huge_page() - return the next node
> + * from which to free a huge page.  Advance the next node id
> + * whether or not we find a free huge page to free so that the
> + * next attempt to free addresses the next node.
>   */
>  static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
>  {
> -	int next_nid;
> +	int nid, next_nid;
>  
>  	if (!nodes_allowed)
>  		nodes_allowed = &node_online_map;
>  
> -	next_nid = next_node(h->next_nid_to_free, *nodes_allowed);
> -	if (next_nid == MAX_NUMNODES)
> -		next_nid = first_node(*nodes_allowed);
> +	nid = this_node_allowed(h->next_nid_to_free, nodes_allowed);
> +	next_nid = next_node_allowed(nid, nodes_allowed);
>  	h->next_nid_to_free = next_nid;
> -	return next_nid;
> +
> +	return nid;
>  }
>  
>  /*
> @@ -702,7 +729,7 @@ static int free_pool_huge_page(struct hs
>  	int next_nid;
>  	int ret = 0;
>  
> -	start_nid = h->next_nid_to_free;
> +	start_nid = hstate_next_node_to_free(h, nodes_allowed);
>  	next_nid = start_nid;
>  

I guess if start_nid is MAX_NUMNODES here as well, we should bail out
early. It means that a pool shrink by the requested number of pages may
fail if there are not enough hugepages allocated on the allowed node but
maybe that's not such a big problem.

hugeadm will actually notice when this situation occurs and gives a
warning about it.

>  	do {
> @@ -726,9 +753,10 @@ static int free_pool_huge_page(struct hs
>  			}
>  			update_and_free_page(h, page);
>  			ret = 1;
> +			break;
>  		}
>   		next_nid = hstate_next_node_to_free(h, nodes_allowed);
> -	} while (!ret && next_nid != start_nid);
> +	} while (next_nid != start_nid);
>  
>  	return ret;
>  }
> @@ -1039,10 +1067,9 @@ int __weak alloc_bootmem_huge_page(struc
>  		void *addr;
>  
>  		addr = __alloc_bootmem_node_nopanic(
> -				NODE_DATA(h->next_nid_to_alloc),
> +				NODE_DATA(hstate_next_node_to_alloc(h, NULL)),
>  				huge_page_size(h), huge_page_size(h), 0);
>  
> -		hstate_next_node_to_alloc(h, NULL); /* always advance nid */
>  		if (addr) {
>  			/*
>  			 * Use the beginning of the huge page to store the
> @@ -1179,29 +1206,33 @@ static int adjust_pool_surplus(struct hs
>  	VM_BUG_ON(delta != -1 && delta != 1);
>  
>  	if (delta < 0)
> -		start_nid = h->next_nid_to_alloc;
> +		start_nid = hstate_next_node_to_alloc(h, nodes_allowed);
>  	else
> -		start_nid = h->next_nid_to_free;
> +		start_nid = hstate_next_node_to_free(h, nodes_allowed);
>  	next_nid = start_nid;
>  
>  	do {
>  		int nid = next_nid;
>  		if (delta < 0)  {
> -			next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
>  			/*
>  			 * To shrink on this node, there must be a surplus page
>  			 */
> -			if (!h->surplus_huge_pages_node[nid])
> +			if (!h->surplus_huge_pages_node[nid]) {
> +				next_nid = hstate_next_node_to_alloc(h,
> +								nodes_allowed);
>  				continue;
> +			}
>  		}
>  		if (delta > 0) {
> -			next_nid = hstate_next_node_to_free(h, nodes_allowed);
>  			/*
>  			 * Surplus cannot exceed the total number of pages
>  			 */
>  			if (h->surplus_huge_pages_node[nid] >=
> -						h->nr_huge_pages_node[nid])
> +						h->nr_huge_pages_node[nid]) {
> +				next_nid = hstate_next_node_to_free(h,
> +								nodes_allowed);
>  				continue;
> +			}
>  		}
>  
>  		h->surplus_huge_pages += delta;
> @@ -1217,10 +1248,13 @@ static int adjust_pool_surplus(struct hs
>  static unsigned long set_max_huge_pages(struct hstate *h, unsigned long count)
>  {
>  	unsigned long min_count, ret;
> +	nodemask_t *nodes_allowed;
>  
>  	if (h->order >= MAX_ORDER)
>  		return h->max_huge_pages;
>  
> +	nodes_allowed = huge_mpol_nodes_allowed();
> +
>  	/*
>  	 * Increase the pool size
>  	 * First take pages out of surplus state.  Then make up the
> @@ -1234,7 +1268,7 @@ static unsigned long set_max_huge_pages(
>  	 */
>  	spin_lock(&hugetlb_lock);
>  	while (h->surplus_huge_pages && count > persistent_huge_pages(h)) {
> -		if (!adjust_pool_surplus(h, NULL, -1))
> +		if (!adjust_pool_surplus(h, nodes_allowed, -1))
>  			break;
>  	}
>  
> @@ -1245,7 +1279,7 @@ static unsigned long set_max_huge_pages(
>  		 * and reducing the surplus.
>  		 */
>  		spin_unlock(&hugetlb_lock);
> -		ret = alloc_fresh_huge_page(h, NULL);
> +		ret = alloc_fresh_huge_page(h, nodes_allowed);
>  		spin_lock(&hugetlb_lock);
>  		if (!ret)
>  			goto out;
> @@ -1271,16 +1305,17 @@ static unsigned long set_max_huge_pages(
>  	min_count = max(count, min_count);
>  	try_to_free_low(h, min_count);
>  	while (min_count < persistent_huge_pages(h)) {
> -		if (!free_pool_huge_page(h, NULL, 0))
> +		if (!free_pool_huge_page(h, nodes_allowed, 0))
>  			break;
>  	}
>  	while (count < persistent_huge_pages(h)) {
> -		if (!adjust_pool_surplus(h, NULL, 1))
> +		if (!adjust_pool_surplus(h, nodes_allowed, 1))
>  			break;
>  	}
>  out:
>  	ret = persistent_huge_pages(h);
>  	spin_unlock(&hugetlb_lock);
> +	kfree(nodes_allowed);
>  	return ret;
>  }
>  
> Index: linux-2.6.31-rc1-mmotm-090625-1549/mm/mempolicy.c
> ===================================================================
> --- linux-2.6.31-rc1-mmotm-090625-1549.orig/mm/mempolicy.c	2009-06-29 12:18:11.000000000 -0400
> +++ linux-2.6.31-rc1-mmotm-090625-1549/mm/mempolicy.c	2009-06-29 23:11:35.000000000 -0400
> @@ -1544,6 +1544,60 @@ struct zonelist *huge_zonelist(struct vm
>  	}
>  	return zl;
>  }
> +
> +/**
> + * huge_mpol_nodes_allowed()
> + *
> + * Return a [pointer to a] nodelist for persistent huge page allocation
> + * based on the current task's mempolicy:
> + *
> + * If the task's mempolicy is "default" [NULL], just return NULL for
> + * default behavior.  Otherwise, extract the policy nodemask for 'bind'
> + * or 'interleave' policy or construct a nodemask for 'preferred' or
> + * 'local' policy and return a pointer to a kmalloc()ed nodemask_t.
> + * It is the caller's responsibility to free this nodemask.
> + */
> +nodemask_t *huge_mpol_nodes_allowed(void)
> +{
> +	nodemask_t *nodes_allowed = NULL;
> +	struct mempolicy *mempolicy;
> +	int nid;
> +
> +	if (!current || !current->mempolicy)
> +		return NULL;
> +

Is it really possible for current to be NULL here?

> +	mpol_get(current->mempolicy);
> +	nodes_allowed = kzalloc(sizeof(*nodes_allowed), GFP_KERNEL);
> +	if (!nodes_allowed) {
> +		printk(KERN_WARNING "Unable to allocate nodes allowed mask "
> +			"for huge page allocation\nFalling back to default\n");
> +		goto out;
> +	}

In terms of memory policies, the huge_mpol_nodes_allowed() would appear
to be unique in allocating a nodemask and expecting the caller to free it
without leaking. Would it be possible to move the kzalloc() to the caller
of huge_mpol_nodes_allowed()? It'd be less surprising to me.

I take it you are not allocating nodemask_t on the stack of the caller because
potentially nodemask_t is very large if MAX_NUMNODES is a big number. Is
that accurate?

nodemasks are meant to be cleared with nodes_clear() but you depend on
kzalloc() zeroing the bitmap for you. While the end result is the same,
is using kzalloc instead of kmalloc+nodes_clear() considered ok?

> +
> +	mempolicy = current->mempolicy;
> +	switch(mempolicy->mode) {
> +	case MPOL_PREFERRED:
> +		if (mempolicy->flags & MPOL_F_LOCAL)
> +			nid = numa_node_id();
> +		else
> +			nid = mempolicy->v.preferred_node;
> +		node_set(nid, *nodes_allowed);
> +		break;
> +

I think a comment is needed here saying that MPOL_PREFERRED when
resizing the pool acts more like MPOL_BIND to the preferred node with no
fallback.

I see your problem though. You can't use set the next_nid to allocate from
to be the preferred node because the second allocation will interleave away
from it though. How messy would it be to check if the MPOL_PREFERRED policy
was in use and avoid updating next_nid_to_alloc while the preferred node is
being allocated from?

It's not a big issue and I'd be ok with your current behaviour to start with.

> +	case MPOL_BIND:
> +		/* Fall through */
> +	case MPOL_INTERLEAVE:
> +			*nodes_allowed =  mempolicy->v.nodes;
> +		break;
> +
> +	default:
> +		BUG();
> +	}
> +
> +out:
> +	mpol_put(current->mempolicy);
> +	return nodes_allowed;
> +}
>  #endif
>  
>  /* Allocate a page in interleaved policy.
> Index: linux-2.6.31-rc1-mmotm-090625-1549/include/linux/mempolicy.h
> ===================================================================
> --- linux-2.6.31-rc1-mmotm-090625-1549.orig/include/linux/mempolicy.h	2009-06-29 12:18:11.000000000 -0400
> +++ linux-2.6.31-rc1-mmotm-090625-1549/include/linux/mempolicy.h	2009-06-29 23:06:34.000000000 -0400
> @@ -201,6 +201,7 @@ extern void mpol_fix_fork_child_flag(str
>  extern struct zonelist *huge_zonelist(struct vm_area_struct *vma,
>  				unsigned long addr, gfp_t gfp_flags,
>  				struct mempolicy **mpol, nodemask_t **nodemask);
> +extern nodemask_t *huge_mpol_nodes_allowed(void);
>  extern unsigned slab_node(struct mempolicy *policy);
>  
>  extern enum zone_type policy_zone;
> @@ -328,6 +329,8 @@ static inline struct zonelist *huge_zone
>  	return node_zonelist(0, gfp_flags);
>  }
>  
> +static inline nodemask_t *huge_mpol_nodes_allowed(void) { return NULL; }
> +
>  static inline int do_migrate_pages(struct mm_struct *mm,
>  			const nodemask_t *from_nodes,
>  			const nodemask_t *to_nodes, int flags)
> 

By and large, this patch would appear to result in reasonable behaviour
for administrators that want to limit the hugepage pool to specific
nodes. Predictably, I prefer this approach to the nodemask-sysctl
approach :/ . With a few crinkles ironed out, I reckon I'd be happy with
this. Certainly, it appears to work as advertised in that I was able to
accurate grow/shrink the pool on specific nodes.

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
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: [RFC 2/3] hugetlb: derive huge pages nodes allowed from task mempolicy

[Lee Schermerhorn]

On Wed, 2009-07-01 at 15:32 +0100, Mel Gorman wrote:
> On Tue, Jun 30, 2009 at 11:48:18AM -0400, Lee Schermerhorn wrote:
> > [RFC 2/3] hugetlb:  derive huge pages nodes allowed from task mempolicy
> > 
> > Against: 25jun09 mmotm atop the "hugetlb: balance freeing..." series
> > 
> > This patch derives a "nodes_allowed" node mask from the numa
> > mempolicy of the task modifying the number of persistent huge
> > pages to control the allocation, freeing and adjusting of surplus
> > huge pages.  This mask is derived as follows:
> > 
> > * For "default" [NULL] task mempolicy, a NULL nodemask_t pointer
> >   is produced.  This will cause the hugetlb subsystem to use
> >   node_online_map as the "nodes_allowed".  This preserves the
> >   behavior before this patch.
> 
> Sensible.
> 
> > * For "preferred" mempolicy, including explicit local allocation,
> >   a nodemask with the single preferred node will be produced. 
> >   "local" policy will NOT track any internode migrations of the
> >   task adjusting nr_hugepages.
> 
> This excludes fallback and could do with an in-code comment. I whinge
> about this more later.
> 
> > * For "bind" and "interleave" policy, the mempolicy's nodemask
> >   will be used.
> 
> Sensible.
> 
> > * Other than to inform the construction of the nodes_allowed node
> >   mask, the actual mempolicy mode is ignored.  That is, all modes
> >   behave like interleave over the resulting nodes_allowed mask
> >   with no "fallback".
> > 
> 
> Again, seems sensible. Resizing the hugepage pool is not like a normal
> applications behaviour.
> 
> > Because we may have allocated or freed a huge page with a 
> > different policy/nodes_allowed previously, we always need to
> > check that the next_node_to_{alloc|free} exists in the current
> > nodes_allowed mask.  To avoid duplication of code, this is done
> > in the hstate_next_node_to_{alloc|free}() functions. 
> 
> Seems fair. While this means that some nodes could be skipped because there
> was a hugepage pool resize with a restricted policy and then no policy, I
> see little problem with that as such.  I believe you caught all the direct
> users of next_nid_to_[alloc|free] and used the helpers which is good.

Uh, I've been meaning to point out that I haven't modified
"try_to_free_low()" to use free_pool_huge_page().  The non-stub version
of try_to_free_low() is only compiled in when '_HIGHMEM is defined--i.e,
32-bit systems.  So, it will still drain nodes completely in node id
order.  If this is a problem and we want try_to_free_low() to balance
freeing, I can enhance free_pool_huge_page() to take another flag
[perhaps combining it with the "acct_surplus" in a single flags arg] to
free only !highmem pages and pass a node mask of any nodes containing
such pages. 

> 
> > So,
> > these functions have been modified to allow them to be called
> > to obtain the "start_nid".  Then, whereas prior to this patch
> > we unconditionally called hstate_next_node_to_{alloc|free}(),
> > whether or not we successfully allocated/freed a huge page on
> > the node, now we only call these functions on failure to alloc/free.
> > 
> > Notes:
> > 
> > 1) This patch introduces a subtle change in behavior:  huge page
> >    allocation and freeing will be constrained by any mempolicy
> >    that the task adjusting the huge page pool inherits from its
> >    parent.  This policy could come from a distant ancestor.  The
> >    adminstrator adjusting the huge page pool without explicitly
> >    specifying a mempolicy via numactl might be surprised by this.
> 
> I would be trying to encourage administrators to use hugeadm instead of
> manually tuning the pools. One possible course of action is for hugeadm
> to check if a policy is currently set and output that as an INFO message.
> That will show up if they run with hugeadm -v. Alternatively, we could note
> as a WARN when any policy is set and print an INFO message on details of
> the policy.

Yes.  I saw mention of direct access to the sysctls being deprecated.
I'm not sure how that will go over with users, but if go with changes
like this, it makes sense to handle them in hugeadm.

> 
> >    Additionaly, any mempolicy specified by numactl will be
> >    constrained by the cpuset in which numactl is invoked.
> > 
> > 2) Hugepages allocated at boot time use the node_online_map.
> >    An additional patch could implement a temporary boot time
> >    huge pages nodes_allowed command line parameter.
> > 
> 
> I'd want for someone to complain before implementing such a patch. Even
> on systems where hugepages must be allocated very early on, an init script
> should be more than sufficient without implementing parsing for mempolicies
> and hugepages.

I agree in principle.  I expect that if one can't allocate the required
number of huge pages in an early init script, either one really needs
more memory, or something is wrong with earlier scripts that is
fragmenting memory.  However, I'll point out that by the time one gets
such a complaint, there is a really long lead time before a change gets
into customer hands.  This is the downside of waiting for a complaint or
definitive use case before providing tunable parameters and such :(.

> 
> > See the updated documentation [next patch] for more information
> > about the implications of this patch.
> > 
> 
> Ideally the change log should show a before and after of using numactl +
> hugeadm to resize pools on a subset of available nodes.

I'll do that. 

> 
> > Signed-off-by: Lee Schermerhorn <lee.schermerhorn@hp.com>
> > 
> >  include/linux/mempolicy.h |    3 +
> >  mm/hugetlb.c              |   99 +++++++++++++++++++++++++++++++---------------
> >  mm/mempolicy.c            |   54 +++++++++++++++++++++++++
> >  3 files changed, 124 insertions(+), 32 deletions(-)
> > 
> > Index: linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c
> > ===================================================================
> > --- linux-2.6.31-rc1-mmotm-090625-1549.orig/mm/hugetlb.c	2009-06-29 23:01:01.000000000 -0400
> > +++ linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c	2009-06-30 11:29:49.000000000 -0400
> > @@ -621,29 +621,52 @@ static struct page *alloc_fresh_huge_pag
> >  }
> >  
> >  /*
> > + * common helper functions for hstate_next_node_to_{alloc|free}.
> > + * We may have allocated or freed a huge pages based on a different
> > + * nodes_allowed, previously, so h->next_node_to_{alloc|free} might
> > + * be outside of *nodes_allowed.  Ensure that we use the next
> > + * allowed node for alloc or free.
> > + */
> > +static int next_node_allowed(int nid, nodemask_t *nodes_allowed)
> > +{
> > +	nid = next_node(nid, *nodes_allowed);
> > +	if (nid == MAX_NUMNODES)
> > +		nid = first_node(*nodes_allowed); /* handle "wrap" */
> > +	return nid;
> > +}
> 
> The handle warp comment is unnecessary there. This is such a common pattern,
> it should be self-evident without placing comments that cause the
> CodingStyle Police to issue tickets.

Thanks for the warning.  I'll remove it.

> 
> > +
> > +static int this_node_allowed(int nid, nodemask_t *nodes_allowed)
> > +{
> > +	if (!node_isset(nid, *nodes_allowed))
> > +		nid = next_node_allowed(nid, nodes_allowed);
> > +	return nid;
> > +}
> > +
> 
> What happens if node hot-remove occured and there is now no node that we
> are allowed to allocate from?

If we return a valid node id [see below] of an off-line node, the
allocation or free will fail and we'll advance to the next node.  If all
the nodes in the nodes_allowed are off-line, we'll end up with next_nid
== start_nid and bail out.

> 
> This thing will end up returning MAX_NUMNODES right? That potentially then
> gets passed to alloc_pages_exact_node() triggering a VM_BUG_ON() there.

No.  That's "next_node_allowed()"--the function above with the spurious
"handle wrap" comment--not the bare "next_node()".  So, we will "handle
wrap" appropriately.  

> 
> > +/*
> >   * Use a helper variable to find the next node and then
> >   * copy it back to next_nid_to_alloc afterwards:
> >   * otherwise there's a window in which a racer might
> >   * pass invalid nid MAX_NUMNODES to alloc_pages_exact_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.
> > + * same nid as we do.  Move nid forward in the mask whether
> > + * or not we just successfully allocated a hugepage so that
> > + * the next allocation addresses the next node.
> >   */
> >  static int hstate_next_node_to_alloc(struct hstate *h,
> >  					nodemask_t *nodes_allowed)
> >  {
> > -	int next_nid;
> > +	int nid, next_nid;
> >  
> >  	if (!nodes_allowed)
> >  		nodes_allowed = &node_online_map;
> >  
> > -	next_nid = next_node(h->next_nid_to_alloc, *nodes_allowed);
> > -	if (next_nid == MAX_NUMNODES)
> > -		next_nid = first_node(*nodes_allowed);
> > +	nid = this_node_allowed(h->next_nid_to_alloc, nodes_allowed);
> > +
> > +	next_nid = next_node_allowed(nid, nodes_allowed);
> >  	h->next_nid_to_alloc = next_nid;
> > -	return next_nid;
> > +
> > +	return nid;
> >  }
> >  
> 
> Seems reasonable. I see now what you mean about next_nid_to_alloc being more
> like its name in patch series than the last.
> 
> >  static int alloc_fresh_huge_page(struct hstate *h, nodemask_t *nodes_allowed)
> > @@ -653,15 +676,17 @@ static int alloc_fresh_huge_page(struct 
> >  	int next_nid;
> >  	int ret = 0;
> >  
> > -	start_nid = h->next_nid_to_alloc;
> > +	start_nid = hstate_next_node_to_alloc(h, nodes_allowed);
> >  	next_nid = start_nid;
> >  
> 
> So, here for example, I think we need to make sure start_nid is not
> MAX_NUMNODES and bail out if it is.

See response above, re: this_node_allowed().  Do you still think it's a
problem?

> 
> >  	do {
> >  		page = alloc_fresh_huge_page_node(h, next_nid);
> > -		if (page)
> > +		if (page) {
> >  			ret = 1;
> > +			break;
> > +		}
> 
> Ok, so this break is necessary on allocation success because
> hstate_next_node_to_alloc() has already bumped next_nid_to_alloc. Right?

Right.  I don't want to fall thru and skip a node.  so, I break and
remove the !page from the loop condition.

However, since you mention it:  I noticed, since I sent this, that with
this arrangement we will skip a node id in the case that we visit all
nodes w/o successfully allocating a huge page or finding a huge page to
free.  When we hit the loop termination condition "next_nid ==
start_nid", we've already advanced hstate_next_node_to_{alloc|free}
beyond next_nid.   So, on next call, start_nid will become that value
and not the same start_nid we used before.

Example:  suppose we try to start a huge page [on, say, a 4 node] system
with no huge pages available anywhere; and the next_node_to_alloc == 0
when we first call alloc_fresh_huge_page().  start_nid will get '0' and
we'll examine nodes 0, 1, 2 and 3.  When hstate_next_node_to_alloc()
returns '0', we'll give up and return NULL page, but next_node_to_alloc
is now 1.  If we try again, maybe after we think some memory has been
freed up and we have a chance of succeeding, we'll visit 1, 2, 3 and 0
[if there are still no huge pages available].  And so forth.  I don't
think this is a problem because if we actually succeed in allocating or
freeing a page, we'll break w/o advancing next_node... and pick up there
on next call.

I have an idea for fixing this behavior, if anyone thinks it's a
problem, by "pushing back" the next_nid to the hstate with something
like:

	} while (next_nid != start_nid || push_back_alloc_node(next_nid))

where the push_back function always returns 0.  Kind of ugly, but I
think it would work.  I don't think it's needed, tho'.

> 
> >  		next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
> > -	} while (!page && next_nid != start_nid);
> > +	} while (next_nid != start_nid);
> >  
> >  	if (ret)
> >  		count_vm_event(HTLB_BUDDY_PGALLOC);
> > @@ -672,21 +697,23 @@ static int alloc_fresh_huge_page(struct 
> >  }
> >  
> >  /*
> > - * helper for free_pool_huge_page() - find next node
> > - * from which to free a huge page
> > + * helper for free_pool_huge_page() - return the next node
> > + * from which to free a huge page.  Advance the next node id
> > + * whether or not we find a free huge page to free so that the
> > + * next attempt to free addresses the next node.
> >   */
> >  static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
> >  {
> > -	int next_nid;
> > +	int nid, next_nid;
> >  
> >  	if (!nodes_allowed)
> >  		nodes_allowed = &node_online_map;
> >  
> > -	next_nid = next_node(h->next_nid_to_free, *nodes_allowed);
> > -	if (next_nid == MAX_NUMNODES)
> > -		next_nid = first_node(*nodes_allowed);
> > +	nid = this_node_allowed(h->next_nid_to_free, nodes_allowed);
> > +	next_nid = next_node_allowed(nid, nodes_allowed);
> >  	h->next_nid_to_free = next_nid;
> > -	return next_nid;
> > +
> > +	return nid;
> >  }
> >  
> >  /*
> > @@ -702,7 +729,7 @@ static int free_pool_huge_page(struct hs
> >  	int next_nid;
> >  	int ret = 0;
> >  
> > -	start_nid = h->next_nid_to_free;
> > +	start_nid = hstate_next_node_to_free(h, nodes_allowed);
> >  	next_nid = start_nid;
> >  
> 
> I guess if start_nid is MAX_NUMNODES here as well, we should bail out
> early. It means that a pool shrink by the requested number of pages may
> fail if there are not enough hugepages allocated on the allowed node but
> maybe that's not such a big problem.

Again, I don't that can happen.  ???

> 
> hugeadm will actually notice when this situation occurs and gives a
> warning about it.
> 
> >  	do {
> > @@ -726,9 +753,10 @@ static int free_pool_huge_page(struct hs
> >  			}
> >  			update_and_free_page(h, page);
> >  			ret = 1;
> > +			break;
> >  		}
> >   		next_nid = hstate_next_node_to_free(h, nodes_allowed);
> > -	} while (!ret && next_nid != start_nid);
> > +	} while (next_nid != start_nid);
> >  
> >  	return ret;
> >  }
> > @@ -1039,10 +1067,9 @@ int __weak alloc_bootmem_huge_page(struc
> >  		void *addr;
> >  
> >  		addr = __alloc_bootmem_node_nopanic(
> > -				NODE_DATA(h->next_nid_to_alloc),
> > +				NODE_DATA(hstate_next_node_to_alloc(h, NULL)),
> >  				huge_page_size(h), huge_page_size(h), 0);
> >  
> > -		hstate_next_node_to_alloc(h, NULL); /* always advance nid */
> >  		if (addr) {
> >  			/*
> >  			 * Use the beginning of the huge page to store the
> > @@ -1179,29 +1206,33 @@ static int adjust_pool_surplus(struct hs
> >  	VM_BUG_ON(delta != -1 && delta != 1);
> >  
> >  	if (delta < 0)
> > -		start_nid = h->next_nid_to_alloc;
> > +		start_nid = hstate_next_node_to_alloc(h, nodes_allowed);
> >  	else
> > -		start_nid = h->next_nid_to_free;
> > +		start_nid = hstate_next_node_to_free(h, nodes_allowed);
> >  	next_nid = start_nid;
> >  
> >  	do {
> >  		int nid = next_nid;
> >  		if (delta < 0)  {
> > -			next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
> >  			/*
> >  			 * To shrink on this node, there must be a surplus page
> >  			 */
> > -			if (!h->surplus_huge_pages_node[nid])
> > +			if (!h->surplus_huge_pages_node[nid]) {
> > +				next_nid = hstate_next_node_to_alloc(h,
> > +								nodes_allowed);
> >  				continue;
> > +			}
> >  		}
> >  		if (delta > 0) {
> > -			next_nid = hstate_next_node_to_free(h, nodes_allowed);
> >  			/*
> >  			 * Surplus cannot exceed the total number of pages
> >  			 */
> >  			if (h->surplus_huge_pages_node[nid] >=
> > -						h->nr_huge_pages_node[nid])
> > +						h->nr_huge_pages_node[nid]) {
> > +				next_nid = hstate_next_node_to_free(h,
> > +								nodes_allowed);
> >  				continue;
> > +			}
> >  		}
> >  
> >  		h->surplus_huge_pages += delta;
> > @@ -1217,10 +1248,13 @@ static int adjust_pool_surplus(struct hs
> >  static unsigned long set_max_huge_pages(struct hstate *h, unsigned long count)
> >  {
> >  	unsigned long min_count, ret;
> > +	nodemask_t *nodes_allowed;
> >  
> >  	if (h->order >= MAX_ORDER)
> >  		return h->max_huge_pages;
> >  
> > +	nodes_allowed = huge_mpol_nodes_allowed();
> > +
> >  	/*
> >  	 * Increase the pool size
> >  	 * First take pages out of surplus state.  Then make up the
> > @@ -1234,7 +1268,7 @@ static unsigned long set_max_huge_pages(
> >  	 */
> >  	spin_lock(&hugetlb_lock);
> >  	while (h->surplus_huge_pages && count > persistent_huge_pages(h)) {
> > -		if (!adjust_pool_surplus(h, NULL, -1))
> > +		if (!adjust_pool_surplus(h, nodes_allowed, -1))
> >  			break;
> >  	}
> >  
> > @@ -1245,7 +1279,7 @@ static unsigned long set_max_huge_pages(
> >  		 * and reducing the surplus.
> >  		 */
> >  		spin_unlock(&hugetlb_lock);
> > -		ret = alloc_fresh_huge_page(h, NULL);
> > +		ret = alloc_fresh_huge_page(h, nodes_allowed);
> >  		spin_lock(&hugetlb_lock);
> >  		if (!ret)
> >  			goto out;
> > @@ -1271,16 +1305,17 @@ static unsigned long set_max_huge_pages(
> >  	min_count = max(count, min_count);
> >  	try_to_free_low(h, min_count);
> >  	while (min_count < persistent_huge_pages(h)) {
> > -		if (!free_pool_huge_page(h, NULL, 0))
> > +		if (!free_pool_huge_page(h, nodes_allowed, 0))
> >  			break;
> >  	}
> >  	while (count < persistent_huge_pages(h)) {
> > -		if (!adjust_pool_surplus(h, NULL, 1))
> > +		if (!adjust_pool_surplus(h, nodes_allowed, 1))
> >  			break;
> >  	}
> >  out:
> >  	ret = persistent_huge_pages(h);
> >  	spin_unlock(&hugetlb_lock);
> > +	kfree(nodes_allowed);
> >  	return ret;
> >  }
> >  
> > Index: linux-2.6.31-rc1-mmotm-090625-1549/mm/mempolicy.c
> > ===================================================================
> > --- linux-2.6.31-rc1-mmotm-090625-1549.orig/mm/mempolicy.c	2009-06-29 12:18:11.000000000 -0400
> > +++ linux-2.6.31-rc1-mmotm-090625-1549/mm/mempolicy.c	2009-06-29 23:11:35.000000000 -0400
> > @@ -1544,6 +1544,60 @@ struct zonelist *huge_zonelist(struct vm
> >  	}
> >  	return zl;
> >  }
> > +
> > +/**
> > + * huge_mpol_nodes_allowed()
> > + *
> > + * Return a [pointer to a] nodelist for persistent huge page allocation
> > + * based on the current task's mempolicy:
> > + *
> > + * If the task's mempolicy is "default" [NULL], just return NULL for
> > + * default behavior.  Otherwise, extract the policy nodemask for 'bind'
> > + * or 'interleave' policy or construct a nodemask for 'preferred' or
> > + * 'local' policy and return a pointer to a kmalloc()ed nodemask_t.
> > + * It is the caller's responsibility to free this nodemask.
> > + */
> > +nodemask_t *huge_mpol_nodes_allowed(void)
> > +{
> > +	nodemask_t *nodes_allowed = NULL;
> > +	struct mempolicy *mempolicy;
> > +	int nid;
> > +
> > +	if (!current || !current->mempolicy)
> > +		return NULL;
> > +
> 
> Is it really possible for current to be NULL here?

Not sure.  I've hit NULL current in mempolicy functions called at init
time before.  Maybe a [VM_]BUG_ON() would be preferable?

> 
> > +	mpol_get(current->mempolicy);
> > +	nodes_allowed = kzalloc(sizeof(*nodes_allowed), GFP_KERNEL);
> > +	if (!nodes_allowed) {
> > +		printk(KERN_WARNING "Unable to allocate nodes allowed mask "
> > +			"for huge page allocation\nFalling back to default\n");
> > +		goto out;
> > +	}
> 
> In terms of memory policies, the huge_mpol_nodes_allowed() would appear
> to be unique in allocating a nodemask and expecting the caller to free it
> without leaking. Would it be possible to move the kzalloc() to the caller
> of huge_mpol_nodes_allowed()? It'd be less surprising to me.

I thought doing that.  Then, I'd have to return an indication of whether
to use the allocated mask, or free it and use NULL.  This seemed
cleaner, as set_max_huge_pages() is the only caller of
huge_mpol_nodes_allowed(), which I thought really should go in
mempolicy.c [like huge_zonelist()].  And I did note the caller's
responsibility here...

> 
> I take it you are not allocating nodemask_t on the stack of the caller because
> potentially nodemask_t is very large if MAX_NUMNODES is a big number. Is
> that accurate?

Well, that and the fact that I need to pass NULL to the alloc/free
functions when we have default policy.  I'd still need a nodes_allowed
pointer and you know how you hate having both the nodemask_t and the
pointer :).

> 
> nodemasks are meant to be cleared with nodes_clear() but you depend on
> kzalloc() zeroing the bitmap for you. While the end result is the same,
> is using kzalloc instead of kmalloc+nodes_clear() considered ok?

That did cross my mind.  And, it's not exactly a fast path.  Shall I
change it to kmalloc+nodes_clear()?  Or maybe add a nodemask_alloc() or
nodemask_new() function?  We don't have one of those, as nodemasks have
mostly been passed around by value rather than reference.

> 
> > +
> > +	mempolicy = current->mempolicy;
> > +	switch(mempolicy->mode) {
> > +	case MPOL_PREFERRED:
> > +		if (mempolicy->flags & MPOL_F_LOCAL)
> > +			nid = numa_node_id();
> > +		else
> > +			nid = mempolicy->v.preferred_node;
> > +		node_set(nid, *nodes_allowed);
> > +		break;
> > +
> 
> I think a comment is needed here saying that MPOL_PREFERRED when
> resizing the pool acts more like MPOL_BIND to the preferred node with no
> fallback.

Yeah, I can add such a comment.  I did document this in the hugetlbfs
doc, but I guess it's good to have it here, as well.

> 
> I see your problem though. You can't use set the next_nid to allocate from
> to be the preferred node because the second allocation will interleave away
> from it though. How messy would it be to check if the MPOL_PREFERRED policy
> was in use and avoid updating next_nid_to_alloc while the preferred node is
> being allocated from?
> 
> It's not a big issue and I'd be ok with your current behaviour to start with.

Yeah, and I don't think we want the hugetlb.c code directly looking at
mempolicy internals.  I'd have to add another one user helper function,
probably in mempolicy.h.  Not a biggy, but I didn't think it was
necessary.  It doesn't hurt to reiterate that allocations for populating
the huge page pool do not fallback.

> 
> > +	case MPOL_BIND:
> > +		/* Fall through */
> > +	case MPOL_INTERLEAVE:
> > +			*nodes_allowed =  mempolicy->v.nodes;
> > +		break;
> > +
> > +	default:
> > +		BUG();
> > +	}
> > +
> > +out:
> > +	mpol_put(current->mempolicy);
> > +	return nodes_allowed;
> > +}
> >  #endif
> >  
> >  /* Allocate a page in interleaved policy.
> > Index: linux-2.6.31-rc1-mmotm-090625-1549/include/linux/mempolicy.h
> > ===================================================================
> > --- linux-2.6.31-rc1-mmotm-090625-1549.orig/include/linux/mempolicy.h	2009-06-29 12:18:11.000000000 -0400
> > +++ linux-2.6.31-rc1-mmotm-090625-1549/include/linux/mempolicy.h	2009-06-29 23:06:34.000000000 -0400
> > @@ -201,6 +201,7 @@ extern void mpol_fix_fork_child_flag(str
> >  extern struct zonelist *huge_zonelist(struct vm_area_struct *vma,
> >  				unsigned long addr, gfp_t gfp_flags,
> >  				struct mempolicy **mpol, nodemask_t **nodemask);
> > +extern nodemask_t *huge_mpol_nodes_allowed(void);
> >  extern unsigned slab_node(struct mempolicy *policy);
> >  
> >  extern enum zone_type policy_zone;
> > @@ -328,6 +329,8 @@ static inline struct zonelist *huge_zone
> >  	return node_zonelist(0, gfp_flags);
> >  }
> >  
> > +static inline nodemask_t *huge_mpol_nodes_allowed(void) { return NULL; }
> > +
> >  static inline int do_migrate_pages(struct mm_struct *mm,
> >  			const nodemask_t *from_nodes,
> >  			const nodemask_t *to_nodes, int flags)
> > 
> 
> By and large, this patch would appear to result in reasonable behaviour
> for administrators that want to limit the hugepage pool to specific
> nodes. Predictably, I prefer this approach to the nodemask-sysctl
> approach :/ . With a few crinkles ironed out, I reckon I'd be happy with
> this. Certainly, it appears to work as advertised in that I was able to
> accurate grow/shrink the pool on specific nodes.
> 

Having written the patch, I'm beginning to warm up to this approach
myself :).  We need to understand and advertise any apparently change in
behavior to be sure others agree.

Thanks for the review,
Lee

--
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: [RFC 2/3] hugetlb: derive huge pages nodes allowed from task mempolicy

[Mel Gorman]

On Wed, Jul 01, 2009 at 12:19:24PM -0400, Lee Schermerhorn wrote:
> On Wed, 2009-07-01 at 15:32 +0100, Mel Gorman wrote:
> > On Tue, Jun 30, 2009 at 11:48:18AM -0400, Lee Schermerhorn wrote:
> > > [RFC 2/3] hugetlb:  derive huge pages nodes allowed from task mempolicy
> > > 
> > > Against: 25jun09 mmotm atop the "hugetlb: balance freeing..." series
> > > 
> > > This patch derives a "nodes_allowed" node mask from the numa
> > > mempolicy of the task modifying the number of persistent huge
> > > pages to control the allocation, freeing and adjusting of surplus
> > > huge pages.  This mask is derived as follows:
> > > 
> > > * For "default" [NULL] task mempolicy, a NULL nodemask_t pointer
> > >   is produced.  This will cause the hugetlb subsystem to use
> > >   node_online_map as the "nodes_allowed".  This preserves the
> > >   behavior before this patch.
> > 
> > Sensible.
> > 
> > > * For "preferred" mempolicy, including explicit local allocation,
> > >   a nodemask with the single preferred node will be produced. 
> > >   "local" policy will NOT track any internode migrations of the
> > >   task adjusting nr_hugepages.
> > 
> > This excludes fallback and could do with an in-code comment. I whinge
> > about this more later.
> > 
> > > * For "bind" and "interleave" policy, the mempolicy's nodemask
> > >   will be used.
> > 
> > Sensible.
> > 
> > > * Other than to inform the construction of the nodes_allowed node
> > >   mask, the actual mempolicy mode is ignored.  That is, all modes
> > >   behave like interleave over the resulting nodes_allowed mask
> > >   with no "fallback".
> > > 
> > 
> > Again, seems sensible. Resizing the hugepage pool is not like a normal
> > applications behaviour.
> > 
> > > Because we may have allocated or freed a huge page with a 
> > > different policy/nodes_allowed previously, we always need to
> > > check that the next_node_to_{alloc|free} exists in the current
> > > nodes_allowed mask.  To avoid duplication of code, this is done
> > > in the hstate_next_node_to_{alloc|free}() functions. 
> > 
> > Seems fair. While this means that some nodes could be skipped because there
> > was a hugepage pool resize with a restricted policy and then no policy, I
> > see little problem with that as such.  I believe you caught all the direct
> > users of next_nid_to_[alloc|free] and used the helpers which is good.
> 
> Uh, I've been meaning to point out that I haven't modified
> "try_to_free_low()" to use free_pool_huge_page().  The non-stub version
> of try_to_free_low() is only compiled in when '_HIGHMEM is defined--i.e,
> 32-bit systems.  So, it will still drain nodes completely in node id
> order.  If this is a problem and we want try_to_free_low() to balance
> freeing, I can enhance free_pool_huge_page() to take another flag
> [perhaps combining it with the "acct_surplus" in a single flags arg] to
> free only !highmem pages and pass a node mask of any nodes containing
> such pages. 
> 

I was basically assuming that 32-bit NUMA machines weren't interesting
but glancing through, it would be a relatively straight-forward
conversion to obey the memory policies here as well. IIRC though, 32-bit
NUMA machines will have the NORMAL zone on one node anyway so
interleaving is basically a no-op if one wants to concentrate on freeing
lowmem pages.

> > 
> > > So,
> > > these functions have been modified to allow them to be called
> > > to obtain the "start_nid".  Then, whereas prior to this patch
> > > we unconditionally called hstate_next_node_to_{alloc|free}(),
> > > whether or not we successfully allocated/freed a huge page on
> > > the node, now we only call these functions on failure to alloc/free.
> > > 
> > > Notes:
> > > 
> > > 1) This patch introduces a subtle change in behavior:  huge page
> > >    allocation and freeing will be constrained by any mempolicy
> > >    that the task adjusting the huge page pool inherits from its
> > >    parent.  This policy could come from a distant ancestor.  The
> > >    adminstrator adjusting the huge page pool without explicitly
> > >    specifying a mempolicy via numactl might be surprised by this.
> > 
> > I would be trying to encourage administrators to use hugeadm instead of
> > manually tuning the pools. One possible course of action is for hugeadm
> > to check if a policy is currently set and output that as an INFO message.
> > That will show up if they run with hugeadm -v. Alternatively, we could note
> > as a WARN when any policy is set and print an INFO message on details of
> > the policy.
> 
> Yes.  I saw mention of direct access to the sysctls being deprecated.
> I'm not sure how that will go over with users, but if go with changes
> like this, it makes sense to handle them in hugeadm.
> 

In this case, we're only worried about notifying the user if they are
under a policy. If they are accessing the sysctl's directly, there is
not much that can be done to warn them.

> > 
> > >    Additionaly, any mempolicy specified by numactl will be
> > >    constrained by the cpuset in which numactl is invoked.
> > > 
> > > 2) Hugepages allocated at boot time use the node_online_map.
> > >    An additional patch could implement a temporary boot time
> > >    huge pages nodes_allowed command line parameter.
> > > 
> > 
> > I'd want for someone to complain before implementing such a patch. Even
> > on systems where hugepages must be allocated very early on, an init script
> > should be more than sufficient without implementing parsing for mempolicies
> > and hugepages.
> 
> I agree in principle.  I expect that if one can't allocate the required
> number of huge pages in an early init script, either one really needs
> more memory, or something is wrong with earlier scripts that is
> fragmenting memory. 

Agreed.

> However, I'll point out that by the time one gets
> such a complaint, there is a really long lead time before a change gets
> into customer hands.  This is the downside of waiting for a complaint or
> definitive use case before providing tunable parameters and such :(.
> 

There is that, but as the workaround here is basically "install more
memory", it feels less critical. I just can't see the lack of node
specification being a show-stopper for anyone. Famous last words of
course :/

> > 
> > > See the updated documentation [next patch] for more information
> > > about the implications of this patch.
> > > 
> > 
> > Ideally the change log should show a before and after of using numactl +
> > hugeadm to resize pools on a subset of available nodes.
> 
> I'll do that. 
> 
> > 
> > > Signed-off-by: Lee Schermerhorn <lee.schermerhorn@hp.com>
> > > 
> > >  include/linux/mempolicy.h |    3 +
> > >  mm/hugetlb.c              |   99 +++++++++++++++++++++++++++++++---------------
> > >  mm/mempolicy.c            |   54 +++++++++++++++++++++++++
> > >  3 files changed, 124 insertions(+), 32 deletions(-)
> > > 
> > > Index: linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c
> > > ===================================================================
> > > --- linux-2.6.31-rc1-mmotm-090625-1549.orig/mm/hugetlb.c	2009-06-29 23:01:01.000000000 -0400
> > > +++ linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c	2009-06-30 11:29:49.000000000 -0400
> > > @@ -621,29 +621,52 @@ static struct page *alloc_fresh_huge_pag
> > >  }
> > >  
> > >  /*
> > > + * common helper functions for hstate_next_node_to_{alloc|free}.
> > > + * We may have allocated or freed a huge pages based on a different
> > > + * nodes_allowed, previously, so h->next_node_to_{alloc|free} might
> > > + * be outside of *nodes_allowed.  Ensure that we use the next
> > > + * allowed node for alloc or free.
> > > + */
> > > +static int next_node_allowed(int nid, nodemask_t *nodes_allowed)
> > > +{
> > > +	nid = next_node(nid, *nodes_allowed);
> > > +	if (nid == MAX_NUMNODES)
> > > +		nid = first_node(*nodes_allowed); /* handle "wrap" */
> > > +	return nid;
> > > +}
> > 
> > The handle warp comment is unnecessary there. This is such a common pattern,
> > it should be self-evident without placing comments that cause the
> > CodingStyle Police to issue tickets.
> 
> Thanks for the warning.  I'll remove it.
> 
> > 
> > > +
> > > +static int this_node_allowed(int nid, nodemask_t *nodes_allowed)
> > > +{
> > > +	if (!node_isset(nid, *nodes_allowed))
> > > +		nid = next_node_allowed(nid, nodes_allowed);
> > > +	return nid;
> > > +}
> > > +
> > 
> > What happens if node hot-remove occured and there is now no node that we
> > are allowed to allocate from?
> 
> If we return a valid node id [see below] of an off-line node, the
> allocation or free will fail and we'll advance to the next node.  If all
> the nodes in the nodes_allowed are off-line, we'll end up with next_nid
> == start_nid and bail out.
> 

Ok, there is a possibilily we'll OOM when returning NULL like this but a
sensible way of dealing with that situation doesn't spring to mind. Mind
you, maybe a user shouldn't be too suprised if they off-line a node that
has active processes still bound to it :/. I guess ordinarily the pages
would be migrated but hugetlbfs does not have such capabilities right
now.

> > 
> > This thing will end up returning MAX_NUMNODES right? That potentially then
> > gets passed to alloc_pages_exact_node() triggering a VM_BUG_ON() there.
> 
> No.  That's "next_node_allowed()"--the function above with the spurious
> "handle wrap" comment--not the bare "next_node()".  So, we will "handle
> wrap" appropriately.  
> 

But first_node() itself can return MAX_NUMNODES, right?

> > 
> > > +/*
> > >   * Use a helper variable to find the next node and then
> > >   * copy it back to next_nid_to_alloc afterwards:
> > >   * otherwise there's a window in which a racer might
> > >   * pass invalid nid MAX_NUMNODES to alloc_pages_exact_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.
> > > + * same nid as we do.  Move nid forward in the mask whether
> > > + * or not we just successfully allocated a hugepage so that
> > > + * the next allocation addresses the next node.
> > >   */
> > >  static int hstate_next_node_to_alloc(struct hstate *h,
> > >  					nodemask_t *nodes_allowed)
> > >  {
> > > -	int next_nid;
> > > +	int nid, next_nid;
> > >  
> > >  	if (!nodes_allowed)
> > >  		nodes_allowed = &node_online_map;
> > >  
> > > -	next_nid = next_node(h->next_nid_to_alloc, *nodes_allowed);
> > > -	if (next_nid == MAX_NUMNODES)
> > > -		next_nid = first_node(*nodes_allowed);
> > > +	nid = this_node_allowed(h->next_nid_to_alloc, nodes_allowed);
> > > +
> > > +	next_nid = next_node_allowed(nid, nodes_allowed);
> > >  	h->next_nid_to_alloc = next_nid;
> > > -	return next_nid;
> > > +
> > > +	return nid;
> > >  }
> > >  
> > 
> > Seems reasonable. I see now what you mean about next_nid_to_alloc being more
> > like its name in patch series than the last.
> > 
> > >  static int alloc_fresh_huge_page(struct hstate *h, nodemask_t *nodes_allowed)
> > > @@ -653,15 +676,17 @@ static int alloc_fresh_huge_page(struct 
> > >  	int next_nid;
> > >  	int ret = 0;
> > >  
> > > -	start_nid = h->next_nid_to_alloc;
> > > +	start_nid = hstate_next_node_to_alloc(h, nodes_allowed);
> > >  	next_nid = start_nid;
> > >  
> > 
> > So, here for example, I think we need to make sure start_nid is not
> > MAX_NUMNODES and bail out if it is.
> 
> See response above, re: this_node_allowed().  Do you still think it's a
> problem?
> 

I'm not fully convinced. As first_node() can return MAX_NUMNODES, it
looks like hstate_next_node_to_alloc() can return MAX_NUMNODES and that
can get passed to alloc_pages_exact_node().

> > 
> > >  	do {
> > >  		page = alloc_fresh_huge_page_node(h, next_nid);
> > > -		if (page)
> > > +		if (page) {
> > >  			ret = 1;
> > > +			break;
> > > +		}
> > 
> > Ok, so this break is necessary on allocation success because
> > hstate_next_node_to_alloc() has already bumped next_nid_to_alloc. Right?
> 
> Right.  I don't want to fall thru and skip a node.  so, I break and
> remove the !page from the loop condition.
> 
> However, since you mention it:  I noticed, since I sent this, that with
> this arrangement we will skip a node id in the case that we visit all
> nodes w/o successfully allocating a huge page or finding a huge page to
> free.  When we hit the loop termination condition "next_nid ==
> start_nid", we've already advanced hstate_next_node_to_{alloc|free}
> beyond next_nid.   So, on next call, start_nid will become that value
> and not the same start_nid we used before.
> 
> Example:  suppose we try to start a huge page [on, say, a 4 node] system
> with no huge pages available anywhere; and the next_node_to_alloc == 0
> when we first call alloc_fresh_huge_page().  start_nid will get '0' and
> we'll examine nodes 0, 1, 2 and 3.  When hstate_next_node_to_alloc()
> returns '0', we'll give up and return NULL page, but next_node_to_alloc
> is now 1.  If we try again, maybe after we think some memory has been
> freed up and we have a chance of succeeding, we'll visit 1, 2, 3 and 0
> [if there are still no huge pages available].  And so forth.  I don't
> think this is a problem because if we actually succeed in allocating or
> freeing a page, we'll break w/o advancing next_node... and pick up there
> on next call.
> 
> I have an idea for fixing this behavior, if anyone thinks it's a
> problem, by "pushing back" the next_nid to the hstate with something
> like:
> 
> 	} while (next_nid != start_nid || push_back_alloc_node(next_nid))
> 
> where the push_back function always returns 0.  Kind of ugly, but I
> think it would work.  I don't think it's needed, tho'.
> 

I don't think it's a problem that is worth worrying about. If the
administrator is interleaving between multiple nodes, I don't see why
they would care which one the first node used.

> > 
> > >  		next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
> > > -	} while (!page && next_nid != start_nid);
> > > +	} while (next_nid != start_nid);
> > >  
> > >  	if (ret)
> > >  		count_vm_event(HTLB_BUDDY_PGALLOC);
> > > @@ -672,21 +697,23 @@ static int alloc_fresh_huge_page(struct 
> > >  }
> > >  
> > >  /*
> > > - * helper for free_pool_huge_page() - find next node
> > > - * from which to free a huge page
> > > + * helper for free_pool_huge_page() - return the next node
> > > + * from which to free a huge page.  Advance the next node id
> > > + * whether or not we find a free huge page to free so that the
> > > + * next attempt to free addresses the next node.
> > >   */
> > >  static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
> > >  {
> > > -	int next_nid;
> > > +	int nid, next_nid;
> > >  
> > >  	if (!nodes_allowed)
> > >  		nodes_allowed = &node_online_map;
> > >  
> > > -	next_nid = next_node(h->next_nid_to_free, *nodes_allowed);
> > > -	if (next_nid == MAX_NUMNODES)
> > > -		next_nid = first_node(*nodes_allowed);
> > > +	nid = this_node_allowed(h->next_nid_to_free, nodes_allowed);
> > > +	next_nid = next_node_allowed(nid, nodes_allowed);
> > >  	h->next_nid_to_free = next_nid;
> > > -	return next_nid;
> > > +
> > > +	return nid;
> > >  }
> > >  
> > >  /*
> > > @@ -702,7 +729,7 @@ static int free_pool_huge_page(struct hs
> > >  	int next_nid;
> > >  	int ret = 0;
> > >  
> > > -	start_nid = h->next_nid_to_free;
> > > +	start_nid = hstate_next_node_to_free(h, nodes_allowed);
> > >  	next_nid = start_nid;
> > >  
> > 
> > I guess if start_nid is MAX_NUMNODES here as well, we should bail out
> > early. It means that a pool shrink by the requested number of pages may
> > fail if there are not enough hugepages allocated on the allowed node but
> > maybe that's not such a big problem.
> 
> Again, I don't that can happen.  ???
> 
> > 
> > hugeadm will actually notice when this situation occurs and gives a
> > warning about it.
> > 
> > >  	do {
> > > @@ -726,9 +753,10 @@ static int free_pool_huge_page(struct hs
> > >  			}
> > >  			update_and_free_page(h, page);
> > >  			ret = 1;
> > > +			break;
> > >  		}
> > >   		next_nid = hstate_next_node_to_free(h, nodes_allowed);
> > > -	} while (!ret && next_nid != start_nid);
> > > +	} while (next_nid != start_nid);
> > >  
> > >  	return ret;
> > >  }
> > > @@ -1039,10 +1067,9 @@ int __weak alloc_bootmem_huge_page(struc
> > >  		void *addr;
> > >  
> > >  		addr = __alloc_bootmem_node_nopanic(
> > > -				NODE_DATA(h->next_nid_to_alloc),
> > > +				NODE_DATA(hstate_next_node_to_alloc(h, NULL)),
> > >  				huge_page_size(h), huge_page_size(h), 0);
> > >  
> > > -		hstate_next_node_to_alloc(h, NULL); /* always advance nid */
> > >  		if (addr) {
> > >  			/*
> > >  			 * Use the beginning of the huge page to store the
> > > @@ -1179,29 +1206,33 @@ static int adjust_pool_surplus(struct hs
> > >  	VM_BUG_ON(delta != -1 && delta != 1);
> > >  
> > >  	if (delta < 0)
> > > -		start_nid = h->next_nid_to_alloc;
> > > +		start_nid = hstate_next_node_to_alloc(h, nodes_allowed);
> > >  	else
> > > -		start_nid = h->next_nid_to_free;
> > > +		start_nid = hstate_next_node_to_free(h, nodes_allowed);
> > >  	next_nid = start_nid;
> > >  
> > >  	do {
> > >  		int nid = next_nid;
> > >  		if (delta < 0)  {
> > > -			next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
> > >  			/*
> > >  			 * To shrink on this node, there must be a surplus page
> > >  			 */
> > > -			if (!h->surplus_huge_pages_node[nid])
> > > +			if (!h->surplus_huge_pages_node[nid]) {
> > > +				next_nid = hstate_next_node_to_alloc(h,
> > > +								nodes_allowed);
> > >  				continue;
> > > +			}
> > >  		}
> > >  		if (delta > 0) {
> > > -			next_nid = hstate_next_node_to_free(h, nodes_allowed);
> > >  			/*
> > >  			 * Surplus cannot exceed the total number of pages
> > >  			 */
> > >  			if (h->surplus_huge_pages_node[nid] >=
> > > -						h->nr_huge_pages_node[nid])
> > > +						h->nr_huge_pages_node[nid]) {
> > > +				next_nid = hstate_next_node_to_free(h,
> > > +								nodes_allowed);
> > >  				continue;
> > > +			}
> > >  		}
> > >  
> > >  		h->surplus_huge_pages += delta;
> > > @@ -1217,10 +1248,13 @@ static int adjust_pool_surplus(struct hs
> > >  static unsigned long set_max_huge_pages(struct hstate *h, unsigned long count)
> > >  {
> > >  	unsigned long min_count, ret;
> > > +	nodemask_t *nodes_allowed;
> > >  
> > >  	if (h->order >= MAX_ORDER)
> > >  		return h->max_huge_pages;
> > >  
> > > +	nodes_allowed = huge_mpol_nodes_allowed();
> > > +
> > >  	/*
> > >  	 * Increase the pool size
> > >  	 * First take pages out of surplus state.  Then make up the
> > > @@ -1234,7 +1268,7 @@ static unsigned long set_max_huge_pages(
> > >  	 */
> > >  	spin_lock(&hugetlb_lock);
> > >  	while (h->surplus_huge_pages && count > persistent_huge_pages(h)) {
> > > -		if (!adjust_pool_surplus(h, NULL, -1))
> > > +		if (!adjust_pool_surplus(h, nodes_allowed, -1))
> > >  			break;
> > >  	}
> > >  
> > > @@ -1245,7 +1279,7 @@ static unsigned long set_max_huge_pages(
> > >  		 * and reducing the surplus.
> > >  		 */
> > >  		spin_unlock(&hugetlb_lock);
> > > -		ret = alloc_fresh_huge_page(h, NULL);
> > > +		ret = alloc_fresh_huge_page(h, nodes_allowed);
> > >  		spin_lock(&hugetlb_lock);
> > >  		if (!ret)
> > >  			goto out;
> > > @@ -1271,16 +1305,17 @@ static unsigned long set_max_huge_pages(
> > >  	min_count = max(count, min_count);
> > >  	try_to_free_low(h, min_count);
> > >  	while (min_count < persistent_huge_pages(h)) {
> > > -		if (!free_pool_huge_page(h, NULL, 0))
> > > +		if (!free_pool_huge_page(h, nodes_allowed, 0))
> > >  			break;
> > >  	}
> > >  	while (count < persistent_huge_pages(h)) {
> > > -		if (!adjust_pool_surplus(h, NULL, 1))
> > > +		if (!adjust_pool_surplus(h, nodes_allowed, 1))
> > >  			break;
> > >  	}
> > >  out:
> > >  	ret = persistent_huge_pages(h);
> > >  	spin_unlock(&hugetlb_lock);
> > > +	kfree(nodes_allowed);
> > >  	return ret;
> > >  }
> > >  
> > > Index: linux-2.6.31-rc1-mmotm-090625-1549/mm/mempolicy.c
> > > ===================================================================
> > > --- linux-2.6.31-rc1-mmotm-090625-1549.orig/mm/mempolicy.c	2009-06-29 12:18:11.000000000 -0400
> > > +++ linux-2.6.31-rc1-mmotm-090625-1549/mm/mempolicy.c	2009-06-29 23:11:35.000000000 -0400
> > > @@ -1544,6 +1544,60 @@ struct zonelist *huge_zonelist(struct vm
> > >  	}
> > >  	return zl;
> > >  }
> > > +
> > > +/**
> > > + * huge_mpol_nodes_allowed()
> > > + *
> > > + * Return a [pointer to a] nodelist for persistent huge page allocation
> > > + * based on the current task's mempolicy:
> > > + *
> > > + * If the task's mempolicy is "default" [NULL], just return NULL for
> > > + * default behavior.  Otherwise, extract the policy nodemask for 'bind'
> > > + * or 'interleave' policy or construct a nodemask for 'preferred' or
> > > + * 'local' policy and return a pointer to a kmalloc()ed nodemask_t.
> > > + * It is the caller's responsibility to free this nodemask.
> > > + */
> > > +nodemask_t *huge_mpol_nodes_allowed(void)
> > > +{
> > > +	nodemask_t *nodes_allowed = NULL;
> > > +	struct mempolicy *mempolicy;
> > > +	int nid;
> > > +
> > > +	if (!current || !current->mempolicy)
> > > +		return NULL;
> > > +
> > 
> > Is it really possible for current to be NULL here?
> 
> Not sure.  I've hit NULL current in mempolicy functions called at init
> time before.  Maybe a [VM_]BUG_ON() would be preferable?
> 

VM_BUG_ON() I'd say would be enough. Even if support is added later to
allocateo with a nodemask at boot-time when current == NULL, they'll need
to know to use the default policy instead of current->mempolicy.

> > 
> > > +	mpol_get(current->mempolicy);
> > > +	nodes_allowed = kzalloc(sizeof(*nodes_allowed), GFP_KERNEL);
> > > +	if (!nodes_allowed) {
> > > +		printk(KERN_WARNING "Unable to allocate nodes allowed mask "
> > > +			"for huge page allocation\nFalling back to default\n");
> > > +		goto out;
> > > +	}
> > 
> > In terms of memory policies, the huge_mpol_nodes_allowed() would appear
> > to be unique in allocating a nodemask and expecting the caller to free it
> > without leaking. Would it be possible to move the kzalloc() to the caller
> > of huge_mpol_nodes_allowed()? It'd be less surprising to me.
> 
> I thought doing that.  Then, I'd have to return an indication of whether
> to use the allocated mask, or free it and use NULL.  This seemed
> cleaner, as set_max_huge_pages() is the only caller of
> huge_mpol_nodes_allowed(), which I thought really should go in
> mempolicy.c [like huge_zonelist()].  And I did note the caller's
> responsibility here...
> 

Fair point.

> > 
> > I take it you are not allocating nodemask_t on the stack of the caller because
> > potentially nodemask_t is very large if MAX_NUMNODES is a big number. Is
> > that accurate?
> 
> Well, that and the fact that I need to pass NULL to the alloc/free
> functions when we have default policy.  I'd still need a nodes_allowed
> pointer and you know how you hate having both the nodemask_t and the
> pointer :).
> 

Yeah, it does cause me to whinge :)

> > 
> > nodemasks are meant to be cleared with nodes_clear() but you depend on
> > kzalloc() zeroing the bitmap for you. While the end result is the same,
> > is using kzalloc instead of kmalloc+nodes_clear() considered ok?
> 
> That did cross my mind.  And, it's not exactly a fast path.  Shall I
> change it to kmalloc+nodes_clear()?  Or maybe add a nodemask_alloc() or
> nodemask_new() function?  We don't have one of those, as nodemasks have
> mostly been passed around by value rather than reference.
> 

I think kmalloc+nodes_clear() might be more future proof in the event
does something like use poison byte pattern to detect when a nodemask is
improperly initialised or something similar. I don't think it needs a
nodemask_alloc() or nodemask_new() helper.

> > 
> > > +
> > > +	mempolicy = current->mempolicy;
> > > +	switch(mempolicy->mode) {
> > > +	case MPOL_PREFERRED:
> > > +		if (mempolicy->flags & MPOL_F_LOCAL)
> > > +			nid = numa_node_id();
> > > +		else
> > > +			nid = mempolicy->v.preferred_node;
> > > +		node_set(nid, *nodes_allowed);
> > > +		break;
> > > +
> > 
> > I think a comment is needed here saying that MPOL_PREFERRED when
> > resizing the pool acts more like MPOL_BIND to the preferred node with no
> > fallback.
> 
> Yeah, I can add such a comment.  I did document this in the hugetlbfs
> doc, but I guess it's good to have it here, as well.
> 

It doesn't hurt.

> > 
> > I see your problem though. You can't use set the next_nid to allocate from
> > to be the preferred node because the second allocation will interleave away
> > from it though. How messy would it be to check if the MPOL_PREFERRED policy
> > was in use and avoid updating next_nid_to_alloc while the preferred node is
> > being allocated from?
> > 
> > It's not a big issue and I'd be ok with your current behaviour to start with.
> 
> Yeah, and I don't think we want the hugetlb.c code directly looking at
> mempolicy internals.  I'd have to add another one user helper function,
> probably in mempolicy.h.  Not a biggy, but I didn't think it was
> necessary.  It doesn't hurt to reiterate that allocations for populating
> the huge page pool do not fallback.
> 

Lets go with your suggested behaviour for now. Double checking the
policy will just over-complicate things in the first iteration.

> > 
> > > +	case MPOL_BIND:
> > > +		/* Fall through */
> > > +	case MPOL_INTERLEAVE:
> > > +			*nodes_allowed =  mempolicy->v.nodes;
> > > +		break;
> > > +
> > > +	default:
> > > +		BUG();
> > > +	}
> > > +
> > > +out:
> > > +	mpol_put(current->mempolicy);
> > > +	return nodes_allowed;
> > > +}
> > >  #endif
> > >  
> > >  /* Allocate a page in interleaved policy.
> > > Index: linux-2.6.31-rc1-mmotm-090625-1549/include/linux/mempolicy.h
> > > ===================================================================
> > > --- linux-2.6.31-rc1-mmotm-090625-1549.orig/include/linux/mempolicy.h	2009-06-29 12:18:11.000000000 -0400
> > > +++ linux-2.6.31-rc1-mmotm-090625-1549/include/linux/mempolicy.h	2009-06-29 23:06:34.000000000 -0400
> > > @@ -201,6 +201,7 @@ extern void mpol_fix_fork_child_flag(str
> > >  extern struct zonelist *huge_zonelist(struct vm_area_struct *vma,
> > >  				unsigned long addr, gfp_t gfp_flags,
> > >  				struct mempolicy **mpol, nodemask_t **nodemask);
> > > +extern nodemask_t *huge_mpol_nodes_allowed(void);
> > >  extern unsigned slab_node(struct mempolicy *policy);
> > >  
> > >  extern enum zone_type policy_zone;
> > > @@ -328,6 +329,8 @@ static inline struct zonelist *huge_zone
> > >  	return node_zonelist(0, gfp_flags);
> > >  }
> > >  
> > > +static inline nodemask_t *huge_mpol_nodes_allowed(void) { return NULL; }
> > > +
> > >  static inline int do_migrate_pages(struct mm_struct *mm,
> > >  			const nodemask_t *from_nodes,
> > >  			const nodemask_t *to_nodes, int flags)
> > > 
> > 
> > By and large, this patch would appear to result in reasonable behaviour
> > for administrators that want to limit the hugepage pool to specific
> > nodes. Predictably, I prefer this approach to the nodemask-sysctl
> > approach :/ . With a few crinkles ironed out, I reckon I'd be happy with
> > this. Certainly, it appears to work as advertised in that I was able to
> > accurate grow/shrink the pool on specific nodes.
> > 
> 
> Having written the patch, I'm beginning to warm up to this approach
> myself :).  We need to understand and advertise any apparently change in
> behavior to be sure others agree.
> 
> Thanks for the review,
> Lee
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
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: [RFC 2/3] hugetlb: derive huge pages nodes allowed from task mempolicy

[Lee Schermerhorn]

On Wed, 2009-07-01 at 19:29 +0100, Mel Gorman wrote:
> On Wed, Jul 01, 2009 at 12:19:24PM -0400, Lee Schermerhorn wrote:
> > On Wed, 2009-07-01 at 15:32 +0100, Mel Gorman wrote:
> > > On Tue, Jun 30, 2009 at 11:48:18AM -0400, Lee Schermerhorn wrote:
> > > > [RFC 2/3] hugetlb:  derive huge pages nodes allowed from task mempolicy
> > > > 
> > > > Against: 25jun09 mmotm atop the "hugetlb: balance freeing..." series
> > > > 
> > > > This patch derives a "nodes_allowed" node mask from the numa
> > > > mempolicy of the task modifying the number of persistent huge
> > > > pages to control the allocation, freeing and adjusting of surplus
> > > > huge pages.  
<snip>
> > > > Notes:
> > > > 
> > > > 1) This patch introduces a subtle change in behavior:  huge page
> > > >    allocation and freeing will be constrained by any mempolicy
> > > >    that the task adjusting the huge page pool inherits from its
> > > >    parent.  This policy could come from a distant ancestor.  The
> > > >    adminstrator adjusting the huge page pool without explicitly
> > > >    specifying a mempolicy via numactl might be surprised by this.
> > > 
> > > I would be trying to encourage administrators to use hugeadm instead of
> > > manually tuning the pools. One possible course of action is for hugeadm
> > > to check if a policy is currently set and output that as an INFO message.
> > > That will show up if they run with hugeadm -v. Alternatively, we could note
> > > as a WARN when any policy is set and print an INFO message on details of
> > > the policy.
> > 
> > Yes.  I saw mention of direct access to the sysctls being deprecated.
> > I'm not sure how that will go over with users, but if go with changes
> > like this, it makes sense to handle them in hugeadm.
> > 
> 
> In this case, we're only worried about notifying the user if they are
> under a policy. If they are accessing the sysctl's directly, there is
> not much that can be done to warn them.

Agree.  Using the tool allows us to warn the user.  Actually, we could
add a mempolicy argument to hugeadm and have it set the policy
explicitly and warn when it inherits a non-default one w/o an explicit
argument.  For users that still want to poke at the sysctls and
attributes directly, numactl will still work.  But, as you say, they're
on their own.

> 
> > > 
> > > >    Additionaly, any mempolicy specified by numactl will be
> > > >    constrained by the cpuset in which numactl is invoked.
> > > > 
> > > > 2) Hugepages allocated at boot time use the node_online_map.
> > > >    An additional patch could implement a temporary boot time
> > > >    huge pages nodes_allowed command line parameter.
> > > > 
> > > 
> > > I'd want for someone to complain before implementing such a patch. Even
> > > on systems where hugepages must be allocated very early on, an init script
> > > should be more than sufficient without implementing parsing for mempolicies
> > > and hugepages.
> > 
> > I agree in principle.  I expect that if one can't allocate the required
> > number of huge pages in an early init script, either one really needs
> > more memory, or something is wrong with earlier scripts that is
> > fragmenting memory. 
> 
> Agreed.
> 
> > However, I'll point out that by the time one gets
> > such a complaint, there is a really long lead time before a change gets
> > into customer hands.  This is the downside of waiting for a complaint or
> > definitive use case before providing tunable parameters and such :(.
> > 
> 
> There is that, but as the workaround here is basically "install more
> memory", it feels less critical. I just can't see the lack of node
> specification being a show-stopper for anyone. Famous last words of
> course :/

Understood.  I was just grousing.

> 
> > > 
> > > > See the updated documentation [next patch] for more information
> > > > about the implications of this patch.
> > > > 
> > > 
> > > Ideally the change log should show a before and after of using numactl +
> > > hugeadm to resize pools on a subset of available nodes.
> > 
> > I'll do that. 
> > 
> > > 
> > > > Signed-off-by: Lee Schermerhorn <lee.schermerhorn@hp.com>
> > > > 
> > > >  include/linux/mempolicy.h |    3 +
> > > >  mm/hugetlb.c              |   99 +++++++++++++++++++++++++++++++---------------
> > > >  mm/mempolicy.c            |   54 +++++++++++++++++++++++++
> > > >  3 files changed, 124 insertions(+), 32 deletions(-)
> > > > 
> > > > Index: linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c
> > > > ===================================================================
> > > > --- linux-2.6.31-rc1-mmotm-090625-1549.orig/mm/hugetlb.c	2009-06-29 23:01:01.000000000 -0400
> > > > +++ linux-2.6.31-rc1-mmotm-090625-1549/mm/hugetlb.c	2009-06-30 11:29:49.000000000 -0400
> > > > @@ -621,29 +621,52 @@ static struct page *alloc_fresh_huge_pag
> > > >  }
> > > >  
> > > >  /*
> > > > + * common helper functions for hstate_next_node_to_{alloc|free}.
> > > > + * We may have allocated or freed a huge pages based on a different
> > > > + * nodes_allowed, previously, so h->next_node_to_{alloc|free} might
> > > > + * be outside of *nodes_allowed.  Ensure that we use the next
> > > > + * allowed node for alloc or free.
> > > > + */
> > > > +static int next_node_allowed(int nid, nodemask_t *nodes_allowed)
> > > > +{
> > > > +	nid = next_node(nid, *nodes_allowed);
> > > > +	if (nid == MAX_NUMNODES)
> > > > +		nid = first_node(*nodes_allowed); /* handle "wrap" */
> > > > +	return nid;
> > > > +}
> > > 
> > > The handle warp comment is unnecessary there. This is such a common pattern,
> > > it should be self-evident without placing comments that cause the
> > > CodingStyle Police to issue tickets.
> > 
> > Thanks for the warning.  I'll remove it.
> > 
> > > 
> > > > +
> > > > +static int this_node_allowed(int nid, nodemask_t *nodes_allowed)
> > > > +{
> > > > +	if (!node_isset(nid, *nodes_allowed))
> > > > +		nid = next_node_allowed(nid, nodes_allowed);
> > > > +	return nid;
> > > > +}
> > > > +
> > > 
> > > What happens if node hot-remove occured and there is now no node that we
> > > are allowed to allocate from?
> > 
> > If we return a valid node id [see below] of an off-line node, the
> > allocation or free will fail and we'll advance to the next node.  If all
> > the nodes in the nodes_allowed are off-line, we'll end up with next_nid
> > == start_nid and bail out.
> > 
> 
> Ok, there is a possibilily we'll OOM when returning NULL like this but a
> sensible way of dealing with that situation doesn't spring to mind. Mind
> you, maybe a user shouldn't be too suprised if they off-line a node that
> has active processes still bound to it :/. I guess ordinarily the pages
> would be migrated but hugetlbfs does not have such capabilities right
> now.

Remember that the alloc_fresh... and free_pool... functions loop over
all nodes [in the node mask] or until they succeed.  If they fail, its
because they didn't find a huge page to alloc/free on any node in the
nodemask.  And, for the alloc case, this is for populating the
persistent huge page pool.  We shouldn't OOM there, right?  

> 
> > > 
> > > This thing will end up returning MAX_NUMNODES right? That potentially then
> > > gets passed to alloc_pages_exact_node() triggering a VM_BUG_ON() there.
> > 
> > No.  That's "next_node_allowed()"--the function above with the spurious
> > "handle wrap" comment--not the bare "next_node()".  So, we will "handle
> > wrap" appropriately.  
> > 
> 
> But first_node() itself can return MAX_NUMNODES, right?

If I read the bit map code correctly, this can occur only if the
nodemask is empty [in the first MAX_NUMNODES bits].  node_online_map
can't be empty if we're here, I think.  And huge_mpol_nodes_allowed()
can't [shouldn't!] return an empty mask.  NULL pointer, yes, meaning
"use node_online_map".

> 
> > > 
> > > > +/*
> > > >   * Use a helper variable to find the next node and then
> > > >   * copy it back to next_nid_to_alloc afterwards:
> > > >   * otherwise there's a window in which a racer might
> > > >   * pass invalid nid MAX_NUMNODES to alloc_pages_exact_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.
> > > > + * same nid as we do.  Move nid forward in the mask whether
> > > > + * or not we just successfully allocated a hugepage so that
> > > > + * the next allocation addresses the next node.
> > > >   */
> > > >  static int hstate_next_node_to_alloc(struct hstate *h,
> > > >  					nodemask_t *nodes_allowed)
> > > >  {
> > > > -	int next_nid;
> > > > +	int nid, next_nid;
> > > >  
> > > >  	if (!nodes_allowed)
> > > >  		nodes_allowed = &node_online_map;
> > > >  
> > > > -	next_nid = next_node(h->next_nid_to_alloc, *nodes_allowed);
> > > > -	if (next_nid == MAX_NUMNODES)
> > > > -		next_nid = first_node(*nodes_allowed);
> > > > +	nid = this_node_allowed(h->next_nid_to_alloc, nodes_allowed);
> > > > +
> > > > +	next_nid = next_node_allowed(nid, nodes_allowed);
> > > >  	h->next_nid_to_alloc = next_nid;
> > > > -	return next_nid;
> > > > +
> > > > +	return nid;
> > > >  }
> > > >  
> > > 
> > > Seems reasonable. I see now what you mean about next_nid_to_alloc being more
> > > like its name in patch series than the last.
> > > 
> > > >  static int alloc_fresh_huge_page(struct hstate *h, nodemask_t *nodes_allowed)
> > > > @@ -653,15 +676,17 @@ static int alloc_fresh_huge_page(struct 
> > > >  	int next_nid;
> > > >  	int ret = 0;
> > > >  
> > > > -	start_nid = h->next_nid_to_alloc;
> > > > +	start_nid = hstate_next_node_to_alloc(h, nodes_allowed);
> > > >  	next_nid = start_nid;
> > > >  
> > > 
> > > So, here for example, I think we need to make sure start_nid is not
> > > MAX_NUMNODES and bail out if it is.
> > 
> > See response above, re: this_node_allowed().  Do you still think it's a
> > problem?
> > 
> 
> I'm not fully convinced. As first_node() can return MAX_NUMNODES, it
> looks like hstate_next_node_to_alloc() can return MAX_NUMNODES and that
> can get passed to alloc_pages_exact_node().

Well, we can add a VM_BUG_ON.

> 
> > > 
> > > >  	do {
> > > >  		page = alloc_fresh_huge_page_node(h, next_nid);
> > > > -		if (page)
> > > > +		if (page) {
> > > >  			ret = 1;
> > > > +			break;
> > > > +		}
> > > 
> > > Ok, so this break is necessary on allocation success because
> > > hstate_next_node_to_alloc() has already bumped next_nid_to_alloc. Right?
> > 
> > Right.  I don't want to fall thru and skip a node.  so, I break and
> > remove the !page from the loop condition.
> > 
> > However, since you mention it:  I noticed, since I sent this, that with
> > this arrangement we will skip a node id in the case that we visit all
> > nodes w/o successfully allocating a huge page or finding a huge page to
> > free.  When we hit the loop termination condition "next_nid ==
> > start_nid", we've already advanced hstate_next_node_to_{alloc|free}
> > beyond next_nid.   So, on next call, start_nid will become that value
> > and not the same start_nid we used before.
> > 
> > Example:  suppose we try to start a huge page [on, say, a 4 node] system
> > with no huge pages available anywhere; and the next_node_to_alloc == 0
> > when we first call alloc_fresh_huge_page().  start_nid will get '0' and
> > we'll examine nodes 0, 1, 2 and 3.  When hstate_next_node_to_alloc()
> > returns '0', we'll give up and return NULL page, but next_node_to_alloc
> > is now 1.  If we try again, maybe after we think some memory has been
> > freed up and we have a chance of succeeding, we'll visit 1, 2, 3 and 0
> > [if there are still no huge pages available].  And so forth.  I don't
> > think this is a problem because if we actually succeed in allocating or
> > freeing a page, we'll break w/o advancing next_node... and pick up there
> > on next call.
> > 
> > I have an idea for fixing this behavior, if anyone thinks it's a
> > problem, by "pushing back" the next_nid to the hstate with something
> > like:
> > 
> > 	} while (next_nid != start_nid || push_back_alloc_node(next_nid))
> > 
> > where the push_back function always returns 0.  Kind of ugly, but I
> > think it would work.  I don't think it's needed, tho'.
> > 
> 
> I don't think it's a problem that is worth worrying about. If the
> administrator is interleaving between multiple nodes, I don't see why
> they would care which one the first node used.

I agree.  It would take a 'series of unfortunate events" for this occur
such that one would notice it in the first place.  And with this
capability, one can add or remove huge pages on specific nodes.
[hugeadm arguments to do that explicitly might be nice.]

> 
<snip>

> > > > +/**
> > > > + * huge_mpol_nodes_allowed()
> > > > + *
> > > > + * Return a [pointer to a] nodelist for persistent huge page allocation
> > > > + * based on the current task's mempolicy:
> > > > + *
> > > > + * If the task's mempolicy is "default" [NULL], just return NULL for
> > > > + * default behavior.  Otherwise, extract the policy nodemask for 'bind'
> > > > + * or 'interleave' policy or construct a nodemask for 'preferred' or
> > > > + * 'local' policy and return a pointer to a kmalloc()ed nodemask_t.
> > > > + * It is the caller's responsibility to free this nodemask.
> > > > + */
> > > > +nodemask_t *huge_mpol_nodes_allowed(void)
> > > > +{
> > > > +	nodemask_t *nodes_allowed = NULL;
> > > > +	struct mempolicy *mempolicy;
> > > > +	int nid;
> > > > +
> > > > +	if (!current || !current->mempolicy)
> > > > +		return NULL;
> > > > +
> > > 
> > > Is it really possible for current to be NULL here?
> > 
> > Not sure.  I've hit NULL current in mempolicy functions called at init
> > time before.  Maybe a [VM_]BUG_ON() would be preferable?
> > 
> 
> VM_BUG_ON() I'd say would be enough. Even if support is added later to
> allocateo with a nodemask at boot-time when current == NULL, they'll need
> to know to use the default policy instead of current->mempolicy.

Will do.

<snip>
> > > 
> > > nodemasks are meant to be cleared with nodes_clear() but you depend on
> > > kzalloc() zeroing the bitmap for you. While the end result is the same,
> > > is using kzalloc instead of kmalloc+nodes_clear() considered ok?
> > 
> > That did cross my mind.  And, it's not exactly a fast path.  Shall I
> > change it to kmalloc+nodes_clear()?  Or maybe add a nodemask_alloc() or
> > nodemask_new() function?  We don't have one of those, as nodemasks have
> > mostly been passed around by value rather than reference.
> > 
> 
> I think kmalloc+nodes_clear() might be more future proof in the event
> does something like use poison byte pattern to detect when a nodemask is
> improperly initialised or something similar. I don't think it needs a
> nodemask_alloc() or nodemask_new() helper.

OK.  Will do.

> > > 
> > > > +
> > > > +	mempolicy = current->mempolicy;
> > > > +	switch(mempolicy->mode) {
> > > > +	case MPOL_PREFERRED:
> > > > +		if (mempolicy->flags & MPOL_F_LOCAL)
> > > > +			nid = numa_node_id();
> > > > +		else
> > > > +			nid = mempolicy->v.preferred_node;
> > > > +		node_set(nid, *nodes_allowed);
> > > > +		break;
> > > > +
> > > 
> > > I think a comment is needed here saying that MPOL_PREFERRED when
> > > resizing the pool acts more like MPOL_BIND to the preferred node with no
> > > fallback.
> > 
> > Yeah, I can add such a comment.  I did document this in the hugetlbfs
> > doc, but I guess it's good to have it here, as well.
> > 
> 
> It doesn't hurt.

OK.

> 
> > > 
> > > I see your problem though. You can't use set the next_nid to allocate from
> > > to be the preferred node because the second allocation will interleave away
> > > from it though. How messy would it be to check if the MPOL_PREFERRED policy
> > > was in use and avoid updating next_nid_to_alloc while the preferred node is
> > > being allocated from?
> > > 
> > > It's not a big issue and I'd be ok with your current behaviour to start with.
> > 
> > Yeah, and I don't think we want the hugetlb.c code directly looking at
> > mempolicy internals.  I'd have to add another one user helper function,
> > probably in mempolicy.h.  Not a biggy, but I didn't think it was
> > necessary.  It doesn't hurt to reiterate that allocations for populating
> > the huge page pool do not fallback.
> > 
> 
> Lets go with your suggested behaviour for now. Double checking the
> policy will just over-complicate things in the first iteration.


works for me.

<snip>
> > > 
> > > By and large, this patch would appear to result in reasonable behaviour
> > > for administrators that want to limit the hugepage pool to specific
> > > nodes. Predictably, I prefer this approach to the nodemask-sysctl
> > > approach :/ . With a few crinkles ironed out, I reckon I'd be happy with
> > > this. Certainly, it appears to work as advertised in that I was able to
> > > accurate grow/shrink the pool on specific nodes.
> > > 
> > 
> > Having written the patch, I'm beginning to warm up to this approach
> > myself :).  We need to understand and advertise any apparently change in
> > behavior to be sure others agree.
> > 
> > Thanks for the review,
> > Lee
> > 
> 

--
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: [RFC 2/3] hugetlb: derive huge pages nodes allowed from task mempolicy

[Andrew Morton]

On Tue, 30 Jun 2009 11:48:18 -0400
Lee Schermerhorn <lee.schermerhorn@hp.com> wrote:

> [RFC 2/3] hugetlb:  derive huge pages nodes allowed from task mempolicy
> 
>
> ...
>
> +/**
> + * huge_mpol_nodes_allowed()
> + *
> + * Return a [pointer to a] nodelist for persistent huge page allocation
> + * based on the current task's mempolicy:
> + *
> + * If the task's mempolicy is "default" [NULL], just return NULL for
> + * default behavior.  Otherwise, extract the policy nodemask for 'bind'
> + * or 'interleave' policy or construct a nodemask for 'preferred' or
> + * 'local' policy and return a pointer to a kmalloc()ed nodemask_t.
> + * It is the caller's responsibility to free this nodemask.
> + */

Comment purports to be kereldoc but doesn't look very kerneldoccy?

> +nodemask_t *huge_mpol_nodes_allowed(void)
> +{
> +	nodemask_t *nodes_allowed = NULL;
> +	struct mempolicy *mempolicy;
> +	int nid;
> +
> +	if (!current || !current->mempolicy)
> +		return NULL;
> +
> +	mpol_get(current->mempolicy);
> +	nodes_allowed = kzalloc(sizeof(*nodes_allowed), GFP_KERNEL);
> +	if (!nodes_allowed) {
> +		printk(KERN_WARNING "Unable to allocate nodes allowed mask "
> +			"for huge page allocation\nFalling back to default\n");

hm.  If we're going to emit a diagnostic on behalf of userspace, it
would be best if that diagnostic were to contain sufficient information
for the identification of the failing application (pid and comm, for
example).  Otherwise this mesasge would be a real head-scratcher on a
large and busy system.

> +		goto out;
> +	}
> +
> +	mempolicy = current->mempolicy;
> +	switch(mempolicy->mode) {
> +	case MPOL_PREFERRED:
> +		if (mempolicy->flags & MPOL_F_LOCAL)
> +			nid = numa_node_id();
> +		else
> +			nid = mempolicy->v.preferred_node;
> +		node_set(nid, *nodes_allowed);
> +		break;
> +
> +	case MPOL_BIND:
> +		/* Fall through */
> +	case MPOL_INTERLEAVE:
> +			*nodes_allowed =  mempolicy->v.nodes;

whitespace broke.

> +		break;
> +
> +	default:
> +		BUG();
> +	}
> +
> +out:
> +	mpol_put(current->mempolicy);
> +	return nodes_allowed;
> +}

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

[RFC 3/3] hugetlb: update hugetlb documentation for mempolicy based management.

[Lee Schermerhorn]

PATCH 3/3 hugetlb:  update hugetlb documentation for mempolicy based management.

Against:  25jun09 mmotm atop the "balance freeing of huge pages" series

This patch updates the kernel huge tlb documentation to describe the
numa memory policy based huge page management.  Additionaly, the patch
includes a fair amount of rework to improve consistency, eliminate
duplication and set the context for documenting the memory policy
interaction.

Signed-off-by: Lee Schermerhorn <lee.schermerhorn@hp.com>

 Documentation/vm/hugetlbpage.txt |  228 ++++++++++++++++++++++++---------------
 1 file changed, 143 insertions(+), 85 deletions(-)

Index: linux-2.6.31-rc1-mmotm-090625-1549/Documentation/vm/hugetlbpage.txt
===================================================================
--- linux-2.6.31-rc1-mmotm-090625-1549.orig/Documentation/vm/hugetlbpage.txt	2009-06-30 11:25:38.000000000 -0400
+++ linux-2.6.31-rc1-mmotm-090625-1549/Documentation/vm/hugetlbpage.txt	2009-06-30 11:50:28.000000000 -0400
@@ -11,23 +11,21 @@ This optimization is more critical now a
 (several GBs) are more readily available.
 
 Users can use the huge page support in Linux kernel by either using the mmap
-system call or standard SYSv shared memory system calls (shmget, shmat).
+system call or standard SYSV shared memory system calls (shmget, shmat).
 
 First the Linux kernel needs to be built with the CONFIG_HUGETLBFS
 (present under "File systems") and CONFIG_HUGETLB_PAGE (selected
 automatically when CONFIG_HUGETLBFS is selected) configuration
 options.
 
-The kernel built with huge page support should show the number of configured
-huge pages in the system by running the "cat /proc/meminfo" command.
+The /proc/meminfo file provides information about the total number of hugetlb
+pages preallocated in the kernel's huge page pool.  It also displays
+information about the number of free, reserved and surplus huge pages and the
+[default] huge page size.  The huge page size is needed for generating the
+proper alignment and size of the arguments to system calls that map huge page
+regions.
 
-/proc/meminfo also provides information about the total number of hugetlb
-pages configured in the kernel.  It also displays information about the
-number of free hugetlb pages at any time.  It also displays information about
-the configured huge page size - this is needed for generating the proper
-alignment and size of the arguments to the above system calls.
-
-The output of "cat /proc/meminfo" will have lines like:
+The output of "cat /proc/meminfo" will include lines like:
 
 .....
 HugePages_Total: vvv
@@ -53,26 +51,25 @@ HugePages_Surp  is short for "surplus," 
 /proc/filesystems should also show a filesystem of type "hugetlbfs" configured
 in the kernel.
 
-/proc/sys/vm/nr_hugepages indicates the current number of configured hugetlb
-pages in the kernel.  Super user can dynamically request more (or free some
-pre-configured) huge pages.
-The allocation (or deallocation) of hugetlb pages is possible only if there are
-enough physically contiguous free pages in system (freeing of huge pages is
-possible only if there are enough hugetlb pages free that can be transferred
-back to regular memory pool).
-
-Pages that are used as hugetlb pages are reserved inside the kernel and cannot
-be used for other purposes.
-
-Once the kernel with Hugetlb page support is built and running, a user can
-use either the mmap system call or shared memory system calls to start using
-the huge pages.  It is required that the system administrator preallocate
-enough memory for huge page purposes.
-
-The administrator can preallocate huge pages on the kernel boot command line by
-specifying the "hugepages=N" parameter, where 'N' = the number of huge pages
-requested.  This is the most reliable method for preallocating huge pages as
-memory has not yet become fragmented.
+/proc/sys/vm/nr_hugepages indicates the current number of huge pages pre-
+allocated in the kernel's huge page pool.  These are called "persistent"
+huge pages.  A user with root privileges can dynamically allocate more or
+free some persistent huge pages by increasing or decreasing the value of
+'nr_hugepages'.
+
+Pages that are used as huge pages are reserved inside the kernel and cannot
+be used for other purposes.  Huge pages can not be swapped out under
+memory pressure.
+
+Once a number of huge pages have been pre-allocated to the kernel huge page
+pool, a user with appropriate privilege can use either the mmap system call
+or shared memory system calls to use the huge pages.  See the discussion of
+Using Huge Pages, below
+
+The administrator can preallocate persistent huge pages on the kernel boot
+command line by specifying the "hugepages=N" parameter, where 'N' = the
+number of requested huge pages requested.  This is the most reliable method
+or preallocating huge pages as memory has not yet become fragmented.
 
 Some platforms support multiple huge page sizes.  To preallocate huge pages
 of a specific size, one must preceed the huge pages boot command parameters
@@ -80,19 +77,23 @@ with a huge page size selection paramete
 be specified in bytes with optional scale suffix [kKmMgG].  The default huge
 page size may be selected with the "default_hugepagesz=<size>" boot parameter.
 
-/proc/sys/vm/nr_hugepages indicates the current number of configured [default
-size] hugetlb pages in the kernel.  Super user can dynamically request more
-(or free some pre-configured) huge pages.
-
-Use the following command to dynamically allocate/deallocate default sized
-huge pages:
+When multiple huge page sizes are supported, /proc/sys/vm/nr_hugepages
+indicates the current number of pre-allocated huge pages of the default size.
+Thus, one can use the following command to dynamically allocate/deallocate
+default sized persistent huge pages:
 
 	echo 20 > /proc/sys/vm/nr_hugepages
 
-This command will try to configure 20 default sized huge pages in the system.
+This command will try to adjust the number of default sized huge pages in the
+huge page pool to 20, allocating or freeing huge pages, as required.
+
 On a NUMA platform, the kernel will attempt to distribute the huge page pool
-over the all on-line nodes.  These huge pages, allocated when nr_hugepages
-is increased, are called "persistent huge pages".
+over the all the nodes specified by the NUMA memory policy of the task that
+modifies nr_hugepages that contain sufficient available contiguous memory.
+These nodes are called the huge pages "allowed nodes".  The default for the
+huge pages allowed nodes--when the task has default memory policy--is all
+on-line nodes.  See the discussion below of the interaction of task memory
+policy, cpusets and persistent huge page allocation and freeing.
 
 The success or failure of huge page allocation depends on the amount of
 physically contiguous memory that is preset in system at the time of the
@@ -101,11 +102,11 @@ some nodes in a NUMA system, it will att
 allocating extra pages on other nodes with sufficient available contiguous
 memory, if any.
 
-System administrators may want to put this command in one of the local rc init
-files.  This will enable the kernel to request huge pages early in the boot
-process when the possibility of getting physical contiguous pages is still
-very high.  Administrators can verify the number of huge pages actually
-allocated by checking the sysctl or meminfo.  To check the per node
+System administrators may want to put this command in one of the local rc
+init files.  This will enable the kernel to preallocate huge pages early in
+the boot process when the possibility of getting physical contiguous pages
+is still very high.  Administrators can verify the number of huge pages
+actually allocated by checking the sysctl or meminfo.  To check the per node
 distribution of huge pages in a NUMA system, use:
 
 	cat /sys/devices/system/node/node*/meminfo | fgrep Huge
@@ -113,39 +114,40 @@ distribution of huge pages in a NUMA sys
 /proc/sys/vm/nr_overcommit_hugepages specifies how large the pool of
 huge pages can grow, if more huge pages than /proc/sys/vm/nr_hugepages are
 requested by applications.  Writing any non-zero value into this file
-indicates that the hugetlb subsystem is allowed to try to obtain "surplus"
-huge pages from the buddy allocator, when the normal pool is exhausted. As
-these surplus huge pages go out of use, they are freed back to the buddy
-allocator.
+indicates that the hugetlb subsystem is allowed to try to obtain that
+number of "surplus" huge pages from the kernel's normal page pool, when the
+persistent huge page pool is exhausted. As these surplus huge pages become
+unused, they are freed back to the kernel's normal page pool.
 
-When increasing the huge page pool size via nr_hugepages, any surplus
+When increasing the huge page pool size via nr_hugepages, any existing surplus
 pages will first be promoted to persistent huge pages.  Then, additional
 huge pages will be allocated, if necessary and if possible, to fulfill
-the new huge page pool size.
+the new persistent huge page pool size.
 
 The administrator may shrink the pool of preallocated huge pages for
 the default huge page size by setting the nr_hugepages sysctl to a
 smaller value.  The kernel will attempt to balance the freeing of huge pages
-across all on-line nodes.  Any free huge pages on the selected nodes will
-be freed back to the buddy allocator.
-
-Caveat: Shrinking the pool via nr_hugepages such that it becomes less
-than the number of huge pages in use will convert the balance to surplus
-huge pages even if it would exceed the overcommit value.  As long as
-this condition holds, however, no more surplus huge pages will be
-allowed on the system until one of the two sysctls are increased
-sufficiently, or the surplus huge pages go out of use and are freed.
+across all nodes in the memory policy of the task modifying nr_hugepages.
+Any free huge pages on the selected nodes will be freed back to the kernel's
+normal page pool.
+
+Caveat: Shrinking the persistent huge page pool via nr_hugepages such that
+it becomes less than the number of huge pages in use will convert the balance
+of the in-use huge pages to surplus huge pages.  This will occur even if
+the number of surplus pages it would exceed the overcommit value.  As long as
+this condition holds--that is, until nr_hugepages+nr_overcommit_hugepages is
+increased sufficiently, or the surplus huge pages go out of use and are freed--
+no more surplus huge pages will be allowed to be allocated.
 
 With support for multiple huge page pools at run-time available, much of
-the huge page userspace interface has been duplicated in sysfs. The above
-information applies to the default huge page size which will be
-controlled by the /proc interfaces for backwards compatibility. The root
-huge page control directory in sysfs is:
+the huge page userspace interface in /proc/sys/vm has been duplicated in sysfs.
+The /proc interfaces discussed above have been retained for backwards
+compatibility. The root huge page control directory in sysfs is:
 
 	/sys/kernel/mm/hugepages
 
 For each huge page size supported by the running kernel, a subdirectory
-will exist, of the form
+will exist, of the form:
 
 	hugepages-${size}kB
 
@@ -159,6 +161,70 @@ Inside each of these directories, the sa
 
 which function as described above for the default huge page-sized case.
 
+
+Interaction of Task Memory Policy with Huge Page Allocation/Freeing:
+
+Whether huge pages are allocated and freed via the /proc interface or
+the /sysfs interface, the NUMA nodes from which huge pages are allocated
+or freed are controlled by the NUMA memory policy of the task that modifies
+the nr_hugepages parameter.  [nr_overcommit_hugepages is a global limit.]
+
+The recommended method to allocate or free huge pages to/from the kernel
+huge page pool, using the nr_hugepages example above, is:
+
+    numactl --interleave <node-list> echo 20 >/proc/sys/vm/nr_hugepages.
+
+or, more succinctly:
+
+    numactl -m <node-list> echo 20 >/proc/sys/vm/nr_hugepages.
+
+This will allocate or free abs(20 - nr_hugepages) to or from the nodes
+specified in <node-list>, depending on whether nr_hugepages is initially
+less than or greater than 20, respectively.  No huge pages will be
+allocated nor freed on any node not included in the specified <node-list>.
+
+Any memory policy mode--bind, preferred, local or interleave--may be
+used.  The effect on persistent huge page allocation will be as follows:
+
+1) Regardless of mempolicy mode [see Documentation/vm/numa_memory_policy.txt],
+   persistent huge pages will be distributed across the node or nodes
+   specified in the mempolicy as if "interleave" had been specified.
+   However, if a node in the policy does not contain sufficient contiguous
+   memory for a huge page, the allocation will not "fallback" to the nearest
+   neighbor node with sufficient contiguous memory.  To do this would cause
+   undesirable imbalance in the distribution of the huge page pool, or
+   possibly, allocation of persistent huge pages on nodes not allowed by
+   the task's memory policy.
+
+2) One or more nodes may be specified with the bind or interleave policy.
+   If more than one node is specified with the preferred policy, only the
+   lowest numeric id will be used.  Local policy will select the node where
+   the task is running at the time the nodes_allowed mask is constructed.
+
+3) For local policy to be deterministic, the task must be bound to a cpu or
+   cpus in a single node.  Otherwise, the task could be migrated to some
+   other node at any time after launch and the resulting node will be
+   indeterminate.  Thus, local policy is not very useful for this purpose.
+   Any of the other mempolicy modes may be used to specify a single node.
+
+4) The nodes allowed mask will be derived from any non-default task mempolicy,
+   whether this policy was set explicitly by the task itself or one of its
+   ancestors, such as numactl.  This means that if the task is invoked from a
+   shell with non-default policy, that policy will be used.  One can specify a
+   node list of "all" with numactl --interleave or --membind [-m] to achieve
+   interleaving over all nodes in the system or cpuset.
+
+5) Any task mempolicy specifed--e.g., using numactl--will be constrained by
+   the resource limits of any cpuset in which the task runs.  Thus, there will
+   be no way for a task with non-default policy running in a cpuset with a
+   subset of the system nodes to allocate huge pages outside the cpuset
+   without first moving to a cpuset that contains all of the desired nodes.
+
+6) Hugepages allocated at boot time always use the node_online_map.
+
+
+Using Huge Pages:
+
 If the user applications are going to request huge pages using mmap system
 call, then it is required that system administrator mount a file system of
 type hugetlbfs:
@@ -204,9 +270,11 @@ mount of filesystem will be required for
  * requesting huge pages.
  *
  * For the ia64 architecture, the Linux kernel reserves Region number 4 for
- * huge pages.  That means the addresses starting with 0x800000... will need
- * to be specified.  Specifying a fixed address is not required on ppc64,
- * i386 or x86_64.
+ * huge pages.  That means that if one requires a fixed address, a huge page
+ * aligned address starting with 0x800000... will be required.  If a fixed
+ * address is not required, the kernel will select an address in the proper
+ * range.
+ * Other architectures, such as ppc64, i386 or x86_64 are not so constrained.
  *
  * Note: The default shared memory limit is quite low on many kernels,
  * you may need to increase it via:
@@ -235,14 +303,8 @@ mount of filesystem will be required for
 
 #define dprintf(x)  printf(x)
 
-/* Only ia64 requires this */
-#ifdef __ia64__
-#define ADDR (void *)(0x8000000000000000UL)
-#define SHMAT_FLAGS (SHM_RND)
-#else
-#define ADDR (void *)(0x0UL)
+#define ADDR (void *)(0x0UL)	/* let kernel choose address */
 #define SHMAT_FLAGS (0)
-#endif
 
 int main(void)
 {
@@ -300,10 +362,12 @@ int main(void)
  * example, the app is requesting memory of size 256MB that is backed by
  * huge pages.
  *
- * For ia64 architecture, Linux kernel reserves Region number 4 for huge pages.
- * That means the addresses starting with 0x800000... will need to be
- * specified.  Specifying a fixed address is not required on ppc64, i386
- * or x86_64.
+ * For the ia64 architecture, the Linux kernel reserves Region number 4 for
+ * huge pages.  That means that if one requires a fixed address, a huge page
+ * aligned address starting with 0x800000... will be required.  If a fixed
+ * address is not required, the kernel will select an address in the proper
+ * range.
+ * Other architectures, such as ppc64, i386 or x86_64 are not so constrained.
  */
 #include <stdlib.h>
 #include <stdio.h>
@@ -315,14 +379,8 @@ int main(void)
 #define LENGTH (256UL*1024*1024)
 #define PROTECTION (PROT_READ | PROT_WRITE)
 
-/* Only ia64 requires this */
-#ifdef __ia64__
-#define ADDR (void *)(0x8000000000000000UL)
-#define FLAGS (MAP_SHARED | MAP_FIXED)
-#else
-#define ADDR (void *)(0x0UL)
+#define ADDR (void *)(0x0UL)	/* let kernel choose address */
 #define FLAGS (MAP_SHARED)
-#endif
 
 void check_bytes(char *addr)
 {

--
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: [RFC 0/3] hugetlb: constrain allocation/free based on task mempolicy

[Lee Schermerhorn]

On Tue, 2009-06-30 at 11:47 -0400, Lee Schermerhorn wrote: 
> RFC 0/3 hugetlb: constrain allocation/free based on task mempolicy
> 
> Against:  25jun09 mmotm atop the "hugetlb:  balance freeing..."
> series
> 
> This is V1 of a series of patches to constrain the allocation and
> freeing of persistent huge pages using the task NUMA mempolicy of
> the task modifying "nr_hugepages".  This series is based on Mel
> Gorman's suggestion to use task mempolicy.
> 
> I have some concerns about a subtle change in behavior [see patch
> 2/3 and the updated documentation] and the fact that
> this mechanism ignores some of the semantics of the mempolicy
> mode [again, see the doc].   However, this method seems to work
> fairly well.  And, IMO, the resulting code doesn't look all that
> bad.
> 
> A couple of limitations in this version:
> 
> 1) I haven't implemented a boot time parameter to constrain the
>    boot time allocation of huge pages.  This can be added if
>    anyone feels strongly that it is required.
> 
> 2) I have not implemented a per node nr_overcommit_hugepages as
>    David Rientjes and I discussed earlier.  Again, this can be
>    added and specific nodes can be addressed using the mempolicy
>    as this series does for allocation and free.
> 

I have tested this series atop the 25jun mmotm, based on .31-rc1, and
the "hugetlb: balance freeing..." series using the libhugetlbfs 2.5 test
suite and instructions from Mel Gorman on how to run it:

./obj/hugeadm --pool-pages-min 2M:64
./obj/hugeadm --create-global-mounts
make func >Log 2>&1 ...

With default mempolicy, the tests complete without error on a 4-node, 8
core x86_64 system w/ 8G/node:

********** TEST SUMMARY
*                      2M
*                      32-bit 64-bit
*     Total testcases:    90     93
*             Skipped:     0      0
*                PASS:    90     93
*                FAIL:     0      0
*    Killed by signal:     0      0
*   Bad configuration:     0      0
*       Expected FAIL:     0      0
*     Unexpected PASS:     0      0
* Strange test result:     0      0
**********

Next, I tried to run the test on just nodes 2 and 3 with the same
hugepage setup as above--64 pages across all 4 nodes:

numactl -m 2,3 make func

This resulted in LOTS of OOM kills of innocent bystander tasks.  I
thought this was because the tests would only have access to 1/2 of the
64 pre-allocated huge pages--those on nodes 2 & 3.  So, I increased the
number of preallocated pages to 256 with default mempolicy, resulting in
64 huge pages per node.  This would give the tests 128 huge pages.  More
than enough, I thought.

However, I still saw OOM kills [but no dumps of the memory state]:

Out of memory: kill process 5225 (httpd) score 59046 or a child
Killed process 5225 (httpd)
Out of memory: kill process 5226 (httpd) score 59046 or a child
Killed process 5226 (httpd)
Out of memory: kill process 5227 (httpd) score 59046 or a child
Killed process 5227 (httpd)
Out of memory: kill process 5228 (httpd) score 59046 or a child
Killed process 5228 (httpd)
Out of memory: kill process 5229 (httpd) score 59046 or a child
Killed process 5229 (httpd)
Out of memory: kill process 5230 (httpd) score 59046 or a child
Killed process 5230 (httpd)
Out of memory: kill process 5828 (alloc-instantia) score 8188 or a child
Killed process 5828 (alloc-instantia)
Out of memory: kill process 5829 (alloc-instantia) score 8349 or a child
Killed process 5829 (alloc-instantia)
Out of memory: kill process 5830 (alloc-instantia) score 8188 or a child
Killed process 5830 (alloc-instantia)
Out of memory: kill process 5831 (alloc-instantia) score 8349 or a child
Killed process 5831 (alloc-instantia)
Out of memory: kill process 5834 (truncate_sigbus) score 8252 or a child
Killed process 5834 (truncate_sigbus)
Out of memory: kill process 5835 (truncate_sigbus) score 8413 or a child
Killed process 5835 (truncate_sigbus)

And 3 of the tests complained about unexpected huge page count--e.g,
expected 0, saw 128.  The '128' huge pages are those on nodes 0 and 1
that the tests couldn't manipulate because of the mempolicy constraints.
It turns out that the libhugetlbfs tests assume they have access to the
entire system and use the global counters from /proc/meminfo
and /sys/kernel/mm/hugepages/* to size the tests and set expectations.
When constrained by mempolicy, these assumptions break down.  I've seen
this behavior in other test suites--e.g., trying to run the numactl
regression tests in a cpuset.

So, I emptied the huge page pool by setting nr_hugepages to 0, and
populated the huge page pool from only the nodes I was going to use in
the tests:

numactl -m 2,3 ./obj/hugeadm --pool-pages-min 2M:64

Then, rerun the tests constrained to nodes 2 and 3:

numactl -m 2,3 make func

This time the tests ran to completion with no OOM kills and no errors.

So, this series doesn't actually break the hugetlb functionality, but
running the test suite under a constrained mempolicy does break its
assumptions.

Perhaps, libhugetlbfs functions like get_huge_page_counter() should be
enhanced, or a numa-aware version provided, to return the sum of the per
node values for the nodes allowed by the calling task's mempolicy,
rather than the system-wide count?

Cpuset interaction:

I created a test cpuset with nodes/mems 2,3.  With my shell in that
cpuset:

./obj/hugeadm --pool-pages-min 2M:64

[i.e., with default mempolicy] still distributes the 64 huge pages
across all 4 nodes, as cpuset mems_allowed does not constrain "fresh"
huge page allocation and default mempolicy translates to all on-line
nodes allowed.  However,

numactl -m all ./obj/hugeadm --pool-pages-min 2M:64

results in 32 huge pages on each of nodes 2 and 3 because the memory
policy installed by numactl IS constrained by the cpuset mems_allowed.
Then,

numactl -m 2,3 make func

from within the test cpuset [nodes 2,3], results in:
********** TEST SUMMARY
*                      2M
*                      32-bit 64-bit
*     Total testcases:    90     93
*             Skipped:     0      0
*                PASS:    87     90
*                FAIL:     1      1
*    Killed by signal:     0      0
*   Bad configuration:     2      2
*       Expected FAIL:     0      0
*     Unexpected PASS:     0      0
* Strange test result:     0      0
**********

The "Bad configuration"s are the result of sched_setaffinity() failing
because of cpuset constraints.  Again, the tests are not set up to work
in a cpuset/mempolicy constrained environment.  The failures are the
result of a child of "alloc-instantiate-race" being killed by a signal
[segfault?] in both 32 and 64 bit modes.

Running the tests in the cpuset with default mempolicy results in a
similar, but different set of failures because the tests free and
reallocate the huge pages and they end up spread across all nodes again.

All of these failures can be attributed to the tests, and perhaps
libhugetlbfs, not considering the effects of running in a constrained
environment.

-------------------

I suspected that some of these errors would occur on a kernel without
this patch set.  Indeed, under 2.6.31-rc1 with mmotm of 25jun only,

numactl -m 2,3 make func

results in OOM kills similar to those listed above, as well as
"strageness" and one failure:

counters.sh (2M: 32):   FAIL Line 338: Bad HugePages_Total: expected 1, actual 2
counters.sh (2M: 64):   FAIL Line 326: Bad HugePages_Total: expected 0, actual 1
********** TEST SUMMARY
*                      2M
*                      32-bit 64-bit
*     Total testcases:    90     93
*             Skipped:     0      0
*                PASS:    86     89
*                FAIL:     1      1
*    Killed by signal:     0      0
*   Bad configuration:     0      0
*       Expected FAIL:     0      0
*     Unexpected PASS:     0      0
* Strange test result:     3      3
**********

Running under the test cpuset [nodes 2 and 3 out of 0-3] with default
mempolicy yields:

chunk-overcommit (2M: 32):      FAIL    mmap() chunk1: Cannot allocate memory
chunk-overcommit (2M: 64):      FAIL    mmap() chunk1: Cannot allocate memory
alloc-instantiate-race shared (2M: 32): FAIL    mmap() 1: Cannot allocate memory
alloc-instantiate-race shared (2M: 64): FAIL    mmap() 1: Cannot allocate memory
alloc-instantiate-race private (2M: 32):        FAIL    mmap() 1: Cannot allocate memory
alloc-instantiate-race private (2M: 64):        FAIL    mmap() 1: Cannot allocate memory
truncate_sigbus_versus_oom (2M: 32):    FAIL    mmap() reserving all pages: Cannot allocate memory
truncate_sigbus_versus_oom (2M: 64):    FAIL    mmap() reserving all pages: Cannot allocate memory

counters.sh (2M: 32):   FAIL Line 326: Bad HugePages_Total: expected 0, actual 1
counters.sh (2M: 64):   FAIL Line 326: Bad HugePages_Total: expected 0, actual 1
********** TEST SUMMARY
*                      2M
*                      32-bit 64-bit
*     Total testcases:    90     93
*             Skipped:     0      0
*                PASS:    84     87
*                FAIL:     5      5
*    Killed by signal:     0      0
*   Bad configuration:     1      1
*       Expected FAIL:     0      0
*     Unexpected PASS:     0      0
* Strange test result:     0      0
**********

Running in the cpuset, under numactl -m 2,3 yields the same results.

So, independent of the "hugetlb:  constrained by mempolicy" patches, the
libhugetlbfs test suite doesn't deal well with cpuset and memory policy
constraints.  In fact, we seem to have fewer failures with this patch
set.






--
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: [RFC 0/3] hugetlb: constrain allocation/free based on task mempolicy

[Mel Gorman]

On Wed, Jul 01, 2009 at 01:28:23PM -0400, Lee Schermerhorn wrote:
> On Tue, 2009-06-30 at 11:47 -0400, Lee Schermerhorn wrote: 
> > RFC 0/3 hugetlb: constrain allocation/free based on task mempolicy
> > 
> > Against:  25jun09 mmotm atop the "hugetlb:  balance freeing..."
> > series
> > 
> > This is V1 of a series of patches to constrain the allocation and
> > freeing of persistent huge pages using the task NUMA mempolicy of
> > the task modifying "nr_hugepages".  This series is based on Mel
> > Gorman's suggestion to use task mempolicy.
> > 
> > I have some concerns about a subtle change in behavior [see patch
> > 2/3 and the updated documentation] and the fact that
> > this mechanism ignores some of the semantics of the mempolicy
> > mode [again, see the doc].   However, this method seems to work
> > fairly well.  And, IMO, the resulting code doesn't look all that
> > bad.
> > 
> > A couple of limitations in this version:
> > 
> > 1) I haven't implemented a boot time parameter to constrain the
> >    boot time allocation of huge pages.  This can be added if
> >    anyone feels strongly that it is required.
> > 
> > 2) I have not implemented a per node nr_overcommit_hugepages as
> >    David Rientjes and I discussed earlier.  Again, this can be
> >    added and specific nodes can be addressed using the mempolicy
> >    as this series does for allocation and free.
> > 
> 
> I have tested this series atop the 25jun mmotm, based on .31-rc1, and
> the "hugetlb: balance freeing..." series using the libhugetlbfs 2.5 test
> suite and instructions from Mel Gorman on how to run it:
> 
> ./obj/hugeadm --pool-pages-min 2M:64
> ./obj/hugeadm --create-global-mounts
> make func >Log 2>&1 ...
> 
> With default mempolicy, the tests complete without error on a 4-node, 8
> core x86_64 system w/ 8G/node:
> 
> ********** TEST SUMMARY
> *                      2M
> *                      32-bit 64-bit
> *     Total testcases:    90     93
> *             Skipped:     0      0
> *                PASS:    90     93
> *                FAIL:     0      0
> *    Killed by signal:     0      0
> *   Bad configuration:     0      0
> *       Expected FAIL:     0      0
> *     Unexpected PASS:     0      0
> * Strange test result:     0      0
> **********
> 
> Next, I tried to run the test on just nodes 2 and 3 with the same
> hugepage setup as above--64 pages across all 4 nodes:
> 
> numactl -m 2,3 make func
> 
> This resulted in LOTS of OOM kills of innocent bystander tasks.  I
> thought this was because the tests would only have access to 1/2 of the
> 64 pre-allocated huge pages--those on nodes 2 & 3.  So, I increased the
> number of preallocated pages to 256 with default mempolicy, resulting in
> 64 huge pages per node.  This would give the tests 128 huge pages.  More
> than enough, I thought.
> 
> However, I still saw OOM kills [but no dumps of the memory state]:
> 

hmm, it's possible the problem is with hugepage reservations are made.
There is a hstate-wide resv_huge_pages counter that is used to determine
if an mmap() should succeed or not. Once mmap() succeeds, the
expectation is that hugepages exist on the free lists sufficient to
satisfy future faults.

Now, lets say you had hugepages on each of the 4 nodes but were bound to
just nodes 2,3. The reservation code would make a calculation based on 4
nodes being avilable, allow the mmap() to succeed and later fail an
allocation. This would cause VM_FAULT_OOM to be returned and trigger the
killer.

> Out of memory: kill process 5225 (httpd) score 59046 or a child
> Killed process 5225 (httpd)
> Out of memory: kill process 5226 (httpd) score 59046 or a child
> Killed process 5226 (httpd)
> Out of memory: kill process 5227 (httpd) score 59046 or a child
> Killed process 5227 (httpd)
> Out of memory: kill process 5228 (httpd) score 59046 or a child
> Killed process 5228 (httpd)
> Out of memory: kill process 5229 (httpd) score 59046 or a child
> Killed process 5229 (httpd)
> Out of memory: kill process 5230 (httpd) score 59046 or a child
> Killed process 5230 (httpd)
> Out of memory: kill process 5828 (alloc-instantia) score 8188 or a child
> Killed process 5828 (alloc-instantia)
> Out of memory: kill process 5829 (alloc-instantia) score 8349 or a child
> Killed process 5829 (alloc-instantia)
> Out of memory: kill process 5830 (alloc-instantia) score 8188 or a child
> Killed process 5830 (alloc-instantia)
> Out of memory: kill process 5831 (alloc-instantia) score 8349 or a child
> Killed process 5831 (alloc-instantia)
> Out of memory: kill process 5834 (truncate_sigbus) score 8252 or a child
> Killed process 5834 (truncate_sigbus)
> Out of memory: kill process 5835 (truncate_sigbus) score 8413 or a child
> Killed process 5835 (truncate_sigbus)
> 
> And 3 of the tests complained about unexpected huge page count--e.g,
> expected 0, saw 128.  The '128' huge pages are those on nodes 0 and 1
> that the tests couldn't manipulate because of the mempolicy constraints.
> It turns out that the libhugetlbfs tests assume they have access to the
> entire system and use the global counters from /proc/meminfo
> and /sys/kernel/mm/hugepages/* to size the tests and set expectations.
> When constrained by mempolicy, these assumptions break down.  I've seen
> this behavior in other test suites--e.g., trying to run the numactl
> regression tests in a cpuset.
> 

Ok, when all of this is ironed out, libhugetlbfs's regression tests will
need a few more smarts.

> So, I emptied the huge page pool by setting nr_hugepages to 0, and
> populated the huge page pool from only the nodes I was going to use in
> the tests:
> 
> numactl -m 2,3 ./obj/hugeadm --pool-pages-min 2M:64
> 
> Then, rerun the tests constrained to nodes 2 and 3:
> 
> numactl -m 2,3 make func
> 
> This time the tests ran to completion with no OOM kills and no errors.
> 

This fits the theory that it's the reservation counters that are the
problem. The top-most path for making reservations is
hugetlb_reserve_pages()

> So, this series doesn't actually break the hugetlb functionality, but
> running the test suite under a constrained mempolicy does break its
> assumptions.
> 
> Perhaps, libhugetlbfs functions like get_huge_page_counter() should be
> enhanced, or a numa-aware version provided, to return the sum of the per
> node values for the nodes allowed by the calling task's mempolicy,
> rather than the system-wide count?
> 

I think the OOM problem is an in-kernel problem. Userspace depends on
mmap() failing when there are insufficient hugepages to guarantee future
faults will succeed.

> Cpuset interaction:
> 
> I created a test cpuset with nodes/mems 2,3.  With my shell in that
> cpuset:
> 
> ./obj/hugeadm --pool-pages-min 2M:64
> 
> [i.e., with default mempolicy] still distributes the 64 huge pages
> across all 4 nodes, as cpuset mems_allowed does not constrain "fresh"
> huge page allocation and default mempolicy translates to all on-line
> nodes allowed.  However,
> 
> numactl -m all ./obj/hugeadm --pool-pages-min 2M:64
> 
> results in 32 huge pages on each of nodes 2 and 3 because the memory
> policy installed by numactl IS constrained by the cpuset mems_allowed.
> Then,
> 
> numactl -m 2,3 make func
> 
> from within the test cpuset [nodes 2,3], results in:
> ********** TEST SUMMARY
> *                      2M
> *                      32-bit 64-bit
> *     Total testcases:    90     93
> *             Skipped:     0      0
> *                PASS:    87     90
> *                FAIL:     1      1
> *    Killed by signal:     0      0
> *   Bad configuration:     2      2
> *       Expected FAIL:     0      0
> *     Unexpected PASS:     0      0
> * Strange test result:     0      0
> **********
> 
> The "Bad configuration"s are the result of sched_setaffinity() failing
> because of cpuset constraints.  Again, the tests are not set up to work
> in a cpuset/mempolicy constrained environment.  The failures are the
> result of a child of "alloc-instantiate-race" being killed by a signal
> [segfault?] in both 32 and 64 bit modes.
> 
> Running the tests in the cpuset with default mempolicy results in a
> similar, but different set of failures because the tests free and
> reallocate the huge pages and they end up spread across all nodes again.
> 
> All of these failures can be attributed to the tests, and perhaps
> libhugetlbfs, not considering the effects of running in a constrained
> environment.
> 

The tests shouldn't be able to trigger a kernel failure or OOM although
they might do stupid setups based on reading counters directly. I think
the underlying problem is that the reservation code within hugetlbfs is
assuming it's not in a constrained environment, be it due to cpusets or
memory policies.

> -------------------
> 
> I suspected that some of these errors would occur on a kernel without
> this patch set.  Indeed, under 2.6.31-rc1 with mmotm of 25jun only,
> 
> numactl -m 2,3 make func
> 
> results in OOM kills similar to those listed above, as well as
> "strageness" and one failure:
> 
> counters.sh (2M: 32):   FAIL Line 338: Bad HugePages_Total: expected 1, actual 2
> counters.sh (2M: 64):   FAIL Line 326: Bad HugePages_Total: expected 0, actual 1
> ********** TEST SUMMARY
> *                      2M
> *                      32-bit 64-bit
> *     Total testcases:    90     93
> *             Skipped:     0      0
> *                PASS:    86     89
> *                FAIL:     1      1
> *    Killed by signal:     0      0
> *   Bad configuration:     0      0
> *       Expected FAIL:     0      0
> *     Unexpected PASS:     0      0
> * Strange test result:     3      3
> **********
> 
> Running under the test cpuset [nodes 2 and 3 out of 0-3] with default
> mempolicy yields:
> 
> chunk-overcommit (2M: 32):      FAIL    mmap() chunk1: Cannot allocate memory
> chunk-overcommit (2M: 64):      FAIL    mmap() chunk1: Cannot allocate memory
> alloc-instantiate-race shared (2M: 32): FAIL    mmap() 1: Cannot allocate memory
> alloc-instantiate-race shared (2M: 64): FAIL    mmap() 1: Cannot allocate memory
> alloc-instantiate-race private (2M: 32):        FAIL    mmap() 1: Cannot allocate memory
> alloc-instantiate-race private (2M: 64):        FAIL    mmap() 1: Cannot allocate memory
> truncate_sigbus_versus_oom (2M: 32):    FAIL    mmap() reserving all pages: Cannot allocate memory
> truncate_sigbus_versus_oom (2M: 64):    FAIL    mmap() reserving all pages: Cannot allocate memory
> 
> counters.sh (2M: 32):   FAIL Line 326: Bad HugePages_Total: expected 0, actual 1
> counters.sh (2M: 64):   FAIL Line 326: Bad HugePages_Total: expected 0, actual 1
> ********** TEST SUMMARY
> *                      2M
> *                      32-bit 64-bit
> *     Total testcases:    90     93
> *             Skipped:     0      0
> *                PASS:    84     87
> *                FAIL:     5      5
> *    Killed by signal:     0      0
> *   Bad configuration:     1      1
> *       Expected FAIL:     0      0
> *     Unexpected PASS:     0      0
> * Strange test result:     0      0
> **********
> 
> Running in the cpuset, under numactl -m 2,3 yields the same results.
> 
> So, independent of the "hugetlb:  constrained by mempolicy" patches, the
> libhugetlbfs test suite doesn't deal well with cpuset and memory policy
> constraints.  In fact, we seem to have fewer failures with this patch
> set.
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

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