linux-kernel - Re: [PATCH V7 2/2] mm: shmem: implement POSIX_FADV

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]

Message-ID: <aa4352d8-a549-32e5-874f-1cfee2a5b3e@google.com>
Date:   Wed, 17 May 2023 04:32:50 -0700 (PDT)
From:   Hugh Dickins <hughd@...gle.com>
To:     Charan Teja Kalla <quic_charante@...cinc.com>
cc:     Hugh Dickins <hughd@...gle.com>, akpm@...ux-foundation.org,
        willy@...radead.org, markhemm@...glemail.com, rientjes@...gle.com,
        surenb@...gle.com, shakeelb@...gle.com, fvdl@...gle.com,
        quic_pkondeti@...cinc.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V7 2/2] mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED
 for shmem

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