Skip to contentSkip to conversation

rcu: uninline rcu_lock_acquire() and rcu_lock_release()

PATCH In review Oleg Nesterov 3 participants 12 messages Jun 30, 2014 – Jul 8, 2014

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v4 cover rcu: uninline rcu_lock_acquire() and rcu_lock_release()

Hello,

May be correct this time ;) Based on paulmck/linux-rcu.git rcu/next.

2/2 is new and hopefully trivial. But! the numbers look suspiciously good, I do not understand where does the difference come from...

Oleg.

 include/linux/rcupdate.h |   66 +++++++++++++++++++++++----------------------
 include/linux/srcu.h     |    6 ++--
 kernel/rcu/rcu.h         |    6 ++--
 kernel/rcu/update.c      |   53 +++++++++++++++++++++++++++++++++++++
 4 files changed, 93 insertions(+), 38 deletions(-)
#1 lore raw

PATCH v4 1/2 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

- 5678315 3027032 14757888 23463235
+ 5578795 3026776 14757888 23363459

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().

---
 include/linux/rcupdate.h |   60 +++++++++++++++++++++++----------------------
 include/linux/srcu.h     |    4 +-
 kernel/rcu/rcu.h         |    6 ++--
 kernel/rcu/update.c      |   53 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 89 insertions(+), 34 deletions(-)
include/linux/rcupdate.h +31 -29
index d231aa1..b82d1d6 100644
@@ -355,21 +355,28 @@ static inline bool rcu_lockdep_current_cpu_online(void)
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 
-static inline void rcu_lock_acquire(struct lockdep_map *map)
+extern struct lockdep_map rcu_lock_map;
+extern struct lockdep_map rcu_bh_lock_map;
+extern struct lockdep_map rcu_sched_lock_map;
+extern struct lockdep_map rcu_callback_map;
+int debug_lockdep_rcu_enabled(void);
+
+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);
 }
 
-extern struct lockdep_map rcu_lock_map;
-extern struct lockdep_map rcu_bh_lock_map;
-extern struct lockdep_map rcu_sched_lock_map;
-extern struct lockdep_map rcu_callback_map;
-int debug_lockdep_rcu_enabled(void);
+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);
 
 /**
  * rcu_read_lock_held() - might we be in RCU read-side critical section?
@@ -463,8 +470,15 @@ 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)
+#define __rcu_lock_acquire(map, ip)	do { } while (0)
+#define __rcu_lock_release(map, ip)	do { } while (0)
+
+#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)
 
 static inline int rcu_read_lock_held(void)
 {
@@ -839,9 +853,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();
 }
 
 /*
@@ -889,9 +901,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();
 }
@@ -917,9 +927,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();
 }
 
 /*
@@ -929,9 +937,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();
 }
@@ -953,9 +959,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. */
@@ -972,9 +976,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 ff1a6de..4782d2f 100644
@@ -107,16 +107,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 true;
 	} 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 false;
 	}
 }
kernel/rcu/update.c +53 -0

59 lines not on this page · raw message

Signature · 1 line
1.5.5.1
#5 lore raw

PATCH v4 2/2 rcu: change rcu_dereference_check(c) to check "c" first

In reply to Oleg Nesterov

Change the order of "c" and "_held" checks in rcu_dereference_check(), this can help if __builtin_constant_p(c).

CONFIG_DEBUG_LOCK_ALLOC + CONFIG_PROVE_RCU + CONFIG_TREE_RCU_TRACE

- 5578795 3026776 14757888 23363459
+ 5542443 3014040 14757888 23314371

saves 49088 bytes according to size vmlinux.

---
 include/linux/rcupdate.h |    6 +++---
 include/linux/srcu.h     |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)
include/linux/rcupdate.h +3 -3
index b82d1d6..e8c55d8 100644
@@ -694,7 +694,7 @@ static inline void rcu_preempt_sleep_check(void)
  * annotated as __rcu.
  */
 #define rcu_dereference_check(p, c) \
-	__rcu_dereference_check((p), rcu_read_lock_held() || (c), __rcu)
+	__rcu_dereference_check((p), (c) || rcu_read_lock_held(), __rcu)
 
 /**
  * rcu_dereference_bh_check() - rcu_dereference_bh with debug checking
@@ -704,7 +704,7 @@ static inline void rcu_preempt_sleep_check(void)
  * This is the RCU-bh counterpart to rcu_dereference_check().
  */
 #define rcu_dereference_bh_check(p, c) \
-	__rcu_dereference_check((p), rcu_read_lock_bh_held() || (c), __rcu)
+	__rcu_dereference_check((p), (c) || rcu_read_lock_bh_held(), __rcu)
 
 /**
  * rcu_dereference_sched_check() - rcu_dereference_sched with debug checking
@@ -714,7 +714,7 @@ static inline void rcu_preempt_sleep_check(void)
  * This is the RCU-sched counterpart to rcu_dereference_check().
  */
 #define rcu_dereference_sched_check(p, c) \
-	__rcu_dereference_check((p), rcu_read_lock_sched_held() || (c), \
+	__rcu_dereference_check((p), (c) || rcu_read_lock_sched_held(), \
 				__rcu)
 
 #define rcu_dereference_raw(p) rcu_dereference_check(p, 1) /*@@@ needed? @@@*/
include/linux/srcu.h +1 -1
index 5c06289..eae58d4 100644
@@ -184,7 +184,7 @@ static inline int srcu_read_lock_held(struct srcu_struct *sp)
  * lockdep_is_held() calls.
  */
 #define srcu_dereference_check(p, sp, c) \
-	__rcu_dereference_check((p), srcu_read_lock_held(sp) || (c), __rcu)
+	__rcu_dereference_check((p), (c) || srcu_read_lock_held(sp), __rcu)
 
 /**
  * srcu_dereference - fetch SRCU-protected pointer for later dereferencing
Signature · 1 line
1.5.5.1
#6 lore raw
In reply to Oleg Nesterov
Quoted 6 lines · Oleg Nesterov

Hello,

May be correct this time ;) Based on paulmck/linux-rcu.git rcu/next.

2/2 is new and hopefully trivial. But! the numbers look suspiciously good, I do not understand where does the difference come from...

Probably from rcu_dereference_raw() and rcu_dereference_check(..., 1). ;-)

Queued and kicked off testing, both mine and (indirectly) Fengguang's.

Thanx, Paul

#2 lore raw
In reply to Oleg Nesterov
Quoted 17 lines · Oleg Nesterov
include/linux/rcupdate.h +8 -0
+static inline void __rcu_lock_acquire(struct lockdep_map *map, unsigned long ip)
 {
+	lock_acquire(map, 0, 0, 2, 0, NULL, ip);
 }
 
+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);
include/linux/srcu.h +0 -0
index a2783cb..5c06289 100644

include/linux/srcu.h static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp) · line 224

Quoted 7 lines
include/linux/srcu.h +1 -1
@@ -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;
 }

Would an srcu_lock_acquire() not make sense here?

In any case, not wrong per se, just a consistency thing that stood out.

#7 lore raw
In reply to Paul E. McKenney
Quoted 10 lines · Paul E. McKenney
Quoted 6 lines · Oleg Nesterov

Hello,

May be correct this time ;) Based on paulmck/linux-rcu.git rcu/next.

2/2 is new and hopefully trivial. But! the numbers look suspiciously good, I do not understand where does the difference come from...

Probably from rcu_dereference_raw() and rcu_dereference_check(..., 1). ;-)

Yes, sure, this was the motivation for the patch. But I didn't expect the 50k difference ;)

OK, I understand now. I forgot that every list_for_each_rcu/list_entry_rcu has rcu_dereference_raw().

Quoted 1 line

Queued and kicked off testing, both mine and (indirectly) Fengguang's.

Thanks!

Oleg.

#3 lore raw
In reply to Peter Zijlstra
Quoted 22 lines · Peter Zijlstra
Quoted 20 lines · Oleg Nesterov
include/linux/rcupdate.h +8 -0
+static inline void __rcu_lock_acquire(struct lockdep_map *map, unsigned long ip)
 {
+	lock_acquire(map, 0, 0, 2, 0, NULL, ip);
 }
 
+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);
include/linux/srcu.h +0 -0
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);
 
Quoted 8 lines

include/linux/srcu.h static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp) · line 224

Quoted 4 lines
include/linux/srcu.h +1 -1
-	rcu_lock_acquire(&(sp)->dep_map);
+	__rcu_lock_acquire(&(sp)->dep_map, _THIS_IP_);
 	return retval;
 }

Would an srcu_lock_acquire() not make sense here?

In any case, not wrong per se, just a consistency thing that stood out.

Yes, I looked at this too...

But probably it would be better to just add __rcu_lock_acquire() into __srcu_read_lock(), and kill that inline in srcu.h ?

Oleg.

#8 lore raw
In reply to Oleg Nesterov
Quoted 8 lines · Oleg Nesterov
Quoted 6 lines · Paul E. McKenney
Quoted 3 lines · Oleg Nesterov

2/2 is new and hopefully trivial. But! the numbers look suspiciously good, I do not understand where does the difference come from...

Quoted 7 lines
Quoted 1 line

Probably from rcu_dereference_raw() and rcu_dereference_check(..., 1). ;-)

Yes, sure, this was the motivation for the patch. But I didn't expect the 50k difference ;)

OK, I understand now. I forgot that every list_for_each_rcu/list_entry_rcu has rcu_dereference_raw().

And this naturally suggests that rcu_read_lock_held() should be uninlined too (and rcu_read_lock_sched_held(), but this needs another patch).

But I still can't understand the difference reported by "size vmlinux",

- 5541731 3014560 14757888 23314179
+ 5513182 3026848 14757888 23297918

it removes 28K from .text, but somehow it adds 12K to .data. I do not see any difference in .data when I compare individual .o files before/ after this patch.

Confused.

Oleg.

include/linux/rcupdate.h +2 -36
index e8c55d8..8980817 100644
@@ -378,42 +378,8 @@ extern void rcu_lock_release_bh(void);
 extern void rcu_lock_acquire_sched(void);
 extern void rcu_lock_release_sched(void);
 
-/**
- * rcu_read_lock_held() - might we be in RCU read-side critical section?
- *
- * If CONFIG_DEBUG_LOCK_ALLOC is selected, returns nonzero iff in an RCU
- * read-side critical section.  In absence of CONFIG_DEBUG_LOCK_ALLOC,
- * this assumes we are in an RCU read-side critical section unless it can
- * prove otherwise.  This is useful for debug checks in functions that
- * require that they be called within an RCU read-side critical section.
- *
- * Checks debug_lockdep_rcu_enabled() to prevent false positives during boot
- * and while lockdep is disabled.
- *
- * Note that rcu_read_lock() and the matching rcu_read_unlock() must
- * occur in the same context, for example, it is illegal to invoke
- * rcu_read_unlock() in process context if the matching rcu_read_lock()
- * was invoked from within an irq handler.
- *
- * Note that rcu_read_lock() is disallowed if the CPU is either idle or
- * offline from an RCU perspective, so check for those as well.
- */
-static inline int rcu_read_lock_held(void)
-{
-	if (!debug_lockdep_rcu_enabled())
-		return 1;
-	if (!rcu_is_watching())
-		return 0;
-	if (!rcu_lockdep_current_cpu_online())
-		return 0;
-	return lock_is_held(&rcu_lock_map);
-}
-
-/*
- * rcu_read_lock_bh_held() is defined out of line to avoid #include-file
- * hell.
- */
-int rcu_read_lock_bh_held(void);
+extern int rcu_read_lock_held(void);
+extern int rcu_read_lock_bh_held(void);
 
 /**
  * rcu_read_lock_sched_held() - might we be in RCU-sched read-side critical section?
kernel/rcu/update.c +32 -0
index c2209eb..ea4af81 100644
@@ -137,6 +137,38 @@ int notrace debug_lockdep_rcu_enabled(void)
 EXPORT_SYMBOL_GPL(debug_lockdep_rcu_enabled);
 
 /**
+ * rcu_read_lock_held() - might we be in RCU read-side critical section?
+ *
+ * If CONFIG_DEBUG_LOCK_ALLOC is selected, returns nonzero iff in an RCU
+ * read-side critical section.  In absence of CONFIG_DEBUG_LOCK_ALLOC,
+ * this assumes we are in an RCU read-side critical section unless it can
+ * prove otherwise.  This is useful for debug checks in functions that
+ * require that they be called within an RCU read-side critical section.
+ *
+ * Checks debug_lockdep_rcu_enabled() to prevent false positives during boot
+ * and while lockdep is disabled.
+ *
+ * Note that rcu_read_lock() and the matching rcu_read_unlock() must
+ * occur in the same context, for example, it is illegal to invoke
+ * rcu_read_unlock() in process context if the matching rcu_read_lock()
+ * was invoked from within an irq handler.
+ *
+ * Note that rcu_read_lock() is disallowed if the CPU is either idle or
+ * offline from an RCU perspective, so check for those as well.
+ */
+int rcu_read_lock_held(void)
+{
+	if (!debug_lockdep_rcu_enabled())
+		return 1;
+	if (!rcu_is_watching())
+		return 0;
+	if (!rcu_lockdep_current_cpu_online())
+		return 0;
+	return lock_is_held(&rcu_lock_map);
+}
+EXPORT_SYMBOL_GPL(rcu_read_lock_held);
+
+/**
  * rcu_read_lock_bh_held() - might we be in RCU-bh read-side critical section?
  *
  * Check for bottom half being disabled, which covers both the
#9 lore raw

cover rcu: uninline rcu_read_lock_held()

Quoted 4 lines · Oleg Nesterov

And this naturally suggests that rcu_read_lock_held() should be uninlined too (and rcu_read_lock_sched_held(), but this needs another patch).

Quoted 8 lines

But I still can't understand the difference reported by "size vmlinux",

- 5541731 3014560 14757888 23314179
+ 5513182 3026848 14757888 23297918

it removes 28K from .text, but somehow it adds 12K to .data. I do not see any difference in .data when I compare individual .o files before/ after this patch.

OK, it doesn't add to .data. This is because of INIT_TASK_DATA(THREAD_SIZE) in vmlinux.lds.S, see the changelog.

If you think this makes sense I will will spam you again. We can also uninline rcu_read_lock_sched_held(), mostly for consistency. But this needs a separate change.

Oleg.

#10 lore raw

PATCH 1/1 rcu: uninline rcu_read_lock_held()

Uninline rcu_read_lock_held(). According to size vmlinux this saves 28549 in .text:

- 5541731 3014560 14757888 23314179
+ 5513182 3026848 14757888 23297918

Note: it looks as if the data grows by 12288 bytes but this is not true, it does not actually grow. But .data starts with ALIGN(THREAD_SIZE) and since .text shrinks the padding grows, and thus .data grows too as it seen by /bin/size. diff System.map:

- ffffffff81510000 D _sdata
- ffffffff81510000 D init_thread_union
+ ffffffff81509000 D _sdata
+ ffffffff8150c000 D init_thread_union

Perhaps we can change vmlinux.lds.S to .data itself, so that /bin/size can't "wrongly" report that .data grows if .text shinks.

---
 include/linux/rcupdate.h |   38 ++------------------------------------
 kernel/rcu/update.c      |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 36 deletions(-)
include/linux/rcupdate.h +2 -36
index e8c55d8..8980817 100644
@@ -378,42 +378,8 @@ extern void rcu_lock_release_bh(void);
 extern void rcu_lock_acquire_sched(void);
 extern void rcu_lock_release_sched(void);
 
-/**
- * rcu_read_lock_held() - might we be in RCU read-side critical section?
- *
- * If CONFIG_DEBUG_LOCK_ALLOC is selected, returns nonzero iff in an RCU
- * read-side critical section.  In absence of CONFIG_DEBUG_LOCK_ALLOC,
- * this assumes we are in an RCU read-side critical section unless it can
- * prove otherwise.  This is useful for debug checks in functions that
- * require that they be called within an RCU read-side critical section.
- *
- * Checks debug_lockdep_rcu_enabled() to prevent false positives during boot
- * and while lockdep is disabled.
- *
- * Note that rcu_read_lock() and the matching rcu_read_unlock() must
- * occur in the same context, for example, it is illegal to invoke
- * rcu_read_unlock() in process context if the matching rcu_read_lock()
- * was invoked from within an irq handler.
- *
- * Note that rcu_read_lock() is disallowed if the CPU is either idle or
- * offline from an RCU perspective, so check for those as well.
- */
-static inline int rcu_read_lock_held(void)
-{
-	if (!debug_lockdep_rcu_enabled())
-		return 1;
-	if (!rcu_is_watching())
-		return 0;
-	if (!rcu_lockdep_current_cpu_online())
-		return 0;
-	return lock_is_held(&rcu_lock_map);
-}
-
-/*
- * rcu_read_lock_bh_held() is defined out of line to avoid #include-file
- * hell.
- */
-int rcu_read_lock_bh_held(void);
+extern int rcu_read_lock_held(void);
+extern int rcu_read_lock_bh_held(void);
 
 /**
  * rcu_read_lock_sched_held() - might we be in RCU-sched read-side critical section?
kernel/rcu/update.c +32 -0
index c2209eb..ea4af81 100644
@@ -137,6 +137,38 @@ int notrace debug_lockdep_rcu_enabled(void)
 EXPORT_SYMBOL_GPL(debug_lockdep_rcu_enabled);
 
 /**
+ * rcu_read_lock_held() - might we be in RCU read-side critical section?
+ *
+ * If CONFIG_DEBUG_LOCK_ALLOC is selected, returns nonzero iff in an RCU
+ * read-side critical section.  In absence of CONFIG_DEBUG_LOCK_ALLOC,
+ * this assumes we are in an RCU read-side critical section unless it can
+ * prove otherwise.  This is useful for debug checks in functions that
+ * require that they be called within an RCU read-side critical section.
+ *
+ * Checks debug_lockdep_rcu_enabled() to prevent false positives during boot
+ * and while lockdep is disabled.
+ *
+ * Note that rcu_read_lock() and the matching rcu_read_unlock() must
+ * occur in the same context, for example, it is illegal to invoke
+ * rcu_read_unlock() in process context if the matching rcu_read_lock()
+ * was invoked from within an irq handler.
+ *
+ * Note that rcu_read_lock() is disallowed if the CPU is either idle or
+ * offline from an RCU perspective, so check for those as well.
+ */
+int rcu_read_lock_held(void)
+{
+	if (!debug_lockdep_rcu_enabled())
+		return 1;
+	if (!rcu_is_watching())
+		return 0;
+	if (!rcu_lockdep_current_cpu_online())
+		return 0;
+	return lock_is_held(&rcu_lock_map);
+}
+EXPORT_SYMBOL_GPL(rcu_read_lock_held);
+
+/**
  * rcu_read_lock_bh_held() - might we be in RCU-bh read-side critical section?
  *
  * Check for bottom half being disabled, which covers both the
Signature · 1 line
1.5.5.1
#11 lore raw

Re: [PATCH 1/1] rcu: uninline rcu_read_lock_held()

Quoted 16 lines · Oleg Nesterov

Uninline rcu_read_lock_held(). According to size vmlinux this saves 28549 in .text:

- 5541731 3014560 14757888 23314179
+ 5513182 3026848 14757888 23297918

Note: it looks as if the data grows by 12288 bytes but this is not true, it does not actually grow. But .data starts with ALIGN(THREAD_SIZE) and since .text shrinks the padding grows, and thus .data grows too as it seen by /bin/size. diff System.map:

- ffffffff81510000 D _sdata
- ffffffff81510000 D init_thread_union
+ ffffffff81509000 D _sdata
+ ffffffff8150c000 D init_thread_union

Quoted 4 lines

Perhaps we can change vmlinux.lds.S to .data itself, so that /bin/size can't "wrongly" report that .data grows if .text shinks.

Queued for 3.18, thank you, Oleg!

I removed the "extern" tags, apparently people don't like them on function declarations anymore.

Thanx, Paul

Quoted 101 lines
---
 include/linux/rcupdate.h |   38 ++------------------------------------
 kernel/rcu/update.c      |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 36 deletions(-)
include/linux/rcupdate.h +2 -36
index e8c55d8..8980817 100644
@@ -378,42 +378,8 @@ extern void rcu_lock_release_bh(void);
 extern void rcu_lock_acquire_sched(void);
 extern void rcu_lock_release_sched(void);
 
-/**
- * rcu_read_lock_held() - might we be in RCU read-side critical section?
- *
- * If CONFIG_DEBUG_LOCK_ALLOC is selected, returns nonzero iff in an RCU
- * read-side critical section.  In absence of CONFIG_DEBUG_LOCK_ALLOC,
- * this assumes we are in an RCU read-side critical section unless it can
- * prove otherwise.  This is useful for debug checks in functions that
- * require that they be called within an RCU read-side critical section.
- *
- * Checks debug_lockdep_rcu_enabled() to prevent false positives during boot
- * and while lockdep is disabled.
- *
- * Note that rcu_read_lock() and the matching rcu_read_unlock() must
- * occur in the same context, for example, it is illegal to invoke
- * rcu_read_unlock() in process context if the matching rcu_read_lock()
- * was invoked from within an irq handler.
- *
- * Note that rcu_read_lock() is disallowed if the CPU is either idle or
- * offline from an RCU perspective, so check for those as well.
- */
-static inline int rcu_read_lock_held(void)
-{
-	if (!debug_lockdep_rcu_enabled())
-		return 1;
-	if (!rcu_is_watching())
-		return 0;
-	if (!rcu_lockdep_current_cpu_online())
-		return 0;
-	return lock_is_held(&rcu_lock_map);
-}
-
-/*
- * rcu_read_lock_bh_held() is defined out of line to avoid #include-file
- * hell.
- */
-int rcu_read_lock_bh_held(void);
+extern int rcu_read_lock_held(void);
+extern int rcu_read_lock_bh_held(void);
 
 /**
  * rcu_read_lock_sched_held() - might we be in RCU-sched read-side critical section?
kernel/rcu/update.c +32 -0
index c2209eb..ea4af81 100644
@@ -137,6 +137,38 @@ int notrace debug_lockdep_rcu_enabled(void)
 EXPORT_SYMBOL_GPL(debug_lockdep_rcu_enabled);
 
 /**
+ * rcu_read_lock_held() - might we be in RCU read-side critical section?
+ *
+ * If CONFIG_DEBUG_LOCK_ALLOC is selected, returns nonzero iff in an RCU
+ * read-side critical section.  In absence of CONFIG_DEBUG_LOCK_ALLOC,
+ * this assumes we are in an RCU read-side critical section unless it can
+ * prove otherwise.  This is useful for debug checks in functions that
+ * require that they be called within an RCU read-side critical section.
+ *
+ * Checks debug_lockdep_rcu_enabled() to prevent false positives during boot
+ * and while lockdep is disabled.
+ *
+ * Note that rcu_read_lock() and the matching rcu_read_unlock() must
+ * occur in the same context, for example, it is illegal to invoke
+ * rcu_read_unlock() in process context if the matching rcu_read_lock()
+ * was invoked from within an irq handler.
+ *
+ * Note that rcu_read_lock() is disallowed if the CPU is either idle or
+ * offline from an RCU perspective, so check for those as well.
+ */
+int rcu_read_lock_held(void)
+{
+	if (!debug_lockdep_rcu_enabled())
+		return 1;
+	if (!rcu_is_watching())
+		return 0;
+	if (!rcu_lockdep_current_cpu_online())
+		return 0;
+	return lock_is_held(&rcu_lock_map);
+}
+EXPORT_SYMBOL_GPL(rcu_read_lock_held);
+
+/**
  * rcu_read_lock_bh_held() - might we be in RCU-bh read-side critical section?
  *
  * Check for bottom half being disabled, which covers both the
Signature · 1 line
1.5.5.1
#4 lore raw
In reply to Oleg Nesterov
Quoted 30 lines · Oleg Nesterov
Quoted 29 lines · Peter Zijlstra
Quoted 17 lines · Oleg Nesterov
include/linux/rcupdate.h +8 -0
+static inline void __rcu_lock_acquire(struct lockdep_map *map, unsigned long ip)
 {
+	lock_acquire(map, 0, 0, 2, 0, NULL, ip);
 }
 
+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);
include/linux/srcu.h +0 -0
index a2783cb..5c06289 100644

include/linux/srcu.h static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp) · line 224

Quoted 7 lines
include/linux/srcu.h +1 -1
@@ -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;
 }

Would an srcu_lock_acquire() not make sense here?

Quoted 6 lines
Quoted 1 line

In any case, not wrong per se, just a consistency thing that stood out.

Yes, I looked at this too...

But probably it would be better to just add __rcu_lock_acquire() into __srcu_read_lock(), and kill that inline in srcu.h ?

Makes sense to me!

Thanx, Paul