(2012/03/29 19:58), Glauber Costa wrote:

On 03/29/2012 09:07 AM, KAMEZAWA Hiroyuki wrote:

quoted

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.

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.

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.

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.

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.

Thanks,
-Kame