Re: [PATCH v2 11/15] mm, swap: use the swap table for the swap cache and switch API

[Kairui Song <ryncsn@gmail.com>]

On Sat, Sep 6, 2025 at 11:35 PM Chris Li <chrisl@kernel.org> wrote:
>
> Acked-by: Chris Li <chrisl@kernel.org>
>
> Some nitpick follows.
>
> Chris

Thanks!

> > +
> > +/*
> > + * swap_cluster_lock_by_folio - Locks the cluster that holds a folio's entries.
> > + * @folio: The folio.
> > + *
> > + * This locks the swap cluster that contains a folio's swap entries. The
> > + * swap entries of a folio are always in one single cluster, and a locked
> > + * swap cache folio is enough to stabilize the entries and the swap device.
>
> I was wondering if we have a better word than stabilize, we haven't
> defined what does stabilize mean. I assume it means protecting from
> racing access to the swap cache entry. If we describe what it protects
> or what it prevents, that would give more detailed meaning than
> stabilize.

Right, I used to use the word "pin". What it means here is: locking
the folio will ensure folio->swap won't change so the folio will have
a stable bind with the swap cluster its folio->swap points to. Also
the swap device can't be swapped off so there is no risk of UAF.

How about:

 * This locks the swap cluster that contains a folio's swap entries. The
 * swap entries of a folio are always in one single cluster. The folio has
 * to be locked so its swap entries won't change and the cluster is binded
 * to the folio.

...

> > @@ -123,57 +136,45 @@ void *swap_cache_get_shadow(swp_entry_t entry)
> >   * SWAP_HAS_CACHE to avoid race or conflict.
> >   * Return: Returns 0 on success, error code otherwise.
> >   */
> > -int swap_cache_add_folio(struct folio *folio, swp_entry_t entry,
> > -                        gfp_t gfp, void **shadowp)
> > +void swap_cache_add_folio(struct folio *folio, swp_entry_t entry, void **shadowp)
> >  {
> > -       struct address_space *address_space = swap_address_space(entry);
> > -       pgoff_t idx = swap_cache_index(entry);
> > -       XA_STATE_ORDER(xas, &address_space->i_pages, idx, folio_order(folio));
> > -       unsigned long i, nr = folio_nr_pages(folio);
> > -       void *old;
> > -
> > -       xas_set_update(&xas, workingset_update_node);
> > -
> > -       VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
> > -       VM_BUG_ON_FOLIO(folio_test_swapcache(folio), folio);
> > -       VM_BUG_ON_FOLIO(!folio_test_swapbacked(folio), folio);
> > +       void *shadow = NULL;
> > +       unsigned long swp_tb, exist;
> > +       struct swap_cluster_info *ci;
> > +       unsigned int ci_start, ci_off, ci_end;
> > +       unsigned long nr_pages = folio_nr_pages(folio);
> > +
> > +       VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);
> > +       VM_WARN_ON_ONCE_FOLIO(folio_test_swapcache(folio), folio);
> > +       VM_WARN_ON_ONCE_FOLIO(!folio_test_swapbacked(folio), folio);
> > +
> > +       swp_tb = folio_to_swp_tb(folio);
> > +       ci_start = swp_cluster_offset(entry);
> > +       ci_end = ci_start + nr_pages;
> > +       ci_off = ci_start;
> > +       ci = swap_cluster_lock(__swap_entry_to_info(entry), swp_offset(entry));
> > +       do {
> > +               exist = __swap_table_xchg(ci, ci_off, swp_tb);
>
> Thanks for changing it to xchg. I understand that by "exist" you mean
> the previous existing swap table entry. However after it was taken out
> from the swap table, is it still considered an "existing entry"? I am
> considering "old" or "prior" might be a better name. Just nitpicks
> anyway. If we use "old", we can rename "swp_tb" to "new_tb" to make it
> obvious what we are replacing with.

Good suggestion.

>
> Also I saw this kind of for loop repeat a few places.
> Maybe considering some for loop macro to do:
>
> for_each_folio_offset(folio, ci, ci_off) {
>       exist = __swap_table_xchg(ci, ci_off, swp_tb);
>       ...
> } end_for_each_folio_offset();
>
> The kernel has a lot of similar for loop macros.
>

There seem to be only a few users like this, but I can have a try.
Will use this style if it helps to reduce LOC or make it easier.
Thanks!