From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx177.postini.com [74.125.245.177]) by kanga.kvack.org (Postfix) with SMTP id F16576B00F5 for ; Fri, 27 Apr 2012 14:26:21 -0400 (EDT) Received: by lagz14 with SMTP id z14so1014171lag.14 for ; Fri, 27 Apr 2012 11:26:20 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <4F9AD455.9030306@parallels.com> References: <4F9A327A.6050409@jp.fujitsu.com> <4F9A34B2.8080103@jp.fujitsu.com> <4F9AD455.9030306@parallels.com> Date: Fri, 27 Apr 2012 11:26:19 -0700 Message-ID: Subject: Re: [RFC][PATCH 4/7 v2] memcg: use res_counter_uncharge_until in move_parent From: Ying Han Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Sender: owner-linux-mm@kvack.org List-ID: To: Glauber Costa Cc: KAMEZAWA Hiroyuki , Linux Kernel , "linux-mm@kvack.org" , "cgroups@vger.kernel.org" , Michal Hocko , Johannes Weiner , Frederic Weisbecker , Tejun Heo , "Aneesh Kumar K.V" , Andrew Morton , kamezawa.hiroyuki@gmail.com On Fri, Apr 27, 2012 at 10:16 AM, Glauber Costa wro= te: > On 04/27/2012 02:54 AM, KAMEZAWA Hiroyuki wrote: >> By using res_counter_uncharge_until(), we can avoid >> unnecessary charging. >> >> Signed-off-by: KAMEZAWA Hiroyuki >> --- >> =A0 mm/memcontrol.c | =A0 63 ++++++++++++++++++++++++++++++++++++-------= ----------- >> =A0 1 files changed, 42 insertions(+), 21 deletions(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 613bb15..ed53d64 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -2420,6 +2420,24 @@ static void __mem_cgroup_cancel_charge(struct mem= _cgroup *memcg, >> =A0 } >> >> =A0 /* >> + * Cancel chages in this cgroup....doesn't propagates to parent cgroup. >> + * This is useful when moving usage to parent cgroup. >> + */ >> +static void __mem_cgroup_cancel_local_charge(struct mem_cgroup *memcg, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 unsigned int nr_pages) >> +{ >> + =A0 =A0 if (!mem_cgroup_is_root(memcg)) { >> + =A0 =A0 =A0 =A0 =A0 =A0 unsigned long bytes =3D nr_pages * PAGE_SIZE; >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 res_counter_uncharge_until(&memcg->res, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 memcg->res.parent, bytes); >> + =A0 =A0 =A0 =A0 =A0 =A0 if (do_swap_account) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 res_counter_uncharge_until(&me= mcg->memsw, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 memcg->memsw.parent, bytes); >> + =A0 =A0 } >> +} > > Kame, this is a nitpick, but I usually prefer to write this like: > > if (mem_cgroup_is_root(memcg)) > =A0 return; > > res_counter... > > Specially with memcg, where function names are bigger than average, in > comparison. > > the code itself seems fine. > >> +/* >> =A0 =A0* A helper function to get mem_cgroup from ID. must be called und= er >> =A0 =A0* rcu_read_lock(). The caller must check css_is_removed() or some= if >> =A0 =A0* it's concern. (dropping refcnt from swap can be called against = removed >> @@ -2677,16 +2695,28 @@ static int mem_cgroup_move_parent(struct page *p= age, >> =A0 =A0 =A0 nr_pages =3D hpage_nr_pages(page); >> >> =A0 =A0 =A0 parent =3D mem_cgroup_from_cont(pcg); >> - =A0 =A0 ret =3D __mem_cgroup_try_charge(NULL, gfp_mask, nr_pages,&pare= nt, false); >> - =A0 =A0 if (ret) >> - =A0 =A0 =A0 =A0 =A0 =A0 goto put_back; >> + =A0 =A0 if (!parent->use_hierarchy) { > Can we avoid testing for use hierarchy ? > Specially given this might go away. > > parent_mem_cgroup() already bundles this information. So maybe we can > test for parent_mem_cgroup(parent) =3D=3D NULL. It is the same thing afte= r all. >> + =A0 =A0 =A0 =A0 =A0 =A0 ret =3D __mem_cgroup_try_charge(NULL, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 gfp_mask, nr_pages,&parent, false); >> + =A0 =A0 =A0 =A0 =A0 =A0 if (ret) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto put_back; >> + =A0 =A0 } > > Why? If we are not hierarchical, we should not charge the parent, right? This is how it is implemented today and I think he changed that to move to root on the next patch. > >> =A0 =A0 =A0 if (nr_pages> =A01) >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 flags =3D compound_lock_irqsave(page); >> >> - =A0 =A0 ret =3D mem_cgroup_move_account(page, nr_pages, pc, child, par= ent, true); >> - =A0 =A0 if (ret) >> - =A0 =A0 =A0 =A0 =A0 =A0 __mem_cgroup_cancel_charge(parent, nr_pages); >> + =A0 =A0 if (parent->use_hierarchy) { >> + =A0 =A0 =A0 =A0 =A0 =A0 ret =3D mem_cgroup_move_account(page, nr_pages= , >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 pc, child, parent, false); >> + =A0 =A0 =A0 =A0 =A0 =A0 if (!ret) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 __mem_cgroup_cancel_local_char= ge(child, nr_pages); >> + =A0 =A0 } else { >> + =A0 =A0 =A0 =A0 =A0 =A0 ret =3D mem_cgroup_move_account(page, nr_pages= , >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 pc, child, parent, true); >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 if (ret) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 __mem_cgroup_cancel_charge(par= ent, nr_pages); >> + =A0 =A0 } > > Calling move account also seems not necessary to me. If we are not > uncharging + charging, we won't even touch the parent. Today for user_hierarchy =3D 0, the charge is moved to parent as well as the stats. But that is changed on the following patches. --Ying -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: email@kvack.org