linux-kernel - Re: [RFC][PATCH 4/7 v2] memcg: use res_counter_uncharge_until in move

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Date:	Sat, 28 Apr 2012 08:58:08 +0900
From:	Hiroyuki Kamezawa <kamezawa.hiroyuki@...il.com>
To:	Glauber Costa <glommer@...allels.com>
Cc:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"cgroups@...r.kernel.org" <cgroups@...r.kernel.org>,
	Michal Hocko <mhocko@...e.cz>,
	Johannes Weiner <hannes@...xchg.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Tejun Heo <tj@...nel.org>, Han Ying <yinghan@...gle.com>,
	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [RFC][PATCH 4/7 v2] memcg: use res_counter_uncharge_until in move_parent

On Sat, Apr 28, 2012 at 2:16 AM, Glauber Costa <glommer@...allels.com> 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@...fujitsu.com>
>> ---
>>   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.
>
Ok, I'll use that style in the next post.

>> +/*
>>    * 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.

We need to find parent even if use_hierarchy==0 in this patch.
I'll consider to use it in later patch, thank you for pointing out.


>> +             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?
Current implementation moves charges to parent regardless of use_hierarchy.
It's handled in  a following 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.

we need to overwrite pc->mem_cgroup and touch other statistics.

Thanks,
-Kame
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/