memcg: make memcg's life cycle the same as cgroup

Use css_get/css_put instead of mem_cgroup_get/put.

Note, if at the same time someone is moving @current to a different cgroup and removing the old cgroup, css_tryget() may return false, and sock->sk_cgrp won't be initialized.

Signed-off-by Li Zefan <[email protected]>

---
 mm/memcontrol.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

mm/memcontrol.c +1 -1

index 23d0f6e..43ca91d 100644
@@ -536,15 +536,15 @@ void sock_update_memcg(struct sock *sk)
 		 */
 		if (sk->sk_cgrp) {
 			BUG_ON(mem_cgroup_is_root(sk->sk_cgrp->memcg));
-			mem_cgroup_get(sk->sk_cgrp->memcg);
+			css_get(&sk->sk_cgrp->memcg->css);
 			return;
 		}

Quoted 11 lines · Li Zefan

Use css_get/css_put instead of mem_cgroup_get/put.

Note, if at the same time someone is moving @current to a different cgroup and removing the old cgroup, css_tryget() may return false, and sock->sk_cgrp won't be initialized.

Signed-off-by Li Zefan <[email protected]>

---
 mm/memcontrol.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

mm/memcontrol.c void sock_update_memcg(struct sock *sk) · line 539

Quoted 10 lines

mm/memcontrol.c +1 -1

index 23d0f6e..43ca91d 100644
@@ -536,15 +536,15 @@ void sock_update_memcg(struct sock *sk)
 		 */
 		if (sk->sk_cgrp) {
 			BUG_ON(mem_cgroup_is_root(sk->sk_cgrp->memcg));
-			mem_cgroup_get(sk->sk_cgrp->memcg);
+			css_get(&sk->sk_cgrp->memcg->css);

mm/memcontrol.c void sock_update_memcg(struct sock *sk) · line 549

Quoted 12 lines

mm/memcontrol.c +2 -2

 			return;
 		}
 
 		rcu_read_lock();
 		memcg = mem_cgroup_from_task(current);
 		cg_proto = sk->sk_prot->proto_cgroup(memcg);
-		if (!mem_cgroup_is_root(memcg) && memcg_proto_active(cg_proto)) {
-			mem_cgroup_get(memcg);
+		if (!mem_cgroup_is_root(memcg) &&
+		    memcg_proto_active(cg_proto) && css_tryget(&memcg->css)) {
 			sk->sk_cgrp = cg_proto;
 		}

What happens if this tryget fails ? Won't we leak a reference here? We will put regardless when the socket is released, and this may go negative. No?

Quoted 21 lines · Glauber Costa

Quoted 20 lines · Li Zefan

Use css_get/css_put instead of mem_cgroup_get/put.

Note, if at the same time someone is moving @current to a different cgroup and removing the old cgroup, css_tryget() may return false, and sock->sk_cgrp won't be initialized.

Signed-off-by Li Zefan <[email protected]>

---
 mm/memcontrol.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

mm/memcontrol.c +0 -1

index 23d0f6e..43ca91d 100644
@@ -536,15 +536,15 @@ void sock_update_memcg(struct sock *sk)
 		 */
 		if (sk->sk_cgrp) {
 			BUG_ON(mem_cgroup_is_root(sk->sk_cgrp->memcg));
-			mem_cgroup_get(sk->sk_cgrp->memcg);

Quoted 1 line

mm/memcontrol.c void sock_update_memcg(struct sock *sk) · line 539

Quoted 1 line

mm/memcontrol.c +1 -0

+			css_get(&sk->sk_cgrp->memcg->css);

I am not sure I understand this one. So we have a goup here (which means that somebody already took a reference on it, right?) and we are taking another reference. If this is released by sock_release_memcg then who releases the previous one? It is not directly related to the patch because this has been done previously already. Could you clarify Glauber, please?

Quoted 4 lines

Quoted 4 lines

mm/memcontrol.c +0 -0

 			return;
 		}
 

Quoted 13 lines

mm/memcontrol.c void sock_update_memcg(struct sock *sk) · line 549

Quoted 9 lines

mm/memcontrol.c +2 -2

 		rcu_read_lock();
 		memcg = mem_cgroup_from_task(current);
 		cg_proto = sk->sk_prot->proto_cgroup(memcg);
-		if (!mem_cgroup_is_root(memcg) && memcg_proto_active(cg_proto)) {
-			mem_cgroup_get(memcg);
+		if (!mem_cgroup_is_root(memcg) &&
+		    memcg_proto_active(cg_proto) && css_tryget(&memcg->css)) {
 			sk->sk_cgrp = cg_proto;
 		}

What happens if this tryget fails ? Won't we leak a reference here? We will put regardless when the socket is released, and this may go negative. No?

AFAICS sock_release_memcg releases the reference only if sk->sk_cgrp and that one wouldn't be set if css_tryget fails.

Quoted 22 lines · Michal Hocko

Quoted 21 lines · Glauber Costa

Quoted 20 lines · Li Zefan

Use css_get/css_put instead of mem_cgroup_get/put.

Note, if at the same time someone is moving @current to a different cgroup and removing the old cgroup, css_tryget() may return false, and sock->sk_cgrp won't be initialized.

Signed-off-by Li Zefan <[email protected]>

---
 mm/memcontrol.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

mm/memcontrol.c +0 -1

index 23d0f6e..43ca91d 100644
@@ -536,15 +536,15 @@ void sock_update_memcg(struct sock *sk)
 		 */
 		if (sk->sk_cgrp) {
 			BUG_ON(mem_cgroup_is_root(sk->sk_cgrp->memcg));
-			mem_cgroup_get(sk->sk_cgrp->memcg);

Quoted 8 lines

Quoted 1 line

mm/memcontrol.c void sock_update_memcg(struct sock *sk) · line 539

Quoted 1 line

mm/memcontrol.c +1 -0

+			css_get(&sk->sk_cgrp->memcg->css);

I am not sure I understand this one. So we have a goup here (which means that somebody already took a reference on it, right?) and we are taking another reference. If this is released by sock_release_memcg then who releases the previous one? It is not directly related to the patch because this has been done previously already. Could you clarify Glauber, please?

This should be documented in the commit that introduced this, and it was one of the first bugs I've handled with this code.

Bottom line, we can create sockets normally, and those will have process context. But we also can create sockets by cloning existing sockets. To the best of my knowledge, this is done by things like accept().

Because those sockets are a clone of their ancestors, they also belong to a workload that should be limited. Also note that because they have cgroup context, we will eventually try to put them. So we need to grab an extra reference.

socket_update_cgroup is always called at socket creation, and the original structures are filled with zeroes. Therefore cloning is the *only* path that takes us here with sk->sk_cgroup filled.

OK, I guess I understand.

Thanks for the clarification, Galuber!

Use css_get/css_put instead of mem_cgroup_get/put.

Note, if at the same time someone is moving @current to a different cgroup and removing the old cgroup, css_tryget() may return false, and sock->sk_cgrp won't be initialized.

Signed-off-by Li Zefan <[email protected]>

Use css_get/css_put instead of mem_cgroup_get/put.

Note, if at the same time someone is moving @current to a different cgroup and removing the old cgroup, css_tryget() may return false, and sock->sk_cgrp won't be initialized.

Signed-off-by Li Zefan <[email protected]>

---
 mm/memcontrol.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

mm/memcontrol.c +4 -4

index 23d0f6e..43ca91d 100644
@@ -536,15 +536,15 @@ void sock_update_memcg(struct sock *sk)
 		 */
 		if (sk->sk_cgrp) {
 			BUG_ON(mem_cgroup_is_root(sk->sk_cgrp->memcg));
-			mem_cgroup_get(sk->sk_cgrp->memcg);
+			css_get(&sk->sk_cgrp->memcg->css);
 			return;
 		}
 
 		rcu_read_lock();
 		memcg = mem_cgroup_from_task(current);
 		cg_proto = sk->sk_prot->proto_cgroup(memcg);
-		if (!mem_cgroup_is_root(memcg) && memcg_proto_active(cg_proto)) {
-			mem_cgroup_get(memcg);
+		if (!mem_cgroup_is_root(memcg) &&
+		    memcg_proto_active(cg_proto) && css_tryget(&memcg->css)) {
 			sk->sk_cgrp = cg_proto;
 		}
 		rcu_read_unlock();

@@ -558,7 +558,7 @@ void sock_release_memcg(struct sock *sk)
 		struct mem_cgroup *memcg;
 		WARN_ON(!sk->sk_cgrp->memcg);
 		memcg = sk->sk_cgrp->memcg;
-		mem_cgroup_put(memcg);
+		css_put(&sk->sk_cgrp->memcg->css);
 	}
 }
 

Signature · 1 line

1.8.0.2

Use css_get()/css_put() instead of mem_cgroup_get()/mem_cgroup_put().

Signed-off-by Li Zefan <[email protected]>

---
 mm/memcontrol.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

mm/memcontrol.c +5 -3

index 43ca91d..dafacb8 100644
@@ -3191,7 +3191,7 @@ void memcg_release_cache(struct kmem_cache *s)
 	list_del(&s->memcg_params->list);
 	mutex_unlock(&memcg->slab_caches_mutex);
 
-	mem_cgroup_put(memcg);
+	css_put(&memcg->css);
 out:
 	kfree(s->memcg_params);
 }

@@ -3350,16 +3350,18 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 
 	mutex_lock(&memcg_cache_mutex);
 	new_cachep = cachep->memcg_params->memcg_caches[idx];
-	if (new_cachep)
+	if (new_cachep) {
+		css_put(&memcg->css);
 		goto out;
+	}
 
 	new_cachep = kmem_cache_dup(memcg, cachep);
 	if (new_cachep == NULL) {
 		new_cachep = cachep;
+		css_put(&memcg->css);
 		goto out;
 	}
 
-	mem_cgroup_get(memcg);
 	atomic_set(&new_cachep->memcg_params->nr_pages , 0);
 
 	cachep->memcg_params->memcg_caches[idx] = new_cachep;

Use css_get()/css_put() instead of mem_cgroup_get()/mem_cgroup_put().

Signed-off-by Li Zefan <[email protected]>

---
 mm/memcontrol.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

mm/memcontrol.c +5 -3

index 43ca91d..dafacb8 100644
@@ -3191,7 +3191,7 @@ void memcg_release_cache(struct kmem_cache *s)
 	list_del(&s->memcg_params->list);
 	mutex_unlock(&memcg->slab_caches_mutex);
 
-	mem_cgroup_put(memcg);
+	css_put(&memcg->css);
 out:
 	kfree(s->memcg_params);
 }

@@ -3350,16 +3350,18 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 
 	mutex_lock(&memcg_cache_mutex);
 	new_cachep = cachep->memcg_params->memcg_caches[idx];
-	if (new_cachep)
+	if (new_cachep) {
+		css_put(&memcg->css);
 		goto out;
+	}
 
 	new_cachep = kmem_cache_dup(memcg, cachep);
 	if (new_cachep == NULL) {
 		new_cachep = cachep;
+		css_put(&memcg->css);
 		goto out;
 	}
 
-	mem_cgroup_get(memcg);
 	atomic_set(&new_cachep->memcg_params->nr_pages , 0);
 
 	cachep->memcg_params->memcg_caches[idx] = new_cachep;

Quoted 43 lines · Li Zefan

Use css_get()/css_put() instead of mem_cgroup_get()/mem_cgroup_put().

Signed-off-by Li Zefan <[email protected]>

---
 mm/memcontrol.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

mm/memcontrol.c +5 -3

index 43ca91d..dafacb8 100644
@@ -3191,7 +3191,7 @@ void memcg_release_cache(struct kmem_cache *s)
 	list_del(&s->memcg_params->list);
 	mutex_unlock(&memcg->slab_caches_mutex);
 
-	mem_cgroup_put(memcg);
+	css_put(&memcg->css);
 out:
 	kfree(s->memcg_params);
 }

@@ -3350,16 +3350,18 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 
 	mutex_lock(&memcg_cache_mutex);
 	new_cachep = cachep->memcg_params->memcg_caches[idx];
-	if (new_cachep)
+	if (new_cachep) {
+		css_put(&memcg->css);
 		goto out;
+	}
 
 	new_cachep = kmem_cache_dup(memcg, cachep);
 	if (new_cachep == NULL) {
 		new_cachep = cachep;
+		css_put(&memcg->css);
 		goto out;
 	}
 
-	mem_cgroup_get(memcg);
 	atomic_set(&new_cachep->memcg_params->nr_pages , 0);
 
 	cachep->memcg_params->memcg_caches[idx] = new_cachep;

@@ -3449,8 +3451,6 @@ static void memcg_create_cache_work_func(struct work_struct *w)
 

mm/memcontrol.c static void memcg_create_cache_work_func(struct work_struct *w) · line 3455

Quoted 6 lines

mm/memcontrol.c +0 -2

 	cw = container_of(w, struct create_work, work);
 	memcg_create_kmem_cache(cw->memcg, cw->cachep);
-	/* Drop the reference gotten when we enqueued. */
-	css_put(&cw->memcg->css);
 	kfree(cw);
 }

You are putting references but I do not see any single css_{try}get here. /me puzzled.

Quoted 44 lines · Michal Hocko

Quoted 43 lines · Li Zefan

Use css_get()/css_put() instead of mem_cgroup_get()/mem_cgroup_put().

Signed-off-by Li Zefan <[email protected]>

---
 mm/memcontrol.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

mm/memcontrol.c +5 -3

index 43ca91d..dafacb8 100644
@@ -3191,7 +3191,7 @@ void memcg_release_cache(struct kmem_cache *s)
 	list_del(&s->memcg_params->list);
 	mutex_unlock(&memcg->slab_caches_mutex);
 
-	mem_cgroup_put(memcg);
+	css_put(&memcg->css);
 out:
 	kfree(s->memcg_params);
 }

@@ -3350,16 +3350,18 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 
 	mutex_lock(&memcg_cache_mutex);
 	new_cachep = cachep->memcg_params->memcg_caches[idx];
-	if (new_cachep)
+	if (new_cachep) {
+		css_put(&memcg->css);
 		goto out;
+	}
 
 	new_cachep = kmem_cache_dup(memcg, cachep);
 	if (new_cachep == NULL) {
 		new_cachep = cachep;
+		css_put(&memcg->css);
 		goto out;
 	}
 
-	mem_cgroup_get(memcg);
 	atomic_set(&new_cachep->memcg_params->nr_pages , 0);
 
 	cachep->memcg_params->memcg_caches[idx] = new_cachep;

@@ -3449,8 +3451,6 @@ static void memcg_create_cache_work_func(struct work_struct *w)
 

Quoted 10 lines

mm/memcontrol.c static void memcg_create_cache_work_func(struct work_struct *w) · line 3455

Quoted 6 lines

mm/memcontrol.c +0 -2

 	cw = container_of(w, struct create_work, work);
 	memcg_create_kmem_cache(cw->memcg, cw->cachep);
-	/* Drop the reference gotten when we enqueued. */
-	css_put(&cw->memcg->css);
 	kfree(cw);
 }

You are putting references but I do not see any single css_{try}get here. /me puzzled.

There are two things being done in this code:
First, we acquired a css_ref to make sure that the underlying cgroup would not go away. That is a short lived reference, and it is put as soon as the cache is created.
At this point, we acquire a long-lived per-cache memcg reference count to guarantee that the memcg will still be alive.

so it is:

enqueue: css_get
create : memcg_get, css_put
destroy: css_put

If I understand Li's patch correctly, he is not touching the first css_get, only turning that into the long lived reference (which was not possible before, since that would prevent rmdir).

Then he only needs to get rid of the memcg_get, change the memcg_put to css_put, and get rid of the now extra css_put.

He is issuing extra css_puts in memcg_create_kmem_cache, but only in failure paths. So the code reads as:
* css_get on enqueue (already done, so not shown in patch)
* if it fails, css_put
* if it succeeds, don't do anything. This is already the long-lived reference count. put it at release time.

The code looks correct, and of course, extremely simpler due to the use of a single reference.

Li, am I right in my understanding that this is your intention?

--
To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 24 lines

Quoted 3 lines

You are putting references but I do not see any single css_{try}get here. /me puzzled.

There are two things being done in this code:
First, we acquired a css_ref to make sure that the underlying cgroup would not go away. That is a short lived reference, and it is put as soon as the cache is created.
At this point, we acquire a long-lived per-cache memcg reference count to guarantee that the memcg will still be alive.

so it is:

enqueue: css_get
create : memcg_get, css_put
destroy: css_put

If I understand Li's patch correctly, he is not touching the first css_get, only turning that into the long lived reference (which was not possible before, since that would prevent rmdir).

Then he only needs to get rid of the memcg_get, change the memcg_put to css_put, and get rid of the now extra css_put.

Quoted 6 lines

He is issuing extra css_puts in memcg_create_kmem_cache, but only in failure paths. So the code reads as:
* css_get on enqueue (already done, so not shown in patch)
* if it fails, css_put
* if it succeeds, don't do anything. This is already the long-lived reference count. put it at release time.

OK, this makes more sense now. It is __memcg_create_cache_enqueue which takes the reference and it is not put after this because it replaced mem_cgroup reference counting.
Li, please put something along these lines into the changelog. This is really tricky and easy to get misunderstand.

You can put my Acked-by then.

Quoted 5 lines

The code looks correct, and of course, extremely simpler due to the use of a single reference.

Li, am I right in my understanding that this is your intention?

Use css_get()/css_put() instead of mem_cgroup_get()/mem_cgroup_put().

Signed-off-by Li Zefan <[email protected]>

---
  mm/memcontrol.c | 10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)

mm/memcontrol.c +1 -1

index 43ca91d..dafacb8 100644
@@ -3191,7 +3191,7 @@ void memcg_release_cache(struct kmem_cache *s)
  	list_del(&s->memcg_params->list);
  	mutex_unlock(&memcg->slab_caches_mutex);
 
-	mem_cgroup_put(memcg);
+	css_put(&memcg->css);
  out:
  	kfree(s->memcg_params);
  }

Quoted 22 lines · Li Zefan

Use css_get()/css_put() instead of mem_cgroup_get()/mem_cgroup_put().

Signed-off-by Li Zefan <[email protected]>

---
  mm/memcontrol.c | 10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)

mm/memcontrol.c +1 -1

index 43ca91d..dafacb8 100644
@@ -3191,7 +3191,7 @@ void memcg_release_cache(struct kmem_cache *s)
  	list_del(&s->memcg_params->list);
  	mutex_unlock(&memcg->slab_caches_mutex);
 
-	mem_cgroup_put(memcg);
+	css_put(&memcg->css);
  out:
  	kfree(s->memcg_params);
  }

@@ -3350,16 +3350,18 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 

mm/memcontrol.c static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, · line 3356

Quoted 7 lines

mm/memcontrol.c +3 -1

  	mutex_lock(&memcg_cache_mutex);
  	new_cachep = cachep->memcg_params->memcg_caches[idx];
-	if (new_cachep)
+	if (new_cachep) {
+		css_put(&memcg->css);
  		goto out;
+	}

Where css_get() against this is done ?

Use css_get/put instead of mem_cgroup_get/put.

We can't do a simple replacement, because here mem_cgroup_put() is called during mem_cgroup_css_free(), while mem_cgroup_css_free() won't be called until css refcnt goes down to 0.

Instead we increment css refcnt in mem_cgroup_css_offline(), and then check if there's still kmem charges. If not, css refcnt will be decremented, otherwise the refcnt will be decremented when kmem charges goes down to 0.

mm/memcontrol.c +1 -9

 	if (memcg_kmem_test_and_clear_dead(memcg))
-		mem_cgroup_put(memcg);
+		css_put(&memcg->css);
 }
 
 void memcg_cache_list_add(struct mem_cgroup *memcg, struct kmem_cache *cachep)

@@ -5089,14 +5089,6 @@ static int memcg_update_kmem_limit(struct cgroup *cont, u64 val)
 		 * starts accounting before all call sites are patched
 		 */
 		memcg_kmem_set_active(memcg);
-
-		/*
-		 * kmem charges can outlive the cgroup. In the case of slab
-		 * pages, for instance, a page contain objects from various
-		 * processes, so it is unfeasible to migrate them away. We
-		 * need to reference count the memcg because of that.
-		 */
-		mem_cgroup_get(memcg);
 
 	} else
 		ret = res_counter_set_limit(&memcg->kmem, val);

out:

mm/memcontrol.c +0 -0

@@ -5129,12 +5121,11 @@ static int memcg_propagate_kmem(struct mem_cgroup *memcg)
 		goto out;
 

mm/memcontrol.c +0 -1

-	mem_cgroup_get(memcg);
 	static_key_slow_inc(&memcg_kmem_enabled_key);
 
 	mutex_lock(&set_limit_mutex);

@@ -5823,23 +5814,33 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 	return mem_cgroup_sockets_init(memcg, ss);
 };
 

mm/memcontrol.c +13 -7

+	/*
+	 * We need to call css_get() first, because memcg_uncharge_kmem()
+	 * will call css_put() if it sees the memcg is dead.
+	 */
 	memcg_kmem_mark_dead(memcg);
 
 	if (res_counter_read_u64(&memcg->kmem, RES_USAGE) != 0)
 		return;
 
 	/*
-	 * Charges already down to 0, undo mem_cgroup_get() done in the charge
-	 * path here, being careful not to race with memcg_uncharge_kmem: it is
-	 * possible that the charges went down to 0 between mark_dead and the
-	 * res_counter read, so in that case, we don't need the put
+	 * Charges already down to 0, undo css_get() done previosly,, being
+	 * careful not to race with memcg_uncharge_kmem: it is possible that
+	 * the charges went down to 0 between mark_dead and the res_counter
+	 * read, so in that case, we don't need the put
 	 */
 
 	if (memcg_kmem_test_and_clear_dead(memcg))
-		mem_cgroup_put(memcg);
+		css_put(&memcg->css);
 }
 #else
 static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss)

@@ -5847,7 +5848,7 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 	return 0;
 }
 
-static void kmem_cgroup_destroy(struct mem_cgroup *memcg)
+static void kmem_cgroup_css_offline(struct mem_cgroup *memcg)
 {
 }
 #endif

@@ -6274,6 +6275,8 @@ static void mem_cgroup_css_offline(struct cgroup *cont)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
 
+	kmem_cgroup_css_offline(memcg);
+
 	mem_cgroup_invalidate_reclaim_iterators(memcg);
 	mem_cgroup_reparent_charges(memcg);
 	mem_cgroup_destroy_all_caches(memcg);

@@ -6283,7 +6286,7 @@ static void mem_cgroup_css_free(struct cgroup *cont)
 {
 	struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
 
-	kmem_cgroup_destroy(memcg);
+	mem_cgroup_sockets_destroy(memcg);
 
 	mem_cgroup_put(memcg);
 }

Signature · 1 line

1.8.0.2

* __mem_cgroup_free will issue static_key_slow_dec because this
* memcg is active already. If the later initialization fails
* then the cgroup core triggers the cleanup so we do not have
* to do it here.
*/

Quoted 8 lines

mm/memcontrol.c +0 -1

-	mem_cgroup_get(memcg);
 	static_key_slow_inc(&memcg_kmem_enabled_key);
 
 	mutex_lock(&set_limit_mutex);

@@ -5823,23 +5814,33 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 	return mem_cgroup_sockets_init(memcg, ss);
 };
 

mm/memcontrol.c static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) · line 5824

Quoted 10 lines

mm/memcontrol.c +7 -2

-static void kmem_cgroup_destroy(struct mem_cgroup *memcg)
+static void kmem_cgroup_css_offline(struct mem_cgroup *memcg)
 {
-	mem_cgroup_sockets_destroy(memcg);
+	/*
+	 * kmem charges can outlive the cgroup. In the case of slab
+	 * pages, for instance, a page contain objects from various
+	 * processes, so it is unfeasible to migrate them away. We
+	 * need to reference count the memcg because of that.
+	 */

I would prefer if we could merge all three comments in this function into a single one. What about something like the following?
/*
* kmem charges can outlive the cgroup. In the case of slab
* pages, for instance, a page contain objects from various
* processes. As we prevent from taking a reference for every
* such allocation we have to be careful when doing uncharge
* (see memcg_uncharge_kmem) and here during offlining.
* The idea is that that only the _last_ uncharge which sees
* the dead memcg will drop the last reference. An additional
* reference is taken here before the group is marked dead
* which is then paired with css_put during uncharge resp. here.
* Although this might sound strange as this path is called when
* the reference has already dropped down to 0 and shouldn't be
* incremented anymore (css_tryget would fail) we do not have
* other options because of the kmem allocations lifetime. */

mm/memcontrol.c static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) · line 5825

Quoted 1 line

mm/memcontrol.c +1 -0

+	css_get(&memcg->css);

I think that you need a write memory barrier here because css_get nor memcg_kmem_mark_dead implies it. memcg_uncharge_kmem uses memcg_kmem_test_and_clear_dead which imply a full memory barrier but it should see the elevated reference count. No?

Quoted 42 lines

* __mem_cgroup_free will issue static_key_slow_dec because this
* memcg is active already. If the later initialization fails
* then the cgroup core triggers the cleanup so we do not have
* to do it here.
*/

Quoted 8 lines

mm/memcontrol.c +0 -1

-	mem_cgroup_get(memcg);
 	static_key_slow_inc(&memcg_kmem_enabled_key);
 
 	mutex_lock(&set_limit_mutex);

@@ -5823,23 +5814,33 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 	return mem_cgroup_sockets_init(memcg, ss);
 };
 

mm/memcontrol.c static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) · line 5824

Quoted 10 lines

mm/memcontrol.c +7 -2

-static void kmem_cgroup_destroy(struct mem_cgroup *memcg)
+static void kmem_cgroup_css_offline(struct mem_cgroup *memcg)
 {
-	mem_cgroup_sockets_destroy(memcg);
+	/*
+	 * kmem charges can outlive the cgroup. In the case of slab
+	 * pages, for instance, a page contain objects from various
+	 * processes, so it is unfeasible to migrate them away. We
+	 * need to reference count the memcg because of that.
+	 */

I would prefer if we could merge all three comments in this function into a single one. What about something like the following?
/*
* kmem charges can outlive the cgroup. In the case of slab
* pages, for instance, a page contain objects from various
* processes. As we prevent from taking a reference for every
* such allocation we have to be careful when doing uncharge
* (see memcg_uncharge_kmem) and here during offlining.
* The idea is that that only the _last_ uncharge which sees
* the dead memcg will drop the last reference. An additional
* reference is taken here before the group is marked dead
* which is then paired with css_put during uncharge resp. here.
* Although this might sound strange as this path is called when
* the reference has already dropped down to 0 and shouldn't be
* incremented anymore (css_tryget would fail) we do not have
* other options because of the kmem allocations lifetime. */

Quoted 7 lines

mm/memcontrol.c static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) · line 5825

Quoted 1 line

mm/memcontrol.c +1 -0

+	css_get(&memcg->css);

I think that you need a write memory barrier here because css_get nor memcg_kmem_mark_dead implies it. memcg_uncharge_kmem uses memcg_kmem_test_and_clear_dead which imply a full memory barrier but it should see the elevated reference count. No?

We don't use barriers for any other kind of reference counting. What is different here?

Use css_get/put instead of mem_cgroup_get/put.

We can't do a simple replacement, because here mem_cgroup_put() is called during mem_cgroup_css_free(), while mem_cgroup_css_free() won't be called until css refcnt goes down to 0.

Instead we increment css refcnt in mem_cgroup_css_offline(), and then check if there's still kmem charges. If not, css refcnt will be decremented, otherwise the refcnt will be decremented when kmem charges goes down to 0.

Use css_get/put instead of mem_cgroup_get/put.

Signed-off-by Li Zefan <[email protected]>

---
 mm/memcontrol.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

mm/memcontrol.c +16 -10

index 877551d..ad576e8 100644
@@ -4137,12 +4137,12 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype,
 	unlock_page_cgroup(pc);
 	/*
 	 * even after unlock, we have memcg->res.usage here and this memcg
-	 * will never be freed.
+	 * will never be freed, so it's safe to call css_get().
 	 */
 	memcg_check_events(memcg, page);
 	if (do_swap_account && ctype == MEM_CGROUP_CHARGE_TYPE_SWAPOUT) {
 		mem_cgroup_swap_statistics(memcg, true);
-		mem_cgroup_get(memcg);
+		css_get(&memcg->css);
 	}
 	/*
 	 * Migration does not charge the res_counter for the

@@ -4242,7 +4242,7 @@ mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout)
 
 	/*
 	 * record memcg information,  if swapout && memcg != NULL,
-	 * mem_cgroup_get() was called in uncharge().
+	 * css_get() was called in uncharge().
 	 */
 	if (do_swap_account && swapout && memcg)
 		swap_cgroup_record(ent, css_id(&memcg->css));

@@ -4273,7 +4273,7 @@ void mem_cgroup_uncharge_swap(swp_entry_t ent)
 		if (!mem_cgroup_is_root(memcg))
 			res_counter_uncharge(&memcg->memsw, PAGE_SIZE);
 		mem_cgroup_swap_statistics(memcg, false);
-		mem_cgroup_put(memcg);
+		css_put(&memcg->css);
 	}
 	rcu_read_unlock();
 }

@@ -4307,11 +4307,14 @@ static int mem_cgroup_move_swap_account(swp_entry_t entry,
 		 * This function is only called from task migration context now.
 		 * It postpones res_counter and refcount handling till the end
 		 * of task migration(mem_cgroup_clear_mc()) for performance
-		 * improvement. But we cannot postpone mem_cgroup_get(to)
-		 * because if the process that has been moved to @to does
-		 * swap-in, the refcount of @to might be decreased to 0.
+		 * improvement. But we cannot postpone css_get(to)  because if
+		 * the process that has been moved to @to does swap-in, the
+		 * refcount of @to might be decreased to 0.
+		 *
+		 * We are in attach() phase, so the cgroup is guaranteed to be
+		 * alive, so we can just call css_get().
 		 */
-		mem_cgroup_get(to);
+		css_get(&to->css);
 		return 0;
 	}
 	return -EINVAL;

@@ -6597,6 +6600,7 @@ static void __mem_cgroup_clear_mc(void)
 {
 	struct mem_cgroup *from = mc.from;
 	struct mem_cgroup *to = mc.to;
+	int i;
 
 	/* we must uncharge all the leftover precharges from mc.to */
 	if (mc.precharge) {

@@ -6617,7 +6621,9 @@ static void __mem_cgroup_clear_mc(void)
 		if (!mem_cgroup_is_root(mc.from))
 			res_counter_uncharge(&mc.from->memsw,
 						PAGE_SIZE * mc.moved_swap);
-		__mem_cgroup_put(mc.from, mc.moved_swap);
+
+		for (i = 0; i < mc.moved_swap; i++)
+			css_put(&mc.from->css);
 
 		if (!mem_cgroup_is_root(mc.to)) {
 			/*

@@ -6627,7 +6633,7 @@ static void __mem_cgroup_clear_mc(void)
 			res_counter_uncharge(&mc.to->res,
 						PAGE_SIZE * mc.moved_swap);
 		}
-		/* we've already done mem_cgroup_get(mc.to) */
+		/* we've already done css_get(mc.to) */
 		mc.moved_swap = 0;
 	}
 	memcg_oom_recover(from);

Signature · 1 line

1.8.0.2

Use css_get/put instead of mem_cgroup_get/put.

Signed-off-by Li Zefan <[email protected]>

---
  mm/memcontrol.c | 26 ++++++++++++++++----------
  1 file changed, 16 insertions(+), 10 deletions(-)

mm/memcontrol.c +15 -9

index 877551d..ad576e8 100644
@@ -4137,12 +4137,12 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype,
  	unlock_page_cgroup(pc);
  	/*
  	 * even after unlock, we have memcg->res.usage here and this memcg
-	 * will never be freed.
+	 * will never be freed, so it's safe to call css_get().
  	 */
  	memcg_check_events(memcg, page);
  	if (do_swap_account && ctype == MEM_CGROUP_CHARGE_TYPE_SWAPOUT) {
  		mem_cgroup_swap_statistics(memcg, true);
-		mem_cgroup_get(memcg);
+		css_get(&memcg->css);
  	}
  	/*
  	 * Migration does not charge the res_counter for the

@@ -4242,7 +4242,7 @@ mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent, bool swapout)
 
  	/*
  	 * record memcg information,  if swapout && memcg != NULL,
-	 * mem_cgroup_get() was called in uncharge().
+	 * css_get() was called in uncharge().
  	 */
  	if (do_swap_account && swapout && memcg)
  		swap_cgroup_record(ent, css_id(&memcg->css));

@@ -4273,7 +4273,7 @@ void mem_cgroup_uncharge_swap(swp_entry_t ent)
  		if (!mem_cgroup_is_root(memcg))
  			res_counter_uncharge(&memcg->memsw, PAGE_SIZE);
  		mem_cgroup_swap_statistics(memcg, false);
-		mem_cgroup_put(memcg);
+		css_put(&memcg->css);
  	}
  	rcu_read_unlock();
  }

@@ -4307,11 +4307,14 @@ static int mem_cgroup_move_swap_account(swp_entry_t entry,
  		 * This function is only called from task migration context now.
  		 * It postpones res_counter and refcount handling till the end
  		 * of task migration(mem_cgroup_clear_mc()) for performance
-		 * improvement. But we cannot postpone mem_cgroup_get(to)
-		 * because if the process that has been moved to @to does
-		 * swap-in, the refcount of @to might be decreased to 0.
+		 * improvement. But we cannot postpone css_get(to)  because if
+		 * the process that has been moved to @to does swap-in, the
+		 * refcount of @to might be decreased to 0.
+		 *
+		 * We are in attach() phase, so the cgroup is guaranteed to be
+		 * alive, so we can just call css_get().
  		 */
-		mem_cgroup_get(to);
+		css_get(&to->css);
  		return 0;
  	}
  	return -EINVAL;

@@ -6597,6 +6600,7 @@ static void __mem_cgroup_clear_mc(void)
  {
  	struct mem_cgroup *from = mc.from;
  	struct mem_cgroup *to = mc.to;
+	int i;
 
  	/* we must uncharge all the leftover precharges from mc.to */
  	if (mc.precharge) {

@@ -6617,7 +6621,9 @@ static void __mem_cgroup_clear_mc(void)
  		if (!mem_cgroup_is_root(mc.from))
  			res_counter_uncharge(&mc.from->memsw,
  						PAGE_SIZE * mc.moved_swap);
-		__mem_cgroup_put(mc.from, mc.moved_swap);
+
+		for (i = 0; i < mc.moved_swap; i++)
+			css_put(&mc.from->css);
 
  		if (!mem_cgroup_is_root(mc.to)) {
  			/*

Suppose we rmdir a cgroup and there're still css refs, this cgroup won't be freed. Then we rmdir the parent cgroup, and the parent is freed due to css ref draining to 0. Now it would be a disaster if the child cgroup tries to access its parent.

Make sure this won't happen.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

kernel/cgroup.c +10 -0

index fa54b92..78204bc 100644
@@ -888,6 +888,13 @@ static void cgroup_free_fn(struct work_struct *work)
 	mutex_unlock(&cgroup_mutex);
 
 	/*
+	 * We get a ref to the parent's dentry, and put the ref when
+	 * this cgroup is being freed, so it's guaranteed that the
+	 * parent won't be destroyed before its children.
+	 */
+	dput(cgrp->parent->dentry);
+
+	/*
 	 * Drop the active superblock reference that we took when we
 	 * created the cgroup
 	 */

@@ -4171,6 +4178,9 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
 	for_each_subsys(root, ss)
 		dget(dentry);
 
+	/* hold a ref to the parent's dentry */
+	dget(parent->dentry);
+
 	/* creation succeeded, notify subsystems */
 	for_each_subsys(root, ss) {
 		err = online_css(ss, cgrp);

Signature · 1 line

1.8.0.2

Quoted 4 lines · Li Zefan

Suppose we rmdir a cgroup and there're still css refs, this cgroup won't be freed. Then we rmdir the parent cgroup, and the parent is freed due to css ref draining to 0. Now it would be a disaster if the child cgroup tries to access its parent.

Hmm, I am not sure what is the correct layer for this to handle - cgroup core or memcg. But we have enforced that in mem_cgroup_css_online where we take an additional reference to the memcg.

Handling it in the memcg code would have an advantage of limiting an additional reference only to use_hierarchy cases which is sufficient as we never touch the parent otherwise (parent_mem_cgroup).

Hey,

Quoted 10 lines · Michal Hocko

Quoted 4 lines · Li Zefan

Suppose we rmdir a cgroup and there're still css refs, this cgroup won't be freed. Then we rmdir the parent cgroup, and the parent is freed due to css ref draining to 0. Now it would be a disaster if the child cgroup tries to access its parent.

Hmm, I am not sure what is the correct layer for this to handle - cgroup core or memcg. But we have enforced that in mem_cgroup_css_online where we take an additional reference to the memcg.

Quoted 3 lines

Handling it in the memcg code would have an advantage of limiting an additional reference only to use_hierarchy cases which is sufficient as we never touch the parent otherwise (parent_mem_cgroup).

But what harm does an additional reference do?

And given that there are cgroup core interfaces which access ->parent, I think it'd be a good idea that parent always exists while there are children.

Thanks.

Quoted 1 line

But what harm does an additional reference do?

No harm at all. I just wanted to be sure that this is not yet another "for memcg" hack. So if this is useful for other controllers then I have no objections of course.

Quoted 5 lines · Michal Hocko

Quoted 1 line

But what harm does an additional reference do?

No harm at all. I just wanted to be sure that this is not yet another "for memcg" hack. So if this is useful for other controllers then I have no objections of course.

I think it makes sense in general, so let's do it in cgroup core.

I suppose it'd be easier for this to be routed together with other memcg changes?

Thanks.

Signature · 5 lines

tejun
--
To unsubscribe from this list: send the line "unsubscribe cgroups" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Michal Hocko

Quoted 1 line

But what harm does an additional reference do?

No harm at all. I just wanted to be sure that this is not yet another "for memcg" hack. So if this is useful for other controllers then I have no objections of course.

I think it makes sense in general, so let's do it in cgroup core. I suppose it'd be easier for this to be routed together with other memcg changes?

Thanks.

Suppose we rmdir a cgroup and there're still css refs, this cgroup won't be freed. Then we rmdir the parent cgroup, and the parent is freed due to css ref draining to 0. Now it would be a disaster if the child cgroup tries to access its parent.

Make sure this won't happen.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

kernel/cgroup.c +10 -0

index fa54b92..78204bc 100644
@@ -888,6 +888,13 @@ static void cgroup_free_fn(struct work_struct *work)
 	mutex_unlock(&cgroup_mutex);
 
 	/*
+	 * We get a ref to the parent's dentry, and put the ref when
+	 * this cgroup is being freed, so it's guaranteed that the
+	 * parent won't be destroyed before its children.
+	 */
+	dput(cgrp->parent->dentry);
+
+	/*
 	 * Drop the active superblock reference that we took when we
 	 * created the cgroup
 	 */

@@ -4171,6 +4178,9 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
 	for_each_subsys(root, ss)
 		dget(dentry);
 
+	/* hold a ref to the parent's dentry */
+	dget(parent->dentry);
+
 	/* creation succeeded, notify subsystems */
 	for_each_subsys(root, ss) {
 		err = online_css(ss, cgrp);

Signature · 1 line

1.8.0.2

Suppose we rmdir a cgroup and there're still css refs, this cgroup won't be freed. Then we rmdir the parent cgroup, and the parent is freed due to css ref draining to 0. Now it would be a disaster if the child cgroup tries to access its parent.

Make sure this won't happen.

Signed-off-by Li Zefan <[email protected]>

---
  kernel/cgroup.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

kernel/cgroup.c +7 -0

index fa54b92..78204bc 100644
@@ -888,6 +888,13 @@ static void cgroup_free_fn(struct work_struct *work)
  	mutex_unlock(&cgroup_mutex);
 
  	/*
+	 * We get a ref to the parent's dentry, and put the ref when
+	 * this cgroup is being freed, so it's guaranteed that the
+	 * parent won't be destroyed before its children.
+	 */
+	dput(cgrp->parent->dentry);
+
+	/*
  	 * Drop the active superblock reference that we took when we
  	 * created the cgroup
  	 */

The cgroup core guarantees it's always safe to access the parent.

Signed-off-by Li Zefan <[email protected]>

---
 mm/memcontrol.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

mm/memcontrol.c +1 -5

index ad576e8..45129cd 100644
@@ -6124,12 +6124,8 @@ static void mem_cgroup_get(struct mem_cgroup *memcg)
 
 static void __mem_cgroup_put(struct mem_cgroup *memcg, int count)
 {
-	if (atomic_sub_and_test(count, &memcg->refcnt)) {
-		struct mem_cgroup *parent = parent_mem_cgroup(memcg);
+	if (atomic_sub_and_test(count, &memcg->refcnt))
 		call_rcu(&memcg->rcu_freeing, free_rcu);
-		if (parent)
-			mem_cgroup_put(parent);
-	}
 }
 
 static void mem_cgroup_put(struct mem_cgroup *memcg)

@@ -6229,14 +6225,6 @@ mem_cgroup_css_online(struct cgroup *cont)
 		res_counter_init(&memcg->res, &parent->res);

mm/memcontrol.c +0 -0

 	} else {
 		res_counter_init(&memcg->res, NULL);
 		res_counter_init(&memcg->memsw, NULL);

Signature · 7 lines

1.8.0.2

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

The cgroup core guarantees it's always safe to access the parent.

Signed-off-by Li Zefan <[email protected]>

Now memcg has the same life cycle as the corresponding cgroup. Kill the useless refcnt.

Signed-off-by Li Zefan <[email protected]>

---
 mm/memcontrol.c | 24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

mm/memcontrol.c +1 -23

index 45129cd..9714a16 100644
@@ -297,8 +297,6 @@ struct mem_cgroup {
 	bool		oom_lock;
 	atomic_t	under_oom;
 
-	atomic_t	refcnt;
-
 	int	swappiness;
 	/* OOM-Killer disable */
 	int		oom_kill_disable;

@@ -501,9 +499,6 @@ enum res_type {
  */
 static DEFINE_MUTEX(memcg_create_mutex);
 
-static void mem_cgroup_get(struct mem_cgroup *memcg);
-static void mem_cgroup_put(struct mem_cgroup *memcg);
-
 static inline
 struct mem_cgroup *mem_cgroup_from_css(struct cgroup_subsys_state *s)
 {

@@ -6117,22 +6112,6 @@ static void free_rcu(struct rcu_head *rcu_head)
 	schedule_work(&memcg->work_freeing);
 }
 
-static void mem_cgroup_get(struct mem_cgroup *memcg)
-{
-	atomic_inc(&memcg->refcnt);
-}
-
-static void __mem_cgroup_put(struct mem_cgroup *memcg, int count)
-{
-	if (atomic_sub_and_test(count, &memcg->refcnt))
-		call_rcu(&memcg->rcu_freeing, free_rcu);
-}
-
-static void mem_cgroup_put(struct mem_cgroup *memcg)
-{
-	__mem_cgroup_put(memcg, 1);
-}
-
 /*
  * Returns the parent mem_cgroup in memcgroup hierarchy with hierarchy enabled.
  */

@@ -6192,7 +6171,6 @@ mem_cgroup_css_alloc(struct cgroup *cont)
 
 	memcg->last_scanned_node = MAX_NUMNODES;
 	INIT_LIST_HEAD(&memcg->oom_notify);
-	atomic_set(&memcg->refcnt, 1);
 	memcg->move_charge_at_immigrate = 0;
 	mutex_init(&memcg->thresholds_lock);
 	spin_lock_init(&memcg->move_lock);

@@ -6279,7 +6257,7 @@ static void mem_cgroup_css_free(struct cgroup *cont)
 
 	mem_cgroup_sockets_destroy(memcg);
 
-	mem_cgroup_put(memcg);
+	call_rcu(&memcg->rcu_freeing, free_rcu);
 }
 
 #ifdef CONFIG_MMU

Signature · 1 line

1.8.0.2

Now memcg has the same life cycle as the corresponding cgroup. Kill the useless refcnt.

Signed-off-by Li Zefan <[email protected]>

The historical reason that memcg didn't use css_get in some cases, is that cgroup couldn't be removed if there're still css refs. The situation has changed so that rmdir a cgroup will succeed regardless css refs, but won't be freed until css refs goes down to 0.

This is an early post, and it's NOT TESTED. I just want to see if the changes are fine in general.

(I'll be off from my office soon, and I won't be responsive in the following 3 days.)

I'm working on converting memcg to use cgroup->id, and then we can kill css_id.

Now memcg has its own refcnt, so when a cgroup is destroyed, the memcg can still be alive. This patchset converts memcg to always use css_get/put, so memcg will have the same life cycle as its corresponding cgroup, and then it's always safe for memcg to use cgroup->id.

The historical reason that memcg didn't use css_get in some cases, is that cgroup couldn't be removed if there're still css refs. The situation has changed so that rmdir a cgroup will succeed regardless css refs, but won't be freed until css refs goes down to 0.

(I'll be off from my office soon, and I won't be responsive in the following 3 days.)

I'm working on converting memcg to use cgroup->id, and then we can kill css_id.

Now memcg has its own refcnt, so when a cgroup is destroyed, the memcg can still be alive. This patchset converts memcg to always use css_get/put, so memcg will have the same life cycle as its corresponding cgroup, and then it's always safe for memcg to use cgroup->id.

The historical reason that memcg didn't use css_get in some cases, is that cgroup couldn't be removed if there're still css refs. The situation has changed so that rmdir a cgroup will succeed regardless css refs, but won't be freed until css refs goes down to 0.

This is an early post, and it's NOT TESTED. I just want to see if the changes are fine in general.

btw, after this patchset I think we don't need to free memcg via RCU, because cgroup is already freed in RCU callback.

Note this patchset is based on a few memcg fixes I sent (but hasn't been accepted)

--

 kernel/cgroup.c |  10 ++++++++
 mm/memcontrol.c | 129 ++++++++++++++++++++++++++++++++++++++-------------------------------------------------------
 2 files changed, 62 insertions(+), 77 deletions(-)

Quoted 10 lines · Li Zefan

(I'll be off from my office soon, and I won't be responsive in the following 3 days.)

I'm working on converting memcg to use cgroup->id, and then we can kill css_id.

Now memcg has its own refcnt, so when a cgroup is destroyed, the memcg can still be alive. This patchset converts memcg to always use css_get/put, so memcg will have the same life cycle as its corresponding cgroup, and then it's always safe for memcg to use cgroup->id.

Quoted 7 lines

The historical reason that memcg didn't use css_get in some cases, is that cgroup couldn't be removed if there're still css refs. The situation has changed so that rmdir a cgroup will succeed regardless css refs, but won't be freed until css refs goes down to 0.

This is an early post, and it's NOT TESTED. I just want to see if the changes are fine in general.

yes, I like the approach and it looks correct as well (some minor things mentioned in the patches). Thanks a lot Li! This will make our lifes much easier. The separate ref counting was PITA especially after introduction of kmem accounting which made its usage even more trickier.

Quoted 2 lines

btw, after this patchset I think we don't need to free memcg via RCU, because cgroup is already freed in RCU callback.

But this depends on changes waiting in for-3.10 branch, right?

Anyway, I think we should be safe with the workqueue based releasing as well once mem_cgroup_{get,put} are gone, right?

Quoted 24 lines · Michal Hocko

Quoted 10 lines · Li Zefan

(I'll be off from my office soon, and I won't be responsive in the following 3 days.)

I'm working on converting memcg to use cgroup->id, and then we can kill css_id.

Now memcg has its own refcnt, so when a cgroup is destroyed, the memcg can still be alive. This patchset converts memcg to always use css_get/put, so memcg will have the same life cycle as its corresponding cgroup, and then it's always safe for memcg to use cgroup->id.

Quoted 7 lines

The historical reason that memcg didn't use css_get in some cases, is that cgroup couldn't be removed if there're still css refs. The situation has changed so that rmdir a cgroup will succeed regardless css refs, but won't be freed until css refs goes down to 0.

This is an early post, and it's NOT TESTED. I just want to see if the changes are fine in general.

yes, I like the approach and it looks correct as well (some minor things mentioned in the patches). Thanks a lot Li! This will make our lifes much easier. The separate ref counting was PITA especially after introduction of kmem accounting which made its usage even more trickier.

Quoted 4 lines

Quoted 2 lines

btw, after this patchset I think we don't need to free memcg via RCU, because cgroup is already freed in RCU callback.

But this depends on changes waiting in for-3.10 branch, right?

What changes? memcg changes or cgroup core changes? I don't think this depends on anything in cgroup 3.10 branch.

(I'll be off from my office soon, and I won't be responsive in the following 3 days.)

I'm working on converting memcg to use cgroup->id, and then we can kill css_id.

Now memcg has its own refcnt, so when a cgroup is destroyed, the memcg can still be alive. This patchset converts memcg to always use css_get/put, so memcg will have the same life cycle as its corresponding cgroup, and then it's always safe for memcg to use cgroup->id.

The historical reason that memcg didn't use css_get in some cases, is that cgroup couldn't be removed if there're still css refs. The situation has changed so that rmdir a cgroup will succeed regardless css refs, but won't be freed until css refs goes down to 0.

This is an early post, and it's NOT TESTED. I just want to see if the changes are fine in general.

btw, after this patchset I think we don't need to free memcg via RCU, because cgroup is already freed in RCU callback.

Note this patchset is based on a few memcg fixes I sent (but hasn't been accepted)

--

 kernel/cgroup.c |  10 ++++++++
 mm/memcontrol.c | 129 ++++++++++++++++++++++++++++++++++++++-------------------------------------------------------
 2 files changed, 62 insertions(+), 77 deletions(-)

Hi,

I'm rebasing this patchset against latest linux-next, and it conflicts with "[PATCH v2] memcg: debugging facility to access dangling memcgs." slightly.

That is a debugging patch and will never be pushed into mainline, so should I still base this patchset on that debugging patch?

Also that patch needs update (and can be simplified) after this patchset:
- move memcg_dangling_add() to mem_cgroup_css_offline()
- remove memcg->memcg_name, and use cgroup_path() in mem_cgroup_dangling_read()?

Hi,

I'm rebasing this patchset against latest linux-next, and it conflicts with "[PATCH v2] memcg: debugging facility to access dangling memcgs." slightly.

That is a debugging patch and will never be pushed into mainline, so should I still base this patchset on that debugging patch?

Also that patch needs update (and can be simplified) after this patchset:
- move memcg_dangling_add() to mem_cgroup_css_offline()
- remove memcg->memcg_name, and use cgroup_path() in mem_cgroup_dangling_read()?