* [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).