Skip to contentSkip to conversation

memcg: use css_get/put when charging/uncharging kmem

PATCH In review Li Zefan 4 participants 6 messages Jun 13, 2013 – Jun 14, 2013

Subsystems (3)
Memory management cgroup memcg
lore
Share & tools
lore

Li Zefan

author
#0 lore raw

PATCH v3 5/9 memcg: use css_get/put when charging/uncharging kmem

Sorry for updating the patchset so late.

I've made some changes for the memory barrier thing, and I agree with Michal that there can be improvement but can be a separate patch.

If this version is ok for everyone, I'll send the whole patchset out to Andrew.

=========================

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 immediately, otherwise the refcnt won't be decremented when kmem charges goes down to 0.

v3:
- changed wmb() to smp_smb(), and moved it to memcg_kmem_mark_dead(), and added comment.

v2:
- added wmb() in kmem_cgroup_css_offline(), pointed out by Michal
- revised comments as suggested by Michal
- fixed to check if kmem is activated in kmem_cgroup_css_offline()

Signed-off-by Li Zefan <[email protected]>
Acked-by Michal Hocko <[email protected]>
Acked-by KAMEZAWA Hiroyuki <[email protected]>
---
 mm/memcontrol.c | 70 ++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 25 deletions(-)
mm/memcontrol.c +45 -25
index 466c595..76dcd0e 100644
@@ -416,6 +416,11 @@ static void memcg_kmem_clear_activated(struct mem_cgroup *memcg)
 
 static void memcg_kmem_mark_dead(struct mem_cgroup *memcg)
 {
+	/*
+	 * We need to call css_get() first, because memcg_uncharge_kmem()
+	 * will call css_put() if it sees the memcg is dead.
+	 */
+	smb_wmb();
 	if (test_bit(KMEM_ACCOUNTED_ACTIVE, &memcg->kmem_account_flags))
 		set_bit(KMEM_ACCOUNTED_DEAD, &memcg->kmem_account_flags);
 }

@@ -3060,8 +3065,16 @@ static void memcg_uncharge_kmem(struct mem_cgroup *memcg, u64 size)
 	if (res_counter_uncharge(&memcg->kmem, size))
 		return;
 
+	/*
+	 * Releases a reference taken in kmem_cgroup_css_offline in case
+	 * this last uncharge is racing with the offlining code or it is
+	 * outliving the memcg existence.
+	 *
+	 * The memory barrier imposed by test&clear is paired with the
+	 * explicit one in memcg_kmem_mark_dead().
+	 */
 	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)

@@ -5165,14 +5178,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:

@@ -5205,12 +5210,10 @@ static int memcg_propagate_kmem(struct mem_cgroup *memcg)
 		goto out;
 
 	/*
-	 * destroy(), called if we fail, will issue static_key_slow_inc() and
-	 * mem_cgroup_put() if kmem is enabled. We have to either call them
-	 * unconditionally, or clear the KMEM_ACTIVE flag. I personally find
-	 * this more consistent, since it always leads to the same destroy path
+	 * __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.
 	 */
-	mem_cgroup_get(memcg);
 	static_key_slow_inc(&memcg_kmem_enabled_key);
 
 	mutex_lock(&set_limit_mutex);

@@ -5893,23 +5896,38 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 	return mem_cgroup_sockets_init(memcg, ss);
 }
 
-static void kmem_cgroup_destroy(struct mem_cgroup *memcg)
+static void kmem_cgroup_css_offline(struct mem_cgroup *memcg)
 {
-	mem_cgroup_sockets_destroy(memcg);
+	if (!memcg_kmem_is_active(memcg))
+		return;
+
+	/*
+	 * 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 from
+	 * css_offline() when the referencemight have 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.
+	 */
+	css_get(&memcg->css);
 
 	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
-	 */
 	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)

@@ -5917,7 +5935,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

@@ -6350,6 +6368,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);

@@ -6359,7 +6379,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
#1 lore raw
Quoted 16 lines · Li Zefan

Sorry for updating the patchset so late.

I've made some changes for the memory barrier thing, and I agree with Michal that there can be improvement but can be a separate patch.

If this version is ok for everyone, I'll send the whole patchset out to Andrew.

=========================

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.

Quoted 6 lines

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

otherwise the refcnt won't be decremented when kmem charges goes down to 0.

This is a bit confusing. What about "otherwise the css refcount will be released after the last kmem allocation is uncharged."

Quoted 22 lines

v3:
- changed wmb() to smp_smb(), and moved it to memcg_kmem_mark_dead(), and added comment.

v2:
- added wmb() in kmem_cgroup_css_offline(), pointed out by Michal
- revised comments as suggested by Michal
- fixed to check if kmem is activated in kmem_cgroup_css_offline()

Signed-off-by Li Zefan <[email protected]>
Acked-by Michal Hocko <[email protected]>
Acked-by KAMEZAWA Hiroyuki <[email protected]>
---
 mm/memcontrol.c | 70 ++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 25 deletions(-)
mm/memcontrol.c +0 -0
index 466c595..76dcd0e 100644

mm/memcontrol.c static void memcg_kmem_clear_activated(struct mem_cgroup *memcg) · line 425

Quoted 10 lines
mm/memcontrol.c +5 -0
@@ -416,6 +416,11 @@ static void memcg_kmem_clear_activated(struct mem_cgroup *memcg)
 
 static void memcg_kmem_mark_dead(struct mem_cgroup *memcg)
 {
+	/*
+	 * We need to call css_get() first, because memcg_uncharge_kmem()
+	 * will call css_put() if it sees the memcg is dead.
+	 */
+	smb_wmb();
 	if (test_bit(KMEM_ACCOUNTED_ACTIVE, &memcg->kmem_account_flags))
 		set_bit(KMEM_ACCOUNTED_DEAD, &memcg->kmem_account_flags);

I do not feel strongly about that but maybe open coding this in mem_cgroup_css_offline would be even better. There is only single caller and there is smaller chance somebody will use the function incorrectly later on.

So I leave the decision on you because this doesn't matter much.

[...]

Signature · 2 lines
Michal Hocko
SUSE Labs
#2 lore raw

[Fix Glauber's new address]

Quoted 75 lines · Michal Hocko
Quoted 16 lines · Li Zefan

Sorry for updating the patchset so late.

I've made some changes for the memory barrier thing, and I agree with Michal that there can be improvement but can be a separate patch.

If this version is ok for everyone, I'll send the whole patchset out to Andrew.

=========================

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.

Quoted 6 lines

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

otherwise the refcnt won't be decremented when kmem charges goes down to 0.

This is a bit confusing. What about "otherwise the css refcount will be released after the last kmem allocation is uncharged."

Quoted 22 lines

v3:
- changed wmb() to smp_smb(), and moved it to memcg_kmem_mark_dead(), and added comment.

v2:
- added wmb() in kmem_cgroup_css_offline(), pointed out by Michal
- revised comments as suggested by Michal
- fixed to check if kmem is activated in kmem_cgroup_css_offline()

Signed-off-by Li Zefan <[email protected]>
Acked-by Michal Hocko <[email protected]>
Acked-by KAMEZAWA Hiroyuki <[email protected]>
---
 mm/memcontrol.c | 70 ++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 25 deletions(-)
mm/memcontrol.c +0 -0
index 466c595..76dcd0e 100644

mm/memcontrol.c static void memcg_kmem_clear_activated(struct mem_cgroup *memcg) · line 425

Quoted 10 lines
mm/memcontrol.c +5 -0
@@ -416,6 +416,11 @@ static void memcg_kmem_clear_activated(struct mem_cgroup *memcg)
 
 static void memcg_kmem_mark_dead(struct mem_cgroup *memcg)
 {
+	/*
+	 * We need to call css_get() first, because memcg_uncharge_kmem()
+	 * will call css_put() if it sees the memcg is dead.
+	 */
+	smb_wmb();
 	if (test_bit(KMEM_ACCOUNTED_ACTIVE, &memcg->kmem_account_flags))
 		set_bit(KMEM_ACCOUNTED_DEAD, &memcg->kmem_account_flags);

I do not feel strongly about that but maybe open coding this in mem_cgroup_css_offline would be even better. There is only single caller and there is smaller chance somebody will use the function incorrectly later on.

So I leave the decision on you because this doesn't matter much.

[...]

Signature · 6 lines
Michal Hocko
SUSE Labs
--
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
Signature · 8 lines
Michal Hocko
SUSE Labs

--
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>
author
#3 lore raw
In reply to Michal Hocko
Quoted 15 lines

mm/memcontrol.c static void memcg_kmem_clear_activated(struct mem_cgroup *memcg) · line 425

Quoted 9 lines

static void memcg_kmem_mark_dead(struct mem_cgroup *memcg) {

mm/memcontrol.c +5 -0
+	/*
+	 * We need to call css_get() first, because memcg_uncharge_kmem()
+	 * will call css_put() if it sees the memcg is dead.
+	 */
+	smb_wmb();
 	if (test_bit(KMEM_ACCOUNTED_ACTIVE, &memcg->kmem_account_flags))
 		set_bit(KMEM_ACCOUNTED_DEAD, &memcg->kmem_account_flags);

I do not feel strongly about that but maybe open coding this in mem_cgroup_css_offline would be even better. There is only single caller and there is smaller chance somebody will use the function incorrectly later on.

Quoted 2 lines

So I leave the decision on you because this doesn't matter much.

Yeah, it should go away soon. I'll post a patch after this patchset gets merged into -mm tree and then we can discuss there.

#4 lore raw
Quoted 15 lines · Li Zefan
Quoted 15 lines

mm/memcontrol.c static void memcg_kmem_clear_activated(struct mem_cgroup *memcg) · line 425

Quoted 9 lines

static void memcg_kmem_mark_dead(struct mem_cgroup *memcg) {

mm/memcontrol.c +5 -0
+	/*
+	 * We need to call css_get() first, because memcg_uncharge_kmem()
+	 * will call css_put() if it sees the memcg is dead.
+	 */
+	smb_wmb();
 	if (test_bit(KMEM_ACCOUNTED_ACTIVE, &memcg->kmem_account_flags))
 		set_bit(KMEM_ACCOUNTED_DEAD, &memcg->kmem_account_flags);

I do not feel strongly about that but maybe open coding this in mem_cgroup_css_offline would be even better. There is only single caller and there is smaller chance somebody will use the function incorrectly later on.

Quoted 5 lines
Quoted 2 lines

So I leave the decision on you because this doesn't matter much.

Yeah, it should go away soon. I'll post a patch after this patchset gets merged into -mm tree and then we can discuss there.

I don't care if it is open coded or not. If there is any strong preference from anyone on this matter, feel free to change it.

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

maintainer Reviewed-by on v3 5/9
#5 lore raw
In reply to Li Zefan
Quoted 7 lines · Li Zefan

Sorry for updating the patchset so late.

I've made some changes for the memory barrier thing, and I agree with Michal that there can be improvement but can be a separate patch.

If this version is ok for everyone, I'll send the whole patchset out to Andrew.

Can you please post an updated patch as reply to the original patch? It's a bit difficult to follow things.

Quoted 17 lines

=========================

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 immediately, otherwise the refcnt won't be decremented when kmem charges goes down to 0.

v3:
- changed wmb() to smp_smb(), and moved it to memcg_kmem_mark_dead(), and added comment.

Quoted 8 lines

v2:
- added wmb() in kmem_cgroup_css_offline(), pointed out by Michal
- revised comments as suggested by Michal
- fixed to check if kmem is activated in kmem_cgroup_css_offline()

Signed-off-by Li Zefan <[email protected]>
Acked-by Michal Hocko <[email protected]>
Acked-by KAMEZAWA Hiroyuki <[email protected]>
Reviewed-by Tejun Heo <[email protected]>

But let's please remove the barrier dancing.

Thanks.

Signature · 7 lines
tejun

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