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

[Johannes Weiner]

On Tue, Nov 03, 2026 at 01:26:35AM +0800, Kairui Song wrote:
> 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?

I did that to keep the write side in the same sequence, and remove the
then fully redundant submission in swap_add_folio(). Seems a bit
easier on readers that way.

> How about I keep your authorship and just send V2 including this ? :)
> Any other suggestions are welcomed!

Works for me, thanks!