Re: [PATCH 7/8] cgroups: Add a task counter subsystem

Frederic Weisbecker
2011-08-09T17:27:57Z

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