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: <20210809223740.59009-1-npache@redhat.com> <YRKa0yzEDALtvSZO@cmpxchg.org>
In-Reply-To: <YRKa0yzEDALtvSZO@cmpxchg.org>
From:   Shakeel Butt <shakeelb@google.com>
Date:   Tue, 10 Aug 2021 14:16:12 -0700
Message-ID: <CALvZod4MEU24X6kST7BwsbdDPOQGN2LZhCN294PJj+hgmGbWTw@mail.gmail.com>
Subject: Re: [PATCH v3] vm_swappiness=0 should still try to avoid swapping
 anon memory
To:     Johannes Weiner <hannes@cmpxchg.org>
Cc:     Nico Pache <npache@redhat.com>, Linux MM <linux-mm@kvack.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Rafael Aquini <aquini@redhat.com>,
        Waiman Long <llong@redhat.com>, Michal Hocko <mhocko@suse.com>,
        hakavlad@inbox.lv
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Johannes,

On Tue, Aug 10, 2021 at 8:27 AM Johannes Weiner <hannes@cmpxchg.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?