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;
Subject: Re: [PATCH] vm_swappiness=0 should still try to avoid swapping anon
 memory
To:     Shakeel Butt <shakeelb@google.com>
Cc:     Linux MM <linux-mm@kvack.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Rafael Aquini <aquini@redhat.com>, longman@redhat.com
References: <20210806231701.106980-1-npache@redhat.com>
 <CALvZod6gCof1bhVwdU7vYYKBRCn_HZBFi4BjSYoSK-dyrmswMA@mail.gmail.com>
From:   Nico Pache <npache@redhat.com>
Message-ID: <91605888-e343-2712-c097-bcade4cb389d@redhat.com>
Date:   Fri, 6 Aug 2021 21:37:13 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <CALvZod6gCof1bhVwdU7vYYKBRCn_HZBFi4BjSYoSK-dyrmswMA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 8/6/21 9:00 PM, Shakeel Butt wrote:
> On Fri, Aug 6, 2021 at 4:17 PM Nico Pache <npache@redhat.com> wrote:
>> Since commit b91ac374346b ("mm: vmscan: enforce inactive:active ratio at the
>> reclaim root") swappiness can start prematurely swapping anon memory.
>> This is due to the assumption that refaulting anon should always allow
>> the shrinker to target anon memory. Add a check for vm_swappiness being
>>> 0 before indiscriminately targeting Anon.
> Did you actually observe this behavior?
Yes, and I've successfully tested this patch. It does solve the issue.
>
>> Signed-off-by: Nico Pache <npache@redhat.com>
>> ---
>>  mm/vmscan.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 4620df62f0ff..8b932ff72e37 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -2909,8 +2909,8 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc)
>>
>>                 refaults = lruvec_page_state(target_lruvec,
>>                                 WORKINGSET_ACTIVATE_ANON);
>> -               if (refaults != target_lruvec->refaults[0] ||
>> -                       inactive_is_low(target_lruvec, LRU_INACTIVE_ANON))
>> +               if (vm_swappiness && (refaults != target_lruvec->refaults[0] ||
>> +                       inactive_is_low(target_lruvec, LRU_INACTIVE_ANON)))
> If you are really seeing the said behavior then why will this fix it.
> This is just about deactivating active anon LRU. I would rather look
> at get_scan_count() to check why swappiness = 0 is still letting the
> kernel to scan anon LRU. BTW in cgroup v1, the memcg can overwrite
> their swappiness which will be preferred over system vm_swappiness.
> Did you set system level swappiness or memcg one?

This fixes the issue because shrink_list() uses the may_deactivate field to determine if it should shrink the active list. This is not the only place that can cause the may_deactivate to deactivate anon, but it is the common path of kswapd/balance_pgdat. I can look into a get_scan_count() solution however this line is the ultimate cause of telling scan controller to go for anon so i figured this is the best spot ( stop the problem at the root, not all the way down in the call path). The get_scan_count balance can also be further modified after some shrinking occurs in shrink_lruvec. 

This is only the system level swappiness. As far as cgroups, I will also take a look into that to make sure we can generalize the solution for that as well. I dont think it should be too hard. 


Thanks for the review!

-- Nico


>>                         sc->may_deactivate |= DEACTIVATE_ANON;
>>                 else
>>                         sc->may_deactivate &= ~DEACTIVATE_ANON;
>> --
>> 2.31.1
>>