Message-ID: <CACePvbWg1hg1G0+unAJKz9SiwJOhahf8Gsoq+VB-5HDdAiRcqw@mail.gmail.com>
Date: Thu, 4 Sep 2025 11:50:57 -0700
From: Chris Li <chrisl@...nel.org>
To: Kairui Song <ryncsn@...il.com>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>, 
	Matthew Wilcox <willy@...radead.org>, Hugh Dickins <hughd@...gle.com>, Barry Song <baohua@...nel.org>, 
	Baoquan He <bhe@...hat.com>, Nhat Pham <nphamcs@...il.com>, 
	Kemeng Shi <shikemeng@...weicloud.com>, Baolin Wang <baolin.wang@...ux.alibaba.com>, 
	Ying Huang <ying.huang@...ux.alibaba.com>, Johannes Weiner <hannes@...xchg.org>, 
	David Hildenbrand <david@...hat.com>, Yosry Ahmed <yosryahmed@...gle.com>, 
	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, Zi Yan <ziy@...dia.com>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/9] mm, swap: introduce swap table as swap cache (phase I)

On Thu, Sep 4, 2025 at 9:36 AM Kairui Song <ryncsn@...il.com> wrote:
>
> On Sat, Aug 30, 2025 at 2:57 PM Chris Li <chrisl@...nel.org> wrote:
> >
> > Hi Kairui,
> >
> > I give one pass of review on your series already. I Ack a portion of
> > it. I expect some clarification or update on the rest.
> >
> > I especially want to double check the swap cache atomic set a range of
> > swap entries to folio.
> > I want to make sure this bug does not happen to swap table:
> > https://lore.kernel.org/linux-mm/5bee194c-9cd3-47e7-919b-9f352441f855@kernel.dk/
> >
> > I just double checked, the swap table should be fine in this regard.
> > The bug is triggered by memory allocation failure in the middle of
> > insert folio. Swap tables already populated the table when the swap
> > entry is allocated and handed out to the caller. We don't do memory
> > allocation when inserting folio into swap cache, which is a good
> > thing. We should not have that bug.
> >
> > I also want some extra pair of eyes on those subtle behavior change
> > patches, I expect you to split them out in the next version.
> > I will need to go through the split out subtle patch one more time as
> > well. This pass I only catch the behavior change, haven't got a chance
> > to reason those behavior changes patches are indeed fine. If you can
> > defer those split out patches, that will save me some time to reason
> > them on the next round. Your call.
>
> Thanks a lot for the review and raising concern about robustness of phase 1.
>
> I just added more atomic runtime checks and ran another few days of
> stress and performance tests. So far I don't think there is a race or
> bug in the code, as I have been testing the longer series for months.
> But with more checks, we are still a lot faster than before, and much
> less error prone. So it seems very reasonable and acceptable to have
> them as this is a quite important part, even for a long term.

Yes, that is what I want it to be. Every time an entry messes up in
the swap table, that is one page worth of corrupted data. I definitely
don't want the corrupted memory to propagate to another place, e.g.
write to the harddrive. That is much worse than having a BUG() and
stopping the machine there.

> That will surely help catch any potential new or historical issue.

Yes, let it rip in the mm-untable and hopefully we don't see any
trigger of that in the wild.

> V2 would have a few more patches splitted from old ones so it should
> be cleaner. The code should be basically still the same though. Some
> parts like the whole new infrastructure are really hard to split though
> as they are supposed to work as a whole.

That is great. Looking forward to it.

> > Oh, I also want to write a design document for the swap table idea. I
> > will send it your way to incorporate into your next version of the
> > series.
> >
> > Thanks for the great work! I am very excited about this.
>
> Later phases will also be exciting where we start to trim down the LOC
> and long existing issues, with net gains :)

Same feeling here. Also looking forward to it.

Chris

Powered by blists - more mailing lists