linux-kernel - Re: [PATCH V7 0/2] mm: shmem: support 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]
Date:   Thu, 13 Apr 2023 12:45:24 -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 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