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

[Oleg Nesterov <oleg@redhat.com>]

On 08/09, Frederic Weisbecker wrote:
>
> On Tue, Aug 09, 2011 at 05:11:55PM +0200, Oleg Nesterov wrote:
> >
> > 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().

cgroup_attach_task() doesn't call ->cancel_attach_task() ;)

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

Afaics this can't help, or I misunderstood. probably attach_task()
can check PF_EXITING...

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

Yes, this is what I meant. Just it wasn't immediately obvious for me,
initially I thought this is buggy.

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

Hmm. Yes, this adds some complexity in the fork callbacks.

But yet another cgroup_task_counter_fork() hook complicates the core kernel
code. And since I personally do not care about cgroups at all, I think this
is much worse ;)

> I would personally prefer that.

I strongly agree.

OK. Lets do it this way. Perhaps we can convince Paul later and cleanup.

Oleg.