Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 17 May 2023 04:32:50 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Charan Teja Kalla <quic_charante@quicinc.com>
cc:     Hugh Dickins <hughd@google.com>, 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
Subject: Re: [PATCH V7 2/2] mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED
 for shmem
In-Reply-To: <eeeba374-9247-96fd-c9f5-8cba8761f1b9@quicinc.com>
Message-ID: <aa4352d8-a549-32e5-874f-1cfee2a5b3e@google.com>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 24 Apr 2023, Charan Teja Kalla wrote:
> On 4/21/2023 5:37 AM, Hugh Dickins wrote:
> > This is where I ran out of time.  I'm afraid all the focus on
> > fadvise_calc_endbyte() has distracted you from looking at the DONTNEED
> > in mm/fadvise.c: where there are detailed comments on why and how it
> > then narrows the DONTNEED range.  And aside from needing to duplicate
> > that here for shmem (or put it into another or combined helper), it
> > implies to me that shmem_isolate_pages_range() needs to do a similar
> > narrowing, when it finds that the range overlaps part of a large folio.
> > 
> 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 :(

> 
> > Something that has crossed my mind as a worry, but I've not had time
> > to look further into (maybe it's no concern at all) is the question
> > of this syscall temporarily isolating a very large number of folios,
> > whether they need to be (or perhaps already are) counted in
> > NR_ISOLATED_ANON, whether too many isolated needs to be limited.
> 
> They are _not_ counted as ISOLATED_ANON now as this operation is for a
> small duration. I do see there exists too_many_isolated() checks in
> direct reclaim/compaction logic where it is necessary to stop the
> multiple processes in the direct reclaim from isolating too many pages.
> 
> I am not able to envisage such problem here, where usually single
> process doing the fadvise operation on a file. Even If the file is
> opened by multiple processes and do fadvise, the operation is limited
> only to the pages of this file and doesn't impact the system.
> 
> 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.

I'm not claiming that mm/vmscan.c's too_many_isolated(), and the way it
gets used by shrink_inactive_list(), is perfect: not at all.  But please
follow existing convention.

Sorry, that's all for now.
Hugh