Re: [PATCH 1/3] mm: migrate: do not touch page->mem_cgroup of live pages

[Hugh Dickins]

On Thu, 4 Feb 2016, Johannes Weiner wrote:
> On Wed, Feb 03, 2016 at 05:39:08PM -0800, Hugh Dickins wrote:
> 
> > And (even more off-topic), I'm slightly sad to see that the lrucare
> > arg which mem_cgroup_migrate() used to have (before I renamed it and
> > you renamed it back!) has gone, so mem_cgroup_migrate() now always
> > demands lrucare of commit_charge().  I'd hoped that with your
> > separation of new from old charge, mem_cgroup_migrate() would never
> > need lrucare; but that's not true for the fuse case, though true
> > for everyone else.  Maybe just not worth bothering about?  Or the
> > reintroduction of some unnecessary zone->lru_lock-ing in page
> > migration, which we ought to try to avoid?
> > 
> > Or am I wrong, and even fuse doesn't need it?  That early return
> > "if (newpage->mem_cgroup)": isn't mem_cgroup_migrate() a no-op for
> > fuse, or is there some corner case by which newpage can be on LRU
> > but its mem_cgroup unset?
> 
> That should be impossible nowadays.
> 
> I went through the git log to find out why we used to do the LRU
> handling for newpage, and the clue is in this patch and the way
> charging used to work at that time:
> 
> commit 5a6475a4e162200f43855e2d42bbf55bcca1a9f2
> Author: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> Date:   Wed Mar 23 16:42:42 2011 -0700
> 
>     memcg: fix leak on wrong LRU with FUSE
>     
>     fs/fuse/dev.c::fuse_try_move_page() does
>     
>        (1) remove a page by ->steal()
>        (2) re-add the page to page cache
>        (3) link the page to LRU if it was not on LRU at (1)
>     
>     This implies the page is _on_ LRU when it's added to radix-tree.  So, the
>     page is added to memory cgroup while it's on LRU.  because LRU is lazy and
>     no one flushs it.
> 
> We used to uncharge the page when deleting it from the page cache, not
> on the final put. So when fuse replaced a page in cache, it would
> uncharge the stolen page while it was on the LRU and then re-charge.
> 
> Nowadays this doesn't happen, and if newpage is a stolen page cache
> page it just remains charged and we bail out of the transfer.
> 
> I don't see a sceniaro where newpage would be uncharged yet on LRU.
> 
> Thanks for your insights, Hugh. I'll send patches to clean this up.

And thank you for following up and identifying that commit, which
explained why it was needed, and now is not.  Not a big deal, but
it is satisfying to be able to eliminate that piece of lrucruft,
as your patch then did: thank you.

Hugh