From: Glauber Costa <glommer@parallels•com>
To: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp•fujitsu.com>
Cc: <netdev@vger•kernel.org>, David Miller <davem@davemloft•net>,
Andrew Morton <akpm@linux-foundation•org>
Subject: Re: [BUGFIX][PATCH 2/3] memcg/tcp: remove static_branch_slow_dec() at changing limit
Date: Fri, 30 Mar 2012 08:18:40 +0200 [thread overview]
Message-ID: <4F755040.5070403@parallels.com> (raw)
In-Reply-To: <4F74F57C.4030001@jp.fujitsu.com>
On 03/30/2012 01:51 AM, KAMEZAWA Hiroyuki wrote:
> (2012/03/29 19:58), Glauber Costa wrote:
>
>> On 03/29/2012 09:07 AM, KAMEZAWA Hiroyuki wrote:
>>> 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.
>
I gave it a bit more thought through the night... and I guess your
solution is okay.
next prev parent reply other threads:[~2012-03-30 6:18 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-03-29 7:01 [BUGFIX][PATCH 0/3] memcg: tcp memcontrol fixes KAMEZAWA Hiroyuki
2012-03-29 7:03 ` [PATCH 1/3] [BUGFIX] memcg/tcp : fix to see use_hierarchy in tcp memcontrol cgroup KAMEZAWA Hiroyuki
2012-03-29 9:14 ` Glauber Costa
2012-03-29 9:16 ` KAMEZAWA Hiroyuki
2012-03-29 7:07 ` [BUGFIX][PATCH 2/3] memcg/tcp: remove static_branch_slow_dec() at changing limit KAMEZAWA Hiroyuki
2012-03-29 10:58 ` Glauber Costa
2012-03-29 23:51 ` KAMEZAWA Hiroyuki
2012-03-30 6:18 ` Glauber Costa [this message]
2012-03-29 7:10 ` [BUGFIX][PATCH 3/3] memcg/tcp: ignore tcp usage before accounting started KAMEZAWA Hiroyuki
2012-03-29 9:21 ` Glauber Costa
2012-03-30 1:44 ` [PATCH] memcg/tcp: fix warning caused b res->usage go to negative KAMEZAWA Hiroyuki
2012-04-06 15:49 ` Glauber Costa
2012-04-10 2:37 ` KAMEZAWA Hiroyuki
2012-04-10 2:51 ` Glauber Costa
2012-04-10 3:01 ` Glauber Costa
2012-04-10 4:15 ` KAMEZAWA Hiroyuki
2012-04-11 2:22 ` Glauber Costa
2012-04-10 3:21 ` KAMEZAWA Hiroyuki
2012-04-13 17:33 ` Glauber Costa
2012-04-18 8:02 ` KAMEZAWA Hiroyuki
2012-04-18 16:32 ` Glauber Costa
2012-04-02 3:41 ` [BUGFIX][PATCH 3/3] memcg/tcp: ignore tcp usage before accounting started David Miller
2012-04-03 22:31 ` Glauber Costa
2012-04-09 0:58 ` KAMEZAWA Hiroyuki
2012-04-09 1:44 ` Glauber Costa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F755040.5070403@parallels.com \
--to=glommer@parallels$(echo .)com \
--cc=akpm@linux-foundation$(echo .)org \
--cc=davem@davemloft$(echo .)net \
--cc=kamezawa.hiroyu@jp$(echo .)fujitsu.com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox