Re: [PATCH v3 6/7] mm, swap: remove swap slot cache

Re: [PATCH v3 6/7] mm, swap: remove swap slot cache

[Heiko Carstens]

Hi Kairui,

On Fri, Mar 14, 2025 at 12:59:34AM +0800, Kairui Song wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> Slot cache is no longer needed now, removing it and all related code.
...
> Signed-off-by: Kairui Song <kasong@tencent.com>
> Reviewed-by: Baoquan He <bhe@redhat.com>
> ---
>  include/linux/swap.h       |   3 -
>  include/linux/swap_slots.h |  28 ----
>  mm/Makefile                |   2 +-
>  mm/swap_slots.c            | 295 -------------------------------------
>  mm/swap_state.c            |   8 +-
>  mm/swapfile.c              | 194 ++++++++----------------
>  6 files changed, 67 insertions(+), 463 deletions(-)
>  delete mode 100644 include/linux/swap_slots.h
>  delete mode 100644 mm/swap_slots.c
...
> diff --git a/mm/swapfile.c b/mm/swapfile.c
...
> +swp_entry_t folio_alloc_swap(struct folio *folio)
>  {
> -	int order = swap_entry_order(entry_order);
> -	unsigned long size = 1 << order;
> +	unsigned int order = folio_order(folio);
> +	unsigned int size = 1 << order;
>  	struct swap_info_struct *si, *next;
> -	int n_ret = 0;
> +	swp_entry_t entry = {};
> +	unsigned long offset;
>  	int node;
>  
> +	if (order) {
> +		/*
> +		 * Should not even be attempting large allocations when huge
> +		 * page swap is disabled. Warn and fail the allocation.
> +		 */
> +		if (!IS_ENABLED(CONFIG_THP_SWAP) || size > SWAPFILE_CLUSTER) {
> +			VM_WARN_ON_ONCE(1);
> +			return entry;
> +		}
> +	}

This warning triggers on s390. CONFIG_THP_SWAP is disabled and order
is 8 when this triggers (reproduced with ltp's swapon01 test case):

------------[ cut here ]------------
WARNING: CPU: 1 PID: 895 at mm/swapfile.c:1227 folio_alloc_swap+0x438/0x440
Modules linked in:
CPU: 1 UID: 0 PID: 895 Comm: swapon01 Not tainted 6.14.0-rc6-00227-g0ff67f990bd4-dirty #25
Hardware name: IBM 3931 A01 704 (z/VM 7.4.0)
Krnl PSW : 0704d00180000000 000003ffe051210c (folio_alloc_swap+0x43c/0x440)
           R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:1 PM:0 RI:0 EA:3
Krnl GPRS: 0000000080000000 0000000000000001 0000000000000013 0000000000070000
           0000000000000006 fffffef40e9da000 0000000000000000 0000037202fc4000
           0000037f00000100 0000000000000100 0000037fe2e4b770 0000037202fc4000
           0000000000000000 0000000000000000 000003ffe0512108 0000037fe2e4b3c8
Krnl Code: 000003ffe05120fe: b9160044            llgfr   %r4,%r4
           000003ffe0512102: c0e5ffdf8c0b        brasl   %r14,000003ffe0103918
          #000003ffe0512108: af000000            mc      0,0
          >000003ffe051210c: a7f4fe94            brc     15,000003ffe0511e34
           000003ffe0512110: c0040069ce74        brcl    0,000003ffe124bdf8
           000003ffe0512116: eb8ff0580024        stmg    %r8,%r15,88(%r15)
           000003ffe051211c: b90400ef            lgr     %r14,%r15
           000003ffe0512120: e3f0ffb8ff71        lay     %r15,-72(%r15)
Call Trace:
 [<000003ffe051210c>] folio_alloc_swap+0x43c/0x440 
 [<000003ffe050afa6>] add_to_swap+0x56/0xf0 
 [<000003ffe045fdc0>] shrink_folio_list+0xe80/0x13b0 
 [<000003ffe0461946>] shrink_inactive_list+0x1a6/0x550 
 [<000003ffe04624a2>] shrink_lruvec+0x2b2/0x410 
 [<000003ffe0462840>] shrink_node_memcgs+0x240/0x2d0 
 [<000003ffe0462986>] shrink_node+0xb6/0x3e0 
 [<000003ffe046302a>] do_try_to_free_pages+0xda/0x610 
 [<000003ffe0464d2c>] try_to_free_mem_cgroup_pages+0x14c/0x2a0 
 [<000003ffe0568270>] try_charge_memcg+0x220/0x5d0 
 [<000003ffe056867a>] charge_memcg+0x5a/0x270 
 [<000003ffe056a484>] __mem_cgroup_charge+0x44/0x80 
 [<000003ffe04acf20>] alloc_anon_folio+0x280/0x610 
 [<000003ffe04ad45a>] do_anonymous_page+0x1aa/0x5e0 
 [<000003ffe04af4c4>] __handle_mm_fault+0x244/0x500 
 [<000003ffe04af820>] handle_mm_fault+0xa0/0x170 
 [<000003ffe01533f8>] do_exception+0x1d8/0x4a0 
 [<000003ffe11fb92a>] __do_pgm_check+0x13a/0x220 
 [<000003ffe120c3ce>] pgm_check_handler+0x11e/0x170 
---[ end trace 0000000000000000 ]---

Re: [PATCH v3 6/7] mm, swap: remove swap slot cache

[Kairui Song]

On Mon, Apr 28, 2025 at 9:53 PM Heiko Carstens <hca@linux.ibm.com> wrote:
>
> Hi Kairui,
>
> On Fri, Mar 14, 2025 at 12:59:34AM +0800, Kairui Song wrote:
> > From: Kairui Song <kasong@tencent.com>
> >
> > Slot cache is no longer needed now, removing it and all related code.
> ...
> > Signed-off-by: Kairui Song <kasong@tencent.com>
> > Reviewed-by: Baoquan He <bhe@redhat.com>
> > ---
> >  include/linux/swap.h       |   3 -
> >  include/linux/swap_slots.h |  28 ----
> >  mm/Makefile                |   2 +-
> >  mm/swap_slots.c            | 295 -------------------------------------
> >  mm/swap_state.c            |   8 +-
> >  mm/swapfile.c              | 194 ++++++++----------------
> >  6 files changed, 67 insertions(+), 463 deletions(-)
> >  delete mode 100644 include/linux/swap_slots.h
> >  delete mode 100644 mm/swap_slots.c
> ...
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> ...
> > +swp_entry_t folio_alloc_swap(struct folio *folio)
> >  {
> > -     int order = swap_entry_order(entry_order);
> > -     unsigned long size = 1 << order;
> > +     unsigned int order = folio_order(folio);
> > +     unsigned int size = 1 << order;
> >       struct swap_info_struct *si, *next;
> > -     int n_ret = 0;
> > +     swp_entry_t entry = {};
> > +     unsigned long offset;
> >       int node;
> >
> > +     if (order) {
> > +             /*
> > +              * Should not even be attempting large allocations when huge
> > +              * page swap is disabled. Warn and fail the allocation.
> > +              */
> > +             if (!IS_ENABLED(CONFIG_THP_SWAP) || size > SWAPFILE_CLUSTER) {
> > +                     VM_WARN_ON_ONCE(1);
> > +                     return entry;
> > +             }
> > +     }
>
> This warning triggers on s390. CONFIG_THP_SWAP is disabled and order
> is 8 when this triggers (reproduced with ltp's swapon01 test case):

Hi Heiko,

Thanks for the report.

>
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 895 at mm/swapfile.c:1227 folio_alloc_swap+0x438/0x440
> Modules linked in:
> CPU: 1 UID: 0 PID: 895 Comm: swapon01 Not tainted 6.14.0-rc6-00227-g0ff67f990bd4-dirty #25
> Hardware name: IBM 3931 A01 704 (z/VM 7.4.0)
> Krnl PSW : 0704d00180000000 000003ffe051210c (folio_alloc_swap+0x43c/0x440)
>            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:1 PM:0 RI:0 EA:3
> Krnl GPRS: 0000000080000000 0000000000000001 0000000000000013 0000000000070000
>            0000000000000006 fffffef40e9da000 0000000000000000 0000037202fc4000
>            0000037f00000100 0000000000000100 0000037fe2e4b770 0000037202fc4000
>            0000000000000000 0000000000000000 000003ffe0512108 0000037fe2e4b3c8
> Krnl Code: 000003ffe05120fe: b9160044            llgfr   %r4,%r4
>            000003ffe0512102: c0e5ffdf8c0b        brasl   %r14,000003ffe0103918
>           #000003ffe0512108: af000000            mc      0,0
>           >000003ffe051210c: a7f4fe94            brc     15,000003ffe0511e34
>            000003ffe0512110: c0040069ce74        brcl    0,000003ffe124bdf8
>            000003ffe0512116: eb8ff0580024        stmg    %r8,%r15,88(%r15)
>            000003ffe051211c: b90400ef            lgr     %r14,%r15
>            000003ffe0512120: e3f0ffb8ff71        lay     %r15,-72(%r15)
> Call Trace:
>  [<000003ffe051210c>] folio_alloc_swap+0x43c/0x440
>  [<000003ffe050afa6>] add_to_swap+0x56/0xf0
>  [<000003ffe045fdc0>] shrink_folio_list+0xe80/0x13b0
>  [<000003ffe0461946>] shrink_inactive_list+0x1a6/0x550
>  [<000003ffe04624a2>] shrink_lruvec+0x2b2/0x410
>  [<000003ffe0462840>] shrink_node_memcgs+0x240/0x2d0
>  [<000003ffe0462986>] shrink_node+0xb6/0x3e0
>  [<000003ffe046302a>] do_try_to_free_pages+0xda/0x610
>  [<000003ffe0464d2c>] try_to_free_mem_cgroup_pages+0x14c/0x2a0
>  [<000003ffe0568270>] try_charge_memcg+0x220/0x5d0
>  [<000003ffe056867a>] charge_memcg+0x5a/0x270
>  [<000003ffe056a484>] __mem_cgroup_charge+0x44/0x80
>  [<000003ffe04acf20>] alloc_anon_folio+0x280/0x610
>  [<000003ffe04ad45a>] do_anonymous_page+0x1aa/0x5e0
>  [<000003ffe04af4c4>] __handle_mm_fault+0x244/0x500
>  [<000003ffe04af820>] handle_mm_fault+0xa0/0x170
>  [<000003ffe01533f8>] do_exception+0x1d8/0x4a0
>  [<000003ffe11fb92a>] __do_pgm_check+0x13a/0x220
>  [<000003ffe120c3ce>] pgm_check_handler+0x11e/0x170
> ---[ end trace 0000000000000000 ]---
>

The !CONFIG_THP_SWAP check existed before because slot cache should
reject high order allocation. But slot cache is gone, so large
allocation will directly go to the allocator.

It was not a meaningful WARN in the first place, and now the allocator
should just fail silently for high order allocation, that's totally
fine and expected and will just inform the caller to split the folio.

I'll just change the WARN_ON condition to `if (order && size >
SWAPFILE_CLUSTER)` then, this should silence the WARN.

Re: [PATCH v3 6/7] mm, swap: remove swap slot cache

[Heiko Carstens]

On Mon, Apr 28, 2025 at 11:31:59PM +0800, Kairui Song wrote:
> On Mon, Apr 28, 2025 at 9:53 PM Heiko Carstens <hca@linux.ibm.com> wrote:
> > > +     if (order) {
> > > +             /*
> > > +              * Should not even be attempting large allocations when huge
> > > +              * page swap is disabled. Warn and fail the allocation.
> > > +              */
> > > +             if (!IS_ENABLED(CONFIG_THP_SWAP) || size > SWAPFILE_CLUSTER) {
> > > +                     VM_WARN_ON_ONCE(1);
> > > +                     return entry;
> > > +             }
> > > +     }
> 
> The !CONFIG_THP_SWAP check existed before because slot cache should
> reject high order allocation. But slot cache is gone, so large
> allocation will directly go to the allocator.
> 
> It was not a meaningful WARN in the first place, and now the allocator
> should just fail silently for high order allocation, that's totally
> fine and expected and will just inform the caller to split the folio.
> 
> I'll just change the WARN_ON condition to `if (order && size >
> SWAPFILE_CLUSTER)` then, this should silence the WARN.

If I understand your suggestion correctly then this would be the
resulting code:

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 2eff8b51a945..5a7797143948 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1276,7 +1276,7 @@ int folio_alloc_swap(struct folio *folio, gfp_t gfp)
         * Should not even be attempting large allocations when huge
         * page swap is disabled. Warn and fail the allocation.
         */
-       if (order && (!IS_ENABLED(CONFIG_THP_SWAP) || size > SWAPFILE_CLUSTER)) {
+       if (order && size > SWAPFILE_CLUSTER) {
                VM_WARN_ON_ONCE(1);
                return -EINVAL;
        }

However, with that change I get this splat (and a few more) instead:

------------[ cut here ]------------
DEBUG_LOCKS_WARN_ON(l->owner != current)
WARNING: CPU: 4 PID: 934 at ./include/linux/local_lock_internal.h:52 folio_alloc_swap+0x22e/0x560
Modules linked in:
CPU: 4 UID: 0 PID: 934 Comm: swapon01 Not tainted 6.15.0-rc4-00021-gca91b9500108-dirty #2 PREEMPT 
Hardware name: IBM 3931 A01 704 (z/VM 7.4.0)
Krnl PSW : 0704f00180000000 000003ffe05231b2 (folio_alloc_swap+0x232/0x560)
           R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:3 PM:0 RI:0 EA:3
Krnl GPRS: 000003ff80000002 0000000000000003 0000000000000028 000003ffe2fe6c90
           0000000000000003 000003ff80000003 000003ffe19fc538 000003720322c000
           0000000000000000 000002f2ab38f000 000002f200000100 000003720322c000
           0000000000000000 000002f3f0eda278 000003ffe05231ae 0000037febbdb3f8
Krnl Code: 000003ffe05231a2: c020008e69bc        larl    %r2,000003ffe16f051a
           000003ffe05231a8: c0e5ffe2f4dc        brasl   %r14,000003ffe0181b60
          #000003ffe05231ae: af000000            mc      0,0
          >000003ffe05231b2: a7f4ff92            brc     15,000003ffe05230d6
           000003ffe05231b6: a7080001            lhi     %r0,1
           000003ffe05231ba: a7a80001            lhi     %r10,1
           000003ffe05231be: a7980000            lhi     %r9,0
           000003ffe05231c2: a7f4ff04            brc     15,000003ffe0522fca
Call Trace:
 [<000003ffe05231b2>] folio_alloc_swap+0x232/0x560 
 [<000003ffe046d2d2>] shrink_folio_list+0xe02/0x12d0 
 [<000003ffe046edc8>] shrink_inactive_list+0x188/0x5a0 
 [<000003ffe046f7d4>] shrink_lruvec+0x104/0x400 
 [<000003ffe046fd0c>] shrink_node_memcgs+0x23c/0x2c0 
 [<000003ffe046fe3a>] shrink_node+0xaa/0x420 
 [<000003ffe0470500>] do_try_to_free_pages+0xd0/0x5c0 
 [<000003ffe0472144>] try_to_free_mem_cgroup_pages+0x144/0x290 
 [<000003ffe057b33a>] try_charge_memcg+0x1ca/0x420 
 [<000003ffe057b5ea>] charge_memcg+0x5a/0x1a0 
 [<000003ffe057d5d4>] __mem_cgroup_charge+0x44/0x160 
 [<000003ffe04bb6ac>] alloc_anon_folio+0x27c/0x5d0 
 [<000003ffe04bbbac>] do_anonymous_page+0x1ac/0x7b0 
 [<000003ffe04bdec2>] __handle_mm_fault+0x212/0x4c0 
 [<000003ffe04be20c>] handle_mm_fault+0x9c/0x230 
 [<000003ffe015422c>] do_exception+0x1dc/0x540 
 [<000003ffe125c560>] __do_pgm_check+0x130/0x220 
 [<000003ffe126e3ae>] pgm_check_handler+0x11e/0x170 
INFO: lockdep is turned off.
Last Breaking-Event-Address:
 [<000003ffe0181c96>] __warn_printk+0x136/0x140
irq event stamp: 4120751
hardirqs last  enabled at (4120751): [<000003ffe126d0ee>] _raw_spin_unlock_irq+0x3e/0x80
hardirqs last disabled at (4120750): [<000003ffe126cca8>] _raw_spin_lock_irq+0x98/0xd0
softirqs last  enabled at (4104986): [<000003ffe018c1fc>] handle_softirqs+0x2fc/0x550
softirqs last disabled at (4104969): [<000003ffe018b9c6>] __irq_exit_rcu+0x126/0x140
---[ end trace 0000000000000000 ]---

Re: [PATCH v3 6/7] mm, swap: remove swap slot cache

[Kairui Song]

On Tue, Apr 29, 2025 at 3:31 PM Heiko Carstens <hca@linux.ibm.com> wrote:
>
> On Mon, Apr 28, 2025 at 11:31:59PM +0800, Kairui Song wrote:
> > On Mon, Apr 28, 2025 at 9:53 PM Heiko Carstens <hca@linux.ibm.com> wrote:
> > > > +     if (order) {
> > > > +             /*
> > > > +              * Should not even be attempting large allocations when huge
> > > > +              * page swap is disabled. Warn and fail the allocation.
> > > > +              */
> > > > +             if (!IS_ENABLED(CONFIG_THP_SWAP) || size > SWAPFILE_CLUSTER) {
> > > > +                     VM_WARN_ON_ONCE(1);
> > > > +                     return entry;
> > > > +             }
> > > > +     }
> >
> > The !CONFIG_THP_SWAP check existed before because slot cache should
> > reject high order allocation. But slot cache is gone, so large
> > allocation will directly go to the allocator.
> >
> > It was not a meaningful WARN in the first place, and now the allocator
> > should just fail silently for high order allocation, that's totally
> > fine and expected and will just inform the caller to split the folio.
> >
> > I'll just change the WARN_ON condition to `if (order && size >
> > SWAPFILE_CLUSTER)` then, this should silence the WARN.
>
> If I understand your suggestion correctly then this would be the
> resulting code:
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 2eff8b51a945..5a7797143948 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1276,7 +1276,7 @@ int folio_alloc_swap(struct folio *folio, gfp_t gfp)
>          * Should not even be attempting large allocations when huge
>          * page swap is disabled. Warn and fail the allocation.
>          */
> -       if (order && (!IS_ENABLED(CONFIG_THP_SWAP) || size > SWAPFILE_CLUSTER)) {
> +       if (order && size > SWAPFILE_CLUSTER) {
>                 VM_WARN_ON_ONCE(1);
>                 return -EINVAL;
>         }
>
> However, with that change I get this splat (and a few more) instead:

Sorry my bad, the allocator needs to fail silencely, not ignore and go
on. So it should be:

diff --git a/mm/swapfile.c b/mm/swapfile.c
index e727021b8e2c..b86637cfb17a 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1272,13 +1272,22 @@ int folio_alloc_swap(struct folio *folio, gfp_t gfp)
        VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
        VM_BUG_ON_FOLIO(!folio_test_uptodate(folio), folio);

-       /*
-        * Should not even be attempting large allocations when huge
-        * page swap is disabled. Warn and fail the allocation.
-        */
-       if (order && (!IS_ENABLED(CONFIG_THP_SWAP) || size >
SWAPFILE_CLUSTER)) {
-               VM_WARN_ON_ONCE(1);
-               return -EINVAL;
+       if (order) {
+               /*
+                * Reject large allocation when THP_SWAP is disabled,
+                * the caller should split the folio and try again.
+                */
+               if (!IS_ENABLED(CONFIG_THP_SWAP))
+                       return -EAGAIN;
+
+               /*
+                * Allocation size should never exceed cluster size
+                * (HPAGE_PMD_SIZE).
+                */
+               if (size > SWAPFILE_CLUSTER) {
+                       VM_WARN_ON_ONCE(1);
+                       return -EINVAL;
+               }
        }

        local_lock(&percpu_swap_cluster.lock);

---

I've tested locally and it seems to work well, I'll send a patch to fix it.