| Message ID | 1691394685-61240-4-git-send-email-renyu.zj@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add aliases and metrics for Arm CMN | expand |
On 07/08/2023 08:51, Jing Zhang wrote: > The usual event descriptions are "event=xxx" or "config=xxx", while the > event descriptions of CMN are "type=xxx, eventid=xxx" or more complex. > > $cat /sys/bus/event_source/devices/arm_cmn_0/events/hnf_cache_fill > type=0x5,eventid=0x3 > > When adding aliases for events described as "event=xxx" or "config=xxx", > EventCode or ConfigCode can be used in the JSON files to describe the > events. But "eventid=xxx, type=xxx" cannot be supported at present. > > 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 to supplement "eventid=xxx" and "type=xxx", the 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. EventIdCode and Type are added to the event > field, and ConfigCode is moved into the event_field array which can also > guarantee its original function. > > Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com> I'll let Ian check this change as he is more familiar with this code. Thanks
在 2023/8/7 下午5:24, John Garry 写道: > On 07/08/2023 08:51, Jing Zhang wrote: >> The usual event descriptions are "event=xxx" or "config=xxx", while the >> event descriptions of CMN are "type=xxx, eventid=xxx" or more complex. >> >> $cat /sys/bus/event_source/devices/arm_cmn_0/events/hnf_cache_fill >> type=0x5,eventid=0x3 >> >> When adding aliases for events described as "event=xxx" or "config=xxx", >> EventCode or ConfigCode can be used in the JSON files to describe the >> events. But "eventid=xxx, type=xxx" cannot be supported at present. >> >> 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 to supplement "eventid=xxx" and "type=xxx", the 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. EventIdCode and Type are added to the event >> field, and ConfigCode is moved into the event_field array which can also >> guarantee its original function. >> >> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com> > > I'll let Ian check this change as he is more familiar with this code. > Ok, thanks!
在 2023/8/7 下午5:24, John Garry 写道: > On 07/08/2023 08:51, Jing Zhang wrote: >> The usual event descriptions are "event=xxx" or "config=xxx", while the >> event descriptions of CMN are "type=xxx, eventid=xxx" or more complex. >> >> $cat /sys/bus/event_source/devices/arm_cmn_0/events/hnf_cache_fill >> type=0x5,eventid=0x3 >> >> When adding aliases for events described as "event=xxx" or "config=xxx", >> EventCode or ConfigCode can be used in the JSON files to describe the >> events. But "eventid=xxx, type=xxx" cannot be supported at present. >> >> 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 to supplement "eventid=xxx" and "type=xxx", the 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. EventIdCode and Type are added to the event >> field, and ConfigCode is moved into the event_field array which can also >> guarantee its original function. >> >> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com> > > I'll let Ian check this change as he is more familiar with this code. > Hi Ian, Could you please help to review this change？:) Thanks, Jing
On Mon, Aug 7, 2023 at 12:51 AM Jing Zhang <renyu.zj@linux.alibaba.com> wrote: > > The usual event descriptions are "event=xxx" or "config=xxx", while the > event descriptions of CMN are "type=xxx, eventid=xxx" or more complex. I found this difficult to read in relation to the code. Perhaps: 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". I think the use of the name "type" here is unfortunate. It conflicts with the PMU's type as defined in perf_event_attr. In general I think the jevents.py code needs improving, the event_fields dictionary is convoluted, we shouldn't be afraid to change the event json for example to get rid of things like ExtSel, we should really ensure the formats in the events are valid for the PMU they are for. > $cat /sys/bus/event_source/devices/arm_cmn_0/events/hnf_cache_fill > type=0x5,eventid=0x3 > > When adding aliases for events described as "event=xxx" or "config=xxx", > EventCode or ConfigCode can be used in the JSON files to describe the > events. But "eventid=xxx, type=xxx" cannot be supported at present. > > 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 to supplement "eventid=xxx" and "type=xxx", the 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. EventIdCode and Type are added to the event > field, and ConfigCode is moved into the event_field array which can also > guarantee its original function. A useful test can be to build with JEVENTS_ARCH=all and confirm the before and after change generated pmu-events.c is the same. > Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com> > --- > tools/perf/pmu-events/jevents.py | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py > index f57a8f2..9c0f63a 100755 > --- a/tools/perf/pmu-events/jevents.py > +++ b/tools/perf/pmu-events/jevents.py > @@ -275,12 +275,6 @@ class JsonEvent: > } > return table[unit] if unit in table else f'uncore_{unit.lower()}' > > - eventcode = 0 > - if 'EventCode' in jd: > - eventcode = int(jd['EventCode'].split(',', 1)[0], 0) > - if 'ExtSel' in jd: > - 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 = '' > self.compat = jd.get('Compat') > @@ -317,7 +311,15 @@ 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)}' > + eventcode = None > + if 'EventCode' in jd: > + eventcode = int(jd['EventCode'].split(',', 1)[0], 0) > + if 'ExtSel' in jd: > + if eventcode is None: > + eventcode = int(jd['ExtSel']) << 8 > + else: > + eventcode |= int(jd['ExtSel']) << 8 > + event = f'event={llx(eventcode)}' if eventcode is not None else None > event_fields = [ > ('AnyThread', 'any='), > ('PortMask', 'ch_mask='), > @@ -327,10 +329,13 @@ class JsonEvent: > ('Invert', 'inv='), > ('SampleAfterValue', 'period='), > ('UMask', 'umask='), > + ('ConfigCode', 'config='), This loses the int and potential base conversion of ConfigCode. Clearly the code was taking care to maintain this behavior so I suspect this change has broken something. JEVENTS_ARCH=all should reveal the answer. > + ('Type', 'type='), > + ('EventIdCode', 'eventid='), > ] > for key, value in event_fields: > if key in jd and jd[key] != '0': > - event += ',' + value + jd[key] > + event = event + ',' + value + jd[key] if event is not None else value + jd[key] Perhaps initialize event above to the empty string then: if key in jd and jd[key] != '0': if event: event += ',' event += value + jd[key] Thanks, Ian > if filter: > event += f',{filter}' > if msr: > -- > 1.8.3.1 >
在 2023/8/15 上午6:31, Ian Rogers 写道: > On Mon, Aug 7, 2023 at 12:51 AM Jing Zhang <renyu.zj@linux.alibaba.com> wrote: >> >> The usual event descriptions are "event=xxx" or "config=xxx", while the >> event descriptions of CMN are "type=xxx, eventid=xxx" or more complex. > > I found this difficult to read in relation to the code. Perhaps: > > 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". > Thank you for providing a more accurate and readable description. I was indeed struggling with how to describe the problem more accurately before. > I think the use of the name "type" here is unfortunate. It conflicts > with the PMU's type as defined in perf_event_attr. > I agree, but it would require modifying the driver, which is not currently being considered. In the meantime, I can describe the event_field as ('NodeType', 'type='). > In general I think the jevents.py code needs improving, the > event_fields dictionary is convoluted, we shouldn't be afraid to > change the event json for example to get rid of things like ExtSel, we > should really ensure the formats in the events are valid for the PMU > they are for. > >> $cat /sys/bus/event_source/devices/arm_cmn_0/events/hnf_cache_fill >> type=0x5,eventid=0x3 >> >> When adding aliases for events described as "event=xxx" or "config=xxx", >> EventCode or ConfigCode can be used in the JSON files to describe the >> events. But "eventid=xxx, type=xxx" cannot be supported at present. >> >> 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 to supplement "eventid=xxx" and "type=xxx", the 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. EventIdCode and Type are added to the event >> field, and ConfigCode is moved into the event_field array which can also >> guarantee its original function. > > A useful test can be to build with JEVENTS_ARCH=all and confirm the > before and after change generated pmu-events.c is the same. > Okay, I will test it right away. >> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com> >> --- >> tools/perf/pmu-events/jevents.py | 21 +++++++++++++-------- >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py >> index f57a8f2..9c0f63a 100755 >> --- a/tools/perf/pmu-events/jevents.py >> +++ b/tools/perf/pmu-events/jevents.py >> @@ -275,12 +275,6 @@ class JsonEvent: >> } >> return table[unit] if unit in table else f'uncore_{unit.lower()}' >> >> - eventcode = 0 >> - if 'EventCode' in jd: >> - eventcode = int(jd['EventCode'].split(',', 1)[0], 0) >> - if 'ExtSel' in jd: >> - 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 = '' >> self.compat = jd.get('Compat') >> @@ -317,7 +311,15 @@ 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)}' >> + eventcode = None >> + if 'EventCode' in jd: >> + eventcode = int(jd['EventCode'].split(',', 1)[0], 0) >> + if 'ExtSel' in jd: >> + if eventcode is None: >> + eventcode = int(jd['ExtSel']) << 8 >> + else: >> + eventcode |= int(jd['ExtSel']) << 8 >> + event = f'event={llx(eventcode)}' if eventcode is not None else None >> event_fields = [ >> ('AnyThread', 'any='), >> ('PortMask', 'ch_mask='), >> @@ -327,10 +329,13 @@ class JsonEvent: >> ('Invert', 'inv='), >> ('SampleAfterValue', 'period='), >> ('UMask', 'umask='), >> + ('ConfigCode', 'config='), > > This loses the int and potential base conversion of ConfigCode. > Clearly the code was taking care to maintain this behavior so I > suspect this change has broken something. JEVENTS_ARCH=all should > reveal the answer. > You are correct, I compared the generated pmu-events.c files before and after, and they are indeed different, with before:config=0x5 vs after:config=0x05. I will keep the original way of handling ConfigCode in the next version. >> + ('Type', 'type='), >> + ('EventIdCode', 'eventid='), >> ] >> for key, value in event_fields: >> if key in jd and jd[key] != '0': >> - event += ',' + value + jd[key] >> + event = event + ',' + value + jd[key] if event is not None else value + jd[key] > > Perhaps initialize event above to the empty string then: > > if key in jd and jd[key] != '0': > if event: > event += ',' > event += value + jd[key] > If the event is None, the statement event += value + jd[key] would result in an error. So, maybe I can use the following way: if event: event += ',' + value + jd[key] else: event = value + jd[key] Thanks, Jing
diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py index f57a8f2..9c0f63a 100755 --- a/tools/perf/pmu-events/jevents.py +++ b/tools/perf/pmu-events/jevents.py @@ -275,12 +275,6 @@ class JsonEvent: } return table[unit] if unit in table else f'uncore_{unit.lower()}' - eventcode = 0 - if 'EventCode' in jd: - eventcode = int(jd['EventCode'].split(',', 1)[0], 0) - if 'ExtSel' in jd: - 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 = '' self.compat = jd.get('Compat') @@ -317,7 +311,15 @@ 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)}' + eventcode = None + if 'EventCode' in jd: + eventcode = int(jd['EventCode'].split(',', 1)[0], 0) + if 'ExtSel' in jd: + if eventcode is None: + eventcode = int(jd['ExtSel']) << 8 + else: + eventcode |= int(jd['ExtSel']) << 8 + event = f'event={llx(eventcode)}' if eventcode is not None else None event_fields = [ ('AnyThread', 'any='), ('PortMask', 'ch_mask='), @@ -327,10 +329,13 @@ class JsonEvent: ('Invert', 'inv='), ('SampleAfterValue', 'period='), ('UMask', 'umask='), + ('ConfigCode', 'config='), + ('Type', 'type='), + ('EventIdCode', 'eventid='), ] for key, value in event_fields: if key in jd and jd[key] != '0': - event += ',' + value + jd[key] + event = event + ',' + value + jd[key] if event is not None else value + jd[key] if filter: event += f',{filter}' if msr:
The usual event descriptions are "event=xxx" or "config=xxx", while the event descriptions of CMN are "type=xxx, eventid=xxx" or more complex. $cat /sys/bus/event_source/devices/arm_cmn_0/events/hnf_cache_fill type=0x5,eventid=0x3 When adding aliases for events described as "event=xxx" or "config=xxx", EventCode or ConfigCode can be used in the JSON files to describe the events. But "eventid=xxx, type=xxx" cannot be supported at present. 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 to supplement "eventid=xxx" and "type=xxx", the 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. EventIdCode and Type are added to the event field, and ConfigCode is moved into the event_field array which can also guarantee its original function. Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com> --- tools/perf/pmu-events/jevents.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)