Re: [patch 2/2] mm: memcontrol: fix missed end-writeback page accounting

Michal Hocko
2014-10-23T15:03:21Z

On Thu 23-10-14 09:57:29, Johannes Weiner wrote:
[...]
> From b518d88254b01be8c6c0c4a496d9f311f0c71b4a Mon Sep 17 00:00:00 2001
> From: Johannes Weiner <hannes@cmpxchg.org>
> Date: Thu, 23 Oct 2014 09:29:06 -0400
> Subject: [patch] mm: rmap: split out page_remove_file_rmap()
>
> page_remove_rmap() has too many branches on PageAnon() and is hard to
> follow. Move the file part into a separate function.
>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Reviewed-by: Michal Hocko <mhocko@suse.cz>

> ---
> mm/rmap.c | 78 +++++++++++++++++++++++++++++++++++++--------------------------
> 1 file changed, 46 insertions(+), 32 deletions(-)
>
> diff --git a/mm/rmap.c b/mm/rmap.c
> index f574046f77d4..19886fb2f13a 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1054,6 +1054,36 @@ void page_add_file_rmap(struct page *page)
> mem_cgroup_end_page_stat(memcg, locked, flags);
> }
>
> +static void page_remove_file_rmap(struct page *page)
> +{
> + struct mem_cgroup *memcg;
> + unsigned long flags;
> + bool locked;
> +
> + memcg = mem_cgroup_begin_page_stat(page, &locked, &flags);
> +
> + /* page still mapped by someone else? */
> + if (!atomic_add_negative(-1, &page->_mapcount))
> + goto out;
> +
> + /* Hugepages are not counted in NR_FILE_MAPPED for now. */
> + if (unlikely(PageHuge(page)))
> + goto out;
> +
> + /*
> + * We use the irq-unsafe __{inc|mod}_zone_page_stat because
> + * these counters are not modified in interrupt context, and
> + * pte lock(a spinlock) is held, which implies preemption disabled.
> + */
> + __dec_zone_page_state(page, NR_FILE_MAPPED);
> + mem_cgroup_dec_page_stat(memcg, MEM_CGROUP_STAT_FILE_MAPPED);
> +
> + if (unlikely(PageMlocked(page)))
> + clear_page_mlock(page);
> +out:
> + mem_cgroup_end_page_stat(memcg, locked, flags);
> +}
> +
> /**
> * page_remove_rmap - take down pte mapping from a page
> * @page: page to remove mapping from
> @@ -1062,46 +1092,33 @@ void page_add_file_rmap(struct page *page)
> */
> void page_remove_rmap(struct page *page)
> {
> - struct mem_cgroup *uninitialized_var(memcg);
> - bool anon = PageAnon(page);
> - unsigned long flags;
> - bool locked;
> -
> - /*
> - * The anon case has no mem_cgroup page_stat to update; but may
> - * uncharge_page() below, where the lock ordering can deadlock if
> - * we hold the lock against page_stat move: so avoid it on anon.
> - */
> - if (!anon)
> - memcg = mem_cgroup_begin_page_stat(page, &locked, &flags);
> + if (!PageAnon(page)) {
> + page_remove_file_rmap(page);
> + return;
> + }
>
> /* page still mapped by someone else? */
> if (!atomic_add_negative(-1, &page->_mapcount))
> - goto out;
> + return;
> +
> + /* Hugepages are not counted in NR_ANON_PAGES for now. */
> + if (unlikely(PageHuge(page)))
> + return;
>
> /*
> - * Hugepages are not counted in NR_ANON_PAGES nor NR_FILE_MAPPED
> - * and not charged by memcg for now.
> - *
> * We use the irq-unsafe __{inc|mod}_zone_page_stat because
> * these counters are not modified in interrupt context, and
> - * these counters are not modified in interrupt context, and
> * pte lock(a spinlock) is held, which implies preemption disabled.
> */
> - if (unlikely(PageHuge(page)))
> - goto out;
> - if (anon) {
> - if (PageTransHuge(page))
> - __dec_zone_page_state(page,
> - NR_ANON_TRANSPARENT_HUGEPAGES);
> - __mod_zone_page_state(page_zone(page), NR_ANON_PAGES,
> - -hpage_nr_pages(page));
> - } else {
> - __dec_zone_page_state(page, NR_FILE_MAPPED);
> - mem_cgroup_dec_page_stat(memcg, MEM_CGROUP_STAT_FILE_MAPPED);
> - }
> + if (PageTransHuge(page))
> + __dec_zone_page_state(page, NR_ANON_TRANSPARENT_HUGEPAGES);
> +
> + __mod_zone_page_state(page_zone(page), NR_ANON_PAGES,
> + -hpage_nr_pages(page));
> +
> if (unlikely(PageMlocked(page)))
> clear_page_mlock(page);
> +
> /*
> * It would be tidy to reset the PageAnon mapping here,
> * but that might overwrite a racing page_add_anon_rmap
> @@ -1111,9 +1128,6 @@ void page_remove_rmap(struct page *page)
> * Leaving it set also helps swapoff to reinstate ptes
> * faster for those pages still in swapcache.
> */
> -out:
> - if (!anon)
> - mem_cgroup_end_page_stat(memcg, locked, flags);
> }
>
> /*
> --
> 2.1.2
>

--
Michal Hocko
SUSE Labs