[PATCH v4 0/8] powerpc/perf: Add json file metric support for the hv_24x7 socket/chip level events

[PATCH v4 0/8] powerpc/perf: Add json file metric support for the hv_24x7 socket/chip level events

[Kajol Jain]

First patch of the patchset fix inconsistent results we are getting when
we run multiple 24x7 events.

Patchset adds json file metric support for the hv_24x7 socket/chip level
events. "hv_24x7" pmu interface events needs system dependent parameter
like socket/chip/core. For example, hv_24x7 chip level events needs
specific chip-id to which the data is requested should be added as part
of pmu events.

So to enable JSON file support to "hv_24x7" interface, patchset expose
total number of sockets and chips per-socket details in sysfs
files (sockets, chips) under "/sys/devices/hv_24x7/interface/".

To get sockets and number of chips per sockets, patchset adds a rtas call
with token "PROCESSOR_MODULE_INFO" to get these details. Patchset also
handles partition migration case to re-init these system depended
parameters by adding proper calls in post_mobility_fixup() (mobility.c).

Patch 6 & 8 of the patchset handles perf tool plumbing needed to replace
the "?" character in the metric expression to proper value and hv_24x7
json metric file for different Socket/chip resources.

Patch set also enable Hz/hz prinitg for --metric-only option to print
metric data for bus frequency.

Applied and tested all these patches cleanly on top of jiri's flex changes
with the changes done by Kan Liang for "Support metric group constraint"
patchset and made required changes.

Changelog:
v3 -> v4
- Made changes suggested by jiri.
- Apply these patch on top of Kan liang changes.

v2 -> v3
- Remove setting  event_count to 0 part in function 'h_24x7_event_read'
  with comment rather then adding 0 to event_count value.
  Suggested by: Sukadev Bhattiprolu

- Apply tool side changes require to replace "?" on Jiri's flex patch
  series and made all require changes to make it compatible with added
  flex change.

v1 -> v2
- Rename hv-24x7 metric json file as nest_metrics.json

Kajol Jain (8):
  powerpc/perf/hv-24x7: Fix inconsistent output values incase multiple
    hv-24x7 events run
  powerpc/hv-24x7: Add rtas call in hv-24x7 driver to get processor
    details
  powerpc/hv-24x7: Add sysfs files inside hv-24x7 device to show
    processor details
  Documentation/ABI: Add ABI documentation for chips and sockets
  powerpc/hv-24x7: Update post_mobility_fixup() to handle migration
  perf/tools: Enhance JSON/metric infrastructure to handle "?"
  tools/perf: Enable Hz/hz prinitg for --metric-only option
  perf/tools/pmu-events/powerpc: Add hv_24x7 socket/chip level metric
    events

 .../sysfs-bus-event_source-devices-hv_24x7    |  14 ++
 arch/powerpc/perf/hv-24x7.c                   | 104 +++++++++++++--
 arch/powerpc/platforms/pseries/mobility.c     |  12 ++
 arch/powerpc/platforms/pseries/pseries.h      |   3 +
 tools/perf/arch/powerpc/util/header.c         |  22 ++++
 .../arch/powerpc/power9/nest_metrics.json     |  19 +++
 tools/perf/util/expr.h                        |   1 +
 tools/perf/util/expr.l                        |  19 ++-
 tools/perf/util/metricgroup.c                 | 124 ++++++++++++++----
 tools/perf/util/metricgroup.h                 |   1 +
 tools/perf/util/stat-display.c                |   2 -
 tools/perf/util/stat-shadow.c                 |   8 ++
 12 files changed, 290 insertions(+), 39 deletions(-)
 create mode 100644 tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json

-- 
2.18.1

[PATCH v4 1/8] powerpc/perf/hv-24x7: Fix inconsistent output values incase multiple hv-24x7 events run

[Kajol Jain]

Commit 2b206ee6b0df ("powerpc/perf/hv-24x7: Display change in counter
values")' added to print _change_ in the counter value rather then raw
value for 24x7 counters. Incase of transactions, the event count
is set to 0 at the beginning of the transaction. It also sets
the event's prev_count to the raw value at the time of initialization.
Because of setting event count to 0, we are seeing some weird behaviour,
whenever we run multiple 24x7 events at a time.

For example:

command#: ./perf stat -e "{hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/,
			   hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/}"
	  		   -C 0 -I 1000 sleep 100

     1.000121704                120 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     1.000121704                  5 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     2.000357733                  8 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     2.000357733                 10 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     3.000495215 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     3.000495215 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     4.000641884                 56 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     4.000641884 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     5.000791887 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/

Getting these large values in case we do -I.

As we are setting event_count to 0, for interval case, overall event_count is not
coming in incremental order. As we may can get new delta lesser then previous count.
Because of which when we print intervals, we are getting negative value which create
these large values.

This patch removes part where we set event_count to 0 in function
'h_24x7_event_read'. There won't be much impact as we do set event->hw.prev_count
to the raw value at the time of initialization to print change value.

With this patch
In power9 platform

command#: ./perf stat -e "{hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/,
		           hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/}"
			   -C 0 -I 1000 sleep 100

     1.000117685                 93 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     1.000117685                  1 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     2.000349331                 98 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     2.000349331                  2 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     3.000495900                131 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     3.000495900                  4 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     4.000645920                204 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     4.000645920                 61 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     4.284169997                 22 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Suggested-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 573e0b309c0c..48e8f4b17b91 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -1400,16 +1400,6 @@ static void h_24x7_event_read(struct perf_event *event)
 			h24x7hw = &get_cpu_var(hv_24x7_hw);
 			h24x7hw->events[i] = event;
 			put_cpu_var(h24x7hw);
-			/*
-			 * Clear the event count so we can compute the _change_
-			 * in the 24x7 raw counter value at the end of the txn.
-			 *
-			 * Note that we could alternatively read the 24x7 value
-			 * now and save its value in event->hw.prev_count. But
-			 * that would require issuing a hcall, which would then
-			 * defeat the purpose of using the txn interface.
-			 */
-			local64_set(&event->count, 0);
 		}
 
 		put_cpu_var(hv_24x7_reqb);
-- 
2.18.1

[PATCH v4 2/8] powerpc/hv-24x7: Add rtas call in hv-24x7 driver to get processor details

[Kajol Jain]

For hv_24x7 socket/chip level events, specific chip-id to which
the data requested should be added as part of pmu events.
But number of chips/socket in the system details are not exposed.

Patch implements read_sys_info_pseries() to get system
parameter values like number of sockets and chips per socket.
Rtas_call with token "PROCESSOR_MODULE_INFO"
is used to get these values.

Sub-sequent patch exports these values via sysfs.

Patch also make these parameters default to 1.

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c              | 72 ++++++++++++++++++++++++
 arch/powerpc/platforms/pseries/pseries.h |  3 +
 2 files changed, 75 insertions(+)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 48e8f4b17b91..9ae00f29bd21 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -20,6 +20,11 @@
 #include <asm/io.h>
 #include <linux/byteorder/generic.h>
 
+#ifdef CONFIG_PPC_RTAS
+#include <asm/rtas.h>
+#include <../../platforms/pseries/pseries.h>
+#endif
+
 #include "hv-24x7.h"
 #include "hv-24x7-catalog.h"
 #include "hv-common.h"
@@ -57,6 +62,69 @@ static bool is_physical_domain(unsigned domain)
 	}
 }
 
+#ifdef CONFIG_PPC_RTAS
+#define PROCESSOR_MODULE_INFO   43
+#define PROCESSOR_MAX_LENGTH	(8 * 1024)
+
+static int strbe16toh(const char *buf, int offset)
+{
+	return (buf[offset] << 8) + buf[offset + 1];
+}
+
+static u32		physsockets;	/* Physical sockets */
+static u32		physchips;	/* Physical chips */
+
+/*
+ * Function read_sys_info_pseries() make a rtas_call which require
+ * data buffer of size 8K. As standard 'rtas_data_buf' is of size
+ * 4K, we are adding new local buffer 'rtas_local_data_buf'.
+ */
+char rtas_local_data_buf[PROCESSOR_MAX_LENGTH] __cacheline_aligned;
+
+/*
+ * read_sys_info_pseries()
+ * Retrieve the number of sockets and chips per socket details
+ * through the get-system-parameter rtas call.
+ */
+void read_sys_info_pseries(void)
+{
+	int call_status, len, ntypes;
+
+	/*
+	 * Making system parameter: chips and sockets default to 1.
+	 */
+	physsockets = 1;
+	physchips = 1;
+	memset(rtas_local_data_buf, 0, PROCESSOR_MAX_LENGTH);
+	spin_lock(&rtas_data_buf_lock);
+
+	call_status = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
+				NULL,
+				PROCESSOR_MODULE_INFO,
+				__pa(rtas_local_data_buf),
+				PROCESSOR_MAX_LENGTH);
+
+	spin_unlock(&rtas_data_buf_lock);
+
+	if (call_status != 0) {
+		pr_info("%s %s Error calling get-system-parameter (0x%x)\n",
+			__FILE__, __func__, call_status);
+	} else {
+		rtas_local_data_buf[PROCESSOR_MAX_LENGTH - 1] = '\0';
+		len = strbe16toh(rtas_local_data_buf, 0);
+		if (len < 6)
+			return;
+
+		ntypes = strbe16toh(rtas_local_data_buf, 2);
+
+		if (!ntypes)
+			return;
+		physsockets = strbe16toh(rtas_local_data_buf, 4);
+		physchips = strbe16toh(rtas_local_data_buf, 6);
+	}
+}
+#endif /* CONFIG_PPC_RTAS */
+
 /* Domains for which more than one result element are returned for each event. */
 static bool domain_needs_aggregation(unsigned int domain)
 {
@@ -1605,6 +1673,10 @@ static int hv_24x7_init(void)
 	if (r)
 		return r;
 
+#ifdef CONFIG_PPC_RTAS
+	read_sys_info_pseries();
+#endif
+
 	return 0;
 }
 
diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
index 13fa370a87e4..1727559ce304 100644
--- a/arch/powerpc/platforms/pseries/pseries.h
+++ b/arch/powerpc/platforms/pseries/pseries.h
@@ -19,6 +19,9 @@ extern void request_event_sources_irqs(struct device_node *np,
 struct pt_regs;
 
 extern int pSeries_system_reset_exception(struct pt_regs *regs);
+#ifdef CONFIG_PPC_RTAS
+extern void read_sys_info_pseries(void);
+#endif
 extern int pSeries_machine_check_exception(struct pt_regs *regs);
 extern long pseries_machine_check_realmode(struct pt_regs *regs);
 
-- 
2.18.1

[PATCH v4 3/8] powerpc/hv-24x7: Add sysfs files inside hv-24x7 device to show processor details

[Kajol Jain]

To expose the system dependent parameter like total number of
sockets and numbers of chips per socket, patch adds two sysfs files.
"sockets" and "chips" are added to /sys/devices/hv_24x7/interface/
of the "hv_24x7" pmu.

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 9ae00f29bd21..a31bd5b88f7a 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -454,6 +454,20 @@ static ssize_t device_show_string(struct device *dev,
 	return sprintf(buf, "%s\n", (char *)d->var);
 }
 
+#ifdef CONFIG_PPC_RTAS
+static ssize_t sockets_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", physsockets);
+}
+
+static ssize_t chips_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	return sprintf(buf, "%d\n", physchips);
+}
+#endif
+
 static struct attribute *device_str_attr_create_(char *name, char *str)
 {
 	struct dev_ext_attribute *attr = kzalloc(sizeof(*attr), GFP_KERNEL);
@@ -1100,6 +1114,10 @@ PAGE_0_ATTR(catalog_len, "%lld\n",
 		(unsigned long long)be32_to_cpu(page_0->length) * 4096);
 static BIN_ATTR_RO(catalog, 0/* real length varies */);
 static DEVICE_ATTR_RO(domains);
+#ifdef CONFIG_PPC_RTAS
+static DEVICE_ATTR_RO(sockets);
+static DEVICE_ATTR_RO(chips);
+#endif
 
 static struct bin_attribute *if_bin_attrs[] = {
 	&bin_attr_catalog,
@@ -1110,6 +1128,10 @@ static struct attribute *if_attrs[] = {
 	&dev_attr_catalog_len.attr,
 	&dev_attr_catalog_version.attr,
 	&dev_attr_domains.attr,
+#ifdef CONFIG_PPC_RTAS
+	&dev_attr_sockets.attr,
+	&dev_attr_chips.attr,
+#endif
 	NULL,
 };
 
-- 
2.18.1

[PATCH v4 4/8] Documentation/ABI: Add ABI documentation for chips and sockets

[Kajol Jain]

Add documentation for the following sysfs files:
/sys/devices/hv_24x7/interface/chips,
/sys/devices/hv_24x7/interface/sockets

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 .../testing/sysfs-bus-event_source-devices-hv_24x7 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7 b/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
index ec27c6c9e737..e17e5b444a1c 100644
--- a/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
+++ b/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
@@ -22,6 +22,20 @@ Description:
 		Exposes the "version" field of the 24x7 catalog. This is also
 		extractable from the provided binary "catalog" sysfs entry.
 
+What:		/sys/devices/hv_24x7/interface/sockets
+Date:		March 2020
+Contact:	Linux on PowerPC Developer List <linuxppc-dev@lists.ozlabs.org>
+Description:	read only
+		This sysfs interface exposes the number of sockets present in the
+		system.
+
+What:		/sys/devices/hv_24x7/interface/chips
+Date:		March 2020
+Contact:	Linux on PowerPC Developer List <linuxppc-dev@lists.ozlabs.org>
+Description:	read only
+		This sysfs interface exposes the number of chips per socket
+		present in the system.
+
 What:		/sys/bus/event_source/devices/hv_24x7/event_descs/<event-name>
 Date:		February 2014
 Contact:	Linux on PowerPC Developer List <linuxppc-dev@lists.ozlabs.org>
-- 
2.18.1

[PATCH v4 5/8] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration

[Kajol Jain]

Function 'read_sys_info_pseries()' is added to get system parameter
values like number of sockets and chips per socket.
and it gets these details via rtas_call with token
"PROCESSOR_MODULE_INFO".

Incase lpar migrate from one system to another, system
parameter details like chips per sockets or number of sockets might
change. So, it needs to be re-initialized otherwise, these values
corresponds to previous system values.
This patch adds a call to 'read_sys_info_pseries()' from
'post-mobility_fixup()' to re-init the physsockets and physchips values.

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/mobility.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index b571285f6c14..226accd6218b 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -371,6 +371,18 @@ void post_mobility_fixup(void)
 	/* Possibly switch to a new RFI flush type */
 	pseries_setup_rfi_flush();
 
+	/*
+	 * Incase lpar migrate from one system to another, system
+	 * parameter details like chips per sockets and number of sockets
+	 * might change. So, it needs to be re-initialized otherwise these
+	 * values corresponds to previous system.
+	 * Here, adding a call to read_sys_info_pseries() declared in
+	 * platforms/pseries/pseries.h to re-init the physsockets and
+	 * physchips value.
+	 */
+	if (IS_ENABLED(CONFIG_HV_PERF_CTRS) && IS_ENABLED(CONFIG_PPC_RTAS))
+		read_sys_info_pseries();
+
 	return;
 }
 
-- 
2.18.1

[PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Kajol Jain]

Patch enhances current metric infrastructure to handle "?" in the metric
expression. The "?" can be use for parameters whose value not known while
creating metric events and which can be replace later at runtime to
the proper value. It also add flexibility to create multiple events out
of single metric event added in json file.

Patch adds function 'arch_get_runtimeparam' which is a arch specific
function, returns the count of metric events need to be created.
By default it return 1.

One loop is added in function 'metricgroup__add_metric', which create
multiple events at run time depend on return value of
'arch_get_runtimeparam' and merge that event in 'group_list'.

This infrastructure needed for hv_24x7 socket/chip level events.
"hv_24x7" chip level events needs specific chip-id to which the
data is requested. Function 'arch_get_runtimeparam' implemented
in header.c which extract number of sockets from sysfs file
"sockets" under "/sys/devices/hv_24x7/interface/".

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 tools/perf/arch/powerpc/util/header.c |  22 +++++
 tools/perf/util/expr.h                |   1 +
 tools/perf/util/expr.l                |  19 +++-
 tools/perf/util/metricgroup.c         | 124 ++++++++++++++++++++------
 tools/perf/util/metricgroup.h         |   1 +
 tools/perf/util/stat-shadow.c         |   8 ++
 6 files changed, 148 insertions(+), 27 deletions(-)

diff --git a/tools/perf/arch/powerpc/util/header.c b/tools/perf/arch/powerpc/util/header.c
index 3b4cdfc5efd6..036f6b2ce202 100644
--- a/tools/perf/arch/powerpc/util/header.c
+++ b/tools/perf/arch/powerpc/util/header.c
@@ -7,6 +7,11 @@
 #include <string.h>
 #include <linux/stringify.h>
 #include "header.h"
+#include "metricgroup.h"
+#include "evlist.h"
+#include <dirent.h>
+#include "pmu.h"
+#include <api/fs/fs.h>
 
 #define mfspr(rn)       ({unsigned long rval; \
 			 asm volatile("mfspr %0," __stringify(rn) \
@@ -16,6 +21,8 @@
 #define PVR_VER(pvr)    (((pvr) >>  16) & 0xFFFF) /* Version field */
 #define PVR_REV(pvr)    (((pvr) >>   0) & 0xFFFF) /* Revison field */
 
+#define SOCKETS_INFO_FILE_PATH "/devices/hv_24x7/interface/"
+
 int
 get_cpuid(char *buffer, size_t sz)
 {
@@ -44,3 +51,18 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
 
 	return bufp;
 }
+
+int arch_get_runtimeparam(void)
+{
+	int count;
+	char path[PATH_MAX];
+	char filename[] = "sockets";
+
+	snprintf(path, PATH_MAX,
+		 SOCKETS_INFO_FILE_PATH "%s", filename);
+
+	if (sysfs__read_ull(path, (unsigned long long *)&count) < 0)
+		return 1;
+	else
+		return count;
+}
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index 9377538f4097..d17664e628db 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -15,6 +15,7 @@ struct parse_ctx {
 	struct parse_id ids[MAX_PARSE_ID];
 };
 
+int expr__runtimeparam;
 void expr__ctx_init(struct parse_ctx *ctx);
 void expr__add_id(struct parse_ctx *ctx, const char *id, double val);
 int expr__parse(double *final_val, struct parse_ctx *ctx, const char *expr);
diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
index 1928f2a3dddc..ec4b00671f67 100644
--- a/tools/perf/util/expr.l
+++ b/tools/perf/util/expr.l
@@ -45,6 +45,21 @@ static char *normalize(char *str)
 			*dst++ = '/';
 		else if (*str == '\\')
 			*dst++ = *++str;
+        else if (*str == '?') {
+
+			int size = snprintf(NULL, 0, "%d", expr__runtimeparam);
+			char * paramval = (char *)malloc(size);
+			int i = 0;
+
+			if(!paramval)
+				*dst++ = '0';
+			else {
+				sprintf(paramval, "%d", expr__runtimeparam);
+				while(i < size)
+					*dst++ = paramval[i++];
+				free(paramval);
+			}
+		}
 		else
 			*dst++ = *str;
 		str++;
@@ -72,8 +87,8 @@ number		[0-9]+
 
 sch		[-,=]
 spec		\\{sch}
-sym		[0-9a-zA-Z_\.:@]+
-symbol		{spec}*{sym}*{spec}*{sym}*
+sym		[0-9a-zA-Z_\.:@?]+
+symbol		{spec}*{sym}*{spec}*{sym}*{spec}*{sym}
 
 %%
 	{
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index c3a8c701609a..11eeeb929b91 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -474,6 +474,98 @@ static bool metricgroup__has_constraint(struct pmu_event *pe)
 	return false;
 }
 
+int __weak arch_get_runtimeparam(void)
+{
+	return 1;
+}
+
+static int metricgroup__add_metric_runtime_param(struct strbuf *events,
+			struct list_head *group_list, struct pmu_event *pe)
+{
+	int i, count;
+	int ret = -EINVAL;
+
+	count = arch_get_runtimeparam();
+
+	/* This loop is added to create multiple
+	 * events depend on count value and add
+	 * those events to group_list.
+	 */
+
+	for (i = 0; i < count; i++) {
+		const char **ids;
+		int idnum;
+		struct egroup *eg;
+		char value[PATH_MAX];
+
+		expr__runtimeparam = i;
+
+		if (expr__find_other(pe->metric_expr,
+					NULL, &ids, &idnum) < 0)
+			return ret;
+
+		if (events->len > 0)
+			strbuf_addf(events, ",");
+
+		if (metricgroup__has_constraint(pe))
+			metricgroup__add_metric_non_group(events, ids, idnum);
+		else
+			metricgroup__add_metric_weak_group(events, ids, idnum);
+
+		eg = malloc(sizeof(struct egroup));
+		if (!eg) {
+			ret = -ENOMEM;
+			return ret;
+		}
+		sprintf(value, "%s%c%d", pe->metric_name, '_', i);
+		eg->ids = ids;
+		eg->idnum = idnum;
+		eg->metric_name = strdup(value);
+		eg->metric_expr = pe->metric_expr;
+		eg->metric_unit = pe->unit;
+		list_add_tail(&eg->nd, group_list);
+		ret = 0;
+
+		if (ret != 0)
+			break;
+	}
+	return ret;
+}
+static int metricgroup__add_metric_param(struct strbuf *events,
+			struct list_head *group_list, struct pmu_event *pe)
+{
+
+	const char **ids;
+	int idnum;
+	struct egroup *eg;
+	int ret = -EINVAL;
+
+	if (expr__find_other(pe->metric_expr,
+					     NULL, &ids, &idnum) < 0)
+		return ret;
+	if (events->len > 0)
+		strbuf_addf(events, ",");
+
+	if (metricgroup__has_constraint(pe))
+		metricgroup__add_metric_non_group(events, ids, idnum);
+	else
+		metricgroup__add_metric_weak_group(events, ids, idnum);
+
+	eg = malloc(sizeof(struct egroup));
+	if (!eg)
+		ret = -ENOMEM;
+
+	eg->ids = ids;
+	eg->idnum = idnum;
+	eg->metric_name = pe->metric_name;
+	eg->metric_expr = pe->metric_expr;
+	eg->metric_unit = pe->unit;
+	list_add_tail(&eg->nd, group_list);
+	ret = 0;
+
+	return ret;
+}
+
 static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 				   struct list_head *group_list)
 {
@@ -493,35 +585,17 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 			continue;
 		if (match_metric(pe->metric_group, metric) ||
 		    match_metric(pe->metric_name, metric)) {
-			const char **ids;
-			int idnum;
-			struct egroup *eg;
 
 			pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
 
-			if (expr__find_other(pe->metric_expr,
-					     NULL, &ids, &idnum) < 0)
-				continue;
-			if (events->len > 0)
-				strbuf_addf(events, ",");
-
-			if (metricgroup__has_constraint(pe))
-				metricgroup__add_metric_non_group(events, ids, idnum);
+			if (strstr(pe->metric_expr, "?"))
+				ret = metricgroup__add_metric_runtime_param(events,
+							group_list, pe);
 			else
-				metricgroup__add_metric_weak_group(events, ids, idnum);
-
-			eg = malloc(sizeof(struct egroup));
-			if (!eg) {
-				ret = -ENOMEM;
-				break;
-			}
-			eg->ids = ids;
-			eg->idnum = idnum;
-			eg->metric_name = pe->metric_name;
-			eg->metric_expr = pe->metric_expr;
-			eg->metric_unit = pe->unit;
-			list_add_tail(&eg->nd, group_list);
-			ret = 0;
+				ret = metricgroup__add_metric_param(events,
+							group_list, pe);
+			if (ret == -EINVAL)
+				continue;
 		}
 	}
 	return ret;
diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
index 475c7f912864..81224ba1270d 100644
--- a/tools/perf/util/metricgroup.h
+++ b/tools/perf/util/metricgroup.h
@@ -34,4 +34,5 @@ int metricgroup__parse_groups(const struct option *opt,
 void metricgroup__print(bool metrics, bool groups, char *filter,
 			bool raw, bool details);
 bool metricgroup__has_metric(const char *metric);
+int arch_get_runtimeparam(void);
 #endif
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 0fd713d3674f..92c4c9abbaa0 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -777,6 +777,14 @@ static void generic_metric(struct perf_stat_config *config,
 	}
 
 	if (!metric_events[i]) {
+
+		if (strstr(metric_expr, "?")) {
+			char *tmp = strrchr(metric_name, '_');
+
+			tmp++;
+			expr__runtimeparam = strtol(tmp, &tmp, 10);
+		}
+
 		if (expr__parse(&ratio, &pctx, metric_expr) == 0) {
 			char *unit;
 			char metric_bf[64];
-- 
2.18.1

[PATCH v4 7/8] tools/perf: Enable Hz/hz prinitg for --metric-only option

[Kajol Jain]

Commit 54b5091606c18 ("perf stat: Implement --metric-only mode")
added function 'valid_only_metric()' which drops "Hz" or "hz",
if it is part of "ScaleUnit". This patch enable it since hv_24x7
supports couple of frequency events.

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 tools/perf/util/stat-display.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 16efdba1973a..ecdebfcdd379 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -237,8 +237,6 @@ static bool valid_only_metric(const char *unit)
 	if (!unit)
 		return false;
 	if (strstr(unit, "/sec") ||
-	    strstr(unit, "hz") ||
-	    strstr(unit, "Hz") ||
 	    strstr(unit, "CPUs utilized"))
 		return false;
 	return true;
-- 
2.18.1

[PATCH v4 8/8] perf/tools/pmu-events/powerpc: Add hv_24x7 socket/chip level metric events

[Kajol Jain]

The hv_24×7 feature in IBM® POWER9™ processor-based servers provide the
facility to continuously collect large numbers of hardware performance
metrics efficiently and accurately.
This patch adds hv_24x7  metric file for different Socket/chip
resources.

Result:

power9 platform:

command:# ./perf stat --metric-only -M Memory_RD_BW_Chip -C 0 -I 1000

     1.000096188                      0.9                      0.3
     2.000285720                      0.5                      0.1
     3.000424990                      0.4                      0.1

command:# ./perf stat --metric-only -M PowerBUS_Frequency -C 0 -I 1000

     1.000097981                        2.3                        2.3
     2.000291713                        2.3                        2.3
     3.000421719                        2.3                        2.3
     4.000550912                        2.3                        2.3

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 .../arch/powerpc/power9/nest_metrics.json     | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json

diff --git a/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json b/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
new file mode 100644
index 000000000000..ac38f5540ac6
--- /dev/null
+++ b/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
@@ -0,0 +1,19 @@
+[
+    {
+        "MetricExpr": "(hv_24x7@PM_MCS01_128B_RD_DISP_PORT01\\,chip\\=?@ + hv_24x7@PM_MCS01_128B_RD_DISP_PORT23\\,chip\\=?@ + hv_24x7@PM_MCS23_128B_RD_DISP_PORT01\\,chip\\=?@ + hv_24x7@PM_MCS23_128B_RD_DISP_PORT23\\,chip\\=?@)",
+        "MetricName": "Memory_RD_BW_Chip",
+        "MetricGroup": "Memory_BW",
+        "ScaleUnit": "1.6e-2MB"
+    },
+    {
+    "MetricExpr": "(hv_24x7@PM_MCS01_128B_WR_DISP_PORT01\\,chip\\=?@ + hv_24x7@PM_MCS01_128B_WR_DISP_PORT23\\,chip\\=?@ + hv_24x7@PM_MCS23_128B_WR_DISP_PORT01\\,chip\\=?@ + hv_24x7@PM_MCS23_128B_WR_DISP_PORT23\\,chip\\=?@ )",
+        "MetricName": "Memory_WR_BW_Chip",
+        "MetricGroup": "Memory_BW",
+        "ScaleUnit": "1.6e-2MB"
+    },
+    {
+    "MetricExpr": "(hv_24x7@PM_PB_CYC\\,chip\\=?@ )",
+        "MetricName": "PowerBUS_Frequency",
+        "ScaleUnit": "2.5e-7GHz"
+    }
+]
-- 
2.18.1

Re: [PATCH v4 0/8] powerpc/perf: Add json file metric support for the hv_24x7 socket/chip level events

[Jiri Olsa]

On Mon, Mar 09, 2020 at 11:55:44AM +0530, Kajol Jain wrote:
> First patch of the patchset fix inconsistent results we are getting when
> we run multiple 24x7 events.
> 
> Patchset adds json file metric support for the hv_24x7 socket/chip level
> events. "hv_24x7" pmu interface events needs system dependent parameter
> like socket/chip/core. For example, hv_24x7 chip level events needs
> specific chip-id to which the data is requested should be added as part
> of pmu events.
> 
> So to enable JSON file support to "hv_24x7" interface, patchset expose
> total number of sockets and chips per-socket details in sysfs
> files (sockets, chips) under "/sys/devices/hv_24x7/interface/".
> 
> To get sockets and number of chips per sockets, patchset adds a rtas call
> with token "PROCESSOR_MODULE_INFO" to get these details. Patchset also
> handles partition migration case to re-init these system depended
> parameters by adding proper calls in post_mobility_fixup() (mobility.c).
> 
> Patch 6 & 8 of the patchset handles perf tool plumbing needed to replace
> the "?" character in the metric expression to proper value and hv_24x7
> json metric file for different Socket/chip resources.
> 
> Patch set also enable Hz/hz prinitg for --metric-only option to print
> metric data for bus frequency.
> 
> Applied and tested all these patches cleanly on top of jiri's flex changes
> with the changes done by Kan Liang for "Support metric group constraint"
> patchset and made required changes.
> 
> Changelog:
> v3 -> v4
> - Made changes suggested by jiri.

could you please mention them next time? ;-)

> - Apply these patch on top of Kan liang changes.

Arnaldo, could you please pull the expr flex changes and Kan's
metric group constraint changes? it's both prereq of this patchset

thanks,
jirka

Re: [PATCH v4 0/8] powerpc/perf: Add json file metric support for the hv_24x7 socket/chip level events

[Arnaldo Carvalho de Melo]

Em Mon, Mar 09, 2020 at 10:35:06AM +0100, Jiri Olsa escreveu:
> On Mon, Mar 09, 2020 at 11:55:44AM +0530, Kajol Jain wrote:
> > First patch of the patchset fix inconsistent results we are getting when
> > we run multiple 24x7 events.
> > 
> > Patchset adds json file metric support for the hv_24x7 socket/chip level
> > events. "hv_24x7" pmu interface events needs system dependent parameter
> > like socket/chip/core. For example, hv_24x7 chip level events needs
> > specific chip-id to which the data is requested should be added as part
> > of pmu events.
> > 
> > So to enable JSON file support to "hv_24x7" interface, patchset expose
> > total number of sockets and chips per-socket details in sysfs
> > files (sockets, chips) under "/sys/devices/hv_24x7/interface/".
> > 
> > To get sockets and number of chips per sockets, patchset adds a rtas call
> > with token "PROCESSOR_MODULE_INFO" to get these details. Patchset also
> > handles partition migration case to re-init these system depended
> > parameters by adding proper calls in post_mobility_fixup() (mobility.c).
> > 
> > Patch 6 & 8 of the patchset handles perf tool plumbing needed to replace
> > the "?" character in the metric expression to proper value and hv_24x7
> > json metric file for different Socket/chip resources.
> > 
> > Patch set also enable Hz/hz prinitg for --metric-only option to print
> > metric data for bus frequency.
> > 
> > Applied and tested all these patches cleanly on top of jiri's flex changes
> > with the changes done by Kan Liang for "Support metric group constraint"
> > patchset and made required changes.
> > 
> > Changelog:
> > v3 -> v4
> > - Made changes suggested by jiri.
> 
> could you please mention them next time? ;-)
> 
> > - Apply these patch on top of Kan liang changes.
> 
> Arnaldo, could you please pull the expr flex changes and Kan's
> metric group constraint changes? it's both prereq of this patchset

Both are now in my perf/core branch, will go upstream soon, should I go
and pickup the perf tooling bits in this patchkit?

- Arnaldo

Re: [PATCH v4 0/8] powerpc/perf: Add json file metric support for the hv_24x7 socket/chip level events

[Jiri Olsa]

On Tue, Mar 10, 2020 at 03:18:36PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Mar 09, 2020 at 10:35:06AM +0100, Jiri Olsa escreveu:
> > On Mon, Mar 09, 2020 at 11:55:44AM +0530, Kajol Jain wrote:
> > > First patch of the patchset fix inconsistent results we are getting when
> > > we run multiple 24x7 events.
> > > 
> > > Patchset adds json file metric support for the hv_24x7 socket/chip level
> > > events. "hv_24x7" pmu interface events needs system dependent parameter
> > > like socket/chip/core. For example, hv_24x7 chip level events needs
> > > specific chip-id to which the data is requested should be added as part
> > > of pmu events.
> > > 
> > > So to enable JSON file support to "hv_24x7" interface, patchset expose
> > > total number of sockets and chips per-socket details in sysfs
> > > files (sockets, chips) under "/sys/devices/hv_24x7/interface/".
> > > 
> > > To get sockets and number of chips per sockets, patchset adds a rtas call
> > > with token "PROCESSOR_MODULE_INFO" to get these details. Patchset also
> > > handles partition migration case to re-init these system depended
> > > parameters by adding proper calls in post_mobility_fixup() (mobility.c).
> > > 
> > > Patch 6 & 8 of the patchset handles perf tool plumbing needed to replace
> > > the "?" character in the metric expression to proper value and hv_24x7
> > > json metric file for different Socket/chip resources.
> > > 
> > > Patch set also enable Hz/hz prinitg for --metric-only option to print
> > > metric data for bus frequency.
> > > 
> > > Applied and tested all these patches cleanly on top of jiri's flex changes
> > > with the changes done by Kan Liang for "Support metric group constraint"
> > > patchset and made required changes.
> > > 
> > > Changelog:
> > > v3 -> v4
> > > - Made changes suggested by jiri.
> > 
> > could you please mention them next time? ;-)
> > 
> > > - Apply these patch on top of Kan liang changes.
> > 
> > Arnaldo, could you please pull the expr flex changes and Kan's
> > metric group constraint changes? it's both prereq of this patchset
> 
> Both are now in my perf/core branch, will go upstream soon, should I go
> and pickup the perf tooling bits in this patchkit?

Kajol mentioned there are still some issues with the
global variable, I plan to check on it this week

thanks,
jirka

Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Arnaldo Carvalho de Melo]

Em Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain escreveu:
> Patch enhances current metric infrastructure to handle "?" in the metric
> expression. The "?" can be use for parameters whose value not known while
> creating metric events and which can be replace later at runtime to
> the proper value. It also add flexibility to create multiple events out
> of single metric event added in json file.
> 
> Patch adds function 'arch_get_runtimeparam' which is a arch specific
> function, returns the count of metric events need to be created.
> By default it return 1.
> 
> One loop is added in function 'metricgroup__add_metric', which create
> multiple events at run time depend on return value of
> 'arch_get_runtimeparam' and merge that event in 'group_list'.
> 
> This infrastructure needed for hv_24x7 socket/chip level events.
> "hv_24x7" chip level events needs specific chip-id to which the
> data is requested. Function 'arch_get_runtimeparam' implemented
> in header.c which extract number of sockets from sysfs file
> "sockets" under "/sys/devices/hv_24x7/interface/".
> 
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
>  tools/perf/arch/powerpc/util/header.c |  22 +++++
>  tools/perf/util/expr.h                |   1 +
>  tools/perf/util/expr.l                |  19 +++-
>  tools/perf/util/metricgroup.c         | 124 ++++++++++++++++++++------
>  tools/perf/util/metricgroup.h         |   1 +
>  tools/perf/util/stat-shadow.c         |   8 ++
>  6 files changed, 148 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/perf/arch/powerpc/util/header.c b/tools/perf/arch/powerpc/util/header.c
> index 3b4cdfc5efd6..036f6b2ce202 100644
> --- a/tools/perf/arch/powerpc/util/header.c
> +++ b/tools/perf/arch/powerpc/util/header.c
> @@ -7,6 +7,11 @@
>  #include <string.h>
>  #include <linux/stringify.h>
>  #include "header.h"
> +#include "metricgroup.h"
> +#include "evlist.h"
> +#include <dirent.h>
> +#include "pmu.h"
> +#include <api/fs/fs.h>
>  
>  #define mfspr(rn)       ({unsigned long rval; \
>  			 asm volatile("mfspr %0," __stringify(rn) \
> @@ -16,6 +21,8 @@
>  #define PVR_VER(pvr)    (((pvr) >>  16) & 0xFFFF) /* Version field */
>  #define PVR_REV(pvr)    (((pvr) >>   0) & 0xFFFF) /* Revison field */
>  
> +#define SOCKETS_INFO_FILE_PATH "/devices/hv_24x7/interface/"
> +
>  int
>  get_cpuid(char *buffer, size_t sz)
>  {
> @@ -44,3 +51,18 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
>  
>  	return bufp;
>  }
> +
> +int arch_get_runtimeparam(void)
> +{
> +	int count;
> +	char path[PATH_MAX];
> +	char filename[] = "sockets";
> +
> +	snprintf(path, PATH_MAX,
> +		 SOCKETS_INFO_FILE_PATH "%s", filename);
> +
> +	if (sysfs__read_ull(path, (unsigned long long *)&count) < 0)
> +		return 1;
> +	else
> +		return count;

Why this cast dance? We have sysfs__read_int(path, &count).

Also this is more compact:

	return sysfs__read_int(path, &count) < 0 ? 1 : count;

> +}
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 9377538f4097..d17664e628db 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -15,6 +15,7 @@ struct parse_ctx {
>  	struct parse_id ids[MAX_PARSE_ID];
>  };
>  
> +int expr__runtimeparam;
>  void expr__ctx_init(struct parse_ctx *ctx);
>  void expr__add_id(struct parse_ctx *ctx, const char *id, double val);
>  int expr__parse(double *final_val, struct parse_ctx *ctx, const char *expr);
> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
> index 1928f2a3dddc..ec4b00671f67 100644
> --- a/tools/perf/util/expr.l
> +++ b/tools/perf/util/expr.l
> @@ -45,6 +45,21 @@ static char *normalize(char *str)
>  			*dst++ = '/';
>  		else if (*str == '\\')
>  			*dst++ = *++str;
> +        else if (*str == '?') {
> +
> +			int size = snprintf(NULL, 0, "%d", expr__runtimeparam);

TIL that C99 allows for a NULL str to format and return the number of
bytes it would write if the string was large enough... wonders never
cease :-)

> +			char * paramval = (char *)malloc(size);

No need for the cast, malloc returns void *, or has that changed? 8-)
and please no space before the variable name.

Humm this is all complicated, why not use asprintf and have something
like:

> +			int i = 0;
> +
> +			if(!paramval)
> +				*dst++ = '0';
> +			else {
> +				sprintf(paramval, "%d", expr__runtimeparam);
> +				while(i < size)
> +					*dst++ = paramval[i++];
> +				free(paramval);
> +			}

			char *paramval;
			int size = asprintf(&paramval, "%d", expr__runtimeparam);

			if (size < 0)
				*dst++ = '0';
			else {
				while (i < size)
					*dst++ = paramval[i++];
				free(paramval);
			}


> +		}
>  		else
>  			*dst++ = *str;
>  		str++;
> @@ -72,8 +87,8 @@ number		[0-9]+
>  
>  sch		[-,=]
>  spec		\\{sch}
> -sym		[0-9a-zA-Z_\.:@]+
> -symbol		{spec}*{sym}*{spec}*{sym}*
> +sym		[0-9a-zA-Z_\.:@?]+
> +symbol		{spec}*{sym}*{spec}*{sym}*{spec}*{sym}
>  
>  %%
>  	{
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index c3a8c701609a..11eeeb929b91 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -474,6 +474,98 @@ static bool metricgroup__has_constraint(struct pmu_event *pe)
>  	return false;
>  }
>  
> +int __weak arch_get_runtimeparam(void)
> +{
> +	return 1;
> +}
> +
> +static int metricgroup__add_metric_runtime_param(struct strbuf *events,
> +			struct list_head *group_list, struct pmu_event *pe)
> +{
> +	int i, count;
> +	int ret = -EINVAL;
> +
> +	count = arch_get_runtimeparam();
> +
> +	/* This loop is added to create multiple
> +	 * events depend on count value and add
> +	 * those events to group_list.
> +	 */
> +
> +	for (i = 0; i < count; i++) {
> +		const char **ids;
> +		int idnum;
> +		struct egroup *eg;
> +		char value[PATH_MAX];
> +
> +		expr__runtimeparam = i;
> +
> +		if (expr__find_other(pe->metric_expr,
> +					NULL, &ids, &idnum) < 0)
> +			return ret;
> +
> +		if (events->len > 0)
> +			strbuf_addf(events, ",");
> +
> +		if (metricgroup__has_constraint(pe))
> +			metricgroup__add_metric_non_group(events, ids, idnum);
> +		else
> +			metricgroup__add_metric_weak_group(events, ids, idnum);
> +
> +		eg = malloc(sizeof(struct egroup));

Shorter form that works even if you change that type:

+		eg = malloc(sizeof(*eg));

> +		if (!eg) {
> +			ret = -ENOMEM;
> +			return ret;
> +		}
> +		sprintf(value, "%s%c%d", pe->metric_name, '_', i);
> +		eg->ids = ids;
> +		eg->idnum = idnum;
> +		eg->metric_name = strdup(value);

Please check strdup() return just like you checked the malloc(sizeof(struct egroup)).

> +		eg->metric_expr = pe->metric_expr;
> +		eg->metric_unit = pe->unit;
> +		list_add_tail(&eg->nd, group_list);
> +		ret = 0;
> +
> +		if (ret != 0)
> +			break;
> +	}
> +	return ret;
> +}
> +static int metricgroup__add_metric_param(struct strbuf *events,
> +			struct list_head *group_list, struct pmu_event *pe)
> +{
> +
> +	const char **ids;
> +	int idnum;
> +	struct egroup *eg;
> +	int ret = -EINVAL;
> +
> +	if (expr__find_other(pe->metric_expr,
> +					     NULL, &ids, &idnum) < 0)

Why break the above in two lines?

> +		return ret;
> +	if (events->len > 0)
> +		strbuf_addf(events, ",");
> +
> +	if (metricgroup__has_constraint(pe))
> +		metricgroup__add_metric_non_group(events, ids, idnum);
> +	else
> +		metricgroup__add_metric_weak_group(events, ids, idnum);
> +
> +	eg = malloc(sizeof(struct egroup));

Ditto

> +	if (!eg)
> +		ret = -ENOMEM;
> +
> +	eg->ids = ids;
> +	eg->idnum = idnum;
> +	eg->metric_name = pe->metric_name;
> +	eg->metric_expr = pe->metric_expr;
> +	eg->metric_unit = pe->unit;
> +	list_add_tail(&eg->nd, group_list);
> +	ret = 0;
> +
> +	return ret;
> +}
> +
>  static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>  				   struct list_head *group_list)
>  {
> @@ -493,35 +585,17 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>  			continue;
>  		if (match_metric(pe->metric_group, metric) ||
>  		    match_metric(pe->metric_name, metric)) {
> -			const char **ids;
> -			int idnum;
> -			struct egroup *eg;
>  
>  			pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>  
> -			if (expr__find_other(pe->metric_expr,
> -					     NULL, &ids, &idnum) < 0)
> -				continue;
> -			if (events->len > 0)
> -				strbuf_addf(events, ",");
> -
> -			if (metricgroup__has_constraint(pe))
> -				metricgroup__add_metric_non_group(events, ids, idnum);
> +			if (strstr(pe->metric_expr, "?"))
> +				ret = metricgroup__add_metric_runtime_param(events,
> +							group_list, pe);
>  			else
> -				metricgroup__add_metric_weak_group(events, ids, idnum);
> -
> -			eg = malloc(sizeof(struct egroup));

                                             *eg
> -			if (!eg) {
> -				ret = -ENOMEM;
> -				break;
> -			}
> -			eg->ids = ids;
> -			eg->idnum = idnum;
> -			eg->metric_name = pe->metric_name;
> -			eg->metric_expr = pe->metric_expr;
> -			eg->metric_unit = pe->unit;
> -			list_add_tail(&eg->nd, group_list);
> -			ret = 0;
> +				ret = metricgroup__add_metric_param(events,
> +							group_list, pe);
> +			if (ret == -EINVAL)
> +				continue;
>  		}
>  	}
>  	return ret;
> diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
> index 475c7f912864..81224ba1270d 100644
> --- a/tools/perf/util/metricgroup.h
> +++ b/tools/perf/util/metricgroup.h
> @@ -34,4 +34,5 @@ int metricgroup__parse_groups(const struct option *opt,
>  void metricgroup__print(bool metrics, bool groups, char *filter,
>  			bool raw, bool details);
>  bool metricgroup__has_metric(const char *metric);
> +int arch_get_runtimeparam(void);
>  #endif
> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> index 0fd713d3674f..92c4c9abbaa0 100644
> --- a/tools/perf/util/stat-shadow.c
> +++ b/tools/perf/util/stat-shadow.c
> @@ -777,6 +777,14 @@ static void generic_metric(struct perf_stat_config *config,
>  	}
>  
>  	if (!metric_events[i]) {
> +
> +		if (strstr(metric_expr, "?")) {
> +			char *tmp = strrchr(metric_name, '_');
> +

So at this point a metric name is guaranteed to have a _?

> +			tmp++;
> +			expr__runtimeparam = strtol(tmp, &tmp, 10);
> +		}
> +
>  		if (expr__parse(&ratio, &pctx, metric_expr) == 0) {
>  			char *unit;
>  			char metric_bf[64];
> -- 
> 2.18.1
> 

-- 

- Arnaldo

Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[kajoljain]

On 3/11/20 12:04 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain escreveu:
>> Patch enhances current metric infrastructure to handle "?" in the metric
>> expression. The "?" can be use for parameters whose value not known while
>> creating metric events and which can be replace later at runtime to
>> the proper value. It also add flexibility to create multiple events out
>> of single metric event added in json file.
>>
>> Patch adds function 'arch_get_runtimeparam' which is a arch specific
>> function, returns the count of metric events need to be created.
>> By default it return 1.
>>
>> One loop is added in function 'metricgroup__add_metric', which create
>> multiple events at run time depend on return value of
>> 'arch_get_runtimeparam' and merge that event in 'group_list'.
>>
>> This infrastructure needed for hv_24x7 socket/chip level events.
>> "hv_24x7" chip level events needs specific chip-id to which the
>> data is requested. Function 'arch_get_runtimeparam' implemented
>> in header.c which extract number of sockets from sysfs file
>> "sockets" under "/sys/devices/hv_24x7/interface/".
>>
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>>  tools/perf/arch/powerpc/util/header.c |  22 +++++
>>  tools/perf/util/expr.h                |   1 +
>>  tools/perf/util/expr.l                |  19 +++-
>>  tools/perf/util/metricgroup.c         | 124 ++++++++++++++++++++------
>>  tools/perf/util/metricgroup.h         |   1 +
>>  tools/perf/util/stat-shadow.c         |   8 ++
>>  6 files changed, 148 insertions(+), 27 deletions(-)
>>
>> diff --git a/tools/perf/arch/powerpc/util/header.c b/tools/perf/arch/powerpc/util/header.c
>> index 3b4cdfc5efd6..036f6b2ce202 100644
>> --- a/tools/perf/arch/powerpc/util/header.c
>> +++ b/tools/perf/arch/powerpc/util/header.c
>> @@ -7,6 +7,11 @@
>>  #include <string.h>
>>  #include <linux/stringify.h>
>>  #include "header.h"
>> +#include "metricgroup.h"
>> +#include "evlist.h"
>> +#include <dirent.h>
>> +#include "pmu.h"
>> +#include <api/fs/fs.h>
>>  
>>  #define mfspr(rn)       ({unsigned long rval; \
>>  			 asm volatile("mfspr %0," __stringify(rn) \
>> @@ -16,6 +21,8 @@
>>  #define PVR_VER(pvr)    (((pvr) >>  16) & 0xFFFF) /* Version field */
>>  #define PVR_REV(pvr)    (((pvr) >>   0) & 0xFFFF) /* Revison field */
>>  
>> +#define SOCKETS_INFO_FILE_PATH "/devices/hv_24x7/interface/"
>> +
>>  int
>>  get_cpuid(char *buffer, size_t sz)
>>  {
>> @@ -44,3 +51,18 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
>>  
>>  	return bufp;
>>  }
>> +
>> +int arch_get_runtimeparam(void)
>> +{
>> +	int count;
>> +	char path[PATH_MAX];
>> +	char filename[] = "sockets";
>> +
>> +	snprintf(path, PATH_MAX,
>> +		 SOCKETS_INFO_FILE_PATH "%s", filename);
>> +
>> +	if (sysfs__read_ull(path, (unsigned long long *)&count) < 0)
>> +		return 1;
>> +	else
>> +		return count;
> 
> Why this cast dance? We have sysfs__read_int(path, &count).
> 
> Also this is more compact:
> 
> 	return sysfs__read_int(path, &count) < 0 ? 1 : count;

Hi Arnaldo,
     Yes it will be better to use like that.
> 
>> +}
>> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
>> index 9377538f4097..d17664e628db 100644
>> --- a/tools/perf/util/expr.h
>> +++ b/tools/perf/util/expr.h
>> @@ -15,6 +15,7 @@ struct parse_ctx {
>>  	struct parse_id ids[MAX_PARSE_ID];
>>  };
>>  
>> +int expr__runtimeparam;
>>  void expr__ctx_init(struct parse_ctx *ctx);
>>  void expr__add_id(struct parse_ctx *ctx, const char *id, double val);
>>  int expr__parse(double *final_val, struct parse_ctx *ctx, const char *expr);
>> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
>> index 1928f2a3dddc..ec4b00671f67 100644
>> --- a/tools/perf/util/expr.l
>> +++ b/tools/perf/util/expr.l
>> @@ -45,6 +45,21 @@ static char *normalize(char *str)
>>  			*dst++ = '/';
>>  		else if (*str == '\\')
>>  			*dst++ = *++str;
>> +        else if (*str == '?') {
>> +
>> +			int size = snprintf(NULL, 0, "%d", expr__runtimeparam);
> 
> TIL that C99 allows for a NULL str to format and return the number of
> bytes it would write if the string was large enough... wonders never
> cease :-)
> 
>> +			char * paramval = (char *)malloc(size);
> 
> No need for the cast, malloc returns void *, or has that changed? 8-)
> and please no space before the variable name.
> 

Okk, Will take care of that.

> Humm this is all complicated, why not use asprintf and have something
> like:
> 
>> +			int i = 0;
>> +
>> +			if(!paramval)
>> +				*dst++ = '0';
>> +			else {
>> +				sprintf(paramval, "%d", expr__runtimeparam);
>> +				while(i < size)
>> +					*dst++ = paramval[i++];
>> +				free(paramval);
>> +			}
> 
> 			char *paramval;
> 			int size = asprintf(&paramval, "%d", expr__runtimeparam);
> 
> 			if (size < 0)
> 				*dst++ = '0';
> 			else {
> 				while (i < size)
> 					*dst++ = paramval[i++];
> 				free(paramval);
> 			}
> 
> 
>> +		}
>>  		else
>>  			*dst++ = *str;
>>  		str++;
>> @@ -72,8 +87,8 @@ number		[0-9]+
>>  
>>  sch		[-,=]
>>  spec		\\{sch}
>> -sym		[0-9a-zA-Z_\.:@]+
>> -symbol		{spec}*{sym}*{spec}*{sym}*
>> +sym		[0-9a-zA-Z_\.:@?]+
>> +symbol		{spec}*{sym}*{spec}*{sym}*{spec}*{sym}
>>  
>>  %%
>>  	{
>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>> index c3a8c701609a..11eeeb929b91 100644
>> --- a/tools/perf/util/metricgroup.c
>> +++ b/tools/perf/util/metricgroup.c
>> @@ -474,6 +474,98 @@ static bool metricgroup__has_constraint(struct pmu_event *pe)
>>  	return false;
>>  }
>>  
>> +int __weak arch_get_runtimeparam(void)
>> +{
>> +	return 1;
>> +}
>> +
>> +static int metricgroup__add_metric_runtime_param(struct strbuf *events,
>> +			struct list_head *group_list, struct pmu_event *pe)
>> +{
>> +	int i, count;
>> +	int ret = -EINVAL;
>> +
>> +	count = arch_get_runtimeparam();
>> +
>> +	/* This loop is added to create multiple
>> +	 * events depend on count value and add
>> +	 * those events to group_list.
>> +	 */
>> +
>> +	for (i = 0; i < count; i++) {
>> +		const char **ids;
>> +		int idnum;
>> +		struct egroup *eg;
>> +		char value[PATH_MAX];
>> +
>> +		expr__runtimeparam = i;
>> +
>> +		if (expr__find_other(pe->metric_expr,
>> +					NULL, &ids, &idnum) < 0)
>> +			return ret;
>> +
>> +		if (events->len > 0)
>> +			strbuf_addf(events, ",");
>> +
>> +		if (metricgroup__has_constraint(pe))
>> +			metricgroup__add_metric_non_group(events, ids, idnum);
>> +		else
>> +			metricgroup__add_metric_weak_group(events, ids, idnum);
>> +
>> +		eg = malloc(sizeof(struct egroup));
> 
> Shorter form that works even if you change that type:
> 
> +		eg = malloc(sizeof(*eg));
> 
>> +		if (!eg) {
>> +			ret = -ENOMEM;
>> +			return ret;
>> +		}
>> +		sprintf(value, "%s%c%d", pe->metric_name, '_', i);
>> +		eg->ids = ids;
>> +		eg->idnum = idnum;
>> +		eg->metric_name = strdup(value);
> 
> Please check strdup() return just like you checked the malloc(sizeof(struct egroup)).

Right, I missed that part, will update it.

> 
>> +		eg->metric_expr = pe->metric_expr;
>> +		eg->metric_unit = pe->unit;
>> +		list_add_tail(&eg->nd, group_list);
>> +		ret = 0;
>> +
>> +		if (ret != 0)
>> +			break;
>> +	}
>> +	return ret;
>> +}
>> +static int metricgroup__add_metric_param(struct strbuf *events,
>> +			struct list_head *group_list, struct pmu_event *pe)
>> +{
>> +
>> +	const char **ids;
>> +	int idnum;
>> +	struct egroup *eg;
>> +	int ret = -EINVAL;
>> +
>> +	if (expr__find_other(pe->metric_expr,
>> +					     NULL, &ids, &idnum) < 0)
> 
> Why break the above in two lines?
> 
>> +		return ret;
>> +	if (events->len > 0)
>> +		strbuf_addf(events, ",");
>> +
>> +	if (metricgroup__has_constraint(pe))
>> +		metricgroup__add_metric_non_group(events, ids, idnum);
>> +	else
>> +		metricgroup__add_metric_weak_group(events, ids, idnum);
>> +
>> +	eg = malloc(sizeof(struct egroup));
> 
> Ditto
> 
>> +	if (!eg)
>> +		ret = -ENOMEM;
>> +
>> +	eg->ids = ids;
>> +	eg->idnum = idnum;
>> +	eg->metric_name = pe->metric_name;
>> +	eg->metric_expr = pe->metric_expr;
>> +	eg->metric_unit = pe->unit;
>> +	list_add_tail(&eg->nd, group_list);
>> +	ret = 0;
>> +
>> +	return ret;
>> +}
>> +
>>  static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>>  				   struct list_head *group_list)
>>  {
>> @@ -493,35 +585,17 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>>  			continue;
>>  		if (match_metric(pe->metric_group, metric) ||
>>  		    match_metric(pe->metric_name, metric)) {
>> -			const char **ids;
>> -			int idnum;
>> -			struct egroup *eg;
>>  
>>  			pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>>  
>> -			if (expr__find_other(pe->metric_expr,
>> -					     NULL, &ids, &idnum) < 0)
>> -				continue;
>> -			if (events->len > 0)
>> -				strbuf_addf(events, ",");
>> -
>> -			if (metricgroup__has_constraint(pe))
>> -				metricgroup__add_metric_non_group(events, ids, idnum);
>> +			if (strstr(pe->metric_expr, "?"))
>> +				ret = metricgroup__add_metric_runtime_param(events,
>> +							group_list, pe);
>>  			else
>> -				metricgroup__add_metric_weak_group(events, ids, idnum);
>> -
>> -			eg = malloc(sizeof(struct egroup));
> 
>                                              *eg
>> -			if (!eg) {
>> -				ret = -ENOMEM;
>> -				break;
>> -			}
>> -			eg->ids = ids;
>> -			eg->idnum = idnum;
>> -			eg->metric_name = pe->metric_name;
>> -			eg->metric_expr = pe->metric_expr;
>> -			eg->metric_unit = pe->unit;
>> -			list_add_tail(&eg->nd, group_list);
>> -			ret = 0;
>> +				ret = metricgroup__add_metric_param(events,
>> +							group_list, pe);
>> +			if (ret == -EINVAL)
>> +				continue;
>>  		}
>>  	}
>>  	return ret;
>> diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
>> index 475c7f912864..81224ba1270d 100644
>> --- a/tools/perf/util/metricgroup.h
>> +++ b/tools/perf/util/metricgroup.h
>> @@ -34,4 +34,5 @@ int metricgroup__parse_groups(const struct option *opt,
>>  void metricgroup__print(bool metrics, bool groups, char *filter,
>>  			bool raw, bool details);
>>  bool metricgroup__has_metric(const char *metric);
>> +int arch_get_runtimeparam(void);
>>  #endif
>> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
>> index 0fd713d3674f..92c4c9abbaa0 100644
>> --- a/tools/perf/util/stat-shadow.c
>> +++ b/tools/perf/util/stat-shadow.c
>> @@ -777,6 +777,14 @@ static void generic_metric(struct perf_stat_config *config,
>>  	}
>>  
>>  	if (!metric_events[i]) {
>> +
>> +		if (strstr(metric_expr, "?")) {
>> +			char *tmp = strrchr(metric_name, '_');
>> +
> 
> So at this point a metric name is guaranteed to have a _?

Yes, because we are appending '_' with parameter value in metric name
when we adding these events in 'group_list'. So, I added it, because
we can have multiple values of that runtime_parameter, so by appending it
in metric name, we can understand to which event the counter data belongs.
Thanks for reviewing the patch, I will send updated patch.

Thanks,
Kajol
> 
>> +			tmp++;
>> +			expr__runtimeparam = strtol(tmp, &tmp, 10);
>> +		}
>> +
>>  		if (expr__parse(&ratio, &pctx, metric_expr) == 0) {
>>  			char *unit;
>>  			char metric_bf[64];
>> -- 
>> 2.18.1
>>
> 

Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain wrote:

SNIP

> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index c3a8c701609a..11eeeb929b91 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -474,6 +474,98 @@ static bool metricgroup__has_constraint(struct pmu_event *pe)
>  	return false;
>  }
>  
> +int __weak arch_get_runtimeparam(void)
> +{
> +	return 1;
> +}
> +
> +static int metricgroup__add_metric_runtime_param(struct strbuf *events,
> +			struct list_head *group_list, struct pmu_event *pe)
> +{
> +	int i, count;
> +	int ret = -EINVAL;
> +
> +	count = arch_get_runtimeparam();
> +
> +	/* This loop is added to create multiple
> +	 * events depend on count value and add
> +	 * those events to group_list.
> +	 */
> +
> +	for (i = 0; i < count; i++) {
> +		const char **ids;
> +		int idnum;
> +		struct egroup *eg;
> +		char value[PATH_MAX];
> +
> +		expr__runtimeparam = i;

so the expr__runtimeparam is always set before we call the
expr parsing function - wither expr__find_other or expr__parse,

and it's used inside the normalize flexer function, which has
access to the passed context.. so I don't see a reason why
expr__runtimeparam couldn't be added in struct parse_ctx
and used from there..

while in this, perhaps we should rename parse_ctx to expr_ctx,
to keep the namespace straight (in separate patch)

thanks,
jirka

Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain wrote:

SNIP

> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
> index 1928f2a3dddc..ec4b00671f67 100644
> --- a/tools/perf/util/expr.l
> +++ b/tools/perf/util/expr.l
> @@ -45,6 +45,21 @@ static char *normalize(char *str)
>  			*dst++ = '/';
>  		else if (*str == '\\')
>  			*dst++ = *++str;
> +        else if (*str == '?') {
> +

extra line ^^^

jirka

> +			int size = snprintf(NULL, 0, "%d", expr__runtimeparam);
> +			char * paramval = (char *)malloc(size);
> +			int i = 0;
> +
> +			if(!paramval)

SNIP

Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Tue, Mar 10, 2020 at 03:34:55PM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> > diff --git a/tools/perf/arch/powerpc/util/header.c b/tools/perf/arch/powerpc/util/header.c
> > index 3b4cdfc5efd6..036f6b2ce202 100644
> > --- a/tools/perf/arch/powerpc/util/header.c
> > +++ b/tools/perf/arch/powerpc/util/header.c
> > @@ -7,6 +7,11 @@
> >  #include <string.h>
> >  #include <linux/stringify.h>
> >  #include "header.h"
> > +#include "metricgroup.h"
> > +#include "evlist.h"
> > +#include <dirent.h>
> > +#include "pmu.h"
> > +#include <api/fs/fs.h>
> >  
> >  #define mfspr(rn)       ({unsigned long rval; \
> >  			 asm volatile("mfspr %0," __stringify(rn) \
> > @@ -16,6 +21,8 @@
> >  #define PVR_VER(pvr)    (((pvr) >>  16) & 0xFFFF) /* Version field */
> >  #define PVR_REV(pvr)    (((pvr) >>   0) & 0xFFFF) /* Revison field */
> >  
> > +#define SOCKETS_INFO_FILE_PATH "/devices/hv_24x7/interface/"
> > +
> >  int
> >  get_cpuid(char *buffer, size_t sz)
> >  {
> > @@ -44,3 +51,18 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
> >  
> >  	return bufp;
> >  }
> > +
> > +int arch_get_runtimeparam(void)
> > +{
> > +	int count;
> > +	char path[PATH_MAX];
> > +	char filename[] = "sockets";
> > +
> > +	snprintf(path, PATH_MAX,
> > +		 SOCKETS_INFO_FILE_PATH "%s", filename);

also, what's the point of using snprintf in here?

jirka

Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain wrote:

SNIP

> +static int metricgroup__add_metric_runtime_param(struct strbuf *events,
> +			struct list_head *group_list, struct pmu_event *pe)
> +{
> +	int i, count;
> +	int ret = -EINVAL;
> +
> +	count = arch_get_runtimeparam();
> +
> +	/* This loop is added to create multiple
> +	 * events depend on count value and add
> +	 * those events to group_list.
> +	 */
> +
> +	for (i = 0; i < count; i++) {
> +		const char **ids;
> +		int idnum;
> +		struct egroup *eg;
> +		char value[PATH_MAX];
> +
> +		expr__runtimeparam = i;
> +
> +		if (expr__find_other(pe->metric_expr,
> +					NULL, &ids, &idnum) < 0)
> +			return ret;
> +
> +		if (events->len > 0)
> +			strbuf_addf(events, ",");
> +
> +		if (metricgroup__has_constraint(pe))
> +			metricgroup__add_metric_non_group(events, ids, idnum);
> +		else
> +			metricgroup__add_metric_weak_group(events, ids, idnum);
> +
> +		eg = malloc(sizeof(struct egroup));
> +		if (!eg) {
> +			ret = -ENOMEM;
> +			return ret;
> +		}
> +		sprintf(value, "%s%c%d", pe->metric_name, '_', i);
> +		eg->ids = ids;
> +		eg->idnum = idnum;
> +		eg->metric_name = strdup(value);
> +		eg->metric_expr = pe->metric_expr;
> +		eg->metric_unit = pe->unit;
> +		list_add_tail(&eg->nd, group_list);
> +		ret = 0;
> +
> +		if (ret != 0)
> +			break;

the inside loop is essentialy what you factor out to
metricgroup__add_metric_param right? please nove
addition of metricgroup__add_metric_param function
into separate patch

jirka


> +	}
> +	return ret;
> +}
> +static int metricgroup__add_metric_param(struct strbuf *events,
> +			struct list_head *group_list, struct pmu_event *pe)
> +{
> +
> +	const char **ids;
> +	int idnum;
> +	struct egroup *eg;
> +	int ret = -EINVAL;
> +
> +	if (expr__find_other(pe->metric_expr,
> +					     NULL, &ids, &idnum) < 0)
> +		return ret;
> +	if (events->len > 0)
> +		strbuf_addf(events, ",");
> +
> +	if (metricgroup__has_constraint(pe))
> +		metricgroup__add_metric_non_group(events, ids, idnum);
> +	else
> +		metricgroup__add_metric_weak_group(events, ids, idnum);
> +
> +	eg = malloc(sizeof(struct egroup));
> +	if (!eg)
> +		ret = -ENOMEM;
> +
> +	eg->ids = ids;
> +	eg->idnum = idnum;
> +	eg->metric_name = pe->metric_name;
> +	eg->metric_expr = pe->metric_expr;
> +	eg->metric_unit = pe->unit;
> +	list_add_tail(&eg->nd, group_list);
> +	ret = 0;
> +
> +	return ret;
> +}

SNIP

Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[Jiri Olsa]

On Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain wrote:

SNIP

> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 9377538f4097..d17664e628db 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -15,6 +15,7 @@ struct parse_ctx {
>  	struct parse_id ids[MAX_PARSE_ID];
>  };
>  
> +int expr__runtimeparam;
>  void expr__ctx_init(struct parse_ctx *ctx);
>  void expr__add_id(struct parse_ctx *ctx, const char *id, double val);
>  int expr__parse(double *final_val, struct parse_ctx *ctx, const char *expr);
> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
> index 1928f2a3dddc..ec4b00671f67 100644
> --- a/tools/perf/util/expr.l
> +++ b/tools/perf/util/expr.l
> @@ -45,6 +45,21 @@ static char *normalize(char *str)
>  			*dst++ = '/';
>  		else if (*str == '\\')
>  			*dst++ = *++str;
> +        else if (*str == '?') {
> +
> +			int size = snprintf(NULL, 0, "%d", expr__runtimeparam);
> +			char * paramval = (char *)malloc(size);

can't we agree that any reasonable number in here
wouldn't cross 20 bytes in string or so and use
buffer for that instead of that malloc exercise?

thanks,
jirka

> +			int i = 0;
> +
> +			if(!paramval)
> +				*dst++ = '0';
> +			else {
> +				sprintf(paramval, "%d", expr__runtimeparam);
> +				while(i < size)
> +					*dst++ = paramval[i++];
> +				free(paramval);
> +			}
> +		}

SNIP

Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"

[kajoljain]

On 3/12/20 4:22 PM, Jiri Olsa wrote:
> On Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain wrote:
> 
> SNIP
> 
>> +static int metricgroup__add_metric_runtime_param(struct strbuf *events,
>> +			struct list_head *group_list, struct pmu_event *pe)
>> +{
>> +	int i, count;
>> +	int ret = -EINVAL;
>> +
>> +	count = arch_get_runtimeparam();
>> +
>> +	/* This loop is added to create multiple
>> +	 * events depend on count value and add
>> +	 * those events to group_list.
>> +	 */
>> +
>> +	for (i = 0; i < count; i++) {
>> +		const char **ids;
>> +		int idnum;
>> +		struct egroup *eg;
>> +		char value[PATH_MAX];
>> +
>> +		expr__runtimeparam = i;
>> +
>> +		if (expr__find_other(pe->metric_expr,
>> +					NULL, &ids, &idnum) < 0)
>> +			return ret;
>> +
>> +		if (events->len > 0)
>> +			strbuf_addf(events, ",");
>> +
>> +		if (metricgroup__has_constraint(pe))
>> +			metricgroup__add_metric_non_group(events, ids, idnum);
>> +		else
>> +			metricgroup__add_metric_weak_group(events, ids, idnum);
>> +
>> +		eg = malloc(sizeof(struct egroup));
>> +		if (!eg) {
>> +			ret = -ENOMEM;
>> +			return ret;
>> +		}
>> +		sprintf(value, "%s%c%d", pe->metric_name, '_', i);
>> +		eg->ids = ids;
>> +		eg->idnum = idnum;
>> +		eg->metric_name = strdup(value);
>> +		eg->metric_expr = pe->metric_expr;
>> +		eg->metric_unit = pe->unit;
>> +		list_add_tail(&eg->nd, group_list);
>> +		ret = 0;
>> +
>> +		if (ret != 0)
>> +			break;
> 
> the inside loop is essentialy what you factor out to
> metricgroup__add_metric_param right? please nove
> addition of metricgroup__add_metric_param function
> into separate patch
> 
> jirka
> 

Ok will put it in seperate patch.

Thanks,
kajol
> 
>> +	}
>> +	return ret;
>> +}
>> +static int metricgroup__add_metric_param(struct strbuf *events,
>> +			struct list_head *group_list, struct pmu_event *pe)
>> +{
>> +
>> +	const char **ids;
>> +	int idnum;
>> +	struct egroup *eg;
>> +	int ret = -EINVAL;
>> +
>> +	if (expr__find_other(pe->metric_expr,
>> +					     NULL, &ids, &idnum) < 0)
>> +		return ret;
>> +	if (events->len > 0)
>> +		strbuf_addf(events, ",");
>> +
>> +	if (metricgroup__has_constraint(pe))
>> +		metricgroup__add_metric_non_group(events, ids, idnum);
>> +	else
>> +		metricgroup__add_metric_weak_group(events, ids, idnum);
>> +
>> +	eg = malloc(sizeof(struct egroup));
>> +	if (!eg)
>> +		ret = -ENOMEM;
>> +
>> +	eg->ids = ids;
>> +	eg->idnum = idnum;
>> +	eg->metric_name = pe->metric_name;
>> +	eg->metric_expr = pe->metric_expr;
>> +	eg->metric_unit = pe->unit;
>> +	list_add_tail(&eg->nd, group_list);
>> +	ret = 0;
>> +
>> +	return ret;
>> +}
> 
> SNIP
>