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?
> 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.
--
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=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>