On 08.10.2007 [18:56:05 -0700], Christoph Lameter wrote:
> On Mon, 8 Oct 2007, Nishanth Aravamudan wrote:
>
> > > struct page * fastcall
> > > __alloc_pages(gfp_t gfp_mask, unsigned int order,
> > > struct zonelist *zonelist)
> > > {
> > > + /*
> > > + * Use a temporary nodemask for __GFP_THISNODE allocations. If the
> > > + * cost of allocating on the stack or the stack usage becomes
> > > + * noticable, allocate the nodemasks per node at boot or compile time
> > > + */
> > > + if (unlikely(gfp_mask & __GFP_THISNODE)) {
> > > + nodemask_t nodemask;
> > > +
> > > + return __alloc_pages_internal(gfp_mask, order,
> > > + zonelist, nodemask_thisnode(&nodemask));
> > > + }
> > > +
> > > return __alloc_pages_internal(gfp_mask, order, zonelist, NULL);
> > > }
> >
> > <snip>
> >
> > So alloc_pages_node() calls here and for THISNODE allocations, we go ask
> > nodemask_thisnode() for a nodemask...
>
> Hmmmm... nodemask_thisnode needs to be passed the zonelist.
>
> > And nodemask_thisnode() always gives us a nodemask with only the node
> > the current process is running on set, I think?
>
> Right.
>
>
> > That seems really wrong -- and would explain what Lee was seeing while
> > using my patches for the hugetlb pool allocator to use THISNODE
> > allocations. All the allocations would end up coming from whatever node
> > the process happened to be running on. This obviously messes up hugetlb
> > accounting, as I rely on THISNODE requests returning NULL if they go
> > off-node.
> >
> > I'm not sure how this would be fixed, as __alloc_pages() no longer has
> > the nid to set in the mask.
> >
> > Am I wrong in my analysis?
>
> No you are right on target. The thisnode function must determine the
> node from the first zone of the zonelist.
It seems like I would zonelist_node_idx() for this, along the lines of:
static nodemask_t *nodemask_thisnode(nodemask_t *nodemask,
struct zonelist *zonelist)
{
int nid = zonelist_node_idx(zonelist);
/* Build a nodemask for just this node */
nodes_clear(*nodemask);
node_set(nid, *nodemask);
return nodemask;
}
But I think I need to check that zonelist->_zonerefs->zone is !NULL, given this
definition of zonelist_node_idx()
static inline int zonelist_node_idx(struct zoneref *zoneref)
{
#ifdef CONFIG_NUMA
/* zone_to_nid not available in this context */
return zoneref->zone->node;
#else
return 0;
#endif /* CONFIG_NUMA */
}
and this comment in __alloc_pages_internal():
....
z = zonelist->_zonerefs; /* the list of zones suitable for gfp_mask */
if (unlikely(!z->zone)) {
/*
* Happens if we have an empty zonelist as a result of
* GFP_THISNODE being used on a memoryless node
*/
return NULL;
}
...
It seems like zoneref->zone may be NULL in zonelist_node_idx()? Maybe
someone else should look into resolving this :)
Thanks,
Nish
--
Nishanth Aravamudan <[EMAIL PROTECTED]>
IBM Linux Technology Center
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
