linux-kernel - Re: [PATCH] workingset: fix confusion around eviction vs refault container

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKEwX=Or-OJ=uiYUh88zGJ1bSAYjueTz-UqUEc_cRwCUni_OxQ@mail.gmail.com>
Date:   Wed, 4 Jan 2023 15:22:35 -0800
From:   Nhat Pham <nphamcs@...il.com>
To:     akpm@...ux-foundation.org
Cc:     hannes@...xchg.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, bfoster@...hat.com,
        willy@...radead.org, kernel-team@...a.com
Subject: Re: [PATCH] workingset: fix confusion around eviction vs refault container

On Wed, Jan 4, 2023 at 2:29 PM Nhat Pham <nphamcs@...il.com> wrote:
>
> From: Johannes Weiner <hannes@...xchg.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@...xchg.org>
> Co-developed-by: Nhat Pham <nphamcs@...il.com>
> Signed-off-by: Nhat Pham <nphamcs@...il.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!