netprio_cgroup: implement hierarchy support

With the introduction of generic cgroup hierarchy iterators, css_id is being phased out. It was unnecessarily complex, id'ing the wrong thing (cgroups need IDs, not CSSes) and has other oddities like not being available at ->css_alloc().

This patch adds cgroup->id, which is a simple per-hierarchy ida-allocated ID which is assigned before ->css_alloc() and released after ->css_free().

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

Quoted 5 lines · Tejun Heo

With the introduction of generic cgroup hierarchy iterators, css_id is being phased out. It was unnecessarily complex, id'ing the wrong thing (cgroups need IDs, not CSSes) and has other oddities like not being available at ->css_alloc().

Quoted 5 lines

This patch adds cgroup->id, which is a simple per-hierarchy ida-allocated ID which is assigned before ->css_alloc() and released after ->css_free().

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

Acked-by Li Zefan <[email protected]>

With the introduction of generic cgroup hierarchy iterators, css_id is being phased out. It was unnecessarily complex, id'ing the wrong thing (cgroups need IDs, not CSSes) and has other oddities like not being available at ->css_alloc().

This patch adds cgroup->id, which is a simple per-hierarchy ida-allocated ID which is assigned before ->css_alloc() and released after ->css_free().

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

With the introduction of generic cgroup hierarchy iterators, css_id is being phased out. It was unnecessarily complex, id'ing the wrong thing (cgroups need IDs, not CSSes) and has other oddities like not being available at ->css_alloc().

This patch adds cgroup->id, which is a simple per-hierarchy ida-allocated ID which is assigned before ->css_alloc() and released after ->css_free().

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

---
  include/linux/cgroup.h |  2 ++
  kernel/cgroup.c        | 15 ++++++++++++++-
  2 files changed, 16 insertions(+), 1 deletion(-)

include/linux/cgroup.h +2 -0

index d5fc8a7..b512469 100644
@@ -159,6 +159,8 @@ struct cgroup {
  	 */
  	atomic_t count;
 
+	int id;				/* ida allocated in-hierarchy ID */
+
  	/*
  	 * We link our 'sibling' struct into our parent's 'children'.
  	 * Our children link their 'sibling' into our 'children'.

kernel/cgroup.c +14 -1

index 99d5e69..0b25dcb 100644
@@ -138,6 +138,9 @@ struct cgroupfs_root {
  	/* Hierarchy-specific flags */
  	unsigned long flags;
 
+	/* IDs for cgroups in this hierarchy */
+	struct ida cgroup_ida;
+
  	/* The path to use for release notifications. */
  	char release_agent_path[PATH_MAX];
 

@@ -890,6 +893,7 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode)
 
  		simple_xattrs_free(&cgrp->xattrs);
 
+		ida_simple_remove(&cgrp->root->cgroup_ida, cgrp->id);
  		kfree(cgrp);
  	} else {
  		struct cfent *cfe = __d_cfe(dentry);

@@ -1465,6 +1469,7 @@ static struct cgroupfs_root *cgroup_root_from_opts(struct cgroup_sb_opts *opts)
 
  	root->subsys_mask = opts->subsys_mask;
  	root->flags = opts->flags;
+	ida_init(&root->cgroup_ida);
  	if (opts->release_agent)
  		strcpy(root->release_agent_path, opts->release_agent);
  	if (opts->name)

@@ -1483,6 +1488,7 @@ static void cgroup_drop_root(struct cgroupfs_root *root)
  	spin_lock(&hierarchy_id_lock);
  	ida_remove(&hierarchy_ida, root->hierarchy_id);
  	spin_unlock(&hierarchy_id_lock);
+	ida_destroy(&root->cgroup_ida);
  	kfree(root);
  }
 

@@ -4093,10 +4099,15 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
  	struct cgroup_subsys *ss;
  	struct super_block *sb = root->sb;
 
+	/* allocate the cgroup and its ID, 0 is reserved for the root */
  	cgrp = kzalloc(sizeof(*cgrp), GFP_KERNEL);
  	if (!cgrp)
  		return -ENOMEM;
 
+	cgrp->id = ida_simple_get(&root->cgroup_ida, 1, 0, GFP_KERNEL);
+	if (cgrp->id < 0)
+		goto err_free_cgrp;
+
  	/*
  	 * Only live parents can have children.  Note that the liveliness
  	 * check isn't strictly necessary because cgroup_mkdir() and

@@ -4106,7 +4117,7 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
  	 */
  	if (!cgroup_lock_live_group(parent)) {
  		err = -ENODEV;
-		goto err_free_cgrp;
+		goto err_free_id;
  	}
 
  	/* Grab a reference on the superblock so the hierarchy doesn't

@@ -4198,6 +4209,8 @@ err_free_all:
  	mutex_unlock(&cgroup_mutex);
  	/* Release the reference count that we took on the superblock */
  	deactivate_super(sb);
+err_free_id:
+	ida_simple_remove(&root->cgroup_ida, cgrp->id);
  err_free_cgrp:
  	kfree(cgrp);
  	return err;

I'm sorry if I misunderstand ... current usage of css-id in memory/swap cgroup is for recording information of memory cgroup which may be destroyed. In some case, a memcg's cgroup is freed but a struct memcgroup and its css are available, swap_cgroup may contain id ot if.
This patch puts cgroup's id at diput(), so, the id used in swap_cgroup can be reused while it's in use. Right ?

Hello, Kamezawa.

Quoted 6 lines · Kamezawa Hiroyuki

I'm sorry if I misunderstand ... current usage of css-id in memory/swap cgroup is for recording information of memory cgroup which may be destroyed. In some case, a memcg's cgroup is freed but a struct memcgroup and its css are available, swap_cgroup may contain id ot if.
This patch puts cgroup's id at diput(), so, the id used in swap_cgroup can be reused while it's in use. Right ?

CSSes hold onto cgroups, so if memcg is around, its cgroup doesn't go away, so the right thing to do would be holding onto CSS whlie there are remaining references, which IMHO is the way it should have been implemented from the beginning. The only reason memcg currently has its own refcnt nested inside css refcnt is because cgroup used to require css refs to be completely drained for cgroup_rmdir() to proceed. Now that that weirdity is gone, we should go back to sane css based reference counting, right?

void __css_put(struct cgroup_subsys_state *css)

{
        struct cgroup *cgrp = css->cgroup;
        int v;

        rcu_read_lock();
        v = css_unbias_refcnt(atomic_dec_return(&css->refcnt));

        switch (v) {
        case 1:
                if (notify_on_release(cgrp)) {
                        set_bit(CGRP_RELEASABLE, &cgrp->flags);
                        check_for_release(cgrp);

                }
                break;
        case 0:
                schedule_work(&css->dput_work);
                break;
        }
        rcu_read_unlock();
}

BTW, css's ID was limited to 65535 to be encoded in 2bytes. If we use INT, this will increase size of swap_cgroup. (2bytes per page => 4bytes per page) It's preallocated at swapon() because allocating memory dynamically when we swap a memory is not good.

Do we really need 4bytes for ID ? If so, swap_cgroup should be totally re-designed.

Hello, Kamezawa.

Quoted 5 lines · Kamezawa Hiroyuki

BTW, css's ID was limited to 65535 to be encoded in 2bytes. If we use INT, this will increase size of swap_cgroup. (2bytes per page => 4bytes per page) It's preallocated at swapon() because allocating memory dynamically when we swap a memory is not good.

Quoted 1 line

Do we really need 4bytes for ID ? If so, swap_cgroup should be totally re-designed.

That's a memcg restriction which shouldn't have been imposed on cgroup core from the beginning. What memcg should do is rejecting to become online from ->css_onilne() if cgrp->id is out of the range it can handle.

BTW, css's ID was limited to 65535 to be encoded in 2bytes. If we use INT, this will increase size of swap_cgroup. (2bytes per page => 4bytes per page) It's preallocated at swapon() because allocating memory dynamically when we swap a memory is not good.

Do we really need 4bytes for ID ? If so, swap_cgroup should be totally re-designed.

Quoted 14 lines · Tejun Heo

Hello, Kamezawa.

Quoted 10 lines · Kamezawa Hiroyuki

I'm sorry if I misunderstand ... current usage of css-id in memory/swap cgroup
is for recording information of memory cgroup which may be destroyed. In some case,
a memcg's cgroup is freed but a struct memcgroup and its css are available, swap_cgroup
may contain id ot if.
This patch puts cgroup's id at diput(), so, the id used in swap_cgroup can be
reused while it's in use. Right ?

Quoted 9 lines

CSSes hold onto cgroups, so if memcg is around, its cgroup doesn't go away, so the right thing to do would be holding onto CSS whlie there are remaining references, which IMHO is the way it should have been implemented from the beginning. The only reason memcg currently has its own refcnt nested inside css refcnt is because cgroup used to require css refs to be completely drained for cgroup_rmdir() to proceed. Now that that weirdity is gone, we should go back to sane css based reference counting, right?

Ah, hm, Maybe I missed new __css_put() implementation...

Quoted 13 lines

void __css_put(struct cgroup_subsys_state *css)

{
        struct cgroup *cgrp = css->cgroup;
        int v;

        rcu_read_lock();
        v = css_unbias_refcnt(atomic_dec_return(&css->refcnt));

        switch (v) {
        case 1:
                if (notify_on_release(cgrp)) {
                        set_bit(CGRP_RELEASABLE, &cgrp->flags);
                        check_for_release(cgrp);

Quoted 8 lines

                }
                break;
        case 0:
                schedule_work(&css->dput_work);
                break;
        }
        rcu_read_unlock();
}

If swap_cgroup holds css's refcnt instead of memcg's.... final dput will be invoked when the last swap_cgroup release a reference.

It seems to work and we can drop memcg's refcnt (maybe).

BTW, css's ID was limited to 65535 to be encoded in 2bytes. If we use INT, this will increase size of swap_cgroup. (2bytes per page => 4bytes per page) It's preallocated at swapon() because allocating memory dynamically when we swap a memory is not good.

Do we really need 4bytes for ID ? If so, swap_cgroup should be totally re-designed.

Quoted 14 lines · Tejun Heo

Hello, Kamezawa.

Quoted 10 lines · Kamezawa Hiroyuki

I'm sorry if I misunderstand ... current usage of css-id in memory/swap cgroup
is for recording information of memory cgroup which may be destroyed. In some case,
a memcg's cgroup is freed but a struct memcgroup and its css are available, swap_cgroup
may contain id ot if.
This patch puts cgroup's id at diput(), so, the id used in swap_cgroup can be
reused while it's in use. Right ?

Quoted 9 lines

CSSes hold onto cgroups, so if memcg is around, its cgroup doesn't go away, so the right thing to do would be holding onto CSS whlie there are remaining references, which IMHO is the way it should have been implemented from the beginning. The only reason memcg currently has its own refcnt nested inside css refcnt is because cgroup used to require css refs to be completely drained for cgroup_rmdir() to proceed. Now that that weirdity is gone, we should go back to sane css based reference counting, right?

Ah, hm, Maybe I missed new __css_put() implementation...

Quoted 13 lines

void __css_put(struct cgroup_subsys_state *css)

{
        struct cgroup *cgrp = css->cgroup;
        int v;

        rcu_read_lock();
        v = css_unbias_refcnt(atomic_dec_return(&css->refcnt));

        switch (v) {
        case 1:
                if (notify_on_release(cgrp)) {
                        set_bit(CGRP_RELEASABLE, &cgrp->flags);
                        check_for_release(cgrp);

Quoted 8 lines

                }
                break;
        case 0:
                schedule_work(&css->dput_work);
                break;
        }
        rcu_read_unlock();
}

If swap_cgroup holds css's refcnt instead of memcg's.... final dput will be invoked when the last swap_cgroup release a reference.

It seems to work and we can drop memcg's refcnt (maybe).

BTW, css's ID was limited to 65535 to be encoded in 2bytes. If we use INT, this will increase size of swap_cgroup. (2bytes per page => 4bytes per page) It's preallocated at swapon() because allocating memory dynamically when we swap a memory is not good.

Do we really need 4bytes for ID ? If so, swap_cgroup should be totally re-designed.

I'm sorry if I misunderstand ... current usage of css-id in memory/swap cgroup is for recording information of memory cgroup which may be destroyed. In some case, a memcg's cgroup is freed but a struct memcgroup and its css are available, swap_cgroup may contain id ot if.
This patch puts cgroup's id at diput(), so, the id used in swap_cgroup can be reused while it's in use. Right ?

Hello, guys.

This patchset implements hierarchy support for netprio_cgroup. netprio_cgroup along with netcls_cgroup is a rather weird in that it really isn't about resource control. It just hitches on cgroup as a convenient mechanism to do stuff to groups of tasks. The hierarchy support reflects such nature. There's no limit being imposed from ancestors. It simply propagates configuration downwards until there's a node with its own config. IOW, any given cgroup inherits priorities from its parent for all netdevs which it doesn't have its own config for.

As a parent isn't affected by child inheriting its config, the hierarchy implementation is pretty simple. It's enough to inherit config from ->css_online() and propagate new config downwards from write_priomap(). As each node needs to know which config is its local one and which is inherited, an extra config array is added - netprio_map->aux[]. It's a separate array to avoid disturbing spatial locality of ->priomap[]. While it currently contains single one-bit flag, I still made it a struct so that adding more configuration (e.g. max_prio) is easy.

Note that this does change userland-visible behavior. Now, nesting is allowed and cgroups at the first level inherit priorities from the root cgroup. I can't think of any better way than just biting the bullet here. :(

0001-cgroup-add-cgroup-id.patch 0002-netprio-simplify-write_priomap.patch 0003-netprio_cgroup-shorten-variable-names-in-extend_netd.patch 0004-netprio_cgroup-reimplement-priomap-expansion.patch 0005-netprio_cgroup-use-cgroup-id-instead-of-cgroup_netpr.patch 0006-netprio_cgroup-implement-netprio-_set-_prio-helpers.patch 0007-netprio_cgroup-keep-track-of-whether-prio-is-set-or-.patch 0008-netprio_cgroup-implement-hierarchy-support.patch

0001 adds cgroup->id. This will eventually replace css_id.

0002-0006 are prep patches.

0007 implements is_local flag which tracks whether a cgroup has its own config or should inherit from its parent.

0008 implements hierarchy support.

This patchset is on top of

cgroup/for-3.8 ef9fe980c6 ("cgroup_freezer: implement proper hierarchy support")
+ [1] "[PATCHSET cgroup/for-3.8] cgroup: allow ->post_create() to fail"
+ [2] "[PATCH 1/2] cgroup: s/CGRP_CLONE_CHILDREN/CGRP_CPUSET_CLONE_CHILDREN/"

      "[PATCH 2/2] cgroup, cpuset: remove cgroup_subsys->post_clone()"

and available in the following git branch.

git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-netprio_cgroup-hierarchy

diffstat follows.

 Documentation/cgroups/net_prio.txt |   21 +-
 include/linux/cgroup.h             |    2
 include/net/netprio_cgroup.h       |   21 +-
 kernel/cgroup.c                    |   15 +
 net/core/netprio_cgroup.c          |  376 +++++++++++++++++++++++--------------
 5 files changed, 284 insertions(+), 151 deletions(-)

Thanks.

--
tejun

[1] http://thread.gmane.org/gmane.linux.kernel.cgroups/5047
[2] http://thread.gmane.org/gmane.linux.kernel/1393151

Hi Tejun,

I have tested this series and it works nicely. I didn't mentioned yet to review it completely. I will do it tomorrow.

Hi Tejun,

I have tested this series and it works nicely. I didn't mentioned yet to review it completely. I will do it tomorrow.