Re: [PATCH v5 15/19] mm, swap: add folio to swap cache directly on allocation

[Baoquan He <bhe@redhat.com>]

On 12/22/25 at 10:42am, Kairui Song wrote:
> On Sat, Dec 20, 2025 at 12:12 PM Baoquan He <bhe@redhat.com> wrote:
> >
> > On 12/20/25 at 03:43am, Kairui Song wrote:
> > > From: Kairui Song <kasong@tencent.com>
...snip...
> > > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > > index 327c051d7cd0..29fa8d313a79 100644
> > > --- a/mm/swap_state.c
> > > +++ b/mm/swap_state.c
> > > @@ -122,35 +122,56 @@ 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)
> > > +{
> > > +     unsigned long new_tb;
> > > +     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);
> > > +
> > > +     new_tb = folio_to_swp_tb(folio);
> > > +     ci_start = swp_cluster_offset(entry);
> > > +     ci_off = ci_start;
> > > +     ci_end = ci_start + nr_pages;
> > > +     do {
> > > +             VM_WARN_ON_ONCE(swp_tb_is_folio(__swap_table_get(ci, ci_off)));
> > > +             __swap_table_set(ci, ci_off, new_tb);
> > > +     } while (++ci_off < ci_end);
> > > +
> > > +     folio_ref_add(folio, nr_pages);
> > > +     folio_set_swapcache(folio);
> > > +     folio->swap = entry;
> > > +
> > > +     node_stat_mod_folio(folio, NR_FILE_PAGES, nr_pages);
> > > +     lruvec_stat_mod_folio(folio, NR_SWAPCACHE, nr_pages);
> > > +}
> > > +
> > >  /**
> > >   * swap_cache_add_folio - Add a folio into the swap cache.
> >
> > With my understanding, __swap_cache_add_folio() is the pure
> > functionality of adding a folio into swap cache. While
> 
> Hi Baoquan, thanks for the review and suggestion!
> 
> > swap_cache_add_folio() is specifically adding a folio into swap cache
> > during swap in path. Not sure if we can rename them to reflect this
> > clearer. At least from the functiona name and below kernel doc we can't
> > feel that. Maybe:
> > __swap_cache_add_folio() -> swap_cache_add_folio()
> 
> The `__` prefix should stay, I think. This function requires the
> caller to lock the swap cluster.
> 
> > swap_cache_add_folio() -> swap_cache_add_swapin_folio()
> 
> Indeed, my plan is that `swap_cache_add_folio` will be gone soon, we
> should always call `swap_cache_alloc_folio` instead to do the swap
> folio allocation in a unified way, and just remove this.
> 
> Currently we can't do that because shmem and anon have different
> routines for swapin folio allocation. Having a unified
> `swap_cache_alloc_folio` will provide better swapin fallout control
> and cleaner cgroup charging to avoid thrashing, etc.
> 
> Also this helper is currently inexplicitly used by zswap writeback
> too, so adding the swapin keyword seems not accurate.

OK, sounds good to me. Thanks for detailed explanation.