OpenVZ Forum - RDF feed
type: channel
items: [PATCH v2 00/11] Request for Inclusion: kmem controller for memcg.
items: [PATCH v2 01/11] memcg: Make it possible to use the stock for more than one page.
items: [PATCH v2 02/11] memcg: Reclaim when more than one page needed.
items: [PATCH v2 03/11] memcg: change defines to an enum
items: [PATCH v2 04/11] kmem accounting basic infrastructure
items: [PATCH v2 05/11] Add a __GFP_KMEMCG flag
items: [PATCH v2 06/11] memcg: kmem controller infrastructure
items: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
items: [PATCH v2 08/11] memcg: disable kmem code when not in use.
items: [PATCH v2 09/11] memcg: propagate kmem limiting information to children
items: [PATCH v2 10/11] memcg: allow a memcg with kmem charges to be destructed.
items: [PATCH v2 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
items: Re: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
items: Re: [PATCH v2 01/11] memcg: Make it possible to use the stock for more than one page.
items: Re: [PATCH v2 02/11] memcg: Reclaim when more than one page needed.
items: Re: [PATCH v2 03/11] memcg: change defines to an enum
items: Re: [PATCH v2 04/11] kmem accounting basic infrastructure
items: Re: [PATCH v2 05/11] Add a __GFP_KMEMCG flag
items: Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
items: Re: [PATCH v2 09/11] memcg: propagate kmem limiting information to children
items: Re: [PATCH v2 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs

[PATCH v2 00/11] Request for Inclusion: kmem controller for memcg.
type: item
description: Hi, This is the first part of the kernel memory controller for memcg. It has been discussed many times, and I consider this stable enough to be on tree. A follow up to this series are the patches to also track slab memory. They are not included here because I believe we could benefit from merging them separately for better testing coverage. If there are any issues preventing this to be merged, let me know. I'll be happy to address them. The slab patches are also mature in my self evaluation and could be merged not too long after this. For the reference, the last discussion about them happened at http://lwn.net/Articles/508087/ A (throwaway) git tree with them is placed at: git://github.com/glommer/linux.git kmemcg-slab A general explanation of what this is all about follows: The kernel memory limitation mechanism for memcg concerns itself with disallowing potentially non-reclaimable allocations to happen in exaggerate quantities by a particular set of processes (cgroup). Those allocations could create pressure that affects the behavior of a different and unrelated set of processes. Its basic working mechanism is to annotate some allocations with the _GFP_KMEMCG flag. When this flag is set, the current process allocating will have its memcg identified and charged against. When reaching a specific limit, further allocations will be denied. One example of such problematic pressure that can be prevented by this work is a fork bomb conducted in a shell. We prevent it by noting that processes use a limited amount of stack pages. Seen this way, a fork bomb is just a special case of resource abuse. If the offender is unable to grab more pages for the stack, no new processes can be created. There are also other things the general mechanism protects against. For example, using too much of pinned dentry and inode cache, by touching files an leaving them in memory forever. In fact, a simple: while true; do mkdir x; cd x; done can halt your system easily because the file system …
creator: Glauber Costa
date: 2012-08-09T13:01:08-00:00

[PATCH v2 01/11] memcg: Make it possible to use the stock for more than one page.
type: item
description: From: Suleiman Souhlal <ssouhlal@FreeBSD.org> We currently have a percpu stock cache scheme that charges one page at a time from memcg->res, the user counter. When the kernel memory controller comes into play, we'll need to charge more than that. This is because kernel memory allocations will also draw from the user counter, and can be bigger than a single page, as it is the case with the stack (usually 2 pages) or some higher order slabs. [ glommer@parallels.com: added a changelog ] Signed-off-by: Suleiman Souhlal <suleiman@google.com> Signed-off-by: Glauber Costa <glommer@parallels.com> Acked-by: David Rientjes <rientjes@google.com> Acked-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- mm/memcontrol.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 95162c9..bc7bfa7 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2096,20 +2096,28 @@ struct memcg_stock_pcp { static DEFINE_PER_CPU(struct memcg_stock_pcp, memcg_stock); static DEFINE_MUTEX(percpu_charge_mutex); -/* - * Try to consume stocked charge on this cpu. If success, one page is consumed - * from local stock and true is returned. If the stock is 0 or charges from a - * cgroup which is not current target, returns false. This stock will be - * refilled. +/** + * consume_stock: Try to consume stocked charge on this cpu. + * @memcg: memcg to consume from. + * @nr_pages: how many pages to charge. + * + * The charges will only happen if @memcg matches the current cpu's memcg + * stock, and at least @nr_pages are available in that stock. Failure to + * service an allocation will refill the stock. + * + * returns true if succesfull, false otherwise. */ -static bool consume_stock(struct mem_cgroup *memcg) +static bool consume_stock(struct mem_cgroup *memcg, int nr_pages) { struct memcg_stock_pcp *stock; bool ret = true; + if (nr_pages > CHARGE_BATCH) + return false; + stock = &get_cpu_var(memcg_stock); - if (memcg ==…
creator: Glauber Costa
date: 2012-08-09T13:01:09-00:00

[PATCH v2 02/11] memcg: Reclaim when more than one page needed.
type: item
description: From: Suleiman Souhlal <ssouhlal@FreeBSD.org> mem_cgroup_do_charge() was written before kmem accounting, and expects three cases: being called for 1 page, being called for a stock of 32 pages, or being called for a hugepage. If we call for 2 or 3 pages (and both the stack and several slabs used in process creation are such, at least with the debug options I had), it assumed it's being called for stock and just retried without reclaiming. Fix that by passing down a minsize argument in addition to the csize. And what to do about that (csize == PAGE_SIZE && ret) retry? If it's needed at all (and presumably is since it's there, perhaps to handle races), then it should be extended to more than PAGE_SIZE, yet how far? And should there be a retry count limit, of what? For now retry up to COSTLY_ORDER (as page_alloc.c does) and make sure not to do it if __GFP_NORETRY. [v4: fixed nr pages calculation pointed out by Christoph Lameter ] Signed-off-by: Suleiman Souhlal <suleiman@google.com> Signed-off-by: Glauber Costa <glommer@parallels.com> Reviewed-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- mm/memcontrol.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index bc7bfa7..2cef99a 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2294,7 +2294,8 @@ enum { }; static int mem_cgroup_do_charge(struct mem_cgroup *memcg, gfp_t gfp_mask, - unsigned int nr_pages, bool oom_check) + unsigned int nr_pages, unsigned int min_pages, + bool oom_check) { unsigned long csize = nr_pages * PAGE_SIZE; struct mem_cgroup *mem_over_limit; @@ -2317,18 +2318,18 @@ static int mem_cgroup_do_charge(struct mem_cgroup *memcg, gfp_t gfp_mask, } else mem_over_limit = mem_cgroup_from_res_counter(fail_res, res); /* - * nr_pages can be either a huge page (HPAGE_PMD_NR), a batch - * of regular pages (CHARGE_BATCH), or a single regular page (1). - * * Never reclaim on behalf of optional batching, retry with a * single page inste…
creator: Glauber Costa
date: 2012-08-09T13:01:10-00:00

[PATCH v2 03/11] memcg: change defines to an enum
type: item
description: This is just a cleanup patch for clarity of expression. In earlier submissions, people asked it to be in a separate patch, so here it is. [ v2: use named enum as type throughout the file as well ] Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Michal Hocko <mhocko@suse.cz> CC: Johannes Weiner <hannes@cmpxchg.org> Acked-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- mm/memcontrol.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 2cef99a..b0e29f4 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -393,9 +393,12 @@ enum charge_type { }; /* for encoding cft->private value on file */ -#define _MEM (0) -#define _MEMSWAP (1) -#define _OOM_TYPE (2) +enum res_type { + _MEM, + _MEMSWAP, + _OOM_TYPE, +}; + #define MEMFILE_PRIVATE(x, val) ((x) << 16 | (val)) #define MEMFILE_TYPE(val) ((val) >> 16 & 0xffff) #define MEMFILE_ATTR(val) ((val) & 0xffff) @@ -3983,7 +3986,8 @@ static ssize_t mem_cgroup_read(struct cgroup *cont, struct cftype *cft, struct mem_cgroup *memcg = mem_cgroup_from_cont(cont); char str[64]; u64 val; - int type, name, len; + int name, len; + enum res_type type; type = MEMFILE_TYPE(cft->private); name = MEMFILE_ATTR(cft->private); @@ -4019,7 +4023,8 @@ static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft, const char *buffer) { struct mem_cgroup *memcg = mem_cgroup_from_cont(cont); - int type, name; + enum res_type type; + int name; unsigned long long val; int ret; @@ -4095,7 +4100,8 @@ out: static int mem_cgroup_reset(struct cgroup *cont, unsigned int event) { struct mem_cgroup *memcg = mem_cgroup_from_cont(cont); - int type, name; + int name; + enum res_type type; type = MEMFILE_TYPE(event); name = MEMFILE_ATTR(event); @@ -4423,7 +4429,7 @@ static int mem_cgroup_usage_register_event(struct cgroup *cgrp, struct mem_cgroup *memcg = mem_cgroup_from_cont(cgrp); struct mem_cgroup_thresholds *thresholds; struct mem_cgroup_threshold_ary *new; …
creator: Glauber Costa
date: 2012-08-09T13:01:11-00:00

[PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: This patch adds the basic infrastructure for the accounting of the slab caches. To control that, the following files are created: * memory.kmem.usage_in_bytes * memory.kmem.limit_in_bytes * memory.kmem.failcnt * memory.kmem.max_usage_in_bytes They have the same meaning of their user memory counterparts. They reflect the state of the "kmem" res_counter. The code is not enabled until a limit is set. This can be tested by the flag "kmem_accounted". This means that after the patch is applied, no behavioral changes exists for whoever is still using memcg to control their memory usage. We always account to both user and kernel resource_counters. This effectively means that an independent kernel limit is in place when the limit is set to a lower value than the user memory. A equal or higher value means that the user limit will always hit first, meaning that kmem is effectively unlimited. People who want to track kernel memory but not limit it, can set this limit to a very high number (like RESOURCE_MAX - 1page - that no one will ever hit, or equal to the user memory) Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Michal Hocko <mhocko@suse.cz> CC: Johannes Weiner <hannes@cmpxchg.org> Reviewed-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- mm/memcontrol.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index b0e29f4..54e93de 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -273,6 +273,10 @@ struct mem_cgroup { }; /* + * the counter to account for kernel memory usage. + */ + struct res_counter kmem; + /* * Per cgroup active and inactive list, similar to the * per zone LRU lists. */ @@ -287,6 +291,7 @@ struct mem_cgroup { * Should the accounting and control be hierarchical, per subtree? */ bool use_hierarchy; + bool kmem_accounted; bool oom_lock; atomic_t under_oom; @@ -397,6 +402,7 @@ enum res_type { _MEM, _MEMSWAP, _OOM_TYPE, + _KMEM, }; #d…
creator: Glauber Costa
date: 2012-08-09T13:01:12-00:00

[PATCH v2 05/11] Add a __GFP_KMEMCG flag
type: item
description: This flag is used to indicate to the callees that this allocation is a kernel allocation in process context, and should be accounted to current's memcg. It takes numerical place of the of the recently removed __GFP_NO_KSWAPD. 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: Rik van Riel <riel@redhat.com> CC: Mel Gorman <mel@csn.ul.ie> --- include/linux/gfp.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/include/linux/gfp.h b/include/linux/gfp.h index f9bc873..d8eae4d 100644 --- a/include/linux/gfp.h +++ b/include/linux/gfp.h @@ -35,6 +35,11 @@ struct vm_area_struct; #else #define ___GFP_NOTRACK 0 #endif +#ifdef CONFIG_MEMCG_KMEM +#define ___GFP_KMEMCG 0x400000u +#else +#define ___GFP_KMEMCG 0 +#endif #define ___GFP_OTHER_NODE 0x800000u #define ___GFP_WRITE 0x1000000u @@ -91,7 +96,7 @@ struct vm_area_struct; #define __GFP_OTHER_NODE ((__force gfp_t)___GFP_OTHER_NODE) /* On behalf of other node */ #define __GFP_WRITE ((__force gfp_t)___GFP_WRITE) /* Allocator intends to dirty page */ - +#define __GFP_KMEMCG ((__force gfp_t)___GFP_KMEMCG) /* Allocation comes from a memcg-accounted resource */ /* * This may seem redundant, but it's a way of annotating false positives vs. * allocations that simply cannot be supported (e.g. page tables). -- 1.7.11.2
creator: Glauber Costa
date: 2012-08-09T13:01:13-00:00

[PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: This patch introduces infrastructure for tracking kernel memory pages to a given memcg. This will happen whenever the caller includes the flag __GFP_KMEMCG flag, and the task belong to a memcg other than the root. In memcontrol.h those functions are wrapped in inline accessors. The idea is to later on, patch those with static branches, so we don't incur any overhead when no mem cgroups with limited kmem are being used. [ v2: improved comments and standardized function names ] 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 | 79 +++++++++++++++++++ mm/memcontrol.c | 185 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 264 insertions(+) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 8d9489f..75b247e 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -21,6 +21,7 @@ #define _LINUX_MEMCONTROL_H #include <linux/cgroup.h> #include <linux/vm_event_item.h> +#include <linux/hardirq.h> struct mem_cgroup; struct page_cgroup; @@ -399,6 +400,11 @@ struct sock; #ifdef CONFIG_MEMCG_KMEM void sock_update_memcg(struct sock *sk); void sock_release_memcg(struct sock *sk); + +#define memcg_kmem_on 1 +bool __memcg_kmem_new_page(gfp_t gfp, void *handle, int order); +void __memcg_kmem_commit_page(struct page *page, void *handle, int order); +void __memcg_kmem_free_page(struct page *page, int order); #else static inline void sock_update_memcg(struct sock *sk) { @@ -406,6 +412,79 @@ static inline void sock_update_memcg(struct sock *sk) static inline void sock_release_memcg(struct sock *sk) { } + +#define memcg_kmem_on 0 +static inline bool +__memcg_kmem_new_page(gfp_t gfp, void *handle, int order) +{ + return false; +} + +static inline void __memcg_kmem_free_page(struct page *page, int…
creator: Glauber Costa
date: 2012-08-09T13:01:14-00:00

[PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: When a process tries to allocate a page with the __GFP_KMEMCG flag, the page allocator will call the corresponding memcg functions to validate the allocation. Tasks in the root memcg can always proceed. To avoid adding markers to the page - and a kmem flag that would necessarily follow, as much as doing page_cgroup lookups for no reason, whoever is marking its allocations with __GFP_KMEMCG flag is responsible for telling the page allocator that this is such an allocation at free_pages() time. This is done by the invocation of __free_accounted_pages() and free_accounted_pages(). 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/gfp.h | 3 +++ mm/page_alloc.c | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/include/linux/gfp.h b/include/linux/gfp.h index d8eae4d..029570f 100644 --- a/include/linux/gfp.h +++ b/include/linux/gfp.h @@ -370,6 +370,9 @@ extern void free_pages(unsigned long addr, unsigned int order); extern void free_hot_cold_page(struct page *page, int cold); extern void free_hot_cold_page_list(struct list_head *list, int cold); +extern void __free_accounted_pages(struct page *page, unsigned int order); +extern void free_accounted_pages(unsigned long addr, unsigned int order); + #define __free_page(page) __free_pages((page), 0) #define free_page(addr) free_pages((addr), 0) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index b956cec..da341dc 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -2532,6 +2532,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, struct page *page = NULL; int migratetype = allocflags_to_migratetype(gfp_mask); unsigned int cpuset_mems_cookie; + void *handle = NULL; gfp_mask &= gfp_allowed_mask; @@ -2543,6 +2…
creator: Glauber Costa
date: 2012-08-09T13:01:15-00:00

[PATCH v2 08/11] memcg: disable kmem code when not in use.
type: item
description: We can use jump labels to patch the code in or out when not used. Because the assignment: memcg->kmem_accounted = true is done after the jump labels increment, we guarantee that the root memcg will always be selected until all call sites are patched (see memcg_kmem_enabled). This guarantees that no mischarges are applied. Jump label decrement happens when the last reference count from the memcg dies. This will only happen when the caches are all dead. Signed-off-by: Glauber Costa <glommer@parallels.com> Acked-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Johannes Weiner <hannes@cmpxchg.org> CC: Suleiman Souhlal <suleiman@google.com> --- include/linux/memcontrol.h | 5 ++++- mm/memcontrol.c | 50 ++++++++++++++++++++++++++++++++++++---------- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 75b247e..f39d933 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -22,6 +22,7 @@ #include <linux/cgroup.h> #include <linux/vm_event_item.h> #include <linux/hardirq.h> +#include <linux/jump_label.h> struct mem_cgroup; struct page_cgroup; @@ -401,7 +402,9 @@ struct sock; void sock_update_memcg(struct sock *sk); void sock_release_memcg(struct sock *sk); -#define memcg_kmem_on 1 +extern struct static_key memcg_kmem_enabled_key; +#define memcg_kmem_on static_key_false(&memcg_kmem_enabled_key) + bool __memcg_kmem_new_page(gfp_t gfp, void *handle, int order); void __memcg_kmem_commit_page(struct page *page, void *handle, int order); void __memcg_kmem_free_page(struct page *page, int order); diff --git a/mm/memcontrol.c b/mm/memcontrol.c index e9824c1..3216292 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -437,6 +437,10 @@ struct mem_cgroup *mem_cgroup_from_css(struct cgroup_subsys_state *s) #include <net/sock.h> #include <net/ip.h> +struct static_key memcg…
creator: Glauber Costa
date: 2012-08-09T13:01:16-00:00

[PATCH v2 09/11] memcg: propagate kmem limiting information to children
type: item
description: The current memcg slab cache management fails to present satisfatory hierarchical behavior in the following scenario: -> /cgroups/memory/A/B/C * kmem limit set at A, * A and B have no tasks, * span a new task in in C. Because kmem_accounted is a boolean that was not set for C, no accounting would be done. This is, however, not what we expect. The basic idea, is that when a cgroup is limited, we walk the tree upwards (something Kame and I already thought about doing for other purposes), and make sure that we store the information about the parent being limited in kmem_accounted (that is turned into a bitmap: two booleans would not be space efficient). The code for that is taken from sched/core.c. My reasons for not putting it into a common place is to dodge the type issues that would arise from a common implementation between memcg and the scheduler - but I think that it should ultimately happen, so if you want me to do it now, let me know. We do the reverse operation when a formerly limited cgroup becomes unlimited. 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/memcontrol.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 79 insertions(+), 9 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 3216292..3d30b79 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -295,7 +295,8 @@ struct mem_cgroup { * Should the accounting and control be hierarchical, per subtree? */ bool use_hierarchy; - bool kmem_accounted; + + unsigned long kmem_accounted; /* See KMEM_ACCOUNTED_*, below */ bool oom_lock; atomic_t under_oom; @@ -348,6 +349,38 @@ struct mem_cgroup { #endif }; +enum { + KMEM_ACCOUNTED_THIS, /* accounted by this cgroup itself */ + KMEM_ACCOUNTED_PARENT, /* account…
creator: Glauber Costa
date: 2012-08-09T13:01:17-00:00

[PATCH v2 10/11] memcg: allow a memcg with kmem charges to be destructed.
type: item
description: Because the ultimate goal of the kmem tracking in memcg is to track slab pages as well, we can't guarantee that we'll always be able to point a page to a particular process, and migrate the charges along with it - since in the common case, a page will contain data belonging to multiple processes. Because of that, when we destroy a memcg, we only make sure the destruction will succeed by discounting the kmem charges from the user charges when we try to empty the cgroup. Signed-off-by: Glauber Costa <glommer@parallels.com> Acked-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Johannes Weiner <hannes@cmpxchg.org> CC: Suleiman Souhlal <suleiman@google.com> --- mm/memcontrol.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 3d30b79..7c1ea49 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -649,6 +649,11 @@ static void disarm_kmem_keys(struct mem_cgroup *memcg) { if (test_bit(KMEM_ACCOUNTED_THIS, &memcg->kmem_accounted)) static_key_slow_dec(&memcg_kmem_enabled_key); + /* + * This check can't live in kmem destruction function, + * since the charges will outlive the cgroup + */ + WARN_ON(res_counter_read_u64(&memcg->kmem, RES_USAGE) != 0); } #else static void disarm_kmem_keys(struct mem_cgroup *memcg) @@ -4005,6 +4010,7 @@ static int mem_cgroup_force_empty(struct mem_cgroup *memcg, bool free_all) int node, zid, shrink; int nr_retries = MEM_CGROUP_RECLAIM_RETRIES; struct cgroup *cgrp = memcg->css.cgroup; + u64 usage; css_get(&memcg->css); @@ -4038,8 +4044,17 @@ move_account: mem_cgroup_end_move(memcg); memcg_oom_recover(memcg); cond_resched(); + /* + * Kernel memory may not necessarily be trackable to a specific + * process. So they are not migrated, and therefore we can't + * expect their value to drop to 0 here. + * + * having res filled up with kmem only is enough + */ +…
creator: Glauber Costa
date: 2012-08-09T13:01:18-00:00

[PATCH v2 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: Because those architectures will draw their stacks directly from the page allocator, rather than the slab cache, we can directly pass __GFP_KMEMCG flag, and issue the corresponding free_pages. This code path is taken when the architecture doesn't define CONFIG_ARCH_THREAD_INFO_ALLOCATOR (only ia64 seems to), and has THREAD_SIZE >= PAGE_SIZE. Luckily, most - if not all - of the remaining architectures fall in this category. This will guarantee that every stack page is accounted to the memcg the process currently lives on, and will have the allocations to fail if they go over limit. For the time being, I am defining a new variant of THREADINFO_GFP, not to mess with the other path. Once the slab is also tracked by memcg, we can get rid of that flag. Tested to successfully protect against :(){ :|:& };: Signed-off-by: Glauber Costa <glommer@parallels.com> Acked-by: Frederic Weisbecker <fweisbec@redhat.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/thread_info.h | 2 ++ kernel/fork.c | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h index ccc1899..e7e0473 100644 --- a/include/linux/thread_info.h +++ b/include/linux/thread_info.h @@ -61,6 +61,8 @@ extern long do_no_restart_syscall(struct restart_block *parm); # define THREADINFO_GFP (GFP_KERNEL | __GFP_NOTRACK) #endif +#define THREADINFO_GFP_ACCOUNTED (THREADINFO_GFP | __GFP_KMEMCG) + /* * flag set/clear/test wrappers * - pass TIF_xxxx constants to these functions diff --git a/kernel/fork.c b/kernel/fork.c index dc3ff16..b0b90c3 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -142,7 +142,7 @@ void __weak arch_release_thread_info(struct thread_info *ti) { } static struct thread_info *alloc_thread_info_node(struct task_str…
creator: Glauber Costa
date: 2012-08-09T13:01:19-00:00

Re: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: On Thu, Aug 09 2012, Glauber Costa wrote: > When a process tries to allocate a page with the __GFP_KMEMCG flag, the > page allocator will call the corresponding memcg functions to validate > the allocation. Tasks in the root memcg can always proceed. > > To avoid adding markers to the page - and a kmem flag that would > necessarily follow, as much as doing page_cgroup lookups for no reason, > whoever is marking its allocations with __GFP_KMEMCG flag is responsible > for telling the page allocator that this is such an allocation at > free_pages() time. This is done by the invocation of > __free_accounted_pages() and free_accounted_pages(). > > 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/gfp.h | 3 +++ > mm/page_alloc.c | 38 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+) > > diff --git a/include/linux/gfp.h b/include/linux/gfp.h > index d8eae4d..029570f 100644 > --- a/include/linux/gfp.h > +++ b/include/linux/gfp.h > @@ -370,6 +370,9 @@ extern void free_pages(unsigned long addr, unsigned int order); > extern void free_hot_cold_page(struct page *page, int cold); > extern void free_hot_cold_page_list(struct list_head *list, int cold); > > +extern void __free_accounted_pages(struct page *page, unsigned int order); > +extern void free_accounted_pages(unsigned long addr, unsigned int order); > + > #define __free_page(page) __free_pages((page), 0) > #define free_page(addr) free_pages((addr), 0) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index b956cec..da341dc 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -2532,6 +2532,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, > struct page *page = NULL; > int migratetype = allocf…
creator: Greg Thelen
date: 2012-08-09T16:33:03-00:00

Re: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: On 08/09/2012 08:33 PM, Greg Thelen wrote: > On Thu, Aug 09 2012, Glauber Costa wrote: > >> When a process tries to allocate a page with the __GFP_KMEMCG flag, the >> page allocator will call the corresponding memcg functions to validate >> the allocation. Tasks in the root memcg can always proceed. >> >> To avoid adding markers to the page - and a kmem flag that would >> necessarily follow, as much as doing page_cgroup lookups for no reason, >> whoever is marking its allocations with __GFP_KMEMCG flag is responsible >> for telling the page allocator that this is such an allocation at >> free_pages() time. This is done by the invocation of >> __free_accounted_pages() and free_accounted_pages(). >> >> 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/gfp.h | 3 +++ >> mm/page_alloc.c | 38 ++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 41 insertions(+) >> >> diff --git a/include/linux/gfp.h b/include/linux/gfp.h >> index d8eae4d..029570f 100644 >> --- a/include/linux/gfp.h >> +++ b/include/linux/gfp.h >> @@ -370,6 +370,9 @@ extern void free_pages(unsigned long addr, unsigned int order); >> extern void free_hot_cold_page(struct page *page, int cold); >> extern void free_hot_cold_page_list(struct list_head *list, int cold); >> >> +extern void __free_accounted_pages(struct page *page, unsigned int order); >> +extern void free_accounted_pages(unsigned long addr, unsigned int order); >> + >> #define __free_page(page) __free_pages((page), 0) >> #define free_page(addr) free_pages((addr), 0) >> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index b956cec..da341dc 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -2532,6 +2532,7 @@ __alloc_pages_nodemask(g…
creator: Glauber Costa
date: 2012-08-09T16:42:01-00:00

Re: [PATCH v2 01/11] memcg: Make it possible to use the stock for more than one page.
type: item
description: On Thu 09-08-12 17:01:09, Glauber Costa wrote: > From: Suleiman Souhlal <ssouhlal@FreeBSD.org> > > We currently have a percpu stock cache scheme that charges one page at a > time from memcg->res, the user counter. When the kernel memory > controller comes into play, we'll need to charge more than that. > > This is because kernel memory allocations will also draw from the user > counter, and can be bigger than a single page, as it is the case with > the stack (usually 2 pages) or some higher order slabs. > > [ glommer@parallels.com: added a changelog ] > > Signed-off-by: Suleiman Souhlal <suleiman@google.com> > Signed-off-by: Glauber Costa <glommer@parallels.com> > Acked-by: David Rientjes <rientjes@google.com> > Acked-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Acked-by: Michal Hocko <mhocko@suse.cz> > --- > mm/memcontrol.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 95162c9..bc7bfa7 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2096,20 +2096,28 @@ struct memcg_stock_pcp { > static DEFINE_PER_CPU(struct memcg_stock_pcp, memcg_stock); > static DEFINE_MUTEX(percpu_charge_mutex); > > -/* > - * Try to consume stocked charge on this cpu. If success, one page is consumed > - * from local stock and true is returned. If the stock is 0 or charges from a > - * cgroup which is not current target, returns false. This stock will be > - * refilled. > +/** > + * consume_stock: Try to consume stocked charge on this cpu. > + * @memcg: memcg to consume from. > + * @nr_pages: how many pages to charge. > + * > + * The charges will only happen if @memcg matches the current cpu's memcg > + * stock, and at least @nr_pages are available in that stock. Failure to > + * service an allocation will refill the stock. > + * > + * returns true if succesfull, false otherwise. > */ > -static bool consume_stock(struct mem_cgroup *memcg) > +static bool consume_stock(struct me…
creator: Michal Hocko
date: 2012-08-10T15:12:39-00:00

Re: [PATCH v2 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On Thu 09-08-12 17:01:10, Glauber Costa wrote: [...] > @@ -2317,18 +2318,18 @@ static int mem_cgroup_do_charge(struct mem_cgroup *memcg, gfp_t gfp_mask, > } else > mem_over_limit = mem_cgroup_from_res_counter(fail_res, res); > /* > - * nr_pages can be either a huge page (HPAGE_PMD_NR), a batch > - * of regular pages (CHARGE_BATCH), or a single regular page (1). > - * > * Never reclaim on behalf of optional batching, retry with a > * single page instead. > */ > - if (nr_pages == CHARGE_BATCH) > + if (nr_pages > min_pages) > return CHARGE_RETRY; This is dangerous because THP charges will be retried now while they previously failed with CHARGE_NOMEM which means that we will keep attempting potentially endlessly. Why cannot we simply do if (nr_pages < CHARGE_BATCH) and get rid of the min_pages altogether? Also the comment doesn't seem to be valid anymore. > > if (!(gfp_mask & __GFP_WAIT)) > return CHARGE_WOULDBLOCK; > > + if (gfp_mask & __GFP_NORETRY) > + return CHARGE_NOMEM; > + > ret = mem_cgroup_reclaim(mem_over_limit, gfp_mask, flags); > if (mem_cgroup_margin(mem_over_limit) >= nr_pages) > return CHARGE_RETRY; > @@ -2341,7 +2342,7 @@ static int mem_cgroup_do_charge(struct mem_cgroup *memcg, gfp_t gfp_mask, > * unlikely to succeed so close to the limit, and we fall back > * to regular pages anyway in case of failure. > */ > - if (nr_pages == 1 && ret) > + if (nr_pages <= (1 << PAGE_ALLOC_COSTLY_ORDER) && ret) > return CHARGE_RETRY; > > /* > @@ -2476,7 +2477,8 @@ again: > nr_oom_retries = MEM_CGROUP_RECLAIM_RETRIES; > } > > - ret = mem_cgroup_do_charge(memcg, gfp_mask, batch, oom_check); > + ret = mem_cgroup_do_charge(memcg, gfp_mask, batch, nr_pages, > + oom_check); > switch (ret) { > case CHARGE_OK: > break; > -- > 1.7.11.2 > > -- > To unsubscribe from this list: send the line "unsubscribe cgroups" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Michal Hocko SUSE Labs
creator: Michal Hocko
date: 2012-08-10T15:42:40-00:00

Re: [PATCH v2 03/11] memcg: change defines to an enum
type: item
description: On Thu 09-08-12 17:01:11, Glauber Costa wrote: > This is just a cleanup patch for clarity of expression. In earlier > submissions, people asked it to be in a separate patch, so here it is. > > [ v2: use named enum as type throughout the file as well ] > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: Michal Hocko <mhocko@suse.cz> > CC: Johannes Weiner <hannes@cmpxchg.org> > Acked-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Acked-by: Michal Hocko <mhocko@suse.cz> > --- > mm/memcontrol.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 2cef99a..b0e29f4 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -393,9 +393,12 @@ enum charge_type { > }; > > /* for encoding cft->private value on file */ > -#define _MEM (0) > -#define _MEMSWAP (1) > -#define _OOM_TYPE (2) > +enum res_type { > + _MEM, > + _MEMSWAP, > + _OOM_TYPE, > +}; > + > #define MEMFILE_PRIVATE(x, val) ((x) << 16 | (val)) > #define MEMFILE_TYPE(val) ((val) >> 16 & 0xffff) > #define MEMFILE_ATTR(val) ((val) & 0xffff) > @@ -3983,7 +3986,8 @@ static ssize_t mem_cgroup_read(struct cgroup *cont, struct cftype *cft, > struct mem_cgroup *memcg = mem_cgroup_from_cont(cont); > char str[64]; > u64 val; > - int type, name, len; > + int name, len; > + enum res_type type; > > type = MEMFILE_TYPE(cft->private); > name = MEMFILE_ATTR(cft->private); > @@ -4019,7 +4023,8 @@ static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft, > const char *buffer) > { > struct mem_cgroup *memcg = mem_cgroup_from_cont(cont); > - int type, name; > + enum res_type type; > + int name; > unsigned long long val; > int ret; > > @@ -4095,7 +4100,8 @@ out: > static int mem_cgroup_reset(struct cgroup *cont, unsigned int event) > { > struct mem_cgroup *memcg = mem_cgroup_from_cont(cont); > - int type, name; > + int name; > + enum res_type type; > > type = MEMFILE_TYPE(event); > name = MEMFILE_ATTR(event); > @@ -44…
creator: Michal Hocko
date: 2012-08-10T15:43:53-00:00

Re: [PATCH v2 02/11] memcg: Reclaim when more than one page needed.
type: item
description: (2012/08/11 0:42), Michal Hocko wrote: > On Thu 09-08-12 17:01:10, Glauber Costa wrote: > [...] >> @@ -2317,18 +2318,18 @@ static int mem_cgroup_do_charge(struct mem_cgroup *memcg, gfp_t gfp_mask, >> } else >> mem_over_limit = mem_cgroup_from_res_counter(fail_res, res); >> /* >> - * nr_pages can be either a huge page (HPAGE_PMD_NR), a batch >> - * of regular pages (CHARGE_BATCH), or a single regular page (1). >> - * >> * Never reclaim on behalf of optional batching, retry with a >> * single page instead. >> */ >> - if (nr_pages == CHARGE_BATCH) >> + if (nr_pages > min_pages) >> return CHARGE_RETRY; > > This is dangerous because THP charges will be retried now while they > previously failed with CHARGE_NOMEM which means that we will keep > attempting potentially endlessly. with THP, I thought nr_pages == min_pages, and no retry. > Why cannot we simply do if (nr_pages < CHARGE_BATCH) and get rid of the > min_pages altogether? Hm, I think a slab can be larger than CHARGE_BATCH. > Also the comment doesn't seem to be valid anymore. > I agree it's not clean. Because our assumption on nr_pages are changed, I think this behavior should not depend on nr_pages value.. Shouldn't we have a flag to indicate "trial-for-batched charge" ? Thanks, -Kame
creator: KAMEZAWA Hiroyuki
date: 2012-08-10T16:49:25-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: (2012/08/09 22:01), Glauber Costa wrote: > This patch adds the basic infrastructure for the accounting of the slab > caches. To control that, the following files are created: > > * memory.kmem.usage_in_bytes > * memory.kmem.limit_in_bytes > * memory.kmem.failcnt > * memory.kmem.max_usage_in_bytes > > They have the same meaning of their user memory counterparts. They > reflect the state of the "kmem" res_counter. > > The code is not enabled until a limit is set. This can be tested by the > flag "kmem_accounted". This means that after the patch is applied, no > behavioral changes exists for whoever is still using memcg to control > their memory usage. > > We always account to both user and kernel resource_counters. This > effectively means that an independent kernel limit is in place when the > limit is set to a lower value than the user memory. A equal or higher > value means that the user limit will always hit first, meaning that kmem > is effectively unlimited. > > People who want to track kernel memory but not limit it, can set this > limit to a very high number (like RESOURCE_MAX - 1page - that no one > will ever hit, or equal to the user memory) > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: Michal Hocko <mhocko@suse.cz> > CC: Johannes Weiner <hannes@cmpxchg.org> > Reviewed-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Could you add a patch for documentation of this new interface and a text explaining the behavior of "kmem_accounting" ? Hm, my concern is the difference of behavior between user page accounting and kmem accounting...but this is how tcp-accounting is working. Once you add Documentation, it's okay to add my Ack. Thanks, -Kame > --- > mm/memcontrol.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 68 insertions(+), 1 deletion(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index b0e29f4..54e93de 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -273,6 +273,10 @@ struct mem_cg…
creator: KAMEZAWA Hiroyuki
date: 2012-08-10T17:02:32-00:00

Re: [PATCH v2 05/11] Add a __GFP_KMEMCG flag
type: item
description: (2012/08/09 22:01), Glauber Costa wrote: > This flag is used to indicate to the callees that this allocation is a > kernel allocation in process context, and should be accounted to > current's memcg. It takes numerical place of the of the recently removed > __GFP_NO_KSWAPD. > > 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: Rik van Riel <riel@redhat.com> > CC: Mel Gorman <mel@csn.ul.ie> Okay, so, only memcg-aware allocations are accounted. It seems a safe way to go. Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
creator: KAMEZAWA Hiroyuki
date: 2012-08-10T17:07:31-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: (2012/08/09 22:01), Glauber Costa wrote: > This patch introduces infrastructure for tracking kernel memory pages to > a given memcg. This will happen whenever the caller includes the flag > __GFP_KMEMCG flag, and the task belong to a memcg other than the root. > > In memcontrol.h those functions are wrapped in inline accessors. The > idea is to later on, patch those with static branches, so we don't incur > any overhead when no mem cgroups with limited kmem are being used. > > [ v2: improved comments and standardized function names ] > > 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 | 79 +++++++++++++++++++ > mm/memcontrol.c | 185 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 264 insertions(+) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 8d9489f..75b247e 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -21,6 +21,7 @@ > #define _LINUX_MEMCONTROL_H > #include <linux/cgroup.h> > #include <linux/vm_event_item.h> > +#include <linux/hardirq.h> > > struct mem_cgroup; > struct page_cgroup; > @@ -399,6 +400,11 @@ struct sock; > #ifdef CONFIG_MEMCG_KMEM > void sock_update_memcg(struct sock *sk); > void sock_release_memcg(struct sock *sk); > + > +#define memcg_kmem_on 1 > +bool __memcg_kmem_new_page(gfp_t gfp, void *handle, int order); > +void __memcg_kmem_commit_page(struct page *page, void *handle, int order); > +void __memcg_kmem_free_page(struct page *page, int order); > #else > static inline void sock_update_memcg(struct sock *sk) > { > @@ -406,6 +412,79 @@ static inline void sock_update_memcg(struct sock *sk) > static inline void sock_release_memcg(struct sock *sk) > { > } > + > +#define memcg_kmem_on 0 > +static inline bool > +…
creator: KAMEZAWA Hiroyuki
date: 2012-08-10T17:27:17-00:00

Re: [PATCH v2 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On Sat 11-08-12 01:49:25, KAMEZAWA Hiroyuki wrote: > (2012/08/11 0:42), Michal Hocko wrote: > >On Thu 09-08-12 17:01:10, Glauber Costa wrote: > >[...] > >>@@ -2317,18 +2318,18 @@ static int mem_cgroup_do_charge(struct mem_cgroup *memcg, gfp_t gfp_mask, > >> } else > >> mem_over_limit = mem_cgroup_from_res_counter(fail_res, res); > >> /* > >>- * nr_pages can be either a huge page (HPAGE_PMD_NR), a batch > >>- * of regular pages (CHARGE_BATCH), or a single regular page (1). > >>- * > >> * Never reclaim on behalf of optional batching, retry with a > >> * single page instead. > >> */ > >>- if (nr_pages == CHARGE_BATCH) > >>+ if (nr_pages > min_pages) > >> return CHARGE_RETRY; > > > >This is dangerous because THP charges will be retried now while they > >previously failed with CHARGE_NOMEM which means that we will keep > >attempting potentially endlessly. > > with THP, I thought nr_pages == min_pages, and no retry. right you are. > >Why cannot we simply do if (nr_pages < CHARGE_BATCH) and get rid of the > >min_pages altogether? > > Hm, I think a slab can be larger than CHARGE_BATCH. > > >Also the comment doesn't seem to be valid anymore. > > > I agree it's not clean. Because our assumption on nr_pages are changed, > I think this behavior should not depend on nr_pages value.. > Shouldn't we have a flag to indicate "trial-for-batched charge" ? dunno, it would require a new parameter anyway (because abusing gfp doesn't seem great idea). > > > Thanks, > -Kame > > > > > -- > To unsubscribe from this list: send the line "unsubscribe cgroups" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Michal Hocko SUSE Labs
creator: Michal Hocko
date: 2012-08-10T17:28:24-00:00

Re: [PATCH v2 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On Thu 09-08-12 17:01:10, Glauber Costa wrote: [...] > For now retry up to COSTLY_ORDER (as page_alloc.c does) and make sure > not to do it if __GFP_NORETRY. Who is using __GFP_NORETRY for user backed memory (except for hugetlb which has its own controller)? -- Michal Hocko SUSE Labs
creator: Michal Hocko
date: 2012-08-10T17:30:00-00:00

Re: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: (2012/08/09 22:01), Glauber Costa wrote: > When a process tries to allocate a page with the __GFP_KMEMCG flag, the > page allocator will call the corresponding memcg functions to validate > the allocation. Tasks in the root memcg can always proceed. > > To avoid adding markers to the page - and a kmem flag that would > necessarily follow, as much as doing page_cgroup lookups for no reason, > whoever is marking its allocations with __GFP_KMEMCG flag is responsible > for telling the page allocator that this is such an allocation at > free_pages() time. This is done by the invocation of > __free_accounted_pages() and free_accounted_pages(). > > 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> Ah, ok. free_accounted_page() seems good. Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> I myself is okay with this. But... Because you add a new hook to alloc_pages(), please get Ack from Mel before requesting merge. Thanks, -Kame > --- > include/linux/gfp.h | 3 +++ > mm/page_alloc.c | 38 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+) > > diff --git a/include/linux/gfp.h b/include/linux/gfp.h > index d8eae4d..029570f 100644 > --- a/include/linux/gfp.h > +++ b/include/linux/gfp.h > @@ -370,6 +370,9 @@ extern void free_pages(unsigned long addr, unsigned int order); > extern void free_hot_cold_page(struct page *page, int cold); > extern void free_hot_cold_page_list(struct list_head *list, int cold); > > +extern void __free_accounted_pages(struct page *page, unsigned int order); > +extern void free_accounted_pages(unsigned long addr, unsigned int order); > + > #define __free_page(page) __free_pages((page), 0) > #define free_page(addr) free_pages((addr), 0) > > diff --git a/mm/pag…
creator: KAMEZAWA Hiroyuki
date: 2012-08-10T17:33:06-00:00

Re: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: On Thu, Aug 09 2012, Glauber Costa wrote: > When a process tries to allocate a page with the __GFP_KMEMCG flag, the > page allocator will call the corresponding memcg functions to validate > the allocation. Tasks in the root memcg can always proceed. > > To avoid adding markers to the page - and a kmem flag that would > necessarily follow, as much as doing page_cgroup lookups for no reason, > whoever is marking its allocations with __GFP_KMEMCG flag is responsible > for telling the page allocator that this is such an allocation at > free_pages() time. This is done by the invocation of > __free_accounted_pages() and free_accounted_pages(). > > 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/gfp.h | 3 +++ > mm/page_alloc.c | 38 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+) > > diff --git a/include/linux/gfp.h b/include/linux/gfp.h > index d8eae4d..029570f 100644 > --- a/include/linux/gfp.h > +++ b/include/linux/gfp.h > @@ -370,6 +370,9 @@ extern void free_pages(unsigned long addr, unsigned int order); > extern void free_hot_cold_page(struct page *page, int cold); > extern void free_hot_cold_page_list(struct list_head *list, int cold); > > +extern void __free_accounted_pages(struct page *page, unsigned int order); > +extern void free_accounted_pages(unsigned long addr, unsigned int order); > + > #define __free_page(page) __free_pages((page), 0) > #define free_page(addr) free_pages((addr), 0) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index b956cec..da341dc 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -2532,6 +2532,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, > struct page *page = NULL; > int migratetype = allocf…
creator: Greg Thelen
date: 2012-08-10T17:36:58-00:00

Re: [PATCH v2 09/11] memcg: propagate kmem limiting information to children
type: item
description: (2012/08/09 22:01), Glauber Costa wrote: > The current memcg slab cache management fails to present satisfatory > hierarchical behavior in the following scenario: > > -> /cgroups/memory/A/B/C > > * kmem limit set at A, > * A and B have no tasks, > * span a new task in in C. > > Because kmem_accounted is a boolean that was not set for C, no > accounting would be done. This is, however, not what we expect. > > The basic idea, is that when a cgroup is limited, we walk the tree > upwards (something Kame and I already thought about doing for other > purposes), and make sure that we store the information about the parent > being limited in kmem_accounted (that is turned into a bitmap: two > booleans would not be space efficient). The code for that is taken from > sched/core.c. My reasons for not putting it into a common place is to > dodge the type issues that would arise from a common implementation > between memcg and the scheduler - but I think that it should ultimately > happen, so if you want me to do it now, let me know. > > We do the reverse operation when a formerly limited cgroup becomes > unlimited. > > 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/memcontrol.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 79 insertions(+), 9 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 3216292..3d30b79 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -295,7 +295,8 @@ struct mem_cgroup { > * Should the accounting and control be hierarchical, per subtree? > */ > bool use_hierarchy; > - bool kmem_accounted; > + > + unsigned long kmem_accounted; /* See KMEM_ACCOUNTED_*, below */ > > bool oom_lock; > atomic_t under_oom; > @@ -348,6 +3…
creator: KAMEZAWA Hiroyuki
date: 2012-08-10T17:51:06-00:00

Re: [PATCH v2 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: (2012/08/09 22:01), Glauber Costa wrote: > Because those architectures will draw their stacks directly from the > page allocator, rather than the slab cache, we can directly pass > __GFP_KMEMCG flag, and issue the corresponding free_pages. > > This code path is taken when the architecture doesn't define > CONFIG_ARCH_THREAD_INFO_ALLOCATOR (only ia64 seems to), and has > THREAD_SIZE >= PAGE_SIZE. Luckily, most - if not all - of the remaining > architectures fall in this category. > > This will guarantee that every stack page is accounted to the memcg the > process currently lives on, and will have the allocations to fail if > they go over limit. > > For the time being, I am defining a new variant of THREADINFO_GFP, not > to mess with the other path. Once the slab is also tracked by memcg, we > can get rid of that flag. > > Tested to successfully protect against :(){ :|:& };: > > Signed-off-by: Glauber Costa <glommer@parallels.com> > Acked-by: Frederic Weisbecker <fweisbec@redhat.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> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> > --- > include/linux/thread_info.h | 2 ++ > kernel/fork.c | 4 ++-- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h > index ccc1899..e7e0473 100644 > --- a/include/linux/thread_info.h > +++ b/include/linux/thread_info.h > @@ -61,6 +61,8 @@ extern long do_no_restart_syscall(struct restart_block *parm); > # define THREADINFO_GFP (GFP_KERNEL | __GFP_NOTRACK) > #endif > > +#define THREADINFO_GFP_ACCOUNTED (THREADINFO_GFP | __GFP_KMEMCG) > + > /* > * flag set/clear/test wrappers > * - pass TIF_xxxx constants to these functions > diff --git a/kernel/fork.c b/kernel/fork.c > index dc3ff16..b0b90c3 1…
creator: KAMEZAWA Hiroyuki
date: 2012-08-10T17:54:48-00:00

Re: [PATCH v2 02/11] memcg: Reclaim when more than one page needed.
type: item
description: (2012/08/11 2:28), Michal Hocko wrote: > On Sat 11-08-12 01:49:25, KAMEZAWA Hiroyuki wrote: >> (2012/08/11 0:42), Michal Hocko wrote: >>> On Thu 09-08-12 17:01:10, Glauber Costa wrote: >>> [...] >>>> @@ -2317,18 +2318,18 @@ static int mem_cgroup_do_charge(struct mem_cgroup *memcg, gfp_t gfp_mask, >>>> } else >>>> mem_over_limit = mem_cgroup_from_res_counter(fail_res, res); >>>> /* >>>> - * nr_pages can be either a huge page (HPAGE_PMD_NR), a batch >>>> - * of regular pages (CHARGE_BATCH), or a single regular page (1). >>>> - * >>>> * Never reclaim on behalf of optional batching, retry with a >>>> * single page instead. >>>> */ >>>> - if (nr_pages == CHARGE_BATCH) >>>> + if (nr_pages > min_pages) >>>> return CHARGE_RETRY; >>> >>> This is dangerous because THP charges will be retried now while they >>> previously failed with CHARGE_NOMEM which means that we will keep >>> attempting potentially endlessly. >> >> with THP, I thought nr_pages == min_pages, and no retry. > > right you are. > >>> Why cannot we simply do if (nr_pages < CHARGE_BATCH) and get rid of the >>> min_pages altogether? >> >> Hm, I think a slab can be larger than CHARGE_BATCH. >> >>> Also the comment doesn't seem to be valid anymore. >>> >> I agree it's not clean. Because our assumption on nr_pages are changed, >> I think this behavior should not depend on nr_pages value.. >> Shouldn't we have a flag to indicate "trial-for-batched charge" ? > > dunno, it would require a new parameter anyway (because abusing gfp > doesn't seem great idea). > ok, agreed. -Kame
creator: KAMEZAWA Hiroyuki
date: 2012-08-10T17:56:20-00:00

Re: [PATCH v2 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On Fri 10-08-12 19:30:00, Michal Hocko wrote: > On Thu 09-08-12 17:01:10, Glauber Costa wrote: > [...] > > For now retry up to COSTLY_ORDER (as page_alloc.c does) and make sure > > not to do it if __GFP_NORETRY. > > Who is using __GFP_NORETRY for user backed memory (except for hugetlb > which has its own controller)? Bahh, friday brain... GFP_THISNODE used by slab. Sorry for noise. -- Michal Hocko SUSE Labs
creator: Michal Hocko
date: 2012-08-10T18:52:52-00:00

Re: [PATCH v2 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On Thu 09-08-12 17:01:10, Glauber Costa wrote: > From: Suleiman Souhlal <ssouhlal@FreeBSD.org> > > mem_cgroup_do_charge() was written before kmem accounting, and expects > three cases: being called for 1 page, being called for a stock of 32 > pages, or being called for a hugepage. If we call for 2 or 3 pages (and > both the stack and several slabs used in process creation are such, at > least with the debug options I had), it assumed it's being called for > stock and just retried without reclaiming. > > Fix that by passing down a minsize argument in addition to the csize. > > And what to do about that (csize == PAGE_SIZE && ret) retry? If it's > needed at all (and presumably is since it's there, perhaps to handle > races), then it should be extended to more than PAGE_SIZE, yet how far? > And should there be a retry count limit, of what? For now retry up to > COSTLY_ORDER (as page_alloc.c does) and make sure not to do it if > __GFP_NORETRY. > > [v4: fixed nr pages calculation pointed out by Christoph Lameter ] > > Signed-off-by: Suleiman Souhlal <suleiman@google.com> > Signed-off-by: Glauber Costa <glommer@parallels.com> > Reviewed-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> I am not happy with the min_pages argument but we can do something more clever later. Acked-by: Michal Hocko <mhocko@suse.cz> > --- > mm/memcontrol.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index bc7bfa7..2cef99a 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2294,7 +2294,8 @@ enum { > }; > > static int mem_cgroup_do_charge(struct mem_cgroup *memcg, gfp_t gfp_mask, > - unsigned int nr_pages, bool oom_check) > + unsigned int nr_pages, unsigned int min_pages, > + bool oom_check) > { > unsigned long csize = nr_pages * PAGE_SIZE; > struct mem_cgroup *mem_over_limit; > @@ -2317,18 +2318,18 @@ static int mem_cgroup_do_charge(struct mem_cgroup *memcg, gfp_t gfp_mask, > } else > mem_over_limit = …
creator: Michal Hocko
date: 2012-08-10T18:54:17-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On Thu, Aug 09 2012, Glauber Costa wrote: > This patch introduces infrastructure for tracking kernel memory pages to > a given memcg. This will happen whenever the caller includes the flag > __GFP_KMEMCG flag, and the task belong to a memcg other than the root. > > In memcontrol.h those functions are wrapped in inline accessors. The > idea is to later on, patch those with static branches, so we don't incur > any overhead when no mem cgroups with limited kmem are being used. > > [ v2: improved comments and standardized function names ] > > 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 | 79 +++++++++++++++++++ > mm/memcontrol.c | 185 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 264 insertions(+) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 8d9489f..75b247e 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -21,6 +21,7 @@ > #define _LINUX_MEMCONTROL_H > #include <linux/cgroup.h> > #include <linux/vm_event_item.h> > +#include <linux/hardirq.h> > > struct mem_cgroup; > struct page_cgroup; > @@ -399,6 +400,11 @@ struct sock; > #ifdef CONFIG_MEMCG_KMEM > void sock_update_memcg(struct sock *sk); > void sock_release_memcg(struct sock *sk); > + > +#define memcg_kmem_on 1 > +bool __memcg_kmem_new_page(gfp_t gfp, void *handle, int order); > +void __memcg_kmem_commit_page(struct page *page, void *handle, int order); > +void __memcg_kmem_free_page(struct page *page, int order); > #else > static inline void sock_update_memcg(struct sock *sk) > { > @@ -406,6 +412,79 @@ static inline void sock_update_memcg(struct sock *sk) > static inline void sock_release_memcg(struct sock *sk) > { > } > + > +#define memcg_kmem_on 0 > +static inline bool > …
creator: Greg Thelen
date: 2012-08-11T05:11:22-00:00

Re: [PATCH v2 09/11] memcg: propagate kmem limiting information to children
type: item
description: On 08/10/2012 09:51 PM, Kamezawa Hiroyuki wrote: >> + /* >> + * Once enabled, can't be disabled. We could in theory disable >> + * it if we haven't yet created any caches, or if we can shrink >> + * them all to death. But it is not worth the trouble >> + */ >> static_key_slow_inc(&memcg_kmem_enabled_key); >> - memcg->kmem_accounted = true; >> + >> + if (!memcg->use_hierarchy) >> + goto out; >> + >> + for_each_mem_cgroup_tree(iter, memcg) { >> + if (iter == memcg) >> + continue; >> + memcg_kmem_account_parent(iter); >> + } > > Could you add an explanation comment ? > Of course, Kame. Thanks
creator: Glauber Costa
date: 2012-08-13T08:01:20-00:00

Re: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: On 08/10/2012 09:36 PM, Greg Thelen wrote: > On Thu, Aug 09 2012, Glauber Costa wrote: > >> When a process tries to allocate a page with the __GFP_KMEMCG flag, the >> page allocator will call the corresponding memcg functions to validate >> the allocation. Tasks in the root memcg can always proceed. >> >> To avoid adding markers to the page - and a kmem flag that would >> necessarily follow, as much as doing page_cgroup lookups for no reason, >> whoever is marking its allocations with __GFP_KMEMCG flag is responsible >> for telling the page allocator that this is such an allocation at >> free_pages() time. This is done by the invocation of >> __free_accounted_pages() and free_accounted_pages(). >> >> 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/gfp.h | 3 +++ >> mm/page_alloc.c | 38 ++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 41 insertions(+) >> >> diff --git a/include/linux/gfp.h b/include/linux/gfp.h >> index d8eae4d..029570f 100644 >> --- a/include/linux/gfp.h >> +++ b/include/linux/gfp.h >> @@ -370,6 +370,9 @@ extern void free_pages(unsigned long addr, unsigned int order); >> extern void free_hot_cold_page(struct page *page, int cold); >> extern void free_hot_cold_page_list(struct list_head *list, int cold); >> >> +extern void __free_accounted_pages(struct page *page, unsigned int order); >> +extern void free_accounted_pages(unsigned long addr, unsigned int order); >> + >> #define __free_page(page) __free_pages((page), 0) >> #define free_page(addr) free_pages((addr), 0) >> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index b956cec..da341dc 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -2532,6 +2532,7 @@ __alloc_pages_nodemask(g…
creator: Glauber Costa
date: 2012-08-13T08:02:16-00:00

Re: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: On 08/10/2012 09:33 PM, Kamezawa Hiroyuki wrote: > (2012/08/09 22:01), Glauber Costa wrote: >> When a process tries to allocate a page with the __GFP_KMEMCG flag, the >> page allocator will call the corresponding memcg functions to validate >> the allocation. Tasks in the root memcg can always proceed. >> >> To avoid adding markers to the page - and a kmem flag that would >> necessarily follow, as much as doing page_cgroup lookups for no reason, >> whoever is marking its allocations with __GFP_KMEMCG flag is responsible >> for telling the page allocator that this is such an allocation at >> free_pages() time. This is done by the invocation of >> __free_accounted_pages() and free_accounted_pages(). >> >> 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> > > Ah, ok. free_accounted_page() seems good. > > Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> > > I myself is okay with this. But... > > Because you add a new hook to alloc_pages(), please get Ack from Mel > before requesting merge. > > Thanks, > -Kame Absolutely. Mel, would you mind taking a look at this series and commenting on this? Thanks in advance.
creator: Glauber Costa
date: 2012-08-13T08:03:38-00:00

Re: [PATCH v2 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On 08/10/2012 10:54 PM, Michal Hocko wrote: > On Thu 09-08-12 17:01:10, Glauber Costa wrote: >> From: Suleiman Souhlal <ssouhlal@FreeBSD.org> >> >> mem_cgroup_do_charge() was written before kmem accounting, and expects >> three cases: being called for 1 page, being called for a stock of 32 >> pages, or being called for a hugepage. If we call for 2 or 3 pages (and >> both the stack and several slabs used in process creation are such, at >> least with the debug options I had), it assumed it's being called for >> stock and just retried without reclaiming. >> >> Fix that by passing down a minsize argument in addition to the csize. >> >> And what to do about that (csize == PAGE_SIZE && ret) retry? If it's >> needed at all (and presumably is since it's there, perhaps to handle >> races), then it should be extended to more than PAGE_SIZE, yet how far? >> And should there be a retry count limit, of what? For now retry up to >> COSTLY_ORDER (as page_alloc.c does) and make sure not to do it if >> __GFP_NORETRY. >> >> [v4: fixed nr pages calculation pointed out by Christoph Lameter ] >> >> Signed-off-by: Suleiman Souhlal <suleiman@google.com> >> Signed-off-by: Glauber Costa <glommer@parallels.com> >> Reviewed-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> > > I am not happy with the min_pages argument but we can do something more > clever later. > > Acked-by: Michal Hocko <mhocko@suse.cz> > I am a bit confused here. Does your ack come before or after your other comments on this patch?
creator: Glauber Costa
date: 2012-08-13T08:05:38-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On 08/11/2012 09:11 AM, Greg Thelen wrote: > On Thu, Aug 09 2012, Glauber Costa wrote: > >> This patch introduces infrastructure for tracking kernel memory pages to >> a given memcg. This will happen whenever the caller includes the flag >> __GFP_KMEMCG flag, and the task belong to a memcg other than the root. >> >> In memcontrol.h those functions are wrapped in inline accessors. The >> idea is to later on, patch those with static branches, so we don't incur >> any overhead when no mem cgroups with limited kmem are being used. >> >> [ v2: improved comments and standardized function names ] >> >> 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 | 79 +++++++++++++++++++ >> mm/memcontrol.c | 185 +++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 264 insertions(+) >> >> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h >> index 8d9489f..75b247e 100644 >> --- a/include/linux/memcontrol.h >> +++ b/include/linux/memcontrol.h >> @@ -21,6 +21,7 @@ >> #define _LINUX_MEMCONTROL_H >> #include <linux/cgroup.h> >> #include <linux/vm_event_item.h> >> +#include <linux/hardirq.h> >> >> struct mem_cgroup; >> struct page_cgroup; >> @@ -399,6 +400,11 @@ struct sock; >> #ifdef CONFIG_MEMCG_KMEM >> void sock_update_memcg(struct sock *sk); >> void sock_release_memcg(struct sock *sk); >> + >> +#define memcg_kmem_on 1 >> +bool __memcg_kmem_new_page(gfp_t gfp, void *handle, int order); >> +void __memcg_kmem_commit_page(struct page *page, void *handle, int order); >> +void __memcg_kmem_free_page(struct page *page, int order); >> #else >> static inline void sock_update_memcg(struct sock *sk) >> { >> @@ -406,6 +412,79 @@ static inline void sock_update_memcg(struct sock *sk) >> static inline void sock_…
creator: Glauber Costa
date: 2012-08-13T08:07:46-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: >> > + * Needs to be called after memcg_kmem_new_page, regardless of success or >> > + * failure of the allocation. if @page is NULL, this function will revert the >> > + * charges. Otherwise, it will commit the memcg given by @handle to the >> > + * corresponding page_cgroup. >> > + */ >> > +static __always_inline void >> > +memcg_kmem_commit_page(struct page *page, struct mem_cgroup *handle, int order) >> > +{ >> > + if (memcg_kmem_on) >> > + __memcg_kmem_commit_page(page, handle, order); >> > +} > Doesn't this 2 functions has no short-cuts ? Sorry kame, what exactly do you mean? > if (memcg_kmem_on && handle) ? I guess this can be done to avoid a function call. > Maybe free() needs to access page_cgroup... > Can you also be a bit more specific here? >> > +bool __memcg_kmem_new_page(gfp_t gfp, void *_handle, int order) >> > +{ >> > + struct mem_cgroup *memcg; >> > + struct mem_cgroup **handle = (struct mem_cgroup **)_handle; >> > + bool ret = true; >> > + size_t size; >> > + struct task_struct *p; >> > + >> > + *handle = NULL; >> > + rcu_read_lock(); >> > + p = rcu_dereference(current->mm->owner); >> > + memcg = mem_cgroup_from_task(p); >> > + if (!memcg_kmem_enabled(memcg)) >> > + goto out; >> > + >> > + mem_cgroup_get(memcg); >> > + > This mem_cgroup_get() will be a potentioal performance problem. > Don't you have good idea to avoid accessing atomic counter here ? > I think some kind of percpu counter or a feature to disable "move task" > will be a help. >> > + pc = lookup_page_cgroup(page); >> > + lock_page_cgroup(pc); >> > + pc->mem_cgroup = memcg; >> > + SetPageCgroupUsed(pc); >> > + unlock_page_cgroup(pc); >> > +} >> > + >> > +void __memcg_kmem_free_page(struct page *page, int order) >> > +{ >> > + struct mem_cgroup *memcg; >> > + size_t size; >> > + struct page_cgroup *pc; >> > + >> > + if (mem_cgroup_disabled()) >> > + return; >> > + >> > + pc = lookup_page_cgroup(page); >> > + lock_page_cgroup(pc); >> > + memcg = pc->mem_cgroup; >> > + pc->mem_cgroup = NU…
creator: Glauber Costa
date: 2012-08-13T08:28:14-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On 08/10/2012 09:02 PM, Kamezawa Hiroyuki wrote: > (2012/08/09 22:01), Glauber Costa wrote: >> This patch adds the basic infrastructure for the accounting of the slab >> caches. To control that, the following files are created: >> >> * memory.kmem.usage_in_bytes >> * memory.kmem.limit_in_bytes >> * memory.kmem.failcnt >> * memory.kmem.max_usage_in_bytes >> >> They have the same meaning of their user memory counterparts. They >> reflect the state of the "kmem" res_counter. >> >> The code is not enabled until a limit is set. This can be tested by the >> flag "kmem_accounted". This means that after the patch is applied, no >> behavioral changes exists for whoever is still using memcg to control >> their memory usage. >> >> We always account to both user and kernel resource_counters. This >> effectively means that an independent kernel limit is in place when the >> limit is set to a lower value than the user memory. A equal or higher >> value means that the user limit will always hit first, meaning that kmem >> is effectively unlimited. >> >> People who want to track kernel memory but not limit it, can set this >> limit to a very high number (like RESOURCE_MAX - 1page - that no one >> will ever hit, or equal to the user memory) >> >> Signed-off-by: Glauber Costa <glommer@parallels.com> >> CC: Michal Hocko <mhocko@suse.cz> >> CC: Johannes Weiner <hannes@cmpxchg.org> >> Reviewed-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> > > Could you add a patch for documentation of this new interface and a text > explaining the behavior of "kmem_accounting" ? > > Hm, my concern is the difference of behavior between user page accounting and > kmem accounting...but this is how tcp-accounting is working. > > Once you add Documentation, it's okay to add my Ack. > I plan to add documentation in a separate patch. Due to that, can I add your ack to this patch here? Also, I find that the description text in patch0 grew to be quite informative and complete. I plan to add that to the …
creator: Glauber Costa
date: 2012-08-13T08:36:51-00:00

Re: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: On Mon, Aug 13, 2012 at 12:03:38PM +0400, Glauber Costa wrote: > On 08/10/2012 09:33 PM, Kamezawa Hiroyuki wrote: > > (2012/08/09 22:01), Glauber Costa wrote: > >> When a process tries to allocate a page with the __GFP_KMEMCG flag, the > >> page allocator will call the corresponding memcg functions to validate > >> the allocation. Tasks in the root memcg can always proceed. > >> > >> To avoid adding markers to the page - and a kmem flag that would > >> necessarily follow, as much as doing page_cgroup lookups for no reason, > >> whoever is marking its allocations with __GFP_KMEMCG flag is responsible > >> for telling the page allocator that this is such an allocation at > >> free_pages() time. This is done by the invocation of > >> __free_accounted_pages() and free_accounted_pages(). > >> > >> 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> > > > > Ah, ok. free_accounted_page() seems good. > > > > Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> > > > > I myself is okay with this. But... > > > > Because you add a new hook to alloc_pages(), please get Ack from Mel > > before requesting merge. > > > > Thanks, > > -Kame > > Absolutely. > > Mel, would you mind taking a look at this series and commenting on this? > It'll take me a few days but I'll get around to it. -- Mel Gorman SUSE Labs
creator: Mel Gorman
date: 2012-08-13T08:57:49-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: > > Here's the dmesg splat. > Do you always get this report in the same way? I managed to get a softirq inconsistency like yours, but the complaint goes for a different lock. > [ 335.550398] ================================= > [ 335.554739] [ INFO: inconsistent lock state ] > [ 335.559091] 3.5.0-dbg-DEV #3 Tainted: G W > [ 335.563946] --------------------------------- > [ 335.568290] inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage. > [ 335.574286] swapper/10/0 [HC0[0]:SC1[1]:HE1:SE0] takes: > [ 335.579508] (&(&rtpz->lock)->rlock){+.?...}, at: [<ffffffff8118216d>] __mem_cgroup_free+0x8d/0x1b0 > [ 335.588525] {SOFTIRQ-ON-W} state was registered at: > [ 335.593389] [<ffffffff810cb073>] __lock_acquire+0x623/0x1a50 > [ 335.599200] [<ffffffff810cca55>] lock_acquire+0x95/0x150 > [ 335.604670] [<ffffffff81582531>] _raw_spin_lock+0x41/0x50 > [ 335.610232] [<ffffffff8118216d>] __mem_cgroup_free+0x8d/0x1b0 > [ 335.616135] [<ffffffff811822d5>] mem_cgroup_put+0x45/0x50 > [ 335.621696] [<ffffffff81182302>] mem_cgroup_destroy+0x22/0x30 > [ 335.627592] [<ffffffff810e093f>] cgroup_diput+0xbf/0x160 > [ 335.633062] [<ffffffff811a07ef>] d_delete+0x12f/0x1a0 > [ 335.638276] [<ffffffff8119671e>] vfs_rmdir+0x11e/0x140 > [ 335.643565] [<ffffffff81199173>] do_rmdir+0x113/0x130 > [ 335.648773] [<ffffffff8119a5e6>] sys_rmdir+0x16/0x20 > [ 335.653900] [<ffffffff8158c74f>] cstar_dispatch+0x7/0x1f > [ 335.659370] irq event stamp: 399732 > [ 335.662846] hardirqs last enabled at (399732): [<ffffffff810e8e08>] res_counter_uncharge_until+0x68/0xa0 > [ 335.672383] hardirqs last disabled at (399731): [<ffffffff810e8dc8>] res_counter_uncharge_until+0x28/0xa0 > [ 335.681916] softirqs last enabled at (399710): [<ffffffff81085dd3>] _local_bh_enable+0x13/0x20 > [ 335.690590] softirqs last disabled at (399711): [<ffffffff8158c48c>] call_softirq+0x1c/0x30 > [ 335.698914] > [ 335.698914] other info that might help us debug this: > [ 335.705415] Possible unsafe locking scenario: > [ 335.705415] > [ 335.711…
creator: Glauber Costa
date: 2012-08-13T09:59:50-00:00

Re: [PATCH v2 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On Mon 13-08-12 12:05:38, Glauber Costa wrote: > On 08/10/2012 10:54 PM, Michal Hocko wrote: > > On Thu 09-08-12 17:01:10, Glauber Costa wrote: > >> From: Suleiman Souhlal <ssouhlal@FreeBSD.org> > >> > >> mem_cgroup_do_charge() was written before kmem accounting, and expects > >> three cases: being called for 1 page, being called for a stock of 32 > >> pages, or being called for a hugepage. If we call for 2 or 3 pages (and > >> both the stack and several slabs used in process creation are such, at > >> least with the debug options I had), it assumed it's being called for > >> stock and just retried without reclaiming. > >> > >> Fix that by passing down a minsize argument in addition to the csize. > >> > >> And what to do about that (csize == PAGE_SIZE && ret) retry? If it's > >> needed at all (and presumably is since it's there, perhaps to handle > >> races), then it should be extended to more than PAGE_SIZE, yet how far? > >> And should there be a retry count limit, of what? For now retry up to > >> COSTLY_ORDER (as page_alloc.c does) and make sure not to do it if > >> __GFP_NORETRY. > >> > >> [v4: fixed nr pages calculation pointed out by Christoph Lameter ] > >> > >> Signed-off-by: Suleiman Souhlal <suleiman@google.com> > >> Signed-off-by: Glauber Costa <glommer@parallels.com> > >> Reviewed-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> > > > > I am not happy with the min_pages argument but we can do something more > > clever later. > > > > Acked-by: Michal Hocko <mhocko@suse.cz> > > > > I am a bit confused here. Does your ack come before or after your other > comments on this patch? Heh, it was hard Friday ;) Yes, it was after the mind fart... -- Michal Hocko SUSE Labs
creator: Michal Hocko
date: 2012-08-13T13:10:14-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On Mon, Aug 13 2012, Glauber Costa wrote: >> >> Here's the dmesg splat. >> > > Do you always get this report in the same way? > I managed to get a softirq inconsistency like yours, but the complaint > goes for a different lock. Yes, I repeatedly get the same dmesg splat below. Once I your 'execute the whole memcg freeing in rcu callback' patch, then the warnings are not printed. I'll take a closer look at the patch soon. >> [ 335.550398] ================================= >> [ 335.554739] [ INFO: inconsistent lock state ] >> [ 335.559091] 3.5.0-dbg-DEV #3 Tainted: G W >> [ 335.563946] --------------------------------- >> [ 335.568290] inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage. >> [ 335.574286] swapper/10/0 [HC0[0]:SC1[1]:HE1:SE0] takes: >> [ 335.579508] (&(&rtpz->lock)->rlock){+.?...}, at: [<ffffffff8118216d>] __mem_cgroup_free+0x8d/0x1b0 >> [ 335.588525] {SOFTIRQ-ON-W} state was registered at: >> [ 335.593389] [<ffffffff810cb073>] __lock_acquire+0x623/0x1a50 >> [ 335.599200] [<ffffffff810cca55>] lock_acquire+0x95/0x150 >> [ 335.604670] [<ffffffff81582531>] _raw_spin_lock+0x41/0x50 >> [ 335.610232] [<ffffffff8118216d>] __mem_cgroup_free+0x8d/0x1b0 >> [ 335.616135] [<ffffffff811822d5>] mem_cgroup_put+0x45/0x50 >> [ 335.621696] [<ffffffff81182302>] mem_cgroup_destroy+0x22/0x30 >> [ 335.627592] [<ffffffff810e093f>] cgroup_diput+0xbf/0x160 >> [ 335.633062] [<ffffffff811a07ef>] d_delete+0x12f/0x1a0 >> [ 335.638276] [<ffffffff8119671e>] vfs_rmdir+0x11e/0x140 >> [ 335.643565] [<ffffffff81199173>] do_rmdir+0x113/0x130 >> [ 335.648773] [<ffffffff8119a5e6>] sys_rmdir+0x16/0x20 >> [ 335.653900] [<ffffffff8158c74f>] cstar_dispatch+0x7/0x1f >> [ 335.659370] irq event stamp: 399732 >> [ 335.662846] hardirqs last enabled at (399732): [<ffffffff810e8e08>] res_counter_uncharge_until+0x68/0xa0 >> [ 335.672383] hardirqs last disabled at (399731): [<ffffffff810e8dc8>] res_counter_uncharge_until+0x28/0xa0 >> [ 335.681916] softirqs last enabled at (399710): [<ffffffff81085dd3>] …
creator: Greg Thelen
date: 2012-08-13T21:21:19-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On 08/10/2012 09:27 PM, Kamezawa Hiroyuki wrote: >> +bool __memcg_kmem_new_page(gfp_t gfp, void *_handle, int order) >> > +{ >> > + struct mem_cgroup *memcg; >> > + struct mem_cgroup **handle = (struct mem_cgroup **)_handle; >> > + bool ret = true; >> > + size_t size; >> > + struct task_struct *p; >> > + >> > + *handle = NULL; >> > + rcu_read_lock(); >> > + p = rcu_dereference(current->mm->owner); >> > + memcg = mem_cgroup_from_task(p); >> > + if (!memcg_kmem_enabled(memcg)) >> > + goto out; >> > + >> > + mem_cgroup_get(memcg); >> > + > This mem_cgroup_get() will be a potentioal performance problem. > Don't you have good idea to avoid accessing atomic counter here ? > I think some kind of percpu counter or a feature to disable "move task" > will be a help. > > I have just sent out a proposal to deal with this. I tried the trick of marking only the first charge and last uncharge, and it works quite alright at the cost of a bit test on most calls to memcg_kmem_charge. Please let me know what you think.
creator: Glauber Costa
date: 2012-08-14T11:00:53-00:00

Re: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: On Thu, Aug 09, 2012 at 05:01:15PM +0400, Glauber Costa wrote: > When a process tries to allocate a page with the __GFP_KMEMCG flag, the > page allocator will call the corresponding memcg functions to validate > the allocation. Tasks in the root memcg can always proceed. > > To avoid adding markers to the page - and a kmem flag that would > necessarily follow, as much as doing page_cgroup lookups for no reason, As you already guessed, doing a page_cgroup in the page allocator free path would be a no-go. This is my first time glancing at the series and I'm only paying close attention to this patch so pardon me if my observations have been made already. > whoever is marking its allocations with __GFP_KMEMCG flag is responsible > for telling the page allocator that this is such an allocation at > free_pages() time. This is done by the invocation of > __free_accounted_pages() and free_accounted_pages(). > > 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/gfp.h | 3 +++ > mm/page_alloc.c | 38 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+) > > diff --git a/include/linux/gfp.h b/include/linux/gfp.h > index d8eae4d..029570f 100644 > --- a/include/linux/gfp.h > +++ b/include/linux/gfp.h > @@ -370,6 +370,9 @@ extern void free_pages(unsigned long addr, unsigned int order); > extern void free_hot_cold_page(struct page *page, int cold); > extern void free_hot_cold_page_list(struct list_head *list, int cold); > > +extern void __free_accounted_pages(struct page *page, unsigned int order); > +extern void free_accounted_pages(unsigned long addr, unsigned int order); > + > #define __free_page(page) __free_pages((page), 0) > #define free_page(addr) free_pages((addr), 0) > …
creator: Mel Gorman
date: 2012-08-14T15:16:16-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Thu 09-08-12 17:01:12, Glauber Costa wrote: > This patch adds the basic infrastructure for the accounting of the slab > caches. To control that, the following files are created: > > * memory.kmem.usage_in_bytes > * memory.kmem.limit_in_bytes > * memory.kmem.failcnt > * memory.kmem.max_usage_in_bytes > > They have the same meaning of their user memory counterparts. They > reflect the state of the "kmem" res_counter. > > The code is not enabled until a limit is set. This can be tested by the > flag "kmem_accounted". This means that after the patch is applied, no > behavioral changes exists for whoever is still using memcg to control > their memory usage. > > We always account to both user and kernel resource_counters. This > effectively means that an independent kernel limit is in place when the > limit is set to a lower value than the user memory. A equal or higher > value means that the user limit will always hit first, meaning that kmem > is effectively unlimited. Well, it contributes to the user limit so it is not unlimited. It just falls under a different limit and it tends to contribute less. This can be quite confusing. I am still not sure whether we should mix the two things together. If somebody wants to limit the kernel memory he has to touch the other limit anyway. Do you have a strong reason to mix the user and kernel counters? My impression was that kernel allocation should simply fail while user allocations might reclaim as well. Why should we reclaim just because of the kernel allocation (which is unreclaimable from hard limit reclaim point of view)? I also think that the whole thing would get much simpler if those two are split. Anyway if this is really a must then this should be documented here. One nit bellow. > People who want to track kernel memory but not limit it, can set this > limit to a very high number (like RESOURCE_MAX - 1page - that no one > will ever hit, or equal to the user memory) > > Signed-off-by: Glauber Costa <glommer@parallels.…
creator: Michal Hocko
date: 2012-08-14T16:21:55-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On Thu 09-08-12 17:01:14, Glauber Costa wrote: > This patch introduces infrastructure for tracking kernel memory pages to > a given memcg. This will happen whenever the caller includes the flag > __GFP_KMEMCG flag, and the task belong to a memcg other than the root. > > In memcontrol.h those functions are wrapped in inline accessors. The > idea is to later on, patch those with static branches, so we don't incur > any overhead when no mem cgroups with limited kmem are being used. > > [ v2: improved comments and standardized function names ] > > 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 | 79 +++++++++++++++++++ > mm/memcontrol.c | 185 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 264 insertions(+) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 8d9489f..75b247e 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h [...] > +/** > + * memcg_kmem_new_page: verify if a new kmem allocation is allowed. > + * @gfp: the gfp allocation flags. > + * @handle: a pointer to the memcg this was charged against. > + * @order: allocation order. > + * > + * returns true if the memcg where the current task belongs can hold this > + * allocation. > + * > + * We return true automatically if this allocation is not to be accounted to > + * any memcg. > + */ > +static __always_inline bool > +memcg_kmem_new_page(gfp_t gfp, void *handle, int order) > +{ > + if (!memcg_kmem_on) > + return true; > + if (!(gfp & __GFP_KMEMCG) || (gfp & __GFP_NOFAIL)) OK, I see the point behind __GFP_NOFAIL but it would deserve a comment or a mention in the changelog. [...] > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 54e93de..e9824c1 100644 > --- a/mm/memcontrol.c > +…
creator: Michal Hocko
date: 2012-08-14T17:25:40-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On Mon, Aug 13 2012, Glauber Costa wrote: >>> > + WARN_ON(mem_cgroup_is_root(memcg)); >>> > + size = (1 << order) << PAGE_SHIFT; >>> > + memcg_uncharge_kmem(memcg, size); >>> > + mem_cgroup_put(memcg); >> Why do we need ref-counting here ? kmem res_counter cannot work as >> reference ? > This is of course the pair of the mem_cgroup_get() you commented on > earlier. If we need one, we need the other. If we don't need one, we > don't need the other =) > > The guarantee we're trying to give here is that the memcg structure will > stay around while there are dangling charges to kmem, that we decided > not to move (remember: moving it for the stack is simple, for the slab > is very complicated and ill-defined, and I believe it is better to treat > all kmem equally here) By keeping memcg structures hanging around until the last referring kmem page is uncharged do such zombie memcg each consume a css_id and thus put pressure on the 64k css_id space? I imagine in pathological cases this would prevent creation of new cgroups until these zombies are dereferenced. Is there any way to see how much kmem such zombie memcg are consuming? I think we could find these with for_each_mem_cgroup_tree(root_mem_cgroup). Basically, I'm wanting to know where kernel memory has been allocated. For live memcg, an admin can cat memory.kmem.usage_in_bytes. But for zombie memcg, I'm not sure how to get this info. It looks like the root_mem_cgroup memory.kmem.usage_in_bytes is not hierarchically charged.
creator: Greg Thelen
date: 2012-08-14T18:58:10-00:00

Re: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: On 08/14/2012 07:16 PM, Mel Gorman wrote: > On Thu, Aug 09, 2012 at 05:01:15PM +0400, Glauber Costa wrote: >> When a process tries to allocate a page with the __GFP_KMEMCG flag, the >> page allocator will call the corresponding memcg functions to validate >> the allocation. Tasks in the root memcg can always proceed. >> >> To avoid adding markers to the page - and a kmem flag that would >> necessarily follow, as much as doing page_cgroup lookups for no reason, > > As you already guessed, doing a page_cgroup in the page allocator free > path would be a no-go. Specifically yes, but in general, you will be able to observe that I am taking all the possible measures to make sure existing paths are disturbed as little as possible. Thanks for your review here >> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index b956cec..da341dc 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -2532,6 +2532,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, >> struct page *page = NULL; >> int migratetype = allocflags_to_migratetype(gfp_mask); >> unsigned int cpuset_mems_cookie; >> + void *handle = NULL; >> >> gfp_mask &= gfp_allowed_mask; >> >> @@ -2543,6 +2544,13 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, >> return NULL; >> >> /* >> + * Will only have any effect when __GFP_KMEMCG is set. >> + * This is verified in the (always inline) callee >> + */ >> + if (!memcg_kmem_new_page(gfp_mask, &handle, order)) > > memcg_kmem_new_page takes a void * parameter already but here you are > passing in a void **. This probably happens to work because you do this > > struct mem_cgroup **handle = (struct mem_cgroup **)_handle; > > but that appears to defeat the purpose of having an opaque type as a > "handle". You have to treat it different then passing it into the commit > function because it expects a void *. The motivation for an opaque type > is completely unclear to me and how it is managed with a mix of void * > and void ** is very confusing. o…
creator: Glauber Costa
date: 2012-08-15T09:08:08-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On 08/14/2012 10:58 PM, Greg Thelen wrote: > On Mon, Aug 13 2012, Glauber Costa wrote: > >>>>> + WARN_ON(mem_cgroup_is_root(memcg)); >>>>> + size = (1 << order) << PAGE_SHIFT; >>>>> + memcg_uncharge_kmem(memcg, size); >>>>> + mem_cgroup_put(memcg); >>> Why do we need ref-counting here ? kmem res_counter cannot work as >>> reference ? >> This is of course the pair of the mem_cgroup_get() you commented on >> earlier. If we need one, we need the other. If we don't need one, we >> don't need the other =) >> >> The guarantee we're trying to give here is that the memcg structure will >> stay around while there are dangling charges to kmem, that we decided >> not to move (remember: moving it for the stack is simple, for the slab >> is very complicated and ill-defined, and I believe it is better to treat >> all kmem equally here) > > By keeping memcg structures hanging around until the last referring kmem > page is uncharged do such zombie memcg each consume a css_id and thus > put pressure on the 64k css_id space? I imagine in pathological cases > this would prevent creation of new cgroups until these zombies are > dereferenced. Yes, but although this patch makes it more likely, it doesn't introduce that. If the tasks, for instance, grab a reference to the cgroup dentry in the filesystem (like their CWD, etc), they will also keep the cgroup around. > Is there any way to see how much kmem such zombie memcg are consuming? > I think we could find these with > for_each_mem_cgroup_tree(root_mem_cgroup). Yes, just need an interface for that. But I think it is something that can be addressed orthogonaly to this work, in a separate patch, not as some fundamental limitation. > Basically, I'm wanting to > know where kernel memory has been allocated. For live memcg, an admin > can cat memory.kmem.usage_in_bytes. But for zombie memcg, I'm not sure > how to get this info. It looks like the root_mem_cgroup > memory.kmem.usage_in_bytes is not hierarchically charged. > Not sure what you m…
creator: Glauber Costa
date: 2012-08-15T09:18:14-00:00

Re: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: On Thu 09-08-12 17:01:15, Glauber Costa wrote: [...] > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index b956cec..da341dc 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -2532,6 +2532,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, > struct page *page = NULL; > int migratetype = allocflags_to_migratetype(gfp_mask); > unsigned int cpuset_mems_cookie; > + void *handle = NULL; > > gfp_mask &= gfp_allowed_mask; > > @@ -2543,6 +2544,13 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, > return NULL; > > /* > + * Will only have any effect when __GFP_KMEMCG is set. > + * This is verified in the (always inline) callee > + */ > + if (!memcg_kmem_new_page(gfp_mask, &handle, order)) > + return NULL; When the previous patch introduced this function I thought the handle obfuscantion is to prevent from spreading struct mem_cgroup inside the page allocator but memcg_kmem_commit_page uses the type directly. So why that obfuscation? Even handle as a name sounds unnecessarily confusing. I would go with struct mem_cgroup **memcgp or even return the pointer on success or NULL otherwise. [...] > +EXPORT_SYMBOL(__free_accounted_pages); Why exported? Btw. this is called from call_rcu context but it itself calls call_rcu down the chain in mem_cgroup_put. Is it safe? [...] > +EXPORT_SYMBOL(free_accounted_pages); here again -- Michal Hocko SUSE Labs
creator: Michal Hocko
date: 2012-08-15T09:24:05-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: >> We always account to both user and kernel resource_counters. This >> effectively means that an independent kernel limit is in place when the >> limit is set to a lower value than the user memory. A equal or higher >> value means that the user limit will always hit first, meaning that kmem >> is effectively unlimited. > > Well, it contributes to the user limit so it is not unlimited. It just > falls under a different limit and it tends to contribute less. You are right, but this is just wording. I will update it, but what I really mean here is that an independent limit is no imposed on kmem. > This can > be quite confusing. I am still not sure whether we should mix the two > things together. If somebody wants to limit the kernel memory he has to > touch the other limit anyway. Do you have a strong reason to mix the > user and kernel counters? This is funny, because the first opposition I found to this work was "Why would anyone want to limit it separately?" =p It seems that a quite common use case is to have a container with a unified view of "memory" that it can use the way he likes, be it with kernel memory, or user memory. I believe those people would be happy to just silently account kernel memory to user memory, or at the most have a switch to enable it. What gets clear from this back and forth, is that there are people interested in both use cases. > My impression was that kernel allocation should simply fail while user > allocations might reclaim as well. Why should we reclaim just because of > the kernel allocation (which is unreclaimable from hard limit reclaim > point of view)? That is not what the kernel does, in general. We assume that if he wants that memory and we can serve it, we should. Also, not all kernel memory is unreclaimable. We can shrink the slabs, for instance. Ying Han claims she has patches for that already... > I also think that the whole thing would get much simpler if those two > are split. Anyway if this is really a must then this sh…
creator: Glauber Costa
date: 2012-08-15T09:33:55-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: >> + * memcg_kmem_new_page: verify if a new kmem allocation is allowed. >> + * @gfp: the gfp allocation flags. >> + * @handle: a pointer to the memcg this was charged against. >> + * @order: allocation order. >> + * >> + * returns true if the memcg where the current task belongs can hold this >> + * allocation. >> + * >> + * We return true automatically if this allocation is not to be accounted to >> + * any memcg. >> + */ >> +static __always_inline bool >> +memcg_kmem_new_page(gfp_t gfp, void *handle, int order) >> +{ >> + if (!memcg_kmem_on) >> + return true; >> + if (!(gfp & __GFP_KMEMCG) || (gfp & __GFP_NOFAIL)) > > OK, I see the point behind __GFP_NOFAIL but it would deserve a comment > or a mention in the changelog. documentation can't hurt! Just added. > [...] >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 54e93de..e9824c1 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c > [...] >> +EXPORT_SYMBOL(__memcg_kmem_new_page); > > Why is this exported? > It shouldn't be. Removed. >> + >> +void __memcg_kmem_commit_page(struct page *page, void *handle, int order) >> +{ >> + struct page_cgroup *pc; >> + struct mem_cgroup *memcg = handle; >> + >> + if (!memcg) >> + return; >> + >> + WARN_ON(mem_cgroup_is_root(memcg)); >> + /* The page allocation must have failed. Revert */ >> + if (!page) { >> + size_t size = PAGE_SIZE << order; >> + >> + memcg_uncharge_kmem(memcg, size); >> + mem_cgroup_put(memcg); >> + return; >> + } >> + >> + pc = lookup_page_cgroup(page); >> + lock_page_cgroup(pc); >> + pc->mem_cgroup = memcg; >> + SetPageCgroupUsed(pc); > > Don't we need a write barrier before assigning memcg? Same as > __mem_cgroup_commit_charge. This tests the Used bit always from within > lock_page_cgroup so it should be safe but I am not 100% sure about the > rest of the code. > Well, I don't see the reason, precisely because we'll always grab it from within the locked region. That should ensure all the necessary serialization. >> +#ifdef CONFIG_MEMCG_KMEM >…
creator: Glauber Costa
date: 2012-08-15T09:42:24-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On 08/15/2012 01:42 PM, Glauber Costa wrote: >> Also, as I >> > have mentioned in the other email in this thread. Why should we reclaim >> > just because of kernel allocation when we are not reclaiming any of it >> > because shrink_slab is ignored in the memcg reclaim. > > Don't get too distracted by the fact that shrink_slab is ignored. It is > temporary, and while this being ignored now leads to suboptimal > behavior, it will 1st, only affect its users, and 2nd, not be disastrous. > > I see it this as more or less on pair with the soft limit reclaim > problem we had. It is not ideal, but it already provided functionality > Okay, I sent the e-mail before finishing it... duh What I meant in this last sentence, is that the situation while the memcg-aware shrinkers doesn't land in the kernel is more or less the same (obviously not exactly) as with the soft reclaim work. It is an evolutionary approach that provides some functionality that is not yet perfect but already solves lots of problems for people willing to live with its temporary drawbacks.
creator: Glauber Costa
date: 2012-08-15T10:44:15-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed, 2012-08-15 at 13:33 +0400, Glauber Costa wrote: > > This can > > be quite confusing. I am still not sure whether we should mix the two > > things together. If somebody wants to limit the kernel memory he has to > > touch the other limit anyway. Do you have a strong reason to mix the > > user and kernel counters? > > This is funny, because the first opposition I found to this work was > "Why would anyone want to limit it separately?" =p > > It seems that a quite common use case is to have a container with a > unified view of "memory" that it can use the way he likes, be it with > kernel memory, or user memory. I believe those people would be happy to > just silently account kernel memory to user memory, or at the most have > a switch to enable it. > > What gets clear from this back and forth, is that there are people > interested in both use cases. Haven't we already had this discussion during the Prague get together? We discussed the use cases and finally agreed to separate accounting for k and then k+u mem because that satisfies both the Google and Parallels cases. No-one was overjoyed by k and k+u but no-one had a better suggestion ... is there a better way of doing this that everyone can agree to? We do need to get this nailed down because it's the foundation of the patch series. James
creator: James Bottomley
date: 2012-08-15T11:12:23-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed 15-08-12 13:33:55, Glauber Costa wrote: [...] > > This can > > be quite confusing. I am still not sure whether we should mix the two > > things together. If somebody wants to limit the kernel memory he has to > > touch the other limit anyway. Do you have a strong reason to mix the > > user and kernel counters? > > This is funny, because the first opposition I found to this work was > "Why would anyone want to limit it separately?" =p > > It seems that a quite common use case is to have a container with a > unified view of "memory" that it can use the way he likes, be it with > kernel memory, or user memory. I believe those people would be happy to > just silently account kernel memory to user memory, or at the most have > a switch to enable it. > > What gets clear from this back and forth, is that there are people > interested in both use cases. I am still not 100% sure myself. It is just clear that the reclaim would need some work in order to do accounting like this. > > My impression was that kernel allocation should simply fail while user > > allocations might reclaim as well. Why should we reclaim just because of > > the kernel allocation (which is unreclaimable from hard limit reclaim > > point of view)? > > That is not what the kernel does, in general. We assume that if he wants > that memory and we can serve it, we should. Also, not all kernel memory > is unreclaimable. We can shrink the slabs, for instance. Ying Han > claims she has patches for that already... Are those patches somewhere around? [...] > > This doesn't check for the hierachy so kmem_accounted might not be in > > sync with it's parents. mem_cgroup_create (below) needs to copy > > kmem_accounted down from the parent and the above needs to check if this > > is a similar dance like mem_cgroup_oom_control_write. > > > > I don't see why we have to. > > I believe in a A/B/C hierarchy, C should be perfectly able to set a > different limit than its parents. Note that this is not a boolean. Ohh,…
creator: Michal Hocko
date: 2012-08-15T12:39:31-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On 08/15/2012 04:39 PM, Michal Hocko wrote: > On Wed 15-08-12 13:33:55, Glauber Costa wrote: > [...] >>> This can >>> be quite confusing. I am still not sure whether we should mix the two >>> things together. If somebody wants to limit the kernel memory he has to >>> touch the other limit anyway. Do you have a strong reason to mix the >>> user and kernel counters? >> >> This is funny, because the first opposition I found to this work was >> "Why would anyone want to limit it separately?" =p >> >> It seems that a quite common use case is to have a container with a >> unified view of "memory" that it can use the way he likes, be it with >> kernel memory, or user memory. I believe those people would be happy to >> just silently account kernel memory to user memory, or at the most have >> a switch to enable it. >> >> What gets clear from this back and forth, is that there are people >> interested in both use cases. > > I am still not 100% sure myself. It is just clear that the reclaim would > need some work in order to do accounting like this. > Note: Besides what I've already said, right *now* in this series we are accounting just stack. So reclaimable vs not-reclaimable doesn't even get to play. It is used while the tasks are running, it gets freed after the tasks exited. I do agree we need to look to the whole picture, and reclaiming will be hard to get right. This is actually why we're addressing them separately: because they are a hard problem on their own, and the current status of accounting already solve real life problems for many, though not for all. >>> My impression was that kernel allocation should simply fail while user >>> allocations might reclaim as well. Why should we reclaim just because of >>> the kernel allocation (which is unreclaimable from hard limit reclaim >>> point of view)? >> >> That is not what the kernel does, in general. We assume that if he wants >> that memory and we can serve it, we should. Also, not all kernel memory >> is unreclaimab…
creator: Glauber Costa
date: 2012-08-15T12:53:40-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed 15-08-12 12:12:23, James Bottomley wrote: > On Wed, 2012-08-15 at 13:33 +0400, Glauber Costa wrote: > > > This can > > > be quite confusing. I am still not sure whether we should mix the two > > > things together. If somebody wants to limit the kernel memory he has to > > > touch the other limit anyway. Do you have a strong reason to mix the > > > user and kernel counters? > > > > This is funny, because the first opposition I found to this work was > > "Why would anyone want to limit it separately?" =p > > > > It seems that a quite common use case is to have a container with a > > unified view of "memory" that it can use the way he likes, be it with > > kernel memory, or user memory. I believe those people would be happy to > > just silently account kernel memory to user memory, or at the most have > > a switch to enable it. > > > > What gets clear from this back and forth, is that there are people > > interested in both use cases. > > Haven't we already had this discussion during the Prague get together? > We discussed the use cases and finally agreed to separate accounting for > k and then k+u mem because that satisfies both the Google and Parallels > cases. No-one was overjoyed by k and k+u but no-one had a better > suggestion ... is there a better way of doing this that everyone can > agree to? > We do need to get this nailed down because it's the foundation of the > patch series. There is a slot in MM/memcg minisum at KS so we have a slot to discuss this. > > James > > > -- > To unsubscribe from this list: send the line "unsubscribe cgroups" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Michal Hocko SUSE Labs
creator: Michal Hocko
date: 2012-08-15T12:55:55-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed 15-08-12 16:53:40, Glauber Costa wrote: [...] > >>> This doesn't check for the hierachy so kmem_accounted might not be in > >>> sync with it's parents. mem_cgroup_create (below) needs to copy > >>> kmem_accounted down from the parent and the above needs to check if this > >>> is a similar dance like mem_cgroup_oom_control_write. > >>> > >> > >> I don't see why we have to. > >> > >> I believe in a A/B/C hierarchy, C should be perfectly able to set a > >> different limit than its parents. Note that this is not a boolean. > > > > Ohh, I wasn't clear enough. I am not against setting the _limit_ I just > > meant that the kmem_accounted should be consistent within the hierarchy. > > > > If a parent of yours is accounted, you get accounted as well. This is > not the state in this patch, but gets added later. Isn't this enough ? But if the parent is not accounted, you can set the children to be accounted, right? Or maybe this is changed later in the series? I didn't get to the end yet. -- Michal Hocko SUSE Labs
creator: Michal Hocko
date: 2012-08-15T13:02:28-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On 08/15/2012 05:02 PM, Michal Hocko wrote: > On Wed 15-08-12 16:53:40, Glauber Costa wrote: > [...] >>>>> This doesn't check for the hierachy so kmem_accounted might not be in >>>>> sync with it's parents. mem_cgroup_create (below) needs to copy >>>>> kmem_accounted down from the parent and the above needs to check if this >>>>> is a similar dance like mem_cgroup_oom_control_write. >>>>> >>>> >>>> I don't see why we have to. >>>> >>>> I believe in a A/B/C hierarchy, C should be perfectly able to set a >>>> different limit than its parents. Note that this is not a boolean. >>> >>> Ohh, I wasn't clear enough. I am not against setting the _limit_ I just >>> meant that the kmem_accounted should be consistent within the hierarchy. >>> >> >> If a parent of yours is accounted, you get accounted as well. This is >> not the state in this patch, but gets added later. Isn't this enough ? > > But if the parent is not accounted, you can set the children to be > accounted, right? Or maybe this is changed later in the series? I didn't > get to the end yet. > Yes, you can. Do you see any problem with that?
creator: Glauber Costa
date: 2012-08-15T13:04:31-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On Wed 15-08-12 13:42:24, Glauber Costa wrote: [...] > >> + > >> + ret = 0; > >> + > >> + if (!memcg) > >> + return ret; > >> + > >> + _memcg = memcg; > >> + ret = __mem_cgroup_try_charge(NULL, gfp, delta / PAGE_SIZE, > >> + &_memcg, may_oom); > > > > This is really dangerous because atomic allocation which seem to be > > possible could result in deadlocks because of the reclaim. > > Can you elaborate on how this would happen? Say you have an atomic allocation and we hit the limit so we get either to reclaim which can sleep or to oom which can sleep as well (depending on the oom_control). > > Also, as I have mentioned in the other email in this thread. Why > > should we reclaim just because of kernel allocation when we are not > > reclaiming any of it because shrink_slab is ignored in the memcg > > reclaim. > > Don't get too distracted by the fact that shrink_slab is ignored. It is > temporary, and while this being ignored now leads to suboptimal > behavior, it will 1st, only affect its users, and 2nd, not be disastrous. It's not just about shrink_slab it is also about triggering memcg-oom which doesn't consider kmem accounted memory so the wrong tasks could be killed. It is true that the impact is packed inside the group (hierarchy) so you are right it won't be disastrous. -- Michal Hocko SUSE Labs
creator: Michal Hocko
date: 2012-08-15T13:09:52-00:00

Re: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: On Wed, Aug 15, 2012 at 01:08:08PM +0400, Glauber Costa wrote: > On 08/14/2012 07:16 PM, Mel Gorman wrote: > > On Thu, Aug 09, 2012 at 05:01:15PM +0400, Glauber Costa wrote: > >> When a process tries to allocate a page with the __GFP_KMEMCG flag, the > >> page allocator will call the corresponding memcg functions to validate > >> the allocation. Tasks in the root memcg can always proceed. > >> > >> To avoid adding markers to the page - and a kmem flag that would > >> necessarily follow, as much as doing page_cgroup lookups for no reason, > > > > As you already guessed, doing a page_cgroup in the page allocator free > > path would be a no-go. > > Specifically yes, but in general, you will be able to observe that I am > taking all the possible measures to make sure existing paths are > disturbed as little as possible. > > Thanks for your review here > > >> > >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c > >> index b956cec..da341dc 100644 > >> --- a/mm/page_alloc.c > >> +++ b/mm/page_alloc.c > >> @@ -2532,6 +2532,7 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, > >> struct page *page = NULL; > >> int migratetype = allocflags_to_migratetype(gfp_mask); > >> unsigned int cpuset_mems_cookie; > >> + void *handle = NULL; > >> > >> gfp_mask &= gfp_allowed_mask; > >> > >> @@ -2543,6 +2544,13 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, > >> return NULL; > >> > >> /* > >> + * Will only have any effect when __GFP_KMEMCG is set. > >> + * This is verified in the (always inline) callee > >> + */ > >> + if (!memcg_kmem_new_page(gfp_mask, &handle, order)) > > > > memcg_kmem_new_page takes a void * parameter already but here you are > > passing in a void **. This probably happens to work because you do this > > > > struct mem_cgroup **handle = (struct mem_cgroup **)_handle; > > > > but that appears to defeat the purpose of having an opaque type as a > > "handle". You have to treat it different then passing it into the commit > > function because it …
creator: Mel Gorman
date: 2012-08-15T13:22:44-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed 15-08-12 17:04:31, Glauber Costa wrote: > On 08/15/2012 05:02 PM, Michal Hocko wrote: > > On Wed 15-08-12 16:53:40, Glauber Costa wrote: > > [...] > >>>>> This doesn't check for the hierachy so kmem_accounted might not be in > >>>>> sync with it's parents. mem_cgroup_create (below) needs to copy > >>>>> kmem_accounted down from the parent and the above needs to check if this > >>>>> is a similar dance like mem_cgroup_oom_control_write. > >>>>> > >>>> > >>>> I don't see why we have to. > >>>> > >>>> I believe in a A/B/C hierarchy, C should be perfectly able to set a > >>>> different limit than its parents. Note that this is not a boolean. > >>> > >>> Ohh, I wasn't clear enough. I am not against setting the _limit_ I just > >>> meant that the kmem_accounted should be consistent within the hierarchy. > >>> > >> > >> If a parent of yours is accounted, you get accounted as well. This is > >> not the state in this patch, but gets added later. Isn't this enough ? > > > > But if the parent is not accounted, you can set the children to be > > accounted, right? Or maybe this is changed later in the series? I didn't > > get to the end yet. > > > > Yes, you can. Do you see any problem with that? Well, if a child contributes with the kmem charges upwards the hierachy then a parent can have kmem.usage > 0 with disabled accounting. I am not saying this is a no-go but it definitely is confusing and I do not see any good reason for it. I've considered it as an overlook rather than a deliberate design decision. -- Michal Hocko SUSE Labs
creator: Michal Hocko
date: 2012-08-15T13:26:21-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed, 2012-08-15 at 14:55 +0200, Michal Hocko wrote: > On Wed 15-08-12 12:12:23, James Bottomley wrote: > > On Wed, 2012-08-15 at 13:33 +0400, Glauber Costa wrote: > > > > This can > > > > be quite confusing. I am still not sure whether we should mix the two > > > > things together. If somebody wants to limit the kernel memory he has to > > > > touch the other limit anyway. Do you have a strong reason to mix the > > > > user and kernel counters? > > > > > > This is funny, because the first opposition I found to this work was > > > "Why would anyone want to limit it separately?" =p > > > > > > It seems that a quite common use case is to have a container with a > > > unified view of "memory" that it can use the way he likes, be it with > > > kernel memory, or user memory. I believe those people would be happy to > > > just silently account kernel memory to user memory, or at the most have > > > a switch to enable it. > > > > > > What gets clear from this back and forth, is that there are people > > > interested in both use cases. > > > > Haven't we already had this discussion during the Prague get together? > > We discussed the use cases and finally agreed to separate accounting for > > k and then k+u mem because that satisfies both the Google and Parallels > > cases. No-one was overjoyed by k and k+u but no-one had a better > > suggestion ... is there a better way of doing this that everyone can > > agree to? > > We do need to get this nailed down because it's the foundation of the > > patch series. > > There is a slot in MM/memcg minisum at KS so we have a slot to discuss > this. Sure, to get things moving, can you pre-prime us with what you're thinking in this area so we can be prepared (and if it doesn't work, tell you beforehand)? Thanks, James
creator: James Bottomley
date: 2012-08-15T13:29:57-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On 08/15/2012 05:26 PM, Michal Hocko wrote: > On Wed 15-08-12 17:04:31, Glauber Costa wrote: >> On 08/15/2012 05:02 PM, Michal Hocko wrote: >>> On Wed 15-08-12 16:53:40, Glauber Costa wrote: >>> [...] >>>>>>> This doesn't check for the hierachy so kmem_accounted might not be in >>>>>>> sync with it's parents. mem_cgroup_create (below) needs to copy >>>>>>> kmem_accounted down from the parent and the above needs to check if this >>>>>>> is a similar dance like mem_cgroup_oom_control_write. >>>>>>> >>>>>> >>>>>> I don't see why we have to. >>>>>> >>>>>> I believe in a A/B/C hierarchy, C should be perfectly able to set a >>>>>> different limit than its parents. Note that this is not a boolean. >>>>> >>>>> Ohh, I wasn't clear enough. I am not against setting the _limit_ I just >>>>> meant that the kmem_accounted should be consistent within the hierarchy. >>>>> >>>> >>>> If a parent of yours is accounted, you get accounted as well. This is >>>> not the state in this patch, but gets added later. Isn't this enough ? >>> >>> But if the parent is not accounted, you can set the children to be >>> accounted, right? Or maybe this is changed later in the series? I didn't >>> get to the end yet. >>> >> >> Yes, you can. Do you see any problem with that? > > Well, if a child contributes with the kmem charges upwards the hierachy > then a parent can have kmem.usage > 0 with disabled accounting. > I am not saying this is a no-go but it definitely is confusing and I do > not see any good reason for it. I've considered it as an overlook rather > than a deliberate design decision. > No, it is not an overlook. It is theoretically possible to skip accounting on non-limited parents, but how expensive is that? This is, indeed, confusing. Of course I can be biased, but the way I see it, once you have hierarchy, you account everything your child accounts. I really don't see what is the concern here.
creator: Glauber Costa
date: 2012-08-15T13:31:24-00:00

Re: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: >> >> As for the type, do you think using struct mem_cgroup would be less >> confusing? >> > > Yes and returning the mem_cgroup or NULL instead of bool. Ok. struct mem_cgroup it is. > >> The placeholder is there, but it is later patched >> to the final thing. >> With that explained, if you want me to change it to something else, I >> can do it. Should I ? >> > > Not in this patch anyway. I would have preferred a pattern like this but > that's about it. > > #ifdef CONFIG_MEMCG_KMEM > extern struct static_key memcg_kmem_enabled_key; > static inline int memcg_kmem_enabled(void) > { > return static_key_false(&memcg_kmem_enabled_key); > } > #else > > static inline bool memcg_kmem_enabled(void) > { > return false; > } > #endif > humm, I'll have to think about this name. "memcg_kmem_enabled" means it is enabled in this cgroup. It is actually used inside memcontrol.c to denote precisely that. Now the static branch, of course, means it is globally enabled. Or as I called here, "on". > Two reasons. One, it does not use the terms "on" and "enabled" > interchangeably. The other reason is down to taste as I'm copying the > pattern I used myself for sk_memalloc_socks(). Of course I am biased. > > Also, why is the key exported? > Same reason. The slab will now have inline functions that will test against that. The alloc functions themselves, are inside the page allocator, and the exports can go away. But the static branch will still be tested inside inlined functions in the slab. That said, for the sake of simplicity, I can make it go away here, and add that to the right place later. >>> I also find it *very* strange to have a function named as if it is an >>> allocation-style function when it in fact it's looking up a mem_cgroup >>> and charging it (and uncharging it in the error path if necessary). If >>> it was called memcg_kmem_newpage_charge I might have found it a little >>> better. >> >> I don't feel strongly about names in general. I can change it. >> Will update to memcg_…
creator: Glauber Costa
date: 2012-08-15T13:39:58-00:00

Re: [PATCH v2 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: On 08/15/2012 05:22 PM, Mel Gorman wrote: >> I believe it >> > to be a better and less complicated approach then letting a page appear >> > and then charging it. Besides being consistent with the rest of memcg, >> > it won't create unnecessary disturbance in the page allocator >> > when the allocation is to fail. >> > > I still don't get why you did not just return a mem_cgroup instead of a > handle. > Forgot this one, sorry: The reason is to keep the semantics simple. What should we return if the code is not compiled in? If we return NULL for failure, the test becomes memcg = memcg_kmem_charge_page(gfp, order); if (!memcg) exit; If we're not compiled in, we'd either return positive garbage or we need to wrap it inside an ifdef I personally believe to be a lot more clear to standardize on true to mean "allocation can proceed". the compiled out case becomes: if (!true) exit; which is easily compiled away altogether. Now of course, using struct mem_cgroup makes sense, and I have already changed that here.
creator: Glauber Costa
date: 2012-08-15T13:51:40-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On 08/15/2012 05:09 PM, Michal Hocko wrote: > On Wed 15-08-12 13:42:24, Glauber Costa wrote: > [...] >>>> + >>>> + ret = 0; >>>> + >>>> + if (!memcg) >>>> + return ret; >>>> + >>>> + _memcg = memcg; >>>> + ret = __mem_cgroup_try_charge(NULL, gfp, delta / PAGE_SIZE, >>>> + &_memcg, may_oom); >>> >>> This is really dangerous because atomic allocation which seem to be >>> possible could result in deadlocks because of the reclaim. >> >> Can you elaborate on how this would happen? > > Say you have an atomic allocation and we hit the limit so we get either > to reclaim which can sleep or to oom which can sleep as well (depending > on the oom_control). > I see now, you seem to be right. How about we change the following code in mem_cgroup_do_charge: if (gfp_mask & __GFP_NORETRY) return CHARGE_NOMEM; to: if ((gfp_mask & __GFP_NORETRY) || (gfp_mask & __GFP_ATOMIC)) return CHARGE_NOMEM; ? Would this take care of the issue ?
creator: Glauber Costa
date: 2012-08-15T14:01:51-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed 15-08-12 17:31:24, Glauber Costa wrote: > On 08/15/2012 05:26 PM, Michal Hocko wrote: > > On Wed 15-08-12 17:04:31, Glauber Costa wrote: > >> On 08/15/2012 05:02 PM, Michal Hocko wrote: > >>> On Wed 15-08-12 16:53:40, Glauber Costa wrote: > >>> [...] > >>>>>>> This doesn't check for the hierachy so kmem_accounted might not be in > >>>>>>> sync with it's parents. mem_cgroup_create (below) needs to copy > >>>>>>> kmem_accounted down from the parent and the above needs to check if this > >>>>>>> is a similar dance like mem_cgroup_oom_control_write. > >>>>>>> > >>>>>> > >>>>>> I don't see why we have to. > >>>>>> > >>>>>> I believe in a A/B/C hierarchy, C should be perfectly able to set a > >>>>>> different limit than its parents. Note that this is not a boolean. > >>>>> > >>>>> Ohh, I wasn't clear enough. I am not against setting the _limit_ I just > >>>>> meant that the kmem_accounted should be consistent within the hierarchy. > >>>>> > >>>> > >>>> If a parent of yours is accounted, you get accounted as well. This is > >>>> not the state in this patch, but gets added later. Isn't this enough ? > >>> > >>> But if the parent is not accounted, you can set the children to be > >>> accounted, right? Or maybe this is changed later in the series? I didn't > >>> get to the end yet. > >>> > >> > >> Yes, you can. Do you see any problem with that? > > > > Well, if a child contributes with the kmem charges upwards the hierachy > > then a parent can have kmem.usage > 0 with disabled accounting. > > I am not saying this is a no-go but it definitely is confusing and I do > > not see any good reason for it. I've considered it as an overlook rather > > than a deliberate design decision. > > > > No, it is not an overlook. > It is theoretically possible to skip accounting on non-limited parents, > but how expensive is that? This is, indeed, confusing. > > Of course I can be biased, but the way I see it, once you have > hierarchy, you account everything your child accounts. > > I …
creator: Michal Hocko
date: 2012-08-15T14:10:41-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: > OK, I missed an important point that kmem_accounted is not exported to > the userspace (I thought it would be done later in the series) which > is not the case so actually nobody get's confused by the inconsistency > because it is about RESOURCE_MAX which they see in both cases. > Sorry about the confusion! > I'll forgive you this time...
creator: Glauber Costa
date: 2012-08-15T14:11:41-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On Wed 15-08-12 18:01:51, Glauber Costa wrote: > On 08/15/2012 05:09 PM, Michal Hocko wrote: > > On Wed 15-08-12 13:42:24, Glauber Costa wrote: > > [...] > >>>> + > >>>> + ret = 0; > >>>> + > >>>> + if (!memcg) > >>>> + return ret; > >>>> + > >>>> + _memcg = memcg; > >>>> + ret = __mem_cgroup_try_charge(NULL, gfp, delta / PAGE_SIZE, > >>>> + &_memcg, may_oom); > >>> > >>> This is really dangerous because atomic allocation which seem to be > >>> possible could result in deadlocks because of the reclaim. > >> > >> Can you elaborate on how this would happen? > > > > Say you have an atomic allocation and we hit the limit so we get either > > to reclaim which can sleep or to oom which can sleep as well (depending > > on the oom_control). > > > > I see now, you seem to be right. No I am not because it seems that I am really blind these days... We were doing this in mem_cgroup_do_charge for ages: if (!(gfp_mask & __GFP_WAIT)) return CHARGE_WOULDBLOCK; /me goes to hide and get with further feedback with a clean head. Sorry about that. -- Michal Hocko SUSE Labs
creator: Michal Hocko
date: 2012-08-15T14:23:38-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: >> >> I see now, you seem to be right. > > No I am not because it seems that I am really blind these days... > We were doing this in mem_cgroup_do_charge for ages: > if (!(gfp_mask & __GFP_WAIT)) > return CHARGE_WOULDBLOCK; > > /me goes to hide and get with further feedback with a clean head. > > Sorry about that. > I am as well, since I went to look at mem_cgroup_do_charge() and missed that. Do you have any other concerns specific to this patch ?
creator: Glauber Costa
date: 2012-08-15T14:27:45-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed, 15 Aug 2012, Michal Hocko wrote: > > That is not what the kernel does, in general. We assume that if he wants > > that memory and we can serve it, we should. Also, not all kernel memory > > is unreclaimable. We can shrink the slabs, for instance. Ying Han > > claims she has patches for that already... > > Are those patches somewhere around? You can already shrink the reclaimable slabs (dentries / inodes) via calls to the subsystem specific shrinkers. Did Ying Han do anything to go beyond that?
creator: Christoph Lameter
date: 2012-08-15T14:47:57-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On 08/15/2012 06:47 PM, Christoph Lameter wrote: > On Wed, 15 Aug 2012, Michal Hocko wrote: > >>> That is not what the kernel does, in general. We assume that if he wants >>> that memory and we can serve it, we should. Also, not all kernel memory >>> is unreclaimable. We can shrink the slabs, for instance. Ying Han >>> claims she has patches for that already... >> >> Are those patches somewhere around? > > You can already shrink the reclaimable slabs (dentries / inodes) via > calls to the subsystem specific shrinkers. Did Ying Han do anything to > go beyond that? > That is not enough for us. We would like to make sure that the objects being discarded belong to the memcg which is under pressure. We don't need to be perfect here, and an occasional slip is totally fine. But if in general, shrinking from memcg A will mostly wipe out objects from memcg B, we harmed the system in return for nothing good.
creator: Glauber Costa
date: 2012-08-15T15:11:49-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed, Aug 15 2012, Christoph Lameter wrote: > On Wed, 15 Aug 2012, Michal Hocko wrote: > >> > That is not what the kernel does, in general. We assume that if he wants >> > that memory and we can serve it, we should. Also, not all kernel memory >> > is unreclaimable. We can shrink the slabs, for instance. Ying Han >> > claims she has patches for that already... >> >> Are those patches somewhere around? > > You can already shrink the reclaimable slabs (dentries / inodes) via > calls to the subsystem specific shrinkers. Did Ying Han do anything to > go beyond that? cc: Ying The Google shrinker patches enhance prune_dcache_sb() to limit dentry pressure to a specific memcg.
creator: Greg Thelen
date: 2012-08-15T15:19:58-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed, 15 Aug 2012, Glauber Costa wrote: > On 08/15/2012 06:47 PM, Christoph Lameter wrote: > > On Wed, 15 Aug 2012, Michal Hocko wrote: > > > >>> That is not what the kernel does, in general. We assume that if he wants > >>> that memory and we can serve it, we should. Also, not all kernel memory > >>> is unreclaimable. We can shrink the slabs, for instance. Ying Han > >>> claims she has patches for that already... > >> > >> Are those patches somewhere around? > > > > You can already shrink the reclaimable slabs (dentries / inodes) via > > calls to the subsystem specific shrinkers. Did Ying Han do anything to > > go beyond that? > > > That is not enough for us. > We would like to make sure that the objects being discarded belong to > the memcg which is under pressure. We don't need to be perfect here, and > an occasional slip is totally fine. But if in general, shrinking from > memcg A will mostly wipe out objects from memcg B, we harmed the system > in return for nothing good. How can you figure out which objects belong to which memcg? The ownerships of dentries and inodes is a dubious concept already.
creator: Christoph Lameter
date: 2012-08-15T15:34:39-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On 08/15/2012 07:34 PM, Christoph Lameter wrote: > On Wed, 15 Aug 2012, Glauber Costa wrote: > >> On 08/15/2012 06:47 PM, Christoph Lameter wrote: >>> On Wed, 15 Aug 2012, Michal Hocko wrote: >>> >>>>> That is not what the kernel does, in general. We assume that if he wants >>>>> that memory and we can serve it, we should. Also, not all kernel memory >>>>> is unreclaimable. We can shrink the slabs, for instance. Ying Han >>>>> claims she has patches for that already... >>>> >>>> Are those patches somewhere around? >>> >>> You can already shrink the reclaimable slabs (dentries / inodes) via >>> calls to the subsystem specific shrinkers. Did Ying Han do anything to >>> go beyond that? >>> >> That is not enough for us. >> We would like to make sure that the objects being discarded belong to >> the memcg which is under pressure. We don't need to be perfect here, and >> an occasional slip is totally fine. But if in general, shrinking from >> memcg A will mostly wipe out objects from memcg B, we harmed the system >> in return for nothing good. > > How can you figure out which objects belong to which memcg? The ownerships > of dentries and inodes is a dubious concept already. > Remember we copy over the metadata and create copies of the caches per-memcg. Therefore, a dentry belongs to a memcg if it was allocated from the slab pertaining to that memcg. It is not 100 % accurate, but it is good enough.
creator: Glauber Costa
date: 2012-08-15T15:35:13-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed, 15 Aug 2012, Greg Thelen wrote: > > You can already shrink the reclaimable slabs (dentries / inodes) via > > calls to the subsystem specific shrinkers. Did Ying Han do anything to > > go beyond that? > > cc: Ying > > The Google shrinker patches enhance prune_dcache_sb() to limit dentry > pressure to a specific memcg. Ok then its restricted to the reclaimable slab caches already. The main issue to sort out then is who is the "owner" of an inode/dentry (if something like that exists). If you separate the objects into different pages then the objects may be cleanly separated at the price of more memory use.
creator: Christoph Lameter
date: 2012-08-15T15:36:28-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On Wed, Aug 15 2012, Glauber Costa wrote: > On 08/14/2012 10:58 PM, Greg Thelen wrote: >> On Mon, Aug 13 2012, Glauber Costa wrote: >> >>>>>> + WARN_ON(mem_cgroup_is_root(memcg)); >>>>>> + size = (1 << order) << PAGE_SHIFT; >>>>>> + memcg_uncharge_kmem(memcg, size); >>>>>> + mem_cgroup_put(memcg); >>>> Why do we need ref-counting here ? kmem res_counter cannot work as >>>> reference ? >>> This is of course the pair of the mem_cgroup_get() you commented on >>> earlier. If we need one, we need the other. If we don't need one, we >>> don't need the other =) >>> >>> The guarantee we're trying to give here is that the memcg structure will >>> stay around while there are dangling charges to kmem, that we decided >>> not to move (remember: moving it for the stack is simple, for the slab >>> is very complicated and ill-defined, and I believe it is better to treat >>> all kmem equally here) >> >> By keeping memcg structures hanging around until the last referring kmem >> page is uncharged do such zombie memcg each consume a css_id and thus >> put pressure on the 64k css_id space? I imagine in pathological cases >> this would prevent creation of new cgroups until these zombies are >> dereferenced. > > Yes, but although this patch makes it more likely, it doesn't introduce > that. If the tasks, for instance, grab a reference to the cgroup dentry > in the filesystem (like their CWD, etc), they will also keep the cgroup > around. Fair point. But this doesn't seems like a feature. It's probably not needed initially, but what do you think about creating a memcg_kernel_context structure which is allocated when memcg is allocated? Kernel pages charged to a memcg would have page_cgroup->mem_cgroup=memcg_kernel_context rather than memcg. This would allow the mem_cgroup and its css_id to be deleted when the cgroup is unlinked from cgroupfs while allowing for the active kernel pages to continue pointing to a valid memcg_kernel_context. This would be a reference counted structure much li…
creator: Greg Thelen
date: 2012-08-15T16:38:57-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On 08/15/2012 08:38 PM, Greg Thelen wrote: > On Wed, Aug 15 2012, Glauber Costa wrote: > >> On 08/14/2012 10:58 PM, Greg Thelen wrote: >>> On Mon, Aug 13 2012, Glauber Costa wrote: >>> >>>>>>> + WARN_ON(mem_cgroup_is_root(memcg)); >>>>>>> + size = (1 << order) << PAGE_SHIFT; >>>>>>> + memcg_uncharge_kmem(memcg, size); >>>>>>> + mem_cgroup_put(memcg); >>>>> Why do we need ref-counting here ? kmem res_counter cannot work as >>>>> reference ? >>>> This is of course the pair of the mem_cgroup_get() you commented on >>>> earlier. If we need one, we need the other. If we don't need one, we >>>> don't need the other =) >>>> >>>> The guarantee we're trying to give here is that the memcg structure will >>>> stay around while there are dangling charges to kmem, that we decided >>>> not to move (remember: moving it for the stack is simple, for the slab >>>> is very complicated and ill-defined, and I believe it is better to treat >>>> all kmem equally here) >>> >>> By keeping memcg structures hanging around until the last referring kmem >>> page is uncharged do such zombie memcg each consume a css_id and thus >>> put pressure on the 64k css_id space? I imagine in pathological cases >>> this would prevent creation of new cgroups until these zombies are >>> dereferenced. >> >> Yes, but although this patch makes it more likely, it doesn't introduce >> that. If the tasks, for instance, grab a reference to the cgroup dentry >> in the filesystem (like their CWD, etc), they will also keep the cgroup >> around. > > Fair point. But this doesn't seems like a feature. It's probably not > needed initially, but what do you think about creating a > memcg_kernel_context structure which is allocated when memcg is > allocated? Kernel pages charged to a memcg would have > page_cgroup->mem_cgroup=memcg_kernel_context rather than memcg. This > would allow the mem_cgroup and its css_id to be deleted when the cgroup > is unlinked from cgroupfs while allowing for the active kernel pages to > continue…
creator: Glauber Costa
date: 2012-08-15T17:00:31-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On Wed, Aug 15 2012, Glauber Costa wrote: > On 08/15/2012 08:38 PM, Greg Thelen wrote: >> On Wed, Aug 15 2012, Glauber Costa wrote: >> >>> On 08/14/2012 10:58 PM, Greg Thelen wrote: >>>> On Mon, Aug 13 2012, Glauber Costa wrote: >>>> >>>>>>>> + WARN_ON(mem_cgroup_is_root(memcg)); >>>>>>>> + size = (1 << order) << PAGE_SHIFT; >>>>>>>> + memcg_uncharge_kmem(memcg, size); >>>>>>>> + mem_cgroup_put(memcg); >>>>>> Why do we need ref-counting here ? kmem res_counter cannot work as >>>>>> reference ? >>>>> This is of course the pair of the mem_cgroup_get() you commented on >>>>> earlier. If we need one, we need the other. If we don't need one, we >>>>> don't need the other =) >>>>> >>>>> The guarantee we're trying to give here is that the memcg structure will >>>>> stay around while there are dangling charges to kmem, that we decided >>>>> not to move (remember: moving it for the stack is simple, for the slab >>>>> is very complicated and ill-defined, and I believe it is better to treat >>>>> all kmem equally here) >>>> >>>> By keeping memcg structures hanging around until the last referring kmem >>>> page is uncharged do such zombie memcg each consume a css_id and thus >>>> put pressure on the 64k css_id space? I imagine in pathological cases >>>> this would prevent creation of new cgroups until these zombies are >>>> dereferenced. >>> >>> Yes, but although this patch makes it more likely, it doesn't introduce >>> that. If the tasks, for instance, grab a reference to the cgroup dentry >>> in the filesystem (like their CWD, etc), they will also keep the cgroup >>> around. >> >> Fair point. But this doesn't seems like a feature. It's probably not >> needed initially, but what do you think about creating a >> memcg_kernel_context structure which is allocated when memcg is >> allocated? Kernel pages charged to a memcg would have >> page_cgroup->mem_cgroup=memcg_kernel_context rather than memcg. This >> would allow the mem_cgroup and its css_id to be deleted when the cgroup >>…
creator: Greg Thelen
date: 2012-08-15T17:12:21-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed, 15 Aug 2012, Glauber Costa wrote: > Remember we copy over the metadata and create copies of the caches > per-memcg. Therefore, a dentry belongs to a memcg if it was allocated > from the slab pertaining to that memcg. The dentry could be used by other processes in the system though. F.e. directory names could easily be created by one process and then used by a multitude of others. > It is not 100 % accurate, but it is good enough. Lets hope that is true.
creator: Christoph Lameter
date: 2012-08-15T17:26:25-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On 08/15/2012 10:01 PM, Ying Han wrote: > On Wed, Aug 15, 2012 at 5:39 AM, Michal Hocko <mhocko@suse.cz> wrote: >> On Wed 15-08-12 13:33:55, Glauber Costa wrote: >> [...] >>>> This can >>>> be quite confusing. I am still not sure whether we should mix the two >>>> things together. If somebody wants to limit the kernel memory he has to >>>> touch the other limit anyway. Do you have a strong reason to mix the >>>> user and kernel counters? >>> >>> This is funny, because the first opposition I found to this work was >>> "Why would anyone want to limit it separately?" =p >>> >>> It seems that a quite common use case is to have a container with a >>> unified view of "memory" that it can use the way he likes, be it with >>> kernel memory, or user memory. I believe those people would be happy to >>> just silently account kernel memory to user memory, or at the most have >>> a switch to enable it. >>> >>> What gets clear from this back and forth, is that there are people >>> interested in both use cases. >> >> I am still not 100% sure myself. It is just clear that the reclaim would >> need some work in order to do accounting like this. >> >>>> My impression was that kernel allocation should simply fail while user >>>> allocations might reclaim as well. Why should we reclaim just because of >>>> the kernel allocation (which is unreclaimable from hard limit reclaim >>>> point of view)? >>> >>> That is not what the kernel does, in general. We assume that if he wants >>> that memory and we can serve it, we should. Also, not all kernel memory >>> is unreclaimable. We can shrink the slabs, for instance. Ying Han >>> claims she has patches for that already... >> >> Are those patches somewhere around? > > Yes, I am working on it to post it sometime *this week*. My last > rebase is based on v3.3 and now I am trying to get it rebased to > github-memcg. The patch itself has a functional dependency on kernel > slab accounting, and I am trying to get that rebased on Glauber's tree > but…
creator: Glauber Costa
date: 2012-08-15T18:00:43-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed, Aug 15, 2012 at 5:39 AM, Michal Hocko <mhocko@suse.cz> wrote: > On Wed 15-08-12 13:33:55, Glauber Costa wrote: > [...] >> > This can >> > be quite confusing. I am still not sure whether we should mix the two >> > things together. If somebody wants to limit the kernel memory he has to >> > touch the other limit anyway. Do you have a strong reason to mix the >> > user and kernel counters? >> >> This is funny, because the first opposition I found to this work was >> "Why would anyone want to limit it separately?" =p >> >> It seems that a quite common use case is to have a container with a >> unified view of "memory" that it can use the way he likes, be it with >> kernel memory, or user memory. I believe those people would be happy to >> just silently account kernel memory to user memory, or at the most have >> a switch to enable it. >> >> What gets clear from this back and forth, is that there are people >> interested in both use cases. > > I am still not 100% sure myself. It is just clear that the reclaim would > need some work in order to do accounting like this. > >> > My impression was that kernel allocation should simply fail while user >> > allocations might reclaim as well. Why should we reclaim just because of >> > the kernel allocation (which is unreclaimable from hard limit reclaim >> > point of view)? >> >> That is not what the kernel does, in general. We assume that if he wants >> that memory and we can serve it, we should. Also, not all kernel memory >> is unreclaimable. We can shrink the slabs, for instance. Ying Han >> claims she has patches for that already... > > Are those patches somewhere around? Yes, I am working on it to post it sometime *this week*. My last rebase is based on v3.3 and now I am trying to get it rebased to github-memcg. The patch itself has a functional dependency on kernel slab accounting, and I am trying to get that rebased on Glauber's tree but has some difficulty now. What I am planning to do is post the RFC w/ only comp…
creator: Ying Han
date: 2012-08-15T18:01:41-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed, Aug 15, 2012 at 8:11 AM, Glauber Costa <glommer@parallels.com> wrote: > On 08/15/2012 06:47 PM, Christoph Lameter wrote: >> On Wed, 15 Aug 2012, Michal Hocko wrote: >> >>>> That is not what the kernel does, in general. We assume that if he wants >>>> that memory and we can serve it, we should. Also, not all kernel memory >>>> is unreclaimable. We can shrink the slabs, for instance. Ying Han >>>> claims she has patches for that already... >>> >>> Are those patches somewhere around? >> >> You can already shrink the reclaimable slabs (dentries / inodes) via >> calls to the subsystem specific shrinkers. Did Ying Han do anything to >> go beyond that? >> > That is not enough for us. > We would like to make sure that the objects being discarded belong to > the memcg which is under pressure. We don't need to be perfect here, and > an occasional slip is totally fine. But if in general, shrinking from > memcg A will mostly wipe out objects from memcg B, we harmed the system > in return for nothing good. Correct. For example, we have per-superblock shrinker today for vfs caches. That is not enough since we need to isolate the dentry caches per-memcg basis. --Ying > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org
creator: Ying Han
date: 2012-08-15T18:07:09-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed, Aug 15, 2012 at 8:34 AM, Christoph Lameter <cl@linux.com> wrote: > On Wed, 15 Aug 2012, Glauber Costa wrote: > >> On 08/15/2012 06:47 PM, Christoph Lameter wrote: >> > On Wed, 15 Aug 2012, Michal Hocko wrote: >> > >> >>> That is not what the kernel does, in general. We assume that if he wants >> >>> that memory and we can serve it, we should. Also, not all kernel memory >> >>> is unreclaimable. We can shrink the slabs, for instance. Ying Han >> >>> claims she has patches for that already... >> >> >> >> Are those patches somewhere around? >> > >> > You can already shrink the reclaimable slabs (dentries / inodes) via >> > calls to the subsystem specific shrinkers. Did Ying Han do anything to >> > go beyond that? >> > >> That is not enough for us. >> We would like to make sure that the objects being discarded belong to >> the memcg which is under pressure. We don't need to be perfect here, and >> an occasional slip is totally fine. But if in general, shrinking from >> memcg A will mostly wipe out objects from memcg B, we harmed the system >> in return for nothing good. > > How can you figure out which objects belong to which memcg? The ownerships > of dentries and inodes is a dubious concept already. I figured it out based on the kernel slab accounting. obj->page->kmem_cache->memcg --Ying > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org
creator: Ying Han
date: 2012-08-15T18:11:49-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed, 15 Aug 2012, Ying Han wrote: > > How can you figure out which objects belong to which memcg? The ownerships > > of dentries and inodes is a dubious concept already. > > I figured it out based on the kernel slab accounting. > obj->page->kmem_cache->memcg Well that is only the memcg which allocated it. It may be in use heavily by other processes.
creator: Christoph Lameter
date: 2012-08-15T18:25:04-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On 08/15/2012 10:25 PM, Christoph Lameter wrote: > On Wed, 15 Aug 2012, Ying Han wrote: > >>> How can you figure out which objects belong to which memcg? The ownerships >>> of dentries and inodes is a dubious concept already. >> >> I figured it out based on the kernel slab accounting. >> obj->page->kmem_cache->memcg > > Well that is only the memcg which allocated it. It may be in use heavily > by other processes. > Yes, but a lot of the use cases for cgroups/containers are pretty local. That is why we have been able to get away with a first-touch mechanism even in user pages memcg. In those cases - which we expect to be the majority of them - this will perform well. Now, this is not of course representative of the whole range of possible use cases, and others are valid. There are people like Greg and Ying Han herself that want a more fine grained control on which memcg gets the accounting. That is one of the topics for the summit. But even then: regardless of what mechanism is in place, one cgroup is to be accounted (or not accounted at all, meaning it belongs to a non-accounted cgroup). And then we can just grab whichever memcg it was allocated from and shrink it.
creator: Glauber Costa
date: 2012-08-15T19:22:34-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On 08/15/2012 09:12 PM, Greg Thelen wrote: > On Wed, Aug 15 2012, Glauber Costa wrote: > >> On 08/15/2012 08:38 PM, Greg Thelen wrote: >>> On Wed, Aug 15 2012, Glauber Costa wrote: >>> >>>> On 08/14/2012 10:58 PM, Greg Thelen wrote: >>>>> On Mon, Aug 13 2012, Glauber Costa wrote: >>>>> >>>>>>>>> + WARN_ON(mem_cgroup_is_root(memcg)); >>>>>>>>> + size = (1 << order) << PAGE_SHIFT; >>>>>>>>> + memcg_uncharge_kmem(memcg, size); >>>>>>>>> + mem_cgroup_put(memcg); >>>>>>> Why do we need ref-counting here ? kmem res_counter cannot work as >>>>>>> reference ? >>>>>> This is of course the pair of the mem_cgroup_get() you commented on >>>>>> earlier. If we need one, we need the other. If we don't need one, we >>>>>> don't need the other =) >>>>>> >>>>>> The guarantee we're trying to give here is that the memcg structure will >>>>>> stay around while there are dangling charges to kmem, that we decided >>>>>> not to move (remember: moving it for the stack is simple, for the slab >>>>>> is very complicated and ill-defined, and I believe it is better to treat >>>>>> all kmem equally here) >>>>> >>>>> By keeping memcg structures hanging around until the last referring kmem >>>>> page is uncharged do such zombie memcg each consume a css_id and thus >>>>> put pressure on the 64k css_id space? I imagine in pathological cases >>>>> this would prevent creation of new cgroups until these zombies are >>>>> dereferenced. >>>> >>>> Yes, but although this patch makes it more likely, it doesn't introduce >>>> that. If the tasks, for instance, grab a reference to the cgroup dentry >>>> in the filesystem (like their CWD, etc), they will also keep the cgroup >>>> around. >>> >>> Fair point. But this doesn't seems like a feature. It's probably not >>> needed initially, but what do you think about creating a >>> memcg_kernel_context structure which is allocated when memcg is >>> allocated? Kernel pages charged to a memcg would have >>> page_cgroup->mem_cgroup=memcg_kernel_context rather than memc…
creator: Glauber Costa
date: 2012-08-15T19:31:34-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Tue, Aug 14, 2012 at 9:21 AM, Michal Hocko <mhocko@suse.cz> wrote: > On Thu 09-08-12 17:01:12, Glauber Costa wrote: >> This patch adds the basic infrastructure for the accounting of the slab >> caches. To control that, the following files are created: >> >> * memory.kmem.usage_in_bytes >> * memory.kmem.limit_in_bytes >> * memory.kmem.failcnt >> * memory.kmem.max_usage_in_bytes >> >> They have the same meaning of their user memory counterparts. They >> reflect the state of the "kmem" res_counter. >> >> The code is not enabled until a limit is set. This can be tested by the >> flag "kmem_accounted". This means that after the patch is applied, no >> behavioral changes exists for whoever is still using memcg to control >> their memory usage. >> >> We always account to both user and kernel resource_counters. This >> effectively means that an independent kernel limit is in place when the >> limit is set to a lower value than the user memory. A equal or higher >> value means that the user limit will always hit first, meaning that kmem >> is effectively unlimited. > > Well, it contributes to the user limit so it is not unlimited. It just > falls under a different limit and it tends to contribute less. This can > be quite confusing. I am still not sure whether we should mix the two > things together. If somebody wants to limit the kernel memory he has to > touch the other limit anyway. Do you have a strong reason to mix the > user and kernel counters? The reason to mix the two together is a compromise of the two use cases we've heard by far. In google, we only need one limit which limits u & k, and the reclaim kicks in when the total usage hits the limit. > My impression was that kernel allocation should simply fail while user > allocations might reclaim as well. Why should we reclaim just because of > the kernel allocation (which is unreclaimable from hard limit reclaim > point of view)? Some of kernel objects are reclaimable if we have per-memcg shrinker. > I also think…
creator: Ying Han
date: 2012-08-15T19:50:55-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On Wed, Aug 15 2012, Glauber Costa wrote: > On 08/15/2012 09:12 PM, Greg Thelen wrote: >> On Wed, Aug 15 2012, Glauber Costa wrote: >> >>> On 08/15/2012 08:38 PM, Greg Thelen wrote: >>>> On Wed, Aug 15 2012, Glauber Costa wrote: >>>> >>>>> On 08/14/2012 10:58 PM, Greg Thelen wrote: >>>>>> On Mon, Aug 13 2012, Glauber Costa wrote: >>>>>> >>>>>>>>>> + WARN_ON(mem_cgroup_is_root(memcg)); >>>>>>>>>> + size = (1 << order) << PAGE_SHIFT; >>>>>>>>>> + memcg_uncharge_kmem(memcg, size); >>>>>>>>>> + mem_cgroup_put(memcg); >>>>>>>> Why do we need ref-counting here ? kmem res_counter cannot work as >>>>>>>> reference ? >>>>>>> This is of course the pair of the mem_cgroup_get() you commented on >>>>>>> earlier. If we need one, we need the other. If we don't need one, we >>>>>>> don't need the other =) >>>>>>> >>>>>>> The guarantee we're trying to give here is that the memcg structure will >>>>>>> stay around while there are dangling charges to kmem, that we decided >>>>>>> not to move (remember: moving it for the stack is simple, for the slab >>>>>>> is very complicated and ill-defined, and I believe it is better to treat >>>>>>> all kmem equally here) >>>>>> >>>>>> By keeping memcg structures hanging around until the last referring kmem >>>>>> page is uncharged do such zombie memcg each consume a css_id and thus >>>>>> put pressure on the 64k css_id space? I imagine in pathological cases >>>>>> this would prevent creation of new cgroups until these zombies are >>>>>> dereferenced. >>>>> >>>>> Yes, but although this patch makes it more likely, it doesn't introduce >>>>> that. If the tasks, for instance, grab a reference to the cgroup dentry >>>>> in the filesystem (like their CWD, etc), they will also keep the cgroup >>>>> around. >>>> >>>> Fair point. But this doesn't seems like a feature. It's probably not >>>> needed initially, but what do you think about creating a >>>> memcg_kernel_context structure which is allocated when memcg is >>>> allocated? Kernel pages charged to a…
creator: Greg Thelen
date: 2012-08-16T03:37:01-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On 08/16/2012 07:37 AM, Greg Thelen wrote: > On Wed, Aug 15 2012, Glauber Costa wrote: > >> On 08/15/2012 09:12 PM, Greg Thelen wrote: >>> On Wed, Aug 15 2012, Glauber Costa wrote: >>> >>>> On 08/15/2012 08:38 PM, Greg Thelen wrote: >>>>> On Wed, Aug 15 2012, Glauber Costa wrote: >>>>> >>>>>> On 08/14/2012 10:58 PM, Greg Thelen wrote: >>>>>>> On Mon, Aug 13 2012, Glauber Costa wrote: >>>>>>> >>>>>>>>>>> + WARN_ON(mem_cgroup_is_root(memcg)); >>>>>>>>>>> + size = (1 << order) << PAGE_SHIFT; >>>>>>>>>>> + memcg_uncharge_kmem(memcg, size); >>>>>>>>>>> + mem_cgroup_put(memcg); >>>>>>>>> Why do we need ref-counting here ? kmem res_counter cannot work as >>>>>>>>> reference ? >>>>>>>> This is of course the pair of the mem_cgroup_get() you commented on >>>>>>>> earlier. If we need one, we need the other. If we don't need one, we >>>>>>>> don't need the other =) >>>>>>>> >>>>>>>> The guarantee we're trying to give here is that the memcg structure will >>>>>>>> stay around while there are dangling charges to kmem, that we decided >>>>>>>> not to move (remember: moving it for the stack is simple, for the slab >>>>>>>> is very complicated and ill-defined, and I believe it is better to treat >>>>>>>> all kmem equally here) >>>>>>> >>>>>>> By keeping memcg structures hanging around until the last referring kmem >>>>>>> page is uncharged do such zombie memcg each consume a css_id and thus >>>>>>> put pressure on the 64k css_id space? I imagine in pathological cases >>>>>>> this would prevent creation of new cgroups until these zombies are >>>>>>> dereferenced. >>>>>> >>>>>> Yes, but although this patch makes it more likely, it doesn't introduce >>>>>> that. If the tasks, for instance, grab a reference to the cgroup dentry >>>>>> in the filesystem (like their CWD, etc), they will also keep the cgroup >>>>>> around. >>>>> >>>>> Fair point. But this doesn't seems like a feature. It's probably not >>>>> needed initially, but what do you think about creating a >>>>> memcg_kernel_contex…
creator: Glauber Costa
date: 2012-08-16T07:47:43-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On Wed 15-08-12 18:27:45, Glauber Costa wrote: > > >> > >> I see now, you seem to be right. > > > > No I am not because it seems that I am really blind these days... > > We were doing this in mem_cgroup_do_charge for ages: > > if (!(gfp_mask & __GFP_WAIT)) > > return CHARGE_WOULDBLOCK; > > > > /me goes to hide and get with further feedback with a clean head. > > > > Sorry about that. > > > I am as well, since I went to look at mem_cgroup_do_charge() and missed > that. I thought we are not doing atomic allocations in user pages accounting but I was obviously wrong because at least shmem uses atomic allocations for ages. > Do you have any other concerns specific to this patch ? I understood you changed also handle thingy. So the patch should be correct. Do you plan to send an updated version? -- Michal Hocko SUSE Labs
creator: Michal Hocko
date: 2012-08-16T09:53:09-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On 08/16/2012 01:53 PM, Michal Hocko wrote: > On Wed 15-08-12 18:27:45, Glauber Costa wrote: >> >>>> >>>> I see now, you seem to be right. >>> >>> No I am not because it seems that I am really blind these days... >>> We were doing this in mem_cgroup_do_charge for ages: >>> if (!(gfp_mask & __GFP_WAIT)) >>> return CHARGE_WOULDBLOCK; >>> >>> /me goes to hide and get with further feedback with a clean head. >>> >>> Sorry about that. >>> >> I am as well, since I went to look at mem_cgroup_do_charge() and missed >> that. > > I thought we are not doing atomic allocations in user pages accounting > but I was obviously wrong because at least shmem uses atomic > allocations for ages. > >> Do you have any other concerns specific to this patch ? > > I understood you changed also handle thingy. So the patch should be > correct. > Do you plan to send an updated version? > That depends more on you than on me! =) Do you still have any concerns regarding the u+k charging as it stands now? That would be the last big concern I heard during this iteration. If you are happy with the answers you got so far, and believe it is acceptable to proceed with the charging this way, I will be ready to send an updated version soon.
creator: Glauber Costa
date: 2012-08-16T09:57:07-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On Thu 16-08-12 13:57:07, Glauber Costa wrote: > On 08/16/2012 01:53 PM, Michal Hocko wrote: > > On Wed 15-08-12 18:27:45, Glauber Costa wrote: > >> > >>>> > >>>> I see now, you seem to be right. > >>> > >>> No I am not because it seems that I am really blind these days... > >>> We were doing this in mem_cgroup_do_charge for ages: > >>> if (!(gfp_mask & __GFP_WAIT)) > >>> return CHARGE_WOULDBLOCK; > >>> > >>> /me goes to hide and get with further feedback with a clean head. > >>> > >>> Sorry about that. > >>> > >> I am as well, since I went to look at mem_cgroup_do_charge() and missed > >> that. > > > > I thought we are not doing atomic allocations in user pages accounting > > but I was obviously wrong because at least shmem uses atomic > > allocations for ages. > > > >> Do you have any other concerns specific to this patch ? > > > > I understood you changed also handle thingy. So the patch should be > > correct. > > Do you plan to send an updated version? > > > That depends more on you than on me! =) > > Do you still have any concerns regarding the u+k charging as it stands > now? That would be the last big concern I heard during this iteration. Well, I am still not 100% sure because I still see technical difficulties that are not addressed by the patchset (memcg-oom, memcg slab shrinking, possibly others). More importantly this is changing the current semantic of the limit so we should better be careful about it and check that we are not making the code tight to specific workloads without a way out. On the other hand I do not want to block the progress here without having _really_ good arguments against that couldn't be handled later (and it seems that some of my concerns are work in progress already). I have to admit I like several things about the patchset. Especially the way how it enables easy-to-setup (aka don't care about kmem details just make sure you can cap the thing) as well as "I know exactly what I want to do" usecases. It is also good nice that only …
creator: Michal Hocko
date: 2012-08-16T15:05:18-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: On 08/16/2012 07:05 PM, Michal Hocko wrote: > On Thu 16-08-12 13:57:07, Glauber Costa wrote: >> On 08/16/2012 01:53 PM, Michal Hocko wrote: >>> On Wed 15-08-12 18:27:45, Glauber Costa wrote: >>>> >>>>>> >>>>>> I see now, you seem to be right. >>>>> >>>>> No I am not because it seems that I am really blind these days... >>>>> We were doing this in mem_cgroup_do_charge for ages: >>>>> if (!(gfp_mask & __GFP_WAIT)) >>>>> return CHARGE_WOULDBLOCK; >>>>> >>>>> /me goes to hide and get with further feedback with a clean head. >>>>> >>>>> Sorry about that. >>>>> >>>> I am as well, since I went to look at mem_cgroup_do_charge() and missed >>>> that. >>> >>> I thought we are not doing atomic allocations in user pages accounting >>> but I was obviously wrong because at least shmem uses atomic >>> allocations for ages. >>> >>>> Do you have any other concerns specific to this patch ? >>> >>> I understood you changed also handle thingy. So the patch should be >>> correct. >>> Do you plan to send an updated version? >>> >> That depends more on you than on me! =) >> >> Do you still have any concerns regarding the u+k charging as it stands >> now? That would be the last big concern I heard during this iteration. > > Well, I am still not 100% sure because I still see technical > difficulties that are not addressed by the patchset (memcg-oom, memcg > slab shrinking, possibly others). More importantly this is changing the > current semantic of the limit so we should better be careful about it > and check that we are not making the code tight to specific workloads > without a way out. > > On the other hand I do not want to block the progress here without > having _really_ good arguments against that couldn't be handled later > (and it seems that some of my concerns are work in progress already). > > I have to admit I like several things about the patchset. Especially the > way how it enables easy-to-setup (aka don't care about kmem details just > make sure you can cap the thing) as wel…
creator: Glauber Costa
date: 2012-08-16T15:22:45-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Wed 15-08-12 12:50:55, Ying Han wrote: > On Tue, Aug 14, 2012 at 9:21 AM, Michal Hocko <mhocko@suse.cz> wrote: > > On Thu 09-08-12 17:01:12, Glauber Costa wrote: > >> This patch adds the basic infrastructure for the accounting of the slab > >> caches. To control that, the following files are created: > >> > >> * memory.kmem.usage_in_bytes > >> * memory.kmem.limit_in_bytes > >> * memory.kmem.failcnt > >> * memory.kmem.max_usage_in_bytes > >> > >> They have the same meaning of their user memory counterparts. They > >> reflect the state of the "kmem" res_counter. > >> > >> The code is not enabled until a limit is set. This can be tested by the > >> flag "kmem_accounted". This means that after the patch is applied, no > >> behavioral changes exists for whoever is still using memcg to control > >> their memory usage. > >> > >> We always account to both user and kernel resource_counters. This > >> effectively means that an independent kernel limit is in place when the > >> limit is set to a lower value than the user memory. A equal or higher > >> value means that the user limit will always hit first, meaning that kmem > >> is effectively unlimited. > > > > Well, it contributes to the user limit so it is not unlimited. It just > > falls under a different limit and it tends to contribute less. This can > > be quite confusing. I am still not sure whether we should mix the two > > things together. If somebody wants to limit the kernel memory he has to > > touch the other limit anyway. Do you have a strong reason to mix the > > user and kernel counters? > > The reason to mix the two together is a compromise of the two use > cases we've heard by far. In google, we only need one limit which > limits u & k, and the reclaim kicks in when the total usage hits the > limit. > > > My impression was that kernel allocation should simply fail while user > > allocations might reclaim as well. Why should we reclaim just because of > > the kernel allocation (which is unreclaimable from h…
creator: Michal Hocko
date: 2012-08-16T15:25:44-00:00

Re: [PATCH v2 06/11] memcg: kmem controller infrastructure
type: item
description: (2012/08/13 17:28), Glauber Costa wrote: >>>> + * Needs to be called after memcg_kmem_new_page, regardless of success or >>>> + * failure of the allocation. if @page is NULL, this function will revert the >>>> + * charges. Otherwise, it will commit the memcg given by @handle to the >>>> + * corresponding page_cgroup. >>>> + */ >>>> +static __always_inline void >>>> +memcg_kmem_commit_page(struct page *page, struct mem_cgroup *handle, int order) >>>> +{ >>>> + if (memcg_kmem_on) >>>> + __memcg_kmem_commit_page(page, handle, order); >>>> +} >> Doesn't this 2 functions has no short-cuts ? > > Sorry kame, what exactly do you mean? > I meant avoinding function call. But please ignore, I missed following patches. >> if (memcg_kmem_on && handle) ? > I guess this can be done to avoid a function call. > >> Maybe free() needs to access page_cgroup... >> > Can you also be a bit more specific here? > Please ignore, I misunderstood the usage of free_accounted_pages(). >>>> +bool __memcg_kmem_new_page(gfp_t gfp, void *_handle, int order) >>>> +{ >>>> + struct mem_cgroup *memcg; >>>> + struct mem_cgroup **handle = (struct mem_cgroup **)_handle; >>>> + bool ret = true; >>>> + size_t size; >>>> + struct task_struct *p; >>>> + >>>> + *handle = NULL; >>>> + rcu_read_lock(); >>>> + p = rcu_dereference(current->mm->owner); >>>> + memcg = mem_cgroup_from_task(p); >>>> + if (!memcg_kmem_enabled(memcg)) >>>> + goto out; >>>> + >>>> + mem_cgroup_get(memcg); >>>> + >> This mem_cgroup_get() will be a potentioal performance problem. >> Don't you have good idea to avoid accessing atomic counter here ? >> I think some kind of percpu counter or a feature to disable "move task" >> will be a help. > > > > >>>> + pc = lookup_page_cgroup(page); >>>> + lock_page_cgroup(pc); >>>> + pc->mem_cgroup = memcg; >>>> + SetPageCgroupUsed(pc); >>>> + unlock_page_cgroup(pc); >>>> +} >>>> + >>>> +void __memcg_kmem_free_page(struct page *page, int order) >>>> +{ >>>> + struct mem_cgroup *memcg; >>>> + size_t size;…
creator: KAMEZAWA Hiroyuki
date: 2012-08-17T02:36:26-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: (2012/08/13 17:36), Glauber Costa wrote: > On 08/10/2012 09:02 PM, Kamezawa Hiroyuki wrote: >> (2012/08/09 22:01), Glauber Costa wrote: >>> This patch adds the basic infrastructure for the accounting of the slab >>> caches. To control that, the following files are created: >>> >>> * memory.kmem.usage_in_bytes >>> * memory.kmem.limit_in_bytes >>> * memory.kmem.failcnt >>> * memory.kmem.max_usage_in_bytes >>> >>> They have the same meaning of their user memory counterparts. They >>> reflect the state of the "kmem" res_counter. >>> >>> The code is not enabled until a limit is set. This can be tested by the >>> flag "kmem_accounted". This means that after the patch is applied, no >>> behavioral changes exists for whoever is still using memcg to control >>> their memory usage. >>> >>> We always account to both user and kernel resource_counters. This >>> effectively means that an independent kernel limit is in place when the >>> limit is set to a lower value than the user memory. A equal or higher >>> value means that the user limit will always hit first, meaning that kmem >>> is effectively unlimited. >>> >>> People who want to track kernel memory but not limit it, can set this >>> limit to a very high number (like RESOURCE_MAX - 1page - that no one >>> will ever hit, or equal to the user memory) >>> >>> Signed-off-by: Glauber Costa <glommer@parallels.com> >>> CC: Michal Hocko <mhocko@suse.cz> >>> CC: Johannes Weiner <hannes@cmpxchg.org> >>> Reviewed-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> >> >> Could you add a patch for documentation of this new interface and a text >> explaining the behavior of "kmem_accounting" ? >> >> Hm, my concern is the difference of behavior between user page accounting and >> kmem accounting...but this is how tcp-accounting is working. >> >> Once you add Documentation, it's okay to add my Ack. >> > I plan to add documentation in a separate patch. Due to that, can I add > your ack to this patch here? > > Also, I find that the descr…
creator: KAMEZAWA Hiroyuki
date: 2012-08-17T02:38:34-00:00

Re: [PATCH v2 04/11] kmem accounting basic infrastructure
type: item
description: On Thu, Aug 16, 2012 at 8:25 AM, Michal Hocko <mhocko@suse.cz> wrote: > On Wed 15-08-12 12:50:55, Ying Han wrote: >> On Tue, Aug 14, 2012 at 9:21 AM, Michal Hocko <mhocko@suse.cz> wrote: >> > On Thu 09-08-12 17:01:12, Glauber Costa wrote: >> >> This patch adds the basic infrastructure for the accounting of the slab >> >> caches. To control that, the following files are created: >> >> >> >> * memory.kmem.usage_in_bytes >> >> * memory.kmem.limit_in_bytes >> >> * memory.kmem.failcnt >> >> * memory.kmem.max_usage_in_bytes >> >> >> >> They have the same meaning of their user memory counterparts. They >> >> reflect the state of the "kmem" res_counter. >> >> >> >> The code is not enabled until a limit is set. This can be tested by the >> >> flag "kmem_accounted". This means that after the patch is applied, no >> >> behavioral changes exists for whoever is still using memcg to control >> >> their memory usage. >> >> >> >> We always account to both user and kernel resource_counters. This >> >> effectively means that an independent kernel limit is in place when the >> >> limit is set to a lower value than the user memory. A equal or higher >> >> value means that the user limit will always hit first, meaning that kmem >> >> is effectively unlimited. >> > >> > Well, it contributes to the user limit so it is not unlimited. It just >> > falls under a different limit and it tends to contribute less. This can >> > be quite confusing. I am still not sure whether we should mix the two >> > things together. If somebody wants to limit the kernel memory he has to >> > touch the other limit anyway. Do you have a strong reason to mix the >> > user and kernel counters? >> >> The reason to mix the two together is a compromise of the two use >> cases we've heard by far. In google, we only need one limit which >> limits u & k, and the reclaim kicks in when the total usage hits the >> limit. >> >> > My impression was that kernel allocation should simply fail while user >> > allocations might r…
creator: Ying Han
date: 2012-08-17T05:58:21-00:00

Re: [PATCH v2 08/11] memcg: disable kmem code when not in use.
type: item
description: On 08/17/2012 11:02 AM, Michal Hocko wrote: > On Thu 09-08-12 17:01:16, Glauber Costa wrote: >> We can use jump labels to patch the code in or out when not used. >> >> Because the assignment: memcg->kmem_accounted = true is done after the >> jump labels increment, we guarantee that the root memcg will always be >> selected until all call sites are patched (see memcg_kmem_enabled). > > Not that it would be really important because kmem_accounted goes away > in a subsequent patch but I think the wording is a bit misleading here. > First of all there is no guanratee that kmem_accounted=true is seen > before atomic_inc(&key->enabled) because there is no memory barrier and > the lock serves just a leave barrier. But I do not think this is > important at all because key->enabled is what matters here. Even if > memcg_kmem_enabled is true we do not consider it if the key is disabled, > right? > Right.
creator: Glauber Costa
date: 2012-08-17T07:01:06-00:00