Re: [PATCH 4/4] mm, swap: Remove unneeded swap_extend_table_try_free() in swap_dup_entries_cluster()

[Youngjun Park]

On Mon, Jul 20, 2026 at 03:13:42PM +0800, Kemeng Shi wrote:
> Since commit 0475fde0f68de ("mm, swap: avoid leaving unused extend table
> after alloc race"), extend table is always allocated when any swap count

Looks like this? 395085eacdfa

> reach MAX - 1 and is always freed when swap count decrease to MAX - 1
> or MAX - 2 with cluster lock held. So the extend table will always be
> freed properly when decrease swap count in __swap_cluster_put_entry().
> So swap_extend_table_try_free() outside of __swap_cluster_put_entry()
> is unneeded and can be removed.
> 
> Signed-off-by: Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx>
> ---
>  mm/swapfile.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 3fcbd10c6353..fc9f16bc2e4e 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1732,7 +1732,6 @@ static int swap_dup_entries_cluster(struct swap_info_struct *si,
>  failed:
>  	while (ci_off-- > ci_start)
>  		__swap_cluster_put_entry(ci, ci_off);

Right, it handles duplicate here.

> -	swap_extend_table_try_free(ci);
>  	swap_cluster_unlock(ci);
>  	return err;
>  }
> -- 
> 2.36.1

However, looking at this part, since swap_extend_table_alloc() uses
GFP_ATOMIC, I think it could be further improved by not releasing the ci
lock.

This would also clarify the situation where scount becomes MAX - 2.
But there seem to be some drawbacks. For instance, the original intention
might have been to make the swap_extend_table_alloc() API work seamlessly
for both sleepable and unsleepable contexts, and some implementation details
might not look as clean.

Here is what I was thinking:
...
        err = __swap_cluster_dup_entry(ci, ci_off);
        /* <- Could -ENOMEM handling be improved here? */
        if (unlikely(err)) {
                if (err == -ENOMEM) {
                        spin_unlock(&ci->lock);
                        /* <- Change to NOT release the lock, and make
                         * swap_extend_table_alloc aware of it with
                         * GFP_ATOMIC(aware cluster locked) */
                        err = swap_extend_table_alloc(si, ci, ci_off,
                                                      GFP_ATOMIC);
                        spin_lock(&ci->lock);
                        if (!err)
                                goto restart;

Just sharing my thoughts! (Wondering if this could be a good direction for
further improvement?)

The current modification looks good enough to me.

Reviewed-by: Youngjun Park <youngjun.park.lge.com>

Youngjun