Re: [PATCH -mmotm 1/8] cgroup: introduce cancel_attach()

Daisuke Nishimura
2009-11-09T07:30:15Z

On Mon, 9 Nov 2009 12:27:59 +0530, Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> * nishimura@mxp.nes.nec.co.jp <nishimura@mxp.nes.nec.co.jp> [2009-11-06 14:11:06]:
>
> > This patch adds cancel_attach() operation to struct cgroup_subsys.
> > cancel_attach() can be used when can_attach() operation prepares something
> > for the subsys, but we should rollback what can_attach() operation has prepared
> > if attach task fails after we've succeeded in can_attach().
> >
> > Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > ---
> > Documentation/cgroups/cgroups.txt | 13 +++++++++++-
> > include/linux/cgroup.h | 2 +
> > kernel/cgroup.c | 38 ++++++++++++++++++++++++++++++------
> > 3 files changed, 45 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/cgroups/cgroups.txt b/Documentation/cgroups/cgroups.txt
> > index 0b33bfe..c86947c 100644
> > --- a/Documentation/cgroups/cgroups.txt
> > +++ b/Documentation/cgroups/cgroups.txt
> > @@ -536,10 +536,21 @@ returns an error, this will abort the attach operation. If a NULL
> > task is passed, then a successful result indicates that *any*
> > unspecified task can be moved into the cgroup. Note that this isn't
> > called on a fork. If this method returns 0 (success) then this should
> > -remain valid while the caller holds cgroup_mutex. If threadgroup is
> > +remain valid while the caller holds cgroup_mutex and it is ensured that either
> > +attach() or cancel_attach() will be called in futer. If threadgroup is
> > true, then a successful result indicates that all threads in the given
> > thread's threadgroup can be moved together.
> >
> > +void cancel_attach(struct cgroup_subsys *ss, struct cgroup *cgrp,
> > + struct task_struct *task, bool threadgroup)
> > +(cgroup_mutex held by caller)
> > +
> > +Called when a task attach operation has failed after can_attach() has succeeded.
> > +A subsystem whose can_attach() has some side-effects should provide this
> > +function, so that the subsytem can implement a rollback. If not, not necessary.
> > +This will be called only about subsystems whose can_attach() operation have
> > +succeeded.
> > +
> > void attach(struct cgroup_subsys *ss, struct cgroup *cgrp,
> > struct cgroup *old_cgrp, struct task_struct *task,
> > bool threadgroup)
> > diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> > index 0008dee..d4cc200 100644
> > --- a/include/linux/cgroup.h
> > +++ b/include/linux/cgroup.h
> > @@ -427,6 +427,8 @@ struct cgroup_subsys {
> > void (*destroy)(struct cgroup_subsys *ss, struct cgroup *cgrp);
> > int (*can_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
> > struct task_struct *tsk, bool threadgroup);
> > + void (*cancel_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
> > + struct task_struct *tsk, bool threadgroup);
> > void (*attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
> > struct cgroup *old_cgrp, struct task_struct *tsk,
> > bool threadgroup);
> > diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> > index 0249f4b..e443742 100644
> > --- a/kernel/cgroup.c
> > +++ b/kernel/cgroup.c
> > @@ -1539,7 +1539,7 @@ int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen)
> > int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> > {
> > int retval = 0;
> > - struct cgroup_subsys *ss;
> > + struct cgroup_subsys *ss, *failed_ss = NULL;
> > struct cgroup *oldcgrp;
> > struct css_set *cg;
> > struct css_set *newcg;
> > @@ -1553,8 +1553,16 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> > for_each_subsys(root, ss) {
> > if (ss->can_attach) {
> > retval = ss->can_attach(ss, cgrp, tsk, false);
> > - if (retval)
> > - return retval;
> > + if (retval) {
> > + /*
> > + * Remember at which subsystem we've failed in
> > + * can_attach() to call cancel_attach() only
> > + * against subsystems whose attach() have
> > + * succeeded(see below).
> > + */
> > + failed_ss = ss;
>
> failed_ss is global? Is it a marker into an array of subsystems? Don't
> we need more than one failed_ss for each failed subsystem? Or do we
> find the first failed subsystem, cancel_attach and fail all
> migrations?
>
failed_ss is just a local valiable(see above definition) :)
It is used to remember "at which subsystem of for_each_subsys we failed in can_attach()".
By remembering it, we can avoid calling cancel_attach() against subsystems whose
can_attach() has failed or has not even called.

Thanks,
Daisuke Nishimura.

> > + goto out;
> > + }
> > }
> > }
> >
> > @@ -1568,14 +1576,17 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> > */
> > newcg = find_css_set(cg, cgrp);
> > put_css_set(cg);
> > - if (!newcg)
> > - return -ENOMEM;
> > + if (!newcg) {
> > + retval = -ENOMEM;
> > + goto out;
> > + }
> >
> > task_lock(tsk);
> > if (tsk->flags & PF_EXITING) {
> > task_unlock(tsk);
> > put_css_set(newcg);
> > - return -ESRCH;
> > + retval = -ESRCH;
> > + goto out;
> > }
> > rcu_assign_pointer(tsk->cgroups, newcg);
> > task_unlock(tsk);
> > @@ -1601,7 +1612,20 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> > * is no longer empty.
> > */
> > cgroup_wakeup_rmdir_waiter(cgrp);
> > - return 0;
> > +out:
> > + if (retval)
> > + for_each_subsys(root, ss) {
> > + if (ss == failed_ss)
> > + /*
> > + * This means can_attach() of this subsystem
> > + * have failed, so we don't need to call
> > + * cancel_attach() against rests of subsystems.
> > + */
> > + break;
> > + if (ss->cancel_attach)
> > + ss->cancel_attach(ss, cgrp, tsk, false);
> > + }
> > + return retval;
> > }
> >
>
> --
> Balbir