Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <alpine.LSU.2.11.2008301343270.5954@eggly.anvils> <alpine.LSU.2.11.2008301408230.5954@eggly.anvils>
In-Reply-To: <alpine.LSU.2.11.2008301408230.5954@eggly.anvils>
From:   Yang Shi <shy828301@gmail.com>
Date:   Tue, 1 Sep 2020 08:41:11 -0700
Message-ID: <CAHbLzkpdW5hLVP0wUcwrrEnnHpqnrHRWZnMWtiGnhezNaEDyzA@mail.gmail.com>
Subject: Re: [PATCH 5/5] mlock: fix unevictable_pgs event counts on THP
To:     Hugh Dickins <hughd@google.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Alex Shi <alex.shi@linux.alibaba.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@suse.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Shakeel Butt <shakeelb@google.com>,
        Matthew Wilcox <willy@infradead.org>, Qian Cai <cai@lca.pw>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux MM <linux-mm@kvack.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Aug 30, 2020 at 2:09 PM Hugh Dickins <hughd@google.com> wrote:
>
> 5.8 commit 5d91f31faf8e ("mm: swap: fix vmstats for huge page") has
> established that vm_events should count every subpage of a THP,
> including unevictable_pgs_culled and unevictable_pgs_rescued; but
> lru_cache_add_inactive_or_unevictable() was not doing so for
> unevictable_pgs_mlocked, and mm/mlock.c was not doing so for
> unevictable_pgs mlocked, munlocked, cleared and stranded.
>
> Fix them; but THPs don't go the pagevec way in mlock.c,
> so no fixes needed on that path.
>
> Fixes: 5d91f31faf8e ("mm: swap: fix vmstats for huge page")
> Signed-off-by: Hugh Dickins <hughd@google.com>

Acked-by: Yang Shi <shy828301@gmail.com>

> ---
> I've only checked UNEVICTABLEs: there may be more inconsistencies left.
> The check_move_unevictable_pages() patch brought me to this one, but
> this is more important because mlock works on all THPs, without needing
> special testing "force".  But, it's still just monotonically increasing
> event counts, so not all that important.
>
>  mm/mlock.c |   24 +++++++++++++++---------
>  mm/swap.c  |    6 +++---
>  2 files changed, 18 insertions(+), 12 deletions(-)
>
> --- 5.9-rc2/mm/mlock.c  2020-08-16 17:32:50.665507048 -0700
> +++ linux/mm/mlock.c    2020-08-28 17:42:07.975278411 -0700
> @@ -58,11 +58,14 @@ EXPORT_SYMBOL(can_do_mlock);
>   */
>  void clear_page_mlock(struct page *page)
>  {
> +       int nr_pages;
> +
>         if (!TestClearPageMlocked(page))
>                 return;
>
> -       mod_zone_page_state(page_zone(page), NR_MLOCK, -thp_nr_pages(page));
> -       count_vm_event(UNEVICTABLE_PGCLEARED);
> +       nr_pages = thp_nr_pages(page);
> +       mod_zone_page_state(page_zone(page), NR_MLOCK, -nr_pages);
> +       count_vm_events(UNEVICTABLE_PGCLEARED, nr_pages);
>         /*
>          * The previous TestClearPageMlocked() corresponds to the smp_mb()
>          * in __pagevec_lru_add_fn().
> @@ -76,7 +79,7 @@ void clear_page_mlock(struct page *page)
>                  * We lost the race. the page already moved to evictable list.
>                  */
>                 if (PageUnevictable(page))
> -                       count_vm_event(UNEVICTABLE_PGSTRANDED);
> +                       count_vm_events(UNEVICTABLE_PGSTRANDED, nr_pages);
>         }
>  }
>
> @@ -93,9 +96,10 @@ void mlock_vma_page(struct page *page)
>         VM_BUG_ON_PAGE(PageCompound(page) && PageDoubleMap(page), page);
>
>         if (!TestSetPageMlocked(page)) {
> -               mod_zone_page_state(page_zone(page), NR_MLOCK,
> -                                   thp_nr_pages(page));
> -               count_vm_event(UNEVICTABLE_PGMLOCKED);
> +               int nr_pages = thp_nr_pages(page);
> +
> +               mod_zone_page_state(page_zone(page), NR_MLOCK, nr_pages);
> +               count_vm_events(UNEVICTABLE_PGMLOCKED, nr_pages);
>                 if (!isolate_lru_page(page))
>                         putback_lru_page(page);
>         }
> @@ -138,7 +142,7 @@ static void __munlock_isolated_page(stru
>
>         /* Did try_to_unlock() succeed or punt? */
>         if (!PageMlocked(page))
> -               count_vm_event(UNEVICTABLE_PGMUNLOCKED);
> +               count_vm_events(UNEVICTABLE_PGMUNLOCKED, thp_nr_pages(page));
>
>         putback_lru_page(page);
>  }
> @@ -154,10 +158,12 @@ static void __munlock_isolated_page(stru
>   */
>  static void __munlock_isolation_failed(struct page *page)
>  {
> +       int nr_pages = thp_nr_pages(page);
> +
>         if (PageUnevictable(page))
> -               __count_vm_event(UNEVICTABLE_PGSTRANDED);
> +               __count_vm_events(UNEVICTABLE_PGSTRANDED, nr_pages);
>         else
> -               __count_vm_event(UNEVICTABLE_PGMUNLOCKED);
> +               __count_vm_events(UNEVICTABLE_PGMUNLOCKED, nr_pages);
>  }
>
>  /**
> --- 5.9-rc2/mm/swap.c   2020-08-16 17:32:50.709507284 -0700
> +++ linux/mm/swap.c     2020-08-28 17:42:07.975278411 -0700
> @@ -494,14 +494,14 @@ void lru_cache_add_inactive_or_unevictab
>
>         unevictable = (vma->vm_flags & (VM_LOCKED | VM_SPECIAL)) == VM_LOCKED;
>         if (unlikely(unevictable) && !TestSetPageMlocked(page)) {
> +               int nr_pages = thp_nr_pages(page);
>                 /*
>                  * We use the irq-unsafe __mod_zone_page_stat because this
>                  * counter is not modified from interrupt context, and the pte
>                  * lock is held(spinlock), which implies preemption disabled.
>                  */
> -               __mod_zone_page_state(page_zone(page), NR_MLOCK,
> -                                   thp_nr_pages(page));
> -               count_vm_event(UNEVICTABLE_PGMLOCKED);
> +               __mod_zone_page_state(page_zone(page), NR_MLOCK, nr_pages);
> +               count_vm_events(UNEVICTABLE_PGMLOCKED, nr_pages);
>         }
>         lru_cache_add(page);
>  }
>