Re: swap_cluster_info lockdep splat

["Huang\, Ying" <ying.huang@intel.com>]

Hi, Minchan,

Minchan Kim <minchan@kernel.org> writes:

> 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 <ying.huang@intel.com>
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" <ying.huang@intel.com>
---
 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