On Thu, 2009-09-10 at 16:05 -0700, Andrew Morton wrote:
> On Wed, 09 Sep 2009 12:31:46 -0400
> Lee Schermerhorn <lee.schermerhorn@xxxxxx> wrote:
>
> > [PATCH 3/6] - hugetlb: introduce alloc_nodemask_of_node()
> >
> > Against: 2.6.31-rc7-mmotm-090827-1651
> >
> > New in V5 of series
> >
> > V6: + rename 'init_nodemask_of_nodes()' to 'init_nodemask_of_node()'
> > + redefine init_nodemask_of_node() as static inline fcn
> > + move this patch back 1 in series
> >
> > Introduce nodemask macro to allocate a nodemask and
> > initialize it to contain a single node, using the macro
> > init_nodemask_of_node() factored out of the nodemask_of_node()
> > macro.
> >
> > alloc_nodemask_of_node() 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 basing nodes_allowed on a
> > preferred/local mempolicy or when a persistent huge page
> > pool page count is modified via a per node sysfs attribute.
> >
> > ...
> >
> > +/*
> > + * 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_node(nmp, (node)); \
> > + nmp; \
> > +})
>
> All right, I give up. What's with this `typeof(_unused_nodemask_arg_)'
> stuff?
You got me. I would have used a bar nodemask_t, but I was following the
style of the nodemask_of_node() in the same header.
>
>
> Was there a reason why this had to be implemented as a macro.
> One
> which evaluates its arg either one or zero times, btw?
Well, one, unless the alloc fails.
>
> hm. "to avoid header dependency hell". What hell? Self-inflicted?
Well, I tried to make it a static inline function, but nodemask.h gets
included, indirectly, in various places where, e.g., kmalloc() is not
defined. I tried including slab.h, but that had problems with other
missing definitions. I didn't want to end up with the entire
include/linux directory included in nodemask.h.
I would have put it in a .c file, but there is no, e.g., nodemask.c.
Guess I could have created alloc_bitmap_of_bit() in bitmap.c with a
wrapper in nodemask.h. Would that be preferable?
>
> alloc_nodemask_of_node() has no callers, so I can think of a good fix
> for these problems. If it _did_ have a caller then I might ask "can't
> we fix this by moving alloc_nodemask_of_node() into the .c file". But
> it doesn't so I can't.
This patch was a later addition. The function is used by the following
patch. Originally, I had a private function in hugetlb.c that
kmalloc()'d and initialized the nodes_allowed mask. Mel suggested that
I use the generic nodemask_of_node(). That didn't have the semantics I
wanted, so I created this variant.
>
> It's a bit rude to assume that the caller wanted to use GFP_KERNEL.
I can add a gfp_t parameter to the macro, but I'll still need to select
value in the caller. Do you have a suggested alternative to GFP_KERNEL
[for both here and in alloc_nodemask_of_mempolicy()]? We certainly
don't want to loop forever, killing off tasks, as David mentioned.
Silently failing is OK. We handle that.
Lee
--
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
