* [PATCH 1/4] cgroup: fix incorrect destination cgroup in cgroup_update_dfl_csses()
2016-03-03 17:19 [PATCHSET cgroup/for-4.6] cgroup: update migration destination cgroup handling Tejun Heo
@ 2016-03-03 17:19 ` Tejun Heo
2016-03-03 17:20 ` [PATCH 2/4] cgroup: move migration destination verification out of cgroup_migrate_prepare_dst() Tejun Heo
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Tejun Heo @ 2016-03-03 17:19 UTC (permalink / raw)
To: lizefan, hannes; +Cc: cgroups, linux-kernel, kernel-team, Tejun Heo
cgroup_update_dfl_csses() should move each task in the subtree to
self; however, it was incorrectly calling cgroup_migrate_add_src()
with the root of the subtree as @dst_cgrp. Fortunately,
cgroup_migrate_add_src() currently uses @dst_cgrp only to determine
the hierarchy and the bug doesn't cause any actual breakages. Fix it.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
kernel/cgroup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index c63fce0..50879aa 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2901,7 +2901,7 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
struct cgrp_cset_link *link;
list_for_each_entry(link, &dsct->cset_links, cset_link)
- cgroup_migrate_add_src(link->cset, cgrp,
+ cgroup_migrate_add_src(link->cset, dsct,
&preloaded_csets);
}
spin_unlock_bh(&css_set_lock);
--
2.5.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/4] cgroup: move migration destination verification out of cgroup_migrate_prepare_dst()
2016-03-03 17:19 [PATCHSET cgroup/for-4.6] cgroup: update migration destination cgroup handling Tejun Heo
2016-03-03 17:19 ` [PATCH 1/4] cgroup: fix incorrect destination cgroup in cgroup_update_dfl_csses() Tejun Heo
@ 2016-03-03 17:20 ` Tejun Heo
2016-03-03 17:20 ` [PATCH 3/4] cgroup: make cgroup[_taskset]_migrate() take cgroup_root instead of cgroup Tejun Heo
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Tejun Heo @ 2016-03-03 17:20 UTC (permalink / raw)
To: lizefan, hannes; +Cc: cgroups, linux-kernel, kernel-team, Tejun Heo
cgroup_migrate_prepare_dst() verifies whether the destination cgroup
is allowable; however, the test doesn't really belong there. It's too
deep and common in the stack and as a result the test itself is gated
by another test.
Separate the test out into cgroup_may_migrate_to() and update
cgroup_attach_task() and cgroup_transfer_tasks() to perform the test
directly. This doesn't cause any behavior differences.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
kernel/cgroup.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 50879aa..8a02076 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2444,6 +2444,20 @@ static int cgroup_taskset_migrate(struct cgroup_taskset *tset,
}
/**
+ * cgroup_may_migrate_to - verify whether a cgroup can be migration destination
+ * @dst_cgrp: destination cgroup to test
+ *
+ * On the default hierarchy, except for the root, subtree_control must be
+ * zero for migration destination cgroups with tasks so that child cgroups
+ * don't compete against tasks.
+ */
+static bool cgroup_may_migrate_to(struct cgroup *dst_cgrp)
+{
+ return !cgroup_on_dfl(dst_cgrp) || !cgroup_parent(dst_cgrp) ||
+ !dst_cgrp->subtree_control;
+}
+
+/**
* cgroup_migrate_finish - cleanup after attach
* @preloaded_csets: list of preloaded css_sets
*
@@ -2529,14 +2543,6 @@ static int cgroup_migrate_prepare_dst(struct cgroup *dst_cgrp,
lockdep_assert_held(&cgroup_mutex);
- /*
- * Except for the root, subtree_control must be zero for a cgroup
- * with tasks so that child cgroups don't compete against tasks.
- */
- if (dst_cgrp && cgroup_on_dfl(dst_cgrp) && cgroup_parent(dst_cgrp) &&
- dst_cgrp->subtree_control)
- return -EBUSY;
-
/* look up the dst cset for each src cset and link it to src */
list_for_each_entry_safe(src_cset, tmp_cset, preloaded_csets, mg_preload_node) {
struct css_set *dst_cset;
@@ -2634,6 +2640,9 @@ static int cgroup_attach_task(struct cgroup *dst_cgrp,
struct task_struct *task;
int ret;
+ if (!cgroup_may_migrate_to(dst_cgrp))
+ return -EBUSY;
+
/* look up all src csets */
spin_lock_bh(&css_set_lock);
rcu_read_lock();
@@ -4136,6 +4145,9 @@ int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from)
struct task_struct *task;
int ret;
+ if (!cgroup_may_migrate_to(to))
+ return -EBUSY;
+
mutex_lock(&cgroup_mutex);
/* all tasks in @from are being moved, all csets are source */
--
2.5.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/4] cgroup: make cgroup[_taskset]_migrate() take cgroup_root instead of cgroup
2016-03-03 17:19 [PATCHSET cgroup/for-4.6] cgroup: update migration destination cgroup handling Tejun Heo
2016-03-03 17:19 ` [PATCH 1/4] cgroup: fix incorrect destination cgroup in cgroup_update_dfl_csses() Tejun Heo
2016-03-03 17:20 ` [PATCH 2/4] cgroup: move migration destination verification out of cgroup_migrate_prepare_dst() Tejun Heo
@ 2016-03-03 17:20 ` Tejun Heo
2016-03-03 17:20 ` [PATCH 4/4] cgroup: use css_set->mg_dst_cgrp for the migration target cgroup Tejun Heo
2016-03-08 16:50 ` [PATCHSET cgroup/for-4.6] cgroup: update migration destination cgroup handling Tejun Heo
4 siblings, 0 replies; 6+ messages in thread
From: Tejun Heo @ 2016-03-03 17:20 UTC (permalink / raw)
To: lizefan, hannes; +Cc: cgroups, linux-kernel, kernel-team, Tejun Heo
On the default hierarchy, a migration can be multi-source and/or
multi-destination. cgroup_taskest_migrate() used to incorrectly
assume single destination cgroup but the bug has been fixed by
1f7dd3e5a6e4 ("cgroup: fix handling of multi-destination migration
from subtree_control enabling").
Since the commit, @dst_cgrp to cgroup[_taskset]_migrate() is only used
to determine which subsystems are affected or which cgroup_root the
migration is taking place in. As such, @dst_cgrp is misleading. This
patch replaces @dst_cgrp with @root.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
kernel/cgroup.c | 70 ++++++++++++++++++++++++++++-----------------------------
1 file changed, 35 insertions(+), 35 deletions(-)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 8a02076..5dd7613 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2355,38 +2355,38 @@ struct task_struct *cgroup_taskset_next(struct cgroup_taskset *tset,
}
/**
- * cgroup_taskset_migrate - migrate a taskset to a cgroup
+ * cgroup_taskset_migrate - migrate a taskset
* @tset: taget taskset
- * @dst_cgrp: destination cgroup
+ * @root: cgroup root the migration is taking place on
*
- * Migrate tasks in @tset to @dst_cgrp. This function fails iff one of the
- * ->can_attach callbacks fails and guarantees that either all or none of
- * the tasks in @tset are migrated. @tset is consumed regardless of
- * success.
+ * Migrate tasks in @tset as setup by migration preparation functions.
+ * This function fails iff one of the ->can_attach callbacks fails and
+ * guarantees that either all or none of the tasks in @tset are migrated.
+ * @tset is consumed regardless of success.
*/
static int cgroup_taskset_migrate(struct cgroup_taskset *tset,
- struct cgroup *dst_cgrp)
+ struct cgroup_root *root)
{
- struct cgroup_subsys_state *css, *failed_css = NULL;
+ struct cgroup_subsys *ss;
struct task_struct *task, *tmp_task;
struct css_set *cset, *tmp_cset;
- int i, ret;
+ int ssid, failed_ssid, ret;
/* methods shouldn't be called if no task is actually migrating */
if (list_empty(&tset->src_csets))
return 0;
/* check that we can legitimately attach to the cgroup */
- for_each_e_css(css, i, dst_cgrp) {
- if (css->ss->can_attach) {
- tset->ssid = i;
- ret = css->ss->can_attach(tset);
+ do_each_subsys_mask(ss, ssid, root->subsys_mask) {
+ if (ss->can_attach) {
+ tset->ssid = ssid;
+ ret = ss->can_attach(tset);
if (ret) {
- failed_css = css;
+ failed_ssid = ssid;
goto out_cancel_attach;
}
}
- }
+ } while_each_subsys_mask();
/*
* Now that we're guaranteed success, proceed to move all tasks to
@@ -2413,25 +2413,25 @@ static int cgroup_taskset_migrate(struct cgroup_taskset *tset,
*/
tset->csets = &tset->dst_csets;
- for_each_e_css(css, i, dst_cgrp) {
- if (css->ss->attach) {
- tset->ssid = i;
- css->ss->attach(tset);
+ do_each_subsys_mask(ss, ssid, root->subsys_mask) {
+ if (ss->attach) {
+ tset->ssid = ssid;
+ ss->attach(tset);
}
- }
+ } while_each_subsys_mask();
ret = 0;
goto out_release_tset;
out_cancel_attach:
- for_each_e_css(css, i, dst_cgrp) {
- if (css == failed_css)
+ do_each_subsys_mask(ss, ssid, root->subsys_mask) {
+ if (ssid == failed_ssid)
break;
- if (css->ss->cancel_attach) {
- tset->ssid = i;
- css->ss->cancel_attach(tset);
+ if (ss->cancel_attach) {
+ tset->ssid = ssid;
+ ss->cancel_attach(tset);
}
- }
+ } while_each_subsys_mask();
out_release_tset:
spin_lock_bh(&css_set_lock);
list_splice_init(&tset->dst_csets, &tset->src_csets);
@@ -2586,11 +2586,11 @@ static int cgroup_migrate_prepare_dst(struct cgroup *dst_cgrp,
* cgroup_migrate - migrate a process or task to a cgroup
* @leader: the leader of the process or the task to migrate
* @threadgroup: whether @leader points to the whole process or a single task
- * @cgrp: the destination cgroup
+ * @root: cgroup root migration is taking place on
*
- * Migrate a process or task denoted by @leader to @cgrp. If migrating a
- * process, the caller must be holding cgroup_threadgroup_rwsem. The
- * caller is also responsible for invoking cgroup_migrate_add_src() and
+ * Migrate a process or task denoted by @leader. If migrating a process,
+ * the caller must be holding cgroup_threadgroup_rwsem. The caller is also
+ * responsible for invoking cgroup_migrate_add_src() and
* cgroup_migrate_prepare_dst() on the targets before invoking this
* function and following up with cgroup_migrate_finish().
*
@@ -2601,7 +2601,7 @@ static int cgroup_migrate_prepare_dst(struct cgroup *dst_cgrp,
* actually starting migrating.
*/
static int cgroup_migrate(struct task_struct *leader, bool threadgroup,
- struct cgroup *cgrp)
+ struct cgroup_root *root)
{
struct cgroup_taskset tset = CGROUP_TASKSET_INIT(tset);
struct task_struct *task;
@@ -2622,7 +2622,7 @@ static int cgroup_migrate(struct task_struct *leader, bool threadgroup,
rcu_read_unlock();
spin_unlock_bh(&css_set_lock);
- return cgroup_taskset_migrate(&tset, cgrp);
+ return cgroup_taskset_migrate(&tset, root);
}
/**
@@ -2659,7 +2659,7 @@ static int cgroup_attach_task(struct cgroup *dst_cgrp,
/* prepare dst csets and commit */
ret = cgroup_migrate_prepare_dst(dst_cgrp, &preloaded_csets);
if (!ret)
- ret = cgroup_migrate(leader, threadgroup, dst_cgrp);
+ ret = cgroup_migrate(leader, threadgroup, dst_cgrp->root);
cgroup_migrate_finish(&preloaded_csets);
return ret;
@@ -2934,7 +2934,7 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
}
spin_unlock_bh(&css_set_lock);
- ret = cgroup_taskset_migrate(&tset, cgrp);
+ ret = cgroup_taskset_migrate(&tset, cgrp->root);
out_finish:
cgroup_migrate_finish(&preloaded_csets);
percpu_up_write(&cgroup_threadgroup_rwsem);
@@ -4172,7 +4172,7 @@ int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from)
css_task_iter_end(&it);
if (task) {
- ret = cgroup_migrate(task, false, to);
+ ret = cgroup_migrate(task, false, to->root);
put_task_struct(task);
}
} while (task && !ret);
--
2.5.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 4/4] cgroup: use css_set->mg_dst_cgrp for the migration target cgroup
2016-03-03 17:19 [PATCHSET cgroup/for-4.6] cgroup: update migration destination cgroup handling Tejun Heo
` (2 preceding siblings ...)
2016-03-03 17:20 ` [PATCH 3/4] cgroup: make cgroup[_taskset]_migrate() take cgroup_root instead of cgroup Tejun Heo
@ 2016-03-03 17:20 ` Tejun Heo
2016-03-08 16:50 ` [PATCHSET cgroup/for-4.6] cgroup: update migration destination cgroup handling Tejun Heo
4 siblings, 0 replies; 6+ messages in thread
From: Tejun Heo @ 2016-03-03 17:20 UTC (permalink / raw)
To: lizefan, hannes; +Cc: cgroups, linux-kernel, kernel-team, Tejun Heo
Migration can be multi-target on the default hierarchy when a
controller is enabled - processes belonging to each child cgroup have
to be moved to the child cgroup itself to refresh css association.
This isn't a problem for cgroup_migrate_add_src() as each source
css_set still maps to single source and target cgroups; however,
cgroup_migrate_prepare_dst() is called once after all source css_sets
are added and thus might not have a single destination cgroup. This
is currently worked around by specifying NULL for @dst_cgrp and using
the source's default cgroup as destination as the only multi-target
migration in use is self-targetting. While this works, it's subtle
and clunky.
As all taget cgroups are already specified while preparing the source
css_sets, this clunkiness can easily be removed by recording the
target cgroup in each source css_set. This patch adds
css_set->mg_dst_cgrp which is recorded on cgroup_migrate_src() and
used by cgroup_migrate_prepare_dst(). This also makes migration code
ready for arbitrary multi-target migration.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
include/linux/cgroup-defs.h | 9 +++++----
kernel/cgroup.c | 26 +++++++++++++-------------
2 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index aae8c94..590291d 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -191,12 +191,13 @@ struct css_set {
/*
* If this cset is acting as the source of migration the following
- * two fields are set. mg_src_cgrp is the source cgroup of the
- * on-going migration and mg_dst_cset is the destination cset the
- * target tasks on this cset should be migrated to. Protected by
- * cgroup_mutex.
+ * two fields are set. mg_src_cgrp and mg_dst_cgrp are
+ * respectively the source and destination cgroups of the on-going
+ * migration. mg_dst_cset is the destination cset the target tasks
+ * on this cset should be migrated to. Protected by cgroup_mutex.
*/
struct cgroup *mg_src_cgrp;
+ struct cgroup *mg_dst_cgrp;
struct css_set *mg_dst_cset;
/*
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 5dd7613..fbd3e99 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2473,6 +2473,7 @@ static void cgroup_migrate_finish(struct list_head *preloaded_csets)
spin_lock_bh(&css_set_lock);
list_for_each_entry_safe(cset, tmp_cset, preloaded_csets, mg_preload_node) {
cset->mg_src_cgrp = NULL;
+ cset->mg_dst_cgrp = NULL;
cset->mg_dst_cset = NULL;
list_del_init(&cset->mg_preload_node);
put_css_set_locked(cset);
@@ -2511,32 +2512,31 @@ static void cgroup_migrate_add_src(struct css_set *src_cset,
return;
WARN_ON(src_cset->mg_src_cgrp);
+ WARN_ON(src_cset->mg_dst_cgrp);
WARN_ON(!list_empty(&src_cset->mg_tasks));
WARN_ON(!list_empty(&src_cset->mg_node));
src_cset->mg_src_cgrp = src_cgrp;
+ src_cset->mg_dst_cgrp = dst_cgrp;
get_css_set(src_cset);
list_add(&src_cset->mg_preload_node, preloaded_csets);
}
/**
* cgroup_migrate_prepare_dst - prepare destination css_sets for migration
- * @dst_cgrp: the destination cgroup (may be %NULL)
* @preloaded_csets: list of preloaded source css_sets
*
- * Tasks are about to be moved to @dst_cgrp and all the source css_sets
- * have been preloaded to @preloaded_csets. This function looks up and
- * pins all destination css_sets, links each to its source, and append them
- * to @preloaded_csets. If @dst_cgrp is %NULL, the destination of each
- * source css_set is assumed to be its cgroup on the default hierarchy.
+ * Tasks are about to be moved and all the source css_sets have been
+ * preloaded to @preloaded_csets. This function looks up and pins all
+ * destination css_sets, links each to its source, and append them to
+ * @preloaded_csets.
*
* This function must be called after cgroup_migrate_add_src() has been
* called on each migration source css_set. After migration is performed
* using cgroup_migrate(), cgroup_migrate_finish() must be called on
* @preloaded_csets.
*/
-static int cgroup_migrate_prepare_dst(struct cgroup *dst_cgrp,
- struct list_head *preloaded_csets)
+static int cgroup_migrate_prepare_dst(struct list_head *preloaded_csets)
{
LIST_HEAD(csets);
struct css_set *src_cset, *tmp_cset;
@@ -2547,8 +2547,7 @@ static int cgroup_migrate_prepare_dst(struct cgroup *dst_cgrp,
list_for_each_entry_safe(src_cset, tmp_cset, preloaded_csets, mg_preload_node) {
struct css_set *dst_cset;
- dst_cset = find_css_set(src_cset,
- dst_cgrp ?: src_cset->dfl_cgrp);
+ dst_cset = find_css_set(src_cset, src_cset->mg_dst_cgrp);
if (!dst_cset)
goto err;
@@ -2561,6 +2560,7 @@ static int cgroup_migrate_prepare_dst(struct cgroup *dst_cgrp,
*/
if (src_cset == dst_cset) {
src_cset->mg_src_cgrp = NULL;
+ src_cset->mg_dst_cgrp = NULL;
list_del_init(&src_cset->mg_preload_node);
put_css_set(src_cset);
put_css_set(dst_cset);
@@ -2657,7 +2657,7 @@ static int cgroup_attach_task(struct cgroup *dst_cgrp,
spin_unlock_bh(&css_set_lock);
/* prepare dst csets and commit */
- ret = cgroup_migrate_prepare_dst(dst_cgrp, &preloaded_csets);
+ ret = cgroup_migrate_prepare_dst(&preloaded_csets);
if (!ret)
ret = cgroup_migrate(leader, threadgroup, dst_cgrp->root);
@@ -2916,7 +2916,7 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
spin_unlock_bh(&css_set_lock);
/* NULL dst indicates self on default hierarchy */
- ret = cgroup_migrate_prepare_dst(NULL, &preloaded_csets);
+ ret = cgroup_migrate_prepare_dst(&preloaded_csets);
if (ret)
goto out_finish;
@@ -4156,7 +4156,7 @@ int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from)
cgroup_migrate_add_src(link->cset, to, &preloaded_csets);
spin_unlock_bh(&css_set_lock);
- ret = cgroup_migrate_prepare_dst(to, &preloaded_csets);
+ ret = cgroup_migrate_prepare_dst(&preloaded_csets);
if (ret)
goto out_err;
--
2.5.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCHSET cgroup/for-4.6] cgroup: update migration destination cgroup handling
2016-03-03 17:19 [PATCHSET cgroup/for-4.6] cgroup: update migration destination cgroup handling Tejun Heo
` (3 preceding siblings ...)
2016-03-03 17:20 ` [PATCH 4/4] cgroup: use css_set->mg_dst_cgrp for the migration target cgroup Tejun Heo
@ 2016-03-08 16:50 ` Tejun Heo
4 siblings, 0 replies; 6+ messages in thread
From: Tejun Heo @ 2016-03-08 16:50 UTC (permalink / raw)
To: lizefan, hannes; +Cc: cgroups, linux-kernel, kernel-team
On Thu, Mar 03, 2016 at 12:19:58PM -0500, Tejun Heo wrote:
> cgroup migrations used to always be single destination. There could
> be multiple sources but there was only one destination cgroup. This
> changed with default hierarchy as enabling a controller would move
> processes in child cgroups into separate csses. This was developed
> over time and is working correctly now but the way multiple
> destinations are handled is hacky and fragile. This patchset updates
> migration logic so that multi-destination handling is integral.
>
> This patchset contains the following four patches.
>
> 0001-cgroup-fix-incorrect-destination-cgroup-in-cgroup_up.patch
> 0002-cgroup-move-migration-destination-verification-out-o.patch
> 0003-cgroup-make-cgroup-_taskset-_migrate-take-cgroup_roo.patch
> 0004-cgroup-use-css_set-mg_dst_cgrp-for-the-migration-tar.patch
These changes are mostly mechanical and I want them to be in for-4.6
for a while before the merge window opens up. Applying to
cgroup/for-4.6. In the unlikely case that this breaks something,
let's fix with followup patches.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 6+ messages in thread