Message-ID: <CAPTztWadceJtPUrSab1Tj2WV=uAhLo+CrxqyeSQ8rLn0FtM_zA@mail.gmail.com>
Date:   Fri, 14 Apr 2023 10:44:18 -0700
From:   Frank van der Linden <fvdl@...gle.com>
To:     Charan Teja Kalla <quic_charante@...cinc.com>
Cc:     akpm@...ux-foundation.org, hughd@...gle.com, willy@...radead.org,
        markhemm@...glemail.com, rientjes@...gle.com, surenb@...gle.com,
        shakeelb@...gle.com, quic_pkondeti@...cinc.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V7 0/2] mm: shmem: support POSIX_FADV_[WILL|DONT]NEED for
 shmem files

On Thu, Apr 13, 2023 at 12:45 PM Frank van der Linden <fvdl@...gle.com> wrote:
>
> On Tue, Feb 14, 2023 at 4:53 AM Charan Teja Kalla
> <quic_charante@...cinc.com> wrote:
> >
> > This patch aims to implement POSIX_FADV_WILLNEED and POSIX_FADV_DONTNEED
> > advices to shmem files which can be helpful for the drivers who may want
> > to manage the pages of shmem files on their own, like, that are created
> > through shmem_file_setup[_with_mnt]().
> >
> > Changes in V7:
> >  -- Use folio based interface, shmem_read_folio(), for FADV_WILLNEED.
> >  -- Don't swap the SHM_LOCK'ed pages.
> >
> > Changes in V6:
> >  -- Replaced the pages with folio's for shmem changes.
> >  -- https://lore.kernel.org/all/cover.1675690847.git.quic_charante@quicinc.com/
> >
> > Changes in V5:
> >  -- Moved the 'endbyte' calculations to a header function for use by shmem_fadvise().
> >  -- Addressed comments from suren.
> >  -- No changes in resend. Retested on the latest tip.
> >  -- https://lore.kernel.org/all/cover.1648706231.git.quic_charante@quicinc.com/
> >
> > Changes in V4:
> >   -- Changed the code to use reclaim_pages() to writeout the shmem pages to swap and then reclaim.
> >   -- Addressed comments from Mark Hemment and Matthew.
> >   -- fadvise() on shmem file may even unmap a page.
> >   -- https://patchwork.kernel.org/project/linux-mm/patch/1644572051-24091-1-git-send-email-quic_charante@quicinc.com/
> >
> > Changes in V3:
> >   -- Considered THP pages while doing FADVISE_[DONT|WILL]NEED, identified by Matthew.
> >   -- xarray used properly, as identified by Matthew.
> >   -- Excluded mapped pages as it requires unmapping and the man pages of fadvise don't talk about them.
> >   -- RESEND: Fixed the compilation issue when CONFIG_TMPFS is not defined.
> >   -- https://patchwork.kernel.org/project/linux-mm/patch/1641488717-13865-1-git-send-email-quic_charante@quicinc.com/
> >
> > Changes in V2:
> >   -- Rearranged the code to not to sleep with rcu_lock while using xas_() functionality.
> >   -- Addressed the comments from Suren.
> >   -- https://patchwork.kernel.org/project/linux-mm/patch/1638442253-1591-1-git-send-email-quic_charante@quicinc.com/
> >
> > changes in V1:
> >   -- Created the interface for fadvise(2) to work on shmem files.
> >   -- https://patchwork.kernel.org/project/linux-mm/patch/1633701982-22302-1-git-send-email-charante@codeaurora.org/
> >
> >
> > Charan Teja Kalla (2):
> >   mm: fadvise: move 'endbyte' calculations to helper function
> >   mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED for shmem
> >
> >  mm/fadvise.c  |  11 +-----
> >  mm/internal.h |  21 +++++++++++
> >  mm/shmem.c    | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 138 insertions(+), 10 deletions(-)
> >
> > --
> > 2.7.4
> >
> >
>
> 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.
>
> If DONTNEED also reclaims mapped pages (e.g. they get pushed out to
> swap, if any), implementing the above use case efficiently is simple:
> use io_uring with a vector that contains each region and the fadvise
> method.
>
> Without DONTNEED reclaiming mapped pages, you'd have to do mmap +
> madvise(MADV_PAGEOUT) for each region that you want swapped out, which
> is rather inefficient.
>
> I understand that the semantics for POSIX_FADV_DONTNEED on shmem/tmpfs
> files are open to interpretation, as it is a special case. And you can
> certainly make the argument that relying on behavior caused by what
> can be considered an implementation detail is bad.
>
> So, is there any way we can make this use case work efficiently using
> this patch?
>
> You state in the commit message:
>
> > So, FADV_DONTNEED also covers the semantics of MADV_PAGEOUT for file pages
> > and there is no purpose of PAGEOUT for file pages.
>
> But that doesn't seem correct: for shmem file pages, there actually
> can be a purpose, and the FADV_DONTNEED as implemented for shmem in
> this patch set does not cover the semantics.
>
> You can say that it doesn't need to cover the pageout case of mapped
> shmem pages, and that's fair. But I don't think you can claim that it
> covers the case as currently implemented.
>
> I suppose there are three options here:
>
> 1) Do nothing, this use case will just have to spend more time doing
> mmap+madvise
> 2) Don't skip mapped pages for POSIX_FADV_DONTNEED in shmem_fadvise
> 3) Implement something like POSIX_FADV_PAGEOUT_NP, which would include
> mapped pages.
>
> What do people think?
>
> - Frank

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).

Also, the madvise loop issue can probably also be dealt with via io_uring.

So, I think we can deal with the use case I mentioned in a different
way, that is mostly unrelated to this patchset, so basically:
disregard my previous email.

- Frank

Powered by blists - more mailing lists