On 10/07, Tejun Heo wrote:
>
> Hey,
>
> On Fri, Oct 04, 2013 at 03:02:07PM +0200, Oleg Nesterov wrote:
> > Not sure I understand, but just in case: yes the lockless
> > while_each_thread() is buggy and should be fixed (it should actually
> > die eventually). And a lot of current users of while_each_thread()
> > are themselves buggy and need the fixes no matter what we do with
> > while_each_thread.
> >
> > Oh. and just in case... I am (slowly) working on this, but didn't
> > finish it yet, sorry.
>
> Is there anything I can do to make the hang go away in the meantime?
well, probably tasklist, or pid_alive() check... note that this code
rcu/while_each_thread() logic looks wrong even if while_each_thread()
was fine. Will try to makes a patch today.
But! Anjana! I am so sorry!! I simply can't understand how I managed
to misunderstand (and confuse!) you.
Tejun, could you please look at the patch from Anjana again?
> > > --- a/kernel/cgroup.c
> > > +++ b/kernel/cgroup.c
> > > @@ -2002,7 +2002,9 @@ static int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk,
> > > ent.task = tsk;
> > > ent.cgrp = task_cgroup_from_root(tsk, root);
> > > /* nothing to do if this task is already in the cgroup */
> > > - if (ent.cgrp == cgrp)
> > > + if (ent.cgrp == cgrp && !threadgroup)
> > > + break;
> > > + else if(ent.cgrp == cgrp)
> > > continue;
>
> I have no idea know how this would make any difference. Hmm...
I guess the patch needs a cleanup, but this code looks indeed wrong
even if we forget about rcu/while_each_thread? Really, we should not
"continue" if threadgroup == T, in this case we miss the last
if (!threadgroup)
break;
check in the main loop. So Anjana was right (sorry again!), and we
should probably do
ent.cgrp = task_cgroup_from_root(...);
if (ent.cgrp != cgrp) {
retval = flex_array_put(...);
...
}
if (!threadgroup)
break;
Or I am wrong again?
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe cgroups" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
