slub: make dead caches discard free slabs immediately

SLUB's kmem_cache_shrink not only removes empty slabs from the cache, but also sorts slabs by the number of objects in-use to cope with fragmentation. To achieve that, it tries to allocate a temporary array. If it fails, it will abort the whole procedure.

Quoted 4 lines · Vladimir Davydov

SLUB's kmem_cache_shrink not only removes empty slabs from the cache, but also sorts slabs by the number of objects in-use to cope with fragmentation. To achieve that, it tries to allocate a temporary array. If it fails, it will abort the whole procedure.

I do not think its worth optimizing this. If we cannot allocate even a small object then the system is in an extremely bad state anyways.

mm/slub.c int __kmem_cache_shrink(struct kmem_cache *s) · line 3415

Quoted 12 lines

mm/slub.c +3 -1

@@ -3400,7 +3407,9 @@ int __kmem_cache_shrink(struct kmem_cache *s)
 		 * list_lock. page->inuse here is the upper limit.
 		 */
 		list_for_each_entry_safe(page, t, &n->partial, lru) {
-			list_move(&page->lru, slabs_by_inuse + page->inuse);
+			if (page->inuse < objects)
+				list_move(&page->lru,
+					  slabs_by_inuse + page->inuse);
 			if (!page->inuse)
 				n->nr_partial--;
 		}

The condition is always true. A page that has page->inuse == objects would not be on the partial list.

Anyways, I think that silently relying on the fact that the allocator never fails small allocations is kind of unreliable. What if this

Quoted 10 lines

mm/slub.c int __kmem_cache_shrink(struct kmem_cache *s) · line 3415

Quoted 6 lines

mm/slub.c +3 -0

+			if (page->inuse < objects)
+				list_move(&page->lru,
+					  slabs_by_inuse + page->inuse);
 			if (!page->inuse)
 				n->nr_partial--;
 		}

The condition is always true. A page that has page->inuse == objects would not be on the partial list.

This is in case we failed to allocate the slabs_by_inuse array. We only have a list for empty slabs then (on stack).

Quoted 2 lines · Vladimir Davydov

Anyways, I think that silently relying on the fact that the allocator never fails small allocations is kind of unreliable. What if this

We are not doing that though. If the allocation fails we do nothing.

Quoted 12 lines

Quoted 11 lines

mm/slub.c int __kmem_cache_shrink(struct kmem_cache *s) · line 3415

Quoted 7 lines

mm/slub.c +3 -0

+			if (page->inuse < objects)
+				list_move(&page->lru,
+					  slabs_by_inuse + page->inuse);
 			if (!page->inuse)
 				n->nr_partial--;
 		}

The condition is always true. A page that has page->inuse == objects would not be on the partial list.

Quoted 2 lines

This is in case we failed to allocate the slabs_by_inuse array. We only have a list for empty slabs then (on stack).

Ok in that case objects == 1. If you want to do this maybe do it in a more general way?

You could allocate an array on the stack to deal with the common cases. I believe an array of 32 objects would be fine to allocate and cover most of the slab caches on the system? Would eliminate most of the allocations in kmem_cache_shrink.

We could do that, but IMO that would only complicate the code w/o yielding any real benefits. This function is slow and called rarely anyway, so I don't think there is any point to optimize out a page allocation here.

Quoted 4 lines · Vladimir Davydov

We could do that, but IMO that would only complicate the code w/o yielding any real benefits. This function is slow and called rarely anyway, so I don't think there is any point to optimize out a page allocation here.

I think you already have the code there. Simply allow the sizeing of the empty_page[] array. And rename it.

May be, we could remove this allocation at all then? I mean, always distribute slabs among constant number of buckets, say 32, like this:

What you could do is simply put all slab pages with more than 32 objects available at the end of the list.

Quoted 4 lines · Vladimir Davydov

mm/slab.c +0 -0

@@ -2400,11 +2400,16 @@ int __kmem_cache_shrink(struct kmem_cache *cachep)
 	return (ret ? 1 : 0);
 }
 

mm/slab.c int __kmem_cache_shrink(struct kmem_cache *cachep) · line 2407

Quoted 5 lines

mm/slab.c +5 -0

+void __kmem_cache_shrink(struct kmem_cache *cachep)
+{
+	__cache_shrink(cachep);
+}
+

Why do we need this wrapper? Rename __cache_shrink to __kmem_cache_shrink instead?

__cache_shrink() is used not only in __kmem_cache_shrink(), but also in SLAB's __kmem_cache_shutdown(), where we do need its return value to check if the cache is empty.

Quoted 3 lines · Vladimir Davydov

__cache_shrink() is used not only in __kmem_cache_shrink(), but also in SLAB's __kmem_cache_shutdown(), where we do need its return value to check if the cache is empty.

It could be useful to know if a slab is empty. So maybe leave kmem_cache_shrink the way it is and instead fix up slub to return the proper value?

Hmm, why? The return value has existed since this function was introduced, but nobody seems to have ever used it outside the slab core. Besides, this check is racy, so IMO we shouldn't encourage users of the API to rely on it. That said, I believe we should drop the return value for now. If anybody ever needs it, we can reintroduce it.

Quoted 5 lines · Vladimir Davydov

Hmm, why? The return value has existed since this function was introduced, but nobody seems to have ever used it outside the slab core. Besides, this check is racy, so IMO we shouldn't encourage users of the API to rely on it. That said, I believe we should drop the return value for now. If anybody ever needs it, we can reintroduce it.

The check is only racy if you have concurrent users. It is not racy if a subsystem shuts down access to the slabs and then checks if everything is clean before closing the cache.

Slab creation and destruction are not serialized. It is the responsibility of the subsystem to make sure that there are no concurrent users and that there are no objects remaining before destroying a slab.

Right, but I just don't see why a subsystem using a kmem_cache would need to check whether there are any objects left in the cache. I mean, it should somehow keep track of the objects it's allocated anyway, e.g. by linking them in a list. That means it must already have a way to check if it is safe to destroy its cache or not.

Suppose we leave the return value as is. A subsystem, right before going to destroy a cache, calls kmem_cache_shrink, which returns 1 (slab is not empty). What is it supposed to do then?

Quoted 5 lines · Vladimir Davydov

Right, but I just don't see why a subsystem using a kmem_cache would need to check whether there are any objects left in the cache. I mean, it should somehow keep track of the objects it's allocated anyway, e.g. by linking them in a list. That means it must already have a way to check if it is safe to destroy its cache or not.

The acpi subsystem did that at some point.

Quoted 3 lines

Suppose we leave the return value as is. A subsystem, right before going to destroy a cache, calls kmem_cache_shrink, which returns 1 (slab is not empty). What is it supposed to do then?

That is up to the subsystem. If it has a means of tracking down the missing object then it can deal with it. If not then it cannot shutdown the cache and do a proper recovery action.

To speed up further allocations SLUB may store empty slabs in per cpu/node partial lists instead of freeing them immediately. This prevents per memcg caches destruction, because kmem caches created for a memory cgroup are only destroyed after the last page charged to the cgroup is freed.

To fix this issue, this patch resurrects approach first proposed in [1]. It forbids SLUB to cache empty slabs after the memory cgroup that the cache belongs to was destroyed. It is achieved by setting kmem_cache's cpu_partial and min_partial constants to 0 and tuning put_cpu_partial() so that it would drop frozen empty slabs immediately if cpu_partial = 0.

The runtime overhead is minimal. From all the hot functions, we only touch relatively cold put_cpu_partial(): we make it call unfreeze_partials() after freezing a slab that belongs to an offline memory cgroup. Since slab freezing exists to avoid moving slabs from/to a partial list on free/alloc, and there can't be allocations from dead caches, it shouldn't cause any overhead. We do have to disable preemption for put_cpu_partial() to achieve that though.

The original patch was accepted well and even merged to the mm tree. However, I decided to withdraw it due to changes happening to the memcg core at that time. I had an idea of introducing per-memcg shrinkers for kmem caches, but now, as memcg has finally settled down, I do not see it as an option, because SLUB shrinker would be too costly to call since SLUB does not keep free slabs on a separate list. Besides, we currently do not even call per-memcg shrinkers for offline memcgs. Overall, it would introduce much more complexity to both SLUB and memcg than this small patch.

Regarding to SLAB, there's no problem with it, because it shrinks per-cpu/node caches periodically. Thanks to list_lru reparenting, we no longer keep entries for offline cgroups in per-memcg arrays (such as memcg_cache_params->memcg_caches), so we do not have to bother if a per-memcg cache will be shrunk a bit later than it could be.

[1] http://thread.gmane.org/gmane.linux.kernel.mm/118649/focus=118650

Signed-off-by Vladimir Davydov <[email protected]>

---
 mm/slab.c        |    2 +-
 mm/slab.h        |    2 +-
 mm/slab_common.c |   15 +++++++++++++--
 mm/slob.c        |    2 +-
 mm/slub.c        |   25 ++++++++++++++++++++-----
 5 files changed, 36 insertions(+), 10 deletions(-)

mm/slab.c +1 -1

index 279c44d6d8e1..f0514df07b85 100644
@@ -2400,7 +2400,7 @@ static int __cache_shrink(struct kmem_cache *cachep)
 	return (ret ? 1 : 0);
 }
 
-void __kmem_cache_shrink(struct kmem_cache *cachep)
+void __kmem_cache_shrink(struct kmem_cache *cachep, bool deactivate)
 {
 	__cache_shrink(cachep);
 }

mm/slab.h +1 -1

index c036e520d2cf..041260197984 100644
@@ -138,7 +138,7 @@ static inline unsigned long kmem_cache_flags(unsigned long object_size,
 #define CACHE_CREATE_MASK (SLAB_CORE_FLAGS | SLAB_DEBUG_FLAGS | SLAB_CACHE_FLAGS)
 
 int __kmem_cache_shutdown(struct kmem_cache *);
-void __kmem_cache_shrink(struct kmem_cache *);
+void __kmem_cache_shrink(struct kmem_cache *, bool);
 void slab_kmem_cache_release(struct kmem_cache *);
 
 struct seq_file;

mm/slab_common.c +13 -2

index 6803639fdff0..472ab7fcffd4 100644
@@ -549,10 +549,13 @@ void memcg_deactivate_kmem_caches(struct mem_cgroup *memcg)
 {
 	int idx;
 	struct memcg_cache_array *arr;
-	struct kmem_cache *s;
+	struct kmem_cache *s, *c;
 
 	idx = memcg_cache_id(memcg);
 
+	get_online_cpus();
+	get_online_mems();
+
 	mutex_lock(&slab_mutex);
 	list_for_each_entry(s, &slab_caches, list) {
 		if (!is_root_cache(s))

@@ -560,9 +563,17 @@ void memcg_deactivate_kmem_caches(struct mem_cgroup *memcg)
 
 		arr = rcu_dereference_protected(s->memcg_params.memcg_caches,
 						lockdep_is_held(&slab_mutex));
+		c = arr->entries[idx];
+		if (!c)
+			continue;
+
+		__kmem_cache_shrink(c, true);
 		arr->entries[idx] = NULL;
 	}
 	mutex_unlock(&slab_mutex);
+
+	put_online_mems();
+	put_online_cpus();
 }
 
 void memcg_destroy_kmem_caches(struct mem_cgroup *memcg)

@@ -646,7 +657,7 @@ void kmem_cache_shrink(struct kmem_cache *cachep)
 {
 	get_online_cpus();
 	get_online_mems();
-	__kmem_cache_shrink(cachep);
+	__kmem_cache_shrink(cachep, false);
 	put_online_mems();
 	put_online_cpus();
 }

mm/slob.c +1 -1

index 043a14b6ccbe..e63ff9d926dc 100644
@@ -618,7 +618,7 @@ int __kmem_cache_shutdown(struct kmem_cache *c)
 	return 0;
 }
 
-void __kmem_cache_shrink(struct kmem_cache *c)
+void __kmem_cache_shrink(struct kmem_cache *c, bool deactivate)
 {
 }
 

mm/slub.c +10 -1

index c09d93dde40e..6f57824af019 100644
@@ -2007,6 +2007,7 @@ static void put_cpu_partial(struct kmem_cache *s, struct page *page, int drain)
 	int pages;
 	int pobjects;
 
+	preempt_disable();
 	do {
 		pages = 0;
 		pobjects = 0;

@@ -2040,6 +2041,14 @@ static void put_cpu_partial(struct kmem_cache *s, struct page *page, int drain)
 
 	} while (this_cpu_cmpxchg(s->cpu_slab->partial, oldpage, page)
 								!= oldpage);
+	if (unlikely(!s->cpu_partial)) {
+		unsigned long flags;
+
+		local_irq_save(flags);
+		unfreeze_partials(s, this_cpu_ptr(s->cpu_slab));
+		local_irq_restore(flags);
+	}
+	preempt_enable();
 #endif
 }
 

@@ -3368,7 +3377,7 @@ EXPORT_SYMBOL(kfree);
  * being allocated from last increasing the chance that the last objects
  * are freed in them.
  */
-void __kmem_cache_shrink(struct kmem_cache *s)
+void __kmem_cache_shrink(struct kmem_cache *s, bool deactivate)
 {
 	int node;
 	int i;

@@ -3381,6 +3390,15 @@ void __kmem_cache_shrink(struct kmem_cache *s)
 		kmalloc(sizeof(struct list_head) * objects, GFP_KERNEL);
 	unsigned long flags;

Quoted 4 lines · Vladimir Davydov

mm/slub.c +0 -0

@@ -3381,6 +3390,15 @@ void __kmem_cache_shrink(struct kmem_cache *s)
 		kmalloc(sizeof(struct list_head) * objects, GFP_KERNEL);
 	unsigned long flags;
 

mm/slub.c void __kmem_cache_shrink(struct kmem_cache *s) · line 3401

Quoted 9 lines

mm/slub.c +9 -0

+	if (deactivate) {
+		/*
+		 * Disable empty slabs caching. Used to avoid pinning offline
+		 * memory cgroups by freeable kmem pages.
+		 */
+		s->cpu_partial = 0;
+		s->min_partial = 0;
+	}
+

Maybe, kick_all_cpus_sync() is needed here since object would be freed asynchronously so they can't see this updated value.

Hi Joonsoo,

Quoted 5 lines · Joonsoo Kim

Quoted 4 lines · Vladimir Davydov

mm/slub.c +0 -0

@@ -3381,6 +3390,15 @@ void __kmem_cache_shrink(struct kmem_cache *s)
            kmalloc(sizeof(struct list_head) * objects, GFP_KERNEL);
    unsigned long flags;
 

Quoted 12 lines

mm/slub.c void __kmem_cache_shrink(struct kmem_cache *s) · line 3401

Quoted 9 lines

mm/slub.c +9 -0

+   if (deactivate) {
+           /*
+            * Disable empty slabs caching. Used to avoid pinning offline
+            * memory cgroups by freeable kmem pages.
+            */
+           s->cpu_partial = 0;
+           s->min_partial = 0;
+   }
+

Maybe, kick_all_cpus_sync() is needed here since object would be freed asynchronously so they can't see this updated value.

I thought flush_all() should do the trick, no?

2015-01-27 17:23 GMT+09:00 Vladimir Davydov <[email protected]>:

Quoted 21 lines

Hi Joonsoo,

Quoted 17 lines · Joonsoo Kim

mm/slub.c void __kmem_cache_shrink(struct kmem_cache *s) · line 3401

Quoted 13 lines · Vladimir Davydov

mm/slub.c +9 -0

@@ -3381,6 +3390,15 @@ void __kmem_cache_shrink(struct kmem_cache *s)
            kmalloc(sizeof(struct list_head) * objects, GFP_KERNEL);
    unsigned long flags;
 
+   if (deactivate) {
+           /*
+            * Disable empty slabs caching. Used to avoid pinning offline
+            * memory cgroups by freeable kmem pages.
+            */
+           s->cpu_partial = 0;
+           s->min_partial = 0;
+   }
+

Maybe, kick_all_cpus_sync() is needed here since object would be freed asynchronously so they can't see this updated value.

Quoted 1 line

I thought flush_all() should do the trick, no?

Unfortunately, it doesn't.

flush_all() sends IPI to not all cpus. It only sends IPI to cpus where some conditions
are met and freeing could occur on the other ones.