Re: [PATCH v2 11/15] mm, swap: use the swap table for the swap cache and switch API

[Kairui Song]

On Wed, Sep 10, 2025 at 10:53 AM SeongJae Park <sj@xxxxxxxxxx> wrote:
>
> Hi Kairui,

Hi SeongJea,

>
> On Sat,  6 Sep 2025 03:13:53 +0800 Kairui Song <ryncsn@xxxxxxxxx> wrote:
>
> > From: Kairui Song <kasong@xxxxxxxxxxx>
> >
> > Introduce basic swap table infrastructures, which are now just a
> > fixed-sized flat array inside each swap cluster, with access wrappers.
> >
> > Each cluster contains a swap table of 512 entries. Each table entry is
> > an opaque atomic long. It could be in 3 types: a shadow type (XA_VALUE),
> > a folio type (pointer), or NULL.
> >
> > In this first step, it only supports storing a folio or shadow, and it
> > is a drop-in replacement for the current swap cache. Convert all swap
> > cache users to use the new sets of APIs. Chris Li has been suggesting
> > using a new infrastructure for swap cache for better performance, and
> > that idea combined well with the swap table as the new backing
> > structure. Now the lock contention range is reduced to 2M clusters,
> > which is much smaller than the 64M address_space. And we can also drop
> > the multiple address_space design.
> >
> > All the internal works are done with swap_cache_get_* helpers. Swap
> > cache lookup is still lock-less like before, and the helper's contexts
> > are same with original swap cache helpers. They still require a pin
> > on the swap device to prevent the backing data from being freed.
> >
> > Swap cache updates are now protected by the swap cluster lock
> > instead of the Xarray lock. This is mostly handled internally, but new
> > __swap_cache_* helpers require the caller to lock the cluster. So, a
> > few new cluster access and locking helpers are also introduced.
> >
> > A fully cluster-based unified swap table can be implemented on top
> > of this to take care of all count tracking and synchronization work,
> > with dynamic allocation. It should reduce the memory usage while
> > making the performance even better.
>
> Thank you for continuing this nice work.  I was unfortunately unable to get
> time to review this thoroughly, but found below.
>
> >
> > Co-developed-by: Chris Li <chrisl@xxxxxxxxxx>
> > Signed-off-by: Chris Li <chrisl@xxxxxxxxxx>
> > Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> > ---
> [...]
> > --- a/mm/swap.h
> > +++ b/mm/swap.h
> [...]
> > @@ -367,7 +452,7 @@ static inline int non_swapcache_batch(swp_entry_t entry, int max_nr)
> >  static inline pgoff_t folio_index(struct folio *folio)
> >  {
> >       if (unlikely(folio_test_swapcache(folio)))
> > -             return swap_cache_index(folio->swap);
> > +             return swp_offset(folio->swap);
> >       return folio->index;
> >  }
>
> This makes i386 build on my setup fails, like below:
>
>     In file included from /mm/shmem.c:44:
>     /mm/swap.h: In function ‘folio_index’:
>     /mm/swap.h:462:24: error: implicit declaration of function ‘swp_offset’; did you mean ‘pmd_offset’? [-Werror=implicit-function-declaration]
>       462 |                 return swp_offset(folio->swap);
>           |                        ^~~~~~~~~~
>           |                        pmd_offset
>     In file included from /mm/shmem.c:69:
>     /include/linux/swapops.h: At top level:
>     /include/linux/swapops.h:107:23: error: conflicting types for ‘swp_offset’; have ‘long unsigned int(swp_entry_t)’
>       107 | static inline pgoff_t swp_offset(swp_entry_t entry)
>           |                       ^~~~~~~~~~
>     /mm/swap.h:462:24: note: previous implicit declaration of ‘swp_offset’ with type ‘int()’
>       462 |                 return swp_offset(folio->swap);
>           |                        ^~~~~~~~~~
>     cc1: some warnings being treated as errors
>
> You may be able to reproduce this using my script [1].
>
> I also found including swapops.h as below fix this on my setup.  I didn't read
> this code thoroughly, so not really sure if it is the right approach, though.
>
>     --- a/mm/swap.h
>     +++ b/mm/swap.h
>     @@ -3,6 +3,7 @@
>      #define _MM_SWAP_H
>
>      #include <linux/atomic.h> /* for atomic_long_t */
>     +#include <linux/swapops.h>
>      struct mempolicy;
>      struct swap_iocb;
>
> [1] https://github.com/damonitor/damon-tests/blob/master/corr/tests/build_i386.sh

Yes, I also saw a report from the build bot. I tested !SWAP build but
didn't test !SWAP !SHMEM build.

Adjust the include header then the problem is gone, I'll send a V3 soon to
include this fix.

Thanks!