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

Balbir Singh
2009-11-09T06:58:07Z

* 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?

> + 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