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

[SeongJae Park <sj@kernel.org>]

Hi Kairui,

On Sat,  6 Sep 2025 03:13:53 +0800 Kairui Song <ryncsn@gmail.com> wrote:

> From: Kairui Song <kasong@tencent.com>
> 
> 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@kernel.org>
> Signed-off-by: Chris Li <chrisl@kernel.org>
> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
[...]
> --- 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


Thanks,
SJ

[...]