Message-ID: <20090803110048.GB29252@hawkmoon.kerlabs.com>
Date:	Mon, 3 Aug 2009 13:00:48 +0200
From:	Louis Rilling <Louis.Rilling@...labs.com>
To:	Benjamin Blum <bblum@...gle.com>
Cc:	linux-kernel@...r.kernel.org,
	containers@...ts.linux-foundation.org, akpm@...ux-foundation.org,
	serue@...ibm.com, lizf@...fujitsu.com, menage@...gle.com
Subject: Re: [PATCH 5/6] Makes procs file writable to move all threads by
	tgid at once

On 28/07/09 17:23 -0700, Benjamin Blum wrote:
> On Fri, Jul 24, 2009 at 2:52 PM, Benjamin Blum<bblum@...gle.com> wrote:
> > On Fri, Jul 24, 2009 at 3:02 AM, Louis Rilling<Louis.Rilling@...labs.com> wrote:
> >> On 23/07/09 20:22 -0700, Ben Blum wrote:
> >>> @@ -408,6 +409,15 @@ You can attach the current shell task by echoing 0:
> >>>
> >>>  # echo 0 > tasks
> >>>
> >>> +The cgroup.procs file is useful for managing all tasks in a threadgroup at
> >>> +once. It works the same way as the tasks file, but moves all tasks in the
> >>> +threadgroup with the specified tgid.
> >>> +
> >>> +Writing the pid of a task that's not the threadgroup leader (i.e., a pid
> >>> +that isn't a tgid) is treated as invalid. Writing a '0' to cgroup.procs will
> >>> +attach the writing task and all tasks in its threadgroup, but is invalid if
> >>> +the writing task is not the leader of the threadgroup.
> >>> +
> >>
> >> This restriction sounds unfortunate and I'm not sure that there are good reasons
> >> for it (see below).
> >
> > Would it be preferable to allow any thread in a threadgroup to move
> > the whole group to a new cgroup? Would that undermine the desired
> > restriction of the tasks file in which a thread needs to have
> > sufficient perms to move a thread that's not itself, or is the task
> > file's restriction there undesirable in the case of threadgroups?
> 
> Still wondering on what to do about this. Noting that this restriction
> approximately parallels the behaviour you'd get from using the tasks
> file, I think it'd be safer to leave it like this then possibly in the
> future lift the restriction, rather than make it currently lifted then
> decide in the future that we want to re-impose it.

Sorry, I was on vacations last week.

To me it looks like all threads should be treated equally, since AFAIK POSIX has
no notion of thread group leader. Moreover, a thread group (process) can live an
arbitrary long time after the death of its leader (that remains as a zombie), so
this restriction just prevents such processes to user the procs file.

> 
> >>> +      * to move all tasks to the new cgroup. the only fail case henceforth
> >>> +      * is if the threadgroup leader has PF_EXITING set (in which case all
> >>> +      * the other threads get killed) - if other threads happen to be
> >>
> >> This statement is wrong. A thread group leader can have PF_EXITING (and even
> >> become zombie) while other sub-threads continue their execution. For instance it
> >> is perfectly valid for a thread group leader to call sys_exit(), and no other
> >> thread will be affected.
> >
> > Is that the case? Then, what becomes of his task_struct when he's
> > gone, since presumably all the sub-threads will need it. Does it just
> > hang around?

Yes. The thread leader remains in zombie state (and cannot be reaped) until all
other threads have exited.

> >
> > If so, would the desired behaviour be to proceed with moving all
> > sub-threads if the leader is exiting? (That's not a very big change
> > code-wise)
> >
> 
> I've fixed this, and also implemented the rwsem as threadgroup-local.
> If there's no more discussion on this version of the patch, I'll send
> out the revised series soon.

Ok. Thanks.

Louis

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)

Powered by blists - more mailing lists