Re: [PATCH V3 6/7] Enable per-memcg background reclaim.

[Ying Han <yinghan@google.com>]

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

On Wed, Apr 13, 2011 at 2:05 AM, KAMEZAWA Hiroyuki <
kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Wed, 13 Apr 2011 00:03:06 -0700
> Ying Han <yinghan@google.com> wrote:
>
> > By default the per-memcg background reclaim is disabled when the
> limit_in_bytes
> > is set the maximum or the wmark_ratio is 0. The kswapd_run() is called
> when the
> > memcg is being resized, and kswapd_stop() is called when the memcg is
> being
> > deleted.
> >
> > The per-memcg kswapd is waked up based on the usage and low_wmark, which
> is
> > checked once per 1024 increments per cpu. The memcg's kswapd is waked up
> if the
> > usage is larger than the low_wmark.
> >
> > changelog v3..v2:
> > 1. some clean-ups
> >
> > changelog v2..v1:
> > 1. start/stop the per-cgroup kswapd at create/delete cgroup stage.
> > 2. remove checking the wmark from per-page charging. now it checks the
> wmark
> > periodically based on the event counter.
> >
> > Signed-off-by: Ying Han <yinghan@google.com>
>
> This event logic seems to make sense.
>
> > ---
> >  mm/memcontrol.c |   37 +++++++++++++++++++++++++++++++++++++
> >  1 files changed, 37 insertions(+), 0 deletions(-)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index efeade3..bfa8646 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -105,10 +105,12 @@ enum mem_cgroup_events_index {
> >  enum mem_cgroup_events_target {
> >       MEM_CGROUP_TARGET_THRESH,
> >       MEM_CGROUP_TARGET_SOFTLIMIT,
> > +     MEM_CGROUP_WMARK_EVENTS_THRESH,
> >       MEM_CGROUP_NTARGETS,
> >  };
> >  #define THRESHOLDS_EVENTS_TARGET (128)
> >  #define SOFTLIMIT_EVENTS_TARGET (1024)
> > +#define WMARK_EVENTS_TARGET (1024)
> >
> >  struct mem_cgroup_stat_cpu {
> >       long count[MEM_CGROUP_STAT_NSTATS];
> > @@ -366,6 +368,7 @@ static void mem_cgroup_put(struct mem_cgroup *mem);
> >  static struct mem_cgroup *parent_mem_cgroup(struct mem_cgroup *mem);
> >  static void drain_all_stock_async(void);
> >  static unsigned long get_wmark_ratio(struct mem_cgroup *mem);
> > +static void wake_memcg_kswapd(struct mem_cgroup *mem);
> >
> >  static struct mem_cgroup_per_zone *
> >  mem_cgroup_zoneinfo(struct mem_cgroup *mem, int nid, int zid)
> > @@ -545,6 +548,12 @@ mem_cgroup_largest_soft_limit_node(struct
> mem_cgroup_tree_per_zone *mctz)
> >       return mz;
> >  }
> >
> > +static void mem_cgroup_check_wmark(struct mem_cgroup *mem)
> > +{
> > +     if (!mem_cgroup_watermark_ok(mem, CHARGE_WMARK_LOW))
> > +             wake_memcg_kswapd(mem);
> > +}
> > +
> >  /*
> >   * Implementation Note: reading percpu statistics for memcg.
> >   *
> > @@ -675,6 +684,9 @@ static void __mem_cgroup_target_update(struct
> mem_cgroup *mem, int target)
> >       case MEM_CGROUP_TARGET_SOFTLIMIT:
> >               next = val + SOFTLIMIT_EVENTS_TARGET;
> >               break;
> > +     case MEM_CGROUP_WMARK_EVENTS_THRESH:
> > +             next = val + WMARK_EVENTS_TARGET;
> > +             break;
> >       default:
> >               return;
> >       }
> > @@ -698,6 +710,10 @@ static void memcg_check_events(struct mem_cgroup
> *mem, struct page *page)
> >                       __mem_cgroup_target_update(mem,
> >                               MEM_CGROUP_TARGET_SOFTLIMIT);
> >               }
> > +             if (unlikely(__memcg_event_check(mem,
> > +                     MEM_CGROUP_WMARK_EVENTS_THRESH))){
> > +                     mem_cgroup_check_wmark(mem);
> > +             }
> >       }
> >  }
> >
> > @@ -3384,6 +3400,10 @@ static int mem_cgroup_resize_limit(struct
> mem_cgroup *memcg,
> >       if (!ret && enlarge)
> >               memcg_oom_recover(memcg);
> >
> > +     if (!mem_cgroup_is_root(memcg) && !memcg->kswapd_wait &&
> > +                     memcg->wmark_ratio)
> > +             kswapd_run(0, memcg);
> > +
>
> Isn't it enough to have trigger in charge() path ?
>

why? kswapd_run() is to create the kswapd thread for the memcg. If the
memcg's limit doesn't change from the initial value, we don't want to create
a kswapd thread for it. Only if the limit_in_byte is being changed. Adding
the hook in the charge path sounds too much overhead to the hotpath.

However, I might need to add checks here, where if the limit_in_byte is set
to RESOURCE_MAX.

>
> rather than here, I think we should check _move_task(). It changes res
> usage
> dramatically without updating events.
>

I see both the mem_cgroup_charge_statistics() and memcg_check_events()  are
being called in mem_cgroup_move_account(). Am i missing anything here?


Thanks
--Ying



>
> Thanks,
> -Kame
>
>
> >       return ret;
> >  }
> >
> > @@ -4680,6 +4700,7 @@ static void __mem_cgroup_free(struct mem_cgroup
> *mem)
> >  {
> >       int node;
> >
> > +     kswapd_stop(0, mem);
> >       mem_cgroup_remove_from_trees(mem);
> >       free_css_id(&mem_cgroup_subsys, &mem->css);
> >
>
> I think kswapd should stop at mem_cgroup_destroy(). No more tasks will use
> this memcg after _destroy().
>

I made the change.

>
> Thanks,
> -Kame
>
>
>
> > @@ -4786,6 +4807,22 @@ int mem_cgroup_last_scanned_node(struct mem_cgroup
> *mem)
> >       return mem->last_scanned_node;
> >  }
> >
> > +static inline
> > +void wake_memcg_kswapd(struct mem_cgroup *mem)
> > +{
> > +     wait_queue_head_t *wait;
> > +
> > +     if (!mem || !mem->wmark_ratio)
> > +             return;
> > +
> > +     wait = mem->kswapd_wait;
> > +
> > +     if (!wait || !waitqueue_active(wait))
> > +             return;
> > +
> > +     wake_up_interruptible(wait);
> > +}
> > +
> >  static int mem_cgroup_soft_limit_tree_init(void)
> >  {
> >       struct mem_cgroup_tree_per_node *rtpn;
> > --
> > 1.7.3.1
> >
> > --
> > 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/ .
> > Fight unfair telecom internet charges in Canada: sign
> http://stopthemeter.ca/
> > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> >
>
>

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