linux-kernel - Re: [PATCH v2 5/6] mm: move folio_index to mm/swap.h and remove no longer needed helper

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]
Message-ID: <CAMgjq7AkaA+q8tNXurv4w2dNN5q6iLaGZ_7tfYRWSr5Oqrd3uQ@mail.gmail.com>
Date: Tue, 29 Apr 2025 21:35:10 +0800
From: Kairui Song <ryncsn@...il.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>, 
	Matthew Wilcox <willy@...radead.org>, Hugh Dickins <hughd@...gle.com>, Chris Li <chrisl@...nel.org>, 
	Yosry Ahmed <yosryahmed@...gle.com>, "Huang, Ying" <ying.huang@...ux.alibaba.com>, 
	Nhat Pham <nphamcs@...il.com>, Johannes Weiner <hannes@...xchg.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/6] mm: move folio_index to mm/swap.h and remove no
 longer needed helper

On Tue, Apr 29, 2025 at 8:24 PM David Hildenbrand <david@...hat.com> wrote:
>
> On 29.04.25 13:49, Kairui Song wrote:
> > From: Kairui Song <kasong@...cent.com>
> >
> > There are no remaining users of folio_index() outside the mm subsystem.
> > Move it to mm/swap.h to co-locate it with swap_cache_index(), eliminating
> > a forward declaration, and a function call overhead.
> >
> > Also remove the helper that was used to fix circular header dependency
> > issue.
> >
> > Signed-off-by: Kairui Song <kasong@...cent.com>
> > ---
> >   include/linux/pagemap.h | 20 --------------------
> >   mm/memfd.c              |  1 +
> >   mm/migrate.c            |  1 +
> >   mm/swap.h               | 23 +++++++++++++++++++++++
> >   mm/swapfile.c           |  6 ------
> >   5 files changed, 25 insertions(+), 26 deletions(-)
> >
> > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> > index 1dc3416a9c0d..47b5746d5a65 100644
> > --- a/include/linux/pagemap.h
> > +++ b/include/linux/pagemap.h
> > @@ -884,26 +884,6 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
> >                       mapping_gfp_mask(mapping));
> >   }
> >
> > -extern pgoff_t __folio_swap_cache_index(struct folio *folio);
> > -
> > -/**
> > - * folio_index - File index of a folio.
> > - * @folio: The folio.
> > - *
> > - * For a folio which is either in the page cache or the swap cache,
> > - * return its index within the address_space it belongs to.  If you know
> > - * the page is definitely in the page cache, you can look at the folio's
> > - * index directly.
> > - *
> > - * Return: The index (offset in units of pages) of a folio in its file.
> > - */
> > -static inline pgoff_t folio_index(struct folio *folio)
> > -{
> > -     if (unlikely(folio_test_swapcache(folio)))
> > -             return __folio_swap_cache_index(folio);
> > -     return folio->index;
> > -}
> > -
> >   /**
> >    * folio_next_index - Get the index of the next folio.
> >    * @folio: The current folio.
> > diff --git a/mm/memfd.c b/mm/memfd.c
> > index c64df1343059..ab367e61553d 100644
> > --- a/mm/memfd.c
> > +++ b/mm/memfd.c
> > @@ -20,6 +20,7 @@
> >   #include <linux/memfd.h>
> >   #include <linux/pid_namespace.h>
> >   #include <uapi/linux/memfd.h>
> > +#include "swap.h"
> >
> >   /*
> >    * We need a tag: a new tag would expand every xa_node by 8 bytes,
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index f3ee6d8d5e2e..662e5dc44b33 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -50,6 +50,7 @@
> >   #include <trace/events/migrate.h>
> >
> >   #include "internal.h"
> > +#include "swap.h"
> >
> >   bool isolate_movable_page(struct page *page, isolate_mode_t mode)
> >   {
> > diff --git a/mm/swap.h b/mm/swap.h
> > index 6f4a3f927edb..91439d0ce50b 100644
> > --- a/mm/swap.h
> > +++ b/mm/swap.h
> > @@ -50,6 +50,24 @@ static inline pgoff_t swap_cache_index(swp_entry_t entry)
> >       return swp_offset(entry) & SWAP_ADDRESS_SPACE_MASK;
> >   }
> >
> > +/**
> > + * folio_index - File index of a folio.
> > + * @folio: The folio.
> > + *
> > + * For a folio which is either in the page cache or the swap cache,
> > + * return its index within the address_space it belongs to.  If you know
> > + * the folio is definitely in the page cache, you can look at the folio's
> > + * index directly.
> > + *
> > + * Return: The index (offset in units of pages) of a folio in its file.
> > + */
> > +static inline pgoff_t folio_index(struct folio *folio)
> > +{
> > +     if (unlikely(folio_test_swapcache(folio)))
> > +             return swap_cache_index(folio->swap);
> > +     return folio->index;
> > +}
> > +
> >   void show_swap_cache_info(void);
> >   void *get_shadow_from_swap_cache(swp_entry_t entry);
> >   int add_to_swap_cache(struct folio *folio, swp_entry_t entry,
> > @@ -125,6 +143,11 @@ static inline pgoff_t swap_cache_index(swp_entry_t entry)
> >       return 0;
> >   }
> >
> > +static inline pgoff_t folio_index(struct folio *folio)
> > +{
> > +     return folio->index;
> > +}
> > +
>
> Could we just have a single generic function that does something like:
>
> static inline pgoff_t folio_index(struct folio *folio)
> {
>         if (unlikely(IS_ENABLED(CONFIG_SWAP) &&
>                      folio_test_swapcache(folio)))
>                 return swap_cache_index(folio->swap);
>         return folio->index;
> }
>

Sure, this is doable. I wanted to put the file in swap.h so it can use
swap_cache_index without introducing another extra header dependency,
and put in right next to swap_cache_index, I thought this way looks
prettier :)

I can move it to the tail of this file (out of ifdef CONFIG_SWAP) and
use one single generic function.

>
> --
> Cheers,
>
> David / dhildenb
>
>