memcg: tcp memcontrol fixes.

Now, tcp memory control cgroup ignores memcg's use_hierarchy value and act as use_hierarchy=1 always. After this patch, tcp memcontrol will work as memcg is designed.

Note:
I know there is a discussion to remove use_hierarchy but this is BUG, now.

Quoted 8 lines · KAMEZAWA Hiroyuki

Now, tcp memory control cgroup ignores memcg's use_hierarchy value and act as use_hierarchy=1 always. After this patch, tcp memcontrol will work as memcg is designed.

Note:
I know there is a discussion to remove use_hierarchy but this is BUG, now.

Kame,

Are you sure about that?

I just tried it myself, and it seems to work:

root@inf5072-11:~/glommer-temporary/a/b# cat memory.kmem.tcp.usage_in_bytes
724992
root@inf5072-11:~/glommer-temporary/a/b# cat
../memory.kmem.tcp.usage_in_bytes
0

Did you got this conclusion through testing or code inspection?

As a matter of fact, that's why I believe the current behavior is indeed correct:

the res_counter is initialized as:

        parent_cg = tcp_prot.proto_cgroup(parent);
        if (parent_cg)
                res_parent = parent_cg->memory_allocated;

        res_counter_init(&tcp->tcp_memory_allocated, res_parent);

now, parent is drawn from parent_mem_cgroup(), that reads as follows:

struct mem_cgroup *parent_mem_cgroup(struct mem_cgroup *memcg)

{
        if (!memcg->res.parent)
                return NULL;
        return mem_cgroup_from_res_counter(memcg->res.parent, res);
}

so if we have use_hierarchy = 0, res.parent should be NULL (because that is the way we initialize it)

tcp memcontrol uses static_branch to optimize limit=RESOURCE_MAX case. If all cgroup's limit=RESOUCE_MAX, resource usage is not accounted. But it's buggy now.

For example, do following

  # while sleep 1;do
    echo 9223372036854775807>  /cgroup/memory/A/memory.kmem.tcp.limit_in_bytes;
    echo 300M>  /cgroup/memory/A/memory.kmem.tcp.limit_in_bytes;
    done

and run network application under A. tcp's usage is sometimes accounted and sometimes not accounted because of frequent changes of static_branch. Then, finally, you can see broken tcp.usage_in_bytes.
WARN_ON() is printed because res_counter->usage goes below 0.
==
kernel: ------------[ cut here ]----------
kernel: WARNING: at kernel/res_counter.c:96 res_counter_uncharge_locked+0x37/0x40()
<snip>
kernel: Pid: 17753, comm: bash Tainted: G W 3.3.0+ #99
kernel: Call Trace:
kernel:<IRQ> [<ffffffff8104cc9f>] warn_slowpath_common+0x7f/0xc0
kernel: [<ffffffff810d7e88>] ? rb_reserve__next_event+0x68/0x470
kernel: [<ffffffff8104ccfa>] warn_slowpath_null+0x1a/0x20
kernel: [<ffffffff810b4e37>] res_counter_uncharge_locked+0x37/0x40
...
==

This patch removes static_branch_slow_dec() at changing res_counter's limit to RESOUCE_MAX. By this, once accounting started, the accountting will continue until the tcp cgroup is destroyed.

I think this will not be problem in real use.

Quoted 31 lines · KAMEZAWA Hiroyuki

tcp memcontrol uses static_branch to optimize limit=RESOURCE_MAX case. If all cgroup's limit=RESOUCE_MAX, resource usage is not accounted. But it's buggy now.

For example, do following

  # while sleep 1;do
    echo 9223372036854775807>  /cgroup/memory/A/memory.kmem.tcp.limit_in_bytes;
    echo 300M>  /cgroup/memory/A/memory.kmem.tcp.limit_in_bytes;
    done

and run network application under A. tcp's usage is sometimes accounted and sometimes not accounted because of frequent changes of static_branch. Then, finally, you can see broken tcp.usage_in_bytes.
WARN_ON() is printed because res_counter->usage goes below 0.
==
kernel: ------------[ cut here ]----------
kernel: WARNING: at kernel/res_counter.c:96 res_counter_uncharge_locked+0x37/0x40()
<snip>
kernel: Pid: 17753, comm: bash Tainted: G W 3.3.0+ #99
kernel: Call Trace:
kernel:<IRQ> [<ffffffff8104cc9f>] warn_slowpath_common+0x7f/0xc0
kernel: [<ffffffff810d7e88>] ? rb_reserve__next_event+0x68/0x470
kernel: [<ffffffff8104ccfa>] warn_slowpath_null+0x1a/0x20
kernel: [<ffffffff810b4e37>] res_counter_uncharge_locked+0x37/0x40
...
==

This patch removes static_branch_slow_dec() at changing res_counter's limit to RESOUCE_MAX. By this, once accounting started, the accountting will continue until the tcp cgroup is destroyed.

Quoted 2 lines

I think this will not be problem in real use.

So...

Are the warnings still there if you have your other patch in this series?

Maybe what we should do is, flush the resource counters so they go back to 0 besides decrementing the static branch. This way we get a more consistent behavior.

Another thing to keep in mind, is that the static branch will only be inactive if we turn off *all* controllers. You see this happening because you are only testing with one.

So even if we go to the route you're proposing, we could probably try doing something on the
global level, instead of a per-memcg boolean flat.

Quoted 32 lines · Glauber Costa

Quoted 31 lines · KAMEZAWA Hiroyuki

tcp memcontrol uses static_branch to optimize limit=RESOURCE_MAX case. If all cgroup's limit=RESOUCE_MAX, resource usage is not accounted. But it's buggy now.

For example, do following

   # while sleep 1;do
     echo 9223372036854775807>   /cgroup/memory/A/memory.kmem.tcp.limit_in_bytes;
     echo 300M>   /cgroup/memory/A/memory.kmem.tcp.limit_in_bytes;
     done

and run network application under A. tcp's usage is sometimes accounted and sometimes not accounted because of frequent changes of static_branch. Then, finally, you can see broken tcp.usage_in_bytes.
WARN_ON() is printed because res_counter->usage goes below 0.
==
kernel: ------------[ cut here ]----------
kernel: WARNING: at kernel/res_counter.c:96 res_counter_uncharge_locked+0x37/0x40()
<snip>
kernel: Pid: 17753, comm: bash Tainted: G W 3.3.0+ #99
kernel: Call Trace:
kernel:<IRQ> [<ffffffff8104cc9f>] warn_slowpath_common+0x7f/0xc0
kernel: [<ffffffff810d7e88>] ? rb_reserve__next_event+0x68/0x470
kernel: [<ffffffff8104ccfa>] warn_slowpath_null+0x1a/0x20
kernel: [<ffffffff810b4e37>] res_counter_uncharge_locked+0x37/0x40
...
==

This patch removes static_branch_slow_dec() at changing res_counter's limit to RESOUCE_MAX. By this, once accounting started, the accountting will continue until the tcp cgroup is destroyed.

Quoted 6 lines

Quoted 2 lines

I think this will not be problem in real use.

So...

Are the warnings still there if you have your other patch in this series?

I wrote patch 3/3 after 2/3 because I found all case cannot be fixed by this.

So, comparing patch 3/3 this fixes is leaking.
Considering following sequence

enable accounting
tcp allocate buffer
disable accounting
tcp free buffer

The accounted usage nerver disappear. This is the probelem which cannot be covered by patch 3/3. Maybe it's better to change order of patches 3/3 -> 2/3 and describe this explicitly.

Quoted 4 lines

Maybe what we should do is, flush the resource counters so they go back to 0 besides decrementing the static branch. This way we get a more consistent behavior.

set all memcg's usage to be 0 at enable/disable accounting ? But, there is a problem which static_branch() update is slow. So, IIUC, we can't catch all cases because of races.

Quoted 3 lines

Another thing to keep in mind, is that the static branch will only be inactive if we turn off *all* controllers. You see this happening because you are only testing with one.

yes. So, the behavior change by this patch will not affect usual cases.

Quoted 3 lines

So even if we go to the route you're proposing, we could probably try doing something on the
global level, instead of a per-memcg boolean flat.

In global level, static_key's counter handles it.

tcp memcontrol starts accouting after res->limit is set. So, if a sockets starts before setting res->limit, there are already used resource. After setting res->limit, the resource (already used) will be uncharged and make res_counter below 0 because they are not charged. This causes warning.

This patch fixes that by adding res_counter_uncharge_nowarn(). (*) We cannot avoid this while we have 'account start' switch.

Signed-off-by KAMEZAWA Hiroyuki<[email protected]>

From 4ab80f84bbcb02a790342426c1de84aeb17fcbe9 Mon Sep 17 00:00:00 2001

Maybe what we can do before lsf/mm summit will be this (avoid warning.) This patch is onto linus's git tree. Patch description is updated.

Thanks.
-Kame
==
From 4ab80f84bbcb02a790342426c1de84aeb17fcbe9 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki<[email protected]>
Date: Thu, 29 Mar 2012 14:59:04 +0900
Subject: [PATCH] memcg/tcp: fix warning caused b res->usage go to negative.

tcp memcontrol starts accouting after res->limit is set. So, if a sockets starts before setting res->limit, there are already used resource. At setting res->limit, accounting starts. The resource will be uncharged and make res_counter below 0 because they are not charged. This causes warning.

From c40bbd69cbb655b6389c2398ce89abb06e64910d Mon Sep 17 00:00:00 2001

Quoted 11 lines · KAMEZAWA Hiroyuki

Maybe what we can do before lsf/mm summit will be this (avoid warning.) This patch is onto linus's git tree. Patch description is updated.

Thanks.
-Kame
==
From 4ab80f84bbcb02a790342426c1de84aeb17fcbe9 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki<[email protected]>
Date: Thu, 29 Mar 2012 14:59:04 +0900
Subject: [PATCH] memcg/tcp: fix warning caused b res->usage go to negative.

Quoted 6 lines

tcp memcontrol starts accouting after res->limit is set. So, if a sockets starts before setting res->limit, there are already used resource. At setting res->limit, accounting starts. The resource will be uncharged and make res_counter below 0 because they are not charged. This causes warning.

Kame,

Please test the following patch and see if it fixes your problems (I tested locally, and it triggers me no warnings running the test script you provided + an inbound scp -r copy of an iso directory from a remote machine)

When you are reviewing, keep in mind that we're likely to have the same problems with slab jump labels - since the slab pages will outlive the cgroup as well, and it might be worthy to keep this in mind, and provide a central point for the jump labels to be set of on cgroup destruction.

Hm. What happens in following sequence ?

1. a memcg is created
2. put a task into the memcg, start tcp steam
3. set tcp memory limit

The resource used between 2 and 3 will cause the problem finally.

Then, Dave's request
==
You must either:

1) Integrate the socket's existing usage when the limit is set.

2) Avoid accounting completely for a socket that started before the limit was set.
==
are not satisfied. So, we need to have a state per sockets, it's accounted or not. I'll look into this problem again, today.

Quoted 7 lines · KAMEZAWA Hiroyuki

Hm. What happens in following sequence ?

1. a memcg is created
2. put a task into the memcg, start tcp steam
3. set tcp memory limit

The resource used between 2 and 3 will cause the problem finally.

I don't get it. if a task is in memcg, but no limit is set, that socket will be assigned null memcg, and will stay like that forever. Only new sockets will have the new memcg pointer.

And previously, we could have the memcg pointer alive, but the jump labels to be disabled. With the patch I posted, this can't happen anymore, since the jump labels are guaranteed to live throughout the whole socket life.

Quoted 4 lines

Then, Dave's request
==
You must either:

Quoted 8 lines

1) Integrate the socket's existing usage when the limit is set.

2) Avoid accounting completely for a socket that started before the limit was set.
==
are not satisfied. So, we need to have a state per sockets, it's accounted or not. I'll look into this problem again, today.

Of course they are.

Every socket created before we set the limit is not accounted. This is 2) that Dave mentioned, and it was *always* this way.

The problem here was the opposite: You could disable the jump labels with sockets still in flight, because we were disabling it based on the limit being set back to unlimited.

What this patch does, is defer that until the last socket limited dies.

Quoted 33 lines · Glauber Costa

Quoted 7 lines · KAMEZAWA Hiroyuki

Hm. What happens in following sequence ?

1. a memcg is created
2. put a task into the memcg, start tcp steam
3. set tcp memory limit

The resource used between 2 and 3 will cause the problem finally.

I don't get it. if a task is in memcg, but no limit is set, that socket will be assigned null memcg, and will stay like that forever. Only new sockets will have the new memcg pointer.

And previously, we could have the memcg pointer alive, but the jump labels to be disabled. With the patch I posted, this can't happen anymore, since the jump labels are guaranteed to live throughout the whole socket life.

Quoted 4 lines

Then, Dave's request
==
You must either:

Quoted 8 lines

1) Integrate the socket's existing usage when the limit is set.

2) Avoid accounting completely for a socket that started before the limit was set.
==
are not satisfied. So, we need to have a state per sockets, it's accounted or not. I'll look into this problem again, today.

Of course they are.

Quoted 9 lines

Every socket created before we set the limit is not accounted. This is 2) that Dave mentioned, and it was *always* this way.

The problem here was the opposite: You could disable the jump labels with sockets still in flight, because we were disabling it based on the limit being set back to unlimited.

What this patch does, is defer that until the last socket limited dies.

Okay, there is an additional thing to be considered here:

Due to the nature of how jump label works, once they are enabled for one of the cgroups, they will be enabled for all of them. So the patch I sent may still break in some scenarios because of the way we record that the limit was set.

However, if my theory behind what is causing the problem is correct, this patch should fix the issue for you.

Let me know if it does, and

I'll work on the final solution.

The problem is that jump_label updating is not atomic_ops.

I'm _not_ sure the update order of the jump_label in sock_update_memcg() and other jump instructions inserted at accounting.

For example, if the jump instruction in sock_update_memcg() is updated 1st and others are updated later, it's unclear whether sockets which has _valid_ sock->sk_cgrp will be accounted or not because accounting jump instruction may not be updated yet.

Hopefully, label in sock_update_memcg should be updated last...

Hm. If I do, I'll add one more key as:

atomic_t sock_should_memcg_aware;

And update 2 keys in following order.

At enable

	static_key_slow_inc(&memcg_socket_limit_enabled)
	atomic_inc(&sock_should_memcg_aware);

At disable

	atomic_dec(&sock_should_memcg_aware);
	static_key_slow_dec(&memcg_socket_limit_enabled)

And
==
void sock_update_memcg(struct sock *sk)

{
         if (atomic_read(&sock_should_memcg_aware)) {

==

Quoted 7 lines · KAMEZAWA Hiroyuki

Hm. What happens in following sequence ?

1. a memcg is created
2. put a task into the memcg, start tcp steam
3. set tcp memory limit

The resource used between 2 and 3 will cause the problem finally.

I don't get it. if a task is in memcg, but no limit is set, that socket will be assigned null memcg, and will stay like that forever. Only new sockets will have the new memcg pointer.

And previously, we could have the memcg pointer alive, but the jump labels to be disabled. With the patch I posted, this can't happen anymore, since the jump labels are guaranteed to live throughout the whole socket life.

Quoted 4 lines

Then, Dave's request
==
You must either:

Quoted 8 lines

1) Integrate the socket's existing usage when the limit is set.

2) Avoid accounting completely for a socket that started before the limit was set.
==
are not satisfied. So, we need to have a state per sockets, it's accounted or not. I'll look into this problem again, today.

Of course they are.

Every socket created before we set the limit is not accounted. This is 2) that Dave mentioned, and it was *always* this way.

The problem here was the opposite: You could disable the jump labels with sockets still in flight, because we were disabling it based on the limit being set back to unlimited.

What this patch does, is defer that until the last socket limited dies.

Quoted 26 lines · Glauber Costa

Quoted 11 lines · KAMEZAWA Hiroyuki

Maybe what we can do before lsf/mm summit will be this (avoid warning.) This patch is onto linus's git tree. Patch description is updated.

Thanks.
-Kame
==
From 4ab80f84bbcb02a790342426c1de84aeb17fcbe9 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki<[email protected]>
Date: Thu, 29 Mar 2012 14:59:04 +0900
Subject: [PATCH] memcg/tcp: fix warning caused b res->usage go to negative.

Quoted 6 lines

tcp memcontrol starts accouting after res->limit is set. So, if a sockets starts before setting res->limit, there are already used resource. At setting res->limit, accounting starts. The resource will be uncharged and make res_counter below 0 because they are not charged. This causes warning.

Kame,

Please test the following patch and see if it fixes your problems (I tested locally, and it triggers me no warnings running the test script you provided + an inbound scp -r copy of an iso directory from a remote machine)

Quoted 5 lines

When you are reviewing, keep in mind that we're likely to have the same problems with slab jump labels - since the slab pages will outlive the cgroup as well, and it might be worthy to keep this in mind, and provide a central point for the jump labels to be set of on cgroup destruction.

Hm. What happens in following sequence ?

1. a memcg is created
2. put a task into the memcg, start tcp steam
3. set tcp memory limit

The resource used between 2 and 3 will cause the problem finally.

Then, Dave's request
==
You must either:

1) Integrate the socket's existing usage when the limit is set.

2) Avoid accounting completely for a socket that started before the limit was set.
==
are not satisfied. So, we need to have a state per sockets, it's accounted or not. I'll look into this problem again, today.

From cdebff23dceeb9d35fd27e5988748a506bb47985 Mon Sep 17 00:00:00 2001

Quoted 47 lines · KAMEZAWA Hiroyuki

Quoted 26 lines · Glauber Costa

Quoted 11 lines · KAMEZAWA Hiroyuki

Maybe what we can do before lsf/mm summit will be this (avoid warning.) This patch is onto linus's git tree. Patch description is updated.

Thanks.
-Kame
==
From 4ab80f84bbcb02a790342426c1de84aeb17fcbe9 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki<[email protected]>
Date: Thu, 29 Mar 2012 14:59:04 +0900
Subject: [PATCH] memcg/tcp: fix warning caused b res->usage go to negative.

Quoted 6 lines

tcp memcontrol starts accouting after res->limit is set. So, if a sockets starts before setting res->limit, there are already used resource. At setting res->limit, accounting starts. The resource will be uncharged and make res_counter below 0 because they are not charged. This causes warning.

Kame,

Please test the following patch and see if it fixes your problems (I tested locally, and it triggers me no warnings running the test script you provided + an inbound scp -r copy of an iso directory from a remote machine)

Quoted 5 lines

When you are reviewing, keep in mind that we're likely to have the same problems with slab jump labels - since the slab pages will outlive the cgroup as well, and it might be worthy to keep this in mind, and provide a central point for the jump labels to be set of on cgroup destruction.

Hm. What happens in following sequence ?

1. a memcg is created
2. put a task into the memcg, start tcp steam
3. set tcp memory limit

The resource used between 2 and 3 will cause the problem finally.

Then, Dave's request
==
You must either:

Quoted 8 lines

1) Integrate the socket's existing usage when the limit is set.

2) Avoid accounting completely for a socket that started before the limit was set.
==
are not satisfied. So, we need to have a state per sockets, it's accounted or not. I'll look into this problem again, today.

Kame,

Let me know what you think of the attached fix.
I still need to compile test it in other configs to be sure it doesn't break, etc. But let's agree on it first.

Subject: [PATCH] decrement static keys on real destroy time

We call the destroy function when a cgroup starts to be removed, such as by a rmdir event.

However, because of our reference counters, some objects are still inflight. Right now, we are decrementing the static_keys at destroy() time, meaning that if we get rid of the last static_key reference, some objects will still have charges, but the code to properly uncharge them won't be run.

This becomes a problem specially if it is ever enabled again, because now new charges will be added to the staled charges making keeping it pretty much impossible.

We just need to be careful with the static branch activation:
since there is no particular preferred order of their activation, we need to make sure that we only start using it after all call sites are active. This is achieved by having a per-memcg flag that is only updated after static_key_slow_inc() returns. At this time, we are sure all sites are active.

This is made per-memcg, not global, for a reason:
it also has the effect of making socket accounting more consistent. The first memcg to be limited will trigger static_key() activation, therefore, accounting. But all the others will then be accounted no matter what. After this patch, only limited memcgs will have its sockets accounted.

[v2: changed a tcp limited flag for a generic proto limited flag ]

Signed-off-by Glauber Costa <[email protected]>

---
 include/net/sock.h        |    1 +
 mm/memcontrol.c           |   20 ++++++++++++++++++--
 net/ipv4/tcp_memcontrol.c |   12 ++++++------
 3 files changed, 25 insertions(+), 8 deletions(-)

include/net/sock.h +0 -0

index b3ebe6b..f35ff7d 100644

include/net/sock.h +0 -0

 	/*
 	 * memcg field is used to find which memcg we belong directly
 	 * Each memcg struct can hold more than one cg_proto, so container_of

mm/memcontrol.c +1 -0

index 02b01d2..61f2d31 100644
@@ -404,6 +404,7 @@ void sock_update_memcg(struct sock *sk)
 {
 	if (mem_cgroup_sockets_enabled) {
 		struct mem_cgroup *memcg;
+		struct cg_proto *cg_proto;
 
 		BUG_ON(!sk->sk_prot->proto_cgroup);
 

mm/memcontrol.c +14 -0

 		rcu_read_unlock();
 	}

@@ -442,6 +444,14 @@ void sock_release_memcg(struct sock *sk)
 	}
 }
 
+static void disarm_static_keys(struct mem_cgroup *memcg)
+{
+#ifdef CONFIG_INET
+	if (memcg->tcp_mem.cg_proto.limited)
+		static_key_slow_dec(&memcg_socket_limit_enabled);
+#endif
+}
+
 #ifdef CONFIG_INET
 struct cg_proto *tcp_proto_cgroup(struct mem_cgroup *memcg)
 {

@@ -452,6 +462,11 @@ struct cg_proto *tcp_proto_cgroup(struct mem_cgroup *memcg)
 }
 EXPORT_SYMBOL(tcp_proto_cgroup);
 #endif /* CONFIG_INET */
+#else
+static inline void disarm_static_keys(struct mem_cgroup *memcg)
+{
+}
+
 #endif /* CONFIG_CGROUP_MEM_RES_CTLR_KMEM */
 
 static void drain_all_stock_async(struct mem_cgroup *memcg);

@@ -4883,6 +4898,7 @@ static void __mem_cgroup_put(struct mem_cgroup *memcg, int count)
 {
 	if (atomic_sub_and_test(count, &memcg->refcnt)) {
 		struct mem_cgroup *parent = parent_mem_cgroup(memcg);
+		disarm_static_keys(memcg);
 		__mem_cgroup_free(memcg);
 		if (parent)
 			mem_cgroup_put(parent);

net/ipv4/tcp_memcontrol.c +1 -3

index 1517037..8005667 100644
@@ -54,6 +54,7 @@ int tcp_init_cgroup(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 	cg_proto->sysctl_mem = tcp->tcp_prot_mem;
 	cg_proto->memory_allocated = &tcp->tcp_memory_allocated;
 	cg_proto->sockets_allocated = &tcp->tcp_sockets_allocated;
+	cg_proto->limited = false;
 	cg_proto->memcg = memcg;
 
 	return 0;

@@ -74,9 +75,6 @@ void tcp_destroy_cgroup(struct mem_cgroup *memcg)
 	percpu_counter_destroy(&tcp->tcp_sockets_allocated);
 
 	val = res_counter_read_u64(&tcp->tcp_memory_allocated, RES_LIMIT);
-
-	if (val != RESOURCE_MAX)
-		static_key_slow_dec(&memcg_socket_limit_enabled);
 }
 EXPORT_SYMBOL(tcp_destroy_cgroup);
 

Quoted 103 lines · Glauber Costa

Quoted 47 lines · KAMEZAWA Hiroyuki

Quoted 26 lines · Glauber Costa

Quoted 11 lines · KAMEZAWA Hiroyuki

Maybe what we can do before lsf/mm summit will be this (avoid warning.) This patch is onto linus's git tree. Patch description is updated.

Thanks.
-Kame
==
From 4ab80f84bbcb02a790342426c1de84aeb17fcbe9 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki<[email protected]>
Date: Thu, 29 Mar 2012 14:59:04 +0900
Subject: [PATCH] memcg/tcp: fix warning caused b res->usage go to negative.

Quoted 6 lines

tcp memcontrol starts accouting after res->limit is set. So, if a sockets starts before setting res->limit, there are already used resource. At setting res->limit, accounting starts. The resource will be uncharged and make res_counter below 0 because they are not charged. This causes warning.

Kame,

Please test the following patch and see if it fixes your problems (I tested locally, and it triggers me no warnings running the test script you provided + an inbound scp -r copy of an iso directory from a remote machine)

Quoted 5 lines

When you are reviewing, keep in mind that we're likely to have the same problems with slab jump labels - since the slab pages will outlive the cgroup as well, and it might be worthy to keep this in mind, and provide a central point for the jump labels to be set of on cgroup destruction.

Hm. What happens in following sequence ?

1. a memcg is created
2. put a task into the memcg, start tcp steam
3. set tcp memory limit

The resource used between 2 and 3 will cause the problem finally.

Then, Dave's request
==
You must either:

Quoted 8 lines

1) Integrate the socket's existing usage when the limit is set.

2) Avoid accounting completely for a socket that started before the limit was set.
==
are not satisfied. So, we need to have a state per sockets, it's accounted or not. I'll look into this problem again, today.

Kame,

Let me know what you think of the attached fix.
I still need to compile test it in other configs to be sure it doesn't break, etc. But let's agree on it first.

Subject: [PATCH] decrement static keys on real destroy time

We call the destroy function when a cgroup starts to be removed, such as by a rmdir event.

However, because of our reference counters, some objects are still inflight. Right now, we are decrementing the static_keys at destroy() time, meaning that if we get rid of the last static_key reference, some objects will still have charges, but the code to properly uncharge them won't be run.

This becomes a problem specially if it is ever enabled again, because now new charges will be added to the staled charges making keeping it pretty much impossible.

We just need to be careful with the static branch activation:
since there is no particular preferred order of their activation, we need to make sure that we only start using it after all call sites are active. This is achieved by having a per-memcg flag that is only updated after static_key_slow_inc() returns. At this time, we are sure all sites are active.

This is made per-memcg, not global, for a reason:
it also has the effect of making socket accounting more consistent. The first memcg to be limited will trigger static_key() activation, therefore, accounting. But all the others will then be accounted no matter what. After this patch, only limited memcgs will have its sockets accounted.

[v2: changed a tcp limited flag for a generic proto limited flag ]

Signed-off-by Glauber Costa<[email protected]>

---
  include/net/sock.h        |    1 +
  mm/memcontrol.c           |   20 ++++++++++++++++++--
  net/ipv4/tcp_memcontrol.c |   12 ++++++------
  3 files changed, 25 insertions(+), 8 deletions(-)

include/net/sock.h struct cg_proto { · line 916

Quoted 9 lines

include/net/sock.h +1 -0

index b3ebe6b..f35ff7d 100644
@@ -913,6 +913,7 @@ struct cg_proto {
  	struct percpu_counter	*sockets_allocated;	/* Current number of sockets. */
  	int			*memory_pressure;
  	long			*sysctl_mem;
+	bool			limited;

please add comment somewhere. Hmm, 'limited' is good name ?

Quoted 16 lines

include/net/sock.h +0 -0

  	/*
  	 * memcg field is used to find which memcg we belong directly
  	 * Each memcg struct can hold more than one cg_proto, so container_of

mm/memcontrol.c +1 -0

index 02b01d2..61f2d31 100644
@@ -404,6 +404,7 @@ void sock_update_memcg(struct sock *sk)
  {
  	if (mem_cgroup_sockets_enabled) {
  		struct mem_cgroup *memcg;
+		struct cg_proto *cg_proto;
 
  		BUG_ON(!sk->sk_prot->proto_cgroup);
 

mm/memcontrol.c void sock_update_memcg(struct sock *sk) · line 431

Quoted 10 lines

mm/memcontrol.c +3 -2

@@ -423,9 +424,10 @@ void sock_update_memcg(struct sock *sk)
 
  		rcu_read_lock();
  		memcg = mem_cgroup_from_task(current);
-		if (!mem_cgroup_is_root(memcg)) {
+		cg_proto = sk->sk_prot->proto_cgroup(memcg);
+		if (!mem_cgroup_is_root(memcg)&&  cg_proto->limited) {
  			mem_cgroup_get(memcg);
-			sk->sk_cgrp = sk->sk_prot->proto_cgroup(memcg);
+			sk->sk_cgrp = cg_proto;
  		}

Ok, then, sk->sk_cgroup is set only after jump_label is enabled and its memcg has limitation.

Why we can reset this to be false ? disarm_static_keys() will not work at destroy()....

mm/memcontrol.c void sock_update_memcg(struct sock *sk)

Quoted 6 lines

mm/memcontrol.c +3 -0

+	else if (val != RESOURCE_MAX&&  !cg_proto->limited) {
  		static_key_slow_inc(&memcg_socket_limit_enabled);
+		cg_proto->limited = true;
+	}

Hmm. don't we need any mutex ?

Quoted 5 lines

tcp memcontrol starts accouting after res->limit is set. So, if a sockets starts before setting res->limit, there are already used resource. After setting res->limit, the resource (already used) will be uncharged and make res_counter below 0 because they are not charged. This causes warning.

Quoted 4 lines

This patch fixes that by adding res_counter_uncharge_nowarn(). (*) We cannot avoid this while we have 'account start' switch.

Signed-off-by KAMEZAWA Hiroyuki<[email protected]>

Fine by me.

Acked-by Glauber Costa <[email protected]>

From: Glauber Costa<[email protected]>
Date: Thu, 29 Mar 2012 11:21:07 +0200

Quoted 6 lines · KAMEZAWA Hiroyuki

Quoted 5 lines

tcp memcontrol starts accouting after res->limit is set. So, if a sockets starts before setting res->limit, there are already used resource. After setting res->limit, the resource (already used) will be uncharged and make res_counter below 0 because they are not charged. This causes warning.

Quoted 8 lines

Quoted 4 lines

This patch fixes that by adding res_counter_uncharge_nowarn(). (*) We cannot avoid this while we have 'account start' switch.

Signed-off-by KAMEZAWA Hiroyuki<[email protected]>

Fine by me.

Acked-by Glauber Costa<[email protected]>

I'm not applying patches that simply ignore accounting counter underflows.

You must either:

1) Integrate the socket's existing usage when the limit is set.

2) Avoid accounting completely for a socket that started before the limit was set.

No half-way solutions, please. Otherwise it is impossible to design validations of the resource usage for a particular socket or group of sockets, because they can always be potentially "wrong" and over the limit. That's a design for a buggy system.

Quoted 25 lines · David Miller

From: Glauber Costa<[email protected]>
Date: Thu, 29 Mar 2012 11:21:07 +0200

Quoted 6 lines · KAMEZAWA Hiroyuki

Quoted 5 lines

tcp memcontrol starts accouting after res->limit is set. So, if a sockets starts before setting res->limit, there are already used resource. After setting res->limit, the resource (already used) will be uncharged and make res_counter below 0 because they are not charged. This causes warning.

Quoted 8 lines

Quoted 4 lines

This patch fixes that by adding res_counter_uncharge_nowarn(). (*) We cannot avoid this while we have 'account start' switch.

Signed-off-by KAMEZAWA Hiroyuki<[email protected]>

Fine by me.

Acked-by Glauber Costa<[email protected]>

I'm not applying patches that simply ignore accounting counter underflows.

You must either:

1) Integrate the socket's existing usage when the limit is set.

Quoted 9 lines

2) Avoid accounting completely for a socket that started before the limit was set.

No half-way solutions, please. Otherwise it is impossible to design validations of the resource usage for a particular socket or group of sockets, because they can always be potentially "wrong" and over the limit. That's a design for a buggy system.

Kame,

I agree with Dave FWIW.

We should be able to do this by dropping the reference count when the cgroup is finally destroyed, instead of from the remove callback. At that point, no more pending sockets should be attached to it.

Prior to increasing the static key, they are all assigned to the global cgroup, so we shouldn't care about them.

Quoted 43 lines · Glauber Costa

Quoted 25 lines · David Miller

From: Glauber Costa<[email protected]>
Date: Thu, 29 Mar 2012 11:21:07 +0200

Quoted 6 lines · KAMEZAWA Hiroyuki

Quoted 5 lines

tcp memcontrol starts accouting after res->limit is set. So, if a sockets starts before setting res->limit, there are already used resource. After setting res->limit, the resource (already used) will be uncharged and make res_counter below 0 because they are not charged. This causes warning.

Quoted 8 lines

Quoted 4 lines

This patch fixes that by adding res_counter_uncharge_nowarn(). (*) We cannot avoid this while we have 'account start' switch.

Signed-off-by KAMEZAWA Hiroyuki<[email protected]>

Fine by me.

Acked-by Glauber Costa<[email protected]>

I'm not applying patches that simply ignore accounting counter underflows.

You must either:

1) Integrate the socket's existing usage when the limit is set.

Quoted 9 lines

2) Avoid accounting completely for a socket that started before the limit was set.

No half-way solutions, please. Otherwise it is impossible to design validations of the resource usage for a particular socket or group of sockets, because they can always be potentially "wrong" and over the limit. That's a design for a buggy system.

Kame,

I agree with Dave FWIW.

We should be able to do this by dropping the reference count when the cgroup is finally destroyed, instead of from the remove callback. At that point, no more pending sockets should be attached to it.

Quoted 3 lines

Prior to increasing the static key, they are all assigned to the global cgroup, so we shouldn't care about them.

Could you do the fix ?

Thanks,
-Kame