[PATCH 0/3] fixes for tcp memcg

Hi Dave,

Please consider adding the following fixes to 
your tree.

The first is a remaining build bug, that managed to survive
the bunch of randconfigs I tested with from my last fix (It
still breaks without CONFIG_NET, which is likely selected
by a bunch of other options).

The other two patches are a fix for a bug I verified in
my followup testings in the controller itself, that causes
a charge/uncharge mismatch under certain circumnstances.

Thanks

Glauber Costa (3):
  net: fix socket memcg build with !CONFIG_NET
  cgroup: make sure memcg margin is 0 when over limit
  net: introduce res_counter_charge_nofail() for socket allocations

 include/linux/res_counter.h |   11 ++++++++++-
 include/net/sock.h          |   12 +++++-------
 kernel/res_counter.c        |   25 +++++++++++++++++++++++++
 mm/memcontrol.c             |    4 ++--
 net/core/sock.c             |    4 ++--
 5 files changed, 44 insertions(+), 12 deletions(-)

-- 
1.7.7.4

There is still a build bug with the sock memcg code, that triggers
with !CONFIG_NET, that survived my series of randconfig builds.

Signed-off-by: Glauber Costa <redacted>
Reported-by: Randy Dunlap <redacted>
CC: Hiroyouki Kamezawa <redacted>
---
 include/net/sock.h |    2 +-
 mm/memcontrol.c    |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index bb972d2..3cae112 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h

@@ -921,7 +921,7 @@ inline void sk_refcnt_debug_release(const struct sock *sk)
 #define sk_refcnt_debug_release(sk) do { } while (0)
 #endif /* SOCK_REFCNT_DEBUG */
 
-#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM
+#if defined(CONFIG_CGROUP_MEM_RES_CTLR_KMEM) && defined(CONFIG_NET)
 extern struct jump_label_key memcg_socket_limit_enabled;
 static inline struct cg_proto *parent_cg_proto(struct proto *proto,
 					       struct cg_proto *cg_proto)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 3dbff4d..c3688df 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c

@@ -379,7 +379,7 @@ static void mem_cgroup_put(struct mem_cgroup *memcg);
 static bool mem_cgroup_is_root(struct mem_cgroup *memcg);
 void sock_update_memcg(struct sock *sk)
 {
-	if (static_branch(&memcg_socket_limit_enabled)) {
+	if (mem_cgroup_sockets_enabled) {
 		struct mem_cgroup *memcg;
 
 		BUG_ON(!sk->sk_prot->proto_cgroup);

@@ -411,7 +411,7 @@ EXPORT_SYMBOL(sock_update_memcg);
 
 void sock_release_memcg(struct sock *sk)
 {
-	if (static_branch(&memcg_socket_limit_enabled) && sk->sk_cgrp) {
+	if (mem_cgroup_sockets_enabled && sk->sk_cgrp) {
 		struct mem_cgroup *memcg;
 		WARN_ON(!sk->sk_cgrp->memcg);
 		memcg = sk->sk_cgrp->memcg;

-- 
1.7.7.4

For the memcg sock code, we'll need to register allocations
that are temporarily over limit. Let's make sure that margin
is 0 in this case.

I am keeping this as a separate patch, so that if any weirdness
interaction appears in the future, we can now exactly what caused
it.

Suggested by Johannes Weiner 

Signed-off-by: Glauber Costa <redacted>
CC: KAMEZAWA Hiroyuki <redacted>
CC: Johannes Weiner <redacted>
CC: Michal Hocko <redacted>
CC: Tejun Heo <redacted>
CC: Li Zefan <redacted>
---
 include/linux/res_counter.h |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
index c9d625c..d06d014 100644
--- a/include/linux/res_counter.h
+++ b/include/linux/res_counter.h

@@ -142,7 +142,10 @@ static inline unsigned long long res_counter_margin(struct res_counter *cnt)
 	unsigned long flags;
 
 	spin_lock_irqsave(&cnt->lock, flags);
-	margin = cnt->limit - cnt->usage;
+	if (cnt->limit > cnt->usage)
+		margin = cnt->limit - cnt->usage;
+	else
+		margin = 0;
 	spin_unlock_irqrestore(&cnt->lock, flags);
 	return margin;
 }

-- 
1.7.7.4

There is a case in __sk_mem_schedule(), where an allocation
is beyond the maximum, but yet we are allowed to proceed.
It happens under the following condition:

	sk->sk_wmem_queued + size >= sk->sk_sndbuf

The network code won't revert the allocation in this case,
meaning that at some point later it'll try to do it. Since
this is never communicated to the underlying res_counter
code, there is an inbalance in res_counter uncharge operation.

I see two ways of fixing this:

1) storing the information about those allocations somewhere
   in memcg, and then deducting from that first, before
   we start draining the res_counter,
2) providing a slightly different allocation function for
   the res_counter, that matches the original behavior of
   the network code more closely.

I decided to go for #2 here, believing it to be more elegant,
since #1 would require us to do basically that, but in a more
obscure way.

Signed-off-by: Glauber Costa <redacted>
Cc: KAMEZAWA Hiroyuki <redacted>
Cc: Johannes Weiner <redacted>
Cc: Michal Hocko <redacted>
CC: Tejun Heo <redacted>
CC: Li Zefan <redacted>
CC: Laurent Chavey <redacted>
---
 include/linux/res_counter.h |    6 ++++++
 include/net/sock.h          |   10 ++++------
 kernel/res_counter.c        |   25 +++++++++++++++++++++++++
 net/core/sock.c             |    4 ++--
 4 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
index d06d014..da81af0 100644
--- a/include/linux/res_counter.h
+++ b/include/linux/res_counter.h

@@ -109,12 +109,18 @@ void res_counter_init(struct res_counter *counter, struct res_counter *parent);
  *
  * returns 0 on success and <0 if the counter->usage will exceed the
  * counter->limit _locked call expects the counter->lock to be taken
+ *
+ * charge_nofail works the same, except that it charges the resource
+ * counter unconditionally, and returns < 0 if the after the current
+ * charge we are over limit.
  */
 
 int __must_check res_counter_charge_locked(struct res_counter *counter,
 		unsigned long val);
 int __must_check res_counter_charge(struct res_counter *counter,
 		unsigned long val, struct res_counter **limit_fail_at);
+int __must_check res_counter_charge_nofail(struct res_counter *counter,
+		unsigned long val, struct res_counter **limit_fail_at);
 
 /*
  * uncharge - tell that some portion of the resource is released

diff --git a/include/net/sock.h b/include/net/sock.h
index 3cae112..e3f1ea4 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h

@@ -1007,9 +1007,8 @@ static inline void memcg_memory_allocated_add(struct cg_proto *prot,
 	struct res_counter *fail;
 	int ret;
 
-	ret = res_counter_charge(prot->memory_allocated,
-				 amt << PAGE_SHIFT, &fail);
-
+	ret = res_counter_charge_nofail(prot->memory_allocated,
+					amt << PAGE_SHIFT, &fail);
 	if (ret < 0)
 		*parent_status = OVER_LIMIT;
 }

@@ -1053,12 +1052,11 @@ sk_memory_allocated_add(struct sock *sk, int amt, int *parent_status)
 }
 
 static inline void
-sk_memory_allocated_sub(struct sock *sk, int amt, int parent_status)
+sk_memory_allocated_sub(struct sock *sk, int amt)
 {
 	struct proto *prot = sk->sk_prot;
 
-	if (mem_cgroup_sockets_enabled && sk->sk_cgrp &&
-	    parent_status != OVER_LIMIT) /* Otherwise was uncharged already */
+	if (mem_cgroup_sockets_enabled && sk->sk_cgrp)
 		memcg_memory_allocated_sub(sk->sk_cgrp, amt);
 
 	atomic_long_sub(amt, prot->memory_allocated);

diff --git a/kernel/res_counter.c b/kernel/res_counter.c
index 6d269cc..d508363 100644
--- a/kernel/res_counter.c
+++ b/kernel/res_counter.c

@@ -66,6 +66,31 @@ done:
 	return ret;
 }
 
+int res_counter_charge_nofail(struct res_counter *counter, unsigned long val,
+			      struct res_counter **limit_fail_at)
+{
+	int ret, r;
+	unsigned long flags;
+	struct res_counter *c;
+
+	r = ret = 0;
+	*limit_fail_at = NULL;
+	local_irq_save(flags);
+	for (c = counter; c != NULL; c = c->parent) {
+		spin_lock(&c->lock);
+		r = res_counter_charge_locked(c, val);
+		if (r)
+			c->usage += val;
+		spin_unlock(&c->lock);
+		if (r < 0 && ret == 0) {
+			*limit_fail_at = c;
+			ret = r;
+		}
+	}
+	local_irq_restore(flags);
+
+	return ret;
+}
 void res_counter_uncharge_locked(struct res_counter *counter, unsigned long val)
 {
 	if (WARN_ON(counter->usage < val))

diff --git a/net/core/sock.c b/net/core/sock.c
index 5c5af998..3e81fd2 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c

@@ -1827,7 +1827,7 @@ suppress_allocation:
 	/* Alas. Undo changes. */
 	sk->sk_forward_alloc -= amt * SK_MEM_QUANTUM;
 
-	sk_memory_allocated_sub(sk, amt, parent_status);
+	sk_memory_allocated_sub(sk, amt);
 
 	return 0;
 }

@@ -1840,7 +1840,7 @@ EXPORT_SYMBOL(__sk_mem_schedule);
 void __sk_mem_reclaim(struct sock *sk)
 {
 	sk_memory_allocated_sub(sk,
-				sk->sk_forward_alloc >> SK_MEM_QUANTUM_SHIFT, 0);
+				sk->sk_forward_alloc >> SK_MEM_QUANTUM_SHIFT);
 	sk->sk_forward_alloc &= SK_MEM_QUANTUM - 1;
 
 	if (sk_under_memory_pressure(sk) &&

-- 
1.7.7.4

On Fri, Jan 20, 2012 at 06:57:16PM +0400, Glauber Costa wrote:

There is a case in __sk_mem_schedule(), where an allocation
is beyond the maximum, but yet we are allowed to proceed.
It happens under the following condition:

	sk->sk_wmem_queued + size >= sk->sk_sndbuf

The network code won't revert the allocation in this case,
meaning that at some point later it'll try to do it. Since
this is never communicated to the underlying res_counter
code, there is an inbalance in res_counter uncharge operation.

I see two ways of fixing this:

1) storing the information about those allocations somewhere
   in memcg, and then deducting from that first, before
   we start draining the res_counter,
2) providing a slightly different allocation function for
   the res_counter, that matches the original behavior of
   the network code more closely.

I decided to go for #2 here, believing it to be more elegant,
since #1 would require us to do basically that, but in a more
obscure way.

Signed-off-by: Glauber Costa <redacted>
Cc: KAMEZAWA Hiroyuki <redacted>
Cc: Johannes Weiner <redacted>
Cc: Michal Hocko <redacted>
CC: Tejun Heo <redacted>
CC: Li Zefan <redacted>
CC: Laurent Chavey <redacted>

Acked-by: Tejun Heo <redacted>

Thanks.

-- 
tejun

On Fri, Jan 20, 2012 at 06:57:15PM +0400, Glauber Costa wrote:

For the memcg sock code, we'll need to register allocations
that are temporarily over limit. Let's make sure that margin
is 0 in this case.

I am keeping this as a separate patch, so that if any weirdness
interaction appears in the future, we can now exactly what caused
it.

Suggested by Johannes Weiner 

Signed-off-by: Glauber Costa <redacted>
CC: KAMEZAWA Hiroyuki <redacted>
CC: Johannes Weiner <redacted>
CC: Michal Hocko <redacted>
CC: Tejun Heo <redacted>
CC: Li Zefan <redacted>

Acked-by: Tejun Heo <redacted>

Thanks.

-- 
tejun

From: Glauber Costa <redacted>
Date: Fri, 20 Jan 2012 18:57:14 +0400

There is still a build bug with the sock memcg code, that triggers
with !CONFIG_NET, that survived my series of randconfig builds.

Signed-off-by: Glauber Costa <redacted>
Reported-by: Randy Dunlap <rdunlap-/UHa2rfvQTnk1uMJSBkQmQ@public.gmane.org>
CC: Hiroyouki Kamezawa <redacted>

Why are you undoing the static branch optimization?  That was a one of
the things that made me agree to even putting this code in.

Find a way to fix this without removing the static branch.

On 01/20/2012 11:14 PM, David Miller wrote:

From: Glauber Costa<redacted>
Date: Fri, 20 Jan 2012 18:57:14 +0400

quoted

There is still a build bug with the sock memcg code, that triggers
with !CONFIG_NET, that survived my series of randconfig builds.

Signed-off-by: Glauber Costa<redacted>
Reported-by: Randy Dunlap<rdunlap-/UHa2rfvQTnk1uMJSBkQmQ@public.gmane.org>
CC: Hiroyouki Kamezawa<redacted>

Why are you undoing the static branch optimization?  That was a one of
the things that made me agree to even putting this code in.

Find a way to fix this without removing the static branch.

Dave,

I am not removing the static branch. This is just a macro, that expands 
to the static branch when the options are in place, and to 0 if they are 
not.

if (0) { } is certainly be removed to the compiler, so the code is out
anyway.

From: Glauber Costa <redacted>
Date: Sat, 21 Jan 2012 18:17:26 +0400

I am not removing the static branch. This is just a macro, that
expands to the static branch when the options are in place, and to 0
if they are not.

if (0) { } is certainly be removed to the compiler, so the code is out
anyway.

That's rather non-intuitive, oh well.

I'll apply this, but all of this stuff is an ifdef mess.

On Fri, Jan 20, 2012 at 06:57:15PM +0400, Glauber Costa wrote:

For the memcg sock code, we'll need to register allocations
that are temporarily over limit. Let's make sure that margin
is 0 in this case.

I am keeping this as a separate patch, so that if any weirdness
interaction appears in the future, we can now exactly what caused
it.

Suggested by Johannes Weiner 

Signed-off-by: Glauber Costa <redacted>
CC: KAMEZAWA Hiroyuki <redacted>
CC: Johannes Weiner <redacted>
CC: Michal Hocko <redacted>
CC: Tejun Heo <redacted>
CC: Li Zefan <redacted>

Thanks,

Acked-by: Johannes Weiner <redacted>

On Fri, Jan 20, 2012 at 06:57:16PM +0400, Glauber Costa wrote:

There is a case in __sk_mem_schedule(), where an allocation
is beyond the maximum, but yet we are allowed to proceed.
It happens under the following condition:

	sk->sk_wmem_queued + size >= sk->sk_sndbuf

The network code won't revert the allocation in this case,
meaning that at some point later it'll try to do it. Since
this is never communicated to the underlying res_counter
code, there is an inbalance in res_counter uncharge operation.

I see two ways of fixing this:

1) storing the information about those allocations somewhere
   in memcg, and then deducting from that first, before
   we start draining the res_counter,
2) providing a slightly different allocation function for
   the res_counter, that matches the original behavior of
   the network code more closely.

I decided to go for #2 here, believing it to be more elegant,
since #1 would require us to do basically that, but in a more
obscure way.

Signed-off-by: Glauber Costa <redacted>
Cc: KAMEZAWA Hiroyuki <redacted>
Cc: Johannes Weiner <redacted>
Cc: Michal Hocko <redacted>
CC: Tejun Heo <redacted>
CC: Li Zefan <redacted>
CC: Laurent Chavey <redacted>

Acked-by: Johannes Weiner <redacted>