Skip to contentSkip to conversation

various lockdep patches

PATCH In review Peter Zijlstra 4 participants 21 messages Sep 28, 2007 – Oct 3, 2007

Subsystems (18)
Memory management VFS NFS client MM core +14 more subsystems
lore
Share & tools
lore

Peter Zijlstra

author
#0 lore raw

cover various lockdep patches

The first 3 patches provide a new lockdep check, it verifies we don't hold any locks when returning to userspace.

	lockdep-sys_exit.patch
	lockdep-i386-sys_exit.patch
	lockdep-x86_64-sys_exit.patch

Could the various arch maintainers that have support for lockdep help out with placing this hook?

A journal_start annotation:

jbd-lock.patch

Up until this point stuff should be safe to merge (assuming there are no objections and bugs.. :-)

The rest of the series annotates lock_page():

rework lock_page() API:
trylock_page.patch
lock_page.patch
lockdep-page_lock-hooks.patch

actuall lockdep annotation:
lockdep-depth.patch
lockdep-fs-page.patch
lockdep-page-async.patch
set_page_mapping.patch
lockdep-lock_page.patch

Part of that is re-working the lock_page() API a bit. It introduces trylock_page(), and removes all the raw *PageLock() usage.

I've compile and boot tested this series on x86_64 (with and without lockdep). And it completes an LTP run with lockdep enabled.

Andrew, can we start by pusing the lock_page() API rework into -mm?

[ this series is against -linus, but if you're willing I'll do some
patches against -mm ]

author
#1 lore raw

PATCH 1/12 lockdep: syscall exit check

Provide a check to validate that we do not hold any locks when switching back to user-space.

Signed-off-by Peter Zijlstra <[email protected]>
---
 include/linux/lockdep.h |    2 ++
 kernel/lockdep.c        |   16 ++++++++++++++++
 2 files changed, 18 insertions(+)
include/linux/lockdep.h +2 -0
Index: linux-2.6/include/linux/lockdep.h
===================================================================
@@ -238,6 +238,7 @@ extern void lockdep_info(void);
 extern void lockdep_reset(void);
 extern void lockdep_reset_lock(struct lockdep_map *lock);
 extern void lockdep_free_key_range(void *start, unsigned long size);
+extern void lockdep_sys_exit(void);
 
 extern void lockdep_off(void);
 extern void lockdep_on(void);

@@ -317,6 +318,7 @@ static inline void lockdep_on(void)
 # define INIT_LOCKDEP
 # define lockdep_reset()		do { debug_locks = 1; } while (0)
 # define lockdep_free_key_range(start, size)	do { } while (0)
+# define lockdep_sys_exit() 			do { } while (0)
 /*
  * The class key takes no space if lockdep is disabled:
  */
kernel/lockdep.c +16 -0
Index: linux-2.6/kernel/lockdep.c
===================================================================
@@ -3199,3 +3199,19 @@ void debug_show_held_locks(struct task_s
 }
 
 EXPORT_SYMBOL_GPL(debug_show_held_locks);
+
+void lockdep_sys_exit(void)
+{
+	struct task_struct *curr = current;
+
+	if (unlikely(curr->lockdep_depth)) {
+		if (!debug_locks_off())
+			return;
+		printk("\n========================================\n");
+		printk(  "[ BUG: lock held at syscall exit time! ]\n");
+		printk(  "----------------------------------------\n");
+		printk("%s/%d is leaving the kernel with locks still held!\n",
+				curr->comm, curr->pid);
+		lockdep_print_held_locks(curr);
+	}
+}

--

#2 lore raw

Re: [PATCH 01/12] lockdep: syscall exit check

Quoted 5 lines
kernel/lockdep.c +5 -0
+void lockdep_sys_exit(void)
+{
+	struct task_struct *curr = current;
+
+	if (unlikely(curr->lockdep_depth)) {

kernel/lockdep.c void debug_show_held_locks(struct task_s · line 3217

Quoted 10 lines
kernel/lockdep.c +10 -0
+		if (!debug_locks_off())
+			return;
+		printk("\n========================================\n");
+		printk(  "[ BUG: lock held at syscall exit time! ]\n");
+		printk(  "----------------------------------------\n");
+		printk("%s/%d is leaving the kernel with locks still held!\n",
+				curr->comm, curr->pid);
+		lockdep_print_held_locks(curr);
+	}
+}

By the way, the s390 patch I just posted also checks if we hold any locks when returning from interrupt context to user space. Maybe the above text could be changed to "lock held when returning to user space" ?

author
#3 lore raw

Re: [PATCH 01/12] lockdep: syscall exit check

Quoted 11 lines · Heiko Carstens
Quoted 11 lines
(quoted diff) +11 -0
+void lockdep_sys_exit(void)
+{
+	struct task_struct *curr = current;
+
+	if (unlikely(curr->lockdep_depth)) {
+		if (!debug_locks_off())
+			return;
+		printk("\n========================================\n");
+		printk(  "[ BUG: lock held at syscall exit time! ]\n");
+		printk(  "----------------------------------------\n");
+		printk("%s/%d is leaving the kernel with locks still held!\n",
Quoted 8 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+				curr->comm, curr->pid);
+		lockdep_print_held_locks(curr);
+	}
+}

By the way, the s390 patch I just posted also checks if we hold any locks when returning from interrupt context to user space. Maybe the above text could be changed to "lock held when returning to user space" ?

Good idea, I'll look at doing the same for i386/x86_64. Traps (page faults) would also make sense I guess.

---

Subject: lockdep: syscall exit check

Provide a check to validate that we do not hold any locks when switching back to user-space.

Signed-off-by Peter Zijlstra <[email protected]>
---
 include/linux/lockdep.h |    2 ++
 kernel/lockdep.c        |   16 ++++++++++++++++
 2 files changed, 18 insertions(+)
include/linux/lockdep.h +2 -0
Index: linux-2.6/include/linux/lockdep.h
===================================================================
@@ -238,6 +238,7 @@ extern void lockdep_info(void);
 extern void lockdep_reset(void);
 extern void lockdep_reset_lock(struct lockdep_map *lock);
 extern void lockdep_free_key_range(void *start, unsigned long size);
+extern void lockdep_sys_exit(void);
 
 extern void lockdep_off(void);
 extern void lockdep_on(void);

@@ -317,6 +318,7 @@ static inline void lockdep_on(void)
 # define INIT_LOCKDEP
 # define lockdep_reset()		do { debug_locks = 1; } while (0)
 # define lockdep_free_key_range(start, size)	do { } while (0)
+# define lockdep_sys_exit() 			do { } while (0)
 /*
  * The class key takes no space if lockdep is disabled:
  */
kernel/lockdep.c +16 -0
Index: linux-2.6/kernel/lockdep.c
===================================================================
@@ -3199,3 +3199,19 @@ void debug_show_held_locks(struct task_s
 }
 
 EXPORT_SYMBOL_GPL(debug_show_held_locks);
+
+void lockdep_sys_exit(void)
+{
+	struct task_struct *curr = current;
+
+	if (unlikely(curr->lockdep_depth)) {
+		if (!debug_locks_off())
+			return;
+		printk("\n================================================\n");
+		printk(  "[ BUG: lock held when returning to user space! ]\n");
+		printk(  "------------------------------------------------\n");
+		printk("%s/%d is leaving the kernel with locks still held!\n",
+				curr->comm, curr->pid);
+		lockdep_print_held_locks(curr);
+	}
+}
#4 lore raw

Re: [PATCH 01/12] lockdep: syscall exit check

Quoted 21 lines · Peter Zijlstra
Quoted 11 lines · Heiko Carstens
Quoted 11 lines
kernel/lockdep.c +11 -0
+void lockdep_sys_exit(void)
+{
+	struct task_struct *curr = current;
+
+	if (unlikely(curr->lockdep_depth)) {
+		if (!debug_locks_off())
+			return;
+		printk("\n========================================\n");
+		printk(  "[ BUG: lock held at syscall exit time! ]\n");
+		printk(  "----------------------------------------\n");
+		printk("%s/%d is leaving the kernel with locks still held!\n",
Quoted 8 lines

kernel/lockdep.c void debug_show_held_locks(struct task_s · line 3217

Quoted 4 lines
kernel/lockdep.c +4 -0
+				curr->comm, curr->pid);
+		lockdep_print_held_locks(curr);
+	}
+}

By the way, the s390 patch I just posted also checks if we hold any locks when returning from interrupt context to user space. Maybe the above text could be changed to "lock held when returning to user space" ?

Quoted 2 lines

Good idea, I'll look at doing the same for i386/x86_64. Traps (page faults) would also make sense I guess.

Yes, traps and syscalls have the same exit path on s390. So we do that alreasy as well.

author
#5 lore raw

PATCH 2/12 lockdep: i386: connect the sysexit hook

In reply to Peter Zijlstra

Run the lockdep_sys_exit hook after all other C code on the syscall return path.

Signed-off-by Peter Zijlstra <[email protected]>
---
 arch/i386/kernel/entry.S    |    3 +++
 include/asm-i386/irqflags.h |   13 +++++++++++++
 2 files changed, 16 insertions(+)
arch/i386/kernel/entry.S +3 -0
Index: linux-2.6/arch/i386/kernel/entry.S
===================================================================
@@ -338,6 +338,7 @@ sysenter_past_esp:
 	jae syscall_badsys
 	call *sys_call_table(,%eax,4)
 	movl %eax,PT_EAX(%esp)
+	LOCKDEP_SYS_EXIT
 	DISABLE_INTERRUPTS(CLBR_ANY)
 	TRACE_IRQS_OFF
 	movl TI_flags(%ebp), %ecx

@@ -376,6 +377,7 @@ ENTRY(system_call)
 syscall_call:
 	call *sys_call_table(,%eax,4)
 	movl %eax,PT_EAX(%esp)		# store the return value
+	LOCKDEP_SYS_EXIT
 syscall_exit:
 	DISABLE_INTERRUPTS(CLBR_ANY)	# make sure we don't miss an interrupt
 					# setting need_resched or sigpending

@@ -532,6 +534,7 @@ syscall_exit_work:
 	movl %esp, %eax
 	movl $1, %edx
 	call do_syscall_trace
+	LOCKDEP_SYS_EXIT
 	jmp resume_userspace
 END(syscall_exit_work)
 	CFI_ENDPROC
include/asm-i386/irqflags.h +13 -0
Index: linux-2.6/include/asm-i386/irqflags.h
===================================================================
@@ -160,4 +160,17 @@ static inline int raw_irqs_disabled(void
 # define TRACE_IRQS_OFF
 #endif
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+# define LOCKDEP_SYS_EXIT			\
+	pushl %eax;				\
+	pushl %ecx;				\
+	pushl %edx;				\
+	call lockdep_sys_exit;			\
+	popl %edx;				\
+	popl %ecx;				\
+	popl %eax;
+#else
+# define LOCKDEP_SYS_EXIT
+#endif
+
 #endif

--

author
#6 lore raw

PATCH 3/12 lockdep: x86_64: connect the sysexit hook

In reply to Peter Zijlstra

Run the lockdep_sys_exit hook after all other C code on the syscall return path.

Signed-off-by Peter Zijlstra <[email protected]>
---
 arch/x86_64/kernel/entry.S    |    3 +++
 arch/x86_64/lib/thunk.S       |    4 ++++
 include/asm-x86_64/irqflags.h |    9 +++++++++
 3 files changed, 16 insertions(+)
arch/x86_64/kernel/entry.S +3 -0
Index: linux-2.6/arch/x86_64/kernel/entry.S
===================================================================
@@ -164,6 +164,7 @@ ENTRY(ret_from_fork)
 	testl $(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT),threadinfo_flags(%rcx)
 	jnz rff_trace
 rff_action:	
+	LOCKDEP_SYS_EXIT
 	RESTORE_REST
 	testl $3,CS-ARGOFFSET(%rsp)	# from kernel_thread?
 	je   int_ret_from_sys_call

@@ -326,6 +327,7 @@ tracesys:			 
  */
 	.globl int_ret_from_sys_call
 int_ret_from_sys_call:
+	LOCKDEP_SYS_EXIT_REST
 	cli
 	TRACE_IRQS_OFF
 	testl $3,CS-ARGOFFSET(%rsp)

@@ -382,6 +384,7 @@ int_signal:
 	call do_notify_resume
 1:	movl $_TIF_NEED_RESCHED,%edi	
 int_restore_rest:
+	LOCKDEP_SYS_EXIT
 	RESTORE_REST
 	cli
 	TRACE_IRQS_OFF
arch/x86_64/lib/thunk.S +4 -0
Index: linux-2.6/arch/x86_64/lib/thunk.S
===================================================================
@@ -50,6 +50,10 @@
 	thunk trace_hardirqs_on_thunk,trace_hardirqs_on
 	thunk trace_hardirqs_off_thunk,trace_hardirqs_off
 #endif
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	thunk lockdep_sys_exit_thunk,lockdep_sys_exit
+#endif
 	
 	/* SAVE_ARGS below is used only for the .cfi directives it contains. */
 	CFI_STARTPROC
include/asm-x86_64/irqflags.h +9 -0
Index: linux-2.6/include/asm-x86_64/irqflags.h
===================================================================
@@ -137,6 +137,15 @@ static inline void halt(void)
 #  define TRACE_IRQS_ON
 #  define TRACE_IRQS_OFF
 # endif
+# ifdef CONFIG_DEBUG_LOCK_ALLOC
+#  define LOCKDEP_SYS_EXIT	call lockdep_sys_exit_thunk
+#  define LOCKDEP_SYS_EXIT_REST	SAVE_REST; \
+				LOCKDEP_SYS_EXIT; \
+				RESTORE_REST;
+# else
+#  define LOCKDEP_SYS_EXIT
+#  define LOCKDEP_SYS_EXIT_REST
+# endif
 #endif
 
 #endif

--

author
#7 lore raw

PATCH 4/12 lockdep: annotate journal_start()

In reply to Peter Zijlstra
Quoted 2 lines · Andrew Morton

Except lockdep doesn't know about journal_start(), which has ranking requirements similar to a semaphore.

Signed-off-by Peter Zijlstra <[email protected]>
---
 fs/jbd/transaction.c |    9 +++++++++
 include/linux/jbd.h  |    5 +++++
 2 files changed, 14 insertions(+)
fs/jbd/transaction.c +9 -0
Index: linux-2.6/fs/jbd/transaction.c
===================================================================
@@ -233,6 +233,8 @@ out:
 	return ret;
 }
 
+static struct lock_class_key jbd_handle_key;
+
 /* Allocate a new handle.  This should probably be in a slab... */
 static handle_t *new_handle(int nblocks)
 {

@@ -243,6 +245,8 @@ static handle_t *new_handle(int nblocks)
 	handle->h_buffer_credits = nblocks;
 	handle->h_ref = 1;
 
+	lockdep_init_map(&handle->h_lockdep_map, "jbd_handle", &jbd_handle_key, 0);
+
 	return handle;
 }
 

@@ -286,6 +290,9 @@ handle_t *journal_start(journal_t *journ
 		current->journal_info = NULL;
 		handle = ERR_PTR(err);
 	}
+
+	lock_acquire(&handle->h_lockdep_map, 0, 0, 0, 2, _THIS_IP_);
+
 	return handle;
 }
 

@@ -1411,6 +1418,8 @@ int journal_stop(handle_t *handle)
 		spin_unlock(&journal->j_state_lock);
 	}
 
+	lock_release(&handle->h_lockdep_map, 1, _THIS_IP_);
+
 	jbd_free_handle(handle);
 	return err;
 }
include/linux/jbd.h +5 -0
Index: linux-2.6/include/linux/jbd.h
===================================================================
@@ -30,6 +30,7 @@
 #include <linux/bit_spinlock.h>
 #include <linux/mutex.h>
 #include <linux/timer.h>
+#include <linux/lockdep.h>
 
 #include <asm/semaphore.h>
 #endif

@@ -396,6 +397,10 @@ struct handle_s
 	unsigned int	h_sync:		1;	/* sync-on-close */
 	unsigned int	h_jdata:	1;	/* force data journaling */
 	unsigned int	h_aborted:	1;	/* fatal error on handle */
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	struct lockdep_map	h_lockdep_map;
+#endif
 };

--

author
#8 lore raw

PATCH 5/12 mm: trylock_page

In reply to Peter Zijlstra

Replace raw TestSetPageLocked() usage with trylock_page()

Signed-off-by Peter Zijlstra <[email protected]>
---
 fs/afs/write.c              |    2 +-
 fs/cifs/file.c              |    2 +-
 fs/jbd/commit.c             |    2 +-
 fs/jbd2/commit.c            |    2 +-
 fs/splice.c                 |    2 +-
 fs/xfs/linux-2.6/xfs_aops.c |    4 ++--
 include/linux/pagemap.h     |    5 +++++
 mm/filemap.c                |    4 ++--
 mm/memory.c                 |    2 +-
 mm/migrate.c                |    4 ++--
 mm/rmap.c                   |    2 +-
 mm/shmem.c                  |    4 ++--
 mm/swap.c                   |    2 +-
 mm/swap_state.c             |    2 +-
 mm/swapfile.c               |    2 +-
 mm/truncate.c               |    4 ++--
 mm/vmscan.c                 |    4 ++--
 17 files changed, 27 insertions(+), 22 deletions(-)
fs/afs/write.c +1 -1
Index: linux-2.6/fs/afs/write.c
===================================================================
@@ -404,7 +404,7 @@ static int afs_write_back_from_locked_pa
 			page = pages[loop];
 			if (page->index > wb->last)
 				break;
-			if (TestSetPageLocked(page))
+			if (!trylock_page(page))
 				break;
 			if (!PageDirty(page) ||
 			    page_private(page) != (unsigned long) wb) {
fs/cifs/file.c +1 -1
Index: linux-2.6/fs/cifs/file.c
===================================================================
@@ -1205,7 +1205,7 @@ retry:
 
 			if (first < 0)
 				lock_page(page);
-			else if (TestSetPageLocked(page))
+			else if (!trylock_page(page))
 				break;
 
 			if (unlikely(page->mapping != mapping)) {
fs/jbd/commit.c +1 -1
Index: linux-2.6/fs/jbd/commit.c
===================================================================
@@ -63,7 +63,7 @@ static void release_buffer_page(struct b
 		goto nope;
 
 	/* OK, it's a truncated page */
-	if (TestSetPageLocked(page))
+	if (!trylock_page(page))
 		goto nope;
 
 	page_cache_get(page);
fs/jbd2/commit.c +1 -1
Index: linux-2.6/fs/jbd2/commit.c
===================================================================
@@ -63,7 +63,7 @@ static void release_buffer_page(struct b
 		goto nope;
 
 	/* OK, it's a truncated page */
-	if (TestSetPageLocked(page))
+	if (!trylock_page(page))
 		goto nope;
 
 	page_cache_get(page);
fs/splice.c +1 -1
Index: linux-2.6/fs/splice.c
===================================================================
@@ -364,7 +364,7 @@ __generic_file_splice_read(struct file *
 			 * for an in-flight io page
 			 */
 			if (flags & SPLICE_F_NONBLOCK) {
-				if (TestSetPageLocked(page))
+				if (!trylock_page(page))
 					break;
 			} else
 				lock_page(page);
fs/xfs/linux-2.6/xfs_aops.c +2 -2
Index: linux-2.6/fs/xfs/linux-2.6/xfs_aops.c
===================================================================
@@ -665,7 +665,7 @@ xfs_probe_cluster(
 			} else
 				pg_offset = PAGE_CACHE_SIZE;
 
-			if (page->index == tindex && !TestSetPageLocked(page)) {
+			if (page->index == tindex && trylock_page(page)) {
 				pg_len = xfs_probe_page(page, pg_offset, mapped);
 				unlock_page(page);
 			}

@@ -749,7 +749,7 @@ xfs_convert_page(
 
 	if (page->index != tindex)
 		goto fail;
-	if (TestSetPageLocked(page))
+	if (!trylock_page(page))
 		goto fail;
 	if (PageWriteback(page))
 		goto fail_unlock_page;
include/linux/pagemap.h +5 -0
Index: linux-2.6/include/linux/pagemap.h
===================================================================
@@ -167,6 +167,11 @@ static inline void lock_page(struct page
 		__lock_page(page);
 }
 
+static inline int trylock_page(struct page *page)
+{
+	return !TestSetPageLocked(page);
+}
+
 /*
  * lock_page_nosync should only be used if we can't pin the page's inode.
  * Doesn't play quite so well with block device plugging.
mm/filemap.c +2 -2
Index: linux-2.6/mm/filemap.c
===================================================================
@@ -626,7 +626,7 @@ repeat:
 	page = radix_tree_lookup(&mapping->page_tree, offset);
 	if (page) {
 		page_cache_get(page);
-		if (TestSetPageLocked(page)) {
+		if (!trylock_page(page)) {
 			read_unlock_irq(&mapping->tree_lock);
 			__lock_page(page);
 			read_lock_irq(&mapping->tree_lock);

@@ -802,7 +802,7 @@ grab_cache_page_nowait(struct address_sp
 	struct page *page = find_get_page(mapping, index);
 
 	if (page) {
-		if (!TestSetPageLocked(page))
+		if (trylock_page(page))
 			return page;
 		page_cache_release(page);
 		return NULL;
mm/memory.c +1 -1
Index: linux-2.6/mm/memory.c
===================================================================
@@ -1650,7 +1650,7 @@ static int do_wp_page(struct mm_struct *
 	 * not dirty accountable.
 	 */
 	if (PageAnon(old_page)) {
-		if (!TestSetPageLocked(old_page)) {
+		if (trylock_page(old_page)) {
 			reuse = can_share_swap_page(old_page);
 			unlock_page(old_page);
 		}
mm/migrate.c +2 -2
Index: linux-2.6/mm/migrate.c
===================================================================
@@ -568,7 +568,7 @@ static int move_to_new_page(struct page 
 	 * establishing additional references. We are the only one
 	 * holding a reference to the new page at this point.
 	 */
-	if (TestSetPageLocked(newpage))
+	if (!trylock_page(newpage))
 		BUG();
 
 	/* Prepare mapping for the new page.*/

@@ -621,7 +621,7 @@ static int unmap_and_move(new_page_t get
 		goto move_newpage;
 
 	rc = -EAGAIN;
-	if (TestSetPageLocked(page)) {
+	if (!trylock_page(page)) {
 		if (!force)
 			goto move_newpage;
 		lock_page(page);
mm/rmap.c +1 -1
Index: linux-2.6/mm/rmap.c
===================================================================
@@ -401,7 +401,7 @@ int page_referenced(struct page *page, i
 			referenced += page_referenced_anon(page);
 		else if (is_locked)
 			referenced += page_referenced_file(page);
-		else if (TestSetPageLocked(page))
+		else if (!trylock_page(page))
 			referenced++;
 		else {
 			if (page->mapping)
mm/shmem.c +2 -2
Index: linux-2.6/mm/shmem.c
===================================================================
@@ -1165,7 +1165,7 @@ repeat:
 		}
 
 		/* We have to do this with page locked to prevent races */
-		if (TestSetPageLocked(swappage)) {
+		if (!trylock_page(swappage)) {
 			shmem_swp_unmap(entry);
 			spin_unlock(&info->lock);
 			wait_on_page_locked(swappage);

@@ -1224,7 +1224,7 @@ repeat:
 		shmem_swp_unmap(entry);
 		filepage = find_get_page(mapping, idx);
 		if (filepage &&
-		    (!PageUptodate(filepage) || TestSetPageLocked(filepage))) {
+		    (!PageUptodate(filepage) || !trylock_page(filepage))) {
 			spin_unlock(&info->lock);
 			wait_on_page_locked(filepage);
 			page_cache_release(filepage);
mm/swap.c +1 -1
Index: linux-2.6/mm/swap.c
===================================================================
@@ -412,7 +412,7 @@ void pagevec_strip(struct pagevec *pvec)
 	for (i = 0; i < pagevec_count(pvec); i++) {
 		struct page *page = pvec->pages[i];
 
-		if (PagePrivate(page) && !TestSetPageLocked(page)) {
+		if (PagePrivate(page) && trylock_page(page)) {
 			if (PagePrivate(page))
 				try_to_release_page(page, 0);
 			unlock_page(page);
mm/swap_state.c +1 -1
Index: linux-2.6/mm/swap_state.c
===================================================================
@@ -252,7 +252,7 @@ int move_from_swap_cache(struct page *pa
  */
 static inline void free_swap_cache(struct page *page)
 {
-	if (PageSwapCache(page) && !TestSetPageLocked(page)) {
+	if (PageSwapCache(page) && trylock_page(page)) {
 		remove_exclusive_swap_page(page);
 		unlock_page(page);
 	}
mm/swapfile.c +1 -1
Index: linux-2.6/mm/swapfile.c
===================================================================
@@ -401,7 +401,7 @@ void free_swap_and_cache(swp_entry_t ent
 	if (p) {
 		if (swap_entry_free(p, swp_offset(entry)) == 1) {
 			page = find_get_page(&swapper_space, entry.val);
-			if (page && unlikely(TestSetPageLocked(page))) {
+			if (page && unlikely(!trylock_page(page))) {
 				page_cache_release(page);
 				page = NULL;
 			}
mm/truncate.c +2 -2
Index: linux-2.6/mm/truncate.c
===================================================================
@@ -186,7 +186,7 @@ void truncate_inode_pages_range(struct a
 			if (page_index > next)
 				next = page_index;
 			next++;
-			if (TestSetPageLocked(page))
+			if (!trylock_page(page))
 				continue;
 			if (PageWriteback(page)) {
 				unlock_page(page);

@@ -279,7 +279,7 @@ unsigned long __invalidate_mapping_pages
 			pgoff_t index;
 			int lock_failed;
 
-			lock_failed = TestSetPageLocked(page);
+			lock_failed = !trylock_page(page);
 
 			/*
 			 * We really shouldn't be looking at the ->index of an
mm/vmscan.c +2 -2
Index: linux-2.6/mm/vmscan.c
===================================================================
@@ -461,7 +461,7 @@ static unsigned long shrink_page_list(st
 		page = lru_to_page(page_list);
 		list_del(&page->lru);
 
-		if (TestSetPageLocked(page))
+		if (!trylock_page(page))
 			goto keep;
 
 		VM_BUG_ON(PageActive(page));

@@ -547,7 +547,7 @@ static unsigned long shrink_page_list(st
 				 * A synchronous write - probably a ramdisk.  Go
 				 * ahead and try to reclaim the page.
 				 */
-				if (TestSetPageLocked(page))
+				if (!trylock_page(page))
 					goto keep;
 				if (PageDirty(page) || PageWriteback(page))
 					goto keep_locked;

--

#9 lore raw

Re: [PATCH 05/12] mm: trylock_page

Quoted 1 line · Peter Zijlstra

Replace raw TestSetPageLocked() usage with trylock_page()

I have such a thing queued too, for the lock bitops patches for when 2.6.24 opens, Andrew promises me :).

I guess they should be identical, except I don't like doing trylock_page in place of SetPageLocked, for memory ordering performance and aesthetic reasons... I've got an init_page_locked (or set_page_locked... I can't remember, the patch is at home).

Fine idea to lockdep the page lock, anyway. Does it show up any of the buffered write deadlock possibilities? :)

buffer lock is another notable bit-mutex that might be converted (I have the patch to do the similar nice !tas->trylock conversion for that too). I think it is used widely enough by tricky code that it would be useful to annotate as well.

Unfortunately we can't convert bit_spinlock.h easily, I guess?

author
#10 lore raw

Re: [PATCH 05/12] mm: trylock_page

Quoted 10 lines · Nick Piggin
Quoted 1 line · Peter Zijlstra

Replace raw TestSetPageLocked() usage with trylock_page()

I have such a thing queued too, for the lock bitops patches for when 2.6.24 opens, Andrew promises me :).

I guess they should be identical, except I don't like doing trylock_page in place of SetPageLocked, for memory ordering performance and aesthetic reasons... I've got an init_page_locked (or set_page_locked... I can't remember, the patch is at home).

Sure, that might work, or we could just make it so that add_to_*_cache is never passed an unlocked page. But sure...

Quoted 2 lines

Fine idea to lockdep the page lock, anyway. Does it show up any of the buffered write deadlock possibilities? :)

Not yet, it might just be that the concessions done to annotate this type of lock were too severe.

What I basically did was treat all the page locks as a single recursive lock.

Quoted 4 lines

buffer lock is another notable bit-mutex that might be converted (I have the patch to do the similar nice !tas->trylock conversion for that too). I think it is used widely enough by tricky code that it would be useful to annotate as well.

Not at all familiar with that lock, but yeah, we could have a look at doing that too.

Quoted 1 line

Unfortunately we can't convert bit_spinlock.h easily, I guess?

Yeah, the space constraints make that rather hard. Each of these locks needs some form of external meta-data.

For the page lock I used one lock instance per file system type.

#11 lore raw

Re: [PATCH 05/12] mm: trylock_page

Quoted 7 lines · Peter Zijlstra
Quoted 6 lines · Nick Piggin
Quoted 1 line · Peter Zijlstra

Replace raw TestSetPageLocked() usage with trylock_page()

I have such a thing queued too, for the lock bitops patches for when 2.6.24 opens, Andrew promises me :).

Quoted 7 lines
Quoted 4 lines

I guess they should be identical, except I don't like doing trylock_page in place of SetPageLocked, for memory ordering performance and aesthetic reasons... I've got an init_page_locked (or set_page_locked... I can't remember, the patch is at home).

Sure, that might work, or we could just make it so that add_to_*_cache is never passed an unlocked page. But sure...

It does kind of make sense to have it there (because you want the page to be locked iff it gets inserted into pagecache). And wherever you lock the page, we'll still want an init_page_locked to be able to lock it while we are the only owner of it, for the same performance reason.

Quoted 8 lines
Quoted 2 lines

Fine idea to lockdep the page lock, anyway. Does it show up any of the buffered write deadlock possibilities? :)

Not yet, it might just be that the concessions done to annotate this type of lock were too severe.

What I basically did was treat all the page locks as a single recursive lock.

Hmm, OK. There are a couple of page lock deadlocks there that wouldn't be picked up, but the page lock vs mmap_sem one probably should be.

Quoted 7 lines
Quoted 4 lines

buffer lock is another notable bit-mutex that might be converted (I have the patch to do the similar nice !tas->trylock conversion for that too). I think it is used widely enough by tricky code that it would be useful to annotate as well.

Not at all familiar with that lock, but yeah, we could have a look at doing that too.

Should be pretty well identical to the page lock. I'll cc you on the patch to do this equivalent API conversion, if you'd like.

Quoted 4 lines
Quoted 1 line

Unfortunately we can't convert bit_spinlock.h easily, I guess?

Yeah, the space constraints make that rather hard. Each of these locks needs some form of external meta-data.

Yeah.

Quoted 1 line

For the page lock I used one lock instance per file system type.

Seems OK.

author
#12 lore raw

PATCH 6/12 mm: remove raw SetPageLocked() usage

In reply to Peter Zijlstra

Remove the last few SetPageLocked() instances.

use trylock_page() instead of SetPageLocked() because we hold tree_lock so we cannot suffer the might_sleep(). Also at least add_to_page_cache() can be handed a locked page.

Signed-off-by Peter Zijlstra <[email protected]>
---
 mm/filemap.c    |    2 +-
 mm/swap_state.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
mm/filemap.c +1 -1
Index: linux-2.6/mm/filemap.c
===================================================================
@@ -445,7 +445,7 @@ int add_to_page_cache(struct page *page,
 		error = radix_tree_insert(&mapping->page_tree, offset, page);
 		if (!error) {
 			page_cache_get(page);
-			SetPageLocked(page);
+			trylock_page(page);
 			page->mapping = mapping;
 			page->index = offset;
 			mapping->nrpages++;
mm/swap_state.c +1 -1
Index: linux-2.6/mm/swap_state.c
===================================================================
@@ -83,7 +83,7 @@ static int __add_to_swap_cache(struct pa
 						entry.val, page);
 		if (!error) {
 			page_cache_get(page);
-			SetPageLocked(page);
+			trylock_page(page);
 			SetPageSwapCache(page);
 			set_page_private(page, entry.val);
 			total_swapcache_pages++;

--

#13 lore raw

Re: [PATCH 06/12] mm: remove raw SetPageLocked() usage

  		error = radix_tree_insert(&mapping->page_tree, offset, page);
>  		if (!error) {
>  			page_cache_get(page);

I don't think we want an unchecked trylock_page without a comment explaining it. I'd go as far as saying trylock_page should get a __must_check attribute, and we'll need a dummy variable to actually ignore it.

author
#14 lore raw

PATCH 7/12 lockdep: page lock hooks

In reply to Peter Zijlstra

rework the now complete page lock API to provide acquisition and release hooks.

Signed-off-by Peter Zijlstra <[email protected]>
---
 include/linux/pagemap.h |   40 +++++++++++++++++++++++++++++++---------
 mm/filemap.c            |   13 +++++++------
 2 files changed, 38 insertions(+), 15 deletions(-)
include/linux/pagemap.h +31 -9
Index: linux-2.6/include/linux/pagemap.h
===================================================================
@@ -153,23 +153,45 @@ static inline pgoff_t linear_page_index(
 	return pgoff >> (PAGE_CACHE_SHIFT - PAGE_SHIFT);
 }
 
-extern void FASTCALL(__lock_page(struct page *page));
-extern void FASTCALL(__lock_page_nosync(struct page *page));
-extern void FASTCALL(unlock_page(struct page *page));
+extern void FASTCALL(__lock_page(struct page *page, unsigned long ip));
+extern void FASTCALL(__lock_page_nosync(struct page *page, unsigned long ip));
+extern void FASTCALL(unlock_page_nocheck(struct page *page));
+
+static inline void __lock_page_check(struct page *page, int try, unsigned long ip)
+{
+}
+
+static inline void __unlock_page_check(struct page *page, unsigned long ip)
+{
+}
 
 /*
  * lock_page may only be called if we have the page's inode pinned.
  */
+static inline int __trylock_page(struct page *page, int try)
+{
+	int ret = !TestSetPageLocked(page);
+	if (ret)
+		__lock_page_check(page, try, _RET_IP_);
+	return ret;
+}
+
+static inline int trylock_page(struct page *page)
+{
+	return __trylock_page(page, 1);
+}
+
 static inline void lock_page(struct page *page)
 {
 	might_sleep();
-	if (TestSetPageLocked(page))
-		__lock_page(page);
+	if (!__trylock_page(page, 0))
+		__lock_page(page, _RET_IP_);
 }
 
-static inline int trylock_page(struct page *page)
+static inline void unlock_page(struct page *page)
 {
-	return !TestSetPageLocked(page);
+	__unlock_page_check(page, _RET_IP_);
+	unlock_page_nocheck(page);
 }
 
 /*

@@ -179,8 +201,8 @@ static inline int trylock_page(struct pa
 static inline void lock_page_nosync(struct page *page)
 {
 	might_sleep();
-	if (TestSetPageLocked(page))
-		__lock_page_nosync(page);
+	if (!__trylock_page(page, 0))
+		__lock_page_nosync(page, _RET_IP_);
 }
 	
 /*
mm/filemap.c +7 -6
Index: linux-2.6/mm/filemap.c
===================================================================
@@ -531,7 +531,7 @@ EXPORT_SYMBOL(wait_on_page_bit);
  * the clear_bit and the read of the waitqueue (to avoid SMP races with a
  * parallel wait_on_page_locked()).
  */
-void fastcall unlock_page(struct page *page)
+void fastcall unlock_page_nocheck(struct page *page)
 {
 	smp_mb__before_clear_bit();
 	if (!TestClearPageLocked(page))

@@ -539,7 +539,7 @@ void fastcall unlock_page(struct page *p
 	smp_mb__after_clear_bit(); 
 	wake_up_page(page, PG_locked);
 }
-EXPORT_SYMBOL(unlock_page);
+EXPORT_SYMBOL(unlock_page_nocheck);
 
 /**
  * end_page_writeback - end writeback against a page

@@ -565,12 +565,12 @@ EXPORT_SYMBOL(end_page_writeback);
  * chances are that on the second loop, the block layer's plug list is empty,
  * so sync_page() will then return in state TASK_UNINTERRUPTIBLE.
  */
-void fastcall __lock_page(struct page *page)
+void fastcall __lock_page(struct page *page, unsigned long ip)
 {
 	DEFINE_WAIT_BIT(wait, &page->flags, PG_locked);
-
 	__wait_on_bit_lock(page_waitqueue(page), &wait, sync_page,
 							TASK_UNINTERRUPTIBLE);
+	__lock_page_check(page, 0, ip);
 }
 EXPORT_SYMBOL(__lock_page);
 

@@ -578,11 +578,12 @@ EXPORT_SYMBOL(__lock_page);
  * Variant of lock_page that does not require the caller to hold a reference
  * on the page's mapping.
  */
-void fastcall __lock_page_nosync(struct page *page)
+void fastcall __lock_page_nosync(struct page *page, unsigned long ip)
 {
 	DEFINE_WAIT_BIT(wait, &page->flags, PG_locked);
 	__wait_on_bit_lock(page_waitqueue(page), &wait, __sleep_on_page_lock,
 							TASK_UNINTERRUPTIBLE);
+	__lock_page_check(page, 0, ip);
 }
 
 /**

@@ -628,7 +629,7 @@ repeat:
 		page_cache_get(page);
 		if (!trylock_page(page)) {
 			read_unlock_irq(&mapping->tree_lock);
-			__lock_page(page);
+			__lock_page(page, _THIS_IP_);
 			read_lock_irq(&mapping->tree_lock);
 
 			/* Has the page been truncated while we slept? */

--

author
#15 lore raw

PATCH 8/12 lockdep: increase MAX_LOCK_DEPTH

In reply to Peter Zijlstra

we can easily hold PAGE_VEC (and more in places) page locks.

Signed-off-by Peter Zijlstra <[email protected]>
---
 include/linux/sched.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/sched.h +1 -1
Index: linux-2.6/include/linux/sched.h
===================================================================
@@ -1246,7 +1246,7 @@ struct task_struct {
 	int softirq_context;
 #endif
 #ifdef CONFIG_LOCKDEP
-# define MAX_LOCK_DEPTH 30UL
+# define MAX_LOCK_DEPTH 48UL
 	u64 curr_chain_key;
 	int lockdep_depth;
 	struct held_lock held_locks[MAX_LOCK_DEPTH];

--

author
#16 lore raw

PATCH 9/12 lockdep: add a page lock class per filesystem type

In reply to Peter Zijlstra

Provide a different page lock class per filesystem, so we can properly express the neseting relations between filesystems.

Signed-off-by Peter Zijlstra <[email protected]>
---
 fs/filesystems.c   |    1 +
 include/linux/fs.h |    5 +++++
 2 files changed, 6 insertions(+)
fs/filesystems.c +1 -0
Index: linux-2.6/fs/filesystems.c
===================================================================
@@ -73,6 +73,7 @@ int register_filesystem(struct file_syst
 	if (fs->next)
 		return -EBUSY;
 	INIT_LIST_HEAD(&fs->fs_supers);
+	lockdep_init_map(&fs->s_mapping_map, fs->name, &fs->s_mapping_key, 0);
 	write_lock(&file_systems_lock);
 	p = find_filesystem(fs->name, strlen(fs->name));
 	if (*p)
include/linux/fs.h +5 -0
Index: linux-2.6/include/linux/fs.h
===================================================================
@@ -1302,8 +1302,13 @@ struct file_system_type {
 	struct module *owner;
 	struct file_system_type * next;
 	struct list_head fs_supers;
+
 	struct lock_class_key s_lock_key;
 	struct lock_class_key s_umount_key;
+	struct lock_class_key s_mapping_key;
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	struct lockdep_map s_mapping_map;
+#endif
 };
 
 extern int get_sb_bdev(struct file_system_type *fs_type,

--

author
#17 lore raw

PATCH 10/12 lockdep: lock_page: handle IO-completions

In reply to Peter Zijlstra

IO-completions unlock the page in interrupt context, which is not the same context as that locked the page.

Avoid this problem by doing the unlock tracking when submitting the page for IO.

Signed-off-by Peter Zijlstra <[email protected]>
---
 fs/buffer.c             |    4 +++-
 fs/mpage.c              |   19 +++++++++++++++----
 fs/nfs/read.c           |    4 +++-
 include/linux/pagemap.h |    5 +++++
 mm/page_io.c            |    3 ++-
 5 files changed, 28 insertions(+), 7 deletions(-)
fs/buffer.c +3 -1
Index: linux-2.6/fs/buffer.c
===================================================================
@@ -413,7 +413,7 @@ static void end_buffer_async_read(struct
 	 */
 	if (page_uptodate && !PageError(page))
 		SetPageUptodate(page);
-	unlock_page(page);
+	unlock_page_nocheck(page);
 	return;
 
 still_busy:

@@ -1988,6 +1988,8 @@ int block_read_full_page(struct page *pa
 		mark_buffer_async_read(bh);
 	}
 
+	unlock_page_check(page);
+
 	/*
 	 * Stage 3: start the IO.  Check for uptodateness
 	 * inside the buffer lock in case another process reading
fs/mpage.c +15 -4
Index: linux-2.6/fs/mpage.c
===================================================================
@@ -59,7 +59,7 @@ static int mpage_end_io_read(struct bio 
 			ClearPageUptodate(page);
 			SetPageError(page);
 		}
-		unlock_page(page);
+		unlock_page_nocheck(page);
 	} while (bvec >= bio->bi_io_vec);
 	bio_put(bio);
 	return 0;

@@ -92,9 +92,20 @@ static int mpage_end_io_write(struct bio
 
 static struct bio *mpage_bio_submit(int rw, struct bio *bio)
 {
-	bio->bi_end_io = mpage_end_io_read;
-	if (rw == WRITE)
-		bio->bi_end_io = mpage_end_io_write;
+	bio->bi_end_io = mpage_end_io_write;
+	if (rw == READ) {
+		struct bio_vec *bvec = bio->bi_io_vec;
+		int i;
+
+		bio->bi_end_io = mpage_end_io_read;
+
+		for (i = 0; i < bio->bi_vcnt; i++) {
+			struct page *page = bvec[i].bv_page;
+
+			if (page)
+				unlock_page_check(page);
+		}
+	}
 	submit_bio(rw, bio);
 	return NULL;
 }
fs/nfs/read.c +3 -1
Index: linux-2.6/fs/nfs/read.c
===================================================================
@@ -137,12 +137,13 @@ static int nfs_readpage_async(struct nfs
 		nfs_pagein_multi(inode, &one_request, 1, len, 0);
 	else
 		nfs_pagein_one(inode, &one_request, 1, len, 0);
+	unlock_page_check(page);
 	return 0;
 }
 
 static void nfs_readpage_release(struct nfs_page *req)
 {
-	unlock_page(req->wb_page);
+	unlock_page_nocheck(req->wb_page);
 
 	dprintk("NFS: read done (%s/%Ld %d@%Ld)\n",
 			req->wb_context->path.dentry->d_inode->i_sb->s_id,

@@ -540,6 +541,7 @@ readpage_async_filler(void *data, struct
 	if (len < PAGE_CACHE_SIZE)
 		zero_user_page(page, len, PAGE_CACHE_SIZE - len, KM_USER0);
 	nfs_pageio_add_request(desc->pgio, new);
+	unlock_page_check(page);
 	return 0;
 out_error:
 	error = PTR_ERR(new);
mm/page_io.c +2 -1
Index: linux-2.6/mm/page_io.c
===================================================================
@@ -92,7 +92,7 @@ int end_swap_bio_read(struct bio *bio, u
 	} else {
 		SetPageUptodate(page);
 	}
-	unlock_page(page);
+	unlock_page_nocheck(page);
 	bio_put(bio);
 	return 0;
 }

@@ -144,6 +144,7 @@ int swap_readpage(struct file *file, str
 	}
 	count_vm_event(PSWPIN);
 	submit_bio(READ, bio);
+	unlock_page_check(page);
 out:
 	return ret;
 }
include/linux/pagemap.h +5 -0
Index: linux-2.6/include/linux/pagemap.h
===================================================================
@@ -189,6 +189,11 @@ static inline void unlock_page(struct pa
 	unlock_page_nocheck(page);
 }
 
+static inline void unlock_page_check(struct page *page)
+{
+	__unlock_page_check(page, _RET_IP_);
+}
+
 /*
  * lock_page_nosync should only be used if we can't pin the page's inode.
  * Doesn't play quite so well with block device plugging.

--

author
#18 lore raw

PATCH 11/12 mm: set_page_mapping()

In reply to Peter Zijlstra

wrap:
page->mapping = some_mapping;

because the lock class is dependent on page_mapping()

Signed-off-by Peter Zijlstra <[email protected]>
---
 include/linux/pagemap.h |    9 +++++++++
 mm/filemap.c            |    4 ++--
 mm/migrate.c            |    6 +++---
 3 files changed, 14 insertions(+), 5 deletions(-)
include/linux/pagemap.h +9 -0
Index: linux-2.6/include/linux/pagemap.h
===================================================================
@@ -199,6 +199,15 @@ static inline void unlock_page_check(str
 	__unlock_page_check(page, _RET_IP_);
 }
 
+static inline struct address_space *set_page_mapping(struct page *page,
+		struct address_space *mapping)
+{
+	__unlock_page_check(page, _RET_IP_);
+	page->mapping = mapping;
+	__lock_page_check(page, 0, _RET_IP_); /* XXX we lost trylock */
+	return mapping;
+}
+
 /*
  * lock_page_nosync should only be used if we can't pin the page's inode.
  * Doesn't play quite so well with block device plugging.
mm/filemap.c +2 -2
Index: linux-2.6/mm/filemap.c
===================================================================
@@ -117,7 +117,7 @@ void __remove_from_page_cache(struct pag
 	struct address_space *mapping = page->mapping;
 
 	radix_tree_delete(&mapping->page_tree, page->index);
-	page->mapping = NULL;
+	set_page_mapping(page, NULL);
 	mapping->nrpages--;
 	__dec_zone_page_state(page, NR_FILE_PAGES);
 	BUG_ON(page_mapped(page));

@@ -446,7 +446,7 @@ int add_to_page_cache(struct page *page,
 		if (!error) {
 			page_cache_get(page);
 			trylock_page(page);
-			page->mapping = mapping;
+			set_page_mapping(page, mapping);
 			page->index = offset;
 			mapping->nrpages++;
 			__inc_zone_page_state(page, NR_FILE_PAGES);
mm/migrate.c +3 -3
Index: linux-2.6/mm/migrate.c
===================================================================
@@ -381,7 +381,7 @@ static void migrate_page_copy(struct pag
 	ClearPageActive(page);
 	ClearPagePrivate(page);
 	set_page_private(page, 0);
-	page->mapping = NULL;
+	set_page_mapping(page, NULL);
 
 	/*
 	 * If any waiters have accumulated on the new page then

@@ -573,7 +573,7 @@ static int move_to_new_page(struct page 
 
 	/* Prepare mapping for the new page.*/
 	newpage->index = page->index;
-	newpage->mapping = page->mapping;
+	set_page_mapping(newpage, page->mapping);
 
 	mapping = page_mapping(page);
 	if (!mapping)

@@ -594,7 +594,7 @@ static int move_to_new_page(struct page 
 	if (!rc)
 		remove_migration_ptes(page, newpage);
 	else
-		newpage->mapping = NULL;
+		set_page_mapping(newpage, NULL);
 
 	unlock_page(newpage);

--

author
#19 lore raw

PATCH 12/12 lockdep: enable lock_page lockdep annotation

In reply to Peter Zijlstra

With everything ready, connect the dots.

Signed-off-by Peter Zijlstra <[email protected]>
---
 include/linux/lockdep.h |    7 +++++++
 include/linux/pagemap.h |   10 ++++++++++
 mm/filemap.c            |   35 +++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)
include/linux/lockdep.h +7 -0
Index: linux-2.6/include/linux/lockdep.h
===================================================================
@@ -253,6 +253,13 @@ extern void lockdep_init_map(struct lock
 			     struct lock_class_key *key, int subclass);
 
 /*
+ * To initialize a lockdep_map statically use this macro.
+ * Note that _name must not be NULL.
+ */
+#define STATIC_LOCKDEP_MAP_INIT(_name, _key) \
+	{ .name = (_name), .key = (void *)(_key), }
+
+/*
  * Reinitialize a lock key - for cases where there is special locking or
  * special initialization of locks so that the validator gets the scope
  * of dependencies wrong: they are either too broad (they need a class-split)
include/linux/pagemap.h +10 -0
Index: linux-2.6/include/linux/pagemap.h
===================================================================
@@ -157,12 +157,22 @@ extern void FASTCALL(__lock_page(struct 
 extern void FASTCALL(__lock_page_nosync(struct page *page, unsigned long ip));
 extern void FASTCALL(unlock_page_nocheck(struct page *page));
 
+extern struct lockdep_map *page_lock_map(struct page *page);
+
 static inline void __lock_page_check(struct page *page, int try, unsigned long ip)
 {
+	/*
+	 * use recursive read locks to annotate the page lock.
+	 * this allows us to have a single lock instance per filesystem.
+	 * obviously this reduces the usefulness of the lock checker
+	 * but it also keeps struct page from bloating.
+	 */
+	lock_acquire(page_lock_map(page), 0, try, 2, 2, ip);
 }
 
 static inline void __unlock_page_check(struct page *page, unsigned long ip)
 {
+	lock_release(page_lock_map(page), 1, ip);
 }
 
 /*
mm/filemap.c +35 -0
Index: linux-2.6/mm/filemap.c
===================================================================
@@ -556,6 +556,41 @@ void end_page_writeback(struct page *pag
 }
 EXPORT_SYMBOL(end_page_writeback);
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+static struct lock_class_key lock_page_key;
+static struct lockdep_map lock_page_map =
+	STATIC_LOCKDEP_MAP_INIT("lock_page", &lock_page_key);
+
+struct lockdep_map *page_lock_map(struct page *page)
+{
+	struct address_space *mapping;
+	struct inode *inode;
+	struct super_block *sb;
+	struct file_system_type *type;
+	struct lockdep_map *map = &lock_page_map;
+
+	mapping = page_mapping(page);
+	if (!mapping)
+		goto out;
+
+	inode = mapping->host;
+	if (!inode)
+		goto out;
+
+	sb = inode->i_sb;
+	if (!sb)
+		goto out;
+
+	type = sb->s_type;
+	if (type)
+		map = &type->s_mapping_map;
+
+out:
+	return map;
+}
+EXPORT_SYMBOL(page_lock_map);
+#endif
+
 /**
  * __lock_page - get a lock on the page, assuming we need to sleep to get it
  * @page: the page to lock

--

Reviewed-by Martin Schwidefsky on the v1 series
#20 lore raw
In reply to Peter Zijlstra
Quoted 9 lines · Peter Zijlstra

The first 3 patches provide a new lockdep check, it verifies we don't hold any locks when returning to userspace.

	lockdep-sys_exit.patch
	lockdep-i386-sys_exit.patch
	lockdep-x86_64-sys_exit.patch

Could the various arch maintainers that have support for lockdep help out with placing this hook?

Subject: [PATCH] lockdep: s390: connect the sysexit hook

From: Heiko Carstens <[email protected]>

Run the lockdep_sys_exit hook before returning to user space.

Reviewed-by Martin Schwidefsky <[email protected]>
Signed-off-by Heiko Carstens <[email protected]>
---
 arch/s390/kernel/entry.S   |   12 ++++++++++++
 arch/s390/kernel/entry64.S |    9 +++++++++
 2 files changed, 21 insertions(+)
arch/s390/kernel/entry64.S +9 -0
Index: linux-2.6/arch/s390/kernel/entry64.S
===================================================================
@@ -66,9 +66,14 @@ _TIF_WORK_INT = (_TIF_SIGPENDING | _TIF_
 	.macro	TRACE_IRQS_OFF
 	 brasl	%r14,trace_hardirqs_off
 	.endm
+
+	.macro	LOCKDEP_SYS_EXIT
+	 brasl	%r14,lockdep_sys_exit
+	.endm
 #else
 #define TRACE_IRQS_ON
 #define TRACE_IRQS_OFF
+#define LOCKDEP_SYS_EXIT
 #endif
 
 	.macro	STORE_TIMER lc_offset

@@ -255,6 +260,7 @@ sysc_return:
 	jno	sysc_leave
 	tm	__TI_flags+7(%r9),_TIF_WORK_SVC
 	jnz	sysc_work	# there is work to do (signals etc.)
+	LOCKDEP_SYS_EXIT
 sysc_leave:
 	RESTORE_ALL __LC_RETURN_PSW,1
 

@@ -278,6 +284,7 @@ sysc_work:
 	jo	sysc_restart
 	tm	__TI_flags+7(%r9),_TIF_SINGLE_STEP
 	jo	sysc_singlestep
+	LOCKDEP_SYS_EXIT
 	j	sysc_leave
 
 #

@@ -558,6 +565,7 @@ io_return:
 #endif
 	tm	__TI_flags+7(%r9),_TIF_WORK_INT
 	jnz	io_work 		# there is work to do (signals etc.)
+	LOCKDEP_SYS_EXIT
 io_leave:
 	RESTORE_ALL __LC_RETURN_PSW,0
 io_done:

@@ -605,6 +613,7 @@ io_work_loop:
 	jo	io_reschedule
 	tm	__TI_flags+7(%r9),(_TIF_SIGPENDING | _TIF_RESTORE_SIGMASK)
 	jnz	io_sigpending
+	LOCKDEP_SYS_EXIT
 	j	io_leave
 
 #
arch/s390/kernel/entry.S +12 -0
Index: linux-2.6/arch/s390/kernel/entry.S
===================================================================
@@ -68,9 +68,15 @@ STACK_SIZE  = 1 << STACK_SHIFT
 	l	%r1,BASED(.Ltrace_irq_off)
 	basr	%r14,%r1
 	.endm
+
+	.macro	LOCKDEP_SYS_EXIT
+	l	%r1,BASED(.Llockdep_sys_exit)
+	basr	%r14,%r1
+	.endm
 #else
 #define TRACE_IRQS_ON
 #define TRACE_IRQS_OFF
+#define LOCKDEP_SYS_EXIT
 #endif
 
 /*

@@ -260,6 +266,7 @@ sysc_return:
 	bno	BASED(sysc_leave)
 	tm	__TI_flags+3(%r9),_TIF_WORK_SVC
 	bnz	BASED(sysc_work)  # there is work to do (signals etc.)
+	LOCKDEP_SYS_EXIT
 sysc_leave:
 	RESTORE_ALL __LC_RETURN_PSW,1
 

@@ -283,6 +290,7 @@ sysc_work:
 	bo	BASED(sysc_restart)
 	tm	__TI_flags+3(%r9),_TIF_SINGLE_STEP
 	bo	BASED(sysc_singlestep)
+	LOCKDEP_SYS_EXIT
 	b	BASED(sysc_leave)
 
 #

@@ -572,6 +580,7 @@ io_return:
 #endif
 	tm	__TI_flags+3(%r9),_TIF_WORK_INT
 	bnz	BASED(io_work)		# there is work to do (signals etc.)
+	LOCKDEP_SYS_EXIT
 io_leave:
 	RESTORE_ALL __LC_RETURN_PSW,0
 io_done:

@@ -618,6 +627,7 @@ io_work_loop:
 	bo	BASED(io_reschedule)
 	tm	__TI_flags+3(%r9),(_TIF_SIGPENDING | _TIF_RESTORE_SIGMASK)
 	bnz	BASED(io_sigpending)
+	LOCKDEP_SYS_EXIT
 	b	BASED(io_leave)
 
 #

@@ -1040,6 +1050,8 @@ cleanup_io_leave_insn:
 .Ltrace_irq_on: .long	trace_hardirqs_on
 .Ltrace_irq_off:
 		.long	trace_hardirqs_off
+.Llockdep_sys_exit:
+		.long	lockdep_sys_exit
 #endif
 .Lcritical_start:
 		.long	__critical_start + 0x80000000