Re: [PATCH 1/5] cgroup: change allocation of css ID placement

[KAMEZAWA Hiroyuki]

Thank you for review.

On Thu, 9 Sep 2010 09:32:32 -0700
Greg Thelen <gthelen@xxxxxxxxxx> wrote:

> On Tue, Aug 31, 2010 at 11:41 PM, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@xxxxxxxxxxxxxx> wrote:
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> >
> > Now, css'id is allocated after ->create() is called. But to make use of ID
> > in ->create(), it should be available before ->create().
> >
> > In another thinking, considering the ID is tightly coupled with "css",
> > it should be allocated when "css" is allocated.
> > This patch moves alloc_css_id() to css allocation routine. Now, only 2 subsys,
> > memory and blkio are using ID. (To support complicated hierarchy walk.)
> >
> > ID will be used in mem cgroup's ->create(), later.
> >
> > This patch adds css ID documentation which is not provided.
> >
> > Note:
> > If someone changes rules of css allocation, ID allocation should be changed.
> >
> > Changelog: 2010/09/01
> > Â- modified cgroups.txt
> >
> > Reviewed-by: Daisuke Nishimura <nishimura@xxxxxxxxxxxxxxxxx>
> > Acked-by: Balbir Singh <balbir@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> > ---
> > ÂDocumentation/cgroups/cgroups.txt | Â 48 ++++++++++++++++++++++++++++++++++++
> > Âblock/blk-cgroup.c        Â|  Â9 ++++++
> > Âinclude/linux/cgroup.h      Â|  16 ++++++------
> > Âkernel/cgroup.c          |  50 +++++++++++---------------------------
> > Âmm/memcontrol.c          |  Â5 +++
> > Â5 files changed, 86 insertions(+), 42 deletions(-)
> >
> > Index: mmotm-0827/kernel/cgroup.c
> > ===================================================================
> > --- mmotm-0827.orig/kernel/cgroup.c
> > +++ mmotm-0827/kernel/cgroup.c
> > @@ -289,9 +289,6 @@ struct cg_cgroup_link {
> > Âstatic struct css_set init_css_set;
> > Âstatic struct cg_cgroup_link init_css_set_link;
> >
> > -static int cgroup_init_idr(struct cgroup_subsys *ss,
> > - Â Â Â Â Â Â Â Â Â Â Â Â Âstruct cgroup_subsys_state *css);
> > -
> > Â/* css_set_lock protects the list of css_set objects, and the
> > Â* chain of tasks off each css_set. ÂNests outside task->alloc_lock
> > Â* due to cgroup_iter_start() */
> > @@ -770,9 +767,6 @@ static struct backing_dev_info cgroup_ba
> >    Â.capabilities  = BDI_CAP_NO_ACCT_AND_WRITEBACK,
> > Â};
> >
> > -static int alloc_css_id(struct cgroup_subsys *ss,
> > - Â Â Â Â Â Â Â Â Â Â Â struct cgroup *parent, struct cgroup *child);
> > -
> > Âstatic struct inode *cgroup_new_inode(mode_t mode, struct super_block *sb)
> > Â{
> > Â Â Â Âstruct inode *inode = new_inode(sb);
> > @@ -3258,7 +3252,8 @@ static void init_cgroup_css(struct cgrou
> > Â Â Â Âcss->cgroup = cgrp;
> > Â Â Â Âatomic_set(&css->refcnt, 1);
> > Â Â Â Âcss->flags = 0;
> > - Â Â Â css->id = NULL;
> > + Â Â Â if (!ss->use_id)
> > + Â Â Â Â Â Â Â css->id = NULL;
> > Â Â Â Âif (cgrp == dummytop)
> > Â Â Â Â Â Â Â Âset_bit(CSS_ROOT, &css->flags);
> > Â Â Â ÂBUG_ON(cgrp->subsys[ss->subsys_id]);
> > @@ -3343,12 +3338,6 @@ static long cgroup_create(struct cgroup
> > Â Â Â Â Â Â Â Â Â Â Â Âgoto err_destroy;
> > Â Â Â Â Â Â Â Â}
> > Â Â Â Â Â Â Â Âinit_cgroup_css(css, ss, cgrp);
> > - Â Â Â Â Â Â Â if (ss->use_id) {
> > - Â Â Â Â Â Â Â Â Â Â Â err = alloc_css_id(ss, parent, cgrp);
> > - Â Â Â Â Â Â Â Â Â Â Â if (err)
> > - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â goto err_destroy;
> > - Â Â Â Â Â Â Â }
> > - Â Â Â Â Â Â Â /* At error, ->destroy() callback has to free assigned ID. */
> > Â Â Â Â}
> >
> > Â Â Â Âcgroup_lock_hierarchy(root);
> > @@ -3710,17 +3699,6 @@ int __init_or_module cgroup_load_subsys(
> >
> > Â Â Â Â/* our new subsystem will be attached to the dummy hierarchy. */
> > Â Â Â Âinit_cgroup_css(css, ss, dummytop);
> > - Â Â Â /* init_idr must be after init_cgroup_css because it sets css->id. */
> > - Â Â Â if (ss->use_id) {
> > - Â Â Â Â Â Â Â int ret = cgroup_init_idr(ss, css);
> > - Â Â Â Â Â Â Â if (ret) {
> > - Â Â Â Â Â Â Â Â Â Â Â dummytop->subsys[ss->subsys_id] = NULL;
> > - Â Â Â Â Â Â Â Â Â Â Â ss->destroy(ss, dummytop);
> > - Â Â Â Â Â Â Â Â Â Â Â subsys[i] = NULL;
> > - Â Â Â Â Â Â Â Â Â Â Â mutex_unlock(&cgroup_mutex);
> > - Â Â Â Â Â Â Â Â Â Â Â return ret;
> > - Â Â Â Â Â Â Â }
> > - Â Â Â }
> >
> > Â Â Â Â/*
> > Â Â Â Â * Now we need to entangle the css into the existing css_sets. unlike
> > @@ -3889,8 +3867,6 @@ int __init cgroup_init(void)
> > Â Â Â Â Â Â Â Âstruct cgroup_subsys *ss = subsys[i];
> > Â Â Â Â Â Â Â Âif (!ss->early_init)
> > Â Â Â Â Â Â Â Â Â Â Â Âcgroup_init_subsys(ss);
> > - Â Â Â Â Â Â Â if (ss->use_id)
> > - Â Â Â Â Â Â Â Â Â Â Â cgroup_init_idr(ss, init_css_set.subsys[ss->subsys_id]);
> > Â Â Â Â}
> >
> > Â Â Â Â/* Add init_css_set to the hash table */
> > @@ -4604,8 +4580,8 @@ err_out:
> >
> > Â}
> >
> > -static int __init_or_module cgroup_init_idr(struct cgroup_subsys *ss,
> > - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct cgroup_subsys_state *rootcss)
> > +static int cgroup_init_idr(struct cgroup_subsys *ss,
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â struct cgroup_subsys_state *rootcss)
> > Â{
> > Â Â Â Âstruct css_id *newid;
> >
> > @@ -4617,21 +4593,25 @@ static int __init_or_module cgroup_init_
> > Â Â Â Â Â Â Â Âreturn PTR_ERR(newid);
> >
> > Â Â Â Ânewid->stack[0] = newid->id;
> > - Â Â Â newid->css = rootcss;
> > - Â Â Â rootcss->id = newid;
> > + Â Â Â rcu_assign_pointer(newid->css, rootcss);
> > + Â Â Â rcu_assign_pointer(rootcss->id, newid);
> > Â Â Â Âreturn 0;
> > Â}
> >
> > -static int alloc_css_id(struct cgroup_subsys *ss, struct cgroup *parent,
> > - Â Â Â Â Â Â Â Â Â Â Â struct cgroup *child)
> > +int alloc_css_id(struct cgroup_subsys *ss,
> > + Â Â Â struct cgroup *cgrp, struct cgroup_subsys_state *css)
> Must also add EXPORT_SYMBOL_GPL(alloc_css_id) to supported CONFIG_BLK_CGROUP=m.

Ah, yes. 

> > Â{
> > Â Â Â Âint subsys_id, i, depth = 0;
> > - Â Â Â struct cgroup_subsys_state *parent_css, *child_css;
> > + Â Â Â struct cgroup_subsys_state *parent_css;
> > + Â Â Â struct cgroup *parent;
> > Â Â Â Âstruct css_id *child_id, *parent_id;
> >
> > + Â Â Â if (cgrp == dummytop)
> > + Â Â Â Â Â Â Â return cgroup_init_idr(ss, css);
> > +
> > + Â Â Â parent = cgrp->parent;
> > Â Â Â Âsubsys_id = ss->subsys_id;
> > Â Â Â Âparent_css = parent->subsys[subsys_id];
> > - Â Â Â child_css = child->subsys[subsys_id];
> > Â Â Â Âparent_id = parent_css->id;
> > Â Â Â Âdepth = parent_id->depth + 1;
> >
> > @@ -4646,7 +4626,7 @@ static int alloc_css_id(struct cgroup_su
> > Â Â Â Â * child_id->css pointer will be set after this cgroup is available
> > Â Â Â Â * see cgroup_populate_dir()
> > Â Â Â Â */
> > - Â Â Â rcu_assign_pointer(child_css->id, child_id);
> > + Â Â Â rcu_assign_pointer(css->id, child_id);
> >
> > Â Â Â Âreturn 0;
> > Â}
> > Index: mmotm-0827/include/linux/cgroup.h
> > ===================================================================
> > --- mmotm-0827.orig/include/linux/cgroup.h
> > +++ mmotm-0827/include/linux/cgroup.h
> > @@ -588,9 +588,11 @@ static inline int cgroup_attach_task_cur
> > Â/*
> > Â* CSS ID is ID for cgroup_subsys_state structs under subsys. This only works
> > Â* if cgroup_subsys.use_id == true. It can be used for looking up and scanning.
> > - * CSS ID is assigned at cgroup allocation (create) automatically
> > - * and removed when subsys calls free_css_id() function. This is because
> > - * the lifetime of cgroup_subsys_state is subsys's matter.
> > + * CSS ID must be assigned by subsys itself at cgroup creation and deleted
> > + * when subsys calls free_css_id() function. This is because the life time of
> To be consistent with document: s/life time/lifetime/
> > + * of cgroup_subsys_state is subsys's matter.
> > + *
> > + * ID->css look up is available after cgroup's directory is populated.
> > Â*
> > Â* Looking up and scanning function should be called under rcu_read_lock().
> > Â* Taking cgroup_mutex()/hierarchy_mutex() is not necessary for following calls.
> > @@ -598,10 +600,10 @@ static inline int cgroup_attach_task_cur
> > Â* destroyed". The caller should check css and cgroup's status.
> > Â*/
> >
> > -/*
> > - * Typically Called at ->destroy(), or somewhere the subsys frees
> > - * cgroup_subsys_state.
> > - */
> > +/* Should be called in ->create() by subsys itself */
> > +int alloc_css_id(struct cgroup_subsys *ss, struct cgroup *newgr,
> > + Â Â Â Â Â Â Â struct cgroup_subsys_state *css);
> > +/* Typically Called at ->destroy(), or somewhere the subsys frees css */
> s/Called/called/

will fix.


> > Âvoid free_css_id(struct cgroup_subsys *ss, struct cgroup_subsys_state *css);
> >
> > Â/* Find a cgroup_subsys_state which has given ID */
> > Index: mmotm-0827/mm/memcontrol.c
> > ===================================================================
> > --- mmotm-0827.orig/mm/memcontrol.c
> > +++ mmotm-0827/mm/memcontrol.c
> > @@ -4141,6 +4141,11 @@ mem_cgroup_create(struct cgroup_subsys *
> > Â Â Â Â Â Â Â Âif (alloc_mem_cgroup_per_zone_info(mem, node))
> > Â Â Â Â Â Â Â Â Â Â Â Âgoto free_out;
> >
> > + Â Â Â error = alloc_css_id(ss, cont, &mem->css);
> > + Â Â Â if (error)
> > + Â Â Â Â Â Â Â goto free_out;
> > + Â Â Â /* Here, css_id(&mem->css) works. but css_lookup(id)->mem doesn't */
> > +
> > Â Â Â Â/* root ? */
> > Â Â Â Âif (cont->parent == NULL) {
> > Â Â Â Â Â Â Â Âint cpu;
> > Index: mmotm-0827/block/blk-cgroup.c
> > ===================================================================
> > --- mmotm-0827.orig/block/blk-cgroup.c
> > +++ mmotm-0827/block/blk-cgroup.c
> > @@ -958,9 +958,13 @@ blkiocg_create(struct cgroup_subsys *sub
> > Â{
> > Â Â Â Âstruct blkio_cgroup *blkcg;
> > Â Â Â Âstruct cgroup *parent = cgroup->parent;
> > + Â Â Â int ret;
> >
> > Â Â Â Âif (!parent) {
> > Â Â Â Â Â Â Â Âblkcg = &blkio_root_cgroup;
> > + Â Â Â Â Â Â Â ret = alloc_css_id(subsys, cgroup, &blkcg->css);
> > + Â Â Â Â Â Â Â if (ret)
> > + Â Â Â Â Â Â Â Â Â Â Â return ERR_PTR(ret);
> > Â Â Â Â Â Â Â Âgoto done;
> > Â Â Â Â}
> >
> > @@ -971,6 +975,11 @@ blkiocg_create(struct cgroup_subsys *sub
> > Â Â Â Âblkcg = kzalloc(sizeof(*blkcg), GFP_KERNEL);
> > Â Â Â Âif (!blkcg)
> > Â Â Â Â Â Â Â Âreturn ERR_PTR(-ENOMEM);
> > + Â Â Â ret = alloc_css_id(subsys, cgroup, &blkcg->css);
> > + Â Â Â if (ret) {
> > + Â Â Â Â Â Â Â kfree(blkcg);
> > + Â Â Â Â Â Â Â return ERR_PTR(ret);
> > + Â Â Â }
> >
> > Â Â Â Âblkcg->weight = BLKIO_WEIGHT_DEFAULT;
> > Âdone:
> > Index: mmotm-0827/Documentation/cgroups/cgroups.txt
> > ===================================================================
> > --- mmotm-0827.orig/Documentation/cgroups/cgroups.txt
> > +++ mmotm-0827/Documentation/cgroups/cgroups.txt
> > @@ -621,6 +621,54 @@ and root cgroup. Currently this will onl
> > Âthe default hierarchy (which never has sub-cgroups) and a hierarchy
> > Âthat is being created/destroyed (and hence has no sub-cgroups).
> >
> > +3.4 cgroup subsys state IDs.
> > +------------
> > +When subsystem sets use_id == true, an ID per [cgroup, subsys] is added
> > +and it will be tied to cgroup_subsys_state object.
> > +
> > +When use_id==true can use following interfaces. But please note that
> > +allocation/free an ID is subsystem's job because cgroup_subsys_state
> > +object's lifetime is subsystem's matter.
> > +
> > +unsigned short css_id(struct cgroup_subsys_state *css)
> > +
> > +Returns ID of cgroup_subsys_state
> Please add trailing '.' (period character).
> 

will fix.

> > +
> > +unsigend short css_depth(struct cgroup_subsys_state *css)
> Typo: s/unsigend/unsigned/
> > +
> > +Returns the level which "css" is exisiting under hierarchy tree.
> > +The root cgroup's depth 0, its children are 1, children's children are
> > +2....
> > +
> > +int alloc_css_id(struct struct cgroup_subsys *ss, struct cgroup *newgr,
> > + Â Â Â Â Â Â Â Âstruct cgroup_subsys_state *css);
> > +
> > +Attach an new ID to given css under subsystem ([ss, cgroup])
> > +should be called in ->create() callback.
> > +
> > +void free_css_id(struct cgroup_subsys *ss, struct cgroup_subsys_state *css);
> > +
> > +Free ID attached to "css" under subsystem. Should be called before
> > +"css" is freed.
> > +
> > +struct cgroup_subsys_state *css_lookup(struct cgroup_subsys *ss, int id);
> > +
> > +Look up cgroup_subsys_state via ID. Should be called under rcu_read_lock().
> > +
> > +struct cgroup_subsys_state *css_get_next(struct cgroup_subsys *ss, int id,
> > + Â Â Â Â Â Â Â Âstruct cgroup_subsys_state *root, int *foundid);
> > +
> > +Returns ID which is under "root" i.e. under sub-directory of "root"
> > +cgroup's directory at considering cgroup hierarchy. The order of IDs
> > +returned by this function is not sorted. Please be careful.
> > +
> > +bool css_is_ancestor(struct cgroup_subsys_state *cg,
> > + Â Â Â Â Â Â Â Â Â Â const struct cgroup_subsys_state *root);
> 
> To match code: s/cg/child/
> 
will fix.

> > +
> > +Returns true if "root" and "cs" is under the same hierarchy and
> > +"root" can be found when you see all ->parent from "cs" until
> This may be more clear: s/see all/walk all/
> 
> > +the root cgroup.
> As above: s/cs/child/
> 

will fix.

I'll reorder patches and post file-stat ones 1st.

Thanks,
-Kame

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/