events: Annotate parent_ctx with __rcu
PATCH 1/1 events: Annotate parent_ctx with __rcu
parent_ctx is used under RCU context in kernel/events/core.c, tell sparse about it aswell.
Fixes the following instances of sparse error:kernel/events/core.c:3221:18: error: incompatible types in comparison kernel/events/core.c:3222:23: error: incompatible types in comparison
This introduces the following two sparse errors:kernel/events/core.c:3117:18: error: incompatible types in comparison kernel/events/core.c:3121:30: error: incompatible types in comparison
Hence, use rcu_dereference() to access parent_ctx and silence the newly introduced errors.
--- include/linux/perf_event.h | 2 +- kernel/events/core.c | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-)
include/linux/perf_event.h +1 -1
@@ -810,7 +810,7 @@ struct perf_event_context {* These fields let us detect when two contexts have both * been cloned (inherited) from a common ancestor. */ -struct perf_event_context *parent_ctx; +struct perf_event_context __rcu *parent_ctx;u64 parent_gen;u64 generation;int pin_count;
kernel/events/core.c +8 -3
@@ -3106,26 +3106,31 @@ static void ctx_sched_out(struct perf_event_context *ctx,static int context_equiv (struct perf_event_context *ctx1,struct perf_event_context *ctx2) { +struct perf_event_context *parent_ctx1, *parent_ctx2; +lockdep_assert_held (&ctx1->lock);lockdep_assert_held (&ctx2->lock); + parent_ctx1 =rcu_dereference (ctx1->parent_ctx); + parent_ctx2 =rcu_dereference (ctx2->parent_ctx); +/* Pinning disables the swap optimization */ if (ctx1->pin_count || ctx2->pin_count)return 0 ;/* If ctx1 is the parent of ctx2 */ -if (ctx1 == ctx2->parent_ctx && ctx1->generation == ctx2->parent_gen) +if (ctx1 == parent_ctx2 && ctx1->generation == ctx2->parent_gen)return 1 ;/* If ctx2 is the parent of ctx1 */ -if (ctx1->parent_ctx == ctx2 && ctx1->parent_gen == ctx2->generation) +if (parent_ctx1 == ctx2 && ctx1->parent_gen == ctx2->generation)return 1 ;/* * If ctx1 and ctx2 have the same parent; we flatten the parent * hierarchy, see perf_event_init_context(). */ -if (ctx1->parent_ctx && ctx1->parent_ctx == ctx2->parent_ctx && +if (ctx1->parent_ctx && parent_ctx1 == parent_ctx2 && ctx1->parent_gen == ctx2->parent_gen)return 1 ;
Signature · 1 line
2.24.1
Peter Zijlstra
maintainerQuoted 38 lines · Amol Grover
parent_ctxis used under RCU context inkernel/events/core.c, tell sparse about it aswell.Fixes the following instances of sparse error:
kernel/events/core.c:3221:18: error: incompatible types in comparisonkernel/events/core.c:3222:23: error: incompatible types in comparisonThis introduces the following two sparse errors:
kernel/events/core.c:3117:18: error: incompatible types in comparisonkernel/events/core.c:3121:30: error: incompatible types in comparisonHence, use
rcu_dereference()to accessparent_ctxand silence the newly introduced errors.Signed-off-by Amol Grover <[email protected]>--- include/linux/perf_event.h | 2 +- kernel/events/core.c | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-)include/linux/perf_event.h +1
-1@@ -810,7 +810,7 @@ struct perf_event_context {* These fields let us detect when two contexts have both * been cloned (inherited) from a common ancestor. */ -struct perf_event_context *parent_ctx; +struct perf_event_context __rcu *parent_ctx;u64 parent_gen;u64 generation;int pin_count;kernel/events/core.c +0
-0
kernel/events/core.c static void ctx_sched_out(struct perf_event_context *ctx, · line 3115
Quoted 10 lines
kernel/events/core.c +4
-0@@ -3106,26 +3106,31 @@ static void ctx_sched_out(struct perf_event_context *ctx,static int context_equiv (struct perf_event_context *ctx1,struct perf_event_context *ctx2) { +struct perf_event_context *parent_ctx1, *parent_ctx2; +lockdep_assert_held (&ctx1->lock);lockdep_assert_held (&ctx2->lock); + parent_ctx1 =rcu_dereference (ctx1->parent_ctx); + parent_ctx2 =rcu_dereference (ctx2->parent_ctx);
Bah.
Why are you fixing all this sparse crap and making the code worse?
Quoted 47 lines · Peter Zijlstra
Quoted 46 lines · Amol Grover
parent_ctxis used under RCU context inkernel/events/core.c, tell sparse about it aswell.Fixes the following instances of sparse error:
kernel/events/core.c:3221:18: error: incompatible types in comparisonkernel/events/core.c:3222:23: error: incompatible types in comparisonThis introduces the following two sparse errors:
kernel/events/core.c:3117:18: error: incompatible types in comparisonkernel/events/core.c:3121:30: error: incompatible types in comparisonHence, use
rcu_dereference()to accessparent_ctxand silence the newly introduced errors.Signed-off-by Amol Grover <[email protected]>--- include/linux/perf_event.h | 2 +- kernel/events/core.c | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-)include/linux/perf_event.h +1
-1@@ -810,7 +810,7 @@ struct perf_event_context {* These fields let us detect when two contexts have both * been cloned (inherited) from a common ancestor. */ -struct perf_event_context *parent_ctx; +struct perf_event_context __rcu *parent_ctx;u64 parent_gen;u64 generation;int pin_count;kernel/events/core.c +2
-0@@ -3106,26 +3106,31 @@ static void ctx_sched_out(struct perf_event_context *ctx,static int context_equiv (struct perf_event_context *ctx1,struct perf_event_context *ctx2) { +struct perf_event_context *parent_ctx1, *parent_ctx2; +lockdep_assert_held (&ctx1->lock);lockdep_assert_held (&ctx2->lock);
Quoted 6 lines
kernel/events/core.c static void ctx_sched_out(struct perf_event_context *ctx, · line 3115
Quoted 2 lines
kernel/events/core.c +2
-0+ parent_ctx1 =rcu_dereference (ctx1->parent_ctx); + parent_ctx2 =rcu_dereference (ctx2->parent_ctx);Bah.
Why are you fixing all this sparse crap and making the code worse?
Hi Peter,
Sparse is quite noisy and we need to eliminate false-positives, right? __rcu will tell the developer, this pointer could change and he needs to take the required steps to make sure the code doesn't break.
Thanks
Amol
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
Peter Zijlstra
maintainerQuoted 17 lines · Amol Grover
Quoted 16 lines · Peter Zijlstra
kernel/events/core.c static void ctx_sched_out(struct perf_event_context *ctx, · line 3115
Quoted 11 lines · Amol Grover
kernel/events/core.c +4
-0@@ -3106,26 +3106,31 @@ static void ctx_sched_out(struct perf_event_context *ctx,static int context_equiv (struct perf_event_context *ctx1,struct perf_event_context *ctx2) { +struct perf_event_context *parent_ctx1, *parent_ctx2; +lockdep_assert_held (&ctx1->lock);lockdep_assert_held (&ctx2->lock); + parent_ctx1 =rcu_dereference (ctx1->parent_ctx); + parent_ctx2 =rcu_dereference (ctx2->parent_ctx);Bah.
Quoted 5 lines
Quoted 1 line
Why are you fixing all this sparse crap and making the code worse?
Hi Peter,
Sparse is quite noisy and we need to eliminate false-positives, right?
Dunno, I've been happy just ignoring it all.
Quoted 2 lines
__rcuwill tell the developer, this pointer could change and he needs to take the required steps to make sure the code doesn't break.
I know what it does; what I don't know is why you need to make the code worse. In paricular, __rcu doesn't mandate rcu_dereference(), esp. not when you're actually holding the write side lock.
_______________________________________________
Linux-kernel-mentees mailing list [email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
Quoted 24 lines · Peter Zijlstra
Quoted 17 lines · Amol Grover
Quoted 16 lines · Peter Zijlstra
kernel/events/core.c static void ctx_sched_out(struct perf_event_context *ctx, · line 3115
Quoted 11 lines · Amol Grover
kernel/events/core.c +4
-0@@ -3106,26 +3106,31 @@ static void ctx_sched_out(struct perf_event_context *ctx,static int context_equiv (struct perf_event_context *ctx1,struct perf_event_context *ctx2) { +struct perf_event_context *parent_ctx1, *parent_ctx2; +lockdep_assert_held (&ctx1->lock);lockdep_assert_held (&ctx2->lock); + parent_ctx1 =rcu_dereference (ctx1->parent_ctx); + parent_ctx2 =rcu_dereference (ctx2->parent_ctx);Bah.
Quoted 5 lines
Quoted 1 line
Why are you fixing all this sparse crap and making the code worse?
Hi Peter,
Sparse is quite noisy and we need to eliminate false-positives, right?
Quoted 8 lines
Dunno, I've been happy just ignoring it all.
Quoted 2 lines
__rcuwill tell the developer, this pointer could change and he needs to take the required steps to make sure the code doesn't break.I know what it does; what I don't know is why you need to make the code worse. In paricular,
__rcudoesn't mandatercu_dereference(), esp. not when you're actually holding the write side lock.
I might've misinterpreted the code. How does replacing rcu_dereference() withparent_ctx1 = rcu_dereference_protected(ctx1->parent_ctx,
lockdep_is_held(&ctx1->lock));
sound?
Thanks
Amol
Quoted 14 lines · Amol Grover
Quoted 13 lines · Peter Zijlstra
Quoted 12 lines · Amol Grover
Quoted 11 lines · Peter Zijlstra
Quoted 9 lines · Amol Grover
kernel/events/core.c +2
-0@@ -3106,26 +3106,31 @@ static void ctx_sched_out(struct perf_event_context *ctx,static int context_equiv (struct perf_event_context *ctx1,struct perf_event_context *ctx2) { +struct perf_event_context *parent_ctx1, *parent_ctx2; +lockdep_assert_held (&ctx1->lock);lockdep_assert_held (&ctx2->lock);
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
kernel/events/core.c static void ctx_sched_out(struct perf_event_context *ctx, · line 3115
Quoted 2 lines
kernel/events/core.c +2
-0+ parent_ctx1 =rcu_dereference (ctx1->parent_ctx); + parent_ctx2 =rcu_dereference (ctx2->parent_ctx);
You can probably remove the earlier lockdep_assert_held(s) if you're going to use rcu_dereference_protected() here, since that would do the checking anyway.
Quoted 10 lines
Quoted 10 lines
Quoted 8 lines
Quoted 4 lines
Bah.
Why are you fixing all this sparse crap and making the code worse?
Hi Peter,
Sparse is quite noisy and we need to eliminate false-positives, right?
Dunno, I've been happy just ignoring it all.
FWIW some of the sparse fixes Amol made recently did uncover so existing "bugs" :) (Not in perf but other code).
Quoted 7 lines
Quoted 6 lines
Quoted 2 lines
__rcuwill tell the developer, this pointer could change and he needs to take the required steps to make sure the code doesn't break.I know what it does; what I don't know is why you need to make the code worse. In paricular,
__rcudoesn't mandatercu_dereference(), esp. not when you're actually holding the write side lock.
Quoted 5 lines
I might've misinterpreted the code. How does replacing
rcu_dereference()withparent_ctx1= rcu_dereference_protected(ctx1->parent_ctx,lockdep_is_held(&ctx1->lock));sound?
FWIW, some maintainers do hate calling RCU APIs when write side lock is held. Evidently it does make the code readability a bit worse and I can see Peter's point of view because the existing code is correct. I leave it to you guys to decide how you want to handle it.
thanks!
- Joel
Quoted 15 lines · Joel Fernandes
Quoted 14 lines · Amol Grover
Quoted 13 lines · Peter Zijlstra
Quoted 12 lines · Amol Grover
Quoted 11 lines · Peter Zijlstra
Quoted 9 lines · Amol Grover
kernel/events/core.c +2
-0@@ -3106,26 +3106,31 @@ static void ctx_sched_out(struct perf_event_context *ctx,static int context_equiv (struct perf_event_context *ctx1,struct perf_event_context *ctx2) { +struct perf_event_context *parent_ctx1, *parent_ctx2; +lockdep_assert_held (&ctx1->lock);lockdep_assert_held (&ctx2->lock);
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
kernel/events/core.c static void ctx_sched_out(struct perf_event_context *ctx, · line 3115
Quoted 2 lines
kernel/events/core.c +2
-0+ parent_ctx1 =rcu_dereference (ctx1->parent_ctx); + parent_ctx2 =rcu_dereference (ctx2->parent_ctx);You can probably remove the earlier
lockdep_assert_held(s)if you're going to usercu_dereference_protected()here, since that would do the checking anyway.
Ah yes, I was thinking this aswell.
Quoted 27 lines
Quoted 10 lines
Quoted 10 lines
Quoted 8 lines
Quoted 4 lines
Bah.
Why are you fixing all this sparse crap and making the code worse?
Hi Peter,
Sparse is quite noisy and we need to eliminate false-positives, right?
Dunno, I've been happy just ignoring it all.
FWIW some of the sparse fixes Amol made recently did uncover so existing "bugs" :) (Not in perf but other code).
Quoted 7 lines
Quoted 6 lines
Quoted 2 lines
__rcuwill tell the developer, this pointer could change and he needs to take the required steps to make sure the code doesn't break.I know what it does; what I don't know is why you need to make the code worse. In paricular,
__rcudoesn't mandatercu_dereference(), esp. not when you're actually holding the write side lock.Quoted 5 lines
I might've misinterpreted the code. How does replacing
rcu_dereference()withparent_ctx1= rcu_dereference_protected(ctx1->parent_ctx,lockdep_is_held(&ctx1->lock));sound?
Quoted 5 lines
FWIW, some maintainers do hate calling RCU APIs when write side lock is held. Evidently it does make the code readability a bit worse and I can see Peter's point of view because the existing code is correct. I leave it to you guys to decide how you want to handle it.
In that case, I think the code is fine as it is. Thank you for the review both!
Thanks
Amol
Quoted 4 lines
thanks!
- Joel