[PATCH v6 0/5] powerpc/perf: Add json file support for hv_24x7 core level events

[PATCH v6 0/5] powerpc/perf: Add json file support for hv_24x7 core level events

[Kajol Jain]

Patchset enhance current runtime parameter support. It introduces new
fields like "PerChip" and "PerCore" similar to the field "PerPkg" which is
used to specify perpkg events.

The "PerCore" and "PerChip" specifies whether its core or chip events.
Based on which we can decide which runtime parameter user want to
access. Now character  '?' can refers different parameter based on user
requirement.

Initially, every time we want to add new terms like chip, core, thread
etc, we need to create corrsponding fields in pmu_events and event
struct.
This patchset adds an enum called 'aggr_mode_class' which store all these
aggregation like perchip/percore. It also adds new field 'AggregationMode'
to capture these terms.
Now, if user wants to add any new term, they just need to add it in
the enum defined.

This patchset also adds  changes of adding new structure
called 'json_event' inside jevents.h
file to improve the callback prototype inside jevent files.
Initially, whenever user want to add new field, they need to update
in all function callback which makes it more and more complex with
increased number of parmeters.
With this change, we just need to add it in new structure 'json_event'.
link to the patch: https://lkml.org/lkml/2020/8/25/217

Changelog:
v5 -> v6
- Made changes to improve callback prototype inside jevent file
  by adding new struture 'json_event' as suggested by Andi Kleen.

- Added Reviewd-by tag from Andi Kleen

- Made changes suggested by Jiri Olsa and John garry includes:
  - Removing jevents.h file.
  - Some nits like freeing je->event mem, adding typedef for func
    and not allocating mem for json_event structure.
  - Added free for each field in json_event as suggested by
    John Garry.
  - In real_event function, rather then returning fixed structure
    pointer, used strcpy to copy event to je->event field.

- Also merge some changes in this patch set from the patch-
  https://lkml.org/lkml/2020/8/21/222. except nest event
  change which was using capability of defining metric using other metric.

- Make power side changes as per new struture added.

Link to the previous version patchset: https://lkml.org/lkml/2020/8/16/50

Kajol Jain (5):
  perf/jevents: Remove jevents.h file
  perf/jevents: Add new structure to pass json fields.
  perf jevents: Add support for parsing perchip/percore events
  perf/tools: Pass pmu_event structure as a parameter for
    arch_get_runtimeparam
  perf/tools/pmu_events/powerpc: Add hv_24x7 core level metric events

 tools/perf/arch/powerpc/util/header.c         |   7 +-
 .../arch/powerpc/power9/nest_metrics.json     |  35 ++-
 tools/perf/pmu-events/jevents.c               | 247 +++++++++---------
 tools/perf/pmu-events/jevents.h               |  23 --
 tools/perf/pmu-events/pmu-events.h            |   6 +
 tools/perf/util/metricgroup.c                 |   5 +-
 tools/perf/util/metricgroup.h                 |   3 +-
 7 files changed, 166 insertions(+), 160 deletions(-)
 delete mode 100644 tools/perf/pmu-events/jevents.h

-- 
2.26.2

[PATCH v6 1/5] perf/jevents: Remove jevents.h file

[Kajol Jain]

This patch removes jevents.h file and add its data inside
jevents.c as this file is only included there.

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 tools/perf/pmu-events/jevents.c |  9 ++++++++-
 tools/perf/pmu-events/jevents.h | 23 -----------------------
 2 files changed, 8 insertions(+), 24 deletions(-)
 delete mode 100644 tools/perf/pmu-events/jevents.h

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index fa86c5f997cc..1c55cc754b5a 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -48,11 +48,18 @@
 #include <linux/list.h>
 #include "jsmn.h"
 #include "json.h"
-#include "jevents.h"
 
 int verbose;
 char *prog;
 
+#ifndef min
+#define min(x, y) ({				\
+	typeof(x) _min1 = (x);			\
+	typeof(y) _min2 = (y);			\
+	(void)(&_min1 == &_min2);		\
+	_min1 < _min2 ? _min1 : _min2; })
+#endif
+
 int eprintf(int level, int var, const char *fmt, ...)
 {
 
diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h
deleted file mode 100644
index 2afc8304529e..000000000000
--- a/tools/perf/pmu-events/jevents.h
+++ /dev/null
@@ -1,23 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef JEVENTS_H
-#define JEVENTS_H 1
-
-int json_events(const char *fn,
-		int (*func)(void *data, char *name, char *event, char *desc,
-				char *long_desc,
-				char *pmu,
-				char *unit, char *perpkg, char *metric_expr,
-				char *metric_name, char *metric_group,
-				char *deprecated, char *metric_constraint),
-		void *data);
-char *get_cpu_str(void);
-
-#ifndef min
-#define min(x, y) ({                            \
-	typeof(x) _min1 = (x);                  \
-	typeof(y) _min2 = (y);                  \
-	(void) (&_min1 == &_min2);              \
-	_min1 < _min2 ? _min1 : _min2; })
-#endif
-
-#endif
-- 
2.26.2

[PATCH v6 2/5] perf/jevents: Add new structure to pass json fields.

[Kajol Jain]

This patch adds new structure called 'json_event' inside jevents.h
file to improve the callback prototype inside jevent files.
Initially, whenever user want to add new field, they need to update
in all function callback which make it more and more complex with
increased number of parmeters.
With this change, we just need to add it in new structure 'json_event'.

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Reviewed-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/pmu-events/jevents.c | 222 +++++++++++++++-----------------
 1 file changed, 105 insertions(+), 117 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 1c55cc754b5a..b205cd904a4f 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -52,6 +52,21 @@
 int verbose;
 char *prog;
 
+struct json_event {
+	char *name;
+	char *event;
+	char *desc;
+	char *long_desc;
+	char *pmu;
+	char *unit;
+	char *perpkg;
+	char *metric_expr;
+	char *metric_name;
+	char *metric_group;
+	char *deprecated;
+	char *metric_constraint;
+};
+
 #ifndef min
 #define min(x, y) ({				\
 	typeof(x) _min1 = (x);			\
@@ -60,6 +75,8 @@ char *prog;
 	_min1 < _min2 ? _min1 : _min2; })
 #endif
 
+typedef int (*func)(void *data, struct json_event *je);
+
 int eprintf(int level, int var, const char *fmt, ...)
 {
 
@@ -325,12 +342,7 @@ static void print_events_table_prefix(FILE *fp, const char *tblname)
 	close_table = 1;
 }
 
-static int print_events_table_entry(void *data, char *name, char *event,
-				    char *desc, char *long_desc,
-				    char *pmu, char *unit, char *perpkg,
-				    char *metric_expr,
-				    char *metric_name, char *metric_group,
-				    char *deprecated, char *metric_constraint)
+static int print_events_table_entry(void *data, struct json_event *je)
 {
 	struct perf_entry_data *pd = data;
 	FILE *outfp = pd->outfp;
@@ -342,30 +354,30 @@ static int print_events_table_entry(void *data, char *name, char *event,
 	 */
 	fprintf(outfp, "{\n");
 
-	if (name)
-		fprintf(outfp, "\t.name = \"%s\",\n", name);
-	if (event)
-		fprintf(outfp, "\t.event = \"%s\",\n", event);
-	fprintf(outfp, "\t.desc = \"%s\",\n", desc);
+	if (je->name)
+		fprintf(outfp, "\t.name = \"%s\",\n", je->name);
+	if (je->event)
+		fprintf(outfp, "\t.event = \"%s\",\n", je->event);
+	fprintf(outfp, "\t.desc = \"%s\",\n", je->desc);
 	fprintf(outfp, "\t.topic = \"%s\",\n", topic);
-	if (long_desc && long_desc[0])
-		fprintf(outfp, "\t.long_desc = \"%s\",\n", long_desc);
-	if (pmu)
-		fprintf(outfp, "\t.pmu = \"%s\",\n", pmu);
-	if (unit)
-		fprintf(outfp, "\t.unit = \"%s\",\n", unit);
-	if (perpkg)
-		fprintf(outfp, "\t.perpkg = \"%s\",\n", perpkg);
-	if (metric_expr)
-		fprintf(outfp, "\t.metric_expr = \"%s\",\n", metric_expr);
-	if (metric_name)
-		fprintf(outfp, "\t.metric_name = \"%s\",\n", metric_name);
-	if (metric_group)
-		fprintf(outfp, "\t.metric_group = \"%s\",\n", metric_group);
-	if (deprecated)
-		fprintf(outfp, "\t.deprecated = \"%s\",\n", deprecated);
-	if (metric_constraint)
-		fprintf(outfp, "\t.metric_constraint = \"%s\",\n", metric_constraint);
+	if (je->long_desc && je->long_desc[0])
+		fprintf(outfp, "\t.long_desc = \"%s\",\n", je->long_desc);
+	if (je->pmu)
+		fprintf(outfp, "\t.pmu = \"%s\",\n", je->pmu);
+	if (je->unit)
+		fprintf(outfp, "\t.unit = \"%s\",\n", je->unit);
+	if (je->perpkg)
+		fprintf(outfp, "\t.perpkg = \"%s\",\n", je->perpkg);
+	if (je->metric_expr)
+		fprintf(outfp, "\t.metric_expr = \"%s\",\n", je->metric_expr);
+	if (je->metric_name)
+		fprintf(outfp, "\t.metric_name = \"%s\",\n", je->metric_name);
+	if (je->metric_group)
+		fprintf(outfp, "\t.metric_group = \"%s\",\n", je->metric_group);
+	if (je->deprecated)
+		fprintf(outfp, "\t.deprecated = \"%s\",\n", je->deprecated);
+	if (je->metric_constraint)
+		fprintf(outfp, "\t.metric_constraint = \"%s\",\n", je->metric_constraint);
 	fprintf(outfp, "},\n");
 
 	return 0;
@@ -387,17 +399,17 @@ struct event_struct {
 	char *metric_constraint;
 };
 
-#define ADD_EVENT_FIELD(field) do { if (field) {		\
-	es->field = strdup(field);				\
+#define ADD_EVENT_FIELD(field) do { if (je->field) {		\
+	es->field = strdup(je->field);				\
 	if (!es->field)						\
 		goto out_free;					\
 } } while (0)
 
 #define FREE_EVENT_FIELD(field) free(es->field)
 
-#define TRY_FIXUP_FIELD(field) do { if (es->field && !*field) {\
-	*field = strdup(es->field);				\
-	if (!*field)						\
+#define TRY_FIXUP_FIELD(field) do { if (es->field && !je->field) {\
+	je->field = strdup(es->field);				\
+	if (!je->field)						\
 		return -ENOMEM;					\
 } } while (0)
 
@@ -428,11 +440,7 @@ static void free_arch_std_events(void)
 	}
 }
 
-static int save_arch_std_events(void *data, char *name, char *event,
-				char *desc, char *long_desc, char *pmu,
-				char *unit, char *perpkg, char *metric_expr,
-				char *metric_name, char *metric_group,
-				char *deprecated, char *metric_constraint)
+static int save_arch_std_events(void *data, struct json_event *je)
 {
 	struct event_struct *es;
 
@@ -486,17 +494,16 @@ static char *real_event(const char *name, char *event)
 		return NULL;
 
 	for (i = 0; fixed[i].name; i++)
-		if (!strcasecmp(name, fixed[i].name))
-			return (char *)fixed[i].event;
+		if (!strcasecmp(name, fixed[i].name)) {
+			strcpy(event, fixed[i].event);
+			return event;
+		}
 	return event;
 }
 
 static int
-try_fixup(const char *fn, char *arch_std, char **event, char **desc,
-	  char **name, char **long_desc, char **pmu, char **filter,
-	  char **perpkg, char **unit, char **metric_expr, char **metric_name,
-	  char **metric_group, unsigned long long eventcode,
-	  char **deprecated, char **metric_constraint)
+try_fixup(const char *fn, char *arch_std, unsigned long long eventcode,
+	  struct json_event *je)
 {
 	/* try to find matching event from arch standard values */
 	struct event_struct *es;
@@ -505,8 +512,8 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc,
 		if (!strcmp(arch_std, es->name)) {
 			if (!eventcode && es->event) {
 				/* allow EventCode to be overridden */
-				free(*event);
-				*event = NULL;
+				free(je->event);
+				je->event = NULL;
 			}
 			FOR_ALL_EVENT_STRUCT_FIELDS(TRY_FIXUP_FIELD);
 			return 0;
@@ -520,13 +527,8 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc,
 
 /* Call func with each event in the json file */
 int json_events(const char *fn,
-	  int (*func)(void *data, char *name, char *event, char *desc,
-		      char *long_desc,
-		      char *pmu, char *unit, char *perpkg,
-		      char *metric_expr,
-		      char *metric_name, char *metric_group,
-		      char *deprecated, char *metric_constraint),
-	  void *data)
+		int (*func)(void *data, struct json_event *je),
+			void *data)
 {
 	int err;
 	size_t size;
@@ -544,18 +546,9 @@ int json_events(const char *fn,
 	EXPECT(tokens->type == JSMN_ARRAY, tokens, "expected top level array");
 	tok = tokens + 1;
 	for (i = 0; i < tokens->size; i++) {
-		char *event = NULL, *desc = NULL, *name = NULL;
-		char *long_desc = NULL;
 		char *extra_desc = NULL;
-		char *pmu = NULL;
 		char *filter = NULL;
-		char *perpkg = NULL;
-		char *unit = NULL;
-		char *metric_expr = NULL;
-		char *metric_name = NULL;
-		char *metric_group = NULL;
-		char *deprecated = NULL;
-		char *metric_constraint = NULL;
+		struct json_event je = {};
 		char *arch_std = NULL;
 		unsigned long long eventcode = 0;
 		struct msrmap *msr = NULL;
@@ -577,7 +570,7 @@ int json_events(const char *fn,
 			       "Expected string value");
 
 			nz = !json_streq(map, val, "0");
-			if (match_field(map, field, nz, &event, val)) {
+			if (match_field(map, field, nz, &je.event, val)) {
 				/* ok */
 			} else if (json_streq(map, field, "EventCode")) {
 				char *code = NULL;
@@ -590,14 +583,14 @@ int json_events(const char *fn,
 				eventcode |= strtoul(code, NULL, 0) << 21;
 				free(code);
 			} else if (json_streq(map, field, "EventName")) {
-				addfield(map, &name, "", "", val);
+				addfield(map, &je.name, "", "", val);
 			} else if (json_streq(map, field, "BriefDescription")) {
-				addfield(map, &desc, "", "", val);
-				fixdesc(desc);
+				addfield(map, &je.desc, "", "", val);
+				fixdesc(je.desc);
 			} else if (json_streq(map, field,
 					     "PublicDescription")) {
-				addfield(map, &long_desc, "", "", val);
-				fixdesc(long_desc);
+				addfield(map, &je.long_desc, "", "", val);
+				fixdesc(je.long_desc);
 			} else if (json_streq(map, field, "PEBS") && nz) {
 				precise = val;
 			} else if (json_streq(map, field, "MSRIndex") && nz) {
@@ -617,34 +610,34 @@ int json_events(const char *fn,
 
 				ppmu = field_to_perf(unit_to_pmu, map, val);
 				if (ppmu) {
-					pmu = strdup(ppmu);
+					je.pmu = strdup(ppmu);
 				} else {
-					if (!pmu)
-						pmu = strdup("uncore_");
-					addfield(map, &pmu, "", "", val);
-					for (s = pmu; *s; s++)
+					if (!je.pmu)
+						je.pmu = strdup("uncore_");
+					addfield(map, &je.pmu, "", "", val);
+					for (s = je.pmu; *s; s++)
 						*s = tolower(*s);
 				}
-				addfield(map, &desc, ". ", "Unit: ", NULL);
-				addfield(map, &desc, "", pmu, NULL);
-				addfield(map, &desc, "", " ", NULL);
+				addfield(map, &je.desc, ". ", "Unit: ", NULL);
+				addfield(map, &je.desc, "", je.pmu, NULL);
+				addfield(map, &je.desc, "", " ", NULL);
 			} else if (json_streq(map, field, "Filter")) {
 				addfield(map, &filter, "", "", val);
 			} else if (json_streq(map, field, "ScaleUnit")) {
-				addfield(map, &unit, "", "", val);
+				addfield(map, &je.unit, "", "", val);
 			} else if (json_streq(map, field, "PerPkg")) {
-				addfield(map, &perpkg, "", "", val);
+				addfield(map, &je.perpkg, "", "", val);
 			} else if (json_streq(map, field, "Deprecated")) {
-				addfield(map, &deprecated, "", "", val);
+				addfield(map, &je.deprecated, "", "", val);
 			} else if (json_streq(map, field, "MetricName")) {
-				addfield(map, &metric_name, "", "", val);
+				addfield(map, &je.metric_name, "", "", val);
 			} else if (json_streq(map, field, "MetricGroup")) {
-				addfield(map, &metric_group, "", "", val);
+				addfield(map, &je.metric_group, "", "", val);
 			} else if (json_streq(map, field, "MetricConstraint")) {
-				addfield(map, &metric_constraint, "", "", val);
+				addfield(map, &je.metric_constraint, "", "", val);
 			} else if (json_streq(map, field, "MetricExpr")) {
-				addfield(map, &metric_expr, "", "", val);
-				for (s = metric_expr; *s; s++)
+				addfield(map, &je.metric_expr, "", "", val);
+				for (s = je.metric_expr; *s; s++)
 					*s = tolower(*s);
 			} else if (json_streq(map, field, "ArchStdEvent")) {
 				addfield(map, &arch_std, "", "", val);
@@ -653,7 +646,7 @@ int json_events(const char *fn,
 			}
 			/* ignore unknown fields */
 		}
-		if (precise && desc && !strstr(desc, "(Precise Event)")) {
+		if (precise && je.desc && !strstr(je.desc, "(Precise Event)")) {
 			if (json_streq(map, precise, "2"))
 				addfield(map, &extra_desc, " ",
 						"(Must be precise)", NULL);
@@ -662,49 +655,44 @@ int json_events(const char *fn,
 						"(Precise event)", NULL);
 		}
 		snprintf(buf, sizeof buf, "event=%#llx", eventcode);
-		addfield(map, &event, ",", buf, NULL);
-		if (desc && extra_desc)
-			addfield(map, &desc, " ", extra_desc, NULL);
-		if (long_desc && extra_desc)
-			addfield(map, &long_desc, " ", extra_desc, NULL);
+		addfield(map, &je.event, ",", buf, NULL);
+		if (je.desc && extra_desc)
+			addfield(map, &je.desc, " ", extra_desc, NULL);
+		if (je.long_desc && extra_desc)
+			addfield(map, &je.long_desc, " ", extra_desc, NULL);
 		if (filter)
-			addfield(map, &event, ",", filter, NULL);
+			addfield(map, &je.event, ",", filter, NULL);
 		if (msr != NULL)
-			addfield(map, &event, ",", msr->pname, msrval);
-		if (name)
-			fixname(name);
+			addfield(map, &je.event, ",", msr->pname, msrval);
+		if (je.name)
+			fixname(je.name);
 
 		if (arch_std) {
 			/*
 			 * An arch standard event is referenced, so try to
 			 * fixup any unassigned values.
 			 */
-			err = try_fixup(fn, arch_std, &event, &desc, &name,
-					&long_desc, &pmu, &filter, &perpkg,
-					&unit, &metric_expr, &metric_name,
-					&metric_group, eventcode,
-					&deprecated, &metric_constraint);
+			err = try_fixup(fn, arch_std, eventcode, &je);
 			if (err)
 				goto free_strings;
 		}
-		err = func(data, name, real_event(name, event), desc, long_desc,
-			   pmu, unit, perpkg, metric_expr, metric_name,
-			   metric_group, deprecated, metric_constraint);
+		je.event = real_event(je.name, je.event);
+		err = func(data, &je);
 free_strings:
-		free(event);
-		free(desc);
-		free(name);
-		free(long_desc);
+		free(je.event);
+		free(je.desc);
+		free(je.name);
+		free(je.long_desc);
 		free(extra_desc);
-		free(pmu);
+		free(je.pmu);
 		free(filter);
-		free(perpkg);
-		free(deprecated);
-		free(unit);
-		free(metric_expr);
-		free(metric_name);
-		free(metric_group);
-		free(metric_constraint);
+		free(je.perpkg);
+		free(je.deprecated);
+		free(je.unit);
+		free(je.metric_expr);
+		free(je.metric_name);
+		free(je.metric_group);
+		free(je.metric_constraint);
 		free(arch_std);
 
 		if (err)
-- 
2.26.2

[PATCH v6 3/5] perf jevents: Add support for parsing perchip/percore events

[Kajol Jain]

Initially, every time we want to add new terms like chip, core thread etc,
we need to create corrsponding fields in pmu_events and event struct.
This patch adds an enum called 'aggr_mode_class' which store all these
aggregation like perchip/percore. It also adds new field 'aggr_mode'
to capture these terms.
Now, if user wants to add any new term, they just need to add it in
the enum defined.

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 tools/perf/pmu-events/jevents.c    | 16 ++++++++++++++++
 tools/perf/pmu-events/pmu-events.h |  6 ++++++
 2 files changed, 22 insertions(+)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index b205cd904a4f..f4ad2d403533 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -48,6 +48,7 @@
 #include <linux/list.h>
 #include "jsmn.h"
 #include "json.h"
+#include "pmu-events.h"
 
 int verbose;
 char *prog;
@@ -60,6 +61,7 @@ struct json_event {
 	char *pmu;
 	char *unit;
 	char *perpkg;
+	char *aggr_mode;
 	char *metric_expr;
 	char *metric_name;
 	char *metric_group;
@@ -74,6 +76,14 @@ struct json_event {
 	(void)(&_min1 == &_min2);		\
 	_min1 < _min2 ? _min1 : _min2; })
 #endif
+enum aggr_mode_class convert(const char *aggr_mode)
+{
+	if (!strcmp(aggr_mode, "PerCore"))
+		return PerCore;
+	else if (!strcmp(aggr_mode, "PerChip"))
+		return PerChip;
+	return -1;
+}
 
 typedef int (*func)(void *data, struct json_event *je);
 
@@ -368,6 +378,8 @@ static int print_events_table_entry(void *data, struct json_event *je)
 		fprintf(outfp, "\t.unit = \"%s\",\n", je->unit);
 	if (je->perpkg)
 		fprintf(outfp, "\t.perpkg = \"%s\",\n", je->perpkg);
+	if (je->aggr_mode)
+		fprintf(outfp, "\t.aggr_mode = \"%d\",\n", convert(je->aggr_mode));
 	if (je->metric_expr)
 		fprintf(outfp, "\t.metric_expr = \"%s\",\n", je->metric_expr);
 	if (je->metric_name)
@@ -392,6 +404,7 @@ struct event_struct {
 	char *pmu;
 	char *unit;
 	char *perpkg;
+	char *aggr_mode;
 	char *metric_expr;
 	char *metric_name;
 	char *metric_group;
@@ -421,6 +434,7 @@ struct event_struct {
 	op(pmu);						\
 	op(unit);						\
 	op(perpkg);						\
+	op(aggr_mode);						\
 	op(metric_expr);					\
 	op(metric_name);					\
 	op(metric_group);					\
@@ -627,6 +641,8 @@ int json_events(const char *fn,
 				addfield(map, &je.unit, "", "", val);
 			} else if (json_streq(map, field, "PerPkg")) {
 				addfield(map, &je.perpkg, "", "", val);
+			} else if (json_streq(map, field, "AggregationMode")) {
+				addfield(map, &je.aggr_mode, "", "", val);
 			} else if (json_streq(map, field, "Deprecated")) {
 				addfield(map, &je.deprecated, "", "", val);
 			} else if (json_streq(map, field, "MetricName")) {
diff --git a/tools/perf/pmu-events/pmu-events.h b/tools/perf/pmu-events/pmu-events.h
index c8f306b572f4..7da1a3743b77 100644
--- a/tools/perf/pmu-events/pmu-events.h
+++ b/tools/perf/pmu-events/pmu-events.h
@@ -2,6 +2,11 @@
 #ifndef PMU_EVENTS_H
 #define PMU_EVENTS_H
 
+enum aggr_mode_class {
+	PerChip = 1,
+	PerCore
+};
+
 /*
  * Describe each PMU event. Each CPU has a table of PMU events.
  */
@@ -14,6 +19,7 @@ struct pmu_event {
 	const char *pmu;
 	const char *unit;
 	const char *perpkg;
+	const char *aggr_mode;
 	const char *metric_expr;
 	const char *metric_name;
 	const char *metric_group;
-- 
2.26.2

[PATCH v6 4/5] perf/tools: Pass pmu_event structure as a parameter for arch_get_runtimeparam

[Kajol Jain]

This patch adds passing of  pmu_event as a parameter in function
'arch_get_runtimeparam' which can be used to get details like
if the event is percore/perchip.

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Acked-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/powerpc/util/header.c | 7 +++++--
 tools/perf/util/metricgroup.c         | 5 ++---
 tools/perf/util/metricgroup.h         | 3 ++-
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/tools/perf/arch/powerpc/util/header.c b/tools/perf/arch/powerpc/util/header.c
index 1a950171a66f..58b2d610aadb 100644
--- a/tools/perf/arch/powerpc/util/header.c
+++ b/tools/perf/arch/powerpc/util/header.c
@@ -40,8 +40,11 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
 	return bufp;
 }
 
-int arch_get_runtimeparam(void)
+int arch_get_runtimeparam(struct pmu_event *pe)
 {
 	int count;
-	return sysfs__read_int("/devices/hv_24x7/interface/sockets", &count) < 0 ? 1 : count;
+	char path[PATH_MAX] = "/devices/hv_24x7/interface/";
+
+	atoi(pe->aggr_mode) == PerChip ? strcat(path, "sockets") : strcat(path, "coresperchip");
+	return sysfs__read_int(path, &count) < 0 ? 1 : count;
 }
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 8831b964288f..c387aa1615ba 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -15,7 +15,6 @@
 #include "rblist.h"
 #include <string.h>
 #include <errno.h>
-#include "pmu-events/pmu-events.h"
 #include "strlist.h"
 #include <assert.h>
 #include <linux/ctype.h>
@@ -634,7 +633,7 @@ static bool metricgroup__has_constraint(struct pmu_event *pe)
 	return false;
 }
 
-int __weak arch_get_runtimeparam(void)
+int __weak arch_get_runtimeparam(struct pmu_event *pe __maybe_unused)
 {
 	return 1;
 }
@@ -902,7 +901,7 @@ static int add_metric(struct list_head *metric_list,
 	} else {
 		int j, count;
 
-		count = arch_get_runtimeparam();
+		count = arch_get_runtimeparam(pe);
 
 		/* This loop is added to create multiple
 		 * events depend on count value and add
diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
index 62623a39cbec..491a5d78252d 100644
--- a/tools/perf/util/metricgroup.h
+++ b/tools/perf/util/metricgroup.h
@@ -5,6 +5,7 @@
 #include <linux/list.h>
 #include <linux/rbtree.h>
 #include <stdbool.h>
+#include "pmu-events/pmu-events.h"
 
 struct evsel;
 struct evlist;
@@ -52,6 +53,6 @@ int metricgroup__parse_groups_test(struct evlist *evlist,
 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);
+int arch_get_runtimeparam(struct pmu_event *pe __maybe_unused);
 void metricgroup__rblist_exit(struct rblist *metric_events);
 #endif
-- 
2.26.2

[PATCH v6 5/5] perf/tools/pmu_events/powerpc: Add hv_24x7 core level metric events

[Kajol Jain]

This patch adds hv_24x7 core level events in nest_metric.json file
and also add PerChip/PerCore field in metric events.

Result:

power9 platform:

command:# ./perf stat --metric-only -M PowerBUS_Frequency -C 0 -I 1000
     1.000070601                        1.9                        2.0
     2.000253881                        2.0                        1.9
     3.000364810                        2.0                        2.0

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Acked-by: Ian Rogers <irogers@google.com>
---
 .../arch/powerpc/power9/nest_metrics.json     | 35 ++++++++++++-------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json b/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
index 8383a37647ad..7a5d1bf543f8 100644
--- a/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
+++ b/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
@@ -1,37 +1,46 @@
 [
     {
-        "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_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",
+	"AggregationMode": "PerChip"
     },
     {
 	"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"
+	"MetricName": "Memory_WR_BW_Chip",
+	"MetricGroup": "Memory_BW",
+	"ScaleUnit": "1.6e-2MB",
+	"AggregationMode": "PerChip"
     },
     {
 	"MetricExpr": "(hv_24x7@PM_PB_CYC\\,chip\\=?@ )",
-        "MetricName": "PowerBUS_Frequency",
-        "ScaleUnit": "2.5e-7GHz"
+	"MetricName": "PowerBUS_Frequency",
+	"ScaleUnit": "2.5e-7GHz",
+	"AggregationMode": "PerChip"
+    },
+    {
+	"MetricExpr": "(hv_24x7@CPM_CS_32MHZ_CYC\\,domain\\=3\\,core\\=?@ )",
+	"MetricName": "CPM_CS_32MHZ_CYC",
+	"ScaleUnit": "1MHz",
+	"AggregationMode": "PerCore"
     },
     {
 	"MetricExpr" : "nest_mcs01_imc@PM_MCS01_128B_RD_DISP_PORT01@ + nest_mcs01_imc@PM_MCS01_128B_RD_DISP_PORT23@",
 	"MetricName" : "mcs01-read",
-	"MetricGroup" : "memory_bw",
+	"MetricGroup" : "memory-bandwidth",
 	"ScaleUnit": "6.1e-5MB"
     },
     {
 	"MetricExpr" : "nest_mcs23_imc@PM_MCS23_128B_RD_DISP_PORT01@ + nest_mcs23_imc@PM_MCS23_128B_RD_DISP_PORT23@",
 	"MetricName" : "mcs23-read",
-	"MetricGroup" : "memory_bw",
+	"MetricGroup" : "memory-bandwidth",
 	"ScaleUnit": "6.1e-5MB"
     },
     {
 	"MetricExpr" : "nest_mcs01_imc@PM_MCS01_128B_WR_DISP_PORT01@ + nest_mcs01_imc@PM_MCS01_128B_WR_DISP_PORT23@",
 	"MetricName" : "mcs01-write",
-	"MetricGroup" : "memory_bw",
+	"MetricGroup" : "memory-bandwidth",
 	"ScaleUnit": "6.1e-5MB"
     },
     {
@@ -48,7 +57,7 @@
     {
 	"MetricExpr" : "(nest_mcs01_imc@PM_MCS01_128B_RD_DISP_PORT01@ + nest_mcs01_imc@PM_MCS01_128B_RD_DISP_PORT23@ + nest_mcs23_imc@PM_MCS23_128B_RD_DISP_PORT01@ + nest_mcs23_imc@PM_MCS23_128B_RD_DISP_PORT23@ + nest_mcs01_imc@PM_MCS01_128B_WR_DISP_PORT01@ + nest_mcs01_imc@PM_MCS01_128B_WR_DISP_PORT23@ + nest_mcs23_imc@PM_MCS23_128B_WR_DISP_PORT01@ + nest_mcs23_imc@PM_MCS23_128B_WR_DISP_PORT23@)",
 	"MetricName" : "Memory-bandwidth-MCS",
-	"MetricGroup" : "memory_bw",
+	"MetricGroup" : "memory-bandwidth",
 	"ScaleUnit": "6.1e-5MB"
     }
 ]
-- 
2.26.2

Re: [PATCH v6 1/5] perf/jevents: Remove jevents.h file

[Jiri Olsa]

On Thu, Aug 27, 2020 at 06:39:54PM +0530, Kajol Jain wrote:
> This patch removes jevents.h file and add its data inside
> jevents.c as this file is only included there.
> 
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
>  tools/perf/pmu-events/jevents.c |  9 ++++++++-
>  tools/perf/pmu-events/jevents.h | 23 -----------------------
>  2 files changed, 8 insertions(+), 24 deletions(-)
>  delete mode 100644 tools/perf/pmu-events/jevents.h
> 
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index fa86c5f997cc..1c55cc754b5a 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -48,11 +48,18 @@
>  #include <linux/list.h>
>  #include "jsmn.h"
>  #include "json.h"
> -#include "jevents.h"
>  
>  int verbose;
>  char *prog;
>  
> +#ifndef min
> +#define min(x, y) ({				\
> +	typeof(x) _min1 = (x);			\
> +	typeof(y) _min2 = (y);			\
> +	(void)(&_min1 == &_min2);		\
> +	_min1 < _min2 ? _min1 : _min2; })
> +#endif
> +
>  int eprintf(int level, int var, const char *fmt, ...)
>  {
>  
> diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h
> deleted file mode 100644
> index 2afc8304529e..000000000000
> --- a/tools/perf/pmu-events/jevents.h
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef JEVENTS_H
> -#define JEVENTS_H 1
> -
> -int json_events(const char *fn,
> -		int (*func)(void *data, char *name, char *event, char *desc,
> -				char *long_desc,
> -				char *pmu,
> -				char *unit, char *perpkg, char *metric_expr,
> -				char *metric_name, char *metric_group,
> -				char *deprecated, char *metric_constraint),
> -		void *data);
> -char *get_cpu_str(void);

I think you can also remove get_cpu_str from jevents.c

thanks,
jirka

Re: [PATCH v6 2/5] perf/jevents: Add new structure to pass json fields.

[Jiri Olsa]

On Thu, Aug 27, 2020 at 06:39:55PM +0530, Kajol Jain wrote:

SNIP

> -	if (!*field)						\
> +#define TRY_FIXUP_FIELD(field) do { if (es->field && !je->field) {\
> +	je->field = strdup(es->field);				\
> +	if (!je->field)						\
>  		return -ENOMEM;					\
>  } } while (0)
>  
> @@ -428,11 +440,7 @@ static void free_arch_std_events(void)
>  	}
>  }
>  
> -static int save_arch_std_events(void *data, char *name, char *event,
> -				char *desc, char *long_desc, char *pmu,
> -				char *unit, char *perpkg, char *metric_expr,
> -				char *metric_name, char *metric_group,
> -				char *deprecated, char *metric_constraint)
> +static int save_arch_std_events(void *data, struct json_event *je)
>  {
>  	struct event_struct *es;
>  
> @@ -486,17 +494,16 @@ static char *real_event(const char *name, char *event)
>  		return NULL;
>  
>  	for (i = 0; fixed[i].name; i++)
> -		if (!strcasecmp(name, fixed[i].name))
> -			return (char *)fixed[i].event;
> +		if (!strcasecmp(name, fixed[i].name)) {
> +			strcpy(event, fixed[i].event);

hum what's this strcpy for in here? we're just replacing separated
variables with struct members, why do you need to copy the event in
here?

thanks,
jirka

Re: [PATCH v6 3/5] perf jevents: Add support for parsing perchip/percore events

[Jiri Olsa]

On Thu, Aug 27, 2020 at 06:39:56PM +0530, Kajol Jain wrote:
> Initially, every time we want to add new terms like chip, core thread etc,
> we need to create corrsponding fields in pmu_events and event struct.
> This patch adds an enum called 'aggr_mode_class' which store all these
> aggregation like perchip/percore. It also adds new field 'aggr_mode'
> to capture these terms.
> Now, if user wants to add any new term, they just need to add it in
> the enum defined.
> 
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
>  tools/perf/pmu-events/jevents.c    | 16 ++++++++++++++++
>  tools/perf/pmu-events/pmu-events.h |  6 ++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index b205cd904a4f..f4ad2d403533 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -48,6 +48,7 @@
>  #include <linux/list.h>
>  #include "jsmn.h"
>  #include "json.h"
> +#include "pmu-events.h"
>  
>  int verbose;
>  char *prog;
> @@ -60,6 +61,7 @@ struct json_event {
>  	char *pmu;
>  	char *unit;
>  	char *perpkg;
> +	char *aggr_mode;
>  	char *metric_expr;
>  	char *metric_name;
>  	char *metric_group;
> @@ -74,6 +76,14 @@ struct json_event {
>  	(void)(&_min1 == &_min2);		\
>  	_min1 < _min2 ? _min1 : _min2; })
>  #endif

please add new line in here

> +enum aggr_mode_class convert(const char *aggr_mode)
> +{
> +	if (!strcmp(aggr_mode, "PerCore"))
> +		return PerCore;
> +	else if (!strcmp(aggr_mode, "PerChip"))
> +		return PerChip;
> +	return -1;

should we display some warning in here?

thanks,
jirka

Re: [PATCH v6 1/5] perf/jevents: Remove jevents.h file

[John Garry]

On 31/08/2020 09:43, Jiri Olsa wrote:
> On Thu, Aug 27, 2020 at 06:39:54PM +0530, Kajol Jain wrote:
>> This patch removes jevents.h file and add its data inside
>> jevents.c as this file is only included there.
>>
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>>   tools/perf/pmu-events/jevents.c |  9 ++++++++-
>>   tools/perf/pmu-events/jevents.h | 23 -----------------------
>>   2 files changed, 8 insertions(+), 24 deletions(-)
>>   delete mode 100644 tools/perf/pmu-events/jevents.h
>>
>> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
>> index fa86c5f997cc..1c55cc754b5a 100644
>> --- a/tools/perf/pmu-events/jevents.c
>> +++ b/tools/perf/pmu-events/jevents.c
>> @@ -48,11 +48,18 @@
>>   #include <linux/list.h>
>>   #include "jsmn.h"
>>   #include "json.h"
>> -#include "jevents.h"
>>   
>>   int verbose;
>>   char *prog;
>>   
>> +#ifndef min
>> +#define min(x, y) ({				\
>> +	typeof(x) _min1 = (x);			\
>> +	typeof(y) _min2 = (y);			\
>> +	(void)(&_min1 == &_min2);		\
>> +	_min1 < _min2 ? _min1 : _min2; })
>> +#endif

Wondering what is special about this definition of min that it's 
required? Compiled ok for me without it.

>> +
>>   int eprintf(int level, int var, const char *fmt, ...)
>>   {
>>   
>> diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h
>> deleted file mode 100644
>> index 2afc8304529e..000000000000
>> --- a/tools/perf/pmu-events/jevents.h
>> +++ /dev/null
>> @@ -1,23 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0 */
>> -#ifndef JEVENTS_H
>> -#define JEVENTS_H 1
>> -
>> -int json_events(const char *fn,
>> -		int (*func)(void *data, char *name, char *event, char *desc,
>> -				char *long_desc,
>> -				char *pmu,
>> -				char *unit, char *perpkg, char *metric_expr,
>> -				char *metric_name, char *metric_group,
>> -				char *deprecated, char *metric_constraint),
>> -		void *data);
>> -char *get_cpu_str(void);
> 
> I think you can also remove get_cpu_str from jevents.c
> 
> thanks,
> jirka
> 
> .
> 

Re: [PATCH v6 1/5] perf/jevents: Remove jevents.h file

[kajoljain]

On 8/31/20 2:13 PM, Jiri Olsa wrote:
> On Thu, Aug 27, 2020 at 06:39:54PM +0530, Kajol Jain wrote:
>> This patch removes jevents.h file and add its data inside
>> jevents.c as this file is only included there.
>>
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>>  tools/perf/pmu-events/jevents.c |  9 ++++++++-
>>  tools/perf/pmu-events/jevents.h | 23 -----------------------
>>  2 files changed, 8 insertions(+), 24 deletions(-)
>>  delete mode 100644 tools/perf/pmu-events/jevents.h
>>
>> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
>> index fa86c5f997cc..1c55cc754b5a 100644
>> --- a/tools/perf/pmu-events/jevents.c
>> +++ b/tools/perf/pmu-events/jevents.c
>> @@ -48,11 +48,18 @@
>>  #include <linux/list.h>
>>  #include "jsmn.h"
>>  #include "json.h"
>> -#include "jevents.h"
>>  
>>  int verbose;
>>  char *prog;
>>  
>> +#ifndef min
>> +#define min(x, y) ({				\
>> +	typeof(x) _min1 = (x);			\
>> +	typeof(y) _min2 = (y);			\
>> +	(void)(&_min1 == &_min2);		\
>> +	_min1 < _min2 ? _min1 : _min2; })
>> +#endif
>> +
>>  int eprintf(int level, int var, const char *fmt, ...)
>>  {
>>  
>> diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h
>> deleted file mode 100644
>> index 2afc8304529e..000000000000
>> --- a/tools/perf/pmu-events/jevents.h
>> +++ /dev/null
>> @@ -1,23 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0 */
>> -#ifndef JEVENTS_H
>> -#define JEVENTS_H 1
>> -
>> -int json_events(const char *fn,
>> -		int (*func)(void *data, char *name, char *event, char *desc,
>> -				char *long_desc,
>> -				char *pmu,
>> -				char *unit, char *perpkg, char *metric_expr,
>> -				char *metric_name, char *metric_group,
>> -				char *deprecated, char *metric_constraint),
>> -		void *data);
>> -char *get_cpu_str(void);
> 
> I think you can also remove get_cpu_str from jevents.c

Hi Jiri,
     Yes, I will check that part.

Thanks,
Kajol Jain
> 
> thanks,
> jirka
> 

Re: [PATCH v6 1/5] perf/jevents: Remove jevents.h file

[kajoljain]

On 8/31/20 2:31 PM, John Garry wrote:
> On 31/08/2020 09:43, Jiri Olsa wrote:
>> On Thu, Aug 27, 2020 at 06:39:54PM +0530, Kajol Jain wrote:
>>> This patch removes jevents.h file and add its data inside
>>> jevents.c as this file is only included there.
>>>
>>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>>> ---
>>>   tools/perf/pmu-events/jevents.c |  9 ++++++++-
>>>   tools/perf/pmu-events/jevents.h | 23 -----------------------
>>>   2 files changed, 8 insertions(+), 24 deletions(-)
>>>   delete mode 100644 tools/perf/pmu-events/jevents.h
>>>
>>> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
>>> index fa86c5f997cc..1c55cc754b5a 100644
>>> --- a/tools/perf/pmu-events/jevents.c
>>> +++ b/tools/perf/pmu-events/jevents.c
>>> @@ -48,11 +48,18 @@
>>>   #include <linux/list.h>
>>>   #include "jsmn.h"
>>>   #include "json.h"
>>> -#include "jevents.h"
>>>     int verbose;
>>>   char *prog;
>>>   +#ifndef min
>>> +#define min(x, y) ({                \
>>> +    typeof(x) _min1 = (x);            \
>>> +    typeof(y) _min2 = (y);            \
>>> +    (void)(&_min1 == &_min2);        \
>>> +    _min1 < _min2 ? _min1 : _min2; })
>>> +#endif
> 
> Wondering what is special about this definition of min that it's required? Compiled ok for me without it.

Hi John,
     You are right, for me also in power it compiled without any issues, but not sure if somewhere we have dependency,
that's why I didn't remove it. 

Thanks,
Kajol Jain
> 
>>> +
>>>   int eprintf(int level, int var, const char *fmt, ...)
>>>   {
>>>   diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h
>>> deleted file mode 100644
>>> index 2afc8304529e..000000000000
>>> --- a/tools/perf/pmu-events/jevents.h
>>> +++ /dev/null
>>> @@ -1,23 +0,0 @@
>>> -/* SPDX-License-Identifier: GPL-2.0 */
>>> -#ifndef JEVENTS_H
>>> -#define JEVENTS_H 1
>>> -
>>> -int json_events(const char *fn,
>>> -        int (*func)(void *data, char *name, char *event, char *desc,
>>> -                char *long_desc,
>>> -                char *pmu,
>>> -                char *unit, char *perpkg, char *metric_expr,
>>> -                char *metric_name, char *metric_group,
>>> -                char *deprecated, char *metric_constraint),
>>> -        void *data);
>>> -char *get_cpu_str(void);
>>
>> I think you can also remove get_cpu_str from jevents.c
>>
>> thanks,
>> jirka
>>
>> .
>>
> 

Re: [PATCH v6 2/5] perf/jevents: Add new structure to pass json fields.

[kajoljain]

On 8/31/20 2:13 PM, Jiri Olsa wrote:
> On Thu, Aug 27, 2020 at 06:39:55PM +0530, Kajol Jain wrote:
> 
> SNIP
> 
>> -	if (!*field)						\
>> +#define TRY_FIXUP_FIELD(field) do { if (es->field && !je->field) {\
>> +	je->field = strdup(es->field);				\
>> +	if (!je->field)						\
>>  		return -ENOMEM;					\
>>  } } while (0)
>>  
>> @@ -428,11 +440,7 @@ static void free_arch_std_events(void)
>>  	}
>>  }
>>  
>> -static int save_arch_std_events(void *data, char *name, char *event,
>> -				char *desc, char *long_desc, char *pmu,
>> -				char *unit, char *perpkg, char *metric_expr,
>> -				char *metric_name, char *metric_group,
>> -				char *deprecated, char *metric_constraint)
>> +static int save_arch_std_events(void *data, struct json_event *je)
>>  {
>>  	struct event_struct *es;
>>  
>> @@ -486,17 +494,16 @@ static char *real_event(const char *name, char *event)
>>  		return NULL;
>>  
>>  	for (i = 0; fixed[i].name; i++)
>> -		if (!strcasecmp(name, fixed[i].name))
>> -			return (char *)fixed[i].event;
>> +		if (!strcasecmp(name, fixed[i].name)) {
>> +			strcpy(event, fixed[i].event);
> 
> hum what's this strcpy for in here? we're just replacing separated
> variables with struct members, why do you need to copy the event in
> here?
> 

Hi Jiri,
    Actually, initially when events is not part of 'json_event' structure, we are not
assigning result of function `real_event` to event field. But as we are not passing
event filed separately in functions like 'save_arch_std_events', freeing event
memory was giving me issue as we are returning pointer from real_event function in some cases.
So, that's why I replace it to strcpy to make sure we free je.event memory. Or should I remove
free(je.event) part?

-		err = func(data, name, real_event(name, event), desc, long_desc,
-			   pmu, unit, perpkg, metric_expr, metric_name,
-			   metric_group, deprecated, metric_constraint);
+		je.event = real_event(je.name, je.event);

This is the part, I am referring, here we are assigning result of real_event into je.event field.

Thanks,
Kajol Jain
> thanks,
> jirka
> 

Re: [PATCH v6 3/5] perf jevents: Add support for parsing perchip/percore events

[kajoljain]

On 8/31/20 2:14 PM, Jiri Olsa wrote:
> On Thu, Aug 27, 2020 at 06:39:56PM +0530, Kajol Jain wrote:
>> Initially, every time we want to add new terms like chip, core thread etc,
>> we need to create corrsponding fields in pmu_events and event struct.
>> This patch adds an enum called 'aggr_mode_class' which store all these
>> aggregation like perchip/percore. It also adds new field 'aggr_mode'
>> to capture these terms.
>> Now, if user wants to add any new term, they just need to add it in
>> the enum defined.
>>
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>>  tools/perf/pmu-events/jevents.c    | 16 ++++++++++++++++
>>  tools/perf/pmu-events/pmu-events.h |  6 ++++++
>>  2 files changed, 22 insertions(+)
>>
>> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
>> index b205cd904a4f..f4ad2d403533 100644
>> --- a/tools/perf/pmu-events/jevents.c
>> +++ b/tools/perf/pmu-events/jevents.c
>> @@ -48,6 +48,7 @@
>>  #include <linux/list.h>
>>  #include "jsmn.h"
>>  #include "json.h"
>> +#include "pmu-events.h"
>>  
>>  int verbose;
>>  char *prog;
>> @@ -60,6 +61,7 @@ struct json_event {
>>  	char *pmu;
>>  	char *unit;
>>  	char *perpkg;
>> +	char *aggr_mode;
>>  	char *metric_expr;
>>  	char *metric_name;
>>  	char *metric_group;
>> @@ -74,6 +76,14 @@ struct json_event {
>>  	(void)(&_min1 == &_min2);		\
>>  	_min1 < _min2 ? _min1 : _min2; })
>>  #endif
> 
> please add new line in here
> 

yes, will add.

>> +enum aggr_mode_class convert(const char *aggr_mode)
>> +{
>> +	if (!strcmp(aggr_mode, "PerCore"))
>> +		return PerCore;
>> +	else if (!strcmp(aggr_mode, "PerChip"))
>> +		return PerChip;
>> +	return -1;
> 
> should we display some warning in here?

Sure can do that.

Thanks,
Kajol Jain
> 
> thanks,
> jirka
> 

Re: [PATCH v6 2/5] perf/jevents: Add new structure to pass json fields.

[Jiri Olsa]

On Tue, Sep 01, 2020 at 11:32:45AM +0530, kajoljain wrote:
> 
> 
> On 8/31/20 2:13 PM, Jiri Olsa wrote:
> > On Thu, Aug 27, 2020 at 06:39:55PM +0530, Kajol Jain wrote:
> > 
> > SNIP
> > 
> >> -	if (!*field)						\
> >> +#define TRY_FIXUP_FIELD(field) do { if (es->field && !je->field) {\
> >> +	je->field = strdup(es->field);				\
> >> +	if (!je->field)						\
> >>  		return -ENOMEM;					\
> >>  } } while (0)
> >>  
> >> @@ -428,11 +440,7 @@ static void free_arch_std_events(void)
> >>  	}
> >>  }
> >>  
> >> -static int save_arch_std_events(void *data, char *name, char *event,
> >> -				char *desc, char *long_desc, char *pmu,
> >> -				char *unit, char *perpkg, char *metric_expr,
> >> -				char *metric_name, char *metric_group,
> >> -				char *deprecated, char *metric_constraint)
> >> +static int save_arch_std_events(void *data, struct json_event *je)
> >>  {
> >>  	struct event_struct *es;
> >>  
> >> @@ -486,17 +494,16 @@ static char *real_event(const char *name, char *event)
> >>  		return NULL;
> >>  
> >>  	for (i = 0; fixed[i].name; i++)
> >> -		if (!strcasecmp(name, fixed[i].name))
> >> -			return (char *)fixed[i].event;
> >> +		if (!strcasecmp(name, fixed[i].name)) {
> >> +			strcpy(event, fixed[i].event);
> > 
> > hum what's this strcpy for in here? we're just replacing separated
> > variables with struct members, why do you need to copy the event in
> > here?
> > 
> 
> Hi Jiri,
>     Actually, initially when events is not part of 'json_event' structure, we are not
> assigning result of function `real_event` to event field. But as we are not passing
> event filed separately in functions like 'save_arch_std_events', freeing event
> memory was giving me issue as we are returning pointer from real_event function in some cases.
> So, that's why I replace it to strcpy to make sure we free je.event memory. Or should I remove
> free(je.event) part?
> 
> -		err = func(data, name, real_event(name, event), desc, long_desc,
> -			   pmu, unit, perpkg, metric_expr, metric_name,
> -			   metric_group, deprecated, metric_constraint);
> +		je.event = real_event(je.name, je.event);
> 
> This is the part, I am referring, here we are assigning result of real_event into je.event field.

hum, I dont't think you can strcpy like that in real_event,

but how about keeping the event variable on stack and freeing
just that one as the original code is doing.. like in change
below, it's diff against your patch

thanks,
jirka


---
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index b205cd904a4f..6abd2abf62fc 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -494,10 +494,8 @@ static char *real_event(const char *name, char *event)
 		return NULL;
 
 	for (i = 0; fixed[i].name; i++)
-		if (!strcasecmp(name, fixed[i].name)) {
-			strcpy(event, fixed[i].event);
+		if (!strcasecmp(name, fixed[i].name))
 			return event;
-		}
 	return event;
 }
 
@@ -546,6 +544,7 @@ int json_events(const char *fn,
 	EXPECT(tokens->type == JSMN_ARRAY, tokens, "expected top level array");
 	tok = tokens + 1;
 	for (i = 0; i < tokens->size; i++) {
+		char *event = NULL;
 		char *extra_desc = NULL;
 		char *filter = NULL;
 		struct json_event je = {};
@@ -570,7 +569,7 @@ int json_events(const char *fn,
 			       "Expected string value");
 
 			nz = !json_streq(map, val, "0");
-			if (match_field(map, field, nz, &je.event, val)) {
+			if (match_field(map, field, nz, &event, val)) {
 				/* ok */
 			} else if (json_streq(map, field, "EventCode")) {
 				char *code = NULL;
@@ -655,15 +654,15 @@ int json_events(const char *fn,
 						"(Precise event)", NULL);
 		}
 		snprintf(buf, sizeof buf, "event=%#llx", eventcode);
-		addfield(map, &je.event, ",", buf, NULL);
+		addfield(map, &event, ",", buf, NULL);
 		if (je.desc && extra_desc)
 			addfield(map, &je.desc, " ", extra_desc, NULL);
 		if (je.long_desc && extra_desc)
 			addfield(map, &je.long_desc, " ", extra_desc, NULL);
 		if (filter)
-			addfield(map, &je.event, ",", filter, NULL);
+			addfield(map, &event, ",", filter, NULL);
 		if (msr != NULL)
-			addfield(map, &je.event, ",", msr->pname, msrval);
+			addfield(map, &event, ",", msr->pname, msrval);
 		if (je.name)
 			fixname(je.name);
 
@@ -676,10 +675,10 @@ int json_events(const char *fn,
 			if (err)
 				goto free_strings;
 		}
-		je.event = real_event(je.name, je.event);
+		je.event = real_event(je.name, event);
 		err = func(data, &je);
 free_strings:
-		free(je.event);
+		free(event);
 		free(je.desc);
 		free(je.name);
 		free(je.long_desc);

Re: [PATCH v6 1/5] perf/jevents: Remove jevents.h file

[John Garry]

On 01/09/2020 06:56, kajoljain wrote:
>>>> +#define min(x, y) ({                \
>>>> +    typeof(x) _min1 = (x);            \
>>>> +    typeof(y) _min2 = (y);            \
>>>> +    (void)(&_min1 == &_min2);        \
>>>> +    _min1 < _min2 ? _min1 : _min2; })
>>>> +#endif
>> Wondering what is special about this definition of min that it's required? Compiled ok for me without it.
> Hi John,
>       You are right, for me also in power it compiled without any issues, but not sure if somewhere we have dependency,
> that's why I didn't remove it.

If it builds for x86, then that's main thing ;) But seriously, Arnaldo 
has lots of bots to test builds also.

BTW, I got this from your patchset:

pmu-events/jevents.c:98:29: warning: no previous prototype for 
‘get_cpu_str’ [-Wmissing-prototypes]
__attribute__((weak)) char *get_cpu_str(void)
                             ^~~~~~~~~~~
pmu-events/jevents.c:529:5: warning: no previous prototype for 
‘json_events’ [-Wmissing-prototypes]
int json_events(const char *fn,
     ^~~~~~~~~~~

But I think that you will remove this.

Finally, generated pmu-events.c looks ok for arm64, which I am 
interested in.

Thanks,
John