cgroup: avoid spurious identity ->*attach() invocations
Tejun Heo
maintainerPATCH 1/1 cgroup: avoid spurious identity ->*attach() invocations
Hello,
On the v2 hierarchy, when controllers are enabled and disabled, other ->*attach() callbacks of other controllers are called spuriously with the same source and destination. While this isn't critical, it's a bit nasty and can lead to temporary double charging on certain controllers. This patchset fixes the issue.
This patchset contains the following three patches.
0001-cgroup-cosmetic-update-to-cgroup_taskset_add.patch 0002-cgroup-track-migration-context-in-cgroup_mgctx.patch 0003-cgroup-call-subsys-attach-only-for-subsystems-which-.patch
0001-0002 restructures migration context tracking so that extra state can be tracked easily. 0003 fixes the spurious ->*attach() invocations.
This patchset is also available in the following git branch.
git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-avoid-identity-attach
diffstat follows.
kernel/cgroup/cgroup-internal.h | 69 +++++++++++++++-- kernel/cgroup/cgroup-v1.c | 10 +- kernel/cgroup/cgroup.c | 161 ++++++++++++++++------------------------ 3 files changed, 135 insertions(+), 105 deletions(-)
Thanks.
Signature · 1 line
tejun
Tejun Heo
maintainerPATCH 1/3 cgroup: cosmetic update to cgroup_taskset_add()
cgroup_taskset_add() was using list_add_tail() when for source csets but list_move_tail() for destination. As the operations are gated by list_empty() test, list_move_tail() is equivalent to list_add_tail() here. Use list_add_tail() too for destination csets too.
This doesn't cause any functional changes.
--- kernel/cgroup/cgroup.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/cgroup/cgroup.c +2 -2
@@ -1980,8 +1980,8 @@ static void cgroup_taskset_add(struct task_struct *task,if (list_empty (&cset->mg_node))list_add_tail (&cset->mg_node, &tset->src_csets);if (list_empty (&cset->mg_dst_cset->mg_node)) -list_move_tail (&cset->mg_dst_cset->mg_node, - &tset->dst_csets); +list_add_tail (&cset->mg_dst_cset->mg_node, + &tset->dst_csets); }/**
Signature · 1 line
2.9.3
Tejun Heo
maintainerPATCH 2/3 cgroup: track migration context in cgroup_mgctx
In reply to Tejun Heocgroup migration is performed in four steps - css_set preloading, addition of target tasks, actual migration, and clean up. A list named preloaded_csets is used to track the preloading. This is a bit too restricted and the code is already depending on the subtlety that all source css_sets appear before destination ones.
Let's create struct cgroup_mgctx which keeps track of everything during migration. Currently, it has separate preload lists for source and destination csets and also embeds cgroup_taskset which is used during the actual migration. This moves struct cgroup_taskset definition to cgroup-internal.h.
This patch doesn't cause any functional changes.
--- kernel/cgroup/cgroup-internal.h | 66 ++++++++++++++++-- kernel/cgroup/cgroup-v1.c | 10 +-- kernel/cgroup/cgroup.c | 144 ++++++++++++++++------------------------ 3 files changed, 122 insertions(+), 98 deletions(-)
kernel/cgroup/cgroup-internal.h +60 -6
@@ -26,6 +26,61 @@ struct cgrp_cset_link {struct list_head cgrp_link; }; +/* used to track tasks and csets during migration */ +struct cgroup_taskset { +/* the src and dst cset list running through cset->mg_node */ +struct list_head src_csets; +struct list_head dst_csets; + +/* the subsys currently being processed */ +int ssid; + +/* +* Fields for cgroup_taskset_*() iteration. +* +* Before migration is committed, the target migration tasks are on +* ->mg_tasks of the csets on ->src_csets. After, on ->mg_tasks of +* the csets on ->dst_csets. ->csets point to either ->src_csets +* or ->dst_csets depending on whether migration is committed. +* +* ->cur_csets and ->cur_task point to the current task position +* during iteration. +*/ +struct list_head *csets; +struct css_set *cur_cset; +struct task_struct *cur_task; +}; + +/* migration context also tracks preloading */ +struct cgroup_mgctx { +/* +* Preloaded source and destination csets. Used to guarantee +* atomic success or failure on actual migration. +*/ +struct list_head preloaded_src_csets; +struct list_head preloaded_dst_csets; + +/* tasks and csets to migrate */ +struct cgroup_taskset tset; +}; + +#define CGROUP_TASKSET_INIT (tset) \ +{ \ + .src_csets = LIST_HEAD_INIT(tset.src_csets), \ + .dst_csets = LIST_HEAD_INIT(tset.dst_csets), \ + .csets = &tset.src_csets, \ +} + +#define CGROUP_MGCTX_INIT (name) \ +{ \ + LIST_HEAD_INIT(name.preloaded_src_csets), \ + LIST_HEAD_INIT(name.preloaded_dst_csets), \ + CGROUP_TASKSET_INIT(name.tset), \ +} + +#define DEFINE_CGROUP_MGCTX (name) \ + struct cgroup_mgctx name = CGROUP_MGCTX_INIT(name) +struct cgroup_sb_opts {u16 subsys_mask;unsigned int flags;
@@ -112,13 +167,12 @@ struct dentry *cgroup_do_mount(struct file_system_type *fs_type, int flags, struct cgroup_namespace *ns);bool cgroup_may_migrate_to (struct cgroup *dst_cgrp); -void cgroup_migrate_finish (struct list_head *preloaded_csets); -void cgroup_migrate_add_src (struct css_set *src_cset, -struct cgroup *dst_cgrp, -struct list_head *preloaded_csets); -int cgroup_migrate_prepare_dst (struct list_head *preloaded_csets); +void cgroup_migrate_finish (struct cgroup_mgctx *mgctx); +void cgroup_migrate_add_src (struct css_set *src_cset,struct cgroup *dst_cgrp, +struct cgroup_mgctx *mgctx); +int cgroup_migrate_prepare_dst (struct cgroup_mgctx *mgctx);int cgroup_migrate (struct task_struct *leader,bool threadgroup, -struct cgroup_root *root); +struct cgroup_mgctx *mgctx,struct cgroup_root *root);int cgroup_attach_task (struct cgroup *dst_cgrp,struct task_struct *leader,bool threadgroup);
kernel/cgroup/cgroup-v1.c +5 -5
@@ -87,7 +87,7 @@ EXPORT_SYMBOL_GPL(cgroup_attach_task_all);*/ int cgroup_transfer_tasks (struct cgroup *to,struct cgroup *from) { -LIST_HEAD (preloaded_csets); +DEFINE_CGROUP_MGCTX (mgctx);struct cgrp_cset_link *link;struct css_task_iter it;struct task_struct *task;
@@ -106,10 +106,10 @@ int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from)/* all tasks in @from are being moved, all csets are source */ spin_lock_irq (&css_set_lock);list_for_each_entry (link, &from->cset_links, cset_link) -cgroup_migrate_add_src (link->cset, to, &preloaded_csets); +cgroup_migrate_add_src (link->cset, to, &mgctx);spin_unlock_irq (&css_set_lock); - ret =cgroup_migrate_prepare_dst (&preloaded_csets); + ret =cgroup_migrate_prepare_dst (&mgctx);if (ret) goto out_err;
@@ -125,14 +125,14 @@ int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from)css_task_iter_end (&it);if (task) { - ret =cgroup_migrate (task, false, to->root); + ret =cgroup_migrate (task, false, &mgctx, to->root);if (!ret)trace_cgroup_transfer_tasks (to, task, false);put_task_struct (task); } }while (task && !ret); out_err: -cgroup_migrate_finish (&preloaded_csets); +cgroup_migrate_finish (&mgctx);percpu_up_write (&cgroup_threadgroup_rwsem);mutex_unlock (&cgroup_mutex);return ret;
kernel/cgroup/cgroup.c +57 -87
318 lines not on this page · raw message
Signature · 1 line
2.9.3
Tejun Heo
maintainerPATCH 3/3 cgroup: call subsys->*attach() only for subsystems which are actually affected by migration
In reply to Tejun HeoCurrently, subsys->*attach() callbacks are called for all subsystems which are attached to the hierarchy on which the migration is taking place.
With cgroup_migrate_prepare_dst() filtering out identity migrations, v1 hierarchies can avoid spurious ->*attach() callback invocations where the source and destination csses are identical; however, this isn't enough on v2 as only a subset of the attached controllers can be affected on controller enable/disable.
While spurious ->*attach() invocations aren't critically broken, they're unnecessary overhead and can lead to temporary overcharges on certain controllers. Fix it by tracking which subsystems are affected by a migration and invoking ->*attach() callbacks only on those subsystems.
--- kernel/cgroup/cgroup-internal.h | 5 ++++- kernel/cgroup/cgroup-v1.c | 2 +- kernel/cgroup/cgroup.c | 25 ++++++++++++++----------- 3 files changed, 19 insertions(+), 13 deletions(-)
kernel/cgroup/cgroup-internal.h +4 -1
@@ -62,6 +62,9 @@ struct cgroup_mgctx {/* tasks and csets to migrate */ struct cgroup_taskset tset; + +/* subsystems affected by migration */ +u16 ss_mask; };#define CGROUP_TASKSET_INIT (tset) \
@@ -172,7 +175,7 @@ void cgroup_migrate_add_src(struct css_set *src_cset, struct cgroup *dst_cgrp, struct cgroup_mgctx *mgctx);int cgroup_migrate_prepare_dst (struct cgroup_mgctx *mgctx);int cgroup_migrate (struct task_struct *leader,bool threadgroup, -struct cgroup_mgctx *mgctx,struct cgroup_root *root); +struct cgroup_mgctx *mgctx);int cgroup_attach_task (struct cgroup *dst_cgrp,struct task_struct *leader,bool threadgroup);
kernel/cgroup/cgroup-v1.c +1 -1
@@ -125,7 +125,7 @@ int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from)css_task_iter_end (&it);if (task) { - ret =cgroup_migrate (task, false, &mgctx, to->root); + ret =cgroup_migrate (task, false, &mgctx);if (!ret)trace_cgroup_transfer_tasks (to, task, false);put_task_struct (task);
kernel/cgroup/cgroup.c +14 -11
@@ -2019,15 +2019,13 @@ struct task_struct *cgroup_taskset_next(struct cgroup_taskset *tset,/** * cgroup_taskset_migrate - migrate a taskset * @mgctx: migration context -* @root: cgroup root the migration is taking place on * * Migrate tasks in @mgctx 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 @mgctx are migrated. * @mgctx is consumed regardless of success. */ -static int cgroup_migrate_execute (struct cgroup_mgctx *mgctx, -struct cgroup_root *root) +static int cgroup_migrate_execute (struct cgroup_mgctx *mgctx) {struct cgroup_taskset *tset = &mgctx->tset;struct cgroup_subsys *ss;
@@ -2040,7 +2038,7 @@ static int cgroup_migrate_execute(struct cgroup_mgctx *mgctx,return 0 ;/* check that we can legitimately attach to the cgroup */ - do_each_subsys_mask(ss, ssid, root->subsys_mask) { + do_each_subsys_mask(ss, ssid, mgctx->ss_mask) {if (ss->can_attach) { tset->ssid = ssid; ret = ss->can_attach (tset);
@@ -2076,7 +2074,7 @@ static int cgroup_migrate_execute(struct cgroup_mgctx *mgctx,*/ tset->csets = &tset->dst_csets; - do_each_subsys_mask(ss, ssid, root->subsys_mask) { + do_each_subsys_mask(ss, ssid, mgctx->ss_mask) {if (ss->attach) { tset->ssid = ssid; ss->attach (tset);
@@ -2087,7 +2085,7 @@ static int cgroup_migrate_execute(struct cgroup_mgctx *mgctx, goto out_release_tset; out_cancel_attach: - do_each_subsys_mask(ss, ssid, root->subsys_mask) { + do_each_subsys_mask(ss, ssid, mgctx->ss_mask) {if (ssid == failed_ssid)break ;if (ss->cancel_attach) {
@@ -2223,6 +2221,8 @@ int cgroup_migrate_prepare_dst(struct cgroup_mgctx *mgctx)list_for_each_entry_safe (src_cset, tmp_cset, &mgctx->preloaded_src_csets, mg_preload_node) {struct css_set *dst_cset; +struct cgroup_subsys *ss; +int ssid; dst_cset =find_css_set (src_cset, src_cset->mg_dst_cgrp);if (!dst_cset)
@@ -2251,6 +2251,10 @@ int cgroup_migrate_prepare_dst(struct cgroup_mgctx *mgctx) &mgctx->preloaded_dst_csets); elseput_css_set (dst_cset ); + +for_each_subsys (ss, ssid) +if (src_cset->subsys[ssid] != dst_cset->subsys[ssid]) + mgctx->ss_mask |=1 << ssid; }return 0 ;
@@ -2263,7 +2267,6 @@ int cgroup_migrate_prepare_dst(struct cgroup_mgctx *mgctx)* 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 -* @root: cgroup root migration is taking place on * @mgctx: migration context * * Migrate a process or task denoted by @leader. If migrating a process,
@@ -2279,7 +2282,7 @@ int cgroup_migrate_prepare_dst(struct cgroup_mgctx *mgctx)* actually starting migrating. */ int cgroup_migrate (struct task_struct *leader,bool threadgroup, -struct cgroup_mgctx *mgctx,struct cgroup_root *root) +struct cgroup_mgctx *mgctx) {struct task_struct *task;
@@ -2299,7 +2302,7 @@ int cgroup_migrate(struct task_struct *leader, bool threadgroup,rcu_read_unlock ();spin_unlock_irq (&css_set_lock); -return cgroup_migrate_execute (mgctx, root); +return cgroup_migrate_execute (mgctx); }/**
@@ -2335,7 +2338,7 @@ int cgroup_attach_task(struct cgroup *dst_cgrp, struct task_struct *leader,/* prepare dst csets and commit */ ret =cgroup_migrate_prepare_dst (&mgctx);if (!ret) - ret =cgroup_migrate (leader, threadgroup, &mgctx, dst_cgrp->root); + ret =cgroup_migrate (leader, threadgroup, &mgctx);cgroup_migrate_finish (&mgctx);
@@ -2539,7 +2542,7 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp) }spin_unlock_irq (&css_set_lock); - ret =cgroup_migrate_execute (&mgctx, cgrp->root); + ret =cgroup_migrate_execute (&mgctx); out_finish:cgroup_migrate_finish (&mgctx);percpu_up_write (&cgroup_threadgroup_rwsem);
Signature · 1 line
2.9.3
Quoted 20 lines · Tejun Heo
Hello,
On the v2 hierarchy, when controllers are enabled and disabled, other ->*attach() callbacks of other controllers are called spuriously with the same source and destination. While this isn't critical, it's a bit nasty and can lead to temporary double charging on certain controllers. This patchset fixes the issue.
This patchset contains the following three patches.
0001-cgroup-cosmetic-update-to-cgroup_taskset_add.patch 0002-cgroup-track-migration-context-in-cgroup_mgctx.patch 0003-cgroup-call-subsys-attach-only-for-subsystems-which-.patch
0001-0002 restructures migration context tracking so that extra state can be tracked easily. 0003 fixes the spurious ->*attach() invocations.
This patchset is also available in the following git branch.
Quoted 9 lines
git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.gitreview-avoid-identity-attachdiffstat follows.
kernel/cgroup/cgroup-internal.h | 69 +++++++++++++++-- kernel/cgroup/cgroup-v1.c | 10 +- kernel/cgroup/cgroup.c | 161 ++++++++++++++++------------------------ 3 files changed, 135 insertions(+), 105 deletions(-)
Quoted 25 lines · Zefan Li
Quoted 24 lines · Tejun Heo
Hello,
On the v2 hierarchy, when controllers are enabled and disabled, other ->*attach() callbacks of other controllers are called spuriously with the same source and destination. While this isn't critical, it's a bit nasty and can lead to temporary double charging on certain controllers. This patchset fixes the issue.
This patchset contains the following three patches.
0001-cgroup-cosmetic-update-to-cgroup_taskset_add.patch 0002-cgroup-track-migration-context-in-cgroup_mgctx.patch 0003-cgroup-call-subsys-attach-only-for-subsystems-which-.patch
0001-0002 restructures migration context tracking so that extra state can be tracked easily. 0003 fixes the spurious ->*attach() invocations.
This patchset is also available in the following git branch.
git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.gitreview-avoid-identity-attachdiffstat follows.
Quoted 7 lines
Quoted 5 lines
kernel/cgroup/cgroup-internal.h | 69 +++++++++++++++-- kernel/cgroup/cgroup-v1.c | 10 +- kernel/cgroup/cgroup.c | 161 ++++++++++++++++------------------------ 3 files changed, 135 insertions(+), 105 deletions(-)Acked-by Zefan Li <[email protected]>
Applied to cgroup/for-4.11.
Thanks.
Signature · 1 line
tejun