lkml.org 
[lkml]   [2009]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 0/4] Memory controller soft limit patches (v3)
On Mon, 2 Mar 2009 10:10:43 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-03-02 09:24:04]:
>
> > On Sun, 01 Mar 2009 11:59:59 +0530
> > Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> >
> > >
> > > From: Balbir Singh <balbir@linux.vnet.ibm.com>

> >
> > At first, it's said "When cgroup people adds something, the kernel gets slow".
> > This is my start point of reviewing. Below is comments to this version of patch.
> >
> > 1. I think it's bad to add more hooks to res_counter. It's enough slow to give up
> > adding more fancy things..
>
> res_counters was desgined to be extensible, why is adding anything to
> it going to make it slow, unless we turn on soft_limits?
>
You inserted new "if" logic in the core loop.
(What I want to say here is not that this is definitely bad but that "isn't there
any alternatives which is less overhead.)


> >
> > 2. please avoid to add hooks to hot-path. In your patch, especially a hook to
> > mem_cgroup_uncharge_common() is annoying me.
>
> If soft limits are not enabled, the function does a small check and
> leaves.
>
&soft_fail_res is passed always even if memory.soft_limit==ULONG_MAX
res_counter_soft_limit_excess() adds one more function call and spinlock, and irq-off.

> >
> > 3. please avoid to use global spinlock more.
> > no lock is best. mutex is better, maybe.
> >
>
> No lock to update a tree which is update concurrently?
>
Using tree/sort itself is nonsense, I believe.


> > 4. RB-tree seems broken. Following is example. (please note you do all ops
> > in lazy manner (once in HZ/4.)
> >
> > i). while running, the tree is constructed as following
> >
> > R R=exceed=300M
> > / \
> > A B A=exceed=200M B=exceed=400M
> > ii) A process B exits, but and usage goes down.
>
> That is why we have the hook in uncharge. Even if we update and the
> usage goes down, the tree is ordered by usage_in_excess which is
> updated only when the tree is updated. So what you show below does not
> occur. I think I should document the design better.
>

time_check==true. So, update-tree at uncharge() only happens once in HZ/4
==
@@ -1422,6 +1520,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
mz = page_cgroup_zoneinfo(pc);
unlock_page_cgroup(pc);

+ mem_cgroup_check_and_update_tree(mem, true);
/* at swapout, this memcg will be accessed to record to swap */
if (ctype != MEM_CGROUP_CHARGE_TYPE_SWAPOUT)
css_put(&mem->css);
==
Then, not-sorted RB-tree can be there.

BTW,
time_after(jiffies, 0)
is buggy (see definition). If you want make this true always,
time_after(jiffies, jiffies +1)

Thanks,
-Kame





\
 
 \ /
  Last update: 2009-03-02 06:37    [W:6.440 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site