OpenVZ Forum - RDF feed
type: channel
items: [PATCH v3 00/16] slab accounting for memcg
items: [PATCH v3 01/16] slab/slub: struct memcg_params
items: [PATCH v3 02/16] slub: use free_page instead of put_page for freeing kmalloc allocation
items: [PATCH v3 03/16] slab: Ignore the cflgs bit in cache creation
items: [PATCH v3 04/16] provide a common place for initcall processing in kmem_cache
items: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
items: [PATCH v3 06/16] memcg: infrastructure to match an allocation to the right cache
items: [PATCH v3 07/16] memcg: skip memcg kmem allocations in specified code regions
items: [PATCH v3 08/16] slab: allow enable_cpu_cache to use preset values for its tunables
items: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
items: [PATCH v3 10/16] sl[au]b: Allocate objects from memcg cache
items: [PATCH v3 11/16] memcg: destroy memcg caches
items: [PATCH v3 12/16] memcg/sl[au]b Track all the memcg children of a kmem_cache.
items: [PATCH v3 13/16] slab: slab-specific propagation changes.
items: [PATCH v3 14/16] slub: slub-specific propagation changes.
items: [PATCH v3 15/16] memcg/sl[au]b: shrink dead caches
items: [PATCH v3 16/16] Add documentation about the kmem controller
items: Re: [PATCH v3 03/16] slab: Ignore the cflgs bit in cache creation
items: Re: [PATCH v3 04/16] provide a common place for initcall processing in kmem_cache
items: Re: [PATCH v3 08/16] slab: allow enable_cpu_cache to use preset values for its tunables
items: Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
items: Re: [PATCH v3 13/16] slab: slab-specific propagation changes.
items: Re: [PATCH v3 15/16] memcg/sl[au]b: shrink dead caches
items: Re: [PATCH v3 00/16] slab accounting for memcg
items: Re: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
items: Re: [PATCH v3 06/16] memcg: infrastructure to match an allocation to the right cache
items: Re: [PATCH v3 07/16] memcg: skip memcg kmem allocations in specified code regions
items: Re: [PATCH v3 11/16] memcg: destroy memcg caches
items: Re: [PATCH v3 12/16] memcg/sl[au]b Track all the memcg children of a kmem_cache.

[PATCH v3 00/16] slab accounting for memcg
type: item
description: This is a followup to the previous kmem series. I divided them logically so it gets easier for reviewers. But I believe they are ready to be merged together (although we can do a two-pass merge if people would prefer) Throwaway git tree found at: git://git.kernel.org/pub/scm/linux/kernel/git/glommer/memcg. git kmemcg-slab There are mostly bugfixes since last submission. For a detailed explanation about this series, please refer to my previous post (Subj: [PATCH v3 00/13] kmem controller for memcg.) Glauber Costa (16): slab/slub: struct memcg_params slub: use free_page instead of put_page for freeing kmalloc allocation slab: Ignore the cflgs bit in cache creation provide a common place for initcall processing in kmem_cache consider a memcg parameter in kmem_create_cache memcg: infrastructure to match an allocation to the right cache memcg: skip memcg kmem allocations in specified code regions slab: allow enable_cpu_cache to use preset values for its tunables sl[au]b: always get the cache from its page in kfree sl[au]b: Allocate objects from memcg cache memcg: destroy memcg caches memcg/sl[au]b Track all the memcg children of a kmem_cache. slab: slab-specific propagation changes. slub: slub-specific propagation changes. memcg/sl[au]b: shrink dead caches Add documentation about the kmem controller Documentation/cgroups/memory.txt | 73 ++++++- include/linux/memcontrol.h | 60 ++++++ include/linux/sched.h | 1 + include/linux/slab.h | 23 +++ include/linux/slab_def.h | 4 + include/linux/slub_def.h | 18 +- init/Kconfig | 2 +- mm/memcontrol.c | 403 +++++++++++++++++++++++++++++++++++++++ mm/slab.c | 70 ++++++- mm/slab.h | 72 ++++++- mm/slab_common.c | 85 ++++++++- mm/slob.c | 5 + mm/slub.c | 54 ++++-- 13 files changed, 829 insertions(+), 41 deletions(-) -- 1.7.11.4
creator: Glauber Costa
date: 2012-09-18T14:11:54-00:00

[PATCH v3 01/16] slab/slub: struct memcg_params
type: item
description: For the kmem slab controller, we need to record some extra information in the kmem_cache structure. Signed-off-by: Glauber Costa <glommer@parallels.com> Signed-off-by: Suleiman Souhlal <suleiman@google.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Johannes Weiner <hannes@cmpxchg.org> --- include/linux/slab.h | 7 +++++++ include/linux/slab_def.h | 4 ++++ include/linux/slub_def.h | 3 +++ 3 files changed, 14 insertions(+) diff --git a/include/linux/slab.h b/include/linux/slab.h index 0dd2dfa..3152bcd 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -177,6 +177,13 @@ unsigned int kmem_cache_size(struct kmem_cache *); #define ARCH_SLAB_MINALIGN __alignof__(unsigned long long) #endif +#ifdef CONFIG_MEMCG_KMEM +struct mem_cgroup_cache_params { + struct mem_cgroup *memcg; + int id; +}; +#endif + /* * Common kmalloc functions provided by all allocators */ diff --git a/include/linux/slab_def.h b/include/linux/slab_def.h index 0c634fa..39c5e9d 100644 --- a/include/linux/slab_def.h +++ b/include/linux/slab_def.h @@ -83,6 +83,10 @@ struct kmem_cache { int obj_offset; #endif /* CONFIG_DEBUG_SLAB */ +#ifdef CONFIG_MEMCG_KMEM + struct mem_cgroup_cache_params memcg_params; +#endif + /* 6) per-cpu/per-node data, touched during every alloc/free */ /* * We put array[] at the end of kmem_cache, because we want to size diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h index df448ad..8bb8ad2 100644 --- a/include/linux/slub_def.h +++ b/include/linux/slub_def.h @@ -101,6 +101,9 @@ struct kmem_cache { #ifdef CONFIG_SYSFS struct kobject kobj; /* For sysfs */ #endif +#ifdef CONFIG_MEMCG_KMEM + struct mem_cgroup_cache_params memcg_params; +#endif #ifdef CONFIG_NUMA /* -- 1.7.11.4
creator: Glauber Costa
date: 2012-09-18T14:11:55-00:00

[PATCH v3 02/16] slub: use free_page instead of put_page for freeing kmalloc allocation
type: item
description: When freeing objects, the slub allocator will most of the time free empty pages by calling __free_pages(). But high-order kmalloc will be diposed by means of put_page() instead. It makes no sense to call put_page() in kernel pages that are provided by the object allocators, so we shouldn't be doing this ourselves. Aside from the consistency change, we don't change the flow too much. put_page()'s would call its dtor function, which is __free_pages. We also already do all of the Compound page tests ourselves, and the Mlock test we lose don't really matter. [v2: modified Changelog ] Signed-off-by: Glauber Costa <glommer@parallels.com> Acked-by: Christoph Lameter <cl@linux.com> CC: David Rientjes <rientjes@google.com> CC: Pekka Enberg <penberg@kernel.org> --- mm/slub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/slub.c b/mm/slub.c index 9f86353..09a91d0 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -3451,7 +3451,7 @@ void kfree(const void *x) if (unlikely(!PageSlab(page))) { BUG_ON(!PageCompound(page)); kmemleak_free(x); - put_page(page); + __free_pages(page, compound_order(page)); return; } slab_free(page->slab, page, object, _RET_IP_); -- 1.7.11.4
creator: Glauber Costa
date: 2012-09-18T14:11:56-00:00

[PATCH v3 03/16] slab: Ignore the cflgs bit in cache creation
type: item
description: No cache should ever pass that as a creation flag, since this bit is used to mark an internal decision of the slab about object placement. We can just ignore this bit if it happens to be passed (such as when duplicating a cache in the kmem memcg patches) Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: David Rientjes <rientjes@google.com> --- mm/slab.c | 1 + 1 file changed, 1 insertion(+) diff --git a/mm/slab.c b/mm/slab.c index a7ed60f..ccf496c 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -2373,6 +2373,7 @@ __kmem_cache_create (struct kmem_cache *cachep, unsigned long flags) int err; size_t size = cachep->size; + flags &= ~CFLGS_OFF_SLAB; #if DEBUG #if FORCED_DEBUG /* -- 1.7.11.4
creator: Glauber Costa
date: 2012-09-18T14:11:57-00:00

[PATCH v3 04/16] provide a common place for initcall processing in kmem_cache
type: item
description: Both SLAB and SLUB depend on some initialization to happen when the system is already booted, with all subsystems working. This is done by issuing an initcall that does the final initialization. This patch moves that to slab_common.c, while creating an empty placeholder for the SLOB. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: David Rientjes <rientjes@google.com> --- mm/slab.c | 5 ++--- mm/slab.h | 2 ++ mm/slab_common.c | 6 ++++++ mm/slob.c | 5 +++++ mm/slub.c | 4 +--- 5 files changed, 16 insertions(+), 6 deletions(-) diff --git a/mm/slab.c b/mm/slab.c index ccf496c..3bac667 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -894,7 +894,7 @@ static void __cpuinit start_cpu_timer(int cpu) struct delayed_work *reap_work = &per_cpu(slab_reap_work, cpu); /* - * When this gets called from do_initcalls via cpucache_init(), + * When this gets called from do_initcalls via __kmem_cache_initcall(), * init_workqueues() has already run, so keventd will be setup * at that time. */ @@ -1822,7 +1822,7 @@ void __init kmem_cache_init_late(void) */ } -static int __init cpucache_init(void) +int __init __kmem_cache_initcall(void) { int cpu; @@ -1836,7 +1836,6 @@ static int __init cpucache_init(void) slab_state = FULL; return 0; } -__initcall(cpucache_init); static noinline void slab_out_of_memory(struct kmem_cache *cachep, gfp_t gfpflags, int nodeid) diff --git a/mm/slab.h b/mm/slab.h index 7deeb44..7a2698b 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -47,4 +47,6 @@ static inline struct kmem_cache *__kmem_cache_alias(const char *name, size_t siz int __kmem_cache_shutdown(struct kmem_cache *); +int __kmem_cache_initcall(void); + #endif diff --git a/mm/slab_common.c b/mm/slab_common.c index 9c21725..eddbb8a 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -189,3 +189,9 @@ int slab_is_available(void) { return slab_state >= UP; } + +static int __init kmem_cache_initcall(void) +{ + return __kmem_cache_ini…
creator: Glauber Costa
date: 2012-09-18T14:11:58-00:00

[PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
type: item
description: Allow a memcg parameter to be passed during cache creation. When the slub allocator is being used, it will only merge caches that belong to the same memcg. Default function is created as a wrapper, passing NULL to the memcg version. We only merge caches that belong to the same memcg. >From the memcontrol.c side, 3 helper functions are created: 1) memcg_css_id: because slub needs a unique cache name for sysfs. Since this is visible, but not the canonical location for slab data, the cache name is not used, the css_id should suffice. 2) mem_cgroup_register_cache: is responsible for assigning a unique index to each cache, and other general purpose setup. The index is only assigned for the root caches. All others are assigned index == -1. 3) mem_cgroup_release_cache: can be called from the root cache destruction, and will release the index for other caches. We can't assign indexes until the basic slab is up and running this is because the ida subsystem will itself call slab functions such as kmalloc a couple of times. Because of that, we have a late_initcall that scan all caches and register them after the kernel is booted up. Only caches registered after that receive their index right away. This index mechanism was developed by Suleiman Souhlal. Changed to a idr/ida based approach based on suggestion from Kamezawa. [ v2: moved to idr/ida instead of redoing the indexes ] [ v3: moved call to ida_init away from cgroup creation to fix a bug ] Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Johannes Weiner <hannes@cmpxchg.org> CC: Suleiman Souhlal <suleiman@google.com> --- include/linux/memcontrol.h | 20 ++++++++++++++++++++ include/linux/slab.h | 8 ++++++++ mm/memcontrol.c | 28 +++++++++++++++++++++++++++ mm/slab.c | 1 + mm/slab.h | 26 +++++++++++++++++++++---- mm/slab_common.c | 47 +++++++++++++++++…
creator: Glauber Costa
date: 2012-09-18T14:11:59-00:00

[PATCH v3 06/16] memcg: infrastructure to match an allocation to the right cache
type: item
description: The page allocator is able to bind a page to a memcg when it is allocated. But for the caches, we'd like to have as many objects as possible in a page belonging to the same cache. This is done in this patch by calling memcg_kmem_get_cache in the beginning of every allocation function. This routing is patched out by static branches when kernel memory controller is not being used. It assumes that the task allocating, which determines the memcg in the page allocator, belongs to the same cgroup throughout the whole process. Misacounting can happen if the task calls memcg_kmem_get_cache() while belonging to a cgroup, and later on changes. This is considered acceptable, and should only happen upon task migration. Before the cache is created by the memcg core, there is also a possible imbalance: the task belongs to a memcg, but the cache being allocated from is the global cache, since the child cache is not yet guaranteed to be ready. This case is also fine, since in this case the GFP_KMEMCG will not be passed and the page allocator will not attempt any cgroup accounting. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Johannes Weiner <hannes@cmpxchg.org> CC: Suleiman Souhlal <suleiman@google.com> --- include/linux/memcontrol.h | 38 +++++++++ init/Kconfig | 2 +- mm/memcontrol.c | 203 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 242 insertions(+), 1 deletion(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index a5f3055..c44a5f2 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -419,6 +419,8 @@ extern void memcg_init_kmem_cache(void); extern void memcg_register_cache(struct mem_cgroup *memcg, struct kmem_cache *s); extern void memcg_release_cache(struct kmem_cache *cachep); +struct kmem_cache * +__memcg_kmem_get_cache(struct kmem_cach…
creator: Glauber Costa
date: 2012-09-18T14:12:00-00:00

[PATCH v3 07/16] memcg: skip memcg kmem allocations in specified code regions
type: item
description: This patch creates a mechanism that skip memcg allocations during certain pieces of our core code. It basically works in the same way as preempt_disable()/preempt_enable(): By marking a region under which all allocations will be accounted to the root memcg. We need this to prevent races in early cache creation, when we allocate data using caches that are not necessarily created already. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Johannes Weiner <hannes@cmpxchg.org> CC: Suleiman Souhlal <suleiman@google.com> --- include/linux/sched.h | 1 + mm/memcontrol.c | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/include/linux/sched.h b/include/linux/sched.h index 81967b1..4c983c6 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1574,6 +1574,7 @@ struct task_struct { unsigned long nr_pages; /* uncharged usage */ unsigned long memsw_nr_pages; /* uncharged mem+swap usage */ } memcg_batch; + unsigned int memcg_kmem_skip_account; #endif #ifdef CONFIG_HAVE_HW_BREAKPOINT atomic_t ptrace_bp_refcnt; diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 1cce5c3..54247ec 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -544,6 +544,22 @@ static inline bool memcg_can_account_kmem(struct mem_cgroup *memcg) (memcg->kmem_accounted & (KMEM_ACCOUNTED_MASK)); } +static void memcg_stop_kmem_account(void) +{ + if (!current->mm) + return; + + current->memcg_kmem_skip_account++; +} + +static void memcg_resume_kmem_account(void) +{ + if (!current->mm) + return; + + current->memcg_kmem_skip_account--; +} + static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *cachep) { char *name; @@ -721,7 +737,9 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, if (new_cachep) goto out; + memcg_stop_kmem_account(); new_cachep = kmem_cache_…
creator: Glauber Costa
date: 2012-09-18T14:12:01-00:00

[PATCH v3 08/16] slab: allow enable_cpu_cache to use preset values for its tunables
type: item
description: SLAB allows us to tune a particular cache behavior with tunables. When creating a new memcg cache copy, we'd like to preserve any tunables the parent cache already had. This could be done by an explicit call to do_tune_cpucache() after the cache is created. But this is not very convenient now that the caches are created from common code, since this function is SLAB-specific. Another method of doing that is taking advantage of the fact that do_tune_cpucache() is always called from enable_cpucache(), which is called at cache initialization. We can just preset the values, and then things work as expected. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Johannes Weiner <hannes@cmpxchg.org> CC: Suleiman Souhlal <suleiman@google.com> --- include/linux/slab.h | 3 ++- mm/memcontrol.c | 2 +- mm/slab.c | 19 ++++++++++++++++--- mm/slab_common.c | 6 ++++-- 4 files changed, 23 insertions(+), 7 deletions(-) diff --git a/include/linux/slab.h b/include/linux/slab.h index dc6daac..9d298db 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -128,7 +128,7 @@ struct kmem_cache *kmem_cache_create(const char *, size_t, size_t, void (*)(void *)); struct kmem_cache * kmem_cache_create_memcg(struct mem_cgroup *, const char *, size_t, size_t, - unsigned long, void (*)(void *)); + unsigned long, void (*)(void *), struct kmem_cache *); void kmem_cache_destroy(struct kmem_cache *); int kmem_cache_shrink(struct kmem_cache *); void kmem_cache_free(struct kmem_cache *, void *); @@ -184,6 +184,7 @@ unsigned int kmem_cache_size(struct kmem_cache *); #ifdef CONFIG_MEMCG_KMEM struct mem_cgroup_cache_params { struct mem_cgroup *memcg; + struct kmem_cache *parent; int id; }; #endif diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 54247ec..ee982aa 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -588,7 +588,7 …
creator: Glauber Costa
date: 2012-09-18T14:12:02-00:00

[PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
type: item
description: struct page already have this information. If we start chaining caches, this information will always be more trustworthy than whatever is passed into the function A parent pointer is added to the slub structure, so we can make sure the freeing comes from either the right slab, or from its rightful parent. [ v3: added parent testing with VM_BUG_ON ] Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> --- mm/slab.c | 5 ++++- mm/slab.h | 11 +++++++++++ mm/slub.c | 4 +++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/mm/slab.c b/mm/slab.c index f2d760c..18de3f6 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -3938,9 +3938,12 @@ EXPORT_SYMBOL(__kmalloc); * Free an object which was previously allocated from this * cache. */ -void kmem_cache_free(struct kmem_cache *cachep, void *objp) +void kmem_cache_free(struct kmem_cache *s, void *objp) { unsigned long flags; + struct kmem_cache *cachep = virt_to_cache(objp); + + VM_BUG_ON(!slab_equal_or_parent(cachep, s)); local_irq_save(flags); debug_check_no_locks_freed(objp, cachep->object_size); diff --git a/mm/slab.h b/mm/slab.h index 6f2a34d..f2501ab 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -60,11 +60,22 @@ static inline bool cache_match_memcg(struct kmem_cache *cachep, return cachep->memcg_params.memcg == memcg; } +static inline bool slab_equal_or_parent(struct kmem_cache *s, + struct kmem_cache *p) +{ + return (p == s) || (p == s->memcg_params.parent); +} #else static inline bool cache_match_memcg(struct kmem_cache *cachep, struct mem_cgroup *memcg) { return true; } + +static inline bool slab_equal_or_parent(struct kmem_cache *s, + struct kmem_cache *p) +{ + return true; +} #endif #endif diff --git a/mm/slub.c b/mm/slub.c index 4778548..a045dfc 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -2604,7 +2604,9 @@ void kmem_cache_free(struct kmem_cache *s, void *x) page = virt_to_head_page(x); - slab_free(s, page, x, _RET_IP_); + VM_BUG_ON(!slab_e…
creator: Glauber Costa
date: 2012-09-18T14:12:03-00:00

[PATCH v3 10/16] sl[au]b: Allocate objects from memcg cache
type: item
description: We are able to match a cache allocation to a particular memcg. If the task doesn't change groups during the allocation itself - a rare event, this will give us a good picture about who is the first group to touch a cache page. This patch uses the now available infrastructure by calling memcg_kmem_get_cache() before all the cache allocations. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Johannes Weiner <hannes@cmpxchg.org> CC: Suleiman Souhlal <suleiman@google.com> --- include/linux/slub_def.h | 15 ++++++++++----- mm/memcontrol.c | 9 +++++++++ mm/slab.c | 6 +++++- mm/slub.c | 5 +++-- 4 files changed, 27 insertions(+), 8 deletions(-) diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h index 8bb8ad2..76bf6da 100644 --- a/include/linux/slub_def.h +++ b/include/linux/slub_def.h @@ -13,6 +13,8 @@ #include <linux/kobject.h> #include <linux/kmemleak.h> +#include <linux/memcontrol.h> +#include <linux/mm.h> enum stat_item { ALLOC_FASTPATH, /* Allocation from cpu slab */ @@ -209,14 +211,14 @@ static __always_inline int kmalloc_index(size_t size) * This ought to end up with a global pointer to the right cache * in kmalloc_caches. */ -static __always_inline struct kmem_cache *kmalloc_slab(size_t size) +static __always_inline struct kmem_cache *kmalloc_slab(gfp_t flags, size_t size) { int index = kmalloc_index(size); if (index == 0) return NULL; - return kmalloc_caches[index]; + return memcg_kmem_get_cache(kmalloc_caches[index], flags); } void *kmem_cache_alloc(struct kmem_cache *, gfp_t); @@ -225,7 +227,10 @@ void *__kmalloc(size_t size, gfp_t flags); static __always_inline void * kmalloc_order(size_t size, gfp_t flags, unsigned int order) { - void *ret = (void *) __get_free_pages(flags | __GFP_COMP, order); + void *ret; + + flags |= (__GFP_COMP | __GFP_KMEMCG); + ret = (void *) __get_free…
creator: Glauber Costa
date: 2012-09-18T14:12:04-00:00

[PATCH v3 11/16] memcg: destroy memcg caches
type: item
description: This patch implements destruction of memcg caches. Right now, only caches where our reference counter is the last remaining are deleted. If there are any other reference counters around, we just leave the caches lying around until they go away. When that happen, a destruction function is called from the cache code. Caches are only destroyed in process context, so we queue them up for later processing in the general case. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Johannes Weiner <hannes@cmpxchg.org> CC: Suleiman Souhlal <suleiman@google.com> --- include/linux/memcontrol.h | 1 + include/linux/slab.h | 3 ++ mm/memcontrol.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++ mm/slab.c | 3 ++ mm/slab.h | 23 +++++++++++++ mm/slub.c | 7 +++- 6 files changed, 120 insertions(+), 1 deletion(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index c44a5f2..204a43a 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -421,6 +421,7 @@ extern void memcg_register_cache(struct mem_cgroup *memcg, extern void memcg_release_cache(struct kmem_cache *cachep); struct kmem_cache * __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp); +void mem_cgroup_destroy_cache(struct kmem_cache *cachep); #else static inline void memcg_init_kmem_cache(void) diff --git a/include/linux/slab.h b/include/linux/slab.h index 9d298db..9a41d85 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -186,6 +186,9 @@ struct mem_cgroup_cache_params { struct mem_cgroup *memcg; struct kmem_cache *parent; int id; + bool dead; + atomic_t nr_pages; + struct list_head destroyed_list; /* Used when deleting memcg cache */ }; #endif diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 0068b7d..a184d42 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -614,6 +614,8 @@ void memcg_r…
creator: Glauber Costa
date: 2012-09-18T14:12:05-00:00

[PATCH v3 12/16] memcg/sl[au]b Track all the memcg children of a kmem_cache.
type: item
description: This enables us to remove all the children of a kmem_cache being destroyed, if for example the kernel module it's being used in gets unloaded. Otherwise, the children will still point to the destroyed parent. Signed-off-by: Suleiman Souhlal <suleiman@google.com> Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Johannes Weiner <hannes@cmpxchg.org> --- include/linux/memcontrol.h | 1 + include/linux/slab.h | 1 + mm/memcontrol.c | 16 +++++++++++++++- mm/slab_common.c | 31 +++++++++++++++++++++++++++++++ 4 files changed, 48 insertions(+), 1 deletion(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 204a43a..6d5e212 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -422,6 +422,7 @@ extern void memcg_release_cache(struct kmem_cache *cachep); struct kmem_cache * __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp); void mem_cgroup_destroy_cache(struct kmem_cache *cachep); +void mem_cgroup_remove_child_kmem_cache(struct kmem_cache *cachep, int id); #else static inline void memcg_init_kmem_cache(void) diff --git a/include/linux/slab.h b/include/linux/slab.h index 9a41d85..9badb8c 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -189,6 +189,7 @@ struct mem_cgroup_cache_params { bool dead; atomic_t nr_pages; struct list_head destroyed_list; /* Used when deleting memcg cache */ + struct list_head sibling_list; }; #endif diff --git a/mm/memcontrol.c b/mm/memcontrol.c index a184d42..da38652 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -596,8 +596,11 @@ static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg, new = kmem_cache_create_memcg(memcg, name, s->object_size, s->align, (s->flags & ~SLAB_PANIC), s->ctor, s); - if (new) + if (new) { new->allocflags |= __GFP_KMEMCG; + list_add(&new->memcg_params.sibling_list, …
creator: Glauber Costa
date: 2012-09-18T14:12:06-00:00

[PATCH v3 13/16] slab: slab-specific propagation changes.
type: item
description: When a parent cache does tune_cpucache, we need to propagate that to the children as well. For that, we unfortunately need to tap into the slab core. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Johannes Weiner <hannes@cmpxchg.org> CC: Suleiman Souhlal <suleiman@google.com> --- mm/slab.c | 28 +++++++++++++++++++++++++++- mm/slab_common.c | 1 + 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/mm/slab.c b/mm/slab.c index d5c196b..bd9928f 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -4104,7 +4104,7 @@ static void do_ccupdate_local(void *info) } /* Always called with the slab_mutex held */ -static int do_tune_cpucache(struct kmem_cache *cachep, int limit, +static int __do_tune_cpucache(struct kmem_cache *cachep, int limit, int batchcount, int shared, gfp_t gfp) { struct ccupdate_struct *new; @@ -4147,6 +4147,32 @@ static int do_tune_cpucache(struct kmem_cache *cachep, int limit, return alloc_kmemlist(cachep, gfp); } +static int do_tune_cpucache(struct kmem_cache *cachep, int limit, + int batchcount, int shared, gfp_t gfp) +{ + int ret; +#ifdef CONFIG_MEMCG_KMEM + struct kmem_cache *c; + struct mem_cgroup_cache_params *p; +#endif + + ret = __do_tune_cpucache(cachep, limit, batchcount, shared, gfp); +#ifdef CONFIG_MEMCG_KMEM + if (slab_state < FULL) + return ret; + + if ((ret < 0) || (cachep->memcg_params.id == -1)) + return ret; + + list_for_each_entry(p, &cachep->memcg_params.sibling_list, sibling_list) { + c = container_of(p, struct kmem_cache, memcg_params); + /* return value determined by the parent cache only */ + __do_tune_cpucache(c, limit, batchcount, shared, gfp); + } +#endif + return ret; +} + /* Called with slab_mutex held always */ static int enable_cpucache(struct kmem_cache *cachep, gfp_t gfp) { diff --git a/mm/slab_common.c b/mm/slab_common.c index c6fb4a7..341c3e1 1006…
creator: Glauber Costa
date: 2012-09-18T14:12:07-00:00

[PATCH v3 14/16] slub: slub-specific propagation changes.
type: item
description: When a parent cache changes a sysfs attr, we need to propagate that to the children as well. For that, we unfortunately need to tap into the slub core. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Johannes Weiner <hannes@cmpxchg.org> CC: Suleiman Souhlal <suleiman@google.com> --- mm/slub.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/mm/slub.c b/mm/slub.c index f90f612..0b68d15 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -5174,6 +5174,10 @@ static ssize_t slab_attr_store(struct kobject *kobj, struct slab_attribute *attribute; struct kmem_cache *s; int err; +#ifdef CONFIG_MEMCG_KMEM + struct kmem_cache *c; + struct mem_cgroup_cache_params *p; +#endif attribute = to_slab_attr(attr); s = to_slab(kobj); @@ -5182,7 +5186,19 @@ static ssize_t slab_attr_store(struct kobject *kobj, return -EIO; err = attribute->store(s, buf, len); +#ifdef CONFIG_MEMCG_KMEM + if (slab_state < FULL) + return err; + if ((err < 0) || (s->memcg_params.id == -1)) + return err; + + list_for_each_entry(p, &s->memcg_params.sibling_list, sibling_list) { + c = container_of(p, struct kmem_cache, memcg_params); + /* return value determined by the parent cache only */ + attribute->store(c, buf, len); + } +#endif return err; } -- 1.7.11.4
creator: Glauber Costa
date: 2012-09-18T14:12:08-00:00

[PATCH v3 15/16] memcg/sl[au]b: shrink dead caches
type: item
description: In the slub allocator, when the last object of a page goes away, we don't necessarily free it - there is not necessarily a test for empty page in any slab_free path. This means that when we destroy a memcg cache that happened to be empty, those caches may take a lot of time to go away: removing the memcg reference won't destroy them - because there are pending references, and the empty pages will stay there, until a shrinker is called upon for any reason. This patch marks all memcg caches as dead. kmem_cache_shrink is called for the ones who are not yet dead - this will force internal cache reorganization, and then all references to empty pages will be removed. An unlikely branch is used to make sure this case does not affect performance in the usual slab_free path. The slab allocator has a time based reaper that would eventually get rid of the objects, but we can also call it explicitly, since dead caches are not a likely event. [ v2: also call verify_dead for the slab ] Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Johannes Weiner <hannes@cmpxchg.org> CC: Suleiman Souhlal <suleiman@google.com> --- include/linux/slab.h | 3 +++ mm/memcontrol.c | 44 +++++++++++++++++++++++++++++++++++++++++++- mm/slab.c | 2 ++ mm/slab.h | 10 ++++++++++ mm/slub.c | 1 + 5 files changed, 59 insertions(+), 1 deletion(-) diff --git a/include/linux/slab.h b/include/linux/slab.h index 9badb8c..765e12c 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -182,6 +182,8 @@ unsigned int kmem_cache_size(struct kmem_cache *); #endif #ifdef CONFIG_MEMCG_KMEM +#include <linux/workqueue.h> + struct mem_cgroup_cache_params { struct mem_cgroup *memcg; struct kmem_cache *parent; @@ -190,6 +192,7 @@ struct mem_cgroup_cache_params { atomic_t nr_pages; struct list_head destroyed_list; /* Used when deleting memcg cache …
creator: Glauber Costa
date: 2012-09-18T14:12:09-00:00

[PATCH v3 16/16] Add documentation about the kmem controller
type: item
description: Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Randy Dunlap <rdunlap@xenotime.net> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Johannes Weiner <hannes@cmpxchg.org> CC: Suleiman Souhlal <suleiman@google.com> CC: Mel Gorman <mgorman@suse.de> --- Documentation/cgroups/memory.txt | 73 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 72 insertions(+), 1 deletion(-) diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt index 4372e6b..6356379 100644 --- a/Documentation/cgroups/memory.txt +++ b/Documentation/cgroups/memory.txt @@ -71,6 +71,11 @@ Brief summary of control files. memory.oom_control # set/show oom controls. memory.numa_stat # show the number of memory usage per numa node + memory.kmem.limit_in_bytes # set/show hard limit for kernel memory + memory.kmem.usage_in_bytes # show current kernel memory allocation + memory.kmem.failcnt # show the number of kernel memory usage hits limits + memory.kmem.max_usage_in_bytes # show max kernel memory usage recorded + memory.kmem.tcp.limit_in_bytes # set/show hard limit for tcp buf memory memory.kmem.tcp.usage_in_bytes # show current tcp buf memory allocation memory.kmem.tcp.failcnt # show the number of tcp buf memory usage hits limits @@ -268,20 +273,80 @@ the amount of kernel memory used by the system. Kernel memory is fundamentally different than user memory, since it can't be swapped out, which makes it possible to DoS the system by consuming too much of this precious resource. +Kernel memory won't be accounted at all until it is limited. This allows for +existing setups to continue working without disruption. Note that it is +possible to account it without an effective limit by setting the limits +to a very high number (like RESOURCE_MAX -1page). After a controller is first +limited, it will be kept being accounted until it is removed. The memory +lim…
creator: Glauber Costa
date: 2012-09-18T14:12:10-00:00

Re: [PATCH v3 03/16] slab: Ignore the cflgs bit in cache creation
type: item
description: On Tue, 18 Sep 2012, Glauber Costa wrote: > No cache should ever pass that as a creation flag, since this bit is > used to mark an internal decision of the slab about object placement. We > can just ignore this bit if it happens to be passed (such as when > duplicating a cache in the kmem memcg patches) If we do this then I would like to see a general masking of internal allocator bits in kmem_cache_create. We could declare the highest byte to be the internal slab flags. SLUB uses two flags in that area. SLAB uses one. F.e. add #define SLAB_INTERNAL 0xFF00000000UL to slab.h. Then the flags can then be masked in mm/slab_common.c
creator: Christoph Lameter
date: 2012-09-18T15:20:11-00:00

Re: [PATCH v3 04/16] provide a common place for initcall processing in kmem_cache
type: item
description: On Tue, 18 Sep 2012, Glauber Costa wrote: > Both SLAB and SLUB depend on some initialization to happen when the > system is already booted, with all subsystems working. This is done > by issuing an initcall that does the final initialization. > > This patch moves that to slab_common.c, while creating an empty > placeholder for the SLOB. Acked-by: Christoph Lameter <cl@linux.com>
creator: Christoph Lameter
date: 2012-09-18T15:22:00-00:00

Re: [PATCH v3 08/16] slab: allow enable_cpu_cache to use preset values for its tunables
type: item
description: On Tue, 18 Sep 2012, Glauber Costa wrote: > SLAB allows us to tune a particular cache behavior with tunables. > When creating a new memcg cache copy, we'd like to preserve any tunables > the parent cache already had. Again the same is true for SLUB. Some generic way of preserving tuning parameters would be appreciated.
creator: Christoph Lameter
date: 2012-09-18T15:25:54-00:00

Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
type: item
description: On Tue, 18 Sep 2012, Glauber Costa wrote: > index f2d760c..18de3f6 100644 > --- a/mm/slab.c > +++ b/mm/slab.c > @@ -3938,9 +3938,12 @@ EXPORT_SYMBOL(__kmalloc); > * Free an object which was previously allocated from this > * cache. > */ > -void kmem_cache_free(struct kmem_cache *cachep, void *objp) > +void kmem_cache_free(struct kmem_cache *s, void *objp) > { > unsigned long flags; > + struct kmem_cache *cachep = virt_to_cache(objp); > + > + VM_BUG_ON(!slab_equal_or_parent(cachep, s)); > This is an extremely hot path of the kernel and you are adding significant processing. Check how the benchmarks are influenced by this change. virt_to_cache can be a bit expensive. > diff --git a/mm/slub.c b/mm/slub.c > index 4778548..a045dfc 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -2604,7 +2604,9 @@ void kmem_cache_free(struct kmem_cache *s, void *x) > > page = virt_to_head_page(x); > > - slab_free(s, page, x, _RET_IP_); > + VM_BUG_ON(!slab_equal_or_parent(page->slab, s)); > + > + slab_free(page->slab, page, x, _RET_IP_); > Less of a problem here but you are eroding one advantage that slab has had in the past over slub in terms of freeing objects.
creator: Christoph Lameter
date: 2012-09-18T15:28:19-00:00

Re: [PATCH v3 13/16] slab: slab-specific propagation changes.
type: item
description: On Tue, 18 Sep 2012, Glauber Costa wrote: > When a parent cache does tune_cpucache, we need to propagate that to the > children as well. For that, we unfortunately need to tap into the slab core. One of the todo list items for the common stuff is to have actually a common kmem_cache structure. If we add a common callback then we could put that also into the core.
creator: Christoph Lameter
date: 2012-09-18T17:00:20-00:00

Re: [PATCH v3 15/16] memcg/sl[au]b: shrink dead caches
type: item
description: Why doesnt slab need that too? It keeps a number of free pages on the per node lists until shrink is called.
creator: Christoph Lameter
date: 2012-09-18T17:02:19-00:00

Re: [PATCH v3 15/16] memcg/sl[au]b: shrink dead caches
type: item
description: On 09/18/2012 09:02 PM, Christoph Lameter wrote: > Why doesnt slab need that too? It keeps a number of free pages on the per > node lists until shrink is called. > You have already given me this feedback, and I forgot to include it here. I am sorry for this slip. It was my intention to this for the slab as well. Thanks for the eyes!
creator: Glauber Costa
date: 2012-09-19T07:40:26-00:00

Re: [PATCH v3 13/16] slab: slab-specific propagation changes.
type: item
description: On 09/18/2012 09:00 PM, Christoph Lameter wrote: > On Tue, 18 Sep 2012, Glauber Costa wrote: > >> When a parent cache does tune_cpucache, we need to propagate that to the >> children as well. For that, we unfortunately need to tap into the slab core. > > One of the todo list items for the common stuff is to have actually a > common kmem_cache structure. If we add a common callback then we could put > that also into the core. > At this point, I'd like to keep it this way if possible. I agree that it would be a lot better living in a common location, and I commit to helping you to move in that direction. But I see no reason to have this commonization to happen first, as a pre-requisite for this.
creator: Glauber Costa
date: 2012-09-19T07:41:30-00:00

Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
type: item
description: On 09/18/2012 07:28 PM, Christoph Lameter wrote: > On Tue, 18 Sep 2012, Glauber Costa wrote: > >> index f2d760c..18de3f6 100644 >> --- a/mm/slab.c >> +++ b/mm/slab.c >> @@ -3938,9 +3938,12 @@ EXPORT_SYMBOL(__kmalloc); >> * Free an object which was previously allocated from this >> * cache. >> */ >> -void kmem_cache_free(struct kmem_cache *cachep, void *objp) >> +void kmem_cache_free(struct kmem_cache *s, void *objp) >> { >> unsigned long flags; >> + struct kmem_cache *cachep = virt_to_cache(objp); >> + >> + VM_BUG_ON(!slab_equal_or_parent(cachep, s)); >> > > This is an extremely hot path of the kernel and you are adding significant > processing. Check how the benchmarks are influenced by this change. > virt_to_cache can be a bit expensive. > Would it be enough for you to have a separate code path for !CONFIG_MEMCG_KMEM? I don't really see another way to do it, aside from deriving the cache from the object in our case. I am open to suggestions if you do. >> diff --git a/mm/slub.c b/mm/slub.c >> index 4778548..a045dfc 100644 >> --- a/mm/slub.c >> +++ b/mm/slub.c >> @@ -2604,7 +2604,9 @@ void kmem_cache_free(struct kmem_cache *s, void *x) >> >> page = virt_to_head_page(x); >> >> - slab_free(s, page, x, _RET_IP_); >> + VM_BUG_ON(!slab_equal_or_parent(page->slab, s)); >> + >> + slab_free(page->slab, page, x, _RET_IP_); >> > > Less of a problem here but you are eroding one advantage that slab has had > in the past over slub in terms of freeing objects. > likewise.
creator: Glauber Costa
date: 2012-09-19T07:42:54-00:00

Re: [PATCH v3 08/16] slab: allow enable_cpu_cache to use preset values for its tunables
type: item
description: On 09/18/2012 07:25 PM, Christoph Lameter wrote: > On Tue, 18 Sep 2012, Glauber Costa wrote: > >> SLAB allows us to tune a particular cache behavior with tunables. >> When creating a new memcg cache copy, we'd like to preserve any tunables >> the parent cache already had. > > Again the same is true for SLUB. Some generic way of preserving tuning > parameters would be appreciated. So you would like me to extend "slub: slub-specific propagation changes" to also allow for pre-set values, right?
creator: Glauber Costa
date: 2012-09-19T07:44:24-00:00

Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
type: item
description: On Wed, 19 Sep 2012, Glauber Costa wrote: > > This is an extremely hot path of the kernel and you are adding significant > > processing. Check how the benchmarks are influenced by this change. > > virt_to_cache can be a bit expensive. > Would it be enough for you to have a separate code path for > !CONFIG_MEMCG_KMEM? Yes, at least add an #ifdef around this. > I don't really see another way to do it, aside from deriving the cache > from the object in our case. I am open to suggestions if you do. Rethink the whole memcg approach and find some other way to do it? This whole scheme is very intrusive and is likely to increase instability in the VM given the explosion of lru lists, duplication of slab caches and significantly more complex processing throughout the VM.
creator: Christoph Lameter
date: 2012-09-19T14:14:23-00:00

Re: [PATCH v3 15/16] memcg/sl[au]b: shrink dead caches
type: item
description: Hi Glauber. 2012/9/18 Glauber Costa <glommer@parallels.com>: > diff --git a/mm/slub.c b/mm/slub.c > index 0b68d15..9d79216 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -2602,6 +2602,7 @@ redo: > } else > __slab_free(s, page, x, addr); > > + kmem_cache_verify_dead(s); > } As far as u know, I am not a expert and don't know anything about memcg. IMHO, this implementation may hurt system performance in some case. In case of memcg is destoried, remained kmem_cache is marked "dead". After it is marked, every free operation to this "dead" kmem_cache call kmem_cache_verify_dead() and finally call kmem_cache_shrink(). kmem_cache_shrink() do invoking kmalloc and flush_all() and taking a lock for online node and invoking kfree. Especially, flush_all() may hurt performance largely, because it call has_cpu_slab() against all the cpus. And I know some other case it can hurt system performance. But, I don't mention it, because above case is sufficient to worry. And, I found one case that destroying memcg's kmem_cache don't works properly. If we destroy memcg after all object is freed, current implementation doesn't destroy kmem_cache. kmem_cache_destroy_work_func() check "cachep->memcg_params.nr_pages == 0", but in this case, it return false, because kmem_cache may have cpu_slab, and cpu_partials_slabs. As we already free all objects, kmem_cache_verify_dead() is not invoked forever. I think that we need another kmem_cache_shrink() in kmem_cache_destroy_work_func(). I don't convince that I am right, so think carefully my humble opinion. Thanks.
creator: JoonSoo Kim
date: 2012-09-21T04:48:30-00:00

Re: [PATCH v3 15/16] memcg/sl[au]b: shrink dead caches
type: item
description: On 09/21/2012 08:48 AM, JoonSoo Kim wrote: > Hi Glauber. > Hi > 2012/9/18 Glauber Costa <glommer@parallels.com>: >> diff --git a/mm/slub.c b/mm/slub.c >> index 0b68d15..9d79216 100644 >> --- a/mm/slub.c >> +++ b/mm/slub.c >> @@ -2602,6 +2602,7 @@ redo: >> } else >> __slab_free(s, page, x, addr); >> >> + kmem_cache_verify_dead(s); >> } > > As far as u know, I am not a expert and don't know anything about memcg. > IMHO, this implementation may hurt system performance in some case. > > In case of memcg is destoried, remained kmem_cache is marked "dead". > After it is marked, > every free operation to this "dead" kmem_cache call > kmem_cache_verify_dead() and finally call kmem_cache_shrink(). As long as it is restricted to that cache, this is a non issue. dead caches are exactly what they name imply: dead. Means that we actively want them to go away, and just don't kill them right away because they have some inflight objects - which we expect not to be too much. > kmem_cache_shrink() do invoking kmalloc and flush_all() and taking a > lock for online node and invoking kfree. > Especially, flush_all() may hurt performance largely, because it call > has_cpu_slab() against all the cpus. Again, this is all right, but being a dead cache, it shouldn't be on any hot path. > > And, I found one case that destroying memcg's kmem_cache don't works properly. > If we destroy memcg after all object is freed, current implementation > doesn't destroy kmem_cache. > kmem_cache_destroy_work_func() check "cachep->memcg_params.nr_pages == 0", > but in this case, it return false, because kmem_cache may have > cpu_slab, and cpu_partials_slabs. > As we already free all objects, kmem_cache_verify_dead() is not invoked forever. > I think that we need another kmem_cache_shrink() in > kmem_cache_destroy_work_func(). I'll take a look here. What you describe makes sense, and can potentially happen. I tried to handle this case with care in destroy_all_caches, but I may have always made a mistake... Di…
creator: Glauber Costa
date: 2012-09-21T08:40:04-00:00

Re: [PATCH v3 15/16] memcg/sl[au]b: shrink dead caches
type: item
description: Hi, Glauber. >> 2012/9/18 Glauber Costa <glommer@parallels.com>: >>> diff --git a/mm/slub.c b/mm/slub.c >>> index 0b68d15..9d79216 100644 >>> --- a/mm/slub.c >>> +++ b/mm/slub.c >>> @@ -2602,6 +2602,7 @@ redo: >>> } else >>> __slab_free(s, page, x, addr); >>> >>> + kmem_cache_verify_dead(s); >>> } >> >> As far as u know, I am not a expert and don't know anything about memcg. >> IMHO, this implementation may hurt system performance in some case. >> >> In case of memcg is destoried, remained kmem_cache is marked "dead". >> After it is marked, >> every free operation to this "dead" kmem_cache call >> kmem_cache_verify_dead() and finally call kmem_cache_shrink(). > > As long as it is restricted to that cache, this is a non issue. > dead caches are exactly what they name imply: dead. > > Means that we actively want them to go away, and just don't kill them > right away because they have some inflight objects - which we expect not > to be too much. Hmm.. I don't think so. We can destroy memcg whenever we want, is it right? If it is right, there is many inflight objects when we destory memcg. If there is so many inflight objects, performance of these processes can be hurt too much. >> And, I found one case that destroying memcg's kmem_cache don't works properly. >> If we destroy memcg after all object is freed, current implementation >> doesn't destroy kmem_cache. >> kmem_cache_destroy_work_func() check "cachep->memcg_params.nr_pages == 0", >> but in this case, it return false, because kmem_cache may have >> cpu_slab, and cpu_partials_slabs. >> As we already free all objects, kmem_cache_verify_dead() is not invoked forever. >> I think that we need another kmem_cache_shrink() in >> kmem_cache_destroy_work_func(). > > I'll take a look here. What you describe makes sense, and can > potentially happen. I tried to handle this case with care in > destroy_all_caches, but I may have always made a mistake... > > Did you see this actively happening, or are you just assuming this can…
creator: JoonSoo Kim
date: 2012-09-21T09:28:02-00:00

Re: [PATCH v3 08/16] slab: allow enable_cpu_cache to use preset values for its tunables
type: item
description: On Tue, Sep 18, 2012 at 5:12 PM, Glauber Costa <glommer@parallels.com> wrote: > diff --git a/mm/slab.c b/mm/slab.c > index e2cf984..f2d760c 100644 > --- a/mm/slab.c > +++ b/mm/slab.c > @@ -4141,8 +4141,19 @@ static int do_tune_cpucache(struct kmem_cache *cachep, int limit, > static int enable_cpucache(struct kmem_cache *cachep, gfp_t gfp) > { > int err; > - int limit, shared; > - > + int limit = 0; > + int shared = 0; > + int batchcount = 0; > + > +#ifdef CONFIG_MEMCG_KMEM > + if (cachep->memcg_params.parent) { > + limit = cachep->memcg_params.parent->limit; > + shared = cachep->memcg_params.parent->shared; > + batchcount = cachep->memcg_params.parent->batchcount; Style nit: please introduce a variable for "cachep->memcg_params.parent" to make this human-readable.
creator: Pekka Enberg
date: 2012-09-21T09:29:20-00:00

Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
type: item
description: On 09/21/2012 01:33 PM, Pekka Enberg wrote: > On Wed, Sep 19, 2012 at 10:42 AM, Glauber Costa <glommer@parallels.com> wrote: >>>> index f2d760c..18de3f6 100644 >>>> --- a/mm/slab.c >>>> +++ b/mm/slab.c >>>> @@ -3938,9 +3938,12 @@ EXPORT_SYMBOL(__kmalloc); >>>> * Free an object which was previously allocated from this >>>> * cache. >>>> */ >>>> -void kmem_cache_free(struct kmem_cache *cachep, void *objp) >>>> +void kmem_cache_free(struct kmem_cache *s, void *objp) >>>> { >>>> unsigned long flags; >>>> + struct kmem_cache *cachep = virt_to_cache(objp); >>>> + >>>> + VM_BUG_ON(!slab_equal_or_parent(cachep, s)); >>> >>> This is an extremely hot path of the kernel and you are adding significant >>> processing. Check how the benchmarks are influenced by this change. >>> virt_to_cache can be a bit expensive. >> >> Would it be enough for you to have a separate code path for >> !CONFIG_MEMCG_KMEM? >> >> I don't really see another way to do it, aside from deriving the cache >> from the object in our case. I am open to suggestions if you do. > > We should assume that most distributions enable CONFIG_MEMCG_KMEM, > right? Therfore, any performance impact should be dependent on whether > or not kmem memcg is *enabled* at runtime or not. > > Can we use the "static key" thingy introduced by tracing folks for this? > Yes. I am already using static keys extensively in this patchset, and that is how I intend to handle this particular case.
creator: Glauber Costa
date: 2012-09-21T09:30:59-00:00

Re: [PATCH v3 15/16] memcg/sl[au]b: shrink dead caches
type: item
description: On 09/21/2012 01:28 PM, JoonSoo Kim wrote: > Hi, Glauber. > >>> 2012/9/18 Glauber Costa <glommer@parallels.com>: >>>> diff --git a/mm/slub.c b/mm/slub.c >>>> index 0b68d15..9d79216 100644 >>>> --- a/mm/slub.c >>>> +++ b/mm/slub.c >>>> @@ -2602,6 +2602,7 @@ redo: >>>> } else >>>> __slab_free(s, page, x, addr); >>>> >>>> + kmem_cache_verify_dead(s); >>>> } >>> >>> As far as u know, I am not a expert and don't know anything about memcg. >>> IMHO, this implementation may hurt system performance in some case. >>> >>> In case of memcg is destoried, remained kmem_cache is marked "dead". >>> After it is marked, >>> every free operation to this "dead" kmem_cache call >>> kmem_cache_verify_dead() and finally call kmem_cache_shrink(). >> >> As long as it is restricted to that cache, this is a non issue. >> dead caches are exactly what they name imply: dead. >> >> Means that we actively want them to go away, and just don't kill them >> right away because they have some inflight objects - which we expect not >> to be too much. > > Hmm.. I don't think so. > We can destroy memcg whenever we want, is it right? wrong. it is impossible to track objects to a task, so when tasks are moved, objects stay. Which means that some objects are still referenced by the cache. > If it is right, there is many inflight objects when we destory memcg. > If there is so many inflight objects, performance of these processes > can be hurt too much. > There are in-flight objects. It is not expected to have "many inflight objects", which is a different statement. We are assuming all the time that the workloads that goes into a cgroup are mostly nature. When it goes away, most of the objects that were referenced are expected to go away. Because we can't guarantee all of them will, the references stay. When we are able to call shrink_slab() directly on a memcg slab, which is WIP, we'll be able make those objects even rarer. >>> And, I found one case that destroying memcg's kmem_cache don't works properly. >…
creator: Glauber Costa
date: 2012-09-21T09:31:55-00:00

Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
type: item
description: On Wed, Sep 19, 2012 at 10:42 AM, Glauber Costa <glommer@parallels.com> wrote: >>> index f2d760c..18de3f6 100644 >>> --- a/mm/slab.c >>> +++ b/mm/slab.c >>> @@ -3938,9 +3938,12 @@ EXPORT_SYMBOL(__kmalloc); >>> * Free an object which was previously allocated from this >>> * cache. >>> */ >>> -void kmem_cache_free(struct kmem_cache *cachep, void *objp) >>> +void kmem_cache_free(struct kmem_cache *s, void *objp) >>> { >>> unsigned long flags; >>> + struct kmem_cache *cachep = virt_to_cache(objp); >>> + >>> + VM_BUG_ON(!slab_equal_or_parent(cachep, s)); >> >> This is an extremely hot path of the kernel and you are adding significant >> processing. Check how the benchmarks are influenced by this change. >> virt_to_cache can be a bit expensive. > > Would it be enough for you to have a separate code path for > !CONFIG_MEMCG_KMEM? > > I don't really see another way to do it, aside from deriving the cache > from the object in our case. I am open to suggestions if you do. We should assume that most distributions enable CONFIG_MEMCG_KMEM, right? Therfore, any performance impact should be dependent on whether or not kmem memcg is *enabled* at runtime or not. Can we use the "static key" thingy introduced by tracing folks for this?
creator: Pekka Enberg
date: 2012-09-21T09:33:23-00:00

Re: [PATCH v3 00/16] slab accounting for memcg
type: item
description: Hi Glauber, On Tue, Sep 18, 2012 at 5:11 PM, Glauber Costa <glommer@parallels.com> wrote: > This is a followup to the previous kmem series. I divided them logically > so it gets easier for reviewers. But I believe they are ready to be merged > together (although we can do a two-pass merge if people would prefer) > > Throwaway git tree found at: > > git://git.kernel.org/pub/scm/linux/kernel/git/glommer/memcg. git kmemcg-slab > > There are mostly bugfixes since last submission. Overall, I like this series a lot. However, I don't really see this as a v3.7 material because we already have largeish pending updates to the slab allocators. I also haven't seen any performance numbers for this which is a problem. So what I'd really like to see is this series being merged early in the v3.8 development cycle to maximize the number of people eyeballing the code and looking at performance impact. Does this sound reasonable to you Glauber? Pekka
creator: Pekka Enberg
date: 2012-09-21T09:40:15-00:00

Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
type: item
description: On Fri, 21 Sep 2012, Glauber Costa wrote: > > We should assume that most distributions enable CONFIG_MEMCG_KMEM, > > right? Therfore, any performance impact should be dependent on whether > > or not kmem memcg is *enabled* at runtime or not. > > > > Can we use the "static key" thingy introduced by tracing folks for this? > > Yes. > > I am already using static keys extensively in this patchset, and that is > how I intend to handle this particular case. Cool. The key point here is that !CONFIG_MEMCG_KMEM should have exactly *zero* performance impact and CONFIG_MEMCG_KMEM disabled at runtime should have absolute minimal impact. Pekka
creator: Pekka Enberg
date: 2012-09-21T09:41:52-00:00

Re: [PATCH v3 00/16] slab accounting for memcg
type: item
description: On 09/21/2012 01:40 PM, Pekka Enberg wrote: > Hi Glauber, > > On Tue, Sep 18, 2012 at 5:11 PM, Glauber Costa <glommer@parallels.com> wrote: >> This is a followup to the previous kmem series. I divided them logically >> so it gets easier for reviewers. But I believe they are ready to be merged >> together (although we can do a two-pass merge if people would prefer) >> >> Throwaway git tree found at: >> >> git://git.kernel.org/pub/scm/linux/kernel/git/glommer/memcg. git kmemcg-slab >> >> There are mostly bugfixes since last submission. > > Overall, I like this series a lot. However, I don't really see this as a > v3.7 material because we already have largeish pending updates to the > slab allocators. I also haven't seen any performance numbers for this > which is a problem. > > So what I'd really like to see is this series being merged early in the > v3.8 development cycle to maximize the number of people eyeballing the > code and looking at performance impact. > > Does this sound reasonable to you Glauber? Absolutely. As I've stated before, I actually believe the kmemcg-stack and kmemcg-slab (this one) portions should be merged separately. (So we can sort out issues more easily, and point to the right place) The first one is a lot more stable and got a lot more love. The goal of this one is to get it reviewed so we can merge as soon as we can - but not sooner. early v3.8 sounds perfect to me.
creator: Glauber Costa
date: 2012-09-21T09:43:48-00:00

Re: [PATCH v3 03/16] slab: Ignore the cflgs bit in cache creation
type: item
description: On Tue, Sep 18, 2012 at 06:11:57PM +0400, Glauber Costa wrote: > No cache should ever pass that as a creation flag, since this bit is > used to mark an internal decision of the slab about object placement. We > can just ignore this bit if it happens to be passed (such as when > duplicating a cache in the kmem memcg patches) > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: Christoph Lameter <cl@linux.com> > CC: Pekka Enberg <penberg@cs.helsinki.fi> > CC: David Rientjes <rientjes@google.com> > --- > mm/slab.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/slab.c b/mm/slab.c > index a7ed60f..ccf496c 100644 > --- a/mm/slab.c > +++ b/mm/slab.c > @@ -2373,6 +2373,7 @@ __kmem_cache_create (struct kmem_cache *cachep, unsigned long flags) > int err; > size_t size = cachep->size; > > + flags &= ~CFLGS_OFF_SLAB; A comment explaining why this is necessary wouldn't hurt. Thanks. -- tejun
creator: Tejun Heo
date: 2012-09-21T17:33:52-00:00

Re: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
type: item
description: Hello, Glauber. On Tue, Sep 18, 2012 at 06:11:59PM +0400, Glauber Costa wrote: > +void memcg_register_cache(struct mem_cgroup *memcg, struct kmem_cache *cachep) > +{ > + int id = -1; > + > + if (!memcg) > + id = ida_simple_get(&cache_types, 0, MAX_KMEM_CACHE_TYPES, > + GFP_KERNEL); > + cachep->memcg_params.id = id; > +} I'm a bit confused. Why is id allocated only when memcg is NULL? Also, how would the per-memcg slab/slubs appear in slabinfo? If they appear separately it might be better to give them readable cgroup names. Thanks. -- tejun
creator: Tejun Heo
date: 2012-09-21T18:14:58-00:00

Re: [PATCH v3 06/16] memcg: infrastructure to match an allocation to the right cache
type: item
description: On Tue, Sep 18, 2012 at 06:12:00PM +0400, Glauber Costa wrote: > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 04851bb..1cce5c3 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -339,6 +339,11 @@ struct mem_cgroup { > #ifdef CONFIG_INET > struct tcp_memcontrol tcp_mem; > #endif > + > +#ifdef CONFIG_MEMCG_KMEM > + /* Slab accounting */ > + struct kmem_cache *slabs[MAX_KMEM_CACHE_TYPES]; > +#endif Bah, 400 entry array in struct mem_cgroup. Can't we do something a bit more flexible? > +static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *cachep) > +{ > + char *name; > + struct dentry *dentry; > + > + rcu_read_lock(); > + dentry = rcu_dereference(memcg->css.cgroup->dentry); > + rcu_read_unlock(); > + > + BUG_ON(dentry == NULL); > + > + name = kasprintf(GFP_KERNEL, "%s(%d:%s)", > + cachep->name, css_id(&memcg->css), dentry->d_name.name); Maybe including full path is better, I don't know. > + return name; > +} ... > void __init memcg_init_kmem_cache(void) > @@ -665,6 +704,170 @@ static void disarm_kmem_keys(struct mem_cgroup *memcg) > */ > WARN_ON(res_counter_read_u64(&memcg->kmem, RES_USAGE) != 0); > } > + > +static DEFINE_MUTEX(memcg_cache_mutex); Blank line missing. Or if it's used inside memcg_create_kmem_cache() only move it inside the function? > +static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, > + struct kmem_cache *cachep) > +{ > + struct kmem_cache *new_cachep; > + int idx; > + > + BUG_ON(!memcg_can_account_kmem(memcg)); WARN_ON_ONCE() generally preferred. > + idx = cachep->memcg_params.id; Ah, okay so the id is assigned to the "base" cache. Maybe explain it somewhere? > + mutex_lock(&memcg_cache_mutex); > + new_cachep = memcg->slabs[idx]; > + if (new_cachep) > + goto out; > + > + new_cachep = kmem_cache_dup(memcg, cachep); > + > + if (new_cachep == NULL) { > + new_cachep = cachep; > + goto out; > + } > + > + mem_cgroup_get(memcg); > + memcg->slabs[idx] = new_cachep; > + new_cachep->memcg_params…
creator: Tejun Heo
date: 2012-09-21T18:32:17-00:00

Re: [PATCH v3 07/16] memcg: skip memcg kmem allocations in specified code regions
type: item
description: Hello, On Tue, Sep 18, 2012 at 06:12:01PM +0400, Glauber Costa wrote: > +static void memcg_stop_kmem_account(void) > +{ > + if (!current->mm) > + return; > + > + current->memcg_kmem_skip_account++; > +} > + > +static void memcg_resume_kmem_account(void) > +{ > + if (!current->mm) > + return; > + > + current->memcg_kmem_skip_account--; > +} I can't say I'm a big fan of this approach. If there are enough users, maybe but can't we just annotate the affected allocations explicitly? Is this gonna have many more users? Also, in general, can we please add some comments? I know memcg code is dearth of comments but let's please not keep it that way. Thanks. -- tejun
creator: Tejun Heo
date: 2012-09-21T19:59:29-00:00

Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
type: item
description: Hello, On Fri, Sep 21, 2012 at 12:41:52PM +0300, Pekka Enberg wrote: > > I am already using static keys extensively in this patchset, and that is > > how I intend to handle this particular case. > > Cool. > > The key point here is that !CONFIG_MEMCG_KMEM should have exactly *zero* > performance impact and CONFIG_MEMCG_KMEM disabled at runtime should have > absolute minimal impact. Not necessarily disagreeing, but I don't think it's helpful to set the bar impossibly high. Even static_key doesn't have "exactly *zero*" impact. Let's stick to as minimal as possible when not in use and reasonable in use. And, yeah, this one can be easily solved by using static_key. Thanks. -- tejun
creator: Tejun Heo
date: 2012-09-21T20:07:33-00:00

Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
type: item
description: On Fri, Sep 21, 2012 at 11:07 PM, Tejun Heo <tj@kernel.org> wrote: > Not necessarily disagreeing, but I don't think it's helpful to set the > bar impossibly high. Even static_key doesn't have "exactly *zero*" > impact. Let's stick to as minimal as possible when not in use and > reasonable in use. For !CONFIG_MEMCG_KMEM, it should be exactly zero. No need to play games with static_key.
creator: Pekka Enberg
date: 2012-09-21T20:14:45-00:00

Re: [PATCH v3 09/16] sl[au]b: always get the cache from its page in kfree
type: item
description: On Fri, Sep 21, 2012 at 11:14:45PM +0300, Pekka Enberg wrote: > On Fri, Sep 21, 2012 at 11:07 PM, Tejun Heo <tj@kernel.org> wrote: > > Not necessarily disagreeing, but I don't think it's helpful to set the > > bar impossibly high. Even static_key doesn't have "exactly *zero*" > > impact. Let's stick to as minimal as possible when not in use and > > reasonable in use. > > For !CONFIG_MEMCG_KMEM, it should be exactly zero. No need to play > games with static_key. Ah, misread, though you were talking about MEMCG_KMEM && not in use. Yeap, if turned off in Kconfig, it shouldn't have any performance overhead. Thanks. -- tejun
creator: Tejun Heo
date: 2012-09-21T20:16:02-00:00

Re: [PATCH v3 11/16] memcg: destroy memcg caches
type: item
description: Hello, Glauber. On Tue, Sep 18, 2012 at 06:12:05PM +0400, Glauber Costa wrote: > +static LIST_HEAD(destroyed_caches); > + > +static void kmem_cache_destroy_work_func(struct work_struct *w) > +{ > + struct kmem_cache *cachep; > + struct mem_cgroup_cache_params *p, *tmp; > + unsigned long flags; > + LIST_HEAD(del_unlocked); > + > + spin_lock_irqsave(&cache_queue_lock, flags); > + list_for_each_entry_safe(p, tmp, &destroyed_caches, destroyed_list) { > + cachep = container_of(p, struct kmem_cache, memcg_params); > + list_move(&cachep->memcg_params.destroyed_list, &del_unlocked); > + } > + spin_unlock_irqrestore(&cache_queue_lock, flags); > + > + list_for_each_entry_safe(p, tmp, &del_unlocked, destroyed_list) { > + cachep = container_of(p, struct kmem_cache, memcg_params); > + list_del(&cachep->memcg_params.destroyed_list); > + if (!atomic_read(&cachep->memcg_params.nr_pages)) { > + mem_cgroup_put(cachep->memcg_params.memcg); > + kmem_cache_destroy(cachep); > + } > + } > +} > +static DECLARE_WORK(kmem_cache_destroy_work, kmem_cache_destroy_work_func); Again, please don't build your own worklist. Just embed a work item into mem_cgroup_cache_params and manipulate them. No need to duplicate what workqueue already implements. Thanks. -- tejun
creator: Tejun Heo
date: 2012-09-21T20:22:20-00:00

Re: [PATCH v3 12/16] memcg/sl[au]b Track all the memcg children of a kmem_cache.
type: item
description: Hello, Glauber. On Tue, Sep 18, 2012 at 06:12:06PM +0400, Glauber Costa wrote: > This enables us to remove all the children of a kmem_cache being > destroyed, if for example the kernel module it's being used in > gets unloaded. Otherwise, the children will still point to the > destroyed parent. I find the terms parent / child / sibling a bit confusing. It usually implies proper tree structure. Maybe we can use better terms which reflect the single layer structure better? And, again, in general, please add some comments. If someone tries to understand this for the first time and takes a look at mem_cgroup_cache_params, there's almost nothing to guide that person. What's the struct for? What does each field do? What are the synchronization rules? > @@ -626,6 +630,9 @@ void memcg_release_cache(struct kmem_cache *cachep) > { > if (cachep->memcg_params.id != -1) > ida_simple_remove(&cache_types, cachep->memcg_params.id); > + else > + list_del(&cachep->memcg_params.sibling_list); > + list_del_init() please. Thanks. -- tejun
creator: Tejun Heo
date: 2012-09-21T20:31:01-00:00

Re: [PATCH v3 15/16] memcg/sl[au]b: shrink dead caches
type: item
description: Hello, Glauber. On Tue, Sep 18, 2012 at 06:12:09PM +0400, Glauber Costa wrote: > @@ -764,10 +777,21 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, > goto out; > } > > + /* > + * Because the cache is expected to duplicate the string, > + * we must make sure it has opportunity to copy its full > + * name. Only now we can remove the dead part from it > + */ > + name = (char *)new_cachep->name; > + if (name) > + name[strlen(name) - 4] = '\0'; This is kinda nasty. Do we really need to do this? How long would a dead cache stick around? > diff --git a/mm/slab.c b/mm/slab.c > index bd9928f..6cb4abf 100644 > --- a/mm/slab.c > +++ b/mm/slab.c > @@ -3785,6 +3785,8 @@ static inline void __cache_free(struct kmem_cache *cachep, void *objp, > } > > ac_put_obj(cachep, ac, objp); > + > + kmem_cache_verify_dead(cachep); Reaping dead caches doesn't exactly sound like a high priority thing and adding a branch to hot path for that might not be the best way to do it. Why not schedule an extremely lazy deferrable delayed_work which polls for emptiness, say, every miniute or whatever? Thanks. -- tejun
creator: Tejun Heo
date: 2012-09-21T20:40:35-00:00

Re: [PATCH v3 00/16] slab accounting for memcg
type: item
description: Hello, On Tue, Sep 18, 2012 at 06:11:54PM +0400, Glauber Costa wrote: > This is a followup to the previous kmem series. I divided them logically > so it gets easier for reviewers. But I believe they are ready to be merged > together (although we can do a two-pass merge if people would prefer) > > Throwaway git tree found at: > > git://git.kernel.org/pub/scm/linux/kernel/git/glommer/memcg. git kmemcg-slab > > There are mostly bugfixes since last submission. > > For a detailed explanation about this series, please refer to my previous post > (Subj: [PATCH v3 00/13] kmem controller for memcg.) In general, things look good to me. I think the basic approach is manageable and does a decent job of avoiding introducing complications on the usual code paths. Pekka seems generally happy with the approach too. Christoph, what do you think? Thanks. -- tejun
creator: Tejun Heo
date: 2012-09-21T20:46:19-00:00

Re: [PATCH v3 00/16] slab accounting for memcg
type: item
description: On Fri, Sep 21, 2012 at 01:46:19PM -0700, Tejun Heo wrote: > In general, things look good to me. I think the basic approach is > manageable and does a decent job of avoiding introducing complications > on the usual code paths. > > Pekka seems generally happy with the approach too. Christoph, what do > you think? Ooh, also, why aren't Andrew and Michal not cc'd? -- tejun
creator: Tejun Heo
date: 2012-09-21T20:47:27-00:00

Re: [PATCH v3 06/16] memcg: infrastructure to match an allocation to the right cache
type: item
description: Missed some stuff. On Tue, Sep 18, 2012 at 06:12:00PM +0400, Glauber Costa wrote: > +static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, > + struct kmem_cache *cachep) > +{ ... > + memcg->slabs[idx] = new_cachep; ... > +struct kmem_cache *__memcg_kmem_get_cache(struct kmem_cache *cachep, > + gfp_t gfp) > +{ ... > + return memcg->slabs[idx]; I think you need memory barriers for the above pair. Thanks. -- tejun
creator: Tejun Heo
date: 2012-09-21T20:52:36-00:00

Re: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
type: item
description: On 09/21/2012 10:14 PM, Tejun Heo wrote: > Hello, Glauber. > > On Tue, Sep 18, 2012 at 06:11:59PM +0400, Glauber Costa wrote: >> +void memcg_register_cache(struct mem_cgroup *memcg, struct kmem_cache *cachep) >> +{ >> + int id = -1; >> + >> + if (!memcg) >> + id = ida_simple_get(&cache_types, 0, MAX_KMEM_CACHE_TYPES, >> + GFP_KERNEL); >> + cachep->memcg_params.id = id; >> +} > > I'm a bit confused. Why is id allocated only when memcg is NULL? > I think you figured that out already from your answer in another patch, right? But I'll add a comment here since it seems to be a a natural search point for people, explaining the mechanism. > Also, how would the per-memcg slab/slubs appear in slabinfo? If they > appear separately it might be better to give them readable cgroup > names. > The new caches will appear under /proc/slabinfo with the rest, with a string appended that identifies the group. > Thanks. >
creator: Glauber Costa
date: 2012-09-24T08:12:23-00:00

Re: [PATCH v3 00/16] slab accounting for memcg
type: item
description: On 09/22/2012 12:46 AM, Tejun Heo wrote: > Hello, > > On Tue, Sep 18, 2012 at 06:11:54PM +0400, Glauber Costa wrote: >> This is a followup to the previous kmem series. I divided them logically >> so it gets easier for reviewers. But I believe they are ready to be merged >> together (although we can do a two-pass merge if people would prefer) >> >> Throwaway git tree found at: >> >> git://git.kernel.org/pub/scm/linux/kernel/git/glommer/memcg. git kmemcg-slab >> >> There are mostly bugfixes since last submission. >> >> For a detailed explanation about this series, please refer to my previous post >> (Subj: [PATCH v3 00/13] kmem controller for memcg.) > > In general, things look good to me. I think the basic approach is > manageable and does a decent job of avoiding introducing complications > on the usual code paths. > > Pekka seems generally happy with the approach too. Christoph, what do > you think? > > Thanks. > I myself think that while the approach is okay, this would need one or two more versions for us to sort out some issues that are still remaining. I'd very much like to have the kmemcg-stack series seeing its way forward first. Mel Gorman said he would try his best to review it his week, and I don't plan to resubmit anything before that. After that, I plan to rebase at least this one again.
creator: Glauber Costa
date: 2012-09-24T08:15:51-00:00

Re: [PATCH v3 06/16] memcg: infrastructure to match an allocation to the right cache
type: item
description: On 09/22/2012 12:52 AM, Tejun Heo wrote: > Missed some stuff. > > On Tue, Sep 18, 2012 at 06:12:00PM +0400, Glauber Costa wrote: >> +static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, >> + struct kmem_cache *cachep) >> +{ > ... >> + memcg->slabs[idx] = new_cachep; > ... >> +struct kmem_cache *__memcg_kmem_get_cache(struct kmem_cache *cachep, >> + gfp_t gfp) >> +{ > ... >> + return memcg->slabs[idx]; > > I think you need memory barriers for the above pair. > > Thanks. > Why is that? We'll either see a value, or NULL. If we see NULL, we assume the cache is not yet created. Not a big deal.
creator: Glauber Costa
date: 2012-09-24T08:17:37-00:00

Re: [PATCH v3 15/16] memcg/sl[au]b: shrink dead caches
type: item
description: On 09/22/2012 12:40 AM, Tejun Heo wrote: > Hello, Glauber. > > On Tue, Sep 18, 2012 at 06:12:09PM +0400, Glauber Costa wrote: >> @@ -764,10 +777,21 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, >> goto out; >> } >> >> + /* >> + * Because the cache is expected to duplicate the string, >> + * we must make sure it has opportunity to copy its full >> + * name. Only now we can remove the dead part from it >> + */ >> + name = (char *)new_cachep->name; >> + if (name) >> + name[strlen(name) - 4] = '\0'; > > This is kinda nasty. Do we really need to do this? How long would a > dead cache stick around? Without targeted shrinking, until all objects are manually freed, which may need to wait global reclaim to kick in. In general, if we agree with duplicating the caches, the problem that they may stick around for some time will not be avoidable. If you have any suggestions about alternative ways for it, I'm all ears. > >> diff --git a/mm/slab.c b/mm/slab.c >> index bd9928f..6cb4abf 100644 >> --- a/mm/slab.c >> +++ b/mm/slab.c >> @@ -3785,6 +3785,8 @@ static inline void __cache_free(struct kmem_cache *cachep, void *objp, >> } >> >> ac_put_obj(cachep, ac, objp); >> + >> + kmem_cache_verify_dead(cachep); > > Reaping dead caches doesn't exactly sound like a high priority thing > and adding a branch to hot path for that might not be the best way to > do it. Why not schedule an extremely lazy deferrable delayed_work > which polls for emptiness, say, every miniute or whatever? > Because this branch is marked as unlikely, I would expect it not to be a big problem. It will be not taken most of the time, and becomes a very cheap branch. I considered this to be simpler than a deferred work mechanism. If even then, you guys believe this is still too high, I can resort to that. > Thanks. >
creator: Glauber Costa
date: 2012-09-24T08:25:00-00:00

Re: [PATCH v3 06/16] memcg: infrastructure to match an allocation to the right cache
type: item
description: On 09/21/2012 10:32 PM, Tejun Heo wrote: > On Tue, Sep 18, 2012 at 06:12:00PM +0400, Glauber Costa wrote: >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 04851bb..1cce5c3 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -339,6 +339,11 @@ struct mem_cgroup { >> #ifdef CONFIG_INET >> struct tcp_memcontrol tcp_mem; >> #endif >> + >> +#ifdef CONFIG_MEMCG_KMEM >> + /* Slab accounting */ >> + struct kmem_cache *slabs[MAX_KMEM_CACHE_TYPES]; >> +#endif > > Bah, 400 entry array in struct mem_cgroup. Can't we do something a > bit more flexible? > I guess. I still would like it to be an array, so we can easily access its fields. There are two ways around this: 1) Do like the events mechanism and allocate this in a separate structure. Add a pointer chase in the access, and I don't think it helps much because it gets allocated anyway. But we could at least defer it to the time when we limit the cache. 2) The indexes are only assigned after the slab is FULL. At that time, a lot of the caches are already initialized. We can, for instance, allow for "twice the number we have in the system", which already provides room for a couple of more appearing. Combining with the 1st approach, we can defer it to limit-time, and then allow for, say, 50 % more caches than we already have. The pointer chasing may very well be worth it... >> +static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *cachep) >> +{ >> + char *name; >> + struct dentry *dentry; >> + >> + rcu_read_lock(); >> + dentry = rcu_dereference(memcg->css.cgroup->dentry); >> + rcu_read_unlock(); >> + >> + BUG_ON(dentry == NULL); >> + >> + name = kasprintf(GFP_KERNEL, "%s(%d:%s)", >> + cachep->name, css_id(&memcg->css), dentry->d_name.name); > > Maybe including full path is better, I don't know. It can get way too big. >> +static void memcg_create_cache_enqueue(struct mem_cgroup *memcg, >> + struct kmem_cache *cachep) >> +{ >> + struct create_work *cw; >> + unsigned long flags; >> + >> + …
creator: Glauber Costa
date: 2012-09-24T08:46:35-00:00

Re: [PATCH v3 07/16] memcg: skip memcg kmem allocations in specified code regions
type: item
description: On 09/21/2012 11:59 PM, Tejun Heo wrote: > Hello, > > On Tue, Sep 18, 2012 at 06:12:01PM +0400, Glauber Costa wrote: >> +static void memcg_stop_kmem_account(void) >> +{ >> + if (!current->mm) >> + return; >> + >> + current->memcg_kmem_skip_account++; >> +} >> + >> +static void memcg_resume_kmem_account(void) >> +{ >> + if (!current->mm) >> + return; >> + >> + current->memcg_kmem_skip_account--; >> +} > > I can't say I'm a big fan of this approach. If there are enough > users, maybe but can't we just annotate the affected allocations > explicitly? Is this gonna have many more users? > What exactly do you mean by annotating the affected allocations? There are currently two users of this. In both places, we are interested in disallowing recursion, because cache creation will trigger new cache allocations that will bring us back here. We can't rely on unsetting the GFP flag we're using for this, because that affects only the page allocation, not the metadata allocation for the cache. > Also, in general, can we please add some comments? I know memcg code > is dearth of comments but let's please not keep it that way. > All right here.
creator: Glauber Costa
date: 2012-09-24T09:09:23-00:00

Re: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
type: item
description: On Sep 24, 2012, at 3:12, Glauber Costa <glommer@parallels.com> wrote: > On 09/21/2012 10:14 PM, Tejun Heo wrote: > > The new caches will appear under /proc/slabinfo with the rest, with a > string appended that identifies the group. There are f.e. meminfo files in the per node directories in sysfs. It would make sense to have a slabinfo file there (if you want to keep that around). Then the format would be the same.
creator: christoph
date: 2012-09-24T12:41:26-00:00

Re: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
type: item
description: On 09/24/2012 04:41 PM, Christoph wrote: > > On Sep 24, 2012, at 3:12, Glauber Costa <glommer@parallels.com> wrote: > >> On 09/21/2012 10:14 PM, Tejun Heo wrote: >> >> The new caches will appear under /proc/slabinfo with the rest, with a >> string appended that identifies the group. > > There are f.e. meminfo files in the per node directories in sysfs. It would make sense to have a slabinfo file there (if you want to keep that around). Then the format would be the same. > But that is orthogonal, isn't it? People will still expect to see it in the old slabinfo file.
creator: Glauber Costa
date: 2012-09-24T12:41:36-00:00

Re: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
type: item
description: On Mon, 24 Sep 2012, Glauber Costa wrote: > But that is orthogonal, isn't it? People will still expect to see it in > the old slabinfo file. The current scheme for memory statistics is /proc/meminfo contains global counters /sys/devices/system/node/nodeX/meminfo contains node specific counters. The cgroups directory already contains various files. Adding a slabinfo file would make sense and it could be found easily. The sysfs hierachy /sys/kernel/slab could also show up there as a "slab" directory under which all the details of the various caches would be available and tunable. Very much in sync with the way the cgroups directories operate.
creator: Christoph Lameter
date: 2012-09-24T13:42:05-00:00

Re: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
type: item
description: On 09/24/2012 05:42 PM, Christoph Lameter wrote: > On Mon, 24 Sep 2012, Glauber Costa wrote: > >> But that is orthogonal, isn't it? People will still expect to see it in >> the old slabinfo file. > > The current scheme for memory statistics is > > /proc/meminfo contains global counters > > /sys/devices/system/node/nodeX/meminfo > > contains node specific counters. > > The cgroups directory already contains various files. Adding a slabinfo > file would make sense and it could be found easily. > > The sysfs hierachy /sys/kernel/slab could also show up there as a "slab" > directory under which all the details of the various caches would be > available and tunable. Very much in sync with the way the cgroups > directories operate. > The reason I say it is orthogonal, is that people will still want to see their caches in /proc/slabinfo, regardless of wherever else they'll be. It was a requirement from Pekka in one of the first times I posted this, IIRC.
creator: Glauber Costa
date: 2012-09-24T13:44:24-00:00

Re: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
type: item
description: On Mon, 24 Sep 2012, Glauber Costa wrote: > The reason I say it is orthogonal, is that people will still want to see > their caches in /proc/slabinfo, regardless of wherever else they'll be. > It was a requirement from Pekka in one of the first times I posted this, > IIRC. They want to see total counts there true. But as I said we already have a duplication of the statistics otherwise. We have never done the scheme that you propose. That is unexpected. I would not expect the numbers to be there.
creator: Christoph Lameter
date: 2012-09-24T13:56:32-00:00

Re: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
type: item
description: On 09/24/2012 05:56 PM, Christoph Lameter wrote: > On Mon, 24 Sep 2012, Glauber Costa wrote: > >> The reason I say it is orthogonal, is that people will still want to see >> their caches in /proc/slabinfo, regardless of wherever else they'll be. >> It was a requirement from Pekka in one of the first times I posted this, >> IIRC. > > They want to see total counts there true. But as I said we already have a > duplication of the statistics otherwise. We have never done the scheme > that you propose. That is unexpected. I would not expect the numbers to be > there. > I myself personally believe it can potentially clutter slabinfo, and won't put my energy in defending the current implementation. What I don't want is to keep switching between implementations. Pekka, Tejun, what do you guys say here?
creator: Glauber Costa
date: 2012-09-24T13:57:55-00:00

Re: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
type: item
description: On 09/24/2012 07:38 PM, Pekka Enberg wrote: > On 09/24/2012 05:56 PM, Christoph Lameter wrote: >>> On Mon, 24 Sep 2012, Glauber Costa wrote: >>> >>>> The reason I say it is orthogonal, is that people will still want to see >>>> their caches in /proc/slabinfo, regardless of wherever else they'll be. >>>> It was a requirement from Pekka in one of the first times I posted this, >>>> IIRC. >>> >>> They want to see total counts there true. But as I said we already have a >>> duplication of the statistics otherwise. We have never done the scheme >>> that you propose. That is unexpected. I would not expect the numbers to be >>> there. > > On Mon, Sep 24, 2012 at 4:57 PM, Glauber Costa <glommer@parallels.com> wrote: >> I myself personally believe it can potentially clutter slabinfo, and >> won't put my energy in defending the current implementation. What I >> don't want is to keep switching between implementations. >> >> Pekka, Tejun, what do you guys say here? > > So Christoph is proposing that the new caches appear somewhere under > the cgroups directory and /proc/slabinfo includes aggregated counts, > right? I'm certainly OK with that. > Just for clarification, I am not sure about the aggregate counts - although it surely makes sense. Christoph, is that what you're proposing ?
creator: Glauber Costa
date: 2012-09-24T15:36:44-00:00

Re: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
type: item
description: On 09/24/2012 05:56 PM, Christoph Lameter wrote: >> On Mon, 24 Sep 2012, Glauber Costa wrote: >> >>> The reason I say it is orthogonal, is that people will still want to see >>> their caches in /proc/slabinfo, regardless of wherever else they'll be. >>> It was a requirement from Pekka in one of the first times I posted this, >>> IIRC. >> >> They want to see total counts there true. But as I said we already have a >> duplication of the statistics otherwise. We have never done the scheme >> that you propose. That is unexpected. I would not expect the numbers to be >> there. On Mon, Sep 24, 2012 at 4:57 PM, Glauber Costa <glommer@parallels.com> wrote: > I myself personally believe it can potentially clutter slabinfo, and > won't put my energy in defending the current implementation. What I > don't want is to keep switching between implementations. > > Pekka, Tejun, what do you guys say here? So Christoph is proposing that the new caches appear somewhere under the cgroups directory and /proc/slabinfo includes aggregated counts, right? I'm certainly OK with that.
creator: Pekka Enberg
date: 2012-09-24T15:38:48-00:00

Re: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
type: item
description: On Mon, 24 Sep 2012, Glauber Costa wrote: > > So Christoph is proposing that the new caches appear somewhere under > > the cgroups directory and /proc/slabinfo includes aggregated counts, > > right? I'm certainly OK with that. > > > Just for clarification, I am not sure about the aggregate counts - > although it surely makes sense. > > Christoph, is that what you're proposing ? Yes. Make it similar to the way /proc/meminfo is handled.
creator: Christoph Lameter
date: 2012-09-24T17:38:17-00:00

Re: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
type: item
description: On Mon, 24 Sep 2012, Pekka Enberg wrote: > So Christoph is proposing that the new caches appear somewhere under > the cgroups directory and /proc/slabinfo includes aggregated counts, > right? I'm certainly OK with that. Caches would appear either in cgroup/slabinfo (which would have the same format as /proc/slabinfo) or in cgroup/slab/<slabname>/<fieldname> (similar to /sys/kernel/slab/...)
creator: Christoph Lameter
date: 2012-09-24T17:39:49-00:00

Re: [PATCH v3 15/16] memcg/sl[au]b: shrink dead caches
type: item
description: Hello, On Mon, Sep 24, 2012 at 12:25:00PM +0400, Glauber Costa wrote: > > This is kinda nasty. Do we really need to do this? How long would a > > dead cache stick around? > > Without targeted shrinking, until all objects are manually freed, which > may need to wait global reclaim to kick in. > > In general, if we agree with duplicating the caches, the problem that > they may stick around for some time will not be avoidable. If you have > any suggestions about alternative ways for it, I'm all ears. I don't have much problem with caches sticking around waiting to be reaped. I'm just wondering whether renaming trick is really necessary. > > Reaping dead caches doesn't exactly sound like a high priority thing > > and adding a branch to hot path for that might not be the best way to > > do it. Why not schedule an extremely lazy deferrable delayed_work > > which polls for emptiness, say, every miniute or whatever? > > > > Because this branch is marked as unlikely, I would expect it not to be a > big problem. It will be not taken most of the time, and becomes a very > cheap branch. I considered this to be simpler than a deferred work > mechanism. > > If even then, you guys believe this is still too high, I can resort to that. It's still an otherwise unnecessary branch on a very hot path. If you can remove it, there's no reason not to. Thanks. -- tejun
creator: Tejun Heo
date: 2012-09-24T17:43:46-00:00

Re: [PATCH v3 07/16] memcg: skip memcg kmem allocations in specified code regions
type: item
description: Hello, On Mon, Sep 24, 2012 at 01:09:23PM +0400, Glauber Costa wrote: > > I can't say I'm a big fan of this approach. If there are enough > > users, maybe but can't we just annotate the affected allocations > > explicitly? Is this gonna have many more users? > > What exactly do you mean by annotating the affected allocations? IOW, can't you just pass down an extra argument / flag / whatever instead? Thanks. -- tejun
creator: Tejun Heo
date: 2012-09-24T17:47:42-00:00

Re: [PATCH v3 06/16] memcg: infrastructure to match an allocation to the right cache
type: item
description: Hello, Glauber. On Mon, Sep 24, 2012 at 12:46:35PM +0400, Glauber Costa wrote: > >> +#ifdef CONFIG_MEMCG_KMEM > >> + /* Slab accounting */ > >> + struct kmem_cache *slabs[MAX_KMEM_CACHE_TYPES]; > >> +#endif > > > > Bah, 400 entry array in struct mem_cgroup. Can't we do something a > > bit more flexible? > > > > I guess. I still would like it to be an array, so we can easily access > its fields. There are two ways around this: > > 1) Do like the events mechanism and allocate this in a separate > structure. Add a pointer chase in the access, and I don't think it helps > much because it gets allocated anyway. But we could at least > defer it to the time when we limit the cache. Start at some reasonable size and then double it as usage grows? How many kmem_caches do we typically end up using? > >> + if (memcg->slabs[idx] == NULL) { > >> + memcg_create_cache_enqueue(memcg, cachep); > > > > Do we want to wait for the work item if @gfp allows? > > > > I tried this once, and it got complicated enough that I deemed as "not > worth it". I honestly don't remember much of the details now, it was one > of the first things I tried, and a bunch of time has passed. If you > think it is absolutely worth it, I can try it again. But at the very > best, I view this as an optimization. I don't know. It seems like a logical thing to try and depends on how complex it gets. I don't think it's a must. The whole thing is somewhat opportunistic after all. Thanks. -- tejun
creator: Tejun Heo
date: 2012-09-24T17:56:19-00:00

Re: [PATCH v3 06/16] memcg: infrastructure to match an allocation to the right cache
type: item
description: On Mon, Sep 24, 2012 at 12:17:37PM +0400, Glauber Costa wrote: > On 09/22/2012 12:52 AM, Tejun Heo wrote: > > Missed some stuff. > > > > On Tue, Sep 18, 2012 at 06:12:00PM +0400, Glauber Costa wrote: > >> +static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, > >> + struct kmem_cache *cachep) > >> +{ > > ... > >> + memcg->slabs[idx] = new_cachep; > > ... > >> +struct kmem_cache *__memcg_kmem_get_cache(struct kmem_cache *cachep, > >> + gfp_t gfp) > >> +{ > > ... > >> + return memcg->slabs[idx]; > > > > I think you need memory barriers for the above pair. > > > > Thanks. > > > > Why is that? > > We'll either see a value, or NULL. If we see NULL, we assume the cache > is not yet created. Not a big deal. Because when you see !NULL cache pointer you want to be able to see the cache fully initialized. You need wmb between cache creation and pointer assignment and at least read_barrier_depends() between fetching the cache pointer and dereferencing it. Thanks. -- tejun
creator: Tejun Heo
date: 2012-09-24T17:58:01-00:00

Re: [PATCH v3 06/16] memcg: infrastructure to match an allocation to the right cache
type: item
description: On 09/24/2012 09:56 PM, Tejun Heo wrote: > Hello, Glauber. > > On Mon, Sep 24, 2012 at 12:46:35PM +0400, Glauber Costa wrote: >>>> +#ifdef CONFIG_MEMCG_KMEM >>>> + /* Slab accounting */ >>>> + struct kmem_cache *slabs[MAX_KMEM_CACHE_TYPES]; >>>> +#endif >>> >>> Bah, 400 entry array in struct mem_cgroup. Can't we do something a >>> bit more flexible? >>> >> >> I guess. I still would like it to be an array, so we can easily access >> its fields. There are two ways around this: >> >> 1) Do like the events mechanism and allocate this in a separate >> structure. Add a pointer chase in the access, and I don't think it helps >> much because it gets allocated anyway. But we could at least >> defer it to the time when we limit the cache. > > Start at some reasonable size and then double it as usage grows? How > many kmem_caches do we typically end up using? > So my Fedora box here, recently booted on a Fedora kernel, will have 111 caches. How would 150 sound to you?
creator: Glauber Costa
date: 2012-09-25T13:57:38-00:00

Re: [PATCH v3 06/16] memcg: infrastructure to match an allocation to the right cache
type: item
description: On Tue, 25 Sep 2012, Glauber Costa wrote: > >> 1) Do like the events mechanism and allocate this in a separate > >> structure. Add a pointer chase in the access, and I don't think it helps > >> much because it gets allocated anyway. But we could at least > >> defer it to the time when we limit the cache. > > > > Start at some reasonable size and then double it as usage grows? How > > many kmem_caches do we typically end up using? > > > > So my Fedora box here, recently booted on a Fedora kernel, will have 111 > caches. How would 150 sound to you? Some drivers/subsystems can dynamically create slabs as needed for new devices or instances of metadata. You cannot use a fixed size array and cannot establish an upper boundary for the number of slabs on the system.
creator: Christoph Lameter
date: 2012-09-25T16:28:11-00:00

Re: [PATCH v3 05/16] consider a memcg parameter in kmem_create_cache
type: item
description: On Tue 18-09-12 18:11:59, Glauber Costa wrote: > Allow a memcg parameter to be passed during cache creation. > When the slub allocator is being used, it will only merge > caches that belong to the same memcg. > > Default function is created as a wrapper, passing NULL > to the memcg version. We only merge caches that belong > to the same memcg. > > From the memcontrol.c side, 3 helper functions are created: > > 1) memcg_css_id: because slub needs a unique cache name > for sysfs. Since this is visible, but not the canonical > location for slab data, the cache name is not used, the > css_id should suffice. > > 2) mem_cgroup_register_cache: is responsible for assigning > a unique index to each cache, and other general purpose > setup. The index is only assigned for the root caches. All > others are assigned index == -1. It would be nice to describe what is memcg_params.id intended for. There is no usage in this patch (except for create_unique_id in slub). I guess that by root caches you mean all default caches with memcg==NULL, right? [...] -- Michal Hocko SUSE Labs
creator: Michal Hocko
date: 2012-10-02T14:46:10-00:00