Re: [mmotm][PATCH 2/2 v2] memcg: reduce calls for soft limit excess

[Balbir Singh <balbir@linux.vnet.ibm.com>]

On Wed, Sep 2, 2009 at 11:26 AM, KAMEZAWA
Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> In charge/uncharge/reclaim path, usage_in_excess is calculated repeatedly and
> it takes res_counter's spin_lock every time.
>

I think the changelog needs to mention some refactoring you've done
below as well, like change new_charge_in_excess to excess.



> This patch removes unnecessary calls for res_count_soft_limit_excess.
>
> Changelog:
>  - fixed description.
>  - fixed unsigned long to be unsigned long long (Thanks, Nishimura)
>
> Reviewed-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  mm/memcontrol.c |   31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
>
> Index: mmotm-2.6.31-Aug27/mm/memcontrol.c
> ===================================================================
> --- mmotm-2.6.31-Aug27.orig/mm/memcontrol.c
> +++ mmotm-2.6.31-Aug27/mm/memcontrol.c
> @@ -313,7 +313,8 @@ soft_limit_tree_from_page(struct page *p
>  static void
>  __mem_cgroup_insert_exceeded(struct mem_cgroup *mem,
>                                struct mem_cgroup_per_zone *mz,
> -                               struct mem_cgroup_tree_per_zone *mctz)
> +                               struct mem_cgroup_tree_per_zone *mctz,
> +                               unsigned long long new_usage_in_excess)
>  {
>        struct rb_node **p = &mctz->rb_root.rb_node;
>        struct rb_node *parent = NULL;
> @@ -322,7 +323,9 @@ __mem_cgroup_insert_exceeded(struct mem_
>        if (mz->on_tree)
>                return;
>
> -       mz->usage_in_excess = res_counter_soft_limit_excess(&mem->res);
> +       mz->usage_in_excess = new_usage_in_excess;
> +       if (!mz->usage_in_excess)
> +               return;
>        while (*p) {
>                parent = *p;
>                mz_node = rb_entry(parent, struct mem_cgroup_per_zone,
> @@ -382,7 +385,7 @@ static bool mem_cgroup_soft_limit_check(
>
>  static void mem_cgroup_update_tree(struct mem_cgroup *mem, struct page *page)
>  {
> -       unsigned long long new_usage_in_excess;
> +       unsigned long long excess;
>        struct mem_cgroup_per_zone *mz;
>        struct mem_cgroup_tree_per_zone *mctz;
>        int nid = page_to_nid(page);
> @@ -395,25 +398,21 @@ static void mem_cgroup_update_tree(struc
>         */
>        for (; mem; mem = parent_mem_cgroup(mem)) {
>                mz = mem_cgroup_zoneinfo(mem, nid, zid);
> -               new_usage_in_excess =
> -                       res_counter_soft_limit_excess(&mem->res);
> +               excess = res_counter_soft_limit_excess(&mem->res);
>                /*
>                 * We have to update the tree if mz is on RB-tree or
>                 * mem is over its softlimit.
>                 */
> -               if (new_usage_in_excess || mz->on_tree) {
> +               if (excess || mz->on_tree) {
>                        spin_lock(&mctz->lock);
>                        /* if on-tree, remove it */
>                        if (mz->on_tree)
>                                __mem_cgroup_remove_exceeded(mem, mz, mctz);
>                        /*
> -                        * if over soft limit, insert again. mz->usage_in_excess
> -                        * will be updated properly.
> +                        * Insert again. mz->usage_in_excess will be updated.
> +                        * If excess is 0, no tree ops.
>                         */
> -                       if (new_usage_in_excess)
> -                               __mem_cgroup_insert_exceeded(mem, mz, mctz);
> -                       else
> -                               mz->usage_in_excess = 0;
> +                       __mem_cgroup_insert_exceeded(mem, mz, mctz, excess);
>                        spin_unlock(&mctz->lock);
>                }
>        }
> @@ -2216,6 +2215,7 @@ unsigned long mem_cgroup_soft_limit_recl
>        unsigned long reclaimed;
>        int loop = 0;
>        struct mem_cgroup_tree_per_zone *mctz;
> +       unsigned long long excess;
>
>        if (order > 0)
>                return 0;
> @@ -2260,9 +2260,8 @@ unsigned long mem_cgroup_soft_limit_recl
>                                __mem_cgroup_largest_soft_limit_node(mctz);
>                        } while (next_mz == mz);
>                }
> -               mz->usage_in_excess =
> -                       res_counter_soft_limit_excess(&mz->mem->res);
>                __mem_cgroup_remove_exceeded(mz->mem, mz, mctz);
> +               excess = res_counter_soft_limit_excess(&mz->mem->res);
>                /*
>                 * One school of thought says that we should not add
>                 * back the node to the tree if reclaim returns 0.
> @@ -2271,8 +2270,8 @@ unsigned long mem_cgroup_soft_limit_recl
>                 * memory to reclaim from. Consider this as a longer
>                 * term TODO.
>                 */
> -               if (mz->usage_in_excess)
> -                       __mem_cgroup_insert_exceeded(mz->mem, mz, mctz);
> +               /* If excess == 0, no tree ops */
> +               __mem_cgroup_insert_exceeded(mz->mem, mz, mctz, excess);
>                spin_unlock(&mctz->lock);
>                css_put(&mz->mem->css);
>                loop++;

OK.. so everytime we call __mem_cgroup_insert_exceeded we save one
res_counter operation.

Looks good

Acked-by: Balbir Singh <balbir@linux.vnet.ibm.com>

Balbir Singh

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>