[PATCH v3 0/7] Add JSON metrics for arm CMN and Yitian710 DDR

[PATCH v3 0/7] Add JSON metrics for arm CMN and Yitian710 DDR

[Jing Zhang]

Changes since v2:
- Refact cmn identifier and use model and revision to form identifier.
- Let "Compat" support matching multiple identifier.
- Improved the ali_drw PMU event alias Brief Description.
- Update ali_drw PMU metric usage in documentation.

Changes since RFC:
- Refact arm-cmn PMU identifier.
- Not add arm-cmn PMU aliasing currently because it's Eventcode is
  difficult to define.
- Rename ali_drw PMU identifier and Unit name.
- Divide ali_drw PMU metric and aliasing into two patches.

Add an identifier sysfs file for the yitian710 SoC DDR and arm CMN to
allow userspace to identify the specific implementation of the device,
so that the perf tool can match the corresponding uncore events and
metrics through the identifier. Then added several general CMN metrics
and yitian710 soc DDR metrics and events alias.


$perf list:
...
ali_drw:
  chi_rxdat
       [A packet at CHI RXDAT interface (write data). Unit: ali_drw]
  chi_rxrsp
       [A packet at CHI RXRSP interface. Unit: ali_drw]
  chi_txdat
       [A packet at CHI TXDAT interface (read data). Unit: ali_drw]
  chi_txreq
       [A packet at CHI TXREQ interface (request). Unit: ali_drw]
  cycle
       [The ddr cycle. Unit: ali_drw]
...
arm_cmn:
  mc_message_retry_rate
       [The memory controller request retries rate indicates whether the memory controller is the bottleneck. Unit: arm_cmn ]
  rni_actual_read_bandwidth.all
       [This event measure the actual bandwidth(MB/sec) that RN-I bridge sends to the interconnect. Unit: arm_cmn ]
  rni_actual_write_bandwidth.all
       [This event measures the actual write bandwidth(MB/sec) at RN-I bridges. Unit: arm_cmn ]
  rni_retry_rate
       [RN-I bridge retry rate indicates whether the memory controller is the bottleneck. Unit: arm_cmn ]
  sbsx_actual_write_bandwidth.all
       [sbsx actual write bandwidth(MB/sec). Unit: arm_cmn ]
  sf_hit_rate
       [Snoop filter hit rate can be used to measure the Snoop Filter efficiency. Unit: arm_cmn ]
  slc_miss_rate
       [The system level cache miss rate include. Unit: arm_cmn ]
ali_drw:
  ddr_read_bandwidth.all
       [The ddr read bandwidth(MB/s). Unit: ali_drw ]
  ddr_write_bandwidth.all
       [The ddr write bandwidth(MB/s). Unit: ali_drw ]
...

$perf stat -M ddr_read_bandwidth.all ./test

Performance counter stats for 'system wide':

            38,150      hif_rd        #  2.4 MB/s  ddr_read_bandwidth.all
     1,000,957,941 ns   duration_time

       1.000957941 seconds time elapsed

Jing Zhang (7):
  driver/perf: Add identifier sysfs file for CMN
  perf metric: Event "Compat" value supports matching multiple
    identifiers
  perf vendor events: Add JSON metrics for CMN
  driver/perf: Add identifier sysfs file for Yitian 710 DDR
  perf jevents: Add support for Yitian 710 DDR PMU aliasing
  perf vendor events: Add JSON metrics for Yitian 710 DDR
  docs: perf: Update metric usage for Alibaba's T-Head PMU driver

 Documentation/admin-guide/perf/alibaba_pmu.rst     |   5 +
 drivers/perf/alibaba_uncore_drw_pmu.c              |  27 ++
 drivers/perf/arm-cmn.c                             |  79 ++++-
 .../pmu-events/arch/arm64/arm/cmn/sys/metrics.json |  74 ++++
 .../arm64/freescale/yitian710/sys/ali_drw.json     | 373 +++++++++++++++++++++
 .../arm64/freescale/yitian710/sys/metrics.json     |  20 ++
 tools/perf/pmu-events/jevents.py                   |   2 +
 tools/perf/util/metricgroup.c                      |  24 +-
 8 files changed, 595 insertions(+), 9 deletions(-)
 create mode 100644 tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
 create mode 100644 tools/perf/pmu-events/arch/arm64/freescale/yitian710/sys/ali_drw.json
 create mode 100644 tools/perf/pmu-events/arch/arm64/freescale/yitian710/sys/metrics.json

-- 
1.8.3.1

[PATCH v3 1/7] driver/perf: Add identifier sysfs file for CMN

[Jing Zhang]

To allow userspace to identify the specific implementation of the device,
add an "identifier" sysfs file. The "identifier" consists of model name
and revision. One of possible identifier is "arm_cmn700_0".

The perf tool can match the arm CMN metric through the identifier.

Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
---
 drivers/perf/arm-cmn.c | 79 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 71 insertions(+), 8 deletions(-)

diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index c968986..cd6962b 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -334,6 +334,7 @@ struct arm_cmn {
 
 	struct pmu pmu;
 	struct dentry *debug;
+	const char *identifier;
 };
 
 #define to_cmn(p)	container_of(p, struct arm_cmn, pmu)
@@ -347,6 +348,11 @@ struct arm_cmn_nodeid {
 	u8 dev;
 };
 
+struct arm_cmn_device_data {
+	const char * model_name;
+	enum cmn_model model;
+};
+
 static int arm_cmn_xyidbits(const struct arm_cmn *cmn)
 {
 	return fls((cmn->mesh_x - 1) | (cmn->mesh_y - 1) | 2);
@@ -1168,10 +1174,43 @@ static ssize_t arm_cmn_cpumask_show(struct device *dev,
 	.attrs = arm_cmn_cpumask_attrs,
 };
 
+static ssize_t arm_cmn_identifier_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev));
+
+	return sysfs_emit(buf, "%s\n", cmn->identifier);
+}
+
+static umode_t arm_cmn_identifier_attr_visible(struct kobject *kobj,
+					       struct attribute *attr, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev));
+
+	if (cmn->identifier == NULL)
+		return 0;
+	return attr->mode;
+}
+
+static struct device_attribute arm_cmn_identifier_attr =
+	__ATTR(identifier, 0444, arm_cmn_identifier_show, NULL);
+
+static struct attribute *arm_cmn_identifier_attrs[] = {
+	&arm_cmn_identifier_attr.attr,
+	NULL
+};
+
+static struct attribute_group arm_cmn_identifier_attr_group = {
+	.attrs = arm_cmn_identifier_attrs,
+	.is_visible = arm_cmn_identifier_attr_visible
+};
+
 static const struct attribute_group *arm_cmn_attr_groups[] = {
 	&arm_cmn_event_attrs_group,
 	&arm_cmn_format_attrs_group,
 	&arm_cmn_cpumask_attr_group,
+	&arm_cmn_identifier_attr_group,
 	NULL
 };
 
@@ -2247,13 +2286,15 @@ static int arm_cmn_probe(struct platform_device *pdev)
 	const char *name;
 	static atomic_t id;
 	int err, rootnode, this_id;
+	const struct arm_cmn_device_data * dev_data;
 
 	cmn = devm_kzalloc(&pdev->dev, sizeof(*cmn), GFP_KERNEL);
 	if (!cmn)
 		return -ENOMEM;
 
 	cmn->dev = &pdev->dev;
-	cmn->model = (unsigned long)device_get_match_data(cmn->dev);
+	dev_data = (const struct arm_cmn_device_data *)device_get_match_data(cmn->dev);
+	cmn->model = dev_data->model;
 	platform_set_drvdata(pdev, cmn);
 
 	if (cmn->model == CMN600 && has_acpi_companion(cmn->dev)) {
@@ -2281,6 +2322,8 @@ static int arm_cmn_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	cmn->identifier = devm_kasprintf(
+		cmn->dev, GFP_KERNEL, "%s_%d", dev_data->model_name, cmn->rev);
 	cmn->cpu = cpumask_local_spread(0, dev_to_node(cmn->dev));
 	cmn->pmu = (struct pmu) {
 		.module = THIS_MODULE,
@@ -2330,12 +2373,32 @@ static int arm_cmn_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct arm_cmn_device_data arm_cmn600_data = {
+	.model_name = "arm_cmn600",
+	.model = CMN600
+};
+
+static const struct arm_cmn_device_data arm_cmn650_data = {
+	.model_name = "arm_cmn650",
+	.model = CMN650
+};
+
+static const struct arm_cmn_device_data arm_cmn700_data = {
+	.model_name = "arm_cmn700",
+	.model = CMN700
+};
+
+static const struct arm_cmn_device_data arm_ci700_data = {
+	.model_name = "arm_ci700",
+	.model = CI700
+};
+
 #ifdef CONFIG_OF
 static const struct of_device_id arm_cmn_of_match[] = {
-	{ .compatible = "arm,cmn-600", .data = (void *)CMN600 },
-	{ .compatible = "arm,cmn-650", .data = (void *)CMN650 },
-	{ .compatible = "arm,cmn-700", .data = (void *)CMN700 },
-	{ .compatible = "arm,ci-700", .data = (void *)CI700 },
+	{ .compatible = "arm,cmn-600", .data = (void *)&arm_cmn600_data },
+	{ .compatible = "arm,cmn-650", .data = (void *)&arm_cmn650_data },
+	{ .compatible = "arm,cmn-700", .data = (void *)&arm_cmn700_data },
+	{ .compatible = "arm,ci-700", .data = (void *)&arm_ci700_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, arm_cmn_of_match);
@@ -2343,9 +2406,9 @@ static int arm_cmn_remove(struct platform_device *pdev)
 
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id arm_cmn_acpi_match[] = {
-	{ "ARMHC600", CMN600 },
-	{ "ARMHC650", CMN650 },
-	{ "ARMHC700", CMN700 },
+	{ "ARMHC600", (kernel_ulong_t)&arm_cmn600_data },
+	{ "ARMHC650", (kernel_ulong_t)&arm_cmn650_data },
+	{ "ARMHC700", (kernel_ulong_t)&arm_cmn700_data },
 	{}
 };
 MODULE_DEVICE_TABLE(acpi, arm_cmn_acpi_match);
-- 
1.8.3.1

[PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Jing Zhang]

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 "arm_cmn600;arm_cmn700;arm_ci700" can match the PMU
identifier "arm_cmn600X" or "arm_cmn700X" or "arm_ci700X", where "X" is a
wildcard. Tokens in Unit field are delimited by ';'.

Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
---
 tools/perf/util/metricgroup.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index f3559be..c12ccd9 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -456,6 +456,28 @@ struct metricgroup_iter_data {
 	void *data;
 };
 
+static bool match_pmu_identifier(const char *id, const char *compat)
+{
+	char *tmp = NULL, *tok, *str;
+	bool res;
+
+	str = strdup(compat);
+	if (!str)
+		return false;
+
+	tok = strtok_r(str, ";", &tmp);
+	for (; tok; tok = strtok_r(NULL, ";", &tmp)) {
+		if (!strncmp(id, tok, strlen(tok))) {
+			res = true;
+			goto out;
+		}
+	}
+	res = false;
+out:
+	free(str);
+	return res;
+}
+
 static int metricgroup__sys_event_iter(const struct pmu_metric *pm,
 				       const struct pmu_metrics_table *table,
 				       void *data)
@@ -468,7 +490,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 || !match_pmu_identifier(pmu->id, pm->compat))
 			continue;
 
 		return d->fn(pm, table, d->data);
-- 
1.8.3.1

[PATCH v3 3/7] perf vendor events: Add JSON metrics for CMN

[Jing Zhang]

Add JSON metrics for arm CMN. Currently just add part of CMN PMU
metrics which are general and compatible for any SoC and CMN-ANY.

Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
---
 .../pmu-events/arch/arm64/arm/cmn/sys/metrics.json | 74 ++++++++++++++++++++++
 tools/perf/pmu-events/jevents.py                   |  1 +
 2 files changed, 75 insertions(+)
 create mode 100644 tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json

diff --git a/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
new file mode 100644
index 0000000..e70ac1a
--- /dev/null
+++ b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
@@ -0,0 +1,74 @@
+[
+	{
+		"MetricName": "slc_miss_rate",
+		"BriefDescription": "The system level cache miss rate include.",
+		"MetricGroup": "arm_cmn",
+		"MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
+		"ScaleUnit": "100%",
+		"Unit": "arm_cmn",
+		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
+	},
+	{
+		"MetricName": "hnf_message_retry_rate",
+		"BriefDescription": "HN-F message retry rate indicates whether a lack of credits is causing the bottlenecks.",
+		"MetricGroup": "arm_cmn",
+		"MetricExpr": "hnf_pocq_retry / hnf_pocq_reqs_recvd",
+		"ScaleUnit": "100%",
+		"Unit": "arm_cmn",
+		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
+	},
+	{
+		"MetricName": "sf_hit_rate",
+		"BriefDescription": "Snoop filter hit rate can be used to measure the Snoop Filter efficiency.",
+		"MetricGroup": "arm_cmn",
+		"MetricExpr": "hnf_sf_hit / hnf_slc_sf_cache_access",
+		"ScaleUnit": "100%",
+		"Unit": "arm_cmn",
+		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
+	},
+	{
+		"MetricName": "mc_message_retry_rate",
+		"BriefDescription": "The memory controller request retries rate indicates whether the memory controller is the bottleneck.",
+		"MetricGroup": "arm_cmn",
+		"MetricExpr": "hnf_mc_retries / hnf_mc_reqs",
+		"ScaleUnit": "100%",
+		"Unit": "arm_cmn",
+		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
+	},
+	{
+		"MetricName": "rni_actual_read_bandwidth.all",
+		"BriefDescription": "This event measure the actual bandwidth(MB/sec) that RN-I bridge sends to the interconnect.",
+		"MetricGroup": "arm_cmn",
+		"MetricExpr": "rnid_rxdat_flits * 32 / 1e6 / duration_time",
+		"ScaleUnit": "1MB/s",
+		"Unit": "arm_cmn",
+		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
+	},
+	{
+		"MetricName": "rni_actual_write_bandwidth.all",
+		"BriefDescription": "This event measures the actual write bandwidth(MB/sec) at RN-I bridges.",
+		"MetricGroup": "arm_cmn",
+		"MetricExpr": "rnid_txdat_flits * 32 / 1e6 / duration_time",
+		"ScaleUnit": "1MB/s",
+		"Unit": "arm_cmn",
+		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
+	},
+	{
+		"MetricName": "rni_retry_rate",
+		"BriefDescription": "RN-I bridge retry rate indicates whether the memory controller is the bottleneck.",
+		"MetricGroup": "arm_cmn",
+		"MetricExpr": "rnid_txreq_flits_retried / rnid_txreq_flits_total",
+		"ScaleUnit": "100%",
+		"Unit": "arm_cmn",
+		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
+	},
+	{
+		"MetricName": "sbsx_actual_write_bandwidth.all",
+		"BriefDescription": "sbsx actual write bandwidth(MB/sec).",
+		"MetricGroup": "arm_cmn",
+		"MetricExpr": "sbsx_txdat_flitv * 32 / 1e6 / duration_time",
+		"ScaleUnit": "1MB/s",
+		"Unit": "arm_cmn",
+		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
+	}
+]
diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 2bcd07c..7cff2c6 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -256,6 +256,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

[PATCH v3 4/7] driver/perf: Add identifier sysfs file for Yitian 710 DDR

[Jing Zhang]

To allow userspace to identify the specific implementation of the device,
add an "identifier" sysfs file.

The perf tool can match the Yitian 710 DDR metric through the identifier.

Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
Acked-by: Ian Rogers <irogers@google.com>
Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
---
 drivers/perf/alibaba_uncore_drw_pmu.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/perf/alibaba_uncore_drw_pmu.c b/drivers/perf/alibaba_uncore_drw_pmu.c
index a7689fe..fe075fd 100644
--- a/drivers/perf/alibaba_uncore_drw_pmu.c
+++ b/drivers/perf/alibaba_uncore_drw_pmu.c
@@ -236,10 +236,37 @@ static ssize_t ali_drw_pmu_cpumask_show(struct device *dev,
 	.attrs = ali_drw_pmu_cpumask_attrs,
 };
 
+static ssize_t ali_drw_pmu_identifier_show(struct device *dev,
+					struct device_attribute *attr,
+					char *page)
+{
+	return sysfs_emit(page, "%s\n", "ali_drw_pmu");
+}
+
+static umode_t ali_drw_pmu_identifier_attr_visible(struct kobject *kobj,
+						struct attribute *attr, int n)
+{
+	return attr->mode;
+}
+
+static struct device_attribute ali_drw_pmu_identifier_attr =
+	__ATTR(identifier, 0444, ali_drw_pmu_identifier_show, NULL);
+
+static struct attribute *ali_drw_pmu_identifier_attrs[] = {
+	&ali_drw_pmu_identifier_attr.attr,
+	NULL
+};
+
+static const struct attribute_group ali_drw_pmu_identifier_attr_group = {
+	.attrs = ali_drw_pmu_identifier_attrs,
+	.is_visible = ali_drw_pmu_identifier_attr_visible
+};
+
 static const struct attribute_group *ali_drw_pmu_attr_groups[] = {
 	&ali_drw_pmu_events_attr_group,
 	&ali_drw_pmu_cpumask_attr_group,
 	&ali_drw_pmu_format_group,
+	&ali_drw_pmu_identifier_attr_group,
 	NULL,
 };
 
-- 
1.8.3.1

[PATCH v3 5/7] perf jevents: Add support for Yitian 710 DDR PMU aliasing

[Jing Zhang]

Add support for T-HEAD Yitian 710 SoC DDR PMU aliasing.

Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
Acked-by: Ian Rogers <irogers@google.com>
Reviewed-by: John Garry <john.g.garry@oracle.com>
Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>
---
 .../arm64/freescale/yitian710/sys/ali_drw.json     | 373 +++++++++++++++++++++
 tools/perf/pmu-events/jevents.py                   |   1 +
 2 files changed, 374 insertions(+)
 create mode 100644 tools/perf/pmu-events/arch/arm64/freescale/yitian710/sys/ali_drw.json

diff --git a/tools/perf/pmu-events/arch/arm64/freescale/yitian710/sys/ali_drw.json b/tools/perf/pmu-events/arch/arm64/freescale/yitian710/sys/ali_drw.json
new file mode 100644
index 0000000..2d49bf7
--- /dev/null
+++ b/tools/perf/pmu-events/arch/arm64/freescale/yitian710/sys/ali_drw.json
@@ -0,0 +1,373 @@
+[
+	{
+		"BriefDescription": "A Write or Read Op at HIF interface. The unit is 64B.",
+		"ConfigCode": "0x0",
+		"EventName": "hif_rd_or_wr",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A Write Op at HIF interface. The unit is 64B.",
+		"ConfigCode": "0x1",
+		"EventName": "hif_wr",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A Read Op at HIF interface. The unit is 64B.",
+		"ConfigCode": "0x2",
+		"EventName": "hif_rd",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A Read-Modify-Write Op at HIF interface. The unit is 64B.",
+		"ConfigCode": "0x3",
+		"EventName": "hif_rmw",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A high priority Read at HIF interface. The unit is 64B.",
+		"ConfigCode": "0x4",
+		"EventName": "hif_hi_pri_rd",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A write data cycle at DFI interface (to DRAM).",
+		"ConfigCode": "0x7",
+		"EventName": "dfi_wr_data_cycles",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A read data cycle at DFI interface (to DRAM).",
+		"ConfigCode": "0x8",
+		"EventName": "dfi_rd_data_cycles",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A high priority read becomes critical.",
+		"ConfigCode": "0x9",
+		"EventName": "hpr_xact_when_critical",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A low priority read becomes critical.",
+		"ConfigCode": "0xA",
+		"EventName": "lpr_xact_when_critical",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A write becomes critical.",
+		"ConfigCode": "0xB",
+		"EventName": "wr_xact_when_critical",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "An Activate(ACT) command to DRAM.",
+		"ConfigCode": "0xC",
+		"EventName": "op_is_activate",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A Read or Write CAS command to DRAM.",
+		"ConfigCode": "0xD",
+		"EventName": "op_is_rd_or_wr",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "An Activate(ACT) command for read to DRAM.",
+		"ConfigCode": "0xE",
+		"EventName": "op_is_rd_activate",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A Read CAS command to DRAM.",
+		"ConfigCode": "0xF",
+		"EventName": "op_is_rd",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A Write CAS command to DRAM.",
+		"ConfigCode": "0x10",
+		"EventName": "op_is_wr",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A Masked Write command to DRAM.",
+		"ConfigCode": "0x11",
+		"EventName": "op_is_mwr",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A Precharge(PRE) command to DRAM.",
+		"ConfigCode": "0x12",
+		"EventName": "op_is_precharge",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A Precharge(PRE) required by read or write.",
+		"ConfigCode": "0x13",
+		"EventName": "precharge_for_rdwr",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A Precharge(PRE) required by other conditions.",
+		"ConfigCode": "0x14",
+		"EventName": "precharge_for_other",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A read-write turnaround.",
+		"ConfigCode": "0x15",
+		"EventName": "rdwr_transitions",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A write combine(merge) in write data buffer.",
+		"ConfigCode": "0x16",
+		"EventName": "write_combine",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A Write-After-Read hazard.",
+		"ConfigCode": "0x17",
+		"EventName": "war_hazard",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A Read-After-Write hazard.",
+		"ConfigCode": "0x18",
+		"EventName": "raw_hazard",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A Write-After-Write hazard.",
+		"ConfigCode": "0x19",
+		"EventName": "waw_hazard",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "Rank0 enters self-refresh(SRE).",
+		"ConfigCode": "0x1A",
+		"EventName": "op_is_enter_selfref_rk0",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "Rank1 enters self-refresh(SRE).",
+		"ConfigCode": "0x1B",
+		"EventName": "op_is_enter_selfref_rk1",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "Rank2 enters self-refresh(SRE).",
+		"ConfigCode": "0x1C",
+		"EventName": "op_is_enter_selfref_rk2",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "Rank3 enters self-refresh(SRE).",
+		"ConfigCode": "0x1D",
+		"EventName": "op_is_enter_selfref_rk3",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "Rank0 enters power-down(PDE).",
+		"ConfigCode": "0x1E",
+		"EventName": "op_is_enter_powerdown_rk0",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "Rank1 enters power-down(PDE).",
+		"ConfigCode": "0x1F",
+		"EventName": "op_is_enter_powerdown_rk1",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "Rank2 enters power-down(PDE).",
+		"ConfigCode": "0x20",
+		"EventName": "op_is_enter_powerdown_rk2",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "Rank3 enters power-down(PDE).",
+		"ConfigCode": "0x21",
+		"EventName": "op_is_enter_powerdown_rk3",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A cycle that Rank0 stays in self-refresh mode.",
+		"ConfigCode": "0x26",
+		"EventName": "selfref_mode_rk0",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A cycle that Rank1 stays in self-refresh mode.",
+		"ConfigCode": "0x27",
+		"EventName": "selfref_mode_rk1",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A cycle that Rank2 stays in self-refresh mode.",
+		"ConfigCode": "0x28",
+		"EventName": "selfref_mode_rk2",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A cycle that Rank3 stays in self-refresh mode.",
+		"ConfigCode": "0x29",
+		"EventName": "selfref_mode_rk3",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "An auto-refresh(REF) command to DRAM.",
+		"ConfigCode": "0x2A",
+		"EventName": "op_is_refresh",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A critical auto-refresh(REF) command to DRAM.",
+		"ConfigCode": "0x2B",
+		"EventName": "op_is_crit_ref",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "An MRR or MRW command to DRAM.",
+		"ConfigCode": "0x2D",
+		"EventName": "op_is_load_mode",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A ZQCal command to DRAM.",
+		"ConfigCode": "0x2E",
+		"EventName": "op_is_zqcl",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "At least one entry in read queue reaches the visible window limit.",
+		"ConfigCode": "0x30",
+		"EventName": "visible_window_limit_reached_rd",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "At least one entry in write queue reaches the visible window limit.",
+		"ConfigCode": "0x31",
+		"EventName": "visible_window_limit_reached_wr",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A DQS Oscillator MPC command to DRAM.",
+		"ConfigCode": "0x34",
+		"EventName": "op_is_dqsosc_mpc",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A DQS Oscillator MRR command to DRAM.",
+		"ConfigCode": "0x35",
+		"EventName": "op_is_dqsosc_mrr",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A Temperature Compensated Refresh(TCR) MRR command to DRAM.",
+		"ConfigCode": "0x36",
+		"EventName": "op_is_tcr_mrr",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A ZQCal Start command to DRAM.",
+		"ConfigCode": "0x37",
+		"EventName": "op_is_zqstart",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A ZQCal Latch command to DRAM.",
+		"ConfigCode": "0x38",
+		"EventName": "op_is_zqlatch",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A packet at CHI TXREQ interface (request).",
+		"ConfigCode": "0x39",
+		"EventName": "chi_txreq",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A packet at CHI TXDAT interface (read data).",
+		"ConfigCode": "0x3A",
+		"EventName": "chi_txdat",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A packet at CHI RXDAT interface (write data).",
+		"ConfigCode": "0x3B",
+		"EventName": "chi_rxdat",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A packet at CHI RXRSP interface.",
+		"ConfigCode": "0x3C",
+		"EventName": "chi_rxrsp",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "A violation detected in TZC.",
+		"ConfigCode": "0x3D",
+		"EventName": "tsz_vio",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"BriefDescription": "The ddr cycle.",
+		"ConfigCode": "0x80",
+		"EventName": "cycle",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	}
+]
diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 7cff2c6..aff4051 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -257,6 +257,7 @@ class JsonEvent:
           'cpu_core': 'cpu_core',
           'cpu_atom': 'cpu_atom',
           'arm_cmn': 'arm_cmn',
+          'ali_drw': 'ali_drw',
       }
       return table[unit] if unit in table else f'uncore_{unit.lower()}'
 
-- 
1.8.3.1

[PATCH v3 6/7] perf vendor events: Add JSON metrics for Yitian 710 DDR

[Jing Zhang]

Add JSON metrics for T-HEAD Yitian 710 SoC DDR.

Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
Acked-by: Ian Rogers <irogers@google.com>
Reviewed-by: John Garry <john.g.garry@oracle.com>
---
 .../arch/arm64/freescale/yitian710/sys/metrics.json  | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 tools/perf/pmu-events/arch/arm64/freescale/yitian710/sys/metrics.json

diff --git a/tools/perf/pmu-events/arch/arm64/freescale/yitian710/sys/metrics.json b/tools/perf/pmu-events/arch/arm64/freescale/yitian710/sys/metrics.json
new file mode 100644
index 0000000..1a92477
--- /dev/null
+++ b/tools/perf/pmu-events/arch/arm64/freescale/yitian710/sys/metrics.json
@@ -0,0 +1,20 @@
+[
+	{
+		"MetricName": "ddr_read_bandwidth.all",
+		"BriefDescription": "The ddr read bandwidth(MB/s).",
+		"MetricGroup": "ali_drw",
+		"MetricExpr": "hif_rd * 64 / 1e6 / duration_time",
+		"ScaleUnit": "1MB/s",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	},
+	{
+		"MetricName": "ddr_write_bandwidth.all",
+		"BriefDescription": "The ddr write bandwidth(MB/s).",
+		"MetricGroup": "ali_drw",
+		"MetricExpr": "(hif_wr + hif_rmw) * 64 / 1e6 / duration_time",
+		"ScaleUnit": "1MB/s",
+		"Unit": "ali_drw",
+		"Compat": "ali_drw_pmu"
+	}
+]
-- 
1.8.3.1

[PATCH v3 7/7] docs: perf: Update metric usage for Alibaba's T-Head PMU driver

[Jing Zhang]

Alibaba's T-Head ali_drw PMU supports DDR bandwidth metrics. Update
its usage in the documentation.

Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
---
 Documentation/admin-guide/perf/alibaba_pmu.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/admin-guide/perf/alibaba_pmu.rst b/Documentation/admin-guide/perf/alibaba_pmu.rst
index 11de998..7d84002 100644
--- a/Documentation/admin-guide/perf/alibaba_pmu.rst
+++ b/Documentation/admin-guide/perf/alibaba_pmu.rst
@@ -88,6 +88,11 @@ data bandwidth::
     -e ali_drw_27080/hif_rmw/ \
     -e ali_drw_27080/cycle/ -- sleep 10
 
+Example usage of counting all memory read/write bandwidth by metric::
+
+  perf stat -M ddr_read_bandwidth.all -- sleep 10
+  perf stat -M ddr_write_bandwidth.all -- sleep 10
+
 The average DRAM bandwidth can be calculated as follows:
 
 - Read Bandwidth =  perf_hif_rd * DDRC_WIDTH * DDRC_Freq / DDRC_Cycle
-- 
1.8.3.1

Re: [PATCH v3 3/7] perf vendor events: Add JSON metrics for CMN

[Ian Rogers]

On Tue, May 30, 2023 at 2:19 AM Jing Zhang <renyu.zj@linux.alibaba.com> wrote:
>
> Add JSON metrics for arm CMN. Currently just add part of CMN PMU
> metrics which are general and compatible for any SoC and CMN-ANY.
>
> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
> ---
>  .../pmu-events/arch/arm64/arm/cmn/sys/metrics.json | 74 ++++++++++++++++++++++
>  tools/perf/pmu-events/jevents.py                   |  1 +
>  2 files changed, 75 insertions(+)
>  create mode 100644 tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
>
> diff --git a/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
> new file mode 100644
> index 0000000..e70ac1a
> --- /dev/null
> +++ b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
> @@ -0,0 +1,74 @@
> +[
> +       {
> +               "MetricName": "slc_miss_rate",
> +               "BriefDescription": "The system level cache miss rate include.",

Nit, partial sentence?

> +               "MetricGroup": "arm_cmn",
> +               "MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
> +               "ScaleUnit": "100%",
> +               "Unit": "arm_cmn",
> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +       },
> +       {
> +               "MetricName": "hnf_message_retry_rate",
> +               "BriefDescription": "HN-F message retry rate indicates whether a lack of credits is causing the bottlenecks.",
> +               "MetricGroup": "arm_cmn",
> +               "MetricExpr": "hnf_pocq_retry / hnf_pocq_reqs_recvd",
> +               "ScaleUnit": "100%",
> +               "Unit": "arm_cmn",
> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +       },
> +       {
> +               "MetricName": "sf_hit_rate",
> +               "BriefDescription": "Snoop filter hit rate can be used to measure the Snoop Filter efficiency.",

Nit, inconsistent capitalization?

> +               "MetricGroup": "arm_cmn",
> +               "MetricExpr": "hnf_sf_hit / hnf_slc_sf_cache_access",
> +               "ScaleUnit": "100%",
> +               "Unit": "arm_cmn",
> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +       },
> +       {
> +               "MetricName": "mc_message_retry_rate",
> +               "BriefDescription": "The memory controller request retries rate indicates whether the memory controller is the bottleneck.",
> +               "MetricGroup": "arm_cmn",
> +               "MetricExpr": "hnf_mc_retries / hnf_mc_reqs",
> +               "ScaleUnit": "100%",
> +               "Unit": "arm_cmn",
> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +       },
> +       {
> +               "MetricName": "rni_actual_read_bandwidth.all",
> +               "BriefDescription": "This event measure the actual bandwidth(MB/sec) that RN-I bridge sends to the interconnect.",

Nit, the MB/sec is in the ScaleUnit so could be dropped here.

> +               "MetricGroup": "arm_cmn",
> +               "MetricExpr": "rnid_rxdat_flits * 32 / 1e6 / duration_time",
> +               "ScaleUnit": "1MB/s",
> +               "Unit": "arm_cmn",
> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +       },
> +       {
> +               "MetricName": "rni_actual_write_bandwidth.all",
> +               "BriefDescription": "This event measures the actual write bandwidth(MB/sec) at RN-I bridges.",

Nit, same thing.

> +               "MetricGroup": "arm_cmn",
> +               "MetricExpr": "rnid_txdat_flits * 32 / 1e6 / duration_time",
> +               "ScaleUnit": "1MB/s",
> +               "Unit": "arm_cmn",
> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +       },
> +       {
> +               "MetricName": "rni_retry_rate",
> +               "BriefDescription": "RN-I bridge retry rate indicates whether the memory controller is the bottleneck.",
> +               "MetricGroup": "arm_cmn",
> +               "MetricExpr": "rnid_txreq_flits_retried / rnid_txreq_flits_total",
> +               "ScaleUnit": "100%",
> +               "Unit": "arm_cmn",
> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +       },
> +       {
> +               "MetricName": "sbsx_actual_write_bandwidth.all",
> +               "BriefDescription": "sbsx actual write bandwidth(MB/sec).",

Nit, same thing.

Thanks,
Ian

> +               "MetricGroup": "arm_cmn",
> +               "MetricExpr": "sbsx_txdat_flitv * 32 / 1e6 / duration_time",
> +               "ScaleUnit": "1MB/s",
> +               "Unit": "arm_cmn",
> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +       }
> +]
> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 2bcd07c..7cff2c6 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
> @@ -256,6 +256,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
>

Re: [PATCH v3 7/7] docs: perf: Update metric usage for Alibaba's T-Head PMU driver

[Ian Rogers]

On Tue, May 30, 2023 at 2:19 AM Jing Zhang <renyu.zj@linux.alibaba.com> wrote:
>
> Alibaba's T-Head ali_drw PMU supports DDR bandwidth metrics. Update
> its usage in the documentation.
>
> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  Documentation/admin-guide/perf/alibaba_pmu.rst | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/admin-guide/perf/alibaba_pmu.rst b/Documentation/admin-guide/perf/alibaba_pmu.rst
> index 11de998..7d84002 100644
> --- a/Documentation/admin-guide/perf/alibaba_pmu.rst
> +++ b/Documentation/admin-guide/perf/alibaba_pmu.rst
> @@ -88,6 +88,11 @@ data bandwidth::
>      -e ali_drw_27080/hif_rmw/ \
>      -e ali_drw_27080/cycle/ -- sleep 10
>
> +Example usage of counting all memory read/write bandwidth by metric::
> +
> +  perf stat -M ddr_read_bandwidth.all -- sleep 10
> +  perf stat -M ddr_write_bandwidth.all -- sleep 10
> +
>  The average DRAM bandwidth can be calculated as follows:
>
>  - Read Bandwidth =  perf_hif_rd * DDRC_WIDTH * DDRC_Freq / DDRC_Cycle
> --
> 1.8.3.1
>

Re: [PATCH v3 3/7] perf vendor events: Add JSON metrics for CMN

[Shuai Xue]

On 2023/5/30 17:19, 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 and CMN-ANY.

Is it a typo? You mean "any SoC integration with CMN-ANY" right?

Thanks,
Shuai

> 
> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
> ---
>  .../pmu-events/arch/arm64/arm/cmn/sys/metrics.json | 74 ++++++++++++++++++++++
>  tools/perf/pmu-events/jevents.py                   |  1 +
>  2 files changed, 75 insertions(+)
>  create mode 100644 tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
> 
> diff --git a/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
> new file mode 100644
> index 0000000..e70ac1a
> --- /dev/null
> +++ b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
> @@ -0,0 +1,74 @@
> +[
> +	{
> +		"MetricName": "slc_miss_rate",
> +		"BriefDescription": "The system level cache miss rate include.",
> +		"MetricGroup": "arm_cmn",
> +		"MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
> +		"ScaleUnit": "100%",
> +		"Unit": "arm_cmn",
> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +	},
> +	{
> +		"MetricName": "hnf_message_retry_rate",
> +		"BriefDescription": "HN-F message retry rate indicates whether a lack of credits is causing the bottlenecks.",
> +		"MetricGroup": "arm_cmn",
> +		"MetricExpr": "hnf_pocq_retry / hnf_pocq_reqs_recvd",
> +		"ScaleUnit": "100%",
> +		"Unit": "arm_cmn",
> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +	},
> +	{
> +		"MetricName": "sf_hit_rate",
> +		"BriefDescription": "Snoop filter hit rate can be used to measure the Snoop Filter efficiency.",
> +		"MetricGroup": "arm_cmn",
> +		"MetricExpr": "hnf_sf_hit / hnf_slc_sf_cache_access",
> +		"ScaleUnit": "100%",
> +		"Unit": "arm_cmn",
> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +	},
> +	{
> +		"MetricName": "mc_message_retry_rate",
> +		"BriefDescription": "The memory controller request retries rate indicates whether the memory controller is the bottleneck.",
> +		"MetricGroup": "arm_cmn",
> +		"MetricExpr": "hnf_mc_retries / hnf_mc_reqs",
> +		"ScaleUnit": "100%",
> +		"Unit": "arm_cmn",
> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +	},
> +	{
> +		"MetricName": "rni_actual_read_bandwidth.all",
> +		"BriefDescription": "This event measure the actual bandwidth(MB/sec) that RN-I bridge sends to the interconnect.",
> +		"MetricGroup": "arm_cmn",
> +		"MetricExpr": "rnid_rxdat_flits * 32 / 1e6 / duration_time",
> +		"ScaleUnit": "1MB/s",
> +		"Unit": "arm_cmn",
> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +	},
> +	{
> +		"MetricName": "rni_actual_write_bandwidth.all",
> +		"BriefDescription": "This event measures the actual write bandwidth(MB/sec) at RN-I bridges.",
> +		"MetricGroup": "arm_cmn",
> +		"MetricExpr": "rnid_txdat_flits * 32 / 1e6 / duration_time",
> +		"ScaleUnit": "1MB/s",
> +		"Unit": "arm_cmn",
> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +	},
> +	{
> +		"MetricName": "rni_retry_rate",
> +		"BriefDescription": "RN-I bridge retry rate indicates whether the memory controller is the bottleneck.",
> +		"MetricGroup": "arm_cmn",
> +		"MetricExpr": "rnid_txreq_flits_retried / rnid_txreq_flits_total",
> +		"ScaleUnit": "100%",
> +		"Unit": "arm_cmn",
> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +	},
> +	{
> +		"MetricName": "sbsx_actual_write_bandwidth.all",
> +		"BriefDescription": "sbsx actual write bandwidth(MB/sec).",
> +		"MetricGroup": "arm_cmn",
> +		"MetricExpr": "sbsx_txdat_flitv * 32 / 1e6 / duration_time",
> +		"ScaleUnit": "1MB/s",
> +		"Unit": "arm_cmn",
> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +	}
> +]
> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 2bcd07c..7cff2c6 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
> @@ -256,6 +256,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()}'
>  

Re: [PATCH v3 1/7] driver/perf: Add identifier sysfs file for CMN

[kernel test robot]

Hi Jing,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tip/perf/core]
[also build test WARNING on acme/perf/core arm-perf/for-next/perf linus/master v6.4-rc4 next-20230530]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jing-Zhang/driver-perf-Add-identifier-sysfs-file-for-CMN/20230530-172139
base:   tip/perf/core
patch link:    https://lore.kernel.org/r/1685438374-33287-2-git-send-email-renyu.zj%40linux.alibaba.com
patch subject: [PATCH v3 1/7] driver/perf: Add identifier sysfs file for CMN
config: hexagon-buildonly-randconfig-r006-20230530 (https://download.01.org/0day-ci/archive/20230531/202305311005.YR2SisSe-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/c7483d062f1b91e98b028fa720b8a98b94ec9bc5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jing-Zhang/driver-perf-Add-identifier-sysfs-file-for-CMN/20230530-172139
        git checkout c7483d062f1b91e98b028fa720b8a98b94ec9bc5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/perf/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305311005.YR2SisSe-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/perf/arm-cmn.c:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/perf/arm-cmn.c:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/perf/arm-cmn.c:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
>> drivers/perf/arm-cmn.c:2379:41: warning: unused variable 'arm_cmn600_data' [-Wunused-const-variable]
   static const struct arm_cmn_device_data arm_cmn600_data = {
                                           ^
>> drivers/perf/arm-cmn.c:2384:41: warning: unused variable 'arm_cmn650_data' [-Wunused-const-variable]
   static const struct arm_cmn_device_data arm_cmn650_data = {
                                           ^
>> drivers/perf/arm-cmn.c:2389:41: warning: unused variable 'arm_cmn700_data' [-Wunused-const-variable]
   static const struct arm_cmn_device_data arm_cmn700_data = {
                                           ^
>> drivers/perf/arm-cmn.c:2394:41: warning: unused variable 'arm_ci700_data' [-Wunused-const-variable]
   static const struct arm_cmn_device_data arm_ci700_data = {
                                           ^
   10 warnings generated.


vim +/arm_cmn600_data +2379 drivers/perf/arm-cmn.c

  2378	
> 2379	static const struct arm_cmn_device_data arm_cmn600_data = {
  2380		.model_name = "arm_cmn600",
  2381		.model = CMN600
  2382	};
  2383	
> 2384	static const struct arm_cmn_device_data arm_cmn650_data = {
  2385		.model_name = "arm_cmn650",
  2386		.model = CMN650
  2387	};
  2388	
> 2389	static const struct arm_cmn_device_data arm_cmn700_data = {
  2390		.model_name = "arm_cmn700",
  2391		.model = CMN700
  2392	};
  2393	
> 2394	static const struct arm_cmn_device_data arm_ci700_data = {
  2395		.model_name = "arm_ci700",
  2396		.model = CI700
  2397	};
  2398	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[John Garry]

On 30/05/2023 10:19, 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 "arm_cmn600;arm_cmn700;arm_ci700" can match the PMU
> identifier "arm_cmn600X" or "arm_cmn700X" or "arm_ci700X", where "X" is a
> wildcard.

 From checking the driver, it seems that we have model names 
"arm_cmn600" and "arm_cmn650". Are you saying that "arm_cmn600X" would 
match for those? I am most curious about how "arm_cmn600X" matches 
"arm_cmn650".

> Tokens in Unit field are delimited by ';'.

Thanks for taking a stab at solving this problem.

I have to admit that I am not the biggest fan of having multiple values 
to match in the "Compat" value possibly for every event. It doesn't 
really scale.

I would hope that there are at least some events which we are guaranteed 
to always be present. From what Robin said on the v2 series, for the 
implementations which we care about, events are generally added per 
subsequent version. So we should have some base set of fixed events.

If we are confident that we have a fixed set of base set of events, can 
we ensure that those events would not require this compat string which 
needs each version explicitly stated?

Robin, please let us know what you think of this.

Thanks,
John

> 
> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
> ---
>   tools/perf/util/metricgroup.c | 24 +++++++++++++++++++++++-
>   1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index f3559be..c12ccd9 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -456,6 +456,28 @@ struct metricgroup_iter_data {
>   	void *data;
>   };
>   
> +static bool match_pmu_identifier(const char *id, const char *compat)
> +{
> +	char *tmp = NULL, *tok, *str;
> +	bool res;
> +
> +	str = strdup(compat);
> +	if (!str)
> +		return false;
> +
> +	tok = strtok_r(str, ";", &tmp);
> +	for (; tok; tok = strtok_r(NULL, ";", &tmp)) {
> +		if (!strncmp(id, tok, strlen(tok))) {
> +			res = true;
> +			goto out;
> +		}
> +	}
> +	res = false;
> +out:
> +	free(str);
> +	return res;
> +}
> +
>   static int metricgroup__sys_event_iter(const struct pmu_metric *pm,
>   				       const struct pmu_metrics_table *table,
>   				       void *data)
> @@ -468,7 +490,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 || !match_pmu_identifier(pmu->id, pm->compat))
>   			continue;
>   
>   		return d->fn(pm, table, d->data);

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Jing Zhang]

在 2023/5/31 下午9:18, John Garry 写道:
> On 30/05/2023 10:19, 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 "arm_cmn600;arm_cmn700;arm_ci700" can match the PMU
>> identifier "arm_cmn600X" or "arm_cmn700X" or "arm_ci700X", where "X" is a
>> wildcard.
> 
> From checking the driver, it seems that we have model names "arm_cmn600" and "arm_cmn650". Are you saying that "arm_cmn600X" would match for those? I am most curious about how "arm_cmn600X" matches "arm_cmn650".
> 

Hi John,

From patch #1 we have identifiers "arm_cmn600_0" and "arm_cmn650_0" etc. The identifier consists of model_name and revision.
The compatible value "arm_cmn600;arm_cmn650" can match the identifier "arm_cmn600_0" or "arm_cmn650_0". Maybe the message log
is not clear enough.

For example in patch #3 the metric "slc_miss_rate" is a generic metric for cmn-any. So we can define:

+	{
+		"MetricName": "slc_miss_rate",
+		"BriefDescription": "The system level cache miss rate include.",
+		"MetricGroup": "arm_cmn",
+		"MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
+		"ScaleUnit": "100%",
+		"Unit": "arm_cmn",
+		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
+	},


It can match identifiers "arm_cmn600_{0,1,2..X}" or "arm_cmn650_{0,1,2..X}" or "arm_cmn700_{0,1,2..X}" or "arm_ci700_{ 0,1,2..X}".
In other words, it can match all identifiers prefixed with “arm_cmn600” or “arm_cmn650” or “arm_cmn700” or “arm_ci700”.

If a new model arm_cmn driver with identifier "arm_cmn750_0", it will not be matched, but if a new revision arm_cmn driver with identifier
"arm_cmn700_4", it can be matched.


>> Tokens in Unit field are delimited by ';'.
> 
> Thanks for taking a stab at solving this problem.
> 
> I have to admit that I am not the biggest fan of having multiple values to match in the "Compat" value possibly for every event. It doesn't really scale.
> 
> I would hope that there are at least some events which we are guaranteed to always be present. From what Robin said on the v2 series, for the implementations which we care about, events are generally added per subsequent version. So we should have some base set of fixed events.
> 
> If we are confident that we have a fixed set of base set of events, can we ensure that those events would not require this compat string which needs each version explicitly stated?
> 

If we are sure that some events will always exist in subsequent versions, we can set the Compat field to "arm_cmn;arm_ci". After that,
whether it is a different model or a different revision of the cmn PMU, it will be compatible. That is, it matches all whose identifier
is prefixed with “arm_cmn” or “arm_ci”.

Maybe it's not a perfect solution yet, looking forward to your suggestions.


Thanks,
Jing


> Robin, please let us know what you think of this.
> 
> Thanks,
> John
> 
>>
>> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
>> ---
>>   tools/perf/util/metricgroup.c | 24 +++++++++++++++++++++++-
>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>> index f3559be..c12ccd9 100644
>> --- a/tools/perf/util/metricgroup.c
>> +++ b/tools/perf/util/metricgroup.c
>> @@ -456,6 +456,28 @@ struct metricgroup_iter_data {
>>       void *data;
>>   };
>>   +static bool match_pmu_identifier(const char *id, const char *compat)
>> +{
>> +    char *tmp = NULL, *tok, *str;
>> +    bool res;
>> +
>> +    str = strdup(compat);
>> +    if (!str)
>> +        return false;
>> +
>> +    tok = strtok_r(str, ";", &tmp);
>> +    for (; tok; tok = strtok_r(NULL, ";", &tmp)) {
>> +        if (!strncmp(id, tok, strlen(tok))) {
>> +            res = true;
>> +            goto out;
>> +        }
>> +    }
>> +    res = false;
>> +out:
>> +    free(str);
>> +    return res;
>> +}
>> +
>>   static int metricgroup__sys_event_iter(const struct pmu_metric *pm,
>>                          const struct pmu_metrics_table *table,
>>                          void *data)
>> @@ -468,7 +490,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 || !match_pmu_identifier(pmu->id, pm->compat))
>>               continue;
>>             return d->fn(pm, table, d->data);

Re: [PATCH v3 3/7] perf vendor events: Add JSON metrics for CMN

[Jing Zhang]

在 2023/5/31 上午9:18, Ian Rogers 写道:
> On Tue, May 30, 2023 at 2:19 AM Jing Zhang <renyu.zj@linux.alibaba.com> wrote:
>>
>> Add JSON metrics for arm CMN. Currently just add part of CMN PMU
>> metrics which are general and compatible for any SoC and CMN-ANY.
>>
>> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
>> ---
>>  .../pmu-events/arch/arm64/arm/cmn/sys/metrics.json | 74 ++++++++++++++++++++++
>>  tools/perf/pmu-events/jevents.py                   |  1 +
>>  2 files changed, 75 insertions(+)
>>  create mode 100644 tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
>>
>> diff --git a/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
>> new file mode 100644
>> index 0000000..e70ac1a
>> --- /dev/null
>> +++ b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
>> @@ -0,0 +1,74 @@
>> +[
>> +       {
>> +               "MetricName": "slc_miss_rate",
>> +               "BriefDescription": "The system level cache miss rate include.",
> 
> Nit, partial sentence?

Yes my mistake.

> 
>> +               "MetricGroup": "arm_cmn",
>> +               "MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
>> +               "ScaleUnit": "100%",
>> +               "Unit": "arm_cmn",
>> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +       },
>> +       {
>> +               "MetricName": "hnf_message_retry_rate",
>> +               "BriefDescription": "HN-F message retry rate indicates whether a lack of credits is causing the bottlenecks.",
>> +               "MetricGroup": "arm_cmn",
>> +               "MetricExpr": "hnf_pocq_retry / hnf_pocq_reqs_recvd",
>> +               "ScaleUnit": "100%",
>> +               "Unit": "arm_cmn",
>> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +       },
>> +       {
>> +               "MetricName": "sf_hit_rate",
>> +               "BriefDescription": "Snoop filter hit rate can be used to measure the Snoop Filter efficiency.",
> 
> Nit, inconsistent capitalization?

Ok, fix it in next version.

> 
>> +               "MetricGroup": "arm_cmn",
>> +               "MetricExpr": "hnf_sf_hit / hnf_slc_sf_cache_access",
>> +               "ScaleUnit": "100%",
>> +               "Unit": "arm_cmn",
>> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +       },
>> +       {
>> +               "MetricName": "mc_message_retry_rate",
>> +               "BriefDescription": "The memory controller request retries rate indicates whether the memory controller is the bottleneck.",
>> +               "MetricGroup": "arm_cmn",
>> +               "MetricExpr": "hnf_mc_retries / hnf_mc_reqs",
>> +               "ScaleUnit": "100%",
>> +               "Unit": "arm_cmn",
>> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +       },
>> +       {
>> +               "MetricName": "rni_actual_read_bandwidth.all",
>> +               "BriefDescription": "This event measure the actual bandwidth(MB/sec) that RN-I bridge sends to the interconnect.",
> 
> Nit, the MB/sec is in the ScaleUnit so could be dropped here.

ok

> 
>> +               "MetricGroup": "arm_cmn",
>> +               "MetricExpr": "rnid_rxdat_flits * 32 / 1e6 / duration_time",
>> +               "ScaleUnit": "1MB/s",
>> +               "Unit": "arm_cmn",
>> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +       },
>> +       {
>> +               "MetricName": "rni_actual_write_bandwidth.all",
>> +               "BriefDescription": "This event measures the actual write bandwidth(MB/sec) at RN-I bridges.",
> 
> Nit, same thing.

ok

> 
>> +               "MetricGroup": "arm_cmn",
>> +               "MetricExpr": "rnid_txdat_flits * 32 / 1e6 / duration_time",
>> +               "ScaleUnit": "1MB/s",
>> +               "Unit": "arm_cmn",
>> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +       },
>> +       {
>> +               "MetricName": "rni_retry_rate",
>> +               "BriefDescription": "RN-I bridge retry rate indicates whether the memory controller is the bottleneck.",
>> +               "MetricGroup": "arm_cmn",
>> +               "MetricExpr": "rnid_txreq_flits_retried / rnid_txreq_flits_total",
>> +               "ScaleUnit": "100%",
>> +               "Unit": "arm_cmn",
>> +               "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +       },
>> +       {
>> +               "MetricName": "sbsx_actual_write_bandwidth.all",
>> +               "BriefDescription": "sbsx actual write bandwidth(MB/sec).",
> 
> Nit, same thing.
> 

Ok, Thanks Ian.

Re: [PATCH v3 3/7] perf vendor events: Add JSON metrics for CMN

[Jing Zhang]

在 2023/5/31 上午10:43, Shuai Xue 写道:
> 
> 
> On 2023/5/30 17:19, 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 and CMN-ANY.
> 
> Is it a typo? You mean "any SoC integration with CMN-ANY" right?
> 

Yes, I will fix it in the next version.

Thanks,
Jing

> Thanks,
> Shuai
> 
>>
>> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
>> ---
>>  .../pmu-events/arch/arm64/arm/cmn/sys/metrics.json | 74 ++++++++++++++++++++++
>>  tools/perf/pmu-events/jevents.py                   |  1 +
>>  2 files changed, 75 insertions(+)
>>  create mode 100644 tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
>>
>> diff --git a/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
>> new file mode 100644
>> index 0000000..e70ac1a
>> --- /dev/null
>> +++ b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
>> @@ -0,0 +1,74 @@
>> +[
>> +	{
>> +		"MetricName": "slc_miss_rate",
>> +		"BriefDescription": "The system level cache miss rate include.",
>> +		"MetricGroup": "arm_cmn",
>> +		"MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
>> +		"ScaleUnit": "100%",
>> +		"Unit": "arm_cmn",
>> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +	},
>> +	{
>> +		"MetricName": "hnf_message_retry_rate",
>> +		"BriefDescription": "HN-F message retry rate indicates whether a lack of credits is causing the bottlenecks.",
>> +		"MetricGroup": "arm_cmn",
>> +		"MetricExpr": "hnf_pocq_retry / hnf_pocq_reqs_recvd",
>> +		"ScaleUnit": "100%",
>> +		"Unit": "arm_cmn",
>> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +	},
>> +	{
>> +		"MetricName": "sf_hit_rate",
>> +		"BriefDescription": "Snoop filter hit rate can be used to measure the Snoop Filter efficiency.",
>> +		"MetricGroup": "arm_cmn",
>> +		"MetricExpr": "hnf_sf_hit / hnf_slc_sf_cache_access",
>> +		"ScaleUnit": "100%",
>> +		"Unit": "arm_cmn",
>> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +	},
>> +	{
>> +		"MetricName": "mc_message_retry_rate",
>> +		"BriefDescription": "The memory controller request retries rate indicates whether the memory controller is the bottleneck.",
>> +		"MetricGroup": "arm_cmn",
>> +		"MetricExpr": "hnf_mc_retries / hnf_mc_reqs",
>> +		"ScaleUnit": "100%",
>> +		"Unit": "arm_cmn",
>> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +	},
>> +	{
>> +		"MetricName": "rni_actual_read_bandwidth.all",
>> +		"BriefDescription": "This event measure the actual bandwidth(MB/sec) that RN-I bridge sends to the interconnect.",
>> +		"MetricGroup": "arm_cmn",
>> +		"MetricExpr": "rnid_rxdat_flits * 32 / 1e6 / duration_time",
>> +		"ScaleUnit": "1MB/s",
>> +		"Unit": "arm_cmn",
>> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +	},
>> +	{
>> +		"MetricName": "rni_actual_write_bandwidth.all",
>> +		"BriefDescription": "This event measures the actual write bandwidth(MB/sec) at RN-I bridges.",
>> +		"MetricGroup": "arm_cmn",
>> +		"MetricExpr": "rnid_txdat_flits * 32 / 1e6 / duration_time",
>> +		"ScaleUnit": "1MB/s",
>> +		"Unit": "arm_cmn",
>> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +	},
>> +	{
>> +		"MetricName": "rni_retry_rate",
>> +		"BriefDescription": "RN-I bridge retry rate indicates whether the memory controller is the bottleneck.",
>> +		"MetricGroup": "arm_cmn",
>> +		"MetricExpr": "rnid_txreq_flits_retried / rnid_txreq_flits_total",
>> +		"ScaleUnit": "100%",
>> +		"Unit": "arm_cmn",
>> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +	},
>> +	{
>> +		"MetricName": "sbsx_actual_write_bandwidth.all",
>> +		"BriefDescription": "sbsx actual write bandwidth(MB/sec).",
>> +		"MetricGroup": "arm_cmn",
>> +		"MetricExpr": "sbsx_txdat_flitv * 32 / 1e6 / duration_time",
>> +		"ScaleUnit": "1MB/s",
>> +		"Unit": "arm_cmn",
>> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +	}
>> +]
>> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
>> index 2bcd07c..7cff2c6 100755
>> --- a/tools/perf/pmu-events/jevents.py
>> +++ b/tools/perf/pmu-events/jevents.py
>> @@ -256,6 +256,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()}'
>>  

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[John Garry]

On 01/06/2023 09:58, Jing Zhang wrote:
>>  From checking the driver, it seems that we have model names "arm_cmn600" and "arm_cmn650". Are you saying that "arm_cmn600X" would match for those? I am most curious about how "arm_cmn600X" matches "arm_cmn650".
>>
> Hi John,
> 
>  From patch #1 we have identifiers "arm_cmn600_0" and "arm_cmn650_0" etc. 

ok, I see. Your idea for the cmn driver HW identifier format is odd to 
me. Your HW identifier is a mix of the HW IP model name (from 
arm_cmn_device_data.model_name) with some the kernel revision identifier 
(from cmn_revision). The kernel revision identifier is an enum, and I 
don't think that it is a good idea to expose enum values through sysfs 
files.

I assume that there is some ordering requirement for cmn_revision, 
considering that we have equality operations on the revision in the driver.

> The identifier consists of model_name and revision.
> The compatible value "arm_cmn600;arm_cmn650" can match the identifier "arm_cmn600_0" or "arm_cmn650_0". Maybe the message log
> is not clear enough.
> 
> For example in patch #3 the metric "slc_miss_rate" is a generic metric for cmn-any. So we can define:
> 
> +	{
> +		"MetricName": "slc_miss_rate",
> +		"BriefDescription": "The system level cache miss rate include.",
> +		"MetricGroup": "arm_cmn",
> +		"MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
> +		"ScaleUnit": "100%",
> +		"Unit": "arm_cmn",
> +		"Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
> +	},
> 
> 
> It can match identifiers "arm_cmn600_{0,1,2..X}" or "arm_cmn650_{0,1,2..X}" or "arm_cmn700_{0,1,2..X}" or "arm_ci700_{ 0,1,2..X}".
> In other words, it can match all identifiers prefixed with “arm_cmn600” or “arm_cmn650” or “arm_cmn700” or “arm_ci700”.
> 
> If a new model arm_cmn driver with identifier "arm_cmn750_0", it will not be matched, but if a new revision arm_cmn driver with identifier
> "arm_cmn700_4", it can be matched.

OK, I see what you mean. My confusion came about though your commit 
message on this same patch, which did not mention cmn650. I assumed that 
the example event which you were describing was supported for arm_cmn650 
and you intentionally omitted it.

> 
> 
>>> Tokens in Unit field are delimited by ';'.
>> Thanks for taking a stab at solving this problem.
>>
>> I have to admit that I am not the biggest fan of having multiple values to match in the "Compat" value possibly for every event. It doesn't really scale.
>>
>> I would hope that there are at least some events which we are guaranteed to always be present. From what Robin said on the v2 series, for the implementations which we care about, events are generally added per subsequent version. So we should have some base set of fixed events.
>>
>> If we are confident that we have a fixed set of base set of events, can we ensure that those events would not require this compat string which needs each version explicitly stated?
>>
> If we are sure that some events will always exist in subsequent versions, we can set the Compat field to "arm_cmn;arm_ci". After that,
> whether it is a different model or a different revision of the cmn PMU, it will be compatible. That is, it matches all whose identifier
> is prefixed with “arm_cmn” or “arm_ci”.

Sure, we could do something like that. Or if we are super-confident that 
every model and rev will support some event, then we can change perf 
tool to just not check Compat for that event.

> 
> Maybe it's not a perfect solution yet, looking forward to your suggestions.

Well first we need to define kernel HW identifier format. I don't know 
why we don't encode model and revision name, like "cmn650_r1p1". Robin?

Thanks,
John

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Jing Zhang]

在 2023/6/3 上午12:20, John Garry 写道:
> On 01/06/2023 09:58, Jing Zhang wrote:
>>>  From checking the driver, it seems that we have model names "arm_cmn600" and "arm_cmn650". Are you saying that "arm_cmn600X" would match for those? I am most curious about how "arm_cmn600X" matches "arm_cmn650".
>>>
>> Hi John,
>>
>>  From patch #1 we have identifiers "arm_cmn600_0" and "arm_cmn650_0" etc. 
> 
> ok, I see. Your idea for the cmn driver HW identifier format is odd to me. Your HW identifier is a mix of the HW IP model name (from arm_cmn_device_data.model_name) with some the kernel revision identifier (from cmn_revision). The kernel revision identifier is an enum, and I don't think that it is a good idea to expose enum values through sysfs files.
> 
> I assume that there is some ordering requirement for cmn_revision, considering that we have equality operations on the revision in the driver.
> 

Actually revision is a register value rather than an enumeration value. If I modify the revision to r0p0, etc., it is also possible, but I need
to convert the enumeration to a string.


>> The identifier consists of model_name and revision.
>> The compatible value "arm_cmn600;arm_cmn650" can match the identifier "arm_cmn600_0" or "arm_cmn650_0". Maybe the message log
>> is not clear enough.
>>
>> For example in patch #3 the metric "slc_miss_rate" is a generic metric for cmn-any. So we can define:
>>
>> +    {
>> +        "MetricName": "slc_miss_rate",
>> +        "BriefDescription": "The system level cache miss rate include.",
>> +        "MetricGroup": "arm_cmn",
>> +        "MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
>> +        "ScaleUnit": "100%",
>> +        "Unit": "arm_cmn",
>> +        "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +    },
>>
>>
>> It can match identifiers "arm_cmn600_{0,1,2..X}" or "arm_cmn650_{0,1,2..X}" or "arm_cmn700_{0,1,2..X}" or "arm_ci700_{ 0,1,2..X}".
>> In other words, it can match all identifiers prefixed with “arm_cmn600” or “arm_cmn650” or “arm_cmn700” or “arm_ci700”.
>>
>> If a new model arm_cmn driver with identifier "arm_cmn750_0", it will not be matched, but if a new revision arm_cmn driver with identifier
>> "arm_cmn700_4", it can be matched.
> 
> OK, I see what you mean. My confusion came about though your commit message on this same patch, which did not mention cmn650. I assumed that the example event which you were describing was supported for arm_cmn650 and you intentionally omitted it.
> 

Yes, I didn't write cmn650 in the commit message, just because I omitted it. I will add it in a later version.

>>
>>
>>>> Tokens in Unit field are delimited by ';'.
>>> Thanks for taking a stab at solving this problem.
>>>
>>> I have to admit that I am not the biggest fan of having multiple values to match in the "Compat" value possibly for every event. It doesn't really scale.
>>>
>>> I would hope that there are at least some events which we are guaranteed to always be present. From what Robin said on the v2 series, for the implementations which we care about, events are generally added per subsequent version. So we should have some base set of fixed events.
>>>
>>> If we are confident that we have a fixed set of base set of events, can we ensure that those events would not require this compat string which needs each version explicitly stated?
>>>
>> If we are sure that some events will always exist in subsequent versions, we can set the Compat field to "arm_cmn;arm_ci". After that,
>> whether it is a different model or a different revision of the cmn PMU, it will be compatible. That is, it matches all whose identifier
>> is prefixed with “arm_cmn” or “arm_ci”.
> 
> Sure, we could do something like that. Or if we are super-confident that every model and rev will support some event, then we can change perf tool to just not check Compat for that event.
> 

Yes, I have also thought about this solution. If it is an event supported by all versions, as long as it meets the Unit match, it does not need
to check Compat. However, the current perf tools tool seems to ignore the "Unit" inspection for the metric event.

>>
>> Maybe it's not a perfect solution yet, looking forward to your suggestions.
> 
> Well first we need to define kernel HW identifier format. I don't know why we don't encode model and revision name, like "cmn650_r1p1". Robin?
> 

As mentioned earlier, revision is a register value rather than an enumeration value. If change revision to revision name, I need a more redundant
operation of converting enumeration value to string. If you think changing the naming method to "cmn650_r1p1" is clearer, of course
there is no problem.

Thanks,
Jing

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Arnaldo Carvalho de Melo]

Em Mon, Jun 05, 2023 at 10:46:36AM +0800, Jing Zhang escreveu:
> 在 2023/6/3 上午12:20, John Garry 写道:
> > On 01/06/2023 09:58, Jing Zhang wrote:
> >> It can match identifiers "arm_cmn600_{0,1,2..X}" or "arm_cmn650_{0,1,2..X}" or "arm_cmn700_{0,1,2..X}" or "arm_ci700_{ 0,1,2..X}".
> >> In other words, it can match all identifiers prefixed with “arm_cmn600” or “arm_cmn650” or “arm_cmn700” or “arm_ci700”.
> >>
> >> If a new model arm_cmn driver with identifier "arm_cmn750_0", it will not be matched, but if a new revision arm_cmn driver with identifier
> >> "arm_cmn700_4", it can be matched.
> > 
> > OK, I see what you mean. My confusion came about though your commit message on this same patch, which did not mention cmn650. I assumed that the example event which you were describing was supported for arm_cmn650 and you intentionally omitted it.
> > 
> 
> Yes, I didn't write cmn650 in the commit message, just because I omitted it. I will add it in a later version.

Ok, will wait for v4 then.

- Arnaldo

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[John Garry]

On 05/06/2023 03:46, Jing Zhang wrote:
>> I assume that there is some ordering requirement for cmn_revision, considering that we have equality operations on the revision in the driver.
>>
> Actually revision is a register value rather than an enumeration value.

ok, got it.

> If I modify the revision to r0p0, etc., it is also possible, but I need
> to convert the enumeration to a string.

understood

> 
> 
>>> The identifier consists of model_name and revision.
>>> The compatible value "arm_cmn600;arm_cmn650" can match the identifier "arm_cmn600_0" or "arm_cmn650_0". Maybe the message log
>>> is not clear enough.
>>>
>>> For example in patch #3 the metric "slc_miss_rate" is a generic metric for cmn-any. So we can define:
>>>
>>> +    {
>>> +        "MetricName": "slc_miss_rate",
>>> +        "BriefDescription": "The system level cache miss rate include.",
>>> +        "MetricGroup": "arm_cmn",
>>> +        "MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
>>> +        "ScaleUnit": "100%",
>>> +        "Unit": "arm_cmn",
>>> +        "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>>> +    },
>>>
>>>
>>> It can match identifiers "arm_cmn600_{0,1,2..X}" or "arm_cmn650_{0,1,2..X}" or "arm_cmn700_{0,1,2..X}" or "arm_ci700_{ 0,1,2..X}".
>>> In other words, it can match all identifiers prefixed with “arm_cmn600” or “arm_cmn650” or “arm_cmn700” or “arm_ci700”.
>>>
>>> If a new model arm_cmn driver with identifier "arm_cmn750_0", it will not be matched, but if a new revision arm_cmn driver with identifier
>>> "arm_cmn700_4", it can be matched.
>> OK, I see what you mean. My confusion came about though your commit message on this same patch, which did not mention cmn650. I assumed that the example event which you were describing was supported for arm_cmn650 and you intentionally omitted it.
>>
> Yes, I didn't write cmn650 in the commit message, just because I omitted it. I will add it in a later version.
> 
>>>
>>>>> Tokens in Unit field are delimited by ';'.
>>>> Thanks for taking a stab at solving this problem.
>>>>
>>>> I have to admit that I am not the biggest fan of having multiple values to match in the "Compat" value possibly for every event. It doesn't really scale.
>>>>
>>>> I would hope that there are at least some events which we are guaranteed to always be present. From what Robin said on the v2 series, for the implementations which we care about, events are generally added per subsequent version. So we should have some base set of fixed events.
>>>>
>>>> If we are confident that we have a fixed set of base set of events, can we ensure that those events would not require this compat string which needs each version explicitly stated?
>>>>
>>> If we are sure that some events will always exist in subsequent versions, we can set the Compat field to "arm_cmn;arm_ci". After that,
>>> whether it is a different model or a different revision of the cmn PMU, it will be compatible. That is, it matches all whose identifier
>>> is prefixed with “arm_cmn” or “arm_ci”.
>> Sure, we could do something like that. Or if we are super-confident that every model and rev will support some event, then we can change perf tool to just not check Compat for that event.
>>
> Yes, I have also thought about this solution. If it is an event supported by all versions, as long as it meets the Unit match, it does not need
> to check Compat.


>However, the current perf tools tool seems to ignore the "Unit" inspection for the metric event.

Unit is the format of the event_source device name. We should match 
based on that as well as compat. I need to check the code again to 
understand how that is done... it has changed a good bit in 3 years.

> 
>>> Maybe it's not a perfect solution yet, looking forward to your suggestions.
>> Well first we need to define kernel HW identifier format. I don't know why we don't encode model and revision name, like "cmn650_r1p1". Robin?
>>
> As mentioned earlier, revision is a register value rather than an enumeration value. If change revision to revision name, I need a more redundant
> operation of converting enumeration value to string. If you think changing the naming method to "cmn650_r1p1" is clearer, of course
> there is no problem.

It's just odd to mix a string and revision number in this way.

Robin knows more about this HW than me, so I'll let him decide on the 
the preferred format.

Thanks,
John

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Robin Murphy]

On 02/06/2023 5:20 pm, John Garry wrote:
> On 01/06/2023 09:58, Jing Zhang wrote:
>>>  From checking the driver, it seems that we have model names 
>>> "arm_cmn600" and "arm_cmn650". Are you saying that "arm_cmn600X" 
>>> would match for those? I am most curious about how "arm_cmn600X" 
>>> matches "arm_cmn650".
>>>
>> Hi John,
>>
>>  From patch #1 we have identifiers "arm_cmn600_0" and "arm_cmn650_0" etc. 
> 
> ok, I see. Your idea for the cmn driver HW identifier format is odd to 
> me. Your HW identifier is a mix of the HW IP model name (from 
> arm_cmn_device_data.model_name) with some the kernel revision identifier 
> (from cmn_revision). The kernel revision identifier is an enum, and I 
> don't think that it is a good idea to expose enum values through sysfs 
> files.
> 
> I assume that there is some ordering requirement for cmn_revision, 
> considering that we have equality operations on the revision in the driver.

That enum does actually follow the revision identifiers as provided by 
the hardware (see CMN_CFGM_PID2_REVISION), so I don't see any major 
issue with putting it into user ABI. And TBH I think I would prefer to 
just use a numeric value rather than have to maintain yet more tables of 
strings which given the usage model here would effectively only mangle a 
matchable value into a different matchable value anyway.

I am inclined to agree that the mix between part 
driver-generated-string, part hardware-value looks a little funky. I 
still need to check with the hardware team exactly how the part number 
field from PERIPH_ID_0/1 is "configuration-dependent", and whether there 
might actually be a chance of using that as well.

One nagging doubt that remains for metrics are any baked-in assumptions 
which may not always simply depend on the product version - for instance 
it happens to be the case currently that everything has a fixed flit 
size of 256 bits, hence the magic "32" in the bandwidth calculations, 
but if that ever became configurable in some future product, we may 
potentially have a problem guaranteeing a meaningful calculation.

>> The identifier consists of model_name and revision.
>> The compatible value "arm_cmn600;arm_cmn650" can match the identifier 
>> "arm_cmn600_0" or "arm_cmn650_0". Maybe the message log
>> is not clear enough.
>>
>> For example in patch #3 the metric "slc_miss_rate" is a generic metric 
>> for cmn-any. So we can define:
>>
>> +    {
>> +        "MetricName": "slc_miss_rate",
>> +        "BriefDescription": "The system level cache miss rate include.",
>> +        "MetricGroup": "arm_cmn",
>> +        "MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
>> +        "ScaleUnit": "100%",
>> +        "Unit": "arm_cmn",
>> +        "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>> +    },
>>
>>
>> It can match identifiers "arm_cmn600_{0,1,2..X}" or 
>> "arm_cmn650_{0,1,2..X}" or "arm_cmn700_{0,1,2..X}" or "arm_ci700_{ 
>> 0,1,2..X}".
>> In other words, it can match all identifiers prefixed with 
>> “arm_cmn600” or “arm_cmn650” or “arm_cmn700” or “arm_ci700”.
>>
>> If a new model arm_cmn driver with identifier "arm_cmn750_0", it will 
>> not be matched, but if a new revision arm_cmn driver with identifier
>> "arm_cmn700_4", it can be matched.
> 
> OK, I see what you mean. My confusion came about though your commit 
> message on this same patch, which did not mention cmn650. I assumed that 
> the example event which you were describing was supported for arm_cmn650 
> and you intentionally omitted it.
> 
>>
>>
>>>> Tokens in Unit field are delimited by ';'.
>>> Thanks for taking a stab at solving this problem.
>>>
>>> I have to admit that I am not the biggest fan of having multiple 
>>> values to match in the "Compat" value possibly for every event. It 
>>> doesn't really scale.
>>>
>>> I would hope that there are at least some events which we are 
>>> guaranteed to always be present. From what Robin said on the v2 
>>> series, for the implementations which we care about, events are 
>>> generally added per subsequent version. So we should have some base 
>>> set of fixed events.

Note that there's a slight difference between "present" and "valid", 
e.g. in the current driver-internal aliases, all MTSX events are marked 
CMN_ANY, meaning they're considered valid on any CMN configuration with 
an MTSX node, regardless of model. The events don't exist on CMN-600 or 
CMN-650, but that's because the MTSX itself wasn't a thing yet, so for 
simplicity we don't have to bother considering the events invalid when 
we know they will always be non-present and thus filtered anyway.

>>> If we are confident that we have a fixed set of base set of events, 
>>> can we ensure that those events would not require this compat string 
>>> which needs each version explicitly stated?
>>>
>> If we are sure that some events will always exist in subsequent 
>> versions, we can set the Compat field to "arm_cmn;arm_ci". After that,
>> whether it is a different model or a different revision of the cmn 
>> PMU, it will be compatible. That is, it matches all whose identifier
>> is prefixed with “arm_cmn” or “arm_ci”.
> 
> Sure, we could do something like that. Or if we are super-confident that 
> every model and rev will support some event, then we can change perf 
> tool to just not check Compat for that event.

The majority of events have stayed unchanged since the introduction of 
their respective node type, so assuming we already have a basic match on 
the PMU name to know which JSON to be looking at in the first place, I'd 
imagine the Compat field could be optional, and only needed for events 
which first appear in a subsequent revision or model, or the fiddly 
cases like where DVM node events got entirely rewritten in CMN-650.

Thanks,
Robin.

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Jing Zhang]

在 2023/6/6 下午10:11, John Garry 写道:
> On 05/06/2023 03:46, Jing Zhang wrote:
>>> I assume that there is some ordering requirement for cmn_revision, considering that we have equality operations on the revision in the driver.
>>>
>> Actually revision is a register value rather than an enumeration value.
> 
> ok, got it.
> 
>> If I modify the revision to r0p0, etc., it is also possible, but I need
>> to convert the enumeration to a string.
> 
> understood
> 
>>
>>
>>>> The identifier consists of model_name and revision.
>>>> The compatible value "arm_cmn600;arm_cmn650" can match the identifier "arm_cmn600_0" or "arm_cmn650_0". Maybe the message log
>>>> is not clear enough.
>>>>
>>>> For example in patch #3 the metric "slc_miss_rate" is a generic metric for cmn-any. So we can define:
>>>>
>>>> +    {
>>>> +        "MetricName": "slc_miss_rate",
>>>> +        "BriefDescription": "The system level cache miss rate include.",
>>>> +        "MetricGroup": "arm_cmn",
>>>> +        "MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
>>>> +        "ScaleUnit": "100%",
>>>> +        "Unit": "arm_cmn",
>>>> +        "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>>>> +    },
>>>>
>>>>
>>>> It can match identifiers "arm_cmn600_{0,1,2..X}" or "arm_cmn650_{0,1,2..X}" or "arm_cmn700_{0,1,2..X}" or "arm_ci700_{ 0,1,2..X}".
>>>> In other words, it can match all identifiers prefixed with “arm_cmn600” or “arm_cmn650” or “arm_cmn700” or “arm_ci700”.
>>>>
>>>> If a new model arm_cmn driver with identifier "arm_cmn750_0", it will not be matched, but if a new revision arm_cmn driver with identifier
>>>> "arm_cmn700_4", it can be matched.
>>> OK, I see what you mean. My confusion came about though your commit message on this same patch, which did not mention cmn650. I assumed that the example event which you were describing was supported for arm_cmn650 and you intentionally omitted it.
>>>
>> Yes, I didn't write cmn650 in the commit message, just because I omitted it. I will add it in a later version.
>>
>>>>
>>>>>> Tokens in Unit field are delimited by ';'.
>>>>> Thanks for taking a stab at solving this problem.
>>>>>
>>>>> I have to admit that I am not the biggest fan of having multiple values to match in the "Compat" value possibly for every event. It doesn't really scale.
>>>>>
>>>>> I would hope that there are at least some events which we are guaranteed to always be present. From what Robin said on the v2 series, for the implementations which we care about, events are generally added per subsequent version. So we should have some base set of fixed events.
>>>>>
>>>>> If we are confident that we have a fixed set of base set of events, can we ensure that those events would not require this compat string which needs each version explicitly stated?
>>>>>
>>>> If we are sure that some events will always exist in subsequent versions, we can set the Compat field to "arm_cmn;arm_ci". After that,
>>>> whether it is a different model or a different revision of the cmn PMU, it will be compatible. That is, it matches all whose identifier
>>>> is prefixed with “arm_cmn” or “arm_ci”.
>>> Sure, we could do something like that. Or if we are super-confident that every model and rev will support some event, then we can change perf tool to just not check Compat for that event.
>>>
>> Yes, I have also thought about this solution. If it is an event supported by all versions, as long as it meets the Unit match, it does not need
>> to check Compat.
> 
> 
>> However, the current perf tools tool seems to ignore the "Unit" inspection for the metric event.
> 
> Unit is the format of the event_source device name. We should match based on that as well as compat. I need to check the code again to understand how that is done... it has changed a good bit in 3 years.
> 

This situation only happens on uncore metric. I happened to write wrong Unit, but the metric still matches.

>>
>>>> Maybe it's not a perfect solution yet, looking forward to your suggestions.
>>> Well first we need to define kernel HW identifier format. I don't know why we don't encode model and revision name, like "cmn650_r1p1". Robin?
>>>
>> As mentioned earlier, revision is a register value rather than an enumeration value. If change revision to revision name, I need a more redundant
>> operation of converting enumeration value to string. If you think changing the naming method to "cmn650_r1p1" is clearer, of course
>> there is no problem.
> 
> It's just odd to mix a string and revision number in this way.
> 
> Robin knows more about this HW than me, so I'll let him decide on the the preferred format.
> 

Ok, thank you John.

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Jing Zhang]

在 2023/6/7 上午12:27, Robin Murphy 写道:
> On 02/06/2023 5:20 pm, John Garry wrote:
>> On 01/06/2023 09:58, Jing Zhang wrote:
>>>>  From checking the driver, it seems that we have model names "arm_cmn600" and "arm_cmn650". Are you saying that "arm_cmn600X" would match for those? I am most curious about how "arm_cmn600X" matches "arm_cmn650".
>>>>
>>> Hi John,
>>>
>>>  From patch #1 we have identifiers "arm_cmn600_0" and "arm_cmn650_0" etc. 
>>
>> ok, I see. Your idea for the cmn driver HW identifier format is odd to me. Your HW identifier is a mix of the HW IP model name (from arm_cmn_device_data.model_name) with some the kernel revision identifier (from cmn_revision). The kernel revision identifier is an enum, and I don't think that it is a good idea to expose enum values through sysfs files.
>>
>> I assume that there is some ordering requirement for cmn_revision, considering that we have equality operations on the revision in the driver.
> 
> That enum does actually follow the revision identifiers as provided by the hardware (see CMN_CFGM_PID2_REVISION), so I don't see any major issue with putting it into user ABI. And TBH I think I would prefer to just use a numeric value rather than have to maintain yet more tables of strings which given the usage model here would effectively only mangle a matchable value into a different matchable value anyway.
> 
> I am inclined to agree that the mix between part driver-generated-string, part hardware-value looks a little funky. I still need to check with the hardware team exactly how the part number field from PERIPH_ID_0/1 is "configuration-dependent", and whether there might actually be a chance of using that as well.
> 

Thanks Robin. So should we wait to confirm the configuration of the PERIPH_ID_0/1 field before pushing this patch? Or is it
acceptable to use "cmn600_r0p0" as an identifier? Looking forward to your suggestion.

> One nagging doubt that remains for metrics are any baked-in assumptions which may not always simply depend on the product version - for instance it happens to be the case currently that everything has a fixed flit size of 256 bits, hence the magic "32" in the bandwidth calculations, but if that ever became configurable in some future product, we may potentially have a problem guaranteeing a meaningful calculation.
> 

In this case, we may use "literal" to solve it later. It can use variables in bandwidth calculations. For example,
"#slots" can get the value from the file of the current architecture and use it in the metric.

>>> The identifier consists of model_name and revision.
>>> The compatible value "arm_cmn600;arm_cmn650" can match the identifier "arm_cmn600_0" or "arm_cmn650_0". Maybe the message log
>>> is not clear enough.
>>>
>>> For example in patch #3 the metric "slc_miss_rate" is a generic metric for cmn-any. So we can define:
>>>
>>> +    {
>>> +        "MetricName": "slc_miss_rate",
>>> +        "BriefDescription": "The system level cache miss rate include.",
>>> +        "MetricGroup": "arm_cmn",
>>> +        "MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
>>> +        "ScaleUnit": "100%",
>>> +        "Unit": "arm_cmn",
>>> +        "Compat": "arm_cmn600;arm_cmn650;arm_cmn700;arm_ci700"
>>> +    },
>>>
>>>
>>> It can match identifiers "arm_cmn600_{0,1,2..X}" or "arm_cmn650_{0,1,2..X}" or "arm_cmn700_{0,1,2..X}" or "arm_ci700_{ 0,1,2..X}".
>>> In other words, it can match all identifiers prefixed with “arm_cmn600” or “arm_cmn650” or “arm_cmn700” or “arm_ci700”.
>>>
>>> If a new model arm_cmn driver with identifier "arm_cmn750_0", it will not be matched, but if a new revision arm_cmn driver with identifier
>>> "arm_cmn700_4", it can be matched.
>>
>> OK, I see what you mean. My confusion came about though your commit message on this same patch, which did not mention cmn650. I assumed that the example event which you were describing was supported for arm_cmn650 and you intentionally omitted it.
>>
>>>
>>>
>>>>> Tokens in Unit field are delimited by ';'.
>>>> Thanks for taking a stab at solving this problem.
>>>>
>>>> I have to admit that I am not the biggest fan of having multiple values to match in the "Compat" value possibly for every event. It doesn't really scale.
>>>>
>>>> I would hope that there are at least some events which we are guaranteed to always be present. From what Robin said on the v2 series, for the implementations which we care about, events are generally added per subsequent version. So we should have some base set of fixed events.
> 
> Note that there's a slight difference between "present" and "valid", e.g. in the current driver-internal aliases, all MTSX events are marked CMN_ANY, meaning they're considered valid on any CMN configuration with an MTSX node, regardless of model. The events don't exist on CMN-600 or CMN-650, but that's because the MTSX itself wasn't a thing yet, so for simplicity we don't have to bother considering the events invalid when we know they will always be non-present and thus filtered anyway.
> 
>>>> If we are confident that we have a fixed set of base set of events, can we ensure that those events would not require this compat string which needs each version explicitly stated?
>>>>
>>> If we are sure that some events will always exist in subsequent versions, we can set the Compat field to "arm_cmn;arm_ci". After that,
>>> whether it is a different model or a different revision of the cmn PMU, it will be compatible. That is, it matches all whose identifier
>>> is prefixed with “arm_cmn” or “arm_ci”.
>>
>> Sure, we could do something like that. Or if we are super-confident that every model and rev will support some event, then we can change perf tool to just not check Compat for that event.
> 
> The majority of events have stayed unchanged since the introduction of their respective node type, so assuming we already have a basic match on the PMU name to know which JSON to be looking at in the first place, I'd imagine the Compat field could be optional, and only needed for events which first appear in a subsequent revision or model, or the fiddly cases like where DVM node events got entirely rewritten in CMN-650.
> 

OK, thanks. Maybe we first need to confirm how to set the identifier format in the driver. Then it will be clearer how to implement Compat matching.


Thanks，
Jing

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[John Garry]

On 08/06/2023 10:44, Jing Zhang wrote:
>> Unit is the format of the event_source device name. We should match based on that as well as compat. I need to check the code again to understand how that is done... it has changed a good bit in 3 years.
>>
> This situation only happens on uncore metric. I happened to write wrong Unit, but the metric still matches.
> 

I'm just double checking this now. I think any possible fix should be 
easy enough for current code but may be tricky for backport with lots of 
metric code changes.

Thanks,
John

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[John Garry]

On 12/06/2023 19:15, John Garry wrote:
> On 08/06/2023 10:44, Jing Zhang wrote:
>>> Unit is the format of the event_source device name. We should match 
>>> based on that as well as compat. I need to check the code again to 
>>> understand how that is done... it has changed a good bit in 3 years.
>>>
>> This situation only happens on uncore metric. I happened to write 
>> wrong Unit, but the metric still matches.
>>
> 
> I'm just double checking this now. I think any possible fix should be 
> easy enough for current code but may be tricky for backport with lots of 
> metric code changes.

I also have code to re-work sys event metric support such that we don't 
require "compat" or "Unit" values for a metric when the metric is 
described in terms of event aliases. That code is 2 years old, so may 
take a bit of time to rebase. I'll look to do that now.

Thanks,
John

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Jing Zhang]

在 2023/6/14 上午12:36, John Garry 写道:
> On 12/06/2023 19:15, John Garry wrote:
>> On 08/06/2023 10:44, Jing Zhang wrote:
>>>> Unit is the format of the event_source device name. We should match based on that as well as compat. I need to check the code again to understand how that is done... it has changed a good bit in 3 years.
>>>>
>>> This situation only happens on uncore metric. I happened to write wrong Unit, but the metric still matches.
>>>
>>
>> I'm just double checking this now. I think any possible fix should be easy enough for current code but may be tricky for backport with lots of metric code changes.
> 
> I also have code to re-work sys event metric support such that we don't require "compat" or "Unit" values for a metric when the metric is described in terms of event aliases. That code is 2 years old, so may take a bit of time to rebase. I'll look to do that now.
> 

Sounds good!

Thanks,
Jing


> Thanks,
> John
> 

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Jing Zhang]

在 2023/6/6 上午3:39, Arnaldo Carvalho de Melo 写道:
> Em Mon, Jun 05, 2023 at 10:46:36AM +0800, Jing Zhang escreveu:
>> 在 2023/6/3 上午12:20, John Garry 写道:
>>> On 01/06/2023 09:58, Jing Zhang wrote:
>>>> It can match identifiers "arm_cmn600_{0,1,2..X}" or "arm_cmn650_{0,1,2..X}" or "arm_cmn700_{0,1,2..X}" or "arm_ci700_{ 0,1,2..X}".
>>>> In other words, it can match all identifiers prefixed with “arm_cmn600” or “arm_cmn650” or “arm_cmn700” or “arm_ci700”.
>>>>
>>>> If a new model arm_cmn driver with identifier "arm_cmn750_0", it will not be matched, but if a new revision arm_cmn driver with identifier
>>>> "arm_cmn700_4", it can be matched.
>>>
>>> OK, I see what you mean. My confusion came about though your commit message on this same patch, which did not mention cmn650. I assumed that the example event which you were describing was supported for arm_cmn650 and you intentionally omitted it.
>>>
>>
>> Yes, I didn't write cmn650 in the commit message, just because I omitted it. I will add it in a later version.
> 
> Ok, will wait for v4 then.
> 
> - Arnaldo

Hi Arnaldo,

Thank you for following up this patchset, maybe we can merge patch4 to patch7 into your branch first?
They have all been reviewed/acked and there is no dispute. And patch4-7 does not depend on patch1-3,
because patch4-7 is about the metric of ali_drw_pmu, and patch1-3 is about the metric of arm_cmn.
It may take some time for patch1-3 to reach a consensus.

Thanks，
Jing

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[John Garry]

On 15/06/2023 03:18, Jing Zhang wrote:
>>>>> Unit is the format of the event_source device name. We should match based on that as well as compat. I need to check the code again to understand how that is done... it has changed a good bit in 3 years.
>>>>>
>>>> This situation only happens on uncore metric. I happened to write wrong Unit, but the metric still matches.
>>>>
>>> I'm just double checking this now. I think any possible fix should be easy enough for current code but may be tricky for backport with lots of metric code changes.
>> I also have code to re-work sys event metric support such that we don't require "compat" or "Unit" values for a metric when the metric is described in terms of event aliases. That code is 2 years old, so may take a bit of time to rebase. I'll look to do that now.
>>
> Sounds good!

BTW, I am just looking at your cmn JSONs in this series, and we have 
something like this:

index 0000000..e70ac1a
--- /dev/null
+++ b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
@@ -0,0 +1,74 @@
+[
+	{
+		"MetricName": "slc_miss_rate",
+		"BriefDescription": "The system level cache miss rate include.",
+		"MetricGroup": "arm_cmn",
+		"MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",

So this expression uses event aliases hnf_cache_miss and 
hnf_slc_sf_cache_access - where are they defined in a JSON?

I could not see them. If they are not needed, then I may be missing 
something...

Thanks,
John

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Jing Zhang]

在 2023/6/16 下午7:41, John Garry 写道:
> On 15/06/2023 03:18, Jing Zhang wrote:
>>>>>> Unit is the format of the event_source device name. We should match based on that as well as compat. I need to check the code again to understand how that is done... it has changed a good bit in 3 years.
>>>>>>
>>>>> This situation only happens on uncore metric. I happened to write wrong Unit, but the metric still matches.
>>>>>
>>>> I'm just double checking this now. I think any possible fix should be easy enough for current code but may be tricky for backport with lots of metric code changes.
>>> I also have code to re-work sys event metric support such that we don't require "compat" or "Unit" values for a metric when the metric is described in terms of event aliases. That code is 2 years old, so may take a bit of time to rebase. I'll look to do that now.
>>>
>> Sounds good!
> 
> BTW, I am just looking at your cmn JSONs in this series, and we have something like this:
> 
> index 0000000..e70ac1a
> --- /dev/null
> +++ b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
> @@ -0,0 +1,74 @@
> +[
> +    {
> +        "MetricName": "slc_miss_rate",
> +        "BriefDescription": "The system level cache miss rate include.",
> +        "MetricGroup": "arm_cmn",
> +        "MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
> 
> So this expression uses event aliases hnf_cache_miss and hnf_slc_sf_cache_access - where are they defined in a JSON?
> 

Hi John,

I defined the aliases for these events in the JSON file during the RFC version. However, I later removed the alias
definitions for these events in subsequent versions due to the possibility of non-uniqueness and difficulty in defining
their EventCode. But this does not affect their usage in metrics. In other words, metrics can use the aliases without
defining event aliases in the JSON file.

In the past, I always thought that the function of the alias was to explain the meaning of these events in the perf list.
Or maybe I'm missing something?

Thanks,
Jing

> I could not see them. If they are not needed, then I may be missing something...
> 
> Thanks,
> John
> 
> 

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[John Garry]

On 19/06/2023 03:58, Jing Zhang wrote:
>> +++ b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
>> @@ -0,0 +1,74 @@
>> +[
>> +    {
>> +        "MetricName": "slc_miss_rate",
>> +        "BriefDescription": "The system level cache miss rate include.",
>> +        "MetricGroup": "arm_cmn",
>> +        "MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
>>
>> So this expression uses event aliases hnf_cache_miss and hnf_slc_sf_cache_access - where are they defined in a JSON?
>>
> Hi John,
> 
> I defined the aliases for these events in the JSON file during the RFC version. However, I later removed the alias
> definitions for these events in subsequent versions due to the possibility of non-uniqueness and difficulty in defining
> their EventCode. But this does not affect their usage in metrics. In other words, metrics can use the aliases without
> defining event aliases in the JSON file.

Really? So how can we resolve the event aliases when we try to run the 
metric?

Please verify running these metrics with 'perf stat', like 'perf stat -v 
-M slc_miss_rate'

> 
> In the past, I always thought that the function of the alias was to explain the meaning of these events in the perf list.
> Or maybe I'm missing something?

Event aliases do give the ability to describe the event in perf list. 
But we can also run them for 'perf stat', like:

./perf list uncore
List of pre-defined events (to be used in -e or -M):

   uncore_cbox_0/clockticks/                          [Kernel PMU event]
   uncore_cbox_1/clockticks/                          [Kernel PMU event]
   uncore_imc/data_reads/                             [Kernel PMU event]
   uncore_imc/data_writes/                            [Kernel PMU event]
   uncore_imc/gt_requests/                            [Kernel PMU event]
   uncore_imc/ia_requests/                            [Kernel PMU event]
   uncore_imc/io_requests/                            [Kernel PMU event]

uncore cache:
   unc_cbo_cache_lookup.any_es
        [L3 Lookup any request that access cache and found line in E or 
S-state. Unit: uncore_cbox]
...

sudo ./perf stat -v -e unc_cbo_cache_lookup.any_es
Using CPUID GenuineIntel-6-3D-4
unc_cbo_cache_lookup.any_es -> uncore_cbox_0/event=0x34,umask=0x86/
unc_cbo_cache_lookup.any_es -> uncore_cbox_1/event=0x34,umask=0x86/
Control descriptor is not initialized
^Cunc_cbo_cache_lookup.any_es: 14361103 1853372468 1853372468
unc_cbo_cache_lookup.any_es: 14322188 1853360415 1853360415

  Performance counter stats for 'system wide':

         14,361,103      unc_cbo_cache_lookup.any_es 

         14,322,188      unc_cbo_cache_lookup.any_es 


        1.853388227 seconds time elapsed


Thanks,
John

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Jing Zhang]

在 2023/6/19 下午3:07, John Garry 写道:
> On 19/06/2023 03:58, Jing Zhang wrote:
>>> +++ b/tools/perf/pmu-events/arch/arm64/arm/cmn/sys/metrics.json
>>> @@ -0,0 +1,74 @@
>>> +[
>>> +    {
>>> +        "MetricName": "slc_miss_rate",
>>> +        "BriefDescription": "The system level cache miss rate include.",
>>> +        "MetricGroup": "arm_cmn",
>>> +        "MetricExpr": "hnf_cache_miss / hnf_slc_sf_cache_access",
>>>
>>> So this expression uses event aliases hnf_cache_miss and hnf_slc_sf_cache_access - where are they defined in a JSON?
>>>
>> Hi John,
>>
>> I defined the aliases for these events in the JSON file during the RFC version. However, I later removed the alias
>> definitions for these events in subsequent versions due to the possibility of non-uniqueness and difficulty in defining
>> their EventCode. But this does not affect their usage in metrics. In other words, metrics can use the aliases without
>> defining event aliases in the JSON file.
> 
> Really? So how can we resolve the event aliases when we try to run the metric?
> 
> Please verify running these metrics with 'perf stat', like 'perf stat -v -M slc_miss_rate'
> 

Ok, it shows:
#./perf stat -v -M slc_miss_rate sleep 1

metric expr hnf_cache_miss / hnf_slc_sf_cache_access for slc_miss_rate
found event duration_time
found event hnf_slc_sf_cache_access
found event hnf_cache_miss
Parsing metric events '{hnf_slc_sf_cache_access/metric-id=hnf_slc_sf_cache_access/,hnf_cache_miss/metric-id=hnf_cache_miss/}:W,duration_time'
hnf_slc_sf_cache_access -> arm_cmn_0/type=0x5,eventid=0x2/
hnf_slc_sf_cache_access -> arm_cmn_1/type=0x5,eventid=0x2/
hnf_cache_miss -> arm_cmn_0/type=0x5,eventid=0x1/
hnf_cache_miss -> arm_cmn_1/type=0x5,eventid=0x1/
Control descriptor is not initialized
hnf_slc_sf_cache_access: 127615 1001344900 1001344900
hnf_cache_miss: 36829 1001344900 1001344900
hnf_slc_sf_cache_access: 131526 1001343540 1001343540
hnf_cache_miss: 40587 1001343540 1001343540
duration_time: 1001381687 1001381687 1001381687

 Performance counter stats for 'system wide':

           259,141      hnf_slc_sf_cache_access   #     29.9 %  slc_miss_rate
            77,416      hnf_cache_miss
     1,001,381,687 ns   duration_time

       1.001381687 seconds time elapsed



#./perf list
...
 arm_cmn_0/hnf_cache_miss/                          [Kernel PMU event]
 arm_cmn_0/hnf_slc_sf_cache_access/                 [Kernel PMU event]
...
 arm_cmn_1/hnf_cache_miss/                          [Kernel PMU event]
 arm_cmn_1/hnf_slc_sf_cache_access/                 [Kernel PMU event]
...

>>
>> In the past, I always thought that the function of the alias was to explain the meaning of these events in the perf list.
>> Or maybe I'm missing something?
> 
> Event aliases do give the ability to describe the event in perf list. But we can also run them for 'perf stat', like:
> 
> ./perf list uncore
> List of pre-defined events (to be used in -e or -M):
> 
>   uncore_cbox_0/clockticks/                          [Kernel PMU event]
>   uncore_cbox_1/clockticks/                          [Kernel PMU event]
>   uncore_imc/data_reads/                             [Kernel PMU event]
>   uncore_imc/data_writes/                            [Kernel PMU event]
>   uncore_imc/gt_requests/                            [Kernel PMU event]
>   uncore_imc/ia_requests/                            [Kernel PMU event]
>   uncore_imc/io_requests/                            [Kernel PMU event]
> 
> uncore cache:
>   unc_cbo_cache_lookup.any_es
>        [L3 Lookup any request that access cache and found line in E or S-state. Unit: uncore_cbox]
> ...
> 
> sudo ./perf stat -v -e unc_cbo_cache_lookup.any_es
> Using CPUID GenuineIntel-6-3D-4
> unc_cbo_cache_lookup.any_es -> uncore_cbox_0/event=0x34,umask=0x86/
> unc_cbo_cache_lookup.any_es -> uncore_cbox_1/event=0x34,umask=0x86/
> Control descriptor is not initialized
> ^Cunc_cbo_cache_lookup.any_es: 14361103 1853372468 1853372468
> unc_cbo_cache_lookup.any_es: 14322188 1853360415 1853360415
> 
>  Performance counter stats for 'system wide':
> 
>         14,361,103      unc_cbo_cache_lookup.any_es
>         14,322,188      unc_cbo_cache_lookup.any_es
> 
>        1.853388227 seconds time elapsed
> 

Ok, thanks. If I use events without a prefix, such as perf stat -e clockticks sleep 1, will this also work?

Thanks,
Jing


> 
> Thanks,
> John

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[John Garry]

On 19/06/2023 09:59, Jing Zhang wrote:
>> Please verify running these metrics with 'perf stat', like 'perf stat -v -M slc_miss_rate'
>>
> Ok, it shows:
> #./perf stat -v -M slc_miss_rate sleep 1
> 
> metric expr hnf_cache_miss / hnf_slc_sf_cache_access for slc_miss_rate
> found event duration_time
> found event hnf_slc_sf_cache_access

In the earlier RFC series you had 
tools/perf/pmu-events/arch/arm64/arm/cmn700/sys/cmn.json, which 
describes event hnf_slc_sf_cache_access

But that JSON is not in this series. Why is it not included?

The cmn kernel driver exposes event hnf_slc_sf_cache_access, but I did 
not think that perf tool metric code matches those events described in 
/bus/event_sourcs/devices/<PMU>/events

> found event hnf_cache_miss
> Parsing metric events '{hnf_slc_sf_cache_access/metric-id=hnf_slc_sf_cache_access/,hnf_cache_miss/metric-id=hnf_cache_miss/}:W,duration_time'
> hnf_slc_sf_cache_access -> arm_cmn_0/type=0x5,eventid=0x2/
> hnf_slc_sf_cache_access -> arm_cmn_1/type=0x5,eventid=0x2/
> hnf_cache_miss -> arm_cmn_0/type=0x5,eventid=0x1/
> hnf_cache_miss -> arm_cmn_1/type=0x5,eventid=0x1/
> Control descriptor is not initialized
> hnf_slc_sf_cache_access: 127615 1001344900 1001344900
> hnf_cache_miss: 36829 1001344900 1001344900
> hnf_slc_sf_cache_access: 131526 1001343540 1001343540
> hnf_cache_miss: 40587 1001343540 1001343540
> duration_time: 1001381687 1001381687 1001381687
> 
>   Performance counter stats for 'system wide':
> 
>             259,141      hnf_slc_sf_cache_access   #     29.9 %  slc_miss_rate
>              77,416      hnf_cache_miss
>       1,001,381,687 ns   duration_time
> 
>         1.001381687 seconds time elapsed
> 
> 
> 
> #./perf list
> ...
>   arm_cmn_0/hnf_cache_miss/                          [Kernel PMU event]
>   arm_cmn_0/hnf_slc_sf_cache_access/                 [Kernel PMU event]
> ...
>   arm_cmn_1/hnf_cache_miss/                          [Kernel PMU event]
>   arm_cmn_1/hnf_slc_sf_cache_access/                 [Kernel PMU event]
> ...
> 
>>> In the past, I always thought that the function of the alias was to explain the meaning of these events in the perf list.
>>> Or maybe I'm missing something?
>> Event aliases do give the ability to describe the event in perf list. But we can also run them for 'perf stat', like:
>>
>> ./perf list uncore
>> List of pre-defined events (to be used in -e or -M):
>>
>>    uncore_cbox_0/clockticks/                          [Kernel PMU event]
>>    uncore_cbox_1/clockticks/                          [Kernel PMU event]
>>    uncore_imc/data_reads/                             [Kernel PMU event]
>>    uncore_imc/data_writes/                            [Kernel PMU event]
>>    uncore_imc/gt_requests/                            [Kernel PMU event]
>>    uncore_imc/ia_requests/                            [Kernel PMU event]
>>    uncore_imc/io_requests/                            [Kernel PMU event]
>>
>> uncore cache:
>>    unc_cbo_cache_lookup.any_es
>>         [L3 Lookup any request that access cache and found line in E or S-state. Unit: uncore_cbox]
>> ...
>>
>> sudo ./perf stat -v -e unc_cbo_cache_lookup.any_es
>> Using CPUID GenuineIntel-6-3D-4
>> unc_cbo_cache_lookup.any_es -> uncore_cbox_0/event=0x34,umask=0x86/
>> unc_cbo_cache_lookup.any_es -> uncore_cbox_1/event=0x34,umask=0x86/
>> Control descriptor is not initialized
>> ^Cunc_cbo_cache_lookup.any_es: 14361103 1853372468 1853372468
>> unc_cbo_cache_lookup.any_es: 14322188 1853360415 1853360415
>>
>>   Performance counter stats for 'system wide':
>>
>>          14,361,103      unc_cbo_cache_lookup.any_es
>>          14,322,188      unc_cbo_cache_lookup.any_es
>>
>>         1.853388227 seconds time elapsed
>>
> Ok, thanks. If I use events without a prefix, such as perf stat -e clockticks sleep 1, will this also work?

In this case, yes - it would work for uncore_cbox_0/clockticks/ and 
uncore_cbox_1/clockticks/

But you need to be careful to here - if another PMU has same event name, 
then it might also match.

Thanks,
John

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Jing Zhang]

在 2023/6/19 下午5:31, John Garry 写道:
> On 19/06/2023 09:59, Jing Zhang wrote:
>>> Please verify running these metrics with 'perf stat', like 'perf stat -v -M slc_miss_rate'
>>>
>> Ok, it shows:
>> #./perf stat -v -M slc_miss_rate sleep 1
>>
>> metric expr hnf_cache_miss / hnf_slc_sf_cache_access for slc_miss_rate
>> found event duration_time
>> found event hnf_slc_sf_cache_access
> 
> In the earlier RFC series you had tools/perf/pmu-events/arch/arm64/arm/cmn700/sys/cmn.json, which describes event hnf_slc_sf_cache_access
> 
> But that JSON is not in this series. Why is it not included?
> 

Because the RFC version of the cmn.json file does not define the EventCode for each event, this will not take effect, so I temporarily removed it.
The EventID of CMN events is different from other events. For example, hnf_slc_sf_cache_access corresponds to arm_cmn_0/type=0x5,eventid=0x2/.
The current JSON format parsing does not support this EventID, and jevent.py needs to be extended.

> The cmn kernel driver exposes event hnf_slc_sf_cache_access, but I did not think that perf tool metric code matches those events described in /bus/event_sourcs/devices/<PMU>/events
> 

If there is no alias defined, other events with the same name may be matched, it is indeed necessary to define an alias for each event first,
and I will add it in the next version. But first I need to extend jevent.py to support cmn EventID.

>> found event hnf_cache_miss
>> Parsing metric events '{hnf_slc_sf_cache_access/metric-id=hnf_slc_sf_cache_access/,hnf_cache_miss/metric-id=hnf_cache_miss/}:W,duration_time'
>> hnf_slc_sf_cache_access -> arm_cmn_0/type=0x5,eventid=0x2/
>> hnf_slc_sf_cache_access -> arm_cmn_1/type=0x5,eventid=0x2/
>> hnf_cache_miss -> arm_cmn_0/type=0x5,eventid=0x1/
>> hnf_cache_miss -> arm_cmn_1/type=0x5,eventid=0x1/
>> Control descriptor is not initialized
>> hnf_slc_sf_cache_access: 127615 1001344900 1001344900
>> hnf_cache_miss: 36829 1001344900 1001344900
>> hnf_slc_sf_cache_access: 131526 1001343540 1001343540
>> hnf_cache_miss: 40587 1001343540 1001343540
>> duration_time: 1001381687 1001381687 1001381687
>>
>>   Performance counter stats for 'system wide':
>>
>>             259,141      hnf_slc_sf_cache_access   #     29.9 %  slc_miss_rate
>>              77,416      hnf_cache_miss
>>       1,001,381,687 ns   duration_time
>>
>>         1.001381687 seconds time elapsed
>>
>>
>>
>> #./perf list
>> ...
>>   arm_cmn_0/hnf_cache_miss/                          [Kernel PMU event]
>>   arm_cmn_0/hnf_slc_sf_cache_access/                 [Kernel PMU event]
>> ...
>>   arm_cmn_1/hnf_cache_miss/                          [Kernel PMU event]
>>   arm_cmn_1/hnf_slc_sf_cache_access/                 [Kernel PMU event]
>> ...
>>
>>>> In the past, I always thought that the function of the alias was to explain the meaning of these events in the perf list.
>>>> Or maybe I'm missing something?
>>> Event aliases do give the ability to describe the event in perf list. But we can also run them for 'perf stat', like:
>>>
>>> ./perf list uncore
>>> List of pre-defined events (to be used in -e or -M):
>>>
>>>    uncore_cbox_0/clockticks/                          [Kernel PMU event]
>>>    uncore_cbox_1/clockticks/                          [Kernel PMU event]
>>>    uncore_imc/data_reads/                             [Kernel PMU event]
>>>    uncore_imc/data_writes/                            [Kernel PMU event]
>>>    uncore_imc/gt_requests/                            [Kernel PMU event]
>>>    uncore_imc/ia_requests/                            [Kernel PMU event]
>>>    uncore_imc/io_requests/                            [Kernel PMU event]
>>>
>>> uncore cache:
>>>    unc_cbo_cache_lookup.any_es
>>>         [L3 Lookup any request that access cache and found line in E or S-state. Unit: uncore_cbox]
>>> ...
>>>
>>> sudo ./perf stat -v -e unc_cbo_cache_lookup.any_es
>>> Using CPUID GenuineIntel-6-3D-4
>>> unc_cbo_cache_lookup.any_es -> uncore_cbox_0/event=0x34,umask=0x86/
>>> unc_cbo_cache_lookup.any_es -> uncore_cbox_1/event=0x34,umask=0x86/
>>> Control descriptor is not initialized
>>> ^Cunc_cbo_cache_lookup.any_es: 14361103 1853372468 1853372468
>>> unc_cbo_cache_lookup.any_es: 14322188 1853360415 1853360415
>>>
>>>   Performance counter stats for 'system wide':
>>>
>>>          14,361,103      unc_cbo_cache_lookup.any_es
>>>          14,322,188      unc_cbo_cache_lookup.any_es
>>>
>>>         1.853388227 seconds time elapsed
>>>
>> Ok, thanks. If I use events without a prefix, such as perf stat -e clockticks sleep 1, will this also work?
> 
> In this case, yes - it would work for uncore_cbox_0/clockticks/ and uncore_cbox_1/clockticks/
> 
> But you need to be careful to here - if another PMU has same event name, then it might also match.
> 

Ok, I got it.

Thanks,
Jing

> Thanks,
> John

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[John Garry]

On 20/06/2023 03:12, Jing Zhang wrote:
>> But that JSON is not in this series. Why is it not included?
>>
> Because the RFC version of the cmn.json file does not define the EventCode for each event, this will not take effect, so I temporarily removed it.
> The EventID of CMN events is different from other events. For example, hnf_slc_sf_cache_access corresponds to arm_cmn_0/type=0x5,eventid=0x2/.
> The current JSON format parsing does not support this EventID, and jevent.py needs to be extended.

So please do that then. I would suggest just to first support event 
aliasing, and then support metrics. JFYI, I am still reworking current 
perf tool metric code for sys events, which should take a few more days.

> 
>> The cmn kernel driver exposes event hnf_slc_sf_cache_access, but I did not think that perf tool metric code matches those events described in/bus/event_sourcs/devices/<PMU>/events
>>
> If there is no alias defined, other events with the same name may be matched, it is indeed necessary to define an alias for each event first,
> and I will add it in the next version. But first I need to extend jevent.py to support cmn EventID.

ok, please do this. I assume that you are talking about wildcard 
matching for cmn HW identifier. We would also need perf tool self-test 
expanded to cover this wildcard matching - see tests/pmu-events.c

Thanks,
John

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Jing Zhang]

在 2023/6/20 下午3:01, John Garry 写道:
> On 20/06/2023 03:12, Jing Zhang wrote:
>>> But that JSON is not in this series. Why is it not included?
>>>
>> Because the RFC version of the cmn.json file does not define the EventCode for each event, this will not take effect, so I temporarily removed it.
>> The EventID of CMN events is different from other events. For example, hnf_slc_sf_cache_access corresponds to arm_cmn_0/type=0x5,eventid=0x2/.
>> The current JSON format parsing does not support this EventID, and jevent.py needs to be extended.
> 
> So please do that then. I would suggest just to first support event aliasing, and then support metrics. JFYI, I am still reworking current perf tool metric code for sys events, which should take a few more days.
> 

Ok, thank you.

>>
>>> The cmn kernel driver exposes event hnf_slc_sf_cache_access, but I did not think that perf tool metric code matches those events described in/bus/event_sourcs/devices/<PMU>/events
>>>
>> If there is no alias defined, other events with the same name may be matched, it is indeed necessary to define an alias for each event first,
>> and I will add it in the next version. But first I need to extend jevent.py to support cmn EventID.
> 
> ok, please do this. I assume that you are talking about wildcard matching for cmn HW identifier. We would also need perf tool self-test expanded to cover this wildcard matching - see tests/pmu-events.c
> 

Ok, let me try.

Thanks,
Jing

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Namhyung Kim]

Hello,

On Wed, Jun 14, 2023 at 8:42 PM Jing Zhang <renyu.zj@linux.alibaba.com> wrote:
>
>
>
> 在 2023/6/6 上午3:39, Arnaldo Carvalho de Melo 写道:
> > Em Mon, Jun 05, 2023 at 10:46:36AM +0800, Jing Zhang escreveu:
> >> 在 2023/6/3 上午12:20, John Garry 写道:
> >>> On 01/06/2023 09:58, Jing Zhang wrote:
> >>>> It can match identifiers "arm_cmn600_{0,1,2..X}" or "arm_cmn650_{0,1,2..X}" or "arm_cmn700_{0,1,2..X}" or "arm_ci700_{ 0,1,2..X}".
> >>>> In other words, it can match all identifiers prefixed with “arm_cmn600” or “arm_cmn650” or “arm_cmn700” or “arm_ci700”.
> >>>>
> >>>> If a new model arm_cmn driver with identifier "arm_cmn750_0", it will not be matched, but if a new revision arm_cmn driver with identifier
> >>>> "arm_cmn700_4", it can be matched.
> >>>
> >>> OK, I see what you mean. My confusion came about though your commit message on this same patch, which did not mention cmn650. I assumed that the example event which you were describing was supported for arm_cmn650 and you intentionally omitted it.
> >>>
> >>
> >> Yes, I didn't write cmn650 in the commit message, just because I omitted it. I will add it in a later version.
> >
> > Ok, will wait for v4 then.
> >
> > - Arnaldo
>
> Hi Arnaldo,
>
> Thank you for following up this patchset, maybe we can merge patch4 to patch7 into your branch first?
> They have all been reviewed/acked and there is no dispute. And patch4-7 does not depend on patch1-3,
> because patch4-7 is about the metric of ali_drw_pmu, and patch1-3 is about the metric of arm_cmn.
> It may take some time for patch1-3 to reach a consensus.

IIUC patch 4 is for the kernel and patch 5-7 semantically depend on it.
So the patch 4 should be picked up by the kernel maintainers first.

Thanks,
Namhyung

Re: [PATCH v3 2/7] perf metric: Event "Compat" value supports matching multiple identifiers

[Jing Zhang]

在 2023/6/24 上午7:52, Namhyung Kim 写道:
> Hello,
> 
> On Wed, Jun 14, 2023 at 8:42 PM Jing Zhang <renyu.zj@linux.alibaba.com> wrote:
>>
>>
>>
>> 在 2023/6/6 上午3:39, Arnaldo Carvalho de Melo 写道:
>>> Em Mon, Jun 05, 2023 at 10:46:36AM +0800, Jing Zhang escreveu:
>>>> 在 2023/6/3 上午12:20, John Garry 写道:
>>>>> On 01/06/2023 09:58, Jing Zhang wrote:
>>>>>> It can match identifiers "arm_cmn600_{0,1,2..X}" or "arm_cmn650_{0,1,2..X}" or "arm_cmn700_{0,1,2..X}" or "arm_ci700_{ 0,1,2..X}".
>>>>>> In other words, it can match all identifiers prefixed with “arm_cmn600” or “arm_cmn650” or “arm_cmn700” or “arm_ci700”.
>>>>>>
>>>>>> If a new model arm_cmn driver with identifier "arm_cmn750_0", it will not be matched, but if a new revision arm_cmn driver with identifier
>>>>>> "arm_cmn700_4", it can be matched.
>>>>>
>>>>> OK, I see what you mean. My confusion came about though your commit message on this same patch, which did not mention cmn650. I assumed that the example event which you were describing was supported for arm_cmn650 and you intentionally omitted it.
>>>>>
>>>>
>>>> Yes, I didn't write cmn650 in the commit message, just because I omitted it. I will add it in a later version.
>>>
>>> Ok, will wait for v4 then.
>>>
>>> - Arnaldo
>>
>> Hi Arnaldo,
>>
>> Thank you for following up this patchset, maybe we can merge patch4 to patch7 into your branch first?
>> They have all been reviewed/acked and there is no dispute. And patch4-7 does not depend on patch1-3,
>> because patch4-7 is about the metric of ali_drw_pmu, and patch1-3 is about the metric of arm_cmn.
>> It may take some time for patch1-3 to reach a consensus.
> 
> IIUC patch 4 is for the kernel and patch 5-7 semantically depend on it.
> So the patch 4 should be picked up by the kernel maintainers first.
> 

Yes, that's right, thanks.

> Thanks,
> Namhyung