Re: [PATCH v4] mm/swap: strengthen locking assertions and invariants in cluster allocation

[hui.zhu@linux.dev]

2026年3月12日 01:34, "Chris Li" <chrisl@kernel.org mailto:chrisl@kernel.org?to=%22Chris%20Li%22%20%3Cchrisl%40kernel.org%3E > 写到:


> 
> On Tue, Mar 10, 2026 at 7:23 PM Hui Zhu <hui.zhu@linux.dev> wrote:
> 
> > 
> > From: Hui Zhu <zhuhui@kylinos.cn>
> > 
> >  The swap_cluster_alloc_table() function requires several locks to be held
> >  by its callers: ci->lock, the per-CPU swap_cluster lock, and, for
> >  non-solid-state devices (non-SWP_SOLIDSTATE), the si->global_cluster_lock.
> > 
> >  While most call paths (e.g., via cluster_alloc_swap_entry() or
> >  alloc_swap_scan_list()) correctly acquire these locks before invocation,
> >  the path through swap_reclaim_work() -> swap_reclaim_full_clusters() ->
> >  isolate_lock_cluster() is distinct. This path operates exclusively on
> >  si->full_clusters, where the swap allocation tables are guaranteed to be
> >  already allocated. Consequently, isolate_lock_cluster() should never
> >  trigger a call to swap_cluster_alloc_table() for these clusters.
> > 
> >  Strengthen the locking and state assertions to formalize these invariants:
> > 
> >  1. Add a lockdep_assert_held() for si->global_cluster_lock in
> >  swap_cluster_alloc_table() for non-SWP_SOLIDSTATE devices.
> >  2. Reorder existing lockdep assertions in swap_cluster_alloc_table() to
> >  match the actual lock acquisition order (per-CPU lock, then global lock,
> >  then cluster lock).
> >  3. Add a VM_WARN_ON_ONCE() in isolate_lock_cluster() to ensure that table
> >  allocations are only attempted for clusters being isolated from the
> >  free list. Attempting to allocate a table for a cluster from other
> >  lists (like the full list during reclaim) indicates a violation of
> >  subsystem invariants.
> > 
> >  These changes ensure locking consistency and help catch potential
> >  synchronization or logic issues during development.
> > 
> >  Changelog:
> >  v4:
> >  According to the comments of Barry Song, remove redundant comment.
> >  v3:
> >  According to the comments of Kairui Song, squash patches and fix logic
> >  bug in isolate_lock_cluster() where flags were cleared before check.
> >  v2:
> >  According to the comments of YoungJun Park, Kairui Song and Chris Li,
> >  change acquire locks in swap_reclaim_work() to adds a VM_WARN_ON in
> >  isolate_lock_cluster().
> >  According to the comments of YoungJun Park, add code in patch 2 to Change
> >  the order of lockdep_assert_held() to match the actual lock acquisition
> >  order.
> > 
> >  Reviewed-by: Youngjun Park <youngjun.park@lge.com>
> >  Reviewed-by: Barry Song <baohua@kernel.org>
> >  Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
> > 
> Acked-by: Chris Li <chrisl@kernel.org>
> 
> > 
> > ---
> >  mm/swapfile.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> >  diff --git a/mm/swapfile.c b/mm/swapfile.c
> >  index 94af29d1de88..e25cdb0046d8 100644
> >  --- a/mm/swapfile.c
> >  +++ b/mm/swapfile.c
> >  @@ -476,8 +476,10 @@ swap_cluster_alloc_table(struct swap_info_struct *si,
> >  * Only cluster isolation from the allocator does table allocation.
> >  * Swap allocator uses percpu clusters and holds the local lock.
> >  */
> >  - lockdep_assert_held(&ci->lock);
> >  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);
> > 
> >  /* The cluster must be free and was just isolated from the free list. */
> >  VM_WARN_ON_ONCE(ci->flags || !cluster_is_empty(ci));
> >  @@ -577,6 +579,7 @@ static struct swap_cluster_info *isolate_lock_cluster(
> >  struct swap_info_struct *si, struct list_head *list)
> >  {
> >  struct swap_cluster_info *ci, *found = NULL;
> >  + u8 flags;
> > 
> Nit pick: consider initializing the value. The flags assignment occurs
> in a conditional block. The compiler might or might not realize the
> "flags" assigned only if "found" is also assigned, and might complain
> that flags can be used without initialization.
> 
> > 
> > spin_lock(&si->lock);
> >  list_for_each_entry(ci, list, list) {
> >  @@ -589,6 +592,7 @@ static struct swap_cluster_info *isolate_lock_cluster(
> >  ci->flags != CLUSTER_FLAG_FULL);
> > 
> >  list_del(&ci->list);
> >  + flags = ci->flags;
> > 
> If VM debug is disabled, this variable is not used after its value is
> assigned. Please test it with gcc and llvm (VM debug disabled) to
> ensure it doesn't generate any warnings. I don't expect it to be, I
> just want to make sure.

After adding the initialization code, I turned off VM_DEBUG and compiled
it with both clang18 and gcc13. No warnings during compilation.

Best,
Hui

> 
> Chris
> 
> > 
> > ci->flags = CLUSTER_FLAG_NONE;
> >  found = ci;
> >  break;
> >  @@ -597,6 +601,7 @@ static struct swap_cluster_info *isolate_lock_cluster(
> > 
> >  if (found && !cluster_table_is_alloced(found)) {
> >  /* Only an empty free cluster's swap table can be freed. */
> >  + VM_WARN_ON_ONCE(flags != CLUSTER_FLAG_FREE);
> >  VM_WARN_ON_ONCE(list != &si->free_clusters);
> >  VM_WARN_ON_ONCE(!cluster_is_empty(found));
> >  return swap_cluster_alloc_table(si, found);
> >  --
> >  2.43.0
> >
>