On Tue, Aug 25, 2009 at 04:49:34PM -0400, Lee Schermerhorn wrote:
> > > <SNIP>
> > > +static int hstate_next_node_to_alloc(struct hstate *h,
> > > + nodemask_t *nodes_allowed)
> > > {
> > > int nid, next_nid;
> > >
> > > - nid = h->next_nid_to_alloc;
> > > - next_nid = next_node_allowed(nid);
> > > + if (!nodes_allowed)
> > > + nodes_allowed = &node_online_map;
> > > +
> > > + nid = this_node_allowed(h->next_nid_to_alloc, nodes_allowed);
> > > +
> > > + next_nid = next_node_allowed(nid, nodes_allowed);
> > > h->next_nid_to_alloc = next_nid;
> > > +
> > > return nid;
> > > }
> >
> > Don't need next_nid.
>
> Well, the pre-existing comment block indicated that the use of the
> apparently spurious next_nid variable is necessary to close a race. Not
> sure whether that comment still applies with this rework. What do you
> think?
>
The original intention was not to return h->next_nid_to_alloc because
there is a race window where it's MAX_NUMNODES.
nid is a stack-local variable here, it should not become MAX_NUMNODES by
accident because this_node_allowed() and next_node_allowed() are both taking
care not to return MAX_NUMNODES so it's safe as a return value. Even in the
presense of races with the code structure you currently have. I think it's
safe to have
nid = this_node_allowed(h->next_nid_to_alloc, nodes_allowed);
h->next_nid_to_alloc = next_node_allowed(nid, nodes_allowed);
return nid;
because at worse in the presense of races, h->next_nid_to_alloc gets
assigned to the same value twice, but never MAX_NUMNODES.
--
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
