Re: [PATCH 4/6] hugetlb: introduce alloc_nodemask_of_node

[David Rientjes <rientjes@google.com>]

On Tue, 1 Sep 2009, Lee Schermerhorn wrote:

> > > Index: linux-2.6.31-rc7-mmotm-090827-0057/include/linux/nodemask.h
> > > ===================================================================
> > > --- linux-2.6.31-rc7-mmotm-090827-0057.orig/include/linux/nodemask.h	2009-08-28 09:21:19.000000000 -0400
> > > +++ linux-2.6.31-rc7-mmotm-090827-0057/include/linux/nodemask.h	2009-08-28 09:21:29.000000000 -0400
> > > @@ -245,18 +245,34 @@ static inline int __next_node(int n, con
> > >  	return min_t(int,MAX_NUMNODES,find_next_bit(srcp->bits, MAX_NUMNODES, n+1));
> > >  }
> > >  
> > > +#define init_nodemask_of_nodes(mask, node)				\
> > > +	nodes_clear(*(mask));						\
> > > +	node_set((node), *(mask));
> > > +
> > 
> > Is the done thing to either make this a static inline or else wrap it in
> > a do { } while(0) ? The reasoning being that if this is used as part of an
> > another statement (e.g. a for loop) that it'll actually compile instead of
> > throw up weird error messages.
> 
> Right.  I'll fix this [and signoff/review orders] next time [maybe last
> time?].  It occurs to me that I can also use this for
> huge_mpol_nodes_allowed(), so I'll move it up in the series and fix that
> [which you've already ack'd].  I'll wait a bit to hear from David before
> I respin.
> 

I think it should be an inline function just so there's typechecking on 
the first argument passed in (and so alloc_nodemask_of_node() below 
doesn't get a NULL pointer dereference on node_set() if nmp can't be 
allocated).

I've seen the issue about the signed-off-by/reviewed-by/acked-by order 
come up before.  I've always put my signed-off-by line last whenever 
proposing patches because it shows a clear order in who gathered those 
lines when submitting to -mm, for example.  If I write

	Cc: Mel Gorman <mel@csn.ul.ie>
	Signed-off-by: David Rientjes <rientjes@google.com>

it is clear that I cc'd Mel on the initial proposal.  If it is the other 
way around, for example,

	Signed-off-by: David Rientjes <rientjes@google.com>
	Cc: Mel Gorman <mel@csn.ul.ie>
	Signed-off-by: Andrew Morton...

then it indicates Andrew added the cc when merging into -mm.  That's more 
relevant when such a line is acked-by or reviewed-by since it is now 
possible to determine who received such acknowledgement from the 
individual and is responsible for correctly relaying it in the patch 
submission.

If it's done this way, it indicates that whoever is signing off the patch 
is responsible for everything above it.  The type of line (signed-off-by, 
reviewed-by, acked-by) is enough of an indication about the development 
history of the patch, I believe, and it doesn't require specific ordering 
to communicate (and the first line having to be a signed-off-by line isn't 
really important, it doesn't replace the From: line).

It also appears to be how both Linus merges his own patches with Cc's.

> > > +/*
> > > + * returns pointer to kmalloc()'d nodemask initialized to contain the
> > > + * specified node.  Caller must free with kfree().
> > > + */
> > > +#define alloc_nodemask_of_node(node)					\
> > > +({									\
> > > +	typeof(_unused_nodemask_arg_) *nmp;				\
> > > +	nmp = kmalloc(sizeof(*nmp), GFP_KERNEL);			\
> > > +	if (nmp)							\
> > > +		init_nodemask_of_nodes(nmp, (node));			\
> > > +	nmp;								\
> > > +})
> > > +
> > 

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