[PATCH 2/2] mm/swap: add lockdep for si->global_cluster_lock in swap_cluster_alloc_table()

Hui Zhu posted 2 patches 1 month ago
[PATCH 2/2] mm/swap: add lockdep for si->global_cluster_lock in swap_cluster_alloc_table()
Posted by Hui Zhu 1 month ago
From: Hui Zhu <zhuhui@kylinos.cn>

Add a lockdep_assert_held(&si->global_cluster_lock) in
swap_cluster_alloc_table() for non-SWP_SOLIDSTATE devices.

The function already requires the caller to hold both ci->lock
and percpu_swap_cluster.lock.
And it also necessitates si->global_cluster_lock when the device is not
SWP_SOLIDSTATE.
Adding this assertion ensures locking consistency and helps catch
potential synchronization issues during development.

Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
---
 mm/swapfile.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 2e8717f84ba3..1400a1585033 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -477,6 +477,8 @@ swap_cluster_alloc_table(struct swap_info_struct *si,
 	 * Swap allocator uses percpu clusters and holds the local lock.
 	 */
 	lockdep_assert_held(&ci->lock);
+	if (!(si->flags & SWP_SOLIDSTATE))
+		lockdep_assert_held(&si->global_cluster_lock);
 	lockdep_assert_held(&this_cpu_ptr(&percpu_swap_cluster)->lock);
 
 	/* The cluster must be free and was just isolated from the free list. */
-- 
2.43.0
Re: [PATCH 2/2] mm/swap: add lockdep for si->global_cluster_lock in swap_cluster_alloc_table()
Posted by YoungJun Park 1 month ago
On Fri, Mar 06, 2026 at 07:50:37PM +0800, Hui Zhu wrote:
> From: Hui Zhu <zhuhui@kylinos.cn>
> 
> Add a lockdep_assert_held(&si->global_cluster_lock) in
> swap_cluster_alloc_table() for non-SWP_SOLIDSTATE devices.
> 
> The function already requires the caller to hold both ci->lock
> and percpu_swap_cluster.lock.
> And it also necessitates si->global_cluster_lock when the device is not
> SWP_SOLIDSTATE.
> Adding this assertion ensures locking consistency and helps catch
> potential synchronization issues during development.
> 
> Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
> ---
>  mm/swapfile.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 2e8717f84ba3..1400a1585033 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -477,6 +477,8 @@ swap_cluster_alloc_table(struct swap_info_struct *si,
>  	 * Swap allocator uses percpu clusters and holds the local lock.
>  	 */
>  	lockdep_assert_held(&ci->lock);
> +	if (!(si->flags & SWP_SOLIDSTATE))
> +		lockdep_assert_held(&si->global_cluster_lock);
>  	lockdep_assert_held(&this_cpu_ptr(&percpu_swap_cluster)->lock);

The addition looks fine to me.

If others agree, one minor suggestion: it might be slightly cleaner to
order the lockdep_assert_held() calls to match the actual lock
acquisition order:

        lockdep_assert_held(&this_cpu_ptr(&percpu_swap_cluster)->lock);
        if (!(si->flags & SWP_SOLIDSTATE))
                lockdep_assert_held(&si->global_cluster_lock);
        lockdep_assert_held(&ci->lock);

No strong opinion on this, just a thought. :)

Best regards,
Youngjun Park
>  	/* The cluster must be free and was just isolated from the free list. */
> -- 
> 2.43.0
> 
>