OpenVZ Forum - RDF feed
type: channel
items: [PATCH v7 00/10] Request for Inclusion: per-cgroup tcp memory pressure
items: [PATCH v7 01/10] Basic kernel memory functionality for the Memory Controller
items: [PATCH v7 02/10] foundations of per-cgroup memory pressure controlling.
items: [PATCH v7 03/10] socket: initial cgroup code.
items: [PATCH v7 04/10] tcp memory pressure controls
items: [PATCH v7 05/10] per-netns ipv4 sysctl_tcp_mem
items: [PATCH v7 06/10] tcp buffer limitation: per-cgroup limit
items: [PATCH v7 07/10] Display current tcp memory allocation in kmem cgroup
items: [PATCH v7 08/10] Display current tcp failcnt in kmem cgroup
items: [PATCH v7 09/10] Display maximum tcp memory allocation in kmem cgroup
items: [PATCH v7 10/10] Disable task moving when using kernel memory accounting
items: Re: [PATCH v7 02/10] foundations of per-cgroup memory pressure controlling.
items: Re: [PATCH v7 01/10] Basic kernel memory functionality for the Memory Controller
items: Re: [PATCH v7 03/10] socket: initial cgroup code.
items: Re: [PATCH v7 04/10] tcp memory pressure controls
items: Re: [PATCH v7 06/10] tcp buffer limitation: per-cgroup limit
items: Re: [PATCH v7 08/10] Display current tcp failcnt in kmem cgroup
items: Re: [PATCH v7 09/10] Display maximum tcp memory allocation in kmem cgroup
items: Re: [PATCH v7 00/10] Request for Inclusion: per-cgroup tcp memory pressure
items: Re: [PATCH v7 10/10] Disable task moving when using kernel memory accounting

[PATCH v7 00/10] Request for Inclusion: per-cgroup tcp memory pressure
type: item
description: Hi, This patchset implements per-cgroup tcp memory pressure controls. It did not change significantly since last submission: rather, it just merges the comments Kame had. Most of them are style-related and/or Documentation, but there are two real bugs he managed to spot (thanks) Please let me know if there is anything else I should address. Glauber Costa (10): Basic kernel memory functionality for the Memory Controller foundations of per-cgroup memory pressure controlling. socket: initial cgroup code. tcp memory pressure controls per-netns ipv4 sysctl_tcp_mem tcp buffer limitation: per-cgroup limit Display current tcp memory allocation in kmem cgroup Display current tcp failcnt in kmem cgroup Display maximum tcp memory allocation in kmem cgroup Disable task moving when using kernel memory accounting Documentation/cgroups/memory.txt | 52 +++++++- include/linux/memcontrol.h | 19 +++ include/net/netns/ipv4.h | 1 + include/net/sock.h | 298 +++++++++++++++++++++++++++++++++++++- include/net/tcp.h | 4 +- include/net/tcp_memcontrol.h | 19 +++ init/Kconfig | 14 ++ mm/memcontrol.c | 202 ++++++++++++++++++++++++-- net/core/sock.c | 120 +++++++++++---- net/ipv4/Makefile | 1 + net/ipv4/af_inet.c | 2 + net/ipv4/proc.c | 7 +- net/ipv4/sysctl_net_ipv4.c | 65 ++++++++- net/ipv4/tcp.c | 11 +- net/ipv4/tcp_input.c | 12 +- net/ipv4/tcp_ipv4.c | 13 +- net/ipv4/tcp_memcontrol.c | 268 ++++++++++++++++++++++++++++++++++ net/ipv4/tcp_output.c | 2 +- net/ipv4/tcp_timer.c | 2 +- net/ipv6/af_inet6.c | 2 + net/ipv6/tcp_ipv6.c | 7 +- 21 files changed, 1036 insertions(+), 85 deletions(-) create mode 100644 include/net/tcp_memcontrol.h create mode 100644 net/ipv4/tcp_memcontrol.c -- 1.7.6.4
creator: Glauber Costa
date: 2011-11-29T23:56:51-00:00

[PATCH v7 01/10] Basic kernel memory functionality for the Memory Controller
type: item
description: This patch lays down the foundation for the kernel memory component of the Memory Controller. As of today, I am only laying down the following files: * memory.independent_kmem_limit * memory.kmem.limit_in_bytes (currently ignored) * memory.kmem.usage_in_bytes (always zero) Signed-off-by: Glauber Costa <glommer@parallels.com> Reviewed-by: Kirill A. Shutemov <kirill@shutemov.name> CC: Paul Menage <paul@paulmenage.org> CC: Greg Thelen <gthelen@google.com> --- Documentation/cgroups/memory.txt | 46 ++++++++++++++++- init/Kconfig | 14 +++++ mm/memcontrol.c | 103 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 156 insertions(+), 7 deletions(-) diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt index cc0ebc5..764781c 100644 --- a/Documentation/cgroups/memory.txt +++ b/Documentation/cgroups/memory.txt @@ -44,8 +44,9 @@ Features: - oom-killer disable knob and oom-notifier - Root cgroup has no limit controls. - Kernel memory and Hugepages are not under control yet. We just manage - pages on LRU. To add more controls, we have to take care of performance. + Hugepages is not under control yet. We just manage pages on LRU. To add more + controls, we have to take care of performance. Kernel memory support is work + in progress, and the current version provides basically functionality. Brief summary of control files. @@ -56,8 +57,11 @@ Brief summary of control files. (See 5.5 for details) memory.memsw.usage_in_bytes # show current res_counter usage for memory+Swap (See 5.5 for details) + memory.kmem.usage_in_bytes # show current res_counter usage for kmem only. + (See 2.7 for details) memory.limit_in_bytes # set/show limit of memory usage memory.memsw.limit_in_bytes # set/show limit of memory+Swap usage + memory.kmem.limit_in_bytes # if allowed, set/show limit of kernel memory memory.failcnt # show the number of memory usage hits limits memory.memsw.failcnt # show the number of memory+Swap hits limits memory.max_usage_in_bytes # show max memory …
creator: Glauber Costa
date: 2011-11-29T23:56:52-00:00

[PATCH v7 02/10] foundations of per-cgroup memory pressure controlling.
type: item
description: This patch replaces all uses of struct sock fields' memory_pressure, memory_allocated, sockets_allocated, and sysctl_mem to acessor macros. Those macros can either receive a socket argument, or a mem_cgroup argument, depending on the context they live in. Since we're only doing a macro wrapping here, no performance impact at all is expected in the case where we don't have cgroups disabled. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: David S. Miller <davem@davemloft.net> CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> CC: Eric W. Biederman <ebiederm@xmission.com> CC: Eric Dumazet <eric.dumazet@gmail.com> --- include/net/sock.h | 96 ++++++++++++++++++++++++++++++++++++++++++++++++- include/net/tcp.h | 3 +- net/core/sock.c | 62 +++++++++++++++++++------------- net/ipv4/proc.c | 7 ++-- net/ipv4/tcp_input.c | 12 +++--- net/ipv4/tcp_ipv4.c | 4 +- net/ipv4/tcp_output.c | 2 +- net/ipv4/tcp_timer.c | 2 +- net/ipv6/tcp_ipv6.c | 2 +- 9 files changed, 149 insertions(+), 41 deletions(-) diff --git a/include/net/sock.h b/include/net/sock.h index abb6e0f..a71447b 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -53,6 +53,7 @@ #include <linux/security.h> #include <linux/slab.h> #include <linux/uaccess.h> +#include <linux/memcontrol.h> #include <linux/filter.h> #include <linux/rculist_nulls.h> @@ -863,6 +864,99 @@ static inline void sk_refcnt_debug_release(const struct sock *sk) #define sk_refcnt_debug_release(sk) do { } while (0) #endif /* SOCK_REFCNT_DEBUG */ +static inline bool sk_has_memory_pressure(const struct sock *sk) +{ + return sk->sk_prot->memory_pressure != NULL; +} + +static inline bool sk_under_memory_pressure(const struct sock *sk) +{ + if (!sk->sk_prot->memory_pressure) + return false; + return !!*sk->sk_prot->memory_pressure; +} + +static inline void sk_leave_memory_pressure(struct sock *sk) +{ + int *memory_pressure = sk->sk_prot->memory_pressure; + + if (memory_pressure && *memory_pressure) + *memory_pressure = 0; +} + +static inlin…
creator: Glauber Costa
date: 2011-11-29T23:56:53-00:00

[PATCH v7 03/10] socket: initial cgroup code.
type: item
description: The goal of this work is to move the memory pressure tcp controls to a cgroup, instead of just relying on global conditions. To avoid excessive overhead in the network fast paths, the code that accounts allocated memory to a cgroup is hidden inside a static_branch(). This branch is patched out until the first non-root cgroup is created. So when nobody is using cgroups, even if it is mounted, no significant performance penalty should be seen. This patch handles the generic part of the code, and has nothing tcp-specific. Signed-off-by: Glauber Costa <glommer@parallels.com> Acked-by: Kirill A. Shutemov<kirill@shutemov.name> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujtsu.com> CC: David S. Miller <davem@davemloft.net> CC: Eric W. Biederman <ebiederm@xmission.com> CC: Eric Dumazet <eric.dumazet@gmail.com> --- Documentation/cgroups/memory.txt | 4 +- include/linux/memcontrol.h | 16 +++ include/net/sock.h | 210 +++++++++++++++++++++++++++++++++++++- mm/memcontrol.c | 40 ++++++- net/core/sock.c | 24 +++-- 5 files changed, 274 insertions(+), 20 deletions(-) diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt index 764781c..3cf9d96 100644 --- a/Documentation/cgroups/memory.txt +++ b/Documentation/cgroups/memory.txt @@ -295,7 +295,9 @@ and set kmem extension config option carefully. 2.7.1 Current Kernel Memory resources accounted -None +* sockets memory pressure: some sockets protocols have memory pressure +thresholds. The Memory Controller allows them to be controlled individually +per cgroup, instead of globally. 3. User Interface diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index b87068a..60964c3 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -381,5 +381,21 @@ mem_cgroup_print_bad_page(struct page *page) } #endif +#ifdef CONFIG_INET +enum { + UNDER_LIMIT, + SOFT_LIMIT, + OVER_LIMIT, +}; + +struct sock; +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM +void sock_update_memcg(struct sock *sk); +…
creator: Glauber Costa
date: 2011-11-29T23:56:54-00:00

[PATCH v7 04/10] tcp memory pressure controls
type: item
description: This patch introduces memory pressure controls for the tcp protocol. It uses the generic socket memory pressure code introduced in earlier patches, and fills in the necessary data in cg_proto struct. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujtisu.com> CC: Eric W. Biederman <ebiederm@xmission.com> --- Documentation/cgroups/memory.txt | 2 + include/linux/memcontrol.h | 3 ++ include/net/sock.h | 2 + include/net/tcp_memcontrol.h | 17 +++++++++ mm/memcontrol.c | 36 +++++++++++++++++-- net/core/sock.c | 42 ++++++++++++++++++++-- net/ipv4/Makefile | 1 + net/ipv4/tcp_ipv4.c | 8 ++++- net/ipv4/tcp_memcontrol.c | 73 ++++++++++++++++++++++++++++++++++++++ net/ipv6/tcp_ipv6.c | 4 ++ 10 files changed, 181 insertions(+), 7 deletions(-) create mode 100644 include/net/tcp_memcontrol.h create mode 100644 net/ipv4/tcp_memcontrol.c diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt index 3cf9d96..1e43da4 100644 --- a/Documentation/cgroups/memory.txt +++ b/Documentation/cgroups/memory.txt @@ -299,6 +299,8 @@ and set kmem extension config option carefully. thresholds. The Memory Controller allows them to be controlled individually per cgroup, instead of globally. +* tcp memory pressure: sockets memory pressure for the tcp protocol. + 3. User Interface 0. Configuration diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 60964c3..fa2482a 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -85,6 +85,9 @@ extern struct mem_cgroup *try_get_mem_cgroup_from_page(struct page *page); extern struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p); extern struct mem_cgroup *try_get_mem_cgroup_from_mm(struct mm_struct *mm); +extern struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont); +extern struct mem_cgroup *parent_mem_cgroup(struct mem_cgroup *mem); + static inline int mm_match_cgroup(const struct mm_struct *mm, const struct mem_cgroup *cgroup) { di…
creator: Glauber Costa
date: 2011-11-29T23:56:55-00:00

[PATCH v7 05/10] per-netns ipv4 sysctl_tcp_mem
type: item
description: This patch allows each namespace to independently set up its levels for tcp memory pressure thresholds. This patch alone does not buy much: we need to make this values per group of process somehow. This is achieved in the patches that follows in this patchset. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> CC: David S. Miller <davem@davemloft.net> CC: Eric W. Biederman <ebiederm@xmission.com> --- include/net/netns/ipv4.h | 1 + include/net/tcp.h | 1 - net/ipv4/af_inet.c | 2 + net/ipv4/sysctl_net_ipv4.c | 51 +++++++++++++++++++++++++++++++++++++------ net/ipv4/tcp.c | 11 +------- net/ipv4/tcp_ipv4.c | 1 - net/ipv4/tcp_memcontrol.c | 9 +++++-- net/ipv6/af_inet6.c | 2 + net/ipv6/tcp_ipv6.c | 1 - 9 files changed, 57 insertions(+), 22 deletions(-) diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h index d786b4f..bbd023a 100644 --- a/include/net/netns/ipv4.h +++ b/include/net/netns/ipv4.h @@ -55,6 +55,7 @@ struct netns_ipv4 { int current_rt_cache_rebuild_count; unsigned int sysctl_ping_group_range[2]; + long sysctl_tcp_mem[3]; atomic_t rt_genid; atomic_t dev_addr_genid; diff --git a/include/net/tcp.h b/include/net/tcp.h index f080e0b..61c7e76 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -230,7 +230,6 @@ extern int sysctl_tcp_fack; extern int sysctl_tcp_reordering; extern int sysctl_tcp_ecn; extern int sysctl_tcp_dsack; -extern long sysctl_tcp_mem[3]; extern int sysctl_tcp_wmem[3]; extern int sysctl_tcp_rmem[3]; extern int sysctl_tcp_app_win; diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index 1b5096a..a8bbcff 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -1671,6 +1671,8 @@ static int __init inet_init(void) ip_static_sysctl_init(); #endif + tcp_prot.sysctl_mem = init_net.ipv4.sysctl_tcp_mem; + /* * Add all the base protocols. */ diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c index 69fd720..bbd67ab 100644 --- a/net/ipv4/sysctl_net_ipv4.c +++ b/ne…
creator: Glauber Costa
date: 2011-11-29T23:56:56-00:00

[PATCH v7 06/10] tcp buffer limitation: per-cgroup limit
type: item
description: This patch uses the "tcp.limit_in_bytes" field of the kmem_cgroup to effectively control the amount of kernel memory pinned by a cgroup. This value is ignored in the root cgroup, and in all others, caps the value specified by the admin in the net namespaces' view of tcp_sysctl_mem. If namespaces are being used, the admin is allowed to set a value bigger than cgroup's maximum, the same way it is allowed to set pretty much unlimited values in a real box. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: David S. Miller <davem@davemloft.net> CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> CC: Eric W. Biederman <ebiederm@xmission.com> --- Documentation/cgroups/memory.txt | 1 + include/net/tcp_memcontrol.h | 2 + net/ipv4/sysctl_net_ipv4.c | 14 ++++ net/ipv4/tcp_memcontrol.c | 134 +++++++++++++++++++++++++++++++++++++- 4 files changed, 149 insertions(+), 2 deletions(-) diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt index 1e43da4..4d9ed1f 100644 --- a/Documentation/cgroups/memory.txt +++ b/Documentation/cgroups/memory.txt @@ -78,6 +78,7 @@ Brief summary of control files. memory.independent_kmem_limit # select whether or not kernel memory limits are independent of user limits + memory.kmem.tcp.limit_in_bytes # set/show hard limit for tcp buf memory 1. History diff --git a/include/net/tcp_memcontrol.h b/include/net/tcp_memcontrol.h index 5f5e158..3512082 100644 --- a/include/net/tcp_memcontrol.h +++ b/include/net/tcp_memcontrol.h @@ -14,4 +14,6 @@ struct tcp_memcontrol { struct cg_proto *tcp_proto_cgroup(struct mem_cgroup *memcg); int tcp_init_cgroup(struct cgroup *cgrp, struct cgroup_subsys *ss); void tcp_destroy_cgroup(struct cgroup *cgrp, struct cgroup_subsys *ss); +unsigned long long tcp_max_memory(const struct mem_cgroup *memcg); +void tcp_prot_mem(struct mem_cgroup *memcg, long val, int idx); #endif /* _TCP_MEMCG_H */ diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c index bbd67ab..fe9bf91 100644 --- a…
creator: Glauber Costa
date: 2011-11-29T23:56:57-00:00

[PATCH v7 07/10] Display current tcp memory allocation in kmem cgroup
type: item
description: This patch introduces kmem.tcp.usage_in_bytes file, living in the kmem_cgroup filesystem. It is a simple read-only file that displays the amount of kernel memory currently consumed by the cgroup. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: David S. Miller <davem@davemloft.net> CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> CC: Eric W. Biederman <ebiederm@xmission.com> --- Documentation/cgroups/memory.txt | 1 + net/ipv4/tcp_memcontrol.c | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 0 deletions(-) diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt index 4d9ed1f..09fcc82 100644 --- a/Documentation/cgroups/memory.txt +++ b/Documentation/cgroups/memory.txt @@ -79,6 +79,7 @@ Brief summary of control files. memory.independent_kmem_limit # select whether or not kernel memory limits are independent of user limits memory.kmem.tcp.limit_in_bytes # set/show hard limit for tcp buf memory + memory.kmem.tcp.usage_in_bytes # show current tcp buf memory allocation 1. History diff --git a/net/ipv4/tcp_memcontrol.c b/net/ipv4/tcp_memcontrol.c index 3edcf5f..bc232fc 100644 --- a/net/ipv4/tcp_memcontrol.c +++ b/net/ipv4/tcp_memcontrol.c @@ -17,6 +17,11 @@ static struct cftype tcp_files[] = { .read_u64 = tcp_cgroup_read, .private = RES_LIMIT, }, + { + .name = "kmem.tcp.usage_in_bytes", + .read_u64 = tcp_cgroup_read, + .private = RES_USAGE, + }, }; static inline struct tcp_memcontrol *tcp_from_cgproto(struct cg_proto *cg_proto) @@ -163,6 +168,19 @@ static u64 tcp_read_stat(struct mem_cgroup *memcg, int type, u64 default_val) return res_counter_read_u64(&tcp->tcp_memory_allocated, type); } +static u64 tcp_read_usage(struct mem_cgroup *memcg) +{ + struct tcp_memcontrol *tcp; + struct cg_proto *cg_proto; + + cg_proto = tcp_prot.proto_cgroup(memcg); + if (!cg_proto) + return atomic_long_read(&tcp_memory_allocated) << PAGE_SHIFT; + + tcp = tcp_from_cgproto(cg_proto); + return res_counter_read_u64(&tcp->tcp_memory_allocated, RE…
creator: Glauber Costa
date: 2011-11-29T23:56:58-00:00

[PATCH v7 08/10] Display current tcp failcnt in kmem cgroup
type: item
description: This patch introduces kmem.tcp.failcnt file, living in the kmem_cgroup filesystem. Following the pattern in the other memcg resources, this files keeps a counter of how many times allocation failed due to limits being hit in this cgroup. The root cgroup will always show a failcnt of 0. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: David S. Miller <davem@davemloft.net> CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> CC: Eric W. Biederman <ebiederm@xmission.com> --- net/ipv4/tcp_memcontrol.c | 31 +++++++++++++++++++++++++++++++ 1 files changed, 31 insertions(+), 0 deletions(-) diff --git a/net/ipv4/tcp_memcontrol.c b/net/ipv4/tcp_memcontrol.c index bc232fc..f7350fa1 100644 --- a/net/ipv4/tcp_memcontrol.c +++ b/net/ipv4/tcp_memcontrol.c @@ -9,6 +9,7 @@ static u64 tcp_cgroup_read(struct cgroup *cont, struct cftype *cft); static int tcp_cgroup_write(struct cgroup *cont, struct cftype *cft, const char *buffer); +static int tcp_cgroup_reset(struct cgroup *cont, unsigned int event); static struct cftype tcp_files[] = { { @@ -22,6 +23,12 @@ static struct cftype tcp_files[] = { .read_u64 = tcp_cgroup_read, .private = RES_USAGE, }, + { + .name = "kmem.tcp.failcnt", + .private = RES_FAILCNT, + .trigger = tcp_cgroup_reset, + .read_u64 = tcp_cgroup_read, + }, }; static inline struct tcp_memcontrol *tcp_from_cgproto(struct cg_proto *cg_proto) @@ -193,12 +200,36 @@ static u64 tcp_cgroup_read(struct cgroup *cont, struct cftype *cft) case RES_USAGE: val = tcp_read_usage(memcg); break; + case RES_FAILCNT: + val = tcp_read_stat(memcg, RES_FAILCNT, 0); + break; default: BUG(); } return val; } +static int tcp_cgroup_reset(struct cgroup *cont, unsigned int event) +{ + struct mem_cgroup *memcg; + struct tcp_memcontrol *tcp; + struct cg_proto *cg_proto; + + memcg = mem_cgroup_from_cont(cont); + cg_proto = tcp_prot.proto_cgroup(memcg); + if (!cg_proto) + return 0; + tcp = tcp_from_cgproto(cg_proto); + + switch (event) { + case RES_FAILCNT: + res_counter_reset_failcnt(&tcp-…
creator: Glauber Costa
date: 2011-11-29T23:56:59-00:00

[PATCH v7 09/10] Display maximum tcp memory allocation in kmem cgroup
type: item
description: This patch introduces kmem.tcp.max_usage_in_bytes file, living in the kmem_cgroup filesystem. The root cgroup will display a value equal to RESOURCE_MAX. This is to avoid introducing any locking schemes in the network paths when cgroups are not being actively used. All others, will see the maximum memory ever used by this cgroup. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: David S. Miller <davem@davemloft.net> CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> CC: Eric W. Biederman <ebiederm@xmission.com> --- net/ipv4/tcp_memcontrol.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-) diff --git a/net/ipv4/tcp_memcontrol.c b/net/ipv4/tcp_memcontrol.c index f7350fa1..6ec90de 100644 --- a/net/ipv4/tcp_memcontrol.c +++ b/net/ipv4/tcp_memcontrol.c @@ -29,6 +29,12 @@ static struct cftype tcp_files[] = { .trigger = tcp_cgroup_reset, .read_u64 = tcp_cgroup_read, }, + { + .name = "kmem.tcp.max_usage_in_bytes", + .private = RES_MAX_USAGE, + .trigger = tcp_cgroup_reset, + .read_u64 = tcp_cgroup_read, + }, }; static inline struct tcp_memcontrol *tcp_from_cgproto(struct cg_proto *cg_proto) @@ -201,7 +207,8 @@ static u64 tcp_cgroup_read(struct cgroup *cont, struct cftype *cft) val = tcp_read_usage(memcg); break; case RES_FAILCNT: - val = tcp_read_stat(memcg, RES_FAILCNT, 0); + case RES_MAX_USAGE: + val = tcp_read_stat(memcg, cft->private, 0); break; default: BUG(); @@ -222,6 +229,9 @@ static int tcp_cgroup_reset(struct cgroup *cont, unsigned int event) tcp = tcp_from_cgproto(cg_proto); switch (event) { + case RES_MAX_USAGE: + res_counter_reset_max(&tcp->tcp_memory_allocated); + break; case RES_FAILCNT: res_counter_reset_failcnt(&tcp->tcp_memory_allocated); break; -- 1.7.6.4
creator: Glauber Costa
date: 2011-11-29T23:57:00-00:00

[PATCH v7 10/10] Disable task moving when using kernel memory accounting
type: item
description: Since this code is still experimental, we are leaving the exact details of how to move tasks between cgroups when kernel memory accounting is used as future work. For now, we simply disallow movement if there are any pending accounted memory. Signed-off-by: Glauber Costa <glommer@parallels.com> CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> --- mm/memcontrol.c | 23 ++++++++++++++++++++++- 1 files changed, 22 insertions(+), 1 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index a31a278..dd9a6d9 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -5453,10 +5453,19 @@ static int mem_cgroup_can_attach(struct cgroup_subsys *ss, { int ret = 0; struct mem_cgroup *memcg = mem_cgroup_from_cont(cgroup); + struct mem_cgroup *from = mem_cgroup_from_task(p); + +#if defined(CONFIG_CGROUP_MEM_RES_CTLR_KMEM) && defined(CONFIG_INET) + if (from != memcg && !mem_cgroup_is_root(from) && + res_counter_read_u64(&from->tcp_mem.tcp_memory_allocated, RES_USAGE)) { + printk(KERN_WARNING "Can't move tasks between cgroups: " + "Kernel memory held.\n"); + return 1; + } +#endif if (memcg->move_charge_at_immigrate) { struct mm_struct *mm; - struct mem_cgroup *from = mem_cgroup_from_task(p); VM_BUG_ON(from == memcg); @@ -5624,6 +5633,18 @@ static int mem_cgroup_can_attach(struct cgroup_subsys *ss, struct cgroup *cgroup, struct task_struct *p) { + struct mem_cgroup *memcg = mem_cgroup_from_cont(cgroup); + struct mem_cgroup *from = mem_cgroup_from_task(p); + +#if defined(CONFIG_CGROUP_MEM_RES_CTLR_KMEM) && defined(CONFIG_INET) + if (from != memcg && !mem_cgroup_is_root(from) && + res_counter_read_u64(&from->tcp_mem.tcp_memory_allocated, RES_USAGE)) { + printk(KERN_WARNING "Can't move tasks between cgroups: " + "Kernel memory held.\n"); + return 1; + } +#endif + return 0; } static void mem_cgroup_cancel_attach(struct cgroup_subsys *ss, -- 1.7.6.4
creator: Glauber Costa
date: 2011-11-29T23:57:01-00:00

Re: [PATCH v7 02/10] foundations of per-cgroup memory pressure controlling.
type: item
description: On Tue, 29 Nov 2011 21:56:53 -0200 Glauber Costa <glommer@parallels.com> wrote: > This patch replaces all uses of struct sock fields' memory_pressure, > memory_allocated, sockets_allocated, and sysctl_mem to acessor > macros. Those macros can either receive a socket argument, or a mem_cgroup > argument, depending on the context they live in. > > Since we're only doing a macro wrapping here, no performance impact at all is > expected in the case where we don't have cgroups disabled. > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: David S. Miller <davem@davemloft.net> > CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> > CC: Eric W. Biederman <ebiederm@xmission.com> > CC: Eric Dumazet <eric.dumazet@gmail.com> <snip> > +static inline bool > +memcg_memory_pressure(struct proto *prot, struct mem_cgroup *memcg) > +{ > + if (!prot->memory_pressure) > + return false; > + return !!prot->memory_pressure; > +} I think you should take a deep breath and write patech relaxedly, and do enough test. This should be return !!*prot->memory_pressure; BTW, I don't like to receive tons of everyday-update even if you're in hurry. > static void proto_seq_printf(struct seq_file *seq, struct proto *proto) > { > + struct mem_cgroup *memcg = mem_cgroup_from_task(current); > + > seq_printf(seq, "%-9s %4u %6d %6ld %-3s %6u %-3s %-10s " > "%2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c\n", > proto->name, > proto->obj_size, > sock_prot_inuse_get(seq_file_net(seq), proto), > - proto->memory_allocated != NULL ? atomic_long_read(proto->memory_allocated) : -1L, > - proto->memory_pressure != NULL ? *proto->memory_pressure ? "yes" : "no" : "NI", > + sock_prot_memory_allocated(proto, memcg), > + sock_prot_memory_pressure(proto, memcg), I wonder I should say NO, here. (Networking guys are ok ??) IIUC, this means there is no way to see aggregated sockstat of all system. And the result depends on the cgroup which the caller is under control. I think you shou…
creator: KAMEZAWA Hiroyuki
date: 2011-11-30T00:43:05-00:00

Re: [PATCH v7 01/10] Basic kernel memory functionality for the Memory Controller
type: item
description: On Tue, 29 Nov 2011 21:56:52 -0200 Glauber Costa <glommer@parallels.com> wrote: > This patch lays down the foundation for the kernel memory component > of the Memory Controller. > > As of today, I am only laying down the following files: > > * memory.independent_kmem_limit > * memory.kmem.limit_in_bytes (currently ignored) > * memory.kmem.usage_in_bytes (always zero) > > Signed-off-by: Glauber Costa <glommer@parallels.com> > Reviewed-by: Kirill A. Shutemov <kirill@shutemov.name> > CC: Paul Menage <paul@paulmenage.org> > CC: Greg Thelen <gthelen@google.com> Now, there are new memcg maintainers as Johannes Weiner <hannes@cmpxchg.org> Michal Hocko <mhocko@suse.cz> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Balbir Singh <bsingharora@gmail.com> So, please CC them. I may not be able to make a quick/good reply. Hmm, my concern is + memory.independent_kmem_limit # select whether or not kernel memory limits are + independent of user limits I'll be okay with this. other ones ? Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
creator: KAMEZAWA Hiroyuki
date: 2011-11-30T00:48:51-00:00

Re: [PATCH v7 03/10] socket: initial cgroup code.
type: item
description: On Tue, 29 Nov 2011 21:56:54 -0200 Glauber Costa <glommer@parallels.com> wrote: > The goal of this work is to move the memory pressure tcp > controls to a cgroup, instead of just relying on global > conditions. > > To avoid excessive overhead in the network fast paths, > the code that accounts allocated memory to a cgroup is > hidden inside a static_branch(). This branch is patched out > until the first non-root cgroup is created. So when nobody > is using cgroups, even if it is mounted, no significant performance > penalty should be seen. > > This patch handles the generic part of the code, and has nothing > tcp-specific. > > Signed-off-by: Glauber Costa <glommer@parallels.com> > Acked-by: Kirill A. Shutemov<kirill@shutemov.name> > Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujtsu.com> > CC: David S. Miller <davem@davemloft.net> > CC: Eric W. Biederman <ebiederm@xmission.com> > CC: Eric Dumazet <eric.dumazet@gmail.com> <snip> > +extern struct jump_label_key memcg_socket_limit_enabled; > static inline bool sk_has_memory_pressure(const struct sock *sk) > { > return sk->sk_prot->memory_pressure != NULL; > @@ -873,6 +900,17 @@ static inline bool sk_under_memory_pressure(const struct sock *sk) > { > if (!sk->sk_prot->memory_pressure) > return false; > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM > + if (static_branch(&memcg_socket_limit_enabled)) { > + struct cg_proto *cg_proto = sk->sk_cgrp; > + > + if (!cg_proto) > + goto nocgroup; > + return !!*cg_proto->memory_pressure; > + } else What is dangling 'else' for ? > +nocgroup: > +#endif > + > return !!*sk->sk_prot->memory_pressure; > } > > @@ -880,52 +918,176 @@ static inline void sk_leave_memory_pressure(struct sock *sk) > { > int *memory_pressure = sk->sk_prot->memory_pressure; > > - if (memory_pressure && *memory_pressure) > + if (!memory_pressure) > + return; > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM > + if (static_branch(&memcg_socket_limit_enabled)) { > + struct cg_proto *cg_proto = sk->sk_cgrp; > + > + if (!cg…
creator: KAMEZAWA Hiroyuki
date: 2011-11-30T01:07:38-00:00

Re: [PATCH v7 04/10] tcp memory pressure controls
type: item
description: On Tue, 29 Nov 2011 21:56:55 -0200 Glauber Costa <glommer@parallels.com> wrote: > This patch introduces memory pressure controls for the tcp > protocol. It uses the generic socket memory pressure code > introduced in earlier patches, and fills in the > necessary data in cg_proto struct. > > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujtisu.com> > CC: Eric W. Biederman <ebiederm@xmission.com> some comments. > --- > Documentation/cgroups/memory.txt | 2 + > include/linux/memcontrol.h | 3 ++ > include/net/sock.h | 2 + > include/net/tcp_memcontrol.h | 17 +++++++++ > mm/memcontrol.c | 36 +++++++++++++++++-- > net/core/sock.c | 42 ++++++++++++++++++++-- > net/ipv4/Makefile | 1 + > net/ipv4/tcp_ipv4.c | 8 ++++- > net/ipv4/tcp_memcontrol.c | 73 ++++++++++++++++++++++++++++++++++++++ > net/ipv6/tcp_ipv6.c | 4 ++ > 10 files changed, 181 insertions(+), 7 deletions(-) > create mode 100644 include/net/tcp_memcontrol.h > create mode 100644 net/ipv4/tcp_memcontrol.c > > diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt > index 3cf9d96..1e43da4 100644 > --- a/Documentation/cgroups/memory.txt > +++ b/Documentation/cgroups/memory.txt > @@ -299,6 +299,8 @@ and set kmem extension config option carefully. > thresholds. The Memory Controller allows them to be controlled individually > per cgroup, instead of globally. > > +* tcp memory pressure: sockets memory pressure for the tcp protocol. > + > 3. User Interface > > 0. Configuration > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 60964c3..fa2482a 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -85,6 +85,9 @@ extern struct mem_cgroup *try_get_mem_cgroup_from_page(struct page *page); > extern struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p); > extern struct mem_cgroup *try_get_mem_cgroup_from_mm(struct mm_struct *mm); > > +extern struct mem_cgroup *mem_cgroup_from_cont(struct…
creator: KAMEZAWA Hiroyuki
date: 2011-11-30T01:49:43-00:00

Re: [PATCH v7 06/10] tcp buffer limitation: per-cgroup limit
type: item
description: On Tue, 29 Nov 2011 21:56:57 -0200 Glauber Costa <glommer@parallels.com> wrote: > This patch uses the "tcp.limit_in_bytes" field of the kmem_cgroup to > effectively control the amount of kernel memory pinned by a cgroup. > > This value is ignored in the root cgroup, and in all others, > caps the value specified by the admin in the net namespaces' > view of tcp_sysctl_mem. > > If namespaces are being used, the admin is allowed to set a > value bigger than cgroup's maximum, the same way it is allowed > to set pretty much unlimited values in a real box. > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: David S. Miller <davem@davemloft.net> > CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> > CC: Eric W. Biederman <ebiederm@xmission.com> you need one more fix. (please add changelog.) > +static int tcp_update_limit(struct mem_cgroup *memcg, u64 val) > +{ > + struct net *net = current->nsproxy->net_ns; > + struct tcp_memcontrol *tcp; > + struct cg_proto *cg_proto; > + u64 old_lim; > + int i; > + int ret; > + > + cg_proto = tcp_prot.proto_cgroup(memcg); > + if (!cg_proto) > + return -EINVAL; > + > + tcp = tcp_from_cgproto(cg_proto); > + > + old_lim = res_counter_read_u64(&tcp->tcp_memory_allocated, RES_LIMIT); > + ret = res_counter_set_limit(&tcp->tcp_memory_allocated, val); > + if (ret) > + return ret; > + > + for (i = 0; i < 3; i++) > + tcp->tcp_prot_mem[i] = min_t(long, val >> PAGE_SHIFT, > + net->ipv4.sysctl_tcp_mem[i]); > + > + if (val == RESOURCE_MAX) > + jump_label_dec(&memcg_socket_limit_enabled); if (val == RESOUCE_MAX && old_lim != RESOUCE_MAX) Thanks, -Kame
creator: KAMEZAWA Hiroyuki
date: 2011-11-30T02:00:29-00:00

Re: [PATCH v7 08/10] Display current tcp failcnt in kmem cgroup
type: item
description: On Tue, 29 Nov 2011 21:56:59 -0200 Glauber Costa <glommer@parallels.com> wrote: > This patch introduces kmem.tcp.failcnt file, living in the > kmem_cgroup filesystem. Following the pattern in the other > memcg resources, this files keeps a counter of how many times > allocation failed due to limits being hit in this cgroup. > The root cgroup will always show a failcnt of 0. > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: David S. Miller <davem@davemloft.net> > CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> > CC: Eric W. Biederman <ebiederm@xmission.com> Reviewed-by : KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
creator: KAMEZAWA Hiroyuki
date: 2011-11-30T02:01:31-00:00

Re: [PATCH v7 09/10] Display maximum tcp memory allocation in kmem cgroup
type: item
description: On Tue, 29 Nov 2011 21:57:00 -0200 Glauber Costa <glommer@parallels.com> wrote: > This patch introduces kmem.tcp.max_usage_in_bytes file, living in the > kmem_cgroup filesystem. The root cgroup will display a value equal > to RESOURCE_MAX. This is to avoid introducing any locking schemes in > the network paths when cgroups are not being actively used. > > All others, will see the maximum memory ever used by this cgroup. > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: David S. Miller <davem@davemloft.net> > CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> > CC: Eric W. Biederman <ebiederm@xmission.com> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
creator: KAMEZAWA Hiroyuki
date: 2011-11-30T02:02:20-00:00

Re: [PATCH v7 00/10] Request for Inclusion: per-cgroup tcp memory pressure
type: item
description: On Tue, 29 Nov 2011 21:56:51 -0200 Glauber Costa <glommer@parallels.com> wrote: > Hi, > > This patchset implements per-cgroup tcp memory pressure controls. It did not change > significantly since last submission: rather, it just merges the comments Kame had. > Most of them are style-related and/or Documentation, but there are two real bugs he > managed to spot (thanks) > > Please let me know if there is anything else I should address. > After reading all codes again, I feel some strange. Could you clarify ? Here. == +void sock_update_memcg(struct sock *sk) +{ + /* right now a socket spends its whole life in the same cgroup */ + if (sk->sk_cgrp) { + WARN_ON(1); + return; + } + if (static_branch(&memcg_socket_limit_enabled)) { + struct mem_cgroup *memcg; + + BUG_ON(!sk->sk_prot->proto_cgroup); + + rcu_read_lock(); + memcg = mem_cgroup_from_task(current); + if (!mem_cgroup_is_root(memcg)) + sk->sk_cgrp = sk->sk_prot->proto_cgroup(memcg); + rcu_read_unlock(); == sk->sk_cgrp is set to a memcg without any reference count. Then, no check for preventing rmdir() and freeing memcgroup. Is there some css_get() or mem_cgroup_get() somewhere ? Thanks, -Kame
creator: KAMEZAWA Hiroyuki
date: 2011-11-30T02:11:52-00:00

Re: [PATCH v7 10/10] Disable task moving when using kernel memory accounting
type: item
description: On Tue, 29 Nov 2011 21:57:01 -0200 Glauber Costa <glommer@parallels.com> wrote: > Since this code is still experimental, we are leaving the exact > details of how to move tasks between cgroups when kernel memory > accounting is used as future work. > > For now, we simply disallow movement if there are any pending > accounted memory. > > Signed-off-by: Glauber Costa <glommer@parallels.com> > CC: Hiroyouki Kamezawa <kamezawa.hiroyu@jp.fujitsu.com> > --- > mm/memcontrol.c | 23 ++++++++++++++++++++++- > 1 files changed, 22 insertions(+), 1 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index a31a278..dd9a6d9 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -5453,10 +5453,19 @@ static int mem_cgroup_can_attach(struct cgroup_subsys *ss, > { > int ret = 0; > struct mem_cgroup *memcg = mem_cgroup_from_cont(cgroup); > + struct mem_cgroup *from = mem_cgroup_from_task(p); > + > +#if defined(CONFIG_CGROUP_MEM_RES_CTLR_KMEM) && defined(CONFIG_INET) > + if (from != memcg && !mem_cgroup_is_root(from) && > + res_counter_read_u64(&from->tcp_mem.tcp_memory_allocated, RES_USAGE)) { > + printk(KERN_WARNING "Can't move tasks between cgroups: " > + "Kernel memory held.\n"); > + return 1; > + } > +#endif I wonder....reading all codes again, this is incorrect check. Hm, let me cralify. IIUC, in old code, "prevent moving" is because you hold reference count of cgroup, which can cause trouble at rmdir() as leaking refcnt. BTW, because socket is a shared resource between cgroup, changes in mm->owner may cause task cgroup moving implicitly. So, if you allow leak of resource here, I guess... you can take mem_cgroup_get() refcnt which is memcg-local and allow rmdir(). Then, this limitation may disappear. Then, users will be happy but admins will have unseen kernel resource usage in not populated(by rmdir) memcg. Hm, big trouble ? Thanks, -Kame
creator: KAMEZAWA Hiroyuki
date: 2011-11-30T02:22:10-00:00

Re: [PATCH v7 02/10] foundations of per-cgroup memory pressure controlling.
type: item
description: >> static void proto_seq_printf(struct seq_file *seq, struct proto *proto) >> { >> + struct mem_cgroup *memcg = mem_cgroup_from_task(current); >> + >> seq_printf(seq, "%-9s %4u %6d %6ld %-3s %6u %-3s %-10s " >> "%2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c\n", >> proto->name, >> proto->obj_size, >> sock_prot_inuse_get(seq_file_net(seq), proto), >> - proto->memory_allocated != NULL ? atomic_long_read(proto->memory_allocated) : -1L, >> - proto->memory_pressure != NULL ? *proto->memory_pressure ? "yes" : "no" : "NI", >> + sock_prot_memory_allocated(proto, memcg), >> + sock_prot_memory_pressure(proto, memcg), > > I wonder I should say NO, here. (Networking guys are ok ??) > > IIUC, this means there is no way to see aggregated sockstat of all system. > And the result depends on the cgroup which the caller is under control. > > I think you should show aggregated sockstat(global + per-memcg) here and > show per-memcg ones via /cgroup interface or add private_sockstat to show > per cgroup summary. > Hi Kame, Yes, the statistics displayed depends on which cgroup you live. Also, note that the parent cgroup here is always updated (even when use_hierarchy is set to 0). So it is always possible to grab global statistics, by being in the root cgroup. For the others, I believe it to be a question of naturalization. Any tool that is fetching these values is likely interested in the amount of resources available/used. When you are on a cgroup, the amount of resources available/used changes, so that's what you should see. Also brings the point of resource isolation: if you shouldn't interfere with other set of process' resources, there is no reason for you to see them in the first place. So given all that, I believe that whenever we talk about resources in a cgroup, we should talk about cgroup-local ones.
creator: Glauber Costa
date: 2011-12-02T17:46:46-00:00

Re: [PATCH v7 04/10] tcp memory pressure controls
type: item
description: On 11/29/2011 11:49 PM, KAMEZAWA Hiroyuki wrote: > >> -static struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont) >> +struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont) >> { >> return container_of(cgroup_subsys_state(cont, >> mem_cgroup_subsys_id), struct mem_cgroup, >> @@ -4717,14 +4732,27 @@ static int register_kmem_files(struct cgroup *cont, struct cgroup_subsys *ss) >> >> ret = cgroup_add_files(cont, ss, kmem_cgroup_files, >> ARRAY_SIZE(kmem_cgroup_files)); >> + >> + if (!ret) >> + ret = mem_cgroup_sockets_init(cont, ss); >> return ret; >> }; > > You does initizalication here. The reason what I think is > 1. 'proto_list' is not available at createion of root cgroup and > you need to delay set up until mounting. > > If so, please add comment or find another way. > This seems not very clean to me. Yes, we do can run into some ordering issues. A part of the initialization can be done earlier. But I preferred to move it all later instead of creating two functions for it. But I can change that if you want, no big deal. > > > > >> +static DEFINE_RWLOCK(proto_list_lock); >> +static LIST_HEAD(proto_list); >> + >> +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM >> +int mem_cgroup_sockets_init(struct cgroup *cgrp, struct cgroup_subsys *ss) >> +{ >> + struct proto *proto; >> + int ret = 0; >> + >> + read_lock(&proto_list_lock); >> + list_for_each_entry(proto,&proto_list, node) { >> + if (proto->init_cgroup) >> + ret = proto->init_cgroup(cgrp, ss); >> + if (ret) >> + goto out; >> + } > > seems indent is bad or {} is missing. > Thanks. I'll rewrite it, since I did miss {} around the first if. But no test could possibly catch it, since what I wanted to write, and what I wrote by mistake end up being equivalent. >> +EXPORT_SYMBOL(memcg_tcp_enter_memory_pressure); >> + >> +int tcp_init_cgroup(struct cgroup *cgrp, struct cgroup_subsys *ss) >> +{ >> + /* >> + * The root cgroup does not use res_counters, but rather, >> + * rely on the data already collected by the netwo…
creator: Glauber Costa
date: 2011-12-02T17:57:28-00:00

Re: [PATCH v7 00/10] Request for Inclusion: per-cgroup tcp memory pressure
type: item
description: On 11/30/2011 12:11 AM, KAMEZAWA Hiroyuki wrote: > On Tue, 29 Nov 2011 21:56:51 -0200 > Glauber Costa<glommer@parallels.com> wrote: > >> Hi, >> >> This patchset implements per-cgroup tcp memory pressure controls. It did not change >> significantly since last submission: rather, it just merges the comments Kame had. >> Most of them are style-related and/or Documentation, but there are two real bugs he >> managed to spot (thanks) >> >> Please let me know if there is anything else I should address. >> > > After reading all codes again, I feel some strange. Could you clarify ? > > Here. > == > +void sock_update_memcg(struct sock *sk) > +{ > + /* right now a socket spends its whole life in the same cgroup */ > + if (sk->sk_cgrp) { > + WARN_ON(1); > + return; > + } > + if (static_branch(&memcg_socket_limit_enabled)) { > + struct mem_cgroup *memcg; > + > + BUG_ON(!sk->sk_prot->proto_cgroup); > + > + rcu_read_lock(); > + memcg = mem_cgroup_from_task(current); > + if (!mem_cgroup_is_root(memcg)) > + sk->sk_cgrp = sk->sk_prot->proto_cgroup(memcg); > + rcu_read_unlock(); > == > > sk->sk_cgrp is set to a memcg without any reference count. > > Then, no check for preventing rmdir() and freeing memcgroup. > > Is there some css_get() or mem_cgroup_get() somewhere ? > There were a css_get in the first version of this patchset. It was removed, however, because it was deemed anti-intuitive to prevent rmdir, since we can't know which sockets are blocking it, or do anything about it. Or did I misunderstand something ?
creator: Glauber Costa
date: 2011-12-02T18:04:08-00:00

Re: [PATCH v7 10/10] Disable task moving when using kernel memory accounting
type: item
description: On 11/30/2011 12:22 AM, KAMEZAWA Hiroyuki wrote: > On Tue, 29 Nov 2011 21:57:01 -0200 > Glauber Costa<glommer@parallels.com> wrote: > >> Since this code is still experimental, we are leaving the exact >> details of how to move tasks between cgroups when kernel memory >> accounting is used as future work. >> >> For now, we simply disallow movement if there are any pending >> accounted memory. >> >> Signed-off-by: Glauber Costa<glommer@parallels.com> >> CC: Hiroyouki Kamezawa<kamezawa.hiroyu@jp.fujitsu.com> >> --- >> mm/memcontrol.c | 23 ++++++++++++++++++++++- >> 1 files changed, 22 insertions(+), 1 deletions(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index a31a278..dd9a6d9 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -5453,10 +5453,19 @@ static int mem_cgroup_can_attach(struct cgroup_subsys *ss, >> { >> int ret = 0; >> struct mem_cgroup *memcg = mem_cgroup_from_cont(cgroup); >> + struct mem_cgroup *from = mem_cgroup_from_task(p); >> + >> +#if defined(CONFIG_CGROUP_MEM_RES_CTLR_KMEM)&& defined(CONFIG_INET) >> + if (from != memcg&& !mem_cgroup_is_root(from)&& >> + res_counter_read_u64(&from->tcp_mem.tcp_memory_allocated, RES_USAGE)) { >> + printk(KERN_WARNING "Can't move tasks between cgroups: " >> + "Kernel memory held.\n"); >> + return 1; >> + } >> +#endif > > I wonder....reading all codes again, this is incorrect check. > > Hm, let me cralify. IIUC, in old code, "prevent moving" is because you hold > reference count of cgroup, which can cause trouble at rmdir() as leaking refcnt. right. > BTW, because socket is a shared resource between cgroup, changes in mm->owner > may cause task cgroup moving implicitly. So, if you allow leak of resource > here, I guess... you can take mem_cgroup_get() refcnt which is memcg-local and > allow rmdir(). Then, this limitation may disappear. Sorry, I didn't fully understand. Can you clarify further? If the task is implicitly moved, it will end up calling can_attach as well, right? > > Then, users wil…
creator: Glauber Costa
date: 2011-12-02T18:11:56-00:00

Re: [PATCH v7 02/10] foundations of per-cgroup memory pressure controlling.
type: item
description: On Fri, 2 Dec 2011 15:46:46 -0200 Glauber Costa <glommer@parallels.com> wrote: > > >> static void proto_seq_printf(struct seq_file *seq, struct proto *proto) > >> { > >> + struct mem_cgroup *memcg = mem_cgroup_from_task(current); > >> + > >> seq_printf(seq, "%-9s %4u %6d %6ld %-3s %6u %-3s %-10s " > >> "%2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c\n", > >> proto->name, > >> proto->obj_size, > >> sock_prot_inuse_get(seq_file_net(seq), proto), > >> - proto->memory_allocated != NULL ? atomic_long_read(proto->memory_allocated) : -1L, > >> - proto->memory_pressure != NULL ? *proto->memory_pressure ? "yes" : "no" : "NI", > >> + sock_prot_memory_allocated(proto, memcg), > >> + sock_prot_memory_pressure(proto, memcg), > > > > I wonder I should say NO, here. (Networking guys are ok ??) > > > > IIUC, this means there is no way to see aggregated sockstat of all system. > > And the result depends on the cgroup which the caller is under control. > > > > I think you should show aggregated sockstat(global + per-memcg) here and > > show per-memcg ones via /cgroup interface or add private_sockstat to show > > per cgroup summary. > > > > Hi Kame, > > Yes, the statistics displayed depends on which cgroup you live. > Also, note that the parent cgroup here is always updated (even when > use_hierarchy is set to 0). So it is always possible to grab global > statistics, by being in the root cgroup. > > For the others, I believe it to be a question of naturalization. Any > tool that is fetching these values is likely interested in the amount of > resources available/used. When you are on a cgroup, the amount of > resources available/used changes, so that's what you should see. > > Also brings the point of resource isolation: if you shouldn't interfere > with other set of process' resources, there is no reason for you to see > them in the first place. > > So given all that, I believe that whenever we talk about resources in a > cgroup, we should talk about cgro…
creator: KAMEZAWA Hiroyuki
date: 2011-12-05T01:59:16-00:00

Re: [PATCH v7 04/10] tcp memory pressure controls
type: item
description: On Fri, 2 Dec 2011 15:57:28 -0200 Glauber Costa <glommer@parallels.com> wrote: > On 11/29/2011 11:49 PM, KAMEZAWA Hiroyuki wrote: > > > >> -static struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont) > >> +struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont) > >> { > >> return container_of(cgroup_subsys_state(cont, > >> mem_cgroup_subsys_id), struct mem_cgroup, > >> @@ -4717,14 +4732,27 @@ static int register_kmem_files(struct cgroup *cont, struct cgroup_subsys *ss) > >> > >> ret = cgroup_add_files(cont, ss, kmem_cgroup_files, > >> ARRAY_SIZE(kmem_cgroup_files)); > >> + > >> + if (!ret) > >> + ret = mem_cgroup_sockets_init(cont, ss); > >> return ret; > >> }; > > > > You does initizalication here. The reason what I think is > > 1. 'proto_list' is not available at createion of root cgroup and > > you need to delay set up until mounting. > > > > If so, please add comment or find another way. > > This seems not very clean to me. > > Yes, we do can run into some ordering issues. A part of the > initialization can be done earlier. But I preferred to move it all later > instead of creating two functions for it. But I can change that if you > want, no big deal. > Hmm. please add comments about the 'issue'. It will help readers. > >> + tcp->tcp_prot_mem[0] = sysctl_tcp_mem[0]; > >> + tcp->tcp_prot_mem[1] = sysctl_tcp_mem[1]; > >> + tcp->tcp_prot_mem[2] = sysctl_tcp_mem[2]; > >> + tcp->tcp_memory_pressure = 0; > > > > Question: > > > > Is this value will be updated when an admin chages sysctl ? > > yes. > > > I guess, this value is set at system init script or some which may > > happen later than mounting cgroup. > > I don't like to write a guideline 'please set sysctl val before > > mounting cgroup' > > Agreed. > > This code is in patch 6 (together with the limiting): > > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM > + rcu_read_lock(); > + memcg = mem_cgroup_from_task(current); > + > + tcp_prot_mem(memcg, vec[0], 0); > + tcp_prot_mem(memcg, vec[1], 1); > + tcp_pr…
creator: KAMEZAWA Hiroyuki
date: 2011-12-05T02:01:58-00:00

Re: [PATCH v7 00/10] Request for Inclusion: per-cgroup tcp memory pressure
type: item
description: On Fri, 2 Dec 2011 16:04:08 -0200 Glauber Costa <glommer@parallels.com> wrote: > On 11/30/2011 12:11 AM, KAMEZAWA Hiroyuki wrote: > > On Tue, 29 Nov 2011 21:56:51 -0200 > > Glauber Costa<glommer@parallels.com> wrote: > > > >> Hi, > >> > >> This patchset implements per-cgroup tcp memory pressure controls. It did not change > >> significantly since last submission: rather, it just merges the comments Kame had. > >> Most of them are style-related and/or Documentation, but there are two real bugs he > >> managed to spot (thanks) > >> > >> Please let me know if there is anything else I should address. > >> > > > > After reading all codes again, I feel some strange. Could you clarify ? > > > > Here. > > == > > +void sock_update_memcg(struct sock *sk) > > +{ > > + /* right now a socket spends its whole life in the same cgroup */ > > + if (sk->sk_cgrp) { > > + WARN_ON(1); > > + return; > > + } > > + if (static_branch(&memcg_socket_limit_enabled)) { > > + struct mem_cgroup *memcg; > > + > > + BUG_ON(!sk->sk_prot->proto_cgroup); > > + > > + rcu_read_lock(); > > + memcg = mem_cgroup_from_task(current); > > + if (!mem_cgroup_is_root(memcg)) > > + sk->sk_cgrp = sk->sk_prot->proto_cgroup(memcg); > > + rcu_read_unlock(); > > == > > > > sk->sk_cgrp is set to a memcg without any reference count. > > > > Then, no check for preventing rmdir() and freeing memcgroup. > > > > Is there some css_get() or mem_cgroup_get() somewhere ? > > > > There were a css_get in the first version of this patchset. It was > removed, however, because it was deemed anti-intuitive to prevent rmdir, > since we can't know which sockets are blocking it, or do anything about > it. Or did I misunderstand something ? > Maybe I misuderstood. Thank you. Ok, there is no css_get/put and rmdir() is allowed. But, hmm....what's guarding threads from stale pointer access ? Does a memory cgroup which is pointed by sk->sk_cgrp always exist ? -Kame
creator: KAMEZAWA Hiroyuki
date: 2011-12-05T02:06:19-00:00

Re: [PATCH v7 10/10] Disable task moving when using kernel memory accounting
type: item
description: On Fri, 2 Dec 2011 16:11:56 -0200 Glauber Costa <glommer@parallels.com> wrote: > On 11/30/2011 12:22 AM, KAMEZAWA Hiroyuki wrote: > > On Tue, 29 Nov 2011 21:57:01 -0200 > > Glauber Costa<glommer@parallels.com> wrote: > > > >> Since this code is still experimental, we are leaving the exact > >> details of how to move tasks between cgroups when kernel memory > >> accounting is used as future work. > >> > >> For now, we simply disallow movement if there are any pending > >> accounted memory. > >> > >> Signed-off-by: Glauber Costa<glommer@parallels.com> > >> CC: Hiroyouki Kamezawa<kamezawa.hiroyu@jp.fujitsu.com> > >> --- > >> mm/memcontrol.c | 23 ++++++++++++++++++++++- > >> 1 files changed, 22 insertions(+), 1 deletions(-) > >> > >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c > >> index a31a278..dd9a6d9 100644 > >> --- a/mm/memcontrol.c > >> +++ b/mm/memcontrol.c > >> @@ -5453,10 +5453,19 @@ static int mem_cgroup_can_attach(struct cgroup_subsys *ss, > >> { > >> int ret = 0; > >> struct mem_cgroup *memcg = mem_cgroup_from_cont(cgroup); > >> + struct mem_cgroup *from = mem_cgroup_from_task(p); > >> + > >> +#if defined(CONFIG_CGROUP_MEM_RES_CTLR_KMEM)&& defined(CONFIG_INET) > >> + if (from != memcg&& !mem_cgroup_is_root(from)&& > >> + res_counter_read_u64(&from->tcp_mem.tcp_memory_allocated, RES_USAGE)) { > >> + printk(KERN_WARNING "Can't move tasks between cgroups: " > >> + "Kernel memory held.\n"); > >> + return 1; > >> + } > >> +#endif > > > > I wonder....reading all codes again, this is incorrect check. > > > > Hm, let me cralify. IIUC, in old code, "prevent moving" is because you hold > > reference count of cgroup, which can cause trouble at rmdir() as leaking refcnt. > right. > > > BTW, because socket is a shared resource between cgroup, changes in mm->owner > > may cause task cgroup moving implicitly. So, if you allow leak of resource > > here, I guess... you can take mem_cgroup_get() refcnt which is memcg-local and > > allow rmdir(). Then, this limitation may d…
creator: KAMEZAWA Hiroyuki
date: 2011-12-05T02:18:35-00:00

Re: [PATCH v7 02/10] foundations of per-cgroup memory pressure controlling.
type: item
description: On 12/04/2011 11:59 PM, KAMEZAWA Hiroyuki wrote: > On Fri, 2 Dec 2011 15:46:46 -0200 > Glauber Costa<glommer@parallels.com> wrote: > >> >>>> static void proto_seq_printf(struct seq_file *seq, struct proto *proto) >>>> { >>>> + struct mem_cgroup *memcg = mem_cgroup_from_task(current); >>>> + >>>> seq_printf(seq, "%-9s %4u %6d %6ld %-3s %6u %-3s %-10s " >>>> "%2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c %2c\n", >>>> proto->name, >>>> proto->obj_size, >>>> sock_prot_inuse_get(seq_file_net(seq), proto), >>>> - proto->memory_allocated != NULL ? atomic_long_read(proto->memory_allocated) : -1L, >>>> - proto->memory_pressure != NULL ? *proto->memory_pressure ? "yes" : "no" : "NI", >>>> + sock_prot_memory_allocated(proto, memcg), >>>> + sock_prot_memory_pressure(proto, memcg), >>> >>> I wonder I should say NO, here. (Networking guys are ok ??) >>> >>> IIUC, this means there is no way to see aggregated sockstat of all system. >>> And the result depends on the cgroup which the caller is under control. >>> >>> I think you should show aggregated sockstat(global + per-memcg) here and >>> show per-memcg ones via /cgroup interface or add private_sockstat to show >>> per cgroup summary. >>> >> >> Hi Kame, >> >> Yes, the statistics displayed depends on which cgroup you live. >> Also, note that the parent cgroup here is always updated (even when >> use_hierarchy is set to 0). So it is always possible to grab global >> statistics, by being in the root cgroup. >> >> For the others, I believe it to be a question of naturalization. Any >> tool that is fetching these values is likely interested in the amount of >> resources available/used. When you are on a cgroup, the amount of >> resources available/used changes, so that's what you should see. >> >> Also brings the point of resource isolation: if you shouldn't interfere >> with other set of process' resources, there is no reason for you to see >> them in the first place. >> >> So given all that, I believe that …
creator: Glauber Costa
date: 2011-12-05T09:06:20-00:00

Re: [PATCH v7 00/10] Request for Inclusion: per-cgroup tcp memory pressure
type: item
description: On 12/05/2011 12:06 AM, KAMEZAWA Hiroyuki wrote: > On Fri, 2 Dec 2011 16:04:08 -0200 > Glauber Costa<glommer@parallels.com> wrote: > >> On 11/30/2011 12:11 AM, KAMEZAWA Hiroyuki wrote: >>> On Tue, 29 Nov 2011 21:56:51 -0200 >>> Glauber Costa<glommer@parallels.com> wrote: >>> >>>> Hi, >>>> >>>> This patchset implements per-cgroup tcp memory pressure controls. It did not change >>>> significantly since last submission: rather, it just merges the comments Kame had. >>>> Most of them are style-related and/or Documentation, but there are two real bugs he >>>> managed to spot (thanks) >>>> >>>> Please let me know if there is anything else I should address. >>>> >>> >>> After reading all codes again, I feel some strange. Could you clarify ? >>> >>> Here. >>> == >>> +void sock_update_memcg(struct sock *sk) >>> +{ >>> + /* right now a socket spends its whole life in the same cgroup */ >>> + if (sk->sk_cgrp) { >>> + WARN_ON(1); >>> + return; >>> + } >>> + if (static_branch(&memcg_socket_limit_enabled)) { >>> + struct mem_cgroup *memcg; >>> + >>> + BUG_ON(!sk->sk_prot->proto_cgroup); >>> + >>> + rcu_read_lock(); >>> + memcg = mem_cgroup_from_task(current); >>> + if (!mem_cgroup_is_root(memcg)) >>> + sk->sk_cgrp = sk->sk_prot->proto_cgroup(memcg); >>> + rcu_read_unlock(); >>> == >>> >>> sk->sk_cgrp is set to a memcg without any reference count. >>> >>> Then, no check for preventing rmdir() and freeing memcgroup. >>> >>> Is there some css_get() or mem_cgroup_get() somewhere ? >>> >> >> There were a css_get in the first version of this patchset. It was >> removed, however, because it was deemed anti-intuitive to prevent rmdir, >> since we can't know which sockets are blocking it, or do anything about >> it. Or did I misunderstand something ? >> > > Maybe I misuderstood. Thank you. Ok, there is no css_get/put and > rmdir() is allowed. But, hmm....what's guarding threads from stale > pointer access ? > > Does a memory cgroup which is pointed by sk->sk_cgrp always exist ? > If I am …
creator: Glauber Costa
date: 2011-12-05T09:09:51-00:00

Re: [PATCH v7 10/10] Disable task moving when using kernel memory accounting
type: item
description: On 12/05/2011 12:18 AM, KAMEZAWA Hiroyuki wrote: > On Fri, 2 Dec 2011 16:11:56 -0200 > Glauber Costa<glommer@parallels.com> wrote: > >> On 11/30/2011 12:22 AM, KAMEZAWA Hiroyuki wrote: >>> On Tue, 29 Nov 2011 21:57:01 -0200 >>> Glauber Costa<glommer@parallels.com> wrote: >>> >>>> Since this code is still experimental, we are leaving the exact >>>> details of how to move tasks between cgroups when kernel memory >>>> accounting is used as future work. >>>> >>>> For now, we simply disallow movement if there are any pending >>>> accounted memory. >>>> >>>> Signed-off-by: Glauber Costa<glommer@parallels.com> >>>> CC: Hiroyouki Kamezawa<kamezawa.hiroyu@jp.fujitsu.com> >>>> --- >>>> mm/memcontrol.c | 23 ++++++++++++++++++++++- >>>> 1 files changed, 22 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >>>> index a31a278..dd9a6d9 100644 >>>> --- a/mm/memcontrol.c >>>> +++ b/mm/memcontrol.c >>>> @@ -5453,10 +5453,19 @@ static int mem_cgroup_can_attach(struct cgroup_subsys *ss, >>>> { >>>> int ret = 0; >>>> struct mem_cgroup *memcg = mem_cgroup_from_cont(cgroup); >>>> + struct mem_cgroup *from = mem_cgroup_from_task(p); >>>> + >>>> +#if defined(CONFIG_CGROUP_MEM_RES_CTLR_KMEM)&& defined(CONFIG_INET) >>>> + if (from != memcg&& !mem_cgroup_is_root(from)&& >>>> + res_counter_read_u64(&from->tcp_mem.tcp_memory_allocated, RES_USAGE)) { >>>> + printk(KERN_WARNING "Can't move tasks between cgroups: " >>>> + "Kernel memory held.\n"); >>>> + return 1; >>>> + } >>>> +#endif >>> >>> I wonder....reading all codes again, this is incorrect check. >>> >>> Hm, let me cralify. IIUC, in old code, "prevent moving" is because you hold >>> reference count of cgroup, which can cause trouble at rmdir() as leaking refcnt. >> right. >> >>> BTW, because socket is a shared resource between cgroup, changes in mm->owner >>> may cause task cgroup moving implicitly. So, if you allow leak of resource >>> here, I guess... you can take mem_cgroup_get() refcnt which is memcg…
creator: Glauber Costa
date: 2011-12-05T09:18:37-00:00

Re: [PATCH v7 00/10] Request for Inclusion: per-cgroup tcp memory pressure
type: item
description: On Mon, 5 Dec 2011 07:09:51 -0200 Glauber Costa <glommer@parallels.com> wrote: > On 12/05/2011 12:06 AM, KAMEZAWA Hiroyuki wrote: > > On Fri, 2 Dec 2011 16:04:08 -0200 > > Glauber Costa<glommer@parallels.com> wrote: > > > >> On 11/30/2011 12:11 AM, KAMEZAWA Hiroyuki wrote: > >>> On Tue, 29 Nov 2011 21:56:51 -0200 > >>> Glauber Costa<glommer@parallels.com> wrote: > >>> > >>>> Hi, > >>>> > >>>> This patchset implements per-cgroup tcp memory pressure controls. It did not change > >>>> significantly since last submission: rather, it just merges the comments Kame had. > >>>> Most of them are style-related and/or Documentation, but there are two real bugs he > >>>> managed to spot (thanks) > >>>> > >>>> Please let me know if there is anything else I should address. > >>>> > >>> > >>> After reading all codes again, I feel some strange. Could you clarify ? > >>> > >>> Here. > >>> == > >>> +void sock_update_memcg(struct sock *sk) > >>> +{ > >>> + /* right now a socket spends its whole life in the same cgroup */ > >>> + if (sk->sk_cgrp) { > >>> + WARN_ON(1); > >>> + return; > >>> + } > >>> + if (static_branch(&memcg_socket_limit_enabled)) { > >>> + struct mem_cgroup *memcg; > >>> + > >>> + BUG_ON(!sk->sk_prot->proto_cgroup); > >>> + > >>> + rcu_read_lock(); > >>> + memcg = mem_cgroup_from_task(current); > >>> + if (!mem_cgroup_is_root(memcg)) > >>> + sk->sk_cgrp = sk->sk_prot->proto_cgroup(memcg); > >>> + rcu_read_unlock(); > >>> == > >>> > >>> sk->sk_cgrp is set to a memcg without any reference count. > >>> > >>> Then, no check for preventing rmdir() and freeing memcgroup. > >>> > >>> Is there some css_get() or mem_cgroup_get() somewhere ? > >>> > >> > >> There were a css_get in the first version of this patchset. It was > >> removed, however, because it was deemed anti-intuitive to prevent rmdir, > >> since we can't know which sockets are blocking it, or do anything about > >> it. Or did I misunderstand something ? > >> > > > > Maybe I misuderstood. Thank you. Ok, there is …
creator: KAMEZAWA Hiroyuki
date: 2011-12-05T09:51:08-00:00

Re: [PATCH v7 00/10] Request for Inclusion: per-cgroup tcp memory pressure
type: item
description: On 12/05/2011 07:51 AM, KAMEZAWA Hiroyuki wrote: > On Mon, 5 Dec 2011 07:09:51 -0200 > Glauber Costa<glommer@parallels.com> wrote: > >> On 12/05/2011 12:06 AM, KAMEZAWA Hiroyuki wrote: >>> On Fri, 2 Dec 2011 16:04:08 -0200 >>> Glauber Costa<glommer@parallels.com> wrote: >>> >>>> On 11/30/2011 12:11 AM, KAMEZAWA Hiroyuki wrote: >>>>> On Tue, 29 Nov 2011 21:56:51 -0200 >>>>> Glauber Costa<glommer@parallels.com> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> This patchset implements per-cgroup tcp memory pressure controls. It did not change >>>>>> significantly since last submission: rather, it just merges the comments Kame had. >>>>>> Most of them are style-related and/or Documentation, but there are two real bugs he >>>>>> managed to spot (thanks) >>>>>> >>>>>> Please let me know if there is anything else I should address. >>>>>> >>>>> >>>>> After reading all codes again, I feel some strange. Could you clarify ? >>>>> >>>>> Here. >>>>> == >>>>> +void sock_update_memcg(struct sock *sk) >>>>> +{ >>>>> + /* right now a socket spends its whole life in the same cgroup */ >>>>> + if (sk->sk_cgrp) { >>>>> + WARN_ON(1); >>>>> + return; >>>>> + } >>>>> + if (static_branch(&memcg_socket_limit_enabled)) { >>>>> + struct mem_cgroup *memcg; >>>>> + >>>>> + BUG_ON(!sk->sk_prot->proto_cgroup); >>>>> + >>>>> + rcu_read_lock(); >>>>> + memcg = mem_cgroup_from_task(current); >>>>> + if (!mem_cgroup_is_root(memcg)) >>>>> + sk->sk_cgrp = sk->sk_prot->proto_cgroup(memcg); >>>>> + rcu_read_unlock(); >>>>> == >>>>> >>>>> sk->sk_cgrp is set to a memcg without any reference count. >>>>> >>>>> Then, no check for preventing rmdir() and freeing memcgroup. >>>>> >>>>> Is there some css_get() or mem_cgroup_get() somewhere ? >>>>> >>>> >>>> There were a css_get in the first version of this patchset. It was >>>> removed, however, because it was deemed anti-intuitive to prevent rmdir, >>>> since we can't know which sockets are blocking it, or do anything about >>>> it. Or did I misunderstand something ? >>>>…
creator: Glauber Costa
date: 2011-12-05T10:28:56-00:00

Re: [PATCH v7 10/10] Disable task moving when using kernel memory accounting
type: item
description: On Mon, 5 Dec 2011 07:18:37 -0200 Glauber Costa <glommer@parallels.com> wrote: > On 12/05/2011 12:18 AM, KAMEZAWA Hiroyuki wrote: > > On Fri, 2 Dec 2011 16:11:56 -0200 > > Glauber Costa<glommer@parallels.com> wrote: > > > >> On 11/30/2011 12:22 AM, KAMEZAWA Hiroyuki wrote: > >>> On Tue, 29 Nov 2011 21:57:01 -0200 > >>> Glauber Costa<glommer@parallels.com> wrote: > >>> > >>>> Since this code is still experimental, we are leaving the exact > >>>> details of how to move tasks between cgroups when kernel memory > >>>> accounting is used as future work. > >>>> > >>>> For now, we simply disallow movement if there are any pending > >>>> accounted memory. > >>>> > >>>> Signed-off-by: Glauber Costa<glommer@parallels.com> > >>>> CC: Hiroyouki Kamezawa<kamezawa.hiroyu@jp.fujitsu.com> > >>>> --- > >>>> mm/memcontrol.c | 23 ++++++++++++++++++++++- > >>>> 1 files changed, 22 insertions(+), 1 deletions(-) > >>>> > >>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c > >>>> index a31a278..dd9a6d9 100644 > >>>> --- a/mm/memcontrol.c > >>>> +++ b/mm/memcontrol.c > >>>> @@ -5453,10 +5453,19 @@ static int mem_cgroup_can_attach(struct cgroup_subsys *ss, > >>>> { > >>>> int ret = 0; > >>>> struct mem_cgroup *memcg = mem_cgroup_from_cont(cgroup); > >>>> + struct mem_cgroup *from = mem_cgroup_from_task(p); > >>>> + > >>>> +#if defined(CONFIG_CGROUP_MEM_RES_CTLR_KMEM)&& defined(CONFIG_INET) > >>>> + if (from != memcg&& !mem_cgroup_is_root(from)&& > >>>> + res_counter_read_u64(&from->tcp_mem.tcp_memory_allocated, RES_USAGE)) { > >>>> + printk(KERN_WARNING "Can't move tasks between cgroups: " > >>>> + "Kernel memory held.\n"); > >>>> + return 1; > >>>> + } > >>>> +#endif > >>> > >>> I wonder....reading all codes again, this is incorrect check. > >>> > >>> Hm, let me cralify. IIUC, in old code, "prevent moving" is because you hold > >>> reference count of cgroup, which can cause trouble at rmdir() as leaking refcnt. > >> right. > >> > >>> BTW, because socket is a shared resource between cgroup,…
creator: KAMEZAWA Hiroyuki
date: 2011-12-06T00:07:26-00:00