Re: [PATCH V4 03/10] New APIs to adjust per-memcg wmarks

[Ying Han <yinghan@google.com>]

[-- Attachment #1: Type: text/plain, Size: 4413 bytes --]

On Thu, Apr 14, 2011 at 5:25 PM, KAMEZAWA Hiroyuki <
kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Thu, 14 Apr 2011 15:54:22 -0700
> Ying Han <yinghan@google.com> wrote:
>
> > Add memory.low_wmark_distance, memory.high_wmark_distance and
> reclaim_wmarks
> > APIs per-memcg. The first two adjust the internal low/high wmark
> calculation
> > and the reclaim_wmarks exports the current value of watermarks.
> >
> > By default, the low/high_wmark is calculated by subtracting the distance
> from
> > the hard_limit(limit_in_bytes).
> >
> > $ echo 500m >/dev/cgroup/A/memory.limit_in_bytes
> > $ cat /dev/cgroup/A/memory.limit_in_bytes
> > 524288000
> >
> > $ echo 50m >/dev/cgroup/A/memory.high_wmark_distance
> > $ echo 40m >/dev/cgroup/A/memory.low_wmark_distance
> >
> > $ cat /dev/cgroup/A/memory.reclaim_wmarks
> > low_wmark 482344960
> > high_wmark 471859200
> >
> > changelog v4..v3:
> > 1. replace the "wmark_ratio" API with individual tunable for
> low/high_wmarks.
> >
> > changelog v3..v2:
> > 1. replace the "min_free_kbytes" api with "wmark_ratio". This is part of
> > feedbacks
> >
> > Signed-off-by: Ying Han <yinghan@google.com>
>
> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>
> But please add a sanity check (see below.)
>
>
>
> > ---
> >  mm/memcontrol.c |   95
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 95 insertions(+), 0 deletions(-)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 1ec4014..685645c 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -3974,6 +3974,72 @@ static int mem_cgroup_swappiness_write(struct
> cgroup *cgrp, struct cftype *cft,
> >       return 0;
> >  }
> >
> > +static u64 mem_cgroup_high_wmark_distance_read(struct cgroup *cgrp,
> > +                                            struct cftype *cft)
> > +{
> > +     struct mem_cgroup *memcg = mem_cgroup_from_cont(cgrp);
> > +
> > +     return memcg->high_wmark_distance;
> > +}
> > +
> > +static u64 mem_cgroup_low_wmark_distance_read(struct cgroup *cgrp,
> > +                                           struct cftype *cft)
> > +{
> > +     struct mem_cgroup *memcg = mem_cgroup_from_cont(cgrp);
> > +
> > +     return memcg->low_wmark_distance;
> > +}
> > +
> > +static int mem_cgroup_high_wmark_distance_write(struct cgroup *cont,
> > +                                             struct cftype *cft,
> > +                                             const char *buffer)
> > +{
> > +     struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
> > +     u64 low_wmark_distance = memcg->low_wmark_distance;
> > +     unsigned long long val;
> > +     u64 limit;
> > +     int ret;
> > +
> > +     ret = res_counter_memparse_write_strategy(buffer, &val);
> > +     if (ret)
> > +             return -EINVAL;
> > +
> > +     limit = res_counter_read_u64(&memcg->res, RES_LIMIT);
> > +     if ((val >= limit) || (val < low_wmark_distance) ||
> > +        (low_wmark_distance && val == low_wmark_distance))
> > +             return -EINVAL;
> > +
> > +     memcg->high_wmark_distance = val;
> > +
> > +     setup_per_memcg_wmarks(memcg);
> > +     return 0;
> > +}
>
> IIUC, as limit_in_bytes, 'distance' should not be able to set against ROOT
> memcg
> because it doesn't work.
>
> thanks for review. will change in next post.
>
> > +
> > +static int mem_cgroup_low_wmark_distance_write(struct cgroup *cont,
> > +                                            struct cftype *cft,
> > +                                            const char *buffer)
> > +{
> > +     struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
> > +     u64 high_wmark_distance = memcg->high_wmark_distance;
> > +     unsigned long long val;
> > +     u64 limit;
> > +     int ret;
> > +
> > +     ret = res_counter_memparse_write_strategy(buffer, &val);
> > +     if (ret)
> > +             return -EINVAL;
> > +
> > +     limit = res_counter_read_u64(&memcg->res, RES_LIMIT);
> > +     if ((val >= limit) || (val > high_wmark_distance) ||
> > +         (high_wmark_distance && val == high_wmark_distance))
> > +             return -EINVAL;
> > +
> > +     memcg->low_wmark_distance = val;
> > +
> > +     setup_per_memcg_wmarks(memcg);
> > +     return 0;
> > +}
> > +
>
> Here, too.
>
> Will add


> I wonder we should have a method to hide unnecessary interfaces in ROOT
> cgroup...
>
> hmm. something to think about..

--Ying


> Thanks,
> -Kame
>
>

[-- Attachment #2: Type: text/html, Size: 5998 bytes --]