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

[Chris Li <chrisl@kernel.org>]

On Mon, Nov 17, 2025 at 2:04 AM Kairui Song <ryncsn@gmail.com> wrote:
>
> On Mon, Nov 17, 2025 at 4:27 PM Barry Song <21cnbao@gmail.com> wrote:
> >
> > On Mon, Nov 17, 2025 at 2:12 AM Kairui Song <ryncsn@gmail.com> wrote:
> > >
> > > From: Kairui Song <kasong@tencent.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@tencent.com>
> >
> > Overall,
> > Reviewed-by: Barry Song <baohua@kernel.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@kernel.org>

Chris