Messages in this thread | | | Date | Tue, 9 Aug 2011 19:57:08 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 7/8] cgroups: Add a task counter subsystem |
| |
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.
| |