* [Linux-kernel-mentees] [PATCH] events: Annotate parent_ctx with __rcu @ 2020-02-08 14:46 Amol Grover 2020-02-10 9:36 ` Peter Zijlstra 0 siblings, 1 reply; 7+ messages in thread From: Amol Grover @ 2020-02-08 14:46 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim Cc: Paul E . McKenney, linux-kernel, Madhuparna Bhowmik, Joel Fernandes, linux-kernel-mentees 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. Signed-off-by: Amol Grover <frextrite@gmail.com> --- include/linux/perf_event.h | 2 +- kernel/events/core.c | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 6d4c22aee384..e18a7bdc6f98 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -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; diff --git a/kernel/events/core.c b/kernel/events/core.c index 2173c23c25b4..2a8c5670b254 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -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; -- 2.24.1 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] events: Annotate parent_ctx with __rcu 2020-02-08 14:46 [Linux-kernel-mentees] [PATCH] events: Annotate parent_ctx with __rcu Amol Grover @ 2020-02-10 9:36 ` Peter Zijlstra 2020-02-10 12:59 ` Amol Grover 0 siblings, 1 reply; 7+ messages in thread From: Peter Zijlstra @ 2020-02-10 9:36 UTC (permalink / raw) To: Amol Grover Cc: Mark Rutland, Paul E . McKenney, Alexander Shishkin, linux-kernel-mentees, linux-kernel, Arnaldo Carvalho de Melo, Madhuparna Bhowmik, Ingo Molnar, Joel Fernandes, Namhyung Kim, Jiri Olsa On Sat, Feb 08, 2020 at 08:16:49PM +0530, Amol Grover wrote: > 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. > > Signed-off-by: Amol Grover <frextrite@gmail.com> > --- > include/linux/perf_event.h | 2 +- > kernel/events/core.c | 11 ++++++++--- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index 6d4c22aee384..e18a7bdc6f98 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -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; > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 2173c23c25b4..2a8c5670b254 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -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? _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] events: Annotate parent_ctx with __rcu 2020-02-10 9:36 ` Peter Zijlstra @ 2020-02-10 12:59 ` Amol Grover 2020-02-10 13:34 ` Peter Zijlstra 0 siblings, 1 reply; 7+ messages in thread From: Amol Grover @ 2020-02-10 12:59 UTC (permalink / raw) To: Peter Zijlstra Cc: Mark Rutland, Paul E . McKenney, Alexander Shishkin, linux-kernel-mentees, linux-kernel, Arnaldo Carvalho de Melo, Madhuparna Bhowmik, Ingo Molnar, Joel Fernandes, Namhyung Kim, Jiri Olsa On Mon, Feb 10, 2020 at 10:36:24AM +0100, Peter Zijlstra wrote: > On Sat, Feb 08, 2020 at 08:16:49PM +0530, Amol Grover wrote: > > 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. > > > > Signed-off-by: Amol Grover <frextrite@gmail.com> > > --- > > include/linux/perf_event.h | 2 +- > > kernel/events/core.c | 11 ++++++++--- > > 2 files changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > > index 6d4c22aee384..e18a7bdc6f98 100644 > > --- a/include/linux/perf_event.h > > +++ b/include/linux/perf_event.h > > @@ -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; > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > index 2173c23c25b4..2a8c5670b254 100644 > > --- a/kernel/events/core.c > > +++ b/kernel/events/core.c > > @@ -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? 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 Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] events: Annotate parent_ctx with __rcu 2020-02-10 12:59 ` Amol Grover @ 2020-02-10 13:34 ` Peter Zijlstra 2020-02-10 16:47 ` Amol Grover 0 siblings, 1 reply; 7+ messages in thread From: Peter Zijlstra @ 2020-02-10 13:34 UTC (permalink / raw) To: Amol Grover Cc: Mark Rutland, Paul E . McKenney, Alexander Shishkin, linux-kernel-mentees, linux-kernel, Arnaldo Carvalho de Melo, Madhuparna Bhowmik, Ingo Molnar, Joel Fernandes, Namhyung Kim, Jiri Olsa On Mon, Feb 10, 2020 at 06:29:48PM +0530, Amol Grover wrote: > On Mon, Feb 10, 2020 at 10:36:24AM +0100, Peter Zijlstra wrote: > > On Sat, Feb 08, 2020 at 08:16:49PM +0530, Amol Grover wrote: > > > @@ -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? > > Hi Peter, > > Sparse is quite noisy and we need to eliminate false-positives, right? Dunno, I've been happy just ignoring it all. > __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. 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 Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] events: Annotate parent_ctx with __rcu 2020-02-10 13:34 ` Peter Zijlstra @ 2020-02-10 16:47 ` Amol Grover 2020-02-10 17:08 ` Joel Fernandes 0 siblings, 1 reply; 7+ messages in thread From: Amol Grover @ 2020-02-10 16:47 UTC (permalink / raw) To: Peter Zijlstra Cc: Mark Rutland, Paul E . McKenney, Alexander Shishkin, linux-kernel-mentees, linux-kernel, Arnaldo Carvalho de Melo, Madhuparna Bhowmik, Ingo Molnar, Joel Fernandes, Namhyung Kim, Jiri Olsa On Mon, Feb 10, 2020 at 02:34:59PM +0100, Peter Zijlstra wrote: > On Mon, Feb 10, 2020 at 06:29:48PM +0530, Amol Grover wrote: > > On Mon, Feb 10, 2020 at 10:36:24AM +0100, Peter Zijlstra wrote: > > > On Sat, Feb 08, 2020 at 08:16:49PM +0530, Amol Grover wrote: > > > > > @@ -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? > > > > Hi Peter, > > > > Sparse is quite noisy and we need to eliminate false-positives, right? > > Dunno, I've been happy just ignoring it all. > > > __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. > > 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. I might've misinterpreted the code. How does replacing rcu_dereference() with parent_ctx1 = rcu_dereference_protected(ctx1->parent_ctx, lockdep_is_held(&ctx1->lock)); sound? Thanks Amol _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] events: Annotate parent_ctx with __rcu 2020-02-10 16:47 ` Amol Grover @ 2020-02-10 17:08 ` Joel Fernandes 2020-02-13 6:44 ` Amol Grover 0 siblings, 1 reply; 7+ messages in thread From: Joel Fernandes @ 2020-02-10 17:08 UTC (permalink / raw) To: Amol Grover Cc: Mark Rutland, Paul E . McKenney, Madhuparna Bhowmik, Peter Zijlstra, linux-kernel-mentees, linux-kernel, Arnaldo Carvalho de Melo, Alexander Shishkin, Ingo Molnar, Namhyung Kim, Jiri Olsa On Mon, Feb 10, 2020 at 10:17:27PM +0530, Amol Grover wrote: > On Mon, Feb 10, 2020 at 02:34:59PM +0100, Peter Zijlstra wrote: > > On Mon, Feb 10, 2020 at 06:29:48PM +0530, Amol Grover wrote: > > > On Mon, Feb 10, 2020 at 10:36:24AM +0100, Peter Zijlstra wrote: > > > > On Sat, Feb 08, 2020 at 08:16:49PM +0530, Amol Grover wrote: > > > > > > > @@ -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); 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. > > > > > > > > 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). > > > __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. > > > > 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. > > I might've misinterpreted the code. How does replacing rcu_dereference() > with > parent_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 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] events: Annotate parent_ctx with __rcu 2020-02-10 17:08 ` Joel Fernandes @ 2020-02-13 6:44 ` Amol Grover 0 siblings, 0 replies; 7+ messages in thread From: Amol Grover @ 2020-02-13 6:44 UTC (permalink / raw) To: Joel Fernandes Cc: Mark Rutland, Paul E . McKenney, Madhuparna Bhowmik, Peter Zijlstra, linux-kernel-mentees, linux-kernel, Arnaldo Carvalho de Melo, Alexander Shishkin, Ingo Molnar, Namhyung Kim, Jiri Olsa On Mon, Feb 10, 2020 at 12:08:31PM -0500, Joel Fernandes wrote: > On Mon, Feb 10, 2020 at 10:17:27PM +0530, Amol Grover wrote: > > On Mon, Feb 10, 2020 at 02:34:59PM +0100, Peter Zijlstra wrote: > > > On Mon, Feb 10, 2020 at 06:29:48PM +0530, Amol Grover wrote: > > > > On Mon, Feb 10, 2020 at 10:36:24AM +0100, Peter Zijlstra wrote: > > > > > On Sat, Feb 08, 2020 at 08:16:49PM +0530, Amol Grover wrote: > > > > > > > > > @@ -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); > > 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. > Ah yes, I was thinking this aswell. > > > > > > > > > > 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). > > > > > __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. > > > > > > 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. > > > > I might've misinterpreted the code. How does replacing rcu_dereference() > > with > > parent_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. > In that case, I think the code is fine as it is. Thank you for the review both! Thanks Amol > thanks! > > - Joel > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-02-13 6:44 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-08 14:46 [Linux-kernel-mentees] [PATCH] events: Annotate parent_ctx with __rcu Amol Grover 2020-02-10 9:36 ` Peter Zijlstra 2020-02-10 12:59 ` Amol Grover 2020-02-10 13:34 ` Peter Zijlstra 2020-02-10 16:47 ` Amol Grover 2020-02-10 17:08 ` Joel Fernandes 2020-02-13 6:44 ` Amol Grover
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).