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

[YoungJun Park <youngjun.park@lge.com>]

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? 

Thanks, 
Youngjun Park