Re: [PATCH] slub: Don't throw away partial remote slabs if there is no local memory

On Tue, Jan 07, 2014 at 05:52:31PM +0800, Wanpeng Li wrote:

On Tue, Jan 07, 2014 at 04:41:36PM +0900, Joonsoo Kim wrote:

quoted

On Tue, Jan 07, 2014 at 01:21:00PM +1100, Anton Blanchard wrote:

quoted

quoted

Index: b/mm/slub.c
===================================================================

--- a/mm/slub.c
+++ b/mm/slub.c

@@ -2278,10 +2278,17 @@ redo:
 
 	if (unlikely(!node_match(page, node))) {
 		stat(s, ALLOC_NODE_MISMATCH);
-		deactivate_slab(s, page, c->freelist);
-		c->page = NULL;
-		c->freelist = NULL;
-		goto new_slab;
+
+		/*
+		 * If the node contains no memory there is no point in trying
+		 * to allocate a new node local slab
+		 */
+		if (node_spanned_pages(node)) {
+			deactivate_slab(s, page, c->freelist);
+			c->page = NULL;
+			c->freelist = NULL;
+			goto new_slab;
+		}
 	}
 
 	/*

Hello,

I think that we need more efforts to solve unbalanced node problem.

With this patch, even if node of current cpu slab is not favorable to
unbalanced node, allocation would proceed and we would get the unintended memory.

And there is one more problem. Even if we have some partial slabs on
compatible node, we would allocate new slab, because get_partial() cannot handle
this unbalance node case.

To fix this correctly, how about following patch?

Thanks.

------------->8--------------------

diff --git a/mm/slub.c b/mm/slub.c
index c3eb3d3..a1f6dfa 100644
--- a/mm/slub.c
+++ b/mm/slub.c

@@ -1672,7 +1672,19 @@ static void *get_partial(struct kmem_cache *s, gfp_t flags, int node,

{
       void *object;
       int searchnode = (node == NUMA_NO_NODE) ? numa_node_id() : node;
+       struct zonelist *zonelist;
+       struct zoneref *z;
+       struct zone *zone;
+       enum zone_type high_zoneidx = gfp_zone(flags);

+       if (!node_present_pages(searchnode)) {
+               zonelist = node_zonelist(searchnode, flags);
+               for_each_zone_zonelist(zone, z, zonelist, high_zoneidx) {
+                       searchnode = zone_to_nid(zone);
+                       if (node_present_pages(searchnode))
+                               break;
+               }
+       }

Why change searchnode instead of depending on fallback zones/nodes in 
get_any_partial() to allocate partial slabs?

If node != NUMA_NO_NODE, get_any_partial() isn't called.
That's why I change searchnode here instead of get_any_partial().

Thanks.