Received-SPF: pass (google.com: domain of linux-kernel+bounces-64931-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <64ed46f4-459c-63b0-a69e-81353e9fcbc9@quicinc.com>
Date: Wed, 14 Feb 2024 14:43:57 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH V7 2/2] mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED
 for shmem
Content-Language: en-US
From: Charan Teja Kalla <quic_charante@quicinc.com>
To: Hugh Dickins <hughd@google.com>
CC: <akpm@linux-foundation.org>, <willy@infradead.org>,
        <markhemm@googlemail.com>, <rientjes@google.com>, <surenb@google.com>,
        <shakeelb@google.com>, <fvdl@google.com>, <quic_pkondeti@quicinc.com>,
        <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>,
        Minchan Kim
	<minchan@kernel.org>
References: <cover.1676378702.git.quic_charante@quicinc.com>
 <631e42b6dffdcc4b4b24f5be715c37f78bf903db.1676378702.git.quic_charante@quicinc.com>
 <2d56e1dd-68b5-c99e-522f-f8dadf6ad69e@google.com>
 <eeeba374-9247-96fd-c9f5-8cba8761f1b9@quicinc.com>
 <aa4352d8-a549-32e5-874f-1cfee2a5b3e@google.com>
 <e8e85d7d-edf1-7a8b-8cfe-9976dd9cfb0b@quicinc.com>
In-Reply-To: <e8e85d7d-edf1-7a8b-8cfe-9976dd9cfb0b@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

Hello Hugh,

Based on offline discussion with some folks in the list, it seems that
this syscall can be helpful. This patch might have forgotten and I hope
this ping helps in resurrecting this thread.

On 5/18/2023 6:16 PM, Charan Teja Kalla wrote:
> On 5/17/2023 5:02 PM, Hugh Dickins wrote:
>>> Sure, will include those range calculations for shmem pages too.
>> Oh, I forgot this issue, you would have liked me to look at V8 by now,
>> to see whether I agree with your resolution there.  Sorry, no, I've
>> not been able to divert my concentration to it yet.
>>
>> And it's quite likely that I shall disagree, because I've a history of
>> disagreeing even with myself on such range widening/narrowing issues -
>> reconciling conflicting precedents is difficult ????
>>
> If you can at least help by commenting which part of the patch you
> disagree with, I can try hard to convince you there:) .
> 
>>> Please let me know if I'm missing something where I should be counting
>>> these as NR_ISOLATED.
>> Please grep for NR_ISOLATED, to see where and how they get manipulated
>> already, and follow the existing examples.  The case that sticks in my
>> mind is in mm/mempolicy.c, where the migrate_pages() syscall can build
>> up a gigantic quantity of transiently isolated pages: your syscall can
>> do the same, so should account for itself in the same way.

Based on the grep, it seems almost all the call stacks that isolates the
folios is for migrating the pages where after migration the NR_ISOLATED
is decremented (in migrate_folio_done()). The call paths are(compaction,
memory hotplug, mempolicy).

The another call path is reclaim where we isolate 'nr' pages belongs to
a pgdat, account/unaccount them in NR_ISOLATED across the reclaim.

I think it is easy to account for the above call paths as we know "which
folio corresponds to which pgdat".

Where as in this patch, we are isolating a set of folios(can corresponds
to different nodes) and relying on the reclaim_pages() to do the swap
out. It is straightforward to account NR_ISOLATED while isolating, but
it requires unaccounting changes in the shrink_folio_list() where folio
is being freed after swap out.  Doing so requires changes in all the
code places(eg: shrink_inactive_list()), where it now requires to
account NR_ISOLATED while isolating and the shrink_folio_list()
unaccounts it.

So, accounting NR_ISOLATED requires changes in other code places where
this patch has not touched.

If isolating a large amount of pages and not being recorded in
NR_ISOLATED is really a problem, then may I please know your opinion on
isolating(with out accounting) and reclaiming in small batches? The
batch size can be considered as SWAP_CLUSTER_MAX of pages.

> I had a V8 posted without this into accounting. Let me make the changes
> to account for the NR_ISOLATED too.

Thanks,
Charan