linux-kernel - Re: [PATCH v2 02/19] mm, swap: split swap cache preparation loop into a standalone helper

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CACePvbXbx2bT3K4KZMAh3yLuC1p0j80DS3XJfPHChVkqNJkLmA@mail.gmail.com>
Date: Sat, 22 Nov 2025 00:50:35 -0800
From: Chris Li <chrisl@...nel.org>
To: Kairui Song <ryncsn@...il.com>
Cc: Barry Song <21cnbao@...il.com>, linux-mm@...ck.org, 
	Andrew Morton <akpm@...ux-foundation.org>, Baoquan He <bhe@...hat.com>, 
	Nhat Pham <nphamcs@...il.com>, Yosry Ahmed <yosry.ahmed@...ux.dev>, 
	David Hildenbrand <david@...nel.org>, Johannes Weiner <hannes@...xchg.org>, 
	Youngjun Park <youngjun.park@....com>, Hugh Dickins <hughd@...gle.com>, 
	Baolin Wang <baolin.wang@...ux.alibaba.com>, Ying Huang <ying.huang@...ux.alibaba.com>, 
	Kemeng Shi <shikemeng@...weicloud.com>, Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, 
	"Matthew Wilcox (Oracle)" <willy@...radead.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 02/19] mm, swap: split swap cache preparation loop into
 a standalone helper

On Mon, Nov 17, 2025 at 2:04 AM Kairui Song <ryncsn@...il.com> wrote:
>
> On Mon, Nov 17, 2025 at 4:27 PM Barry Song <21cnbao@...il.com> wrote:
> >
> > On Mon, Nov 17, 2025 at 2:12 AM Kairui Song <ryncsn@...il.com> wrote:
> > >
> > > From: Kairui Song <kasong@...cent.com>
> > >
> > > To prepare for the removal of swap cache bypass swapin, introduce a new
> > > helper that accepts an allocated and charged fresh folio, prepares the
> > > folio, the swap map, and then adds the folio to the swap cache.
> > >
> > > This doesn't change how swap cache works yet, we are still depending on
> > > the SWAP_HAS_CACHE in the swap map for synchronization. But all
> > > synchronization hacks are now all in this single helper.
> > >
> > > No feature change.
> > >
> > > Signed-off-by: Kairui Song <kasong@...cent.com>
> >
> > Overall,
> > Reviewed-by: Barry Song <baohua@...nel.org>
> > with a few minor nits.
> >
> > > ---
> > >  mm/swap_state.c | 197 +++++++++++++++++++++++++++++++-------------------------
> > >  1 file changed, 109 insertions(+), 88 deletions(-)
> > >
> > > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > > index 08252eaef32f..7b93704fcbe7 100644
> > > --- a/mm/swap_state.c
> > > +++ b/mm/swap_state.c
> > > @@ -402,6 +402,97 @@ void swap_update_readahead(struct folio *folio, struct vm_area_struct *vma,
> > >         }
> > >  }
> > >
> > > +/**
> > > + * __swap_cache_prepare_and_add - Prepare the folio and add it to swap cache.
> > > + * @entry: swap entry to be bound to the folio.
> > > + * @folio: folio to be added.
> > > + * @gfp: memory allocation flags for charge, can be 0 if @charged if true.
> > > + * @charged: if the folio is already charged.
> > > + * @skip_if_exists: if the slot is in a cached state, return NULL.
> > > + *                  This is an old workaround that will be removed shortly.
> > > + *
> > > + * Update the swap_map and add folio as swap cache, typically before swapin.
> > > + * All swap slots covered by the folio must have a non-zero swap count.
> > > + *
> > > + * Context: Caller must protect the swap device with reference count or locks.
> > > + * Return: Returns the folio being added on success. Returns the existing
> > > + * folio if @entry is cached. Returns NULL if raced with swapin or swapoff.
> >
> > Not a native speaker, but I wonder if adding *already* would make the
> > comment clearer, for example: "Returns the existing folio if @entry is
> > already cached."
>
> Agree, that sounds better, thanks!
>
> >
> > BTW, returning NULL may not be the best option. Returning an ERR_PTR
> > seems more specific?
>
> I also don't like the idea of returning NULL, but all existing callers
> are all using this convention already. To not introduce too much
> noise, this maybe better be cleaned up / changed after this series I
> think.

There is only one possible error condition to return NULL there, it is
fine to stay as NULL. The caller only cares about whether it has an
error or not rather than which error anyway. Not a big deal.

Acked-by: Chris Li <chrisl@...nel.org>

Chris