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

Commit Message

Kairui Song April 17, 2024, 4:08 p.m. UTC

From: Kairui Song <[e-mail]>

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 <[e-mail]>
---
 include/linux/mm.h      | 13 -------------
 include/linux/pagemap.h | 19 +++++++++----------
 mm/swapfile.c           | 13 +++++++++----
 3 files changed, 18 insertions(+), 27 deletions(-)

Comments

Barry Song April 18, 2024, 1:55 a.m. UTC | #1

On Thu, Apr 18, 2024 at 4:12 AM Kairui Song <[e-mail]> wrote:
>
> From: Kairui Song <[e-mail]>
>
> 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 <[e-mail]>

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?

> ---
>  include/linux/mm.h      | 13 -------------
>  include/linux/pagemap.h | 19 +++++++++----------
>  mm/swapfile.c           | 13 +++++++++----
>  3 files changed, 18 insertions(+), 27 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 0436b919f1c7..797480e76c9c 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2245,19 +2245,6 @@ static inline void *folio_address(const struct folio *folio)
>         return page_address(&folio->page);
>  }
>
> -extern pgoff_t __page_file_index(struct page *page);
> -
> -/*
> - * Return the pagecache index of the passed page.  Regular pagecache pages
> - * use ->index whereas swapcache pages use swp_offset(->private)
> - */
> -static inline pgoff_t page_index(struct page *page)
> -{
> -       if (unlikely(PageSwapCache(page)))
> -               return __page_file_index(page);
> -       return page->index;
> -}
> -
>  /*
>   * Return true only if the page has been allocated with
>   * ALLOC_NO_WATERMARKS and the low watermark was not
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 2df35e65557d..313f3144406e 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -780,7 +780,7 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
>                         mapping_gfp_mask(mapping));
>  }
>
> -#define swapcache_index(folio) __page_file_index(&(folio)->page)
> +extern pgoff_t __folio_swap_cache_index(struct folio *folio);
>
>  /**
>   * folio_index - File index of a folio.
> @@ -795,9 +795,9 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
>   */
>  static inline pgoff_t folio_index(struct folio *folio)
>  {
> -        if (unlikely(folio_test_swapcache(folio)))
> -                return swapcache_index(folio);
> -        return folio->index;
> +       if (unlikely(folio_test_swapcache(folio)))
> +               return __folio_swap_cache_index(folio);
> +       return folio->index;
>  }
>
>  /**
> @@ -920,11 +920,6 @@ static inline loff_t page_offset(struct page *page)
>         return ((loff_t)page->index) << PAGE_SHIFT;
>  }
>
> -static inline loff_t page_file_offset(struct page *page)
> -{
> -       return ((loff_t)page_index(page)) << PAGE_SHIFT;
> -}
> -
>  /**
>   * folio_pos - Returns the byte position of this folio in its file.
>   * @folio: The folio.
> @@ -934,6 +929,8 @@ static inline loff_t folio_pos(struct folio *folio)
>         return page_offset(&folio->page);
>  }
>
> +extern loff_t __folio_swap_file_pos(struct folio *folio);
> +
>  /**
>   * folio_file_pos - Returns the byte position of this folio in its file.
>   * @folio: The folio.
> @@ -943,7 +940,9 @@ static inline loff_t folio_pos(struct folio *folio)
>   */
>  static inline loff_t folio_file_pos(struct folio *folio)
>  {
> -       return page_file_offset(&folio->page);
> +       if (unlikely(folio_test_swapcache(folio)))
> +               return __folio_swap_file_pos(folio);
> +       return ((loff_t)folio->index << PAGE_SHIFT);
>  }
>
>  /*
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 4919423cce76..0c36a5c2400f 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3419,12 +3419,17 @@ struct address_space *swapcache_mapping(struct folio *folio)
>  }
>  EXPORT_SYMBOL_GPL(swapcache_mapping);
>
> -pgoff_t __page_file_index(struct page *page)
> +pgoff_t __folio_swap_cache_index(struct folio *folio)
>  {
> -       swp_entry_t swap = page_swap_entry(page);
> -       return swp_offset(swap);
> +       return swp_offset(folio->swap);
>  }
> -EXPORT_SYMBOL_GPL(__page_file_index);
> +EXPORT_SYMBOL_GPL(__folio_swap_cache_index);
> +
> +loff_t __folio_swap_file_pos(struct folio *folio)
> +{
> +       return swap_file_pos(folio->swap);
> +}
> +EXPORT_SYMBOL_GPL(__folio_swap_file_pos);
>
>  /*
>   * add_swap_count_continuation - called when a swap count is duplicated
> --
> 2.44.0
>

Thanks
Barry

Kairui Song April 18, 2024, 2:42 a.m. UTC | #2

On Thu, Apr 18, 2024 at 9:55 AM Barry Song <[e-mail]> wrote:
>
> On Thu, Apr 18, 2024 at 4:12 AM Kairui Song <[e-mail]> wrote:
> >
> > From: Kairui Song <[e-mail]>
> >
> > 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 <[e-mail]>
>
> 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.

Matthew Wilcox (Oracle) April 18, 2024, 3:30 a.m. UTC | #3

On Thu, Apr 18, 2024 at 01:55:28PM +1200, Barry Song wrote:
> 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?

Why do you find it odd?  What would you propose instead?

Barry Song April 18, 2024, 3:55 a.m. UTC | #4

On Thu, Apr 18, 2024 at 3:31 PM Matthew Wilcox <[e-mail]> wrote:
>
> On Thu, Apr 18, 2024 at 01:55:28PM +1200, Barry Song wrote:
> > 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?
>
> Why do you find it odd?  What would you propose instead?

i'd prefer something like folio_swap_page(folio, entry);

Barry Song April 18, 2024, 10:19 a.m. UTC | #5

On Thu, Apr 18, 2024 at 2:42 PM Kairui Song <[e-mail]> wrote:
>
> On Thu, Apr 18, 2024 at 9:55 AM Barry Song <[e-mail]> wrote:
> >
> > On Thu, Apr 18, 2024 at 4:12 AM Kairui Song <[e-mail]> wrote:
> > >
> > > From: Kairui Song <[e-mail]>
> > >
> > > 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 <[e-mail]>
> >
> > 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/[e-mail]/

Patch

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 0436b919f1c7..797480e76c9c 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2245,19 +2245,6 @@  static inline void *folio_address(const struct folio *folio)
 	return page_address(&folio->page);
 }
 
-extern pgoff_t __page_file_index(struct page *page);
-
-/*
- * Return the pagecache index of the passed page.  Regular pagecache pages
- * use ->index whereas swapcache pages use swp_offset(->private)
- */
-static inline pgoff_t page_index(struct page *page)
-{
-	if (unlikely(PageSwapCache(page)))
-		return __page_file_index(page);
-	return page->index;
-}
-
 /*
  * Return true only if the page has been allocated with
  * ALLOC_NO_WATERMARKS and the low watermark was not
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 2df35e65557d..313f3144406e 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -780,7 +780,7 @@  static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
 			mapping_gfp_mask(mapping));
 }
 
-#define swapcache_index(folio)	__page_file_index(&(folio)->page)
+extern pgoff_t __folio_swap_cache_index(struct folio *folio);
 
 /**
  * folio_index - File index of a folio.
@@ -795,9 +795,9 @@  static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
  */
 static inline pgoff_t folio_index(struct folio *folio)
 {
-        if (unlikely(folio_test_swapcache(folio)))
-                return swapcache_index(folio);
-        return folio->index;
+	if (unlikely(folio_test_swapcache(folio)))
+		return __folio_swap_cache_index(folio);
+	return folio->index;
 }
 
 /**
@@ -920,11 +920,6 @@  static inline loff_t page_offset(struct page *page)
 	return ((loff_t)page->index) << PAGE_SHIFT;
 }
 
-static inline loff_t page_file_offset(struct page *page)
-{
-	return ((loff_t)page_index(page)) << PAGE_SHIFT;
-}
-
 /**
  * folio_pos - Returns the byte position of this folio in its file.
  * @folio: The folio.
@@ -934,6 +929,8 @@  static inline loff_t folio_pos(struct folio *folio)
 	return page_offset(&folio->page);
 }
 
+extern loff_t __folio_swap_file_pos(struct folio *folio);
+
 /**
  * folio_file_pos - Returns the byte position of this folio in its file.
  * @folio: The folio.
@@ -943,7 +940,9 @@  static inline loff_t folio_pos(struct folio *folio)
  */
 static inline loff_t folio_file_pos(struct folio *folio)
 {
-	return page_file_offset(&folio->page);
+	if (unlikely(folio_test_swapcache(folio)))
+		return __folio_swap_file_pos(folio);
+	return ((loff_t)folio->index << PAGE_SHIFT);
 }
 
 /*
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 4919423cce76..0c36a5c2400f 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3419,12 +3419,17 @@  struct address_space *swapcache_mapping(struct folio *folio)
 }
 EXPORT_SYMBOL_GPL(swapcache_mapping);
 
-pgoff_t __page_file_index(struct page *page)
+pgoff_t __folio_swap_cache_index(struct folio *folio)
 {
-	swp_entry_t swap = page_swap_entry(page);
-	return swp_offset(swap);
+	return swp_offset(folio->swap);
 }
-EXPORT_SYMBOL_GPL(__page_file_index);
+EXPORT_SYMBOL_GPL(__folio_swap_cache_index);
+
+loff_t __folio_swap_file_pos(struct folio *folio)
+{
+	return swap_file_pos(folio->swap);
+}
+EXPORT_SYMBOL_GPL(__folio_swap_file_pos);
 
 /*
  * add_swap_count_continuation - called when a swap count is duplicated