| Message ID | 1692606977-92009-5-git-send-email-renyu.zj@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf vendor events: Add JSON metrics for Arm CMN | expand |
On 2023-08-21 09:36, Jing Zhang wrote: > The previous code assumes an event has either an "event=" or "config" > field at the beginning. For CMN neither of these may be present, as an > event is typically "type=xx,eventid=xxx". > > If EventCode and ConfigCode is not added in the alias JSON file, the > event description will add "event=0" by default. So, even if the event > field is added "eventid=xxx" and "type=xxx", the CMN events final > parsing result will be "event=0, eventid=xxx, type=xxx". > > Therefore, when EventCode and ConfigCode are missing in JSON, "event=0" > is no longer added by default. And add EventIdCode and Type to the event > field. > > I compared pmu_event.c before and after compiling with JEVENT_ARCH=all, > they are consistent. > > Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com> > --- > tools/perf/pmu-events/jevents.py | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py > index f57a8f2..369c8bf 100755 > --- a/tools/perf/pmu-events/jevents.py > +++ b/tools/perf/pmu-events/jevents.py > @@ -275,11 +275,14 @@ class JsonEvent: > } > return table[unit] if unit in table else f'uncore_{unit.lower()}' > > - eventcode = 0 > + eventcode = None > if 'EventCode' in jd: > eventcode = int(jd['EventCode'].split(',', 1)[0], 0) > if 'ExtSel' in jd: > - eventcode |= int(jd['ExtSel']) << 8 > + if eventcode is None: > + eventcode = int(jd['ExtSel']) << 8 > + else: > + eventcode |= int(jd['ExtSel']) << 8 > configcode = int(jd['ConfigCode'], 0) if 'ConfigCode' in jd else None > self.name = jd['EventName'].lower() if 'EventName' in jd else None > self.topic = '' > @@ -317,7 +320,11 @@ class JsonEvent: > if precise and self.desc and '(Precise Event)' not in self.desc: > extra_desc += ' (Must be precise)' if precise == '2' else (' (Precise ' > 'event)') > - event = f'config={llx(configcode)}' if configcode is not None else f'event={llx(eventcode)}' > + event = None > + if eventcode is not None: > + event = f'event={llx(eventcode)}' > + elif configcode is not None: > + event = f'config={llx(configcode)}' > event_fields = [ > ('AnyThread', 'any='), > ('PortMask', 'ch_mask='), > @@ -327,10 +334,15 @@ class JsonEvent: > ('Invert', 'inv='), > ('SampleAfterValue', 'period='), > ('UMask', 'umask='), > + ('NodeType', 'type='), > + ('EventIdCode', 'eventid='), FWIW, this smells like another brewing scalability problem, given that these are entirely driver-specific. Not sure off-hand how feasible it might be, but my instinct says that a neat solution would be to encode them right in the JSON, e.g.: "FormatAttr": { "type": 0x5 } such that jevents should then only really need to consider whether an event is defined in terms of a raw "ConfigCode", one or more "FormatAttr"s which it can then parse dynamically, or reasonable special cases like "EventCode" (given how "event" is one of the most commonly used formats). Thanks, Robin. > ] > for key, value in event_fields: > if key in jd and jd[key] != '0': > - event += ',' + value + jd[key] > + if event: > + event += ',' + value + jd[key] > + else: > + event = value + jd[key] > if filter: > event += f',{filter}' > if msr:
On Wed, Aug 23, 2023 at 2:12 AM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2023-08-21 09:36, Jing Zhang wrote: > > The previous code assumes an event has either an "event=" or "config" > > field at the beginning. For CMN neither of these may be present, as an > > event is typically "type=xx,eventid=xxx". > > > > If EventCode and ConfigCode is not added in the alias JSON file, the > > event description will add "event=0" by default. So, even if the event > > field is added "eventid=xxx" and "type=xxx", the CMN events final > > parsing result will be "event=0, eventid=xxx, type=xxx". > > > > Therefore, when EventCode and ConfigCode are missing in JSON, "event=0" > > is no longer added by default. And add EventIdCode and Type to the event > > field. > > > > I compared pmu_event.c before and after compiling with JEVENT_ARCH=all, > > they are consistent. > > > > Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com> > > --- > > tools/perf/pmu-events/jevents.py | 20 ++++++++++++++++---- > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py > > index f57a8f2..369c8bf 100755 > > --- a/tools/perf/pmu-events/jevents.py > > +++ b/tools/perf/pmu-events/jevents.py > > @@ -275,11 +275,14 @@ class JsonEvent: > > } > > return table[unit] if unit in table else f'uncore_{unit.lower()}' > > > > - eventcode = 0 > > + eventcode = None > > if 'EventCode' in jd: > > eventcode = int(jd['EventCode'].split(',', 1)[0], 0) > > if 'ExtSel' in jd: > > - eventcode |= int(jd['ExtSel']) << 8 > > + if eventcode is None: > > + eventcode = int(jd['ExtSel']) << 8 > > + else: > > + eventcode |= int(jd['ExtSel']) << 8 > > configcode = int(jd['ConfigCode'], 0) if 'ConfigCode' in jd else None > > self.name = jd['EventName'].lower() if 'EventName' in jd else None > > self.topic = '' > > @@ -317,7 +320,11 @@ class JsonEvent: > > if precise and self.desc and '(Precise Event)' not in self.desc: > > extra_desc += ' (Must be precise)' if precise == '2' else (' (Precise ' > > 'event)') > > - event = f'config={llx(configcode)}' if configcode is not None else f'event={llx(eventcode)}' > > + event = None > > + if eventcode is not None: > > + event = f'event={llx(eventcode)}' > > + elif configcode is not None: > > + event = f'config={llx(configcode)}' > > event_fields = [ > > ('AnyThread', 'any='), > > ('PortMask', 'ch_mask='), > > @@ -327,10 +334,15 @@ class JsonEvent: > > ('Invert', 'inv='), > > ('SampleAfterValue', 'period='), > > ('UMask', 'umask='), > > + ('NodeType', 'type='), > > + ('EventIdCode', 'eventid='), > > FWIW, this smells like another brewing scalability problem, given that > these are entirely driver-specific. Not sure off-hand how feasible it > might be, but my instinct says that a neat solution would be to encode > them right in the JSON, e.g.: > > "FormatAttr": { "type": 0x5 } > > such that jevents should then only really need to consider whether an > event is defined in terms of a raw "ConfigCode", one or more > "FormatAttr"s which it can then parse dynamically, or reasonable special > cases like "EventCode" (given how "event" is one of the most commonly > used formats). Hi Robin, I'm not sure about scalability but I think it is a problem that we encode names into the event that should correspond with formats, but we don't test that the PMU driver in question supports the format. If we tar up sysfs directories I think we can check/test this and it makes sense for the perf tool to be able to parse a sysfs type directory structure. I think the hard coded names and matching to dictionary entries is suboptimal, we should really be able to know the formats, abstract this, etc. The code is deliberately done this way so that we could migrate away from a legacy C version of this code and generate an identical pmu-events.c. As much as is possible the python code matches the C code, but now the transition has happened there is no reason to maintain this behavior. Thanks, Ian > Thanks, > Robin. > > > ] > > for key, value in event_fields: > > if key in jd and jd[key] != '0': > > - event += ',' + value + jd[key] > > + if event: > > + event += ',' + value + jd[key] > > + else: > > + event = value + jd[key] > > if filter: > > event += f',{filter}' > > if msr:
diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py index f57a8f2..369c8bf 100755 --- a/tools/perf/pmu-events/jevents.py +++ b/tools/perf/pmu-events/jevents.py @@ -275,11 +275,14 @@ class JsonEvent: } return table[unit] if unit in table else f'uncore_{unit.lower()}' - eventcode = 0 + eventcode = None if 'EventCode' in jd: eventcode = int(jd['EventCode'].split(',', 1)[0], 0) if 'ExtSel' in jd: - eventcode |= int(jd['ExtSel']) << 8 + if eventcode is None: + eventcode = int(jd['ExtSel']) << 8 + else: + eventcode |= int(jd['ExtSel']) << 8 configcode = int(jd['ConfigCode'], 0) if 'ConfigCode' in jd else None self.name = jd['EventName'].lower() if 'EventName' in jd else None self.topic = '' @@ -317,7 +320,11 @@ class JsonEvent: if precise and self.desc and '(Precise Event)' not in self.desc: extra_desc += ' (Must be precise)' if precise == '2' else (' (Precise ' 'event)') - event = f'config={llx(configcode)}' if configcode is not None else f'event={llx(eventcode)}' + event = None + if eventcode is not None: + event = f'event={llx(eventcode)}' + elif configcode is not None: + event = f'config={llx(configcode)}' event_fields = [ ('AnyThread', 'any='), ('PortMask', 'ch_mask='), @@ -327,10 +334,15 @@ class JsonEvent: ('Invert', 'inv='), ('SampleAfterValue', 'period='), ('UMask', 'umask='), + ('NodeType', 'type='), + ('EventIdCode', 'eventid='), ] for key, value in event_fields: if key in jd and jd[key] != '0': - event += ',' + value + jd[key] + if event: + event += ',' + value + jd[key] + else: + event = value + jd[key] if filter: event += f',{filter}' if msr:
The previous code assumes an event has either an "event=" or "config" field at the beginning. For CMN neither of these may be present, as an event is typically "type=xx,eventid=xxx". If EventCode and ConfigCode is not added in the alias JSON file, the event description will add "event=0" by default. So, even if the event field is added "eventid=xxx" and "type=xxx", the CMN events final parsing result will be "event=0, eventid=xxx, type=xxx". Therefore, when EventCode and ConfigCode are missing in JSON, "event=0" is no longer added by default. And add EventIdCode and Type to the event field. I compared pmu_event.c before and after compiling with JEVENT_ARCH=all, they are consistent. Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com> --- tools/perf/pmu-events/jevents.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)