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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. 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: <a href