Re: [PATCH] populated_map: fix !NUMA case, remove comment

[Nishanth Aravamudan <nacc@us.ibm.com>]

On 13.06.2007 [14:21:14 -0400], Lee Schermerhorn wrote:
> On Wed, 2007-06-13 at 10:58 -0700, Nishanth Aravamudan wrote:
> > On 13.06.2007 [11:30:06 -0400], Lee Schermerhorn wrote:
> > > On Tue, 2007-06-12 at 13:01 -0700, Nishanth Aravamudan wrote:
> > > > On 12.06.2007 [12:58:16 -0700], Christoph Lameter wrote:
> > > > > On Tue, 12 Jun 2007, Christoph Lameter wrote:
> > > > > 
> > > > > > Uhhh... Right there is another special case. The recently 
> > > > > > introduces zonelist swizzle makes the DMA zone come last and if a 
> > > > > > node had only a DMA zone then it may become swizzled to the end of 
> > > > > > the zonelist.
> > > > > 
> > > > > Maybe we can ignore that case for now:
> > > > > 
> > > I wish we wouldn't.  We need the "DMA zone comes last" for both HP and
> > > Fujitsu platforms.  That's why Kame and I worked on that patch
> > > together.  
> > 
> > Right. I interpreted the "for now" as for this first stack of patches.
> > We'll need a fix for your platform on top, but it seems to be a minority
> > case? Not saying it shouldn't be fixed, by any means, just trying to get
> > a handle on it.
> 
> Yep.  I'm testing the stack "as is" now.  If it doesn't spread the
> huge pages evenly because of our funky DMA-only node, I'll post a fix
> up patch for consideration.

Great, thanks.

> By the way, your sysfs attribute patch doesn't compile.  I'll post
> comments/fixes in response to your message that submitted the patch.

Dang, much appreciated.

> <snip>
> 
> > > 
> > > I think that the "node has memory" mask is fine for scanning nodes
> > > that might have memory in the zone of interest--including in the
> > > hugetlb alloc_fresh_huge_page() loop.  However, I think that to
> > > support all platforms in a generic way, alloc_pages_node() and
> > > alloc_page_interleave() [both take a node id arg] should be more
> > > strict when the gfp mask includes 'THISNODE and not assume that a
> > > populated node always has on-node memory in the zone of interest.
> > 
> > Hrm, perhaps.
> > 
> > > E.g., something like:
> > > 
> > > 	pgdat_t *pgdat;
> > > 	struct zonelist *zonelist;
> > > 
> > > 	...
> > > 
> > > 	/* 
> > > 	 * after validating nid, ... 
> > > 	 * Note that we need to fetch these values anyway for the
> > > 	 * [likely?] call to __alloc_pages().  
> > > 	 */
> > > 	pgdat = NODE_DATA(nid);
> > > 	zonelist = pgdat->node_zonelists + gfp_zone(gfp_mask);
> > > 
> > > 	if ((gfp_mask & __GFP_THISNODE) &&
> > > 		zonelist->zones[0]->zone_pgdat != pgdat)
> > > 		return NULL;
> > > 	
> > > 	return __alloc_pages(gfp_mask, order, zonelist);
> > > 
> > > 
> > > I see you've submitted a new patch set.  I grab it [when Nish reposts]
> > > and test it as is and modified to look something like the above, if
> > > needed.
> > 
> > I think your code above makes sense -- I'd still leave in the earlier
> > check, though.
> > 
> > So it probably should be:
> > 
> > 	pgdat = NODE_DATA(nid);
> > 	zonelist = pgdat->node_zonelists + gfp_zone(gfp_mask);
> > 
> > 	if (unlikely((gfp_mask & __GFP_THISNODE) &&
> > 		(!node_memory(nid) ||
> > 		 zonelist->zones[0]->zone_pgdat != pgdat)))
> > 		 return NULL;
> > 
> > That way, if the node has no memory whatsoever, we don't bother checking
> > the pgdat of the relevant zone?
> 
> Well, since most nodes WILL, I think, have memory, that just adds an
> extra check in the most frequent case.  Then, we'll have to go ahead
> and check the pgdat.  However, if the first zone in the selected
> zonelist IS "on-node" [pgdats match], we know that the node has memory
> [altho' the zone may not have available pages].  And since we have to
> fetch the pgdat and the zonelist, anyway, as the argument to
> __alloc_pages(), I don't think my proposed change adds any additional
> memory ref's, while eliminating the ref to the node_memory_map.  I'm
> assuming here that the compiler will optimize away any stores to the
> pgdat/zonelist variables.

You're right, sorry for the noise.

> So, we can use the node_memory() test at higher levels--like the
> alloc_fresh_huge_page() loop, to avoid attempting allocations from
> nodes that we know have no memory, but I think the
> allocate_pages_node() and allocate_interleave_page() should test the
> selected zonelist explicitly.

Yep.

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center

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