| Message ID | 20240514180050.182454-1-namhyung@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf/arm-dmc620: Fix lockdep assert in ->event_init() | expand |
On 2024-05-14 7:00 pm, Namhyung Kim wrote: > for_each_sibling_event() checks leader's ctx but it doesn't have the ctx > yet if it's the leader. Like in perf_event_validate_size(), we should > skip checking siblings in that case. Ugh, looking around for_each_sibling_event() sites, it looks like there are a fair few other drivers using this pattern as well :( I'd love for groups to be less horribly complicated, but I think I can follow the underlying reasoning here. I suppose one could argue that the assertion could take into account that there's nothing to protect in the case where event->ctx is still NULL, since nobody else should be able to touch the event's own empty sibling list at this point before perf_event_open() has even returned. However by the same token there's also no real reason for drivers *not* to return early when they equally can tell that the sibling list must be empty, and indeed that seems to be a fairly common pattern too, so I see no issue with fixing up all the offending drivers for consistency either. Reviewed-by: Robin Murphy <robin.murphy@arm.com> > Fixes: f3c0eba287049 ("perf: Add a few assertions") > Reported-by: Greg Thelen <gthelen@google.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Tuan Phan <tuanphan@os.amperecomputing.com> > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > drivers/perf/arm_dmc620_pmu.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/perf/arm_dmc620_pmu.c b/drivers/perf/arm_dmc620_pmu.c > index 8a81be2dd5ec..88c17c1d6d49 100644 > --- a/drivers/perf/arm_dmc620_pmu.c > +++ b/drivers/perf/arm_dmc620_pmu.c > @@ -542,12 +542,16 @@ static int dmc620_pmu_event_init(struct perf_event *event) > if (event->cpu < 0) > return -EINVAL; > > + hwc->idx = -1; > + > + if (event->group_leader == event) > + return 0; > + > /* > * We can't atomically disable all HW counters so only one event allowed, > * although software events are acceptable. > */ > - if (event->group_leader != event && > - !is_software_event(event->group_leader)) > + if (!is_software_event(event->group_leader)) > return -EINVAL; > > for_each_sibling_event(sibling, event->group_leader) { > @@ -556,7 +560,6 @@ static int dmc620_pmu_event_init(struct perf_event *event) > return -EINVAL; > } > > - hwc->idx = -1; > return 0; > } >
On Tue, May 14, 2024 at 11:00:50AM -0700, Namhyung Kim wrote: > for_each_sibling_event() checks leader's ctx but it doesn't have the ctx > yet if it's the leader. Like in perf_event_validate_size(), we should > skip checking siblings in that case. > > Fixes: f3c0eba287049 ("perf: Add a few assertions") > Reported-by: Greg Thelen <gthelen@google.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Tuan Phan <tuanphan@os.amperecomputing.com> > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > drivers/perf/arm_dmc620_pmu.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/perf/arm_dmc620_pmu.c b/drivers/perf/arm_dmc620_pmu.c > index 8a81be2dd5ec..88c17c1d6d49 100644 > --- a/drivers/perf/arm_dmc620_pmu.c > +++ b/drivers/perf/arm_dmc620_pmu.c > @@ -542,12 +542,16 @@ static int dmc620_pmu_event_init(struct perf_event *event) > if (event->cpu < 0) > return -EINVAL; > > + hwc->idx = -1; > + > + if (event->group_leader == event) > + return 0; > + > /* > * We can't atomically disable all HW counters so only one event allowed, > * although software events are acceptable. > */ > - if (event->group_leader != event && > - !is_software_event(event->group_leader)) > + if (!is_software_event(event->group_leader)) > return -EINVAL; > > for_each_sibling_event(sibling, event->group_leader) { > @@ -556,7 +560,6 @@ static int dmc620_pmu_event_init(struct perf_event *event) > return -EINVAL; > } > > - hwc->idx = -1; > return 0; > } Thanks, I'll pick this up, although Mark reckoned he'd found some other issues over at: https://lore.kernel.org/r/Zg0l642PgQ7T3a8Z@FVFF77S0Q05N but didn't elaborate on what exactly he'd found :/ Will
On 17/05/2024 1:02 pm, Will Deacon wrote: > On Tue, May 14, 2024 at 11:00:50AM -0700, Namhyung Kim wrote: >> for_each_sibling_event() checks leader's ctx but it doesn't have the ctx >> yet if it's the leader. Like in perf_event_validate_size(), we should >> skip checking siblings in that case. >> >> Fixes: f3c0eba287049 ("perf: Add a few assertions") >> Reported-by: Greg Thelen <gthelen@google.com> >> Cc: Robin Murphy <robin.murphy@arm.com> >> Cc: Tuan Phan <tuanphan@os.amperecomputing.com> >> Signed-off-by: Namhyung Kim <namhyung@kernel.org> >> --- >> drivers/perf/arm_dmc620_pmu.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/perf/arm_dmc620_pmu.c b/drivers/perf/arm_dmc620_pmu.c >> index 8a81be2dd5ec..88c17c1d6d49 100644 >> --- a/drivers/perf/arm_dmc620_pmu.c >> +++ b/drivers/perf/arm_dmc620_pmu.c >> @@ -542,12 +542,16 @@ static int dmc620_pmu_event_init(struct perf_event *event) >> if (event->cpu < 0) >> return -EINVAL; >> >> + hwc->idx = -1; >> + >> + if (event->group_leader == event) >> + return 0; >> + >> /* >> * We can't atomically disable all HW counters so only one event allowed, >> * although software events are acceptable. >> */ >> - if (event->group_leader != event && >> - !is_software_event(event->group_leader)) >> + if (!is_software_event(event->group_leader)) >> return -EINVAL; Oh, come to think of it, I believe we shouldn't actually need to keep this check either, since commit bf480f938566 ("perf/core: Don't allow grouping events from different hw pmus"). Thanks, Robin. >> >> for_each_sibling_event(sibling, event->group_leader) { >> @@ -556,7 +560,6 @@ static int dmc620_pmu_event_init(struct perf_event *event) >> return -EINVAL; >> } >> >> - hwc->idx = -1; >> return 0; >> } > > Thanks, I'll pick this up, although Mark reckoned he'd found some other > issues over at: > > https://lore.kernel.org/r/Zg0l642PgQ7T3a8Z@FVFF77S0Q05N > > but didn't elaborate on what exactly he'd found :/ > > Will > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, May 17, 2024 at 01:02:34PM +0100, Will Deacon wrote: > On Tue, May 14, 2024 at 11:00:50AM -0700, Namhyung Kim wrote: > > for_each_sibling_event() checks leader's ctx but it doesn't have the ctx > > yet if it's the leader. Like in perf_event_validate_size(), we should > > skip checking siblings in that case. > > > > Fixes: f3c0eba287049 ("perf: Add a few assertions") > > Reported-by: Greg Thelen <gthelen@google.com> > > Cc: Robin Murphy <robin.murphy@arm.com> > > Cc: Tuan Phan <tuanphan@os.amperecomputing.com> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > --- > > drivers/perf/arm_dmc620_pmu.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/perf/arm_dmc620_pmu.c b/drivers/perf/arm_dmc620_pmu.c > > index 8a81be2dd5ec..88c17c1d6d49 100644 > > --- a/drivers/perf/arm_dmc620_pmu.c > > +++ b/drivers/perf/arm_dmc620_pmu.c > > @@ -542,12 +542,16 @@ static int dmc620_pmu_event_init(struct perf_event *event) > > if (event->cpu < 0) > > return -EINVAL; > > > > + hwc->idx = -1; > > + > > + if (event->group_leader == event) > > + return 0; > > + > > /* > > * We can't atomically disable all HW counters so only one event allowed, > > * although software events are acceptable. > > */ > > - if (event->group_leader != event && > > - !is_software_event(event->group_leader)) > > + if (!is_software_event(event->group_leader)) > > return -EINVAL; > > > > for_each_sibling_event(sibling, event->group_leader) { > > @@ -556,7 +560,6 @@ static int dmc620_pmu_event_init(struct perf_event *event) > > return -EINVAL; > > } > > > > - hwc->idx = -1; > > return 0; > > } > > Thanks, I'll pick this up, although Mark reckoned he'd found some other > issues over at: > > https://lore.kernel.org/r/Zg0l642PgQ7T3a8Z@FVFF77S0Q05N > > but didn't elaborate on what exactly he'd found :/ Sorry; what I was referring to was that some drivers (including this one) also forgot to validate that the group size could actually fit on the PMU, which we're *supposed* to check, but if we fail to do so the only fallout is that events won't count and we waste a bit of time trying to schedule events unnecessarily. For this patch as-is: Acked-by: Mark Rutland <mark.rutland@arm.com> ... and I'll try to take a look at the rest when I'm back in the UK. Mark.
On Tue, 14 May 2024 11:00:50 -0700, Namhyung Kim wrote: > for_each_sibling_event() checks leader's ctx but it doesn't have the ctx > yet if it's the leader. Like in perf_event_validate_size(), we should > skip checking siblings in that case. > > Applied to arm64 (for-next/core), thanks! [1/1] perf/arm-dmc620: Fix lockdep assert in ->event_init() https://git.kernel.org/arm64/c/a4c5a457c610 Cheers,
diff --git a/drivers/perf/arm_dmc620_pmu.c b/drivers/perf/arm_dmc620_pmu.c index 8a81be2dd5ec..88c17c1d6d49 100644 --- a/drivers/perf/arm_dmc620_pmu.c +++ b/drivers/perf/arm_dmc620_pmu.c @@ -542,12 +542,16 @@ static int dmc620_pmu_event_init(struct perf_event *event) if (event->cpu < 0) return -EINVAL; + hwc->idx = -1; + + if (event->group_leader == event) + return 0; + /* * We can't atomically disable all HW counters so only one event allowed, * although software events are acceptable. */ - if (event->group_leader != event && - !is_software_event(event->group_leader)) + if (!is_software_event(event->group_leader)) return -EINVAL; for_each_sibling_event(sibling, event->group_leader) { @@ -556,7 +560,6 @@ static int dmc620_pmu_event_init(struct perf_event *event) return -EINVAL; } - hwc->idx = -1; return 0; }
for_each_sibling_event() checks leader's ctx but it doesn't have the ctx yet if it's the leader. Like in perf_event_validate_size(), we should skip checking siblings in that case. Fixes: f3c0eba287049 ("perf: Add a few assertions") Reported-by: Greg Thelen <gthelen@google.com> Cc: Robin Murphy <robin.murphy@arm.com> Cc: Tuan Phan <tuanphan@os.amperecomputing.com> Signed-off-by: Namhyung Kim <namhyung@kernel.org> --- drivers/perf/arm_dmc620_pmu.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)