[PATCH 0/4] mm, swap: some random fixes and cleanups

[PATCH 0/4] mm, swap: some random fixes and cleanups

[Kemeng Shi]

Hello all,

This series contains some random fixes and cleanups. More details
can be found in respective patches.

Thanks!

Kemeng Shi (4):
  mm, swap: Fix potential NULL dereference when trying a sleep table
    allocation
  mm, swap: Move setup_swap_clusters_info() after SWP_SOLIDSTATE
    initialization
  mm, swap: return early from swap_extend_table_try_free() on first
    non-zero entry
  mm, swap: Remove unneeded swap_extend_table_try_free() in
    swap_dup_entries_cluster()

 mm/swapfile.c | 44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)

-- 
2.36.1

[PATCH 1/4] mm, swap: Fix potential NULL dereference when trying a sleep table allocation

[Kemeng Shi]

The root cause of this issue is because multi-tables are updated in non
atomic context. To be more specific, the issue could be triggerred as
following:

swap_alloc_fast         swap_cluster_populate()
                         /* Try a sleep allocation */
                         spin_unlock(&ci->lock);
                         swap_cluster_alloc_table()
                          rcu_assign_pointer(ci->table, table);

 ci = swap_cluster_lock(si, offset)
 cluster_is_usable(ci, order）
  if (!cluster_table_is_alloced(ci)) // ok
 alloc_swap_scan_cluster()
  cluster_scan_range()
   __swap_table_get()

                          /* free table when more table allocation fails */
                          ci->memcg_table = kzalloc_obj(*ci->memcg_table,
                                                        gfp);
                          if (!ci->memcg_table)
                           swap_cluster_free_table()
                            rcu_assign_pointer(ci->table, NULL);

    table = rcu_dereference_check(ci->table, lockdep_is_held(&ci->lock));
     atomic_long_read(&table[off]); // NULL dereference

Fix the issue by updating allocated tables in atomic context.

Fixes: 2fe7a6f5024b8 ("mm/memcg, swap: store cgroup id in cluster table directly")
Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/swapfile.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 615d90867111..d29062d9c3cd 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -490,6 +490,20 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
 	return 0;
 }
 
+static void swap_cluster_copy_table(struct swap_cluster_info *d_ci,
+				    struct swap_cluster_info *s_ci)
+{
+	rcu_assign_pointer(d_ci->table, rcu_access_pointer(s_ci->table));
+
+#ifdef CONFIG_MEMCG
+	d_ci->memcg_table = s_ci->memcg_table;
+#endif
+
+#if !SWAP_TABLE_HAS_ZEROFLAG
+	d_ci->zero_bitmap = s_ci->zero_bitmap;
+#endif
+}
+
 /*
  * Sanity check to ensure nothing leaked, and the specified range is empty.
  * One special case is that bad slots can't be freed, so check the number of
@@ -527,6 +541,7 @@ static struct swap_cluster_info *
 swap_cluster_populate(struct swap_info_struct *si,
 			 struct swap_cluster_info *ci)
 {
+	struct swap_cluster_info tmp_ci;
 	int ret;
 
 	/*
@@ -552,7 +567,8 @@ swap_cluster_populate(struct swap_info_struct *si,
 		spin_unlock(&si->global_cluster_lock);
 	local_unlock(&percpu_swap_cluster.lock);
 
-	ret = swap_cluster_alloc_table(ci, __GFP_HIGH | __GFP_NOMEMALLOC |
+	tmp_ci = *ci;
+	ret = swap_cluster_alloc_table(&tmp_ci, __GFP_HIGH | __GFP_NOMEMALLOC |
 					   GFP_KERNEL);
 
 	/*
@@ -573,6 +589,9 @@ swap_cluster_populate(struct swap_info_struct *si,
 		spin_unlock(&ci->lock);
 		return NULL;
 	}
+
+	/* Update tables in atomic context */
+	swap_cluster_copy_table(ci, &tmp_ci);
 	return ci;
 }
 
-- 
2.36.1

Re: [PATCH 1/4] mm, swap: Fix potential NULL dereference when trying a sleep table allocation

[Youngjun Park]

On Mon, Jul 20, 2026 at 03:13:39PM +0800, Kemeng Shi wrote:

Hello Kemeng Shi,

Good catch!

it indeed looks like a possible race condition 
(though I haven't verified it at runtime either).

> The root cause of this issue is because multi-tables are updated in non
> atomic context. To be more specific, the issue could be triggerred as
> following:

Here the cluster is isolated (CLUSTER_FLAG_NONE).

> swap_alloc_fast         swap_cluster_populate()
>                          /* Try a sleep allocation */
>                          spin_unlock(&ci->lock);

And from here, the table becomes visible,

>                          swap_cluster_alloc_table()
>                           rcu_assign_pointer(ci->table, table);

All entry on this cluster is freed(e.g process dead) , 
so this might be the free cluster.

>  ci = swap_cluster_lock(si, offset)
>  cluster_is_usable(ci, order）

Since it's a normal cluster (CLUSTER_FLAG_NONE) and the table exists,
it passes here...

>   if (!cluster_table_is_alloced(ci)) // ok 
>  alloc_swap_scan_cluster()
>   cluster_scan_range()
>    __swap_table_get()
> 
>                           /* free table when more table allocation fails */
>                           ci->memcg_table = kzalloc_obj(*ci->memcg_table,
>                                                         gfp);
>                           if (!ci->memcg_table)
>                            swap_cluster_free_table()

Nullified

>                             rcu_assign_pointer(ci->table, NULL);

Now it happens.

>     table = rcu_dereference_check(ci->table, lockdep_is_held(&ci->lock));
>      atomic_long_read(&table[off]); // NULL dereference
> 
> Fix the issue by updating allocated tables in atomic context.
> 
> Fixes: 2fe7a6f5024b8 ("mm/memcg, swap: store cgroup id in cluster table directly")
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  mm/swapfile.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 615d90867111..d29062d9c3cd 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -490,6 +490,20 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
>  	return 0;
>  }
>  
> +static void swap_cluster_copy_table(struct swap_cluster_info *d_ci,
> +				    struct swap_cluster_info *s_ci)
> +{
> +	rcu_assign_pointer(d_ci->table, rcu_access_pointer(s_ci->table));
> +
> +#ifdef CONFIG_MEMCG
> +	d_ci->memcg_table = s_ci->memcg_table;
> +#endif
> +
> +#if !SWAP_TABLE_HAS_ZEROFLAG
> +	d_ci->zero_bitmap = s_ci->zero_bitmap;
> +#endif
> +}
> +
>  /*
>   * Sanity check to ensure nothing leaked, and the specified range is empty.
>   * One special case is that bad slots can't be freed, so check the number of
> @@ -527,6 +541,7 @@ static struct swap_cluster_info *
>  swap_cluster_populate(struct swap_info_struct *si,
>  			 struct swap_cluster_info *ci)
>  {
> +	struct swap_cluster_info tmp_ci;
>  	int ret;

IMHO, How about resolving everything inside swap_cluster_alloc_table() instead?

We could consider the following two things.

- Assign the table at the very end inside swap_cluster_alloc_table().
- Handle the table freeing properly if subsequent allocations fail.

Rather than allocating a temp_ci (which adds special handling
for this case), wouldn't it be better to maintain the original intention
of the swap_cluster_alloc_table() function?

Although this is not a hot path and table allocation will usually succeed with
the ATOMIC allocator, this approach would also save the memcpy overhead
and stack memory usage.

(Another option might be checking memcg_table and zero_bitmap inside
cluster_table_is_alloced(). However, I personally dislike this idea as
it might introduce side effects regarding its coverage.)

Your current approach is also a good direction, but please review this
idea as well :)

Thanks,
Youngjun

Re: [PATCH 1/4] mm, swap: Fix potential NULL dereference when trying a sleep table allocation

[Kemeng Shi]

在 2026/7/20 17:11:54, Youngjun Park 写道:
> On Mon, Jul 20, 2026 at 03:13:39PM +0800, Kemeng Shi wrote:
> 
> Hello Kemeng Shi,
> 
> Good catch!
> 
> it indeed looks like a possible race condition 
> (though I haven't verified it at runtime either).
> 
>> The root cause of this issue is because multi-tables are updated in non
>> atomic context. To be more specific, the issue could be triggerred as
>> following:
> 
> Here the cluster is isolated (CLUSTER_FLAG_NONE).
> 
>> swap_alloc_fast         swap_cluster_populate()
>>                          /* Try a sleep allocation */
>>                          spin_unlock(&ci->lock);
> 
> And from here, the table becomes visible,
> 
>>                          swap_cluster_alloc_table()
>>                           rcu_assign_pointer(ci->table, table);
> 
> All entry on this cluster is freed(e.g process dead) , 
> so this might be the free cluster.
> 
>>  ci = swap_cluster_lock(si, offset)
>>  cluster_is_usable(ci, order）
> 
> Since it's a normal cluster (CLUSTER_FLAG_NONE) and the table exists,
> it passes here...
> 
>>   if (!cluster_table_is_alloced(ci)) // ok 
>>  alloc_swap_scan_cluster()
>>   cluster_scan_range()
>>    __swap_table_get()
>>
>>                           /* free table when more table allocation fails */
>>                           ci->memcg_table = kzalloc_obj(*ci->memcg_table,
>>                                                         gfp);
>>                           if (!ci->memcg_table)
>>                            swap_cluster_free_table()
> 
> Nullified
> 
>>                             rcu_assign_pointer(ci->table, NULL);
> 
> Now it happens.
> 
>>     table = rcu_dereference_check(ci->table, lockdep_is_held(&ci->lock));
>>      atomic_long_read(&table[off]); // NULL dereference
>>
>> Fix the issue by updating allocated tables in atomic context.
>>
>> Fixes: 2fe7a6f5024b8 ("mm/memcg, swap: store cgroup id in cluster table directly")
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>> ---
>>  mm/swapfile.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 615d90867111..d29062d9c3cd 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -490,6 +490,20 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
>>  	return 0;
>>  }
>>  
>> +static void swap_cluster_copy_table(struct swap_cluster_info *d_ci,
>> +				    struct swap_cluster_info *s_ci)
>> +{
>> +	rcu_assign_pointer(d_ci->table, rcu_access_pointer(s_ci->table));
>> +
>> +#ifdef CONFIG_MEMCG
>> +	d_ci->memcg_table = s_ci->memcg_table;
>> +#endif
>> +
>> +#if !SWAP_TABLE_HAS_ZEROFLAG
>> +	d_ci->zero_bitmap = s_ci->zero_bitmap;
>> +#endif
>> +}
>> +
>>  /*
>>   * Sanity check to ensure nothing leaked, and the specified range is empty.
>>   * One special case is that bad slots can't be freed, so check the number of
>> @@ -527,6 +541,7 @@ static struct swap_cluster_info *
>>  swap_cluster_populate(struct swap_info_struct *si,
>>  			 struct swap_cluster_info *ci)
>>  {
>> +	struct swap_cluster_info tmp_ci;
>>  	int ret;
> 
Hello, Youngjun

Thanks for feedback.> IMHO, How about resolving everything inside swap_cluster_alloc_table() instead?
> 
> We could consider the following two things.
> 
> - Assign the table at the very end inside swap_cluster_alloc_table().
> - Handle the table freeing properly if subsequent allocations fail.
> 
> Rather than allocating a temp_ci (which adds special handling
> for this case), wouldn't it be better to maintain the original intention
> of the swap_cluster_alloc_table() function?
> 
> Although this is not a hot path and table allocation will usually succeed with
> the ATOMIC allocator, this approach would also save the memcpy overhead
> and stack memory usage.
Yes, it will be better to handle the issue inside the swap_cluster_alloc_table()
and I also consider this before. However, I didn't find a way to properly handle
the lock as swap_cluster_alloc_table() will be used for both ATOMIC allocation
and sleep allocation. So what about introduce a new helper like:
swap_cluster_alloc_table_unlocked()
{
	table = kmem_cache_zalloc()
#ifdef CONFIG_MEMCG
	memcg_table = kzalloc_obj()
#endif
#if !SWAP_TABLE_HAS_ZEROFLAG
	zero_bitmap = bitmap_zalloc()
#endif

	spin_lock(&ci->lock);
	/* update all tables in atomic */
	spin_unlock(&ci->lock);
}
In this way, there are some repeated code, so it is also a little ugly...
I will appreciate if you have any better idea!
> 
> (Another option might be checking memcg_table and zero_bitmap inside
> cluster_table_is_alloced(). However, I personally dislike this idea as
> it might introduce side effects regarding its coverage.)
Yes, I also think we should try not to introduce any cost in hot path.> 
> Your current approach is also a good direction, but please review this
> idea as well :)
> 
> Thanks,
> Youngjun

Re: [PATCH 1/4] mm, swap: Fix potential NULL dereference when trying a sleep table allocation

[Youngjun Park]

On Tue, Jul 21, 2026 at 09:49:53AM +0800, Kemeng Shi wrote:
> 在 2026/7/20 17:11:54, Youngjun Park 写道:
> > On Mon, Jul 20, 2026 at 03:13:39PM +0800, Kemeng Shi wrote:
> > 
> > Hello Kemeng Shi,
> > 
> > Good catch!
> > 
> > it indeed looks like a possible race condition 
> > (though I haven't verified it at runtime either).
> > 
> >> The root cause of this issue is because multi-tables are updated in non
> >> atomic context. To be more specific, the issue could be triggerred as
> >> following:
> > 
> > Here the cluster is isolated (CLUSTER_FLAG_NONE).
> > 
> >> swap_alloc_fast         swap_cluster_populate()
> >>                          /* Try a sleep allocation */
> >>                          spin_unlock(&ci->lock);
> > 
> > And from here, the table becomes visible,
> > 
> >>                          swap_cluster_alloc_table()
> >>                           rcu_assign_pointer(ci->table, table);
> > 
> > All entry on this cluster is freed(e.g process dead) , 
> > so this might be the free cluster.
> > 
> >>  ci = swap_cluster_lock(si, offset)
> >>  cluster_is_usable(ci, order）
> > 
> > Since it's a normal cluster (CLUSTER_FLAG_NONE) and the table exists,
> > it passes here...
> > 
> >>   if (!cluster_table_is_alloced(ci)) // ok 
> >>  alloc_swap_scan_cluster()
> >>   cluster_scan_range()
> >>    __swap_table_get()
> >>
> >>                           /* free table when more table allocation fails */
> >>                           ci->memcg_table = kzalloc_obj(*ci->memcg_table,
> >>                                                         gfp);
> >>                           if (!ci->memcg_table)
> >>                            swap_cluster_free_table()
> > 
> > Nullified
> > 
> >>                             rcu_assign_pointer(ci->table, NULL);
> > 
> > Now it happens.
> > 
> >>     table = rcu_dereference_check(ci->table, lockdep_is_held(&ci->lock));
> >>      atomic_long_read(&table[off]); // NULL dereference
> >>
> >> Fix the issue by updating allocated tables in atomic context.
> >>
> >> Fixes: 2fe7a6f5024b8 ("mm/memcg, swap: store cgroup id in cluster table directly")
> >> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> >> ---
> >>  mm/swapfile.c | 21 ++++++++++++++++++++-
> >>  1 file changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/mm/swapfile.c b/mm/swapfile.c
> >> index 615d90867111..d29062d9c3cd 100644
> >> --- a/mm/swapfile.c
> >> +++ b/mm/swapfile.c
> >> @@ -490,6 +490,20 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
> >>  	return 0;
> >>  }
> >>  
> >> +static void swap_cluster_copy_table(struct swap_cluster_info *d_ci,
> >> +				    struct swap_cluster_info *s_ci)
> >> +{
> >> +	rcu_assign_pointer(d_ci->table, rcu_access_pointer(s_ci->table));
> >> +
> >> +#ifdef CONFIG_MEMCG
> >> +	d_ci->memcg_table = s_ci->memcg_table;
> >> +#endif
> >> +
> >> +#if !SWAP_TABLE_HAS_ZEROFLAG
> >> +	d_ci->zero_bitmap = s_ci->zero_bitmap;
> >> +#endif
> >> +}
> >> +
> >>  /*
> >>   * Sanity check to ensure nothing leaked, and the specified range is empty.
> >>   * One special case is that bad slots can't be freed, so check the number of
> >> @@ -527,6 +541,7 @@ static struct swap_cluster_info *
> >>  swap_cluster_populate(struct swap_info_struct *si,
> >>  			 struct swap_cluster_info *ci)
> >>  {
> >> +	struct swap_cluster_info tmp_ci;
> >>  	int ret;
> > 
> Hello, Youngjun
> 
> Thanks for feedback.> IMHO, How about resolving everything inside swap_cluster_alloc_table() instead?
> > 
> > We could consider the following two things.
> > 
> > - Assign the table at the very end inside swap_cluster_alloc_table().
> > - Handle the table freeing properly if subsequent allocations fail.
> > 
> > Rather than allocating a temp_ci (which adds special handling
> > for this case), wouldn't it be better to maintain the original intention
> > of the swap_cluster_alloc_table() function?
> > 
> > Although this is not a hot path and table allocation will usually succeed with
> > the ATOMIC allocator, this approach would also save the memcpy overhead
> > and stack memory usage.
> Yes, it will be better to handle the issue inside the swap_cluster_alloc_table()
> and I also consider this before. However, I didn't find a way to properly handle
> the lock as swap_cluster_alloc_table() will be used for both ATOMIC allocation
> and sleep allocation. So what about introduce a new helper like:
> swap_cluster_alloc_table_unlocked()
> {
> 	table = kmem_cache_zalloc()
> #ifdef CONFIG_MEMCG
> 	memcg_table = kzalloc_obj()
> #endif
> #if !SWAP_TABLE_HAS_ZEROFLAG
> 	zero_bitmap = bitmap_zalloc()
> #endif
> 
> 	spin_lock(&ci->lock);
> 	/* update all tables in atomic */
> 	spin_unlock(&ci->lock);
> }
> In this way, there are some repeated code, so it is also a little ugly...
> I will appreciate if you have any better idea!

Hello!

I don't quite understand why we need to introduce a new helper function.
Is it because the table assignment must be done strictly inside the lock?
In successful cases, wouldn't it be fine to assign the table without holding
the lock? (Please let me know if I am missing something here.)

If we just add error handling to free the allocated table inside
swap_cluster_alloc_table(), swap_cluster_free_table() will properly free
memcg_table and zero_bitmap as long as ci->table is not assigned yet.
I don't think this will cause any issues.

My intention is something like this:

-       rcu_assign_pointer(ci->table, table);     /* Remove this */
...

#ifdef CONFIG_MEMCG
        if (!mem_cgroup_disabled()) {
                VM_WARN_ON_ONCE(ci->memcg_table);
                ci->memcg_table = kzalloc_obj(*ci->memcg_table, gfp);
                if (!ci->memcg_table) {
			+ free table
                        swap_cluster_free_table(ci);
                        return -ENOMEM;
                }
        }
#endif

#if !SWAP_TABLE_HAS_ZEROFLAG
        VM_WARN_ON_ONCE(ci->zero_bitmap);
        ci->zero_bitmap = bitmap_zalloc(SWAPFILE_CLUSTER, gfp);
        if (!ci->zero_bitmap) {
                + free table
                swap_cluster_free_table(ci);
                return -ENOMEM;
        }
#endif

...
        /* Assign here! */
        rcu_assign_pointer(ci->table, table);

	return
}
If it becomes visible at the very end, wouldn't it be safe from any issues?

Youngjun

Re: [PATCH 1/4] mm, swap: Fix potential NULL dereference when trying a sleep table allocation

[Kemeng Shi]

在 2026/7/21 10:15:40, Youngjun Park 写道:
> On Tue, Jul 21, 2026 at 09:49:53AM +0800, Kemeng Shi wrote:
>> 在 2026/7/20 17:11:54, Youngjun Park 写道:
>>> On Mon, Jul 20, 2026 at 03:13:39PM +0800, Kemeng Shi wrote:
>>>
>>> Hello Kemeng Shi,
>>>
>>> Good catch!
>>>
>>> it indeed looks like a possible race condition 
>>> (though I haven't verified it at runtime either).
>>>
>>>> The root cause of this issue is because multi-tables are updated in non
>>>> atomic context. To be more specific, the issue could be triggerred as
>>>> following:
>>>
>>> Here the cluster is isolated (CLUSTER_FLAG_NONE).
>>>
>>>> swap_alloc_fast         swap_cluster_populate()
>>>>                          /* Try a sleep allocation */
>>>>                          spin_unlock(&ci->lock);
>>>
>>> And from here, the table becomes visible,
>>>
>>>>                          swap_cluster_alloc_table()
>>>>                           rcu_assign_pointer(ci->table, table);
>>>
>>> All entry on this cluster is freed(e.g process dead) , 
>>> so this might be the free cluster.
>>>
>>>>  ci = swap_cluster_lock(si, offset)
>>>>  cluster_is_usable(ci, order）
>>>
>>> Since it's a normal cluster (CLUSTER_FLAG_NONE) and the table exists,
>>> it passes here...
>>>
>>>>   if (!cluster_table_is_alloced(ci)) // ok 
>>>>  alloc_swap_scan_cluster()
>>>>   cluster_scan_range()
>>>>    __swap_table_get()
>>>>
>>>>                           /* free table when more table allocation fails */
>>>>                           ci->memcg_table = kzalloc_obj(*ci->memcg_table,
>>>>                                                         gfp);
>>>>                           if (!ci->memcg_table)
>>>>                            swap_cluster_free_table()
>>>
>>> Nullified
>>>
>>>>                             rcu_assign_pointer(ci->table, NULL);
>>>
>>> Now it happens.
>>>
>>>>     table = rcu_dereference_check(ci->table, lockdep_is_held(&ci->lock));
>>>>      atomic_long_read(&table[off]); // NULL dereference
>>>>
>>>> Fix the issue by updating allocated tables in atomic context.
>>>>
>>>> Fixes: 2fe7a6f5024b8 ("mm/memcg, swap: store cgroup id in cluster table directly")
>>>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>>>> ---
>>>>  mm/swapfile.c | 21 ++++++++++++++++++++-
>>>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>>>> index 615d90867111..d29062d9c3cd 100644
>>>> --- a/mm/swapfile.c
>>>> +++ b/mm/swapfile.c
>>>> @@ -490,6 +490,20 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static void swap_cluster_copy_table(struct swap_cluster_info *d_ci,
>>>> +				    struct swap_cluster_info *s_ci)
>>>> +{
>>>> +	rcu_assign_pointer(d_ci->table, rcu_access_pointer(s_ci->table));
>>>> +
>>>> +#ifdef CONFIG_MEMCG
>>>> +	d_ci->memcg_table = s_ci->memcg_table;
>>>> +#endif
>>>> +
>>>> +#if !SWAP_TABLE_HAS_ZEROFLAG
>>>> +	d_ci->zero_bitmap = s_ci->zero_bitmap;
>>>> +#endif
>>>> +}
>>>> +
>>>>  /*
>>>>   * Sanity check to ensure nothing leaked, and the specified range is empty.
>>>>   * One special case is that bad slots can't be freed, so check the number of
>>>> @@ -527,6 +541,7 @@ static struct swap_cluster_info *
>>>>  swap_cluster_populate(struct swap_info_struct *si,
>>>>  			 struct swap_cluster_info *ci)
>>>>  {
>>>> +	struct swap_cluster_info tmp_ci;
>>>>  	int ret;
>>>
>> Hello, Youngjun
>>
>> Thanks for feedback.> IMHO, How about resolving everything inside swap_cluster_alloc_table() instead?
>>>
>>> We could consider the following two things.
>>>
>>> - Assign the table at the very end inside swap_cluster_alloc_table().
>>> - Handle the table freeing properly if subsequent allocations fail.
>>>
>>> Rather than allocating a temp_ci (which adds special handling
>>> for this case), wouldn't it be better to maintain the original intention
>>> of the swap_cluster_alloc_table() function?
>>>
>>> Although this is not a hot path and table allocation will usually succeed with
>>> the ATOMIC allocator, this approach would also save the memcpy overhead
>>> and stack memory usage.
>> Yes, it will be better to handle the issue inside the swap_cluster_alloc_table()
>> and I also consider this before. However, I didn't find a way to properly handle
>> the lock as swap_cluster_alloc_table() will be used for both ATOMIC allocation
>> and sleep allocation. So what about introduce a new helper like:
>> swap_cluster_alloc_table_unlocked()
>> {
>> 	table = kmem_cache_zalloc()
>> #ifdef CONFIG_MEMCG
>> 	memcg_table = kzalloc_obj()
>> #endif
>> #if !SWAP_TABLE_HAS_ZEROFLAG
>> 	zero_bitmap = bitmap_zalloc()
>> #endif
>>
>> 	spin_lock(&ci->lock);
>> 	/* update all tables in atomic */
>> 	spin_unlock(&ci->lock);
>> }
>> In this way, there are some repeated code, so it is also a little ugly...
>> I will appreciate if you have any better idea!
> 
> Hello!
> 
> I don't quite understand why we need to introduce a new helper function.
> Is it because the table assignment must be done strictly inside the lock?
> In successful cases, wouldn't it be fine to assign the table without holding
> the lock? (Please let me know if I am missing something here.)
> 
> If we just add error handling to free the allocated table inside
> swap_cluster_alloc_table(), swap_cluster_free_table() will properly free
> memcg_table and zero_bitmap as long as ci->table is not assigned yet.
> I don't think this will cause any issues.
> 
> My intention is something like this:
> 
> -       rcu_assign_pointer(ci->table, table);     /* Remove this */
> ...
> 
> #ifdef CONFIG_MEMCG
>         if (!mem_cgroup_disabled()) {
>                 VM_WARN_ON_ONCE(ci->memcg_table);
>                 ci->memcg_table = kzalloc_obj(*ci->memcg_table, gfp);
>                 if (!ci->memcg_table) {
> 			+ free table
>                         swap_cluster_free_table(ci);
>                         return -ENOMEM;
>                 }
>         }
> #endif
> 
> #if !SWAP_TABLE_HAS_ZEROFLAG
>         VM_WARN_ON_ONCE(ci->zero_bitmap);
>         ci->zero_bitmap = bitmap_zalloc(SWAPFILE_CLUSTER, gfp);
>         if (!ci->zero_bitmap) {
>                 + free table
>                 swap_cluster_free_table(ci);
>                 return -ENOMEM;
>         }
> #endif
> 
> ...
>         /* Assign here! */
>         rcu_assign_pointer(ci->table, table);
> 
> 	return
> }
> If it becomes visible at the very end, wouldn't it be safe from any issues?
Sorry, I misunderstand what you mean. I think this is a better direction!
As we rely on memory ordering, I'm checking if we need add memory barrier
betwwen access ci->table and zero_bitmap or betwwen ci->table and memcg_table.

On write side:
rcu_assign_pointer(ci->table, table) will offer release to ensure
zero_bitmap and memcg_table visible before ci->table.

On read side:
folio_alloc_swap
 swap_alloc_fast/swap_alloc_slow
  /* access ci->table under cluster lock */
  swap_cluster_lock
  cluster_is_usable
  ...
  __swap_table_set
  ...
  swap_cluster_unlock
  
 mem_cgroup_try_charge_swap
  ...
  /* cluster lock will ensure order betwwen ci->table and memcg_table */
  swap_cluster_get_and_lock
  __swap_cgroup_set
  swap_cluster_unlock

swap_writeout
 swap_zeromap_folio_set
  /* cluster lock will ensure order betwwen ci->table and zero_bitmap */
  swap_cluster_get_and_lock
  __swap_table_set_zero
  swap_cluster_unlock

So I think we can fix the issue in this way and I will do it in the next
version. Thanks for this great idea.

Kemeng Shi

Re: [PATCH 1/4] mm, swap: Fix potential NULL dereference when trying a sleep table allocation

[Kairui Song]

On Mon, Jul 20, 2026 at 03:13:39PM +0800, Kemeng Shi wrote:
> The root cause of this issue is because multi-tables are updated in non
> atomic context. To be more specific, the issue could be triggerred as
> following:
> 
> swap_alloc_fast         swap_cluster_populate()
>                          /* Try a sleep allocation */
>                          spin_unlock(&ci->lock);
>                          swap_cluster_alloc_table()
>                           rcu_assign_pointer(ci->table, table);
> 
>  ci = swap_cluster_lock(si, offset)
>  cluster_is_usable(ci, order）
>   if (!cluster_table_is_alloced(ci)) // ok
>  alloc_swap_scan_cluster()
>   cluster_scan_range()
>    __swap_table_get()
> 
>                           /* free table when more table allocation fails */
>                           ci->memcg_table = kzalloc_obj(*ci->memcg_table,
>                                                         gfp);
>                           if (!ci->memcg_table)
>                            swap_cluster_free_table()
>                             rcu_assign_pointer(ci->table, NULL);
> 
>     table = rcu_dereference_check(ci->table, lockdep_is_held(&ci->lock));
>      atomic_long_read(&table[off]); // NULL dereference
> 
> Fix the issue by updating allocated tables in atomic context.

Thanks, that's a really tricky one, good catch.

> 
> Fixes: 2fe7a6f5024b8 ("mm/memcg, swap: store cgroup id in cluster table directly")

This commit doesn't exist, you mean b197d41462c2 right?

> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  mm/swapfile.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 615d90867111..d29062d9c3cd 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -490,6 +490,20 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
>  	return 0;
>  }
>  
> +static void swap_cluster_copy_table(struct swap_cluster_info *d_ci,
> +				    struct swap_cluster_info *s_ci)

Nit: The name is a bit misleading, it not copying the table content,
just filled the d_ci with s_ci. And do we really need a helper for this?

> +{
> +	rcu_assign_pointer(d_ci->table, rcu_access_pointer(s_ci->table));
> +
> +#ifdef CONFIG_MEMCG
> +	d_ci->memcg_table = s_ci->memcg_table;
> +#endif
> +
> +#if !SWAP_TABLE_HAS_ZEROFLAG
> +	d_ci->zero_bitmap = s_ci->zero_bitmap;
> +#endif
> +}
> +
>  /*
>   * Sanity check to ensure nothing leaked, and the specified range is empty.
>   * One special case is that bad slots can't be freed, so check the number of
> @@ -527,6 +541,7 @@ static struct swap_cluster_info *
>  swap_cluster_populate(struct swap_info_struct *si,
>  			 struct swap_cluster_info *ci)
>  {
> +	struct swap_cluster_info tmp_ci;
>  	int ret;
>  
>  	/*
> @@ -552,7 +567,8 @@ swap_cluster_populate(struct swap_info_struct *si,
>  		spin_unlock(&si->global_cluster_lock);
>  	local_unlock(&percpu_swap_cluster.lock);
>  
> -	ret = swap_cluster_alloc_table(ci, __GFP_HIGH | __GFP_NOMEMALLOC |
> +	tmp_ci = *ci;
> +	ret = swap_cluster_alloc_table(&tmp_ci, __GFP_HIGH | __GFP_NOMEMALLOC |
>  					   GFP_KERNEL);

This allocation could be largely simplified with mempool, the reason I didn't
use that is due to lack of RCU support, which can be done later to clean
this up.

Please fix the Fixes tags, other parts are mostly fine, thanks agian!

Re: [PATCH 1/4] mm, swap: Fix potential NULL dereference when trying a sleep table allocation

[Kemeng Shi]

在 2026/7/20 17:11:58, Kairui Song 写道:
> On Mon, Jul 20, 2026 at 03:13:39PM +0800, Kemeng Shi wrote:
>> The root cause of this issue is because multi-tables are updated in non
>> atomic context. To be more specific, the issue could be triggerred as
>> following:
>>
>> swap_alloc_fast         swap_cluster_populate()
>>                          /* Try a sleep allocation */
>>                          spin_unlock(&ci->lock);
>>                          swap_cluster_alloc_table()
>>                           rcu_assign_pointer(ci->table, table);
>>
>>  ci = swap_cluster_lock(si, offset)
>>  cluster_is_usable(ci, order）
>>   if (!cluster_table_is_alloced(ci)) // ok
>>  alloc_swap_scan_cluster()
>>   cluster_scan_range()
>>    __swap_table_get()
>>
>>                           /* free table when more table allocation fails */
>>                           ci->memcg_table = kzalloc_obj(*ci->memcg_table,
>>                                                         gfp);
>>                           if (!ci->memcg_table)
>>                            swap_cluster_free_table()
>>                             rcu_assign_pointer(ci->table, NULL);
>>
>>     table = rcu_dereference_check(ci->table, lockdep_is_held(&ci->lock));
>>      atomic_long_read(&table[off]); // NULL dereference
>>
>> Fix the issue by updating allocated tables in atomic context.
> 
> Thanks, that's a really tricky one, good catch.
> 
>>
>> Fixes: 2fe7a6f5024b8 ("mm/memcg, swap: store cgroup id in cluster table directly")
> 
> This commit doesn't exist, you mean b197d41462c2 right?
The commit 2fe7a6f5024b8 and b197d41462c2 both exist in this tree:
https://kernel.googlesource.com/pub/scm/linux/kernel/git/akpm/mm.git.
I search in another tree and only your commit exsits, so I believe
your commit is correct. Will fix this in next version. Thanks for
correction.

> 
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>> ---
>>  mm/swapfile.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 615d90867111..d29062d9c3cd 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -490,6 +490,20 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
>>  	return 0;
>>  }
>>  
>> +static void swap_cluster_copy_table(struct swap_cluster_info *d_ci,
>> +				    struct swap_cluster_info *s_ci)
> 
> Nit: The name is a bit misleading, it not copying the table content,
> just filled the d_ci with s_ci. And do we really need a helper for this?
As swap_cluster_alloc_table() grows from only table allocation to table,
memcg_table and zero_bitmap allocation. So I think maybe a helper may
inform us not to forget to do copy when new member is added. But I also
think about open-coding it. So both ways look good to me. I will open-code
it if you prefer it. :)> 
>> +{
>> +	rcu_assign_pointer(d_ci->table, rcu_access_pointer(s_ci->table));
>> +
>> +#ifdef CONFIG_MEMCG
>> +	d_ci->memcg_table = s_ci->memcg_table;
>> +#endif
>> +
>> +#if !SWAP_TABLE_HAS_ZEROFLAG
>> +	d_ci->zero_bitmap = s_ci->zero_bitmap;
>> +#endif
>> +}
>> +
>>  /*
>>   * Sanity check to ensure nothing leaked, and the specified range is empty.
>>   * One special case is that bad slots can't be freed, so check the number of
>> @@ -527,6 +541,7 @@ static struct swap_cluster_info *
>>  swap_cluster_populate(struct swap_info_struct *si,
>>  			 struct swap_cluster_info *ci)
>>  {
>> +	struct swap_cluster_info tmp_ci;
>>  	int ret;
>>  
>>  	/*
>> @@ -552,7 +567,8 @@ swap_cluster_populate(struct swap_info_struct *si,
>>  		spin_unlock(&si->global_cluster_lock);
>>  	local_unlock(&percpu_swap_cluster.lock);
>>  
>> -	ret = swap_cluster_alloc_table(ci, __GFP_HIGH | __GFP_NOMEMALLOC |
>> +	tmp_ci = *ci;
>> +	ret = swap_cluster_alloc_table(&tmp_ci, __GFP_HIGH | __GFP_NOMEMALLOC |
>>  					   GFP_KERNEL);
> 
> This allocation could be largely simplified with mempool, the reason I didn't
> use that is due to lack of RCU support, which can be done later to clean
> this up.
> 
> Please fix the Fixes tags, other parts are mostly fine, thanks agian!

[PATCH 2/4] mm, swap: Move setup_swap_clusters_info() after SWP_SOLIDSTATE initialization

[Kemeng Shi]

In setup_swap_clusters_info(), SWP_SOLIDSTATE is used to decide
global_cluster allocation. Move setup_swap_clusters_info() after
SWP_SOLIDSTATE initialization to avoid unneeded global_cluster
allocation.

Fixes: 451c6326105b2 ("mm, swap: clean up swapon process and locking")
Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/swapfile.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index d29062d9c3cd..81c4040912be 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3607,11 +3607,6 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 
 	maxpages = si->max;
 
-	/* Set up the swap cluster info */
-	error = setup_swap_clusters_info(si, swap_header, maxpages);
-	if (error)
-		goto bad_swap_unlock_inode;
-
 	if (si->bdev && bdev_stable_writes(si->bdev))
 		si->flags |= SWP_STABLE_WRITES;
 
@@ -3625,6 +3620,14 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 		inced_nr_rotate_swap = true;
 	}
 
+	/*
+	 * Set up the swap cluster info. SWP_SOLIDSTATE is used for
+	 * global_cluster allocation
+	 */
+	error = setup_swap_clusters_info(si, swap_header, maxpages);
+	if (error)
+		goto bad_swap_unlock_inode;
+
 	if ((swap_flags & SWAP_FLAG_DISCARD) &&
 	    si->bdev && bdev_max_discard_sectors(si->bdev)) {
 		/*
-- 
2.36.1

Re: [PATCH 2/4] mm, swap: Move setup_swap_clusters_info() after SWP_SOLIDSTATE initialization

[Luiz Capitulino]

On 2026-07-20 03:13, Kemeng Shi wrote:
> In setup_swap_clusters_info(), SWP_SOLIDSTATE is used to decide
> global_cluster allocation. Move setup_swap_clusters_info() after
> SWP_SOLIDSTATE initialization to avoid unneeded global_cluster
> allocation.
> 
> Fixes: 451c6326105b2 ("mm, swap: clean up swapon process and locking")
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

I was about to send the same fix :) (one nit below)

> ---
>   mm/swapfile.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index d29062d9c3cd..81c4040912be 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3607,11 +3607,6 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>   
>   	maxpages = si->max;
>   
> -	/* Set up the swap cluster info */
> -	error = setup_swap_clusters_info(si, swap_header, maxpages);
> -	if (error)
> -		goto bad_swap_unlock_inode;
> -
>   	if (si->bdev && bdev_stable_writes(si->bdev))
>   		si->flags |= SWP_STABLE_WRITES;
>   
> @@ -3625,6 +3620,14 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>   		inced_nr_rotate_swap = true;
>   	}
>   
> +	/*
> +	 * Set up the swap cluster info. SWP_SOLIDSTATE is used for
> +	 * global_cluster allocation
> +	 */

Maybe something like:

/*
  * Set up the swap cluster info after SWP_ flags handling as
  * setup_swap_clusters_info() checks SWP_SOLIDSTATE.
  */

As it's minor:

Reviewed-by: Luiz Capitulino <luizcap@redhat.com>

> +	error = setup_swap_clusters_info(si, swap_header, maxpages);
> +	if (error)
> +		goto bad_swap_unlock_inode;
> +
>   	if ((swap_flags & SWAP_FLAG_DISCARD) &&
>   	    si->bdev && bdev_max_discard_sectors(si->bdev)) {
>   		/*

Re: [PATCH 2/4] mm, swap: Move setup_swap_clusters_info() after SWP_SOLIDSTATE initialization

[Kemeng Shi]

在 2026/7/20 23:54:07, Luiz Capitulino 写道:
> On 2026-07-20 03:13, Kemeng Shi wrote:
>> In setup_swap_clusters_info(), SWP_SOLIDSTATE is used to decide
>> global_cluster allocation. Move setup_swap_clusters_info() after
>> SWP_SOLIDSTATE initialization to avoid unneeded global_cluster
>> allocation.
>>
>> Fixes: 451c6326105b2 ("mm, swap: clean up swapon process and locking")
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> 
> I was about to send the same fix :) (one nit below)
What a coincidence :)> 
>> ---
>>   mm/swapfile.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index d29062d9c3cd..81c4040912be 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -3607,11 +3607,6 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>>         maxpages = si->max;
>>   -    /* Set up the swap cluster info */
>> -    error = setup_swap_clusters_info(si, swap_header, maxpages);
>> -    if (error)
>> -        goto bad_swap_unlock_inode;
>> -
>>       if (si->bdev && bdev_stable_writes(si->bdev))
>>           si->flags |= SWP_STABLE_WRITES;
>>   @@ -3625,6 +3620,14 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>>           inced_nr_rotate_swap = true;
>>       }
>>   +    /*
>> +     * Set up the swap cluster info. SWP_SOLIDSTATE is used for
>> +     * global_cluster allocation
>> +     */
> 
> Maybe something like:
> 
> /*
>  * Set up the swap cluster info after SWP_ flags handling as
>  * setup_swap_clusters_info() checks SWP_SOLIDSTATE.
>  */
Thanks for feedback. I will do this in next version.> 
> As it's minor:
> 
> Reviewed-by: Luiz Capitulino <luizcap@redhat.com>
> 
>> +    error = setup_swap_clusters_info(si, swap_header, maxpages);
>> +    if (error)
>> +        goto bad_swap_unlock_inode;
>> +
>>       if ((swap_flags & SWAP_FLAG_DISCARD) &&
>>           si->bdev && bdev_max_discard_sectors(si->bdev)) {
>>           /*
> 

[PATCH 3/4] mm, swap: return early from swap_extend_table_try_free() on first non-zero entry

[Kemeng Shi]

Return immediately when the first non-zero swap count is found as
any non-zero swap count prevents freeing extend_table and further
iteration is pointless.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/swapfile.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 81c4040912be..3fcbd10c6353 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1530,20 +1530,17 @@ int swap_retry_table_alloc(swp_entry_t entry, gfp_t gfp)
 static void swap_extend_table_try_free(struct swap_cluster_info *ci)
 {
 	unsigned long i;
-	bool can_free = true;
 
 	if (!ci->extend_table)
 		return;
 
 	for (i = 0; i < SWAPFILE_CLUSTER; i++) {
 		if (ci->extend_table[i])
-			can_free = false;
+			return;
 	}
 
-	if (can_free) {
-		kfree(ci->extend_table);
-		ci->extend_table = NULL;
-	}
+	kfree(ci->extend_table);
+	ci->extend_table = NULL;
 }
 
 /* Decrease the swap count of one slot, without freeing it */
-- 
2.36.1

Re: [PATCH 3/4] mm, swap: return early from swap_extend_table_try_free() on first non-zero entry

[Youngjun Park]

On Mon, Jul 20, 2026 at 03:13:41PM +0800, Kemeng Shi wrote:
> Return immediately when the first non-zero swap count is found as
> any non-zero swap count prevents freeing extend_table and further
> iteration is pointless.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  mm/swapfile.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 81c4040912be..3fcbd10c6353 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1530,20 +1530,17 @@ int swap_retry_table_alloc(swp_entry_t entry, gfp_t gfp)
>  static void swap_extend_table_try_free(struct swap_cluster_info *ci)
>  {
>  	unsigned long i;
> -	bool can_free = true;
>  
>  	if (!ci->extend_table)
>  		return;
>  
>  	for (i = 0; i < SWAPFILE_CLUSTER; i++) {
>  		if (ci->extend_table[i])
> -			can_free = false;
> +			return;
>  	}
>  
> -	if (can_free) {
> -		kfree(ci->extend_table);
> -		ci->extend_table = NULL;
> -	}
> +	kfree(ci->extend_table);
> +	ci->extend_table = NULL;
>  }
>  
>  /* Decrease the swap count of one slot, without freeing it */
> -- 
> 2.36.1
> 

LGTM :)
Thanks

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

Youngjun

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

[Kemeng Shi]

Since commit 0475fde0f68de ("mm, swap: avoid leaving unused extend table
after alloc race"), extend table is always allocated when any swap count
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@huaweicloud.com>
---
 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);
-	swap_extend_table_try_free(ci);
 	swap_cluster_unlock(ci);
 	return err;
 }
-- 
2.36.1

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@huaweicloud.com>
> ---
>  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