Changes since v4:
- Support "Compat" to match multiple identifiers for regular events
aliases as suggested by John.
- Add self-test cases for "Compat" new feature and new event_filed
as suggested by John.
- Link: https://lore.kernel.org/all/[email protected]/
Jing Zhang (5):
perf metric: Event "Compat" value supports matching multiple
identifiers
perf jevents: Support more event fields
perf test: Add pmu-event test for "Compat" and new event_field.
perf jevents: Add support for Arm CMN PMU aliasing
perf vendor events: Add JSON metrics for Arm CMN
.../pmu-events/arch/arm64/arm/cmn/sys/cmn.json | 266 +++++++++++++++++++++
.../pmu-events/arch/arm64/arm/cmn/sys/metric.json | 74 ++++++
.../pmu-events/arch/test/test_soc/sys/uncore.json | 8 +
tools/perf/pmu-events/jevents.py | 22 +-
tools/perf/tests/pmu-events.c | 55 +++++
tools/perf/util/metricgroup.c | 2 +-
tools/perf/util/pmu.c | 27 ++-
tools/perf/util/pmu.h | 1 +
8 files changed, 445 insertions(+), 10 deletions(-)
create mode 100644 tools/perf/pmu-events/arch/arm64/arm/cmn/sys/cmn.json
create mode 100644 tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metric.json
--
1.8.3.1
The jevent "Compat" is used for uncore PMU alias or metric definitions.
The same PMU driver has different PMU identifiers due to different hardware
versions and types, but they may have some common PMU event/metric. Since a
Compat value can only match one identifier, when adding the same event
alias and metric to PMUs with different identifiers, each identifier needs
to be defined once, which is not streamlined enough.
So let "Compat" value supports matching multiple identifiers. For example,
the Compat value {abcde;123*} can match the PMU identifier "abcde" and the
the PMU identifier with the prefix "123", where "*" is a wildcard.
Tokens in Unit field are delimited by ';' with no spaces.
Signed-off-by: Jing Zhang <[email protected]>
---
tools/perf/util/metricgroup.c | 2 +-
tools/perf/util/pmu.c | 27 ++++++++++++++++++++++++++-
tools/perf/util/pmu.h | 1 +
3 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 5e9c657..ff81bc5 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -477,7 +477,7 @@ static int metricgroup__sys_event_iter(const struct pmu_metric *pm,
while ((pmu = perf_pmu__scan(pmu))) {
- if (!pmu->id || strcmp(pmu->id, pm->compat))
+ if (!pmu->id || !pmu_uncore_identifier_match(pmu->id, pm->compat))
continue;
return d->fn(pm, table, d->data);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index ad209c8..3ae249b 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -776,6 +776,31 @@ static bool pmu_uncore_alias_match(const char *pmu_name, const char *name)
return res;
}
+bool pmu_uncore_identifier_match(const char *id, const char *compat)
+{
+ char *tmp = NULL, *tok, *str;
+ bool res;
+ int n;
+
+ str = strdup(compat);
+ if (!str)
+ return false;
+
+ tok = strtok_r(str, ";", &tmp);
+ for (; tok; tok = strtok_r(NULL, ";", &tmp)) {
+ n = strlen(tok);
+ if ((tok[n - 1] == '*' && !strncmp(id, tok, n - 1)) ||
+ !strcmp(id, tok)) {
+ res = true;
+ goto out;
+ }
+ }
+ res = false;
+out:
+ free(str);
+ return res;
+}
+
struct pmu_add_cpu_aliases_map_data {
struct list_head *head;
const char *name;
@@ -847,7 +872,7 @@ static int pmu_add_sys_aliases_iter_fn(const struct pmu_event *pe,
if (!pe->compat || !pe->pmu)
return 0;
- if (!strcmp(pmu->id, pe->compat) &&
+ if (pmu_uncore_identifier_match(pmu->id, pe->compat) &&
pmu_uncore_alias_match(pe->pmu, pmu->name)) {
__perf_pmu__new_alias(idata->head, -1,
(char *)pe->name,
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index b9a02de..9d4385d 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -241,6 +241,7 @@ void pmu_add_cpu_aliases_table(struct list_head *head, struct perf_pmu *pmu,
char *perf_pmu__getcpuid(struct perf_pmu *pmu);
const struct pmu_events_table *pmu_events_table__find(void);
const struct pmu_metrics_table *pmu_metrics_table__find(void);
+bool pmu_uncore_identifier_match(const char *id, const char *compat);
void perf_pmu_free_alias(struct perf_pmu_alias *alias);
int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
--
1.8.3.1
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 <[email protected]>
---
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='),
+ ('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:
--
1.8.3.1
Add JSON metrics for Arm CMN. Currently just add part of CMN PMU
metrics which are general and compatible for any SoC with CMN-ANY.
Signed-off-by: Jing Zhang <[email protected]>
---
.../pmu-events/arch/arm64/arm/cmn/sys/metric.json | 74 ++++++++++++++++++++++
1 file changed, 74 insertions(+)
create mode 100644 tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metric.json
diff --git a/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metric.json b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metric.json
new file mode 100644
index 0000000..64db534
--- /dev/null
+++ b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metric.json
@@ -0,0 +1,74 @@
+[
+ {
+ "MetricName": "slc_miss_rate",
+ "BriefDescription": "The system level cache miss rate.",
+ "MetricGroup": "cmn",
+ "MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
+ "ScaleUnit": "100%",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "MetricName": "hnf_message_retry_rate",
+ "BriefDescription": "HN-F message retry rate indicates whether a lack of credits is causing the bottlenecks.",
+ "MetricGroup": "cmn",
+ "MetricExpr": "hnf_pocq_retry / hnf_pocq_reqs_recvd",
+ "ScaleUnit": "100%",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "MetricName": "sf_hit_rate",
+ "BriefDescription": "Snoop filter hit rate can be used to measure the snoop filter efficiency.",
+ "MetricGroup": "cmn",
+ "MetricExpr": "hnf_sf_hit / hnf_slc_sf_cache_access",
+ "ScaleUnit": "100%",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "MetricName": "mc_message_retry_rate",
+ "BriefDescription": "The memory controller request retries rate indicates whether the memory controller is the bottleneck.",
+ "MetricGroup": "cmn",
+ "MetricExpr": "hnf_mc_retries / hnf_mc_reqs",
+ "ScaleUnit": "100%",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "MetricName": "rni_actual_read_bandwidth.all",
+ "BriefDescription": "This event measure the actual bandwidth that RN-I bridge sends to the interconnect.",
+ "MetricGroup": "cmn",
+ "MetricExpr": "rnid_rxdat_flits * 32 / 1e6 / duration_time",
+ "ScaleUnit": "1MB/s",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "MetricName": "rni_actual_write_bandwidth.all",
+ "BriefDescription": "This event measures the actual write bandwidth at RN-I bridges.",
+ "MetricGroup": "cmn",
+ "MetricExpr": "rnid_txdat_flits * 32 / 1e6 / duration_time",
+ "ScaleUnit": "1MB/s",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "MetricName": "rni_retry_rate",
+ "BriefDescription": "RN-I bridge retry rate indicates whether the memory controller is the bottleneck.",
+ "MetricGroup": "cmn",
+ "MetricExpr": "rnid_txreq_flits_retried / rnid_txreq_flits_total",
+ "ScaleUnit": "100%",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "MetricName": "sbsx_actual_write_bandwidth.all",
+ "BriefDescription": "sbsx actual write bandwidth.",
+ "MetricGroup": "cmn",
+ "MetricExpr": "sbsx_txdat_flitv * 32 / 1e6 / duration_time",
+ "ScaleUnit": "1MB/s",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ }
+]
--
1.8.3.1
Currently just add aliases for part of Arm CMN PMU events which are
general and compatible for any SoC and CMN-ANY.
"Compat" value "434*;436*;43c*;43a*" means it is compatible with all
CMN600/CMN650/CMN700/Ci700.
Signed-off-by: Jing Zhang <[email protected]>
---
.../pmu-events/arch/arm64/arm/cmn/sys/cmn.json | 266 +++++++++++++++++++++
tools/perf/pmu-events/jevents.py | 1 +
2 files changed, 267 insertions(+)
create mode 100644 tools/perf/pmu-events/arch/arm64/arm/cmn/sys/cmn.json
diff --git a/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/cmn.json b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/cmn.json
new file mode 100644
index 0000000..e54036c
--- /dev/null
+++ b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/cmn.json
@@ -0,0 +1,266 @@
+[
+ {
+ "EventName": "hnf_cache_miss",
+ "EventIdCode": "0x1",
+ "Type": "0x5",
+ "BriefDescription": "Counts total cache misses in first lookup result (high priority).",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hnf_slc_sf_cache_access",
+ "EventIdCode": "0x2",
+ "Type": "0x5",
+ "BriefDescription": "Counts number of cache accesses in first access (high priority).",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hnf_cache_fill",
+ "EventIdCode": "0x3",
+ "Type": "0x5",
+ "BriefDescription": "Counts total allocations in HN SLC (all cache line allocations to SLC).",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hnf_pocq_retry",
+ "EventIdCode": "0x4",
+ "Type": "0x5",
+ "BriefDescription": "Counts number of retried requests.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hnf_pocq_reqs_recvd",
+ "EventIdCode": "0x5",
+ "Type": "0x5",
+ "BriefDescription": "Counts number of requests that HN receives.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hnf_sf_hit",
+ "EventIdCode": "0x6",
+ "Type": "0x5",
+ "BriefDescription": "Counts number of SF hits.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hnf_sf_evictions",
+ "EventIdCode": "0x7",
+ "Type": "0x5",
+ "BriefDescription": "Counts number of SF eviction cache invalidations initiated.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hnf_dir_snoops_sent",
+ "EventIdCode": "0x8",
+ "Type": "0x5",
+ "BriefDescription": "Counts number of directed snoops sent (not including SF back invalidation).",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hnf_brd_snoops_sent",
+ "EventIdCode": "0x9",
+ "Type": "0x5",
+ "BriefDescription": "Counts number of multicast snoops sent (not including SF back invalidation).",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hnf_slc_eviction",
+ "EventIdCode": "0xa",
+ "Type": "0x5",
+ "BriefDescription": "Counts number of SLC evictions (dirty only).",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hnf_slc_fill_invalid_way",
+ "EventIdCode": "0xb",
+ "Type": "0x5",
+ "BriefDescription": "Counts number of SLC fills to an invalid way.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hnf_mc_retries",
+ "EventIdCode": "0xc",
+ "Type": "0x5",
+ "BriefDescription": "Counts number of retried transactions by the MC.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hnf_mc_reqs",
+ "EventIdCode": "0xd",
+ "Type": "0x5",
+ "BriefDescription": "Counts number of requests that are sent to MC.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hnf_qos_hh_retry",
+ "EventIdCode": "0xe",
+ "Type": "0x5",
+ "BriefDescription": "Counts number of times a HighHigh priority request is protocolretried at the HN‑F.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "rnid_s0_rdata_beats",
+ "EventIdCode": "0x1",
+ "Type": "0xa",
+ "BriefDescription": "Number of RData beats (RVALID and RREADY) dispatched on port 0. This event measures the read bandwidth, including CMO responses.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "rnid_s1_rdata_beats",
+ "EventIdCode": "0x2",
+ "Type": "0xa",
+ "BriefDescription": "Number of RData beats (RVALID and RREADY) dispatched on port 1. This event measures the read bandwidth, including CMO responses.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "rnid_s2_rdata_beats",
+ "EventIdCode": "0x3",
+ "Type": "0xa",
+ "BriefDescription": "Number of RData beats (RVALID and RREADY) dispatched on port 2. This event measures the read bandwidth, including CMO responses.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "rnid_rxdat_flits",
+ "EventIdCode": "0x4",
+ "Type": "0xa",
+ "BriefDescription": "Number of RXDAT flits received. This event measures the true read data bandwidth, excluding CMOs.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "rnid_txdat_flits",
+ "EventIdCode": "0x5",
+ "Type": "0xa",
+ "BriefDescription": "Number of TXDAT flits dispatched. This event measures the write bandwidth.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "rnid_txreq_flits_total",
+ "EventIdCode": "0x6",
+ "Type": "0xa",
+ "BriefDescription": "Number of TXREQ flits dispatched. This event measures the total request bandwidth.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "rnid_txreq_flits_retried",
+ "EventIdCode": "0x7",
+ "Type": "0xa",
+ "BriefDescription": "Number of retried TXREQ flits dispatched. This event measures the retry rate.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "sbsx_txrsp_retryack",
+ "EventIdCode": "0x4",
+ "Type": "0x7",
+ "BriefDescription": "Number of RXREQ flits dispatched. This event is a measure of the retry rate.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "sbsx_txdat_flitv",
+ "EventIdCode": "0x5",
+ "Type": "0x7",
+ "BriefDescription": "Number of TXDAT flits dispatched from XP to SBSX. This event is a measure of the write bandwidth.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "sbsx_arvalid_no_arready",
+ "EventIdCode": "0x21",
+ "Type": "0x7",
+ "BriefDescription": "Number of cycles the SBSX bridge is stalled because of backpressure on AR channel.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "sbsx_awvalid_no_awready",
+ "EventIdCode": "0x22",
+ "Type": "0x7",
+ "BriefDescription": "Number of cycles the SBSX bridge is stalled because of backpressure on AW channel.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "sbsx_wvalid_no_wready",
+ "EventIdCode": "0x23",
+ "Type": "0x7",
+ "BriefDescription": "Number of cycles the SBSX bridge is stalled because of backpressure on W channel.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hni_txrsp_retryack",
+ "EventIdCode": "0x2a",
+ "Type": "0x4",
+ "BriefDescription": "Number of RXREQ flits dispatched. This event is a measure of the retry rate.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hni_arvalid_no_arready",
+ "EventIdCode": "0x2b",
+ "Type": "0x4",
+ "BriefDescription": "Number of cycles the HN-I bridge is stalled because of backpressure on AR channel.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hni_arready_no_arvalid",
+ "EventIdCode": "0x2c",
+ "Type": "0x4",
+ "BriefDescription": "Number of cycles the AR channel is waiting for new requests from HN-I bridge.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hni_awvalid_no_awready",
+ "EventIdCode": "0x2d",
+ "Type": "0x4",
+ "BriefDescription": "Number of cycles the HN-I bridge is stalled because of backpressure on AW channel.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hni_awready_no_awvalid",
+ "EventIdCode": "0x2e",
+ "Type": "0x4",
+ "BriefDescription": "Number of cycles the AW channel is waiting for new requests from HN-I bridge.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hni_wvalid_no_wready",
+ "EventIdCode": "0x2f",
+ "Type": "0x4",
+ "BriefDescription": "Number of cycles the HN-I bridge is stalled because of backpressure on W channel.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ },
+ {
+ "EventName": "hni_txdat_stall",
+ "EventIdCode": "0x30",
+ "Type": "0x4",
+ "BriefDescription": "TXDAT valid but no link credit available.",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a*"
+ }
+]
diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 9c0f63a..bfdfb67 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -272,6 +272,7 @@ class JsonEvent:
'DFPMC': 'amd_df',
'cpu_core': 'cpu_core',
'cpu_atom': 'cpu_atom',
+ 'arm_cmn': 'arm_cmn',
}
return table[unit] if unit in table else f'uncore_{unit.lower()}'
--
1.8.3.1
Add new event test for uncore system event which is used to verify the
functionality of "Compat" matching multiple identifiers and the new event
fields "EventIdCode" and "Type".
Signed-off-by: Jing Zhang <[email protected]>
---
.../pmu-events/arch/test/test_soc/sys/uncore.json | 8 ++++
tools/perf/tests/pmu-events.c | 55 ++++++++++++++++++++++
2 files changed, 63 insertions(+)
diff --git a/tools/perf/pmu-events/arch/test/test_soc/sys/uncore.json b/tools/perf/pmu-events/arch/test/test_soc/sys/uncore.json
index c7e7528..879a0ae 100644
--- a/tools/perf/pmu-events/arch/test/test_soc/sys/uncore.json
+++ b/tools/perf/pmu-events/arch/test/test_soc/sys/uncore.json
@@ -12,5 +12,13 @@
"EventName": "sys_ccn_pmu.read_cycles",
"Unit": "sys_ccn_pmu",
"Compat": "0x01"
+ },
+ {
+ "BriefDescription": "Counts total cache misses in first lookup result (high priority).",
+ "Type": "0x05",
+ "EventIdCode": "0x01",
+ "EventName": "sys_cmn_pmu.hnf_cache_miss",
+ "Unit": "arm_cmn",
+ "Compat": "434*;436*;43c*;43a01"
}
]
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index 1dff863b..e227dcd 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -255,9 +255,24 @@ struct perf_pmu_test_pmu {
.matching_pmu = "uncore_sys_ccn_pmu",
};
+static const struct perf_pmu_test_event sys_cmn_pmu_hnf_cache_miss = {
+ .event = {
+ .name = "sys_cmn_pmu.hnf_cache_miss",
+ .event = "type=0x05,eventid=0x01",
+ .desc = "Counts total cache misses in first lookup result (high priority). Unit: uncore_arm_cmn ",
+ .topic = "uncore",
+ .pmu = "uncore_arm_cmn",
+ .compat = "434*;436*;43c*;43a01",
+ },
+ .alias_str = "type=0x5,eventid=0x1",
+ .alias_long_desc = "Counts total cache misses in first lookup result (high priority). Unit: uncore_arm_cmn ",
+ .matching_pmu = "uncore_arm_cmn_0",
+};
+
static const struct perf_pmu_test_event *sys_events[] = {
&sys_ddr_pmu_write_cycles,
&sys_ccn_pmu_read_cycles,
+ &sys_cmn_pmu_hnf_cache_miss,
NULL
};
@@ -699,6 +714,46 @@ static int __test_uncore_pmu_event_aliases(struct perf_pmu_test_pmu *test_pmu)
&sys_ccn_pmu_read_cycles,
},
},
+ {
+ .pmu = {
+ .name = (char *)"uncore_arm_cmn_0",
+ .is_uncore = 1,
+ .id = (char *)"43401",
+ },
+ .aliases = {
+ &sys_cmn_pmu_hnf_cache_miss,
+ },
+ },
+ {
+ .pmu = {
+ .name = (char *)"uncore_arm_cmn_0",
+ .is_uncore = 1,
+ .id = (char *)"43602",
+ },
+ .aliases = {
+ &sys_cmn_pmu_hnf_cache_miss,
+ },
+ },
+ {
+ .pmu = {
+ .name = (char *)"uncore_arm_cmn_1",
+ .is_uncore = 1,
+ .id = (char *)"43c03",
+ },
+ .aliases = {
+ &sys_cmn_pmu_hnf_cache_miss,
+ },
+ },
+ {
+ .pmu = {
+ .name = (char *)"uncore_arm_cmn_1",
+ .is_uncore = 1,
+ .id = (char *)"43a01",
+ },
+ .aliases = {
+ &sys_cmn_pmu_hnf_cache_miss,
+ },
+ }
};
/* Test that aliases generated are as expected */
--
1.8.3.1
On 28/07/2023 07:17, Jing Zhang wrote:
> The jevent "Compat" is used for uncore PMU alias or metric definitions.
>
> The same PMU driver has different PMU identifiers due to different hardware
> versions and types, but they may have some common PMU event/metric. Since a
> Compat value can only match one identifier, when adding the same event
> alias and metric to PMUs with different identifiers, each identifier needs
> to be defined once, which is not streamlined enough.
>
> So let "Compat" value supports matching multiple identifiers. For example,
> the Compat value {abcde;123*}
why not use a comma-separated list? that is more common
> can match the PMU identifier "abcde" and the
> the PMU identifier with the prefix "123",
I have to admit that this is not a great example as it does not match an
expected real-life scenario. I mean, I would not expect a PMU identifier
for the same PMU to be in either format "abcde" or "123*". I would
expect to be in only ever one format.
> where "*" is a wildcard.
> Tokens in Unit field are delimited by ';' with no spaces.
>
> Signed-off-by: Jing Zhang <[email protected]>
> ---
> tools/perf/util/metricgroup.c | 2 +-
> tools/perf/util/pmu.c | 27 ++++++++++++++++++++++++++-
> tools/perf/util/pmu.h | 1 +
> 3 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 5e9c657..ff81bc5 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -477,7 +477,7 @@ static int metricgroup__sys_event_iter(const struct pmu_metric *pm,
>
> while ((pmu = perf_pmu__scan(pmu))) {
>
> - if (!pmu->id || strcmp(pmu->id, pm->compat))
> + if (!pmu->id || !pmu_uncore_identifier_match(pmu->id, pm->compat))
> continue;
>
> return d->fn(pm, table, d->data);
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index ad209c8..3ae249b 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -776,6 +776,31 @@ static bool pmu_uncore_alias_match(const char *pmu_name, const char *name)
> return res;
> }
>
> +bool pmu_uncore_identifier_match(const char *id, const char *compat)
> +{
> + char *tmp = NULL, *tok, *str;
> + bool res;
> + int n;
> +
> + str = strdup(compat);
why duplicate this? are you modifying something?
> + if (!str)
> + return false;
> +
> + tok = strtok_r(str, ";", &tmp);
> + for (; tok; tok = strtok_r(NULL, ";", &tmp)) {
> + n = strlen(tok);
> + if ((tok[n - 1] == '*' && !strncmp(id, tok, n - 1)) ||
> + !strcmp(id, tok)) {
> + res = true;
> + goto out;
> + }
> + }
> + res = false;
> +out:
> + free(str);
> + return res;
> +}
> +
> struct pmu_add_cpu_aliases_map_data {
> struct list_head *head;
> const char *name;
> @@ -847,7 +872,7 @@ static int pmu_add_sys_aliases_iter_fn(const struct pmu_event *pe,
This is not for metrics specifically. You are really doing 2x things
here. I suggest that you split the patch out into 1st pmu.c change and
2nd metricgroup.c change
> if (!pe->compat || !pe->pmu)
> return 0;
>
> - if (!strcmp(pmu->id, pe->compat) &&
> + if (pmu_uncore_identifier_match(pmu->id, pe->compat) &&
> pmu_uncore_alias_match(pe->pmu, pmu->name)) {
nit: let's change order to check alias and then identifier
> __perf_pmu__new_alias(idata->head, -1,
> (char *)pe->name,
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index b9a02de..9d4385d 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -241,6 +241,7 @@ void pmu_add_cpu_aliases_table(struct list_head *head, struct perf_pmu *pmu,
> char *perf_pmu__getcpuid(struct perf_pmu *pmu);
> const struct pmu_events_table *pmu_events_table__find(void);
> const struct pmu_metrics_table *pmu_metrics_table__find(void);
> +bool pmu_uncore_identifier_match(const char *id, const char *compat);
> void perf_pmu_free_alias(struct perf_pmu_alias *alias);
>
> int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
Thanks,
John
On 28/07/2023 07:17, Jing Zhang wrote:
> Currently just add aliases for part of Arm CMN PMU events which are
> general and compatible for any SoC and CMN-ANY.
>
> "Compat" value "434*;436*;43c*;43a*" means it is compatible with all
As mentioned in patch 1/5, a comma-separated list seems a better
delimiter to me
> CMN600/CMN650/CMN700/Ci700.
It would be good if you could provide a link to where you got these
events. I think that it is the publicly available CMN TRM document.
>
> Signed-off-by: Jing Zhang<[email protected]>
Apart from comments, above:
Reviewed-by: John Garry <[email protected]>
On 28/07/2023 07:17, Jing Zhang wrote:
> Add new event test for uncore system event which is used to verify the
> functionality of "Compat" matching multiple identifiers and the new event
> fields "EventIdCode" and "Type".
>
Thanks for doing this. It looks ok. I just have a comment, below.
> Signed-off-by: Jing Zhang <[email protected]>
> ---
> .../pmu-events/arch/test/test_soc/sys/uncore.json | 8 ++++
> tools/perf/tests/pmu-events.c | 55 ++++++++++++++++++++++
> 2 files changed, 63 insertions(+)
>
> diff --git a/tools/perf/pmu-events/arch/test/test_soc/sys/uncore.json b/tools/perf/pmu-events/arch/test/test_soc/sys/uncore.json
> index c7e7528..879a0ae 100644
> --- a/tools/perf/pmu-events/arch/test/test_soc/sys/uncore.json
> +++ b/tools/perf/pmu-events/arch/test/test_soc/sys/uncore.json
> @@ -12,5 +12,13 @@
> "EventName": "sys_ccn_pmu.read_cycles",
> "Unit": "sys_ccn_pmu",
> "Compat": "0x01"
> + },
> + {
> + "BriefDescription": "Counts total cache misses in first lookup result (high priority).",
> + "Type": "0x05",
> + "EventIdCode": "0x01",
> + "EventName": "sys_cmn_pmu.hnf_cache_miss",
> + "Unit": "arm_cmn",
> + "Compat": "434*;436*;43c*;43a01"
> }
> ]
> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> index 1dff863b..e227dcd 100644
> --- a/tools/perf/tests/pmu-events.c
> +++ b/tools/perf/tests/pmu-events.c
> @@ -255,9 +255,24 @@ struct perf_pmu_test_pmu {
> .matching_pmu = "uncore_sys_ccn_pmu",
> };
>
> +static const struct perf_pmu_test_event sys_cmn_pmu_hnf_cache_miss = {
> + .event = {
> + .name = "sys_cmn_pmu.hnf_cache_miss",
> + .event = "type=0x05,eventid=0x01",
> + .desc = "Counts total cache misses in first lookup result (high priority). Unit: uncore_arm_cmn ",
> + .topic = "uncore",
> + .pmu = "uncore_arm_cmn",
> + .compat = "434*;436*;43c*;43a01",
> + },
> + .alias_str = "type=0x5,eventid=0x1",
> + .alias_long_desc = "Counts total cache misses in first lookup result (high priority). Unit: uncore_arm_cmn ",
> + .matching_pmu = "uncore_arm_cmn_0",
> +};
> +
> static const struct perf_pmu_test_event *sys_events[] = {
> &sys_ddr_pmu_write_cycles,
> &sys_ccn_pmu_read_cycles,
> + &sys_cmn_pmu_hnf_cache_miss,
> NULL
> };
>
> @@ -699,6 +714,46 @@ static int __test_uncore_pmu_event_aliases(struct perf_pmu_test_pmu *test_pmu)
> &sys_ccn_pmu_read_cycles,
> },
> },
> + {
> + .pmu = {
> + .name = (char *)"uncore_arm_cmn_0",
> + .is_uncore = 1,
> + .id = (char *)"43401",
> + },
> + .aliases = {
> + &sys_cmn_pmu_hnf_cache_miss,
> + },
> + },
> + {
> + .pmu = {
> + .name = (char *)"uncore_arm_cmn_0",
> + .is_uncore = 1,
> + .id = (char *)"43602",
> + },
> + .aliases = {
> + &sys_cmn_pmu_hnf_cache_miss,
> + },
> + },
> + {
> + .pmu = {
> + .name = (char *)"uncore_arm_cmn_1",
Shouldn't this match some perf_pmu_test_event entry with same
matching_pmu member? But is perf_pmu_test_event.matching_pmu member ever
used for any checking???
Thanks,
John
> + .is_uncore = 1,
> + .id = (char *)"43c03",
> + },
> + .aliases = {
> + &sys_cmn_pmu_hnf_cache_miss,
> + },
> + },
> + {
> + .pmu = {
> + .name = (char *)"uncore_arm_cmn_1",
> + .is_uncore = 1,
> + .id = (char *)"43a01",
> + },
> + .aliases = {
> + &sys_cmn_pmu_hnf_cache_miss,
> + },
> + }
> };
>
> /* Test that aliases generated are as expected */
On 28/07/2023 07:17, Jing Zhang wrote:
> Add JSON metrics for Arm CMN. Currently just add part of CMN PMU
> metrics which are general and compatible for any SoC with CMN-ANY.
>
> Signed-off-by: Jing Zhang<[email protected]>
The same comments apply here as patch 4/5, and, apart from that:
Reviewed-by: John Garry <[email protected]>
在 2023/7/28 下午4:11, John Garry 写道:
> On 28/07/2023 07:17, Jing Zhang wrote:
>> The jevent "Compat" is used for uncore PMU alias or metric definitions.
>>
>> The same PMU driver has different PMU identifiers due to different hardware
>> versions and types, but they may have some common PMU event/metric. Since a
>> Compat value can only match one identifier, when adding the same event
>> alias and metric to PMUs with different identifiers, each identifier needs
>> to be defined once, which is not streamlined enough.
>>
>> So let "Compat" value supports matching multiple identifiers. For example,
>> the Compat value {abcde;123*}
> why not use a comma-separated list? that is more common
>
Hi John,
I use a semicolon instead of a comma because I want to distinguish it from the function
of the comma in "Unit" and avoid confusion between the use of commas in "Unit" and "Compat".
Because in “Compat”, the semicolon means "or". So I think semicolons are more appropriate,
what do you think?
>> can match the PMU identifier "abcde" and the
>> the PMU identifier with the prefix "123",
>
> I have to admit that this is not a great example as it does not match an expected real-life scenario. I mean, I would not expect a PMU identifier for the same PMU to be in either format "abcde" or "123*". I would expect to be in only ever one format.
>
Get, I'll pick a more appropriate example {43401;436*}(CMN600 r0p0 and all CMN650).
>> where "*" is a wildcard.
>> Tokens in Unit field are delimited by ';' with no spaces.
>>
>> Signed-off-by: Jing Zhang <[email protected]>
>> ---
>> tools/perf/util/metricgroup.c | 2 +-
>> tools/perf/util/pmu.c | 27 ++++++++++++++++++++++++++-
>> tools/perf/util/pmu.h | 1 +
>> 3 files changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>> index 5e9c657..ff81bc5 100644
>> --- a/tools/perf/util/metricgroup.c
>> +++ b/tools/perf/util/metricgroup.c
>> @@ -477,7 +477,7 @@ static int metricgroup__sys_event_iter(const struct pmu_metric *pm,
>> while ((pmu = perf_pmu__scan(pmu))) {
>> - if (!pmu->id || strcmp(pmu->id, pm->compat))
>> + if (!pmu->id || !pmu_uncore_identifier_match(pmu->id, pm->compat))
>> continue;
>> return d->fn(pm, table, d->data);
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index ad209c8..3ae249b 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -776,6 +776,31 @@ static bool pmu_uncore_alias_match(const char *pmu_name, const char *name)
>> return res;
>> }
>> +bool pmu_uncore_identifier_match(const char *id, const char *compat)
>> +{
>> + char *tmp = NULL, *tok, *str;
>> + bool res;
>> + int n;
>> +
>> + str = strdup(compat);
>
> why duplicate this? are you modifying something?
>
This is really a redundant step, I will remove it.
>> + if (!str)
>> + return false;
>> +
>> + tok = strtok_r(str, ";", &tmp);
>> + for (; tok; tok = strtok_r(NULL, ";", &tmp)) {
>> + n = strlen(tok);
>> + if ((tok[n - 1] == '*' && !strncmp(id, tok, n - 1)) ||
>> + !strcmp(id, tok)) {
>> + res = true;
>> + goto out;
>> + }
>> + }
>> + res = false;
>> +out:
>> + free(str);
>> + return res;
>> +}
>> +
>> struct pmu_add_cpu_aliases_map_data {
>> struct list_head *head;
>> const char *name;
>> @@ -847,7 +872,7 @@ static int pmu_add_sys_aliases_iter_fn(const struct pmu_event *pe,
>
> This is not for metrics specifically. You are really doing 2x things here. I suggest that you split the patch out into 1st pmu.c change and 2nd metricgroup.c change
>
Ok, will do.
>> if (!pe->compat || !pe->pmu)
>> return 0;
>> - if (!strcmp(pmu->id, pe->compat) &&
>> + if (pmu_uncore_identifier_match(pmu->id, pe->compat) &&
>> pmu_uncore_alias_match(pe->pmu, pmu->name)) {
>
> nit: let's change order to check alias and then identifier
>
Will do.
Thanks,
Jing
>> __perf_pmu__new_alias(idata->head, -1,
>> (char *)pe->name,
>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
>> index b9a02de..9d4385d 100644
>> --- a/tools/perf/util/pmu.h
>> +++ b/tools/perf/util/pmu.h
>> @@ -241,6 +241,7 @@ void pmu_add_cpu_aliases_table(struct list_head *head, struct perf_pmu *pmu,
>> char *perf_pmu__getcpuid(struct perf_pmu *pmu);
>> const struct pmu_events_table *pmu_events_table__find(void);
>> const struct pmu_metrics_table *pmu_metrics_table__find(void);
>> +bool pmu_uncore_identifier_match(const char *id, const char *compat);
>> void perf_pmu_free_alias(struct perf_pmu_alias *alias);
>> int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
>
> Thanks,
> John
在 2023/7/28 下午4:17, John Garry 写道:
> On 28/07/2023 07:17, Jing Zhang wrote:
>> Currently just add aliases for part of Arm CMN PMU events which are
>> general and compatible for any SoC and CMN-ANY.
>>
>> "Compat" value "434*;436*;43c*;43a*" means it is compatible with all
>
> As mentioned in patch 1/5, a comma-separated list seems a better delimiter to me
>
>> CMN600/CMN650/CMN700/Ci700.
>
> It would be good if you could provide a link to where you got these events. I think that it is the publicly available CMN TRM document.
>
Ok, will do.
Thanks,
Jing
>>
>> Signed-off-by: Jing Zhang<[email protected]>
>
> Apart from comments, above:
>
> Reviewed-by: John Garry <[email protected]>
在 2023/7/28 下午4:30, John Garry 写道:
> On 28/07/2023 07:17, Jing Zhang wrote:
>> Add new event test for uncore system event which is used to verify the
>> functionality of "Compat" matching multiple identifiers and the new event
>> fields "EventIdCode" and "Type".
>>
>
> Thanks for doing this. It looks ok. I just have a comment, below.
>
Thanks.
>> Signed-off-by: Jing Zhang <[email protected]>
>> ---
>> .../pmu-events/arch/test/test_soc/sys/uncore.json | 8 ++++
>> tools/perf/tests/pmu-events.c | 55 ++++++++++++++++++++++
>> 2 files changed, 63 insertions(+)
>>
>> diff --git a/tools/perf/pmu-events/arch/test/test_soc/sys/uncore.json b/tools/perf/pmu-events/arch/test/test_soc/sys/uncore.json
>> index c7e7528..879a0ae 100644
>> --- a/tools/perf/pmu-events/arch/test/test_soc/sys/uncore.json
>> +++ b/tools/perf/pmu-events/arch/test/test_soc/sys/uncore.json
>> @@ -12,5 +12,13 @@
>> "EventName": "sys_ccn_pmu.read_cycles",
>> "Unit": "sys_ccn_pmu",
>> "Compat": "0x01"
>> + },
>> + {
>> + "BriefDescription": "Counts total cache misses in first lookup result (high priority).",
>> + "Type": "0x05",
>> + "EventIdCode": "0x01",
>> + "EventName": "sys_cmn_pmu.hnf_cache_miss",
>> + "Unit": "arm_cmn",
>> + "Compat": "434*;436*;43c*;43a01"
>> }
>> ]
>> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
>> index 1dff863b..e227dcd 100644
>> --- a/tools/perf/tests/pmu-events.c
>> +++ b/tools/perf/tests/pmu-events.c
>> @@ -255,9 +255,24 @@ struct perf_pmu_test_pmu {
>> .matching_pmu = "uncore_sys_ccn_pmu",
>> };
>> +static const struct perf_pmu_test_event sys_cmn_pmu_hnf_cache_miss = {
>> + .event = {
>> + .name = "sys_cmn_pmu.hnf_cache_miss",
>> + .event = "type=0x05,eventid=0x01",
>> + .desc = "Counts total cache misses in first lookup result (high priority). Unit: uncore_arm_cmn ",
>> + .topic = "uncore",
>> + .pmu = "uncore_arm_cmn",
>> + .compat = "434*;436*;43c*;43a01",
>> + },
>> + .alias_str = "type=0x5,eventid=0x1",
>> + .alias_long_desc = "Counts total cache misses in first lookup result (high priority). Unit: uncore_arm_cmn ",
>> + .matching_pmu = "uncore_arm_cmn_0",
>> +};
>> +
>> static const struct perf_pmu_test_event *sys_events[] = {
>> &sys_ddr_pmu_write_cycles,
>> &sys_ccn_pmu_read_cycles,
>> + &sys_cmn_pmu_hnf_cache_miss,
>> NULL
>> };
>> @@ -699,6 +714,46 @@ static int __test_uncore_pmu_event_aliases(struct perf_pmu_test_pmu *test_pmu)
>> &sys_ccn_pmu_read_cycles,
>> },
>> },
>> + {
>> + .pmu = {
>> + .name = (char *)"uncore_arm_cmn_0",
>> + .is_uncore = 1,
>> + .id = (char *)"43401",
>> + },
>> + .aliases = {
>> + &sys_cmn_pmu_hnf_cache_miss,
>> + },
>> + },
>> + {
>> + .pmu = {
>> + .name = (char *)"uncore_arm_cmn_0",
>> + .is_uncore = 1,
>> + .id = (char *)"43602",
>> + },
>> + .aliases = {
>> + &sys_cmn_pmu_hnf_cache_miss,
>> + },
>> + },
>> + {
>> + .pmu = {
>> + .name = (char *)"uncore_arm_cmn_1",
>
> Shouldn't this match some perf_pmu_test_event entry with same matching_pmu member? But is perf_pmu_test_event.matching_pmu member ever used for any checking???
>
I need to double check because I was testing against 6.3-rc2.
Thanks,
Jing
> Thanks,
> John
>
>> + .is_uncore = 1,
>> + .id = (char *)"43c03",
>> + },
>> + .aliases = {
>> + &sys_cmn_pmu_hnf_cache_miss,
>> + },
>> + },
>> + {
>> + .pmu = {
>> + .name = (char *)"uncore_arm_cmn_1",
>> + .is_uncore = 1,
>> + .id = (char *)"43a01",
>> + },
>> + .aliases = {
>> + &sys_cmn_pmu_hnf_cache_miss,
>> + },
>> + }
>> };
>> /* Test that aliases generated are as expected */
On 31/07/2023 13:30, Jing Zhang wrote:
>>> + .pmu = {
>>> + .name = (char *)"uncore_arm_cmn_0",
>>> + .is_uncore = 1,
>>> + .id = (char *)"43602",
>>> + },
>>> + .aliases = {
>>> + &sys_cmn_pmu_hnf_cache_miss,
>>> + },
>>> + },
>>> + {
>>> + .pmu = {
>>> + .name = (char *)"uncore_arm_cmn_1",
>> Shouldn't this match some perf_pmu_test_event entry with same matching_pmu member? But is perf_pmu_test_event.matching_pmu member ever used for any checking???
>>
> I need to double check because I was testing against 6.3-rc2.
That 6.3-rc2, was for the the kernel? Or baseline for this series? See
see maintainers for git/branch to base perf tool dev on.
Thanks,
John
On 31/07/2023 11:59, Jing Zhang wrote:
>> why not use a comma-separated list? that is more common
>>
> Hi John,
>
> I use a semicolon instead of a comma because I want to distinguish it from the function
> of the comma in "Unit" and avoid confusion between the use of commas in "Unit" and "Compat".
> Because in “Compat”, the semicolon means "or". So I think semicolons are more appropriate,
> what do you think?
>
I suppose semicolon is ok in this case.
Thanks,
John
在 2023/7/31 下午9:12, John Garry 写道:
> On 31/07/2023 13:30, Jing Zhang wrote:
>>>> + .pmu = {
>>>> + .name = (char *)"uncore_arm_cmn_0",
>>>> + .is_uncore = 1,
>>>> + .id = (char *)"43602",
>>>> + },
>>>> + .aliases = {
>>>> + &sys_cmn_pmu_hnf_cache_miss,
>>>> + },
>>>> + },
>>>> + {
>>>> + .pmu = {
>>>> + .name = (char *)"uncore_arm_cmn_1",
>>> Shouldn't this match some perf_pmu_test_event entry with same matching_pmu member? But is perf_pmu_test_event.matching_pmu member ever used for any checking???
>>>
>> I need to double check because I was testing against 6.3-rc2.
>
> That 6.3-rc2, was for the the kernel? Or baseline for this series? See see maintainers for git/branch to base perf tool dev on.
>
I have now developed based on the latest perf tool, but I'm still confused.
"matching_pmu" does not seem to have any effect. No matter what value matching_pmu
is, it will not affect the final test result.
Thank,
Jing
On 01/08/2023 10:19, Jing Zhang wrote:
>>> I need to double check because I was testing against 6.3-rc2.
>> That 6.3-rc2, was for the the kernel? Or baseline for this series? See see maintainers for git/branch to base perf tool dev on.
>>
> I have now developed based on the latest perf tool, but I'm still confused.
> "matching_pmu" does not seem to have any effect. No matter what value matching_pmu
> is, it will not affect the final test result.
Yeah, that is what I was saying - matching_pmu is not used. Can you fix
that up (to be used), please?
Thanks,
John
在 2023/8/1 下午11:10, John Garry 写道:
> On 01/08/2023 10:19, Jing Zhang wrote:
>>>> I need to double check because I was testing against 6.3-rc2.
>>> That 6.3-rc2, was for the the kernel? Or baseline for this series? See see maintainers for git/branch to base perf tool dev on.
>>>
>> I have now developed based on the latest perf tool, but I'm still confused.
>> "matching_pmu" does not seem to have any effect. No matter what value matching_pmu
>> is, it will not affect the final test result.
>
> Yeah, that is what I was saying - matching_pmu is not used. Can you fix that up (to be used), please?
>
Ok, let me try :)
Thanks,
Jing
在 2023/7/31 下午6:59, Jing Zhang 写道:
>
> 在 2023/7/28 下午4:11, John Garry 写道:
>> On 28/07/2023 07:17, Jing Zhang wrote:
>>> The jevent "Compat" is used for uncore PMU alias or metric definitions.
>>>
>>> The same PMU driver has different PMU identifiers due to different hardware
>>> versions and types, but they may have some common PMU event/metric. Since a
>>> Compat value can only match one identifier, when adding the same event
>>> alias and metric to PMUs with different identifiers, each identifier needs
>>> to be defined once, which is not streamlined enough.
>>>
>>> So let "Compat" value supports matching multiple identifiers. For example,
>>> the Compat value {abcde;123*}
>> why not use a comma-separated list? that is more common
>>
>
> Hi John,
>
> I use a semicolon instead of a comma because I want to distinguish it from the function
> of the comma in "Unit" and avoid confusion between the use of commas in "Unit" and "Compat".
> Because in “Compat”, the semicolon means "or". So I think semicolons are more appropriate,
> what do you think?
>
>>> can match the PMU identifier "abcde" and the
>>> the PMU identifier with the prefix "123",
>>
>> I have to admit that this is not a great example as it does not match an expected real-life scenario. I mean, I would not expect a PMU identifier for the same PMU to be in either format "abcde" or "123*". I would expect to be in only ever one format.
>>
>
> Get, I'll pick a more appropriate example {43401;436*}(CMN600 r0p0 and all CMN650).
>
>>> where "*" is a wildcard.
>>> Tokens in Unit field are delimited by ';' with no spaces.
>>>
>>> Signed-off-by: Jing Zhang <[email protected]>
>>> ---
>>> tools/perf/util/metricgroup.c | 2 +-
>>> tools/perf/util/pmu.c | 27 ++++++++++++++++++++++++++-
>>> tools/perf/util/pmu.h | 1 +
>>> 3 files changed, 28 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>>> index 5e9c657..ff81bc5 100644
>>> --- a/tools/perf/util/metricgroup.c
>>> +++ b/tools/perf/util/metricgroup.c
>>> @@ -477,7 +477,7 @@ static int metricgroup__sys_event_iter(const struct pmu_metric *pm,
>>> while ((pmu = perf_pmu__scan(pmu))) {
>>> - if (!pmu->id || strcmp(pmu->id, pm->compat))
>>> + if (!pmu->id || !pmu_uncore_identifier_match(pmu->id, pm->compat))
>>> continue;
>>> return d->fn(pm, table, d->data);
>>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>>> index ad209c8..3ae249b 100644
>>> --- a/tools/perf/util/pmu.c
>>> +++ b/tools/perf/util/pmu.c
>>> @@ -776,6 +776,31 @@ static bool pmu_uncore_alias_match(const char *pmu_name, const char *name)
>>> return res;
>>> }
>>> +bool pmu_uncore_identifier_match(const char *id, const char *compat)
>>> +{
>>> + char *tmp = NULL, *tok, *str;
>>> + bool res;
>>> + int n;
>>> +
>>> + str = strdup(compat);
>>
>> why duplicate this? are you modifying something?
>>
>
> This is really a redundant step, I will remove it.
>
Hi John,
I reviewed this code again and found that it still needs to duplicate "compat" because "compat" is a
const str* type and cannot be used as a parameter for the strtok_r function. If it is cast to char*,
using "compat" as a parameter for strtok_r is also unsafe and can cause a "Segmentation fault" error.
Therefore, let's keep the step of duplicating "compat".
Thanks,
Jing
>>> + if (!str)
>>> + return false;
>>> +
>>> + tok = strtok_r(str, ";", &tmp);
>>> + for (; tok; tok = strtok_r(NULL, ";", &tmp)) {
>>> + n = strlen(tok);
>>> + if ((tok[n - 1] == '*' && !strncmp(id, tok, n - 1)) ||
>>> + !strcmp(id, tok)) {
>>> + res = true;
>>> + goto out;
>>> + }
>>> + }
>>> + res = false;
>>> +out:
>>> + free(str);
>>> + return res;
>>> +}
>>> +
>>> struct pmu_add_cpu_aliases_map_data {
>>> struct list_head *head;
>>> const char *name;
>>> @@ -847,7 +872,7 @@ static int pmu_add_sys_aliases_iter_fn(const struct pmu_event *pe,
>>
>> This is not for metrics specifically. You are really doing 2x things here. I suggest that you split the patch out into 1st pmu.c change and 2nd metricgroup.c change
>>
>
> Ok, will do.
>
>>> if (!pe->compat || !pe->pmu)
>>> return 0;
>>> - if (!strcmp(pmu->id, pe->compat) &&
>>> + if (pmu_uncore_identifier_match(pmu->id, pe->compat) &&
>>> pmu_uncore_alias_match(pe->pmu, pmu->name)) {
>>
>> nit: let's change order to check alias and then identifier
>>
>
> Will do.
>
>
> Thanks,
> Jing
>
>>> __perf_pmu__new_alias(idata->head, -1,
>>> (char *)pe->name,
>>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
>>> index b9a02de..9d4385d 100644
>>> --- a/tools/perf/util/pmu.h
>>> +++ b/tools/perf/util/pmu.h
>>> @@ -241,6 +241,7 @@ void pmu_add_cpu_aliases_table(struct list_head *head, struct perf_pmu *pmu,
>>> char *perf_pmu__getcpuid(struct perf_pmu *pmu);
>>> const struct pmu_events_table *pmu_events_table__find(void);
>>> const struct pmu_metrics_table *pmu_metrics_table__find(void);
>>> +bool pmu_uncore_identifier_match(const char *id, const char *compat);
>>> void perf_pmu_free_alias(struct perf_pmu_alias *alias);
>>> int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
>>
>> Thanks,
>> John
在 2023/8/2 下午5:43, John Garry 写道:
> On 02/08/2023 10:38, Jing Zhang wrote:
>>>>> n;
>>>>> +
>>>>> + str = strdup(compat);
>>>> why duplicate this? are you modifying something?
>>>>
>>> This is really a redundant step, I will remove it.
>>>
>> Hi John,
>>
>> I reviewed this code again and found that it still needs to duplicate "compat" because "compat" is a
>> const str* type and cannot be used as a parameter for the strtok_r function. If it is cast to char*,
>> using "compat" as a parameter for strtok_r is also unsafe and can cause a "Segmentation fault" error.
>> Therefore, let's keep the step of duplicating "compat".
>
> ok, so then please add a small comment on why the strdup() call is needed.
>
No problem.
Thanks,
Jing
On 02/08/2023 10:38, Jing Zhang wrote:
>>>> n;
>>>> +
>>>> + str = strdup(compat);
>>> why duplicate this? are you modifying something?
>>>
>> This is really a redundant step, I will remove it.
>>
> Hi John,
>
> I reviewed this code again and found that it still needs to duplicate "compat" because "compat" is a
> const str* type and cannot be used as a parameter for the strtok_r function. If it is cast to char*,
> using "compat" as a parameter for strtok_r is also unsafe and can cause a "Segmentation fault" error.
> Therefore, let's keep the step of duplicating "compat".
ok, so then please add a small comment on why the strdup() call is needed.
Thanks,
John