Re: [PATCH V7 0/2] mm: shmem: support POSIX_FADV_[WILL|DONT]NEED for shmem files

[Frank van der Linden <fvdl@google.com>]

On Fri, Apr 14, 2023 at 12:10 PM Charan Teja Kalla
<quic_charante@quicinc.com> wrote:
>
> Thanks Frank!!
>
> On 4/14/2023 11:14 PM, Frank van der Linden wrote:
> >> I didn't see this patch before, so I looked a bit at the history. At
> >> some point, in v3, dealing with mapped pages for DONTNEED was left
> >> out, they are now skipped. Unfortunately, that makes this patch no
> >> longer usable for a case that we have: restoring the (approximate)
> >> swap state of a tmpfs file. This involves walking a potentially large
> >> number of regions, and explicitly pushing them out to swap. This can
> >> be used to e.g. restore the state VM memory that is backed by a tmpfs
> >> file, avoiding memory usage by cold VM pages after resume.
> >>
>
> This is an interesting use case and I feel this really supports this
> patchset. IIUC, supporting the reclaim of mapped file pages through
> fadvise() helps this usecase where you can avoid traversing the large
> number of vma regions as you can directly issue the fadvise() on the
> shmem file 'fd' and it takes care. Am I correct?

Right, that's correct. The only snag here is that fadvise, with your
patch set, will skip mapped pages, which might be an issue for this
case.

>
> > Hmm, actually, looking at it a bit more, there are several issues
> > here. One is that with fadvise, you can't be sure if you are the only
> > one dealing with the page in a mapped way(with madvise, if mapcount ==
> > 1, that mean's it's just you, but you don't know that for fadvise, so
> > that makes correctly dealing with mapped pages harder).
> >
> Sorry, Why not for fadvise()? I can still attempt only if the page is
> mapped and its mapcount == 1, but then we already have madvise() for
> such pages and why not we simply use it.

Yes, you could use madvise (as I was thinking). One issue with that
is, though, that madvise(PAGEOUT) is based on a page table walk of
present PTEs. So you actually need to map and touch the page before
madvise will work, which is suboptimal. A direct fadvise solution
would be nicer, since that does a file mapping walk. However, that can
be addressed later, my comments weren't intended to raise an objection
- merely that there is a chance here to address this usecase. But that
shouldn't block anything. It's something to keep in mind, though.

I'll do some experiments to see what the best solution is here. But,
any follow-ups would be on top of your patch, so I'm not raising
objections, merely hoping that this, and possible extra work, could
solve this case.


- Frank