Re: [PATCH 4/5] hugetlb: add per node hstate attributes

[Mel Gorman <mel@xxxxxxxxx>]

On Wed, Aug 26, 2009 at 04:46:43PM -0400, Lee Schermerhorn wrote:
> On Wed, 2009-08-26 at 12:47 -0700, David Rientjes wrote:
> > On Wed, 26 Aug 2009, Lee Schermerhorn wrote:
> > 
> > > Against: 2.6.31-rc6-mmotm-090820-1918
> > > 
> > > Introduce nodemask macro to allocate a nodemask and 
> > > initialize it to contain a single node, using existing
> > > nodemask_of_node() macro.  Coded as a macro to avoid header
> > > dependency hell.
> > > 
> > > This will be used to construct the huge pages "nodes_allowed"
> > > nodemask for a single node when a persistent huge page
> > > pool page count is modified via a per node sysfs attribute.
> > > 
> > > Signed-off-by: Lee Schermerhorn <lee.schermerhorn@xxxxxx>
> > > 
> > >  include/linux/nodemask.h |   10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > Index: linux-2.6.31-rc6-mmotm-090820-1918/include/linux/nodemask.h
> > > ===================================================================
> > > --- linux-2.6.31-rc6-mmotm-090820-1918.orig/include/linux/nodemask.h	2009-08-24 10:16:56.000000000 -0400
> > > +++ linux-2.6.31-rc6-mmotm-090820-1918/include/linux/nodemask.h	2009-08-26 12:38:31.000000000 -0400
> > > @@ -257,6 +257,16 @@ static inline int __next_node(int n, con
> > >  	m;								\
> > >  })
> > >  
> > > +#define alloc_nodemask_of_node(node)					\
> > > +({									\
> > > +	typeof(_unused_nodemask_arg_) *nmp;				\
> > > +	nmp = kmalloc(sizeof(*nmp), GFP_KERNEL);			\
> > > +	if (nmp)							\
> > > +		*nmp = nodemask_of_node(node);				\
> > > +	nmp;								\
> > > +})
> > > +
> > > +
> > >  #define first_unset_node(mask) __first_unset_node(&(mask))
> > >  static inline int __first_unset_node(const nodemask_t *maskp)
> > >  {
> > 
> > I think it would probably be better to use the generic NODEMASK_ALLOC() 
> > interface by requiring it to pass the entire type (including "struct") as 
> > part of the first parameter.  Then it automatically takes care of 
> > dynamically allocating large nodemasks vs. allocating them on the stack.
> > 
> > Would it work by redefining NODEMASK_ALLOC() in the NODES_SHIFT > 8 case 
> > to be this:
> > 
> > 	#define NODEMASK_ALLOC(x, m) x *m = kmalloc(sizeof(*m), GFP_KERNEL);
> > 
> > and converting NODEMASK_SCRATCH(x) to NODEMASK_ALLOC(struct 
> > nodemask_scratch, x), and then doing this in your code:
> > 
> > 	NODEMASK_ALLOC(nodemask_t, nodes_allowed);
> > 	if (nodes_allowed)
> > 		*nodes_allowed = nodemask_of_node(node);
> > 
> > The NODEMASK_{ALLOC,SCRATCH}() interface is in its infancy so it can 
> > probably be made more general to handle cases like this.
> 
> I just don't know what that would accomplish.  Heck, I'm not all that
> happy with the alloc_nodemask_from_node() because it's allocating both a
> hidden nodemask_t and a pointer thereto on the stack just to return a
> pointer to a kmalloc()ed nodemask_t--which is what I want/need here.
> 
> One issue I have with NODEMASK_ALLOC() [and nodemask_of_node(), et al]
> is that it declares the pointer variable as well as initializing it,
> perhaps with kmalloc(), ...   Indeed, it's purpose is to replace on
> stack nodemask declarations.
> 
> So, to use it at the start of, e.g., set_max_huge_pages() where I can
> safely use it throughout the function, I'll end up allocating the
> nodes_allowed mask on every call, whether or not a node is specified or
> there is a non-default mempolicy.   If it turns out that no node was
> specified and we have default policy, we need to free the mask and NULL
> out nodes_allowed up front so that we get default behavior.  That seems
> uglier to me that only allocating the nodemask when we know we need one.
> 
> I'm not opposed to using a generic function/macro where one exists that
> suits my purposes.   I just don't see one.  I tried to create
> one--alloc_nodemask_from_node(), and to keep Mel happy, I tried to reuse
> nodemask_from_node() to initialize it.  I'm really not happy with the
> results--because of those extra, hidden stack variables.  I could
> eliminate those by creating a out of line function, but there's no good
> place to put a generic nodemask function--no nodemask.c.  
> 

Ok. When I brought the subject up, it looked like you were creating a
hugetlbfs-specific helper that looked like it would have generic helpers. While
that is still the case, it's looking like generic helpers make things worse
and hide side-effects in helper functions that might cause greater difficulty
in the future. I'm happier to go with the existing code than I was before
so consider my objection dropped.

> I'm leaning towards going back to my original hugetlb-private
> "nodes_allowed_from_node()" or such.  I can use nodemask_from_node to
> initialize it, if that will make Mel happy, but trying to force fit an
> existing "generic" function just because it's generic seems pointless.
> 
> So, I'm going to let this series rest until I hear back from you and Mel
> on how to proceed with this. 
> 

I hate to do it to you, but at this point, I'm leaning towards your current
approach.

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab
--
To unsubscribe from this list: send the line "unsubscribe linux-numa" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html