[PATCH 0/2] tighten lglock lockdep annotations

[PATCH 0/2] tighten lglock lockdep annotations

[Michel Lespinasse]

Oleg Nesterov recently noticed that the lockdep annotations in lglock.c
are not sufficient to detect some obvious deadlocks, such as
lg_local_lock(LOCK) + lg_local_lock(LOCK) or
spin_lock(X) + lg_local_lock(Y) vs lg_local_lock(Y) + spin_lock(X).

Both issues can be fixed by indicating to lockdep that lglock's local
locks are not recursive.

Patch 1 introduces helper macros for lockdep annotations.

Patch 2 makes sure to use the appropriate helper macros to indicate that
the lglock local lock is a shared, non-recursive lock.

Michel Lespinasse (2):
  lockdep: introduce lock_acquire_exclusive/shared helper macros
  lglock: update lockdep annotations to report recursive local locks

 include/linux/lockdep.h | 92 +++++++++++++------------------------------------
 kernel/lglock.c         | 12 +++----
 2 files changed, 29 insertions(+), 75 deletions(-)

-- 
1.8.1.3

[PATCH 1/2] lockdep: introduce lock_acquire_exclusive/shared helper macros

[Michel Lespinasse]

In lockdep.h, the spinlock/mutex/rwsem/rwlock/lock_map acquire macros
have different definitions based on the value of CONFIG_PROVE_LOCKING.
We have separate ifdefs for each of these definitions, which seems
redundant.

Introduce lock_acquire_{exclusive,shared,shared_recursive} helpers
which will have different definitions based on CONFIG_PROVE_LOCKING.
Then all other helper macros can be defined based on the above ones,
which reduces the amount of ifdefined code.

Signed-off-by: Michel Lespinasse <walken@google.com>

---
 include/linux/lockdep.h | 92 +++++++++++++------------------------------------
 1 file changed, 23 insertions(+), 69 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index f1e877b79ed8..cfc2f119779a 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -365,7 +365,7 @@ extern void lockdep_trace_alloc(gfp_t mask);
 
 #define lockdep_recursing(tsk)	((tsk)->lockdep_recursion)
 
-#else /* !LOCKDEP */
+#else /* !CONFIG_LOCKDEP */
 
 static inline void lockdep_off(void)
 {
@@ -479,82 +479,36 @@ static inline void print_irqtrace_events(struct task_struct *curr)
  * 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)
+#ifdef CONFIG_PROVE_LOCKING
+ #define lock_acquire_exclusive(l, s, t, n, i)		lock_acquire(l, s, t, 0, 2, n, i)
+ #define lock_acquire_shared(l, s, t, n, i)		lock_acquire(l, s, t, 1, 2, n, i)
+ #define lock_acquire_shared_recursive(l, s, t, n, i)	lock_acquire(l, s, t, 2, 2, n, i)
 #else
-# define spin_acquire(l, s, t, i)		do { } while (0)
-# define spin_release(l, n, i)			do { } while (0)
+ #define lock_acquire_exclusive(l, s, t, n, i)		lock_acquire(l, s, t, 0, 1, n, i)
+ #define lock_acquire_shared(l, s, t, n, i)		lock_acquire(l, s, t, 1, 1, n, i)
+ #define lock_acquire_shared_recursive(l, s, t, n, i)	lock_acquire(l, s, t, 2, 1, n, i)
 #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
+#define spin_acquire(l, s, t, i)		lock_acquire_exclusive(l, s, t, NULL, i)
+#define spin_acquire_nest(l, s, t, n, i)	lock_acquire_exclusive(l, s, t, n, i)
+#define spin_release(l, n, i)			lock_release(l, n, i)
 
-#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)
-#  define mutex_acquire_nest(l, s, t, n, i)	lock_acquire(l, s, t, 0, 2, n, i)
-# else
-#  define mutex_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 1, NULL, i)
-#  define mutex_acquire_nest(l, s, t, n, i)	lock_acquire(l, s, t, 0, 1, n, 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_acquire_nest(l, s, t, n, i)	do { } while (0)
-# define mutex_release(l, n, i)			do { } while (0)
-#endif
+#define rwlock_acquire(l, s, t, i)		lock_acquire_exclusive(l, s, t, NULL, i)
+#define rwlock_acquire_read(l, s, t, i)		lock_acquire_shared_recursive(l, s, t, NULL, i)
+#define rwlock_release(l, n, i)			lock_release(l, n, i)
 
-#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_nest(l, s, t, n, i)	lock_acquire(l, s, t, 0, 2, n, 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_nest(l, s, t, n, i)	lock_acquire(l, s, t, 0, 1, n, i)
-#  define rwsem_acquire_read(l, s, t, i)	lock_acquire(l, s, t, 1, 1, NULL, i)
-# endif
+#define mutex_acquire(l, s, t, i)		lock_acquire_exclusive(l, s, t, NULL, i)
+#define mutex_acquire_nest(l, s, t, n, i)	lock_acquire_exclusive(l, s, t, n, i)
+#define mutex_release(l, n, i)			lock_release(l, n, i)
+
+#define rwsem_acquire(l, s, t, i)		lock_acquire_exclusive(l, s, t, NULL, i)
+#define rwsem_acquire_nest(l, s, t, n, i)	lock_acquire_exclusive(l, s, t, n, i)
+#define rwsem_acquire_read(l, s, t, i)		lock_acquire_shared(l, s, t, NULL, i)
 # 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_nest(l, s, t, n, 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_read(l)	lock_acquire(l, 0, 0, 2, 2, NULL, _THIS_IP_)
-# else
-#  define lock_map_acquire(l)		lock_acquire(l, 0, 0, 0, 1, NULL, _THIS_IP_)
-#  define lock_map_acquire_read(l)	lock_acquire(l, 0, 0, 2, 1, NULL, _THIS_IP_)
-# endif
+#define lock_map_acquire(l)			lock_acquire_exclusive(l, 0, 0, NULL, _THIS_IP_)
+#define lock_map_acquire_read(l)		lock_acquire_shared_recursive(l, 0, 0, NULL, _THIS_IP_)
 # define lock_map_release(l)			lock_release(l, 1, _THIS_IP_)
-#else
-# define lock_map_acquire(l)			do { } while (0)
-# define lock_map_acquire_read(l)		do { } while (0)
-# define lock_map_release(l)			do { } while (0)
-#endif
 
 #ifdef CONFIG_PROVE_LOCKING
 # define might_lock(lock) 						\
-- 
1.8.1.3

[PATCH 2/2] lglock: update lockdep annotations to report recursive local locks

[Michel Lespinasse]

Oleg Nesterov recently noticed that the lockdep annotations in lglock.c
are not sufficient to detect some obvious deadlocks, such as
lg_local_lock(LOCK) + lg_local_lock(LOCK) or
spin_lock(X) + lg_local_lock(Y) vs lg_local_lock(Y) + spin_lock(X).

Both issues are easily fixed by indicating to lockdep that lglock's local
locks are not recursive. We shouldn't use the rwlock acquire/release
functions here, as lglock doesn't share the same semantics. Instead
we can base our lockdep annotations on the lock_acquire_shared
(for local lglock) and lock_acquire_exclusive (for global lglock)
helpers.

I am not proposing new lglock specific helpers as I don't see the point
of the existing second level of helpers :)

Signed-off-by: Michel Lespinasse <walken@google.com>

---
 kernel/lglock.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/lglock.c b/kernel/lglock.c
index 6535a667a5a7..86ae2aebf004 100644
--- a/kernel/lglock.c
+++ b/kernel/lglock.c
@@ -21,7 +21,7 @@ void lg_local_lock(struct lglock *lg)
 	arch_spinlock_t *lock;
 
 	preempt_disable();
-	rwlock_acquire_read(&lg->lock_dep_map, 0, 0, _RET_IP_);
+	lock_acquire_shared(&lg->lock_dep_map, 0, 0, NULL, _RET_IP_);
 	lock = this_cpu_ptr(lg->lock);
 	arch_spin_lock(lock);
 }
@@ -31,7 +31,7 @@ void lg_local_unlock(struct lglock *lg)
 {
 	arch_spinlock_t *lock;
 
-	rwlock_release(&lg->lock_dep_map, 1, _RET_IP_);
+	lock_release(&lg->lock_dep_map, 1, _RET_IP_);
 	lock = this_cpu_ptr(lg->lock);
 	arch_spin_unlock(lock);
 	preempt_enable();
@@ -43,7 +43,7 @@ void lg_local_lock_cpu(struct lglock *lg, int cpu)
 	arch_spinlock_t *lock;
 
 	preempt_disable();
-	rwlock_acquire_read(&lg->lock_dep_map, 0, 0, _RET_IP_);
+	lock_acquire_shared(&lg->lock_dep_map, 0, 0, NULL, _RET_IP_);
 	lock = per_cpu_ptr(lg->lock, cpu);
 	arch_spin_lock(lock);
 }
@@ -53,7 +53,7 @@ void lg_local_unlock_cpu(struct lglock *lg, int cpu)
 {
 	arch_spinlock_t *lock;
 
-	rwlock_release(&lg->lock_dep_map, 1, _RET_IP_);
+	lock_release(&lg->lock_dep_map, 1, _RET_IP_);
 	lock = per_cpu_ptr(lg->lock, cpu);
 	arch_spin_unlock(lock);
 	preempt_enable();
@@ -65,7 +65,7 @@ void lg_global_lock(struct lglock *lg)
 	int i;
 
 	preempt_disable();
-	rwlock_acquire(&lg->lock_dep_map, 0, 0, _RET_IP_);
+	lock_acquire_exclusive(&lg->lock_dep_map, 0, 0, NULL, _RET_IP_);
 	for_each_possible_cpu(i) {
 		arch_spinlock_t *lock;
 		lock = per_cpu_ptr(lg->lock, i);
@@ -78,7 +78,7 @@ void lg_global_unlock(struct lglock *lg)
 {
 	int i;
 
-	rwlock_release(&lg->lock_dep_map, 1, _RET_IP_);
+	lock_release(&lg->lock_dep_map, 1, _RET_IP_);
 	for_each_possible_cpu(i) {
 		arch_spinlock_t *lock;
 		lock = per_cpu_ptr(lg->lock, i);
-- 
1.8.1.3

Re: [PATCH 1/2] lockdep: introduce lock_acquire_exclusive/shared helper macros

[Lai Jiangshan]

On 05/03/13 10:17, Michel Lespinasse wrote:
> In lockdep.h, the spinlock/mutex/rwsem/rwlock/lock_map acquire macros
> have different definitions based on the value of CONFIG_PROVE_LOCKING.
> We have separate ifdefs for each of these definitions, which seems
> redundant.
> 
> Introduce lock_acquire_{exclusive,shared,shared_recursive} helpers
> which will have different definitions based on CONFIG_PROVE_LOCKING.
> Then all other helper macros can be defined based on the above ones,
> which reduces the amount of ifdefined code.
> 
> Signed-off-by: Michel Lespinasse <walken@google.com>
> 
> ---
>  include/linux/lockdep.h | 92 +++++++++++++------------------------------------
>  1 file changed, 23 insertions(+), 69 deletions(-)
> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index f1e877b79ed8..cfc2f119779a 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -365,7 +365,7 @@ extern void lockdep_trace_alloc(gfp_t mask);
>  
>  #define lockdep_recursing(tsk)	((tsk)->lockdep_recursion)
>  
> -#else /* !LOCKDEP */
> +#else /* !CONFIG_LOCKDEP */
>  
>  static inline void lockdep_off(void)
>  {
> @@ -479,82 +479,36 @@ static inline void print_irqtrace_events(struct task_struct *curr)
>   * 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)
> +#ifdef CONFIG_PROVE_LOCKING
> + #define lock_acquire_exclusive(l, s, t, n, i)		lock_acquire(l, s, t, 0, 2, n, i)
> + #define lock_acquire_shared(l, s, t, n, i)		lock_acquire(l, s, t, 1, 2, n, i)
> + #define lock_acquire_shared_recursive(l, s, t, n, i)	lock_acquire(l, s, t, 2, 2, n, i)

Hi, Michel

I don't like the name lock_acquire_shared_recursive().
(I mean the name is wrong, ......)

In the lockdep design, lock_acquire(l, s, t, 2, 2, n, i) is used for
read-preference locks(rwlock) and all types of RCU. not for "recursive"
read-preference implies "recursive".

But the name lock_acquire_shared_recursive() don't tell us it is
read-preference.

Example if we do have a lock which is write-preference but allow read_lock recursive,
it will be still deadlock in this way, "recursive" does not help:

cpu0: spin_lock(a);				recursiveable_read_lock(b)
cpu1: recursiveable_read_lock(b); 		spin_lock(a);
cpu2:				write_lock(b);


I also noticed the lockdep annotations problem of lglock. and patch2 is good,
so for patch2: Reviewed-by: Lai Jiangshan <laijs@cn.fujitsu.com> 


Thanks,
Lai

>  #else
> -# define spin_acquire(l, s, t, i)		do { } while (0)
> -# define spin_release(l, n, i)			do { } while (0)
> + #define lock_acquire_exclusive(l, s, t, n, i)		lock_acquire(l, s, t, 0, 1, n, i)
> + #define lock_acquire_shared(l, s, t, n, i)		lock_acquire(l, s, t, 1, 1, n, i)
> + #define lock_acquire_shared_recursive(l, s, t, n, i)	lock_acquire(l, s, t, 2, 1, n, i)
>  #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
> +#define spin_acquire(l, s, t, i)		lock_acquire_exclusive(l, s, t, NULL, i)
> +#define spin_acquire_nest(l, s, t, n, i)	lock_acquire_exclusive(l, s, t, n, i)
> +#define spin_release(l, n, i)			lock_release(l, n, i)
>  
> -#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)
> -#  define mutex_acquire_nest(l, s, t, n, i)	lock_acquire(l, s, t, 0, 2, n, i)
> -# else
> -#  define mutex_acquire(l, s, t, i)		lock_acquire(l, s, t, 0, 1, NULL, i)
> -#  define mutex_acquire_nest(l, s, t, n, i)	lock_acquire(l, s, t, 0, 1, n, 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_acquire_nest(l, s, t, n, i)	do { } while (0)
> -# define mutex_release(l, n, i)			do { } while (0)
> -#endif
> +#define rwlock_acquire(l, s, t, i)		lock_acquire_exclusive(l, s, t, NULL, i)
> +#define rwlock_acquire_read(l, s, t, i)		lock_acquire_shared_recursive(l, s, t, NULL, i)
> +#define rwlock_release(l, n, i)			lock_release(l, n, i)
>  
> -#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_nest(l, s, t, n, i)	lock_acquire(l, s, t, 0, 2, n, 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_nest(l, s, t, n, i)	lock_acquire(l, s, t, 0, 1, n, i)
> -#  define rwsem_acquire_read(l, s, t, i)	lock_acquire(l, s, t, 1, 1, NULL, i)
> -# endif
> +#define mutex_acquire(l, s, t, i)		lock_acquire_exclusive(l, s, t, NULL, i)
> +#define mutex_acquire_nest(l, s, t, n, i)	lock_acquire_exclusive(l, s, t, n, i)
> +#define mutex_release(l, n, i)			lock_release(l, n, i)
> +
> +#define rwsem_acquire(l, s, t, i)		lock_acquire_exclusive(l, s, t, NULL, i)
> +#define rwsem_acquire_nest(l, s, t, n, i)	lock_acquire_exclusive(l, s, t, n, i)
> +#define rwsem_acquire_read(l, s, t, i)		lock_acquire_shared(l, s, t, NULL, i)
>  # 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_nest(l, s, t, n, 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_read(l)	lock_acquire(l, 0, 0, 2, 2, NULL, _THIS_IP_)
> -# else
> -#  define lock_map_acquire(l)		lock_acquire(l, 0, 0, 0, 1, NULL, _THIS_IP_)
> -#  define lock_map_acquire_read(l)	lock_acquire(l, 0, 0, 2, 1, NULL, _THIS_IP_)
> -# endif
> +#define lock_map_acquire(l)			lock_acquire_exclusive(l, 0, 0, NULL, _THIS_IP_)
> +#define lock_map_acquire_read(l)		lock_acquire_shared_recursive(l, 0, 0, NULL, _THIS_IP_)
>  # define lock_map_release(l)			lock_release(l, 1, _THIS_IP_)
> -#else
> -# define lock_map_acquire(l)			do { } while (0)
> -# define lock_map_acquire_read(l)		do { } while (0)
> -# define lock_map_release(l)			do { } while (0)
> -#endif
>  
>  #ifdef CONFIG_PROVE_LOCKING
>  # define might_lock(lock) 						\

Re: [PATCH 1/2] lockdep: introduce lock_acquire_exclusive/shared helper macros

[Michel Lespinasse]

On Tue, Mar 5, 2013 at 7:19 AM, Lai Jiangshan <laijs@cn.fujitsu.com> wrote:
> I don't like the name lock_acquire_shared_recursive().
> (I mean the name is wrong, ......)

Names are hard :/ Do you have a better suggestion ?

> In the lockdep design, lock_acquire(l, s, t, 2, 2, n, i) is used for
> read-preference locks(rwlock) and all types of RCU. not for "recursive"
> read-preference implies "recursive".

I based my names on the following description from lockdep.h:

 * Values for "read":
 *
 *   0: exclusive (write) acquire
 *   1: read-acquire (no recursion allowed)
 *   2: read-acquire with same-instance recursion allowed

> But the name lock_acquire_shared_recursive() don't tell us it is
> read-preference.
>
> Example if we do have a lock which is write-preference but allow read_lock recursive,
> it will be still deadlock in this way, "recursive" does not help:
>
> cpu0: spin_lock(a);                             recursiveable_read_lock(b)
> cpu1: recursiveable_read_lock(b);               spin_lock(a);
> cpu2:                           write_lock(b);

I think this is a very theorical distinction as 1- we don't have such
locks in linux currently, and 2- lockdep does not currently support
this. That is, lockdep will either report both of these scenarios as
valid or invalid, but you can't tell it to allow the first scenario
while disallowing the second:

read_lock(a); read_lock(a);
(the recursive read lock scenario)

read_lock(a); lock(b); vs lock(b); read_lock(a);
(the lock ordering vs other locks scenario)

So, I would think that since lockdep doesn't make this distinction the
helper macros don't need to either ?

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.

Re: [PATCH 1/2] lockdep: introduce lock_acquire_exclusive/shared helper macros

[Oleg Nesterov]

On 03/04, Michel Lespinasse wrote:
>
> In lockdep.h, the spinlock/mutex/rwsem/rwlock/lock_map acquire macros
> have different definitions based on the value of CONFIG_PROVE_LOCKING.
> We have separate ifdefs for each of these definitions, which seems
> redundant.

You know, I should not even try to comment this patch. I never really
understood this magic, and I forgot everything I knew.

But I like this patch because it makes lockdep.h more readable to me,
so I hope it is correct ;)

One question,

> +#ifdef CONFIG_PROVE_LOCKING
> + #define lock_acquire_exclusive(l, s, t, n, i)		lock_acquire(l, s, t, 0, 2, n, i)
> + #define lock_acquire_shared(l, s, t, n, i)		lock_acquire(l, s, t, 1, 2, n, i)
> + #define lock_acquire_shared_recursive(l, s, t, n, i)	lock_acquire(l, s, t, 2, 2, n, i)
>  #else
> -# define spin_acquire(l, s, t, i)		do { } while (0)
> -# define spin_release(l, n, i)			do { } while (0)
> + #define lock_acquire_exclusive(l, s, t, n, i)		lock_acquire(l, s, t, 0, 1, n, i)
> + #define lock_acquire_shared(l, s, t, n, i)		lock_acquire(l, s, t, 1, 1, n, i)
> + #define lock_acquire_shared_recursive(l, s, t, n, i)	lock_acquire(l, s, t, 2, 1, n, i)
>  #endif

Do we really need this ifdef? Can't we pass check == 2 unconditionally?

__lock_acquire() does:

	if (!prove_locking)
		check = 1;

anyway, and without CONFIG_PROVE_LOCKING prove_locking == 0.

And every time I need to look into lockdep.h these hardcoded constants
confuse me, and of course it is not possible to remember if this particular
"1" means "read" or "check". Imho it would be nice to add some symbolic
names. But this is offtopic.

Oleg.

Re: [PATCH 2/2] lglock: update lockdep annotations to report recursive local locks

[Oleg Nesterov]

On 03/04, Michel Lespinasse wrote:
>
> Both issues are easily fixed by indicating to lockdep that lglock's local
> locks are not recursive. We shouldn't use the rwlock acquire/release
> functions here, as lglock doesn't share the same semantics. Instead
> we can base our lockdep annotations on the lock_acquire_shared
> (for local lglock) and lock_acquire_exclusive (for global lglock)
> helpers.

IOW, with this patch lglock looks like rw_semaphore for lockdep...

Again, I can't ack this change, but afaics it is fine.

Oleg.

Re: [PATCH 2/2] lglock: update lockdep annotations to report recursive local locks

[Michel Lespinasse]

On Tue, Mar 5, 2013 at 9:42 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/04, Michel Lespinasse wrote:
>>
>> Both issues are easily fixed by indicating to lockdep that lglock's local
>> locks are not recursive. We shouldn't use the rwlock acquire/release
>> functions here, as lglock doesn't share the same semantics. Instead
>> we can base our lockdep annotations on the lock_acquire_shared
>> (for local lglock) and lock_acquire_exclusive (for global lglock)
>> helpers.
>
> IOW, with this patch lglock looks like rw_semaphore for lockdep...

Yes.

I could have actually made lglock use the rwsem helpers, but I think
that would be quite confusing...

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.