[Intel-gfx] [PATCH] memcg, shmem: fix shmem migration to use lrucare. (was: Re: memcontrol.c BUG)

[Michal Hocko]

On Thu 29-01-15 18:04:15, Hugh Dickins wrote:
> On Wed, 28 Jan 2015, Michal Hocko wrote:
> > On Wed 28-01-15 08:48:52, Chris Wilson wrote:
> > > On Wed, Jan 28, 2015 at 08:13:06AM +1000, Dave Airlie wrote:
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1165369
> > > > 
> > > > ov 18 09:23:22 elissa.gathman.org kernel: page:f5e36a40 count:2
> > > > mapcount:0 mapping:  (null) index:0x0
> > > > Nov 18 09:23:22 elissa.gathman.org kernel: page flags:
> > > > 0x80090029(locked|uptodate|lru|swapcache|swapbacked)
> > > > Nov 18 09:23:22 elissa.gathman.org kernel: page dumped because:
> > > > VM_BUG_ON_PAGE(!lrucare && PageLRU(oldpage))
> > > > Nov 18 09:23:23 elissa.gathman.org kernel: ------------[ cut here ]------------
> > > > Nov 18 09:23:23 elissa.gathman.org kernel: kernel BUG at mm/memcontrol.c:6733!
> > 
> > I guess this matches the following bugon in your kernel:
> >         VM_BUG_ON_PAGE(!lrucare && PageLRU(oldpage), oldpage);
> > 
> > so the oldpage is on the LRU list already. I am completely unfamiliar
> > with 965GM but is the page perhaps shared with somebody with a different
> > gfp mask requirement (e.g. userspace accessing the memory via mmap)? So
> > the other (racing) caller didn't need to move the page and put it on
> > LRU.
> 
> It would be surprising (but not impossible) for oldpage not to be on
> the LRU already: it's a swapin readahead page that has every right to
> be on LRU,

True, thanks for pointing this out.

> but turns out to have been allocated from an unsuitable zone,
> once we discover that it's needed in one of these odd hardware-limited
> mappings.  (Whereas newpage is newly allocated and not yet on LRU.)
> 
> > 
> > If yes we need to tell shmem_replace_page to do the lrucare handling.
> 
> Absolutely, thanks Michal.  It would also be good to change the comment
> on mem_cgroup_migrate() in mm/memcontrol.c, from "@lrucare: both pages..."
> to "@lrucare: either or both pages..." - though I certainly won't pretend
> that the corrected wording would have prevented this bug creeping in!

Yes, I have updated the wording.
 
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 339e06639956..e3cdc1a16c0f 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1013,7 +1013,7 @@ static int shmem_replace_page(struct page **pagep, gfp_t gfp,
> >  		 */
> >  		oldpage = newpage;
> >  	} else {
> > -		mem_cgroup_migrate(oldpage, newpage, false);
> > +		mem_cgroup_migrate(oldpage, newpage, true);
> >  		lru_cache_add_anon(newpage);
> >  		*pagep = newpage;
> >  	}
> 
> Acked-by: Hugh Dickins <hughd at google.com>

Thanks! The full patch is below. I wasn't sure who was the one to report
the issue so I hope the credits are right. I have marked the patch for
stable because some people are running with VM debugging enabled. AFAICS
the issue is not so harmful without debugging on because the stale
oldpage would be removed from the LRU list eventually.
---
>From 508815bfdaae75e3286ab2dd714a07201665709c Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko at suse.cz>
Date: Mon, 2 Feb 2015 15:22:19 +0100
Subject: [PATCH] memcg, shmem: fix shmem migration to use lrucare.

It has been reported that 965GM might trigger

VM_BUG_ON_PAGE(!lrucare && PageLRU(oldpage), oldpage)

in mem_cgroup_migrate when shmem wants to replace a swap cache page
because of shmem_should_replace_page (the page is allocated from an
inappropriate zone). shmem_replace_page expects that the oldpage is not
on LRU list and calls mem_cgroup_migrate without lrucare. This is obviously
incorrect because swapcache pages might be on the LRU list (e.g. swapin
readahead page).

Fix this by enabling lrucare for the migration in shmem_replace_page.
Also clarify that lrucare should be used even if one of the pages might
be on LRU list.

The BUG_ON will trigger only when CONFIG_DEBUG_VM is enabled but even
without that the migration code might leave the old page on an
inappropriate memcg' LRU which is not that critical because the page
would get removed with its last reference but it is still confusing.

Fixes: 0a31bc97c80c (mm: memcontrol: rewrite uncharge API)
Cc: stable at vger.kernel.org # 3.17+
Reported-by: Chris Wilson <chris at chris-wilson.co.uk>
Reported-by: Dave Airlie <airlied at gmail.com>
Acked-by: Hugh Dickins <hughd at google.com>
Signed-off-by: Michal Hocko <mhocko at suse.cz>
---
 mm/memcontrol.c | 2 +-
 mm/shmem.c      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 7ce5aa24bc19..c5ac0e209868 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5743,7 +5743,7 @@ void mem_cgroup_uncharge_list(struct list_head *page_list)
  * mem_cgroup_migrate - migrate a charge to another page
  * @oldpage: currently charged page
  * @newpage: page to transfer the charge to
- * @lrucare: both pages might be on the LRU already
+ * @lrucare: either or both pages might be on the LRU already
  *
  * Migrate the charge from @oldpage to @newpage.
  *
diff --git a/mm/shmem.c b/mm/shmem.c
index 339e06639956..e3cdc1a16c0f 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1013,7 +1013,7 @@ static int shmem_replace_page(struct page **pagep, gfp_t gfp,
 		 */
 		oldpage = newpage;
 	} else {
-		mem_cgroup_migrate(oldpage, newpage, false);
+		mem_cgroup_migrate(oldpage, newpage, true);
 		lru_cache_add_anon(newpage);
 		*pagep = newpage;
 	}
-- 
2.1.4

-- 
Michal Hocko
SUSE Labs