cgroup: removing css reference drain wait during cgroup removal

I'm appending the patch for the cgroup part of the change. It removes good amount of complication. I think cgroup_has_css_refs() check should be dropped from check_for_release() but other than that it seems to work fine with blkcg in linux-next.

I'm appending the patch for the cgroup part of the change. It removes good amount of complication. I think cgroup_has_css_refs() check should be dropped from check_for_release() but other than that it seems to work fine with blkcg in linux-next.

(wrong mailing list addresses, reposing. sorry guys)

Hello, guys.

While working on blkcg, I learned that cgroup removal path tries to drain all internal references synchronously before proceeding with removal, and may be aborted by by pre_destroy() failing.

I find both quite unusual. While there are some occassions where we try to drain reference counts synchronously, the norm is deactivating the target and then releasing it when the reference count hits zero and exposing this synchronous behavior directly to userland makes it worse. This also requires allowing rmdir to be aborted from userland.

pre_destroy() being allowed to veto rmdir might be okay if there's only one subsystem using it or it implemented proper prepare-commit/cancel transaction, but as it currently stands, pre_destroy() operations are not reversible and even within memcg itself the state wouldn't be consistent after failure (some moved to parent while the rest on child). Note that abort from userland also has the same problem.

It also complicates and adds even more subtleties to cgroup code. A lot of it was just me being dumb but making sense of cgroup_exclude_rmdir() and cgroup_release_and_wakeup_rmdir() usages in memcontrol took me quite some time even with Hugh's help.

In general, IMHO, it's a bad idea to expose purely internal implementation details to userland directly. Internal ref counts can be kept around for whatever reason (e.g. blkcg does it for lookup caching), such details shouldn't be visible to userland. Midlayers like cgroup which sit between userland and mechanism implementations should provide isolation between the two so that each mechanism implementation doesn't have to worry about things like that.

1. disallow further css_tryget() on all affected subsystems

2. call pre_destroy() on each subsystem. pre_destroy() can't fail and each subsystem is responsible for guaranteeing that all reference counts to its css will be released in finite amount of time.

3. destroy() will be called when all css refs drop to zero.

To do this, memcg, the only current user of pre_destroy() callback, should be modified such that

* pre_destroy() doesn't fail. Greg helped me walking through the different failure paths. Two of them seem to be from race conditions which just require retries (I think it's wrong to fail userland operation for things like this). pre_destroy() itself may loop or may schedule a work item if it's expected to take a long time.

The last one seems more tricky. On destruction of cgroup, the charges are transferred to its parent and the parent may not have enough room for that. Greg told me that this should only be a problem for !hierarchical case. I think this can be dealt with by dumping what's left over to root cgroup with a warning message.

* cgroup_exclude_rmdir() and cgroup_release_and_wakeup_rmdir() usages. The pair is used to re-trigger pre_destroy() for operations which may add css references while rmdir is in progress. I think the right thing to do is calling (directly or through a work item) mem_cgroup_force_empty() if the css is dead after the operation. I think such change would make following the logic easier too.

I'm appending the patch for the cgroup part of the change. It removes good amount of complication. I think cgroup_has_css_refs() check should be dropped from check_for_release() but other than that it seems to work fine with blkcg in linux-next.

If memcg's pre_destroy() can be updated as described above, I can apply that first and then apply the following cgroup behavior change.

What do you think? Li, how does the cgroup portion look?

Thank you.

RFC PATCH. DON'T APPLY.

---
 include/linux/cgroup.h |   41 +-------
 kernel/cgroup.c        |  225 +++++++++++++------------------------------------
 2 files changed, 69 insertions(+), 197 deletions(-)

Index: work/include/linux/cgroup.h
===================================================================

include/linux/cgroup.h +9 -32

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

@@ -81,7 +82,6 @@ struct cgroup_subsys_state {
 /* 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 */
 };
 
 /* Caller must verify that the css is not for root cgroup */

@@ -104,27 +104,18 @@ static inline void css_get(struct cgroup
 		__css_get(css, 1);
 }
 
-static inline bool css_is_removed(struct cgroup_subsys_state *css)
-{
-	return test_bit(CSS_REMOVED, &css->flags);
-}
-
 /*
  * Call css_tryget() to take a reference on a css if your existing
  * (known-valid) reference isn't already ref-counted. Returns false if
  * 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 +123,11 @@ static inline bool css_tryget(struct cgr
  * 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 */

@@ -211,6 +202,9 @@ struct cgroup {
 	/* List of events which userspace want to receive */
 	struct list_head event_list;
 	spinlock_t event_list_lock;
+
+	/* Used to dput @cgroup->dentry from css_put() */
+	struct work_struct dput_work;
 };
 
 /*

@@ -408,23 +402,6 @@ int cgroup_task_count(const struct cgrou
 int cgroup_is_descendant(const struct cgroup *cgrp, struct task_struct *task);
 
 /*
- * When the subsys has to access css and may add permanent refcnt to css,
- * it should take care of racy conditions with rmdir(). Following set of
- * functions, is for stop/restart rmdir if necessary.
- * Because these will call css_get/put, "css" should be alive css.
- *
- *  cgroup_exclude_rmdir();
- *  ...do some jobs which may access arbitrary empty cgroup
- *  cgroup_release_and_wakeup_rmdir();
- *
- *  When someone removes a cgroup while cgroup_exclude_rmdir() holds it,
- *  it sleeps and cgroup_release_and_wakeup_rmdir() will wake him up.
- */
-
-void cgroup_exclude_rmdir(struct cgroup_subsys_state *css);
-void cgroup_release_and_wakeup_rmdir(struct cgroup_subsys_state *css);
-
-/*
  * Control Group taskset, used to pass around set of tasks to cgroup_subsys
  * methods.
  */

@@ -453,7 +430,7 @@ int cgroup_taskset_size(struct cgroup_ta
 
 struct cgroup_subsys {
 	struct cgroup_subsys_state *(*create)(struct cgroup *cgrp);
-	int (*pre_destroy)(struct cgroup *cgrp);
+	void (*pre_destroy)(struct cgroup *cgrp);
 	void (*destroy)(struct cgroup *cgrp);
 	int (*can_attach)(struct cgroup *cgrp, struct cgroup_taskset *tset);
 	void (*cancel_attach)(struct cgroup *cgrp, struct cgroup_taskset *tset);

Index: work/kernel/cgroup.c
===================================================================

kernel/cgroup.c +60 -166

@@ -63,6 +63,8 @@
 
 #include <linux/atomic.h>
 
+#define CSS_DEAD_BIAS		INT_MIN
+
 /*
  * cgroup_mutex is the master lock.  Any modification to cgroup or its
  * hierarchy must be performed while holding it.

@@ -154,8 +156,8 @@ struct css_id {
 	 * The css to which this ID points. This pointer is set to valid value
 	 * after cgroup is populated. If cgroup is removed, this will be NULL.
 	 * This pointer is expected to be RCU-safe because destroy()
-	 * is called after synchronize_rcu(). But for safe use, css_is_removed()
-	 * css_tryget() should be used for avoiding race.
+	 * is called after synchronize_rcu(). But for safe use, css_tryget()
+	 * should be used for avoiding race.
 	 */
 	struct cgroup_subsys_state __rcu *css;
 	/*

@@ -807,39 +809,14 @@ static struct inode *cgroup_new_inode(um
 	return inode;
 }
 
-/*
- * Call subsys's pre_destroy handler.
- * This is called before css refcnt check.
- */
-static int cgroup_call_pre_destroy(struct cgroup *cgrp)
-{
-	struct cgroup_subsys *ss;
-	int ret = 0;
-
-	for_each_subsys(cgrp->root, ss)
-		if (ss->pre_destroy) {
-			ret = ss->pre_destroy(cgrp);
-			if (ret)
-				break;
-		}
-
-	return ret;
-}
-
 static void cgroup_diput(struct dentry *dentry, struct inode *inode)
 {
 	/* is dentry a directory ? if so, kfree() associated cgroup */
 	if (S_ISDIR(inode->i_mode)) {
 		struct cgroup *cgrp = dentry->d_fsdata;
 		struct cgroup_subsys *ss;
+
 		BUG_ON(!(cgroup_is_removed(cgrp)));
-		/* It's possible for external users to be holding css
-		 * reference counts on a cgroup; css_put() needs to
-		 * be able to access the cgroup after decrementing
-		 * the reference count in order to know if it needs to
-		 * queue the cgroup to be handled by the release
-		 * agent */
-		synchronize_rcu();
 
 		mutex_lock(&cgroup_mutex);
 		/*

@@ -931,33 +908,6 @@ static void cgroup_d_remove_dir(struct d
 }
 
 /*
- * A queue for waiters to do rmdir() cgroup. A tasks will sleep when
- * cgroup->count == 0 && list_empty(&cgroup->children) && subsys has some
- * reference to css->refcnt. In general, this refcnt is expected to goes down
- * to zero, soon.
- *
- * CGRP_WAIT_ON_RMDIR flag is set under cgroup's inode->i_mutex;
- */
-static DECLARE_WAIT_QUEUE_HEAD(cgroup_rmdir_waitq);
-
-static void cgroup_wakeup_rmdir_waiter(struct cgroup *cgrp)
-{
-	if (unlikely(test_and_clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags)))
-		wake_up_all(&cgroup_rmdir_waitq);
-}
-
-void cgroup_exclude_rmdir(struct cgroup_subsys_state *css)
-{
-	css_get(css);
-}
-
-void cgroup_release_and_wakeup_rmdir(struct cgroup_subsys_state *css)
-{
-	cgroup_wakeup_rmdir_waiter(css->cgroup);
-	css_put(css);
-}
-
-/*
  * Call with cgroup_mutex held. Drops reference counts on modules, including
  * any duplicate ones that parse_cgroupfs_options took. If this function
  * returns an error, no reference counts are touched.

@@ -1329,6 +1279,13 @@ static const struct super_operations cgr
 	.remount_fs = cgroup_remount,
 };
 
+static void cgroup_dput_fn(struct work_struct *work)
+{
+	struct cgroup *cgrp = container_of(work, struct cgroup, dput_work);
+
+	dput(cgrp->dentry);
+}
+
 static void init_cgroup_housekeeping(struct cgroup *cgrp)
 {
 	INIT_LIST_HEAD(&cgrp->sibling);

@@ -1339,6 +1296,7 @@ static void init_cgroup_housekeeping(str
 	mutex_init(&cgrp->pidlist_mutex);
 	INIT_LIST_HEAD(&cgrp->event_list);
 	spin_lock_init(&cgrp->event_list_lock);
+	INIT_WORK(&cgrp->dput_work, cgroup_dput_fn);
 }
 
 static void init_cgroup_root(struct cgroupfs_root *root)

@@ -1936,12 +1894,6 @@ int cgroup_attach_task(struct cgroup *cg
 	}
 
 	synchronize_rcu();
-
-	/*
-	 * wake up rmdir() waiter. the rmdir should fail since the cgroup
-	 * is no longer empty.
-	 */
-	cgroup_wakeup_rmdir_waiter(cgrp);
 out:
 	if (retval) {
 		for_each_subsys(root, ss) {

@@ -2111,7 +2063,6 @@ static int cgroup_attach_proc(struct cgr
 	 * step 5: success! and cleanup
 	 */
 	synchronize_rcu();
-	cgroup_wakeup_rmdir_waiter(cgrp);
 	retval = 0;
 out_put_css_set_refs:
 	if (retval) {

@@ -3768,6 +3719,7 @@ static long cgroup_create(struct cgroup 
 			err = PTR_ERR(css);
 			goto err_destroy;
 		}
+		dget(dentry);		/* will be put on the last @css put */
 		init_cgroup_css(css, ss, cgrp);
 		if (ss->use_id) {
 			err = alloc_css_id(ss, parent, cgrp);

@@ -3809,8 +3761,10 @@ static long cgroup_create(struct cgroup 
  err_destroy:
 
 	for_each_subsys(root, ss) {
-		if (cgrp->subsys[ss->subsys_id])
+		if (cgrp->subsys[ss->subsys_id]) {
+			dput(dentry);
 			ss->destroy(cgrp);
+		}
 	}
 
 	mutex_unlock(&cgroup_mutex);

@@ -3866,70 +3820,15 @@ static int cgroup_has_css_refs(struct cg
 	return 0;
 }
 
-/*
- * Atomically mark all (or else none) of the cgroup's CSS objects as
- * CSS_REMOVED. Return true on success, or false if the cgroup has
- * busy subsystems. Call with cgroup_mutex held
- */
-
-static int cgroup_clear_css_refs(struct cgroup *cgrp)
-{
-	struct cgroup_subsys *ss;
-	unsigned long flags;
-	bool failed = false;
-	local_irq_save(flags);
-	for_each_subsys(cgrp->root, ss) {
-		struct cgroup_subsys_state *css = cgrp->subsys[ss->subsys_id];
-		int refcnt;
-		while (1) {
-			/* We can only remove a CSS with a refcnt==1 */
-			refcnt = atomic_read(&css->refcnt);
-			if (refcnt > 1) {
-				failed = true;
-				goto done;
-			}
-			BUG_ON(!refcnt);
-			/*
-			 * Drop the refcnt to 0 while we check other
-			 * subsystems. This will cause any racing
-			 * css_tryget() to spin until we set the
-			 * CSS_REMOVED bits or abort
-			 */
-			if (atomic_cmpxchg(&css->refcnt, refcnt, 0) == refcnt)
-				break;
-			cpu_relax();
-		}
-	}
- done:
-	for_each_subsys(cgrp->root, ss) {
-		struct cgroup_subsys_state *css = cgrp->subsys[ss->subsys_id];
-		if (failed) {
-			/*
-			 * Restore old refcnt if we previously managed
-			 * to clear it from 1 to 0
-			 */
-			if (!atomic_read(&css->refcnt))
-				atomic_set(&css->refcnt, 1);
-		} else {
-			/* Commit the fact that the CSS is removed */
-			set_bit(CSS_REMOVED, &css->flags);
-		}
-	}
-	local_irq_restore(flags);
-	return !failed;
-}
-
 static int cgroup_rmdir(struct inode *unused_dir, struct dentry *dentry)
 {
 	struct cgroup *cgrp = dentry->d_fsdata;
 	struct dentry *d;
 	struct cgroup *parent;
-	DEFINE_WAIT(wait);
+	struct cgroup_subsys *ss;
 	struct cgroup_event *event, *tmp;
-	int ret;
 
 	/* the vfs holds both inode->i_mutex already */
-again:
 	mutex_lock(&cgroup_mutex);
 	if (atomic_read(&cgrp->count) != 0) {
 		mutex_unlock(&cgroup_mutex);

@@ -3941,52 +3840,34 @@ again:
 	}
 	mutex_unlock(&cgroup_mutex);
 
-	/*
-	 * In general, subsystem has no css->refcnt after pre_destroy(). But
-	 * in racy cases, subsystem may have to get css->refcnt after
-	 * pre_destroy() and it makes rmdir return with -EBUSY. This sometimes
-	 * make rmdir return -EBUSY too often. To avoid that, we use waitqueue
-	 * for cgroup's rmdir. CGRP_WAIT_ON_RMDIR is for synchronizing rmdir
-	 * and subsystem's reference count handling. Please see css_get/put
-	 * and css_tryget() and cgroup_wakeup_rmdir_waiter() implementation.
-	 */
-	set_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
-
-	/*
-	 * Call pre_destroy handlers of subsys. Notify subsystems
-	 * that rmdir() request comes.
-	 */
-	ret = cgroup_call_pre_destroy(cgrp);
-	if (ret) {
-		clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
-		return ret;
-	}
-
 	mutex_lock(&cgroup_mutex);
 	parent = cgrp->parent;
 	if (atomic_read(&cgrp->count) || !list_empty(&cgrp->children)) {
-		clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
 		mutex_unlock(&cgroup_mutex);
 		return -EBUSY;
 	}
-	prepare_to_wait(&cgroup_rmdir_waitq, &wait, TASK_INTERRUPTIBLE);
-	if (!cgroup_clear_css_refs(cgrp)) {
-		mutex_unlock(&cgroup_mutex);
-		/*
-		 * Because someone may call cgroup_wakeup_rmdir_waiter() before
-		 * prepare_to_wait(), we need to check this flag.
-		 */
-		if (test_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags))
-			schedule();
-		finish_wait(&cgroup_rmdir_waitq, &wait);
-		clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
-		if (signal_pending(current))
-			return -EINTR;
-		goto again;
+
+	/* deny new css_tryget() attempts */
+	for_each_subsys(cgrp->root, ss) {
+		struct cgroup_subsys_state *css = cgrp->subsys[ss->subsys_id];
+
+		WARN_ON(atomic_read(&css->refcnt) < 0);
+		atomic_add(CSS_DEAD_BIAS, &css->refcnt);
+	}
+
+	/*
+	 * No new tryget reference will be handed out.  Tell subsystems to
+	 * drain the existing references.
+	 */
+	for_each_subsys(cgrp->root, ss)
+		if (ss->pre_destroy)
+			ss->pre_destroy(cgrp);
+
+	for_each_subsys(cgrp->root, ss) {
+		struct cgroup_subsys_state *css = cgrp->subsys[ss->subsys_id];
+
+		css_put(css);
 	}
-	/* NO css_tryget() can success after here. */
-	finish_wait(&cgroup_rmdir_waitq, &wait);
-	clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
 
 	raw_spin_lock(&release_list_lock);
 	set_bit(CGRP_REMOVED, &cgrp->flags);

@@ -4689,21 +4570,35 @@ static void check_for_release(struct cgr
 }
 
 /* Caller must verify that the css is not for root cgroup */
-void __css_put(struct cgroup_subsys_state *css, int count)
+bool __css_tryget(struct cgroup_subsys_state *css)
+{
+	while (1) {
+		int v, t;
+
+		v = atomic_read(&css->refcnt);
+		if (unlikely(v < 0))
+			return false;
+
+		t = atomic_cmpxchg(&css->refcnt, v, v + 1);
+		if (likely(t == v))
+			return true;
+	}
+}
+
+/* Caller must verify that the css is not for root cgroup */
+void __css_put(struct cgroup_subsys_state *css)
 {
 	struct cgroup *cgrp = css->cgroup;
 	int val;
-	rcu_read_lock();
-	val = atomic_sub_return(count, &css->refcnt);
-	if (val == 1) {
+
+	val = atomic_dec_return(&css->refcnt);
+	if (val == CSS_DEAD_BIAS) {
 		if (notify_on_release(cgrp)) {
 			set_bit(CGRP_RELEASABLE, &cgrp->flags);
 			check_for_release(cgrp);
 		}
-		cgroup_wakeup_rmdir_waiter(cgrp);
+		schedule_work(&cgrp->dput_work);
 	}
-	rcu_read_unlock();
-	WARN_ON_ONCE(val < 1);
 }
 EXPORT_SYMBOL_GPL(__css_put);
 
 
--

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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

The trouble for pre_destroy() is _not_ refcount, Memory cgroup has its own refcnt and use it internally. The problem is 'charges'. It's not related to refcnt.

Cgroup is designed to exists with 'tasks'. But memory may not be related to any task...just related to a cgroup.

But ok, pre_destory() & rmdir() is complicated, I agree.

Now, we prevent rmdir() if we can't move charges to its parent. If pre_destory() shouldn't fail, I can think of some alternatives.

* move all charges to the parent and if it fails...move all charges to root cgroup.
(drop_from_memory may not work well in swapless system.)

I think.. if pre_destory() never fails, we don't need pre_destroy().

Quoted 5 lines

The last one seems more tricky. On destruction of cgroup, the charges are transferred to its parent and the parent may not have enough room for that. Greg told me that this should only be a problem for !hierarchical case. I think this can be dealt with by dumping what's left over to root cgroup with a warning message.

I don't like warning ;)

I think we can do all in 'destroy()'.

Hello, KAMEZAWA.

Quoted 2 lines · KAMEZAWA Hiroyuki

The trouble for pre_destroy() is _not_ refcount, Memory cgroup has its own refcnt and use it internally. The problem is 'charges'. It's not related to refcnt.

Hmmm.... yeah, I'm not familiar with memcg internals at all. For blkcg, refcnt matters but if it doesn't for memcg, great.

Quoted 8 lines

Cgroup is designed to exists with 'tasks'. But memory may not be related to any task...just related to a cgroup.

But ok, pre_destory() & rmdir() is complicated, I agree.

Now, we prevent rmdir() if we can't move charges to its parent. If pre_destory() shouldn't fail, I can think of some alternatives.

Quoted 3 lines

* move all charges to the parent and if it fails...move all charges to root cgroup.
(drop_from_memory may not work well in swapless system.)

I think this one is better and this shouldn't fail if hierarchical mode is in use, right?

Quoted 1 line

I think.. if pre_destory() never fails, we don't need pre_destroy().

For memcg maybe, blkcg still needs it.

Quoted 6 lines

Quoted 5 lines

The last one seems more tricky. On destruction of cgroup, the charges are transferred to its parent and the parent may not have enough room for that. Greg told me that this should only be a problem for !hierarchical case. I think this can be dealt with by dumping what's left over to root cgroup with a warning message.

Quoted 1 line

I don't like warning ;)

I agree this isn't perfect but then again failing rmdir isn't perfect either and given that the condition can be wholly avoided in hierarchical mode, which should be the default anyway (is there any reason to keep flat mode except for backward compatibility?), I don't think the trade off is too bad.

Quoted 5 lines

I agree this isn't perfect but then again failing rmdir isn't perfect either and given that the condition can be wholly avoided in hierarchical mode, which should be the default anyway (is there any reason to keep flat mode except for backward compatibility?), I don't think the trade off is too bad.

One reason is 'performance'. You can see performance trouble when you creates deep tree of memcgs in hierarchy mode. The deeper memcg tree, the more res_coutners will be shared.

For example, libvirt creates cgroup tree as

/cgroup/memory/libvirt/qemu/GuestXXX/....
/cgroup/memory/libvirt/lxc/GuestXXX/...

No one don't want to count up 4 res_coutner, which is very very heavy, for handling independent workloads of "Guest".

IIUC, in general, even in the processes are in a tree, in major case of servers, their workloads are independent.
I think FLAT mode is the dafault. 'heararchical' is a crazy thing which cannot be managed.

Quoted 5 lines

I agree this isn't perfect but then again failing rmdir isn't perfect either and given that the condition can be wholly avoided in hierarchical mode, which should be the default anyway (is there any reason to keep flat mode except for backward compatibility?), I don't think the trade off is too bad.

One reason is 'performance'. You can see performance trouble when you creates deep tree of memcgs in hierarchy mode. The deeper memcg tree, the more res_coutners will be shared.

For example, libvirt creates cgroup tree as

/cgroup/memory/libvirt/qemu/GuestXXX/....
/cgroup/memory/libvirt/lxc/GuestXXX/...

No one don't want to count up 4 res_coutner, which is very very heavy, for handling independent workloads of "Guest".

IIUC, in general, even in the processes are in a tree, in major case of servers, their workloads are independent.
I think FLAT mode is the dafault. 'heararchical' is a crazy thing which cannot be managed.

IIUC, in general, even in the processes are in a tree, in major case of servers, their workloads are independent.
I think FLAT mode is the dafault. 'heararchical' is a crazy thing which cannot be managed.

IIUC, in general, even in the processes are in a tree, in major case of servers, their workloads are independent.
I think FLAT mode is the dafault. 'heararchical' is a crazy thing which cannot be managed.

Quoted 4 lines · KAMEZAWA Hiroyuki

IIUC, in general, even in the processes are in a tree, in major case of servers, their workloads are independent.
I think FLAT mode is the dafault. 'heararchical' is a crazy thing which cannot be managed.

Better pay attention to the current overall cgroups discussions being held by Tejun then. ([RFD] cgroup: about multiple hierarchies)

The topic of whether of adapting all cgroups to be hierarchical by deafult is a recurring one.

I personally think that it is not unachievable to make res_counters cheaper, therefore making this less of a problem.

Quoted 6 lines · Glauber Costa

Quoted 4 lines · KAMEZAWA Hiroyuki

IIUC, in general, even in the processes are in a tree, in major case of servers, their workloads are independent.
I think FLAT mode is the dafault. 'heararchical' is a crazy thing which cannot be managed.

Quoted 9 lines

Better pay attention to the current overall cgroups discussions being held by Tejun then. ([RFD] cgroup: about multiple hierarchies)

The topic of whether of adapting all cgroups to be hierarchical by deafult is a recurring one.

I personally think that it is not unachievable to make res_counters cheaper, therefore making this less of a problem.

I thought of this a little yesterday. Current my idea is applying following rule for res_counter.

1. All res_counter is hierarchical. But behavior should be optimized.

2. If parent res_counter has UNLIMITED limit, 'usage' will not be propagated to its parent at _charge_.

3. If a res_counter has UNLIMITED limit, at reading usage, it must visit all children and returns a sum of them.

Then,
/cgroup/
memory/ (unlimited)
libivirt/ (unlimited)
qeumu/ (unlimited)
guest/(limited)

All dir can show hierarchical usage and the guest will not have any lock contention at runtime.

By this
1. no runtime overhead if the parent has unlimited limit.
2. All res_counter can show aggregate resource usage of children.

To do this
1. res_coutner should have children list by itself.

Implementation problem
- What should happens when a user set new limit to a res_counter which have childrens ? Shouldn't we allow it ? Or take all locks of children and update in atomic ?

- memory.use_hierarchy should be obsolete ?

Quoted 22 lines · KAMEZAWA Hiroyuki

Quoted 6 lines · Glauber Costa

Quoted 4 lines · KAMEZAWA Hiroyuki

IIUC, in general, even in the processes are in a tree, in major case of servers, their workloads are independent.
I think FLAT mode is the dafault. 'heararchical' is a crazy thing which cannot be managed.

Quoted 9 lines

Better pay attention to the current overall cgroups discussions being held by Tejun then. ([RFD] cgroup: about multiple hierarchies)

The topic of whether of adapting all cgroups to be hierarchical by deafult is a recurring one.

I personally think that it is not unachievable to make res_counters cheaper, therefore making this less of a problem.

I thought of this a little yesterday. Current my idea is applying following rule for res_counter.

Quoted 4 lines

1. All res_counter is hierarchical. But behavior should be optimized.

2. If parent res_counter has UNLIMITED limit, 'usage' will not be propagated to its parent at _charge_.

That doesn't seem to make much sense. If you are unlimited, but your parent is limited,
he has a lot more interest to know about the charge than you do.

So the
logic should rather be the opposite: Don't go around getting locks and all that if you are unlimited. Your parent might, though.

I am trying to experiment a bit with billing to percpu counters for unlimited res_counters. But their inexact nature is giving me quite a headache.

Quoted 10 lines

3. If a res_counter has UNLIMITED limit, at reading usage, it must visit all children and returns a sum of them.

Then,
/cgroup/
memory/ (unlimited)
libivirt/ (unlimited)
qeumu/ (unlimited)
guest/(limited)

Quoted 2 lines

All dir can show hierarchical usage and the guest will not have any lock contention at runtime.

If we are okay with summing it up at read time, we may as well keep everything in percpu counters at all times.

Quoted 11 lines

By this
1. no runtime overhead if the parent has unlimited limit.
2. All res_counter can show aggregate resource usage of children.

To do this
1. res_coutner should have children list by itself.

Implementation problem
- What should happens when a user set new limit to a res_counter which have childrens ? Shouldn't we allow it ? Or take all locks of children and update in atomic ?

Well, increasing the limit should be always possible.

As for the kids, how about:

- ) Take their locks
- ) scan through them seeing if their usage is bellow the new allowance -) if it is, then ok
-) if it is not, then try to reclaim (*). Fail if it is not possible.

(*) May be hard to implement, because we already have the res_counter lock taken, and the code may get nasty. So maybe it is better just fail if any of your kids usage is over the new allowance...

Quoted 3 lines

- memory.use_hierarchy should be obsolete ?

If we're going fully hierarchical, yes.

Quoted 5 lines

Quoted 5 lines

I thought of this a little yesterday. Current my idea is applying following rule for res_counter.

Quoted 8 lines

Quoted 4 lines

1. All res_counter is hierarchical. But behavior should be optimized.

2. If parent res_counter has UNLIMITED limit, 'usage' will not be propagated to its parent at _charge_.

That doesn't seem to make much sense. If you are unlimited, but your parent is limited,
he has a lot more interest to know about the charge than you do.

Sorry, I should write "If all ancestors are umlimited'. If parent is limited, the children should be treated as limited.

Quoted 8 lines

So the
logic should rather be the opposite: Don't go around getting locks and all that if you are unlimited. Your parent might, though.

I am trying to experiment a bit with billing to percpu counters for unlimited res_counters. But their inexact nature is giving me quite a headache.

Personally, I think percpu counter is not the best one. Yes, it will work but... Because of its nature of error range, it has scalability problem. Considering to have a tree like

	/A/B/Guest0/tasks
              Guest1/tasks
              Guest2/tasks
              Guest4/tasks
              Guest5/tasks
              ......

percpu res_counter may work scarable in GuestX level but will conflict in level B. And I don't want to think what happens in 256 cpu system. Error in B will be very big.

Another idea is to borrow a resource from memcg to the tasks. i.e.having per-task caching of charges. But it has two problems that draining unused resource is difficult and precise usage is unknown.

IMHO, hard-limited resource counter itself may be a problem ;)

So, an idea, 'if all ancestors are unlimited, don't propagate charges.' comes to my mind. With this, people use resource in FLAT (but has hierarchical cgroup tree) will not see any performance problem.

Quoted 13 lines

Quoted 12 lines

3. If a res_counter has UNLIMITED limit, at reading usage, it must visit all children and returns a sum of them.

Then,
/cgroup/
memory/ (unlimited)
libivirt/ (unlimited)
qeumu/ (unlimited)
guest/(limited)

All dir can show hierarchical usage and the guest will not have any lock contention at runtime.

Quoted 3 lines

If we are okay with summing it up at read time, we may as well keep everything in percpu counters at all times.

If all ancestors are unlimited, we don't need to propagate usage upwards at charging. If one of ancestors are limited, we need to propagate and check usage at charging.

Quoted 6 lines

Quoted 4 lines

- memory.use_hierarchy should be obsolete ?

If we're going fully hierarchical, yes.

Another big problem is 'when' we should do this change.. Maybe this 'hierarchical' problem will be good topic in MM summit.

Quoted 5 lines

Quoted 5 lines

I thought of this a little yesterday. Current my idea is applying following rule for res_counter.

Quoted 8 lines

Quoted 4 lines

1. All res_counter is hierarchical. But behavior should be optimized.

2. If parent res_counter has UNLIMITED limit, 'usage' will not be propagated to its parent at _charge_.

That doesn't seem to make much sense. If you are unlimited, but your parent is limited,
he has a lot more interest to know about the charge than you do.

Sorry, I should write "If all ancestors are umlimited'. If parent is limited, the children should be treated as limited.

Quoted 8 lines

So the
logic should rather be the opposite: Don't go around getting locks and all that if you are unlimited. Your parent might, though.

I am trying to experiment a bit with billing to percpu counters for unlimited res_counters. But their inexact nature is giving me quite a headache.

Personally, I think percpu counter is not the best one. Yes, it will work but... Because of its nature of error range, it has scalability problem. Considering to have a tree like

	/A/B/Guest0/tasks
              Guest1/tasks
              Guest2/tasks
              Guest4/tasks
              Guest5/tasks
              ......

percpu res_counter may work scarable in GuestX level but will conflict in level B. And I don't want to think what happens in 256 cpu system. Error in B will be very big.

Another idea is to borrow a resource from memcg to the tasks. i.e.having per-task caching of charges. But it has two problems that draining unused resource is difficult and precise usage is unknown.

IMHO, hard-limited resource counter itself may be a problem ;)

So, an idea, 'if all ancestors are unlimited, don't propagate charges.' comes to my mind. With this, people use resource in FLAT (but has hierarchical cgroup tree) will not see any performance problem.

Quoted 13 lines

Quoted 12 lines

3. If a res_counter has UNLIMITED limit, at reading usage, it must visit all children and returns a sum of them.

Then,
/cgroup/
memory/ (unlimited)
libivirt/ (unlimited)
qeumu/ (unlimited)
guest/(limited)

All dir can show hierarchical usage and the guest will not have any lock contention at runtime.

Quoted 3 lines

If we are okay with summing it up at read time, we may as well keep everything in percpu counters at all times.

If all ancestors are unlimited, we don't need to propagate usage upwards at charging. If one of ancestors are limited, we need to propagate and check usage at charging.

Quoted 6 lines

Quoted 4 lines

- memory.use_hierarchy should be obsolete ?

If we're going fully hierarchical, yes.

Another big problem is 'when' we should do this change.. Maybe this 'hierarchical' problem will be good topic in MM summit.

Quoted 4 lines · KAMEZAWA Hiroyuki

IIUC, in general, even in the processes are in a tree, in major case of servers, their workloads are independent.
I think FLAT mode is the dafault. 'heararchical' is a crazy thing which cannot be managed.

Better pay attention to the current overall cgroups discussions being held by Tejun then. ([RFD] cgroup: about multiple hierarchies)

The topic of whether of adapting all cgroups to be hierarchical by deafult is a recurring one.

I personally think that it is not unachievable to make res_counters cheaper, therefore making this less of a problem.

Hello, KAMEZAWA.

Quoted 2 lines · KAMEZAWA Hiroyuki

The trouble for pre_destroy() is _not_ refcount, Memory cgroup has its own refcnt and use it internally. The problem is 'charges'. It's not related to refcnt.

Hmmm.... yeah, I'm not familiar with memcg internals at all. For blkcg, refcnt matters but if it doesn't for memcg, great.

Quoted 8 lines

Cgroup is designed to exists with 'tasks'. But memory may not be related to any task...just related to a cgroup.

But ok, pre_destory() & rmdir() is complicated, I agree.

Now, we prevent rmdir() if we can't move charges to its parent. If pre_destory() shouldn't fail, I can think of some alternatives.

Quoted 3 lines

* move all charges to the parent and if it fails...move all charges to root cgroup.
(drop_from_memory may not work well in swapless system.)

I think this one is better and this shouldn't fail if hierarchical mode is in use, right?

Quoted 1 line

I think.. if pre_destory() never fails, we don't need pre_destroy().

For memcg maybe, blkcg still needs it.

Quoted 6 lines

Quoted 5 lines

The last one seems more tricky. On destruction of cgroup, the charges are transferred to its parent and the parent may not have enough room for that. Greg told me that this should only be a problem for !hierarchical case. I think this can be dealt with by dumping what's left over to root cgroup with a warning message.

Quoted 1 line

I don't like warning ;)

I agree this isn't perfect but then again failing rmdir isn't perfect either and given that the condition can be wholly avoided in hierarchical mode, which should be the default anyway (is there any reason to keep flat mode except for backward compatibility?), I don't think the trade off is too bad.

The trouble for pre_destroy() is _not_ refcount, Memory cgroup has its own refcnt and use it internally. The problem is 'charges'. It's not related to refcnt.

Cgroup is designed to exists with 'tasks'. But memory may not be related to any task...just related to a cgroup.

But ok, pre_destory() & rmdir() is complicated, I agree.

Now, we prevent rmdir() if we can't move charges to its parent. If pre_destory() shouldn't fail, I can think of some alternatives.

* move all charges to the parent and if it fails...move all charges to root cgroup.
(drop_from_memory may not work well in swapless system.)

I think.. if pre_destory() never fails, we don't need pre_destroy().

Quoted 5 lines

The last one seems more tricky. On destruction of cgroup, the charges are transferred to its parent and the parent may not have enough room for that. Greg told me that this should only be a problem for !hierarchical case. I think this can be dealt with by dumping what's left over to root cgroup with a warning message.

I don't like warning ;)

I think we can do all in 'destroy()'.

Hello, guys.

While working on blkcg, I learned that cgroup removal path tries to drain all internal references synchronously before proceeding with removal, and may be aborted by by pre_destroy() failing.

I find both quite unusual. While there are some occassions where we try to drain reference counts synchronously, the norm is deactivating the target and then releasing it when the reference count hits zero and exposing this synchronous behavior directly to userland makes it worse. This also requires allowing rmdir to be aborted from userland.

pre_destroy() being allowed to veto rmdir might be okay if there's only one subsystem using it or it implemented proper prepare-commit/cancel transaction, but as it currently stands, pre_destroy() operations are not reversible and even within memcg itself the state wouldn't be consistent after failure (some moved to parent while the rest on child). Note that abort from userland also has the same problem.

It also complicates and adds even more subtleties to cgroup code. A lot of it was just me being dumb but making sense of cgroup_exclude_rmdir() and cgroup_release_and_wakeup_rmdir() usages in memcontrol took me quite some time even with Hugh's help.

In general, IMHO, it's a bad idea to expose purely internal implementation details to userland directly. Internal ref counts can be kept around for whatever reason (e.g. blkcg does it for lookup caching), such details shouldn't be visible to userland. Midlayers like cgroup which sit between userland and mechanism implementations should provide isolation between the two so that each mechanism implementation doesn't have to worry about things like that.

It seems cgroup is going through a lot of the same growing pains that sysfs went through years ago and would probably benefit from using sysfs for userland interfacing rather than trying to replicate features that sysfs already provides. Well, that's another long term thing, I guess. For now, I'd like to make cgroup rmdir path more conventional so that rmdir behaves like the following.

If you want to spend your time doing archaeology there are some old threads that touch on this idea (roughly around 2003-2005). One point against the idea that I distinctly recall:

Somewhat like configfs, object lifetimes in cgroups are determined primarily by the user whereas sysfs object lifetimes are primarily determined by the kernel. I think the closest we come to user-determined objects in sysfs occur through debugfs, and module loading/unloading. However those involve mount/umount and modprobe/rmmod rather than mkdir/rmdir to create and remove the objects.

Hey, Matt.

Quoted 10 lines · Matt Helsley

If you want to spend your time doing archaeology there are some old threads that touch on this idea (roughly around 2003-2005). One point against the idea that I distinctly recall:

Somewhat like configfs, object lifetimes in cgroups are determined primarily by the user whereas sysfs object lifetimes are primarily determined by the kernel. I think the closest we come to user-determined objects in sysfs occur through debugfs, and module loading/unloading. However those involve mount/umount and modprobe/rmmod rather than mkdir/rmdir to create and remove the objects.

The thing is that sysfs itself has been almost completely rewritten since that time to 1. decouple internal representation from vfs objects and 2. provide proper isolation between the userland and kernel code exposing data through sysfs.

#1 began mostly due to the large size of dentries and inodes but, with the benefit of hindsight, I think it just was a bad idea to piggyback on vfs objects for object life-cycle management and locking for stuff which is wholely described in memory with simplistic locking.

#2 was necessary to avoid hanging device detach due to open sysfs file from userland. sysfs now has notion of "active access" encompassing only each show/store op invocation and it only guarantees that the associated device doesn't go away while active accesses are in progress.

The sysfs heritage is almost recognizable and unfortunately almost the same set of problems (nobody wants show/store ops to be called on unlinked css waiting for references to be drained). As refactoring and sharing sysfs won't be a trivial task, my plan is to first augment cgroupfs as necessary with longer term goal of converging and later sharing the same code with sysfs.

If you want to spend your time doing archaeology there are some old threads that touch on this idea (roughly around 2003-2005). One point against the idea that I distinctly recall:

Somewhat like configfs, object lifetimes in cgroups are determined primarily by the user whereas sysfs object lifetimes are primarily determined by the kernel. I think the closest we come to user-determined objects in sysfs occur through debugfs, and module loading/unloading. However those involve mount/umount and modprobe/rmmod rather than mkdir/rmdir to create and remove the objects.

Hello, guys.

While working on blkcg, I learned that cgroup removal path tries to drain all internal references synchronously before proceeding with removal, and may be aborted by by pre_destroy() failing.

I find both quite unusual. While there are some occassions where we try to drain reference counts synchronously, the norm is deactivating the target and then releasing it when the reference count hits zero and exposing this synchronous behavior directly to userland makes it worse. This also requires allowing rmdir to be aborted from userland.

pre_destroy() being allowed to veto rmdir might be okay if there's only one subsystem using it or it implemented proper prepare-commit/cancel transaction, but as it currently stands, pre_destroy() operations are not reversible and even within memcg itself the state wouldn't be consistent after failure (some moved to parent while the rest on child). Note that abort from userland also has the same problem.

It also complicates and adds even more subtleties to cgroup code. A lot of it was just me being dumb but making sense of cgroup_exclude_rmdir() and cgroup_release_and_wakeup_rmdir() usages in memcontrol took me quite some time even with Hugh's help.

In general, IMHO, it's a bad idea to expose purely internal implementation details to userland directly. Internal ref counts can be kept around for whatever reason (e.g. blkcg does it for lookup caching), such details shouldn't be visible to userland. Midlayers like cgroup which sit between userland and mechanism implementations should provide isolation between the two so that each mechanism implementation doesn't have to worry about things like that.

It seems cgroup is going through a lot of the same growing pains that sysfs went through years ago and would probably benefit from using sysfs for userland interfacing rather than trying to replicate features that sysfs already provides. Well, that's another long term thing, I guess. For now, I'd like to make cgroup rmdir path more conventional so that rmdir behaves like the following.