Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230104222944.2380117-1-nphamcs@gmail.com>
In-Reply-To: <20230104222944.2380117-1-nphamcs@gmail.com>
From:   Nhat Pham <nphamcs@gmail.com>
Date:   Wed, 4 Jan 2023 15:22:35 -0800
Message-ID: <CAKEwX=Or-OJ=uiYUh88zGJ1bSAYjueTz-UqUEc_cRwCUni_OxQ@mail.gmail.com>
Subject: Re: [PATCH] workingset: fix confusion around eviction vs refault container
To:     akpm@linux-foundation.org
Cc:     hannes@cmpxchg.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, bfoster@redhat.com,
        willy@infradead.org, kernel-team@meta.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jan 4, 2023 at 2:29 PM Nhat Pham <nphamcs@gmail.com> wrote:
>
> From: Johannes Weiner <hannes@cmpxchg.org>
>
> Refault decisions are made based on the lruvec where the page was
> evicted, as that determined its LRU order while it was alive. Stats
> and workingset aging must then occur on the lruvec of the new page, as
> that's the node and cgroup that experience the refault and that's the
> lruvec whose nonresident info ages out by a new resident page. Those
> lruvecs could be different when a page is shared between cgroups, or
> the refaulting page is allocated on a different node.
>
> There are currently two mix-ups:
>
> 1. When swap is available, the resident anon set must be considered
>    when comparing the refault distance. The comparison is made against
>    the right anon set, but the check for swap is not. When pages get
>    evicted from a cgroup with swap, and refault in one without, this
>    can incorrectly consider a hot refault as cold - and vice
>    versa. Fix that by using the eviction cgroup for the swap check.
>
> 2. The stats and workingset age are updated against the wrong lruvec
>    altogether: the right cgroup but the wrong NUMA node. When a page
>    refaults on a different NUMA node, this will have confusing stats
>    and distort the workingset age on a different lruvec - again
>    possibly resulting in hot/cold misclassifications down the line.
>
> Fix the swap check and the refault pgdat to address both concerns.
>
> This was found during code review. It hasn't caused notable issues in
> production, suggesting that those refault-migrations are relatively
> rare in practice.
>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> Co-developed-by: Nhat Pham <nphamcs@gmail.com>
> Signed-off-by: Nhat Pham <nphamcs@gmail.com>
> ---
>  mm/workingset.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/workingset.c b/mm/workingset.c
> index ae7e984b23c6..79585d55c45d 100644
> --- a/mm/workingset.c
> +++ b/mm/workingset.c
> @@ -457,6 +457,7 @@ void workingset_refault(struct folio *folio, void *shadow)
>          */
>         nr = folio_nr_pages(folio);
>         memcg = folio_memcg(folio);
> +       pgdat = folio_pgdat(folio);
>         lruvec = mem_cgroup_lruvec(memcg, pgdat);
>
>         mod_lruvec_state(lruvec, WORKINGSET_REFAULT_BASE + file, nr);
> @@ -474,7 +475,7 @@ void workingset_refault(struct folio *folio, void *shadow)
>                 workingset_size += lruvec_page_state(eviction_lruvec,
>                                                      NR_INACTIVE_FILE);
>         }
> -       if (mem_cgroup_get_nr_swap_pages(memcg) > 0) {
> +       if (mem_cgroup_get_nr_swap_pages(eviction_memcg) > 0) {
>                 workingset_size += lruvec_page_state(eviction_lruvec,
>                                                      NR_ACTIVE_ANON);
>                 if (file) {
> --
> 2.30.2

Oh this one is sent out twice too... Something is wrong with my pipeline...
Anyway, please disregard the first email and only review this one!