Skip to contentSkip to conversation

events: callchain: Annotate RCU pointer with __rcu

PATCH Superseded Amol Grover 3 participants 7 messages Jan 29, 2020 – Jan 30, 2020 perf events

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Amol Grover

author
#0 lore raw

PATCH 1/2 events: callchain: Annotate RCU pointer with __rcu

Fixes following instances of sparse error
error: incompatible types in comparison expression
(different address spaces)
kernel/events/callchain.c:66:9: error: incompatible types in comparison kernel/events/callchain.c:96:9: error: incompatible types in comparison kernel/events/callchain.c:161:19: error: incompatible types in comparison

This introduces the following warning kernel/events/callchain.c:65:17: warning: incorrect type in assignment

Signed-off-by Amol Grover <[email protected]>
---
 kernel/events/callchain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/events/callchain.c +1 -1
index c2b41a263166..f91e1f41d25d 100644
@@ -32,7 +32,7 @@ static inline size_t perf_callchain_entry__sizeof(void)
 static DEFINE_PER_CPU(int, callchain_recursion[PERF_NR_CONTEXTS]);
 static atomic_t nr_callchain_events;
 static DEFINE_MUTEX(callchain_mutex);
-static struct callchain_cpus_entries *callchain_cpus_entries;
+static struct callchain_cpus_entries __rcu *callchain_cpus_entries;
 
 
 __weak void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
Signature · 1 line
2.24.1
author
#1 lore raw

PATCH 2/2 events: callchain: Use RCU API to access RCU pointer

callchain_cpus_entries is annotated as an RCU pointer. Hence rcu_dereference_protected or similar RCU API is required to dereference the pointer.

This fixes the following sparse warning kernel/events/callchain.c:65:17: warning: incorrect type in assignment

Signed-off-by Amol Grover <[email protected]>
---
 kernel/events/callchain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/events/callchain.c +2 -1
index f91e1f41d25d..a672d02a1b3a 100644
@@ -62,7 +62,8 @@ static void release_callchain_buffers(void)
 {
 	struct callchain_cpus_entries *entries;
 
-	entries = callchain_cpus_entries;
+	entries = rcu_dereference_protected(callchain_cpus_entries,
+					    lockdep_is_held(&callchain_mutex));
 	RCU_INIT_POINTER(callchain_cpus_entries, NULL);
 	call_rcu(&entries->rcu_head, release_callchain_buffers_rcu);
 }
Signature · 1 line
2.24.1
Reviewed-by on v1 2/2
#2 lore raw

Re: [PATCH 2/2] events: callchain: Use RCU API to access RCU pointer

Quoted 17 lines · Amol Grover

callchain_cpus_entries is annotated as an RCU pointer. Hence rcu_dereference_protected or similar RCU API is required to dereference the pointer.

This fixes the following sparse warning kernel/events/callchain.c:65:17: warning: incorrect type in assignment

Signed-off-by Amol Grover <[email protected]>
---
 kernel/events/callchain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/events/callchain.c +0 -0
index f91e1f41d25d..a672d02a1b3a 100644

kernel/events/callchain.c static void release_callchain_buffers(void) · line 66

Quoted 6 lines
kernel/events/callchain.c +2 -1
@@ -62,7 +62,8 @@ static void release_callchain_buffers(void)
 {
 	struct callchain_cpus_entries *entries;
 
-	entries = callchain_cpus_entries;
+	entries = rcu_dereference_protected(callchain_cpus_entries,
+					    lockdep_is_held(&callchain_mutex));
Reviewed-by Joel Fernandes (Google) <[email protected]>

thanks,

- Joel

Quoted 6 lines
kernel/events/callchain.c +0 -0
 	RCU_INIT_POINTER(callchain_cpus_entries, NULL);
 	call_rcu(&entries->rcu_head, release_callchain_buffers_rcu);
 }
Signature · 1 line
2.24.1
maintainer
#3 lore raw

Re: [PATCH 2/2] events: callchain: Use RCU API to access RCU pointer

Quoted 7 lines · Joel Fernandes
Quoted 6 lines · Amol Grover

callchain_cpus_entries is annotated as an RCU pointer. Hence rcu_dereference_protected or similar RCU API is required to dereference the pointer.

This fixes the following sparse warning kernel/events/callchain.c:65:17: warning: incorrect type in assignment

Seems silly to have this two patches; the first introduces the second issue, might as well fix it all in one go.

Also look at the output of:

  git log --oneline kernel/events/

and then at your $subject.

Quoted 13 lines
Quoted 13 lines
Signed-off-by Amol Grover <[email protected]>
---
 kernel/events/callchain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/events/callchain.c +0 -0
index f91e1f41d25d..a672d02a1b3a 100644
@@ -62,7 +62,8 @@ static void release_callchain_buffers(void)
 {
 	struct callchain_cpus_entries *entries;
Quoted 6 lines

kernel/events/callchain.c static void release_callchain_buffers(void) · line 66

Quoted 3 lines
kernel/events/callchain.c +2 -1
-	entries = callchain_cpus_entries;
+	entries = rcu_dereference_protected(callchain_cpus_entries,
+					    lockdep_is_held(&callchain_mutex));
Reviewed-by Joel Fernandes (Google) <[email protected]>

Do we really need that smp_read_barrier_depends() here? Then again, I don't suppose this is a fast path.

IIRC even Alpha got the dependent write ordering right.

Quoted 6 lines
Quoted 6 lines
kernel/events/callchain.c +0 -0
 	RCU_INIT_POINTER(callchain_cpus_entries, NULL);
 	call_rcu(&entries->rcu_head, release_callchain_buffers_rcu);
 }
Signature · 1 line
2.24.1
author
#4 lore raw

Re: [PATCH 2/2] events: callchain: Use RCU API to access RCU pointer

Quoted 6 lines · Peter Zijlstra
Quoted 5 lines · Joel Fernandes
Quoted 4 lines · Amol Grover

callchain_cpus_entries is annotated as an RCU pointer. Hence rcu_dereference_protected or similar RCU API is required to dereference the pointer.

Quoted 6 lines
Quoted 2 lines
Quoted 2 lines

This fixes the following sparse warning kernel/events/callchain.c:65:17: warning: incorrect type in assignment

Seems silly to have this two patches; the first introduces the second issue, might as well fix it all in one go.

Got it. I'll combine them into a single patch and re-send.

Quoted 26 lines

Also look at the output of:

  git log --oneline kernel/events/

and then at your $subject.

Quoted 13 lines
Quoted 13 lines
Signed-off-by Amol Grover <[email protected]>
---
 kernel/events/callchain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/events/callchain.c +0 -0
index f91e1f41d25d..a672d02a1b3a 100644
@@ -62,7 +62,8 @@ static void release_callchain_buffers(void)
 {
 	struct callchain_cpus_entries *entries;
Quoted 6 lines

kernel/events/callchain.c static void release_callchain_buffers(void) · line 66

Quoted 3 lines
kernel/events/callchain.c +2 -1
-	entries = callchain_cpus_entries;
+	entries = rcu_dereference_protected(callchain_cpus_entries,
+					    lockdep_is_held(&callchain_mutex));
Reviewed-by Joel Fernandes (Google) <[email protected]>
Quoted 3 lines

Do we really need that smp_read_barrier_depends() here? Then again, I don't suppose this is a fast path.

rcu_dereference_protected is actually a lightweight API and IIRC it omits the READ_ONCE() and hence the memory barriers.

Thanks
Amol

Quoted 8 lines

IIRC even Alpha got the dependent write ordering right.

Quoted 6 lines
Quoted 6 lines
kernel/events/callchain.c +0 -0
 	RCU_INIT_POINTER(callchain_cpus_entries, NULL);
 	call_rcu(&entries->rcu_head, release_callchain_buffers_rcu);
 }
Signature · 1 line
2.24.1

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

maintainer
#5 lore raw

Re: [PATCH 2/2] events: callchain: Use RCU API to access RCU pointer

Quoted 15 lines · Amol Grover
Quoted 15 lines
Quoted 14 lines

kernel/events/callchain.c static void release_callchain_buffers(void) · line 66

Quoted 11 lines
kernel/events/callchain.c +2 -1
index f91e1f41d25d..a672d02a1b3a 100644
@@ -62,7 +62,8 @@ static void release_callchain_buffers(void)
 {
 	struct callchain_cpus_entries *entries;
 
-	entries = callchain_cpus_entries;
+	entries = rcu_dereference_protected(callchain_cpus_entries,
+					    lockdep_is_held(&callchain_mutex));
Reviewed-by Joel Fernandes (Google) <[email protected]>
Quoted 6 lines
Quoted 3 lines

Do we really need that smp_read_barrier_depends() here? Then again, I don't suppose this is a fast path.

rcu_dereference_protected is actually a lightweight API and IIRC it omits the READ_ONCE() and hence the memory barriers.

Oh argh, indeed. I suppose I should've had more tea this morning.
_______________________________________________
Linux-kernel-mentees mailing list [email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Reviewed-by on v1 1/2
#6 lore raw
In reply to Amol Grover
Quoted 9 lines · Amol Grover

Fixes following instances of sparse error
error: incompatible types in comparison expression
(different address spaces)
kernel/events/callchain.c:66:9: error: incompatible types in comparison kernel/events/callchain.c:96:9: error: incompatible types in comparison kernel/events/callchain.c:161:19: error: incompatible types in comparison

This introduces the following warning kernel/events/callchain.c:65:17: warning: incorrect type in assignment

Would have been nice if you mentioned the warning is fixed in your second patch. But I think its ok.

Reviewed-by Joel Fernandes (Google) <[email protected]>

thanks,

- Joel

Quoted 22 lines
Signed-off-by Amol Grover <[email protected]>
---
 kernel/events/callchain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/events/callchain.c +1 -1
index c2b41a263166..f91e1f41d25d 100644
@@ -32,7 +32,7 @@ static inline size_t perf_callchain_entry__sizeof(void)
 static DEFINE_PER_CPU(int, callchain_recursion[PERF_NR_CONTEXTS]);
 static atomic_t nr_callchain_events;
 static DEFINE_MUTEX(callchain_mutex);
-static struct callchain_cpus_entries *callchain_cpus_entries;
+static struct callchain_cpus_entries __rcu *callchain_cpus_entries;
 
 
 __weak void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
Signature · 1 line
2.24.1