cgroup: use negative bias on css->refcnt to block css_tryget()

PATCH In review Tejun Heo 9 participants 20 messages Mar 30, 2012 – May 28, 2012 6 messages not in the archive

Subsystems (3)

Memory management cgroup memcg

lore

Share & tools

lore

Tejun Heo

author maintainer

2012-03-30 22:34 UTC #0 lore raw

PATCH 1/2 cgroup: use negative bias on css->refcnt to block css_tryget()

When a cgroup is about to be removed, cgroup_clear_css_refs() is called to check and ensure that there are no active css references.

This is currently achieved by dropping the refcnt to zero iff it has only the base ref. If all css refs could be dropped to zero, ref clearing is successful and CSS_REMOVED is set on all css. If not, the base ref is restored. While css ref is zero w/o CSS_REMOVED set, any css_tryget() attempt on it busy loops so that they are atomic w.r.t. the whole css ref clearing.

This does work but dropping and re-instating the base ref is somewhat hairy and makes it difficult to add more logic to the put path as there are two of them - the regular css_put() and the reversible base ref clearing.

This patch updates css ref clearing such that blocking new css_tryget() and putting the base ref are separate operations. CSS_DEACT_BIAS, defined as INT_MIN, is added to css->refcnt and css_tryget() busy loops while refcnt is negative. After all css refs are deactivated, if they were all one, ref clearing succeeded and CSS_REMOVED is set and the base ref is put using the regular css_put(); otherwise, CSS_DEACT_BIAS is subtracted from the refcnts and the original postive values are restored.

css_refcnt() accessor which always returns the unbiased positive reference counts is added and used to simplify refcnt usages. While at it, relocate and reformat comments in cgroup_has_css_refs().

This separates css->refcnt deactivation and putting the base ref, which enables the next patch to make ref clearing optional.

Signed-off-by Tejun Heo <[email protected]>

---
These two patches should be applied on top of the cftype patchset[1] and is available in the following git branch.

git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git cgroup-css-refs

Thanks.

[1] http://thread.gmane.org/gmane.linux.kernel.cgroups/1322

 include/linux/cgroup.h |   12 ++---
 kernel/cgroup.c        |  119 ++++++++++++++++++++++++++++-------------------
 2 files changed, 75 insertions(+), 56 deletions(-)

include/linux/cgroup.h +4 -8

index 028478c..be81faf 100644
@@ -115,16 +115,12 @@ static inline bool css_is_removed(struct cgroup_subsys_state *css)
  * the css has been destroyed.
  */
 
+extern bool __css_tryget(struct cgroup_subsys_state *css);
 static inline bool css_tryget(struct cgroup_subsys_state *css)
 {
 	if (test_bit(CSS_ROOT, &css->flags))
 		return true;
-	while (!atomic_inc_not_zero(&css->refcnt)) {
-		if (test_bit(CSS_REMOVED, &css->flags))
-			return false;
-		cpu_relax();
-	}
-	return true;
+	return __css_tryget(css);
 }
 
 /*

@@ -132,11 +128,11 @@ static inline bool css_tryget(struct cgroup_subsys_state *css)
  * css_get() or css_tryget()
  */
 
-extern void __css_put(struct cgroup_subsys_state *css, int count);
+extern void __css_put(struct cgroup_subsys_state *css);
 static inline void css_put(struct cgroup_subsys_state *css)
 {
 	if (!test_bit(CSS_ROOT, &css->flags))
-		__css_put(css, 1);
+		__css_put(css);
 }
 
 /* bits in struct cgroup flags field */

kernel/cgroup.c +71 ~~-48~~

196 lines not on this page · raw message

Signature · 1 line

1.7.7.3

Tejun Heo

author maintainer

2012-03-30 22:36 UTC #1 lore raw

PATCH 2/2 cgroup: make css->refcnt clearing on cgroup removal optional

Currently, cgroup removal tries to drain all css references. If there are active css references, the removal logic waits and retries ->pre_detroy() until either all refs drop to zero or removal is cancelled.

This semantics is unusual and adds non-trivial complexity to cgroup core and IMHO is fundamentally misguided in that it couples internal implementation details (references to internal data structure) with externally visible operation (rmdir). To userland, this is a behavior peculiarity which is unnecessary and difficult to expect (css refs is otherwise invisible from userland), and, to policy implementations, this is an unnecessary restriction (e.g. blkcg wants to hold css refs for caching purposes but can't as that becomes visible as rmdir hang).

Unfortunately, memcg currently depends on ->pre_destroy() retrials and cgroup removal vetoing and can't be immmediately switched to the new behavior. This patch introduces the new behavior of not waiting for css refs to drain and maintains the old behavior for subsystems which have __DEPRECATED_clear_css_refs set.

Once, memcg is updated, we can drop the code paths for the old behavior as proposed in the following patch. Note that the following patch is incorrect in that dput work item is in cgroup and may lose some of dputs when multiples css's are released back-to-back, and __css_put() triggers check_for_release() when refcnt reaches 0 instead of 1; however, it shows what part can be removed.

http://thread.gmane.org/gmane.linux.kernel.containers/22559/focus=75251

Note that, in not-too-distant future, cgroup core will start emitting warning messages for subsys which require the old behavior, so please get moving.

Signed-off-by Tejun Heo <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Balbir Singh <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

---
So, memcg can keep the current behavior for now w/ __DEPRECATED_clear_css_refs set but I'll keep increasing the pressure. ;P

Thanks.

 include/linux/cgroup.h |   17 +++++++++++
 kernel/cgroup.c        |   71 +++++++++++++++++++++++++++++++++++++++++------
 mm/memcontrol.c        |    1 +
 3 files changed, 80 insertions(+), 9 deletions(-)

include/linux/cgroup.h +17 -0

index be81faf..565c803 100644
@@ -16,6 +16,7 @@
 #include <linux/prio_heap.h>
 #include <linux/rwsem.h>
 #include <linux/idr.h>
+#include <linux/workqueue.h>
 
 #ifdef CONFIG_CGROUPS

@@ -76,12 +77,16 @@ struct cgroup_subsys_state {
 	unsigned long flags;
 	/* ID for this css, if possible */
 	struct css_id __rcu *id;
+
+	/* Used to put @cgroup->dentry on the last css_put() */
+	struct work_struct dput_work;
 };
 
 /* bits in struct cgroup_subsys_state flags field */
 enum {
 	CSS_ROOT, /* This CSS is the root of the subsystem */
 	CSS_REMOVED, /* This CSS is dead */
+	CSS_CLEAR_CSS_REFS,		/* @ss->__DEPRECATED_clear_css_refs */
 };
 
 /* Caller must verify that the css is not for root cgroup */

@@ -480,6 +485,18 @@ struct cgroup_subsys {
 	 * (not available in early_init time.)
 	 */
 	bool use_id;
+
+	/*
+	 * If %true, cgroup removal will try to clear css refs by retrying
+	 * ss->pre_destroy() until there's no css ref left.  This behavior
+	 * is strictly for backward compatibility and will be removed as
+	 * soon as the current user (memcg) is updated.
+	 *
+	 * If %false, ss->pre_destroy() can't fail and cgroup removal won't
+	 * wait for css refs to drop to zero before proceeding.
+	 */
+	bool __DEPRECATED_clear_css_refs;
+
 #define MAX_CGROUP_TYPE_NAMELEN 32
 	const char *name;

kernel/cgroup.c +62 -9

126 lines not on this page · raw message

mm/memcontrol.c +1 -0

index bef1142..d28359c 100644
@@ -5635,6 +5635,7 @@ struct cgroup_subsys mem_cgroup_subsys = {
 	.base_cftypes = mem_cgroup_files,
 	.early_init = 0,
 	.use_id = 1,
+	.__DEPRECATED_clear_css_refs = true,
 };
 
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP

Signature · 1 line

1.7.7.3

Hiroyuki Kamezawa

2012-03-31 21:28 UTC #2 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

2012年3月31日7:36 Tejun Heo <[email protected]>:

Quoted 27 lines

Currently, cgroup removal tries to drain all css references. If there are active css references, the removal logic waits and retries ->pre_detroy() until either all refs drop to zero or removal is cancelled.
This semantics is unusual and adds non-trivial complexity to cgroup core and IMHO is fundamentally misguided in that it couples internal implementation details (references to internal data structure) with externally visible operation (rmdir). To userland, this is a behavior peculiarity which is unnecessary and difficult to expect (css refs is otherwise invisible from userland), and, to policy implementations, this is an unnecessary restriction (e.g. blkcg wants to hold css refs for caching purposes but can't as that becomes visible as rmdir hang).
Unfortunately, memcg currently depends on ->pre_destroy() retrials and cgroup removal vetoing and can't be immmediately switched to the new behavior. This patch introduces the new behavior of not waiting for css refs to drain and maintains the old behavior for subsystems which have __DEPRECATED_clear_css_refs set.
Once, memcg is updated, we can drop the code paths for the old behavior as proposed in the following patch. Note that the following patch is incorrect in that dput work item is in cgroup and may lose some of dputs when multiples css's are released back-to-back, and __css_put() triggers check_for_release() when refcnt reaches 0 instead of 1; however, it shows what part can be removed.

Quoted 6 lines

http://thread.gmane.org/gmane.linux.kernel.containers/22559/focus=75251
Note that, in not-too-distant future, cgroup core will start emitting warning messages for subsys which require the old behavior, so please get moving.

Ok, I'll update memcg ASAP (after LSF/MM).
I'm now thinking to move all resources to root if use_hierarchy=0.

Thanks,
-Kame

A message that is not in the archive (sent off-list or not yet received): [email protected]

Hiroyuki Kamezawa

2012-03-31 21:28 UTC #4 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

2012年3月31日7:36 Tejun Heo <[email protected]>:

Quoted 27 lines

Currently, cgroup removal tries to drain all css references. If there are active css references, the removal logic waits and retries ->pre_detroy() until either all refs drop to zero or removal is cancelled.
This semantics is unusual and adds non-trivial complexity to cgroup core and IMHO is fundamentally misguided in that it couples internal implementation details (references to internal data structure) with externally visible operation (rmdir). To userland, this is a behavior peculiarity which is unnecessary and difficult to expect (css refs is otherwise invisible from userland), and, to policy implementations, this is an unnecessary restriction (e.g. blkcg wants to hold css refs for caching purposes but can't as that becomes visible as rmdir hang).
Unfortunately, memcg currently depends on ->pre_destroy() retrials and cgroup removal vetoing and can't be immmediately switched to the new behavior. This patch introduces the new behavior of not waiting for css refs to drain and maintains the old behavior for subsystems which have __DEPRECATED_clear_css_refs set.
Once, memcg is updated, we can drop the code paths for the old behavior as proposed in the following patch. Note that the following patch is incorrect in that dput work item is in cgroup and may lose some of dputs when multiples css's are released back-to-back, and __css_put() triggers check_for_release() when refcnt reaches 0 instead of 1; however, it shows what part can be removed.

Quoted 6 lines

http://thread.gmane.org/gmane.linux.kernel.containers/22559/focus=75251
Note that, in not-too-distant future, cgroup core will start emitting warning messages for subsys which require the old behavior, so please get moving.

Ok, I'll update memcg ASAP (after LSF/MM).
I'm now thinking to move all resources to root if use_hierarchy=0.

Thanks,
-Kame

Li Zefan

Acked-by

2012-04-01 02:00 UTC #5 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

In reply to a message not in the archive

Quoted 5 lines · Tejun Heo

Currently, cgroup removal tries to drain all css references. If there are active css references, the removal logic waits and retries ->pre_detroy() until either all refs drop to zero or removal is cancelled.

I don't like this either.

Quoted 22 lines

This semantics is unusual and adds non-trivial complexity to cgroup core and IMHO is fundamentally misguided in that it couples internal implementation details (references to internal data structure) with externally visible operation (rmdir). To userland, this is a behavior peculiarity which is unnecessary and difficult to expect (css refs is otherwise invisible from userland), and, to policy implementations, this is an unnecessary restriction (e.g. blkcg wants to hold css refs for caching purposes but can't as that becomes visible as rmdir hang).
Unfortunately, memcg currently depends on ->pre_destroy() retrials and cgroup removal vetoing and can't be immmediately switched to the new behavior. This patch introduces the new behavior of not waiting for css refs to drain and maintains the old behavior for subsystems which have __DEPRECATED_clear_css_refs set.
Once, memcg is updated, we can drop the code paths for the old behavior as proposed in the following patch. Note that the following patch is incorrect in that dput work item is in cgroup and may lose some of dputs when multiples css's are released back-to-back, and __css_put() triggers check_for_release() when refcnt reaches 0 instead of 1; however, it shows what part can be removed.

Quoted 7 lines

http://thread.gmane.org/gmane.linux.kernel.containers/22559/focus=75251
Note that, in not-too-distant future, cgroup core will start emitting warning messages for subsys which require the old behavior, so please get moving.
Signed-off-by Tejun Heo <[email protected]>

Both patches look good.

Acked-by Li Zefan <[email protected]>

and I'd like to see code shrink with memcg updates ASAP.

Quoted 5 lines

Cc Vivek Goyal <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Balbir Singh <[email protected]>
Cc KAMEZAWA Hiroyuki <[email protected]>

Li Zefan

Acked-by on v1 2/2

2012-04-01 02:00 UTC #6 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

In reply to Tejun Heo

Quoted 5 lines · Tejun Heo

Currently, cgroup removal tries to drain all css references. If there are active css references, the removal logic waits and retries ->pre_detroy() until either all refs drop to zero or removal is cancelled.

I don't like this either.

Quoted 22 lines

This semantics is unusual and adds non-trivial complexity to cgroup core and IMHO is fundamentally misguided in that it couples internal implementation details (references to internal data structure) with externally visible operation (rmdir). To userland, this is a behavior peculiarity which is unnecessary and difficult to expect (css refs is otherwise invisible from userland), and, to policy implementations, this is an unnecessary restriction (e.g. blkcg wants to hold css refs for caching purposes but can't as that becomes visible as rmdir hang).
Unfortunately, memcg currently depends on ->pre_destroy() retrials and cgroup removal vetoing and can't be immmediately switched to the new behavior. This patch introduces the new behavior of not waiting for css refs to drain and maintains the old behavior for subsystems which have __DEPRECATED_clear_css_refs set.
Once, memcg is updated, we can drop the code paths for the old behavior as proposed in the following patch. Note that the following patch is incorrect in that dput work item is in cgroup and may lose some of dputs when multiples css's are released back-to-back, and __css_put() triggers check_for_release() when refcnt reaches 0 instead of 1; however, it shows what part can be removed.

Quoted 7 lines

http://thread.gmane.org/gmane.linux.kernel.containers/22559/focus=75251
Note that, in not-too-distant future, cgroup core will start emitting warning messages for subsys which require the old behavior, so please get moving.
Signed-off-by Tejun Heo <[email protected]>

Both patches look good.

Acked-by Li Zefan <[email protected]>

and I'd like to see code shrink with memcg updates ASAP.

Quoted 5 lines

Cc Vivek Goyal <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Balbir Singh <[email protected]>
Cc KAMEZAWA Hiroyuki <[email protected]>

Tejun Heo

author maintainer

2012-04-01 18:54 UTC #7 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

Hello, Li.

Quoted 5 lines · Li Zefan

Quoted 1 line
Signed-off-by Tejun Heo <[email protected]>
Both patches look good.
Acked-by Li Zefan <[email protected]>

Alright, adding the patches to cgroup/for-3.5.

Quoted 1 line

and I'd like to see code shrink with memcg updates ASAP.

Amen. :)

Signature · 1 line

tejun

Sasha Levin

2012-05-16 22:33 UTC #8 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

Hi Tejun,

Quoted 10 lines · Tejun Heo

Hello, Li.
Quoted 5 lines · Li Zefan
Quoted 1 line
Signed-off-by Tejun Heo <[email protected]>
Both patches look good.
Acked-by Li Zefan <[email protected]>
Alright, adding the patches to cgroup/for-3.5.

This patch allows for race when removing a cgroup since one of the css's may still have a dentry ref when the cgroup is removed, no? Is there a plan to deal with that before this patch gets pushed into 3.5?

Tejun Heo

author maintainer

2012-05-18 17:55 UTC #9 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

Quoted 10 lines · Sasha Levin

Hi Tejun,
Quoted 7 lines · Tejun Heo
Hello, Li.
Quoted 4 lines · Li Zefan
Quoted 1 line
Signed-off-by Tejun Heo <[email protected]>
Both patches look good.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Acked-by Li Zefan <[email protected]>
Alright, adding the patches to cgroup/for-3.5.
This patch allows for race when removing a cgroup since one of the css's may still have a dentry ref when the cgroup is removed, no? Is there a plan to deal with that before this patch gets pushed into 3.5?

Can you please elaborate?

Thanks.

Signature · 1 line

tejun

Sasha Levin

2012-05-18 18:28 UTC #10 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

Quoted 13 lines · Tejun Heo

Quoted 12 lines · Sasha Levin
Hi Tejun,
Quoted 9 lines · Tejun Heo
Hello, Li.
Quoted 5 lines · Li Zefan
Quoted 1 line
Signed-off-by Tejun Heo <[email protected]>
Both patches look good.
Acked-by Li Zefan <[email protected]>

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Alright, adding the patches to cgroup/for-3.5.
This patch allows for race when removing a cgroup since one of the css's may still have a dentry ref when the cgroup is removed, no? Is there a plan to deal with that before this patch gets pushed into 3.5?
Can you please elaborate?

From what I gathered, that patch means that you'll have refs to dentries while you're trying to complete the unmount of the cgroup, which would make something really angry.

I tested it by running two scripts to race with eachother:

First:

while [ true ];
do
mount -t cgroup dummy cgroup/
mkdir cgroup/0
rmdir cgroup/0
umount cgroup/
done

Second:

while [ true ];
do
mount -t cgroup dummy cgroup/
umount cgroup/
done

Now, if you give it a go, you'll see a BUG() and a system hang pretty fast.

Tejun Heo

author maintainer

2012-05-23 22:22 UTC #11 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

Hello, Sasha.

Does the following patch fix the problem you're seeing?

Thanks.

---
 kernel/cgroup.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

kernel/cgroup.c +14 -3

index ad8eae5..7cf9669 100644
@@ -896,10 +896,13 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode)
 		mutex_unlock(&cgroup_mutex);
 
 		/*
-		 * Drop the active superblock reference that we took when we
-		 * created the cgroup
+		 * We want to drop the active superblock reference that we
+		 * took when we created the cgroup after all dentry refs
+		 * are gone - kill_sb gets mighty unhappy otherwise.  Set
+		 * dentry->d_fsdata to cgroup_diput() to tell
+		 * cgroup_d_release() to call deactivate_super().
 		 */
-		deactivate_super(cgrp->root->sb);
+		dentry->d_fsdata = cgroup_diput;
 
 		/*
 		 * if we're getting rid of the cgroup, refcount should ensure

@@ -925,6 +928,13 @@ static int cgroup_delete(const struct dentry *d)
 	return 1;
 }
 
+static void cgroup_d_release(struct dentry *dentry)
+{
+	/* did cgroup_diput() tell me to deactivate super? */
+	if (dentry->d_fsdata == cgroup_diput)
+		deactivate_super(dentry->d_sb);
+}
+
 static void remove_dir(struct dentry *d)
 {
 	struct dentry *parent = dget(d->d_parent);

@@ -1532,6 +1542,7 @@ static int cgroup_get_rootdir(struct super_block *sb)
 	static const struct dentry_operations cgroup_dops = {
 		.d_iput = cgroup_diput,
 		.d_delete = cgroup_delete,
+		.d_release = cgroup_d_release,
 	};
 
 	struct inode *inode =

Sasha Levin

2012-05-24 13:21 UTC #12 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

Quoted 5 lines · Tejun Heo

Hello, Sasha.
Does the following patch fix the problem you're seeing?
Thanks.

Yup, that did the trick.

Thanks!

Tejun Heo

author maintainer

2012-05-24 15:41 UTC #13 lore raw

PATCH 1/1 cgroup: superblock can't be released with active dentries

From 88787c483106c5830a46d18deaffdc1e70929af7 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Thu, 24 May 2012 08:24:39 -0700

48ddbe1946 "cgroup: make css->refcnt clearing on cgroup removal optional" allowed a css to linger after the associated cgroup is removed. As a css holds a reference on the cgroup's dentry, it means that cgroup dentries may linger for a while.

cgroup_create() does grab an active reference on the superblock to prevent it from going away while there are !root cgroups; however, the reference is put from cgroup_diput() which is invoked on cgroup removal, so cgroup dentries which are removed but persisting due to lingering csses already have released their superblock active refs allowing superblock to be killed while those dentries are around.

Given the right condition, this makes cgroup_kill_sb() call kill_litter_super() with dentries with non-zero d_count leading to BUG() in shrink_dcache_for_umount_subtree().

Fix it by adding cgroup_dops->d_release() operation and moving deactivate_super() to it. cgroup_diput() now marks dentry->d_fsdata with itself if superblock should be deactivated and cgroup_d_release() deactivates the superblock on dentry release.

Signed-off-by Tejun Heo <[email protected]>

Reported-by Sasha Levin <[email protected]>

Tested-by Sasha Levin <[email protected]>

LKML-Reference: <CA+1xoqe5hMuxzCRhMy7J0XchDk2ZnuxOHJKikROk1-ReAzcT6g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
---
Li, can you please ack this?

Thanks.

 kernel/cgroup.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

kernel/cgroup.c +14 -3

index ad8eae5..e887b55 100644
@@ -896,10 +896,13 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode)
 		mutex_unlock(&cgroup_mutex);
 
 		/*
-		 * Drop the active superblock reference that we took when we
-		 * created the cgroup
+		 * We want to drop the active superblock reference from the
+		 * cgroup creation after all the dentry refs are gone -
+		 * kill_sb gets mighty unhappy otherwise.  Mark
+		 * dentry->d_fsdata with cgroup_diput() to tell
+		 * cgroup_d_release() to call deactivate_super().
 		 */
-		deactivate_super(cgrp->root->sb);
+		dentry->d_fsdata = cgroup_diput;
 
 		/*
 		 * if we're getting rid of the cgroup, refcount should ensure

@@ -925,6 +928,13 @@ static int cgroup_delete(const struct dentry *d)
 	return 1;
 }
 
+static void cgroup_d_release(struct dentry *dentry)
+{
+	/* did cgroup_diput() tell me to deactivate super? */
+	if (dentry->d_fsdata == cgroup_diput)
+		deactivate_super(dentry->d_sb);
+}
+
 static void remove_dir(struct dentry *d)
 {
 	struct dentry *parent = dget(d->d_parent);

@@ -1532,6 +1542,7 @@ static int cgroup_get_rootdir(struct super_block *sb)
 	static const struct dentry_operations cgroup_dops = {
 		.d_iput = cgroup_diput,
 		.d_delete = cgroup_delete,
+		.d_release = cgroup_d_release,
 	};
 
 	struct inode *inode =

Signature · 1 line

1.7.7.3

Li Zefan

Acked-by on v1 1/1

2012-05-28 00:18 UTC #14 lore raw

Re: [PATCH] cgroup: superblock can't be released with active dentries

Quoted 25 lines · Tejun Heo

Quoted 1 line
From 88787c483106c5830a46d18deaffdc1e70929af7 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Thu, 24 May 2012 08:24:39 -0700
48ddbe1946 "cgroup: make css->refcnt clearing on cgroup removal optional" allowed a css to linger after the associated cgroup is removed. As a css holds a reference on the cgroup's dentry, it means that cgroup dentries may linger for a while.
cgroup_create() does grab an active reference on the superblock to prevent it from going away while there are !root cgroups; however, the reference is put from cgroup_diput() which is invoked on cgroup removal, so cgroup dentries which are removed but persisting due to lingering csses already have released their superblock active refs allowing superblock to be killed while those dentries are around.
Given the right condition, this makes cgroup_kill_sb() call kill_litter_super() with dentries with non-zero d_count leading to BUG() in shrink_dcache_for_umount_subtree().
Fix it by adding cgroup_dops->d_release() operation and moving deactivate_super() to it. cgroup_diput() now marks dentry->d_fsdata with itself if superblock should be deactivated and cgroup_d_release() deactivates the superblock on dentry release.

Quoted 4 lines

Signed-off-by Tejun Heo <[email protected]>
Reported-by Sasha Levin <[email protected]>
Tested-by Sasha Levin <[email protected]>
LKML-Reference: <CA+1xoqe5hMuxzCRhMy7J0XchDk2ZnuxOHJKikROk1-ReAzcT6g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

Acked-by Li Zefan <[email protected]>

A message that is not in the archive (sent off-list or not yet received): CA+1xoqdbtkC5Ue3s1qZHHJBLcj9a=djFDGny6AJJSV-pdn7mcA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Tejun Heo

2012-05-24 15:41 UTC #16 lore raw

PATCH 1/1 cgroup: superblock can't be released with active dentries

From 88787c483106c5830a46d18deaffdc1e70929af7 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Thu, 24 May 2012 08:24:39 -0700

Given the right condition, this makes cgroup_kill_sb() call kill_litter_super() with dentries with non-zero d_count leading to BUG() in shrink_dcache_for_umount_subtree().

Signed-off-by Tejun Heo <[email protected]>

Reported-by Sasha Levin <[email protected]>

Tested-by Sasha Levin <[email protected]>

LKML-Reference: <CA+1xoqe5hMuxzCRhMy7J0XchDk2ZnuxOHJKikROk1-ReAzcT6g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
---
Li, can you please ack this?

Thanks.

 kernel/cgroup.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

kernel/cgroup.c +14 -3

index ad8eae5..e887b55 100644
@@ -896,10 +896,13 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode)
 		mutex_unlock(&cgroup_mutex);
 
 		/*
-		 * Drop the active superblock reference that we took when we
-		 * created the cgroup
+		 * We want to drop the active superblock reference from the
+		 * cgroup creation after all the dentry refs are gone -
+		 * kill_sb gets mighty unhappy otherwise.  Mark
+		 * dentry->d_fsdata with cgroup_diput() to tell
+		 * cgroup_d_release() to call deactivate_super().
 		 */
-		deactivate_super(cgrp->root->sb);
+		dentry->d_fsdata = cgroup_diput;
 
 		/*
 		 * if we're getting rid of the cgroup, refcount should ensure

@@ -925,6 +928,13 @@ static int cgroup_delete(const struct dentry *d)
 	return 1;
 }
 
+static void cgroup_d_release(struct dentry *dentry)
+{
+	/* did cgroup_diput() tell me to deactivate super? */
+	if (dentry->d_fsdata == cgroup_diput)
+		deactivate_super(dentry->d_sb);
+}
+
 static void remove_dir(struct dentry *d)
 {
 	struct dentry *parent = dget(d->d_parent);

@@ -1532,6 +1542,7 @@ static int cgroup_get_rootdir(struct super_block *sb)
 	static const struct dentry_operations cgroup_dops = {
 		.d_iput = cgroup_diput,
 		.d_delete = cgroup_delete,
+		.d_release = cgroup_d_release,
 	};
 
 	struct inode *inode =

Signature · 1 line

1.7.7.3

A message that is not in the archive (sent off-list or not yet received): 20120523222242.GD3933-Gd/HAXX7CRxy/[email protected]

Sasha Levin

2012-05-24 13:21 UTC #18 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

Quoted 5 lines · Tejun Heo

Hello, Sasha.
Does the following patch fix the problem you're seeing?
Thanks.

Yup, that did the trick.

Thanks!

A message that is not in the archive (sent off-list or not yet received): CA+1xoqfr5aaGbxoX3YKrJHXTjU9fWSbX_xbuEOU=4K7kMay6XQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Tejun Heo

2012-05-23 22:22 UTC #20 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

Hello, Sasha.

Does the following patch fix the problem you're seeing?

Thanks.

---
 kernel/cgroup.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

kernel/cgroup.c +14 -3

index ad8eae5..7cf9669 100644
@@ -896,10 +896,13 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode)
 		mutex_unlock(&cgroup_mutex);
 
 		/*
-		 * Drop the active superblock reference that we took when we
-		 * created the cgroup
+		 * We want to drop the active superblock reference that we
+		 * took when we created the cgroup after all dentry refs
+		 * are gone - kill_sb gets mighty unhappy otherwise.  Set
+		 * dentry->d_fsdata to cgroup_diput() to tell
+		 * cgroup_d_release() to call deactivate_super().
 		 */
-		deactivate_super(cgrp->root->sb);
+		dentry->d_fsdata = cgroup_diput;
 
 		/*
 		 * if we're getting rid of the cgroup, refcount should ensure

@@ -925,6 +928,13 @@ static int cgroup_delete(const struct dentry *d)
 	return 1;
 }
 
+static void cgroup_d_release(struct dentry *dentry)
+{
+	/* did cgroup_diput() tell me to deactivate super? */
+	if (dentry->d_fsdata == cgroup_diput)
+		deactivate_super(dentry->d_sb);
+}
+
 static void remove_dir(struct dentry *d)
 {
 	struct dentry *parent = dget(d->d_parent);

@@ -1532,6 +1542,7 @@ static int cgroup_get_rootdir(struct super_block *sb)
 	static const struct dentry_operations cgroup_dops = {
 		.d_iput = cgroup_diput,
 		.d_delete = cgroup_delete,
+		.d_release = cgroup_d_release,
 	};
 
 	struct inode *inode =

A message that is not in the archive (sent off-list or not yet received): [email protected]

Sasha Levin

2012-05-18 18:28 UTC #22 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

Quoted 13 lines · Tejun Heo

Quoted 12 lines · Sasha Levin
Hi Tejun,
Quoted 9 lines · Tejun Heo
Hello, Li.
Quoted 5 lines · Li Zefan
Quoted 1 line
Signed-off-by Tejun Heo <[email protected]>
Both patches look good.
Acked-by Li Zefan <[email protected]>

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Alright, adding the patches to cgroup/for-3.5.
This patch allows for race when removing a cgroup since one of the css's may still have a dentry ref when the cgroup is removed, no? Is there a plan to deal with that before this patch gets pushed into 3.5?
Can you please elaborate?

From what I gathered, that patch means that you'll have refs to dentries while you're trying to complete the unmount of the cgroup, which would make something really angry.

I tested it by running two scripts to race with eachother:

First:

while [ true ];
do
mount -t cgroup dummy cgroup/
mkdir cgroup/0
rmdir cgroup/0
umount cgroup/
done

Second:

while [ true ];
do
mount -t cgroup dummy cgroup/
umount cgroup/
done

Now, if you give it a go, you'll see a BUG() and a system hang pretty fast.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Tejun Heo

2012-04-01 18:54 UTC #24 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

Hello, Li.

Quoted 5 lines · Li Zefan

Quoted 1 line
Signed-off-by Tejun Heo <[email protected]>
Both patches look good.
Acked-by Li Zefan <[email protected]>

Alright, adding the patches to cgroup/for-3.5.

Quoted 1 line

and I'd like to see code shrink with memcg updates ASAP.

Amen. :)

Signature · 1 line

tejun

Vivek Goyal

2012-04-01 19:48 UTC #25 lore raw

Re: [PATCH 2/2] cgroup: make css->refcnt clearing on cgroup removal optional

In reply to Tejun Heo

Quoted 5 lines · Tejun Heo

Currently, cgroup removal tries to drain all css references. If there are active css references, the removal logic waits and retries ->pre_detroy() until either all refs drop to zero or removal is cancelled.

Quoted 8 lines

This semantics is unusual and adds non-trivial complexity to cgroup core and IMHO is fundamentally misguided in that it couples internal implementation details (references to internal data structure) with externally visible operation (rmdir). To userland, this is a behavior peculiarity which is unnecessary and difficult to expect (css refs is otherwise invisible from userland), and, to policy implementations, this is an unnecessary restriction (e.g. blkcg wants to hold css refs for caching purposes but can't as that becomes visible as rmdir hang).

Good to see this patch. Now annyoying rmdir hang with blkcg should be gone.

Thanks
Vivek