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

[Kemeng Shi]

在 2026/7/21 15:58:34, 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?)
Hello Youngjun,
Sorry for the late. I agree we can clarify the situation where scount becomes
MAX - 2 in this way and I also agree some implementation details might not
look as clean. So I would keep this change to make thing simple :).

Thanks
Kemeng> 
> The current modification looks good enough to me.
> 
> Reviewed-by: Youngjun Park <youngjun.park.lge.com>
> 
> Youngjun