OpenVZ Forum - RDF feed
type: channel
items: [PATCH 0/7] memcg kernel memory tracking
items: [PATCH 1/7] small cleanup for memcontrol.c
items: [PATCH 2/7] Basic kernel memory functionality for the Memory Controller
items: [PATCH 3/7] per-cgroup slab caches
items: [PATCH 4/7] chained slab caches: move pages to a different cache when a cache is destroyed.
items: [PATCH 5/7] shrink support for memcg kmem controller
items: [PATCH 6/7] track dcache per-memcg
items: [PATCH 7/7] example shrinker for memcg-aware dcache
items: Re: [PATCH 0/7] memcg kernel memory tracking
items: Re: [PATCH 5/7] shrink support for memcg kmem controller
items: Re: [PATCH 4/7] chained slab caches: move pages to a different cache when a cache is destroyed.
items: Re: [PATCH 3/7] per-cgroup slab caches
items: Re: [PATCH 1/7] small cleanup for memcontrol.c

[PATCH 0/7] memcg kernel memory tracking
type: item
description: This is a first structured approach to tracking general kernel memory within the memory controller. Please tell me what you think. As previously proposed, one has the option of keeping kernel memory accounted separatedly, or together with the normal userspace memory. However, this time I made the option to, in this later case, bill the memory directly to memcg->res. It has the disadvantage that it becomes complicated to know which memory came from user or kernel, but OTOH, it does not create any overhead of drawing from multiple res_counters at read time. (and if you want them to be joined, you probably don't care) Kernel memory is never tracked for the root memory cgroup. This means that a system where no memory cgroups exists other than the root, the time cost of this implementation is a couple of branches in the slub code - none of them in fast paths. At the moment, this works only with the slub. At cgroup destruction, memory is billed to the parent. With no hierarchy, this would mean the root memcg. But since we are not billing to that, it simply ceases to be tracked. The caches that we want to be tracked need to explicit register into the infrastructure. If you would like to give it a try, you'll need one of Frederic's patches that is used as a basis for this (cgroups: ability to stop res charge propagation on bounded ancestor) Glauber Costa (7): small cleanup for memcontrol.c Basic kernel memory functionality for the Memory Controller per-cgroup slab caches chained slab caches: move pages to a different cache when a cache is destroyed. shrink support for memcg kmem controller track dcache per-memcg example shrinker for memcg-aware dcache fs/dcache.c | 136 +++++++++++++++++- include/linux/dcache.h | 4 + include/linux/memcontrol.h | 35 +++++ include/linux/shrinker.h | 4 + include/linux/slab.h | 12 ++ include/linux/slub_def.h | 3 + mm/memcontrol.c | 344 +++++++++++++++++++++++++++++++++++++++++++- mm/slub.c | 237 ++++++++++++++++++++++++++++--- mm/vmscan.c | 60 +…
creator: Glauber Costa
date: 2012-02-21T11:34:32-00:00

[PATCH 1/7] small cleanup for memcontrol.c
type: item
description: Move some hardcoded definitions to an enum type. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Kirill A. Shutemov <kirill@shutemov.name> CC: Greg Thelen <gthelen@google.com> CC: Johannes Weiner <jweiner@redhat.com> CC: Michal Hocko <mhocko@suse.cz> CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> CC: Paul Turner <pjt@google.com> CC: Frederic Weisbecker <fweisbec@gmail.com> --- mm/memcontrol.c | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 6728a7a..b15a693 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -351,9 +351,13 @@ enum charge_type { }; /* for encoding cft->private value on file */ -#define _MEM (0) -#define _MEMSWAP (1) -#define _OOM_TYPE (2) + +enum mem_type { + _MEM = 0, + _MEMSWAP, + _OOM_TYPE, +}; + #define MEMFILE_PRIVATE(x, val) (((x) << 16) | (val)) #define MEMFILE_TYPE(val) (((val) >> 16) & 0xffff) #define MEMFILE_ATTR(val) ((val) & 0xffff) -- 1.7.7.6
creator: Glauber Costa
date: 2012-02-21T11:34:33-00:00

[PATCH 2/7] Basic kernel memory functionality for the Memory Controller
type: item
description: This patch lays down the foundation for the kernel memory component of the Memory Controller. As of today, I am only laying down the following files: * memory.independent_kmem_limit * memory.kmem.limit_in_bytes * memory.kmem.soft_limit_in_bytes * memory.kmem.usage_in_bytes I am omitting the Documentation files in this version, at least in the first cycle. But they should not differ much from what I posted previously. The patch itself is not much different than the previous versions I posted. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Kirill A. Shutemov <kirill@shutemov.name> CC: Greg Thelen <gthelen@google.com> CC: Johannes Weiner <jweiner@redhat.com> CC: Michal Hocko <mhocko@suse.cz> CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> CC: Paul Turner <pjt@google.com> CC: Frederic Weisbecker <fweisbec@gmail.com> --- mm/memcontrol.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 97 insertions(+), 1 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index b15a693..26fda11 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -235,6 +235,10 @@ struct mem_cgroup { */ struct res_counter memsw; /* + * the counter to account for kmem usage. + */ + struct res_counter kmem; + /* * Per cgroup active and inactive list, similar to the * per zone LRU lists. */ @@ -280,6 +284,11 @@ struct mem_cgroup { */ unsigned long move_charge_at_immigrate; /* + * Should kernel memory limits be stabilished independently + * from user memory ? + */ + int kmem_independent_accounting; + /* * percpu counter. */ struct mem_cgroup_stat_cpu *stat; @@ -356,6 +365,7 @@ enum mem_type { _MEM = 0, _MEMSWAP, _OOM_TYPE, + _KMEM, }; #define MEMFILE_PRIVATE(x, val) (((x) << 16) | (val)) @@ -3844,6 +3854,11 @@ static u64 mem_cgroup_read(struct cgroup *cont, struct cftype *cft) else val = res_counter_read_u64(&memcg->memsw, name); break; +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM + case _KMEM: + val = res_counter_read_u64(&memcg->kmem, name); + break; +#end…
creator: Glauber Costa
date: 2012-02-21T11:34:34-00:00

[PATCH 3/7] per-cgroup slab caches
type: item
description: This patch creates the infrastructure to allow us to register per-memcg slab caches. As an example implementation, I am tracking the dentry cache, but others will follow. I am using an opt-in istead of opt-out system here: this means the cache needs to explicitly register itself to be tracked by memcg. I prefer this approach since: 1) not all caches are big enough to be relevant, 2) most of the relevant ones will involve shrinking in some way, and it would be better be sure they are shrinker-aware 3) some objects, like network sockets, have their very own idea of memory control, that goes beyond the allocation itself. Once registered, allocations made on behalf of a task living on a cgroup will be billed to it. It is a first-touch mechanism, but if follows what we have today, and the cgroup infrastructure itself. No overhead is expected in object allocation: only when slab pages are allocated and freed, any form of billing occurs. The allocation stays billed to a cgroup until it is destroyed. It is kept if a task leaves the cgroup, since it is close to impossible to efficiently map an object to a task - and the tracking is done by pages, which contain multiple objects. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Kirill A. Shutemov <kirill@shutemov.name> CC: Greg Thelen <gthelen@google.com> CC: Johannes Weiner <jweiner@redhat.com> CC: Michal Hocko <mhocko@suse.cz> CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> CC: Paul Turner <pjt@google.com> CC: Frederic Weisbecker <fweisbec@gmail.com> CC: Pekka Enberg <penberg@kernel.org> CC: Christoph Lameter <cl@linux.com> --- include/linux/memcontrol.h | 29 ++++++++++++++ include/linux/slab.h | 8 ++++ include/linux/slub_def.h | 2 + mm/memcontrol.c | 93 ++++++++++++++++++++++++++++++++++++++++++++ mm/slub.c | 68 +++++++++++++++++++++++++++++-- 5 files changed, 195 insertions(+), 5 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 4d34356..95e7e19 100644 --- a/include/lin…
creator: Glauber Costa
date: 2012-02-21T11:34:35-00:00

[PATCH 4/7] chained slab caches: move pages to a different cache when a cache is destroyed.
type: item
description: In the context of tracking kernel memory objects to a cgroup, the following problem appears: we may need to destroy a cgroup, but this does not guarantee that all objects inside the cache are dead. This can't be guaranteed even if we shrink the cache beforehand. The simple option is to simply leave the cache around. However, intensive workloads may have generated a lot of objects and thus the dead cache will live in memory for a long while. Scanning the list of objects in the dead cache takes time, and would probably require us to lock the free path of every objects to make sure we're not racing against the update. I decided to give a try to a different idea then - but I'd be happy to pursue something else if you believe it would be better. Upon memcg destruction, all the pages on the partial list are moved to the new slab (usually the parent memcg, or root memcg) When an object is freed, there are high stakes that no list locks are needed - so this case poses no overhead. If list manipulation is indeed needed, we can detect this case, and perform it in the right slab. If all pages were residing in the partial list, we can free the cache right away. Otherwise, we do it when the last cache leaves the full list. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Kirill A. Shutemov <kirill@shutemov.name> CC: Greg Thelen <gthelen@google.com> CC: Johannes Weiner <jweiner@redhat.com> CC: Michal Hocko <mhocko@suse.cz> CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> CC: Paul Turner <pjt@google.com> CC: Frederic Weisbecker <fweisbec@gmail.com> CC: Pekka Enberg <penberg@kernel.org> CC: Christoph Lameter <cl@linux.com> --- include/linux/memcontrol.h | 1 + include/linux/slab.h | 4 + include/linux/slub_def.h | 1 + mm/memcontrol.c | 64 ++++++++++++++++- mm/slub.c | 171 ++++++++++++++++++++++++++++++++++++++++---- 5 files changed, 227 insertions(+), 14 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 95e7e19..6138d10 100644 --- …
creator: Glauber Costa
date: 2012-02-21T11:34:36-00:00

[PATCH 5/7] shrink support for memcg kmem controller
type: item
description: This patch adds the shrinker interface to memcg proposed kmem controller. With this, softlimits starts being meaningful. I didn't played to much with softlimits itself, since it is a bit in progress for the general case as well. But this patch at least makes vmscan.c no longer skip shrink_slab for the memcg case. It also allows us to set the hard limit to a lower value than current usage, as it is possible for the current memcg: a reclaim is carried on, and if we succeed in freeing enough of kernel memory, we can lower the limit. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Kirill A. Shutemov <kirill@shutemov.name> CC: Greg Thelen <gthelen@google.com> CC: Johannes Weiner <jweiner@redhat.com> CC: Michal Hocko <mhocko@suse.cz> CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> CC: Paul Turner <pjt@google.com> CC: Frederic Weisbecker <fweisbec@gmail.com> CC: Pekka Enberg <penberg@kernel.org> CC: Christoph Lameter <cl@linux.com> --- include/linux/memcontrol.h | 5 +++ include/linux/shrinker.h | 4 ++ mm/memcontrol.c | 87 ++++++++++++++++++++++++++++++++++++++++++-- mm/vmscan.c | 60 +++++++++++++++++++++++++++++-- 4 files changed, 150 insertions(+), 6 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 6138d10..246b2d4 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -33,12 +33,16 @@ struct mm_struct; struct memcg_kmem_cache { struct kmem_cache *cache; struct work_struct destroy; + struct list_head lru; + u32 nr_objects; struct mem_cgroup *memcg; /* Should be able to do without this */ }; struct memcg_cache_struct { int index; struct kmem_cache *cache; + int (*shrink_fn)(struct shrinker *shrink, struct shrink_control *sc); + struct shrinker shrink; }; enum memcg_cache_indexes { @@ -53,6 +57,7 @@ struct mem_cgroup *memcg_from_shrinker(struct shrinker *s); struct memcg_kmem_cache *memcg_cache_get(struct mem_cgroup *memcg, int index); void register_memcg_cache(struct memcg_cache_struct *cache); vo…
creator: Glauber Costa
date: 2012-02-21T11:34:37-00:00

[PATCH 6/7] track dcache per-memcg
type: item
description: This patch allows to track kernel memory used by dentry caches in the memory controller. It uses the infrastructure already laid down, and register the dcache as the first users of it. A new cache is created for that purpose, and new allocations coming from tasks belonging to that cgroup will be serviced from the new cache. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Kirill A. Shutemov <kirill@shutemov.name> CC: Greg Thelen <gthelen@google.com> CC: Johannes Weiner <jweiner@redhat.com> CC: Michal Hocko <mhocko@suse.cz> CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> CC: Paul Turner <pjt@google.com> CC: Frederic Weisbecker <fweisbec@gmail.com> CC: Dave Chinner <david@fromorbit.com> --- fs/dcache.c | 38 +++++++++++++++++++++++++++++++++----- include/linux/dcache.h | 3 +++ 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/fs/dcache.c b/fs/dcache.c index 16a53cc..a452c19 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -86,7 +86,7 @@ __cacheline_aligned_in_smp DEFINE_SEQLOCK(rename_lock); EXPORT_SYMBOL(rename_lock); -static struct kmem_cache *dentry_cache __read_mostly; +static struct memcg_kmem_cache dentry_cache __read_mostly; /* * This is the single most critical data structure when it comes @@ -144,7 +144,7 @@ static void __d_free(struct rcu_head *head) WARN_ON(!list_empty(&dentry->d_alias)); if (dname_external(dentry)) kfree(dentry->d_name.name); - kmem_cache_free(dentry_cache, dentry); + kmem_cache_free(dentry->d_cache->cache, dentry); } /* @@ -234,6 +234,7 @@ static void dentry_lru_add(struct dentry *dentry) if (list_empty(&dentry->d_lru)) { spin_lock(&dcache_lru_lock); list_add(&dentry->d_lru, &dentry->d_sb->s_dentry_lru); + dentry->d_cache->nr_objects++; dentry->d_sb->s_nr_dentry_unused++; dentry_stat.nr_unused++; spin_unlock(&dcache_lru_lock); @@ -1178,6 +1179,21 @@ void shrink_dcache_parent(struct dentry * parent) } EXPORT_SYMBOL(shrink_dcache_parent); +static struct memcg_kmem_cache *dcache_pick_cache(void) +{ + struct mem_cgrou…
creator: Glauber Costa
date: 2012-02-21T11:34:38-00:00

[PATCH 7/7] example shrinker for memcg-aware dcache
type: item
description: This is an example of a memcg-aware shrinker for the dcache. In the way it works today, it tries to not interfere much with the existing shrinker. We can probably make a list of sb's per-memcg - having the root memcg to hold them all, but this adds a lot of complications along the way. This version is simpler, and serves to pave the way to future work. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Kirill A. Shutemov <kirill@shutemov.name> CC: Greg Thelen <gthelen@google.com> CC: Johannes Weiner <jweiner@redhat.com> CC: Michal Hocko <mhocko@suse.cz> CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> CC: Paul Turner <pjt@google.com> CC: Frederic Weisbecker <fweisbec@gmail.com> CC: Dave Chinner <david@fromorbit.com> --- fs/dcache.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/dcache.h | 1 + 2 files changed, 99 insertions(+), 0 deletions(-) diff --git a/fs/dcache.c b/fs/dcache.c index a452c19..98a0490 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -234,6 +234,7 @@ static void dentry_lru_add(struct dentry *dentry) if (list_empty(&dentry->d_lru)) { spin_lock(&dcache_lru_lock); list_add(&dentry->d_lru, &dentry->d_sb->s_dentry_lru); + list_add(&dentry->d_memcg_lru, &dentry->d_cache->lru); dentry->d_cache->nr_objects++; dentry->d_sb->s_nr_dentry_unused++; dentry_stat.nr_unused++; @@ -243,9 +244,12 @@ static void dentry_lru_add(struct dentry *dentry) static void __dentry_lru_del(struct dentry *dentry) { + WARN_ON(list_empty(&dentry->d_memcg_lru)); list_del_init(&dentry->d_lru); + list_del_init(&dentry->d_memcg_lru); dentry->d_flags &= ~DCACHE_SHRINK_LIST; dentry->d_sb->s_nr_dentry_unused--; + dentry->d_cache->nr_objects--; dentry_stat.nr_unused--; } @@ -283,10 +287,13 @@ static void dentry_lru_move_list(struct dentry *dentry, struct list_head *list) spin_lock(&dcache_lru_lock); if (list_empty(&dentry->d_lru)) { list_add_tail(&dentry->d_lru, list); + list_add_tail(&dentry->d_memcg_lru, &dentry->d_cache->lru); dentry->d_sb->s_nr_dentry_unus…
creator: Glauber Costa
date: 2012-02-21T11:34:39-00:00

Re: [PATCH 0/7] memcg kernel memory tracking
type: item
description: Hi Glauber, On Tue, Feb 21, 2012 at 3:34 AM, Glauber Costa <glommer@parallels.com> wrote: > This is a first structured approach to tracking general kernel > memory within the memory controller. Please tell me what you think. Thanks for posting these. > As previously proposed, one has the option of keeping kernel memory > accounted separatedly, or together with the normal userspace memory. > However, this time I made the option to, in this later case, bill > the memory directly to memcg->res. It has the disadvantage that it becomes > complicated to know which memory came from user or kernel, but OTOH, > it does not create any overhead of drawing from multiple res_counters > at read time. (and if you want them to be joined, you probably don't care) It would be nice to still keep a kernel memory counter (that gets updated at the same time as memcg->res) even when the limits are not independent, because sometimes it's important to know how much kernel memory is being used by a cgroup. > Kernel memory is never tracked for the root memory cgroup. This means > that a system where no memory cgroups exists other than the root, the > time cost of this implementation is a couple of branches in the slub > code - none of them in fast paths. At the moment, this works only > with the slub. > > At cgroup destruction, memory is billed to the parent. With no hierarchy, > this would mean the root memcg. But since we are not billing to that, > it simply ceases to be tracked. > > The caches that we want to be tracked need to explicit register into > the infrastructure. Why not track every cache unless otherwise specified? If you don't, you might end up polluting code all around the kernel to create per-cgroup caches. >From what I've seen, there are a fair amount of different caches that can end up using a significant amount of memory, and having to go around and explicitly mark each one doesn't seem like the right thing to do. -- Suleiman
creator: Suleiman Souhlal
date: 2012-02-21T23:25:18-00:00

Re: [PATCH 5/7] shrink support for memcg kmem controller
type: item
description: On Tue, Feb 21, 2012 at 3:34 AM, Glauber Costa <glommer@parallels.com> wrote: > @@ -5055,8 +5117,21 @@ int memcg_kmem_newpage(struct mem_cgroup *memcg, struct page *page, unsigned lon > { > unsigned long size = pages << PAGE_SHIFT; > struct res_counter *fail; > + int ret; > + bool do_softlimit; > + > + ret = res_counter_charge(memcg_kmem(memcg), size, &fail); > + if (unlikely(mem_cgroup_event_ratelimit(memcg, > + MEM_CGROUP_TARGET_THRESH))) { > + > + do_softlimit = mem_cgroup_event_ratelimit(memcg, > + MEM_CGROUP_TARGET_SOFTLIMIT); > + mem_cgroup_threshold(memcg); > + if (unlikely(do_softlimit)) > + mem_cgroup_update_tree(memcg, page); > + } > > - return res_counter_charge(memcg_kmem(memcg), size, &fail); > + return ret; > } It seems like this might cause a lot of kernel memory allocations to fail whenever we are at the limit, even if we have a lot of reclaimable memory, when we don't have independent accounting. Would it be better to use __mem_cgroup_try_charge() here, when we don't have independent accounting, in order to deal with this situation? -- Suleiman
creator: Suleiman Souhlal
date: 2012-02-21T23:35:10-00:00

Re: [PATCH 4/7] chained slab caches: move pages to a different cache when a cache is destroyed.
type: item
description: On Tue, Feb 21, 2012 at 3:34 AM, Glauber Costa <glommer@parallels.com> wrote: > In the context of tracking kernel memory objects to a cgroup, the > following problem appears: we may need to destroy a cgroup, but > this does not guarantee that all objects inside the cache are dead. > This can't be guaranteed even if we shrink the cache beforehand. > > The simple option is to simply leave the cache around. However, > intensive workloads may have generated a lot of objects and thus > the dead cache will live in memory for a long while. Why is this a problem? Leaving the cache around while there are still active objects in it would certainly be a lot simpler to understand and implement. -- Suleiman
creator: Suleiman Souhlal
date: 2012-02-21T23:40:46-00:00

Re: [PATCH 3/7] per-cgroup slab caches
type: item
description: On Tue, Feb 21, 2012 at 3:34 AM, Glauber Costa <glommer@parallels.com> wrote: > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 26fda11..2aa35b0 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > +struct kmem_cache * > +kmem_cache_dup(struct mem_cgroup *memcg, struct kmem_cache *base) > +{ > + struct kmem_cache *s; > + unsigned long pages; > + struct res_counter *fail; > + /* > + * TODO: We should use an ida-like index here, instead > + * of the kernel address > + */ > + char *kname = kasprintf(GFP_KERNEL, "%s-%p", base->name, memcg); Would it make more sense to use the memcg name instead of the pointer? > + > + WARN_ON(mem_cgroup_is_root(memcg)); > + > + if (!kname) > + return NULL; > + > + s = kmem_cache_create_cg(memcg, kname, base->size, > + base->align, base->flags, base->ctor); > + if (WARN_ON(!s)) > + goto out; > + > + > + pages = slab_nr_pages(s); > + > + if (res_counter_charge(memcg_kmem(memcg), pages << PAGE_SHIFT, &fail)) { > + kmem_cache_destroy(s); > + s = NULL; > + } What are we charging here? Does it ever get uncharged? -- Suleiman
creator: Suleiman Souhlal
date: 2012-02-21T23:50:32-00:00

Re: [PATCH 1/7] small cleanup for memcontrol.c
type: item
description: On Tue, 21 Feb 2012 15:34:33 +0400 Glauber Costa <glommer@parallels.com> wrote: > Move some hardcoded definitions to an enum type. > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: Kirill A. Shutemov <kirill@shutemov.name> > CC: Greg Thelen <gthelen@google.com> > CC: Johannes Weiner <jweiner@redhat.com> > CC: Michal Hocko <mhocko@suse.cz> > CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> > CC: Paul Turner <pjt@google.com> > CC: Frederic Weisbecker <fweisbec@gmail.com> seems ok to me. Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> a nitpick.. > --- > mm/memcontrol.c | 10 +++++++--- > 1 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 6728a7a..b15a693 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -351,9 +351,13 @@ enum charge_type { > }; > > /* for encoding cft->private value on file */ > -#define _MEM (0) > -#define _MEMSWAP (1) > -#define _OOM_TYPE (2) > + > +enum mem_type { > + _MEM = 0, =0 is required ? > + _MEMSWAP, > + _OOM_TYPE, > +}; > + > #define MEMFILE_PRIVATE(x, val) (((x) << 16) | (val)) > #define MEMFILE_TYPE(val) (((val) >> 16) & 0xffff) > #define MEMFILE_ATTR(val) ((val) & 0xffff) > -- > 1.7.7.6 > >
creator: KAMEZAWA Hiroyuki
date: 2012-02-22T00:46:19-00:00

Re: [PATCH 3/7] per-cgroup slab caches
type: item
description: On Tue, 21 Feb 2012 15:34:35 +0400 Glauber Costa <glommer@parallels.com> wrote: > This patch creates the infrastructure to allow us to register > per-memcg slab caches. As an example implementation, I am tracking > the dentry cache, but others will follow. > > I am using an opt-in istead of opt-out system here: this means the > cache needs to explicitly register itself to be tracked by memcg. > I prefer this approach since: > > 1) not all caches are big enough to be relevant, > 2) most of the relevant ones will involve shrinking in some way, > and it would be better be sure they are shrinker-aware > 3) some objects, like network sockets, have their very own idea > of memory control, that goes beyond the allocation itself. > > Once registered, allocations made on behalf of a task living > on a cgroup will be billed to it. It is a first-touch mechanism, > but if follows what we have today, and the cgroup infrastructure > itself. No overhead is expected in object allocation: only when > slab pages are allocated and freed, any form of billing occurs. > > The allocation stays billed to a cgroup until it is destroyed. > It is kept if a task leaves the cgroup, since it is close to > impossible to efficiently map an object to a task - and the tracking > is done by pages, which contain multiple objects. > Hmm....can't we do this by kmem_cache = kmem_cache_create(...., ...., SLAB_XXX_XXX | SLAB_MEMCG_AWARE) kmem_cache_alloc(kmem_cache, flags) => find a memcg_kmem_cache for the thread. if it doesn't exist, create it. BTW, comparing ANON/FILE caches, we'll have many kinds of gfp_t flags. Do we handle it correctly ? Maybe I don't fully understand your implemenatation...but try to comment. > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: Kirill A. Shutemov <kirill@shutemov.name> > CC: Greg Thelen <gthelen@google.com> > CC: Johannes Weiner <jweiner@redhat.com> > CC: Michal Hocko <mhocko@suse.cz> > CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> > CC: Paul Turne…
creator: KAMEZAWA Hiroyuki
date: 2012-02-22T01:21:38-00:00

Re: [PATCH 4/7] chained slab caches: move pages to a different cache when a cache is destroyed.
type: item
description: On Tue, 21 Feb 2012 15:34:36 +0400 Glauber Costa <glommer@parallels.com> wrote: > In the context of tracking kernel memory objects to a cgroup, the > following problem appears: we may need to destroy a cgroup, but > this does not guarantee that all objects inside the cache are dead. > This can't be guaranteed even if we shrink the cache beforehand. > > The simple option is to simply leave the cache around. However, > intensive workloads may have generated a lot of objects and thus > the dead cache will live in memory for a long while. > > Scanning the list of objects in the dead cache takes time, and > would probably require us to lock the free path of every objects > to make sure we're not racing against the update. > > I decided to give a try to a different idea then - but I'd be > happy to pursue something else if you believe it would be better. > > Upon memcg destruction, all the pages on the partial list > are moved to the new slab (usually the parent memcg, or root memcg) > When an object is freed, there are high stakes that no list locks > are needed - so this case poses no overhead. If list manipulation > is indeed needed, we can detect this case, and perform it > in the right slab. > > If all pages were residing in the partial list, we can free > the cache right away. Otherwise, we do it when the last cache > leaves the full list. > How about starting from 'don't handle slabs on dead memcg' if shrink_slab() can find them.... This "move" complicates all implementation, I think... Thanks, -Kame
creator: KAMEZAWA Hiroyuki
date: 2012-02-22T01:25:12-00:00

Re: [PATCH 5/7] shrink support for memcg kmem controller
type: item
description: On Tue, 21 Feb 2012 15:34:37 +0400 Glauber Costa <glommer@parallels.com> wrote: > This patch adds the shrinker interface to memcg proposed kmem > controller. With this, softlimits starts being meaningful. I didn't > played to much with softlimits itself, since it is a bit in progress > for the general case as well. But this patch at least makes vmscan.c > no longer skip shrink_slab for the memcg case. > > It also allows us to set the hard limit to a lower value than > current usage, as it is possible for the current memcg: a reclaim > is carried on, and if we succeed in freeing enough of kernel memory, > we can lower the limit. > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: Kirill A. Shutemov <kirill@shutemov.name> > CC: Greg Thelen <gthelen@google.com> > CC: Johannes Weiner <jweiner@redhat.com> > CC: Michal Hocko <mhocko@suse.cz> > CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> > CC: Paul Turner <pjt@google.com> > CC: Frederic Weisbecker <fweisbec@gmail.com> > CC: Pekka Enberg <penberg@kernel.org> > CC: Christoph Lameter <cl@linux.com> > --- > include/linux/memcontrol.h | 5 +++ > include/linux/shrinker.h | 4 ++ > mm/memcontrol.c | 87 ++++++++++++++++++++++++++++++++++++++++++-- > mm/vmscan.c | 60 +++++++++++++++++++++++++++++-- > 4 files changed, 150 insertions(+), 6 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 6138d10..246b2d4 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -33,12 +33,16 @@ struct mm_struct; > struct memcg_kmem_cache { > struct kmem_cache *cache; > struct work_struct destroy; > + struct list_head lru; > + u32 nr_objects; > struct mem_cgroup *memcg; /* Should be able to do without this */ > }; > > struct memcg_cache_struct { > int index; > struct kmem_cache *cache; > + int (*shrink_fn)(struct shrinker *shrink, struct shrink_control *sc); > + struct shrinker shrink; > }; > > enum memcg_cache_indexes { > @@ -53,6 +57,7 @@ struct mem_cgroup *mem…
creator: KAMEZAWA Hiroyuki
date: 2012-02-22T01:42:56-00:00

Re: [PATCH 0/7] memcg kernel memory tracking
type: item
description: Hi Glauber, On Tue, Feb 21, 2012 at 1:34 PM, Glauber Costa <glommer@parallels.com> wrote: > This is a first structured approach to tracking general kernel > memory within the memory controller. Please tell me what you think. I like it! I only skimmed through the SLUB changes but they seemed reasonable enough. What kind of performance hit are we taking when memcg configuration option is enabled but the feature is disabled? Pekka
creator: Pekka Enberg
date: 2012-02-22T07:08:26-00:00

Re: [PATCH 0/7] memcg kernel memory tracking
type: item
description: >> As previously proposed, one has the option of keeping kernel memory >> accounted separatedly, or together with the normal userspace memory. >> However, this time I made the option to, in this later case, bill >> the memory directly to memcg->res. It has the disadvantage that it becomes >> complicated to know which memory came from user or kernel, but OTOH, >> it does not create any overhead of drawing from multiple res_counters >> at read time. (and if you want them to be joined, you probably don't care) > > It would be nice to still keep a kernel memory counter (that gets > updated at the same time as memcg->res) even when the limits are not > independent, because sometimes it's important to know how much kernel > memory is being used by a cgroup. Can you clarify in this "sometimes" ? The way I see it, we either always use two counters - as did in my original proposal - or use a single counter for this case. Keeping a separated counter and still billing to the user memory is the worst of both worlds to me, since you get the performance hit of updating two resource counters. >> Kernel memory is never tracked for the root memory cgroup. This means >> that a system where no memory cgroups exists other than the root, the >> time cost of this implementation is a couple of branches in the slub >> code - none of them in fast paths. At the moment, this works only >> with the slub. >> >> At cgroup destruction, memory is billed to the parent. With no hierarchy, >> this would mean the root memcg. But since we are not billing to that, >> it simply ceases to be tracked. >> >> The caches that we want to be tracked need to explicit register into >> the infrastructure. > > Why not track every cache unless otherwise specified? If you don't, > you might end up polluting code all around the kernel to create > per-cgroup caches. > From what I've seen, there are a fair amount of different caches that > can end up using a significant amount of memory, and having to go > around and ex…
creator: Glauber Costa
date: 2012-02-22T13:58:58-00:00

Re: [PATCH 5/7] shrink support for memcg kmem controller
type: item
description: On 02/22/2012 03:35 AM, Suleiman Souhlal wrote: > On Tue, Feb 21, 2012 at 3:34 AM, Glauber Costa<glommer@parallels.com> wrote: > >> @@ -5055,8 +5117,21 @@ int memcg_kmem_newpage(struct mem_cgroup *memcg, struct page *page, unsigned lon >> { >> unsigned long size = pages<< PAGE_SHIFT; >> struct res_counter *fail; >> + int ret; >> + bool do_softlimit; >> + >> + ret = res_counter_charge(memcg_kmem(memcg), size,&fail); >> + if (unlikely(mem_cgroup_event_ratelimit(memcg, >> + MEM_CGROUP_TARGET_THRESH))) { >> + >> + do_softlimit = mem_cgroup_event_ratelimit(memcg, >> + MEM_CGROUP_TARGET_SOFTLIMIT); >> + mem_cgroup_threshold(memcg); >> + if (unlikely(do_softlimit)) >> + mem_cgroup_update_tree(memcg, page); >> + } >> >> - return res_counter_charge(memcg_kmem(memcg), size,&fail); >> + return ret; >> } > > It seems like this might cause a lot of kernel memory allocations to > fail whenever we are at the limit, even if we have a lot of > reclaimable memory, when we don't have independent accounting. > > Would it be better to use __mem_cgroup_try_charge() here, when we > don't have independent accounting, in order to deal with this > situation? > Yes, it would. I'll work on that.
creator: Glauber Costa
date: 2012-02-22T14:00:00-00:00

Re: [PATCH 1/7] small cleanup for memcontrol.c
type: item
description: On 02/22/2012 04:46 AM, KAMEZAWA Hiroyuki wrote: > On Tue, 21 Feb 2012 15:34:33 +0400 > Glauber Costa<glommer@parallels.com> wrote: > >> Move some hardcoded definitions to an enum type. >> >> Signed-off-by: Glauber Costa<glommer@parallels.com> >> CC: Kirill A. Shutemov<kirill@shutemov.name> >> CC: Greg Thelen<gthelen@google.com> >> CC: Johannes Weiner<jweiner@redhat.com> >> CC: Michal Hocko<mhocko@suse.cz> >> CC: Hiroyouki Kamezawa<kamezawa.hiroyu@jp.fujitsu.com> >> CC: Paul Turner<pjt@google.com> >> CC: Frederic Weisbecker<fweisbec@gmail.com> > > seems ok to me. > > Acked-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com> BTW, this series is likely to go through many rounds of discussion. This patch can be probably picked separately, if you want to. > a nitpick.. > >> --- >> mm/memcontrol.c | 10 +++++++--- >> 1 files changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 6728a7a..b15a693 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -351,9 +351,13 @@ enum charge_type { >> }; >> >> /* for encoding cft->private value on file */ >> -#define _MEM (0) >> -#define _MEMSWAP (1) >> -#define _OOM_TYPE (2) >> + >> +enum mem_type { >> + _MEM = 0, > > =0 is required ? I believe not, but I always liked to use it to be 100 % explicit. Personal taste... Can change it, if this is a big deal.
creator: Glauber Costa
date: 2012-02-22T14:01:46-00:00

Re: [PATCH 3/7] per-cgroup slab caches
type: item
description: On 02/22/2012 03:50 AM, Suleiman Souhlal wrote: > On Tue, Feb 21, 2012 at 3:34 AM, Glauber Costa<glommer@parallels.com> wrote: >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 26fda11..2aa35b0 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> +struct kmem_cache * >> +kmem_cache_dup(struct mem_cgroup *memcg, struct kmem_cache *base) >> +{ >> + struct kmem_cache *s; >> + unsigned long pages; >> + struct res_counter *fail; >> + /* >> + * TODO: We should use an ida-like index here, instead >> + * of the kernel address >> + */ >> + char *kname = kasprintf(GFP_KERNEL, "%s-%p", base->name, memcg); > > Would it make more sense to use the memcg name instead of the pointer? Well, yes. But at this point in creation time, we still don't have this all setup. The css pointer is NULL, so I could not derive the name from it. Instead of keep fighting what seemed to be a minor issue, I opted to kick the patches out and be clear with a comment that this is not what I intend in the way. Do you know about any good way to grab the cgroup name at create() time ? >> + >> + WARN_ON(mem_cgroup_is_root(memcg)); >> + >> + if (!kname) >> + return NULL; >> + >> + s = kmem_cache_create_cg(memcg, kname, base->size, >> + base->align, base->flags, base->ctor); >> + if (WARN_ON(!s)) >> + goto out; >> + >> + >> + pages = slab_nr_pages(s); >> + >> + if (res_counter_charge(memcg_kmem(memcg), pages<< PAGE_SHIFT,&fail)) { >> + kmem_cache_destroy(s); >> + s = NULL; >> + } > > What are we charging here? Does it ever get uncharged? We're charging the slab initial pages, that comes from allocations outside allocate_slab(). But in this sense, it is not very different than tin foil hats to protect against mind reading. Probably works, but I am not sure the threat is real (also remembering we can probably want to port it to the original slab allocator later - and let me be honest - I know 0 about how that works). So if the slab starts with 0 pages, this is a nop. If in any case it does not, i…
creator: Glauber Costa
date: 2012-02-22T14:08:06-00:00

Re: [PATCH 0/7] memcg kernel memory tracking
type: item
description: On 02/22/2012 11:08 AM, Pekka Enberg wrote: > Hi Glauber, > > On Tue, Feb 21, 2012 at 1:34 PM, Glauber Costa<glommer@parallels.com> wrote: >> This is a first structured approach to tracking general kernel >> memory within the memory controller. Please tell me what you think. > > I like it! I only skimmed through the SLUB changes but they seemed > reasonable enough. What kind of performance hit are we taking when > memcg configuration option is enabled but the feature is disabled? > > Pekka Thanks Pekka. Well, I didn't took any numbers, because I don't consider the whole work any close to final form, but I wanted people to comment anyway. In particular, I intend to use the same trick I used for tcp sock buffers here for this case - (static_branch()), so the performance hit should come from two pointers in the kmem_cache structure - and I believe it is possible to remove one of them. I can definitely measure when I implement that, but I think it is reasonable to expect not that much of a hit.
creator: Glauber Costa
date: 2012-02-22T14:11:41-00:00

Re: [PATCH 3/7] per-cgroup slab caches
type: item
description: On 02/22/2012 05:21 AM, KAMEZAWA Hiroyuki wrote: > On Tue, 21 Feb 2012 15:34:35 +0400 > Glauber Costa<glommer@parallels.com> wrote: > >> This patch creates the infrastructure to allow us to register >> per-memcg slab caches. As an example implementation, I am tracking >> the dentry cache, but others will follow. >> >> I am using an opt-in istead of opt-out system here: this means the >> cache needs to explicitly register itself to be tracked by memcg. >> I prefer this approach since: >> >> 1) not all caches are big enough to be relevant, >> 2) most of the relevant ones will involve shrinking in some way, >> and it would be better be sure they are shrinker-aware >> 3) some objects, like network sockets, have their very own idea >> of memory control, that goes beyond the allocation itself. >> >> Once registered, allocations made on behalf of a task living >> on a cgroup will be billed to it. It is a first-touch mechanism, >> but if follows what we have today, and the cgroup infrastructure >> itself. No overhead is expected in object allocation: only when >> slab pages are allocated and freed, any form of billing occurs. >> >> The allocation stays billed to a cgroup until it is destroyed. >> It is kept if a task leaves the cgroup, since it is close to >> impossible to efficiently map an object to a task - and the tracking >> is done by pages, which contain multiple objects. >> > > Hmm....can't we do this by > > kmem_cache = kmem_cache_create(...., ...., SLAB_XXX_XXX | SLAB_MEMCG_AWARE) > > kmem_cache_alloc(kmem_cache, flags) > => find a memcg_kmem_cache for the thread. if it doesn't exist, create it. Where exactly? Apart for the slab flag, I don't follow the rest of your proposal. > > BTW, comparing ANON/FILE caches, we'll have many kinds of gfp_t flags. > > Do we handle it correctly ? TODO item =) > > Maybe I don't fully understand your implemenatation...but try to comment. Thanks. > > >> >> +struct memcg_kmem_cache { >> + struct kmem_cache *cache; >> + struct mem_cgr…
creator: Glauber Costa
date: 2012-02-22T14:25:46-00:00

Re: [PATCH 4/7] chained slab caches: move pages to a different cache when a cache is destroyed.
type: item
description: On 02/22/2012 03:40 AM, Suleiman Souhlal wrote: > On Tue, Feb 21, 2012 at 3:34 AM, Glauber Costa<glommer@parallels.com> wrote: >> In the context of tracking kernel memory objects to a cgroup, the >> following problem appears: we may need to destroy a cgroup, but >> this does not guarantee that all objects inside the cache are dead. >> This can't be guaranteed even if we shrink the cache beforehand. >> >> The simple option is to simply leave the cache around. However, >> intensive workloads may have generated a lot of objects and thus >> the dead cache will live in memory for a long while. > > Why is this a problem? > > Leaving the cache around while there are still active objects in it > would certainly be a lot simpler to understand and implement. > Yeah, I agree on the simplicity. The chained stuff was probably the hardest one in the patchset to get working alright. However, my assumptions are as follow: 1) If we bother to be tracking kernel memory, it is because we believe its usage can skyrocket under certain circumstances. In those scenarios, we'll have a lot of objects around. If we just let them flowing, it's just wasted memory that was created from the memcg, but can't be reclaimed on its behalf. 2) We can reclaim that, if we have, as a policy, to always start shrinking from those when global pressure kicks in. But then, we move the complication from one part to another.
creator: Glauber Costa
date: 2012-02-22T14:50:58-00:00

Re: [PATCH 5/7] shrink support for memcg kmem controller
type: item
description: On 02/22/2012 05:42 AM, KAMEZAWA Hiroyuki wrote: > On Tue, 21 Feb 2012 15:34:37 +0400 > Glauber Costa<glommer@parallels.com> wrote: > >> This patch adds the shrinker interface to memcg proposed kmem >> controller. With this, softlimits starts being meaningful. I didn't >> played to much with softlimits itself, since it is a bit in progress >> for the general case as well. But this patch at least makes vmscan.c >> no longer skip shrink_slab for the memcg case. >> >> It also allows us to set the hard limit to a lower value than >> current usage, as it is possible for the current memcg: a reclaim >> is carried on, and if we succeed in freeing enough of kernel memory, >> we can lower the limit. >> >> Signed-off-by: Glauber Costa<glommer@parallels.com> >> CC: Kirill A. Shutemov<kirill@shutemov.name> >> CC: Greg Thelen<gthelen@google.com> >> CC: Johannes Weiner<jweiner@redhat.com> >> CC: Michal Hocko<mhocko@suse.cz> >> CC: Hiroyouki Kamezawa<kamezawa.hiroyu@jp.fujitsu.com> >> CC: Paul Turner<pjt@google.com> >> CC: Frederic Weisbecker<fweisbec@gmail.com> >> CC: Pekka Enberg<penberg@kernel.org> >> CC: Christoph Lameter<cl@linux.com> >> --- >> include/linux/memcontrol.h | 5 +++ >> include/linux/shrinker.h | 4 ++ >> mm/memcontrol.c | 87 ++++++++++++++++++++++++++++++++++++++++++-- >> mm/vmscan.c | 60 +++++++++++++++++++++++++++++-- >> 4 files changed, 150 insertions(+), 6 deletions(-) >> >> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h >> index 6138d10..246b2d4 100644 >> --- a/include/linux/memcontrol.h >> +++ b/include/linux/memcontrol.h >> @@ -33,12 +33,16 @@ struct mm_struct; >> struct memcg_kmem_cache { >> struct kmem_cache *cache; >> struct work_struct destroy; >> + struct list_head lru; >> + u32 nr_objects; >> struct mem_cgroup *memcg; /* Should be able to do without this */ >> }; >> >> struct memcg_cache_struct { >> int index; >> struct kmem_cache *cache; >> + int (*shrink_fn)(struct shrinker *shrink, struct shrink_control *sc); >> + struct shrin…
creator: Glauber Costa
date: 2012-02-22T14:53:25-00:00

Re: [PATCH 4/7] chained slab caches: move pages to a different cache when a cache is destroyed.
type: item
description: On 02/22/2012 05:25 AM, KAMEZAWA Hiroyuki wrote: > On Tue, 21 Feb 2012 15:34:36 +0400 > Glauber Costa<glommer@parallels.com> wrote: > >> In the context of tracking kernel memory objects to a cgroup, the >> following problem appears: we may need to destroy a cgroup, but >> this does not guarantee that all objects inside the cache are dead. >> This can't be guaranteed even if we shrink the cache beforehand. >> >> The simple option is to simply leave the cache around. However, >> intensive workloads may have generated a lot of objects and thus >> the dead cache will live in memory for a long while. >> >> Scanning the list of objects in the dead cache takes time, and >> would probably require us to lock the free path of every objects >> to make sure we're not racing against the update. >> >> I decided to give a try to a different idea then - but I'd be >> happy to pursue something else if you believe it would be better. >> >> Upon memcg destruction, all the pages on the partial list >> are moved to the new slab (usually the parent memcg, or root memcg) >> When an object is freed, there are high stakes that no list locks >> are needed - so this case poses no overhead. If list manipulation >> is indeed needed, we can detect this case, and perform it >> in the right slab. >> >> If all pages were residing in the partial list, we can free >> the cache right away. Otherwise, we do it when the last cache >> leaves the full list. >> > > How about starting from 'don't handle slabs on dead memcg' > if shrink_slab() can find them.... > > This "move" complicates all implementation, I think... > You mean, whenever pressure kicks in, start by reclaiming from dead memcg? Well, I can work with that, for sure. I am not that sure that this will be a win, but there is only way to know for sure. Note that in this case, we need to keep the memcg around anyway. Also, it is yet another reason why I believe we should explicit register a cache for being tracked. If your memcg is gone, but the o…
creator: Glauber Costa
date: 2012-02-22T14:57:17-00:00

Re: [PATCH 0/7] memcg kernel memory tracking
type: item
description: On Wed, Feb 22, 2012 at 5:58 AM, Glauber Costa <glommer@parallels.com> wrote: >>> As previously proposed, one has the option of keeping kernel memory >>> accounted separatedly, or together with the normal userspace memory. >>> However, this time I made the option to, in this later case, bill >>> the memory directly to memcg->res. It has the disadvantage that it >>> becomes >>> complicated to know which memory came from user or kernel, but OTOH, >>> it does not create any overhead of drawing from multiple res_counters >>> at read time. (and if you want them to be joined, you probably don't >>> care) >> >> >> It would be nice to still keep a kernel memory counter (that gets >> updated at the same time as memcg->res) even when the limits are not >> independent, because sometimes it's important to know how much kernel >> memory is being used by a cgroup. > > > Can you clarify in this "sometimes" ? The way I see it, we either always use > two counters - as did in my original proposal - or use a single counter for > this case. Keeping a separated counter and still billing to the user memory > is the worst of both worlds to me, since you get the performance hit of > updating two resource counters. By "sometimes", I mean pretty much any time we have to debug why a cgroup is out of memory. If there is no counter for how much kernel memory is used, it's pretty much impossible to determine why the cgroup is full. As for the performance, I do not think it is bad, as the accounting is done in the slow path of slab allocation, when we allocate/free pages. > > >>> Kernel memory is never tracked for the root memory cgroup. This means >>> that a system where no memory cgroups exists other than the root, the >>> time cost of this implementation is a couple of branches in the slub >>> code - none of them in fast paths. At the moment, this works only >>> with the slub. >>> >>> At cgroup destruction, memory is billed to the parent. With no hierarchy, >>> this would mean the root memcg. …
creator: Suleiman Souhlal
date: 2012-02-22T20:32:03-00:00

Re: [PATCH 0/7] memcg kernel memory tracking
type: item
description: On Tue, Feb 21, 2012 at 3:34 AM, Glauber Costa <glommer@parallels.com> wrote: > This is a first structured approach to tracking general kernel > memory within the memory controller. Please tell me what you think. > > As previously proposed, one has the option of keeping kernel memory > accounted separatedly, or together with the normal userspace memory. > However, this time I made the option to, in this later case, bill > the memory directly to memcg->res. It has the disadvantage that it becomes > complicated to know which memory came from user or kernel, but OTOH, > it does not create any overhead of drawing from multiple res_counters > at read time. (and if you want them to be joined, you probably don't care) Keeping one counter for user and kernel pages makes it easier for admins to configure the system. About reporting, we should still report the user and kernel memory separately. It will be extremely useful when diagnosing the system like heavily memory pressure or OOM. > Kernel memory is never tracked for the root memory cgroup. This means > that a system where no memory cgroups exists other than the root, the > time cost of this implementation is a couple of branches in the slub > code - none of them in fast paths. At the moment, this works only > with the slub. > > At cgroup destruction, memory is billed to the parent. With no hierarchy, > this would mean the root memcg. But since we are not billing to that, > it simply ceases to be tracked. > > The caches that we want to be tracked need to explicit register into > the infrastructure. It would be hard to let users to register which slab to track explicitly. We should track them all in general, even with the ones without shrinker, we want to understand how much is used by which cgroup. --Ying > > If you would like to give it a try, you'll need one of Frederic's patches > that is used as a basis for this > (cgroups: ability to stop res charge propagation on bounded ancestor) > > Glauber Costa (7): > small clea…
creator: Ying Han
date: 2012-02-23T18:18:22-00:00

Re: [PATCH 0/7] memcg kernel memory tracking
type: item
description: On 02/23/2012 04:18 PM, Ying Han wrote: > On Tue, Feb 21, 2012 at 3:34 AM, Glauber Costa<glommer@parallels.com> wrote: >> This is a first structured approach to tracking general kernel >> memory within the memory controller. Please tell me what you think. >> >> As previously proposed, one has the option of keeping kernel memory >> accounted separatedly, or together with the normal userspace memory. >> However, this time I made the option to, in this later case, bill >> the memory directly to memcg->res. It has the disadvantage that it becomes >> complicated to know which memory came from user or kernel, but OTOH, >> it does not create any overhead of drawing from multiple res_counters >> at read time. (and if you want them to be joined, you probably don't care) > > Keeping one counter for user and kernel pages makes it easier for > admins to configure the system. About reporting, we should still > report the user and kernel memory separately. It will be extremely > useful when diagnosing the system like heavily memory pressure or OOM. It will also make us charge two different res_counters, which is not a cheap operation. I was wondering if we can do something smarter within the res_counter itself to avoid taking locks for two different res_counters in the charge path?
creator: Glauber Costa
date: 2012-02-28T19:02:32-00:00

Re: [PATCH 1/7] small cleanup for memcontrol.c
type: item
description: On 02/22/2012 12:01 PM, Glauber Costa wrote: > On 02/22/2012 04:46 AM, KAMEZAWA Hiroyuki wrote: >> On Tue, 21 Feb 2012 15:34:33 +0400 >> Glauber Costa<glommer@parallels.com> wrote: >> >>> Move some hardcoded definitions to an enum type. >>> >>> Signed-off-by: Glauber Costa<glommer@parallels.com> >>> CC: Kirill A. Shutemov<kirill@shutemov.name> >>> CC: Greg Thelen<gthelen@google.com> >>> CC: Johannes Weiner<jweiner@redhat.com> >>> CC: Michal Hocko<mhocko@suse.cz> >>> CC: Hiroyouki Kamezawa<kamezawa.hiroyu@jp.fujitsu.com> >>> CC: Paul Turner<pjt@google.com> >>> CC: Frederic Weisbecker<fweisbec@gmail.com> >> >> seems ok to me. >> >> Acked-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com> > > BTW, this series is likely to go through many rounds of discussion. > This patch can be probably picked separately, if you want to. > >> a nitpick.. >> >>> --- >>> mm/memcontrol.c | 10 +++++++--- >>> 1 files changed, 7 insertions(+), 3 deletions(-) >>> >>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >>> index 6728a7a..b15a693 100644 >>> --- a/mm/memcontrol.c >>> +++ b/mm/memcontrol.c >>> @@ -351,9 +351,13 @@ enum charge_type { >>> }; >>> >>> /* for encoding cft->private value on file */ >>> -#define _MEM (0) >>> -#define _MEMSWAP (1) >>> -#define _OOM_TYPE (2) >>> + >>> +enum mem_type { >>> + _MEM = 0, >> >> =0 is required ? > I believe not, but I always liked to use it to be 100 % explicit. > Personal taste... Can change it, if this is a big deal. Kame, would you like me to send this cleanup without the = 0 ?
creator: Glauber Costa
date: 2012-02-29T17:30:35-00:00

Re: [PATCH 1/7] small cleanup for memcontrol.c
type: item
description: On Wed, 29 Feb 2012 14:30:35 -0300 Glauber Costa <glommer@parallels.com> wrote: > On 02/22/2012 12:01 PM, Glauber Costa wrote: > > On 02/22/2012 04:46 AM, KAMEZAWA Hiroyuki wrote: > >> On Tue, 21 Feb 2012 15:34:33 +0400 > >> Glauber Costa<glommer@parallels.com> wrote: > >> > >>> Move some hardcoded definitions to an enum type. > >>> > >>> Signed-off-by: Glauber Costa<glommer@parallels.com> > >>> CC: Kirill A. Shutemov<kirill@shutemov.name> > >>> CC: Greg Thelen<gthelen@google.com> > >>> CC: Johannes Weiner<jweiner@redhat.com> > >>> CC: Michal Hocko<mhocko@suse.cz> > >>> CC: Hiroyouki Kamezawa<kamezawa.hiroyu@jp.fujitsu.com> > >>> CC: Paul Turner<pjt@google.com> > >>> CC: Frederic Weisbecker<fweisbec@gmail.com> > >> > >> seems ok to me. > >> > >> Acked-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com> > > > > BTW, this series is likely to go through many rounds of discussion. > > This patch can be probably picked separately, if you want to. > > > >> a nitpick.. > >> > >>> --- > >>> mm/memcontrol.c | 10 +++++++--- > >>> 1 files changed, 7 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c > >>> index 6728a7a..b15a693 100644 > >>> --- a/mm/memcontrol.c > >>> +++ b/mm/memcontrol.c > >>> @@ -351,9 +351,13 @@ enum charge_type { > >>> }; > >>> > >>> /* for encoding cft->private value on file */ > >>> -#define _MEM (0) > >>> -#define _MEMSWAP (1) > >>> -#define _OOM_TYPE (2) > >>> + > >>> +enum mem_type { > >>> + _MEM = 0, > >> > >> =0 is required ? > > I believe not, but I always liked to use it to be 100 % explicit. > > Personal taste... Can change it, if this is a big deal. > > Kame, would you like me to send this cleanup without the = 0 ? > Not necessary. Sorry for delayed response. Lots of memcg patches are flying.. Thanks, -Kame
creator: KAMEZAWA Hiroyuki
date: 2012-03-01T02:11:47-00:00