kmemcg slab reparenting

Hi Johannes, Michal, Christoph,

Recently I posted my thoughts on how we can handle kmem caches of dead memcgs:

https://lkml.org/lkml/2014/4/20/38

The only feedback I got then was from Johannes who voted for migrating slabs of such caches to the parent memcg's cache (so called reparenting), so in this RFC I'd like to propose a draft of possible implementation of slab reparenting. I'd appreciate if you could look through it and post if it's worth developing in this direction or not.

The implementation of reparenting is given in patch 3, which is the most important part of this set. Patch 1 just makes slub keep full slabs on list, and patch 2 a bit extends percpu-refcnt interface.

NOTE the implementation is given only for slub, though it should be easy to implement the same hack for slab.

Thanks,

Vladimir Davydov (3):
slub: keep full slabs on list for per memcg caches
percpu-refcount: allow to get dead reference
slub: reparent memcg caches' slabs on memcg offline

 include/linux/memcontrol.h      |    4 +-
 include/linux/percpu-refcount.h |   11 +-
 include/linux/slab.h            |    7 +-
 mm/memcontrol.c                 |   54 ++++---
 mm/slab.h                       |    7 +-
 mm/slub.c                       |  299 ++++++++++++++++++++++++++++++++++-----
 6 files changed, 318 insertions(+), 64 deletions(-)

1.7.10.4

Currently full slabs are only kept on per-node lists for debugging, but we need this feature to reparent per memcg caches, so let's enable it for them too.

---
 mm/slab.h |    2 ++
 mm/slub.c |   91 +++++++++++++++++++++++++++++++++++++++++--------------------
 2 files changed, 63 insertions(+), 30 deletions(-)

index 961a3fb1f5a2..0eca922ed7a0 100644
@@ -288,6 +288,8 @@ struct kmem_cache_node {
 #ifdef CONFIG_SLUB_DEBUG
 	atomic_long_t nr_slabs;
 	atomic_long_t total_objects;
+#endif
+#if defined(CONFIG_MEMCG_KMEM) || defined(CONFIG_SLUB_DEBUG)
 	struct list_head full;
 #endif
 #endif

1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

That will significantly impact the fastpaths for alloc and free.

Also a pretty significant change the logic of the fastpaths since they were not designed to handle the full lists. In debug mode all operations were only performed by the slow paths and only the slow paths so far supported tracking full slabs.

That's the minimal price we have to pay for slab re-parenting, because w/o it we won't be able to look up for all slabs of a particular per memcg cache. The question is, can it be tolerated or I'd better try some other way?

No problem, this patch is just a draft. Thanks to static keys, it won't be difficult to eliminate any overhead if there is no kmem-active memcgs.

Thanks.

AFACIT these modifications all together will have a significant impact on performance.

You could avoid the refcounting on free relying on the atomic nature of cmpxchg operations. If you zap the per cpu slab then the fast path will be forced to fall back to the slowpaths where you could do what you need to do.

There is no tracking of full slabs without adding much more logic to the fastpath. You could force any operation that affects tne full list into the slow path. But that also would have an impact.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Hmm, looking at __slab_free once again, I tend to agree that we could rely on cmpxchg to do re-parenting: we could freeze all slabs of the cache being re-parented forcing every on-going kfree to do only a cmpxchg w/o touching any lists and taking any locks, and then unfreeze all the frozen slabs to the target cache. No need in the ugly "slow mode" I introduced in this patch set would be necessary then.

But w/o ref-counting how can we make sure that all kfrees to the cache we are going to re-parent have been completed so that it can be safely destroyed? An example:

CPU0: CPU1:
----- -----
kfree(obj):
page = virt_to_head_page(obj)
s = page->slab_cache
slab_free(s, page, obj):
<<< gets preempted here

reparent_slab_cache:
for each slab page
[...] page->slab_cache = target_cache;

kmem_cache_destroy(old_cache)

<<< continues execution
c = s->cpu_slab /* s points to the previous owner cache,
so we use-after-free here */

If kfree were not preemptable, we could make reparent_slab_cache wait for all cpus to schedule() before destroying the cache to avoid this, but since it is, we need ref-counting...

Thanks.

Keep the old structure around until the counter of slabs (partial, full) of that old structure is zero? Move individual slab pages until you reach zero.

One can lock out frees by setting c->page = NULL, zapping the partial list and taking the per node list lock.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Currently percpu_ref_tryget fails after percpu_ref_kill is called, even if refcnt != 0. In the next patch I need a method to get a ref that will only fail if refcnt == 0, so let's extend the interface to percpu ref a bit to allow that.

---
 include/linux/percpu-refcount.h |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

index 95961f0bf62d..7729e59b9cb7 100644
@@ -129,7 +129,8 @@ static inline void percpu_ref_get(struct percpu_ref *ref)
  * used.  After the confirm_kill callback is invoked, it's guaranteed that
  * no new reference will be given out by percpu_ref_tryget().
  */
-static inline bool percpu_ref_tryget(struct percpu_ref *ref)
+static inline bool __percpu_ref_tryget(struct percpu_ref *ref,
+				       bool maybe_dead)
 {
 	unsigned __percpu *pcpu_count;
 	int ret = false;

@@ -141,13 +142,19 @@ static inline bool percpu_ref_tryget(struct percpu_ref *ref)
 	if (likely(REF_STATUS(pcpu_count) == PCPU_REF_PTR)) {
 		__this_cpu_inc(*pcpu_count);
 		ret = true;
-	}
+	} else if (maybe_dead && unlikely(atomic_read(&ref->count)))
+		ret = atomic_inc_not_zero(&ref->count);
 
 	rcu_read_unlock_sched();
 
 	return ret;
 }
 
+static inline bool percpu_ref_tryget(struct percpu_ref *ref)
+{
+	return __percpu_ref_tryget(ref, false);
+}
+
 /**
  * percpu_ref_put - decrement a percpu refcount
  * @ref: percpu_ref to put

1.7.10.4

As there won't be any allocations from kmem caches that belong to a dead memcg, it's better to move their slab pages to the parent cache on css offline instead of having them hanging around for indefinite time and bothering about reaping them periodically or on vmpressure.

A tricky thing about it is that there still may be free's to those caches, so we should synchronize with them somehow. This is difficult, because slub implementation is mostly lockless, and there is no magic lock that can be used for this.

This patch solves this problem by switching all free's to dead caches to the "slow" mode while we are migrating their slab pages. The "slow" mode uses the PG_locked bit of the slab for synchronization. Under this lock it just puts the object being freed to the page's freelist not bothering about per cpu/node slab lists, which will be handled by the reparenting procedure. This gives us a clear synchronization point between kfree's and the reparenting procedure - it's the PG_locked bit spin lock, which we take for both freeing an object and changing a slab's slab_cache ptr on reparenting. Although the "slow" mode free is really slow, there shouldn't be lot of them, because they can only happen while reparenting is in progress, which shouldn't take long.

Since the "slow" and the "normal" free's can't coexist at the same time, we must assure all conventional free's have finished before switching all further free's to the "slow" mode and starting reparenting. To achieve that, a percpu refcounter is used. It is taken and held during each "normal" free. The refcounter is killed on memcg offline, and the cache's pages migration is initiated from the refcounter's release function. If we fail to take a ref on kfree, it means all "normal" free's have been completed and the cache is being reparented right now, so we should free the object using the "slow" mode.

---
 include/linux/memcontrol.h |    4 +-
 include/linux/slab.h       |    7 +-
 mm/memcontrol.c            |   54 +++++++-----
 mm/slab.h                  |    5 +-
 mm/slub.c                  |  208 +++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 246 insertions(+), 32 deletions(-)

index aa429de275cc..ce6bb47d59e5 100644
@@ -511,8 +511,8 @@ void memcg_update_array_size(int num_groups);
 struct kmem_cache *
 __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp);
 
-int __memcg_charge_slab(struct kmem_cache *cachep, gfp_t gfp, int order);
-void __memcg_uncharge_slab(struct kmem_cache *cachep, int order);
+int memcg_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, u64 size);
+void memcg_uncharge_kmem(struct mem_cgroup *memcg, u64 size);
 
 int __kmem_cache_destroy_memcg_children(struct kmem_cache *s);
 

index 86e5b26fbdab..ce2189ac4899 100644
@@ -14,6 +14,8 @@
 #include <linux/gfp.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
+#include <linux/rcupdate.h>
+#include <linux/percpu-refcount.h>
 
 
 /*

@@ -526,7 +528,6 @@ static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
  * @memcg: pointer to the memcg this cache belongs to
  * @list: list_head for the list of all caches in this memcg
  * @root_cache: pointer to the global, root cache, this cache was derived from
- * @nr_pages: number of pages that belongs to this cache.
  */
 struct memcg_cache_params {
 	bool is_root_cache;

@@ -539,7 +540,9 @@ struct memcg_cache_params {
 			struct mem_cgroup *memcg;
 			struct list_head list;
 			struct kmem_cache *root_cache;
-			atomic_t nr_pages;
+
+			struct percpu_ref refcnt;
+			struct work_struct destroy_work;
 		};
 	};
 };

index 0eca922ed7a0..c084935a1c29 100644
@@ -186,7 +186,8 @@ static __always_inline int memcg_charge_slab(struct kmem_cache *s,
 		return 0;
 	if (is_root_cache(s))
 		return 0;
-	return __memcg_charge_slab(s, gfp, order);
+	return memcg_charge_kmem(s->memcg_params->memcg, gfp,
+				 PAGE_SIZE << order);
 }
 
 static __always_inline void memcg_uncharge_slab(struct kmem_cache *s, int order)

@@ -195,7 +196,7 @@ static __always_inline void memcg_uncharge_slab(struct kmem_cache *s, int order)
 		return;
 	if (is_root_cache(s))
 		return;
-	__memcg_uncharge_slab(s, order);
+	memcg_uncharge_kmem(s->memcg_params->memcg, PAGE_SIZE << order);
 }
 #else
 static inline bool is_root_cache(struct kmem_cache *s)

1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Argh adding more code to the free path touching more cachelines in the process.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Actually, there is not that much active code added, IMO. In fact, it's only percpu ref get/put for per memcg caches plus a couple of conditionals. The "slow" mode code is meant to be executed very rarely, so we can move it to a separate function under unlikely optimization.

I admit that's far not perfect, because kfree is really a hot path, where every byte of code matters, but unfortunately I don't see how we can avoid this in case we want slab re-parenting.

Again, I'd like to hear from you if there is any point in moving in this direction, or I should give up and concentrate on some other approach, because you'll never accept it.

Thanks.

Do we even know that all objects in that slab belong to a certain cgroup? AFAICT the fastpath currently do not allow to make that distinction.

I wish you would find some other way to do this.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

All allocations from a memcg's cache are accounted to the owner memcg, so that all objects on the same slab belong to the same memcg, a pointer to which can be obtained from the page->slab_cache->memcg_params. At least, this is true since commit faebbfe10ec1 ("sl[au]b: charge slabs to kmemcg explicitly").

The only practical alternative to re-parenting I see right now is periodic reaping, but Johannes isn't very fond of it, and his opinion is quite justified, because having caches that will never be allocated from hanging around indefinitely, only because they have a couple of active objects to be freed, doesn't look very good.

Thanks.

I doubt that. The accounting occurs when a new cpu slab page is allocated. But the individual allocations in the fastpath are not accounted to a specific group. Thus allocation in a slab page can belong to various cgroups.

If all objects in the cache are in use then the slab page needs to hang around since the objects presence is required. You may not know exactly which cgroups these object belong to. The only thing that you may now (if you keep a list of full slabs) is which cgroup was in use then the slab page was initially allocated.

Isnt it sufficient to add a counter of full slabs to a cgroup? When you allocate a new slab page add to the counter. When an object in a slab page is freed and the slab page goes on a partial list decrement the counter.

That way you can avoid tracking full slabs.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

On each kmalloc, we pick the cache that belongs to the current memcg, and allocate objects from that cache (see memcg_kmem_get_cache()). And all slab pages allocated for a per memcg cache are accounted to the memcg the cache belongs to (see memcg_charge_slab). So currently, each kmem cache, i.e. each slab of it, can only have objects of one cgroup, namely its owner.

The idea was to destroy all memcg's caches on css offline no matter whether there are active objects on them or not. For that, we have to migrate *all* slabs to the parent cache, which means, at least, changing page->slab_cache ptr on them. For the latter, we have to walk through all slabs (including full ones), which means we have to track them all.

Anyway, you said we shouldn't track full slabs, because it neglects cpu-partial/frozen lists optimization. Actually, I agree with you at this point: I did some testing and found that contended kfrees to the same NUMA node on a 2-node machine are slowed down up to 25% due to full slabs tracking - not an option obviously.

OK, it seems we have no choice but keeping dead caches left after memcg offline until they have active slabs. How can we get rid of them then? Simply counting slabs on cache and destroying cache when the count goes to 0 isn't enough, because slub may keep some free slabs by default (if they are frozen e.g.) Reaping them periodically doesn't look nice.

What if we modify __slab_free so that it won't keep empty slabs for dead caches? That way we would only have to count slabs allocated to a cache, and destroy caches as soon as the counter drops to 0. No periodic/vmpressure reaping would be necessary. I attached the patch that does the trick below. The changes it introduces to __slab_free do not look very intrusive to me. Could you please take a look at it (to diff slub.c primarily) when you have time, and say if, in your opinion, the changes to __slab_free are acceptable or not?

Thank you.

--

index 1d9abb7d22a0..9ad536a756ea 100644
@@ -526,7 +526,8 @@ static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
  * @memcg: pointer to the memcg this cache belongs to
  * @list: list_head for the list of all caches in this memcg
  * @root_cache: pointer to the global, root cache, this cache was derived from
- * @nr_pages: number of pages that belongs to this cache.
+ * @count: the ref count; the cache is destroyed as soon as it reaches 0
+ * @unregister_work: the cache destruction work
  */
 struct memcg_cache_params {
 	bool is_root_cache;

@@ -539,11 +540,32 @@ struct memcg_cache_params {
 			struct mem_cgroup *memcg;
 			struct list_head list;
 			struct kmem_cache *root_cache;
-			atomic_t nr_pages;
+			atomic_t count;
+			struct work_struct unregister_work;
 		};
 	};
 };
 
+/*
+ * Each active slab increments the cache's memcg_params->count, and the owner
+ * memcg, while it's online, adds MEMCG_PARAMS_COUNT_BIAS to the count so that
+ * the cache is dead (i.e. belongs to a memcg that was turned offline) iff
+ * memcg_params->count < MEMCG_PARAMS_COUNT_BIAS.
+ */
+#define MEMCG_PARAMS_COUNT_BIAS		(1U << 31)
+
+/* Returns true if the cache belongs to a memcg that was turned offline. */
+static inline bool memcg_cache_dead(struct kmem_cache *s)
+{
+	bool ret = false;
+
+#ifdef CONFIG_MEMCG_KMEM
+	if (atomic_read(&s->memcg_params->count) < MEMCG_PARAMS_COUNT_BIAS)
+		ret = true;
+#endif
+	return ret;
+}
+
 int memcg_update_all_caches(int num_memcgs);
 
 struct seq_file;

index 0fb108e5b905..2b076f17eae7 100644
@@ -3090,6 +3090,8 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
 	return 0;
 }
 
+static void memcg_unregister_cache_func(struct work_struct *w);
+
 int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
 			     struct kmem_cache *root_cache)
 {

@@ -3111,6 +3113,9 @@ int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
 	if (memcg) {
 		s->memcg_params->memcg = memcg;
 		s->memcg_params->root_cache = root_cache;
+		atomic_set(&s->memcg_params->count, MEMCG_PARAMS_COUNT_BIAS);
+		INIT_WORK(&s->memcg_params->unregister_work,
+			  memcg_unregister_cache_func);
 		css_get(&memcg->css);
 	} else
 		s->memcg_params->is_root_cache = true;

@@ -3192,6 +3197,17 @@ static void memcg_unregister_cache(struct kmem_cache *cachep)
 	kmem_cache_destroy(cachep);
 }
 
+static void memcg_unregister_cache_func(struct work_struct *w)
+{
+	struct memcg_cache_params *params =
+		container_of(w, struct memcg_cache_params, unregister_work);
+	struct kmem_cache *cachep = memcg_params_to_cache(params);
+
+	mutex_lock(&memcg_slab_mutex);
+	memcg_unregister_cache(cachep);
+	mutex_unlock(&memcg_slab_mutex);
+}
+
 /*
  * During the creation a new cache, we need to disable our accounting mechanism
  * altogether. This is true even if we are not creating, but rather just

@@ -3254,8 +3270,21 @@ static void memcg_unregister_all_caches(struct mem_cgroup *memcg)
 	mutex_lock(&memcg_slab_mutex);
 	list_for_each_entry_safe(params, tmp, &memcg->memcg_slab_caches, list) {
 		cachep = memcg_params_to_cache(params);
+
+		/* mark the cache as dead while still holding a ref to it */
+		atomic_sub(MEMCG_PARAMS_COUNT_BIAS - 1, &params->count);
+
+		/*
+		 * Make sure that all ongoing free's see the cache as dead and
+		 * won't do unnecessary slab caching (freezing in case of
+		 * slub, see comment to __slab_free).
+		 */
+		synchronize_sched();
+
 		kmem_cache_shrink(cachep);
-		if (atomic_read(&cachep->memcg_params->nr_pages) == 0)
+
+		/* if nobody except us uses the cache, destroy it immediately */
+		if (atomic_dec_and_test(&params->count))
 			memcg_unregister_cache(cachep);
 	}
 	mutex_unlock(&memcg_slab_mutex);

@@ -3329,14 +3358,15 @@ int __memcg_charge_slab(struct kmem_cache *cachep, gfp_t gfp, int order)
 	res = memcg_charge_kmem(cachep->memcg_params->memcg, gfp,
 				PAGE_SIZE << order);
 	if (!res)
-		atomic_add(1 << order, &cachep->memcg_params->nr_pages);
+		atomic_inc(&cachep->memcg_params->count);
 	return res;
 }
 
 void __memcg_uncharge_slab(struct kmem_cache *cachep, int order)
 {
 	memcg_uncharge_kmem(cachep->memcg_params->memcg, PAGE_SIZE << order);
-	atomic_sub(1 << order, &cachep->memcg_params->nr_pages);
+	if (atomic_dec_and_test(&cachep->memcg_params->count))
+		schedule_work(&cachep->memcg_params->unregister_work);
 }
 
 /*

index fdf0fe4da9a9..da7a3edde2cd 100644
@@ -2550,6 +2550,7 @@ static void __slab_free(struct kmem_cache *s, struct page *page,
 	unsigned long counters;
 	struct kmem_cache_node *n = NULL;
 	unsigned long uninitialized_var(flags);
+	int cache_dead = 0;
 
 	stat(s, FREE_SLOWPATH);
 

@@ -2557,6 +2558,32 @@ static void __slab_free(struct kmem_cache *s, struct page *page,
 		!(n = free_debug_processing(s, page, x, addr, &flags)))
 		return;
 
+	if (!is_root_cache(s)) {
+		/*
+		 * We must not keep free slabs after the memcg the cache
+		 * belongs to is turned offline, otherwise the cache will be
+		 * hanging around for ever. For the slub implementation that
+		 * means we must not freeze slabs and we must ignore the
+		 * min_partial limit for dead caches.
+		 *
+		 * There is one subtle thing, however. If we get preempted
+		 * after we see the cache alive and before we try to
+		 * cmpxchg-free the object to it, the cache may be killed in
+		 * between and we may occasionally freeze a slab for a dead
+		 * cache.
+		 *
+		 * We avoid this by disabling preemption before the check if
+		 * the cache is dead and re-enabling it after cmpxchg-free,
+		 * where we can freeze a slab. Then, to assure a dead cache
+		 * won't have got frozen slabs it's enough to
+		 * synchronize_sched() after marking the cache dead and before
+		 * shrinking it (see memcg_unregister_all_caches()).
+		 */
+		preempt_disable();
+		if (memcg_cache_dead(s))
+			cache_dead = 1;
+	}
+
 	do {
 		if (unlikely(n)) {
 			spin_unlock_irqrestore(&n->list_lock, flags);

@@ -2570,7 +2597,8 @@ static void __slab_free(struct kmem_cache *s, struct page *page,
 		new.inuse--;
 		if ((!new.inuse || !prior) && !was_frozen) {
 
-			if (kmem_cache_has_cpu_partial(s) && !prior) {
+			if (kmem_cache_has_cpu_partial(s) && !prior &&
+			    !cache_dead) {
 
 				/*
 				 * Slab was on no list before and will be

@@ -2601,6 +2629,9 @@ static void __slab_free(struct kmem_cache *s, struct page *page,
 		object, new.counters,
 		"__slab_free"));
 
+	if (!is_root_cache(s))
+		preempt_enable();
+
 	if (likely(!n)) {
 
 		/*

@@ -2620,14 +2651,16 @@ static void __slab_free(struct kmem_cache *s, struct page *page,
                 return;
         }
 
-	if (unlikely(!new.inuse && n->nr_partial > s->min_partial))
+	if (unlikely(!new.inuse &&
+		     (n->nr_partial > s->min_partial || cache_dead)))
 		goto slab_empty;
 
 	/*
 	 * Objects left in the slab. If it was not on the partial list before
 	 * then add it.
 	 */
-	if (!kmem_cache_has_cpu_partial(s) && unlikely(!prior)) {
+	if ((!kmem_cache_has_cpu_partial(s) || cache_dead) &&
+	    unlikely(!prior)) {
 		if (kmem_cache_debug(s))
 			remove_full(s, n, page);
 		add_partial(n, page, DEACTIVATE_TO_TAIL);

Ok that works for kmalloc. What about dentry/inodes and so on?

Then they are moved to a list and therefore you can move them to yours I think.

But those are only limited to one slab per cpu ( plus eventual cpu partial ones but you can switch that feature off).

Well that should already be in there. Se s->min_partial to zero?

Looking now.

19.05.2014 20:03, Christoph Lameter:

The same. Actually, by kmalloc I meant kmem_cache_alloc, or slab_alloc_node, to be more exact.

AFAIU slub can keep free slabs in:

1) One per cpu slab. This is the easiest thing to handle - we only need to shrink the cache, because this slab can only be added on allocation, not on free.

2) Per node partial slab lists. Free slabs can be added there on frees, but only if min_partial > 0, so setting min_partial = 0 would solve that, just as you pointed below.

3) Per cpu partial slabs. We can disable this feature for dead caches by adding appropriate check to kmem_cache_has_cpu_partial.

So far, everything looks very simple - it seems we don't have to modify __slab_free at all if we follow the instruction above.

However, there is one thing regarding preemptable kernels. The problem is after forbidding the cache store free slabs in per-cpu/node partial lists by setting min_partial=0 and kmem_cache_has_cpu_partial=false (i.e. marking the cache as dead), we have to make sure that all frees that saw the cache as alive are over, otherwise they can occasionally add a free slab to a per-cpu/node partial list *after* the cache was marked dead. For instance,

CPU0                            CPU1
----                            ----
memcg offline:                  __slab_free:
                                  // let the slab we're freeing the obj
                                  // to is full, so we are considering
                                  // freezing it;
                                  // since kmem_cache_has_cpu_partial
                                  // returns true, we'll try to freeze
                                  // it;
                                  new.frozen=1

                                  // but before proceeding to cmpxchg
                                  // we get preempted

mark cache s dead:
s->min_partial=0
make kmem_cache_has_cpu_partial return false

kmem_cache_shrink(s) // this removes all empty slabs from

                       // per cpu/node partial lists

                                  // when this thread continues to
                                  // __slab_free, the cache is dead
                                  // and no slabs must be added to
                                  // per-cpu partial list, but the
                                  // following cmpxchg may succeed
                                  // in freezing the slab
                                  cmpxchg_double_slab(s, page,
                                      prior, counters,
                                      object, new.counters)

As a result, we'll eventually end up with a free slab on a per-cpu partial list, so that the cache refcounter will never drop to zero and the cache leaks.

This is very unlikely, but still possible. To avoid that, we should assure all __slab_free's like that running on CPU1 are over before proceeding to kmem_cache_shrink on memcg offline. Currently, it is impossible to achieve that on fully preemptable kernels w/o modifying __slab_free, AFAIU. That's what all that trickery in __slab_free about.

Makes sense or am I missing something?

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

On Mon, May 19, 2014 at 10:27:51PM +0400, Vladimir Davydov wrote:
[...]

Seems I've found a better way to avoid this race, which does not involve messing up free hot paths. The idea is to explicitly zap each per-cpu partial list by setting it pointing to an invalid ptr. Since put_cpu_partial(), which is called from __slab_free(), uses atomic cmpxchg for adding a new partial slab to a per cpu partial list, it is enough to add a check if partials are zapped there and bail out if so.

The patch doing the trick is attached. Could you please take a look at it once time permit?

Thank you.

--

index 1d9abb7d22a0..c1e318247248 100644
@@ -526,7 +526,8 @@ static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
  * @memcg: pointer to the memcg this cache belongs to
  * @list: list_head for the list of all caches in this memcg
  * @root_cache: pointer to the global, root cache, this cache was derived from
- * @nr_pages: number of pages that belongs to this cache.
+ * @count: the ref count; the cache is destroyed as soon as it reaches 0
+ * @unregister_work: the cache destruction work
  */
 struct memcg_cache_params {
 	bool is_root_cache;

@@ -539,11 +540,20 @@ struct memcg_cache_params {
 			struct mem_cgroup *memcg;
 			struct list_head list;
 			struct kmem_cache *root_cache;
-			atomic_t nr_pages;
+			atomic_t count;
+			struct work_struct unregister_work;
 		};
 	};
 };
 
+/*
+ * Each active slab increments the cache's memcg_params->count, and the owner
+ * memcg, while it's online, adds MEMCG_PARAMS_COUNT_BIAS to the count so that
+ * the cache is dead (i.e. belongs to a memcg that was turned offline) iff
+ * memcg_params->count < MEMCG_PARAMS_COUNT_BIAS.
+ */
+#define MEMCG_PARAMS_COUNT_BIAS		(1U << 31)
+
 int memcg_update_all_caches(int num_memcgs);
 
 struct seq_file;

index 0fb108e5b905..6c922dd96fd5 100644
@@ -3090,6 +3090,8 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
 	return 0;
 }
 
+static void memcg_unregister_cache_func(struct work_struct *w);
+
 int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
 			     struct kmem_cache *root_cache)
 {

@@ -3111,6 +3113,9 @@ int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
 	if (memcg) {
 		s->memcg_params->memcg = memcg;
 		s->memcg_params->root_cache = root_cache;
+		atomic_set(&s->memcg_params->count, MEMCG_PARAMS_COUNT_BIAS);
+		INIT_WORK(&s->memcg_params->unregister_work,
+			  memcg_unregister_cache_func);
 		css_get(&memcg->css);
 	} else
 		s->memcg_params->is_root_cache = true;

@@ -3192,6 +3197,17 @@ static void memcg_unregister_cache(struct kmem_cache *cachep)
 	kmem_cache_destroy(cachep);
 }
 
+static void memcg_unregister_cache_func(struct work_struct *w)
+{
+	struct memcg_cache_params *params =
+		container_of(w, struct memcg_cache_params, unregister_work);
+	struct kmem_cache *cachep = memcg_params_to_cache(params);
+
+	mutex_lock(&memcg_slab_mutex);
+	memcg_unregister_cache(cachep);
+	mutex_unlock(&memcg_slab_mutex);
+}
+
 /*
  * During the creation a new cache, we need to disable our accounting mechanism
  * altogether. This is true even if we are not creating, but rather just

@@ -3254,8 +3270,14 @@ static void memcg_unregister_all_caches(struct mem_cgroup *memcg)
 	mutex_lock(&memcg_slab_mutex);
 	list_for_each_entry_safe(params, tmp, &memcg->memcg_slab_caches, list) {
 		cachep = memcg_params_to_cache(params);
+
+		/* mark the cache as dead while still holding a ref to it */
+		atomic_sub(MEMCG_PARAMS_COUNT_BIAS - 1, &params->count);
+
 		kmem_cache_shrink(cachep);
-		if (atomic_read(&cachep->memcg_params->nr_pages) == 0)
+
+		/* if nobody except us uses the cache, destroy it immediately */
+		if (atomic_dec_and_test(&params->count))
 			memcg_unregister_cache(cachep);
 	}
 	mutex_unlock(&memcg_slab_mutex);

@@ -3329,14 +3351,15 @@ int __memcg_charge_slab(struct kmem_cache *cachep, gfp_t gfp, int order)
 	res = memcg_charge_kmem(cachep->memcg_params->memcg, gfp,
 				PAGE_SIZE << order);
 	if (!res)
-		atomic_add(1 << order, &cachep->memcg_params->nr_pages);
+		atomic_inc(&cachep->memcg_params->count);
 	return res;
 }
 
 void __memcg_uncharge_slab(struct kmem_cache *cachep, int order)
 {
 	memcg_uncharge_kmem(cachep->memcg_params->memcg, PAGE_SIZE << order);
-	atomic_sub(1 << order, &cachep->memcg_params->nr_pages);
+	if (atomic_dec_and_test(&cachep->memcg_params->count))
+		schedule_work(&cachep->memcg_params->unregister_work);
 }
 
 /*

index 961a3fb1f5a2..996968c55222 100644
@@ -179,6 +179,14 @@ static inline struct kmem_cache *memcg_root_cache(struct kmem_cache *s)
 	return s->memcg_params->root_cache;
 }
 
+/* Returns true if the cache belongs to a memcg that was turned offline. */
+static inline bool memcg_cache_dead(struct kmem_cache *s)
+{
+	return !is_root_cache(s) &&
+		atomic_read(&s->memcg_params->count) < MEMCG_PARAMS_COUNT_BIAS;
+}
+
+
 static __always_inline int memcg_charge_slab(struct kmem_cache *s,
 					     gfp_t gfp, int order)
 {

@@ -225,6 +233,11 @@ static inline struct kmem_cache *memcg_root_cache(struct kmem_cache *s)
 	return s;
 }
 
+static inline bool memcg_cache_dead(struct kmem_cache *s)
+{
+	return false;
+}
+
 static inline int memcg_charge_slab(struct kmem_cache *s, gfp_t gfp, int order)
 {
 	return 0;

Well if you set s->cpu_partial = 0 then the slab should not be added to the partial lists. Ok its put on there temporarily but then immediately moved to the node partial list in put_cpu_partial().

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Don't think so. AFAIU put_cpu_partial() first checks if the per-cpu partial list has more than s->cpu_partial objects draining it if so, but then it adds the newly frozen slab there anyway.

Thanks.

Hmmm... Ok true. Maybe insert some code there then.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Agree, it's better to add the check to put_cpu_partial() rather than to __slab_free(), because the latter is a hot path.

I'll send the patches soon.

Thank you!

There is already a s->cpu_partial number in kmem_cache. If that is zero then no partial cpu slabs should be kept.

Ok then lets switch off preeempt there? Preemption is not supported by most distribution and so will have the least impact.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Do I understand you correctly that the following change looks OK to you?

index fdf0fe4da9a9..dc3582c2b5bb 100644
@@ -2676,31 +2676,31 @@ static __always_inline void slab_free(struct kmem_cache *s,
 redo:
 	/*
 	 * Determine the currently cpus per cpu slab.
 	 * The cpu may change afterward. However that does not matter since
 	 * data is retrieved via this pointer. If we are on the same cpu
 	 * during the cmpxchg then the free will succedd.
 	 */
 	preempt_disable();
 	c = this_cpu_ptr(s->cpu_slab);
 
 	tid = c->tid;
-	preempt_enable();
 
 	if (likely(page == c->page)) {
 		set_freepointer(s, object, c->freelist);
 
 		if (unlikely(!this_cpu_cmpxchg_double(
 				s->cpu_slab->freelist, s->cpu_slab->tid,
 				c->freelist, tid,
 				object, next_tid(tid)))) {
 
 			note_cmpxchg_failure("slab_free", s, tid);
 			goto redo;
 		}
 		stat(s, FREE_FASTPATH);
 	} else
 		__slab_free(s, page, x, addr);
 
+	preempt_enable();
 }
 
 void kmem_cache_free(struct kmem_cache *s, void *x)

Almost. Preemption needs to be enabled before functions that invoke the page allocator etc etc.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

I need to disable preemption only in slab_free, which never blocks according to its semantics, so everything should be fine just like that.

slab_free calls __slab_free which can release slabs via put_cpu_partial()/unfreeze_partials()/discard_slab() to the page allocator. I'd rather have preemption enabled there.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Hmm, why? IMO, calling __free_pages with preempt disabled won't hurt latency, because it proceeds really fast. BTW, we already call it for a bunch of pages from __slab_free() -> put_cpu_partial() -> unfreeze_partials() with irqs disabled, which is harder. FWIW, SLAB has the whole obj free path executed under local_irq_save/restore, and it doesn't bother enabling irqs for freeing pages.

IMO, the latency improvement we can achieve by enabling preemption while calling __free_pages is rather minor, and it isn't worth complicating the code.

Thanks.

If you look at the end of unfreeze_partials() you see that we release locks and therefore enable preempt before calling into the page allocator.

You never know what other new features they are going to be adding to the page allocator. I'd rather be safe than sorry on this one. We have had some trouble in the past with some debugging logic triggering.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Yes, we release the node's list_lock before calling discard_slab(), but we don't enable irqs, which are disabled in put_cpu_partial(), just before calling it, so we call the page allocator with irqs off and therefore preemption disabled.

I guess by "some troubles in the past with some debugging logic triggering" you mean the issue that was fixed by commit 9ada19342b244 ?

From: Shaohua Li <[email protected]>

slub: move discard_slab out of node lock

Lockdep reports there is potential deadlock for slub node list_lock. discard_slab() is called with the lock hold in unfreeze_partials(), which could trigger a slab allocation, which could hold the lock again.

discard_slab() doesn't need hold the lock actually, if the slab is already removed from partial list.

If so - nothing to worry about, because I'm not going to make calls to the page allocator under an internal slab lock. What I propose is calling __free_pages with preempt disabled, which already happens here and there and can't result in deadlocks or lockdep warns.

Thanks.

Ok that is something that could be fixed.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>