Re: [PATCH 2/3] mm/swap: account synchronous swap reads as memory stall

[Kairui Song]

On Tue, Nov 3, 2026 at 12:57 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
>
> On Sat, Oct 31, 2026 at 04:00:22AM +0800, Kairui Song wrote:
> > On Fri, Oct 30, 2026 at 9:08 PM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
> > >
> > > Hm, I think the only option to avoid this would be to split the batch
> > > and flush more frequently, which I'm not sure is worth it. The batch
> > > size is only 32 folios...
> > >
> > > diff --git a/mm/page_io.c b/mm/page_io.c
> > > index 22e5ac1d298f..806cfe633a95 100644
> > > --- a/mm/page_io.c
> > > +++ b/mm/page_io.c
> > > @@ -207,6 +207,8 @@ static bool swap_can_merge(struct swap_io_ctx *ctx, struct folio *folio,
> > >
> > >         if (ctx->sis != sis)
> > >                 return false;
> > > +       if (rw == READ && ctx->workingset != folio_test_workingset(folio))
> > > +               return false;
> > >         return sis->ops->can_merge(folio, prev_folio, prev_folio_size, rw);
> > >  }
> >
> > We are in a dilemma with this change: if it fires frequently it breaks
> > batching but improves PSI accuracy.  If it doesn't fire frequently, it
> > has no negative effect on batching and performance, and it isn't helping
> > with PSI accuracy either :/
> >
> > But after a second thought, maybe we can live with it by having both this
> > posted patch merged as it is and appending your suggested fix: the two
> > kinds of devices end up differently.
> >
> > For sync devices the batch is now always one folio, so there is nothing
> > fancy, everything is accurate.  Readahead batching for them only happens
> > on the MADV and swapoff walks, so the cost is limited.
> >
> > For async, the window covers submission.  That cost grows only mildly
> > with the folios in the bio, block layer and dispatch work, unlike a
> > synchronous read.  And the previous per folio accounting was not
> > exact either: in a single threaded task, the throttle or cost of
> > a non-workingset readahead folio pays is also holding up the workingset
> > folio's refault, since the task cannot start the next submission until
> > that one returns.  Same story for delayacct.
>
> That sounds reasonable to me. Zero benefit to batching on these
> devices, so no point in taking the fuzz factor that comes with it.
>
> > And we are missing the completion wait either way: if a fault finds the
> > folio in the swap cache with its read still in flight, it blocks in
> > folio_lock_or_retry() in do_swap_page(), which no window covers.
>
> It's there, if you drill down to folio_wait_bit_common():
>
>         if (bit_nr == PG_locked &&
>             !folio_test_uptodate(folio) && folio_test_workingset(folio)) {
>                 delayacct_thrashing_start(&in_thrashing);
>                 psi_memstall_enter(&pflags);
>                 thrashing = true;
>         }

Thanks for the info! Good to know.

> > So perhaps we can live with it for now by having your suggested fix
> > on top of this posted patch? Some comments could be helpful here and
> > maybe some further improvement is needed.
>
> Yes, I think that plan works.
>
> Want me to send a proper patch with changelog on top of Josef's?

I've tested the following code on top of V1, and it worked well; it
seems there is no need to move some functions or am I missing anything?
How about I keep your authorship and just send V2 including this ? :)
Any other suggestions are welcomed!

diff --git a/include/linux/swap_ops.h b/include/linux/swap_ops.h
index 57ac6c703f68..4d9ddd754712 100644
--- a/include/linux/swap_ops.h
+++ b/include/linux/swap_ops.h
@@ -17,6 +17,17 @@ struct swap_iocb {
 struct swap_io_ctx {
     struct swap_iocb    *sio;
     struct swap_info_struct    *sis;
+
+    /*
+     * Pressure accounting window: opened by the addition of first
+     * folio in a batch, closed by submittion of the batch.
+     * Values below tracks per-task state, so the task that opened
+     * the batch must be the one to close it.
+     */
+    bool            workingset;
+    bool            in_delayacct;
+    bool            in_thrashing;
+    unsigned long        in_memstall;
 };

 /*
diff --git a/mm/page_io.c b/mm/page_io.c
index 83ed0246ebfc..7d45e5acb951 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -470,31 +470,45 @@ void swap_read_folio(struct swap_io_ctx *ctx,
struct folio *folio)
     struct swap_info_struct *sis = __swap_entry_to_info(folio->swap);
     bool synchronous = sis->flags & SWP_SYNCHRONOUS_IO;
     bool workingset = folio_test_workingset(folio);
-    unsigned long pflags;
-    bool in_thrashing;

     VM_BUG_ON_FOLIO(!folio_test_swapcache(folio) && !synchronous, folio);
     VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
     VM_BUG_ON_FOLIO(folio_test_uptodate(folio), folio);

+    /*
+     * A batch this folio can't join has to be submitted before the
+     * pressure window, so that each batch is billed for its own read,
+     * especially before the possible lookup and decompression below
+     * rather than making the batch wait for them.
+     */
+    if (ctx->sio && !swap_can_merge(ctx, folio, READ))
+        swap_read_submit(ctx);
+
     /*
      * Count submission time as memory stall and delay. When the device
      * is congested, or the submitting cgroup IO-throttled, submission
-     * can be a significant part of overall IO time.
+     * can be a significant part of overall IO time. Reads are batched and
+     * submitted later, so both windows stay open on @ctx, and are closed
+     * by swap_read_submit() after the submission, also when the batch
+     * ends up with no folio queued.
      */
-    if (workingset) {
-        delayacct_thrashing_start(&in_thrashing);
-        psi_memstall_enter(&pflags);
+    if (!ctx->in_delayacct) {
+        delayacct_swapin_start();
+        ctx->in_delayacct = true;
+    }
+    if (workingset && !ctx->workingset) {
+        delayacct_thrashing_start(&ctx->in_thrashing);
+        psi_memstall_enter(&ctx->in_memstall);
+        ctx->workingset = true;
     }
-    delayacct_swapin_start();

     if (swap_read_folio_zeromap(folio)) {
         folio_unlock(folio);
-        goto finish;
+        return;
     }

     if (zswap_load(folio) != -ENOENT)
-        goto finish;
+        return;

     /* We have to read from slower devices. Increase zswap protection. */
     zswap_folio_swapin(folio);
@@ -506,13 +520,6 @@ void swap_read_folio(struct swap_io_ctx *ctx,
struct folio *folio)
      */
     if (synchronous)
         swap_read_submit(ctx);
-
-finish:
-    if (workingset) {
-        delayacct_thrashing_end(&in_thrashing);
-        psi_memstall_leave(&pflags);
-    }
-    delayacct_swapin_end();
 }

 static void swap_write_end(struct swap_iocb *sio, bool failed)
@@ -725,10 +732,19 @@ void swap_write_submit(struct swap_io_ctx *ctx)

 void swap_read_submit(struct swap_io_ctx *ctx)
 {
-    if (!ctx->sio)
-        return;
-    count_vm_events(NRSWPIN, 1);
-    ctx->sis->ops->submit_read(ctx);
-    ctx->sio = NULL;
-    ctx->sis = NULL;
+    if (ctx->sio) {
+        count_vm_events(NRSWPIN, 1);
+        ctx->sis->ops->submit_read(ctx);
+        ctx->sio = NULL;
+        ctx->sis = NULL;
+    }
+    if (ctx->workingset) {
+        delayacct_thrashing_end(&ctx->in_thrashing);
+        psi_memstall_leave(&ctx->in_memstall);
+        ctx->workingset = false;
+    }
+    if (ctx->in_delayacct) {
+        delayacct_swapin_end();
+        ctx->in_delayacct = false;
+    }
 }