Re: [PATCH v2 3/4] mm/shmem, swap: improve mthp swapin process

[Kairui Song <ryncsn@gmail.com>]

On Fri, Jun 20, 2025 at 3:53 PM YoungJun Park <youngjun.park@lge.com> wrote:
> On Fri, Jun 20, 2025 at 01:55:37AM +0800, Kairui Song wrote:
> > +     if (skip_swapcache) {
> > +             swapcache_clear(si, folio->swap, folio_nr_pages(folio));
> > +             folio->swap.val = 0;
> >       }
> > +     if (folio)
> > +             folio_put(folio);
> >       put_swap_device(si);
>
> I really appreciate the patch. great work!

Hi YoungJun,

Thanks for the suggestions.

>
> It's a rather trivial point, but I was wondering if the following change
> might make sense:
> ...
>         if (skip_swapcache)
>                 swapcache_clear(si, folio->swap, folio_nr_pages(folio));
>
>         put_swap_device(si);
>
>         if (folio)
>                 folio_put(folio);
>
>         return error;
> ...
>
> My intention here is to minimize the reference to si,

The si reference is only used to prevent swapoff from releasing the
underlying swap data structures, which is trivial as the overhead is
tiny, and releasing the folio first might help reduce memory pressure
(even more trivial though).

> and from what I understand, this folio has already been allocated and would
> soon disappear. Is it possible to to reduce the clear operation?
> (folio->swap.val = 0)

Right, that might be not needed, but leaving a dangling swap entry in
the folio->private seems not a very good practice to me, so while at,
I added this clearing (folio->private is always cleared for anon
swapin that bypass swap cache). The chance of a failed
"skip_swapcache" swapin is quite low here, so I think it should be OK.

> Just a small suggestion.
> Thank you again for your work!

Thanks for the review!

> Regards,
> Youngjun Park
>