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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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?
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 <[email protected]>
> > ---
> > 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
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.
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
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
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
>