Re: [PATCH 7/8] mm: drop page_index/page_file_offset and convert swap helpers to use folio

[Barry Song]

On Thu, Apr 18, 2024 at 2:42 PM Kairui Song <ryncsn@xxxxxxxxx> wrote:
>
> On Thu, Apr 18, 2024 at 9:55 AM Barry Song <21cnbao@xxxxxxxxx> wrote:
> >
> > On Thu, Apr 18, 2024 at 4:12 AM Kairui Song <ryncsn@xxxxxxxxx> wrote:
> > >
> > > From: Kairui Song <kasong@xxxxxxxxxxx>
> > >
> > > When applied on swap cache pages, page_index / page_file_offset was used
> > > to retrieve the swap cache index or swap file offset of a page, and they
> > > have their folio equivalence version: folio_index / folio_file_pos.
> > >
> > > We have eliminated all users for page_index / page_file_offset, everything
> > > is using folio_index / folio_file_pos now, so remove the old helpers.
> > >
> > > Then convert the implementation of folio_index / folio_file_pos to
> > > to use folio natively.
> > >
> > > After this commit, all users that might encounter mixed usage of swap
> > > cache and page cache will only use following two helpers:
> > >
> > > folio_index (calls __folio_swap_cache_index)
> > > folio_file_pos (calls __folio_swap_file_pos)
> > >
> > > The offset in swap file and index in swap cache is still basically the
> > > same thing at this moment, but will be different in following commits.
> > >
> > > Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> >
> > Hi Kairui, thanks !
> >
> > I also find it rather odd that folio_file_page() is utilized for both
> > swp and file.
> >
> > mm/memory.c <<do_swap_page>>
> >              page = folio_file_page(folio, swp_offset(entry));
> > mm/swap_state.c <<swapin_readahead>>
> >              return folio_file_page(folio, swp_offset(entry));
> > mm/swapfile.c <<unuse_pte>>
> >              page = folio_file_page(folio, swp_offset(entry));
> >
> > Do you believe it's worthwhile to tidy up?
> >
>
> Hi Barry,
>
> I'm not sure about this. Using folio_file_page doesn't look too bad,
> and it will be gone once we convert them to always use folio, this
> shouldn't take too long.

HI Kairui,
I am not quite sure this is going to be quite soon. our swap-in large
folios refault still have corner cases which can't map large folios even
we hit large folios in swapcache [1].
personally, i feel do_swap_page() isn't handling file, and those pages
are anon but not file.  so a separate helper taking folio and entry as
arguments seem more readable as Chris even wants to remove
the assumption large folios have been to swapped to contiguous
swap offsets. anyway, it is just me :-）

[1] https://lore.kernel.org/linux-mm/20240409082631.187483-1-21cnbao@xxxxxxxxx/