diff mbox

[6/6] cgroup: kill subsys->can_attach_task(), pre_attach() and attach_task()

Message ID 1314312192-26885-7-git-send-email-tj@kernel.org (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Tejun Heo Aug. 25, 2011, 10:43 p.m. UTC 
These three methods are no longer used.  Kill them.

Signed-off-by: Tejun Heo <tj@kernel.org>
Acked-by: Paul Menage <paul@paulmenage.org>
Cc: Li Zefan <lizf@cn.fujitsu.com>
---
 Documentation/cgroups/cgroups.txt |   20 --------------
 include/linux/cgroup.h            |    3 --
 kernel/cgroup.c                   |   53 +++---------------------------------
 3 files changed, 5 insertions(+), 71 deletions(-)

Comments

KAMEZAWA Hiroyuki Aug. 26, 2011, 4:20 a.m. UTC | #1 
On Fri, 26 Aug 2011 00:43:12 +0200
Tejun Heo <tj@kernel.org> wrote:

> These three methods are no longer used.  Kill them.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Acked-by: Paul Menage <paul@paulmenage.org>
> Cc: Li Zefan <lizf@cn.fujitsu.com>

Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Frederic Weisbecker Aug. 30, 2011, 8:10 p.m. UTC | #2 
On Fri, Aug 26, 2011 at 12:43:12AM +0200, Tejun Heo wrote:
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index f3c7f7a..ce765ec 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2075,7 +2064,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>  {
>  	int retval, i, group_size, nr_migrating_tasks;
>  	struct cgroup_subsys *ss, *failed_ss = NULL;
> -	bool cancel_failed_ss = false;
>  	/* guaranteed to be initialized later, but the compiler needs this */
>  	struct css_set *oldcg;
>  	struct cgroupfs_root *root = cgrp->root;
> @@ -2166,21 +2154,6 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>  				goto out_cancel_attach;
>  			}
>  		}
> -		/* a callback to be run on every thread in the threadgroup. */
> -		if (ss->can_attach_task) {
> -			/* run on each task in the threadgroup. */
> -			for (i = 0; i < group_size; i++) {
> -				tc = flex_array_get(group, i);
> -				if (tc->cgrp == cgrp)
> -					continue;
> -				retval = ss->can_attach_task(cgrp, tc->task);
> -				if (retval) {
> -					failed_ss = ss;
> -					cancel_failed_ss = true;
> -					goto out_cancel_attach;
> -				}
> -			}
> -		}
>  	}
>  
>  	/*
> @@ -2217,15 +2190,10 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>  	}
>  
>  	/*
> -	 * step 3: now that we're guaranteed success wrt the css_sets, proceed
> -	 * to move all tasks to the new cgroup, calling ss->attach_task for each
> -	 * one along the way. there are no failure cases after here, so this is
> -	 * the commit point.
> +	 * step 3: now that we're guaranteed success wrt the css_sets,
> +	 * proceed to move all tasks to the new cgroup.  There are no
> +	 * failure cases after here, so this is the commit point.
>  	 */
> -	for_each_subsys(root, ss) {
> -		if (ss->pre_attach)
> -			ss->pre_attach(cgrp);
> -	}
>  	for (i = 0; i < group_size; i++) {
>  		tc = flex_array_get(group, i);
>  		/* leave current thread as it is if it's already there */
> @@ -2235,19 +2203,11 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
>  		/* if the thread is PF_EXITING, it can just get skipped. */
>  		retval = cgroup_task_migrate(cgrp, tc->cgrp, tc->task, true);
>  		BUG_ON(retval != 0 && retval != -ESRCH);
> -
> -		/* attach each task to each subsystem */
> -		for_each_subsys(root, ss) {
> -			if (ss->attach_task)
> -				ss->attach_task(cgrp, tc->task);
> -		}

In order to keep the fix queued in -mm (https://lkml.org/lkml/2011/8/26/262)
the tasks that have failed to migrate should be removed from the iterator
so that they are not included in the batch in ->attach().

>  	}
>  	/* nothing is sensitive to fork() after this point. */
>  
>  	/*
> -	 * step 4: do expensive, non-thread-specific subsystem callbacks.
> -	 * TODO: if ever a subsystem needs to know the oldcgrp for each task
> -	 * being moved, this call will need to be reworked to communicate that.
> +	 * step 4: do subsystem attach callbacks.
>  	 */
>  	for_each_subsys(root, ss) {
>  		if (ss->attach)
> @@ -2271,11 +2231,8 @@ out_cancel_attach:
>  	/* same deal as in cgroup_attach_task */
>  	if (retval) {
>  		for_each_subsys(root, ss) {
> -			if (ss == failed_ss) {
> -				if (cancel_failed_ss && ss->cancel_attach)
> -					ss->cancel_attach(ss, cgrp, &tset);
> +			if (ss == failed_ss)
>  				break;
> -			}
>  			if (ss->cancel_attach)
>  				ss->cancel_attach(ss, cgrp, &tset);
>  		}
> -- 
> 1.7.6
>
Tejun Heo Aug. 31, 2011, 7:03 a.m. UTC | #3 
Hello, Frederic.

On Tue, Aug 30, 2011 at 10:10:32PM +0200, Frederic Weisbecker wrote:
> In order to keep the fix queued in -mm (https://lkml.org/lkml/2011/8/26/262)
> the tasks that have failed to migrate should be removed from the iterator
> so that they are not included in the batch in ->attach().

I don't think that's a good approach.  It breaks the symmetry when
calling different callbacks.  What if ->can_attach() allocates
per-task resources and the task exits in the middle?  I think we
better lock down fork/exit/exec.  I'll send patches but I'm currently
moving / traveling w/ limited access to my toys so it might take some
time.

Thanks.
Frederic Weisbecker Aug. 31, 2011, 1:42 p.m. UTC | #4 
On Wed, Aug 31, 2011 at 09:03:13AM +0200, Tejun Heo wrote:
> Hello, Frederic.
> 
> On Tue, Aug 30, 2011 at 10:10:32PM +0200, Frederic Weisbecker wrote:
> > In order to keep the fix queued in -mm (https://lkml.org/lkml/2011/8/26/262)
> > the tasks that have failed to migrate should be removed from the iterator
> > so that they are not included in the batch in ->attach().
> 
> I don't think that's a good approach.  It breaks the symmetry when
> calling different callbacks.  What if ->can_attach() allocates
> per-task resources and the task exits in the middle?  I think we
> better lock down fork/exit/exec.  I'll send patches but I'm currently
> moving / traveling w/ limited access to my toys so it might take some
> time.

My task counter subsystem patchset brings a cancel_attach_task() callback
that cancels can_attach_task() effects.

I thought that rebased on top of your patch it's going to be merged inside
cancel_attach() but OTOH we can't cancel the effect of failed migration
on a thread that way.

May be we need to keep a cancel_attach_task() just for that purpose?
Tejun Heo Sept. 1, 2011, 11:22 a.m. UTC | #5 
Hello,

On Wed, Aug 31, 2011 at 03:42:24PM +0200, Frederic Weisbecker wrote:
> My task counter subsystem patchset brings a cancel_attach_task() callback
> that cancels can_attach_task() effects.
> 
> I thought that rebased on top of your patch it's going to be merged inside
> cancel_attach() but OTOH we can't cancel the effect of failed migration
> on a thread that way.
> 
> May be we need to keep a cancel_attach_task() just for that purpose?

We can do that but I think that becomes a bit too complex and fragile.
That path won't be traveled unless it races against exit.  Bugs will
be difficult to detect and reproduce.  In this respect, the current
code already seems racy.  ->can_attach (or other methods in the attach
path) and ->exit can race each other and I don't think all subsystems
handle that properly.

IMHO the right thing to do here is simplifying synchronization rules
so that nothing else happens while migration is in progress.

Thanks.
Frederic Weisbecker Sept. 1, 2011, 12:58 p.m. UTC | #6 
On Thu, Sep 01, 2011 at 08:22:21PM +0900, Tejun Heo wrote:
> Hello,
> 
> On Wed, Aug 31, 2011 at 03:42:24PM +0200, Frederic Weisbecker wrote:
> > My task counter subsystem patchset brings a cancel_attach_task() callback
> > that cancels can_attach_task() effects.
> > 
> > I thought that rebased on top of your patch it's going to be merged inside
> > cancel_attach() but OTOH we can't cancel the effect of failed migration
> > on a thread that way.
> > 
> > May be we need to keep a cancel_attach_task() just for that purpose?
> 
> We can do that but I think that becomes a bit too complex and fragile.
> That path won't be traveled unless it races against exit.  Bugs will
> be difficult to detect and reproduce.  In this respect, the current
> code already seems racy.  ->can_attach (or other methods in the attach
> path) and ->exit can race each other and I don't think all subsystems
> handle that properly.

I guess subsystems don't care about that currently, although I haven't
checked. But the task counter will need this per thread cancellation in
migration failure, without the need for any synchronization between
can_attach, attach and exit.

Now if we want to solve this with a synchronization there, either
we task_lock() every tasks in the group in the beginning of cgroup_attach_proc().
But that's not very nice. Or we use cgroup_mutex on cgroup_exit() exit,
but that's even worse.

I guess we need the leader->sighand->siglock on both cgroup_attach_proc()
and cgroup_exit(). Besides we may have more reasons to have that:

https://lkml.org/lkml/2011/8/15/342

> 
> IMHO the right thing to do here is simplifying synchronization rules
> so that nothing else happens while migration is in progress.
> 
> Thanks.
> 
> -- 
> tejun
diff mbox

Patch

diff --git a/Documentation/cgroups/cgroups.txt b/Documentation/cgroups/cgroups.txt
index bf5d6c9..eb1b609 100644
--- a/Documentation/cgroups/cgroups.txt
+++ b/Documentation/cgroups/cgroups.txt
@@ -615,13 +615,6 @@  fork. If this method returns 0 (success) then this should remain valid
 while the caller holds cgroup_mutex and it is ensured that either
 attach() or cancel_attach() will be called in future.
 
-int can_attach_task(struct cgroup *cgrp, struct task_struct *tsk);
-(cgroup_mutex held by caller)
-
-As can_attach, but for operations that must be run once per task to be
-attached (possibly many when using cgroup_attach_proc). Called after
-can_attach.
-
 void cancel_attach(struct cgroup_subsys *ss, struct cgroup *cgrp,
 		   struct cgroup_taskset *tset)
 (cgroup_mutex held by caller)
@@ -632,12 +625,6 @@  function, so that the subsystem can implement a rollback. If not, not necessary.
 This will be called only about subsystems whose can_attach() operation have
 succeeded. The parameters are identical to can_attach().
 
-void pre_attach(struct cgroup *cgrp);
-(cgroup_mutex held by caller)
-
-For any non-per-thread attachment work that needs to happen before
-attach_task. Needed by cpuset.
-
 void attach(struct cgroup_subsys *ss, struct cgroup *cgrp,
 	    struct cgroup_taskset *tset)
 (cgroup_mutex held by caller)
@@ -646,13 +633,6 @@  Called after the task has been attached to the cgroup, to allow any
 post-attachment activity that requires memory allocations or blocking.
 The parameters are identical to can_attach().
 
-void attach_task(struct cgroup *cgrp, struct task_struct *tsk);
-(cgroup_mutex held by caller)
-
-As attach, but for operations that must be run once per task to be attached,
-like can_attach_task. Called before attach. Currently does not support any
-subsystem that might need the old_cgrp for every thread in the group.
-
 void fork(struct cgroup_subsy *ss, struct task_struct *task)
 
 Called when a task is forked into a cgroup.
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 2470c8e..5659d37 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -490,11 +490,8 @@  struct cgroup_subsys {
 	void (*destroy)(struct cgroup_subsys *ss, struct cgroup *cgrp);
 	int (*can_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
 			  struct cgroup_taskset *tset);
-	int (*can_attach_task)(struct cgroup *cgrp, struct task_struct *tsk);
 	void (*cancel_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
 			      struct cgroup_taskset *tset);
-	void (*pre_attach)(struct cgroup *cgrp);
-	void (*attach_task)(struct cgroup *cgrp, struct task_struct *tsk);
 	void (*attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
 		       struct cgroup_taskset *tset);
 	void (*fork)(struct cgroup_subsys *ss, struct task_struct *task);
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index f3c7f7a..ce765ec 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1926,13 +1926,6 @@  int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 				goto out;
 			}
 		}
-		if (ss->can_attach_task) {
-			retval = ss->can_attach_task(cgrp, tsk);
-			if (retval) {
-				failed_ss = ss;
-				goto out;
-			}
-		}
 	}
 
 	retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, false);
@@ -1940,10 +1933,6 @@  int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 		goto out;
 
 	for_each_subsys(root, ss) {
-		if (ss->pre_attach)
-			ss->pre_attach(cgrp);
-		if (ss->attach_task)
-			ss->attach_task(cgrp, tsk);
 		if (ss->attach)
 			ss->attach(ss, cgrp, &tset);
 	}
@@ -2075,7 +2064,6 @@  int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 {
 	int retval, i, group_size, nr_migrating_tasks;
 	struct cgroup_subsys *ss, *failed_ss = NULL;
-	bool cancel_failed_ss = false;
 	/* guaranteed to be initialized later, but the compiler needs this */
 	struct css_set *oldcg;
 	struct cgroupfs_root *root = cgrp->root;
@@ -2166,21 +2154,6 @@  int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 				goto out_cancel_attach;
 			}
 		}
-		/* a callback to be run on every thread in the threadgroup. */
-		if (ss->can_attach_task) {
-			/* run on each task in the threadgroup. */
-			for (i = 0; i < group_size; i++) {
-				tc = flex_array_get(group, i);
-				if (tc->cgrp == cgrp)
-					continue;
-				retval = ss->can_attach_task(cgrp, tc->task);
-				if (retval) {
-					failed_ss = ss;
-					cancel_failed_ss = true;
-					goto out_cancel_attach;
-				}
-			}
-		}
 	}
 
 	/*
@@ -2217,15 +2190,10 @@  int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	}
 
 	/*
-	 * step 3: now that we're guaranteed success wrt the css_sets, proceed
-	 * to move all tasks to the new cgroup, calling ss->attach_task for each
-	 * one along the way. there are no failure cases after here, so this is
-	 * the commit point.
+	 * step 3: now that we're guaranteed success wrt the css_sets,
+	 * proceed to move all tasks to the new cgroup.  There are no
+	 * failure cases after here, so this is the commit point.
 	 */
-	for_each_subsys(root, ss) {
-		if (ss->pre_attach)
-			ss->pre_attach(cgrp);
-	}
 	for (i = 0; i < group_size; i++) {
 		tc = flex_array_get(group, i);
 		/* leave current thread as it is if it's already there */
@@ -2235,19 +2203,11 @@  int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 		/* if the thread is PF_EXITING, it can just get skipped. */
 		retval = cgroup_task_migrate(cgrp, tc->cgrp, tc->task, true);
 		BUG_ON(retval != 0 && retval != -ESRCH);
-
-		/* attach each task to each subsystem */
-		for_each_subsys(root, ss) {
-			if (ss->attach_task)
-				ss->attach_task(cgrp, tc->task);
-		}
 	}
 	/* nothing is sensitive to fork() after this point. */
 
 	/*
-	 * step 4: do expensive, non-thread-specific subsystem callbacks.
-	 * TODO: if ever a subsystem needs to know the oldcgrp for each task
-	 * being moved, this call will need to be reworked to communicate that.
+	 * step 4: do subsystem attach callbacks.
 	 */
 	for_each_subsys(root, ss) {
 		if (ss->attach)
@@ -2271,11 +2231,8 @@  out_cancel_attach:
 	/* same deal as in cgroup_attach_task */
 	if (retval) {
 		for_each_subsys(root, ss) {
-			if (ss == failed_ss) {
-				if (cancel_failed_ss && ss->cancel_attach)
-					ss->cancel_attach(ss, cgrp, &tset);
+			if (ss == failed_ss)
 				break;
-			}
 			if (ss->cancel_attach)
 				ss->cancel_attach(ss, cgrp, &tset);
 		}