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

[Kairui Song <ryncsn@gmail.com>]

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.