Re: [PATCH 3/5] Use per hstate nodes_allowed to constrain huge page allocation

[Mel Gorman <mel@csn.ul.ie>]

On Wed, Jun 17, 2009 at 01:47:03PM -0400, Lee Schermerhorn wrote:
> On Wed, 2009-06-17 at 14:39 +0100, Mel Gorman wrote:
> > On Tue, Jun 16, 2009 at 09:53:01AM -0400, Lee Schermerhorn wrote:
> > > [PATCH 3/5] Use per hstate nodes_allowed to constrain huge page allocation
> > > 
> > > Against:  17may09 mmotm
> > > 
> > > Select only nodes from the per hstate nodes_allowed mask when
> > > promoting surplus pages to persistent or when allocating fresh
> > > huge pages to the pool.
> > > 
> > > Note that alloc_buddy_huge_page() still uses task policy to allocate
> > > surplus huge pages.  This could be changed.
> > > 
> > > Signed-off-by: Lee Schermerhorn <lee.schermerhorn@hp.com>
> > > 
> > >  mm/hugetlb.c |   23 ++++++++++++++++++-----
> > >  1 file changed, 18 insertions(+), 5 deletions(-)
> > > 
> > > Index: linux-2.6.30-rc8-mmotm-090603-1633/mm/hugetlb.c
> > > ===================================================================
> > > --- linux-2.6.30-rc8-mmotm-090603-1633.orig/mm/hugetlb.c	2009-06-04 12:59:32.000000000 -0400
> > > +++ linux-2.6.30-rc8-mmotm-090603-1633/mm/hugetlb.c	2009-06-04 12:59:33.000000000 -0400
> > > @@ -637,9 +637,9 @@ static struct page *alloc_fresh_huge_pag
> > >  static int hstate_next_node(struct hstate *h)
> > >  {
> > >  	int next_nid;
> > > -	next_nid = next_node(h->hugetlb_next_nid, node_online_map);
> > > +	next_nid = next_node(h->hugetlb_next_nid, *h->nodes_allowed);
> > >  	if (next_nid == MAX_NUMNODES)
> > > -		next_nid = first_node(node_online_map);
> > > +		next_nid = first_node(*h->nodes_allowed);
> > >  	h->hugetlb_next_nid = next_nid;
> > >  	return next_nid;
> > >  }
> > > @@ -652,6 +652,11 @@ static int alloc_fresh_huge_page(struct 
> > >  	int ret = 0;
> > >  
> > >  	start_nid = h->hugetlb_next_nid;
> > > +	/*
> > > +	 * we may have allocated with a different nodes_allowed previously
> > > +	 */
> > > +	if (!node_isset(start_nid, *h->nodes_allowed))
> > > +		start_nid = hstate_next_node(h);
> > >  
> > >  	do {
> > >  		page = alloc_fresh_huge_page_node(h, h->hugetlb_next_nid);
> > > @@ -1169,20 +1174,28 @@ static inline void try_to_free_low(struc
> > >  
> > >  /*
> > >   * Increment or decrement surplus_huge_pages.  Keep node-specific counters
> > > - * balanced by operating on them in a round-robin fashion.
> > > + * balanced by operating on them in a round-robin fashion.  Use nodes_allowed
> > > + * mask when decreasing suplus pages as we're "promoting" them to persistent.
> > 
> > s/suplus/surplus/
> 
> ACK
> 
> > 
> > > + * Use node_online_map for increment surplus pages as we're demoting previously
> > > + * persistent huge pages.
> > > + * Called holding the hugetlb_lock.
> > >   * Returns 1 if an adjustment was made.
> > >   */
> > >  static int adjust_pool_surplus(struct hstate *h, int delta)
> > >  {
> > > +	nodemask_t *nodemask = &node_online_map;
> > >  	static int prev_nid;
> > >  	int nid = prev_nid;
> > >  	int ret = 0;
> > >  
> > >  	VM_BUG_ON(delta != -1 && delta != 1);
> > > +	if (delta < 0)
> > > +		nodemask = h->nodes_allowed;
> > > +
> > 
> > Please spell out why nodes_allowed is only used when decreasing the surplus
> > count.
> 
> I thought my addition to the comment block did that.  
> 
> My thinking:  surplus pages are, by definition, in use, so the only time
> we decrease them via adjust_pool_surplus() is when we're increasing
> nr_hugepages.  New "persistent" huge pages are masked by nodes_allowed,
> so promoting surplus pages to persistent should also be so masked.
> 

This in addition to the existing comment is more than adequate.

> Conversely, since this series uses the node_online_mask for freeing
> available persistent huge pages, I mention that we use it for increasing
> the surplus page count.
> 
> Should I add more to the comment?
> 

Do please.

> > 
> > >  	do {
> > > -		nid = next_node(nid, node_online_map);
> > > +		nid = next_node(nid, *nodemask);
> > >  		if (nid == MAX_NUMNODES)
> > > -			nid = first_node(node_online_map);
> > > +			nid = first_node(*nodemask);
> > >  
> > >  		/* To shrink on this node, there must be a surplus page */
> > >  		if (delta < 0 && !h->surplus_huge_pages_node[nid])
> > > 
> > 
> 

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