Re: [PATCH] mm/swap: fix wrong plist empty check in swap_alloc_slow()

[Kairui Song <ryncsn@gmail.com>]

On Thu, Nov 20, 2025 at 12:37 AM YoungJun Park <youngjun.park@lge.com> wrote:
>
> On Wed, Nov 19, 2025 at 09:02:14PM +0800, Kairui Song wrote:
> > On Wed, Nov 19, 2025 at 7:56 PM Youngjun Park <youngjun.park@lge.com> wrote:
> > >
> > > swap_alloc_slow() was checking `si->avail_list` instead of `next->avail_list`
> > > when verifying if the next swap device is still in the list, which could cause
> > > unnecessary restarts during allocation.
> > >
> > > Fixes: 8e689f8ea45ff ("mm/swap: do not choose swap device according to numa node")
> > > Signed-off-by: Youngjun Park <youngjun.park@lge.com>
> > > ---
> > >  mm/swapfile.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > > index 94e0f0c54168..cf780fefaf7d 100644
> > > --- a/mm/swapfile.c
> > > +++ b/mm/swapfile.c
> > > @@ -1374,7 +1374,7 @@ static bool swap_alloc_slow(swp_entry_t *entry,
> > >                  * still in the swap_avail_head list then try it, otherwise
> > >                  * start over if we have not gotten any slots.
> > >                  */
> > > -               if (plist_node_empty(&si->avail_list))
> > > +               if (plist_node_empty(&next->avail_list))
> > >                         goto start_over;
> > >         }
> > >         spin_unlock(&swap_avail_lock);
> > > --
> > > 2.34.1
> >
> > Thanks!
> >
> > Acked-by: Kairui Song <kasong@tencent.com>
> >
> > BTW I noticed the comment here needs update too. And the plist usage
>
> As I think, the first part of the comment is out of date and provides
> wrong information. I suggest we simplify it to keep only the last part:
>
> /*
>  * swap_avail_head list may have been modified; so if next is
>  * no longer in the swap_avail_head list, start over if we have
>  * not gotten any slots.
>  */
>
> > here seems can also be simplified? We never use a lower priority
> > device when there is any higher priority device in the list, as
> > devices are removed from the list when they are full.  And the first
> > thing we do here is plist_requeue. So we can just keep trying the
> > head, until the whole list is empty, seems good enough?
> >
>
> if we can guarantee that cluster_alloc_swap_entry() failure is always due to
> the device being full, then we could indeed improve this by removing
> the "try each device in order" loop and only trying the highest
> priority device, as you suggested.
>
> A simple implementation might look like this (though it needs more
> thought and testing):
>
> spin_lock(&swap_avail_lock);
> while (!plist_head_empty(&swap_avail_head)) {
>     si = plist_first_entry(&swap_avail_head, ...);
>     plist_requeue(&si->avail_list[nid], &swap_avail_head);
>     spin_unlock(&swap_avail_lock);
>     if (cluster_alloc_swap_entry(si, order, SWAP_HAS_CACHE))
>        return;
>     spin_lock(&swap_avail_lock);
> }
> spin_unlock(&swap_avail_lock);
>
> For now, would it be okay if I just address the comment update in this
> patch, and submit your simplification suggestion as a separate patch
> after further consideration?

Sure, no need to do too much refractor here. I'm fine with the patch as it is.