[PATCH RFC 00/11] lock monitor: Separate features related to lock

[PATCH RFC 00/11] lock monitor: Separate features related to lock

[Hitoshi Mitake]

Current lockdep is too complicated because,
 * dependency validation
 * statistics
 * event tracing
are all implemented by it.
This cause problem of overhead.
If user enables one of them, overhead of rests part is not avoidable.
(tracing is exception. If user enables validation or stat,
overhead of tracing doesn't occur.)

So I suggest new subsystem "lock monitor".
This is a general purpose lock event hooking mechanism.

lock monitor will be enable easy implementing and running
these features related to lock.

And I'm hoping that lock monitor will reduce overhead of perf lock.
Because lock monitor separates dependency validation and event tracing clearly,
so calling of functions of lockdep (e.g. lock_acquire()) only for validation
will not occur lock events.

I implemented it on the branch perf/inject of Frederic's random-tracing tree.
Because the branch is hottest place of lock and tracing :)

CAUTION!
This patch is very very beta, if you test it, please be careful!

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Jason Baron <jbaron@redhat.com>

Hitoshi Mitake (11):
  lock monitor: New subsystem for lock event hooking
  Adopt lockdep to lock monitor
  Adopt spinlock to lock monitor
  Adopt rwlock to lock monitor
  Adopt arch dependent rwsem to lock monitor
  Adopt rwsem of x86 to lock monitor
  Adopt the way of initializing semaphore to lock monitor
  Adopt mutex to lock monitor
  Adopt rcu_read_lock() to lock monitor
  Adopt kernel/sched.c to lock monitor
  Very dirty temporal solution for testing lock monitor

 arch/x86/include/asm/rwsem.h     |   12 ++--
 fs/inode.c                       |    1 -
 fs/sysfs/dir.c                   |   24 +++++-
 include/linux/lock_monitor.h     |  171 ++++++++++++++++++++++++++++++++++++++
 include/linux/lockdep.h          |  138 ++++++-------------------------
 include/linux/mutex.h            |   14 ++--
 include/linux/rcupdate.h         |    4 +-
 include/linux/rwlock_api_smp.h   |   36 ++++----
 include/linux/rwlock_types.h     |   12 ++--
 include/linux/rwsem-spinlock.h   |   12 ++--
 include/linux/rwsem.h            |    2 +
 include/linux/semaphore.h        |    2 +-
 include/linux/spinlock.h         |    4 +-
 include/linux/spinlock_api_smp.h |   22 +++---
 include/linux/spinlock_types.h   |   16 ++--
 kernel/Makefile                  |    1 +
 kernel/lock_monitor.c            |  132 +++++++++++++++++++++++++++++
 kernel/lockdep.c                 |   51 ++++++++++--
 kernel/mutex-debug.c             |    2 +-
 kernel/mutex.c                   |   14 ++--
 kernel/rwsem.c                   |   16 ++--
 kernel/sched.c                   |    8 +-
 kernel/spinlock.c                |    8 +-
 lib/Kconfig.debug                |   16 +++-
 lib/rwsem-spinlock.c             |    2 +-
 lib/rwsem.c                      |    2 +-
 lib/spinlock_debug.c             |    4 +-
 net/core/sock.c                  |   15 ++++
 28 files changed, 515 insertions(+), 226 deletions(-)
 create mode 100644 include/linux/lock_monitor.h
 create mode 100644 kernel/lock_monitor.c

[PATCH RFC 01/11] lock monitor: New subsystem for lock event hooking

[Hitoshi Mitake]

Current lockdep is too complicated because,
 * dependency validation
 * statistics
 * event tracing
are all implemented by it.
This cause problem of overhead.
If user enables one of them, overhead of rests part is not avoidable.
(tracing is exception. If user enables validation or stat,
overhead of tracing doesn't occur.)

So I suggest new subsystem lock monitor.
This is a general purpose lock event hooking mechanism.

Programmer who want to hook lock event should prepare this structure,

struct lock_monitor_hook {
	struct list_head list;
	const char *name;

	void (*acquire)(struct lock_monitor *monitor, unsigned int subclass,
			int trylock, int read, int check,
			struct lock_monitor *nest_monitor, unsigned long ip);
	void (*acquired)(struct lock_monitor *monitor, unsigned long ip);
	void (*contended)(struct lock_monitor *monitor, unsigned long ip);
	void (*release)(struct lock_monitor *monitor, int nested,
			unsigned long ip);
};

e.g. lockdep is like this,
static struct lock_monitor_hook lockdep_hook = {
	.name = "lockdep",
	.acquire = lockdep_acquire_hook,
	.acquired = lockdep_acquired_hook,
	.contended = lockdep_contended_hook,
	.release = lockdep_release_hook,
};

Then registering can be done
	lock_monitor_register(&lockdep_hook);

This subsystem makes it possible to enable/disable
each features of lock monitoring. And adding new hook is easy.

Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Jason Baron <jbaron@redhat.com>
---
 include/linux/lock_monitor.h |  171 ++++++++++++++++++++++++++++++++++++++++++
 kernel/Makefile              |    1 +
 kernel/lock_monitor.c        |  132 ++++++++++++++++++++++++++++++++
 lib/Kconfig.debug            |   16 +++-
 4 files changed, 316 insertions(+), 4 deletions(-)
 create mode 100644 include/linux/lock_monitor.h
 create mode 100644 kernel/lock_monitor.c

diff --git a/include/linux/lock_monitor.h b/include/linux/lock_monitor.h
new file mode 100644
index 0000000..f1c8269
--- /dev/null
+++ b/include/linux/lock_monitor.h
@@ -0,0 +1,171 @@
+#ifndef __LINUX_LOCK_MONITOR_H
+#define __LINUX_LOCK_MONITOR_H
+
+#include <linux/list.h>
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+#include <linux/lockdep.h>
+#endif
+
+struct lock_monitor {
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	struct lockdep_map dep_map;
+#endif
+};
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+# define DEP_MAP_INIT(lockname)	.dep_map = { .name = #lockname }
+#else
+# define DEP_MAP_INIT(lockname)
+#endif
+
+#define __LOCK_MONITOR_INIT(lockname)		\
+	DEP_MAP_INIT(lockname)
+
+struct lock_monitor_hook {
+	struct list_head list;
+	const char *name;
+
+	void (*acquire)(struct lock_monitor *monitor, unsigned int subclass,
+			int trylock, int read, int check,
+			struct lock_monitor *nest_monitor, unsigned long ip);
+	void (*acquired)(struct lock_monitor *monitor, unsigned long ip);
+	void (*contended)(struct lock_monitor *monitor, unsigned long ip);
+	void (*release)(struct lock_monitor *monitor, int nested,
+			unsigned long ip);
+};
+
+#ifdef CONFIG_LOCK_MONITOR
+
+extern void lock_monitor_register(struct lock_monitor_hook *new_hook);
+extern void lock_monitor_unregister(const char *name);
+
+/*
+ * Acquire a lock.
+ *
+ * Values for "read":
+ *
+ *   0: exclusive (write) acquire
+ *   1: read-acquire (no recursion allowed)
+ *   2: read-acquire with same-instance recursion allowed
+ *
+ * Values for check:
+ *
+ *   0: disabled
+ *   1: simple checks (freeing, held-at-exit-time, etc.)
+ *   2: full validation
+ */
+extern void lock_acquire(struct lock_monitor *monitor, unsigned int subclass,
+			 int trylock, int read, int check,
+			 struct lock_monitor *nest_monitor, unsigned long ip);
+extern void lock_acquired(struct lock_monitor *monitor, unsigned long ip);
+extern void lock_contended(struct lock_monitor *monitor, unsigned long ip);
+extern void lock_release(struct lock_monitor *monitor, int nested,
+			 unsigned long ip);
+
+#define LOCK_CONTENDED(_lock, try, lock)				\
+	do {								\
+	if (!try(_lock)) {						\
+		lock_contended(&(_lock)->monitor, _RET_IP_);	\
+		lock(_lock);						\
+	}								\
+	lock_acquired(&(_lock)->monitor, _RET_IP_);		\
+} while (0)
+
+#define LOCK_CONTENDED_FLAGS(_lock, try, lock, lockfl, flags) \
+	LOCK_CONTENDED((_lock), (try), (lock))
+
+#else  /* CONFIG_LOCK_MONITOR */
+
+static inline void lock_monitor_register(struct lock_monitor_hook *new_hook)
+{
+}
+
+static inline void lock_monitor_unregister(const char *name)
+{
+}
+
+static inline void lock_acquire(struct lock_monitor *monitor, unsigned int subclass,
+			 int trylock, int read, int check,
+			 struct lock_monitor *nest_monitor, unsigned long ip)
+{
+}
+
+static inline void lock_acquired(struct lock_monitor *monitor, unsigned long ip)
+{
+}
+
+static inline void lock_contended(struct lock_monitor *monitor, unsigned long ip)
+{
+}
+
+static inline void lock_release(struct lock_monitor *monitor, int nested,
+			 unsigned long ip)
+{
+}
+
+#define LOCK_CONTENDED(_lock, try, lock)	\
+	lock(_lock);
+
+#define LOCK_CONTENDED_FLAGS(_lock, try, lock, lockfl, flags)	\
+	lock(_lock);
+
+#endif
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+# ifdef CONFIG_PROVE_LOCKING
+#  define spin_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 2, NULL, i)
+#  define spin_acquire_nest(l, s, t, n, i)	lock_acquire(l, s, t, 0, 2, n, i)
+# else
+#  define spin_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 1, NULL, i)
+#  define spin_acquire_nest(l, s, t, n, i)	lock_acquire(l, s, t, 0, 1, NULL, i)
+# endif
+# define spin_release(l, n, i)			lock_release(l, n, i)
+#else
+# define spin_acquire(l, s, t, i)		do { } while (0)
+# define spin_release(l, n, i)			do { } while (0)
+#endif
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+# ifdef CONFIG_PROVE_LOCKING
+#  define rwlock_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 2, NULL, i)
+#  define rwlock_acquire_read(l, s, t, i)	lock_acquire(l, s, t, 2, 2, NULL, i)
+# else
+#  define rwlock_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 1, NULL, i)
+#  define rwlock_acquire_read(l, s, t, i)	lock_acquire(l, s, t, 2, 1, NULL, i)
+# endif
+# define rwlock_release(l, n, i)		lock_release(l, n, i)
+#else
+# define rwlock_acquire(l, s, t, i)		do { } while (0)
+# define rwlock_acquire_read(l, s, t, i)	do { } while (0)
+# define rwlock_release(l, n, i)		do { } while (0)
+#endif
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+# ifdef CONFIG_PROVE_LOCKING
+#  define mutex_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 2, NULL, i)
+# else
+#  define mutex_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 1, NULL, i)
+# endif
+# define mutex_release(l, n, i)			lock_release(l, n, i)
+#else
+# define mutex_acquire(l, s, t, i)		do { } while (0)
+# define mutex_release(l, n, i)			do { } while (0)
+#endif
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+# ifdef CONFIG_PROVE_LOCKING
+#  define rwsem_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 2, NULL, i)
+#  define rwsem_acquire_read(l, s, t, i)	lock_acquire(l, s, t, 1, 2, NULL, i)
+# else
+#  define rwsem_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 1, NULL, i)
+#  define rwsem_acquire_read(l, s, t, i)	lock_acquire(l, s, t, 1, 1, NULL, i)
+# endif
+# define rwsem_release(l, n, i)			lock_release(l, n, i)
+#else
+# define rwsem_acquire(l, s, t, i)		do { } while (0)
+# define rwsem_acquire_read(l, s, t, i)		do { } while (0)
+# define rwsem_release(l, n, i)			do { } while (0)
+#endif
+
+#endif	/* __LINUX_LOCK_MONITOR_H */
diff --git a/kernel/Makefile b/kernel/Makefile
index 864ff75..aec9155 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_STACKTRACE) += stacktrace.o
 obj-y += time/
 obj-$(CONFIG_DEBUG_MUTEXES) += mutex-debug.o
 obj-$(CONFIG_LOCKDEP) += lockdep.o
+obj-$(CONFIG_LOCK_MONITOR) += lock_monitor.o
 ifeq ($(CONFIG_PROC_FS),y)
 obj-$(CONFIG_LOCKDEP) += lockdep_proc.o
 endif
diff --git a/kernel/lock_monitor.c b/kernel/lock_monitor.c
new file mode 100644
index 0000000..596aead
--- /dev/null
+++ b/kernel/lock_monitor.c
@@ -0,0 +1,132 @@
+/* 
+ * Lock monitor ... general lock event hooking mechanism
+ * Started by Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp> 
+ */
+
+#include <linux/lock_monitor.h>
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+#include <linux/lockdep.h>
+#endif
+
+static arch_rwlock_t hook_lock = __ARCH_RW_LOCK_UNLOCKED;
+static LIST_HEAD(lock_monitor_hooks);
+
+void lock_monitor_register(struct lock_monitor_hook *new_hook)
+{
+	arch_write_lock(&hook_lock);
+	list_add(&new_hook->list, &lock_monitor_hooks);
+	arch_write_unlock(&hook_lock);
+
+	printk(KERN_INFO "new lock hook:%s registered\n", new_hook->name);
+}
+EXPORT_SYMBOL(lock_monitor_register);
+
+void lock_monitor_unregister(const char *name)
+{
+	struct list_head *l;
+	struct lock_monitor_hook *hook;
+
+	arch_write_lock(&hook_lock);
+
+	list_for_each(l, &lock_monitor_hooks) {
+		hook = container_of(l, struct lock_monitor_hook, list);
+		if (!strcmp(hook->name, name)) {
+			list_del(l);
+			printk(KERN_INFO "lock hook:%s unregistered\n", hook->name);
+			goto end;
+		}
+	}
+
+	printk(KERN_ERR "request occured for unregistering "
+	       "unknown look hook:%s\n", name);
+
+end:
+	arch_write_unlock(&hook_lock);
+}
+EXPORT_SYMBOL(lock_monitor_unregister);
+
+void lock_acquire(struct lock_monitor *monitor, unsigned int subclass,
+			 int trylock, int read, int check,
+			 struct lock_monitor *nest_monitor, unsigned long ip)
+{
+	struct list_head *l;
+	struct lock_monitor_hook *hook;
+	unsigned long flags;
+
+	local_irq_save(flags);
+	arch_read_lock(&hook_lock);
+
+	list_for_each(l, &lock_monitor_hooks) {
+		hook = container_of(l, struct lock_monitor_hook, list);
+		hook->acquire(monitor, subclass, trylock, read, check, nest_monitor, ip);
+	}
+
+	arch_read_unlock(&hook_lock);
+	local_irq_restore(flags);
+}
+EXPORT_SYMBOL(lock_acquire);
+
+void lock_acquired(struct lock_monitor *monitor, unsigned long ip)
+{
+	struct list_head *l;
+	struct lock_monitor_hook *hook;
+	unsigned long flags;
+
+	local_irq_save(flags);
+	arch_read_lock(&hook_lock);
+
+	list_for_each(l, &lock_monitor_hooks) {
+		hook = container_of(l, struct lock_monitor_hook, list);
+		hook->acquired(monitor, ip);
+	}
+
+	arch_read_unlock(&hook_lock);
+	local_irq_restore(flags);
+}
+EXPORT_SYMBOL(lock_acquired);
+
+
+void lock_contended(struct lock_monitor *monitor, unsigned long ip)
+{
+	struct list_head *l;
+	struct lock_monitor_hook *hook;
+	unsigned long flags;
+
+	local_irq_save(flags);
+	arch_read_lock(&hook_lock);
+
+	list_for_each(l, &lock_monitor_hooks) {
+		hook = container_of(l, struct lock_monitor_hook, list);
+		hook->contended(monitor, ip);
+	}
+
+	arch_read_unlock(&hook_lock);
+	local_irq_restore(flags);
+}
+EXPORT_SYMBOL(lock_contended);
+
+void lock_release(struct lock_monitor *monitor, int nested,
+			 unsigned long ip)
+{
+	struct list_head *l;
+	struct lock_monitor_hook *hook;
+	unsigned long flags;
+
+	local_irq_save(flags);
+	arch_read_lock(&hook_lock);
+
+	list_for_each(l, &lock_monitor_hooks) {
+		hook = container_of(l, struct lock_monitor_hook, list);
+		hook->release(monitor, nested, ip);
+	}
+
+	arch_read_unlock(&hook_lock);
+	local_irq_restore(flags);
+}
+EXPORT_SYMBOL(lock_release);
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 25c3ed5..89636c2 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -443,9 +443,17 @@ config DEBUG_MUTEXES
 	 This feature allows mutex semantics violations to be detected and
 	 reported.
 
+config LOCK_MONITOR
+	bool "Lock monitoring"
+	depends on DEBUG_KERNEL
+	help
+	  Enable lock monitor.
+	  Lock monitor is a generic lock event hooking mechanism.
+	  You can add any hooks to events of acquire, acquired, contended, release.
+
 config DEBUG_LOCK_ALLOC
 	bool "Lock debugging: detect incorrect freeing of live locks"
-	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
+	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT && LOCK_MONITOR
 	select DEBUG_SPINLOCK
 	select DEBUG_MUTEXES
 	select LOCKDEP
@@ -459,7 +467,7 @@ config DEBUG_LOCK_ALLOC
 
 config PROVE_LOCKING
 	bool "Lock debugging: prove locking correctness"
-	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
+	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT && LOCK_MONITOR
 	select LOCKDEP
 	select DEBUG_SPINLOCK
 	select DEBUG_MUTEXES
@@ -501,7 +509,7 @@ config PROVE_LOCKING
 
 config LOCKDEP
 	bool
-	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
+	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT && LOCK_MONITOR
 	select STACKTRACE
 	select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND && !S390
 	select KALLSYMS
@@ -509,7 +517,7 @@ config LOCKDEP
 
 config LOCK_STAT
 	bool "Lock usage statistics"
-	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
+	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT && LOCK_MONITOR
 	select LOCKDEP
 	select DEBUG_SPINLOCK
 	select DEBUG_MUTEXES
-- 
1.6.5.2

[PATCH RFC 02/11] Adopt lockdep to lock monitor

[Hitoshi Mitake]

Now lockdep is a hook of lock monitor.

lockdep still does stat and event tracing.
I'll remove these from it later. And these can be
individual hook of lock monitor.

Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Jason Baron <jbaron@redhat.com>
---
 include/linux/lockdep.h |  138 ++++++++--------------------------------------
 kernel/lockdep.c        |   51 +++++++++++++++---
 2 files changed, 67 insertions(+), 122 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 371161c..1faa4e1 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -261,18 +261,18 @@ extern void lockdep_init_map(struct lockdep_map *lock, const char *name,
  * or they are too narrow (they suffer from a false class-split):
  */
 #define lockdep_set_class(lock, key) \
-		lockdep_init_map(&(lock)->dep_map, #key, key, 0)
+		lockdep_init_map(&(lock)->monitor.dep_map, #key, key, 0)
 #define lockdep_set_class_and_name(lock, key, name) \
-		lockdep_init_map(&(lock)->dep_map, name, key, 0)
+		lockdep_init_map(&(lock)->monitor.dep_map, name, key, 0)
 #define lockdep_set_class_and_subclass(lock, key, sub) \
-		lockdep_init_map(&(lock)->dep_map, #key, key, sub)
+		lockdep_init_map(&(lock)->monitor.dep_map, #key, key, sub)
 #define lockdep_set_subclass(lock, sub)	\
-		lockdep_init_map(&(lock)->dep_map, #lock, \
-				 (lock)->dep_map.key, sub)
+		lockdep_init_map(&(lock)->monitor.dep_map, #lock, \
+				 (lock)->monitor.dep_map.key, sub)
 /*
  * Compare locking classes
  */
-#define lockdep_match_class(lock, key) lockdep_match_key(&(lock)->dep_map, key)
+#define lockdep_match_class(lock, key) lockdep_match_key(&(lock)->monitor.dep_map, key)
 
 static inline int lockdep_match_key(struct lockdep_map *lock,
 				    struct lock_class_key *key)
@@ -295,14 +295,14 @@ static inline int lockdep_match_key(struct lockdep_map *lock,
  *   1: simple checks (freeing, held-at-exit-time, etc.)
  *   2: full validation
  */
-extern void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
+extern void lockdep_acquire(struct lockdep_map *lock, unsigned int subclass,
 			 int trylock, int read, int check,
 			 struct lockdep_map *nest_lock, unsigned long ip);
 
-extern void lock_release(struct lockdep_map *lock, int nested,
+extern void lockdep_release(struct lockdep_map *lock, int nested,
 			 unsigned long ip);
 
-#define lockdep_is_held(lock)	lock_is_held(&(lock)->dep_map)
+#define lockdep_is_held(lock)	lock_is_held(&(lock)->monitor.dep_map)
 
 extern int lock_is_held(struct lockdep_map *lock);
 
@@ -340,8 +340,8 @@ static inline void lockdep_on(void)
 {
 }
 
-# define lock_acquire(l, s, t, r, c, n, i)	do { } while (0)
-# define lock_release(l, n, i)			do { } while (0)
+# define lockdep_acquire(l, s, t, r, c, n, i)	do { } while (0)
+# define lockdep_release(l, n, i)		do { } while (0)
 # define lock_set_class(l, n, k, s, i)		do { } while (0)
 # define lock_set_subclass(l, s, i)		do { } while (0)
 # define lockdep_set_current_reclaim_state(g)	do { } while (0)
@@ -380,45 +380,16 @@ struct lock_class_key { };
 
 #ifdef CONFIG_LOCK_STAT
 
-extern void lock_contended(struct lockdep_map *lock, unsigned long ip);
-extern void lock_acquired(struct lockdep_map *lock, unsigned long ip);
-
-#define LOCK_CONTENDED(_lock, try, lock)			\
-do {								\
-	if (!try(_lock)) {					\
-		lock_contended(&(_lock)->dep_map, _RET_IP_);	\
-		lock(_lock);					\
-	}							\
-	lock_acquired(&(_lock)->dep_map, _RET_IP_);			\
-} while (0)
+extern void lockdep_contended(struct lockdep_map *lock, unsigned long ip);
+extern void lockdep_acquired(struct lockdep_map *lock, unsigned long ip);
 
 #else /* CONFIG_LOCK_STAT */
 
-#define lock_contended(lockdep_map, ip) do {} while (0)
-#define lock_acquired(lockdep_map, ip) do {} while (0)
-
-#define LOCK_CONTENDED(_lock, try, lock) \
-	lock(_lock)
+#define lockdep_contended(lockdep_map, ip) do {} while (0)
+#define lockdep_acquired(lockdep_map, ip) do {} while (0)
 
 #endif /* CONFIG_LOCK_STAT */
 
-#ifdef CONFIG_LOCKDEP
-
-/*
- * On lockdep we dont want the hand-coded irq-enable of
- * _raw_*_lock_flags() code, because lockdep assumes
- * that interrupts are not re-enabled during lock-acquire:
- */
-#define LOCK_CONTENDED_FLAGS(_lock, try, lock, lockfl, flags) \
-	LOCK_CONTENDED((_lock), (try), (lock))
-
-#else /* CONFIG_LOCKDEP */
-
-#define LOCK_CONTENDED_FLAGS(_lock, try, lock, lockfl, flags) \
-	lockfl((_lock), (flags))
-
-#endif /* CONFIG_LOCKDEP */
-
 #ifdef CONFIG_GENERIC_HARDIRQS
 extern void early_init_irq_lock_class(void);
 #else
@@ -450,74 +421,13 @@ static inline void print_irqtrace_events(struct task_struct *curr)
  */
 #define SINGLE_DEPTH_NESTING			1
 
-/*
- * Map the dependency ops to NOP or to real lockdep ops, depending
- * on the per lock-class debug mode:
- */
-
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-# ifdef CONFIG_PROVE_LOCKING
-#  define spin_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 2, NULL, i)
-#  define spin_acquire_nest(l, s, t, n, i)	lock_acquire(l, s, t, 0, 2, n, i)
-# else
-#  define spin_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 1, NULL, i)
-#  define spin_acquire_nest(l, s, t, n, i)	lock_acquire(l, s, t, 0, 1, NULL, i)
-# endif
-# define spin_release(l, n, i)			lock_release(l, n, i)
-#else
-# define spin_acquire(l, s, t, i)		do { } while (0)
-# define spin_release(l, n, i)			do { } while (0)
-#endif
-
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-# ifdef CONFIG_PROVE_LOCKING
-#  define rwlock_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 2, NULL, i)
-#  define rwlock_acquire_read(l, s, t, i)	lock_acquire(l, s, t, 2, 2, NULL, i)
-# else
-#  define rwlock_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 1, NULL, i)
-#  define rwlock_acquire_read(l, s, t, i)	lock_acquire(l, s, t, 2, 1, NULL, i)
-# endif
-# define rwlock_release(l, n, i)		lock_release(l, n, i)
-#else
-# define rwlock_acquire(l, s, t, i)		do { } while (0)
-# define rwlock_acquire_read(l, s, t, i)	do { } while (0)
-# define rwlock_release(l, n, i)		do { } while (0)
-#endif
-
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-# ifdef CONFIG_PROVE_LOCKING
-#  define mutex_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 2, NULL, i)
-# else
-#  define mutex_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 1, NULL, i)
-# endif
-# define mutex_release(l, n, i)			lock_release(l, n, i)
-#else
-# define mutex_acquire(l, s, t, i)		do { } while (0)
-# define mutex_release(l, n, i)			do { } while (0)
-#endif
-
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-# ifdef CONFIG_PROVE_LOCKING
-#  define rwsem_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 2, NULL, i)
-#  define rwsem_acquire_read(l, s, t, i)	lock_acquire(l, s, t, 1, 2, NULL, i)
-# else
-#  define rwsem_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 1, NULL, i)
-#  define rwsem_acquire_read(l, s, t, i)	lock_acquire(l, s, t, 1, 1, NULL, i)
-# endif
-# define rwsem_release(l, n, i)			lock_release(l, n, i)
-#else
-# define rwsem_acquire(l, s, t, i)		do { } while (0)
-# define rwsem_acquire_read(l, s, t, i)		do { } while (0)
-# define rwsem_release(l, n, i)			do { } while (0)
-#endif
-
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 # ifdef CONFIG_PROVE_LOCKING
-#  define lock_map_acquire(l)		lock_acquire(l, 0, 0, 0, 2, NULL, _THIS_IP_)
+#  define lock_map_acquire(l)		lockdep_acquire(l, 0, 0, 0, 2, NULL, _THIS_IP_)
 # else
-#  define lock_map_acquire(l)		lock_acquire(l, 0, 0, 0, 1, NULL, _THIS_IP_)
+#  define lock_map_acquire(l)		lockdep_acquire(l, 0, 0, 0, 1, NULL, _THIS_IP_)
 # endif
-# define lock_map_release(l)			lock_release(l, 1, _THIS_IP_)
+# define lock_map_release(l)			lockdep_release(l, 1, _THIS_IP_)
 #else
 # define lock_map_acquire(l)			do { } while (0)
 # define lock_map_release(l)			do { } while (0)
@@ -526,15 +436,15 @@ static inline void print_irqtrace_events(struct task_struct *curr)
 #ifdef CONFIG_PROVE_LOCKING
 # define might_lock(lock) 						\
 do {									\
-	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
-	lock_acquire(&(lock)->dep_map, 0, 0, 0, 2, NULL, _THIS_IP_);	\
-	lock_release(&(lock)->dep_map, 0, _THIS_IP_);			\
+	typecheck(struct lockdep_map *, &(lock)->monitor.dep_map);	\
+	lockdep_acquire(&(lock)->monitor.dep_map, 0, 0, 0, 2, NULL, _THIS_IP_);	\
+	lockdep_release(&(lock)->monitor.dep_map, 0, _THIS_IP_);		\
 } while (0)
 # define might_lock_read(lock) 						\
 do {									\
-	typecheck(struct lockdep_map *, &(lock)->dep_map);		\
-	lock_acquire(&(lock)->dep_map, 0, 0, 1, 2, NULL, _THIS_IP_);	\
-	lock_release(&(lock)->dep_map, 0, _THIS_IP_);			\
+	typecheck(struct lockdep_map *, &(lock)->monitor.dep_map);	\
+	lockdep_acquire(&(lock)->monitor.dep_map, 0, 0, 1, 2, NULL, _THIS_IP_);	\
+	lockdep_release(&(lock)->monitor.dep_map, 0, _THIS_IP_);	\
 } while (0)
 #else
 # define might_lock(lock) do { } while (0)
diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index da7435d..f7600a5 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -43,6 +43,7 @@
 #include <linux/ftrace.h>
 #include <linux/stringify.h>
 #include <linux/bitops.h>
+#include <linux/lock_monitor.h>
 
 #include <asm/sections.h>
 
@@ -3208,7 +3209,7 @@ EXPORT_SYMBOL_GPL(lock_set_class);
  * We are not always called with irqs disabled - do that here,
  * and also avoid lockdep recursion:
  */
-void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
+void lockdep_acquire(struct lockdep_map *lock, unsigned int subclass,
 			  int trylock, int read, int check,
 			  struct lockdep_map *nest_lock, unsigned long ip)
 {
@@ -3228,9 +3229,9 @@ void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 	current->lockdep_recursion = 0;
 	raw_local_irq_restore(flags);
 }
-EXPORT_SYMBOL_GPL(lock_acquire);
+EXPORT_SYMBOL_GPL(lockdep_acquire);
 
-void lock_release(struct lockdep_map *lock, int nested,
+void lockdep_release(struct lockdep_map *lock, int nested,
 			  unsigned long ip)
 {
 	unsigned long flags;
@@ -3247,7 +3248,7 @@ void lock_release(struct lockdep_map *lock, int nested,
 	current->lockdep_recursion = 0;
 	raw_local_irq_restore(flags);
 }
-EXPORT_SYMBOL_GPL(lock_release);
+EXPORT_SYMBOL_GPL(lockdep_release);
 
 int lock_is_held(struct lockdep_map *lock)
 {
@@ -3412,7 +3413,7 @@ found_it:
 	lock->ip = ip;
 }
 
-void lock_contended(struct lockdep_map *lock, unsigned long ip)
+void lockdep_contended(struct lockdep_map *lock, unsigned long ip)
 {
 	unsigned long flags;
 
@@ -3431,9 +3432,9 @@ void lock_contended(struct lockdep_map *lock, unsigned long ip)
 	current->lockdep_recursion = 0;
 	raw_local_irq_restore(flags);
 }
-EXPORT_SYMBOL_GPL(lock_contended);
+EXPORT_SYMBOL_GPL(lockdep_contended);
 
-void lock_acquired(struct lockdep_map *lock, unsigned long ip)
+void lockdep_acquired(struct lockdep_map *lock, unsigned long ip)
 {
 	unsigned long flags;
 
@@ -3450,7 +3451,7 @@ void lock_acquired(struct lockdep_map *lock, unsigned long ip)
 	current->lockdep_recursion = 0;
 	raw_local_irq_restore(flags);
 }
-EXPORT_SYMBOL_GPL(lock_acquired);
+EXPORT_SYMBOL_GPL(lockdep_acquired);
 #endif
 
 /*
@@ -3579,6 +3580,38 @@ out_restore:
 	raw_local_irq_restore(flags);
 }
 
+void lockdep_acquire_hook(struct lock_monitor *monitor, unsigned int subclass,
+			  int trylock, int read, int check,
+			  struct lock_monitor *nest_monitor, unsigned long ip)
+{
+	lockdep_acquire(&monitor->dep_map, subclass, trylock, read,
+			check, nest_monitor ? &nest_monitor->dep_map : NULL, ip);
+}
+
+void lockdep_acquired_hook(struct lock_monitor *monitor, unsigned long ip)
+{
+	lockdep_acquired(&monitor->dep_map, ip);
+}
+
+void lockdep_contended_hook(struct lock_monitor *monitor, unsigned long ip)
+{
+	lockdep_contended(&monitor->dep_map, ip);
+}
+
+void lockdep_release_hook(struct lock_monitor *monitor, int nested,
+			  unsigned long ip)
+{
+	lockdep_release(&monitor->dep_map, nested, ip);
+}
+
+static struct lock_monitor_hook lockdep_hook = {
+	.name = "lockdep",
+	.acquire = lockdep_acquire_hook,
+	.acquired = lockdep_acquired_hook,
+	.contended = lockdep_contended_hook,
+	.release = lockdep_release_hook,
+};
+
 void lockdep_init(void)
 {
 	int i;
@@ -3598,6 +3631,8 @@ void lockdep_init(void)
 	for (i = 0; i < CHAINHASH_SIZE; i++)
 		INIT_LIST_HEAD(chainhash_table + i);
 
+	lock_monitor_register(&lockdep_hook);
+
 	lockdep_initialized = 1;
 }
 
-- 
1.6.5.2

[PATCH RFC 03/11] Adopt spinlock to lock monitor

[Hitoshi Mitake]

Current spinlock_t holds struct lockdep_map,
but it is a special thing of lockdep subsystem.

So I replaced it with struct lock_monitor.
Now it contains lockdep_map, and adding new members is easy.

Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Jason Baron <jbaron@redhat.com>
---
 include/linux/spinlock.h         |    4 ++--
 include/linux/spinlock_api_smp.h |   22 +++++++++++-----------
 include/linux/spinlock_types.h   |   16 ++++++++--------
 kernel/spinlock.c                |    8 ++++----
 lib/spinlock_debug.c             |    4 ++--
 5 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 8608821..98a6314 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -171,8 +171,8 @@ static inline void do_raw_spin_unlock(raw_spinlock_t *lock)
 
 # define raw_spin_lock_nest_lock(lock, nest_lock)			\
 	 do {								\
-		 typecheck(struct lockdep_map *, &(nest_lock)->dep_map);\
-		 _raw_spin_lock_nest_lock(lock, &(nest_lock)->dep_map);	\
+		 typecheck(struct lock_monitor *, &(nest_lock)->monitor);\
+		 _raw_spin_lock_nest_lock(lock, &(nest_lock)->monitor);	\
 	 } while (0)
 #else
 # define raw_spin_lock_nested(lock, subclass)		_raw_spin_lock(lock)
diff --git a/include/linux/spinlock_api_smp.h b/include/linux/spinlock_api_smp.h
index e253ccd..3290593 100644
--- a/include/linux/spinlock_api_smp.h
+++ b/include/linux/spinlock_api_smp.h
@@ -23,7 +23,7 @@ void __lockfunc _raw_spin_lock(raw_spinlock_t *lock)		__acquires(lock);
 void __lockfunc _raw_spin_lock_nested(raw_spinlock_t *lock, int subclass)
 								__acquires(lock);
 void __lockfunc
-_raw_spin_lock_nest_lock(raw_spinlock_t *lock, struct lockdep_map *map)
+_raw_spin_lock_nest_lock(raw_spinlock_t *lock, struct lock_monitor *monitor)
 								__acquires(lock);
 void __lockfunc _raw_spin_lock_bh(raw_spinlock_t *lock)		__acquires(lock);
 void __lockfunc _raw_spin_lock_irq(raw_spinlock_t *lock)
@@ -87,7 +87,7 @@ static inline int __raw_spin_trylock(raw_spinlock_t *lock)
 {
 	preempt_disable();
 	if (do_raw_spin_trylock(lock)) {
-		spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
+		spin_acquire(&lock->monitor, 0, 1, _RET_IP_);
 		return 1;
 	}
 	preempt_enable();
@@ -107,7 +107,7 @@ static inline unsigned long __raw_spin_lock_irqsave(raw_spinlock_t *lock)
 
 	local_irq_save(flags);
 	preempt_disable();
-	spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
+	spin_acquire(&lock->monitor, 0, 0, _RET_IP_);
 	/*
 	 * On lockdep we dont want the hand-coded irq-enable of
 	 * do_raw_spin_lock_flags() code, because lockdep assumes
@@ -125,7 +125,7 @@ static inline void __raw_spin_lock_irq(raw_spinlock_t *lock)
 {
 	local_irq_disable();
 	preempt_disable();
-	spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
+	spin_acquire(&lock->monitor, 0, 0, _RET_IP_);
 	LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
 }
 
@@ -133,14 +133,14 @@ static inline void __raw_spin_lock_bh(raw_spinlock_t *lock)
 {
 	local_bh_disable();
 	preempt_disable();
-	spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
+	spin_acquire(&lock->monitor, 0, 0, _RET_IP_);
 	LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
 }
 
 static inline void __raw_spin_lock(raw_spinlock_t *lock)
 {
 	preempt_disable();
-	spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
+	spin_acquire(&lock->monitor, 0, 0, _RET_IP_);
 	LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
 }
 
@@ -148,7 +148,7 @@ static inline void __raw_spin_lock(raw_spinlock_t *lock)
 
 static inline void __raw_spin_unlock(raw_spinlock_t *lock)
 {
-	spin_release(&lock->dep_map, 1, _RET_IP_);
+	spin_release(&lock->monitor, 1, _RET_IP_);
 	do_raw_spin_unlock(lock);
 	preempt_enable();
 }
@@ -156,7 +156,7 @@ static inline void __raw_spin_unlock(raw_spinlock_t *lock)
 static inline void __raw_spin_unlock_irqrestore(raw_spinlock_t *lock,
 					    unsigned long flags)
 {
-	spin_release(&lock->dep_map, 1, _RET_IP_);
+	spin_release(&lock->monitor, 1, _RET_IP_);
 	do_raw_spin_unlock(lock);
 	local_irq_restore(flags);
 	preempt_enable();
@@ -164,7 +164,7 @@ static inline void __raw_spin_unlock_irqrestore(raw_spinlock_t *lock,
 
 static inline void __raw_spin_unlock_irq(raw_spinlock_t *lock)
 {
-	spin_release(&lock->dep_map, 1, _RET_IP_);
+	spin_release(&lock->monitor, 1, _RET_IP_);
 	do_raw_spin_unlock(lock);
 	local_irq_enable();
 	preempt_enable();
@@ -172,7 +172,7 @@ static inline void __raw_spin_unlock_irq(raw_spinlock_t *lock)
 
 static inline void __raw_spin_unlock_bh(raw_spinlock_t *lock)
 {
-	spin_release(&lock->dep_map, 1, _RET_IP_);
+	spin_release(&lock->monitor, 1, _RET_IP_);
 	do_raw_spin_unlock(lock);
 	preempt_enable_no_resched();
 	local_bh_enable_ip((unsigned long)__builtin_return_address(0));
@@ -183,7 +183,7 @@ static inline int __raw_spin_trylock_bh(raw_spinlock_t *lock)
 	local_bh_disable();
 	preempt_disable();
 	if (do_raw_spin_trylock(lock)) {
-		spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
+		spin_acquire(&lock->monitor, 0, 1, _RET_IP_);
 		return 1;
 	}
 	preempt_enable_no_resched();
diff --git a/include/linux/spinlock_types.h b/include/linux/spinlock_types.h
index 851b778..cb113f5 100644
--- a/include/linux/spinlock_types.h
+++ b/include/linux/spinlock_types.h
@@ -15,7 +15,7 @@
 # include <linux/spinlock_types_up.h>
 #endif
 
-#include <linux/lockdep.h>
+#include <linux/lock_monitor.h>
 
 typedef struct raw_spinlock {
 	arch_spinlock_t raw_lock;
@@ -26,8 +26,8 @@ typedef struct raw_spinlock {
 	unsigned int magic, owner_cpu;
 	void *owner;
 #endif
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-	struct lockdep_map dep_map;
+#ifdef CONFIG_LOCK_MONITOR
+	struct lock_monitor monitor;
 #endif
 } raw_spinlock_t;
 
@@ -36,9 +36,9 @@ typedef struct raw_spinlock {
 #define SPINLOCK_OWNER_INIT	((void *)-1L)
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
-# define SPIN_DEP_MAP_INIT(lockname)	.dep_map = { .name = #lockname }
+# define SPIN_LOCK_MONITOR_INIT(lockname)	.monitor = { __LOCK_MONITOR_INIT(lockname) }
 #else
-# define SPIN_DEP_MAP_INIT(lockname)
+# define SPIN_LOCK_MONITOR_INIT(lockname)
 #endif
 
 #ifdef CONFIG_DEBUG_SPINLOCK
@@ -54,7 +54,7 @@ typedef struct raw_spinlock {
 	{					\
 	.raw_lock = __ARCH_SPIN_LOCK_UNLOCKED,	\
 	SPIN_DEBUG_INIT(lockname)		\
-	SPIN_DEP_MAP_INIT(lockname) }
+	SPIN_LOCK_MONITOR_INIT(lockname) }
 
 #define __RAW_SPIN_LOCK_UNLOCKED(lockname)	\
 	(raw_spinlock_t) __RAW_SPIN_LOCK_INITIALIZER(lockname)
@@ -66,10 +66,10 @@ typedef struct spinlock {
 		struct raw_spinlock rlock;
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
-# define LOCK_PADSIZE (offsetof(struct raw_spinlock, dep_map))
+# define LOCK_PADSIZE (offsetof(struct raw_spinlock, monitor))
 		struct {
 			u8 __padding[LOCK_PADSIZE];
-			struct lockdep_map dep_map;
+			struct lock_monitor monitor;
 		};
 #endif
 	};
diff --git a/kernel/spinlock.c b/kernel/spinlock.c
index be6517f..f2150d5 100644
--- a/kernel/spinlock.c
+++ b/kernel/spinlock.c
@@ -344,7 +344,7 @@ EXPORT_SYMBOL(_raw_write_unlock_bh);
 void __lockfunc _raw_spin_lock_nested(raw_spinlock_t *lock, int subclass)
 {
 	preempt_disable();
-	spin_acquire(&lock->dep_map, subclass, 0, _RET_IP_);
+	spin_acquire(&lock->monitor, subclass, 0, _RET_IP_);
 	LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
 }
 EXPORT_SYMBOL(_raw_spin_lock_nested);
@@ -356,7 +356,7 @@ unsigned long __lockfunc _raw_spin_lock_irqsave_nested(raw_spinlock_t *lock,
 
 	local_irq_save(flags);
 	preempt_disable();
-	spin_acquire(&lock->dep_map, subclass, 0, _RET_IP_);
+	spin_acquire(&lock->monitor, subclass, 0, _RET_IP_);
 	LOCK_CONTENDED_FLAGS(lock, do_raw_spin_trylock, do_raw_spin_lock,
 				do_raw_spin_lock_flags, &flags);
 	return flags;
@@ -364,10 +364,10 @@ unsigned long __lockfunc _raw_spin_lock_irqsave_nested(raw_spinlock_t *lock,
 EXPORT_SYMBOL(_raw_spin_lock_irqsave_nested);
 
 void __lockfunc _raw_spin_lock_nest_lock(raw_spinlock_t *lock,
-				     struct lockdep_map *nest_lock)
+				     struct lock_monitor *nest_monitor)
 {
 	preempt_disable();
-	spin_acquire_nest(&lock->dep_map, 0, 0, nest_lock, _RET_IP_);
+	spin_acquire_nest(&lock->monitor, 0, 0, nest_monitor, _RET_IP_);
 	LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
 }
 EXPORT_SYMBOL(_raw_spin_lock_nest_lock);
diff --git a/lib/spinlock_debug.c b/lib/spinlock_debug.c
index 4755b98..b4b3bbe 100644
--- a/lib/spinlock_debug.c
+++ b/lib/spinlock_debug.c
@@ -21,7 +21,7 @@ void __raw_spin_lock_init(raw_spinlock_t *lock, const char *name,
 	 * Make sure we are not reinitializing a held lock:
 	 */
 	debug_check_no_locks_freed((void *)lock, sizeof(*lock));
-	lockdep_init_map(&lock->dep_map, name, key, 0);
+	lockdep_init_map(&lock->monitor.dep_map, name, key, 0);
 #endif
 	lock->raw_lock = (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED;
 	lock->magic = SPINLOCK_MAGIC;
@@ -39,7 +39,7 @@ void __rwlock_init(rwlock_t *lock, const char *name,
 	 * Make sure we are not reinitializing a held lock:
 	 */
 	debug_check_no_locks_freed((void *)lock, sizeof(*lock));
-	lockdep_init_map(&lock->dep_map, name, key, 0);
+	lockdep_init_map(&lock->monitor.dep_map, name, key, 0);
 #endif
 	lock->raw_lock = (arch_rwlock_t) __ARCH_RW_LOCK_UNLOCKED;
 	lock->magic = RWLOCK_MAGIC;
-- 
1.6.5.2

[PATCH RFC 04/11] Adopt rwlock to lock monitor

[Hitoshi Mitake]

Current rwlock_t holds struct lockdep_map,
but it is a special thing of lockdep subsystem.

So I replaced it with struct lock_monitor.
Now it contains lockdep_map, and adding new members is easy.

Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Jason Baron <jbaron@redhat.com>
---
 include/linux/rwlock_api_smp.h |   36 ++++++++++++++++++------------------
 include/linux/rwlock_types.h   |   12 ++++++------
 2 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/include/linux/rwlock_api_smp.h b/include/linux/rwlock_api_smp.h
index 9c9f049..d8458d9 100644
--- a/include/linux/rwlock_api_smp.h
+++ b/include/linux/rwlock_api_smp.h
@@ -118,7 +118,7 @@ static inline int __raw_read_trylock(rwlock_t *lock)
 {
 	preempt_disable();
 	if (do_raw_read_trylock(lock)) {
-		rwlock_acquire_read(&lock->dep_map, 0, 1, _RET_IP_);
+		rwlock_acquire_read(&lock->monitor, 0, 1, _RET_IP_);
 		return 1;
 	}
 	preempt_enable();
@@ -129,7 +129,7 @@ static inline int __raw_write_trylock(rwlock_t *lock)
 {
 	preempt_disable();
 	if (do_raw_write_trylock(lock)) {
-		rwlock_acquire(&lock->dep_map, 0, 1, _RET_IP_);
+		rwlock_acquire(&lock->monitor, 0, 1, _RET_IP_);
 		return 1;
 	}
 	preempt_enable();
@@ -146,7 +146,7 @@ static inline int __raw_write_trylock(rwlock_t *lock)
 static inline void __raw_read_lock(rwlock_t *lock)
 {
 	preempt_disable();
-	rwlock_acquire_read(&lock->dep_map, 0, 0, _RET_IP_);
+	rwlock_acquire_read(&lock->monitor, 0, 0, _RET_IP_);
 	LOCK_CONTENDED(lock, do_raw_read_trylock, do_raw_read_lock);
 }
 
@@ -156,7 +156,7 @@ static inline unsigned long __raw_read_lock_irqsave(rwlock_t *lock)
 
 	local_irq_save(flags);
 	preempt_disable();
-	rwlock_acquire_read(&lock->dep_map, 0, 0, _RET_IP_);
+	rwlock_acquire_read(&lock->monitor, 0, 0, _RET_IP_);
 	LOCK_CONTENDED_FLAGS(lock, do_raw_read_trylock, do_raw_read_lock,
 			     do_raw_read_lock_flags, &flags);
 	return flags;
@@ -166,7 +166,7 @@ static inline void __raw_read_lock_irq(rwlock_t *lock)
 {
 	local_irq_disable();
 	preempt_disable();
-	rwlock_acquire_read(&lock->dep_map, 0, 0, _RET_IP_);
+	rwlock_acquire_read(&lock->monitor, 0, 0, _RET_IP_);
 	LOCK_CONTENDED(lock, do_raw_read_trylock, do_raw_read_lock);
 }
 
@@ -174,7 +174,7 @@ static inline void __raw_read_lock_bh(rwlock_t *lock)
 {
 	local_bh_disable();
 	preempt_disable();
-	rwlock_acquire_read(&lock->dep_map, 0, 0, _RET_IP_);
+	rwlock_acquire_read(&lock->monitor, 0, 0, _RET_IP_);
 	LOCK_CONTENDED(lock, do_raw_read_trylock, do_raw_read_lock);
 }
 
@@ -184,7 +184,7 @@ static inline unsigned long __raw_write_lock_irqsave(rwlock_t *lock)
 
 	local_irq_save(flags);
 	preempt_disable();
-	rwlock_acquire(&lock->dep_map, 0, 0, _RET_IP_);
+	rwlock_acquire(&lock->monitor, 0, 0, _RET_IP_);
 	LOCK_CONTENDED_FLAGS(lock, do_raw_write_trylock, do_raw_write_lock,
 			     do_raw_write_lock_flags, &flags);
 	return flags;
@@ -194,7 +194,7 @@ static inline void __raw_write_lock_irq(rwlock_t *lock)
 {
 	local_irq_disable();
 	preempt_disable();
-	rwlock_acquire(&lock->dep_map, 0, 0, _RET_IP_);
+	rwlock_acquire(&lock->monitor, 0, 0, _RET_IP_);
 	LOCK_CONTENDED(lock, do_raw_write_trylock, do_raw_write_lock);
 }
 
@@ -202,14 +202,14 @@ static inline void __raw_write_lock_bh(rwlock_t *lock)
 {
 	local_bh_disable();
 	preempt_disable();
-	rwlock_acquire(&lock->dep_map, 0, 0, _RET_IP_);
+	rwlock_acquire(&lock->monitor, 0, 0, _RET_IP_);
 	LOCK_CONTENDED(lock, do_raw_write_trylock, do_raw_write_lock);
 }
 
 static inline void __raw_write_lock(rwlock_t *lock)
 {
 	preempt_disable();
-	rwlock_acquire(&lock->dep_map, 0, 0, _RET_IP_);
+	rwlock_acquire(&lock->monitor, 0, 0, _RET_IP_);
 	LOCK_CONTENDED(lock, do_raw_write_trylock, do_raw_write_lock);
 }
 
@@ -217,14 +217,14 @@ static inline void __raw_write_lock(rwlock_t *lock)
 
 static inline void __raw_write_unlock(rwlock_t *lock)
 {
-	rwlock_release(&lock->dep_map, 1, _RET_IP_);
+	rwlock_release(&lock->monitor, 1, _RET_IP_);
 	do_raw_write_unlock(lock);
 	preempt_enable();
 }
 
 static inline void __raw_read_unlock(rwlock_t *lock)
 {
-	rwlock_release(&lock->dep_map, 1, _RET_IP_);
+	rwlock_release(&lock->monitor, 1, _RET_IP_);
 	do_raw_read_unlock(lock);
 	preempt_enable();
 }
@@ -232,7 +232,7 @@ static inline void __raw_read_unlock(rwlock_t *lock)
 static inline void
 __raw_read_unlock_irqrestore(rwlock_t *lock, unsigned long flags)
 {
-	rwlock_release(&lock->dep_map, 1, _RET_IP_);
+	rwlock_release(&lock->monitor, 1, _RET_IP_);
 	do_raw_read_unlock(lock);
 	local_irq_restore(flags);
 	preempt_enable();
@@ -240,7 +240,7 @@ __raw_read_unlock_irqrestore(rwlock_t *lock, unsigned long flags)
 
 static inline void __raw_read_unlock_irq(rwlock_t *lock)
 {
-	rwlock_release(&lock->dep_map, 1, _RET_IP_);
+	rwlock_release(&lock->monitor, 1, _RET_IP_);
 	do_raw_read_unlock(lock);
 	local_irq_enable();
 	preempt_enable();
@@ -248,7 +248,7 @@ static inline void __raw_read_unlock_irq(rwlock_t *lock)
 
 static inline void __raw_read_unlock_bh(rwlock_t *lock)
 {
-	rwlock_release(&lock->dep_map, 1, _RET_IP_);
+	rwlock_release(&lock->monitor, 1, _RET_IP_);
 	do_raw_read_unlock(lock);
 	preempt_enable_no_resched();
 	local_bh_enable_ip((unsigned long)__builtin_return_address(0));
@@ -257,7 +257,7 @@ static inline void __raw_read_unlock_bh(rwlock_t *lock)
 static inline void __raw_write_unlock_irqrestore(rwlock_t *lock,
 					     unsigned long flags)
 {
-	rwlock_release(&lock->dep_map, 1, _RET_IP_);
+	rwlock_release(&lock->monitor, 1, _RET_IP_);
 	do_raw_write_unlock(lock);
 	local_irq_restore(flags);
 	preempt_enable();
@@ -265,7 +265,7 @@ static inline void __raw_write_unlock_irqrestore(rwlock_t *lock,
 
 static inline void __raw_write_unlock_irq(rwlock_t *lock)
 {
-	rwlock_release(&lock->dep_map, 1, _RET_IP_);
+	rwlock_release(&lock->monitor, 1, _RET_IP_);
 	do_raw_write_unlock(lock);
 	local_irq_enable();
 	preempt_enable();
@@ -273,7 +273,7 @@ static inline void __raw_write_unlock_irq(rwlock_t *lock)
 
 static inline void __raw_write_unlock_bh(rwlock_t *lock)
 {
-	rwlock_release(&lock->dep_map, 1, _RET_IP_);
+	rwlock_release(&lock->monitor, 1, _RET_IP_);
 	do_raw_write_unlock(lock);
 	preempt_enable_no_resched();
 	local_bh_enable_ip((unsigned long)__builtin_return_address(0));
diff --git a/include/linux/rwlock_types.h b/include/linux/rwlock_types.h
index bd31808..b4dd24a 100644
--- a/include/linux/rwlock_types.h
+++ b/include/linux/rwlock_types.h
@@ -17,17 +17,17 @@ typedef struct {
 	unsigned int magic, owner_cpu;
 	void *owner;
 #endif
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-	struct lockdep_map dep_map;
+#ifdef CONFIG_LOCK_MONITOR
+	struct lock_monitor monitor;
 #endif
 } rwlock_t;
 
 #define RWLOCK_MAGIC		0xdeaf1eed
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
-# define RW_DEP_MAP_INIT(lockname)	.dep_map = { .name = #lockname }
+# define RW_LOCK_MONITOR_INIT(lockname)	.monitor = { __LOCK_MONITOR_INIT(lockname) }
 #else
-# define RW_DEP_MAP_INIT(lockname)
+# define RW_LOCK_MONITOR_INIT(lockname)
 #endif
 
 #ifdef CONFIG_DEBUG_SPINLOCK
@@ -36,11 +36,11 @@ typedef struct {
 				.magic = RWLOCK_MAGIC,			\
 				.owner = SPINLOCK_OWNER_INIT,		\
 				.owner_cpu = -1,			\
-				RW_DEP_MAP_INIT(lockname) }
+				RW_LOCK_MONITOR_INIT(lockname) }
 #else
 #define __RW_LOCK_UNLOCKED(lockname) \
 	(rwlock_t)	{	.raw_lock = __ARCH_RW_LOCK_UNLOCKED,	\
-				RW_DEP_MAP_INIT(lockname) }
+				RW_LOCK_MONITOR_INIT(lockname) }
 #endif
 
 /*
-- 
1.6.5.2

[PATCH RFC 05/11] Adopt arch dependent rwsem to lock monitor

[Hitoshi Mitake]

Current struct rw_semaphore holds struct lockdep_map,
but it is a special thing of lockdep subsystem.

So I replaced it with struct lock_monitor.
Now it contains lockdep_map, and adding new members is easy.

Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Jason Baron <jbaron@redhat.com>
---
 include/linux/rwsem-spinlock.h |   12 ++++++------
 include/linux/rwsem.h          |    2 ++
 kernel/rwsem.c                 |   16 ++++++++--------
 lib/rwsem-spinlock.c           |    2 +-
 lib/rwsem.c                    |    2 +-
 5 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/include/linux/rwsem-spinlock.h b/include/linux/rwsem-spinlock.h
index bdfcc25..e99c42b 100644
--- a/include/linux/rwsem-spinlock.h
+++ b/include/linux/rwsem-spinlock.h
@@ -32,20 +32,20 @@ struct rw_semaphore {
 	__s32			activity;
 	spinlock_t		wait_lock;
 	struct list_head	wait_list;
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-	struct lockdep_map dep_map;
+#ifdef CONFIG_LOCK_MONITOR
+	struct lock_monitor     monitor;
 #endif
 };
 
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-# define __RWSEM_DEP_MAP_INIT(lockname) , .dep_map = { .name = #lockname }
+#ifdef CONFIG_LOCK_MONITOR
+# define __RWSEM_LOCK_MONITOR_INIT(lockname) , .monitor = { __LOCK_MONITOR_INIT(lockname) }
 #else
-# define __RWSEM_DEP_MAP_INIT(lockname)
+# define __RWSEM_LOCK_MONITOR_INIT(lockname)
 #endif
 
 #define __RWSEM_INITIALIZER(name) \
 { 0, __SPIN_LOCK_UNLOCKED(name.wait_lock), LIST_HEAD_INIT((name).wait_list) \
-  __RWSEM_DEP_MAP_INIT(name) }
+  __RWSEM_LOCK_MONITOR_INIT(name) }
 
 #define DECLARE_RWSEM(name) \
 	struct rw_semaphore name = __RWSEM_INITIALIZER(name)
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index efd348f..5440a95 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -16,6 +16,8 @@
 
 struct rw_semaphore;
 
+#include <linux/lock_monitor.h>
+
 #ifdef CONFIG_RWSEM_GENERIC_SPINLOCK
 #include <linux/rwsem-spinlock.h> /* use a generic implementation */
 #else
diff --git a/kernel/rwsem.c b/kernel/rwsem.c
index cae050b..c2fd9e6 100644
--- a/kernel/rwsem.c
+++ b/kernel/rwsem.c
@@ -19,7 +19,7 @@
 void __sched down_read(struct rw_semaphore *sem)
 {
 	might_sleep();
-	rwsem_acquire_read(&sem->dep_map, 0, 0, _RET_IP_);
+	rwsem_acquire_read(&sem->monitor, 0, 0, _RET_IP_);
 
 	LOCK_CONTENDED(sem, __down_read_trylock, __down_read);
 }
@@ -34,7 +34,7 @@ int down_read_trylock(struct rw_semaphore *sem)
 	int ret = __down_read_trylock(sem);
 
 	if (ret == 1)
-		rwsem_acquire_read(&sem->dep_map, 0, 1, _RET_IP_);
+		rwsem_acquire_read(&sem->monitor, 0, 1, _RET_IP_);
 	return ret;
 }
 
@@ -46,7 +46,7 @@ EXPORT_SYMBOL(down_read_trylock);
 void __sched down_write(struct rw_semaphore *sem)
 {
 	might_sleep();
-	rwsem_acquire(&sem->dep_map, 0, 0, _RET_IP_);
+	rwsem_acquire(&sem->monitor, 0, 0, _RET_IP_);
 
 	LOCK_CONTENDED(sem, __down_write_trylock, __down_write);
 }
@@ -61,7 +61,7 @@ int down_write_trylock(struct rw_semaphore *sem)
 	int ret = __down_write_trylock(sem);
 
 	if (ret == 1)
-		rwsem_acquire(&sem->dep_map, 0, 1, _RET_IP_);
+		rwsem_acquire(&sem->monitor, 0, 1, _RET_IP_);
 	return ret;
 }
 
@@ -72,7 +72,7 @@ EXPORT_SYMBOL(down_write_trylock);
  */
 void up_read(struct rw_semaphore *sem)
 {
-	rwsem_release(&sem->dep_map, 1, _RET_IP_);
+	rwsem_release(&sem->monitor, 1, _RET_IP_);
 
 	__up_read(sem);
 }
@@ -84,7 +84,7 @@ EXPORT_SYMBOL(up_read);
  */
 void up_write(struct rw_semaphore *sem)
 {
-	rwsem_release(&sem->dep_map, 1, _RET_IP_);
+	rwsem_release(&sem->monitor, 1, _RET_IP_);
 
 	__up_write(sem);
 }
@@ -110,7 +110,7 @@ EXPORT_SYMBOL(downgrade_write);
 void down_read_nested(struct rw_semaphore *sem, int subclass)
 {
 	might_sleep();
-	rwsem_acquire_read(&sem->dep_map, subclass, 0, _RET_IP_);
+	rwsem_acquire_read(&sem->monitor, subclass, 0, _RET_IP_);
 
 	LOCK_CONTENDED(sem, __down_read_trylock, __down_read);
 }
@@ -129,7 +129,7 @@ EXPORT_SYMBOL(down_read_non_owner);
 void down_write_nested(struct rw_semaphore *sem, int subclass)
 {
 	might_sleep();
-	rwsem_acquire(&sem->dep_map, subclass, 0, _RET_IP_);
+	rwsem_acquire(&sem->monitor, subclass, 0, _RET_IP_);
 
 	LOCK_CONTENDED(sem, __down_write_trylock, __down_write);
 }
diff --git a/lib/rwsem-spinlock.c b/lib/rwsem-spinlock.c
index ccf95bf..3bd0ed0 100644
--- a/lib/rwsem-spinlock.c
+++ b/lib/rwsem-spinlock.c
@@ -41,7 +41,7 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name,
 	 * Make sure we are not reinitializing a held semaphore:
 	 */
 	debug_check_no_locks_freed((void *)sem, sizeof(*sem));
-	lockdep_init_map(&sem->dep_map, name, key, 0);
+	lockdep_init_map(&sem->monitor.dep_map, name, key, 0);
 #endif
 	sem->activity = 0;
 	spin_lock_init(&sem->wait_lock);
diff --git a/lib/rwsem.c b/lib/rwsem.c
index 3e3365e..fab3db0 100644
--- a/lib/rwsem.c
+++ b/lib/rwsem.c
@@ -19,7 +19,7 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name,
 	 * Make sure we are not reinitializing a held semaphore:
 	 */
 	debug_check_no_locks_freed((void *)sem, sizeof(*sem));
-	lockdep_init_map(&sem->dep_map, name, key, 0);
+	lockdep_init_map(&sem->monitor.dep_map, name, key, 0);
 #endif
 	sem->count = RWSEM_UNLOCKED_VALUE;
 	spin_lock_init(&sem->wait_lock);
-- 
1.6.5.2

[PATCH RFC 06/11] Adopt rwsem of x86 to lock monitor

[Hitoshi Mitake]

Current struct rw_semaphore holds struct lockdep_map,
but it is a special thing of lockdep subsystem.

So I replaced it with struct lock_monitor.
Now it contains lockdep_map, and adding new members is easy.

Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Jason Baron <jbaron@redhat.com>
---
 arch/x86/include/asm/rwsem.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/rwsem.h b/arch/x86/include/asm/rwsem.h
index ca7517d..a6862f1 100644
--- a/arch/x86/include/asm/rwsem.h
+++ b/arch/x86/include/asm/rwsem.h
@@ -68,22 +68,22 @@ struct rw_semaphore {
 	signed long		count;
 	spinlock_t		wait_lock;
 	struct list_head	wait_list;
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-	struct lockdep_map dep_map;
+#ifdef CONFIG_LOCK_MONITOR
+	struct lock_monitor     monitor;
 #endif
 };
 
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-# define __RWSEM_DEP_MAP_INIT(lockname) , .dep_map = { .name = #lockname }
+#ifdef CONFIG_LOCK_MONITOR
+# define __RWSEM_LOCK_MONITOR_INIT(lockname) , .monitor = { __LOCK_MONITOR_INIT(lockname) }
 #else
-# define __RWSEM_DEP_MAP_INIT(lockname)
+# define __RWSEM_LOCK_MONITOR_INIT(lockname)
 #endif
 
 
 #define __RWSEM_INITIALIZER(name)				\
 {								\
 	RWSEM_UNLOCKED_VALUE, __SPIN_LOCK_UNLOCKED((name).wait_lock), \
-	LIST_HEAD_INIT((name).wait_list) __RWSEM_DEP_MAP_INIT(name) \
+	LIST_HEAD_INIT((name).wait_list) __RWSEM_LOCK_MONITOR_INIT(name) \
 }
 
 #define DECLARE_RWSEM(name)					\
-- 
1.6.5.2

[PATCH RFC 07/11] Adopt the way of initializing semaphore to lock monitor

[Hitoshi Mitake]

Now spinlock_t doesn't have struct lockdep_map,
this was replaced with struct lock_monitor.

So this patch fixes the way of initializing semaphore.

Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Jason Baron <jbaron@redhat.com>
---
 include/linux/semaphore.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/semaphore.h b/include/linux/semaphore.h
index 7415839..d5aa2ba 100644
--- a/include/linux/semaphore.h
+++ b/include/linux/semaphore.h
@@ -33,7 +33,7 @@ static inline void sema_init(struct semaphore *sem, int val)
 {
 	static struct lock_class_key __key;
 	*sem = (struct semaphore) __SEMAPHORE_INITIALIZER(*sem, val);
-	lockdep_init_map(&sem->lock.dep_map, "semaphore->lock", &__key, 0);
+	lockdep_init_map(&sem->lock.monitor.dep_map, "semaphore->lock", &__key, 0);
 }
 
 #define init_MUTEX(sem)		sema_init(sem, 1)
-- 
1.6.5.2

[PATCH RFC 08/11] Adopt mutex to lock monitor

[Hitoshi Mitake]

Current struct mutex holds struct lockdep_map,
but it is a special thing of lockdep subsystem.

So I replaced it with struct lock_monitor.
Now it contains lockdep_map, and adding new members is easy.

Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Jason Baron <jbaron@redhat.com>
---
 include/linux/mutex.h |   14 +++++++-------
 kernel/mutex-debug.c  |    2 +-
 kernel/mutex.c        |   14 +++++++-------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 878cab4..0673307 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -13,7 +13,7 @@
 #include <linux/list.h>
 #include <linux/spinlock_types.h>
 #include <linux/linkage.h>
-#include <linux/lockdep.h>
+#include <linux/lock_monitor.h>
 
 #include <asm/atomic.h>
 
@@ -57,8 +57,8 @@ struct mutex {
 	const char 		*name;
 	void			*magic;
 #endif
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-	struct lockdep_map	dep_map;
+#ifdef CONFIG_LOCK_MONITOR
+	struct lock_monitor     monitor;
 #endif
 };
 
@@ -88,10 +88,10 @@ do {							\
 #endif
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
-# define __DEP_MAP_MUTEX_INITIALIZER(lockname) \
-		, .dep_map = { .name = #lockname }
+# define __LOCK_MONITOR_MUTEX_INITIALIZER(lockname) \
+	        , .monitor = { __LOCK_MONITOR_INIT(lockname) }
 #else
-# define __DEP_MAP_MUTEX_INITIALIZER(lockname)
+# define __LOCK_MONITOR_MUTEX_INITIALIZER(lockname)
 #endif
 
 #define __MUTEX_INITIALIZER(lockname) \
@@ -99,7 +99,7 @@ do {							\
 		, .wait_lock = __SPIN_LOCK_UNLOCKED(lockname.wait_lock) \
 		, .wait_list = LIST_HEAD_INIT(lockname.wait_list) \
 		__DEBUG_MUTEX_INITIALIZER(lockname) \
-		__DEP_MAP_MUTEX_INITIALIZER(lockname) }
+		__LOCK_MONITOR_MUTEX_INITIALIZER(lockname) }
 
 #define DEFINE_MUTEX(mutexname) \
 	struct mutex mutexname = __MUTEX_INITIALIZER(mutexname)
diff --git a/kernel/mutex-debug.c b/kernel/mutex-debug.c
index ec815a9..9266f60 100644
--- a/kernel/mutex-debug.c
+++ b/kernel/mutex-debug.c
@@ -88,7 +88,7 @@ void debug_mutex_init(struct mutex *lock, const char *name,
 	 * Make sure we are not reinitializing a held lock:
 	 */
 	debug_check_no_locks_freed((void *)lock, sizeof(*lock));
-	lockdep_init_map(&lock->dep_map, name, key, 0);
+	lockdep_init_map(&lock->monitor.dep_map, name, key, 0);
 #endif
 	lock->magic = lock;
 }
diff --git a/kernel/mutex.c b/kernel/mutex.c
index 632f04c..f9e91ce 100644
--- a/kernel/mutex.c
+++ b/kernel/mutex.c
@@ -147,7 +147,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 	unsigned long flags;
 
 	preempt_disable();
-	mutex_acquire(&lock->dep_map, subclass, 0, ip);
+	mutex_acquire(&lock->monitor, subclass, 0, ip);
 
 #ifdef CONFIG_MUTEX_SPIN_ON_OWNER
 	/*
@@ -180,7 +180,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 			break;
 
 		if (atomic_cmpxchg(&lock->count, 1, 0) == 1) {
-			lock_acquired(&lock->dep_map, ip);
+			lockdep_acquired(&lock->dep_map, ip);
 			mutex_set_owner(lock);
 			preempt_enable();
 			return 0;
@@ -216,7 +216,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 	if (atomic_xchg(&lock->count, -1) == 1)
 		goto done;
 
-	lock_contended(&lock->dep_map, ip);
+	lockdep_contended(&lock->monitor.dep_map, ip);
 
 	for (;;) {
 		/*
@@ -238,7 +238,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 		if (unlikely(signal_pending_state(state, task))) {
 			mutex_remove_waiter(lock, &waiter,
 					    task_thread_info(task));
-			mutex_release(&lock->dep_map, 1, ip);
+			mutex_release(&lock->monitor, 1, ip);
 			spin_unlock_mutex(&lock->wait_lock, flags);
 
 			debug_mutex_free_waiter(&waiter);
@@ -256,7 +256,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 	}
 
 done:
-	lock_acquired(&lock->dep_map, ip);
+	lockdep_acquired(&lock->monitor.dep_map, ip);
 	/* got the lock - rejoice! */
 	mutex_remove_waiter(lock, &waiter, current_thread_info());
 	mutex_set_owner(lock);
@@ -312,7 +312,7 @@ __mutex_unlock_common_slowpath(atomic_t *lock_count, int nested)
 	unsigned long flags;
 
 	spin_lock_mutex(&lock->wait_lock, flags);
-	mutex_release(&lock->dep_map, nested, _RET_IP_);
+	mutex_release(&lock->monitor, nested, _RET_IP_);
 	debug_mutex_unlock(lock);
 
 	/*
@@ -437,7 +437,7 @@ static inline int __mutex_trylock_slowpath(atomic_t *lock_count)
 	prev = atomic_xchg(&lock->count, -1);
 	if (likely(prev == 1)) {
 		mutex_set_owner(lock);
-		mutex_acquire(&lock->dep_map, 0, 1, _RET_IP_);
+		mutex_acquire(&lock->monitor, 0, 1, _RET_IP_);
 	}
 
 	/* Set it back to 0 if there are no waiters: */
-- 
1.6.5.2

[PATCH RFC 09/11] Adopt rcu_read_lock() to lock monitor

[Hitoshi Mitake]

rcu_read_lock() issues lock_acquire() and rcu_read_unlock() issues lock_release().
They don't lock actual locks like spinlock or mutex.

So I replaced these calling of lock_acquire() and lock_release() with
lockdep_acquire() and lockdep_release().

Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Jason Baron <jbaron@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 include/linux/rcupdate.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 24440f4..e45c223 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -80,8 +80,8 @@ extern void rcu_init(void);
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 extern struct lockdep_map rcu_lock_map;
 # define rcu_read_acquire()	\
-			lock_acquire(&rcu_lock_map, 0, 0, 2, 1, NULL, _THIS_IP_)
-# define rcu_read_release()	lock_release(&rcu_lock_map, 1, _THIS_IP_)
+			lockdep_acquire(&rcu_lock_map, 0, 0, 2, 1, NULL, _THIS_IP_)
+# define rcu_read_release()	lockdep_release(&rcu_lock_map, 1, _THIS_IP_)
 #else
 # define rcu_read_acquire()	do { } while (0)
 # define rcu_read_release()	do { } while (0)
-- 
1.6.5.2

[PATCH RFC 10/11] Adopt kernel/sched.c to lock monitor

[Hitoshi Mitake]

There are some points of directly calling the macros spin_{acquire, release}()
and the function lock_set_subclass() in kernel/sched.c.

Now spinlock_t doesn't have struct lockdep_map,
so this patch fixes them with the rule of lock monitor.

Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Jason Baron <jbaron@redhat.com>
---
 kernel/sched.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index 7266b91..6c8a52e 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -891,7 +891,7 @@ static inline void finish_lock_switch(struct rq *rq, struct task_struct *prev)
 	 * fix up the runqueue lock - which gets 'carried over' from
 	 * prev into current:
 	 */
-	spin_acquire(&rq->lock.dep_map, 0, 0, _THIS_IP_);
+	spin_acquire(&rq->lock.monitor, 0, 0, _THIS_IP_);
 
 	raw_spin_unlock_irq(&rq->lock);
 }
@@ -1803,7 +1803,7 @@ static inline void double_unlock_balance(struct rq *this_rq, struct rq *busiest)
 	__releases(busiest->lock)
 {
 	raw_spin_unlock(&busiest->lock);
-	lock_set_subclass(&this_rq->lock.dep_map, 0, _RET_IP_);
+	lock_set_subclass(&this_rq->lock.monitor.dep_map, 0, _RET_IP_);
 }
 #endif
 
@@ -2905,7 +2905,7 @@ context_switch(struct rq *rq, struct task_struct *prev,
 	 * do an early lockdep release here:
 	 */
 #ifndef __ARCH_WANT_UNLOCKED_CTXSW
-	spin_release(&rq->lock.dep_map, 1, _THIS_IP_);
+	spin_release(&rq->lock.monitor, 1, _THIS_IP_);
 #endif
 
 	/* Here we just switch the register state and the stack. */
@@ -6725,7 +6725,7 @@ SYSCALL_DEFINE0(sched_yield)
 	 * no need to preempt or enable interrupts:
 	 */
 	__release(rq->lock);
-	spin_release(&rq->lock.dep_map, 1, _THIS_IP_);
+	spin_release(&rq->lock.monitor, 1, _THIS_IP_);
 	do_raw_spin_unlock(&rq->lock);
 	preempt_enable_no_resched();
 
-- 
1.6.5.2

[PATCH RFC 11/11] Very dirty temporal solution for testing lock monitor

[Hitoshi Mitake]

This patch does some very dirty temporal fix for testing lock monitor.
fs/inode.c, fs/sysfs/dir.c and net/core/sock.c do some special lockdep using,
conflict with lock monitor.

This patch is only for testing!

Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Jason Baron <jbaron@redhat.com>
---
 fs/inode.c      |    1 -
 fs/sysfs/dir.c  |   24 ++++++++++++++++++++----
 net/core/sock.c |   15 +++++++++++++++
 3 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 03dfeb2..f6180eb 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -158,7 +158,6 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 		goto out;
 	spin_lock_init(&inode->i_lock);
 	lockdep_set_class(&inode->i_lock, &sb->s_type->i_lock_key);
-
 	mutex_init(&inode->i_mutex);
 	lockdep_set_class(&inode->i_mutex, &sb->s_type->i_mutex_key);
 
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 699f371..ba5f492 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -24,6 +24,22 @@
 #include <linux/security.h>
 #include "sysfs.h"
 
+/* FIXME: temporal solution */
+#undef rwsem_acquire
+#undef rwsem_acquire_read
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+# ifdef CONFIG_PROVE_LOCKING
+#  define rwsem_acquire(l, s, t, i)		lockdep_acquire(l, s, t, 0, 2, NULL, i)
+#  define rwsem_acquire_read(l, s, t, i)	lockdep_acquire(l, s, t, 1, 2, NULL, i)
+# else
+#  define rwsem_acquire(l, s, t, i)		lockdep_acquire(l, s, t, 0, 1, NULL, i)
+#  define rwsem_acquire_read(l, s, t, i)	lockdep_acquire(l, s, t, 1, 1, NULL, i)
+# endif
+#else
+# define rwsem_acquire(l, s, t, i)		do { } while (0)
+# define rwsem_acquire_read(l, s, t, i)		do { } while (0)
+#endif
+
 DEFINE_MUTEX(sysfs_mutex);
 DEFINE_SPINLOCK(sysfs_assoc_lock);
 
@@ -132,7 +148,7 @@ static void sysfs_put_active(struct sysfs_dirent *sd)
 	if (unlikely(!sd))
 		return;
 
-	rwsem_release(&sd->dep_map, 1, _RET_IP_);
+	lockdep_release(&sd->dep_map, 1, _RET_IP_);
 	v = atomic_dec_return(&sd->s_active);
 	if (likely(v != SD_DEACTIVATED_BIAS))
 		return;
@@ -204,14 +220,14 @@ static void sysfs_deactivate(struct sysfs_dirent *sd)
 	v = atomic_add_return(SD_DEACTIVATED_BIAS, &sd->s_active);
 
 	if (v != SD_DEACTIVATED_BIAS) {
-		lock_contended(&sd->dep_map, _RET_IP_);
+		lockdep_contended(&sd->dep_map, _RET_IP_);
 		wait_for_completion(&wait);
 	}
 
 	sd->s_sibling = NULL;
 
-	lock_acquired(&sd->dep_map, _RET_IP_);
-	rwsem_release(&sd->dep_map, 1, _RET_IP_);
+	lockdep_acquired(&sd->dep_map, _RET_IP_);
+	lockdep_release(&sd->dep_map, 1, _RET_IP_);
 }
 
 static int sysfs_alloc_ino(ino_t *pino)
diff --git a/net/core/sock.c b/net/core/sock.c
index e1f6f22..332bc1d 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -130,6 +130,21 @@
 #include <net/tcp.h>
 #endif
 
+/* FIXME: temporal solution */
+#undef mutex_acquire
+#undef mutex_release
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+# ifdef CONFIG_PROVE_LOCKING
+#  define mutex_acquire(l, s, t, i)		lockdep_acquire(l, s, t, 0, 2, NULL, i)
+# else
+#  define mutex_acquire(l, s, t, i)		lockdep_acquire(l, s, t, 0, 1, NULL, i)
+# endif
+# define mutex_release(l, n, i)			lockdep_release(l, n, i)
+#else
+# define mutex_acquire(l, s, t, i)		do { } while (0)
+# define mutex_release(l, n, i)			do { } while (0)
+#endif
+
 /*
  * Each address family might have different locking rules, so we have
  * one slock key per address family:
-- 
1.6.5.2

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Peter Zijlstra]

On Sun, 2010-03-14 at 19:38 +0900, Hitoshi Mitake wrote:
> Current lockdep is too complicated because,
>  * dependency validation
>  * statistics
>  * event tracing
> are all implemented by it.
> This cause problem of overhead.
> If user enables one of them, overhead of rests part is not avoidable.
> (tracing is exception. If user enables validation or stat,
> overhead of tracing doesn't occur.)
> 
> So I suggest new subsystem "lock monitor".
> This is a general purpose lock event hooking mechanism.
> 
> lock monitor will be enable easy implementing and running
> these features related to lock.
> 
> And I'm hoping that lock monitor will reduce overhead of perf lock.
> Because lock monitor separates dependency validation and event tracing clearly,
> so calling of functions of lockdep (e.g. lock_acquire()) only for validation
> will not occur lock events.
> 
> I implemented it on the branch perf/inject of Frederic's random-tracing tree.
> Because the branch is hottest place of lock and tracing :)

OK, so I really don't like this much..

Building a lockstat kernel (PROVE_LOCKING=n) should not have much more
overhead than the proposed solution, if the simple lock acquistion
tracking bothers you, you can do a patch to weaken that.

I really really dislike how you add a monitor variable between
everything for no reason what so ever.

You use a new rwlock_t, which is an instant fail, those things are worse
than useless.

You add chained indirect calls into all lock ops, that's got to hurt.

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Frederic Weisbecker]

On Sun, Mar 14, 2010 at 07:13:55PM +0100, Peter Zijlstra wrote:
> On Sun, 2010-03-14 at 19:38 +0900, Hitoshi Mitake wrote:
> > Current lockdep is too complicated because,
> >  * dependency validation
> >  * statistics
> >  * event tracing
> > are all implemented by it.
> > This cause problem of overhead.
> > If user enables one of them, overhead of rests part is not avoidable.
> > (tracing is exception. If user enables validation or stat,
> > overhead of tracing doesn't occur.)
> > 
> > So I suggest new subsystem "lock monitor".
> > This is a general purpose lock event hooking mechanism.
> > 
> > lock monitor will be enable easy implementing and running
> > these features related to lock.
> > 
> > And I'm hoping that lock monitor will reduce overhead of perf lock.
> > Because lock monitor separates dependency validation and event tracing clearly,
> > so calling of functions of lockdep (e.g. lock_acquire()) only for validation
> > will not occur lock events.
> > 
> > I implemented it on the branch perf/inject of Frederic's random-tracing tree.
> > Because the branch is hottest place of lock and tracing :)
> 
> OK, so I really don't like this much..
> 
> Building a lockstat kernel (PROVE_LOCKING=n) should not have much more
> overhead than the proposed solution, if the simple lock acquistion
> tracking bothers you, you can do a patch to weaken that.
> 
> I really really dislike how you add a monitor variable between
> everything for no reason what so ever.
> 
> You use a new rwlock_t, which is an instant fail, those things are worse
> than useless.
> 
> You add chained indirect calls into all lock ops, that's got to hurt.


Well, the idea was not bad at the first glance. It was separating
lockdep and lock events codes.

But indeed, the indirect calls plus the locking are not good
for such a fast path.

There is something else, it would be nice to keep the
lockdep_map -> lockdep_class mapping so that we can
do lock profiling based on classes too. So we actually
need the lockdep code. What we don't need is the prove
locking or the lock stats. So I guess we can have a new
config to enable lock events and get rid of the prove
locking / lock stat code if we don't need it.

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Frederic Weisbecker]

On Sun, Mar 14, 2010 at 07:38:37PM +0900, Hitoshi Mitake wrote:
> I implemented it on the branch perf/inject of Frederic's random-tracing tree.
> Because the branch is hottest place of lock and tracing :)


Ouch! You shouldn't do this. The patches inside were
trials submitted for review and the resulting discussion
concluded that the injection must be redesigned.

More generally I don't recommend you to base your patches
on my tree. I use it as a buffer when I send patches
for review or for pull requests.

The branches inside can be randomly rebased (unless a
branch is waiting to be pulled) and they are pretty async
with the -tip tree.

The hottest and most sync tree on which you should base your patches
for perf is:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git
	perf/core

With that you have best chances to work on a sane and up-to-date
base.

Thanks.

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Hitoshi Mitake]

On 03/17/10 10:32, Frederic Weisbecker wrote:
 > On Sun, Mar 14, 2010 at 07:13:55PM +0100, Peter Zijlstra wrote:
 >> On Sun, 2010-03-14 at 19:38 +0900, Hitoshi Mitake wrote:
 >>> Current lockdep is too complicated because,
 >>>   * dependency validation
 >>>   * statistics
 >>>   * event tracing
 >>> are all implemented by it.
 >>> This cause problem of overhead.
 >>> If user enables one of them, overhead of rests part is not avoidable.
 >>> (tracing is exception. If user enables validation or stat,
 >>> overhead of tracing doesn't occur.)
 >>>
 >>> So I suggest new subsystem "lock monitor".
 >>> This is a general purpose lock event hooking mechanism.
 >>>
 >>> lock monitor will be enable easy implementing and running
 >>> these features related to lock.
 >>>
 >>> And I'm hoping that lock monitor will reduce overhead of perf lock.
 >>> Because lock monitor separates dependency validation and event 
tracing clearly,
 >>> so calling of functions of lockdep (e.g. lock_acquire()) only for 
validation
 >>> will not occur lock events.
 >>>
 >>> I implemented it on the branch perf/inject of Frederic's 
random-tracing tree.
 >>> Because the branch is hottest place of lock and tracing :)
 >>
 >> OK, so I really don't like this much..
 >>
 >> Building a lockstat kernel (PROVE_LOCKING=n) should not have much more
 >> overhead than the proposed solution, if the simple lock acquistion
 >> tracking bothers you, you can do a patch to weaken that.
 >>
 >> I really really dislike how you add a monitor variable between
 >> everything for no reason what so ever.
 >>
 >> You use a new rwlock_t, which is an instant fail, those things are worse
 >> than useless.
 >>
 >> You add chained indirect calls into all lock ops, that's got to hurt.
 >
 >
 > Well, the idea was not bad at the first glance. It was separating
 > lockdep and lock events codes.
 >
 > But indeed, the indirect calls plus the locking are not good
 > for such a fast path.
 >
 > There is something else, it would be nice to keep the
 > lockdep_map ->  lockdep_class mapping so that we can
 > do lock profiling based on classes too. So we actually
 > need the lockdep code. What we don't need is the prove
 > locking or the lock stats. So I guess we can have a new
 > config to enable lock events and get rid of the prove
 > locking / lock stat code if we don't need it.
 >
 >

Thanks for your comments, Peter and Frederic.

My main motivation of writing this patch series was that
some kernel codes uses lockdep functions (e.g. lock_acquire()) directly,
so perf lock gets a lot of trace events without actual locks (e.g. 
might_lock_read()).
I think that these are confusable things for users.

But I noticed that these events can be reduced by
turning off CONFIG_PROVE_LOCKING. Yeah, my patch series was pointless... :)

Should perf lock warn not to use with CONFIG_PROVE_LOCKING?

Thanks,
	Hitoshi

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Hitoshi Mitake]

On 03/17/10 10:47, Frederic Weisbecker wrote:
 > On Sun, Mar 14, 2010 at 07:38:37PM +0900, Hitoshi Mitake wrote:
 >> I implemented it on the branch perf/inject of Frederic's 
random-tracing tree.
 >> Because the branch is hottest place of lock and tracing :)
 >
 >
 > Ouch! You shouldn't do this. The patches inside were
 > trials submitted for review and the resulting discussion
 > concluded that the injection must be redesigned.

Oh, sorry...
And I'd like to look at redesigning way of inject.

 >
 > More generally I don't recommend you to base your patches
 > on my tree. I use it as a buffer when I send patches
 > for review or for pull requests.
 >
 > The branches inside can be randomly rebased (unless a
 > branch is waiting to be pulled) and they are pretty async
 > with the -tip tree.
 >
 > The hottest and most sync tree on which you should base your patches
 > for perf is:
 >
 > git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git
 > 	perf/core
 >
 > With that you have best chances to work on a sane and up-to-date
 > base.
 >
 > Thanks.
 >
 >

I'll work on tip/perf/core from next time :)

Thanks,
	Hitoshi

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Ingo Molnar]

* Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp> wrote:

> On 03/17/10 10:47, Frederic Weisbecker wrote:
> > On Sun, Mar 14, 2010 at 07:38:37PM +0900, Hitoshi Mitake wrote:
> >> I implemented it on the branch perf/inject of Frederic's
> random-tracing tree.
> >> Because the branch is hottest place of lock and tracing :)
> >
> >
> > Ouch! You shouldn't do this. The patches inside were
> > trials submitted for review and the resulting discussion
> > concluded that the injection must be redesigned.
> 
> Oh, sorry...
> And I'd like to look at redesigning way of inject.
> 
> >
> > More generally I don't recommend you to base your patches
> > on my tree. I use it as a buffer when I send patches
> > for review or for pull requests.
> >
> > The branches inside can be randomly rebased (unless a
> > branch is waiting to be pulled) and they are pretty async
> > with the -tip tree.
> >
> > The hottest and most sync tree on which you should base your patches
> > for perf is:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git
> > 	perf/core
> >
> > With that you have best chances to work on a sane and up-to-date
> > base.
> >
> > Thanks.
> >
> >
> 
> I'll work on tip/perf/core from next time :)

You can also use tip:master for convenience - especially for RFC patches. That 
way you will have the latest and greatest (and tested) branches combined :)

	Ingo

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> > You add chained indirect calls into all lock ops, that's got to hurt.
> 
> Well, the idea was not bad at the first glance. It was separating lockdep 
> and lock events codes.
> 
> But indeed, the indirect calls plus the locking are not good for such a fast 
> path.

What would be nice to have is some sort of dynamic patching approach to enable 
_both_ lockdep, lockstat and perf lock.

If TRACE_EVENT() tracepoints were patchable we could use them. (but they arent 
right now)

	Ingo

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Jason Baron]

On Wed, Mar 17, 2010 at 10:52:30AM +0100, Ingo Molnar wrote:
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > > You add chained indirect calls into all lock ops, that's got to hurt.
> > 
> > Well, the idea was not bad at the first glance. It was separating lockdep 
> > and lock events codes.
> > 
> > But indeed, the indirect calls plus the locking are not good for such a fast 
> > path.
> 
> What would be nice to have is some sort of dynamic patching approach to enable 
> _both_ lockdep, lockstat and perf lock.
> 

right. this would allow distros to ship lockdep, lockstat in their
default kernels as a runtime option.


> If TRACE_EVENT() tracepoints were patchable we could use them. (but they arent 
> right now)
> 

right. I'm going to re-post the jump labeling work again soon, which
implicitly makes all TRACE_EVENT() tracepoints into dynamic patch
points. The jump label approach can also be deployed independently of
the tracepoints.

Also, any hints, suggestions on where to start with this type of
project? I thought a lot of the lockdep overhead was tied up in the data
structures? If its just a matter of identifying the dynamic patch
points. I can convert them to jump label and run benchmarks, pretty
easily.

thanks,

-Jason

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Frederic Weisbecker]

On Wed, Mar 17, 2010 at 04:30:53PM +0900, Hitoshi Mitake wrote:
> On 03/17/10 10:32, Frederic Weisbecker wrote:
> > On Sun, Mar 14, 2010 at 07:13:55PM +0100, Peter Zijlstra wrote:
> >> On Sun, 2010-03-14 at 19:38 +0900, Hitoshi Mitake wrote:
> >>> Current lockdep is too complicated because,
> >>>   * dependency validation
> >>>   * statistics
> >>>   * event tracing
> >>> are all implemented by it.
> >>> This cause problem of overhead.
> >>> If user enables one of them, overhead of rests part is not avoidable.
> >>> (tracing is exception. If user enables validation or stat,
> >>> overhead of tracing doesn't occur.)
> >>>
> >>> So I suggest new subsystem "lock monitor".
> >>> This is a general purpose lock event hooking mechanism.
> >>>
> >>> lock monitor will be enable easy implementing and running
> >>> these features related to lock.
> >>>
> >>> And I'm hoping that lock monitor will reduce overhead of perf lock.
> >>> Because lock monitor separates dependency validation and event  
> tracing clearly,
> >>> so calling of functions of lockdep (e.g. lock_acquire()) only for  
> validation
> >>> will not occur lock events.
> >>>
> >>> I implemented it on the branch perf/inject of Frederic's  
> random-tracing tree.
> >>> Because the branch is hottest place of lock and tracing :)
> >>
> >> OK, so I really don't like this much..
> >>
> >> Building a lockstat kernel (PROVE_LOCKING=n) should not have much more
> >> overhead than the proposed solution, if the simple lock acquistion
> >> tracking bothers you, you can do a patch to weaken that.
> >>
> >> I really really dislike how you add a monitor variable between
> >> everything for no reason what so ever.
> >>
> >> You use a new rwlock_t, which is an instant fail, those things are worse
> >> than useless.
> >>
> >> You add chained indirect calls into all lock ops, that's got to hurt.
> >
> >
> > Well, the idea was not bad at the first glance. It was separating
> > lockdep and lock events codes.
> >
> > But indeed, the indirect calls plus the locking are not good
> > for such a fast path.
> >
> > There is something else, it would be nice to keep the
> > lockdep_map ->  lockdep_class mapping so that we can
> > do lock profiling based on classes too. So we actually
> > need the lockdep code. What we don't need is the prove
> > locking or the lock stats. So I guess we can have a new
> > config to enable lock events and get rid of the prove
> > locking / lock stat code if we don't need it.
> >
> >
>
> Thanks for your comments, Peter and Frederic.
>
> My main motivation of writing this patch series was that
> some kernel codes uses lockdep functions (e.g. lock_acquire()) directly,
> so perf lock gets a lot of trace events without actual locks (e.g.  
> might_lock_read()).
> I think that these are confusable things for users.
>
> But I noticed that these events can be reduced by
> turning off CONFIG_PROVE_LOCKING. Yeah, my patch series was pointless... :)
>
> Should perf lock warn not to use with CONFIG_PROVE_LOCKING?


Ah I see.

might_lock_read() uses might_fault(), rcu, workqueues and probably
yet some others use sequences of lock_acquire/lock_release to prove
locking while there is actually no real lock operation involved, but
this is to detect dependency/balance mistakes.

I think that these cases are easily detectable in that they never have
any lock_acquired in their scenario. So may be we can just ignore
scenarios without lock_acquired and indeed advise users not to use
PROVE_LOCKING.

Thanks.

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Hitoshi Mitake]

On 03/18/10 00:39, Frederic Weisbecker wrote:
 > On Wed, Mar 17, 2010 at 04:30:53PM +0900, Hitoshi Mitake wrote:
 >> On 03/17/10 10:32, Frederic Weisbecker wrote:
 >>> On Sun, Mar 14, 2010 at 07:13:55PM +0100, Peter Zijlstra wrote:
 >>>> On Sun, 2010-03-14 at 19:38 +0900, Hitoshi Mitake wrote:
 >>>>> Current lockdep is too complicated because,
 >>>>>    * dependency validation
 >>>>>    * statistics
 >>>>>    * event tracing
 >>>>> are all implemented by it.
 >>>>> This cause problem of overhead.
 >>>>> If user enables one of them, overhead of rests part is not avoidable.
 >>>>> (tracing is exception. If user enables validation or stat,
 >>>>> overhead of tracing doesn't occur.)
 >>>>>
 >>>>> So I suggest new subsystem "lock monitor".
 >>>>> This is a general purpose lock event hooking mechanism.
 >>>>>
 >>>>> lock monitor will be enable easy implementing and running
 >>>>> these features related to lock.
 >>>>>
 >>>>> And I'm hoping that lock monitor will reduce overhead of perf lock.
 >>>>> Because lock monitor separates dependency validation and event
 >> tracing clearly,
 >>>>> so calling of functions of lockdep (e.g. lock_acquire()) only for
 >> validation
 >>>>> will not occur lock events.
 >>>>>
 >>>>> I implemented it on the branch perf/inject of Frederic's
 >> random-tracing tree.
 >>>>> Because the branch is hottest place of lock and tracing :)
 >>>>
 >>>> OK, so I really don't like this much..
 >>>>
 >>>> Building a lockstat kernel (PROVE_LOCKING=n) should not have much more
 >>>> overhead than the proposed solution, if the simple lock acquistion
 >>>> tracking bothers you, you can do a patch to weaken that.
 >>>>
 >>>> I really really dislike how you add a monitor variable between
 >>>> everything for no reason what so ever.
 >>>>
 >>>> You use a new rwlock_t, which is an instant fail, those things are 
worse
 >>>> than useless.
 >>>>
 >>>> You add chained indirect calls into all lock ops, that's got to hurt.
 >>>
 >>>
 >>> Well, the idea was not bad at the first glance. It was separating
 >>> lockdep and lock events codes.
 >>>
 >>> But indeed, the indirect calls plus the locking are not good
 >>> for such a fast path.
 >>>
 >>> There is something else, it would be nice to keep the
 >>> lockdep_map ->   lockdep_class mapping so that we can
 >>> do lock profiling based on classes too. So we actually
 >>> need the lockdep code. What we don't need is the prove
 >>> locking or the lock stats. So I guess we can have a new
 >>> config to enable lock events and get rid of the prove
 >>> locking / lock stat code if we don't need it.
 >>>
 >>>
 >>
 >> Thanks for your comments, Peter and Frederic.
 >>
 >> My main motivation of writing this patch series was that
 >> some kernel codes uses lockdep functions (e.g. lock_acquire()) directly,
 >> so perf lock gets a lot of trace events without actual locks (e.g.
 >> might_lock_read()).
 >> I think that these are confusable things for users.
 >>
 >> But I noticed that these events can be reduced by
 >> turning off CONFIG_PROVE_LOCKING. Yeah, my patch series was 
pointless... :)
 >>
 >> Should perf lock warn not to use with CONFIG_PROVE_LOCKING?
 >
 >
 > Ah I see.
 >
 > might_lock_read() uses might_fault(), rcu, workqueues and probably
 > yet some others use sequences of lock_acquire/lock_release to prove
 > locking while there is actually no real lock operation involved, but
 > this is to detect dependency/balance mistakes.
 >
 > I think that these cases are easily detectable in that they never have
 > any lock_acquired in their scenario. So may be we can just ignore
 > scenarios without lock_acquired and indeed advise users not to use
 > PROVE_LOCKING.

Unfortunately, we cannot use this detection method.
Because trylock series (e.g. spin_trylock()) only issues
lock_acquire() like this,

static inline int __raw_spin_trylock(raw_spinlock_t *lock)
{
	preempt_disable();
	if (do_raw_spin_trylock(lock)) {
		spin_acquire(&lock->monitor, 0, 1, _RET_IP_); <- spin_acquire() only 
issues lock_acquire()
		return 1;
	}
	preempt_enable();
	return 0;
}

So distinguishing trylocks and lock_acquire()/lock_release() pairs from
might_lock_read(), might_fault() and etc is hard.

It seems that turning off PROVE_LOCKING must be required
for state machine of perf lock.

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Hitoshi Mitake]

On 03/17/10 18:52, Ingo Molnar wrote:
 >
 > * Frederic Weisbecker<fweisbec@gmail.com>  wrote:
 >
 >>> You add chained indirect calls into all lock ops, that's got to hurt.
 >>
 >> Well, the idea was not bad at the first glance. It was separating 
lockdep
 >> and lock events codes.
 >>
 >> But indeed, the indirect calls plus the locking are not good for 
such a fast
 >> path.
 >
 > What would be nice to have is some sort of dynamic patching approach 
to enable
 > _both_ lockdep, lockstat and perf lock.
 >
 > If TRACE_EVENT() tracepoints were patchable we could use them. (but 
they arent
 > right now)

I'll try it!

And I have a question related to this dynamic patching approach for lockdep.
If dynamic proving turning on/off is provided,
lockdep will be confused by inconsistency of lock acquiring log.

Will the sequence,

lock_acquire(l) -> turning off -> lock_release(l) -> turning on -> 
lock_acquire(l)

detected as double acquiring?

Should turning on/off lockdep be done in the time
when every processes have no lock?

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Frederic Weisbecker]

On Thu, Mar 18, 2010 at 02:49:38PM +0900, Hitoshi Mitake wrote:
> Unfortunately, we cannot use this detection method.
> Because trylock series (e.g. spin_trylock()) only issues
> lock_acquire() like this,
>
> static inline int __raw_spin_trylock(raw_spinlock_t *lock)
> {
> 	preempt_disable();
> 	if (do_raw_spin_trylock(lock)) {
> 		spin_acquire(&lock->monitor, 0, 1, _RET_IP_); <- spin_acquire() only  
> issues lock_acquire()
> 		return 1;
> 	}
> 	preempt_enable();
> 	return 0;
> }
>
> So distinguishing trylocks and lock_acquire()/lock_release() pairs from
> might_lock_read(), might_fault() and etc is hard.
>
> It seems that turning off PROVE_LOCKING must be required
> for state machine of perf lock.


No that's really not a problem. trylocks are pointless in latency
profiling because by definition they don't content. OTOH, they
grab the lock and other locks might wait and raise latencies. So
they are part of the profile. But we don't care about having the
usual acquire/aquired/release sequence as we have the flags that
tell us if this is a trylock.

So we just need to consider that acquire:try_lock - release is
a right lock scenario, but that acquire - release is only a lockdep
check.

Hm?

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Frederic Weisbecker]

On Thu, Mar 18, 2010 at 02:59:29PM +0900, Hitoshi Mitake wrote:
> On 03/17/10 18:52, Ingo Molnar wrote:
> >
> > * Frederic Weisbecker<fweisbec@gmail.com>  wrote:
> >
> >>> You add chained indirect calls into all lock ops, that's got to hurt.
> >>
> >> Well, the idea was not bad at the first glance. It was separating  
> lockdep
> >> and lock events codes.
> >>
> >> But indeed, the indirect calls plus the locking are not good for such 
> a fast
> >> path.
> >
> > What would be nice to have is some sort of dynamic patching approach  
> to enable
> > _both_ lockdep, lockstat and perf lock.
> >
> > If TRACE_EVENT() tracepoints were patchable we could use them. (but  
> they arent
> > right now)
>
> I'll try it!



I sometimes wonder which trick between jmp optimization and hot patching
would be the best to optimize the tracepoints off-cases.

I should look more closely at the jmp optimization. I don't know if
it avoids to push the tracepoints parameters in the off case, in
which case it could be perhaps more efficient than hot patching,
although perhaps most of the time the given arguments are already in
registers because the traced function uses them for its own needs.

Also, adopting hot patching means the tracepoint calls would be
in a non-inlined separated function. The result would be probably
less i-cache footprint from the caller, and better for the off-case,
worse for the on-case. But tracing off-case is most important.

(Adding more people in Cc)


> And I have a question related to this dynamic patching approach for lockdep.
> If dynamic proving turning on/off is provided,
> lockdep will be confused by inconsistency of lock acquiring log.
>
> Will the sequence,
>
> lock_acquire(l) -> turning off -> lock_release(l) -> turning on ->  
> lock_acquire(l)
>
> detected as double acquiring?
>
> Should turning on/off lockdep be done in the time
> when every processes have no lock?


There is almost always a process with a lock somewhere ;-)

This is not a big deal, it's very similar to unfinished scenarios
due to the end of the tracing that can happen anytime and you miss
a lock_release or whatever. We can also begin the tracing anytime,
and you may receive orphan lock_release in the very beginning
because you missed the lock_acquire that happened before the tracing.

Any locking scenario that doesn't fit into the state machine
or is incomplete must be considered as broken and then ignored.

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Mathieu Desnoyers]

* Frederic Weisbecker (fweisbec@gmail.com) wrote:
> On Thu, Mar 18, 2010 at 02:59:29PM +0900, Hitoshi Mitake wrote:
> > On 03/17/10 18:52, Ingo Molnar wrote:
> > >
> > > * Frederic Weisbecker<fweisbec@gmail.com>  wrote:
> > >
> > >>> You add chained indirect calls into all lock ops, that's got to hurt.
> > >>
> > >> Well, the idea was not bad at the first glance. It was separating  
> > lockdep
> > >> and lock events codes.
> > >>
> > >> But indeed, the indirect calls plus the locking are not good for such 
> > a fast
> > >> path.
> > >
> > > What would be nice to have is some sort of dynamic patching approach  
> > to enable
> > > _both_ lockdep, lockstat and perf lock.
> > >
> > > If TRACE_EVENT() tracepoints were patchable we could use them. (but  
> > they arent
> > > right now)
> >
> > I'll try it!
> 
> 
> 
> I sometimes wonder which trick between jmp optimization and hot patching
> would be the best to optimize the tracepoints off-cases.
> 
> I should look more closely at the jmp optimization. I don't know if
> it avoids to push the tracepoints parameters in the off case, in
> which case it could be perhaps more efficient than hot patching,

yep, tracepoints with jump patching will branch over the whole stack setup in
the off case, which is one of the good reasons for using this solution over
patching only a call (leaving the stack setup in place).

Note that if the parameters include side-effects (such as a function call),
these will be executed even when the tracepoint is disabled. This is why people
should implement these calls with side-effects in the appropriate TRACE_EVENT
fields.

> although perhaps most of the time the given arguments are already in
> registers because the traced function uses them for its own needs.
> 
> Also, adopting hot patching means the tracepoint calls would be
> in a non-inlined separated function. The result would be probably
> less i-cache footprint from the caller, and better for the off-case,
> worse for the on-case. But tracing off-case is most important.
> 
> (Adding more people in Cc)
> 

The idea has been discussed to add support in gcc to emit the code for an
unlikely branch into a separate section, which does have the smaller cache-line
footprint benefit your are talking about, but without the overhead of the extra
out-of-line function call in the enabled case. I don't know how this work is
advanced though. We had determined that the "asm goto" was an higher priority
item.

Thanks,

Mathieu

> 
> > And I have a question related to this dynamic patching approach for lockdep.
> > If dynamic proving turning on/off is provided,
> > lockdep will be confused by inconsistency of lock acquiring log.
> >
> > Will the sequence,
> >
> > lock_acquire(l) -> turning off -> lock_release(l) -> turning on ->  
> > lock_acquire(l)
> >
> > detected as double acquiring?
> >
> > Should turning on/off lockdep be done in the time
> > when every processes have no lock?
> 
> 
> There is almost always a process with a lock somewhere ;-)
> 
> This is not a big deal, it's very similar to unfinished scenarios
> due to the end of the tracing that can happen anytime and you miss
> a lock_release or whatever. We can also begin the tracing anytime,
> and you may receive orphan lock_release in the very beginning
> because you missed the lock_acquire that happened before the tracing.
> 
> Any locking scenario that doesn't fit into the state machine
> or is incomplete must be considered as broken and then ignored.
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Frederic Weisbecker]

On Thu, Mar 18, 2010 at 09:08:57PM -0400, Mathieu Desnoyers wrote:
> > I sometimes wonder which trick between jmp optimization and hot patching
> > would be the best to optimize the tracepoints off-cases.
> > 
> > I should look more closely at the jmp optimization. I don't know if
> > it avoids to push the tracepoints parameters in the off case, in
> > which case it could be perhaps more efficient than hot patching,
> 
> yep, tracepoints with jump patching will branch over the whole stack setup in
> the off case, which is one of the good reasons for using this solution over
> patching only a call (leaving the stack setup in place).



Ok that's good to know. It's a pretty good argument against hot
patching in this particular case.


 
> Note that if the parameters include side-effects (such as a function call),
> these will be executed even when the tracepoint is disabled. This is why people
> should implement these calls with side-effects in the appropriate TRACE_EVENT
> fields.


Good to know too.
But this makes me curious. So it guarantees stack setup won't happen but
can't sort it out with functions as parameters or so?

I have no idea how this thing works. Please Cc me for the next batch,
this looks like a cool thing :)



> > although perhaps most of the time the given arguments are already in
> > registers because the traced function uses them for its own needs.
> > 
> > Also, adopting hot patching means the tracepoint calls would be
> > in a non-inlined separated function. The result would be probably
> > less i-cache footprint from the caller, and better for the off-case,
> > worse for the on-case. But tracing off-case is most important.
> > 
> > (Adding more people in Cc)
> > 
> 
> The idea has been discussed to add support in gcc to emit the code for an
> unlikely branch into a separate section, which does have the smaller cache-line
> footprint benefit your are talking about, but without the overhead of the extra
> out-of-line function call in the enabled case. I don't know how this work is
> advanced though. We had determined that the "asm goto" was an higher priority
> item.


Ok.

Thanks!

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Mathieu Desnoyers]

* Frederic Weisbecker (fweisbec@gmail.com) wrote:
> On Thu, Mar 18, 2010 at 09:08:57PM -0400, Mathieu Desnoyers wrote:
> > > I sometimes wonder which trick between jmp optimization and hot patching
> > > would be the best to optimize the tracepoints off-cases.
> > > 
> > > I should look more closely at the jmp optimization. I don't know if
> > > it avoids to push the tracepoints parameters in the off case, in
> > > which case it could be perhaps more efficient than hot patching,
> > 
> > yep, tracepoints with jump patching will branch over the whole stack setup in
> > the off case, which is one of the good reasons for using this solution over
> > patching only a call (leaving the stack setup in place).
> 
> 
> 
> Ok that's good to know. It's a pretty good argument against hot
> patching in this particular case.
> 
> 
>  
> > Note that if the parameters include side-effects (such as a function call),
> > these will be executed even when the tracepoint is disabled. This is why people
> > should implement these calls with side-effects in the appropriate TRACE_EVENT
> > fields.
> 
> 
> Good to know too.
> But this makes me curious. So it guarantees stack setup won't happen but
> can't sort it out with functions as parameters or so?
> 
> I have no idea how this thing works. Please Cc me for the next batch,
> this looks like a cool thing :)
> 

Well, the now deceased "Linux Kernel Markers" (which were based on a single
macro rather than static inline functions) were able to use the preprocessor to
put function calls passed as argument within the conditional branch. But with
tracepoints, we rely on static inlines to have flexible parameter declaration,
so this is not possible.

All the arguments passed to the static inline (eventually used for the stack
setup of the actual function call within the tracepoint) can be moved into the
conditional branch by the compiler optimizations, because they are not needed
otherwise. However, this cannot be done for function calls passed as parameter
to the tracepoint, because the compiler do not know whether or not the function
side-effects are needed outside of the "tracing active" branch.

Mathieu

> 
> 
> > > although perhaps most of the time the given arguments are already in
> > > registers because the traced function uses them for its own needs.
> > > 
> > > Also, adopting hot patching means the tracepoint calls would be
> > > in a non-inlined separated function. The result would be probably
> > > less i-cache footprint from the caller, and better for the off-case,
> > > worse for the on-case. But tracing off-case is most important.
> > > 
> > > (Adding more people in Cc)
> > > 
> > 
> > The idea has been discussed to add support in gcc to emit the code for an
> > unlikely branch into a separate section, which does have the smaller cache-line
> > footprint benefit your are talking about, but without the overhead of the extra
> > out-of-line function call in the enabled case. I don't know how this work is
> > advanced though. We had determined that the "asm goto" was an higher priority
> > item.
> 
> 
> Ok.
> 
> Thanks!
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Frederic Weisbecker]

On Thu, Mar 18, 2010 at 09:36:58PM -0400, Mathieu Desnoyers wrote:
> * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> > On Thu, Mar 18, 2010 at 09:08:57PM -0400, Mathieu Desnoyers wrote:
> > > > I sometimes wonder which trick between jmp optimization and hot patching
> > > > would be the best to optimize the tracepoints off-cases.
> > > > 
> > > > I should look more closely at the jmp optimization. I don't know if
> > > > it avoids to push the tracepoints parameters in the off case, in
> > > > which case it could be perhaps more efficient than hot patching,
> > > 
> > > yep, tracepoints with jump patching will branch over the whole stack setup in
> > > the off case, which is one of the good reasons for using this solution over
> > > patching only a call (leaving the stack setup in place).
> > 
> > 
> > 
> > Ok that's good to know. It's a pretty good argument against hot
> > patching in this particular case.
> > 
> > 
> >  
> > > Note that if the parameters include side-effects (such as a function call),
> > > these will be executed even when the tracepoint is disabled. This is why people
> > > should implement these calls with side-effects in the appropriate TRACE_EVENT
> > > fields.
> > 
> > 
> > Good to know too.
> > But this makes me curious. So it guarantees stack setup won't happen but
> > can't sort it out with functions as parameters or so?
> > 
> > I have no idea how this thing works. Please Cc me for the next batch,
> > this looks like a cool thing :)
> > 
> 
> Well, the now deceased "Linux Kernel Markers" (which were based on a single
> macro rather than static inline functions) were able to use the preprocessor to
> put function calls passed as argument within the conditional branch. But with
> tracepoints, we rely on static inlines to have flexible parameter declaration,
> so this is not possible.


Ok.



> All the arguments passed to the static inline (eventually used for the stack
> setup of the actual function call within the tracepoint) can be moved into the
> conditional branch by the compiler optimizations, because they are not needed
> otherwise. However, this cannot be done for function calls passed as parameter
> to the tracepoint, because the compiler do not know whether or not the function
> side-effects are needed outside of the "tracing active" branch.
> 
> Mathieu



Ok. I just read this: http://lwn.net/Articles/362752/ and
this: http://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

What is funky is that the gcc example takes this jmp/nop patching
as an example to explain asm goto, so it all looks clear to me now :-)

But, looking at __DO_TRACE:

	if (it_func) {						\
		do {						\
			((void(*)(proto))(*it_func))(args);	\
		} while (*(++it_func));				\
	}

I would expect the compiler not to load the parameters in the stack
before first checking the branch.
So, the fact that parameters are not loaded before we know we'll call
the tracepoint is something we already have or is it something that the jump
label brings in the package somehow?

Thanks.

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Mathieu Desnoyers]

* Frederic Weisbecker (fweisbec@gmail.com) wrote:
> On Thu, Mar 18, 2010 at 09:36:58PM -0400, Mathieu Desnoyers wrote:
> > * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> > > On Thu, Mar 18, 2010 at 09:08:57PM -0400, Mathieu Desnoyers wrote:
> > > > > I sometimes wonder which trick between jmp optimization and hot patching
> > > > > would be the best to optimize the tracepoints off-cases.
> > > > > 
> > > > > I should look more closely at the jmp optimization. I don't know if
> > > > > it avoids to push the tracepoints parameters in the off case, in
> > > > > which case it could be perhaps more efficient than hot patching,
> > > > 
> > > > yep, tracepoints with jump patching will branch over the whole stack setup in
> > > > the off case, which is one of the good reasons for using this solution over
> > > > patching only a call (leaving the stack setup in place).
> > > 
> > > 
> > > 
> > > Ok that's good to know. It's a pretty good argument against hot
> > > patching in this particular case.
> > > 
> > > 
> > >  
> > > > Note that if the parameters include side-effects (such as a function call),
> > > > these will be executed even when the tracepoint is disabled. This is why people
> > > > should implement these calls with side-effects in the appropriate TRACE_EVENT
> > > > fields.
> > > 
> > > 
> > > Good to know too.
> > > But this makes me curious. So it guarantees stack setup won't happen but
> > > can't sort it out with functions as parameters or so?
> > > 
> > > I have no idea how this thing works. Please Cc me for the next batch,
> > > this looks like a cool thing :)
> > > 
> > 
> > Well, the now deceased "Linux Kernel Markers" (which were based on a single
> > macro rather than static inline functions) were able to use the preprocessor to
> > put function calls passed as argument within the conditional branch. But with
> > tracepoints, we rely on static inlines to have flexible parameter declaration,
> > so this is not possible.
> 
> 
> Ok.
> 
> 
> 
> > All the arguments passed to the static inline (eventually used for the stack
> > setup of the actual function call within the tracepoint) can be moved into the
> > conditional branch by the compiler optimizations, because they are not needed
> > otherwise. However, this cannot be done for function calls passed as parameter
> > to the tracepoint, because the compiler do not know whether or not the function
> > side-effects are needed outside of the "tracing active" branch.
> > 
> > Mathieu
> 
> 
> 
> Ok. I just read this: http://lwn.net/Articles/362752/ and
> this: http://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
> 
> What is funky is that the gcc example takes this jmp/nop patching
> as an example to explain asm goto, so it all looks clear to me now :-)

Well, the use-case that drove the asm goto implementation _is_ the tracepoints.
;)

> 
> But, looking at __DO_TRACE:
> 
> 	if (it_func) {						\
> 		do {						\
> 			((void(*)(proto))(*it_func))(args);	\
> 		} while (*(++it_func));				\
> 	}
> 
> I would expect the compiler not to load the parameters in the stack
> before first checking the branch.

Note that you have to put that in its full context. It's a macro expanded within
a static inline function. The initial parameters are passed to the static
inline, not directly as "args" here. So parameters with side-effects have to be
evaluated before their result can be passed to the static inline function, so in
that sense their evaluation cannot be moved into the conditional branch.

> So, the fact that parameters are not loaded before we know we'll call
> the tracepoint is something we already have or is it something that the jump
> label brings in the package somehow?

It's standard compiler optimization behavior.

Thanks,

Mathieu

> 
> Thanks.
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Frederic Weisbecker]

On Thu, Mar 18, 2010 at 10:40:42PM -0400, Mathieu Desnoyers wrote:
> Well, the use-case that drove the asm goto implementation _is_ the tracepoints.
> ;)
> 
> > 
> > But, looking at __DO_TRACE:
> > 
> > 	if (it_func) {						\
> > 		do {						\
> > 			((void(*)(proto))(*it_func))(args);	\
> > 		} while (*(++it_func));				\
> > 	}
> > 
> > I would expect the compiler not to load the parameters in the stack
> > before first checking the branch.
> 
> Note that you have to put that in its full context. It's a macro expanded within
> a static inline function. The initial parameters are passed to the static
> inline, not directly as "args" here. So parameters with side-effects have to be
> evaluated before their result can be passed to the static inline function, so in
> that sense their evaluation cannot be moved into the conditional branch.


Evaluation yeah, I agree. A function passed as an argument is
going to be evaluated indeed, or whatever thing that has a side effect.
But there is nothing here that need to setup the parameters to the stack
right before the true tracepoint call, not until we passed the branch check
once.

 
> > So, the fact that parameters are not loaded before we know we'll call
> > the tracepoint is something we already have or is it something that the jump
> > label brings in the package somehow?
> 
> It's standard compiler optimization behavior.


Sure. My doubt is: currently with the upstream version, does the
compiler tend to load the parameters to the stack before the branch is
checked? Or is this a magic that jmp labels bring for whatever reason?

Thanks.

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Mathieu Desnoyers]

* Frederic Weisbecker (fweisbec@gmail.com) wrote:
> On Thu, Mar 18, 2010 at 10:40:42PM -0400, Mathieu Desnoyers wrote:
> > Well, the use-case that drove the asm goto implementation _is_ the tracepoints.
> > ;)
> > 
> > > 
> > > But, looking at __DO_TRACE:
> > > 
> > > 	if (it_func) {						\
> > > 		do {						\
> > > 			((void(*)(proto))(*it_func))(args);	\
> > > 		} while (*(++it_func));				\
> > > 	}
> > > 
> > > I would expect the compiler not to load the parameters in the stack
> > > before first checking the branch.
> > 
> > Note that you have to put that in its full context. It's a macro expanded within
> > a static inline function. The initial parameters are passed to the static
> > inline, not directly as "args" here. So parameters with side-effects have to be
> > evaluated before their result can be passed to the static inline function, so in
> > that sense their evaluation cannot be moved into the conditional branch.
> 
> 
> Evaluation yeah, I agree. A function passed as an argument is
> going to be evaluated indeed, or whatever thing that has a side effect.
> But there is nothing here that need to setup the parameters to the stack
> right before the true tracepoint call, not until we passed the branch check
> once.
> 
>  
> > > So, the fact that parameters are not loaded before we know we'll call
> > > the tracepoint is something we already have or is it something that the jump
> > > label brings in the package somehow?
> > 
> > It's standard compiler optimization behavior.
> 
> 
> Sure. My doubt is: currently with the upstream version, does the
> compiler tend to load the parameters to the stack before the branch is
> checked? Or is this a magic that jmp labels bring for whatever reason?

Even without the static jump patching, the compiler takes care of putting the
stack setup after the branch is checked. That worked with a standard test on a
variable, with immediate values and should still work with asm gotos.

I already did some presentations around these question. You can refer to my OLS
2008 slides, where I proposed static jump patching (ancestor of the asm gotos,
where I did the liveness analysis myself in the back of the compiler; good
enough for a prototype ;) ). Slide 11 discusses the branch vs stack setup
question:

http://lttng.org/files/slides/desnoyers-talk-ols2008.pdf

Even more in Sections 8.1, 8.2 and 8.3 of my thesis:

http://lttng.org/files/thesis/desnoyers-dissertation-2009-12-v27.pdf

8.1 Kernel Markers
8.2 Tracepoints
8.3 Immediate Values

Thanks,

Mathieu

> 
> Thanks.
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Jason Baron]

On Fri, Mar 19, 2010 at 08:56:00AM -0400, Mathieu Desnoyers wrote:
> * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> > On Thu, Mar 18, 2010 at 10:40:42PM -0400, Mathieu Desnoyers wrote:
> > > Well, the use-case that drove the asm goto implementation _is_ the tracepoints.
> > > ;)
> > > 
> > > > 
> > > > But, looking at __DO_TRACE:
> > > > 
> > > > 	if (it_func) {						\
> > > > 		do {						\
> > > > 			((void(*)(proto))(*it_func))(args);	\
> > > > 		} while (*(++it_func));				\
> > > > 	}
> > > > 
> > > > I would expect the compiler not to load the parameters in the stack
> > > > before first checking the branch.
> > > 
> > > Note that you have to put that in its full context. It's a macro expanded within
> > > a static inline function. The initial parameters are passed to the static
> > > inline, not directly as "args" here. So parameters with side-effects have to be
> > > evaluated before their result can be passed to the static inline function, so in
> > > that sense their evaluation cannot be moved into the conditional branch.
> > 
> > 
> > Evaluation yeah, I agree. A function passed as an argument is
> > going to be evaluated indeed, or whatever thing that has a side effect.
> > But there is nothing here that need to setup the parameters to the stack
> > right before the true tracepoint call, not until we passed the branch check
> > once.
> > 
> >  
> > > > So, the fact that parameters are not loaded before we know we'll call
> > > > the tracepoint is something we already have or is it something that the jump
> > > > label brings in the package somehow?
> > > 
> > > It's standard compiler optimization behavior.
> > 
> > 
> > Sure. My doubt is: currently with the upstream version, does the
> > compiler tend to load the parameters to the stack before the branch is
> > checked? Or is this a magic that jmp labels bring for whatever reason?
> 
> Even without the static jump patching, the compiler takes care of putting the
> stack setup after the branch is checked. That worked with a standard test on a
> variable, with immediate values and should still work with asm gotos.

right. stack setup happens after the branch is checked for asm gotos as
well. However, as mentioned functions as parameters, which have side-effects
need to be evaluated in the off case, there's nothing to be done about
that as its a correctness issue.

Hoever, constructs like a->b, do evaluated even in the disabled case.
This could be solved via macros, see my proposed patch set: 
http://marc.info/?l=linux-kernel&m=124276710606872&w=2

However, the conclusion of the thread was that this should be done in
the compiler, and as such I filed a bug with gcc about this issue.

I'll re-post an updated jump label series shortly.

thanks,

-Jason

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Frederic Weisbecker]

On Fri, Mar 19, 2010 at 08:56:00AM -0400, Mathieu Desnoyers wrote:
> > Sure. My doubt is: currently with the upstream version, does the
> > compiler tend to load the parameters to the stack before the branch is
> > checked? Or is this a magic that jmp labels bring for whatever reason?
> 
> Even without the static jump patching, the compiler takes care of putting the
> stack setup after the branch is checked. That worked with a standard test on a
> variable, with immediate values and should still work with asm gotos.


Ok.


 
> I already did some presentations around these question. You can refer to my OLS
> 2008 slides, where I proposed static jump patching (ancestor of the asm gotos,
> where I did the liveness analysis myself in the back of the compiler; good
> enough for a prototype ;) ). Slide 11 discusses the branch vs stack setup
> question:
> 
> http://lttng.org/files/slides/desnoyers-talk-ols2008.pdf
> 
> Even more in Sections 8.1, 8.2 and 8.3 of my thesis:
> 
> http://lttng.org/files/thesis/desnoyers-dissertation-2009-12-v27.pdf
> 
> 8.1 Kernel Markers
> 8.2 Tracepoints
> 8.3 Immediate Values


Cool. I was already keeping your thesis on hand anyway :)

Thanks.

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Frederic Weisbecker]

On Fri, Mar 19, 2010 at 12:00:41PM -0400, Jason Baron wrote:
> On Fri, Mar 19, 2010 at 08:56:00AM -0400, Mathieu Desnoyers wrote:
> > * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> > > On Thu, Mar 18, 2010 at 10:40:42PM -0400, Mathieu Desnoyers wrote:
> > > > Well, the use-case that drove the asm goto implementation _is_ the tracepoints.
> > > > ;)
> > > > 
> > > > > 
> > > > > But, looking at __DO_TRACE:
> > > > > 
> > > > > 	if (it_func) {						\
> > > > > 		do {						\
> > > > > 			((void(*)(proto))(*it_func))(args);	\
> > > > > 		} while (*(++it_func));				\
> > > > > 	}
> > > > > 
> > > > > I would expect the compiler not to load the parameters in the stack
> > > > > before first checking the branch.
> > > > 
> > > > Note that you have to put that in its full context. It's a macro expanded within
> > > > a static inline function. The initial parameters are passed to the static
> > > > inline, not directly as "args" here. So parameters with side-effects have to be
> > > > evaluated before their result can be passed to the static inline function, so in
> > > > that sense their evaluation cannot be moved into the conditional branch.
> > > 
> > > 
> > > Evaluation yeah, I agree. A function passed as an argument is
> > > going to be evaluated indeed, or whatever thing that has a side effect.
> > > But there is nothing here that need to setup the parameters to the stack
> > > right before the true tracepoint call, not until we passed the branch check
> > > once.
> > > 
> > >  
> > > > > So, the fact that parameters are not loaded before we know we'll call
> > > > > the tracepoint is something we already have or is it something that the jump
> > > > > label brings in the package somehow?
> > > > 
> > > > It's standard compiler optimization behavior.
> > > 
> > > 
> > > Sure. My doubt is: currently with the upstream version, does the
> > > compiler tend to load the parameters to the stack before the branch is
> > > checked? Or is this a magic that jmp labels bring for whatever reason?
> > 
> > Even without the static jump patching, the compiler takes care of putting the
> > stack setup after the branch is checked. That worked with a standard test on a
> > variable, with immediate values and should still work with asm gotos.
> 
> right. stack setup happens after the branch is checked for asm gotos as
> well. However, as mentioned functions as parameters, which have side-effects
> need to be evaluated in the off case, there's nothing to be done about
> that as its a correctness issue.
> 
> Hoever, constructs like a->b, do evaluated even in the disabled case.
> This could be solved via macros, see my proposed patch set: 
> http://marc.info/?l=linux-kernel&m=124276710606872&w=2
> 
> However, the conclusion of the thread was that this should be done in
> the compiler, and as such I filed a bug with gcc about this issue.
> 
> I'll re-post an updated jump label series shortly.


Ok, thanks guys for these informations.

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Hitoshi Mitake]

On 03/19/10 05:30, Frederic Weisbecker wrote:
> On Thu, Mar 18, 2010 at 02:49:38PM +0900, Hitoshi Mitake wrote:
>> Unfortunately, we cannot use this detection method.
>> Because trylock series (e.g. spin_trylock()) only issues
>> lock_acquire() like this,
>>
>> static inline int __raw_spin_trylock(raw_spinlock_t *lock)
>> {
>> 	preempt_disable();
>> 	if (do_raw_spin_trylock(lock)) {
>> 		spin_acquire(&lock->monitor, 0, 1, _RET_IP_);<- spin_acquire() only
>> issues lock_acquire()
>> 		return 1;
>> 	}
>> 	preempt_enable();
>> 	return 0;
>> }
>>
>> So distinguishing trylocks and lock_acquire()/lock_release() pairs from
>> might_lock_read(), might_fault() and etc is hard.
>>
>> It seems that turning off PROVE_LOCKING must be required
>> for state machine of perf lock.
>
>
> No that's really not a problem. trylocks are pointless in latency
> profiling because by definition they don't content. OTOH, they
> grab the lock and other locks might wait and raise latencies. So
> they are part of the profile. But we don't care about having the
> usual acquire/aquired/release sequence as we have the flags that
> tell us if this is a trylock.
>
> So we just need to consider that acquire:try_lock - release is
> a right lock scenario, but that acquire - release is only a lockdep
> check.

Ah, I see. The argument flags of lock_acquire event can be used for
distinguishing try or read lock and pure verifying.

Thanks,
	Hitoshi

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Hitoshi Mitake]

On 03/19/10 06:16, Frederic Weisbecker wrote:
 >> And I have a question related to this dynamic patching approach for 
lockdep.
 >> If dynamic proving turning on/off is provided,
 >> lockdep will be confused by inconsistency of lock acquiring log.
 >>
 >> Will the sequence,
 >>
 >> lock_acquire(l) ->  turning off ->  lock_release(l) ->  turning on ->
 >> lock_acquire(l)
 >>
 >> detected as double acquiring?
 >>
 >> Should turning on/off lockdep be done in the time
 >> when every processes have no lock?
 >
 >
 > There is almost always a process with a lock somewhere ;-)

Yeah :)

 >
 > This is not a big deal, it's very similar to unfinished scenarios
 > due to the end of the tracing that can happen anytime and you miss
 > a lock_release or whatever. We can also begin the tracing anytime,
 > and you may receive orphan lock_release in the very beginning
 > because you missed the lock_acquire that happened before the tracing.
 >
 > Any locking scenario that doesn't fit into the state machine
 > or is incomplete must be considered as broken and then ignored.
 >
 >

I see, thanks.
I have to fix state machine of perf lock.
Now it doesn't consider read, try and orphan events,
it is very incompletely..

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Hitoshi Mitake]

On 03/20/10 14:56, Hitoshi Mitake wrote:
 > On 03/19/10 06:16, Frederic Weisbecker wrote:
 >  >> And I have a question related to this dynamic patching approach for
 > lockdep.
 >  >> If dynamic proving turning on/off is provided,
 >  >> lockdep will be confused by inconsistency of lock acquiring log.
 >  >>
 >  >> Will the sequence,
 >  >>
 >  >> lock_acquire(l) -> turning off -> lock_release(l) -> turning on ->
 >  >> lock_acquire(l)
 >  >>
 >  >> detected as double acquiring?
 >  >>
 >  >> Should turning on/off lockdep be done in the time
 >  >> when every processes have no lock?
 >  >
 >  >
 >  > There is almost always a process with a lock somewhere ;-)
 >
 > Yeah :)
 >
 >  >
 >  > This is not a big deal, it's very similar to unfinished scenarios
 >  > due to the end of the tracing that can happen anytime and you miss
 >  > a lock_release or whatever. We can also begin the tracing anytime,
 >  > and you may receive orphan lock_release in the very beginning
 >  > because you missed the lock_acquire that happened before the tracing.
 >  >
 >  > Any locking scenario that doesn't fit into the state machine
 >  > or is incomplete must be considered as broken and then ignored.
 >  >
 >  >
 >
 > I see, thanks.
 > I have to fix state machine of perf lock.
 > Now it doesn't consider read, try and orphan events,
 > it is very incompletely..
 >

Ah, sorry, I've mentioned that these cases might be
a problem for validation part of lockdep, not for events.

If the lock and turning on/off sequence like this happened,
lock_acquire(l) -> turning off -> lock_release(l) -> turning on -> 
lock_acquire(l)
this will confuse validator of lockdep.
At least, task_struct.lockdep_depth will be corrupted.

And I have a trivial question to Ingo.
In lockdep, held_locks of task_struct are accessed this arithmetical way
		prev = curr->held_locks + i;
Of course this is valid way, but I feel it is more simple and natural way
		prev = curr->held_locks[i];

Is there a reason for this style?
This is a pure question. I have no intention to rewrite them :)

Thanks,
	Hitoshi

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Hitoshi Mitake]

On 03/20/10 17:23, Hitoshi Mitake wrote:
 > On 03/20/10 14:56, Hitoshi Mitake wrote:
 >  > On 03/19/10 06:16, Frederic Weisbecker wrote:
 >  > >> And I have a question related to this dynamic patching approach for
 >  > lockdep.
 >  > >> If dynamic proving turning on/off is provided,
 >  > >> lockdep will be confused by inconsistency of lock acquiring log.
 >  > >>
 >  > >> Will the sequence,
 >  > >>
 >  > >> lock_acquire(l) -> turning off -> lock_release(l) -> turning on ->
 >  > >> lock_acquire(l)
 >  > >>
 >  > >> detected as double acquiring?
 >  > >>
 >  > >> Should turning on/off lockdep be done in the time
 >  > >> when every processes have no lock?
 >  > >
 >  > >
 >  > > There is almost always a process with a lock somewhere ;-)
 >  >
 >  > Yeah :)
 >  >
 >  > >
 >  > > This is not a big deal, it's very similar to unfinished scenarios
 >  > > due to the end of the tracing that can happen anytime and you miss
 >  > > a lock_release or whatever. We can also begin the tracing anytime,
 >  > > and you may receive orphan lock_release in the very beginning
 >  > > because you missed the lock_acquire that happened before the 
tracing.
 >  > >
 >  > > Any locking scenario that doesn't fit into the state machine
 >  > > or is incomplete must be considered as broken and then ignored.
 >  > >
 >  > >
 >  >
 >  > I see, thanks.
 >  > I have to fix state machine of perf lock.
 >  > Now it doesn't consider read, try and orphan events,
 >  > it is very incompletely..
 >  >
 >
 > Ah, sorry, I've mentioned that these cases might be
 > a problem for validation part of lockdep, not for events.
 >
 > If the lock and turning on/off sequence like this happened,
 > lock_acquire(l) -> turning off -> lock_release(l) -> turning on ->
 > lock_acquire(l)
 > this will confuse validator of lockdep.
 > At least, task_struct.lockdep_depth will be corrupted.
 >
 > And I have a trivial question to Ingo.
 > In lockdep, held_locks of task_struct are accessed this arithmetical way
 > prev = curr->held_locks + i;
 > Of course this is valid way, but I feel it is more simple and natural way
 > prev = curr->held_locks[i];
 >

Ah, sorry,
     prev = curr->held_locks[i];
is wrong. It's
     prev = &curr->held_locks[i];

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Peter Zijlstra]

On Sat, 2010-03-20 at 17:23 +0900, Hitoshi Mitake wrote:
> In lockdep, held_locks of task_struct are accessed this arithmetical way
>                 prev = curr->held_locks + i;
> Of course this is valid way, but I feel it is more simple and natural way
>                 prev = curr->held_locks[i];
> 

The latter is a type mis-match, an equivalent expression would be:
   &curr->held_locks[i];

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Peter Zijlstra]

On Wed, 2010-03-17 at 16:39 +0100, Frederic Weisbecker wrote:
> 
> might_lock_read() uses might_fault(), rcu, workqueues and probably
> yet some others use sequences of lock_acquire/lock_release to prove
> locking while there is actually no real lock operation involved, but
> this is to detect dependency/balance mistakes. 

might_fault() simply always takes the mmap_sem because actually hitting
the fault path (which otherwise would establish that relation) is very
rare for some cases, so by forcing that dependency we get better
coverage.

rcu_read_lock() is an actual lock :-)

workqueues use 'fake' locks to connect lock chains to flush, so that we
can detect things like trying to flush a workqueue while holding a lock
that is required to complete the work.

Re: [PATCH RFC 00/11] lock monitor: Separate features related to lock

[Hitoshi Mitake]

On 03/24/10 00:32, Peter Zijlstra wrote:
 > On Sat, 2010-03-20 at 17:23 +0900, Hitoshi Mitake wrote:
 >> In lockdep, held_locks of task_struct are accessed this arithmetical way
 >>                  prev = curr->held_locks + i;
 >> Of course this is valid way, but I feel it is more simple and 
natural way
 >>                  prev = curr->held_locks[i];
 >>
 >
 > The latter is a type mis-match, an equivalent expression would be:
 >     &curr->held_locks[i];
 >

Yeah, sorry.

And is there reason that the statement is not
     &curr->held_locks[i];
but
     prev = curr->held_locks + i;
?

Of course, curr->held_locks + i style statement causes no problem.
But I had a little interest in the selection of style.
Do you know the reason?

Thanks,
	Hitoshi