Hi Michal,
[cc'ing Miklos for fuse's use of replace_page_cache()]
On Fri, Jul 18, 2014 at 09:12:46AM +0200, Michal Hocko wrote:
> On Tue 15-07-14 14:19:35, Michal Hocko wrote:
> > [...]
> > > +/**
> > > + * mem_cgroup_migrate - migrate a charge to another page
> > > + * @oldpage: currently charged page
> > > + * @newpage: page to transfer the charge to
> > > + * @lrucare: page might be on LRU already
> >
> > which one? I guess the newpage?
> >
> > > + *
> > > + * Migrate the charge from @oldpage to @newpage.
> > > + *
> > > + * Both pages must be locked, @newpage->mapping must be set up.
> > > + */
> > > +void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
> > > + bool lrucare)
> > > +{
> > > + unsigned int nr_pages = 1;
> > > + struct page_cgroup *pc;
> > > +
> > > + VM_BUG_ON_PAGE(!PageLocked(oldpage), oldpage);
> > > + VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
> > > + VM_BUG_ON_PAGE(PageLRU(oldpage), oldpage);
> > > + VM_BUG_ON_PAGE(PageLRU(newpage), newpage);
> >
> > VM_BUG_ON_PAGE(PageLRU(newpage) && !lruvec, newpage);
>
> I guess everything except these two notes got addressed.
Sorry, they fell through the cracks.
Yes, @newpage can already be on the LRU, and it's what @lrucare is
for. However, you got me thinking about the source page, and so I
went back to replace_page_cache(); and fuse code, which is the only
user of it.
I assumed the source page would always be new, according to this part
in fuse_try_move_page():
/*
* This is a new and locked page, it shouldn't be mapped or
* have any special flags on it
*/
if (WARN_ON(page_mapped(oldpage)))
goto out_fallback_unlock;
if (WARN_ON(page_has_private(oldpage)))
goto out_fallback_unlock;
if (WARN_ON(PageDirty(oldpage) || PageWriteback(oldpage)))
goto out_fallback_unlock;
if (WARN_ON(PageMlocked(oldpage)))
goto out_fallback_unlock;
However, it's in the page cache and I can't really convince myself
that it's not also on the LRU. Miklos, I have trouble pinpointing
where oldpage is instantiated exactly and what state it might be in -
can it already be on the LRU?
If it can, we need to make sure we don't change pc->mem_cgroup while
mem_cgroup_migrate() is looking at it:
---
>From c636935736bafa4d6800fe040a0c3cff7ce334ea Mon Sep 17 00:00:00 2001
From: Johannes Weiner <han...@cmpxchg.org>
Date: Fri, 18 Jul 2014 09:48:42 -0400
Subject: [patch] mm: memcontrol: rewrite uncharge API fix - page cache
migration
It was known that the target page in migration could be on the LRU -
clarify this in mem_cgroup_migrate() and correct the VM_BUG_ON_PAGE().
However, the source page can also be on the LRU in case of page cache
replacement and there is nothing stabilizing pc->mem_cgroup right now:
grab the page lock in mem_cgroup_move_account() to prevent page cache
replacement from racing with charge moving.
Reported-by: Michal Hocko <mho...@suse.cz>
Signed-off-by: Johannes Weiner <han...@cmpxchg.org>
---
mm/memcontrol.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 9db142d83b5c..c9cebf2cf273 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3450,9 +3450,17 @@ static int mem_cgroup_move_account(struct page *page,
if (nr_pages > 1 && !PageTransHuge(page))
goto out;
+ /*
+ * Prevent mem_cgroup_migrate() from looking at pc->mem_cgroup
+ * of its source page while we change it: page migration takes
+ * both pages off the LRU, but page cache replacement doesn't.
+ */
+ if (!trylock_page(page))
+ goto out;
+
ret = -EINVAL;
if (!PageCgroupUsed(pc) || pc->mem_cgroup != from)
- goto out;
+ goto out_unlock;
move_lock_mem_cgroup(from, &flags);
@@ -3487,6 +3495,8 @@ static int mem_cgroup_move_account(struct page *page,
mem_cgroup_charge_statistics(from, page, -nr_pages);
memcg_check_events(from, page);
local_irq_enable();
+out_unlock:
+ unlock_page(page);
out:
return ret;
}
@@ -6614,7 +6624,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: page might be on LRU already
+ * @lrucare: @newpage might be on LRU already
*
* Migrate the charge from @oldpage to @newpage.
*
@@ -6628,8 +6638,7 @@ void mem_cgroup_migrate(struct page *oldpage, struct page
*newpage,
VM_BUG_ON_PAGE(!PageLocked(oldpage), oldpage);
VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
- VM_BUG_ON_PAGE(PageLRU(oldpage), oldpage);
- VM_BUG_ON_PAGE(PageLRU(newpage), newpage);
+ VM_BUG_ON_PAGE(!lrucare && PageLRU(newpage), newpage);
VM_BUG_ON_PAGE(PageAnon(oldpage) != PageAnon(newpage), newpage);
if (mem_cgroup_disabled())
--
2.0.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
