OpenVZ Forum - RDF feed
type: channel
items: [PATCH v5 00/18] slab accounting for memcg
items: [PATCH v5 01/18] move slabinfo processing to slab_common.c
items: [PATCH v5 02/18] move print_slabinfo_header to slab_common.c
items: [PATCH v5 03/18] sl[au]b: process slabinfo_show in common code
items: [PATCH v5 04/18] slab: don't preemptively remove element from list in cache destroy
items: [PATCH v5 05/18] slab/slub: struct memcg_params
items: [PATCH v5 06/18] consider a memcg parameter in kmem_create_cache
items: [PATCH v5 07/18] Allocate memory for memcg caches whenever a new memcg appears
items: [PATCH v5 08/18] memcg: infrastructure to match an allocation to the right cache
items: [PATCH v5 09/18] memcg: skip memcg kmem allocations in specified code regions
items: [PATCH v5 10/18] sl[au]b: always get the cache from its page in kfree
items: [PATCH v5 11/18] sl[au]b: Allocate objects from memcg cache
items: [PATCH v5 12/18] memcg: destroy memcg caches
items: [PATCH v5 13/18] memcg/sl[au]b Track all the memcg children of a kmem_cache.
items: [PATCH v5 14/18] memcg/sl[au]b: shrink dead caches
items: [PATCH v5 15/18] Aggregate memcg cache values in slabinfo
items: [PATCH v5 16/18] slab: propagate tunables values
items: [PATCH v5 17/18] slub: slub-specific propagation changes.
items: [PATCH v5 18/18] Add slab-specific documentation about the kmem controller
items: Re: [PATCH v5 04/18] slab: don't preemptively remove element from list in cache destroy
items: Re: [PATCH v5 10/18] sl[au]b: always get the cache from its page in kfree
items: Re: [PATCH v5 11/18] sl[au]b: Allocate objects from memcg cache
items: Re: [PATCH v5 14/18] memcg/sl[au]b: shrink dead caches
items: Re: [PATCH v5 15/18] Aggregate memcg cache values in slabinfo
items: Re: [PATCH v5 16/18] slab: propagate tunables values
items: Re: [PATCH v5 05/18] slab/slub: struct memcg_params
items: Re: [PATCH v5 06/18] consider a memcg parameter in kmem_create_cache
items: Re: [PATCH v5 01/18] move slabinfo processing to slab_common.c
items: Re: [PATCH v5 08/18] memcg: infrastructure to match an allocation to the right cache
items: Re: [PATCH v5 13/18] memcg/sl[au]b Track all the memcg children of a kmem_cache.

[PATCH v5 00/18] slab accounting for memcg
type: item
description: Note: This is basically the same as v4. During this week, I made some changes to this series in advance based on the feedback I had in the kmemcg-stack last submission. Although the last series was not yet extensively reviewed, I opted for sending this out so you guys have the most up2date code to review. So please review this one instead. 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 v5: * code reorganization, name changes, etc. v4: * no more messing with the cache name after destruction: aggregated figures are shown in /proc/slabinfo. * memory.kmem.slabinfo file with memcg-specific cache information during its lifespan. * full slub attribute propagation. * reusing the standard workqueue mechanism. * cache-side indexing, instead of memcg-side indexing. The memcg css_id serves as an index, and we don't need extra indexes for that. * struct memcg_cache_params no longer bundled in struct kmem_cache: We now will have only a pointer in the struct, allowing memory consumption when disable to fall down ever further. Patches need to be adjusted to cope with those changes, but other than that, look the same - just a lot simpler. I also put quite some effort to overcome my writing disability and get some decent changelogs in place. For a detailed explanation about this whole effort, please refer to my previous post (https://lkml.org/lkml/2012/10/8/119) Glauber Costa (18): move slabinfo processing to slab_common.c move print_slabinfo_header to slab_common.c sl[au]b: process slabinfo_show in common code slab: don't preemptively remove element from list in cache destroy slab/slub: struct memcg_params consider a memcg parameter in kmem_create_cache Allocate memory for memcg caches whenever a new memcg appe…
creator: Glauber Costa
date: 2012-10-19T14:20:24-00:00

[PATCH v5 01/18] move slabinfo processing to slab_common.c
type: item
description: This patch moves all the common machinery to slabinfo processing to slab_common.c. We can do better by noticing that the output is heavily common, and having the allocators to just provide finished information about this. But after this first step, this can be done easier. Signed-off-by: Glauber Costa <glommer@parallels.com> Acked-by: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: David Rientjes <rientjes@google.com> --- mm/slab.c | 72 ++++++++++---------------------------------------------- mm/slab.h | 8 +++++++ mm/slab_common.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ mm/slub.c | 51 ++++----------------------------------- 4 files changed, 96 insertions(+), 105 deletions(-) diff --git a/mm/slab.c b/mm/slab.c index eafef58..e35970a 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -4263,7 +4263,7 @@ out: #ifdef CONFIG_SLABINFO -static void print_slabinfo_header(struct seq_file *m) +void print_slabinfo_header(struct seq_file *m) { /* * Output format version, so at least we can change it @@ -4286,28 +4286,7 @@ static void print_slabinfo_header(struct seq_file *m) seq_putc(m, '\n'); } -static void *s_start(struct seq_file *m, loff_t *pos) -{ - loff_t n = *pos; - - mutex_lock(&slab_mutex); - if (!n) - print_slabinfo_header(m); - - return seq_list_start(&slab_caches, *pos); -} - -static void *s_next(struct seq_file *m, void *p, loff_t *pos) -{ - return seq_list_next(p, &slab_caches, pos); -} - -static void s_stop(struct seq_file *m, void *p) -{ - mutex_unlock(&slab_mutex); -} - -static int s_show(struct seq_file *m, void *p) +int slabinfo_show(struct seq_file *m, void *p) { struct kmem_cache *cachep = list_entry(p, struct kmem_cache, list); struct slab *slabp; @@ -4404,27 +4383,6 @@ static int s_show(struct seq_file *m, void *p) return 0; } -/* - * slabinfo_op - iterator that generates /proc/slabinfo - * - * Output layout: - * cache-name - * num-active-objs - * total-objs - * object size - * num-active-slabs - * total-slabs - * …
creator: Glauber Costa
date: 2012-10-19T14:20:25-00:00

[PATCH v5 02/18] move print_slabinfo_header to slab_common.c
type: item
description: The header format is highly similar between slab and slub. The main difference lays in the fact that slab may optionally have statistics added here in case of CONFIG_SLAB_DEBUG, while the slub will stick them somewhere else. By making sure that information conditionally lives inside a globally-visible CONFIG_DEBUG_SLAB switch, we can move the header printing to a common location. Signed-off-by: Glauber Costa <glommer@parallels.com> Acked-by: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: David Rientjes <rientjes@google.com> --- mm/slab.c | 24 ------------------------ mm/slab.h | 2 -- mm/slab_common.c | 23 +++++++++++++++++++++++ mm/slub.c | 10 ---------- 4 files changed, 23 insertions(+), 36 deletions(-) diff --git a/mm/slab.c b/mm/slab.c index e35970a..864a9e9 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -4262,30 +4262,6 @@ out: } #ifdef CONFIG_SLABINFO - -void print_slabinfo_header(struct seq_file *m) -{ - /* - * Output format version, so at least we can change it - * without _too_ many complaints. - */ -#if STATS - seq_puts(m, "slabinfo - version: 2.1 (statistics)\n"); -#else - seq_puts(m, "slabinfo - version: 2.1\n"); -#endif - seq_puts(m, "# name <active_objs> <num_objs> <objsize> " - "<objperslab> <pagesperslab>"); - seq_puts(m, " : tunables <limit> <batchcount> <sharedfactor>"); - seq_puts(m, " : slabdata <active_slabs> <num_slabs> <sharedavail>"); -#if STATS - seq_puts(m, " : globalstat <listallocs> <maxobjs> <grown> <reaped> " - "<error> <maxfreeable> <nodeallocs> <remotefrees> <alienoverflow>"); - seq_puts(m, " : cpustat <allochit> <allocmiss> <freehit> <freemiss>"); -#endif - seq_putc(m, '\n'); -} - int slabinfo_show(struct seq_file *m, void *p) { struct kmem_cache *cachep = list_entry(p, struct kmem_cache, list); diff --git a/mm/slab.h b/mm/slab.h index 4156d21..e9ba23f 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -74,8 +74,6 @@ int __kmem_cache_shutdown(struct kmem_cache *); struct seq_file; struct file; -void print_slabinfo_he…
creator: Glauber Costa
date: 2012-10-19T14:20:26-00:00

[PATCH v5 03/18] sl[au]b: process slabinfo_show in common code
type: item
description: With all the infrastructure in place, we can now have slabinfo_show done from slab_common.c. A cache-specific function is called to grab information about the cache itself, since that is still heavily dependent on the implementation. But with the values produced by it, all the printing and handling is done from common code. [ v2: moved objects_per_slab and cache_order to slabinfo ] 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 | 26 +++++++++++++++----------- mm/slab.h | 16 +++++++++++++++- mm/slab_common.c | 18 +++++++++++++++++- mm/slub.c | 24 ++++++++++-------------- 4 files changed, 57 insertions(+), 27 deletions(-) diff --git a/mm/slab.c b/mm/slab.c index 864a9e9..98b3460 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -4262,9 +4262,8 @@ out: } #ifdef CONFIG_SLABINFO -int slabinfo_show(struct seq_file *m, void *p) +void get_slabinfo(struct kmem_cache *cachep, struct slabinfo *sinfo) { - struct kmem_cache *cachep = list_entry(p, struct kmem_cache, list); struct slab *slabp; unsigned long active_objs; unsigned long num_objs; @@ -4319,13 +4318,20 @@ int slabinfo_show(struct seq_file *m, void *p) if (error) printk(KERN_ERR "slab: cache %s error: %s\n", name, error); - seq_printf(m, "%-17s %6lu %6lu %6u %4u %4d", - name, active_objs, num_objs, cachep->size, - cachep->num, (1 << cachep->gfporder)); - seq_printf(m, " : tunables %4u %4u %4u", - cachep->limit, cachep->batchcount, cachep->shared); - seq_printf(m, " : slabdata %6lu %6lu %6lu", - active_slabs, num_slabs, shared_avail); + sinfo->active_objs = active_objs; + sinfo->num_objs = num_objs; + sinfo->active_slabs = active_slabs; + sinfo->num_slabs = num_slabs; + sinfo->shared_avail = shared_avail; + sinfo->limit = cachep->limit; + sinfo->batchcount = cachep->batchcount; + sinfo->shared = cachep->shared; + sinfo->objects_per_slab = cachep->num; + sinfo->cache_order = cachep->gfporder…
creator: Glauber Costa
date: 2012-10-19T14:20:27-00:00

[PATCH v5 04/18] slab: don't preemptively remove element from list in cache destroy
type: item
description: After the slab/slub/slob merge, we are deleting the element from the slab_cache lists, and then if the destruction fail, we add it back again. This behavior was present in some caches, but not in others, if my memory doesn't fail me. I, however, see no reason why we need to do so, since we are now locked during the whole deletion (which wasn't necessarily true before). I propose a simplification in which we delete it only when there is no more going back, so we don't need to add it again. 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> CC: Tejun Heo <tj@kernel.org> --- mm/slab_common.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/mm/slab_common.c b/mm/slab_common.c index 1ee1d6f..bf4b4f1 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -174,16 +174,15 @@ void kmem_cache_destroy(struct kmem_cache *s) mutex_lock(&slab_mutex); s->refcount--; if (!s->refcount) { - list_del(&s->list); - if (!__kmem_cache_shutdown(s)) { if (s->flags & SLAB_DESTROY_BY_RCU) rcu_barrier(); + list_del(&s->list); + kfree(s->name); kmem_cache_free(kmem_cache, s); } else { - list_add(&s->list, &slab_caches); printk(KERN_ERR "kmem_cache_destroy %s: Slab cache still has objects\n", s->name); dump_stack(); -- 1.7.11.7
creator: Glauber Costa
date: 2012-10-19T14:20:28-00:00

[PATCH v5 05/18] 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> CC: Tejun Heo <tj@kernel.org> --- include/linux/slab.h | 25 +++++++++++++++++++++++++ include/linux/slab_def.h | 3 +++ include/linux/slub_def.h | 3 +++ mm/slab.h | 13 +++++++++++++ 4 files changed, 44 insertions(+) diff --git a/include/linux/slab.h b/include/linux/slab.h index 0dd2dfa..e4ea48a 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -177,6 +177,31 @@ unsigned int kmem_cache_size(struct kmem_cache *); #define ARCH_SLAB_MINALIGN __alignof__(unsigned long long) #endif +#include <linux/workqueue.h> +/* + * This is the main placeholder for memcg-related information in kmem caches. + * struct kmem_cache will hold a pointer to it, so the memory cost while + * disabled is 1 pointer. The runtime cost while enabled, gets bigger than it + * would otherwise be if that would be bundled in kmem_cache: we'll need an + * extra pointer chase. But the trade off clearly lays in favor of not + * penalizing non-users. + * + * Both the root cache and the child caches will have it. For the root cache, + * this will hold a dynamically allocated array large enough to hold + * information about the currently limited memcgs in the system. + * + * Child caches will hold extra metadata needed for its operation. Fields are: + * + * @memcg: pointer to the memcg this cache belongs to + */ +struct memcg_cache_params { + bool is_root_cache; + union { + struct kmem_cache *memcg_caches[0]; + struct mem_cgroup *memcg; + }; +}; + /* * Common kmalloc functions provided by all allocators */ diff --git a/include/linux/slab_def.h b/include/linux/slab_def.h index 36d7031..665a…
creator: Glauber Costa
date: 2012-10-19T14:20:29-00:00

[PATCH v5 06/18] 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. A helper is provided, 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. [ v2: moved to idr/ida instead of redoing the indexes ] [ v3: moved call to ida_init away from cgroup creation to fix a bug ] [ v4: no longer using the index mechanism ] 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> CC: Tejun Heo <tj@kernel.org> --- include/linux/memcontrol.h | 22 +++++++++++++++++++++ include/linux/slab.h | 12 +++++++++++- mm/memcontrol.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++ mm/slab.h | 23 ++++++++++++++++++---- mm/slab_common.c | 40 +++++++++++++++++++++++++++---------- mm/slub.c | 16 +++++++++++---- 6 files changed, 143 insertions(+), 19 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index fd4ff56..4f89a45 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -28,6 +28,7 @@ struct mem_cgroup; struct page_cgroup; struct page; struct mm_struct; +struct kmem_cache; /* Stats that can be updated by kernel. */ enum mem_cgroup_page_stat_item { @@ -414,6 +415,11 @@ void __memcg_kmem_commit_charge(struct page *page, struct mem_cgroup *memcg, int order); void __memcg_kmem_uncharge_pages(struct page *page, int order); +int memcg_css_id(struct mem_cgroup *memcg); +int memcg_register_cache(struct mem_cgroup *memcg, struct kmem_cache *s); +void memcg…
creator: Glauber Costa
date: 2012-10-19T14:20:30-00:00

[PATCH v5 07/18] Allocate memory for memcg caches whenever a new memcg appears
type: item
description: Every cache that is considered a root cache (basically the "original" caches, tied to the root memcg/no-memcg) will have an array that should be large enough to store a cache pointer per each memcg in the system. Theoreticaly, this is as high as 1 << sizeof(css_id), which is currently in the 64k pointers range. Most of the time, we won't be using that much. What goes in this patch, is a simple scheme to dynamically allocate such an array, in order to minimize memory usage for memcg caches. Because we would also like to avoid allocations all the time, at least for now, the array will only grow. It will tend to be big enough to hold the maximum number of kmem-limited memcgs ever achieved. We'll allocate it to be a minimum of 64 kmem-limited memcgs. When we have more than that, we'll start doubling the size of this array every time the limit is reached. Because we are only considering kmem limited memcgs, a natural point for this to happen is when we write to the limit. At that point, we already have set_limit_mutex held, so that will become our natural synchronization mechanism. 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> CC: Tejun Heo <tj@kernel.org> --- include/linux/memcontrol.h | 2 + mm/memcontrol.c | 171 ++++++++++++++++++++++++++++++++++++++++----- mm/slab_common.c | 25 +++++++ 3 files changed, 181 insertions(+), 17 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 4f89a45..491d96c 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -420,6 +420,8 @@ int memcg_register_cache(struct mem_cgroup *memcg, struct kmem_cache *s); void memcg_release_cache(struct kmem_cache *cachep); void memcg_cache_list_add(struct mem_cgroup *memcg, struct kmem_cache *c…
creator: Glauber Costa
date: 2012-10-19T14:20:31-00:00

[PATCH v5 08/18] 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. [ v4: use a standard workqueue mechanism, create right away if possible, index from cache side ] 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> CC: Tejun Heo <tj@kernel.org> --- include/linux/memcontrol.h | 41 ++++++++++ init/Kconfig | 2 +- mm/memcontrol.c | 186 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 228 insertions(+), 1 deletion(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 491d96c..92fc47a 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -422,6 +422,10 @@ void memcg_cache_list_add(struct mem_cgroup *memcg, struct kmem_cache *cachep); int memcg_update_cache_size(struct …
creator: Glauber Costa
date: 2012-10-19T14:20:32-00:00

[PATCH v5 09/18] 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. [ v2: wrap the whole enqueue process, INIT_WORK can alloc memory ] 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> CC: Tejun Heo <tj@kernel.org> --- include/linux/sched.h | 1 + mm/memcontrol.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/include/linux/sched.h b/include/linux/sched.h index 0d907e1..9fad6c1 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1581,6 +1581,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 ac2e621..3f46daa 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2950,6 +2950,41 @@ out: kfree(s->memcg_params); } +/* + * During the creation a new cache, we need to disable our accounting mechanism + * altogether. This is true even if we are not creating, but rather just + * enqueing new caches to be created. + * + * This is because that process will trigger allocations; some visible, like + * explicit kmallocs to auxiliary data structures, name strings and internal + * cache structures; some well concealed, like INIT_WORK() that can allocate + * objects during debug.…
creator: Glauber Costa
date: 2012-10-19T14:20:33-00:00

[PATCH v5 10/18] 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 ] [ v4: make it faster when kmemcg not in use ] Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> 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: Tejun Heo <tj@kernel.org> --- include/linux/memcontrol.h | 4 ++++ mm/slab.c | 17 ++++++++++++++++- mm/slab.h | 13 +++++++++++++ mm/slub.c | 14 ++++++++++++-- 4 files changed, 45 insertions(+), 3 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 92fc47a..f1ecb4f 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -534,6 +534,10 @@ static inline void sock_release_memcg(struct sock *sk) { } +static inline bool memcg_kmem_enabled(void) +{ + return false; +} static inline bool memcg_kmem_newpage_charge(gfp_t gfp, struct mem_cgroup **memcg, int order) { diff --git a/mm/slab.c b/mm/slab.c index 98b3460..6f22067 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -3911,9 +3911,24 @@ 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; + + /* + * When kmemcg is not being used, both assignments should return the + * same value. but we don't want to pay the assignment price in that + * case. If it is not compiled in, the compiler should be smart en…
creator: Glauber Costa
date: 2012-10-19T14:20:34-00:00

[PATCH v5 11/18] 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> CC: Tejun Heo <tj@kernel.org> --- include/linux/slub_def.h | 15 ++++++++++----- mm/memcontrol.c | 3 +++ mm/slab.c | 6 +++++- mm/slub.c | 5 +++-- 4 files changed, 21 insertions(+), 8 deletions(-) diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h index 961e72e..ed330df 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); + r…
creator: Glauber Costa
date: 2012-10-19T14:20:35-00:00

[PATCH v5 12/18] 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. [ v5: removed cachep backpointer ] 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> CC: Tejun Heo <tj@kernel.org> --- include/linux/memcontrol.h | 2 ++ include/linux/slab.h | 7 +++++++ mm/memcontrol.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++ mm/slab.c | 3 +++ mm/slab.h | 24 ++++++++++++++++++++++ mm/slub.c | 7 ++++++- 6 files changed, 93 insertions(+), 1 deletion(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index f1ecb4f..9152d49 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -426,6 +426,8 @@ void memcg_update_array_size(int num_groups); struct kmem_cache * __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp); +void mem_cgroup_destroy_cache(struct kmem_cache *cachep); + /** * memcg_kmem_newpage_charge: verify if a new kmem allocation is allowed. * @gfp: the gfp allocation flags. diff --git a/include/linux/slab.h b/include/linux/slab.h index b22a158..bb698dc 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -198,6 +198,10 @@ unsigned int kmem_cache_size(struct kmem_cache *); * * @memcg: pointer to the memcg this cache belongs to * @root_cache: pointer to the global, root cache, this cache was derived from + * @dead: set to true after the memcg dies; the cache may still be around. + * @nr_pages: number of pages t…
creator: Glauber Costa
date: 2012-10-19T14:20:36-00:00

[PATCH v5 13/18] 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> CC: Tejun Heo <tj@kernel.org> --- include/linux/memcontrol.h | 5 +++++ mm/memcontrol.c | 32 ++++++++++++++++++++++++++++++-- mm/slab_common.c | 3 +++ 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 9152d49..5c1d234 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -427,6 +427,7 @@ struct kmem_cache * __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp); void mem_cgroup_destroy_cache(struct kmem_cache *cachep); +void kmem_cache_destroy_memcg_children(struct kmem_cache *s); /** * memcg_kmem_newpage_charge: verify if a new kmem allocation is allowed. @@ -576,6 +577,10 @@ memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp) { return cachep; } + +static inline void kmem_cache_destroy_memcg_children(struct kmem_cache *s) +{ +} #endif /* CONFIG_MEMCG_KMEM */ #endif /* _LINUX_MEMCONTROL_H */ diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 0359b3a..f5089b3 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2715,6 +2715,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg, memcg_check_events(memcg, page); } +static DEFINE_MUTEX(set_limit_mutex); + #ifdef CONFIG_MEMCG_KMEM static inline bool memcg_can_account_kmem(struct mem_cgroup *memcg) { @@ -3095,6 +3097,34 @@ out: return new_cachep; } +void kmem_cache_destroy_memcg_children(struct kmem_cache *s) +{ + struct kmem_cache *c; + int i; + + if (!s->memcg_params) + return; +…
creator: Glauber Costa
date: 2012-10-19T14:20:37-00:00

[PATCH v5 14/18] 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 ] [ v3: use delayed_work to avoid calling verify_dead at every free] 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> CC: Tejun Heo <tj@kernel.org> --- include/linux/slab.h | 2 +- mm/memcontrol.c | 47 +++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/include/linux/slab.h b/include/linux/slab.h index bb698dc..4a3a749 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -212,7 +212,7 @@ struct memcg_cache_params { struct kmem_cache *root_cache; bool dead; atomic_t nr_pages; - struct work_struct destroy; + struct delayed_work destroy; }; }; }; diff --git a/mm/memcontrol.c b/mm/memcontrol.c index f5089b3..c7732fa 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2956,14 +2956,3…
creator: Glauber Costa
date: 2012-10-19T14:20:38-00:00

[PATCH v5 15/18] Aggregate memcg cache values in slabinfo
type: item
description: When we create caches in memcgs, we need to display their usage information somewhere. We'll adopt a scheme similar to /proc/meminfo, with aggregate totals shown in the global file, and per-group information stored in the group itself. For the time being, only reads are allowed in the per-group cache. 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> CC: Tejun Heo <tj@kernel.org> --- include/linux/memcontrol.h | 8 ++++++++ include/linux/slab.h | 4 ++++ mm/memcontrol.c | 27 ++++++++++++++++++++++++++- mm/slab.h | 27 +++++++++++++++++++++++++++ mm/slab_common.c | 42 ++++++++++++++++++++++++++++++++++++++---- 5 files changed, 103 insertions(+), 5 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 5c1d234..14def0b 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -404,6 +404,11 @@ void sock_update_memcg(struct sock *sk); void sock_release_memcg(struct sock *sk); extern struct static_key memcg_kmem_enabled_key; + +extern int memcg_limited_groups_array_size; +#define for_each_memcg_cache_index(_idx) \ + for ((_idx) = 0; i < memcg_limited_groups_array_size; (_idx)++) + static inline bool memcg_kmem_enabled(void) { return static_key_false(&memcg_kmem_enabled_key); @@ -537,6 +542,9 @@ static inline void sock_release_memcg(struct sock *sk) { } +#define for_each_memcg_cache_index(_idx) \ + for (; NULL; ) + static inline bool memcg_kmem_enabled(void) { return false; diff --git a/include/linux/slab.h b/include/linux/slab.h index 4a3a749..b521426 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -219,6 +219,10 @@ struct memcg_cache_params { int memcg_update_all_caches(int num_memcgs); +struct seq_file; +int cache_show(struct kmem_cache *s, struct seq_fil…
creator: Glauber Costa
date: 2012-10-19T14:20:39-00:00

[PATCH v5 16/18] slab: propagate tunables values
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. It can also happen that a root cache has its tunables updated during normal system operation. In this case, we will propagate the change to all caches that are already active. This change will require us to move the assignment of root_cache in memcg_params a bit earlier. We need this to be already set - which memcg_kmem_register_cache will do - when we reach __kmem_cache_create() 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> CC: Tejun Heo <tj@kernel.org> --- include/linux/memcontrol.h | 8 +++++--- include/linux/slab.h | 2 +- mm/memcontrol.c | 10 ++++++---- mm/slab.c | 44 +++++++++++++++++++++++++++++++++++++++++--- mm/slab.h | 12 ++++++++++++ mm/slab_common.c | 7 ++++--- 6 files changed, 69 insertions(+), 14 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 14def0b..9da87ff 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -421,7 +421,8 @@ void __memcg_kmem_commit_charge(struct page *page, void __memcg_kmem_uncharge_pages(struct page *page, int order); int memcg_css_id(struct mem_cgroup *memcg); -int memcg_register_cache(struct mem_cgroup *memcg, stru…
creator: Glauber Costa
date: 2012-10-19T14:20:40-00:00

[PATCH v5 17/18] slub: slub-specific propagation changes.
type: item
description: SLUB allows us to tune a particular cache behavior with sysfs-based tunables. When creating a new memcg cache copy, we'd like to preserve any tunables the parent cache already had. This can be done by tapping into the store attribute function provided by the allocator. We of course don't need to mess with read-only fields. Since the attributes can have multiple types and are stored internally by sysfs, the best strategy is to issue a ->show() in the root cache, and then ->store() in the memcg cache. The drawback of that, is that sysfs can allocate up to a page in buffering for show(), that we are likely not to need, but also can't guarantee. To avoid always allocating a page for that, we can update the caches at store time with the maximum attribute size ever stored to the root cache. We will then get a buffer big enough to hold it. The corolary to this, is that if no stores happened, nothing will be propagated. It can also happen that a root cache has its tunables updated during normal system operation. In this case, we will propagate the change to all caches that are already active. 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> CC: Tejun Heo <tj@kernel.org> --- include/linux/slub_def.h | 1 + mm/slub.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h index ed330df..f41acb9 100644 --- a/include/linux/slub_def.h +++ b/include/linux/slub_def.h @@ -105,6 +105,7 @@ struct kmem_cache { #endif #ifdef CONFIG_MEMCG_KMEM struct memcg_cache_params *memcg_params; + int max_attr_size; /* for propagation, maximum size of a stored attr */ #endif #ifdef CONFIG_NUMA diff --git a/mm/slub.c b/mm/slub.c index b5b9…
creator: Glauber Costa
date: 2012-10-19T14:20:41-00:00

[PATCH v5 18/18] Add slab-specific documentation about the kmem controller
type: item
description: 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> CC: Tejun Heo <tj@kernel.org> --- Documentation/cgroups/memory.txt | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt index 206853b..9d9938d 100644 --- a/Documentation/cgroups/memory.txt +++ b/Documentation/cgroups/memory.txt @@ -301,6 +301,13 @@ to trigger slab reclaim when those limits are reached. kernel memory, we prevent new processes from being created when the kernel memory usage is too high. +* slab pages: pages allocated by the SLAB or SLUB allocator are tracked. A copy +of each kmem_cache is created everytime the cache is touched by the first time +from inside the memcg. The creation is done lazily, so some objects can still be +skipped while the cache is being created. All objects in a slab page should +belong to the same memcg. This only fails to hold when a task is migrated to a +different memcg during the page allocation by the cache. + * sockets memory pressure: some sockets protocols have memory pressure thresholds. The Memory Controller allows them to be controlled individually per cgroup, instead of globally. -- 1.7.11.7
creator: Glauber Costa
date: 2012-10-19T14:20:42-00:00

Re: [PATCH v5 04/18] slab: don't preemptively remove element from list in cache destroy
type: item
description: On Fri, 19 Oct 2012, Glauber Costa wrote: > I, however, see no reason why we need to do so, since we are now locked > during the whole deletion (which wasn't necessarily true before). I > propose a simplification in which we delete it only when there is no > more going back, so we don't need to add it again. Ok lets hope that holding the lock does not cause issues. Acked-by: Christoph Lameter <cl@linux.com>
creator: Christoph Lameter
date: 2012-10-19T19:34:14-00:00

Re: [PATCH v5 10/18] sl[au]b: always get the cache from its page in kfree
type: item
description: On Fri, 19 Oct 2012, Glauber Costa wrote: > 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 Yes it does but the information is not standardized between the allocators yet. Coul you unify that? Come out with a struct page overlay that is as much the same as possible. Then kfree can also be unified because the lookup is always the same. That way you can move kfree into slab_common and avoid modifying multiple allocators.
creator: Christoph Lameter
date: 2012-10-19T19:44:49-00:00

Re: [PATCH v5 11/18] sl[au]b: Allocate objects from memcg cache
type: item
description: On Fri, 19 Oct 2012, Glauber Costa wrote: > 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. No that the infrastructure is being reworked currently which will allow you to do this without much of a modification of individual allocators.
creator: Christoph Lameter
date: 2012-10-19T19:46:06-00:00

Re: [PATCH v5 14/18] memcg/sl[au]b: shrink dead caches
type: item
description: On Fri, 19 Oct 2012, Glauber Costa wrote: > 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. This is also something that could be done from slab_common since all allocators have kmem_cache_shrink and kmem_cache_shrink can be used to drain the caches and free up empty slab pages.
creator: Christoph Lameter
date: 2012-10-19T19:47:51-00:00

Re: [PATCH v5 15/18] Aggregate memcg cache values in slabinfo
type: item
description: On Fri, 19 Oct 2012, Glauber Costa wrote: > + > +/* > + * We use suffixes to the name in memcg because we can't have caches > + * created in the system with the same name. But when we print them > + * locally, better refer to them with the base name > + */ > +static inline const char *cache_name(struct kmem_cache *s) > +{ > + if (!is_root_cache(s)) > + return s->memcg_params->root_cache->name; > + return s->name; > +} Could we avoid this uglyness? You can ID a slab cache by combining a memcg pointer and a slabname.
creator: Christoph Lameter
date: 2012-10-19T19:50:37-00:00

Re: [PATCH v5 16/18] slab: propagate tunables values
type: item
description: On Fri, 19 Oct 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. SLAB and SLUB allow tuning. Could you come up with some way to put these things into slab common and make it flexible so that the tuning could be used for future allocators (like SLAM etc)?
creator: Christoph Lameter
date: 2012-10-19T19:51:42-00:00

Re: [PATCH v5 14/18] memcg/sl[au]b: shrink dead caches
type: item
description: On 10/19/2012 11:47 PM, Christoph Lameter wrote: > On Fri, 19 Oct 2012, Glauber Costa wrote: > >> 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. > > This is also something that could be done from slab_common since all > allocators have kmem_cache_shrink and kmem_cache_shrink can be used to > drain the caches and free up empty slab pages. > The changelog needs to be updated. I updated the code, forgot the changelog =( I am actually now following Tejun's last suggestion, and no longer using my old verify_dead code. So I am basically calling shrink_slab every once in a while until the cache disappears. The only change I still need in the allocators is to count the amount of pages they have, so I can differentiate between need-to-shrink and need-to-destroy
creator: Glauber Costa
date: 2012-10-22T07:37:22-00:00

Re: [PATCH v5 16/18] slab: propagate tunables values
type: item
description: On 10/19/2012 11:51 PM, Christoph Lameter wrote: > On Fri, 19 Oct 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. > > SLAB and SLUB allow tuning. Could you come up with some way to put these > things into slab common and make it flexible so that the tuning could be > used for future allocators (like SLAM etc)? > They do, but they also do it very differently. Like slub uses sysfs, while slab don't. I of course fully support the integration, I just don't think this should be a blocker for all kinds of work in the allocators. Converting slab to sysfs seems to be a major work, that you are already tackling. Were it simple, I believe it would be done already. Without it, this is pretty much a fake integration... In summary, adding this doesn't make the integration work any harder in the future, and blocking this particular thing on sysfs integration is unreasonable. This being by far not central to the patchset, if this is an absolute requirement, maybe I should just drop it for the time being so it doesn't stall the rest of the development.
creator: Glauber Costa
date: 2012-10-22T07:48:01-00:00

Re: [PATCH v5 04/18] slab: don't preemptively remove element from list in cache destroy
type: item
description: On 10/19/2012 11:34 PM, Christoph Lameter wrote: > On Fri, 19 Oct 2012, Glauber Costa wrote: > >> I, however, see no reason why we need to do so, since we are now locked >> during the whole deletion (which wasn't necessarily true before). I >> propose a simplification in which we delete it only when there is no >> more going back, so we don't need to add it again. > > Ok lets hope that holding the lock does not cause issues. > > Acked-by: Christoph Lameter <cl@linux.com> > BTW: One of the good things about this set, is that we are naturally exercising cache destruction a lot more than we did before. So if there is any problem, either with this or anything related to cache destruction, it should at least show up a lot more frequently. So far, this does not seem to cause any problems.
creator: Glauber Costa
date: 2012-10-22T08:40:46-00:00

Re: [PATCH v5 10/18] sl[au]b: always get the cache from its page in kfree
type: item
description: On 10/19/2012 11:44 PM, Christoph Lameter wrote: > On Fri, 19 Oct 2012, Glauber Costa wrote: > >> 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 > > Yes it does but the information is not standardized between the allocators > yet. Coul you unify that? Come out with a struct page overlay that is as > much the same as possible. Then kfree can also be unified because the > lookup is always the same. That way you can move kfree into slab_common > and avoid modifying multiple allocators. > Ok, this is yet another changelog mistake from mine. This function is not about kfree, but kmem_cache_free. But part of your comment still applies, the page lookup can still be common. I will take a look at that.
creator: Glauber Costa
date: 2012-10-22T10:13:40-00:00

Re: [PATCH v5 15/18] Aggregate memcg cache values in slabinfo
type: item
description: On 10/19/2012 11:50 PM, Christoph Lameter wrote: > On Fri, 19 Oct 2012, Glauber Costa wrote: > >> + >> +/* >> + * We use suffixes to the name in memcg because we can't have caches >> + * created in the system with the same name. But when we print them >> + * locally, better refer to them with the base name >> + */ >> +static inline const char *cache_name(struct kmem_cache *s) >> +{ >> + if (!is_root_cache(s)) >> + return s->memcg_params->root_cache->name; >> + return s->name; >> +} > > Could we avoid this uglyness? You can ID a slab cache by combining a memcg > pointer and a slabname. > But that is not what I want. What I want is to show the cache by its root name in memcg-specific slabinfo.
creator: Glauber Costa
date: 2012-10-22T15:11:24-00:00

Re: [PATCH v5 05/18] slab/slub: struct memcg_params
type: item
description: Hi, Glauber. 2012/10/19 Glauber Costa <glommer@parallels.com>: > 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> > CC: Tejun Heo <tj@kernel.org> > --- > include/linux/slab.h | 25 +++++++++++++++++++++++++ > include/linux/slab_def.h | 3 +++ > include/linux/slub_def.h | 3 +++ > mm/slab.h | 13 +++++++++++++ > 4 files changed, 44 insertions(+) > > diff --git a/include/linux/slab.h b/include/linux/slab.h > index 0dd2dfa..e4ea48a 100644 > --- a/include/linux/slab.h > +++ b/include/linux/slab.h > @@ -177,6 +177,31 @@ unsigned int kmem_cache_size(struct kmem_cache *); > #define ARCH_SLAB_MINALIGN __alignof__(unsigned long long) > #endif > > +#include <linux/workqueue.h> Why workqueue.h is includede at this time? It may be future use, so is it better to add it later? Adding it at proper time makes git blame works better.
creator: JoonSoo Kim
date: 2012-10-23T17:25:48-00:00

Re: [PATCH v5 06/18] consider a memcg parameter in kmem_create_cache
type: item
description: 2012/10/19 Glauber Costa <glommer@parallels.com>: > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 6a1e096..59f6d54 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -339,6 +339,12 @@ struct mem_cgroup { > #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_INET) > struct tcp_memcontrol tcp_mem; > #endif > +#if defined(CONFIG_MEMCG_KMEM) > + /* analogous to slab_common's slab_caches list. per-memcg */ > + struct list_head memcg_slab_caches; > + /* Not a spinlock, we can take a lot of time walking the list */ > + struct mutex slab_caches_mutex; > +#endif > }; It is better to change name of "slab_caches_mutex to something representing slab cache mutex of memcg. > +int memcg_register_cache(struct mem_cgroup *memcg, struct kmem_cache *s) > +{ > + size_t size = sizeof(struct memcg_cache_params); > + > + if (!memcg_kmem_enabled()) > + return 0; > + > + s->memcg_params = kzalloc(size, GFP_KERNEL); > + if (!s->memcg_params) > + return -ENOMEM; > + > + if (memcg) > + s->memcg_params->memcg = memcg; > + return 0; > +} Now, I don't read full-patchset and I have not enough knowledge about cgroup. So I have a question. When memcg_kmem_enable, creation kmem_cache of normal user(not included in cgroup) also allocate memcg_params. Is it right behavior? > +void memcg_release_cache(struct kmem_cache *s) > +{ > + kfree(s->memcg_params); > +} > + > /* > * We need to verify if the allocation against current->mm->owner's memcg is > * possible for the given order. But the page is not allocated yet, so we'll > diff --git a/mm/slab.h b/mm/slab.h > index 5ee1851..c35ecce 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -35,12 +35,15 @@ extern struct kmem_cache *kmem_cache; > /* Functions provided by the slab allocators */ > extern int __kmem_cache_create(struct kmem_cache *, unsigned long flags); > > +struct mem_cgroup; > #ifdef CONFIG_SLUB > -struct kmem_cache *__kmem_cache_alias(const char *name, size_t size, > - size_t align, unsigned long flags, void (*ctor)(void *)); > +st…
creator: JoonSoo Kim
date: 2012-10-23T17:50:04-00:00

Re: [PATCH v5 16/18] slab: propagate tunables values
type: item
description: On Mon, 22 Oct 2012, Glauber Costa wrote: > On 10/19/2012 11:51 PM, Christoph Lameter wrote: > > On Fri, 19 Oct 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. > > > > SLAB and SLUB allow tuning. Could you come up with some way to put these > > things into slab common and make it flexible so that the tuning could be > > used for future allocators (like SLAM etc)? > > > They do, but they also do it very differently. Like slub uses sysfs, > while slab don't. Well yes that is something that I also want to make more general so that all allocators support sysfs style display of status and tuning. > I of course fully support the integration, I just don't think this > should be a blocker for all kinds of work in the allocators. Converting > slab to sysfs seems to be a major work, that you are already tackling. > Were it simple, I believe it would be done already. Without it, this is > pretty much a fake integration... Well there is quite a bit of infrastructure that needs to be common in order to get this done properly. I hope we will get around to that someday.
creator: Christoph Lameter
date: 2012-10-23T20:44:10-00:00

Re: [PATCH v5 01/18] move slabinfo processing to slab_common.c
type: item
description: On Fri, Oct 19, 2012 at 5:20 PM, Glauber Costa <glommer@parallels.com> wrote: > This patch moves all the common machinery to slabinfo processing > to slab_common.c. We can do better by noticing that the output is > heavily common, and having the allocators to just provide finished > information about this. But after this first step, this can be done > easier. > > Signed-off-by: Glauber Costa <glommer@parallels.com> > Acked-by: Christoph Lameter <cl@linux.com> > CC: Pekka Enberg <penberg@cs.helsinki.fi> > CC: David Rientjes <rientjes@google.com> I've applied patches 1-3. Thanks, Glauber!
creator: Pekka Enberg
date: 2012-10-24T06:43:09-00:00

Re: [PATCH v5 04/18] slab: don't preemptively remove element from list in cache destroy
type: item
description: On Mon, Oct 22, 2012 at 11:40 AM, Glauber Costa <glommer@parallels.com> wrote: > On 10/19/2012 11:34 PM, Christoph Lameter wrote: >> On Fri, 19 Oct 2012, Glauber Costa wrote: >> >>> I, however, see no reason why we need to do so, since we are now locked >>> during the whole deletion (which wasn't necessarily true before). I >>> propose a simplification in which we delete it only when there is no >>> more going back, so we don't need to add it again. >> >> Ok lets hope that holding the lock does not cause issues. >> >> Acked-by: Christoph Lameter <cl@linux.com> >> > BTW: One of the good things about this set, is that we are naturally > exercising cache destruction a lot more than we did before. So if there > is any problem, either with this or anything related to cache > destruction, it should at least show up a lot more frequently. So far, > this does not seem to cause any problems. We no longer hold the mutex the whole time after. See commit 210ed9d ("mm, slab: release slab_mutex earlier in kmem_cache_destroy()") for details.
creator: Pekka Enberg
date: 2012-10-24T06:54:05-00:00

Re: [PATCH v5 04/18] slab: don't preemptively remove element from list in cache destroy
type: item
description: On 10/24/2012 10:54 AM, Pekka Enberg wrote: > On Mon, Oct 22, 2012 at 11:40 AM, Glauber Costa <glommer@parallels.com> wrote: >> On 10/19/2012 11:34 PM, Christoph Lameter wrote: >>> On Fri, 19 Oct 2012, Glauber Costa wrote: >>> >>>> I, however, see no reason why we need to do so, since we are now locked >>>> during the whole deletion (which wasn't necessarily true before). I >>>> propose a simplification in which we delete it only when there is no >>>> more going back, so we don't need to add it again. >>> >>> Ok lets hope that holding the lock does not cause issues. >>> >>> Acked-by: Christoph Lameter <cl@linux.com> >>> >> BTW: One of the good things about this set, is that we are naturally >> exercising cache destruction a lot more than we did before. So if there >> is any problem, either with this or anything related to cache >> destruction, it should at least show up a lot more frequently. So far, >> this does not seem to cause any problems. > > We no longer hold the mutex the whole time after. See commit 210ed9d > ("mm, slab: release slab_mutex earlier in kmem_cache_destroy()") for > details. > I will resubmit then. It doesn't really change the spirit of the patch. I took a look at that fix, and what it does, is it releases the mutex right after kmem_cache_shutdown() succeeds. Removing from the list in there would do the trick.
creator: Glauber Costa
date: 2012-10-24T08:21:36-00:00

Re: [PATCH v5 06/18] consider a memcg parameter in kmem_create_cache
type: item
description: On 10/23/2012 09:50 PM, JoonSoo Kim wrote: > 2012/10/19 Glauber Costa <glommer@parallels.com>: > >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 6a1e096..59f6d54 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -339,6 +339,12 @@ struct mem_cgroup { >> #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_INET) >> struct tcp_memcontrol tcp_mem; >> #endif >> +#if defined(CONFIG_MEMCG_KMEM) >> + /* analogous to slab_common's slab_caches list. per-memcg */ >> + struct list_head memcg_slab_caches; >> + /* Not a spinlock, we can take a lot of time walking the list */ >> + struct mutex slab_caches_mutex; >> +#endif >> }; > > It is better to change name of "slab_caches_mutex to something > representing slab cache mutex of memcg. > Why do you think so? I particularly think keeping the same name is better, because it shows us that this is a memcg version of the global mutex, and wraps all caches. One is unlikely to confuse between them, because one of them is a global name, and the other is always inside a struct. >> +int memcg_register_cache(struct mem_cgroup *memcg, struct kmem_cache *s) >> +{ >> + size_t size = sizeof(struct memcg_cache_params); >> + >> + if (!memcg_kmem_enabled()) >> + return 0; >> + >> + s->memcg_params = kzalloc(size, GFP_KERNEL); >> + if (!s->memcg_params) >> + return -ENOMEM; >> + >> + if (memcg) >> + s->memcg_params->memcg = memcg; >> + return 0; >> +} > > Now, I don't read full-patchset and I have not enough knowledge about cgroup. > So I have a question. > When memcg_kmem_enable, creation kmem_cache of normal user(not > included in cgroup) also allocate memcg_params. > Is it right behavior? > Yes. I would even add that *specially* the normal kmem cache gets this. This is where it will hold the vector of memcg caches. Also note this only happens when a memcg becomes kmem-limited, so people not using memcg at all won't pay this price. This should be documented over struct memcg_params. >> +static inline bool cache_match_memcg(stru…
creator: Glauber Costa
date: 2012-10-24T08:42:09-00:00

Re: [PATCH v5 05/18] slab/slub: struct memcg_params
type: item
description: On 10/23/2012 09:25 PM, JoonSoo Kim wrote: > Hi, Glauber. > > 2012/10/19 Glauber Costa <glommer@parallels.com>: >> 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> >> CC: Tejun Heo <tj@kernel.org> >> --- >> include/linux/slab.h | 25 +++++++++++++++++++++++++ >> include/linux/slab_def.h | 3 +++ >> include/linux/slub_def.h | 3 +++ >> mm/slab.h | 13 +++++++++++++ >> 4 files changed, 44 insertions(+) >> >> diff --git a/include/linux/slab.h b/include/linux/slab.h >> index 0dd2dfa..e4ea48a 100644 >> --- a/include/linux/slab.h >> +++ b/include/linux/slab.h >> @@ -177,6 +177,31 @@ unsigned int kmem_cache_size(struct kmem_cache *); >> #define ARCH_SLAB_MINALIGN __alignof__(unsigned long long) >> #endif >> >> +#include <linux/workqueue.h> > > Why workqueue.h is includede at this time? > It may be future use, so is it better to add it later? > Adding it at proper time makes git blame works better. > It is for later, I missed this. Thanks for spotting.
creator: Glauber Costa
date: 2012-10-24T08:42:31-00:00

Re: [PATCH v5 08/18] memcg: infrastructure to match an allocation to the right cache
type: item
description: 2012/10/19 Glauber Costa <glommer@parallels.com>: > @@ -2930,9 +2937,188 @@ int memcg_register_cache(struct mem_cgroup *memcg, struct kmem_cache *s) > > void memcg_release_cache(struct kmem_cache *s) > { > + struct kmem_cache *root; > + int id = memcg_css_id(s->memcg_params->memcg); > + > + if (s->memcg_params->is_root_cache) > + goto out; > + > + root = s->memcg_params->root_cache; > + root->memcg_params->memcg_caches[id] = NULL; > + mem_cgroup_put(s->memcg_params->memcg); > +out: > kfree(s->memcg_params); > } memcg_css_id should be called after checking "s->memcg_params->is_root_cache". Because when is_root_cache == true, memcg_params has no memcg object. > +/* > + * This lock protects updaters, not readers. We want readers to be as fast as > + * they can, and they will either see NULL or a valid cache value. Our model > + * allow them to see NULL, in which case the root memcg will be selected. > + * > + * We need this lock because multiple allocations to the same cache from a non > + * GFP_WAIT area will span more than one worker. Only one of them can create > + * the cache. > + */ > +static DEFINE_MUTEX(memcg_cache_mutex); > +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)); > + > + idx = memcg_css_id(memcg); > + > + mutex_lock(&memcg_cache_mutex); > + new_cachep = cachep->memcg_params->memcg_caches[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); > + cachep->memcg_params->memcg_caches[idx] = new_cachep; > + wmb(); /* the readers won't lock, make sure everybody sees it */ Is there any rmb() pair? As far as I know, without rmb(), wmb() doesn't guarantee anything. > + new_cachep->memcg_params->memcg = memcg; > + new_cachep->memcg_params->root_cache = cachep; …
creator: JoonSoo Kim
date: 2012-10-24T18:10:29-00:00

Re: [PATCH v5 08/18] memcg: infrastructure to match an allocation to the right cache
type: item
description: On 10/24/2012 10:10 PM, JoonSoo Kim wrote: > 2012/10/19 Glauber Costa <glommer@parallels.com>: >> @@ -2930,9 +2937,188 @@ int memcg_register_cache(struct mem_cgroup *memcg, struct kmem_cache *s) >> >> void memcg_release_cache(struct kmem_cache *s) >> { >> + struct kmem_cache *root; >> + int id = memcg_css_id(s->memcg_params->memcg); >> + >> + if (s->memcg_params->is_root_cache) >> + goto out; >> + >> + root = s->memcg_params->root_cache; >> + root->memcg_params->memcg_caches[id] = NULL; >> + mem_cgroup_put(s->memcg_params->memcg); >> +out: >> kfree(s->memcg_params); >> } > > memcg_css_id should be called after checking "s->memcg_params->is_root_cache". > Because when is_root_cache == true, memcg_params has no memcg object. > Good catch. >> +/* >> + * This lock protects updaters, not readers. We want readers to be as fast as >> + * they can, and they will either see NULL or a valid cache value. Our model >> + * allow them to see NULL, in which case the root memcg will be selected. >> + * >> + * We need this lock because multiple allocations to the same cache from a non >> + * GFP_WAIT area will span more than one worker. Only one of them can create >> + * the cache. >> + */ >> +static DEFINE_MUTEX(memcg_cache_mutex); >> +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)); >> + >> + idx = memcg_css_id(memcg); >> + >> + mutex_lock(&memcg_cache_mutex); >> + new_cachep = cachep->memcg_params->memcg_caches[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); >> + cachep->memcg_params->memcg_caches[idx] = new_cachep; >> + wmb(); /* the readers won't lock, make sure everybody sees it */ > > Is there any rmb() pair? > As far as I know, without rmb(), wmb() do…
creator: Glauber Costa
date: 2012-10-25T11:05:22-00:00

Re: [PATCH v5 06/18] consider a memcg parameter in kmem_create_cache
type: item
description: On 10/23/2012 09:50 PM, JoonSoo Kim wrote: >> -struct kmem_cache *__kmem_cache_alias(const char *name, size_t size, >> > - size_t align, unsigned long flags, void (*ctor)(void *)) >> > +struct kmem_cache * >> > +__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size, >> > + size_t align, unsigned long flags, void (*ctor)(void *)) >> > { >> > struct kmem_cache *s; >> > >> > - s = find_mergeable(size, align, flags, name, ctor); >> > + s = find_mergeable(memcg, size, align, flags, name, ctor); >> > if (s) { >> > s->refcount++; >> > /* > If your intention is that find_mergeable() works for memcg-slab-caches properly, > it cannot works properly with this code. > When memcg is not NULL, slab cache is only added to memcg's slab cache list. > find_mergeable() only interate on original-slab-cache list. > So memcg slab cache never be mergeable. Actually, recent results made me reconsider this. I split this in multiple lists so we could transverse the lists faster for /proc/slabinfo. Turns out, there are many places that will rely on the ability to scan through *all* caches in the system (root or not). This is one (easily fixable) example, but there are others, like the hotplug handlers. That said, I don't think that /proc/slabinfo is *that* performance sensitive, so it is better to just skip the non-root caches, and just keep all caches in the global list. Maybe we would still benefit from a memcg-side list, for example, when we're destructing memcg, so I'll consider keeping that (with a list field in memcg_params). But even for that one, is still doable to transverse the whole list...
creator: Glauber Costa
date: 2012-10-25T13:42:55-00:00

Re: [PATCH v5 08/18] memcg: infrastructure to match an allocation to the right cache
type: item
description: Hello, Glauber. On Thu, Oct 25, 2012 at 03:05:22PM +0400, Glauber Costa wrote: > > Is there any rmb() pair? > > As far as I know, without rmb(), wmb() doesn't guarantee anything. > > > > There should be. But it seems I missed it. Speaking of which, I should You probably can use read_barrier_depends(). > wmb() after the NULL assignment in release cache as well. And you probably don't need that. dangling pointer is protected by RCU and there isn't any memory accesses which can paired with that anyway. Thanks. -- tejun
creator: Tejun Heo
date: 2012-10-25T18:06:52-00:00

Re: [PATCH v5 08/18] memcg: infrastructure to match an allocation to the right cache
type: item
description: On Thu, Oct 25, 2012 at 11:06:52AM -0700, Tejun Heo wrote: > Hello, Glauber. > > On Thu, Oct 25, 2012 at 03:05:22PM +0400, Glauber Costa wrote: > > > Is there any rmb() pair? > > > As far as I know, without rmb(), wmb() doesn't guarantee anything. > > > > > > > There should be. But it seems I missed it. Speaking of which, I should > > You probably can use read_barrier_depends(). Ooh, and when you use memory barrier pairs, please make sure to comment on both sides explaining how they're paired and why. There's nothing more frustrating than barriers without clear annotation. Thanks. -- tejun
creator: Tejun Heo
date: 2012-10-25T18:08:12-00:00

Re: [PATCH v5 11/18] sl[au]b: Allocate objects from memcg cache
type: item
description: Hi, Glauber. 2012/10/19 Glauber Costa <glommer@parallels.com>: > 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> > CC: Tejun Heo <tj@kernel.org> > --- > include/linux/slub_def.h | 15 ++++++++++----- > mm/memcontrol.c | 3 +++ > mm/slab.c | 6 +++++- > mm/slub.c | 5 +++-- > 4 files changed, 21 insertions(+), 8 deletions(-) > > diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h > index 961e72e..ed330df 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); > } You don't need this, because memcg_kmem_get_cache() is invoked in both slab_alloc() and __cache_alloc_node().
creator: JoonSoo Kim
date: 2012-10-29T15:14:34-00:00

Re: [PATCH v5 11/18] sl[au]b: Allocate objects from memcg cache
type: item
description: On 10/29/2012 07:14 PM, JoonSoo Kim wrote: > Hi, Glauber. > > 2012/10/19 Glauber Costa <glommer@parallels.com>: >> 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> >> CC: Tejun Heo <tj@kernel.org> >> --- >> include/linux/slub_def.h | 15 ++++++++++----- >> mm/memcontrol.c | 3 +++ >> mm/slab.c | 6 +++++- >> mm/slub.c | 5 +++-- >> 4 files changed, 21 insertions(+), 8 deletions(-) >> >> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h >> index 961e72e..ed330df 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); >> } > > You don't need this, > because memcg_kmem_get_cache() is invoked …
creator: Glauber Costa
date: 2012-10-29T15:19:53-00:00

Re: [PATCH v5 13/18] memcg/sl[au]b Track all the memcg children of a kmem_cache.
type: item
description: 2012/10/19 Glauber Costa <glommer@parallels.com>: > +void kmem_cache_destroy_memcg_children(struct kmem_cache *s) > +{ > + struct kmem_cache *c; > + int i; > + > + if (!s->memcg_params) > + return; > + if (!s->memcg_params->is_root_cache) > + return; > + > + /* > + * If the cache is being destroyed, we trust that there is no one else > + * requesting objects from it. Even if there are, the sanity checks in > + * kmem_cache_destroy should caught this ill-case. > + * > + * Still, we don't want anyone else freeing memcg_caches under our > + * noses, which can happen if a new memcg comes to life. As usual, > + * we'll take the set_limit_mutex to protect ourselves against this. > + */ > + mutex_lock(&set_limit_mutex); > + for (i = 0; i < memcg_limited_groups_array_size; i++) { > + c = s->memcg_params->memcg_caches[i]; > + if (c) > + kmem_cache_destroy(c); > + } > + mutex_unlock(&set_limit_mutex); > +} It may cause NULL deref. Look at the following scenario. 1. some memcg slab caches has remained object. 2. start to destroy memcg. 3. schedule_delayed_work(kmem_cache_destroy_work_func, @delay 60hz) 4. all remained object is freed. 5. start to destroy root cache. 6. kmem_cache_destroy makes 's->memcg_params->memcg_caches[i]" NULL!! 7. Start delayed work function. 8. cachep in kmem_cache_destroy_work_func() may be NULL Thanks.
creator: JoonSoo Kim
date: 2012-10-29T15:26:43-00:00

Re: [PATCH v5 13/18] memcg/sl[au]b Track all the memcg children of a kmem_cache.
type: item
description: On 10/29/2012 07:26 PM, JoonSoo Kim wrote: > 2012/10/19 Glauber Costa <glommer@parallels.com>: >> +void kmem_cache_destroy_memcg_children(struct kmem_cache *s) >> +{ >> + struct kmem_cache *c; >> + int i; >> + >> + if (!s->memcg_params) >> + return; >> + if (!s->memcg_params->is_root_cache) >> + return; >> + >> + /* >> + * If the cache is being destroyed, we trust that there is no one else >> + * requesting objects from it. Even if there are, the sanity checks in >> + * kmem_cache_destroy should caught this ill-case. >> + * >> + * Still, we don't want anyone else freeing memcg_caches under our >> + * noses, which can happen if a new memcg comes to life. As usual, >> + * we'll take the set_limit_mutex to protect ourselves against this. >> + */ >> + mutex_lock(&set_limit_mutex); >> + for (i = 0; i < memcg_limited_groups_array_size; i++) { >> + c = s->memcg_params->memcg_caches[i]; >> + if (c) >> + kmem_cache_destroy(c); >> + } >> + mutex_unlock(&set_limit_mutex); >> +} > > It may cause NULL deref. > Look at the following scenario. > > 1. some memcg slab caches has remained object. > 2. start to destroy memcg. > 3. schedule_delayed_work(kmem_cache_destroy_work_func, @delay 60hz) > 4. all remained object is freed. > 5. start to destroy root cache. > 6. kmem_cache_destroy makes 's->memcg_params->memcg_caches[i]" NULL!! > 7. Start delayed work function. > 8. cachep in kmem_cache_destroy_work_func() may be NULL > > Thanks. > Thanks for spotting. This is the same problem we have in memcg_cache_destroy(), which I solved by not respawning the worker. In here, I believe it should be possible to just cancel all remaining pending work, since we are now in the process of deleting the caches ourselves.
creator: Glauber Costa
date: 2012-10-30T11:31:37-00:00