Re: [RFC PATCH 1/3] mm/mincore, swap: consolidate swap cache checking for mincore

[Kairui Song <ryncsn@gmail.com>]

On Fri, Aug 8, 2025 at 2:07 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Thu, Aug 7, 2025 at 8:27 AM Kairui Song <ryncsn@gmail.com> wrote:
> >
> > From: Kairui Song <kasong@tencent.com>
> >
> > The filemap_get_incore_folio (previously find_get_incore_page) helper
> > was introduced by commit 61ef18655704 ("mm: factor find_get_incore_page
> > out of mincore_page") to be used by later commit f5df8635c5a3 ("mm: use
> > find_get_incore_page in memcontrol"), so memory cgroup charge move code
> > can be simplified.
> >
> > But commit 6b611388b626 ("memcg-v1: remove charge move code") removed
> > that user completely, it's only used by mincore now.
> >
> > So this commit basically reverts commit 61ef18655704 ("mm: factor
> > find_get_incore_page out of mincore_page"). Move it back to mincore side
> > to simplify the code.
> >
> > Signed-off-by: Kairui Song <kasong@tencent.com>
>
> Seems reasonable to me for the most part - just a couple of questions below.
>
> > ---
> >  mm/mincore.c    | 29 +++++++++++++++++++++++++++--
> >  mm/swap.h       | 10 ----------
> >  mm/swap_state.c | 38 --------------------------------------
> >  3 files changed, 27 insertions(+), 50 deletions(-)
> >
> > diff --git a/mm/mincore.c b/mm/mincore.c
> > index 10dabefc3acc..f0d3c9419e58 100644
> > --- a/mm/mincore.c
> > +++ b/mm/mincore.c
> > @@ -64,8 +64,33 @@ static unsigned char mincore_page(struct address_space *mapping, pgoff_t index)
> >          * any other file mapping (ie. marked !present and faulted in with
> >          * tmpfs's .fault). So swapped out tmpfs mappings are tested here.
> >          */
> > -       folio = filemap_get_incore_folio(mapping, index);
> > -       if (!IS_ERR(folio)) {
> > +       if (IS_ENABLED(CONFIG_SWAP) && shmem_mapping(mapping)) {
>
> Do we need CONFIG_SWAP check here? I suppose if !CONFIG_SWAP we'll
> never end up with an ordinary swap entry stored here right?

Yes, and in the next patch I'd like to introduce a WARN_ON if we see
swap entries with !CONFIG_SWAP. That means the memory is corrupted.

>
> Saves a couple of cycles, I suppose. No strong opinions.

Before 61ef18655704 it used a `#ifdef CONFIG_SWAP`, I used
IS_ENABLED(CONFIG_SWAP) here, same thing, the compiler will optimize
out the unused branch. Just with fewer lines of code and I personally
think this looks prettier.

>
> > +               folio = filemap_get_entry(mapping, index);
> > +               /*
> > +                * shmem/tmpfs may return swap: account for swapcache
> > +                * page too.
> > +                */
> > +               if (xa_is_value(folio)) {
> > +                       struct swap_info_struct *si;
> > +                       swp_entry_t swp = radix_to_swp_entry(folio);
> > +                       /* There might be swapin error entries in shmem mapping. */
> > +                       if (non_swap_entry(swp))
> > +                               return 0;
> > +                       /* Prevent swap device to being swapoff under us */
> > +                       si = get_swap_device(swp);
> > +                       if (si) {
> > +                               folio = filemap_get_folio(swap_address_space(swp),
> > +                                                         swap_cache_index(swp));
> > +                               put_swap_device(si);
> > +                       } else {
> > +                               return 0;
> > +                       }
> > +               }
> > +       } else {
> > +               folio = filemap_get_folio(mapping, index);
> > +       }
> > +
> > +       if (folio) {
>
> Should this check be "if (!IS_ERR(folio))"? Seems like that's how we
> inspect the output of filemap_get_folio() in other locations (for e.g,
> in filemap_fault()).

Yeah you are right, actuall should be IS_ERR_OR_NULL here as it uses
both filemap_get_entry and filemap_get_folio.
I wanted to change to always use filemap_get_entry in the next patch
for better performance, but somehow forgot it...
Will fix it.

>
> >                 present = folio_test_uptodate(folio);
> >                 folio_put(folio);
> >         }
>