Re: [patch 2/8] mm: memcg-aware global reclaim

[Ying Han <yinghan@google.com>]

On Mon, Aug 29, 2011 at 12:15 AM, Ying Han <yinghan@google.com> wrote:
> On Thu, Aug 11, 2011 at 2:09 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
>>
>> On Thu, Aug 11, 2011 at 01:39:45PM -0700, Ying Han wrote:
>> > Please consider including the following patch for the next post. It causes
>> > crash on some of the tests where sc->mem_cgroup is NULL (global kswapd).
>> >
>> > diff --git a/mm/vmscan.c b/mm/vmscan.c
>> > index b72a844..12ab25d 100644
>> > --- a/mm/vmscan.c
>> > +++ b/mm/vmscan.c
>> > @@ -2768,7 +2768,8 @@ loop_again:
>> >                          * Do some background aging of the anon list, to
>> > give
>> >                          * pages a chance to be referenced before
>> > reclaiming.
>> >                          */
>> > -                       if (inactive_anon_is_low(zone, &sc))
>> > +                       if (scanning_global_lru(&sc) &&
>> > +                                       inactive_anon_is_low(zone, &sc))
>> >                                 shrink_active_list(SWAP_CLUSTER_MAX, zone,
>> >                                                         &sc, priority, 0);
>>
>> Thanks!  I completely overlooked this one and only noticed it after
>> changing the arguments to shrink_active_list().
>>
>> On memcg configurations, scanning_global_lru() will essentially never
>> be true again, so I moved the anon pre-aging to a separate function
>> that also does a hierarchy loop to preage the per-memcg anon lists.
>>
>> I hope to send out the next revision soon.
>
> Also, please consider to fold in the following patch as well. It fixes
> the root cgroup lru accounting and we could easily trigger OOM while
> doing some swapoff test w/o it.
>
> mm:fix the lru accounting for root cgroup.
>
> This patch is applied on top of:
> "
> mm: memcg-aware global reclaim
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> "
>
> This patch fixes the lru accounting for root cgroup.
>
> After the "memcg-aware global reclaim" patch, one of the changes is to have
> lru pages linked back to root. Under the global memory pressure, we start from
> the root cgroup lru and walk through the memcg hierarchy of the system. For
> each memcg, we reclaim pages based on the its lru size.
>
> However for root cgroup, we used not having a seperate lru and only counting
> the pages charged to root as part of root lru size. Without this patch, all
> the pages which are linked to root lru but not charged to root like swapcache
> readahead are not visible to page reclaim code and we are easily to get OOM.
>
> After this patch, all the pages linked under root lru are counted in the lru
> size, including Used and !Used.
>
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Signed-off-by: Ying Han <yinghan@google.com>
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 5518f54..f6c5f29 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -888,19 +888,21 @@ void mem_cgroup_del_lru_list(struct page *page,
> enum lru_list lru)
>  {
>  >------struct page_cgroup *pc;
>  >------struct mem_cgroup_per_zone *mz;
> +>------struct mem_cgroup *mem;
> ·
>  >------if (mem_cgroup_disabled())
>  >------>-------return;
>  >------pc = lookup_page_cgroup(page);
> ->------/* can happen while we handle swapcache. */
> ->------if (!TestClearPageCgroupAcctLRU(pc))
> ->------>-------return;
> ->------VM_BUG_ON(!pc->mem_cgroup);
> ->------/*
> ->------ * We don't check PCG_USED bit. It's cleared when the "page" is finally
> ->------ * removed from global LRU.
> ->------ */
> ->------mz = page_cgroup_zoneinfo(pc->mem_cgroup, page);
> +
> +>------if (TestClearPageCgroupAcctLRU(pc) || PageCgroupUsed(pc)) {
> +>------>-------VM_BUG_ON(!pc->mem_cgroup);
> +>------>-------mem = pc->mem_cgroup;
> +>------} else {
> +>------>-------/* can happen while we handle swapcache. */
> +>------>-------mem = root_mem_cgroup;
> +>------}
> +
> +>------mz = page_cgroup_zoneinfo(mem, page);
>  >------MEM_CGROUP_ZSTAT(mz, lru) -= 1;
>  >------VM_BUG_ON(list_empty(&pc->lru));
>  >------list_del_init(&pc->lru);
> @@ -961,22 +963,31 @@ void mem_cgroup_add_lru_list(struct page *page,
> enum lru_list lru)
>  {
>  >------struct page_cgroup *pc;
>  >------struct mem_cgroup_per_zone *mz;
> +>------struct mem_cgroup *mem;
> ·
>  >------if (mem_cgroup_disabled())
>  >------>-------return;
>  >------pc = lookup_page_cgroup(page);
>  >------VM_BUG_ON(PageCgroupAcctLRU(pc));
> ->------/*
> ->------ * Used bit is set without atomic ops but after smp_wmb().
> ->------ * For making pc->mem_cgroup visible, insert smp_rmb() here.
> ->------ */
> ->------smp_rmb();
> ->------if (!PageCgroupUsed(pc))
> ->------>-------return;
> ·
> ->------mz = page_cgroup_zoneinfo(pc->mem_cgroup, page);
> +>------if (PageCgroupUsed(pc)) {
> +>------>-------/* Ensure pc->mem_cgroup is visible after reading PCG_USED. */
> +>------>-------smp_rmb();
> +>------>-------mem = pc->mem_cgroup;
> +>------>-------SetPageCgroupAcctLRU(pc);
> +>------} else {
> +>------>-------/*
> +>------>------- * If the page is no longer charged, add it to the
> +>------>------- * root memcg's lru.  Either it will be freed soon, or
> +>------>------- * it will get charged again and the charger will
> +>------>------- * relink it to the right list.
> +>------>-------mem = root_mem_cgroup;
> +>------}
> +
> +>------mz = page_cgroup_zoneinfo(mem, page);
>  >------MEM_CGROUP_ZSTAT(mz, lru) += 1;
> ->------SetPageCgroupAcctLRU(pc);
> +
>  >------list_add(&pc->lru, &mz->lists[lru]);
>  }
>
> --Ying
>

And this patch fixes the hierarchy_walk :

fix hierarchy_walk() to hold a reference to first mem_cgroup

The first mem_cgroup returned from hierarchy_walk() is used to
terminate a round-trip. However there is no reference hold on
that which the first could be removed during the walking. The
patch including the following change:

1. hold a reference on the first mem_cgroup during the walk.
2. rename the variable "root" to "target", which we found using
"root" is confusing in this content with root_mem_cgroup. better
naming is welcomed.

Signed-off-by: Ying Han <yinghan@google.com>

 include/linux/memcontrol.h |   10 +++++++---
 mm/memcontrol.c            |   41 ++++++++++++++++++++++++-----------------
 mm/vmscan.c                |   10 +++++-----
 3 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 15b713b..4de12ca 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -104,8 +104,10 @@ extern void mem_cgroup_end_migration(struct
mem_cgroup *mem,
 >------struct page *oldpage, struct page *newpage);
·
 struct mem_cgroup *mem_cgroup_hierarchy_walk(struct mem_cgroup *,
+>------>------->------->------->-------     struct mem_cgroup *,
 >------>------->------->------->-------     struct mem_cgroup *);
-void mem_cgroup_stop_hierarchy_walk(struct mem_cgroup *, struct mem_cgroup *);
+void mem_cgroup_stop_hierarchy_walk(struct mem_cgroup *, struct mem_cgroup *,
+>------>------->------->-------    struct mem_cgroup *);
·
 /*
  * For memory reclaim.
@@ -332,13 +334,15 @@ mem_cgroup_get_reclaim_stat_from_page(struct page *page)
 >------return NULL;
 }
·
-static inline struct mem_cgroup *mem_cgroup_hierarchy_walk(struct
mem_cgroup *r,
+static inline struct mem_cgroup *mem_cgroup_hierarchy_walk(struct
mem_cgroup *t,
+>------>------->------->------->------->------->-------   struct mem_cgroup *f,
 >------>------->------->------->------->------->-------   struct mem_cgroup *m)
 {
 >------return NULL;
 }
·
-static inline void mem_cgroup_stop_hierarchy_walk(struct mem_cgroup *r,
+static inline void mem_cgroup_stop_hierarchy_walk(struct mem_cgroup *t,
+>------>------->------->------->------->-------  struct mem_cgroup *f,
 >------>------->------->------->------->-------  struct mem_cgroup *m)
 {
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f6c5f29..80b62aa 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1447,60 +1447,67 @@ u64 mem_cgroup_get_limit(struct mem_cgroup *memcg)
·
 /**
  * mem_cgroup_hierarchy_walk - iterate over a memcg hierarchy
- * @root: starting point of the hierarchy
+ * @target: starting point of the hierarchy
+ * @first: first node of the scanning
  * @prev: previous position or NULL
  *
- * Caller must hold a reference to @root.  While this function will
- * return @root as part of the walk, it will never increase its
+ * Caller must hold a reference to @parent.  While this function will
+ * return @parent as part of the walk, it will never increase its
  * reference count.
  *
  * Caller must clean up with mem_cgroup_stop_hierarchy_walk() when it
  * stops the walk potentially before the full round trip.
  */
-struct mem_cgroup *mem_cgroup_hierarchy_walk(struct mem_cgroup *root,
+struct mem_cgroup *mem_cgroup_hierarchy_walk(struct mem_cgroup *target,
+>------>------->------->------->-------     struct mem_cgroup *first,
 >------>------->------->------->-------     struct mem_cgroup *prev)
 {
->------struct mem_cgroup *mem;
+>------struct mem_cgroup *mem = NULL;
·
 >------if (mem_cgroup_disabled())
 >------>-------return NULL;
·
->------if (!root)
->------>-------root = root_mem_cgroup;
+>------if (!target)
+>------>-------target = root_mem_cgroup;
 >------/*
 >------ * Even without hierarchy explicitely enabled in the root
 >------ * memcg, it is the ultimate parent of all memcgs.
 >------ */
->------if (!(root == root_mem_cgroup || root->use_hierarchy))
->------>-------return root;
->------if (prev && prev != root)
+>------if (!(target == root_mem_cgroup || target->use_hierarchy))
+>------>-------return target;
+>------if (prev && prev != target && prev != first)
 >------>-------css_put(&prev->css);
 >------do {
->------>-------int id = root->last_scanned_child;
+>------>-------int id = target->last_scanned_child;
 >------>-------struct cgroup_subsys_state *css;
·
 >------>-------rcu_read_lock();
->------>-------css = css_get_next(&mem_cgroup_subsys, id + 1, &root->css, &id);
->------>-------if (css && (css == &root->css || css_tryget(css)))
+>------>-------css = css_get_next(&mem_cgroup_subsys, id + 1,
&target->css, &id);
+>------>-------if (css && (css == &target->css || css_tryget(css)))
 >------>------->-------mem = container_of(css, struct mem_cgroup, css);
 >------>-------rcu_read_unlock();
 >------>-------if (!css)
 >------>------->-------id = 0;
->------>-------root->last_scanned_child = id;
+>------>-------target->last_scanned_child = id;
 >------} while (!mem);
 >------return mem;
 }
·
 /**
  * mem_cgroup_stop_hierarchy_walk - clean up after partial hierarchy walk
- * @root: starting point in the hierarchy
+ * @target: starting point in the hierarchy
+ * @first: first node of the scanning
  * @mem: last position during the walk
  */
-void mem_cgroup_stop_hierarchy_walk(struct mem_cgroup *root,
+void mem_cgroup_stop_hierarchy_walk(struct mem_cgroup *target,
+>------>------->------->-------    struct mem_cgroup *first,
 >------>------->------->-------    struct mem_cgroup *mem)
 {
->------if (mem && mem != root)
+>------if (mem && mem != target)
 >------>-------css_put(&mem->css);
+
+>------if (first && first != mem && first != target)
+>------>-------css_put(&first->css);
 }
·
 static unsigned long mem_cgroup_reclaim(struct mem_cgroup *mem,
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 3bcb212..aee958a 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1751,10 +1751,10 @@ static void shrink_zone(int priority, struct zone *zone,
 >------>------->-------struct scan_control *sc)
 {
 >------unsigned long nr_reclaimed_before = sc->nr_reclaimed;
->------struct mem_cgroup *root = sc->target_mem_cgroup;
->------struct mem_cgroup *first, *mem = NULL;
+>------struct mem_cgroup *target = sc->target_mem_cgroup;
+>------struct mem_cgroup *first, *mem;
·
->------first = mem = mem_cgroup_hierarchy_walk(root, mem);
+>------first = mem = mem_cgroup_hierarchy_walk(target, NULL, NULL);
 >------for (;;) {
 >------>-------unsigned long nr_reclaimed;
·
@@ -1765,11 +1765,11 @@ static void shrink_zone(int priority, struct zone *zone,
 >------>-------if (nr_reclaimed >= sc->nr_to_reclaim)
 >------>------->-------break;
·
->------>-------mem = mem_cgroup_hierarchy_walk(root, mem);
+>------>-------mem = mem_cgroup_hierarchy_walk(target, first, mem);
 >------>-------if (mem == first)
 >------>------->-------break;
 >------}
->------mem_cgroup_stop_hierarchy_walk(root, mem);
+>------mem_cgroup_stop_hierarchy_walk(target, first, mem);
 }

--Ying

--
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>