Re: [patch] slab: Do not use mempolicy for kmalloc_node

[Ravikiran G Thirumalai <kiran@scalex86.org>]

On Wed, Sep 13, 2006 at 03:28:01PM -0700, Christoph Lameter wrote:
> I wish first of all that the description would accurately describe the 
> problem.

Well, I thought the subject itself described the problem in one line :), I
guess I should be more verbose?  OK.

> 
> On Wed, 13 Sep 2006, Ravikiran G Thirumalai wrote:
> 
> 
> > +/* Allocate object from the array cache of the executing cpu */
> 
> I am not sure what this adds.

Comments :). Well the idea was to describe the difference between 
__cache_alloc and ____cache_alloc, I mean, these are two similar sounding
functions with extra underscores.  OK, maybe I was being too verbose here?

> 
> > +/* 
> > + * Allocate object from the appropriate node as per mempolicy/cpuset
> > + * constraints
> > + */
> 
> We only do this under certain conditions. The main purpose of this 
> function is to allocate an object without having specified a node.

Yes, the conditions being cpuset constraints or a mempolicy being in place.
Again, since objects can be allocated off other nodes under certain
conditions, I thought it was good to document it...

> 
> >  	unsigned long save_flags;
> >  	void *objp;
> > -
> >  	cache_alloc_debugcheck_before(cachep, flags);
> > -
> >  	local_irq_save(save_flags);
> 
> Extra material.

OK

> 
> > +
> > +#ifdef CONFIG_NUMA
> > +	if (unlikely(current->flags & (PF_SPREAD_SLAB | PF_MEMPOLICY))) {
> > +		objp = alternate_node_alloc(cachep, flags);
> > +		if (objp != NULL) {
> > +			local_irq_restore(save_flags);
> > +			prefetchw(objp);
> > +			return objp;
> > +		}
> > +	}
> > +#endif
> > +
> 
> Ok.
> 
> > @@ -3303,9 +3309,10 @@ void *kmem_cache_alloc_node(struct kmem_
> >  	cache_alloc_debugcheck_before(cachep, flags);
> >  	local_irq_save(save_flags);
> >  
> > -	if (nodeid == -1 || nodeid == numa_node_id() ||
> > -			!cachep->nodelists[nodeid])
> > +	if (nodeid == numa_node_id())
> >  		ptr = ____cache_alloc(cachep, flags);
> > +	else if (nodeid == -1 || !cachep->nodelists[nodeid])
> > +		ptr = __cache_alloc(cachep, flags, __builtin_return_address(0));
> >  	else
> 
> We are still allocating according to policy if nodeid == -1 or if we have 
> an invalid node? I thought we agreed that kmalloc_node should never 
> obey memory policies?
> 
> Simply move the #ifdef CONFIG_NUMA block as we agreed last night. And fix 
> the description to specify under what conditions kmalloc_node will obey 
> memory policies.

This is the case when we are requesting an object from a non existent
node/invalid node.  So we have 2 choices, either to spread the allocations 
as per the cpuset constraints (the same treatment as kmalloc), or to 
allocate from the requesting node, either ways we are not strictly 
confirming to the user's choice of node (which we cannot).  I cannot see 
major advantage or disadvantage either ways, so I chose to keep the policy
in current mainline code -- spread according to the policy set.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/