On 4/3/23 12:10 PM, Matthew Wilcox wrote:

On Sun, Apr 02, 2023 at 06:19:20AM +0800, Rongwei Wang wrote:

...

Without this modification, a core will wait (mostly) 'swap_info_struct->lock' when completing 'del_from_avail_list(p)'. Immediately, other cores soon calling 'add_to_avail_list()' to add the same object again when acquiring the lock that released by former. It's not the desired result but exists indeed. This case can be described as below:

This feels like a very verbose way of saying

"The si->lock must be held when deleting the si from the available list. Otherwise, another thread can re-add the si to the available list, which can lead to memory corruption. The only place we have found where this happens is in the swapoff path."

It looks better than mine. Sorry for my confusing description, it will be fixed in the next version.

...

+++ b/mm/swapfile.c @@ -2610,8 +2610,12 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile) spin_unlock(&swap_lock); goto out_dput; }

• del_from_avail_list(p);

• /*

• * Here lock is used to protect deleting and SWP_WRITEOK clearing
  

• * can be seen concurrently.
  

• */
  

This comment isn't necessary. But I would add a lockdep assert inside __del_from_avail_list() that p->lock is held.

Thanks. Actually, I have this line in previous test version, but delete for saving one line of code.

I will update here as you said.

Thanks for your time.

...

spin_lock(&p->lock);

• del_from_avail_list(p); if (p->prio < 0) { struct swap_info_struct *si = p; int nid;

-- 2.27.0