Hi Andrew
On 4/5/23 3:26 AM, Andrew Morton wrote:
On Tue, 4 Apr 2023 23:47:16 +0800 Rongwei Wang <rongwei.wang@xxxxxxxxxxxxxxxxx> wrote:
The si->lock must be held when deleting the si from
the available list.
...
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -679,6 +679,7 @@ static void __del_from_avail_list(struct swap_info_struct *p)
{
int nid;
+ assert_spin_locked(&p->lock);
for_each_node(nid)
plist_del(&p->avail_lists[nid], &swap_avail_heads[nid]);
}
@@ -2434,8 +2435,8 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
spin_unlock(&swap_lock);
goto out_dput;
}
- del_from_avail_list(p);
spin_lock(&p->lock);
+ del_from_avail_list(p);
if (p->prio < 0) {
struct swap_info_struct *si = p;
int nid;
So we have
swap_avail_lock
swap_info_struct.lock
swap_cluster_info.lock
Is the ranking of these three clearly documented somewhere?
It seems have
swap_lock
swap_info_struct.lock
swap_avail_lock
I just summary the ranking of these three locks by reading code, not
find any documents (maybe have).
Did you test this with lockdep fully enabled?
I'm thinking that Aaron's a2468cc9bfdff ("swap: choose swap device
according to numa node") is the appropriate Fixes: target - do you
agree?
Yes, I'm sure my latest test version has included Aaron's a2468cc9bfdff,
and my test .config has enabled CONFIG
as below:
CONFIG_LOCK_DEBUGGING_SUPPORT=y CONFIG_PROVE_LOCKING=y
CONFIG_DEBUG_SPINLOCK=y CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_LOCKDEP=y
CONFIG_DEBUG_LOCKDEP=y CONFIG_DEBUG_ATOMIC_SLEEP=y
These functions use identifier `p' for the swap_info_struct*, whereas
most other code uses the much more sensible `si'. That's just rude.
But we shouldn't change that within this fix.
Indeed, It's confusing more or less to use both 'si' and 'p'. I can
ready for another patch to replace 'p' with 'si'.
Thanks.
