0a31bc97c80c ("mm: memcontrol: rewrite uncharge API") changed page
migration to uncharge the old page right away. The page is locked,
unmapped, truncated, and off the LRU. But it could race with a
finishing writeback, which then doesn't get unaccounted properly:
test_clear_page_writeback() migration
acquire pc->mem_cgroup->move_lock
wait_on_page_writeback()
TestClearPageWriteback()
mem_cgroup_migrate()
clear PCG_USED
if (PageCgroupUsed(pc))
decrease memcg pages under writeback
release pc->mem_cgroup->move_lock
One solution for this would be to simply remove the PageCgroupUsed()
check, as RCU protects the memcg anyway.
However, it's more robust to acknowledge that migration is really
modifying the charge state of alive pages in this case, and so it
should participate in the protocol specifically designed for this.
Fixes: 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API")
Signed-off-by: Johannes Weiner <[email protected]>
Cc: "3.17" <[email protected]>
---
mm/memcontrol.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 3a203c7ec6c7..b35a44e9cd37 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -6148,6 +6148,7 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
bool lrucare)
{
struct page_cgroup *pc;
+ unsigned long flags;
int isolated;
VM_BUG_ON_PAGE(!PageLocked(oldpage), oldpage);
@@ -6177,7 +6178,14 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
if (lrucare)
lock_page_lru(oldpage, &isolated);
+ /*
+ * The page is locked, unmapped, truncated, and off the LRU,
+ * but there might still be references, e.g. from finishing
+ * writeback. Follow the charge moving protocol here.
+ */
+ move_lock_mem_cgroup(pc->mem_cgroup, &flags);
pc->flags = 0;
+ move_unlock_mem_cgroup(pc->mem_cgroup, &flags);
if (lrucare)
unlock_page_lru(oldpage, isolated);
--
2.1.2
On Tue 21-10-14 14:19:10, Johannes Weiner wrote:
> 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API") changed page
> migration to uncharge the old page right away. The page is locked,
> unmapped, truncated, and off the LRU. But it could race with a
> finishing writeback, which then doesn't get unaccounted properly:
>
> test_clear_page_writeback() migration
> acquire pc->mem_cgroup->move_lock
> wait_on_page_writeback()
> TestClearPageWriteback()
> mem_cgroup_migrate()
> clear PCG_USED
> if (PageCgroupUsed(pc))
> decrease memcg pages under writeback
> release pc->mem_cgroup->move_lock
>
> One solution for this would be to simply remove the PageCgroupUsed()
> check, as RCU protects the memcg anyway.
>
> However, it's more robust to acknowledge that migration is really
> modifying the charge state of alive pages in this case, and so it
> should participate in the protocol specifically designed for this.
It's been a long day so I might be missing something really obvious
here. But how can move_lock help here when the fast path (no task
migration is going on) takes only RCU read lock?
> Fixes: 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API")
> Signed-off-by: Johannes Weiner <[email protected]>
> Cc: "3.17" <[email protected]>
> ---
> mm/memcontrol.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 3a203c7ec6c7..b35a44e9cd37 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -6148,6 +6148,7 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
> bool lrucare)
> {
> struct page_cgroup *pc;
> + unsigned long flags;
> int isolated;
>
> VM_BUG_ON_PAGE(!PageLocked(oldpage), oldpage);
> @@ -6177,7 +6178,14 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage,
> if (lrucare)
> lock_page_lru(oldpage, &isolated);
>
> + /*
> + * The page is locked, unmapped, truncated, and off the LRU,
> + * but there might still be references, e.g. from finishing
> + * writeback. Follow the charge moving protocol here.
> + */
> + move_lock_mem_cgroup(pc->mem_cgroup, &flags);
> pc->flags = 0;
> + move_unlock_mem_cgroup(pc->mem_cgroup, &flags);
>
> if (lrucare)
> unlock_page_lru(oldpage, isolated);
> --
> 2.1.2
>
--
Michal Hocko
SUSE Labs
On Wed, Oct 22, 2014 at 06:30:51PM +0200, Michal Hocko wrote:
> On Tue 21-10-14 14:19:10, Johannes Weiner wrote:
> > 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API") changed page
> > migration to uncharge the old page right away. The page is locked,
> > unmapped, truncated, and off the LRU. But it could race with a
> > finishing writeback, which then doesn't get unaccounted properly:
> >
> > test_clear_page_writeback() migration
> > acquire pc->mem_cgroup->move_lock
> > wait_on_page_writeback()
> > TestClearPageWriteback()
> > mem_cgroup_migrate()
> > clear PCG_USED
> > if (PageCgroupUsed(pc))
> > decrease memcg pages under writeback
> > release pc->mem_cgroup->move_lock
> >
> > One solution for this would be to simply remove the PageCgroupUsed()
> > check, as RCU protects the memcg anyway.
> >
> > However, it's more robust to acknowledge that migration is really
> > modifying the charge state of alive pages in this case, and so it
> > should participate in the protocol specifically designed for this.
>
> It's been a long day so I might be missing something really obvious
> here. But how can move_lock help here when the fast path (no task
> migration is going on) takes only RCU read lock?
Argh, I actually noticed this issue while working on the page stat
simplification and thought I could break out a more isolated fix. But
you are right, that won't be enough, and I can't possibly put a RCU
grace period in mem_cgroup_migration().
I also just realized that we can't remove the PageCgroupUsed() check
when updating the page stat, either, because the "fast path" start of
the transaction does not verify the memcg for us - we can't tell
whether it's gone stale before or during the transaction. Grrr.
Andrew, please scratch this patch and the next 4-part series that
reworks the page stat updates. I'll send a reduced version of it
that's marked for 3.17-stable.
Thanks