Re: [PATCH v3 05/15] mm, swap: always lock and check the swap cache folio before use

[Kairui Song]

On Mon, Sep 15, 2025 at 11:07 PM Chris Li <chrisl@xxxxxxxxxx> wrote:
>
> On Wed, Sep 10, 2025 at 9:09 AM Kairui Song <ryncsn@xxxxxxxxx> wrote:
> > @@ -2004,6 +2002,13 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
> >         bool hwpoisoned = false;
> >         int ret = 1;
> >
> > +       /*
> > +        * If the folio is removed from swap cache by others, continue to
> > +        * unuse other PTEs. try_to_unuse may try again if we missed this one.
> > +        */
>
> It took me a while to figure out why we add a
> folio_matches_swap_entry() check here but we don't have an existing
> check for folio swap entry matching in this function. Can you confirm
> that if a race has happened causing the folio swap entry mismatch,
> then try_to_usue() MUST try again rather than "may" try again. It
> seems to me that it is a MUST rather than "may". I am curious in what
> condition the mismatch happens and the try_to_unuse() does not need to
> try again.

It depends, I think it may, because: for example here we are unusing
folio A with entry S1, and a raced process just swapin folio A then
remove it from swap cache. If the raced process didn't swapout the PTE
again, then there is no need to retry as we are dong with this PTE.

There are many races, someone else swapped out folio A again using S2.
Then here we will see a !folio_matches_swap_entry. And that's OK.

But there have been many other subtle race conditions in other places,
e.g. another folio occupied the same PTE and got swapped out using S1,
causing PTE == S1 and here got a wrong folio A installed. This isn't
happening here, because we have removed the !SWP_WRITEOK flag from the
si during swapoff...

It's really complex and fragile, so just make it easier, check
folio_matches_swap_entry and abort early, is safer and follows the
convention.

> BTW, this function has three types of return value, 1, 0, and negative
> for error. The 0 and 1 are ignored by the caller, the caller only
> cares about the negative value. Overall this unuse_pte() and
> try_to_unuse() walk feels complicated and maybe a candidate for later
> clean up. That is not your patch's fault. I am not requesting a
> cleanup in this series.

Right, totally agree, we can cleanup the swapoff part later.

> With that Nitpick,
>
> Acked-by: Chris Li <chrisl@xxxxxxxxxx>

Thanks!