Messages in this thread | | | Date | Tue, 9 Aug 2011 19:27:50 +0200 | From | Frederic Weisbecker <> | Subject | Re: [PATCH 7/8] cgroups: Add a task counter subsystem |
| |
On Tue, Aug 09, 2011 at 05:11:55PM +0200, Oleg Nesterov wrote: > On 07/29, Frederic Weisbecker wrote: > > > > +static int task_counter_can_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp, > > + struct task_struct *tsk) > > +{ > > + struct res_counter *res = cgroup_task_counter_res(cgrp); > > + struct res_counter *old_res = cgroup_task_counter_res(old_cgrp); > > + struct res_counter *limit_fail_at; > > + > > + common_ancestor = res_counter_common_ancestor(res, old_res); > > + > > + return res_counter_charge_until(res, common_ancestor, 1, &limit_fail_at); > > +} > > > > ... > > > > +static void task_counter_attach_task(struct cgroup *cgrp, struct cgroup *old_cgrp, > > + struct task_struct *tsk) > > +{ > > + res_counter_uncharge_until(cgroup_task_counter_res(old_cgrp), common_ancestor, 1); > > +} > > This doesn't look right or I missed something. > > What if tsk exits in between? Afaics this can happen with both > cgroup_attach_task() and cgroup_attach_proc(). > > Let's look at cgroup_attach_task(). Suppose that > task_counter_can_attach_task() succeeds and charges the new cgrp, > Then cgroup_task_migrate() returns -ESRCH. Who will uncharge the > new cgrp? >
I may totally be missing something but that looks safe to me. If the task has exited then cgroup_task_migrate() fails then we rollback with ->cancel_attach_task().
Let me enumerate the possible scenario (may not be exhaustive):
* The task exits (called cgroup_exit()) before we cgroup_task_migrate() switch the cgroup. In this case we rollback the charge we pushed on the new cgoup and we return an error.
* The task exits after cgroup_task_migrate(), in which case cgroup called ->exit() on the task with the new cgroup, uncharging that task from it. At the same time we call ->attach_task() to uncharge the old cgroup, which is still what we want as we confirmed the cgroup migration.
> cgroup_attach_proc() is different, it calls cgroup_task_migrate() > after ->attach_task(). Cough.
That's bad. I need to fix that.
So if it returns -ESRCH, I shall not call attach_task() on it but cancel_attach_task().
Other than that it should be safe as in the single task case.
> In this case old_cgrp can be uncharged twice, no? And again, nobody > will uncharge the new cgrp?
(see above)
> ->attach_task() can be skipped if cgrp == oldcgrp... Probably this > is fine, in this case can_attach_task() shouldn't actually charge.
In fact in this case it simply doesn't charge. res_counter_common_ancestor() returns the res_counter for cgrp as a limit and thus charging stops as soon as it starts.
> > > @@ -1295,6 +1295,10 @@ static struct task_struct *copy_process(unsigned long clone_flags, > > p->group_leader = p; > > INIT_LIST_HEAD(&p->thread_group); > > > > + retval = cgroup_task_counter_fork(p); > > + if (retval) > > + goto bad_fork_free_pid; > > + > > Well, imho this is not good. You are adding yet another cgroup hook. > Why you can not reuse cgroup_fork_callbacks() ? > > Yes, it returns void. Can't we chane ->fork() to return the error and > make it boolean?
That was my first proposition (minus the rollback with exit() that I forgot) but Paul Menage said that added unnecessary complexity in the fork callbacks.
> > Better yet, > > - cgroup_fork_callbacks(p); > - cgroup_callbacks_done = 1; > + failed_ss = cgroup_fork_callbacks(p); > + if (failed_ss) > + goto bad_fork_free_pid; > > ... > > - cgroup_exit(p, cgroup_callbacks_done); > + cgroup_exit(p, failed_ss); > > What do you think?
I would personally prefer that.
> Oleg. >
| |