Re: [PATCH v3 02/12] mm, swap: move common swap cache operations into standalone helpers

[Kairui Song]

On Wed, May 6, 2026 at 10:46 PM Chris Li <chrisl@xxxxxxxxxx> wrote:
>
> On Tue, Apr 21, 2026 at 8:16 AM Kairui Song via B4 Relay
> <devnull+kasong.tencent.com@xxxxxxxxxx> wrote:
> >
> > From: Kairui Song <kasong@xxxxxxxxxxx>
> >
> > Move a few swap cache checking, adding, and deletion operations
> > into standalone helpers to be used later. And while at it, add
> > proper kernel doc.
> >
> > No feature or behavior change.
> >
> > Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
>
> Acked-by: Chris Li <chrisl@xxxxxxxxxx>
>
>
> > ---
> >  mm/swap_state.c | 141 ++++++++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 95 insertions(+), 46 deletions(-)
> >
> > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > index 204a9499d50c..3da285a891b2 100644
> > --- a/mm/swap_state.c
> > +++ b/mm/swap_state.c
> > @@ -137,8 +137,42 @@ void *swap_cache_get_shadow(swp_entry_t entry)
> >         return NULL;
> >  }
> >
> > -void __swap_cache_add_folio(struct swap_cluster_info *ci,
> > -                           struct folio *folio, swp_entry_t entry)
> > +/**
> > + * __swap_cache_add_check - Check if a range is suitable for adding a folio.
> > + * @ci: The locked swap cluster.
> > + * @ci_off: Range start offset.
> > + * @nr: Number of slots to check.
> > + * @shadow: Returns the shadow value if one exists in the range.
> > + *
> > + * Check if all slots covered by given range have a swap count >= 1.
> > + * Retrieves the shadow if there is one.
> > + *
> > + * Context: Caller must lock the cluster.
> > + */
> > +static int __swap_cache_add_check(struct swap_cluster_info *ci,
> > +                                 unsigned int ci_off, unsigned int nr,
> > +                                 void **shadow)
> > +{
> > +       unsigned int ci_end = ci_off + nr;
> > +       unsigned long old_tb;
> > +
>
> Nitpick: Can add lockdep_assert_held(&ci->lock);
>
> Can check ci_end < SWAPFILE_CLUSTER and bail out on error.

Ack.

>
> > +       if (unlikely(!ci->table))
> > +               return -ENOENT;
> > +       do {
> > +               old_tb = __swap_table_get(ci, ci_off);
> > +               if (unlikely(swp_tb_is_folio(old_tb)))
> > +                       return -EEXIST;
> > +               if (unlikely(!__swp_tb_get_count(old_tb)))
> > +                       return -ENOENT;
> > +               if (swp_tb_is_shadow(old_tb))
> > +                       *shadow = swp_tb_to_shadow(old_tb);
>
> Nitpick: You can create a local variable for the shadow and assign it
> at the end. Because it is a pointer, the compiler can't optimize the
> store away.

This part will be reworked very soon but using a local variable here
is good for an intermediate commit.

> Chris


Thanks.