rcu: uninline rcu_lock_acquire() and rcu_lock_release()

PATCH Superseded Oleg Nesterov 2 participants 6 messages Jun 26, 2014

Subsystems (2)

RCU Sleepable read-copy update (SRCU)

lore

Share & tools

lore

Oleg Nesterov

author

2014-06-26 17:02 UTC #0 lore raw

cover rcu: uninline rcu_lock_acquire() and rcu_lock_release()

Hello,

sorry for delay with resend...

The patch is simple and the numbers look good (and probably we can do more). But when I re-diffed this change I noticed that rcu_lockdep_assert() has the warn-once semantics, and this patch obviously changes the behaviour. So please ack/nack this part too.

Oleg.

Oleg Nesterov

author

2014-06-26 17:03 UTC #1 lore raw

PATCH 1/1 rcu: uninline rcu_lock_acquire() and rcu_lock_release()

Uninline rcu_lock_acquire() and rcu_lock_release() to shrink .text/data. The difference in "size vmlinux" looks good,

with CONFIG_DEBUG_LOCK_ALLOC

- 5377829 3018320 14757888 23154037
+ 5352229 3010160 14757888 23120277

33760 bytes saved.

with CONFIG_DEBUG_LOCK_ALLOC + CONFIG_PROVE_RCU + CONFIG_TREE_RCU_TRACE

- 5682283 3022936 14757888 23463107
+ 5578667 3026776 14757888 23363331

saves 99776 bytes.

However, this obviously means that the "warn once" logic is moved from the current callers of rcu_lockdep_assert(rcu_is_watching()) to update.c.

Also, with this patch we do not bother to report which function abused rcu_is_watching(), this should be clear from dump_stack().

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/linux/rcupdate.h |   55 ++++++++++++++++++++++---------------------
 include/linux/srcu.h     |    4 +-
 kernel/rcu/rcu.h         |    6 ++--
 kernel/rcu/update.c      |   57 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 32 deletions(-)

include/linux/rcupdate.h +28 ~~-27~~

index 5a75d19..a5b1631 100644
@@ -379,18 +379,34 @@ static inline bool rcu_lockdep_current_cpu_online(void)
 }
 #endif /* #else #if defined(CONFIG_HOTPLUG_CPU) && defined(CONFIG_PROVE_RCU) */
 
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-
-static inline void rcu_lock_acquire(struct lockdep_map *map)
+static inline void __rcu_lock_acquire(struct lockdep_map *map, unsigned long ip)
 {
-	lock_acquire(map, 0, 0, 2, 0, NULL, _THIS_IP_);
+	lock_acquire(map, 0, 0, 2, 0, NULL, ip);
 }
 
-static inline void rcu_lock_release(struct lockdep_map *map)
+static inline void __rcu_lock_release(struct lockdep_map *map, unsigned long ip)
 {
-	lock_release(map, 1, _THIS_IP_);
+	lock_release(map, 1, ip);
 }
 
+#if defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PROVE_RCU)
+extern void rcu_lock_acquire(void);
+extern void rcu_lock_release(void);
+extern void rcu_lock_acquire_bh(void);
+extern void rcu_lock_release_bh(void);
+extern void rcu_lock_acquire_sched(void);
+extern void rcu_lock_release_sched(void);
+#else
+#define rcu_lock_acquire()		do { } while (0)
+#define rcu_lock_release()		do { } while (0)
+#define rcu_lock_acquire_bh()		do { } while (0)
+#define rcu_lock_release_bh()		do { } while (0)
+#define rcu_lock_acquire_sched()	do { } while (0)
+#define rcu_lock_release_sched()	do { } while (0)
+#endif
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+
 extern struct lockdep_map rcu_lock_map;
 extern struct lockdep_map rcu_bh_lock_map;
 extern struct lockdep_map rcu_sched_lock_map;

@@ -489,9 +505,6 @@ static inline int rcu_read_lock_sched_held(void)
 
 #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
-# define rcu_lock_acquire(a)		do { } while (0)
-# define rcu_lock_release(a)		do { } while (0)
-
 static inline int rcu_read_lock_held(void)
 {
 	return 1;

@@ -866,9 +879,7 @@ static inline void rcu_read_lock(void)
 {
 	__rcu_read_lock();
 	__acquire(RCU);
-	rcu_lock_acquire(&rcu_lock_map);
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock() used illegally while idle");
+	rcu_lock_acquire();
 }
 
 /*

@@ -888,9 +899,7 @@ static inline void rcu_read_lock(void)
  */
 static inline void rcu_read_unlock(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock() used illegally while idle");
-	rcu_lock_release(&rcu_lock_map);
+	rcu_lock_release();
 	__release(RCU);
 	__rcu_read_unlock();
 }

@@ -916,9 +925,7 @@ static inline void rcu_read_lock_bh(void)
 {
 	local_bh_disable();
 	__acquire(RCU_BH);
-	rcu_lock_acquire(&rcu_bh_lock_map);
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock_bh() used illegally while idle");
+	rcu_lock_acquire_bh();
 }
 
 /*

@@ -928,9 +935,7 @@ static inline void rcu_read_lock_bh(void)
  */
 static inline void rcu_read_unlock_bh(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock_bh() used illegally while idle");
-	rcu_lock_release(&rcu_bh_lock_map);
+	rcu_lock_release_bh();
 	__release(RCU_BH);
 	local_bh_enable();
 }

@@ -952,9 +957,7 @@ static inline void rcu_read_lock_sched(void)
 {
 	preempt_disable();
 	__acquire(RCU_SCHED);
-	rcu_lock_acquire(&rcu_sched_lock_map);
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock_sched() used illegally while idle");
+	rcu_lock_acquire_sched();
 }
 
 /* Used by lockdep and tracing: cannot be traced, cannot call lockdep. */

@@ -971,9 +974,7 @@ static inline notrace void rcu_read_lock_sched_notrace(void)
  */
 static inline void rcu_read_unlock_sched(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock_sched() used illegally while idle");
-	rcu_lock_release(&rcu_sched_lock_map);
+	rcu_lock_release_sched();
 	__release(RCU_SCHED);
 	preempt_enable();
 }

include/linux/srcu.h +2 -2

index a2783cb..5c06289 100644
@@ -219,7 +219,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
 {
 	int retval = __srcu_read_lock(sp);
 
-	rcu_lock_acquire(&(sp)->dep_map);
+	__rcu_lock_acquire(&(sp)->dep_map, _THIS_IP_);
 	return retval;
 }

@@ -233,7 +233,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
 static inline void srcu_read_unlock(struct srcu_struct *sp, int idx)
 	__releases(sp)
 {
-	rcu_lock_release(&(sp)->dep_map);
+	__rcu_lock_release(&(sp)->dep_map, _THIS_IP_);
 	__srcu_read_unlock(sp, idx);
 }

kernel/rcu/rcu.h +3 -3

index bfda272..5702910 100644
@@ -103,16 +103,16 @@ static inline bool __rcu_reclaim(const char *rn, struct rcu_head *head)
 {
 	unsigned long offset = (unsigned long)head->func;
 
-	rcu_lock_acquire(&rcu_callback_map);
+	__rcu_lock_acquire(&rcu_callback_map, _THIS_IP_);
 	if (__is_kfree_rcu_offset(offset)) {
 		RCU_TRACE(trace_rcu_invoke_kfree_callback(rn, head, offset));
 		kfree((void *)head - offset);
-		rcu_lock_release(&rcu_callback_map);
+		__rcu_lock_release(&rcu_callback_map, _THIS_IP_);
 		return 1;
 	} else {
 		RCU_TRACE(trace_rcu_invoke_callback(rn, head));
 		head->func(head);
-		rcu_lock_release(&rcu_callback_map);
+		__rcu_lock_release(&rcu_callback_map, _THIS_IP_);
 		return 0;
 	}
 }

kernel/rcu/update.c +57 -0

63 lines not on this page · raw message

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2014-06-26 17:35 UTC #2 lore raw

PATCH v2 1/1 rcu: uninline rcu_lock_acquire() and rcu_lock_release()

Review

Quoted 5 lines · Oleg Nesterov

(quoted diff) +4 -0

+static void rcu_release_map(struct lockdep_map *map, unsigned long ip)
+{
+	rcu_lockdep_assert_watching();
+	__rcu_lock_release(&rcu_lock_map, ip);

                            ^^^^^^^^^^^^

OOPS. This should be "map". Please see v2 below.

-------------------------------------------------------------------------------
Subject: [PATCH v2 1/1] rcu: uninline rcu_lock_acquire() and rcu_lock_release()

Uninline rcu_lock_acquire() and rcu_lock_release() to shrink .text/data. The difference in "size vmlinux" looks good,

with CONFIG_DEBUG_LOCK_ALLOC

- 5377829 3018320 14757888 23154037
+ 5352229 3010160 14757888 23120277

33760 bytes saved.

with CONFIG_DEBUG_LOCK_ALLOC + CONFIG_PROVE_RCU + CONFIG_TREE_RCU_TRACE

- 5682283 3022936 14757888 23463107
+ 5578667 3026776 14757888 23363331

saves 99776 bytes.

However, this obviously means that the "warn once" logic is moved from the current callers of rcu_lockdep_assert(rcu_is_watching()) to update.c.

Also, with this patch we do not bother to report which function abused rcu_is_watching(), this should be clear from dump_stack().

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/linux/rcupdate.h |   55 ++++++++++++++++++++++---------------------
 include/linux/srcu.h     |    4 +-
 kernel/rcu/rcu.h         |    6 ++--
 kernel/rcu/update.c      |   57 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 32 deletions(-)

include/linux/rcupdate.h +28 ~~-27~~

index 5a75d19..a5b1631 100644
@@ -379,18 +379,34 @@ static inline bool rcu_lockdep_current_cpu_online(void)
 }
 #endif /* #else #if defined(CONFIG_HOTPLUG_CPU) && defined(CONFIG_PROVE_RCU) */
 
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-
-static inline void rcu_lock_acquire(struct lockdep_map *map)
+static inline void __rcu_lock_acquire(struct lockdep_map *map, unsigned long ip)
 {
-	lock_acquire(map, 0, 0, 2, 0, NULL, _THIS_IP_);
+	lock_acquire(map, 0, 0, 2, 0, NULL, ip);
 }
 
-static inline void rcu_lock_release(struct lockdep_map *map)
+static inline void __rcu_lock_release(struct lockdep_map *map, unsigned long ip)
 {
-	lock_release(map, 1, _THIS_IP_);
+	lock_release(map, 1, ip);
 }
 
+#if defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PROVE_RCU)
+extern void rcu_lock_acquire(void);
+extern void rcu_lock_release(void);
+extern void rcu_lock_acquire_bh(void);
+extern void rcu_lock_release_bh(void);
+extern void rcu_lock_acquire_sched(void);
+extern void rcu_lock_release_sched(void);
+#else
+#define rcu_lock_acquire()		do { } while (0)
+#define rcu_lock_release()		do { } while (0)
+#define rcu_lock_acquire_bh()		do { } while (0)
+#define rcu_lock_release_bh()		do { } while (0)
+#define rcu_lock_acquire_sched()	do { } while (0)
+#define rcu_lock_release_sched()	do { } while (0)
+#endif
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+
 extern struct lockdep_map rcu_lock_map;
 extern struct lockdep_map rcu_bh_lock_map;
 extern struct lockdep_map rcu_sched_lock_map;

@@ -489,9 +505,6 @@ static inline int rcu_read_lock_sched_held(void)
 
 #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
-# define rcu_lock_acquire(a)		do { } while (0)
-# define rcu_lock_release(a)		do { } while (0)
-
 static inline int rcu_read_lock_held(void)
 {
 	return 1;

@@ -866,9 +879,7 @@ static inline void rcu_read_lock(void)
 {
 	__rcu_read_lock();
 	__acquire(RCU);
-	rcu_lock_acquire(&rcu_lock_map);
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock() used illegally while idle");
+	rcu_lock_acquire();
 }
 
 /*

@@ -888,9 +899,7 @@ static inline void rcu_read_lock(void)
  */
 static inline void rcu_read_unlock(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock() used illegally while idle");
-	rcu_lock_release(&rcu_lock_map);
+	rcu_lock_release();
 	__release(RCU);
 	__rcu_read_unlock();
 }

@@ -916,9 +925,7 @@ static inline void rcu_read_lock_bh(void)
 {
 	local_bh_disable();
 	__acquire(RCU_BH);
-	rcu_lock_acquire(&rcu_bh_lock_map);
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock_bh() used illegally while idle");
+	rcu_lock_acquire_bh();
 }
 
 /*

@@ -928,9 +935,7 @@ static inline void rcu_read_lock_bh(void)
  */
 static inline void rcu_read_unlock_bh(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock_bh() used illegally while idle");
-	rcu_lock_release(&rcu_bh_lock_map);
+	rcu_lock_release_bh();
 	__release(RCU_BH);
 	local_bh_enable();
 }

@@ -952,9 +957,7 @@ static inline void rcu_read_lock_sched(void)
 {
 	preempt_disable();
 	__acquire(RCU_SCHED);
-	rcu_lock_acquire(&rcu_sched_lock_map);
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock_sched() used illegally while idle");
+	rcu_lock_acquire_sched();
 }
 
 /* Used by lockdep and tracing: cannot be traced, cannot call lockdep. */

@@ -971,9 +974,7 @@ static inline notrace void rcu_read_lock_sched_notrace(void)
  */
 static inline void rcu_read_unlock_sched(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock_sched() used illegally while idle");
-	rcu_lock_release(&rcu_sched_lock_map);
+	rcu_lock_release_sched();
 	__release(RCU_SCHED);
 	preempt_enable();
 }

include/linux/srcu.h +2 -2

index a2783cb..5c06289 100644
@@ -219,7 +219,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
 {
 	int retval = __srcu_read_lock(sp);
 
-	rcu_lock_acquire(&(sp)->dep_map);
+	__rcu_lock_acquire(&(sp)->dep_map, _THIS_IP_);
 	return retval;
 }

@@ -233,7 +233,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
 static inline void srcu_read_unlock(struct srcu_struct *sp, int idx)
 	__releases(sp)
 {
-	rcu_lock_release(&(sp)->dep_map);
+	__rcu_lock_release(&(sp)->dep_map, _THIS_IP_);
 	__srcu_read_unlock(sp, idx);
 }

kernel/rcu/rcu.h +3 -3

index bfda272..5702910 100644
@@ -103,16 +103,16 @@ static inline bool __rcu_reclaim(const char *rn, struct rcu_head *head)
 {
 	unsigned long offset = (unsigned long)head->func;
 
-	rcu_lock_acquire(&rcu_callback_map);
+	__rcu_lock_acquire(&rcu_callback_map, _THIS_IP_);
 	if (__is_kfree_rcu_offset(offset)) {
 		RCU_TRACE(trace_rcu_invoke_kfree_callback(rn, head, offset));
 		kfree((void *)head - offset);
-		rcu_lock_release(&rcu_callback_map);
+		__rcu_lock_release(&rcu_callback_map, _THIS_IP_);
 		return 1;
 	} else {
 		RCU_TRACE(trace_rcu_invoke_callback(rn, head));
 		head->func(head);
-		rcu_lock_release(&rcu_callback_map);
+		__rcu_lock_release(&rcu_callback_map, _THIS_IP_);
 		return 0;
 	}
 }

kernel/rcu/update.c +57 -0

63 lines not on this page · raw message

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2014-06-26 18:38 UTC #3 lore raw

PATCH v3 1/1 rcu: uninline rcu_lock_acquire() and rcu_lock_release()

Quoted 9 lines · Oleg Nesterov

Review

Quoted 5 lines · Oleg Nesterov

(quoted diff) +4 -0

+static void rcu_release_map(struct lockdep_map *map, unsigned long ip)
+{
+	rcu_lockdep_assert_watching();
+	__rcu_lock_release(&rcu_lock_map, ip);

                            ^^^^^^^^^^^^

OOPS. This should be "map". Please see v2 below.

Argh. Probably you should simply ignore me.

Review

Quoted 4 lines

(quoted diff) +4 -0

+static void rcu_release_map(struct lockdep_map *map, unsigned long ip)
+{
+	rcu_lockdep_assert_watching();
+	__rcu_lock_release(&map, ip);

"map", not "&map". I fixed this before I sent v2, but apparently forgot to -add before --amend.

Sorry for noise.

-------------------------------------------------------------------------------
Subject: [PATCH v3 1/1] rcu: uninline rcu_lock_acquire() and rcu_lock_release()

Uninline rcu_lock_acquire() and rcu_lock_release() to shrink .text/data. The difference in "size vmlinux" looks good,

with CONFIG_DEBUG_LOCK_ALLOC

- 5377829 3018320 14757888 23154037
+ 5352229 3010160 14757888 23120277

33760 bytes saved.

with CONFIG_DEBUG_LOCK_ALLOC + CONFIG_PROVE_RCU + CONFIG_TREE_RCU_TRACE

- 5682283 3022936 14757888 23463107
+ 5578667 3026776 14757888 23363331

saves 99776 bytes.

However, this obviously means that the "warn once" logic is moved from the current callers of rcu_lockdep_assert(rcu_is_watching()) to update.c.

Also, with this patch we do not bother to report which function abused rcu_is_watching(), this should be clear from dump_stack().

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/linux/rcupdate.h |   55 ++++++++++++++++++++++---------------------
 include/linux/srcu.h     |    4 +-
 kernel/rcu/rcu.h         |    6 ++--
 kernel/rcu/update.c      |   57 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 32 deletions(-)

include/linux/rcupdate.h +28 ~~-27~~

index 5a75d19..a5b1631 100644
@@ -379,18 +379,34 @@ static inline bool rcu_lockdep_current_cpu_online(void)
 }
 #endif /* #else #if defined(CONFIG_HOTPLUG_CPU) && defined(CONFIG_PROVE_RCU) */
 
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-
-static inline void rcu_lock_acquire(struct lockdep_map *map)
+static inline void __rcu_lock_acquire(struct lockdep_map *map, unsigned long ip)
 {
-	lock_acquire(map, 0, 0, 2, 0, NULL, _THIS_IP_);
+	lock_acquire(map, 0, 0, 2, 0, NULL, ip);
 }
 
-static inline void rcu_lock_release(struct lockdep_map *map)
+static inline void __rcu_lock_release(struct lockdep_map *map, unsigned long ip)
 {
-	lock_release(map, 1, _THIS_IP_);
+	lock_release(map, 1, ip);
 }
 
+#if defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PROVE_RCU)
+extern void rcu_lock_acquire(void);
+extern void rcu_lock_release(void);
+extern void rcu_lock_acquire_bh(void);
+extern void rcu_lock_release_bh(void);
+extern void rcu_lock_acquire_sched(void);
+extern void rcu_lock_release_sched(void);
+#else
+#define rcu_lock_acquire()		do { } while (0)
+#define rcu_lock_release()		do { } while (0)
+#define rcu_lock_acquire_bh()		do { } while (0)
+#define rcu_lock_release_bh()		do { } while (0)
+#define rcu_lock_acquire_sched()	do { } while (0)
+#define rcu_lock_release_sched()	do { } while (0)
+#endif
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+
 extern struct lockdep_map rcu_lock_map;
 extern struct lockdep_map rcu_bh_lock_map;
 extern struct lockdep_map rcu_sched_lock_map;

@@ -489,9 +505,6 @@ static inline int rcu_read_lock_sched_held(void)
 
 #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
-# define rcu_lock_acquire(a)		do { } while (0)
-# define rcu_lock_release(a)		do { } while (0)
-
 static inline int rcu_read_lock_held(void)
 {
 	return 1;

@@ -866,9 +879,7 @@ static inline void rcu_read_lock(void)
 {
 	__rcu_read_lock();
 	__acquire(RCU);
-	rcu_lock_acquire(&rcu_lock_map);
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock() used illegally while idle");
+	rcu_lock_acquire();
 }
 
 /*

@@ -888,9 +899,7 @@ static inline void rcu_read_lock(void)
  */
 static inline void rcu_read_unlock(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock() used illegally while idle");
-	rcu_lock_release(&rcu_lock_map);
+	rcu_lock_release();
 	__release(RCU);
 	__rcu_read_unlock();
 }

@@ -916,9 +925,7 @@ static inline void rcu_read_lock_bh(void)
 {
 	local_bh_disable();
 	__acquire(RCU_BH);
-	rcu_lock_acquire(&rcu_bh_lock_map);
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock_bh() used illegally while idle");
+	rcu_lock_acquire_bh();
 }
 
 /*

@@ -928,9 +935,7 @@ static inline void rcu_read_lock_bh(void)
  */
 static inline void rcu_read_unlock_bh(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock_bh() used illegally while idle");
-	rcu_lock_release(&rcu_bh_lock_map);
+	rcu_lock_release_bh();
 	__release(RCU_BH);
 	local_bh_enable();
 }

@@ -952,9 +957,7 @@ static inline void rcu_read_lock_sched(void)
 {
 	preempt_disable();
 	__acquire(RCU_SCHED);
-	rcu_lock_acquire(&rcu_sched_lock_map);
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock_sched() used illegally while idle");
+	rcu_lock_acquire_sched();
 }
 
 /* Used by lockdep and tracing: cannot be traced, cannot call lockdep. */

@@ -971,9 +974,7 @@ static inline notrace void rcu_read_lock_sched_notrace(void)
  */
 static inline void rcu_read_unlock_sched(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock_sched() used illegally while idle");
-	rcu_lock_release(&rcu_sched_lock_map);
+	rcu_lock_release_sched();
 	__release(RCU_SCHED);
 	preempt_enable();
 }

include/linux/srcu.h +2 -2

index a2783cb..5c06289 100644
@@ -219,7 +219,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
 {
 	int retval = __srcu_read_lock(sp);
 
-	rcu_lock_acquire(&(sp)->dep_map);
+	__rcu_lock_acquire(&(sp)->dep_map, _THIS_IP_);
 	return retval;
 }

@@ -233,7 +233,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
 static inline void srcu_read_unlock(struct srcu_struct *sp, int idx)
 	__releases(sp)
 {
-	rcu_lock_release(&(sp)->dep_map);
+	__rcu_lock_release(&(sp)->dep_map, _THIS_IP_);
 	__srcu_read_unlock(sp, idx);
 }

kernel/rcu/rcu.h +3 -3

index bfda272..5702910 100644
@@ -103,16 +103,16 @@ static inline bool __rcu_reclaim(const char *rn, struct rcu_head *head)
 {
 	unsigned long offset = (unsigned long)head->func;
 
-	rcu_lock_acquire(&rcu_callback_map);
+	__rcu_lock_acquire(&rcu_callback_map, _THIS_IP_);
 	if (__is_kfree_rcu_offset(offset)) {
 		RCU_TRACE(trace_rcu_invoke_kfree_callback(rn, head, offset));
 		kfree((void *)head - offset);
-		rcu_lock_release(&rcu_callback_map);
+		__rcu_lock_release(&rcu_callback_map, _THIS_IP_);
 		return 1;
 	} else {
 		RCU_TRACE(trace_rcu_invoke_callback(rn, head));
 		head->func(head);
-		rcu_lock_release(&rcu_callback_map);
+		__rcu_lock_release(&rcu_callback_map, _THIS_IP_);
 		return 0;
 	}
 }

kernel/rcu/update.c +57 -0

63 lines not on this page · raw message

Signature · 1 line

1.5.5.1

Paul E. McKenney

2014-06-26 19:21 UTC #4 lore raw

Quoted 4 lines · Oleg Nesterov

Quoted 3 lines · Oleg Nesterov
Quoted 1 line · Oleg Nesterov

Quoted 8 lines

Quoted 6 lines
kernel/rcu/update.c EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
Quoted 4 lines
kernel/rcu/update.c +4 -0
+static void rcu_release_map(struct lockdep_map *map, unsigned long ip)
+{
+	rcu_lockdep_assert_watching();
+	__rcu_lock_release(&rcu_lock_map, ip);
                            ^^^^^^^^^^^^
OOPS. This should be "map". Please see v2 below.
Argh. Probably you should simply ignore me.

My day has been a bit like that as well. I can only be thankful for source-code control systems such as git...

Quoted 9 lines

kernel/rcu/update.c EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
Quoted 4 lines
kernel/rcu/update.c +4 -0
+static void rcu_release_map(struct lockdep_map *map, unsigned long ip)
+{
+	rcu_lockdep_assert_watching();
+	__rcu_lock_release(&map, ip);
"map", not "&map". I fixed this before I sent v2, but apparently forgot to -add before --amend.
Sorry for noise.

Not a problem! Looks generally sane, but with a bit of adjustment still needed.

I got some test failures on v2:

o Build breakage if built with CONFIG_DEBUG_LOCK_ALLOC=n. I believe
that the best way to fix this is to #ifdef out the bodies of __rcu_lock_acquire() and __rcu_lock_release(), but maybe you have something else in mind.

o Lockdep splat as follows, which might well be due to the

	"&map" that you noted above:

	[    0.000000] =====================================
	[    0.000000] [ BUG: bad unlock balance detected! ]
	[    0.000000] 3.16.0-rc1+ #1 Not tainted
	[    0.000000] -------------------------------------
	[    0.000000] swapper/0 is trying to release lock (X?à<81>ÿÿÿÿ<97>^Sò<81>ÿÿÿÿX?à<81>ÿÿÿÿ{±ò<81>@B^O) at:
	[    0.000000] [<ffffffff8107cb61>] init_idle+0xc1/0x150
	[    0.000000] but there are no more locks to release!

And a few other things noted below.

Thanx, Paul

Quoted 11 lines

-------------------------------------------------------------------------------
Subject: [PATCH v3 1/1] rcu: uninline rcu_lock_acquire() and rcu_lock_release()
Uninline rcu_lock_acquire() and rcu_lock_release() to shrink .text/data. The difference in "size vmlinux" looks good,
with CONFIG_DEBUG_LOCK_ALLOC
- 5377829 3018320 14757888 23154037
+ 5352229 3010160 14757888 23120277

Quoted 8 lines

33760 bytes saved.
with CONFIG_DEBUG_LOCK_ALLOC + CONFIG_PROVE_RCU + CONFIG_TREE_RCU_TRACE
- 5682283 3022936 14757888 23463107
+ 5578667 3026776 14757888 23363331
saves 99776 bytes.

Nice savings!

Quoted 2 lines

However, this obviously means that the "warn once" logic is moved from the current callers of rcu_lockdep_assert(rcu_is_watching()) to update.c.

Which should be fine.

Quoted 2 lines

Also, with this patch we do not bother to report which function abused rcu_is_watching(), this should be clear from dump_stack().

Which also should be fine. I was going to suggest a macro wrapper, but that would of course bloat the kernel with the function names...

Quoted 13 lines

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/linux/rcupdate.h |   55 ++++++++++++++++++++++---------------------
 include/linux/srcu.h     |    4 +-
 kernel/rcu/rcu.h         |    6 ++--
 kernel/rcu/update.c      |   57 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 32 deletions(-)

include/linux/rcupdate.h +0 -0

index 5a75d19..a5b1631 100644

include/linux/rcupdate.h static inline bool rcu_lockdep_current_cpu_online(void) · line 384

Quoted 10 lines

include/linux/rcupdate.h +2 -4

@@ -379,18 +379,34 @@ static inline bool rcu_lockdep_current_cpu_online(void)
 }
 #endif /* #else #if defined(CONFIG_HOTPLUG_CPU) && defined(CONFIG_PROVE_RCU) */
 
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-
-static inline void rcu_lock_acquire(struct lockdep_map *map)
+static inline void __rcu_lock_acquire(struct lockdep_map *map, unsigned long ip)
 {
-	lock_acquire(map, 0, 0, 2, 0, NULL, _THIS_IP_);
+	lock_acquire(map, 0, 0, 2, 0, NULL, ip);

I believe that the above line of code needs to be under #ifdef CONFIG_DEBUG_LOCK_ALLOC, otherwise we get build failures due to accesses to #ifdefed-out structure members and variables.

include/linux/rcupdate.h static inline bool rcu_lockdep_current_cpu_online(void) · line 389

Quoted 7 lines

include/linux/rcupdate.h +2 -2

 }
 
-static inline void rcu_lock_release(struct lockdep_map *map)
+static inline void __rcu_lock_release(struct lockdep_map *map, unsigned long ip)
 {
-	lock_release(map, 1, _THIS_IP_);
+	lock_release(map, 1, ip);

Ditto.

include/linux/rcupdate.h static inline bool rcu_lockdep_current_cpu_online(void) · line 392

Quoted 3 lines

include/linux/rcupdate.h +1 -0

 }
 
+#if defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PROVE_RCU)

"#ifdef CONFIG_DEBUG_LOCK_ALLOC" should suffice here. If CONFIG_PROVE_RCU is set, then CONFIG_DEBUG_LOCK_ALLOC is guaranteed to also be set.

Quoted 124 lines

include/linux/rcupdate.h +23 ~~-21~~

+extern void rcu_lock_acquire(void);
+extern void rcu_lock_release(void);
+extern void rcu_lock_acquire_bh(void);
+extern void rcu_lock_release_bh(void);
+extern void rcu_lock_acquire_sched(void);
+extern void rcu_lock_release_sched(void);
+#else
+#define rcu_lock_acquire()		do { } while (0)
+#define rcu_lock_release()		do { } while (0)
+#define rcu_lock_acquire_bh()		do { } while (0)
+#define rcu_lock_release_bh()		do { } while (0)
+#define rcu_lock_acquire_sched()	do { } while (0)
+#define rcu_lock_release_sched()	do { } while (0)
+#endif
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+
 extern struct lockdep_map rcu_lock_map;
 extern struct lockdep_map rcu_bh_lock_map;
 extern struct lockdep_map rcu_sched_lock_map;

@@ -489,9 +505,6 @@ static inline int rcu_read_lock_sched_held(void)
 
 #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
-# define rcu_lock_acquire(a)		do { } while (0)
-# define rcu_lock_release(a)		do { } while (0)
-
 static inline int rcu_read_lock_held(void)
 {
 	return 1;

@@ -866,9 +879,7 @@ static inline void rcu_read_lock(void)
 {
 	__rcu_read_lock();
 	__acquire(RCU);
-	rcu_lock_acquire(&rcu_lock_map);
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock() used illegally while idle");
+	rcu_lock_acquire();
 }
 
 /*

@@ -888,9 +899,7 @@ static inline void rcu_read_lock(void)
  */
 static inline void rcu_read_unlock(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock() used illegally while idle");
-	rcu_lock_release(&rcu_lock_map);
+	rcu_lock_release();
 	__release(RCU);
 	__rcu_read_unlock();
 }

@@ -916,9 +925,7 @@ static inline void rcu_read_lock_bh(void)
 {
 	local_bh_disable();
 	__acquire(RCU_BH);
-	rcu_lock_acquire(&rcu_bh_lock_map);
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock_bh() used illegally while idle");
+	rcu_lock_acquire_bh();
 }
 
 /*

@@ -928,9 +935,7 @@ static inline void rcu_read_lock_bh(void)
  */
 static inline void rcu_read_unlock_bh(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock_bh() used illegally while idle");
-	rcu_lock_release(&rcu_bh_lock_map);
+	rcu_lock_release_bh();
 	__release(RCU_BH);
 	local_bh_enable();
 }

@@ -952,9 +957,7 @@ static inline void rcu_read_lock_sched(void)
 {
 	preempt_disable();
 	__acquire(RCU_SCHED);
-	rcu_lock_acquire(&rcu_sched_lock_map);
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock_sched() used illegally while idle");
+	rcu_lock_acquire_sched();
 }
 
 /* Used by lockdep and tracing: cannot be traced, cannot call lockdep. */

@@ -971,9 +974,7 @@ static inline notrace void rcu_read_lock_sched_notrace(void)
  */
 static inline void rcu_read_unlock_sched(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock_sched() used illegally while idle");
-	rcu_lock_release(&rcu_sched_lock_map);
+	rcu_lock_release_sched();
 	__release(RCU_SCHED);
 	preempt_enable();
 }

include/linux/srcu.h +2 -2

index a2783cb..5c06289 100644
@@ -219,7 +219,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
 {
 	int retval = __srcu_read_lock(sp);
 
-	rcu_lock_acquire(&(sp)->dep_map);
+	__rcu_lock_acquire(&(sp)->dep_map, _THIS_IP_);
 	return retval;
 }

@@ -233,7 +233,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
 static inline void srcu_read_unlock(struct srcu_struct *sp, int idx)
 	__releases(sp)
 {
-	rcu_lock_release(&(sp)->dep_map);
+	__rcu_lock_release(&(sp)->dep_map, _THIS_IP_);
 	__srcu_read_unlock(sp, idx);
 }

kernel/rcu/rcu.h +0 -0

index bfda272..5702910 100644
@@ -103,16 +103,16 @@ static inline bool __rcu_reclaim(const char *rn, struct rcu_head *head)
 {

kernel/rcu/rcu.h static inline bool __rcu_reclaim(const char *rn, struct rcu_head *head) · line 111

Quoted 10 lines

kernel/rcu/rcu.h +2 -2

 	unsigned long offset = (unsigned long)head->func;
 
-	rcu_lock_acquire(&rcu_callback_map);
+	__rcu_lock_acquire(&rcu_callback_map, _THIS_IP_);
 	if (__is_kfree_rcu_offset(offset)) {
 		RCU_TRACE(trace_rcu_invoke_kfree_callback(rn, head, offset));
 		kfree((void *)head - offset);
-		rcu_lock_release(&rcu_callback_map);
+		__rcu_lock_release(&rcu_callback_map, _THIS_IP_);
 		return 1;

Commit 08c11a77e8 in my -rcu tree chose a particularly bad time to change this to "true" and the "0" below to "false". Could you please rebase on top of that commit? (Easy enough for me to hand-edit the patch, but the way today is going, I will probably mess it up.)

Quoted 80 lines

kernel/rcu/rcu.h +1 -1

 	} else {
 		RCU_TRACE(trace_rcu_invoke_callback(rn, head));
 		head->func(head);
-		rcu_lock_release(&rcu_callback_map);
+		__rcu_lock_release(&rcu_callback_map, _THIS_IP_);
 		return 0;
 	}
 }

kernel/rcu/update.c +57 -0

index a2aeb4d..9cbdf52 100644
@@ -168,6 +168,63 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
 
 #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
+#if defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PROVE_RCU)
+
+static void rcu_lockdep_assert_watching(void)
+{
+	rcu_lockdep_assert(rcu_is_watching(), "RCU used illegally while idle");
+}
+
+static void rcu_acquire_map(struct lockdep_map *map, unsigned long ip)
+{
+	__rcu_lock_acquire(map, ip);
+	rcu_lockdep_assert_watching();
+}
+
+static void rcu_release_map(struct lockdep_map *map, unsigned long ip)
+{
+	rcu_lockdep_assert_watching();
+	__rcu_lock_release(map, ip);
+}
+
+void rcu_lock_acquire(void)
+{
+	rcu_acquire_map(&rcu_lock_map, _RET_IP_);
+}
+EXPORT_SYMBOL(rcu_lock_acquire);
+
+void rcu_lock_release(void)
+{
+	rcu_release_map(&rcu_lock_map, _RET_IP_);
+}
+EXPORT_SYMBOL(rcu_lock_release);
+
+void rcu_lock_acquire_bh(void)
+{
+	rcu_acquire_map(&rcu_bh_lock_map, _RET_IP_);
+}
+EXPORT_SYMBOL(rcu_lock_acquire_bh);
+
+void rcu_lock_release_bh(void)
+{
+	rcu_release_map(&rcu_bh_lock_map, _RET_IP_);
+}
+EXPORT_SYMBOL(rcu_lock_release_bh);
+
+void rcu_lock_acquire_sched(void)
+{
+	rcu_acquire_map(&rcu_sched_lock_map, _RET_IP_);
+}
+EXPORT_SYMBOL(rcu_lock_acquire_sched);
+
+void rcu_lock_release_sched(void)
+{
+	rcu_release_map(&rcu_sched_lock_map, _RET_IP_);
+}
+EXPORT_SYMBOL(rcu_lock_release_sched);
+
+#endif
+
 struct rcu_synchronize {
 	struct rcu_head head;
 	struct completion completion;

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2014-06-26 19:45 UTC #5 lore raw

Quoted 16 lines · Paul E. McKenney

Quoted 9 lines · Oleg Nesterov
kernel/rcu/update.c EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
Quoted 4 lines
kernel/rcu/update.c +4 -0
+static void rcu_release_map(struct lockdep_map *map, unsigned long ip)
+{
+	rcu_lockdep_assert_watching();
+	__rcu_lock_release(&map, ip);
"map", not "&map". I fixed this before I sent v2, but apparently forgot to -add before --amend.
Sorry for noise.
Not a problem! Looks generally sane, but with a bit of adjustment still needed.

Quoted 6 lines

I got some test failures on v2:
o Build breakage if built with CONFIG_DEBUG_LOCK_ALLOC=n. I believe
that the best way to fix this is to #ifdef out the bodies of __rcu_lock_acquire() and __rcu_lock_release(), but maybe you have something else in mind.

Damn ;) Will fix and send v4. Thanks.

Quoted 2 lines

o Lockdep splat as follows, which might well be due to the
"&map" that you noted above:

Yes, this should be hopefully fixed. Note that

Quoted 4 lines

	[    0.000000] [ BUG: bad unlock balance detected! ]
	[    0.000000] 3.16.0-rc1+ #1 Not tainted
	[    0.000000] -------------------------------------
	[    0.000000] swapper/0 is trying to release lock (X?à<81>ÿÿÿÿ<97>^Sò<81>ÿÿÿÿX?à<81>ÿÿÿÿ{±ò<81>@B^O) at:

prints garbage.

Quoted 1 line

And a few other things noted below.

Yes, will, do.

Thanks!

Oleg.