OpenVZ Forum - RDF feed
type: channel
items: [PATCH v2 0/5] Fix problem with static_key decrement
items: [PATCH v2 1/5] don't attach a task to a dead cgroup
items: [PATCH v2 2/5] blkcg: protect blkcg-&gt;policy_list
items: [PATCH v2 3/5] change number_of_cpusets to an atomic
items: [PATCH v2 4/5] don't take cgroup_mutex in destroy()
items: [PATCH v2 5/5] decrement static keys on real destroy time
items: Re: [PATCH v2 1/5] don't attach a task to a dead cgroup
items: Re: [PATCH v2 3/5] change number_of_cpusets to an atomic
items: Re: [PATCH v2 4/5] don't take cgroup_mutex in destroy()
items: Re: [PATCH v2 5/5] decrement static keys on real destroy time

[PATCH v2 0/5] Fix problem with static_key decrement
type: item
description: This is my proposed fix for the sock memcg static_key problem raised by Kamezawa. In a summary, the problem is as follows: We are decrementing the jump label when the socket limit is set back to unlimited. The problem is that the sockets outlive the memcg, so we can only do that when the last reference count is dropped. It is worth mentioning that kmem controller for memcg will have the exact same problem. If, however, there are no sockets in flight, mem_cgroup_put() during ->destroy() will be the last one, and the decrementing will happen there. But static_key updates cannot happen with the cgroup_mutex held. This is because cpusets hold it from within the cpu_hotplug.lock - that static_keys take through get_online_cpus() in its cpu hotplug handler. Removing the cgroup_lock() dependency from cpusets is a lot harder, since the code for generate_sched_domain() rely on that lock to be held, and it interact with the cgroup core code by quite a bit. The aim of this series is to make ->destroy() a stable point for jump label updating, by calling it without the cgroup_mutex held. I believe it to be a good thing in itself, since it removes a bit the reach of the almighty cgroup_mutex. I am ready to make any further modifications on this that you guys deem necessary. Thanks Glauber Costa (5): don't attach a task to a dead cgroup blkcg: protect blkcg->policy_list change number_of_cpusets to an atomic don't take cgroup_mutex in destroy() decrement static keys on real destroy time block/blk-cgroup.c | 2 + include/linux/cpuset.h | 6 ++-- include/net/sock.h | 9 ++++++++ kernel/cgroup.c | 12 ++++++---- kernel/cpuset.c | 10 ++++---- mm/memcontrol.c | 20 ++++++++++++++++- net/ipv4/tcp_memcontrol.c | 50 ++++++++++++++++++++++++++++++++++++++------ 7 files changed, 87 insertions(+), 22 deletions(-) -- 1.7.7.6
creator: Glauber Costa
date: 2012-04-23T19:37:42-00:00

[PATCH v2 1/5] don't attach a task to a dead cgroup
type: item
description: Not all external callers of cgroup_attach_task() test to see if the cgroup is still live - the internal callers at cgroup.c does. With this test in cgroup_attach_task, we can assure that no tasks are ever moved to a cgroup that is past its destruction point and was already marked as dead. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Tejun Heo <tj@kernel.org> CC: Li Zefan <lizefan@huawei.com> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- kernel/cgroup.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/kernel/cgroup.c b/kernel/cgroup.c index b61b938..932c318 100644 --- a/kernel/cgroup.c +++ b/kernel/cgroup.c @@ -1927,6 +1927,9 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk) struct cgroup_taskset tset = { }; struct css_set *newcg; + if (cgroup_is_removed(cgrp)) + return -ENODEV; + /* @tsk either already exited or can't exit until the end */ if (tsk->flags & PF_EXITING) return -ESRCH; -- 1.7.7.6
creator: Glauber Costa
date: 2012-04-23T19:37:43-00:00

[PATCH v2 2/5] blkcg: protect blkcg-&gt;policy_list
type: item
description: policy_list walks are protected with blkcg->lock everywhere else in the code. In destroy(), they are not. Because destroy is usually protected with the cgroup_mutex(), this is usually not a problem. But it would be a lot better not to assume this. Signed-off-by: Glauber Costa <glommer@parallels.com> --- block/blk-cgroup.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index 126c341..35fd701 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -1557,10 +1557,12 @@ static void blkiocg_destroy(struct cgroup *cgroup) spin_unlock(&blkio_list_lock); } while (1); + spin_lock_irqsave(&blkcg->lock, flags); list_for_each_entry_safe(pn, pntmp, &blkcg->policy_list, node) { blkio_policy_delete_node(pn); kfree(pn); } + spin_unlock_irqrestore(&blkcg->lock, flags); free_css_id(&blkio_subsys, &blkcg->css); rcu_read_unlock(); -- 1.7.7.6
creator: Glauber Costa
date: 2012-04-23T19:37:44-00:00

[PATCH v2 3/5] change number_of_cpusets to an atomic
type: item
description: This will allow us to call destroy() without holding the cgroup_mutex(). Other important updates inside update_flags() are protected by the callback_mutex. We could protect this variable with the callback_mutex as well, as suggested by Li Zefan, but we need to make sure we are protected by that mutex at all times, and some of its updates happen inside the cgroup_mutex - which means we would deadlock. An atomic variable is not expensive, since it is seldom updated, and protect us well. Signed-off-by: Glauber Costa <glommer@parallels.com> --- include/linux/cpuset.h | 6 +++--- kernel/cpuset.c | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h index 668f66b..9b3d468 100644 --- a/include/linux/cpuset.h +++ b/include/linux/cpuset.h @@ -16,7 +16,7 @@ #ifdef CONFIG_CPUSETS -extern int number_of_cpusets; /* How many cpusets are defined in system? */ +extern atomic_t number_of_cpusets; /* How many cpusets are defined in system? */ extern int cpuset_init(void); extern void cpuset_init_smp(void); @@ -33,13 +33,13 @@ extern int __cpuset_node_allowed_hardwall(int node, gfp_t gfp_mask); static inline int cpuset_node_allowed_softwall(int node, gfp_t gfp_mask) { - return number_of_cpusets <= 1 || + return atomic_read(&number_of_cpusets) <= 1 || __cpuset_node_allowed_softwall(node, gfp_mask); } static inline int cpuset_node_allowed_hardwall(int node, gfp_t gfp_mask) { - return number_of_cpusets <= 1 || + return atomic_read(&number_of_cpusets) <= 1 || __cpuset_node_allowed_hardwall(node, gfp_mask); } diff --git a/kernel/cpuset.c b/kernel/cpuset.c index 8c8bd65..65bfd6d 100644 --- a/kernel/cpuset.c +++ b/kernel/cpuset.c @@ -73,7 +73,7 @@ static struct workqueue_struct *cpuset_wq; * When there is only one cpuset (the root cpuset) we can * short circuit some hooks. */ -int number_of_cpusets __read_mostly; +atomic_t number_of_cpusets __read_mostly; /* Forward declare cgroup structures */ struct cgroup_subsys cpuset…
creator: Glauber Costa
date: 2012-04-23T19:37:45-00:00

[PATCH v2 4/5] don't take cgroup_mutex in destroy()
type: item
description: Most of the destroy functions are only doing very simple things like freeing memory. The ones who goes through lists and such, already use its own locking for those. * The cgroup itself won't go away until we free it, (after destroy) * The parent won't go away because we hold a reference count * There are no more tasks in the cgroup, and the cgroup is declared dead (cgroup_is_removed() == true) [v2: don't cgroup_lock the freezer and blkcg ] Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Tejun Heo <tj@kernel.org> CC: Li Zefan <lizefan@huawei.com> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Vivek Goyal <vgoyal@redhat.com> --- kernel/cgroup.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-) diff --git a/kernel/cgroup.c b/kernel/cgroup.c index 932c318..976d332 100644 --- a/kernel/cgroup.c +++ b/kernel/cgroup.c @@ -869,13 +869,13 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode) * agent */ synchronize_rcu(); - mutex_lock(&cgroup_mutex); /* * Release the subsystem state objects. */ for_each_subsys(cgrp->root, ss) ss->destroy(cgrp); + mutex_lock(&cgroup_mutex); cgrp->root->number_of_cgroups--; mutex_unlock(&cgroup_mutex); @@ -3994,13 +3994,12 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry, err_destroy: + mutex_unlock(&cgroup_mutex); for_each_subsys(root, ss) { if (cgrp->subsys[ss->subsys_id]) ss->destroy(cgrp); } - mutex_unlock(&cgroup_mutex); - /* Release the reference count that we took on the superblock */ deactivate_super(sb); @@ -4349,9 +4348,9 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss) int ret = cgroup_init_idr(ss, css); if (ret) { dummytop->subsys[ss->subsys_id] = NULL; + mutex_unlock(&cgroup_mutex); ss->destroy(dummytop); subsys[i] = NULL; - mutex_unlock(&cgroup_mutex); return ret; } } @@ -4447,10 +4446,10 @@ void cgroup_unload_subsys(struct cgroup_subsys *ss) * pointer to find their state. note that this also takes care of * freeing the css_id. */ + mu…
creator: Glauber Costa
date: 2012-04-23T19:37:46-00:00

[PATCH v2 5/5] decrement static keys on real destroy time
type: item
description: We call the destroy function when a cgroup starts to be removed, such as by a rmdir event. However, because of our reference counters, some objects are still inflight. Right now, we are decrementing the static_keys at destroy() time, meaning that if we get rid of the last static_key reference, some objects will still have charges, but the code to properly uncharge them won't be run. This becomes a problem specially if it is ever enabled again, because now new charges will be added to the staled charges making keeping it pretty much impossible. We just need to be careful with the static branch activation: since there is no particular preferred order of their activation, we need to make sure that we only start using it after all call sites are active. This is achieved by having a per-memcg flag that is only updated after static_key_slow_inc() returns. At this time, we are sure all sites are active. This is made per-memcg, not global, for a reason: it also has the effect of making socket accounting more consistent. The first memcg to be limited will trigger static_key() activation, therefore, accounting. But all the others will then be accounted no matter what. After this patch, only limited memcgs will have its sockets accounted. [v2: changed a tcp limited flag for a generic proto limited flag ] [v3: update the current active flag only after the static_key update ] Signed-off-by: Glauber Costa <glommer@parallels.com> --- include/net/sock.h | 9 ++++++++ mm/memcontrol.c | 20 ++++++++++++++++- net/ipv4/tcp_memcontrol.c | 50 ++++++++++++++++++++++++++++++++++++++------ 3 files changed, 70 insertions(+), 9 deletions(-) diff --git a/include/net/sock.h b/include/net/sock.h index b3ebe6b..c5a2010 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -914,6 +914,15 @@ struct cg_proto { int *memory_pressure; long *sysctl_mem; /* + * active means it is currently active, and new sockets should + * be assigned to cgroups. + * + * activated means it was ever activated, and we…
creator: Glauber Costa
date: 2012-04-23T19:37:47-00:00

Re: [PATCH v2 1/5] don't attach a task to a dead cgroup
type: item
description: (2012/04/24 4:37), Glauber Costa wrote: > Not all external callers of cgroup_attach_task() test to > see if the cgroup is still live - the internal callers at > cgroup.c does. > > With this test in cgroup_attach_task, we can assure that > no tasks are ever moved to a cgroup that is past its > destruction point and was already marked as dead. > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: Tejun Heo <tj@kernel.org> > CC: Li Zefan <lizefan@huawei.com> > CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> > --- > kernel/cgroup.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/kernel/cgroup.c b/kernel/cgroup.c > index b61b938..932c318 100644 > --- a/kernel/cgroup.c > +++ b/kernel/cgroup.c > @@ -1927,6 +1927,9 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk) > struct cgroup_taskset tset = { }; > struct css_set *newcg; > > + if (cgroup_is_removed(cgrp)) > + return -ENODEV; > + > /* @tsk either already exited or can't exit until the end */ > if (tsk->flags & PF_EXITING) > return -ESRCH;
creator: KAMEZAWA Hiroyuki
date: 2012-04-24T02:20:45-00:00

Re: [PATCH v2 3/5] change number_of_cpusets to an atomic
type: item
description: (2012/04/24 4:37), Glauber Costa wrote: > This will allow us to call destroy() without holding the > cgroup_mutex(). Other important updates inside update_flags() > are protected by the callback_mutex. > > We could protect this variable with the callback_mutex as well, > as suggested by Li Zefan, but we need to make sure we are protected > by that mutex at all times, and some of its updates happen inside the > cgroup_mutex - which means we would deadlock. > > An atomic variable is not expensive, since it is seldom updated, > and protect us well. > > Signed-off-by: Glauber Costa <glommer@parallels.com> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
creator: KAMEZAWA Hiroyuki
date: 2012-04-24T02:25:46-00:00

Re: [PATCH v2 4/5] don't take cgroup_mutex in destroy()
type: item
description: (2012/04/24 4:37), Glauber Costa wrote: > Most of the destroy functions are only doing very simple things > like freeing memory. > > The ones who goes through lists and such, already use its own > locking for those. > > * The cgroup itself won't go away until we free it, (after destroy) > * The parent won't go away because we hold a reference count > * There are no more tasks in the cgroup, and the cgroup is declared > dead (cgroup_is_removed() == true) > > [v2: don't cgroup_lock the freezer and blkcg ] > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: Tejun Heo <tj@kernel.org> > CC: Li Zefan <lizefan@huawei.com> > CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> > CC: Vivek Goyal <vgoyal@redhat.com> > --- > kernel/cgroup.c | 9 ++++----- > 1 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/kernel/cgroup.c b/kernel/cgroup.c > index 932c318..976d332 100644 > --- a/kernel/cgroup.c > +++ b/kernel/cgroup.c > @@ -869,13 +869,13 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode) > * agent */ > synchronize_rcu(); > > - mutex_lock(&cgroup_mutex); > /* > * Release the subsystem state objects. > */ > for_each_subsys(cgrp->root, ss) > ss->destroy(cgrp); > > + mutex_lock(&cgroup_mutex); > cgrp->root->number_of_cgroups--; > mutex_unlock(&cgroup_mutex); > > @@ -3994,13 +3994,12 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry, > > err_destroy: > > + mutex_unlock(&cgroup_mutex); > for_each_subsys(root, ss) { > if (cgrp->subsys[ss->subsys_id]) > ss->destroy(cgrp); > } > > - mutex_unlock(&cgroup_mutex); > - > /* Release the reference count that we took on the superblock */ > deactivate_super(sb); > > @@ -4349,9 +4348,9 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss) > int ret = cgroup_init_idr(ss, css); > if (ret) { > dummytop->subsys[ss->subsys_id] = NULL; > + mutex_unlock(&cgroup_mutex); > ss->destroy(dummytop); > subsys[i] = NULL; > - mutex_unlock(&cgroup_mutex); > return ret; > } …
creator: KAMEZAWA Hiroyuki
date: 2012-04-24T02:31:54-00:00

Re: [PATCH v2 5/5] decrement static keys on real destroy time
type: item
description: (2012/04/24 4:37), Glauber Costa wrote: > We call the destroy function when a cgroup starts to be removed, > such as by a rmdir event. > > However, because of our reference counters, some objects are still > inflight. Right now, we are decrementing the static_keys at destroy() > time, meaning that if we get rid of the last static_key reference, > some objects will still have charges, but the code to properly > uncharge them won't be run. > > This becomes a problem specially if it is ever enabled again, because > now new charges will be added to the staled charges making keeping > it pretty much impossible. > > We just need to be careful with the static branch activation: > since there is no particular preferred order of their activation, > we need to make sure that we only start using it after all > call sites are active. This is achieved by having a per-memcg > flag that is only updated after static_key_slow_inc() returns. > At this time, we are sure all sites are active. > > This is made per-memcg, not global, for a reason: > it also has the effect of making socket accounting more > consistent. The first memcg to be limited will trigger static_key() > activation, therefore, accounting. But all the others will then be > accounted no matter what. After this patch, only limited memcgs > will have its sockets accounted. > > [v2: changed a tcp limited flag for a generic proto limited flag ] > [v3: update the current active flag only after the static_key update ] > > Signed-off-by: Glauber Costa <glommer@parallels.com> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> A small request below. <snip> > + * ->activated needs to be written after the static_key update. > + * This is what guarantees that the socket activation function > + * is the last one to run. See sock_update_memcg() for details, > + * and note that we don't mark any socket as belonging to this > + * memcg until that flag is up. > + * > + * We need to do this, because static_keys will span multi…
creator: KAMEZAWA Hiroyuki
date: 2012-04-24T02:40:57-00:00

Re: [PATCH v2 5/5] decrement static keys on real destroy time
type: item
description: On 04/23/2012 11:40 PM, KAMEZAWA Hiroyuki wrote: > (2012/04/24 4:37), Glauber Costa wrote: > >> We call the destroy function when a cgroup starts to be removed, >> such as by a rmdir event. >> >> However, because of our reference counters, some objects are still >> inflight. Right now, we are decrementing the static_keys at destroy() >> time, meaning that if we get rid of the last static_key reference, >> some objects will still have charges, but the code to properly >> uncharge them won't be run. >> >> This becomes a problem specially if it is ever enabled again, because >> now new charges will be added to the staled charges making keeping >> it pretty much impossible. >> >> We just need to be careful with the static branch activation: >> since there is no particular preferred order of their activation, >> we need to make sure that we only start using it after all >> call sites are active. This is achieved by having a per-memcg >> flag that is only updated after static_key_slow_inc() returns. >> At this time, we are sure all sites are active. >> >> This is made per-memcg, not global, for a reason: >> it also has the effect of making socket accounting more >> consistent. The first memcg to be limited will trigger static_key() >> activation, therefore, accounting. But all the others will then be >> accounted no matter what. After this patch, only limited memcgs >> will have its sockets accounted. >> >> [v2: changed a tcp limited flag for a generic proto limited flag ] >> [v3: update the current active flag only after the static_key update ] >> >> Signed-off-by: Glauber Costa<glommer@parallels.com> > > > Acked-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com> > > A small request below. > > <snip> > > >> + * ->activated needs to be written after the static_key update. >> + * This is what guarantees that the socket activation function >> + * is the last one to run. See sock_update_memcg() for details, >> + * and note that we don't mark any socket as belonging to thi…
creator: Glauber Costa
date: 2012-04-24T11:41:42-00:00

Re: [PATCH v2 4/5] don't take cgroup_mutex in destroy()
type: item
description: On 04/23/2012 11:31 PM, KAMEZAWA Hiroyuki wrote: > (2012/04/24 4:37), Glauber Costa wrote: > >> Most of the destroy functions are only doing very simple things >> like freeing memory. >> >> The ones who goes through lists and such, already use its own >> locking for those. >> >> * The cgroup itself won't go away until we free it, (after destroy) >> * The parent won't go away because we hold a reference count >> * There are no more tasks in the cgroup, and the cgroup is declared >> dead (cgroup_is_removed() == true) >> >> [v2: don't cgroup_lock the freezer and blkcg ] >> >> Signed-off-by: Glauber Costa<glommer@parallels.com> >> CC: Tejun Heo<tj@kernel.org> >> CC: Li Zefan<lizefan@huawei.com> >> CC: Kamezawa Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com> >> CC: Vivek Goyal<vgoyal@redhat.com> >> --- >> kernel/cgroup.c | 9 ++++----- >> 1 files changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/kernel/cgroup.c b/kernel/cgroup.c >> index 932c318..976d332 100644 >> --- a/kernel/cgroup.c >> +++ b/kernel/cgroup.c >> @@ -869,13 +869,13 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode) >> * agent */ >> synchronize_rcu(); >> >> - mutex_lock(&cgroup_mutex); >> /* >> * Release the subsystem state objects. >> */ >> for_each_subsys(cgrp->root, ss) >> ss->destroy(cgrp); >> >> + mutex_lock(&cgroup_mutex); >> cgrp->root->number_of_cgroups--; >> mutex_unlock(&cgroup_mutex); >> >> @@ -3994,13 +3994,12 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry, >> >> err_destroy: >> >> + mutex_unlock(&cgroup_mutex); >> for_each_subsys(root, ss) { >> if (cgrp->subsys[ss->subsys_id]) >> ss->destroy(cgrp); >> } >> >> - mutex_unlock(&cgroup_mutex); >> - >> /* Release the reference count that we took on the superblock */ >> deactivate_super(sb); >> >> @@ -4349,9 +4348,9 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss) >> int ret = cgroup_init_idr(ss, css); >> if (ret) { >> dummytop->subsys[ss->subsys_id] = NULL; >> + mutex_unlock(&cg…
creator: Glauber Costa
date: 2012-04-24T11:42:32-00:00

Re: [PATCH v2 3/5] change number_of_cpusets to an atomic
type: item
description: On Mon, 23 Apr 2012, Glauber Costa wrote: > This will allow us to call destroy() without holding the > cgroup_mutex(). Other important updates inside update_flags() > are protected by the callback_mutex. > > We could protect this variable with the callback_mutex as well, > as suggested by Li Zefan, but we need to make sure we are protected > by that mutex at all times, and some of its updates happen inside the > cgroup_mutex - which means we would deadlock. Would this not also be a good case to introduce static branching? number_of_cpusets is used to avoid going through unnecessary processing should there be no cpusets in use.
creator: Christoph Lameter
date: 2012-04-24T15:02:02-00:00

Re: [PATCH v2 3/5] change number_of_cpusets to an atomic
type: item
description: On 04/24/2012 12:02 PM, Christoph Lameter wrote: > On Mon, 23 Apr 2012, Glauber Costa wrote: > >> This will allow us to call destroy() without holding the >> cgroup_mutex(). Other important updates inside update_flags() >> are protected by the callback_mutex. >> >> We could protect this variable with the callback_mutex as well, >> as suggested by Li Zefan, but we need to make sure we are protected >> by that mutex at all times, and some of its updates happen inside the >> cgroup_mutex - which means we would deadlock. > > Would this not also be a good case to introduce static branching? > > number_of_cpusets is used to avoid going through unnecessary processing > should there be no cpusets in use. > Well, static branches comes with a set of problems themselves, so I usually prefer to use them only in places where we don't want to pay even a cache miss if we can avoid, or a function call, or anything like that - like the slub cache alloc as you may have seen in my kmem memcg series. It doesn't seem to be the case here.
creator: Glauber Costa
date: 2012-04-24T16:15:36-00:00

Re: [PATCH v2 3/5] change number_of_cpusets to an atomic
type: item
description: On Tue, 24 Apr 2012, Glauber Costa wrote: > > Would this not also be a good case to introduce static branching? > > > > number_of_cpusets is used to avoid going through unnecessary processing > > should there be no cpusets in use. > > static branches comes with a set of problems themselves, so I usually prefer > to use them only in places where we don't want to pay even a cache miss if we > can avoid, or a function call, or anything like that - like the slub cache > alloc as you may have seen in my kmem memcg series. > > It doesn't seem to be the case here. How did you figure that? number_of_cpusets was introduced exactly because the functions are used in places where we do not pay the cost of calling __cpuset_node_allowed_soft/hardwall. Have a look at these. They may take locks etc etc in critical allocation paths
creator: Christoph Lameter
date: 2012-04-24T16:24:14-00:00

Re: [PATCH v2 3/5] change number_of_cpusets to an atomic
type: item
description: On 04/24/2012 01:24 PM, Christoph Lameter wrote: > On Tue, 24 Apr 2012, Glauber Costa wrote: > >>> Would this not also be a good case to introduce static branching? >>> >>> number_of_cpusets is used to avoid going through unnecessary processing >>> should there be no cpusets in use. >> >> static branches comes with a set of problems themselves, so I usually prefer >> to use them only in places where we don't want to pay even a cache miss if we >> can avoid, or a function call, or anything like that - like the slub cache >> alloc as you may have seen in my kmem memcg series. >> >> It doesn't seem to be the case here. > > How did you figure that? number_of_cpusets was introduced exactly because > the functions are used in places where we do not pay the cost of calling > __cpuset_node_allowed_soft/hardwall. Have a look at these. They may take > locks etc etc in critical allocation paths I am not arguing that. You want to avoid the cost of processing a function, that's fair. (Note that by "function call cost" I don't mean the cost of processing a function, but the cost of a (potentially empty) function call.) The real question is: Are you okay with the cost of a branch + a global variable (which is almost read only) fetch? The test of a global variable can - and do as of right now - avoid all the expensive operations like locking, sleeping, etc, and if you don't need to squeeze every nanosecond you can, they are often simpler - and therefore better - than static branching. Just to mention one point I am coming across these days - that initiated all this: static patching holds the cpu_hotplug.lock. So it can't be called if you hold any lock that has been already held under the cpu_hotplug.lock. This will probably mean any lock the cpuset cgroup needs to take, because it is called - and to do a lot of things - from the cpu hotplug handler, that holds the cpu_hotplug.lock. So if if were a case of simple static branch usage, I am not opposed to it. But I foresee it getting …
creator: Glauber Costa
date: 2012-04-24T16:30:05-00:00

Re: [PATCH v2 3/5] change number_of_cpusets to an atomic
type: item
description: On Tue, 24 Apr 2012, Glauber Costa wrote: > > > It doesn't seem to be the case here. > > > > How did you figure that? number_of_cpusets was introduced exactly because > > the functions are used in places where we do not pay the cost of calling > > __cpuset_node_allowed_soft/hardwall. Have a look at these. They may take > > locks etc etc in critical allocation paths > I am not arguing that. > > You want to avoid the cost of processing a function, that's fair. > (Note that by "function call cost" I don't mean the cost of processing a > function, but the cost of a (potentially empty) function call.) > The real question is: Are you okay with the cost of a branch + a global > variable (which is almost read only) fetch? No and that is why the static branching comes in. It takes away the global read of the number_of_cpusets variable in the critical paths. > The test of a global variable can - and do as of right now - avoid all the > expensive operations like locking, sleeping, etc, and if you don't need to > squeeze every nanosecond you can, they are often simpler - and therefore > better - than static branching. Better than static branching? This is in critical VM functions and reducing the cache footprint there is good for everyone. > Just to mention one point I am coming across these days - that initiated all > this: static patching holds the cpu_hotplug.lock. So it can't be called if you > hold any lock that has been already held under the cpu_hotplug.lock. This will > probably mean any lock the cpuset cgroup needs to take, because it is called - > and to do a lot of things - from the cpu hotplug handler, that holds the > cpu_hotplug.lock. Transitions from one to two cpusets are rare and are only done when a cpuset is created in the /dev/cpuset hierachy). You could move the code modification outside of locks or defer action into an event thread if there are locks in the way.
creator: Christoph Lameter
date: 2012-04-24T18:27:09-00:00

Re: [PATCH v2 5/5] decrement static keys on real destroy time
type: item
description: (2012/04/24 20:41), Glauber Costa wrote: > On 04/23/2012 11:40 PM, KAMEZAWA Hiroyuki wrote: >> (2012/04/24 4:37), Glauber Costa wrote: >> >>> We call the destroy function when a cgroup starts to be removed, >>> such as by a rmdir event. >>> >>> However, because of our reference counters, some objects are still >>> inflight. Right now, we are decrementing the static_keys at destroy() >>> time, meaning that if we get rid of the last static_key reference, >>> some objects will still have charges, but the code to properly >>> uncharge them won't be run. >>> >>> This becomes a problem specially if it is ever enabled again, because >>> now new charges will be added to the staled charges making keeping >>> it pretty much impossible. >>> >>> We just need to be careful with the static branch activation: >>> since there is no particular preferred order of their activation, >>> we need to make sure that we only start using it after all >>> call sites are active. This is achieved by having a per-memcg >>> flag that is only updated after static_key_slow_inc() returns. >>> At this time, we are sure all sites are active. >>> >>> This is made per-memcg, not global, for a reason: >>> it also has the effect of making socket accounting more >>> consistent. The first memcg to be limited will trigger static_key() >>> activation, therefore, accounting. But all the others will then be >>> accounted no matter what. After this patch, only limited memcgs >>> will have its sockets accounted. >>> >>> [v2: changed a tcp limited flag for a generic proto limited flag ] >>> [v3: update the current active flag only after the static_key update ] >>> >>> Signed-off-by: Glauber Costa<glommer@parallels.com> >> >> >> Acked-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com> >> >> A small request below. >> >> <snip> >> >> >>> + * ->activated needs to be written after the static_key update. >>> + * This is what guarantees that the socket activation function >>> + * is the last one to run. See sock_updat…
creator: KAMEZAWA Hiroyuki
date: 2012-04-25T00:22:37-00:00

Re: [PATCH v2 4/5] don't take cgroup_mutex in destroy()
type: item
description: Glauber Costa wrote: > On 04/23/2012 11:31 PM, KAMEZAWA Hiroyuki wrote: >> (2012/04/24 4:37), Glauber Costa wrote: >> >>> Most of the destroy functions are only doing very simple things >>> like freeing memory. >>> >>> The ones who goes through lists and such, already use its own >>> locking for those. >>> >>> * The cgroup itself won't go away until we free it, (after destroy) >>> * The parent won't go away because we hold a reference count >>> * There are no more tasks in the cgroup, and the cgroup is declared >>> dead (cgroup_is_removed() == true) >>> >>> [v2: don't cgroup_lock the freezer and blkcg ] >>> >>> Signed-off-by: Glauber Costa<glommer@parallels.com> >>> CC: Tejun Heo<tj@kernel.org> >>> CC: Li Zefan<lizefan@huawei.com> >>> CC: Kamezawa Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com> >>> CC: Vivek Goyal<vgoyal@redhat.com> >>> --- >>> kernel/cgroup.c | 9 ++++----- >>> 1 files changed, 4 insertions(+), 5 deletions(-) >>> >>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c >>> index 932c318..976d332 100644 >>> --- a/kernel/cgroup.c >>> +++ b/kernel/cgroup.c >>> @@ -869,13 +869,13 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode) >>> * agent */ >>> synchronize_rcu(); >>> >>> - mutex_lock(&cgroup_mutex); >>> /* >>> * Release the subsystem state objects. >>> */ >>> for_each_subsys(cgrp->root, ss) >>> ss->destroy(cgrp); >>> >>> + mutex_lock(&cgroup_mutex); >>> cgrp->root->number_of_cgroups--; >>> mutex_unlock(&cgroup_mutex); >>> >>> @@ -3994,13 +3994,12 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry, >>> >>> err_destroy: >>> >>> + mutex_unlock(&cgroup_mutex); >>> for_each_subsys(root, ss) { >>> if (cgrp->subsys[ss->subsys_id]) >>> ss->destroy(cgrp); >>> } >>> >>> - mutex_unlock(&cgroup_mutex); >>> - >>> /* Release the reference count that we took on the superblock */ >>> deactivate_super(sb); >>> >>> @@ -4349,9 +4348,9 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss) >>> int ret = cgroup_init_idr(ss, c…
creator: Li Zefan
date: 2012-04-25T08:01:03-00:00