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

[PATCH 00/11] kmem controller for memcg: stripped down version
type: item
description: Hi, What I am proposing with this series is a stripped down version of the kmem controller for memcg that would allow us to merge significant parts of the infrastructure, while leaving out, for now, the polemic bits about the slab while it is being reworked by Cristoph. Me reasoning for that is that after the last change to introduce a gfp flag to mark kernel allocations, it became clear to me that tracking other resources like the stack would then follow extremely naturaly. I figured that at some point we'd have to solve the issue pointed by David, and avoid testing the Slab flag in the page allocator, since it would soon be made more generic. I do that by having the callers to explicit mark it. So to demonstrate how it would work, I am introducing a stack tracker here, that is already a functionality per-se: it successfully stops fork bombs to happen. (Sorry for doing all your work, Frederic =p ). Note that after all memcg infrastructure is deployed, it becomes very easy to track anything. The last patch of this series is extremely simple. The infrastructure is exactly the same we had in memcg, but stripped down of the slab parts. And because what we have after those patches is a feature per-se, I think it could be considered for merging. Let me know what you think. Glauber Costa (9): memcg: change defines to an enum kmem slab accounting basic infrastructure Add a __GFP_KMEMCG flag memcg: kmem controller infrastructure mm: Allocate kernel pages to the right memcg memcg: disable kmem code when not in use. memcg: propagate kmem limiting information to children memcg: allow a memcg with kmem charges to be destructed. protect architectures where THREAD_SIZE >= PAGE_SIZE against fork bombs Suleiman Souhlal (2): memcg: Make it possible to use the stock for more than one page. memcg: Reclaim when more than one page needed. include/linux/gfp.h | 11 +- include/linux/memcontrol.h | 46 +++++ include/linux/thread_info.h | 6 + kernel/fork.c | 4 +- mm/memcontrol.c | 395 +++++++…
creator: Glauber Costa
date: 2012-06-25T14:15:17-00:00

[PATCH 01/11] memcg: Make it possible to use the stock for more than one page.
type: item
description: From: Suleiman Souhlal <ssouhlal@FreeBSD.org> Signed-off-by: Suleiman Souhlal <suleiman@google.com> Signed-off-by: Glauber Costa <glommer@parallels.com> Acked-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- mm/memcontrol.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index f72b5e5..9304db2 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -1967,19 +1967,22 @@ 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. + * Try to consume stocked charge on this cpu. If success, nr_pages pages are + * 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. */ -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 == stock->cached && stock->nr_pages) - stock->nr_pages--; + if (memcg == stock->cached && stock->nr_pages >= nr_pages) + stock->nr_pages -= nr_pages; else /* need to call res_counter_charge */ ret = false; put_cpu_var(memcg_stock); @@ -2278,7 +2281,7 @@ again: VM_BUG_ON(css_is_removed(&memcg->css)); if (mem_cgroup_is_root(memcg)) goto done; - if (nr_pages == 1 && consume_stock(memcg)) + if (consume_stock(memcg, nr_pages)) goto done; css_get(&memcg->css); } else { @@ -2303,7 +2306,7 @@ again: rcu_read_unlock(); goto done; } - if (nr_pages == 1 && consume_stock(memcg)) { + if (consume_stock(memcg, nr_pages)) { /* * It seems dagerous to access memcg without css_get…
creator: Glauber Costa
date: 2012-06-25T14:15:18-00:00

[PATCH 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 slab 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 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), stay safe with a cond_resched(), 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 | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 9304db2..8e601e8 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2158,8 +2158,16 @@ enum { CHARGE_OOM_DIE, /* the current is killed because of OOM */ }; +/* + * We need a number that is small enough to be likely to have been + * reclaimed even under pressure, but not too big to trigger unnecessary + * retries + */ +#define NR_PAGES_TO_RETRY 2 + 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; @@ -2182,18 +2190,18 @@ static int mem_cgroup_do_charge(struct mem_cgroup *memcg, gfp_t gfp_mask, } else m…
creator: Glauber Costa
date: 2012-06-25T14:15:19-00:00

[PATCH 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. 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 | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 8e601e8..9352d40 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -387,9 +387,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) -- 1.7.10.2
creator: Glauber Costa
date: 2012-06-25T14:15:20-00:00

[PATCH 04/11] kmem slab 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 | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 77 insertions(+), 1 deletion(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 9352d40..6f34b77 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -265,6 +265,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. */ @@ -279,6 +283,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; @@ -391,6 +396,7 @@ enum res_type { _MEM, _MEMSWAP, _OOM_TYPE, + _KMEM, }; #def…
creator: Glauber Costa
date: 2012-06-25T14:15:21-00:00

[PATCH 05/11] Add a __GFP_KMEMCG flag
type: item
description: This flag is used to indicate to the callees that this allocation will be serviced to the kernel. It is not supposed to be passed by the callers of kmem_cache_alloc, but rather by the cache core itself. 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 | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/include/linux/gfp.h b/include/linux/gfp.h index 1e49be4..8f4079f 100644 --- a/include/linux/gfp.h +++ b/include/linux/gfp.h @@ -37,6 +37,9 @@ struct vm_area_struct; #define ___GFP_NO_KSWAPD 0x400000u #define ___GFP_OTHER_NODE 0x800000u #define ___GFP_WRITE 0x1000000u +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM +#define ___GFP_KMEMCG 0x2000000u +#endif /* * GFP bitmasks.. @@ -88,13 +91,16 @@ 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 */ +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM +#define __GFP_KMEMCG ((__force gfp_t)___GFP_KMEMCG)/* Allocation comes from a memcg-accounted resource */ +#endif /* * This may seem redundant, but it's a way of annotating false positives vs. * allocations that simply cannot be supported (e.g. page tables). */ #define __GFP_NOTRACK_FALSE_POSITIVE (__GFP_NOTRACK) -#define __GFP_BITS_SHIFT 25 /* Room for N __GFP_FOO bits */ +#define __GFP_BITS_SHIFT 26 /* Room for N __GFP_FOO bits */ #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1)) /* This equals 0, but use constants in case they ever change */ -- 1.7.10.2
creator: Glauber Costa
date: 2012-06-25T14:15:22-00:00

[PATCH 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 acessors. The idea is to later on, patch those with jump labels, so we don't incur any overhead when no mem cgroups are being used. 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 | 44 ++++++++++++ mm/memcontrol.c | 172 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 216 insertions(+) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 83e7ba9..22479eb 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; @@ -409,6 +410,12 @@ struct sock; #ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM void sock_update_memcg(struct sock *sk); void sock_release_memcg(struct sock *sk); + +#define mem_cgroup_kmem_on 1 +bool __mem_cgroup_new_kmem_page(gfp_t gfp, void *handle, int order); +void __mem_cgroup_commit_kmem_page(struct page *page, void *handle, int order); +void __mem_cgroup_free_kmem_page(struct page *page, int order); +#define is_kmem_tracked_alloc (gfp & __GFP_KMEMCG) #else static inline void sock_update_memcg(struct sock *sk) { @@ -416,6 +423,43 @@ static inline void sock_update_memcg(struct sock *sk) static inline void sock_release_memcg(struct sock *sk) { } + +#define mem_cgroup_kmem_on 0 +#define __mem_cgroup_new_kmem_page(a, b, c) false +#define __mem_cgroup_free_kmem_page(a,b ) +#define __mem_cgroup_commit_kmem_page(a, b, c) +#define is_kmem_tracked…
creator: Glauber Costa
date: 2012-06-25T14:15:23-00:00

[PATCH 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 | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/include/linux/gfp.h b/include/linux/gfp.h index 8f4079f..4e27cd8 100644 --- a/include/linux/gfp.h +++ b/include/linux/gfp.h @@ -368,6 +368,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 4403009..cb8867e 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -2479,6 +2479,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; @@ -2490,6 +2491,13 @@ _…
creator: Glauber Costa
date: 2012-06-25T14:15:24-00:00

[PATCH 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 mem_cgroup_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> CC: Christoph Lameter <cl@linux.com> CC: Pekka Enberg <penberg@cs.helsinki.fi> CC: Michal Hocko <mhocko@suse.cz> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: Johannes Weiner <hannes@cmpxchg.org> CC: Suleiman Souhlal <suleiman@google.com> --- include/linux/memcontrol.h | 4 +++- mm/memcontrol.c | 28 ++++++++++++++++++++++++++-- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 22479eb..4d69ff8 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; @@ -411,7 +412,8 @@ struct sock; void sock_update_memcg(struct sock *sk); void sock_release_memcg(struct sock *sk); -#define mem_cgroup_kmem_on 1 +extern struct static_key mem_cgroup_kmem_enabled_key; +#define mem_cgroup_kmem_on static_key_false(&mem_cgroup_kmem_enabled_key) bool __mem_cgroup_new_kmem_page(gfp_t gfp, void *handle, int order); void __mem_cgroup_commit_kmem_page(struct page *page, void *handle, int order); void __mem_cgroup_free_kmem_page(struct page *page, int order); diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 27b2b6f..fe5388e 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -425,6 +425,10 @@ static void mem_cgroup_put(struct mem_cgroup *memcg); #include <net/sock.h> #include <net/ip.h> +struct static_key mem_cgrou…
creator: Glauber Costa
date: 2012-06-25T14:15:25-00:00

[PATCH 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 empty taskwise * bash in C does find / 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 | 86 +++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 71 insertions(+), 15 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index fe5388e..a6a440b 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -287,7 +287,11 @@ struct mem_cgroup { * Should the accounting and control be hierarchical, per subtree? */ bool use_hierarchy; - bool kmem_accounted; + /* + * bit0: accounted by this cgroup + * bit1: accounted by a parent. + */ + volatile unsigned long kmem_accounted; bool oom_lock; atomic_t under_oom; @@ -340,6 +344,9 @@ struct mem_cgroup { #endif }; +#define KMEM_ACCOUNTED_THIS 0 +#define KMEM_ACCOUNTED_PAR…
creator: Glauber Costa
date: 2012-06-25T14:15:26-00:00

[PATCH 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> 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 | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index a6a440b..bb9b6fe 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -598,6 +598,11 @@ static void disarm_kmem_keys(struct mem_cgroup *memcg) { if (test_bit(KMEM_ACCOUNTED_THIS, &memcg->kmem_accounted)) static_key_slow_dec(&mem_cgroup_kmem_enabled_key); + /* + * This check can't live in kmem destruction function, + * since the charges will outlive the cgroup + */ + BUG_ON(res_counter_read_u64(&memcg->kmem, RES_USAGE) != 0); } #else static void disarm_kmem_keys(struct mem_cgroup *memcg) @@ -3838,6 +3843,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); @@ -3877,8 +3883,10 @@ move_account: if (ret == -ENOMEM) goto try_to_free; cond_resched(); + usage = res_counter_read_u64(&memcg->res, RES_USAGE) - + res_counter_read_u64(&memcg->kmem, RES_USAGE); /* "ret" should also be checked to ensure all lists are empty. */ - } while (res_counter_read_u64(&memcg->res, RES_USAGE) > 0 || ret); + } while (usage > …
creator: Glauber Costa
date: 2012-06-25T14:15:27-00:00

[PATCH 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> 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: Frederic Weisbecker <fweisbec@redhat.com> --- include/linux/thread_info.h | 6 ++++++ kernel/fork.c | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h index ccc1899..914ec07 100644 --- a/include/linux/thread_info.h +++ b/include/linux/thread_info.h @@ -61,6 +61,12 @@ extern long do_no_restart_syscall(struct restart_block *parm); # define THREADINFO_GFP (GFP_KERNEL | __GFP_NOTRACK) #endif +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM +# define THREADINFO_GFP_ACCOUNTED (THREADINFO_GFP | __GFP_KMEMCG) +#else +# define THREADINFO_GFP_ACCOUNTED (THREADINFO_GFP) +#endif + /* * flag set/clear/test wrappers * - pass TIF_xxxx constants to these functions diff --git a/kernel/fork.c b/kernel/fork.c index ab5211b..06b414f 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -142,7 +142,7 @@ void __weak arch_releas…
creator: Glauber Costa
date: 2012-06-25T14:15:28-00:00

Re: [PATCH 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: On 06/25/2012 04:15 PM, 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> > 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: Frederic Weisbecker <fweisbec@redhat.com> Thanks!
creator: Frederic Weisbecker
date: 2012-06-25T16:55:35-00:00

Re: [PATCH 01/11] memcg: Make it possible to use the stock for more than one page.
type: item
description: Hey, Glauber. Just a couple nits. On Mon, Jun 25, 2012 at 06:15:18PM +0400, Glauber Costa wrote: > From: Suleiman Souhlal <ssouhlal@FreeBSD.org> It would be nice to explain why this is being done. Just a simple statement like - "prepare for XXX" or "will be needed by XXX". > /* > - * 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. > + * Try to consume stocked charge on this cpu. If success, nr_pages pages are > + * 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. I hope this were converted to proper /** function comment with arguments and return value description. Thanks. -- tejun
creator: Tejun Heo
date: 2012-06-25T17:44:37-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: Again, nits. On Mon, Jun 25, 2012 at 06:15:23PM +0400, Glauber Costa wrote: > +#define mem_cgroup_kmem_on 1 > +bool __mem_cgroup_new_kmem_page(gfp_t gfp, void *handle, int order); > +void __mem_cgroup_commit_kmem_page(struct page *page, void *handle, int order); > +void __mem_cgroup_free_kmem_page(struct page *page, int order); > +#define is_kmem_tracked_alloc (gfp & __GFP_KMEMCG) Ugh... please do the following instead. static inline bool is_kmem_tracked_alloc(gfp_t gfp) { return gfp & __GFP_KMEMCG; } > #else > static inline void sock_update_memcg(struct sock *sk) > { > @@ -416,6 +423,43 @@ static inline void sock_update_memcg(struct sock *sk) > static inline void sock_release_memcg(struct sock *sk) > { > } > + > +#define mem_cgroup_kmem_on 0 > +#define __mem_cgroup_new_kmem_page(a, b, c) false > +#define __mem_cgroup_free_kmem_page(a,b ) > +#define __mem_cgroup_commit_kmem_page(a, b, c) > +#define is_kmem_tracked_alloc (false) I would prefer static inlines here too. It's a bit more code in the header but leads to less surprises (e.g. arg evals w/ side effects or compiler warning about unused vars) and makes it easier to avoid cosmetic errors. Thanks. -- tejun
creator: Tejun Heo
date: 2012-06-25T18:06:19-00:00

Re: [PATCH 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: On Mon, Jun 25, 2012 at 06:15:24PM +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, 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(). Shouldn't we be documenting that in the code somewhere, preferably in the function comments? -- tejun
creator: Tejun Heo
date: 2012-06-25T18:07:47-00:00

Re: [PATCH 09/11] memcg: propagate kmem limiting information to children
type: item
description: Feeling like a nit pervert but.. On Mon, Jun 25, 2012 at 06:15:26PM +0400, Glauber Costa wrote: > @@ -287,7 +287,11 @@ struct mem_cgroup { > * Should the accounting and control be hierarchical, per subtree? > */ > bool use_hierarchy; > - bool kmem_accounted; > + /* > + * bit0: accounted by this cgroup > + * bit1: accounted by a parent. > + */ > + volatile unsigned long kmem_accounted; Is the volatile declaration really necessary? Why is it necessary? Why no comment explaining it? > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM > +static void mem_cgroup_update_kmem_limit(struct mem_cgroup *memcg, u64 val) > +{ > + struct mem_cgroup *iter; > + > + mutex_lock(&set_limit_mutex); > + if (!test_and_set_bit(KMEM_ACCOUNTED_THIS, &memcg->kmem_accounted) && > + val != RESOURCE_MAX) { > + > + /* > + * 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(&mem_cgroup_kmem_enabled_key); > + > + if (!memcg->use_hierarchy) > + goto out; > + > + for_each_mem_cgroup_tree(iter, memcg) { > + if (iter == memcg) > + continue; > + set_bit(KMEM_ACCOUNTED_PARENT, &iter->kmem_accounted); > + } > + > + } else if (test_and_clear_bit(KMEM_ACCOUNTED_THIS, &memcg->kmem_accounted) > + && val == RESOURCE_MAX) { > + > + if (!memcg->use_hierarchy) > + goto out; > + > + for_each_mem_cgroup_tree(iter, memcg) { > + struct mem_cgroup *parent; Blank line between decl and body please. > + if (iter == memcg) > + continue; > + /* > + * We should only have our parent bit cleared if none of > + * ouri parents are accounted. The transversal order of ^ type > + * our iter function forces us to always look at the > + * parents. Also, it's okay here but the text filling in comments and patch descriptions tend to be quite inconsistent. If you're on emacs, alt-q is your friend and I'm sure vim can do text filling pretty nicely too. > + */ > + parent =…
creator: Tejun Heo
date: 2012-06-25T18:29:07-00:00

Re: [PATCH 10/11] memcg: allow a memcg with kmem charges to be destructed.
type: item
description: On Mon, Jun 25, 2012 at 06:15:27PM +0400, Glauber Costa wrote: > 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> > 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 | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index a6a440b..bb9b6fe 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -598,6 +598,11 @@ static void disarm_kmem_keys(struct mem_cgroup *memcg) > { > if (test_bit(KMEM_ACCOUNTED_THIS, &memcg->kmem_accounted)) > static_key_slow_dec(&mem_cgroup_kmem_enabled_key); > + /* > + * This check can't live in kmem destruction function, > + * since the charges will outlive the cgroup > + */ > + BUG_ON(res_counter_read_u64(&memcg->kmem, RES_USAGE) != 0); WARN_ON() please. Misaccounted kernel usually is better than dead kernel. Thanks. -- tejun
creator: Tejun Heo
date: 2012-06-25T18:34:42-00:00

Re: [PATCH 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: On Mon, Jun 25, 2012 at 06:55:35PM +0200, Frederic Weisbecker wrote: > On 06/25/2012 04:15 PM, 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> > > 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: Frederic Weisbecker <fweisbec@redhat.com> Frederic, does this (with proper slab accounting added later) achieve what you wanted with the task counter? Thanks. -- tejun
creator: Tejun Heo
date: 2012-06-25T18:38:18-00:00

Re: [PATCH 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: On Mon, Jun 25, 2012 at 11:38:18AM -0700, Tejun Heo wrote: > On Mon, Jun 25, 2012 at 06:55:35PM +0200, Frederic Weisbecker wrote: > > On 06/25/2012 04:15 PM, 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> > > > 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: Frederic Weisbecker <fweisbec@redhat.com> > > Frederic, does this (with proper slab accounting added later) achieve > what you wanted with the task counter? I think so yeah. Relying on general kernel memory accounting should do the trick for us. And if we need more finegrained limitation on kernel stack accounting we can still add it incrementally. But I believe global limitation can be enough. Thanks.
creator: Frederic Weisbecker
date: 2012-06-25T20:57:51-00:00

Re: [PATCH 10/11] memcg: allow a memcg with kmem charges to be destructed.
type: item
description: On 06/25/2012 10:34 PM, Tejun Heo wrote: > On Mon, Jun 25, 2012 at 06:15:27PM +0400, Glauber Costa wrote: >> 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> >> 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 | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index a6a440b..bb9b6fe 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -598,6 +598,11 @@ static void disarm_kmem_keys(struct mem_cgroup *memcg) >> { >> if (test_bit(KMEM_ACCOUNTED_THIS, &memcg->kmem_accounted)) >> static_key_slow_dec(&mem_cgroup_kmem_enabled_key); >> + /* >> + * This check can't live in kmem destruction function, >> + * since the charges will outlive the cgroup >> + */ >> + BUG_ON(res_counter_read_u64(&memcg->kmem, RES_USAGE) != 0); > > WARN_ON() please. Misaccounted kernel usually is better than dead > kernel. > You're absolutely right, will change.
creator: Glauber Costa
date: 2012-06-25T22:25:46-00:00

Re: [PATCH 07/11] mm: Allocate kernel pages to the right memcg
type: item
description: On 06/25/2012 10:07 PM, Tejun Heo wrote: > On Mon, Jun 25, 2012 at 06:15:24PM +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, 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(). > > Shouldn't we be documenting that in the code somewhere, preferably in > the function comments? > I can certainly do that.
creator: Glauber Costa
date: 2012-06-25T22:27:36-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: On 06/25/2012 10:06 PM, Tejun Heo wrote: > Again, nits. > > On Mon, Jun 25, 2012 at 06:15:23PM +0400, Glauber Costa wrote: >> +#define mem_cgroup_kmem_on 1 >> +bool __mem_cgroup_new_kmem_page(gfp_t gfp, void *handle, int order); >> +void __mem_cgroup_commit_kmem_page(struct page *page, void *handle, int order); >> +void __mem_cgroup_free_kmem_page(struct page *page, int order); >> +#define is_kmem_tracked_alloc (gfp & __GFP_KMEMCG) > > Ugh... please do the following instead. > > static inline bool is_kmem_tracked_alloc(gfp_t gfp) > { > return gfp & __GFP_KMEMCG; > } > >> #else >> static inline void sock_update_memcg(struct sock *sk) >> { >> @@ -416,6 +423,43 @@ static inline void sock_update_memcg(struct sock *sk) >> static inline void sock_release_memcg(struct sock *sk) >> { >> } >> + >> +#define mem_cgroup_kmem_on 0 >> +#define __mem_cgroup_new_kmem_page(a, b, c) false >> +#define __mem_cgroup_free_kmem_page(a,b ) >> +#define __mem_cgroup_commit_kmem_page(a, b, c) >> +#define is_kmem_tracked_alloc (false) > > I would prefer static inlines here too. It's a bit more code in the > header but leads to less surprises (e.g. arg evals w/ side effects or > compiler warning about unused vars) and makes it easier to avoid > cosmetic errors. > > Thanks. > Sure thing.
creator: Glauber Costa
date: 2012-06-25T22:28:00-00:00

Re: [PATCH 01/11] memcg: Make it possible to use the stock for more than one page.
type: item
description: On 06/25/2012 09:44 PM, Tejun Heo wrote: > Hey, Glauber. > > Just a couple nits. > > On Mon, Jun 25, 2012 at 06:15:18PM +0400, Glauber Costa wrote: >> From: Suleiman Souhlal <ssouhlal@FreeBSD.org> > > It would be nice to explain why this is being done. Just a simple > statement like - "prepare for XXX" or "will be needed by XXX". I picked this patch from Suleiman Souhlal, and tried to keep it as close as possible to his version. But for the sake of documentation, I can do that, yes.
creator: Glauber Costa
date: 2012-06-25T22:29:06-00:00

Re: [PATCH 01/11] memcg: Make it possible to use the stock for more than one page.
type: item
description: Hello, On Mon, Jun 25, 2012 at 3:29 PM, Glauber Costa <glommer@parallels.com> wrote: >> It would be nice to explain why this is being done. Just a simple >> statement like - "prepare for XXX" or "will be needed by XXX". > > > I picked this patch from Suleiman Souhlal, and tried to keep it as close as > possible to his version. > > But for the sake of documentation, I can do that, yes. Yeah, that would be great. Also, the patch does change behavior, right? Explaining / justifying that a bit would be nice too. Thanks! -- tejun
creator: Tejun Heo
date: 2012-06-25T22:33:01-00:00

Re: [PATCH 09/11] memcg: propagate kmem limiting information to children
type: item
description: On 06/25/2012 10:29 PM, Tejun Heo wrote: > Feeling like a nit pervert but.. > > On Mon, Jun 25, 2012 at 06:15:26PM +0400, Glauber Costa wrote: >> @@ -287,7 +287,11 @@ struct mem_cgroup { >> * Should the accounting and control be hierarchical, per subtree? >> */ >> bool use_hierarchy; >> - bool kmem_accounted; >> + /* >> + * bit0: accounted by this cgroup >> + * bit1: accounted by a parent. >> + */ >> + volatile unsigned long kmem_accounted; > > Is the volatile declaration really necessary? Why is it necessary? > Why no comment explaining it? Seems to be required by set_bit and friends. gcc will complain if it is not volatile (take a look at the bit function headers) >> + >> + for_each_mem_cgroup_tree(iter, memcg) { >> + struct mem_cgroup *parent; > > Blank line between decl and body please. ok. > >> + if (iter == memcg) >> + continue; >> + /* >> + * We should only have our parent bit cleared if none of >> + * ouri parents are accounted. The transversal order of > > ^ type > >> + * our iter function forces us to always look at the >> + * parents. > > Also, it's okay here but the text filling in comments and patch > descriptions tend to be quite inconsistent. If you're on emacs, alt-q > is your friend and I'm sure vim can do text filling pretty nicely too. > >> + */ >> + parent = parent_mem_cgroup(iter); >> + while (parent && (parent != memcg)) { >> + if (test_bit(KMEM_ACCOUNTED_THIS, &parent->kmem_accounted)) >> + goto noclear; >> + >> + parent = parent_mem_cgroup(parent); >> + } > > Better written in for (;;)? Also, if we're breaking on parent == > memcg, can we ever hit NULL parent in the above loop? I can simplify to test parent != memcg only, indeed it is not expected to be NULL (but if it happens to be due to any kind of bug, we protect against NULL-dereference, that is why I like to write this way) >> + continue; >> + } >> + } >> +out: >> + mutex_unlock(&set_limit_mutex); > > Can we please branch on val != RECOURSE_MAX first? I'm not even sure > whether the abo…
creator: Glauber Costa
date: 2012-06-25T22:36:27-00:00

Re: [PATCH 09/11] memcg: propagate kmem limiting information to children
type: item
description: Hello, Glauber. On Tue, Jun 26, 2012 at 02:36:27AM +0400, Glauber Costa wrote: > >Is the volatile declaration really necessary? Why is it necessary? > >Why no comment explaining it? > > Seems to be required by set_bit and friends. gcc will complain if it > is not volatile (take a look at the bit function headers) Hmmm? Are you sure gcc includes volatile in type check? There are a lot of bitops users in the kernel but most of them don't use volatile decl on the variable. > >>+ */ > >>+ parent = parent_mem_cgroup(iter); > >>+ while (parent && (parent != memcg)) { > >>+ if (test_bit(KMEM_ACCOUNTED_THIS, &parent->kmem_accounted)) > >>+ goto noclear; > >>+ > >>+ parent = parent_mem_cgroup(parent); > >>+ } > > > >Better written in for (;;)? Also, if we're breaking on parent == > >memcg, can we ever hit NULL parent in the above loop? > > I can simplify to test parent != memcg only, indeed it is not > expected to be NULL (but if it happens to be due to any kind of bug, > we protect against NULL-dereference, that is why I like to write > this way) I personally don't really like that. It doesn't really add meaningful protection (if that happens the tree walking is already severely broken) while causes confusion to future readers of the code (when can parent be NULL?). Thanks. -- tejun
creator: Tejun Heo
date: 2012-06-25T22:49:42-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: On Mon, 25 Jun 2012 18:15:23 +0400 Glauber Costa <glommer@parallels.com> 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 acessors. > The idea is to later on, patch those with jump labels, so we don't > incur any overhead when no mem cgroups are being used. > > > ... > > @@ -416,6 +423,43 @@ static inline void sock_update_memcg(struct sock *sk) > static inline void sock_release_memcg(struct sock *sk) > { > } > + > +#define mem_cgroup_kmem_on 0 > +#define __mem_cgroup_new_kmem_page(a, b, c) false > +#define __mem_cgroup_free_kmem_page(a,b ) > +#define __mem_cgroup_commit_kmem_page(a, b, c) I suggest that the naming consistently follow the model "mem_cgroup_kmem_foo". So "mem_cgroup_kmem_" becomes the well-known identifier for this subsystem. Then, s/mem_cgroup/memcg/g/ - show us some mercy here! > +#define is_kmem_tracked_alloc (false) memcg_kmem_tracked_alloc, perhaps. But what does this actually do? <looks> eww, ick. > +#define is_kmem_tracked_alloc (gfp & __GFP_KMEMCG) What Tejun said. This: /* * Nice comment goes here */ static inline bool memcg_kmem_tracked_alloc(gfp_t gfp) { return gfp & __GFP_KMEMCG; } > #endif /* CONFIG_CGROUP_MEM_RES_CTLR_KMEM */ > + > +static __always_inline > +bool mem_cgroup_new_kmem_page(gfp_t gfp, void *handle, int order) memcg_kmem_new_page(). > +{ > + if (!mem_cgroup_kmem_on) > + return true; > + if (!is_kmem_tracked_alloc) > + return true; > + if (!current->mm) > + return true; > + if (in_interrupt()) > + return true; > + if (gfp & __GFP_NOFAIL) > + return true; > + return __mem_cgroup_new_kmem_page(gfp, handle, order); > +} Add documentation, please. The semantics of the return value are inscrutable. > +static __always_inline > +void mem_cgroup_free_kmem_page(struct page *page, int order) > +{ > + i…
creator: akpm
date: 2012-06-25T23:17:20-00:00

Re: [PATCH 09/11] memcg: propagate kmem limiting information to children
type: item
description: On Tue, 26 Jun 2012 02:36:27 +0400 Glauber Costa <glommer@parallels.com> wrote: > On 06/25/2012 10:29 PM, Tejun Heo wrote: > > Feeling like a nit pervert but.. > > > > On Mon, Jun 25, 2012 at 06:15:26PM +0400, Glauber Costa wrote: > >> @@ -287,7 +287,11 @@ struct mem_cgroup { > >> * Should the accounting and control be hierarchical, per subtree? > >> */ > >> bool use_hierarchy; > >> - bool kmem_accounted; > >> + /* > >> + * bit0: accounted by this cgroup > >> + * bit1: accounted by a parent. > >> + */ > >> + volatile unsigned long kmem_accounted; > > > > Is the volatile declaration really necessary? Why is it necessary? > > Why no comment explaining it? > > Seems to be required by set_bit and friends. gcc will complain if it is > not volatile (take a look at the bit function headers) That would be a broken gcc. We run test_bit()/set_bit() and friends against plain old `unsigned long' in thousands of places. There's nothing special about this one!
creator: akpm
date: 2012-06-25T23:21:58-00:00

Re: [PATCH 09/11] memcg: propagate kmem limiting information to children
type: item
description: On Mon, 25 Jun 2012 18:15:26 +0400 Glauber Costa <glommer@parallels.com> wrote: > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -287,7 +287,11 @@ struct mem_cgroup { > * Should the accounting and control be hierarchical, per subtree? > */ > bool use_hierarchy; > - bool kmem_accounted; > + /* > + * bit0: accounted by this cgroup > + * bit1: accounted by a parent. > + */ > + volatile unsigned long kmem_accounted; I suggest unsigned long kmem_accounted; /* See KMEM_ACCOUNTED_*, below */ > bool oom_lock; > atomic_t under_oom; > @@ -340,6 +344,9 @@ struct mem_cgroup { > #endif > }; > > +#define KMEM_ACCOUNTED_THIS 0 > +#define KMEM_ACCOUNTED_PARENT 1 And then document the fields here.
creator: akpm
date: 2012-06-25T23:23:52-00:00

Re: [PATCH 00/11] kmem controller for memcg: stripped down version
type: item
description: On Mon, 25 Jun 2012 18:15:17 +0400 Glauber Costa <glommer@parallels.com> wrote: > What I am proposing with this series is a stripped down version of the > kmem controller for memcg that would allow us to merge significant parts > of the infrastructure, while leaving out, for now, the polemic bits about > the slab while it is being reworked by Cristoph. > > Me reasoning for that is that after the last change to introduce a gfp > flag to mark kernel allocations, it became clear to me that tracking other > resources like the stack would then follow extremely naturaly. I figured > that at some point we'd have to solve the issue pointed by David, and avoid > testing the Slab flag in the page allocator, since it would soon be made > more generic. I do that by having the callers to explicit mark it. > > So to demonstrate how it would work, I am introducing a stack tracker here, > that is already a functionality per-se: it successfully stops fork bombs to > happen. (Sorry for doing all your work, Frederic =p ). Note that after all > memcg infrastructure is deployed, it becomes very easy to track anything. > The last patch of this series is extremely simple. > > The infrastructure is exactly the same we had in memcg, but stripped down > of the slab parts. And because what we have after those patches is a feature > per-se, I think it could be considered for merging. hm. None of this new code makes the kernel smaller, faster, easier to understand or more fun to read! Presumably we're getting some benefit for all the downside. When the time is appropriate, please do put some time into explaining that benefit, so that others can agree that it is a worthwhile tradeoff.
creator: akpm
date: 2012-06-25T23:27:45-00:00

Re: [PATCH 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On Mon, Jun 25, 2012 at 7:15 AM, Glauber Costa <glommer@parallels.com> wrote: > From: Suleiman Souhlal <ssouhlal@FreeBSD.org> > > mem_cgroup_do_charge() was written before slab 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 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), stay safe with a cond_resched(), > and make sure not to do it if __GFP_NORETRY. The commit description mentions COSTLY_ORDER, but it's not actually used in the patch. -- Suleiman
creator: Suleiman Souhlal
date: 2012-06-25T23:33:29-00:00

Re: [PATCH 01/11] memcg: Make it possible to use the stock for more than one page.
type: item
description: On Mon, 25 Jun 2012, Glauber Costa wrote: > From: Suleiman Souhlal <ssouhlal@FreeBSD.org> > > Signed-off-by: Suleiman Souhlal <suleiman@google.com> > Signed-off-by: Glauber Costa <glommer@parallels.com> > Acked-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Acked-by: David Rientjes <rientjes@google.com>
creator: David Rientjes
date: 2012-06-26T04:01:54-00:00

Re: [PATCH 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On Mon, 25 Jun 2012, Glauber Costa wrote: > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 9304db2..8e601e8 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2158,8 +2158,16 @@ enum { > CHARGE_OOM_DIE, /* the current is killed because of OOM */ > }; > > +/* > + * We need a number that is small enough to be likely to have been > + * reclaimed even under pressure, but not too big to trigger unnecessary Whitespace. > + * retries > + */ > +#define NR_PAGES_TO_RETRY 2 > + Should be 1 << PAGE_ALLOC_COSTLY_ORDER? Where does this number come from? The changelog doesn't specify. > 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; > @@ -2182,18 +2190,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; > > 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; > @@ -2206,7 +2214,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 <= NR_PAGES_TO_RETRY && ret) > return CHARGE_RETRY; > > /* > @@ -2341,7 +2349,8 @@ again: > nr_oom_retri…
creator: David Rientjes
date: 2012-06-26T04:09:11-00:00

Re: [PATCH 03/11] memcg: change defines to an enum
type: item
description: On Mon, 25 Jun 2012, Glauber Costa wrote: > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 8e601e8..9352d40 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -387,9 +387,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) Shouldn't everything that does MEMFILE_TYPE() now be using type enum res_type rather than int?
creator: David Rientjes
date: 2012-06-26T04:11:55-00:00

Re: [PATCH 04/11] kmem slab accounting basic infrastructure
type: item
description: On Mon, 25 Jun 2012, Glauber Costa wrote: > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 9352d40..6f34b77 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -265,6 +265,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. > */ > @@ -279,6 +283,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; > @@ -391,6 +396,7 @@ enum res_type { > _MEM, > _MEMSWAP, > _OOM_TYPE, > + _KMEM, > }; > > #define MEMFILE_PRIVATE(x, val) ((x) << 16 | (val)) > @@ -1438,6 +1444,10 @@ done: > res_counter_read_u64(&memcg->memsw, RES_USAGE) >> 10, > res_counter_read_u64(&memcg->memsw, RES_LIMIT) >> 10, > res_counter_read_u64(&memcg->memsw, RES_FAILCNT)); > + printk(KERN_INFO "kmem: usage %llukB, limit %llukB, failcnt %llu\n", > + res_counter_read_u64(&memcg->kmem, RES_USAGE) >> 10, > + res_counter_read_u64(&memcg->kmem, RES_LIMIT) >> 10, > + res_counter_read_u64(&memcg->kmem, RES_FAILCNT)); > } > > /* > @@ -3879,6 +3889,11 @@ static ssize_t mem_cgroup_read(struct cgroup *cont, struct cftype *cft, > else > val = res_counter_read_u64(&memcg->memsw, name); > break; > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM > + case _KMEM: > + val = res_counter_read_u64(&memcg->kmem, name); > + break; > +#endif This shouldn't need an #ifdef, ->kmem is available on all CONFIG_CGROUP_MEM_RES_CTLR kernels. Same with several of the other instances in this patch. Can't these instances be addressed by not adding kmem_cgroup_files without CONFIG_CGROUP_MEM_RES_CTLR_KMEM? > default: > BUG(); > } > @@ -3916,8 +3931,26 @@ static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft, > break; > if (type == _MEM) > ret = mem_cgroup_resize_limit(memcg, val); > - else > + else if (type == _MEMSWAP) > ret = mem_c…
creator: David Rientjes
date: 2012-06-26T04:22:38-00:00

Re: [PATCH 05/11] Add a __GFP_KMEMCG flag
type: item
description: On Mon, 25 Jun 2012, Glauber Costa wrote: > This flag is used to indicate to the callees that this allocation will be > serviced to the kernel. It is not supposed to be passed by the callers > of kmem_cache_alloc, but rather by the cache core itself. > Not sure what "serviced to the kernel" means, does this mean that the memory will not be accounted for to the root memcg? > 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 | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/include/linux/gfp.h b/include/linux/gfp.h > index 1e49be4..8f4079f 100644 > --- a/include/linux/gfp.h > +++ b/include/linux/gfp.h > @@ -37,6 +37,9 @@ struct vm_area_struct; > #define ___GFP_NO_KSWAPD 0x400000u > #define ___GFP_OTHER_NODE 0x800000u > #define ___GFP_WRITE 0x1000000u > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM > +#define ___GFP_KMEMCG 0x2000000u > +#endif > > /* > * GFP bitmasks.. > @@ -88,13 +91,16 @@ 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 */ > > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM > +#define __GFP_KMEMCG ((__force gfp_t)___GFP_KMEMCG)/* Allocation comes from a memcg-accounted resource */ > +#endif Needs a space. > /* > * This may seem redundant, but it's a way of annotating false positives vs. > * allocations that simply cannot be supported (e.g. page tables). > */ > #define __GFP_NOTRACK_FALSE_POSITIVE (__GFP_NOTRACK) > > -#define __GFP_BITS_SHIFT 25 /* Room for N __GFP_FOO bits */ > +#define __GFP_BITS_SHIFT 26 /* Room for N __GFP_FOO bits */ > #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1)) > > /* This equals 0, but use constants i…
creator: David Rientjes
date: 2012-06-26T04:25:54-00:00

Re: [PATCH 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: On Mon, 25 Jun 2012, Glauber Costa wrote: > diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h > index ccc1899..914ec07 100644 > --- a/include/linux/thread_info.h > +++ b/include/linux/thread_info.h > @@ -61,6 +61,12 @@ extern long do_no_restart_syscall(struct restart_block *parm); > # define THREADINFO_GFP (GFP_KERNEL | __GFP_NOTRACK) > #endif > > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM > +# define THREADINFO_GFP_ACCOUNTED (THREADINFO_GFP | __GFP_KMEMCG) > +#else > +# define THREADINFO_GFP_ACCOUNTED (THREADINFO_GFP) > +#endif > + This type of requirement is going to become nasty very quickly if nobody can use __GFP_KMEMCG without testing for CONFIG_CGROUP_MEM_RES_CTLR_KMEM. Perhaps define __GFP_KMEMCG to be 0x0 if it's not enabled, similar to how kmemcheck does?
creator: David Rientjes
date: 2012-06-26T04:57:54-00:00

Re: [PATCH 09/11] memcg: propagate kmem limiting information to children
type: item
description: On Mon, 25 Jun 2012, Andrew Morton wrote: > > >> */ > > >> bool use_hierarchy; > > >> - bool kmem_accounted; > > >> + /* > > >> + * bit0: accounted by this cgroup > > >> + * bit1: accounted by a parent. > > >> + */ > > >> + volatile unsigned long kmem_accounted; > > > > > > Is the volatile declaration really necessary? Why is it necessary? > > > Why no comment explaining it? > > > > Seems to be required by set_bit and friends. gcc will complain if it is > > not volatile (take a look at the bit function headers) > > That would be a broken gcc. We run test_bit()/set_bit() and friends > against plain old `unsigned long' in thousands of places. There's > nothing special about this one! > No version of gcc would complain about this, even with 4.6 and later with -fstrict-volatile-bitfields, it's a qualifier that determines whether or not the access to memory is the exact size of the bitfield and aligned to its natural boundary. If the type isn't qualified as such then it's simply going to compile to access the native word size of the architecture. No special consideration is needed for a member of struct mem_cgroup.
creator: David Rientjes
date: 2012-06-26T05:23:59-00:00

Re: [PATCH 09/11] memcg: propagate kmem limiting information to children
type: item
description: On Mon, 25 Jun 2012, Andrew Morton wrote: > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -287,7 +287,11 @@ struct mem_cgroup { > > * Should the accounting and control be hierarchical, per subtree? > > */ > > bool use_hierarchy; > > - bool kmem_accounted; > > + /* > > + * bit0: accounted by this cgroup > > + * bit1: accounted by a parent. > > + */ > > + volatile unsigned long kmem_accounted; > > I suggest > > unsigned long kmem_accounted; /* See KMEM_ACCOUNTED_*, below */ > > > bool oom_lock; > > atomic_t under_oom; > > @@ -340,6 +344,9 @@ struct mem_cgroup { > > #endif > > }; > > > > +#define KMEM_ACCOUNTED_THIS 0 > > +#define KMEM_ACCOUNTED_PARENT 1 > > And then document the fields here. > In hex, please?
creator: David Rientjes
date: 2012-06-26T05:24:44-00:00

Re: [PATCH 09/11] memcg: propagate kmem limiting information to children
type: item
description: On Mon, 25 Jun 2012 22:24:44 -0700 (PDT) David Rientjes <rientjes@google.com> wrote: > > > +#define KMEM_ACCOUNTED_THIS 0 > > > +#define KMEM_ACCOUNTED_PARENT 1 > > > > And then document the fields here. > > > > In hex, please? Well, they're bit numbers, not masks. Decimal 0-31 is OK, or an enum.
creator: akpm
date: 2012-06-26T05:31:21-00:00

Re: [PATCH 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: (2012/06/26 13:57), David Rientjes wrote: > On Mon, 25 Jun 2012, Glauber Costa wrote: > >> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h >> index ccc1899..914ec07 100644 >> --- a/include/linux/thread_info.h >> +++ b/include/linux/thread_info.h >> @@ -61,6 +61,12 @@ extern long do_no_restart_syscall(struct restart_block *parm); >> # define THREADINFO_GFP (GFP_KERNEL | __GFP_NOTRACK) >> #endif >> >> +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM >> +# define THREADINFO_GFP_ACCOUNTED (THREADINFO_GFP | __GFP_KMEMCG) >> +#else >> +# define THREADINFO_GFP_ACCOUNTED (THREADINFO_GFP) >> +#endif >> + > > This type of requirement is going to become nasty very quickly if nobody > can use __GFP_KMEMCG without testing for CONFIG_CGROUP_MEM_RES_CTLR_KMEM. > Perhaps define __GFP_KMEMCG to be 0x0 if it's not enabled, similar to how > kmemcheck does? I agree. Thanks, -Kame
creator: KAMEZAWA Hiroyuki
date: 2012-06-26T05:35:34-00:00

Re: [PATCH 08/11] memcg: disable kmem code when not in use.
type: item
description: (2012/06/25 23:15), 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 mem_cgroup_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> > 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>
creator: KAMEZAWA Hiroyuki
date: 2012-06-26T05:51:15-00:00

Re: [PATCH 10/11] memcg: allow a memcg with kmem charges to be destructed.
type: item
description: (2012/06/25 23:15), Glauber Costa wrote: > 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> > 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 | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index a6a440b..bb9b6fe 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -598,6 +598,11 @@ static void disarm_kmem_keys(struct mem_cgroup *memcg) > { > if (test_bit(KMEM_ACCOUNTED_THIS, &memcg->kmem_accounted)) > static_key_slow_dec(&mem_cgroup_kmem_enabled_key); > + /* > + * This check can't live in kmem destruction function, > + * since the charges will outlive the cgroup > + */ > + BUG_ON(res_counter_read_u64(&memcg->kmem, RES_USAGE) != 0); > } > #else > static void disarm_kmem_keys(struct mem_cgroup *memcg) > @@ -3838,6 +3843,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); > > @@ -3877,8 +3883,10 @@ move_account: > if (ret == -ENOMEM) > goto try_to_free; > cond_resched(); > + usage = res_counter_read_u64(&memcg->res, RES_USAGE) - > + res_counter_read_u64(&memcg->kmem, RES_USAGE); > /* "ret" …
creator: KAMEZAWA Hiroyuki
date: 2012-06-26T05:59:56-00:00

Re: [PATCH 05/11] Add a __GFP_KMEMCG flag
type: item
description: On 06/26/2012 08:25 AM, David Rientjes wrote: > On Mon, 25 Jun 2012, Glauber Costa wrote: > >> >This flag is used to indicate to the callees that this allocation will be >> >serviced to the kernel. It is not supposed to be passed by the callers >> >of kmem_cache_alloc, but rather by the cache core itself. >> > > Not sure what "serviced to the kernel" means, does this mean that the > memory will not be accounted for to the root memcg? > In this context, it means that is a kernel allocation, not a userspace one (but in process context, of course), *and* it is to be accounted a specific memcg.
creator: Glauber Costa
date: 2012-06-26T07:08:32-00:00

Re: [PATCH 04/11] kmem slab accounting basic infrastructure
type: item
description: On 06/26/2012 08:22 AM, David Rientjes wrote: > On Mon, 25 Jun 2012, Glauber Costa wrote: > >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 9352d40..6f34b77 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -265,6 +265,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. >> */ >> @@ -279,6 +283,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; >> @@ -391,6 +396,7 @@ enum res_type { >> _MEM, >> _MEMSWAP, >> _OOM_TYPE, >> + _KMEM, >> }; >> >> #define MEMFILE_PRIVATE(x, val) ((x) << 16 | (val)) >> @@ -1438,6 +1444,10 @@ done: >> res_counter_read_u64(&memcg->memsw, RES_USAGE) >> 10, >> res_counter_read_u64(&memcg->memsw, RES_LIMIT) >> 10, >> res_counter_read_u64(&memcg->memsw, RES_FAILCNT)); >> + printk(KERN_INFO "kmem: usage %llukB, limit %llukB, failcnt %llu\n", >> + res_counter_read_u64(&memcg->kmem, RES_USAGE) >> 10, >> + res_counter_read_u64(&memcg->kmem, RES_LIMIT) >> 10, >> + res_counter_read_u64(&memcg->kmem, RES_FAILCNT)); >> } >> >> /* >> @@ -3879,6 +3889,11 @@ static ssize_t mem_cgroup_read(struct cgroup *cont, struct cftype *cft, >> else >> val = res_counter_read_u64(&memcg->memsw, name); >> break; >> +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM >> + case _KMEM: >> + val = res_counter_read_u64(&memcg->kmem, name); >> + break; >> +#endif > > This shouldn't need an #ifdef, ->kmem is available on all > CONFIG_CGROUP_MEM_RES_CTLR kernels. Same with several of the other > instances in this patch. > > Can't these instances be addressed by not adding kmem_cgroup_files without > CONFIG_CGROUP_MEM_RES_CTLR_KMEM? Yes, it can.
creator: Glauber Costa
date: 2012-06-26T07:09:24-00:00

Re: [PATCH 02/11] memcg: Reclaim when more than one page needed.
type: item
description: > >> + * retries >> + */ >> +#define NR_PAGES_TO_RETRY 2 >> + > > Should be 1 << PAGE_ALLOC_COSTLY_ORDER? Where does this number come from? > The changelog doesn't specify. Hocko complained about that, and I changed. Where the number comes from, is stated in the comments: it is a number small enough to have high changes of had been freed by the previous reclaim, and yet around the number of pages of a kernel allocation. Of course there are allocations for nr_pages > 2. But 2 will already service the stack most of the time, and most of the slab caches. >> 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; >> @@ -2182,18 +2190,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; >> >> 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; >> @@ -2206,7 +2214,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 <= NR_PAGES_TO_RETRY && ret) >> return CHARGE_RETRY; >> >> /* >> @@ -2341,7 +2349,8 @@ again: >> nr_oom_re…
creator: Glauber Costa
date: 2012-06-26T07:12:22-00:00

Re: [PATCH 00/11] kmem controller for memcg: stripped down version
type: item
description: On 06/26/2012 03:27 AM, Andrew Morton wrote: > On Mon, 25 Jun 2012 18:15:17 +0400 > Glauber Costa <glommer@parallels.com> wrote: > >> What I am proposing with this series is a stripped down version of the >> kmem controller for memcg that would allow us to merge significant parts >> of the infrastructure, while leaving out, for now, the polemic bits about >> the slab while it is being reworked by Cristoph. >> >> Me reasoning for that is that after the last change to introduce a gfp >> flag to mark kernel allocations, it became clear to me that tracking other >> resources like the stack would then follow extremely naturaly. I figured >> that at some point we'd have to solve the issue pointed by David, and avoid >> testing the Slab flag in the page allocator, since it would soon be made >> more generic. I do that by having the callers to explicit mark it. >> >> So to demonstrate how it would work, I am introducing a stack tracker here, >> that is already a functionality per-se: it successfully stops fork bombs to >> happen. (Sorry for doing all your work, Frederic =p ). Note that after all >> memcg infrastructure is deployed, it becomes very easy to track anything. >> The last patch of this series is extremely simple. >> >> The infrastructure is exactly the same we had in memcg, but stripped down >> of the slab parts. And because what we have after those patches is a feature >> per-se, I think it could be considered for merging. > > hm. None of this new code makes the kernel smaller, faster, easier to > understand or more fun to read! Not sure if this is a general comment - in case I agree - or if targeted to my statement that this is "stripped down". If so, it is of course smaller relative to my previous slab accounting patches. The infrastructure is largely common, but I realized that a future user, tracking the stack, would be a lot simpler and could be done first. > Presumably we're getting some benefit for all the downside. When the > time is appropriate, please …
creator: Glauber Costa
date: 2012-06-26T07:17:49-00:00

Re: [PATCH 10/11] memcg: allow a memcg with kmem charges to be destructed.
type: item
description: On 06/26/2012 09:59 AM, Kamezawa Hiroyuki wrote: > (2012/06/25 23:15), Glauber Costa wrote: >> 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> >> 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 | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index a6a440b..bb9b6fe 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -598,6 +598,11 @@ static void disarm_kmem_keys(struct mem_cgroup *memcg) >> { >> if (test_bit(KMEM_ACCOUNTED_THIS, &memcg->kmem_accounted)) >> static_key_slow_dec(&mem_cgroup_kmem_enabled_key); >> + /* >> + * This check can't live in kmem destruction function, >> + * since the charges will outlive the cgroup >> + */ >> + BUG_ON(res_counter_read_u64(&memcg->kmem, RES_USAGE) != 0); >> } >> #else >> static void disarm_kmem_keys(struct mem_cgroup *memcg) >> @@ -3838,6 +3843,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); >> >> @@ -3877,8 +3883,10 @@ move_account: >> if (ret == -ENOMEM) >> goto try_to_free; >> cond_resched(); >> + usage = res_coun…
creator: Glauber Costa
date: 2012-06-26T07:21:22-00:00

Re: [PATCH 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: On 06/26/2012 08:57 AM, David Rientjes wrote: > On Mon, 25 Jun 2012, Glauber Costa wrote: > >> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h >> index ccc1899..914ec07 100644 >> --- a/include/linux/thread_info.h >> +++ b/include/linux/thread_info.h >> @@ -61,6 +61,12 @@ extern long do_no_restart_syscall(struct restart_block *parm); >> # define THREADINFO_GFP (GFP_KERNEL | __GFP_NOTRACK) >> #endif >> >> +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM >> +# define THREADINFO_GFP_ACCOUNTED (THREADINFO_GFP | __GFP_KMEMCG) >> +#else >> +# define THREADINFO_GFP_ACCOUNTED (THREADINFO_GFP) >> +#endif >> + > > This type of requirement is going to become nasty very quickly if nobody > can use __GFP_KMEMCG without testing for CONFIG_CGROUP_MEM_RES_CTLR_KMEM. > Perhaps define __GFP_KMEMCG to be 0x0 if it's not enabled, similar to how > kmemcheck does? > That is what I've done in my first version of this patch. At that time, Christoph wanted it to be this way so we would make sure it would never be used with #CONFIG_CGROUP_MEM_RES_CTLR_KMEM defined. A value of zero will generate no errors. Undefined value will. Now, if you ask me, I personally prefer following what kmemcheck does here...
creator: Glauber Costa
date: 2012-06-26T07:23:04-00:00

Re: [PATCH 09/11] memcg: propagate kmem limiting information to children
type: item
description: On 06/26/2012 09:31 AM, Andrew Morton wrote: > On Mon, 25 Jun 2012 22:24:44 -0700 (PDT) David Rientjes <rientjes@google.com> wrote: > >>>> +#define KMEM_ACCOUNTED_THIS 0 >>>> +#define KMEM_ACCOUNTED_PARENT 1 >>> >>> And then document the fields here. >>> >> >> In hex, please? > > Well, they're bit numbers, not masks. Decimal 0-31 is OK, or an enum. > enum it will be.
creator: Glauber Costa
date: 2012-06-26T07:23:43-00:00

Re: [PATCH 03/11] memcg: change defines to an enum
type: item
description: On 06/26/2012 08:11 AM, David Rientjes wrote: > On Mon, 25 Jun 2012, Glauber Costa wrote: > >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 8e601e8..9352d40 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -387,9 +387,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) > > Shouldn't everything that does MEMFILE_TYPE() now be using type > enum res_type rather than int? > If you mean the following three fields, no, since they are masks and operations. If you mean something else, what do you mean ?
creator: Glauber Costa
date: 2012-06-26T08:28:34-00:00

Re: [PATCH 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On 06/26/2012 03:33 AM, Suleiman Souhlal wrote: > On Mon, Jun 25, 2012 at 7:15 AM, Glauber Costa <glommer@parallels.com> wrote: >> From: Suleiman Souhlal <ssouhlal@FreeBSD.org> >> >> mem_cgroup_do_charge() was written before slab 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 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), stay safe with a cond_resched(), >> and make sure not to do it if __GFP_NORETRY. > > The commit description mentions COSTLY_ORDER, but it's not actually > used in the patch. > > -- Suleiman > Yeah, forgot to update the changelog =( But much more importantly, are you still happy with those changes?
creator: Glauber Costa
date: 2012-06-26T08:39:55-00:00

Re: [PATCH 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: On 06/26/2012 12:45 PM, David Rientjes wrote: > On Tue, 26 Jun 2012, Glauber Costa wrote: > >>>> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h >>>> index ccc1899..914ec07 100644 >>>> --- a/include/linux/thread_info.h >>>> +++ b/include/linux/thread_info.h >>>> @@ -61,6 +61,12 @@ extern long do_no_restart_syscall(struct restart_block >>>> *parm); >>>> # define THREADINFO_GFP (GFP_KERNEL | __GFP_NOTRACK) >>>> #endif >>>> >>>> +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM >>>> +# define THREADINFO_GFP_ACCOUNTED (THREADINFO_GFP | __GFP_KMEMCG) >>>> +#else >>>> +# define THREADINFO_GFP_ACCOUNTED (THREADINFO_GFP) >>>> +#endif >>>> + >>> >>> This type of requirement is going to become nasty very quickly if nobody >>> can use __GFP_KMEMCG without testing for CONFIG_CGROUP_MEM_RES_CTLR_KMEM. >>> Perhaps define __GFP_KMEMCG to be 0x0 if it's not enabled, similar to how >>> kmemcheck does? >>> >> That is what I've done in my first version of this patch. At that time, >> Christoph wanted it to be this way so we would make sure it would never be >> used with #CONFIG_CGROUP_MEM_RES_CTLR_KMEM defined. A value of zero will >> generate no errors. Undefined value will. >> >> Now, if you ask me, I personally prefer following what kmemcheck does here... >> > > Right, because I'm sure that __GFP_KMEMCG will be used in additional > places outside of this patchset and it will be a shame if we have to > always add #ifdef's. I see no reason why we would care if __GFP_KMEMCG > was used when CONFIG_CGROUP_MEM_RES_CTLR_KMEM=n with the semantics that it > as in this patchset. It's much cleaner by making it 0x0 when disabled. > What I can do, instead, is to WARN_ON conditionally to the config option in the page allocator, and make sure no one is actually passing the flag in that case.
creator: Glauber Costa
date: 2012-06-26T08:44:13-00:00

Re: [PATCH 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: On Tue, 26 Jun 2012, Glauber Costa wrote: > > > diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h > > > index ccc1899..914ec07 100644 > > > --- a/include/linux/thread_info.h > > > +++ b/include/linux/thread_info.h > > > @@ -61,6 +61,12 @@ extern long do_no_restart_syscall(struct restart_block > > > *parm); > > > # define THREADINFO_GFP (GFP_KERNEL | __GFP_NOTRACK) > > > #endif > > > > > > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM > > > +# define THREADINFO_GFP_ACCOUNTED (THREADINFO_GFP | __GFP_KMEMCG) > > > +#else > > > +# define THREADINFO_GFP_ACCOUNTED (THREADINFO_GFP) > > > +#endif > > > + > > > > This type of requirement is going to become nasty very quickly if nobody > > can use __GFP_KMEMCG without testing for CONFIG_CGROUP_MEM_RES_CTLR_KMEM. > > Perhaps define __GFP_KMEMCG to be 0x0 if it's not enabled, similar to how > > kmemcheck does? > > > That is what I've done in my first version of this patch. At that time, > Christoph wanted it to be this way so we would make sure it would never be > used with #CONFIG_CGROUP_MEM_RES_CTLR_KMEM defined. A value of zero will > generate no errors. Undefined value will. > > Now, if you ask me, I personally prefer following what kmemcheck does here... > Right, because I'm sure that __GFP_KMEMCG will be used in additional places outside of this patchset and it will be a shame if we have to always add #ifdef's. I see no reason why we would care if __GFP_KMEMCG was used when CONFIG_CGROUP_MEM_RES_CTLR_KMEM=n with the semantics that it as in this patchset. It's much cleaner by making it 0x0 when disabled.
creator: David Rientjes
date: 2012-06-26T08:45:30-00:00

Re: [PATCH 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On Tue, 26 Jun 2012, Glauber Costa wrote: > > > + * retries > > > + */ > > > +#define NR_PAGES_TO_RETRY 2 > > > + > > > > Should be 1 << PAGE_ALLOC_COSTLY_ORDER? Where does this number come from? > > The changelog doesn't specify. > > Hocko complained about that, and I changed. Where the number comes from, is > stated in the comments: it is a number small enough to have high changes of > had been freed by the previous reclaim, and yet around the number of pages of > a kernel allocation. > PAGE_ALLOC_COSTLY_ORDER _is_ the threshold used to determine where reclaim and compaction is deemed to be too costly to continuously retry, I'm not sure why this is any different? And this is certainly not "around the number of pages of a kernel allocation", that depends very heavily on the slab allocator being used; slub very often uses order-2 and order-3 page allocations as the default settings (it is capped at, you guessed it, PAGE_ALLOC_COSTLY_ORDER internally by default) and can be significantly increased on the command line. > Of course there are allocations for nr_pages > 2. But 2 will already service > the stack most of the time, and most of the slab caches. > Nope, have you checked the output of /sys/kernel/slab/.../order when running slub? On my workstation 127 out of 316 caches have order-2 or higher by default.
creator: David Rientjes
date: 2012-06-26T08:54:09-00:00

Re: [PATCH 03/11] memcg: change defines to an enum
type: item
description: On Tue, 26 Jun 2012, Glauber Costa wrote: > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > index 8e601e8..9352d40 100644 > > > --- a/mm/memcontrol.c > > > +++ b/mm/memcontrol.c > > > @@ -387,9 +387,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) > > > > Shouldn't everything that does MEMFILE_TYPE() now be using type > > enum res_type rather than int? > > > If you mean the following three fields, no, since they are masks and > operations. > No, I mean everything in mm/memcontrol.c that does int type = MEMFILE_TYPE(...). Why define a non-anonymous enum if you're not going to use its type? Either use enum res_type in place of int or define the enum to be anonymous. It's actually quite effective since gcc will warn if you're using the value of an enum type in your switch() statements later in this series and one of the enum fields is missing (if you avoid using a "default" case statement) if you pass -Wswitch, which is included in -Wall.
creator: David Rientjes
date: 2012-06-26T09:01:54-00:00

Re: [PATCH 05/11] Add a __GFP_KMEMCG flag
type: item
description: On Tue, 26 Jun 2012, Glauber Costa wrote: > > > >This flag is used to indicate to the callees that this allocation will be > > > >serviced to the kernel. It is not supposed to be passed by the callers > > > >of kmem_cache_alloc, but rather by the cache core itself. > > > > > > Not sure what "serviced to the kernel" means, does this mean that the > > memory will not be accounted for to the root memcg? > > > In this context, it means that is a kernel allocation, not a userspace one > (but in process context, of course), *and* it is to be accounted a > specific memcg. > Ah, that makes sense. I think it would help if this was included in the changelog as well as a specifying that it is accounted to current's memcg at the time of the allocation in a comment in the code.
creator: David Rientjes
date: 2012-06-26T09:03:13-00:00

Re: [PATCH 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: On Tue, 26 Jun 2012, Glauber Costa wrote: > > Right, because I'm sure that __GFP_KMEMCG will be used in additional > > places outside of this patchset and it will be a shame if we have to > > always add #ifdef's. I see no reason why we would care if __GFP_KMEMCG > > was used when CONFIG_CGROUP_MEM_RES_CTLR_KMEM=n with the semantics that it > > as in this patchset. It's much cleaner by making it 0x0 when disabled. > > > > What I can do, instead, is to WARN_ON conditionally to the config option in > the page allocator, and make sure no one is actually passing the flag in that > case. > I don't think adding a conditional to the page allocator's fastpath when CONFIG_CGROUP_MEM_RES_CTLR_KMEM=n is appropriate. I don't understand why this can't be 0x0 for such a configuration, __GFP_KMEM certainly means nothing when we don't have it enabled so how is this different at all from kmemcheck?
creator: David Rientjes
date: 2012-06-26T09:05:47-00:00

Re: [PATCH 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On 06/26/2012 12:54 PM, David Rientjes wrote: > On Tue, 26 Jun 2012, Glauber Costa wrote: > >>>> + * retries >>>> + */ >>>> +#define NR_PAGES_TO_RETRY 2 >>>> + >>> >>> Should be 1 << PAGE_ALLOC_COSTLY_ORDER? Where does this number come from? >>> The changelog doesn't specify. >> >> Hocko complained about that, and I changed. Where the number comes from, is >> stated in the comments: it is a number small enough to have high changes of >> had been freed by the previous reclaim, and yet around the number of pages of >> a kernel allocation. >> > > PAGE_ALLOC_COSTLY_ORDER _is_ the threshold used to determine where reclaim > and compaction is deemed to be too costly to continuously retry, I'm not > sure why this is any different? > > And this is certainly not "around the number of pages of a kernel > allocation", that depends very heavily on the slab allocator being used; > slub very often uses order-2 and order-3 page allocations as the default > settings (it is capped at, you guessed it, PAGE_ALLOC_COSTLY_ORDER > internally by default) and can be significantly increased on the command > line. I am obviously okay with either. Maybe Michal can comment on this? >> Of course there are allocations for nr_pages > 2. But 2 will already service >> the stack most of the time, and most of the slab caches. >> > > Nope, have you checked the output of /sys/kernel/slab/.../order when > running slub? On my workstation 127 out of 316 caches have order-2 or > higher by default. > Well, this is still on the side of my argument, since this is still a majority of them being low ordered. The code here does not necessarily have to retry - if I understand it correctly - we just retry for very small allocations because that is where our likelihood of succeeding is.
creator: Glauber Costa
date: 2012-06-26T09:08:48-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: On Mon, 25 Jun 2012, Glauber Costa wrote: > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 83e7ba9..22479eb 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; > @@ -409,6 +410,12 @@ struct sock; > #ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM > void sock_update_memcg(struct sock *sk); > void sock_release_memcg(struct sock *sk); > + > +#define mem_cgroup_kmem_on 1 > +bool __mem_cgroup_new_kmem_page(gfp_t gfp, void *handle, int order); > +void __mem_cgroup_commit_kmem_page(struct page *page, void *handle, int order); > +void __mem_cgroup_free_kmem_page(struct page *page, int order); > +#define is_kmem_tracked_alloc (gfp & __GFP_KMEMCG) > #else > static inline void sock_update_memcg(struct sock *sk) > { > @@ -416,6 +423,43 @@ static inline void sock_update_memcg(struct sock *sk) > static inline void sock_release_memcg(struct sock *sk) > { > } > + > +#define mem_cgroup_kmem_on 0 > +#define __mem_cgroup_new_kmem_page(a, b, c) false > +#define __mem_cgroup_free_kmem_page(a,b ) > +#define __mem_cgroup_commit_kmem_page(a, b, c) > +#define is_kmem_tracked_alloc (false) > #endif /* CONFIG_CGROUP_MEM_RES_CTLR_KMEM */ > + > +static __always_inline > +bool mem_cgroup_new_kmem_page(gfp_t gfp, void *handle, int order) > +{ > + if (!mem_cgroup_kmem_on) > + return true; > + if (!is_kmem_tracked_alloc) > + return true; > + if (!current->mm) > + return true; > + if (in_interrupt()) > + return true; You can't test for current->mm in irq context, so you need to check for in_interrupt() first. Also, what prevents __mem_cgroup_new_kmem_page() from being called for a kthread that has called use_mm() before unuse_mm()? > + if (gfp & __GFP_NOFAIL) > + return true; > + return __mem_cgroup_new_kmem_page(gfp, handle, order); > +} > + > +static __always_inline > …
creator: David Rientjes
date: 2012-06-26T09:12:15-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: On 06/26/2012 01:12 PM, David Rientjes wrote: > On Mon, 25 Jun 2012, Glauber Costa wrote: > >> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h >> index 83e7ba9..22479eb 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; >> @@ -409,6 +410,12 @@ struct sock; >> #ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM >> void sock_update_memcg(struct sock *sk); >> void sock_release_memcg(struct sock *sk); >> + >> +#define mem_cgroup_kmem_on 1 >> +bool __mem_cgroup_new_kmem_page(gfp_t gfp, void *handle, int order); >> +void __mem_cgroup_commit_kmem_page(struct page *page, void *handle, int order); >> +void __mem_cgroup_free_kmem_page(struct page *page, int order); >> +#define is_kmem_tracked_alloc (gfp & __GFP_KMEMCG) >> #else >> static inline void sock_update_memcg(struct sock *sk) >> { >> @@ -416,6 +423,43 @@ static inline void sock_update_memcg(struct sock *sk) >> static inline void sock_release_memcg(struct sock *sk) >> { >> } >> + >> +#define mem_cgroup_kmem_on 0 >> +#define __mem_cgroup_new_kmem_page(a, b, c) false >> +#define __mem_cgroup_free_kmem_page(a,b ) >> +#define __mem_cgroup_commit_kmem_page(a, b, c) >> +#define is_kmem_tracked_alloc (false) >> #endif /* CONFIG_CGROUP_MEM_RES_CTLR_KMEM */ >> + >> +static __always_inline >> +bool mem_cgroup_new_kmem_page(gfp_t gfp, void *handle, int order) >> +{ >> + if (!mem_cgroup_kmem_on) >> + return true; >> + if (!is_kmem_tracked_alloc) >> + return true; >> + if (!current->mm) >> + return true; >> + if (in_interrupt()) >> + return true; > > You can't test for current->mm in irq context, so you need to check for > in_interrupt() first. > Right, thanks. > Also, what prevents __mem_cgroup_new_kmem_page() > from being called for a kthread that has called use_mm() before > unuse_mm()? Nothing, but I…
creator: Glauber Costa
date: 2012-06-26T09:17:37-00:00

Re: [PATCH 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On Tue, 26 Jun 2012, Glauber Costa wrote: > > Nope, have you checked the output of /sys/kernel/slab/.../order when > > running slub? On my workstation 127 out of 316 caches have order-2 or > > higher by default. > > > > Well, this is still on the side of my argument, since this is still a majority > of them being low ordered. Ok, so what happens if I pass slub_min_order=2 on the command line? We never retry? > The code here does not necessarily have to retry - > if I understand it correctly - we just retry for very small allocations > because that is where our likelihood of succeeding is. > Well, the comment for NR_PAGES_TO_RETRY says /* * We need a number that is small enough to be likely to have been * reclaimed even under pressure, but not too big to trigger unnecessary * retries */ and mmzone.h says /* * PAGE_ALLOC_COSTLY_ORDER is the order at which allocations are deemed * costly to service. That is between allocation orders which should * coalesce naturally under reasonable reclaim pressure and those which * will not. */ #define PAGE_ALLOC_COSTLY_ORDER 3 so I'm trying to reconcile which one is correct.
creator: David Rientjes
date: 2012-06-26T09:17:45-00:00

Re: [PATCH 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On 06/26/2012 01:17 PM, David Rientjes wrote: > On Tue, 26 Jun 2012, Glauber Costa wrote: > >>> Nope, have you checked the output of /sys/kernel/slab/.../order when >>> running slub? On my workstation 127 out of 316 caches have order-2 or >>> higher by default. >>> >> >> Well, this is still on the side of my argument, since this is still a majority >> of them being low ordered. > > Ok, so what happens if I pass slub_min_order=2 on the command line? We > never retry? Well, indeed. The function has many other RETRY points, but I believe we'd reach none of them without triggering oom first. >> The code here does not necessarily have to retry - >> if I understand it correctly - we just retry for very small allocations >> because that is where our likelihood of succeeding is. >> > > Well, the comment for NR_PAGES_TO_RETRY says > > /* > * We need a number that is small enough to be likely to have been > * reclaimed even under pressure, but not too big to trigger unnecessary > * retries > */ > > and mmzone.h says > > /* > * PAGE_ALLOC_COSTLY_ORDER is the order at which allocations are deemed > * costly to service. That is between allocation orders which should > * coalesce naturally under reasonable reclaim pressure and those which > * will not. > */ > #define PAGE_ALLOC_COSTLY_ORDER 3 > > so I'm trying to reconcile which one is correct. > I am not myself against reverting back to costly order. The check we have here, of course, is stricter than costly order, so there is nothing to reconcile. Now if we really need a stricter check or not, is the subject of discussion.
creator: Glauber Costa
date: 2012-06-26T09:23:40-00:00

Re: [PATCH 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: On 06/25/2012 10:38 PM, Tejun Heo wrote: > On Mon, Jun 25, 2012 at 06:55:35PM +0200, Frederic Weisbecker wrote: >> On 06/25/2012 04:15 PM, 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> >>> 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: Frederic Weisbecker <fweisbec@redhat.com> > > Frederic, does this (with proper slab accounting added later) achieve > what you wanted with the task counter? > A note: Frederic may confirm, but I think he doesn't even need the slab accounting to follow to achieve that goal.
creator: Glauber Costa
date: 2012-06-26T12:48:08-00:00

Re: [PATCH 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: On 06/26/2012 05:38 PM, Frederic Weisbecker wrote: > On Tue, Jun 26, 2012 at 04:48:08PM +0400, Glauber Costa wrote: >> On 06/25/2012 10:38 PM, Tejun Heo wrote: >>> On Mon, Jun 25, 2012 at 06:55:35PM +0200, Frederic Weisbecker wrote: >>>> On 06/25/2012 04:15 PM, 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> >>>>> 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: Frederic Weisbecker <fweisbec@redhat.com> >>> >>> Frederic, does this (with proper slab accounting added later) achieve >>> what you wanted with the task counter? >>> >> >> A note: Frederic may confirm, but I think he doesn't even need >> the slab accounting to follow to achieve that goal. > > Limiting is enough. But that requires internal accounting. > Yes, but why the *slab* needs to get involved? accounting ta…
creator: Glauber Costa
date: 2012-06-26T13:37:41-00:00

Re: [PATCH 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: On Tue, Jun 26, 2012 at 04:48:08PM +0400, Glauber Costa wrote: > On 06/25/2012 10:38 PM, Tejun Heo wrote: > >On Mon, Jun 25, 2012 at 06:55:35PM +0200, Frederic Weisbecker wrote: > >>On 06/25/2012 04:15 PM, 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> > >>>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: Frederic Weisbecker <fweisbec@redhat.com> > > > >Frederic, does this (with proper slab accounting added later) achieve > >what you wanted with the task counter? > > > > A note: Frederic may confirm, but I think he doesn't even need > the slab accounting to follow to achieve that goal. Limiting is enough. But that requires internal accounting.
creator: Frederic Weisbecker
date: 2012-06-26T13:38:41-00:00

Re: [PATCH 11/11] protect architectures where THREAD_SIZE &gt;= PAGE_SIZE against fork bombs
type: item
description: On Tue, Jun 26, 2012 at 05:37:41PM +0400, Glauber Costa wrote: > On 06/26/2012 05:38 PM, Frederic Weisbecker wrote: > >On Tue, Jun 26, 2012 at 04:48:08PM +0400, Glauber Costa wrote: > >>On 06/25/2012 10:38 PM, Tejun Heo wrote: > >>>On Mon, Jun 25, 2012 at 06:55:35PM +0200, Frederic Weisbecker wrote: > >>>>On 06/25/2012 04:15 PM, 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> > >>>>>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: Frederic Weisbecker <fweisbec@redhat.com> > >>> > >>>Frederic, does this (with proper slab accounting added later) achieve > >>>what you wanted with the task counter? > >>> > >> > >>A note: Frederic may confirm, but I think he doesn't even need > >>the slab accounting to follow to achieve that goal. > > > >L…
creator: Frederic Weisbecker
date: 2012-06-26T13:44:15-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: On 06/26/2012 03:17 AM, Andrew Morton wrote: >> } >> >+ >> >+#define mem_cgroup_kmem_on 0 >> >+#define __mem_cgroup_new_kmem_page(a, b, c) false >> >+#define __mem_cgroup_free_kmem_page(a,b ) >> >+#define __mem_cgroup_commit_kmem_page(a, b, c) > I suggest that the naming consistently follow the model > "mem_cgroup_kmem_foo". So "mem_cgroup_kmem_" becomes the well-known > identifier for this subsystem. > > Then, s/mem_cgroup/memcg/g/ - show us some mercy here! > I always prefer shorter names, but mem_cgroup, and not memcg, seems to be the default for external functions. I am nothing but a follower =)
creator: Glauber Costa
date: 2012-06-26T14:40:45-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: On 06/26/2012 03:17 AM, Andrew Morton wrote: >> + memcg_uncharge_kmem(memcg, size); >> >+ mem_cgroup_put(memcg); >> >+} >> >+EXPORT_SYMBOL(__mem_cgroup_free_kmem_page); >> > #endif /* CONFIG_CGROUP_MEM_RES_CTLR_KMEM */ >> > >> > #if defined(CONFIG_INET) && defined(CONFIG_CGROUP_MEM_RES_CTLR_KMEM) >> >@@ -5645,3 +5751,69 @@ static int __init enable_swap_account(char *s) >> > __setup("swapaccount=", enable_swap_account); >> > >> > #endif >> >+ >> >+#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM > gargh. CONFIG_MEMCG_KMEM, please! > Here too. I like it as much as you do. But that is consistent with the rest of the file, and I'd rather have it this way.
creator: Glauber Costa
date: 2012-06-26T15:01:15-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: On 06/26/2012 03:17 AM, Andrew Morton wrote: >> + if (ret == -EINTR) { >> >+ nofail = true; >> >+ /* >> >+ * __mem_cgroup_try_charge() chose to bypass to root due >> >+ * to OOM kill or fatal signal. > Is "bypass" correct? Maybe "fall back"? > Heh, forgot this one, sorry =( __mem_cgroup_try_charge does "goto bypass", so I believe the term "bypass" is better to allow whoever is following this code to follow it.
creator: Glauber Costa
date: 2012-06-26T15:29:44-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: On Tue, 26 Jun 2012 19:01:15 +0400 Glauber Costa <glommer@parallels.com> wrote: > On 06/26/2012 03:17 AM, Andrew Morton wrote: > >> + memcg_uncharge_kmem(memcg, size); > >> >+ mem_cgroup_put(memcg); > >> >+} > >> >+EXPORT_SYMBOL(__mem_cgroup_free_kmem_page); > >> > #endif /* CONFIG_CGROUP_MEM_RES_CTLR_KMEM */ > >> > > >> > #if defined(CONFIG_INET) && defined(CONFIG_CGROUP_MEM_RES_CTLR_KMEM) > >> >@@ -5645,3 +5751,69 @@ static int __init enable_swap_account(char *s) > >> > __setup("swapaccount=", enable_swap_account); > >> > > >> > #endif > >> >+ > >> >+#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM > > gargh. CONFIG_MEMCG_KMEM, please! > > > > Here too. I like it as much as you do. > > But that is consistent with the rest of the file, and I'd rather have > it this way. There's not much point in being consistent with something which is so unpleasant. I'm on a little campaign to rename CONFIG_CGROUP_MEM_RES_CTLR to CONFIG_MEMCG, only nobody has taken my bait yet. Be first!
creator: akpm
date: 2012-06-26T18:01:42-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: On Tue, Jun 26, 2012 at 11:01:42AM -0700, Andrew Morton wrote: > On Tue, 26 Jun 2012 19:01:15 +0400 Glauber Costa <glommer@parallels.com> wrote: > > > On 06/26/2012 03:17 AM, Andrew Morton wrote: > > >> + memcg_uncharge_kmem(memcg, size); > > >> >+ mem_cgroup_put(memcg); > > >> >+} > > >> >+EXPORT_SYMBOL(__mem_cgroup_free_kmem_page); > > >> > #endif /* CONFIG_CGROUP_MEM_RES_CTLR_KMEM */ > > >> > > > >> > #if defined(CONFIG_INET) && defined(CONFIG_CGROUP_MEM_RES_CTLR_KMEM) > > >> >@@ -5645,3 +5751,69 @@ static int __init enable_swap_account(char *s) > > >> > __setup("swapaccount=", enable_swap_account); > > >> > > > >> > #endif > > >> >+ > > >> >+#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM > > > gargh. CONFIG_MEMCG_KMEM, please! > > > > > > > Here too. I like it as much as you do. > > > > But that is consistent with the rest of the file, and I'd rather have > > it this way. > > There's not much point in being consistent with something which is so > unpleasant. I'm on a little campaign to rename > CONFIG_CGROUP_MEM_RES_CTLR to CONFIG_MEMCG, only nobody has taken my > bait yet. Be first! +1. Block cgroup recently did blkio / blkiocg / blkio_cgroup -> blkcg. Join the cool crowd! :P -- tejun
creator: Tejun Heo
date: 2012-06-26T18:08:05-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: On 06/26/2012 10:01 PM, Andrew Morton wrote: > On Tue, 26 Jun 2012 19:01:15 +0400 Glauber Costa <glommer@parallels.com> wrote: > >> On 06/26/2012 03:17 AM, Andrew Morton wrote: >>>> + memcg_uncharge_kmem(memcg, size); >>>>> + mem_cgroup_put(memcg); >>>>> +} >>>>> +EXPORT_SYMBOL(__mem_cgroup_free_kmem_page); >>>>> #endif /* CONFIG_CGROUP_MEM_RES_CTLR_KMEM */ >>>>> >>>>> #if defined(CONFIG_INET) && defined(CONFIG_CGROUP_MEM_RES_CTLR_KMEM) >>>>> @@ -5645,3 +5751,69 @@ static int __init enable_swap_account(char *s) >>>>> __setup("swapaccount=", enable_swap_account); >>>>> >>>>> #endif >>>>> + >>>>> +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM >>> gargh. CONFIG_MEMCG_KMEM, please! >>> >> >> Here too. I like it as much as you do. >> >> But that is consistent with the rest of the file, and I'd rather have >> it this way. > > There's not much point in being consistent with something which is so > unpleasant. I'm on a little campaign to rename > CONFIG_CGROUP_MEM_RES_CTLR to CONFIG_MEMCG, only nobody has taken my > bait yet. Be first! > If you are okay with a preparation mechanical patch to convert the whole file, I can change mine too. But you'll be responsible for arguing with whoever stepping up opposing this =p
creator: Glauber Costa
date: 2012-06-26T18:14:51-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: On Tue, 26 Jun 2012 22:14:51 +0400 Glauber Costa <glommer@parallels.com> wrote: > On 06/26/2012 10:01 PM, Andrew Morton wrote: > > On Tue, 26 Jun 2012 19:01:15 +0400 Glauber Costa <glommer@parallels.com> wrote: > > > >> On 06/26/2012 03:17 AM, Andrew Morton wrote: > >>>> + memcg_uncharge_kmem(memcg, size); > >>>>> + mem_cgroup_put(memcg); > >>>>> +} > >>>>> +EXPORT_SYMBOL(__mem_cgroup_free_kmem_page); > >>>>> #endif /* CONFIG_CGROUP_MEM_RES_CTLR_KMEM */ > >>>>> > >>>>> #if defined(CONFIG_INET) && defined(CONFIG_CGROUP_MEM_RES_CTLR_KMEM) > >>>>> @@ -5645,3 +5751,69 @@ static int __init enable_swap_account(char *s) > >>>>> __setup("swapaccount=", enable_swap_account); > >>>>> > >>>>> #endif > >>>>> + > >>>>> +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM > >>> gargh. CONFIG_MEMCG_KMEM, please! > >>> > >> > >> Here too. I like it as much as you do. > >> > >> But that is consistent with the rest of the file, and I'd rather have > >> it this way. > > > > There's not much point in being consistent with something which is so > > unpleasant. I'm on a little campaign to rename > > CONFIG_CGROUP_MEM_RES_CTLR to CONFIG_MEMCG, only nobody has taken my > > bait yet. Be first! > > > > If you are okay with a preparation mechanical patch to convert the whole > file, I can change mine too. > > But you'll be responsible for arguing with whoever stepping up opposing > this =p > From: Andrew Morton <akpm@linux-foundation.org> Subject: memcg: rename config variables Sanity: CONFIG_CGROUP_MEM_RES_CTLR -> CONFIG_MEMCG CONFIG_CGROUP_MEM_RES_CTLR_SWAP -> CONFIG_MEMCG_SWAP CONFIG_CGROUP_MEM_RES_CTLR_SWAP_ENABLED -> CONFIG_MEMCG_SWAP_ENABLED CONFIG_CGROUP_MEM_RES_CTLR_KMEM -> CONFIG_MEMCG_KMEM Cc: Glauber Costa <glommer@parallels.com> Cc: Michal Hocko <mhocko@suse.cz> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Cc: Hugh Dickins <hughd@google.com> Cc: Tejun Heo <tj@kernel.org> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> Cc: David Rientjes…
creator: akpm
date: 2012-06-26T19:20:50-00:00

Re: [PATCH 00/11] kmem controller for memcg: stripped down version
type: item
description: On Tue, 26 Jun 2012 11:17:49 +0400 Glauber Costa <glommer@parallels.com> wrote: > On 06/26/2012 03:27 AM, Andrew Morton wrote: > > On Mon, 25 Jun 2012 18:15:17 +0400 > > Glauber Costa <glommer@parallels.com> wrote: > > > >> What I am proposing with this series is a stripped down version of the > >> kmem controller for memcg that would allow us to merge significant parts > >> of the infrastructure, while leaving out, for now, the polemic bits about > >> the slab while it is being reworked by Cristoph. > >> > >> Me reasoning for that is that after the last change to introduce a gfp > >> flag to mark kernel allocations, it became clear to me that tracking other > >> resources like the stack would then follow extremely naturaly. I figured > >> that at some point we'd have to solve the issue pointed by David, and avoid > >> testing the Slab flag in the page allocator, since it would soon be made > >> more generic. I do that by having the callers to explicit mark it. > >> > >> So to demonstrate how it would work, I am introducing a stack tracker here, > >> that is already a functionality per-se: it successfully stops fork bombs to > >> happen. (Sorry for doing all your work, Frederic =p ). Note that after all > >> memcg infrastructure is deployed, it becomes very easy to track anything. > >> The last patch of this series is extremely simple. > >> > >> The infrastructure is exactly the same we had in memcg, but stripped down > >> of the slab parts. And because what we have after those patches is a feature > >> per-se, I think it could be considered for merging. > > > > hm. None of this new code makes the kernel smaller, faster, easier to > > understand or more fun to read! > Not sure if this is a general comment - in case I agree - or if targeted > to my statement that this is "stripped down". If so, it is of course > smaller relative to my previous slab accounting patches. It's a general comment. The patch adds overhead: runtime costs and maintenance costs. Do its benefit…
creator: akpm
date: 2012-06-26T21:55:39-00:00

Re: [PATCH 00/11] kmem controller for memcg: stripped down version
type: item
description: On Tue, 26 Jun 2012, Andrew Morton wrote: > mm, maybe. Kernel developers tend to look at code from the point of > view "does it work as designed", "is it clean", "is it efficient", "do > I understand it", etc. We often forget to step back and really > consider whether or not it should be merged at all. > It's appropriate for true memory isolation so that applications cannot cause an excess of slab to be consumed. This allows other applications to have higher reservations without the risk of incurring a global oom condition as the result of the usage of other memcgs. I'm not sure whether it would ever be appropriate to limit the amount of slab for an individual slab cache, however, instead of limiting the sum of all slab for a set of processes. With cache merging in slub this would seem to be difficult to do correctly.
creator: David Rientjes
date: 2012-06-27T01:08:13-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: On Tue, 26 Jun 2012, Glauber Costa wrote: > > > @@ -416,6 +423,43 @@ static inline void sock_update_memcg(struct sock *sk) > > > static inline void sock_release_memcg(struct sock *sk) > > > { > > > } > > > + > > > +#define mem_cgroup_kmem_on 0 > > > +#define __mem_cgroup_new_kmem_page(a, b, c) false > > > +#define __mem_cgroup_free_kmem_page(a,b ) > > > +#define __mem_cgroup_commit_kmem_page(a, b, c) > > > +#define is_kmem_tracked_alloc (false) > > > #endif /* CONFIG_CGROUP_MEM_RES_CTLR_KMEM */ > > > + > > > +static __always_inline > > > +bool mem_cgroup_new_kmem_page(gfp_t gfp, void *handle, int order) > > > +{ > > > + if (!mem_cgroup_kmem_on) > > > + return true; > > > + if (!is_kmem_tracked_alloc) > > > + return true; > > > + if (!current->mm) > > > + return true; > > > + if (in_interrupt()) > > > + return true; > > > > You can't test for current->mm in irq context, so you need to check for > > in_interrupt() first. > > > Right, thanks. > > > Also, what prevents __mem_cgroup_new_kmem_page() > > from being called for a kthread that has called use_mm() before > > unuse_mm()? > > Nothing, but I also don't see how to prevent that. You can test for current->flags & PF_KTHREAD following the check for in_interrupt() and return true, it's what you were trying to do with the check for !current->mm.
creator: David Rientjes
date: 2012-06-27T04:01:32-00:00

Re: [PATCH 00/11] kmem controller for memcg: stripped down version
type: item
description: On 06/27/2012 05:08 AM, David Rientjes wrote: > On Tue, 26 Jun 2012, Andrew Morton wrote: > >> mm, maybe. Kernel developers tend to look at code from the point of >> view "does it work as designed", "is it clean", "is it efficient", "do >> I understand it", etc. We often forget to step back and really >> consider whether or not it should be merged at all. >> > > It's appropriate for true memory isolation so that applications cannot > cause an excess of slab to be consumed. This allows other applications to > have higher reservations without the risk of incurring a global oom > condition as the result of the usage of other memcgs. Just a note for Andrew, we we're in the same page: The slab cache limitation is not included in *this* particular series. The goal was always to have other kernel resources limited as well, and the general argument from David holds: we want a set of applications to run truly independently from others, without creating memory pressure on the global system. The way history develop in this series, I started from the slab cache, and a page-level tracking appeared on that series. I then figured it would be better to start tracking something that is totally page-based, such as the stack - that already accounts for 70 % of the infrastructure, and then merge the slab code later. In this sense, it was just a strategy inversion. But both are, and were, in the goals. > I'm not sure whether it would ever be appropriate to limit the amount of > slab for an individual slab cache, however, instead of limiting the sum of > all slab for a set of processes. With cache merging in slub this would > seem to be difficult to do correctly. Yes, I do agree.
creator: Glauber Costa
date: 2012-06-27T08:39:54-00:00

Fork bomb limitation in memcg WAS: Re: [PATCH 00/11] kmem controller for memcg: stripped down versio
type: item
description: On 06/27/2012 01:55 AM, Andrew Morton wrote: > On Tue, 26 Jun 2012 11:17:49 +0400 > Glauber Costa <glommer@parallels.com> wrote: > >> On 06/26/2012 03:27 AM, Andrew Morton wrote: >>> On Mon, 25 Jun 2012 18:15:17 +0400 >>> Glauber Costa <glommer@parallels.com> wrote: >>> >>>> What I am proposing with this series is a stripped down version of the >>>> kmem controller for memcg that would allow us to merge significant parts >>>> of the infrastructure, while leaving out, for now, the polemic bits about >>>> the slab while it is being reworked by Cristoph. >>>> >>>> Me reasoning for that is that after the last change to introduce a gfp >>>> flag to mark kernel allocations, it became clear to me that tracking other >>>> resources like the stack would then follow extremely naturaly. I figured >>>> that at some point we'd have to solve the issue pointed by David, and avoid >>>> testing the Slab flag in the page allocator, since it would soon be made >>>> more generic. I do that by having the callers to explicit mark it. >>>> >>>> So to demonstrate how it would work, I am introducing a stack tracker here, >>>> that is already a functionality per-se: it successfully stops fork bombs to >>>> happen. (Sorry for doing all your work, Frederic =p ). Note that after all >>>> memcg infrastructure is deployed, it becomes very easy to track anything. >>>> The last patch of this series is extremely simple. >>>> >>>> The infrastructure is exactly the same we had in memcg, but stripped down >>>> of the slab parts. And because what we have after those patches is a feature >>>> per-se, I think it could be considered for merging. >>> >>> hm. None of this new code makes the kernel smaller, faster, easier to >>> understand or more fun to read! >> Not sure if this is a general comment - in case I agree - or if targeted >> to my statement that this is "stripped down". If so, it is of course >> smaller relative to my previous slab accounting patches. > > It's a general comment. The patch adds ov…
creator: Glauber Costa
date: 2012-06-27T09:29:04-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: >> >> Nothing, but I also don't see how to prevent that. > > You can test for current->flags & PF_KTHREAD following the check for > in_interrupt() and return true, it's what you were trying to do with the > check for !current->mm. > am I right to believe that if not in interrupt context - already ruled out - and !(current->flags & PF_KTHREAD), I am guaranteed to have a mm context, and thus, don't need to test against it ?
creator: Glauber Costa
date: 2012-06-27T09:33:05-00:00

Re: [PATCH 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On 06/26/2012 08:09 AM, David Rientjes wrote: > @@ -2206,7 +2214,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 <= NR_PAGES_TO_RETRY && ret) >> return CHARGE_RETRY; Changed to costly order. One more thing. The original version of this patch included a cond_resched() here, that was also removed. From my re-reading of the code in page_alloc.c and vmscan.c now, I tend to think this is indeed not needed, since any cond_resched()s that might be needed to ensure the safety of the code will be properly inserted by the reclaim code itself, so there is no need for us to include any when we signal that a retry is needed. Do you/others agree?
creator: Glauber Costa
date: 2012-06-27T10:03:01-00:00

Re: Fork bomb limitation in memcg WAS: Re: [PATCH 00/11] kmem controller for memcg: stripped down ve
type: item
description: On 06/27/2012 04:29 PM, Frederic Weisbecker wrote: > On Wed, Jun 27, 2012 at 01:29:04PM +0400, Glauber Costa wrote: >> On 06/27/2012 01:55 AM, Andrew Morton wrote: >>>> I can't speak for everybody here, but AFAIK, tracking the stack through >>>> the memory it used, therefore using my proposed kmem controller, was an >>>> idea that good quite a bit of traction with the memcg/memory people. >>>> So here you have something that people already asked a lot for, in a >>>> shape and interface that seem to be acceptable. >>> >>> mm, maybe. Kernel developers tend to look at code from the point of >>> view "does it work as designed", "is it clean", "is it efficient", "do >>> I understand it", etc. We often forget to step back and really >>> consider whether or not it should be merged at all. >>> >>> I mean, unless the code is an explicit simplification, we should have >>> a very strong bias towards "don't merge". >> >> Well, simplifications are welcome - this series itself was >> simplified beyond what I thought initially possible through the >> valuable comments >> of other people. >> >> But of course, this adds more complexity to the kernel as a whole. >> And this is true to every single new feature we may add, now or in >> the >> future. >> >> What I can tell you about this particular one, is that the justification >> for it doesn't come out of nowhere, but from a rather real use case that >> we support and maintain in OpenVZ and our line of products for years. > > Right and we really need a solution to protect against forkbombs in LXC. Small correction: In containers. LXC is not the only one out there =p
creator: Glauber Costa
date: 2012-06-27T12:28:14-00:00

Re: Fork bomb limitation in memcg WAS: Re: [PATCH 00/11] kmem controller for memcg: stripped down ve
type: item
description: On Wed, Jun 27, 2012 at 01:29:04PM +0400, Glauber Costa wrote: > On 06/27/2012 01:55 AM, Andrew Morton wrote: > >>I can't speak for everybody here, but AFAIK, tracking the stack through > >>the memory it used, therefore using my proposed kmem controller, was an > >>idea that good quite a bit of traction with the memcg/memory people. > >>So here you have something that people already asked a lot for, in a > >>shape and interface that seem to be acceptable. > > > >mm, maybe. Kernel developers tend to look at code from the point of > >view "does it work as designed", "is it clean", "is it efficient", "do > >I understand it", etc. We often forget to step back and really > >consider whether or not it should be merged at all. > > > >I mean, unless the code is an explicit simplification, we should have > >a very strong bias towards "don't merge". > > Well, simplifications are welcome - this series itself was > simplified beyond what I thought initially possible through the > valuable comments > of other people. > > But of course, this adds more complexity to the kernel as a whole. > And this is true to every single new feature we may add, now or in > the > future. > > What I can tell you about this particular one, is that the justification > for it doesn't come out of nowhere, but from a rather real use case that > we support and maintain in OpenVZ and our line of products for years. Right and we really need a solution to protect against forkbombs in LXC. The task counter was more simple but only useful for our usecase and defining the number of tasks as a resource was considered unnatural. So limiting kernel stack allocations works for us. This patchset implements this so I'm happy with it. If this is more broadly useful by limiting resources others are interested in, that's even better. I doubt we are interested in a solution that only concerns kernel stack allocation.
creator: Frederic Weisbecker
date: 2012-06-27T12:29:27-00:00

Re: Fork bomb limitation in memcg WAS: Re: [PATCH 00/11] kmem controller for memcg: stripped down ve
type: item
description: On Wed, Jun 27, 2012 at 04:28:14PM +0400, Glauber Costa wrote: > On 06/27/2012 04:29 PM, Frederic Weisbecker wrote: > > On Wed, Jun 27, 2012 at 01:29:04PM +0400, Glauber Costa wrote: > >> On 06/27/2012 01:55 AM, Andrew Morton wrote: > >>>> I can't speak for everybody here, but AFAIK, tracking the stack through > >>>> the memory it used, therefore using my proposed kmem controller, was an > >>>> idea that good quite a bit of traction with the memcg/memory people. > >>>> So here you have something that people already asked a lot for, in a > >>>> shape and interface that seem to be acceptable. > >>> > >>> mm, maybe. Kernel developers tend to look at code from the point of > >>> view "does it work as designed", "is it clean", "is it efficient", "do > >>> I understand it", etc. We often forget to step back and really > >>> consider whether or not it should be merged at all. > >>> > >>> I mean, unless the code is an explicit simplification, we should have > >>> a very strong bias towards "don't merge". > >> > >> Well, simplifications are welcome - this series itself was > >> simplified beyond what I thought initially possible through the > >> valuable comments > >> of other people. > >> > >> But of course, this adds more complexity to the kernel as a whole. > >> And this is true to every single new feature we may add, now or in > >> the > >> future. > >> > >> What I can tell you about this particular one, is that the justification > >> for it doesn't come out of nowhere, but from a rather real use case that > >> we support and maintain in OpenVZ and our line of products for years. > > > > Right and we really need a solution to protect against forkbombs in LXC. > Small correction: In containers. LXC is not the only one out there =p Sure. I was just speaking for the specific project I'm working on :) But I'm definetly interested in solutions that work for everyone in containers in general. And if Openvz is also interested in forkbombs protection that's even better.
creator: Frederic Weisbecker
date: 2012-06-27T12:35:47-00:00

Re: [PATCH 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On Tue, Jun 26, 2012 at 1:39 AM, Glauber Costa <glommer@parallels.com> wrote: > Yeah, forgot to update the changelog =( > > But much more importantly, are you still happy with those changes? Yes, I am OK with those changes. Thanks, -- Suleiman
creator: Suleiman Souhlal
date: 2012-06-27T16:16:51-00:00

Re: Fork bomb limitation in memcg WAS: Re: [PATCH 00/11] kmem controller for memcg: stripped down ve
type: item
description: On Wed, 27 Jun 2012, Glauber Costa wrote: > fork bombs are a way bad behaved processes interfere with the rest of > the system. In here, I propose fork bomb stopping as a natural > consequence of the fact that the amount of kernel memory can be limited, > and each process uses 1 or 2 pages for the stack, that are freed when the > process goes away. > The obvious disadvantage is that if you use the full-featured kmem controller that builds upon this patchset, then you're limiting the about of all kmem, not just the stack that this particular set limits. I hope you're not proposing it to go upstream before full support for the kmem controller is added so that users who use it only to protect again forkbombs soon realize that's no longer possible if your applications do any substantial slab allocations, particularly anything that does a lot of I/O. In other words, if I want to run netperf in a memcg with the full-featured kmem controller enabled, then its kmem limit must be high enough so that it doesn't degrade performance that any limitation on stack allocations would be too high to effectively stop forkbombs.
creator: David Rientjes
date: 2012-06-27T19:38:54-00:00

Re: [PATCH 06/11] memcg: kmem controller infrastructure
type: item
description: On Wed, 27 Jun 2012, Glauber Costa wrote: > > > Nothing, but I also don't see how to prevent that. > > > > You can test for current->flags & PF_KTHREAD following the check for > > in_interrupt() and return true, it's what you were trying to do with the > > check for !current->mm. > > > > am I right to believe that if not in interrupt context - already ruled out - > and !(current->flags & PF_KTHREAD), I am guaranteed to have a mm context, and > thus, don't need to test against it ? > No, because an mm may have been detached in the exit path by running exit_mm(). We'd certainly hope that there are no slab allocations following that point, though, but you'd still need to test current->mm.
creator: David Rientjes
date: 2012-06-27T19:46:08-00:00

Re: [PATCH 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On Wed, 27 Jun 2012, Glauber Costa wrote: > > @@ -2206,7 +2214,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 <= NR_PAGES_TO_RETRY && ret) > > > return CHARGE_RETRY; > > Changed to costly order. > 1 << PAGE_ALLOC_COSTLY_ORDER was the suggestion. > One more thing. The original version of this patch included > a cond_resched() here, that was also removed. From my re-reading > of the code in page_alloc.c and vmscan.c now, I tend to think > this is indeed not needed, since any cond_resched()s that might > be needed to ensure the safety of the code will be properly > inserted by the reclaim code itself, so there is no need for us > to include any when we signal that a retry is needed. > For __GFP_WAIT, that sounds like a safe guarantee.
creator: David Rientjes
date: 2012-06-27T19:48:41-00:00

Re: [PATCH 02/11] memcg: Reclaim when more than one page needed.
type: item
description: On 06/27/2012 11:48 PM, David Rientjes wrote: > On Wed, 27 Jun 2012, Glauber Costa wrote: > >>> @@ -2206,7 +2214,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 <= NR_PAGES_TO_RETRY && ret) >>>> return CHARGE_RETRY; >> >> Changed to costly order. >> > > 1 << PAGE_ALLOC_COSTLY_ORDER was the suggestion. That is what I meant - to the costly order suggestion - , should have been more explicit. >> One more thing. The original version of this patch included >> a cond_resched() here, that was also removed. From my re-reading >> of the code in page_alloc.c and vmscan.c now, I tend to think >> this is indeed not needed, since any cond_resched()s that might >> be needed to ensure the safety of the code will be properly >> inserted by the reclaim code itself, so there is no need for us >> to include any when we signal that a retry is needed. >> > > For __GFP_WAIT, that sounds like a safe guarantee. >
creator: Glauber Costa
date: 2012-06-27T20:47:24-00:00

Re: Fork bomb limitation in memcg WAS: Re: [PATCH 00/11] kmem controller for memcg: stripped down ve
type: item
description: On 06/27/2012 11:38 PM, David Rientjes wrote: > On Wed, 27 Jun 2012, Glauber Costa wrote: > >> fork bombs are a way bad behaved processes interfere with the rest of >> the system. In here, I propose fork bomb stopping as a natural >> consequence of the fact that the amount of kernel memory can be limited, >> and each process uses 1 or 2 pages for the stack, that are freed when the >> process goes away. >> > > The obvious disadvantage is that if you use the full-featured kmem > controller that builds upon this patchset, then you're limiting the about > of all kmem, not just the stack that this particular set limits. I hope > you're not proposing it to go upstream before full support for the kmem > controller is added so that users who use it only to protect again > forkbombs soon realize that's no longer possible if your applications do > any substantial slab allocations, particularly anything that does a lot of > I/O. Point by point: 1) This is not a disadvantage. The whole point of implementing it as kmem, not as a "fork controller" or anything in the like, was our understanding that people should not be "protecting against a x number of processes". Because this is unnatural. All you have is total of kernel memory, because that's what the interface gives you. If you can overuse memory, you can't fork bomb. But that's a consequence. I'll grant that maybe it was a mistake of mine to try to sell it this way here, because it may give the wrong idea. In this case I welcome your comment because it will allow me to be more careful in future communications. But this was just me trying to show off the feature. 2) No admin should never, ever tune the system to a particular kmem limit value. And again, this is the whole point of not limiting "number of processes". I agree that adding slab tracking will raise kernel memory consumption by a reasonable amount. But reality is that even if this controller is totally stable, that not only can, but will happen. Unlike user memory, w…
creator: Glauber Costa
date: 2012-06-28T09:01:23-00:00

Re: Fork bomb limitation in memcg WAS: Re: [PATCH 00/11] kmem controller for memcg: stripped down ve
type: item
description: On Thu, 28 Jun 2012 13:01:23 +0400 Glauber Costa <glommer@parallels.com> wrote: > > ... > OK, that all sounds convincing ;) Please summarise and capture this discussion in the [patch 0/n] changelog so we (or others) don't have to go through this all again. And let's remember this in the next patchset! > Last, but not least, note that it is totally within my interests to > merge the slab tracking as fast as we can. it'll be a matter of going > back to it, and agreeing in the final form. Yes, I'd very much like to have the whole slab implementation in a reasonably mature state before proceeding too far with this base patchset.
creator: akpm
date: 2012-06-28T22:25:40-00:00

Re: Fork bomb limitation in memcg WAS: Re: [PATCH 00/11] kmem controller for memcg: stripped down ve
type: item
description: On 06/29/2012 02:25 AM, Andrew Morton wrote: > On Thu, 28 Jun 2012 13:01:23 +0400 > Glauber Costa <glommer@parallels.com> wrote: > >> >> ... >> > > OK, that all sounds convincing ;) Please summarise and capture this > discussion in the [patch 0/n] changelog so we (or others) don't have to > go through this all again. And let's remember this in the next > patchset! Thanks, will surely do. >> Last, but not least, note that it is totally within my interests to >> merge the slab tracking as fast as we can. it'll be a matter of going >> back to it, and agreeing in the final form. > > Yes, I'd very much like to have the whole slab implementation in a > reasonably mature state before proceeding too far with this base > patchset. Does that means that you want to merge them together? I am more than happy to post the slab part again ontop of that to have people reviewing it. But if possible, I believe that merging this part first would help us to split up testing in a beneficial way, in the sense that if it breaks, we know at least in which part it is. Not to mention, of course, that reviewers will have an easier time reviewing it as two pieces.
creator: Glauber Costa
date: 2012-07-03T11:38:39-00:00

Re: Fork bomb limitation in memcg WAS: Re: [PATCH 00/11] kmem controller for memcg: stripped down ve
type: item
description: On Tue, Jul 03, 2012 at 03:38:39PM +0400, Glauber Costa wrote: > On 06/29/2012 02:25 AM, Andrew Morton wrote: > > On Thu, 28 Jun 2012 13:01:23 +0400 > > Glauber Costa <glommer@parallels.com> wrote: > > > >> > >> ... > >> > > > > OK, that all sounds convincing ;) Please summarise and capture this > > discussion in the [patch 0/n] changelog so we (or others) don't have to > > go through this all again. And let's remember this in the next > > patchset! > > Thanks, will surely do. > > >> Last, but not least, note that it is totally within my interests to > >> merge the slab tracking as fast as we can. it'll be a matter of going > >> back to it, and agreeing in the final form. > > > > Yes, I'd very much like to have the whole slab implementation in a > > reasonably mature state before proceeding too far with this base > > patchset. > > Does that means that you want to merge them together? I am more than > happy to post the slab part again ontop of that to have people reviewing it. > > But if possible, I believe that merging this part first would help us to > split up testing in a beneficial way, in the sense that if it breaks, we > know at least in which part it is. Not to mention, of course, that > reviewers will have an easier time reviewing it as two pieces. Definetly yeah. This makes the review easier for this tricky chunk.
creator: Frederic Weisbecker
date: 2012-07-12T15:40:11-00:00

Re: Fork bomb limitation in memcg WAS: Re: [PATCH 00/11] kmem controller for memcg: stripped down ve
type: item
description: On 06/29/2012 02:25 AM, Andrew Morton wrote: > On Thu, 28 Jun 2012 13:01:23 +0400 > Glauber Costa <glommer@parallels.com> wrote: > >> >> ... >> > > OK, that all sounds convincing ;) Please summarise and capture this > discussion in the [patch 0/n] changelog so we (or others) don't have to > go through this all again. And let's remember this in the next > patchset! > >> Last, but not least, note that it is totally within my interests to >> merge the slab tracking as fast as we can. it'll be a matter of going >> back to it, and agreeing in the final form. > > Yes, I'd very much like to have the whole slab implementation in a > reasonably mature state before proceeding too far with this base > patchset. > So, that was posted separately as well. Although there is a thing to fix here and there - all of them I am working on already - I believe that to be mature enough. Do you have any comments on that? Would you be willing to take this first part (modified with the comments on this thread itself) and let it start sitting in the tree? Thanks!
creator: Glauber Costa
date: 2012-08-07T13:59:25-00:00

Re: Fork bomb limitation in memcg WAS: Re: [PATCH 00/11] kmem controller for memcg: stripped down ve
type: item
description: On 08/07/2012 05:59 PM, Glauber Costa wrote: > On 06/29/2012 02:25 AM, Andrew Morton wrote: >> On Thu, 28 Jun 2012 13:01:23 +0400 >> Glauber Costa <glommer@parallels.com> wrote: >> >>> >>> ... >>> >> >> OK, that all sounds convincing ;) Please summarise and capture this >> discussion in the [patch 0/n] changelog so we (or others) don't have to >> go through this all again. And let's remember this in the next >> patchset! >> >>> Last, but not least, note that it is totally within my interests to >>> merge the slab tracking as fast as we can. it'll be a matter of going >>> back to it, and agreeing in the final form. >> >> Yes, I'd very much like to have the whole slab implementation in a >> reasonably mature state before proceeding too far with this base >> patchset. >> > So, that was posted separately as well. > > Although there is a thing to fix here and there - all of them I am > working on already - I believe that to be mature enough. > > Do you have any comments on that? Would you be willing to take this > first part (modified with the comments on this thread itself) and let it > start sitting in the tree? > In the mean time, for any interested parties, I've set up a tree at: git://github.com/glommer/linux.git branches kmemcg-slab and kmemcg-stack Intended to be a throw-away tree.
creator: Glauber Costa
date: 2012-08-08T14:15:42-00:00