linux-kernel - Re: [PATCH v3] vm_swappiness=0 should still try to avoid swapping anon memory

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Aug 2021 14:16:12 -0700
From:   Shakeel Butt <shakeelb@...gle.com>
To:     Johannes Weiner <hannes@...xchg.org>
Cc:     Nico Pache <npache@...hat.com>, Linux MM <linux-mm@...ck.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Rafael Aquini <aquini@...hat.com>,
        Waiman Long <llong@...hat.com>, Michal Hocko <mhocko@...e.com>,
        hakavlad@...ox.lv
Subject: Re: [PATCH v3] vm_swappiness=0 should still try to avoid swapping
 anon memory

Hi Johannes,

On Tue, Aug 10, 2021 at 8:27 AM Johannes Weiner <hannes@...xchg.org> wrote:
>
[...]
> One thing I think we should do - whether we need more on top or not -
> is allowing file reclaim to continue when sc->file_is_tiny. Yes, we
> also need anon to meet the watermarks, but it's not clear why we
> should stop scanning file pages altogether: it's possible they get us
> there 99% of the way, and somebody clearly wanted us to swap as little
> as possible to end up in a situation like that, so:
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index eeab6611993c..90dac3dc9903 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2477,7 +2477,7 @@ static void get_scan_count(struct lruvec *lruvec, struct scan_control *sc,
>          * If the system is almost out of file pages, force-scan anon.
>          */
>         if (sc->file_is_tiny) {
> -               scan_balance = SCAN_ANON;
> +               scan_balance = SCAN_EQUAL;
>                 goto out;
>         }
>

Another thing we should do is to re-evaluate the sc->file_is_tiny
condition. Currently it is:

anon = node_page_state(pgdat, NR_INACTIVE_ANON);
sc->file_is_tiny = file + free <= total_high_wmark &&
!(sc->may_deactivate & DEACTIVATE_ANON) && anon >> sc->priority;

First convert node_page_state() usage to lruvec_page_state() for
common source of truth.

Second, in the commit b91ac374346b (sc->may_deactivate &
DEACTIVATE_ANON) implies inactive_is_low(LRU_INACTIVE_ANON) but commit
170b04b7ae49 changed that. Was that intended?

Third, the comment above this code says "Consider anon" but it is only
considering inactive anon. Do we need to change the comment or the
check?