[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-06-03 Thread Oleg Nesterov
On 06/03, Ben Blum wrote: On Mon, May 31, 2010 at 07:52:42PM +0200, Oleg Nesterov wrote: OK, the caller has a reference to the argument, leader, + leader = leader-group_leader; But why it is safe to use leader-group_leader if we race with exec? This line means let's try to find

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-06-03 Thread Oleg Nesterov
On 06/03, Ben Blum wrote: On Wed, Jun 02, 2010 at 10:58:55PM +0200, Oleg Nesterov wrote: Hmm. The usage of -thread_group in -can_attach() methods doesn't look safe to me... but currently bool threadgroup is always false. I recall putting a rcu_read_lock() around that part and being assured

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-06-02 Thread Oleg Nesterov
On 06/01, Paul Menage wrote: On Mon, May 31, 2010 at 11:04 AM, Oleg Nesterov o...@redhat.com wrote: And, forgot to mention, I do not understand the PF_EXITING check in attach_task_by_pid() (and some others). At first glance, it buys nothing. PF_EXITING can be set right after the

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-06-02 Thread Paul Menage
On Wed, Jun 2, 2010 at 7:06 AM, Oleg Nesterov o...@redhat.com wrote: Yes sure, I understand this part. cgroup_attach_task() correctly checks PF_EXITING under task_lock(), this protects from the case when this task has already passed cgroup_exit() which takes this lock too. Right. But. This

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-06-02 Thread Oleg Nesterov
On 06/02, Paul Menage wrote: On Wed, Jun 2, 2010 at 7:06 AM, Oleg Nesterov o...@redhat.com wrote: I am not sure. It doesn't hurt when we try to move a thread. But if we want to move the whole thread group, we should proceed even if the group leader has already exited and thus has

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-06-02 Thread Paul Menage
On Wed, Jun 2, 2010 at 1:20 PM, Oleg Nesterov o...@redhat.com wrote: Yes sure. The main thread can exit via sys_exit(), this doesn't affect the thread group. Of course, I am not saying this is common practice, perhaps no important app does this. This check has been in cgroups for quite a

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-06-02 Thread Oleg Nesterov
On 06/02, Paul Menage wrote: On Wed, Jun 2, 2010 at 1:20 PM, Oleg Nesterov o...@redhat.com wrote: Yes sure. The main thread can exit via sys_exit(), this doesn't affect the thread group. Of course, I am not saying this is common practice, perhaps no important app does this. This check

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-06-02 Thread Paul Menage
On Wed, Jun 2, 2010 at 1:58 PM, Oleg Nesterov o...@redhat.com wrote: The it that you're proposing to remove is in fact the code that handles those races. In that case I confused, and I thought we already agreed that the PF_EXITING check in attach_task_by_pid() is not strictly needed for

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-06-02 Thread Oleg Nesterov
On 06/02, Paul Menage wrote: On Wed, Jun 2, 2010 at 1:58 PM, Oleg Nesterov o...@redhat.com wrote: The it that you're proposing to remove is in fact the code that handles those races. In that case I confused, and I thought we already agreed that the PF_EXITING check in

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-06-02 Thread Paul Menage
On Wed, Jun 2, 2010 at 2:38 PM, Oleg Nesterov o...@redhat.com wrote: cgroup_attach_task() does, and this time PF_EXITING is understandable. Oh, OK, I was talking about the one in cgroup_attach_task(), which is called by attach_task_by_pid(), and assumed that you were referring to the same one.

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-06-02 Thread Ben Blum
On Wed, Jun 02, 2010 at 10:58:55PM +0200, Oleg Nesterov wrote: Hmm. The usage of -thread_group in -can_attach() methods doesn't look safe to me... but currently bool threadgroup is always false. Oleg. I recall putting a rcu_read_lock() around that part and being assured that made it

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-06-02 Thread Ben Blum
On Wed, Jun 02, 2010 at 03:03:49PM -0700, Paul Menage wrote: On Wed, Jun 2, 2010 at 2:38 PM, Oleg Nesterov o...@redhat.com wrote: cgroup_attach_task() does, and this time PF_EXITING is understandable. Oh, OK, I was talking about the one in cgroup_attach_task(), which is called by

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-06-02 Thread Ben Blum
On Mon, May 31, 2010 at 07:52:42PM +0200, Oleg Nesterov wrote: I only glanced into one function, cgroup_attach_proc(), and some things look obviously wrong. Sorry, I can't really read these patches now, most probably I misunderstood the code... +int cgroup_attach_proc(struct cgroup *cgrp,

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-06-01 Thread Paul Menage
On Mon, May 31, 2010 at 11:04 AM, Oleg Nesterov o...@redhat.com wrote: And, forgot to mention, I do not understand the PF_EXITING check in attach_task_by_pid() (and some others). At first glance, it buys nothing. PF_EXITING can be set right after the check. It can, but it's a benign race.

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-05-31 Thread Oleg Nesterov
I only glanced into one function, cgroup_attach_proc(), and some things look obviously wrong. Sorry, I can't really read these patches now, most probably I misunderstood the code... +int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader) +{ + int retval; + struct

[Devel] Re: [RFC] [PATCH 2/2] cgroups: make procs file writable

2010-05-31 Thread Oleg Nesterov
On 05/31, Oleg Nesterov wrote: I only glanced into one function, cgroup_attach_proc(), and some things look obviously wrong. Sorry, I can't really read these patches now, most probably I misunderstood the code... And, forgot to mention, I do not understand the PF_EXITING check in