Recently Ilkka reported that the JSONs for the AmpereOne arm64-based
platform included a dud event which referenced a non-existent arch std
event [0].
Previously in the times of jevents.c, we would raise an exception for this.
This is still invalid, even though the current code just ignores such an
event.
Re-introduce code to raise an exception for when no definition exists to
help catch as many invalid JSONs as possible.
[0] https://lore.kernel.org/linux-perf-users/[email protected]/
Signed-off-by: John Garry <[email protected]>
---
Please do not apply before [0], above.
diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 8cd561aa606a..98cccc3fcbbd 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -347,12 +347,15 @@ class JsonEvent:
if self.desc and not self.desc.endswith('. '):
self.desc += '. '
self.desc = (self.desc if self.desc else '') + ('Unit: ' + self.pmu + ' ')
- if arch_std and arch_std.lower() in _arch_std_events:
- event = _arch_std_events[arch_std.lower()].event
- # Copy from the architecture standard event to self for undefined fields.
- for attr, value in _arch_std_events[arch_std.lower()].__dict__.items():
- if hasattr(self, attr) and not getattr(self, attr):
- setattr(self, attr, value)
+ if arch_std:
+ if arch_std.lower() in _arch_std_events:
+ event = _arch_std_events[arch_std.lower()].event
+ # Copy from the architecture standard event to self for undefined fields.
+ for attr, value in _arch_std_events[arch_std.lower()].__dict__.items():
+ if hasattr(self, attr) and not getattr(self, attr):
+ setattr(self, attr, value)
+ else:
+ raise argparse.ArgumentTypeError('Cannot find arch std event:', arch_std)
self.event = real_event(self.name, event)
--
2.35.3
Hi John,
On Mon, 7 Aug 2023, John Garry wrote:
> Recently Ilkka reported that the JSONs for the AmpereOne arm64-based
> platform included a dud event which referenced a non-existent arch std
> event [0].
I wish I had found the bug in my patch a long time ago but, in fact, it
was Dave Kleikamp who initially pointed it out to me and figured out the
difference between jevents.c and jevents.py when porting the patch to 5.15
kernel.
http://lists.infradead.org/pipermail/linux-arm-kernel/2023-June/844874.html
>
> Previously in the times of jevents.c, we would raise an exception for this.
>
> This is still invalid, even though the current code just ignores such an
> event.
>
> Re-introduce code to raise an exception for when no definition exists to
> help catch as many invalid JSONs as possible.
>
> [0] https://lore.kernel.org/linux-perf-users/[email protected]/
>
> Signed-off-by: John Garry <[email protected]>
Thanks for the patch! I quickly tested it and it worked as expected. Just
in case this is needed:
Tested-by: Ilkka Koskinen <[email protected]>
Cheers, Ilkka
> ---
> Please do not apply before [0], above.
>
> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 8cd561aa606a..98cccc3fcbbd 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
> @@ -347,12 +347,15 @@ class JsonEvent:
> if self.desc and not self.desc.endswith('. '):
> self.desc += '. '
> self.desc = (self.desc if self.desc else '') + ('Unit: ' + self.pmu + ' ')
> - if arch_std and arch_std.lower() in _arch_std_events:
> - event = _arch_std_events[arch_std.lower()].event
> - # Copy from the architecture standard event to self for undefined fields.
> - for attr, value in _arch_std_events[arch_std.lower()].__dict__.items():
> - if hasattr(self, attr) and not getattr(self, attr):
> - setattr(self, attr, value)
> + if arch_std:
> + if arch_std.lower() in _arch_std_events:
> + event = _arch_std_events[arch_std.lower()].event
> + # Copy from the architecture standard event to self for undefined fields.
> + for attr, value in _arch_std_events[arch_std.lower()].__dict__.items():
> + if hasattr(self, attr) and not getattr(self, attr):
> + setattr(self, attr, value)
> + else:
> + raise argparse.ArgumentTypeError('Cannot find arch std event:', arch_std)
>
> self.event = real_event(self.name, event)
>
> --
> 2.35.3
>
>
On Wed, Aug 9, 2023 at 10:11 PM Ilkka Koskinen
<[email protected]> wrote:
>
>
> Hi John,
>
> On Mon, 7 Aug 2023, John Garry wrote:
> > Recently Ilkka reported that the JSONs for the AmpereOne arm64-based
> > platform included a dud event which referenced a non-existent arch std
> > event [0].
>
> I wish I had found the bug in my patch a long time ago but, in fact, it
> was Dave Kleikamp who initially pointed it out to me and figured out the
> difference between jevents.c and jevents.py when porting the patch to 5.15
> kernel.
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2023-June/844874.html
>
> >
> > Previously in the times of jevents.c, we would raise an exception for this.
> >
> > This is still invalid, even though the current code just ignores such an
> > event.
> >
> > Re-introduce code to raise an exception for when no definition exists to
> > help catch as many invalid JSONs as possible.
> >
> > [0] https://lore.kernel.org/linux-perf-users/[email protected]/
> >
> > Signed-off-by: John Garry <[email protected]>
>
> Thanks for the patch! I quickly tested it and it worked as expected. Just
> in case this is needed:
>
> Tested-by: Ilkka Koskinen <[email protected]>
Reviewed-by: Ian Rogers <[email protected]>
Thanks,
Ian
> Cheers, Ilkka
>
> > ---
> > Please do not apply before [0], above.
> >
> > diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> > index 8cd561aa606a..98cccc3fcbbd 100755
> > --- a/tools/perf/pmu-events/jevents.py
> > +++ b/tools/perf/pmu-events/jevents.py
> > @@ -347,12 +347,15 @@ class JsonEvent:
> > if self.desc and not self.desc.endswith('. '):
> > self.desc += '. '
> > self.desc = (self.desc if self.desc else '') + ('Unit: ' + self.pmu + ' ')
> > - if arch_std and arch_std.lower() in _arch_std_events:
> > - event = _arch_std_events[arch_std.lower()].event
> > - # Copy from the architecture standard event to self for undefined fields.
> > - for attr, value in _arch_std_events[arch_std.lower()].__dict__.items():
> > - if hasattr(self, attr) and not getattr(self, attr):
> > - setattr(self, attr, value)
> > + if arch_std:
> > + if arch_std.lower() in _arch_std_events:
> > + event = _arch_std_events[arch_std.lower()].event
> > + # Copy from the architecture standard event to self for undefined fields.
> > + for attr, value in _arch_std_events[arch_std.lower()].__dict__.items():
> > + if hasattr(self, attr) and not getattr(self, attr):
> > + setattr(self, attr, value)
> > + else:
> > + raise argparse.ArgumentTypeError('Cannot find arch std event:', arch_std)
> >
> > self.event = real_event(self.name, event)
> >
> > --
> > 2.35.3
> >
> >
Em Mon, Aug 21, 2023 at 11:46:20AM +0100, John Garry escreveu:
> On 10/08/2023 20:27, Ian Rogers wrote:
> > On Wed, Aug 9, 2023 at 10:11 PM Ilkka Koskinen
> > <[email protected]> wrote:
> > >
> > >
> > > Hi John,
> > >
> > > On Mon, 7 Aug 2023, John Garry wrote:
> > > > Recently Ilkka reported that the JSONs for the AmpereOne arm64-based
> > > > platform included a dud event which referenced a non-existent arch std
> > > > event [0].
> > >
> > > I wish I had found the bug in my patch a long time ago but, in fact, it
> > > was Dave Kleikamp who initially pointed it out to me and figured out the
> > > difference between jevents.c and jevents.py when porting the patch to 5.15
> > > kernel.
> > >
> > > https://urldefense.com/v3/__http://lists.infradead.org/pipermail/linux-arm-kernel/2023-June/844874.html__;!!ACWV5N9M2RV99hQ!It9EhKK4s2uBUJyQvLg-ruUfENAA6Sw7TWVo_hF8XmFoQ6q565iYafTnN-yoBNh3EQ1IFa2tHknUjNHs5dI$
> > >
> > > >
> > > > Previously in the times of jevents.c, we would raise an exception for this.
> > > >
> > > > This is still invalid, even though the current code just ignores such an
> > > > event.
> > > >
> > > > Re-introduce code to raise an exception for when no definition exists to
> > > > help catch as many invalid JSONs as possible.
> > > >
> > > > [0] https://urldefense.com/v3/__https://lore.kernel.org/linux-perf-users/[email protected]/__;!!ACWV5N9M2RV99hQ!It9EhKK4s2uBUJyQvLg-ruUfENAA6Sw7TWVo_hF8XmFoQ6q565iYafTnN-yoBNh3EQ1IFa2tHknU_t28TiE$
> > > >
> > > > Signed-off-by: John Garry <[email protected]>
> > >
> > > Thanks for the patch! I quickly tested it and it worked as expected. Just
> > > in case this is needed:
> > >
> > > Tested-by: Ilkka Koskinen <[email protected]>
> >
> > Reviewed-by: Ian Rogers <[email protected]>
>
> Hi Arnaldo,
>
> Can you consider applying this patch along with https://lore.kernel.org/linux-perf-users/CAP-5=fX2uE=B_Vb90nn5EV0mw+AJBpjDecP9w29OUn=j7HKPPg@mail.gmail.com/
>
> I think that we should expect another version of that series with changes
> elsewhere.
Done, thanks for the ping.
- Arnaldo
> Thanks,
> John
>
> >
> > Thanks,
> > Ian
> >
> > > Cheers, Ilkka
> > >
> > > > ---
> > > > Please do not apply before [0], above.
> > > >
> > > > diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> > > > index 8cd561aa606a..98cccc3fcbbd 100755
> > > > --- a/tools/perf/pmu-events/jevents.py
> > > > +++ b/tools/perf/pmu-events/jevents.py
> > > > @@ -347,12 +347,15 @@ class JsonEvent:
> > > > if self.desc and not self.desc.endswith('. '):
> > > > self.desc += '. '
> > > > self.desc = (self.desc if self.desc else '') + ('Unit: ' + self.pmu + ' ')
> > > > - if arch_std and arch_std.lower() in _arch_std_events:
> > > > - event = _arch_std_events[arch_std.lower()].event
> > > > - # Copy from the architecture standard event to self for undefined fields.
> > > > - for attr, value in _arch_std_events[arch_std.lower()].__dict__.items():
> > > > - if hasattr(self, attr) and not getattr(self, attr):
> > > > - setattr(self, attr, value)
> > > > + if arch_std:
> > > > + if arch_std.lower() in _arch_std_events:
> > > > + event = _arch_std_events[arch_std.lower()].event
> > > > + # Copy from the architecture standard event to self for undefined fields.
> > > > + for attr, value in _arch_std_events[arch_std.lower()].__dict__.items():
> > > > + if hasattr(self, attr) and not getattr(self, attr):
> > > > + setattr(self, attr, value)
> > > > + else:
> > > > + raise argparse.ArgumentTypeError('Cannot find arch std event:', arch_std)
> > > >
> > > > self.event = real_event(self.name, event)
> > > >
> > > > --
> > > > 2.35.3
> > > >
> > > >
>
--
- Arnaldo