* + cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch added to -mm tree
@ 2011-08-25 20:44 akpm
2011-08-26 15:12 ` Oleg Nesterov
2011-08-26 15:38 ` [PATCH v2] cgroups: Don't attach task to subsystem if migration failed Frederic Weisbecker
0 siblings, 2 replies; 8+ messages in thread
From: akpm @ 2011-08-25 20:44 UTC (permalink / raw)
To: mm-commits; +Cc: bblum, fweisbec, lizf, oleg, paul, tj
The patch titled
cgroups: fix ordering of calls in cgroup_attach_proc
has been added to the -mm tree. Its filename is
cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch
Before you just go and hit "reply", please:
a) Consider who else should be cc'ed
b) Prefer to cc a suitable mailing list as well
c) Ideally: find the original patch on the mailing list and do a
reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/SubmitChecklist when testing your code ***
See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this
The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
------------------------------------------------------
Subject: cgroups: fix ordering of calls in cgroup_attach_proc
From: Ben Blum <bblum@andrew.cmu.edu>
awaiting useful changelog...
Signed-off-by: Ben Blum <bblum@andrew.cmu.edu>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Paul Menage <paul@paulmenage.org>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
kernel/cgroup.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff -puN kernel/cgroup.c~cgroups-fix-ordering-of-calls-in-cgroup_attach_proc kernel/cgroup.c
--- a/kernel/cgroup.c~cgroups-fix-ordering-of-calls-in-cgroup_attach_proc
+++ a/kernel/cgroup.c
@@ -2135,14 +2135,17 @@ int cgroup_attach_proc(struct cgroup *cg
oldcgrp = task_cgroup_from_root(tsk, root);
if (cgrp == oldcgrp)
continue;
- /* attach each task to each subsystem */
- for_each_subsys(root, ss) {
- if (ss->attach_task)
- ss->attach_task(cgrp, tsk);
- }
/* if the thread is PF_EXITING, it can just get skipped. */
retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, true);
- BUG_ON(retval != 0 && retval != -ESRCH);
+ if (retval == 0) {
+ /* attach each task to each subsystem */
+ for_each_subsys(root, ss) {
+ if (ss->attach_task)
+ ss->attach_task(cgrp, tsk);
+ }
+ } else {
+ BUG_ON(retval != -ESRCH);
+ }
}
/* nothing is sensitive to fork() after this point. */
_
Patches currently in -mm which might be from bblum@andrew.cmu.edu are
cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: + cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch added to -mm tree
2011-08-25 20:44 + cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch added to -mm tree akpm
@ 2011-08-26 15:12 ` Oleg Nesterov
2011-08-26 15:18 ` Paul Menage
2011-08-26 15:21 ` Tejun Heo
2011-08-26 15:38 ` [PATCH v2] cgroups: Don't attach task to subsystem if migration failed Frederic Weisbecker
1 sibling, 2 replies; 8+ messages in thread
From: Oleg Nesterov @ 2011-08-26 15:12 UTC (permalink / raw)
To: akpm; +Cc: linux-kernel, bblum, fweisbec, lizf, paul, tj
On 08/25, Andrew Morton wrote:
>
> From: Ben Blum <bblum@andrew.cmu.edu>
>
> @@ -2135,14 +2135,17 @@ int cgroup_attach_proc(struct cgroup *cg
> oldcgrp = task_cgroup_from_root(tsk, root);
> if (cgrp == oldcgrp)
> continue;
> - /* attach each task to each subsystem */
> - for_each_subsys(root, ss) {
> - if (ss->attach_task)
> - ss->attach_task(cgrp, tsk);
> - }
> /* if the thread is PF_EXITING, it can just get skipped. */
> retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, true);
> - BUG_ON(retval != 0 && retval != -ESRCH);
> + if (retval == 0) {
> + /* attach each task to each subsystem */
> + for_each_subsys(root, ss) {
> + if (ss->attach_task)
> + ss->attach_task(cgrp, tsk);
> + }
Yes, I think this is what we need, the patch itself looks fine.
But this doesn't answer my another question. After that the code does
* step 4: do expensive, non-thread-specific subsystem callbacks.
ss->attach(ss, cgrp, oldcgrp, leader);
OK, non-thread-specific is nice, but how can this "leader" represent
the process?
It can be zombie (but still group_leader) even without any races.
Say, cpuset_attach() and mem_cgroup_move_task() need get_task_mm(p).
How this can work if the leader is dead?
Also. Even if we add the locking around while_each_thread() (btw,
we need this in any case), we can race with exec which can change
the leader. In this case this task_struct has nothing to do with
the process we are going to attach, at all.
And, ss->can_attach(leader) has the same problems, it seems.
And. Say, devcgroup_can_attach() checks CAP_SYS_ADMIN. This is
security check. Why it is enough to check the leader only? We are
going to attach all threads. OK, this is probably fine, and I never
understood why capable/creds are not per-process, but this looks
so strange.
Oleg.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: + cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch added to -mm tree
2011-08-26 15:12 ` Oleg Nesterov
@ 2011-08-26 15:18 ` Paul Menage
2011-08-26 15:21 ` Tejun Heo
1 sibling, 0 replies; 8+ messages in thread
From: Paul Menage @ 2011-08-26 15:18 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: akpm, linux-kernel, bblum, fweisbec, lizf, tj
On Fri, Aug 26, 2011 at 8:12 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
>
>
> But this doesn't answer my another question. After that the code does
>
> * step 4: do expensive, non-thread-specific subsystem callbacks.
>
> ss->attach(ss, cgrp, oldcgrp, leader);
>
> OK, non-thread-specific is nice, but how can this "leader" represent
> the process?
There's a set of patches from Tejun Heo that address some of these
inconsistencies and lack of information - search the list for
cgroup_taskset.
Paul
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: + cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch added to -mm tree
2011-08-26 15:12 ` Oleg Nesterov
2011-08-26 15:18 ` Paul Menage
@ 2011-08-26 15:21 ` Tejun Heo
2011-08-26 15:50 ` Oleg Nesterov
1 sibling, 1 reply; 8+ messages in thread
From: Tejun Heo @ 2011-08-26 15:21 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: akpm, linux-kernel, bblum, fweisbec, lizf, paul
Hello, Oleg.
On Fri, Aug 26, 2011 at 05:12:45PM +0200, Oleg Nesterov wrote:
> Yes, I think this is what we need, the patch itself looks fine.
>
> But this doesn't answer my another question. After that the code does
>
> * step 4: do expensive, non-thread-specific subsystem callbacks.
>
> ss->attach(ss, cgrp, oldcgrp, leader);
>
> OK, non-thread-specific is nice, but how can this "leader" represent
> the process?
>
> It can be zombie (but still group_leader) even without any races.
> Say, cpuset_attach() and mem_cgroup_move_task() need get_task_mm(p).
> How this can work if the leader is dead?
>
> Also. Even if we add the locking around while_each_thread() (btw,
> we need this in any case), we can race with exec which can change
> the leader. In this case this task_struct has nothing to do with
> the process we are going to attach, at all.
>
> And, ss->can_attach(leader) has the same problems, it seems.
Please take a look at the following series.
http://thread.gmane.org/gmane.linux.kernel/1184375
attach racing against exit/exec is problematic and maybe we should
extend task->threadgroup_fork_lock protection to cover both exit and
exec. I can't like it tho. I hope this can be somehow done in
lighter way. cgroup attaches are quite cold paths and we're putting
an extra rwsem in each task for that.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] cgroups: Don't attach task to subsystem if migration failed
2011-08-25 20:44 + cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch added to -mm tree akpm
2011-08-26 15:12 ` Oleg Nesterov
@ 2011-08-26 15:38 ` Frederic Weisbecker
2011-08-26 17:01 ` Ben Blum
1 sibling, 1 reply; 8+ messages in thread
From: Frederic Weisbecker @ 2011-08-26 15:38 UTC (permalink / raw)
To: akpm; +Cc: mm-commits, bblum, lizf, oleg, paul, tj, LKML
On Thu, Aug 25, 2011 at 01:44:36PM -0700, akpm@linux-foundation.org wrote:
>
> The patch titled
> cgroups: fix ordering of calls in cgroup_attach_proc
> has been added to the -mm tree. Its filename is
> cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch
>
> Before you just go and hit "reply", please:
> a) Consider who else should be cc'ed
> b) Prefer to cc a suitable mailing list as well
> c) Ideally: find the original patch on the mailing list and do a
> reply-to-all to that, adding suitable additional cc's
>
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
>
> See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
> out what to do about this
>
> The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
>
> ------------------------------------------------------
> Subject: cgroups: fix ordering of calls in cgroup_attach_proc
> From: Ben Blum <bblum@andrew.cmu.edu>
>
> awaiting useful changelog...
>
Here is the patch with a (trial of a) useful changelog. Subject
has been changed as well:
---
>From a9b84e395a0355cd1aa5ee0f525cd682b16dad63 Mon Sep 17 00:00:00 2001
From: Ben Blum <bblum@andrew.cmu.edu>
Date: Thu, 25 Aug 2011 13:44:36 -0700
Subject: [PATCH] cgroups: Don't attach task to subsystem if migration failed
If a task has exited to the point it has called cgroup_exit()
already, then we can't migrate it to another cgroup anymore.
This can happen when we are attaching a task to a new cgroup
between the call to ->can_attach_task() on subsystems and
the migration that is eventually tried in cgroup_task_migrate().
In this case cgroup_task_migrate() returns -ESRCH and we don't
want to attach the task to the subsystems because the attachment
to the new cgroup itself failed.
Fix this by only calling ->attach_task() on the subsystems if
the cgroup migration succeeded.
Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Ben Blum <bblum@andrew.cmu.edu>
Acked-by: Paul Menage <paul@paulmenage.org>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
kernel/cgroup.c | 15 +++++++++------
1 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 1d2b6ce..84bdace 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2135,14 +2135,17 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
oldcgrp = task_cgroup_from_root(tsk, root);
if (cgrp == oldcgrp)
continue;
- /* attach each task to each subsystem */
- for_each_subsys(root, ss) {
- if (ss->attach_task)
- ss->attach_task(cgrp, tsk);
- }
/* if the thread is PF_EXITING, it can just get skipped. */
retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, true);
- BUG_ON(retval != 0 && retval != -ESRCH);
+ if (retval == 0) {
+ /* attach each task to each subsystem */
+ for_each_subsys(root, ss) {
+ if (ss->attach_task)
+ ss->attach_task(cgrp, tsk);
+ }
+ } else {
+ BUG_ON(retval != -ESRCH);
+ }
}
/* nothing is sensitive to fork() after this point. */
--
1.7.5.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: + cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch added to -mm tree
2011-08-26 15:21 ` Tejun Heo
@ 2011-08-26 15:50 ` Oleg Nesterov
0 siblings, 0 replies; 8+ messages in thread
From: Oleg Nesterov @ 2011-08-26 15:50 UTC (permalink / raw)
To: Tejun Heo; +Cc: akpm, linux-kernel, bblum, fweisbec, lizf, paul
Hi Tejun,
On 08/26, Tejun Heo wrote:
>
> Please take a look at the following series.
>
> http://thread.gmane.org/gmane.linux.kernel/1184375
OK, thanks... looks like 1/6 conflicts with this patch ;)
Tejun, I'll appreciate if you can send me this thread in mbox format
privately. I don't think I can help, just I am curious.
> attach racing against exit/exec is problematic and maybe we should
> extend task->threadgroup_fork_lock protection to cover both exit and
> exec. I can't like it tho.
Yes...
Oleg.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] cgroups: Don't attach task to subsystem if migration failed
2011-08-26 15:38 ` [PATCH v2] cgroups: Don't attach task to subsystem if migration failed Frederic Weisbecker
@ 2011-08-26 17:01 ` Ben Blum
0 siblings, 0 replies; 8+ messages in thread
From: Ben Blum @ 2011-08-26 17:01 UTC (permalink / raw)
To: Frederic Weisbecker; +Cc: akpm, mm-commits, bblum, lizf, oleg, paul, tj, LKML
On Fri, Aug 26, 2011 at 05:38:44PM +0200, Frederic Weisbecker wrote:
> On Thu, Aug 25, 2011 at 01:44:36PM -0700, akpm@linux-foundation.org wrote:
> >
> > The patch titled
> > cgroups: fix ordering of calls in cgroup_attach_proc
> > has been added to the -mm tree. Its filename is
> > cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch
> >
> > Before you just go and hit "reply", please:
> > a) Consider who else should be cc'ed
> > b) Prefer to cc a suitable mailing list as well
> > c) Ideally: find the original patch on the mailing list and do a
> > reply-to-all to that, adding suitable additional cc's
> >
> > *** Remember to use Documentation/SubmitChecklist when testing your code ***
> >
> > See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
> > out what to do about this
> >
> > The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
> >
> > ------------------------------------------------------
> > Subject: cgroups: fix ordering of calls in cgroup_attach_proc
> > From: Ben Blum <bblum@andrew.cmu.edu>
> >
> > awaiting useful changelog...
> >
>
> Here is the patch with a (trial of a) useful changelog. Subject
> has been changed as well:
This description is much better than my original one - thanks!
I'd ack it, but my name is already there. :P
-- Ben
>
> ---
> From a9b84e395a0355cd1aa5ee0f525cd682b16dad63 Mon Sep 17 00:00:00 2001
> From: Ben Blum <bblum@andrew.cmu.edu>
> Date: Thu, 25 Aug 2011 13:44:36 -0700
> Subject: [PATCH] cgroups: Don't attach task to subsystem if migration failed
>
> If a task has exited to the point it has called cgroup_exit()
> already, then we can't migrate it to another cgroup anymore.
>
> This can happen when we are attaching a task to a new cgroup
> between the call to ->can_attach_task() on subsystems and
> the migration that is eventually tried in cgroup_task_migrate().
>
> In this case cgroup_task_migrate() returns -ESRCH and we don't
> want to attach the task to the subsystems because the attachment
> to the new cgroup itself failed.
>
> Fix this by only calling ->attach_task() on the subsystems if
> the cgroup migration succeeded.
>
> Reported-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Ben Blum <bblum@andrew.cmu.edu>
> Acked-by: Paul Menage <paul@paulmenage.org>
> Cc: Li Zefan <lizf@cn.fujitsu.com>
> Cc: Tejun Heo <tj@kernel.org>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
> kernel/cgroup.c | 15 +++++++++------
> 1 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 1d2b6ce..84bdace 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2135,14 +2135,17 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
> oldcgrp = task_cgroup_from_root(tsk, root);
> if (cgrp == oldcgrp)
> continue;
> - /* attach each task to each subsystem */
> - for_each_subsys(root, ss) {
> - if (ss->attach_task)
> - ss->attach_task(cgrp, tsk);
> - }
> /* if the thread is PF_EXITING, it can just get skipped. */
> retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, true);
> - BUG_ON(retval != 0 && retval != -ESRCH);
> + if (retval == 0) {
> + /* attach each task to each subsystem */
> + for_each_subsys(root, ss) {
> + if (ss->attach_task)
> + ss->attach_task(cgrp, tsk);
> + }
> + } else {
> + BUG_ON(retval != -ESRCH);
> + }
> }
> /* nothing is sensitive to fork() after this point. */
>
> --
> 1.7.5.4
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* + cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch added to -mm tree
@ 2011-08-26 19:27 akpm
0 siblings, 0 replies; 8+ messages in thread
From: akpm @ 2011-08-26 19:27 UTC (permalink / raw)
To: mm-commits; +Cc: bblum, fweisbec, lizf, oleg, paul, tj
The patch titled
cgroups: don't attach task to subsystem if migration failed
has been added to the -mm tree. Its filename is
cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch
Before you just go and hit "reply", please:
a) Consider who else should be cc'ed
b) Prefer to cc a suitable mailing list as well
c) Ideally: find the original patch on the mailing list and do a
reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/SubmitChecklist when testing your code ***
See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this
The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
------------------------------------------------------
Subject: cgroups: don't attach task to subsystem if migration failed
From: Ben Blum <bblum@andrew.cmu.edu>
If a task has exited to the point it has called cgroup_exit() already,
then we can't migrate it to another cgroup anymore.
This can happen when we are attaching a task to a new cgroup between the
call to ->can_attach_task() on subsystems and the migration that is
eventually tried in cgroup_task_migrate().
In this case cgroup_task_migrate() returns -ESRCH and we don't want to
attach the task to the subsystems because the attachment to the new cgroup
itself failed.
Fix this by only calling ->attach_task() on the subsystems if the cgroup
migration succeeded.
Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Ben Blum <bblum@andrew.cmu.edu>
Acked-by: Paul Menage <paul@paulmenage.org>
Cc: Li Zefan <lizf@cn.fujitsu.com>
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
kernel/cgroup.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff -puN kernel/cgroup.c~cgroups-fix-ordering-of-calls-in-cgroup_attach_proc kernel/cgroup.c
--- a/kernel/cgroup.c~cgroups-fix-ordering-of-calls-in-cgroup_attach_proc
+++ a/kernel/cgroup.c
@@ -2135,14 +2135,17 @@ int cgroup_attach_proc(struct cgroup *cg
oldcgrp = task_cgroup_from_root(tsk, root);
if (cgrp == oldcgrp)
continue;
- /* attach each task to each subsystem */
- for_each_subsys(root, ss) {
- if (ss->attach_task)
- ss->attach_task(cgrp, tsk);
- }
/* if the thread is PF_EXITING, it can just get skipped. */
retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, true);
- BUG_ON(retval != 0 && retval != -ESRCH);
+ if (retval == 0) {
+ /* attach each task to each subsystem */
+ for_each_subsys(root, ss) {
+ if (ss->attach_task)
+ ss->attach_task(cgrp, tsk);
+ }
+ } else {
+ BUG_ON(retval != -ESRCH);
+ }
}
/* nothing is sensitive to fork() after this point. */
_
Patches currently in -mm which might be from bblum@andrew.cmu.edu are
cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch
cgroups-dont-attach-task-to-subsystem-if-migration-failed.patch
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-08-26 19:27 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-25 20:44 + cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch added to -mm tree akpm
2011-08-26 15:12 ` Oleg Nesterov
2011-08-26 15:18 ` Paul Menage
2011-08-26 15:21 ` Tejun Heo
2011-08-26 15:50 ` Oleg Nesterov
2011-08-26 15:38 ` [PATCH v2] cgroups: Don't attach task to subsystem if migration failed Frederic Weisbecker
2011-08-26 17:01 ` Ben Blum
2011-08-26 19:27 + cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch added to -mm tree akpm
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.