On Fri, Apr 27, 2012 at 10:16 AM, Glauber Costa <glommer@xxxxxxxxxxxxx> wrote: > 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<kamezawa.hiroyu@xxxxxxxxxxxxxx> >> --- >> mm/memcontrol.c | 63 ++++++++++++++++++++++++++++++++++++------------------ >> 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, >> } >> >> /* >> + * 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, >> + unsigned int nr_pages) >> +{ >> + if (!mem_cgroup_is_root(memcg)) { >> + unsigned long bytes = nr_pages * PAGE_SIZE; >> + >> + res_counter_uncharge_until(&memcg->res, >> + memcg->res.parent, bytes); >> + if (do_swap_account) >> + res_counter_uncharge_until(&memcg->memsw, >> + memcg->memsw.parent, bytes); >> + } >> +} > > Kame, this is a nitpick, but I usually prefer to write this like: > > if (mem_cgroup_is_root(memcg)) > return; > > res_counter... > > Specially with memcg, where function names are bigger than average, in > comparison. > > the code itself seems fine. > >> +/* >> * A helper function to get mem_cgroup from ID. must be called under >> * rcu_read_lock(). The caller must check css_is_removed() or some if >> * it's concern. (dropping refcnt from swap can be called against removed >> @@ -2677,16 +2695,28 @@ static int mem_cgroup_move_parent(struct page *page, >> nr_pages = hpage_nr_pages(page); >> >> parent = mem_cgroup_from_cont(pcg); >> - ret = __mem_cgroup_try_charge(NULL, gfp_mask, nr_pages,&parent, false); >> - if (ret) >> - goto put_back; >> + 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) == NULL. It is the same thing after all. >> + ret = __mem_cgroup_try_charge(NULL, >> + gfp_mask, nr_pages,&parent, false); >> + if (ret) >> + goto put_back; >> + } > > 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. > >> if (nr_pages> 1) >> flags = compound_lock_irqsave(page); >> >> - ret = mem_cgroup_move_account(page, nr_pages, pc, child, parent, true); >> - if (ret) >> - __mem_cgroup_cancel_charge(parent, nr_pages); >> + if (parent->use_hierarchy) { >> + ret = mem_cgroup_move_account(page, nr_pages, >> + pc, child, parent, false); >> + if (!ret) >> + __mem_cgroup_cancel_local_charge(child, nr_pages); >> + } else { >> + ret = mem_cgroup_move_account(page, nr_pages, >> + pc, child, parent, true); >> + >> + if (ret) >> + __mem_cgroup_cancel_charge(parent, nr_pages); >> + } > > 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 = 0, the charge is moved to parent as well as the stats. But that is changed on the following patches. --Ying -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html