Re: swap_cluster_info lockdep splat

Hugh Dickins [off-list ref] writes:

On Thu, 16 Feb 2017, Huang, Ying wrote:

quoted

Hi, Minchan,

Minchan Kim [off-list ref] writes:

quoted

Hi Huang,

With changing from bit lock to spinlock of swap_cluster_info, my zram
test failed with below message. It seems nested lock problem so need to
play with lockdep.

Sorry, I could not reproduce the warning in my tests.  Could you try the
patches as below?   And could you share your test case?

Best Regards,
Huang, Ying

------------------------------------------------------------->
From 2b9e2f78a6e389442f308c4f9e8d5ac40fe6aa2f Mon Sep 17 00:00:00 2001
From: Huang Ying <redacted>
Date: Thu, 16 Feb 2017 16:38:17 +0800
Subject: [PATCH] mm, swap: Annotate nested locking for cluster lock

There is a nested locking in cluster_list_add_tail() for cluster lock,
which caused lockdep to complain as below.  The nested locking is safe
because both cluster locks are only acquired when we held the
swap_info_struct->lock.  Annotated the nested locking via
spin_lock_nested() to fix the complain of lockdep.

=============================================
[ INFO: possible recursive locking detected ]
4.10.0-rc8-next-20170214-zram #24 Not tainted
---------------------------------------------
as/6557 is trying to acquire lock:
 (&(&((cluster_info + ci)->lock))->rlock){+.+.-.}, at: [<ffffffff811ddd03>] cluster_list_add_tail.part.31+0x33/0x70

but task is already holding lock:
 (&(&((cluster_info + ci)->lock))->rlock){+.+.-.}, at: [<ffffffff811df2bb>] swapcache_free_entries+0x9b/0x330

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&(&((cluster_info + ci)->lock))->rlock);
  lock(&(&((cluster_info + ci)->lock))->rlock);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

3 locks held by as/6557:
 #0:  (&(&cache->free_lock)->rlock){......}, at: [<ffffffff811c206b>] free_swap_slot+0x8b/0x110
 #1:  (&(&p->lock)->rlock){+.+.-.}, at: [<ffffffff811df295>] swapcache_free_entries+0x75/0x330
 #2:  (&(&((cluster_info + ci)->lock))->rlock){+.+.-.}, at: [<ffffffff811df2bb>] swapcache_free_entries+0x9b/0x330

stack backtrace:
CPU: 3 PID: 6557 Comm: as Not tainted 4.10.0-rc8-next-20170214-zram #24
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
Call Trace:
 dump_stack+0x85/0xc2
 __lock_acquire+0x15ea/0x1640
 lock_acquire+0x100/0x1f0
 ? cluster_list_add_tail.part.31+0x33/0x70
 _raw_spin_lock+0x38/0x50
 ? cluster_list_add_tail.part.31+0x33/0x70
 cluster_list_add_tail.part.31+0x33/0x70
 swapcache_free_entries+0x2f9/0x330
 free_swap_slot+0xf8/0x110
 swapcache_free+0x36/0x40
 delete_from_swap_cache+0x5f/0xa0
 try_to_free_swap+0x6e/0xa0
 free_pages_and_swap_cache+0x7d/0xb0
 tlb_flush_mmu_free+0x36/0x60
 tlb_finish_mmu+0x1c/0x50
 exit_mmap+0xc7/0x150
 mmput+0x51/0x110
 do_exit+0x2b2/0xc30
 ? trace_hardirqs_on_caller+0x129/0x1b0
 do_group_exit+0x50/0xd0
 SyS_exit_group+0x14/0x20
 entry_SYSCALL_64_fastpath+0x23/0xc6
RIP: 0033:0x2b9a2dbdf309
RSP: 002b:00007ffe71887528 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00002b9a2dbdf309
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 00002b9a2ded8858 R08: 000000000000003c R09: 00000000000000e7
R10: ffffffffffffff60 R11: 0000000000000246 R12: 00002b9a2ded8858
R13: 00002b9a2dedde80 R14: 000000000255f770 R15: 0000000000000001

Reported-by: Minchan Kim <minchan@kernel.org>
Signed-off-by: "Huang, Ying" <redacted>
---
 include/linux/swap.h | 6 ++++++
 mm/swapfile.c        | 8 +++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index 4d12b381821f..ef044ea8fe79 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h

@@ -166,6 +166,12 @@ enum {
 #define COUNT_CONTINUED	0x80	/* See swap_map continuation for full count */
 #define SWAP_MAP_SHMEM	0xbf	/* Owned by shmem/tmpfs, in first swap_map */
 
+enum swap_cluster_lock_class
+{
+	SWAP_CLUSTER_LOCK_NORMAL,  /* implicitly used by plain spin_lock() APIs. */
+	SWAP_CLUSTER_LOCK_NESTED,
+};
+
 /*
  * We use this to track usage of a cluster. A cluster is a block of swap disk
  * space with SWAPFILE_CLUSTER pages long and naturally aligns in disk. All

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 5ac2cb40dbd3..0a52e9b2f843 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c

@@ -263,6 +263,12 @@ static inline void __lock_cluster(struct swap_cluster_info *ci)
 	spin_lock(&ci->lock);
 }
 
+static inline void __lock_cluster_nested(struct swap_cluster_info *ci,
+					 unsigned subclass)
+{
+	spin_lock_nested(&ci->lock, subclass);
+}
+
 static inline struct swap_cluster_info *lock_cluster(struct swap_info_struct *si,
 						     unsigned long offset)
 {

@@ -336,7 +342,7 @@ static void cluster_list_add_tail(struct swap_cluster_list *list,
 		 * only acquired when we held swap_info_struct->lock
 		 */
 		ci_tail = ci + tail;
-		__lock_cluster(ci_tail);
+		__lock_cluster_nested(ci_tail, SWAP_CLUSTER_LOCK_NESTED);
 		cluster_set_next(ci_tail, idx);
 		unlock_cluster(ci_tail);
 		cluster_set_next_flag(&list->tail, idx, 0);

-- 
2.11.0

I do not understand your zest for putting wrappers around every little
thing, making it all harder to follow than it need be.  Here's the patch
I've been running with (but you have a leak somewhere, and I don't have
time to search out and fix it: please try sustained swapping and swapoff).

Thanks for your patch.  cluster_lock is bit_spinlock before, the wrapper
made it easier to be converted to normal spinlock.  But especially after
splitting the function into 2 variants, the wrapper looks pure
redundant.  Thanks for fixing that too.

Best Regards,
Huang, Ying

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>