[PATCH v1 1/2] perf header: Support HYBRID_TOPOLOGY feature

[PATCH v1 1/2] perf header: Support HYBRID_TOPOLOGY feature

[Jin Yao]

It would be useful to let user know the hybrid topology.
Adding HYBRID_TOPOLOGY feature in header to indicate the
core cpus and the atom cpus.

With this patch,

For the perf.data generated on hybrid platform,
reports the hybrid cpu list.

  root@otcpl-adl-s-2:~# perf report --header-only -I
  ...
  # cpu_core cpu list : 0-15
  # cpu_atom cpu list : 16-23

For the perf.data generated on non-hybrid platform,
reports the message that HYBRID_TOPOLOGY is missing.

  root@kbl-ppc:~# perf report --header-only -I
  ...
  # missing features: TRACING_DATA BRANCH_STACK GROUP_DESC AUXTRACE STAT CLOCKID DIR_FORMAT COMPRESSED CLOCK_DATA HYBRID_TOPOLOGY

Note that: this patch is on tmp.perf/core.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/util/cputopo.c    | 80 +++++++++++++++++++++++++++++++
 tools/perf/util/cputopo.h    | 13 +++++
 tools/perf/util/env.c        |  6 +++
 tools/perf/util/env.h        |  7 +++
 tools/perf/util/header.c     | 92 ++++++++++++++++++++++++++++++++++++
 tools/perf/util/header.h     |  1 +
 tools/perf/util/pmu-hybrid.h | 11 +++++
 7 files changed, 210 insertions(+)

diff --git a/tools/perf/util/cputopo.c b/tools/perf/util/cputopo.c
index 1b52402a8923..04abf9344dd7 100644
--- a/tools/perf/util/cputopo.c
+++ b/tools/perf/util/cputopo.c
@@ -12,6 +12,7 @@
 #include "cpumap.h"
 #include "debug.h"
 #include "env.h"
+#include "pmu-hybrid.h"
 
 #define CORE_SIB_FMT \
 	"%s/devices/system/cpu/cpu%d/topology/core_siblings_list"
@@ -351,3 +352,82 @@ void numa_topology__delete(struct numa_topology *tp)
 
 	free(tp);
 }
+
+static int load_hybrid_node(struct hybrid_topology_node *node,
+			    struct perf_pmu *pmu)
+{
+	const char *sysfs;
+	char path[PATH_MAX];
+	char *buf = NULL, *p;
+	FILE *fp;
+	size_t len = 0;
+
+	node->pmu_name = strdup(pmu->name);
+	if (!node->pmu_name)
+		return -1;
+
+	sysfs = sysfs__mountpoint();
+	if (!sysfs)
+		goto err;
+
+	snprintf(path, PATH_MAX, CPUS_TEMPLATE_CPU, sysfs, pmu->name);
+	fp = fopen(path, "r");
+	if (!fp)
+		goto err;
+
+	if (getline(&buf, &len, fp) <= 0) {
+		fclose(fp);
+		goto err;
+	}
+
+	p = strchr(buf, '\n');
+	if (p)
+		*p = '\0';
+
+	fclose(fp);
+	node->cpus = buf;
+	return 0;
+
+err:
+	zfree(&node->pmu_name);
+	free(buf);
+	return -1;
+}
+
+struct hybrid_topology *hybrid_topology__new(void)
+{
+	struct perf_pmu *pmu;
+	struct hybrid_topology *tp = NULL;
+	u32 nr = 0, i = 0;
+
+	nr = perf_pmu__hybrid_pmu_num();
+	if (nr == 0)
+		return NULL;
+
+	tp = zalloc(sizeof(*tp) + sizeof(tp->nodes[0]) * nr);
+	if (!tp)
+		return NULL;
+
+	tp->nr = nr;
+	perf_pmu__for_each_hybrid_pmu(pmu) {
+		if (load_hybrid_node(&tp->nodes[i], pmu)) {
+			hybrid_topology__delete(tp);
+			return NULL;
+		}
+		i++;
+	}
+
+	return tp;
+}
+
+void hybrid_topology__delete(struct hybrid_topology *tp)
+{
+	u32 i;
+
+	for (i = 0; i < tp->nr; i++) {
+		zfree(&tp->nodes[i].pmu_name);
+		zfree(&tp->nodes[i].cpus);
+	}
+
+	free(tp);
+}
diff --git a/tools/perf/util/cputopo.h b/tools/perf/util/cputopo.h
index 6201c3790d86..d9af97177068 100644
--- a/tools/perf/util/cputopo.h
+++ b/tools/perf/util/cputopo.h
@@ -25,10 +25,23 @@ struct numa_topology {
 	struct numa_topology_node	nodes[];
 };
 
+struct hybrid_topology_node {
+	char		*pmu_name;
+	char		*cpus;
+};
+
+struct hybrid_topology {
+	u32				nr;
+	struct hybrid_topology_node	nodes[];
+};
+
 struct cpu_topology *cpu_topology__new(void);
 void cpu_topology__delete(struct cpu_topology *tp);
 
 struct numa_topology *numa_topology__new(void);
 void numa_topology__delete(struct numa_topology *tp);
 
+struct hybrid_topology *hybrid_topology__new(void);
+void hybrid_topology__delete(struct hybrid_topology *tp);
+
 #endif /* __PERF_CPUTOPO_H */
diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index 9130f6fad8d5..9e05eca324a1 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -202,6 +202,12 @@ void perf_env__exit(struct perf_env *env)
 	for (i = 0; i < env->nr_memory_nodes; i++)
 		zfree(&env->memory_nodes[i].set);
 	zfree(&env->memory_nodes);
+
+	for (i = 0; i < env->nr_hybrid_nodes; i++) {
+		perf_cpu_map__put(env->hybrid_nodes[i].map);
+		zfree(&env->hybrid_nodes[i].pmu_name);
+	}
+	zfree(&env->hybrid_nodes);
 }
 
 void perf_env__init(struct perf_env *env __maybe_unused)
diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
index ca249bf5e984..9ca7633787e1 100644
--- a/tools/perf/util/env.h
+++ b/tools/perf/util/env.h
@@ -37,6 +37,11 @@ struct memory_node {
 	unsigned long	*set;
 };
 
+struct hybrid_node {
+	char	*pmu_name;
+	struct perf_cpu_map	*map;
+};
+
 struct perf_env {
 	char			*hostname;
 	char			*os_release;
@@ -59,6 +64,7 @@ struct perf_env {
 	int			nr_pmu_mappings;
 	int			nr_groups;
 	int			nr_cpu_pmu_caps;
+	int			nr_hybrid_nodes;
 	char			*cmdline;
 	const char		**cmdline_argv;
 	char			*sibling_cores;
@@ -77,6 +83,7 @@ struct perf_env {
 	struct numa_node	*numa_nodes;
 	struct memory_node	*memory_nodes;
 	unsigned long long	 memory_bsize;
+	struct hybrid_node	*hybrid_nodes;
 #ifdef HAVE_LIBBPF_SUPPORT
 	/*
 	 * bpf_info_lock protects bpf rbtrees. This is needed because the
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index aa1e42518d37..dff89c0be79c 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -932,6 +932,40 @@ static int write_clock_data(struct feat_fd *ff,
 	return do_write(ff, data64, sizeof(*data64));
 }
 
+static int write_hybrid_topology(struct feat_fd *ff,
+				 struct evlist *evlist __maybe_unused)
+{
+	struct hybrid_topology *tp;
+	int ret;
+	u32 i;
+
+	tp = hybrid_topology__new();
+	if (!tp)
+		return -1;
+
+	ret = do_write(ff, &tp->nr, sizeof(u32));
+	if (ret < 0)
+		goto err;
+
+	for (i = 0; i < tp->nr; i++) {
+		struct hybrid_topology_node *n = &tp->nodes[i];
+
+		ret = do_write_string(ff, n->pmu_name);
+		if (ret < 0)
+			goto err;
+
+		ret = do_write_string(ff, n->cpus);
+		if (ret < 0)
+			goto err;
+	}
+
+	ret = 0;
+
+err:
+	hybrid_topology__delete(tp);
+	return ret;
+}
+
 static int write_dir_format(struct feat_fd *ff,
 			    struct evlist *evlist __maybe_unused)
 {
@@ -1623,6 +1657,19 @@ static void print_clock_data(struct feat_fd *ff, FILE *fp)
 		    clockid_name(clockid));
 }
 
+static void print_hybrid_topology(struct feat_fd *ff, FILE *fp)
+{
+	int i;
+	struct hybrid_node *n;
+
+	for (i = 0; i < ff->ph->env.nr_hybrid_nodes; i++) {
+		n = &ff->ph->env.hybrid_nodes[i];
+
+		fprintf(fp, "# %s cpu list : ", n->pmu_name);
+		cpu_map__fprintf(n->map, fp);
+	}
+}
+
 static void print_dir_format(struct feat_fd *ff, FILE *fp)
 {
 	struct perf_session *session;
@@ -2849,6 +2896,50 @@ static int process_clock_data(struct feat_fd *ff,
 	return 0;
 }
 
+static int process_hybrid_topology(struct feat_fd *ff,
+				   void *data __maybe_unused)
+{
+	struct hybrid_node *nodes, *n;
+	u32 nr, i;
+	char *str;
+
+	/* nr nodes */
+	if (do_read_u32(ff, &nr))
+		return -1;
+
+	nodes = zalloc(sizeof(*nodes) * nr);
+	if (!nodes)
+		return -ENOMEM;
+
+	for (i = 0; i < nr; i++) {
+		n = &nodes[i];
+
+		n->pmu_name = do_read_string(ff);
+		if (!n->pmu_name)
+			goto error;
+
+		str = do_read_string(ff);
+		if (!str)
+			goto error;
+
+		n->map = perf_cpu_map__new(str);
+		free(str);
+		if (!n->map)
+			goto error;
+	}
+
+	ff->ph->env.nr_hybrid_nodes = nr;
+	ff->ph->env.hybrid_nodes = nodes;
+	return 0;
+
+error:
+	for (i = 0; i < nr; i++)
+		free(nodes[i].pmu_name);
+
+	free(nodes);
+	return -1;
+}
+
 static int process_dir_format(struct feat_fd *ff,
 			      void *_data __maybe_unused)
 {
@@ -3117,6 +3208,7 @@ const struct perf_header_feature_ops feat_ops[HEADER_LAST_FEATURE] = {
 	FEAT_OPR(COMPRESSED,	compressed,	false),
 	FEAT_OPR(CPU_PMU_CAPS,	cpu_pmu_caps,	false),
 	FEAT_OPR(CLOCK_DATA,	clock_data,	false),
+	FEAT_OPN(HYBRID_TOPOLOGY,	hybrid_topology,	true),
 };
 
 struct header_print_data {
diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
index 2aca71763ecf..3f12ec0eb84e 100644
--- a/tools/perf/util/header.h
+++ b/tools/perf/util/header.h
@@ -45,6 +45,7 @@ enum {
 	HEADER_COMPRESSED,
 	HEADER_CPU_PMU_CAPS,
 	HEADER_CLOCK_DATA,
+	HEADER_HYBRID_TOPOLOGY,
 	HEADER_LAST_FEATURE,
 	HEADER_FEAT_BITS	= 256,
 };
diff --git a/tools/perf/util/pmu-hybrid.h b/tools/perf/util/pmu-hybrid.h
index d0fa7bc50a76..2b186c26a43e 100644
--- a/tools/perf/util/pmu-hybrid.h
+++ b/tools/perf/util/pmu-hybrid.h
@@ -19,4 +19,15 @@ struct perf_pmu *perf_pmu__find_hybrid_pmu(const char *name);
 bool perf_pmu__is_hybrid(const char *name);
 char *perf_pmu__hybrid_type_to_pmu(const char *type);
 
+static inline int perf_pmu__hybrid_pmu_num(void)
+{
+	struct perf_pmu *pmu;
+	int num = 0;
+
+	perf_pmu__for_each_hybrid_pmu(pmu)
+		num++;
+
+	return num;
+}
+
 #endif /* __PMU_HYBRID_H */
-- 
2.17.1

[PATCH v1 2/2] perf header: Support hybrid CPU_PMU_CAPS

[Jin Yao]

On hybrid platform, it may have several cpu pmus, such as,
"cpu_core" and "cpu_atom". The CPU_PMU_CAPS feature in perf
header needs to be improved to support multiple cpu pmus.

The new layout in header is defined as:

<nr_caps>
<caps string>
<caps string>
<pmu name>
<nr of rest pmus>

It's considered to be compatible with old perf.data (the
perf.data generated by old perf tool).

With this patch, some examples,

New perf tool with new perf.data
(new perf.data is generated on hybrid platform):

  root@otcpl-adl-s-2:~# perf report --header-only -I
  ...
  # cpu_core pmu capabilities: branches=32, max_precise=3, pmu_name=alderlake_hybrid
  # cpu_atom pmu capabilities: branches=32, max_precise=3, pmu_name=alderlake_hybrid

New perf tool with new perf.data
(new perf.data is generated on non-hybrid platform):

  root@kbl-ppc:~# perf report --header-only -I
  ...
  # cpu pmu capabilities: branches=32, max_precise=3, pmu_name=skylake

New perf tool with old perf.data
(old perf.data is generated by old perf tool on non-hybrid platform):

  root@kbl-ppc:~# perf report --header-only -I
  ...
  # cpu pmu capabilities: branches=32, max_precise=3, pmu_name=skylake

Note that: this patch is on tmp.perf/core.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/util/env.c    |   6 ++
 tools/perf/util/env.h    |  11 ++-
 tools/perf/util/header.c | 175 ++++++++++++++++++++++++++++++++++-----
 3 files changed, 168 insertions(+), 24 deletions(-)

diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index 9e05eca324a1..8ef24aad2152 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -208,6 +208,12 @@ void perf_env__exit(struct perf_env *env)
 		zfree(&env->hybrid_nodes[i].pmu_name);
 	}
 	zfree(&env->hybrid_nodes);
+
+	for (i = 0; i < env->nr_cpu_pmu_caps_nodes; i++) {
+		zfree(&env->cpu_pmu_caps_nodes[i].cpu_pmu_caps);
+		zfree(&env->cpu_pmu_caps_nodes[i].pmu_name);
+	}
+	zfree(&env->cpu_pmu_caps_nodes);
 }
 
 void perf_env__init(struct perf_env *env __maybe_unused)
diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
index 9ca7633787e1..5552c98a6a76 100644
--- a/tools/perf/util/env.h
+++ b/tools/perf/util/env.h
@@ -42,6 +42,13 @@ struct hybrid_node {
 	struct perf_cpu_map	*map;
 };
 
+struct cpu_pmu_caps_node {
+	int		nr_cpu_pmu_caps;
+	unsigned int	max_branches;
+	char		*cpu_pmu_caps;
+	char		*pmu_name;
+};
+
 struct perf_env {
 	char			*hostname;
 	char			*os_release;
@@ -63,15 +70,14 @@ struct perf_env {
 	int			nr_memory_nodes;
 	int			nr_pmu_mappings;
 	int			nr_groups;
-	int			nr_cpu_pmu_caps;
 	int			nr_hybrid_nodes;
+	int			nr_cpu_pmu_caps_nodes;
 	char			*cmdline;
 	const char		**cmdline_argv;
 	char			*sibling_cores;
 	char			*sibling_dies;
 	char			*sibling_threads;
 	char			*pmu_mappings;
-	char			*cpu_pmu_caps;
 	struct cpu_topology_map	*cpu;
 	struct cpu_cache_level	*caches;
 	int			 caches_cnt;
@@ -84,6 +90,7 @@ struct perf_env {
 	struct memory_node	*memory_nodes;
 	unsigned long long	 memory_bsize;
 	struct hybrid_node	*hybrid_nodes;
+	struct cpu_pmu_caps_node	*cpu_pmu_caps_nodes;
 #ifdef HAVE_LIBBPF_SUPPORT
 	/*
 	 * bpf_info_lock protects bpf rbtrees. This is needed because the
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index dff89c0be79c..6989c57b57e6 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -49,6 +49,7 @@
 #include "cputopo.h"
 #include "bpf-event.h"
 #include "clockid.h"
+#include "pmu-hybrid.h"
 
 #include <linux/ctype.h>
 #include <internal/lib.h>
@@ -1459,18 +1460,22 @@ static int write_compressed(struct feat_fd *ff __maybe_unused,
 	return do_write(ff, &(ff->ph->env.comp_mmap_len), sizeof(ff->ph->env.comp_mmap_len));
 }
 
-static int write_cpu_pmu_caps(struct feat_fd *ff,
-			      struct evlist *evlist __maybe_unused)
+static int write_per_cpu_pmu_caps(struct feat_fd *ff, struct perf_pmu *pmu,
+				  int nr)
 {
-	struct perf_pmu *cpu_pmu = perf_pmu__find("cpu");
 	struct perf_pmu_caps *caps = NULL;
 	int nr_caps;
 	int ret;
 
-	if (!cpu_pmu)
-		return -ENOENT;
-
-	nr_caps = perf_pmu__caps_parse(cpu_pmu);
+	/*
+	 * The layout is:
+	 * <nr_caps>
+	 * <caps string>
+	 * <caps string>
+	 * <pmu name>
+	 * <nr of rest pmus>
+	 */
+	nr_caps = perf_pmu__caps_parse(pmu);
 	if (nr_caps < 0)
 		return nr_caps;
 
@@ -1478,7 +1483,7 @@ static int write_cpu_pmu_caps(struct feat_fd *ff,
 	if (ret < 0)
 		return ret;
 
-	list_for_each_entry(caps, &cpu_pmu->caps, list) {
+	list_for_each_entry(caps, &pmu->caps, list) {
 		ret = do_write_string(ff, caps->name);
 		if (ret < 0)
 			return ret;
@@ -1488,9 +1493,49 @@ static int write_cpu_pmu_caps(struct feat_fd *ff,
 			return ret;
 	}
 
+	ret = do_write_string(ff, pmu->name);
+	if (ret < 0)
+		return ret;
+
+	ret = do_write(ff, &nr, sizeof(nr));
+	if (ret < 0)
+		return ret;
+
 	return ret;
 }
 
+static int write_cpu_pmu_caps(struct feat_fd *ff,
+			      struct evlist *evlist __maybe_unused)
+{
+	struct perf_pmu *pmu = perf_pmu__find("cpu");
+	u32 nr;
+	int ret;
+
+	if (pmu)
+		nr = 1;
+	else {
+		nr = perf_pmu__hybrid_pmu_num();
+		pmu = NULL;
+	}
+
+	if (nr == 0)
+		return -1;
+
+	if (pmu) {
+		ret = write_per_cpu_pmu_caps(ff, pmu, 0);
+		if (ret < 0)
+			return ret;
+	} else {
+		perf_pmu__for_each_hybrid_pmu(pmu) {
+			ret = write_per_cpu_pmu_caps(ff, pmu, --nr);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
 static void print_hostname(struct feat_fd *ff, FILE *fp)
 {
 	fprintf(fp, "# hostname : %s\n", ff->ph->env.hostname);
@@ -1963,18 +2008,28 @@ static void print_compressed(struct feat_fd *ff, FILE *fp)
 		ff->ph->env.comp_level, ff->ph->env.comp_ratio);
 }
 
-static void print_cpu_pmu_caps(struct feat_fd *ff, FILE *fp)
+static void print_per_cpu_pmu_caps(FILE *fp, struct cpu_pmu_caps_node *n)
 {
-	const char *delimiter = "# cpu pmu capabilities: ";
-	u32 nr_caps = ff->ph->env.nr_cpu_pmu_caps;
-	char *str;
+	const char *delimiter;
+	u32 nr_caps = n->nr_cpu_pmu_caps;
+	char *str, buf[128];
 
 	if (!nr_caps) {
-		fprintf(fp, "# cpu pmu capabilities: not available\n");
+		if (!n->pmu_name)
+			fprintf(fp, "# cpu pmu capabilities: not available\n");
+		else
+			fprintf(fp, "# %s pmu capabilities: not available\n", n->pmu_name);
 		return;
 	}
 
-	str = ff->ph->env.cpu_pmu_caps;
+	if (!n->pmu_name)
+		scnprintf(buf, sizeof(buf), "# cpu pmu capabilities: ");
+	else
+		scnprintf(buf, sizeof(buf), "# %s pmu capabilities: ", n->pmu_name);
+
+	delimiter = buf;
+
+	str = n->cpu_pmu_caps;
 	while (nr_caps--) {
 		fprintf(fp, "%s%s", delimiter, str);
 		delimiter = ", ";
@@ -1984,6 +2039,17 @@ static void print_cpu_pmu_caps(struct feat_fd *ff, FILE *fp)
 	fprintf(fp, "\n");
 }
 
+static void print_cpu_pmu_caps(struct feat_fd *ff, FILE *fp)
+{
+	struct cpu_pmu_caps_node *n;
+	int i;
+
+	for (i = 0; i < ff->ph->env.nr_cpu_pmu_caps_nodes; i++) {
+		n = &ff->ph->env.cpu_pmu_caps_nodes[i];
+		print_per_cpu_pmu_caps(fp, n);
+	}
+}
+
 static void print_pmu_mappings(struct feat_fd *ff, FILE *fp)
 {
 	const char *delimiter = "# pmu mappings: ";
@@ -3093,13 +3159,14 @@ static int process_compressed(struct feat_fd *ff,
 	return 0;
 }
 
-static int process_cpu_pmu_caps(struct feat_fd *ff,
-				void *data __maybe_unused)
+static int process_cpu_pmu_caps_node(struct feat_fd *ff,
+				     struct cpu_pmu_caps_node *n, bool *end)
 {
-	char *name, *value;
+	char *name, *value, *pmu_name;
 	struct strbuf sb;
-	u32 nr_caps;
+	u32 nr_caps, nr;
 
+	*end = false;
 	if (do_read_u32(ff, &nr_caps))
 		return -1;
 
@@ -3108,7 +3175,7 @@ static int process_cpu_pmu_caps(struct feat_fd *ff,
 		return 0;
 	}
 
-	ff->ph->env.nr_cpu_pmu_caps = nr_caps;
+	n->nr_cpu_pmu_caps = nr_caps;
 
 	if (strbuf_init(&sb, 128) < 0)
 		return -1;
@@ -3129,13 +3196,33 @@ static int process_cpu_pmu_caps(struct feat_fd *ff,
 		if (strbuf_add(&sb, "", 1) < 0)
 			goto free_value;
 
-		if (!strcmp(name, "branches"))
-			ff->ph->env.max_branches = atoi(value);
+		if (!strcmp(name, "branches")) {
+			n->max_branches = atoi(value);
+			if (n->max_branches > ff->ph->env.max_branches)
+				ff->ph->env.max_branches = n->max_branches;
+		}
 
 		free(value);
 		free(name);
 	}
-	ff->ph->env.cpu_pmu_caps = strbuf_detach(&sb, NULL);
+
+	/*
+	 * Old perf.data may not have pmu_name,
+	 */
+	pmu_name = do_read_string(ff);
+	if (!pmu_name || strncmp(pmu_name, "cpu_", 4)) {
+		*end = true;
+		goto out;
+	}
+
+	if (do_read_u32(ff, &nr))
+		return -1;
+
+	if (nr == 0)
+		*end = true;
+out:
+	n->cpu_pmu_caps = strbuf_detach(&sb, NULL);
+	n->pmu_name = pmu_name;
 	return 0;
 
 free_value:
@@ -3147,6 +3234,50 @@ static int process_cpu_pmu_caps(struct feat_fd *ff,
 	return -1;
 }
 
+static int process_cpu_pmu_caps(struct feat_fd *ff,
+				void *data __maybe_unused)
+{
+	struct cpu_pmu_caps_node *nodes = NULL, *tmp;
+	int ret, i, nr_alloc = 1, nr_used = 0;
+	bool end;
+
+	while (1) {
+		if (nr_used == nr_alloc || !nodes) {
+			nr_alloc *= 2;
+			tmp = realloc(nodes, sizeof(*nodes) * nr_alloc);
+			if (!tmp)
+				return -ENOMEM;
+			memset(tmp + nr_used, 0,
+			       sizeof(*nodes) * (nr_alloc - nr_used));
+			nodes = tmp;
+		}
+
+		ret = process_cpu_pmu_caps_node(ff, &nodes[nr_used], &end);
+		if (ret) {
+			if (nr_used)
+				break;
+			goto err;
+		}
+
+		nr_used++;
+		if (end)
+			break;
+	}
+
+	ff->ph->env.nr_cpu_pmu_caps_nodes = (u32)nr_used;
+	ff->ph->env.cpu_pmu_caps_nodes = nodes;
+	return 0;
+
+err:
+	for (i = 0; i < nr_used; i++) {
+		free(nodes[i].cpu_pmu_caps);
+		free(nodes[i].pmu_name);
+	}
+
+	free(nodes);
+	return ret;
+}
+
 #define FEAT_OPR(n, func, __full_only) \
 	[HEADER_##n] = {					\
 		.name	    = __stringify(n),			\
-- 
2.17.1

Re: [PATCH v1 1/2] perf header: Support HYBRID_TOPOLOGY feature

[Arnaldo Carvalho de Melo]

Em Fri, Apr 30, 2021 at 03:46:01PM +0800, Jin Yao escreveu:
> +struct hybrid_topology *hybrid_topology__new(void)
> +{
> +	struct perf_pmu *pmu;
> +	struct hybrid_topology *tp = NULL;
> +	u32 nr = 0, i = 0;
> +
> +	nr = perf_pmu__hybrid_pmu_num();

Initialize it to zero and right away overwrite it with a function
return?

> +	u32 nr = 0, i = 0;

	u32 nr = perf_pmu__hybrid_pmu_num(), i;

> +	if (nr == 0)
> +		return NULL;
> +
> +	tp = zalloc(sizeof(*tp) + sizeof(tp->nodes[0]) * nr);
> +	if (!tp)
> +		return NULL;
> +
> +	tp->nr = nr;

	i = 0;

Please fix these and add an entry to
tools/perf/Documentation/perf.data-file-format.txt for this new feature,
then submit v2.

And on 2/2 please update the CPU_PMU_CAPS entry in
tools/perf/Documentation/perf.data-file-format.txt.

Thanks,

- Arnaldo


> +	perf_pmu__for_each_hybrid_pmu(pmu) {
> +		if (load_hybrid_node(&tp->nodes[i], pmu)) {
> +			hybrid_topology__delete(tp);
> +			return NULL;
> +		}
> +		i++;
> +	}
> +
> +	return tp;
> +}
> +
> +void hybrid_topology__delete(struct hybrid_topology *tp)
> +{
> +	u32 i;
> +
> +	for (i = 0; i < tp->nr; i++) {
> +		zfree(&tp->nodes[i].pmu_name);
> +		zfree(&tp->nodes[i].cpus);
> +	}
> +
> +	free(tp);
> +}
> diff --git a/tools/perf/util/cputopo.h b/tools/perf/util/cputopo.h
> index 6201c3790d86..d9af97177068 100644
> --- a/tools/perf/util/cputopo.h
> +++ b/tools/perf/util/cputopo.h
> @@ -25,10 +25,23 @@ struct numa_topology {
>  	struct numa_topology_node	nodes[];
>  };
>  
> +struct hybrid_topology_node {
> +	char		*pmu_name;
> +	char		*cpus;
> +};
> +
> +struct hybrid_topology {
> +	u32				nr;
> +	struct hybrid_topology_node	nodes[];
> +};
> +
>  struct cpu_topology *cpu_topology__new(void);
>  void cpu_topology__delete(struct cpu_topology *tp);
>  
>  struct numa_topology *numa_topology__new(void);
>  void numa_topology__delete(struct numa_topology *tp);
>  
> +struct hybrid_topology *hybrid_topology__new(void);
> +void hybrid_topology__delete(struct hybrid_topology *tp);
> +
>  #endif /* __PERF_CPUTOPO_H */
> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index 9130f6fad8d5..9e05eca324a1 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
> @@ -202,6 +202,12 @@ void perf_env__exit(struct perf_env *env)
>  	for (i = 0; i < env->nr_memory_nodes; i++)
>  		zfree(&env->memory_nodes[i].set);
>  	zfree(&env->memory_nodes);
> +
> +	for (i = 0; i < env->nr_hybrid_nodes; i++) {
> +		perf_cpu_map__put(env->hybrid_nodes[i].map);
> +		zfree(&env->hybrid_nodes[i].pmu_name);
> +	}
> +	zfree(&env->hybrid_nodes);
>  }
>  
>  void perf_env__init(struct perf_env *env __maybe_unused)
> diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
> index ca249bf5e984..9ca7633787e1 100644
> --- a/tools/perf/util/env.h
> +++ b/tools/perf/util/env.h
> @@ -37,6 +37,11 @@ struct memory_node {
>  	unsigned long	*set;
>  };
>  
> +struct hybrid_node {
> +	char	*pmu_name;
> +	struct perf_cpu_map	*map;
> +};
> +
>  struct perf_env {
>  	char			*hostname;
>  	char			*os_release;
> @@ -59,6 +64,7 @@ struct perf_env {
>  	int			nr_pmu_mappings;
>  	int			nr_groups;
>  	int			nr_cpu_pmu_caps;
> +	int			nr_hybrid_nodes;
>  	char			*cmdline;
>  	const char		**cmdline_argv;
>  	char			*sibling_cores;
> @@ -77,6 +83,7 @@ struct perf_env {
>  	struct numa_node	*numa_nodes;
>  	struct memory_node	*memory_nodes;
>  	unsigned long long	 memory_bsize;
> +	struct hybrid_node	*hybrid_nodes;
>  #ifdef HAVE_LIBBPF_SUPPORT
>  	/*
>  	 * bpf_info_lock protects bpf rbtrees. This is needed because the
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index aa1e42518d37..dff89c0be79c 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -932,6 +932,40 @@ static int write_clock_data(struct feat_fd *ff,
>  	return do_write(ff, data64, sizeof(*data64));
>  }
>  
> +static int write_hybrid_topology(struct feat_fd *ff,
> +				 struct evlist *evlist __maybe_unused)
> +{
> +	struct hybrid_topology *tp;
> +	int ret;
> +	u32 i;
> +
> +	tp = hybrid_topology__new();
> +	if (!tp)
> +		return -1;
> +
> +	ret = do_write(ff, &tp->nr, sizeof(u32));
> +	if (ret < 0)
> +		goto err;
> +
> +	for (i = 0; i < tp->nr; i++) {
> +		struct hybrid_topology_node *n = &tp->nodes[i];
> +
> +		ret = do_write_string(ff, n->pmu_name);
> +		if (ret < 0)
> +			goto err;
> +
> +		ret = do_write_string(ff, n->cpus);
> +		if (ret < 0)
> +			goto err;
> +	}
> +
> +	ret = 0;
> +
> +err:
> +	hybrid_topology__delete(tp);
> +	return ret;
> +}
> +
>  static int write_dir_format(struct feat_fd *ff,
>  			    struct evlist *evlist __maybe_unused)
>  {
> @@ -1623,6 +1657,19 @@ static void print_clock_data(struct feat_fd *ff, FILE *fp)
>  		    clockid_name(clockid));
>  }
>  
> +static void print_hybrid_topology(struct feat_fd *ff, FILE *fp)
> +{
> +	int i;
> +	struct hybrid_node *n;
> +
> +	for (i = 0; i < ff->ph->env.nr_hybrid_nodes; i++) {
> +		n = &ff->ph->env.hybrid_nodes[i];
> +
> +		fprintf(fp, "# %s cpu list : ", n->pmu_name);
> +		cpu_map__fprintf(n->map, fp);
> +	}
> +}
> +
>  static void print_dir_format(struct feat_fd *ff, FILE *fp)
>  {
>  	struct perf_session *session;
> @@ -2849,6 +2896,50 @@ static int process_clock_data(struct feat_fd *ff,
>  	return 0;
>  }
>  
> +static int process_hybrid_topology(struct feat_fd *ff,
> +				   void *data __maybe_unused)
> +{
> +	struct hybrid_node *nodes, *n;
> +	u32 nr, i;
> +	char *str;
> +
> +	/* nr nodes */
> +	if (do_read_u32(ff, &nr))
> +		return -1;
> +
> +	nodes = zalloc(sizeof(*nodes) * nr);
> +	if (!nodes)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nr; i++) {
> +		n = &nodes[i];
> +
> +		n->pmu_name = do_read_string(ff);
> +		if (!n->pmu_name)
> +			goto error;
> +
> +		str = do_read_string(ff);
> +		if (!str)
> +			goto error;
> +
> +		n->map = perf_cpu_map__new(str);
> +		free(str);
> +		if (!n->map)
> +			goto error;
> +	}
> +
> +	ff->ph->env.nr_hybrid_nodes = nr;
> +	ff->ph->env.hybrid_nodes = nodes;
> +	return 0;
> +
> +error:
> +	for (i = 0; i < nr; i++)
> +		free(nodes[i].pmu_name);
> +
> +	free(nodes);
> +	return -1;
> +}
> +
>  static int process_dir_format(struct feat_fd *ff,
>  			      void *_data __maybe_unused)
>  {
> @@ -3117,6 +3208,7 @@ const struct perf_header_feature_ops feat_ops[HEADER_LAST_FEATURE] = {
>  	FEAT_OPR(COMPRESSED,	compressed,	false),
>  	FEAT_OPR(CPU_PMU_CAPS,	cpu_pmu_caps,	false),
>  	FEAT_OPR(CLOCK_DATA,	clock_data,	false),
> +	FEAT_OPN(HYBRID_TOPOLOGY,	hybrid_topology,	true),
>  };
>  
>  struct header_print_data {
> diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
> index 2aca71763ecf..3f12ec0eb84e 100644
> --- a/tools/perf/util/header.h
> +++ b/tools/perf/util/header.h
> @@ -45,6 +45,7 @@ enum {
>  	HEADER_COMPRESSED,
>  	HEADER_CPU_PMU_CAPS,
>  	HEADER_CLOCK_DATA,
> +	HEADER_HYBRID_TOPOLOGY,
>  	HEADER_LAST_FEATURE,
>  	HEADER_FEAT_BITS	= 256,
>  };
> diff --git a/tools/perf/util/pmu-hybrid.h b/tools/perf/util/pmu-hybrid.h
> index d0fa7bc50a76..2b186c26a43e 100644
> --- a/tools/perf/util/pmu-hybrid.h
> +++ b/tools/perf/util/pmu-hybrid.h
> @@ -19,4 +19,15 @@ struct perf_pmu *perf_pmu__find_hybrid_pmu(const char *name);
>  bool perf_pmu__is_hybrid(const char *name);
>  char *perf_pmu__hybrid_type_to_pmu(const char *type);
>  
> +static inline int perf_pmu__hybrid_pmu_num(void)
> +{
> +	struct perf_pmu *pmu;
> +	int num = 0;
> +
> +	perf_pmu__for_each_hybrid_pmu(pmu)
> +		num++;
> +
> +	return num;
> +}
> +
>  #endif /* __PMU_HYBRID_H */
> -- 
> 2.17.1
> 

-- 

- Arnaldo

Re: [PATCH v1 1/2] perf header: Support HYBRID_TOPOLOGY feature

[Jin, Yao]

Hi Arnaldo,

On 5/3/2021 11:18 PM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Apr 30, 2021 at 03:46:01PM +0800, Jin Yao escreveu:
>> +struct hybrid_topology *hybrid_topology__new(void)
>> +{
>> +	struct perf_pmu *pmu;
>> +	struct hybrid_topology *tp = NULL;
>> +	u32 nr = 0, i = 0;
>> +
>> +	nr = perf_pmu__hybrid_pmu_num();
> 
> Initialize it to zero and right away overwrite it with a function
> return?
> 

Hmm, yes, the 'nr' doesn't need to be initialized to 0, sorry about that.

>> +	u32 nr = 0, i = 0;
> 
> 	u32 nr = perf_pmu__hybrid_pmu_num(), i;
> 
>> +	if (nr == 0)
>> +		return NULL;
>> +
>> +	tp = zalloc(sizeof(*tp) + sizeof(tp->nodes[0]) * nr);
>> +	if (!tp)
>> +		return NULL;
>> +
>> +	tp->nr = nr;
> 
> 	i = 0;
> 
> Please fix these and add an entry to
> tools/perf/Documentation/perf.data-file-format.txt for this new feature,
> then submit v2.
> 
> And on 2/2 please update the CPU_PMU_CAPS entry in
> tools/perf/Documentation/perf.data-file-format.txt.
> 
> Thanks,
> 

OK, I will add these features to perf.data-file-format.txt and then post v2.

Thanks
Jin Yao

> - Arnaldo
> 
> 
>> +	perf_pmu__for_each_hybrid_pmu(pmu) {
>> +		if (load_hybrid_node(&tp->nodes[i], pmu)) {
>> +			hybrid_topology__delete(tp);
>> +			return NULL;
>> +		}
>> +		i++;
>> +	}
>> +
>> +	return tp;
>> +}
>> +
>> +void hybrid_topology__delete(struct hybrid_topology *tp)
>> +{
>> +	u32 i;
>> +
>> +	for (i = 0; i < tp->nr; i++) {
>> +		zfree(&tp->nodes[i].pmu_name);
>> +		zfree(&tp->nodes[i].cpus);
>> +	}
>> +
>> +	free(tp);
>> +}
>> diff --git a/tools/perf/util/cputopo.h b/tools/perf/util/cputopo.h
>> index 6201c3790d86..d9af97177068 100644
>> --- a/tools/perf/util/cputopo.h
>> +++ b/tools/perf/util/cputopo.h
>> @@ -25,10 +25,23 @@ struct numa_topology {
>>   	struct numa_topology_node	nodes[];
>>   };
>>   
>> +struct hybrid_topology_node {
>> +	char		*pmu_name;
>> +	char		*cpus;
>> +};
>> +
>> +struct hybrid_topology {
>> +	u32				nr;
>> +	struct hybrid_topology_node	nodes[];
>> +};
>> +
>>   struct cpu_topology *cpu_topology__new(void);
>>   void cpu_topology__delete(struct cpu_topology *tp);
>>   
>>   struct numa_topology *numa_topology__new(void);
>>   void numa_topology__delete(struct numa_topology *tp);
>>   
>> +struct hybrid_topology *hybrid_topology__new(void);
>> +void hybrid_topology__delete(struct hybrid_topology *tp);
>> +
>>   #endif /* __PERF_CPUTOPO_H */
>> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
>> index 9130f6fad8d5..9e05eca324a1 100644
>> --- a/tools/perf/util/env.c
>> +++ b/tools/perf/util/env.c
>> @@ -202,6 +202,12 @@ void perf_env__exit(struct perf_env *env)
>>   	for (i = 0; i < env->nr_memory_nodes; i++)
>>   		zfree(&env->memory_nodes[i].set);
>>   	zfree(&env->memory_nodes);
>> +
>> +	for (i = 0; i < env->nr_hybrid_nodes; i++) {
>> +		perf_cpu_map__put(env->hybrid_nodes[i].map);
>> +		zfree(&env->hybrid_nodes[i].pmu_name);
>> +	}
>> +	zfree(&env->hybrid_nodes);
>>   }
>>   
>>   void perf_env__init(struct perf_env *env __maybe_unused)
>> diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
>> index ca249bf5e984..9ca7633787e1 100644
>> --- a/tools/perf/util/env.h
>> +++ b/tools/perf/util/env.h
>> @@ -37,6 +37,11 @@ struct memory_node {
>>   	unsigned long	*set;
>>   };
>>   
>> +struct hybrid_node {
>> +	char	*pmu_name;
>> +	struct perf_cpu_map	*map;
>> +};
>> +
>>   struct perf_env {
>>   	char			*hostname;
>>   	char			*os_release;
>> @@ -59,6 +64,7 @@ struct perf_env {
>>   	int			nr_pmu_mappings;
>>   	int			nr_groups;
>>   	int			nr_cpu_pmu_caps;
>> +	int			nr_hybrid_nodes;
>>   	char			*cmdline;
>>   	const char		**cmdline_argv;
>>   	char			*sibling_cores;
>> @@ -77,6 +83,7 @@ struct perf_env {
>>   	struct numa_node	*numa_nodes;
>>   	struct memory_node	*memory_nodes;
>>   	unsigned long long	 memory_bsize;
>> +	struct hybrid_node	*hybrid_nodes;
>>   #ifdef HAVE_LIBBPF_SUPPORT
>>   	/*
>>   	 * bpf_info_lock protects bpf rbtrees. This is needed because the
>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>> index aa1e42518d37..dff89c0be79c 100644
>> --- a/tools/perf/util/header.c
>> +++ b/tools/perf/util/header.c
>> @@ -932,6 +932,40 @@ static int write_clock_data(struct feat_fd *ff,
>>   	return do_write(ff, data64, sizeof(*data64));
>>   }
>>   
>> +static int write_hybrid_topology(struct feat_fd *ff,
>> +				 struct evlist *evlist __maybe_unused)
>> +{
>> +	struct hybrid_topology *tp;
>> +	int ret;
>> +	u32 i;
>> +
>> +	tp = hybrid_topology__new();
>> +	if (!tp)
>> +		return -1;
>> +
>> +	ret = do_write(ff, &tp->nr, sizeof(u32));
>> +	if (ret < 0)
>> +		goto err;
>> +
>> +	for (i = 0; i < tp->nr; i++) {
>> +		struct hybrid_topology_node *n = &tp->nodes[i];
>> +
>> +		ret = do_write_string(ff, n->pmu_name);
>> +		if (ret < 0)
>> +			goto err;
>> +
>> +		ret = do_write_string(ff, n->cpus);
>> +		if (ret < 0)
>> +			goto err;
>> +	}
>> +
>> +	ret = 0;
>> +
>> +err:
>> +	hybrid_topology__delete(tp);
>> +	return ret;
>> +}
>> +
>>   static int write_dir_format(struct feat_fd *ff,
>>   			    struct evlist *evlist __maybe_unused)
>>   {
>> @@ -1623,6 +1657,19 @@ static void print_clock_data(struct feat_fd *ff, FILE *fp)
>>   		    clockid_name(clockid));
>>   }
>>   
>> +static void print_hybrid_topology(struct feat_fd *ff, FILE *fp)
>> +{
>> +	int i;
>> +	struct hybrid_node *n;
>> +
>> +	for (i = 0; i < ff->ph->env.nr_hybrid_nodes; i++) {
>> +		n = &ff->ph->env.hybrid_nodes[i];
>> +
>> +		fprintf(fp, "# %s cpu list : ", n->pmu_name);
>> +		cpu_map__fprintf(n->map, fp);
>> +	}
>> +}
>> +
>>   static void print_dir_format(struct feat_fd *ff, FILE *fp)
>>   {
>>   	struct perf_session *session;
>> @@ -2849,6 +2896,50 @@ static int process_clock_data(struct feat_fd *ff,
>>   	return 0;
>>   }
>>   
>> +static int process_hybrid_topology(struct feat_fd *ff,
>> +				   void *data __maybe_unused)
>> +{
>> +	struct hybrid_node *nodes, *n;
>> +	u32 nr, i;
>> +	char *str;
>> +
>> +	/* nr nodes */
>> +	if (do_read_u32(ff, &nr))
>> +		return -1;
>> +
>> +	nodes = zalloc(sizeof(*nodes) * nr);
>> +	if (!nodes)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < nr; i++) {
>> +		n = &nodes[i];
>> +
>> +		n->pmu_name = do_read_string(ff);
>> +		if (!n->pmu_name)
>> +			goto error;
>> +
>> +		str = do_read_string(ff);
>> +		if (!str)
>> +			goto error;
>> +
>> +		n->map = perf_cpu_map__new(str);
>> +		free(str);
>> +		if (!n->map)
>> +			goto error;
>> +	}
>> +
>> +	ff->ph->env.nr_hybrid_nodes = nr;
>> +	ff->ph->env.hybrid_nodes = nodes;
>> +	return 0;
>> +
>> +error:
>> +	for (i = 0; i < nr; i++)
>> +		free(nodes[i].pmu_name);
>> +
>> +	free(nodes);
>> +	return -1;
>> +}
>> +
>>   static int process_dir_format(struct feat_fd *ff,
>>   			      void *_data __maybe_unused)
>>   {
>> @@ -3117,6 +3208,7 @@ const struct perf_header_feature_ops feat_ops[HEADER_LAST_FEATURE] = {
>>   	FEAT_OPR(COMPRESSED,	compressed,	false),
>>   	FEAT_OPR(CPU_PMU_CAPS,	cpu_pmu_caps,	false),
>>   	FEAT_OPR(CLOCK_DATA,	clock_data,	false),
>> +	FEAT_OPN(HYBRID_TOPOLOGY,	hybrid_topology,	true),
>>   };
>>   
>>   struct header_print_data {
>> diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
>> index 2aca71763ecf..3f12ec0eb84e 100644
>> --- a/tools/perf/util/header.h
>> +++ b/tools/perf/util/header.h
>> @@ -45,6 +45,7 @@ enum {
>>   	HEADER_COMPRESSED,
>>   	HEADER_CPU_PMU_CAPS,
>>   	HEADER_CLOCK_DATA,
>> +	HEADER_HYBRID_TOPOLOGY,
> Return-Path: <linux-kernel-owner@vger.kernel.org>
> X-Original-To: yao.jin@linux.intel.com
> Delivered-To: yao.jin@linux.intel.com
> Received: from fmsmga001.fm.intel.com (fmsmga001.fm.intel.com [10.253.24.23])
> 	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
> 	(No client certificate requested)
> 	by linux.intel.com (Postfix) with ESMTPS id 36D9D580906;
> 	Mon,  3 May 2021 14:36:22 -0700 (PDT)
> IronPort-SDR: esi0FcPI009YsspGtVZcbgLx4B9FM+DVeKXdx+v+ajHsniBl02v1XF8B+GMpGwaPP2H9NXBHuF
>   joSEEGiTA5aJAoGf5ZBZFiJIiA6enxguk=
> X-IronPort-AV: E=McAfee;i="6200,9189,9973"; a="530707860"
> X-IronPort-AV: E=Sophos;i="5.82,271,1613462400";
>     d="scan'208";a="530707860"
> Received: from orsmga105.jf.intel.com ([10.7.208.20])
>    by fmsmga001-1.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 May 2021 14:36:20 -0700
> IronPort-SDR: 1MJbwfsAApKPsXRZYJaw3MaQlazYe6d7MM5ej4rEMjuYIk6Y+2Z9DqulfOSCsHkn0nZr0G2HbP
>   q+BNQ+kZ+1D8MTqu9shQXHRg9QFF2M1HE=
> X-SG-BADATTACHMENTNOREPLY: True
> X-IPAS-Result: =?us-ascii?q?A0FLAAAhbJBghxJggBdaHAEBAQEBAQcBARIBAQQEAQGCA?=
>   =?us-ascii?q?wcBAQsBg3gBASUSMYxoYIcDgR8qJQOBDJlzFIFfFAEBAQEBAQEBAQkdFAECB?=
>   =?us-ascii?q?AEBhk4CJTQJDQECBAEBAQMCAQIBAQEBAQECAgEBAQIBAQUCAQEBAQIQAQEBA?=
>   =?us-ascii?q?YEFAWBjAYFGAYFxBgQ9gjgig3QDAwECJAsBRgYJAR8yA1QHEgWCbIMIBapPM?=
>   =?us-ascii?q?4EBiDmBRIE6AYhzdIN4J4EEgSOBFTOCOXSEKIYSBII3DwEqE1EBMBI9eEcTB?=
>   =?us-ascii?q?UkDnlCcPgeDE4Eom3YQJ4NUkSmQJZUvghadQ4QwgVQ4gVwzGiODOU0BAgECA?=
>   =?us-ascii?q?QwBAgIDAQIBAgEHAQECAY4nFo5LIQEBMAI2AgYKAQEDCVkBAYw0AQE?=
> IronPort-PHdr: A9a23:XIaJvhVUx1aBDpY6+i5LE4vNh53V8KyPWjF92vIco4ILSbyq+tHYB
>   Gea288FpGHAUYiT0f9Yke2e6/mmBTVRp8za7DtbKsIEfiRGoP1epxYnDs+BBB+zB9/RRAt+I
>   v5/UkR49WqwK0lfFZW2TVTTpnqv8WxaQU2nZkJ6KevvB4Hdkdm82fys9J3PeQVIgye2ba9vI
>   Bmsogjdq80bjZF8Jqs/xRbFrWZEd/pXyGh1OV+dghHw69uq8pV+6SpQofUh98BBUaX+Yas1S
>   KFTASolPW4o+sDlrAHPQgST6HQSVGUWiQdIDBPe7B7mRJfxszD1ufR71SKHIMD5V7E0WTCl7
>   6d2VB/ljToMOjAl/G3LjMF7kaRWqw+jqRNi2Y7ZeIGbOuRjcK3Tft0UQmhOXsheWCJOGoOxb
>   ZACA/YCMOtCs4XwvUcCoQe4CAKxBO3v0DhIhnru0KMmzuQhChrG1xEgEN0QsXTUo9X1NLsPW
>   u2y1qnI0C7Db+hI1jf57IjEaBAhreuCXL1ucMrRxkkvFx/eg1WRr4zlIy2a1uAXv2eH6OpgU
>   Puihmg6oA5+vjah3N0jipXVho0L0FDE8z10zoY7KNO2SkN2bsOoHIVeuS2GOIV7TccvTm92t
>   Ss4ybALuJG1cSkJxZkmxxPTd/OKfpaV7x/9SuqdPCp1iX1mdb+5mh288lCgx/XiWsS6ylpGs
>   zRJn9rWun0J1hHf8NWLR/hl8ku81zuC2Brf5vxKLE07j6bXNpAsz78qmpYOr0jOHyn7k1jsg
>   qCMbEUr4O2o5vznYrr4op+cMJd5ih/xMqswgMyzG+c4PRYUX2id5+u80Kfv/UrjQLVFlvE2k
>   6/Zv47GJckDuKK0DRVZ3ps95xu8FTur1NUVkWMaIF9GYB6HipLmO1DKIPD2F/e/hFGsnS9vx
>   /DHOL3hH5rMImHAkLj/Z7Z97VBTyA4qwdBY6ZNUDK8OIOjoV0/vu9zXEAU5Pxa3w+n5EtV90
>   J0RWWaVDq+eKqPSvkeE5vgzLOmUeI8VpDH9JuA+6P7vi385lkQSfbSz0ZQLcn+4Ge9rI0GYY
>   Xrqn9cAHn0Gvgs4TOz2llKCVSRfaGq1X6I5/js7Ep6pDZ/fRoCxh7yMxD20HplIaWFJFF+NE
>   W3kd5ueV/gXci2TItJukiEeVbe7VYAhywqjtAv7y7phM+rV9TcUtZPl1Nhp+eLTkQs++iBzD
>   8SYy2uNVX17nnsURz8q26ByuUh9ykqA0adimPxZFMZf5/NTXQY0L57dz+h6C9P1Wg/aeteJS
>   VCmQsipAD0rT9IxxcMObFh5G9m4kh/D2C+qCacPl7OXHJw07r7c33/pKsZ9zHbG0qYhjlk7Q
>   sdVM22mh6h/9wfICo7NkkWZkbuqdKsG0C7M8meD0XSBvEVCXAFsVqXFWGgVZlHKotTh+kPCU
>   7iuBKw6PQRbz86CK7VFatn3gllcXvjjPMrRY3mqm2iqAxaH26mMY5Tue2UZ3SXQE0wEkwEV/
>   XabOgkyHCaho2TCDDNwEVLjeV/j8e57qHmjVE870xmKb1F917qy4hMVh+aTRO8Q3r4eoCchq
>   jN0HFmn093KC9qAoQVhcb5YYdM85ldHyG3YuxZ8PpymM6BtmFoefx5rsEPp0hV9Ep9AntQyr
>   HM20ApyLrqV0FFAdzOe0pD8JKfbKm3z/BCycKPW3lDe0NCL+qYA8vg4qlPjvB23GUom6Xloz
>   95V036E7JXQEAUSSY7xUlow9xViqLDaYzMy54TU1HJ2Nqm4qDvC29MyCeshyxagecpfMayeG
>   A/zFc0aG9ahKOgwl1e1aRIEOfhY9LQoMMO+a/uGxKmrMf5+nD28kGtH4Z593VyI9yZmT+7Iw
>   owFw+yX3gSdUTf9g02tstrtloBBeDESAmq/xjb6BIFLfK1/ZoILCWaoI82qydRynZ/tW3hE9
>   FG9A1MKwtOmeR2Xb1blxw1fyVwXoWC7mSu/1zF7jSopr6SF0yzU2evubhkHOmlXRGZ4llrsJ
>   Ym0j9YHXEmndQQplR2l5VrkyKhfvqhwM27TQUJQdSjsM25iSrewtqaFY8NX9JwosTtYUOO4Y
>   VyASr/9pB0a0yz5E2tdxTA7ciqnupH4nxx8lWKcI2x/rHvfecFs2xjf4MbQSuJW3joDXCN4k
>   yXYBkCgP9m1+tWZj4/DvfqwVm27UJ1fayrrzZ6FtCSh/21mGxK/n+2rmt37Cwc3yiv719hsV
>   SXVoxfweIjr16KmMe15eklkHkPz681/GotmiIs/mIkQ2WQGhpWS5XcHlWbzMdZF1q7kY3sNW
>   CUGw9rU4AjjxU1iIWiFx4P/VnWB3MRhY8O2bX8R2iI498pKErub7KRYnStppVq1tRjeYeN9n
>   jcayvsi8mQajPsLuAc2yiWdA7YSHVRXPCD2lhSI6cy+o7tTZGq1bbew009+l8i7DL6eugFcR
>   Gr5epA6EC9z9Mp/NkjA0Hny6o7+ftnQYskethmVkxfGkuhUJ4g9lvsMhSp7J239uWcpxPI8j
>   RxrxZu6ppSIK31x/KKlBR5VLiD6aNkN+j7zl6pentyZ34a0Hpp/HDUHW5/oTfGtEDIWrvnnM
>   weOEDshqnaUA7bfHAmf6Ft4oHLLCZykK3aXJHwBx9V4WBadPFBfgBwTXDgig5E2DB6qy9L7f
>   Ed5/D8R4EX4pQVKyu9zMxn/U2HfpBqnazsuSZifKgZW4R9G50vPLcOe6edzFTlC/pK9tAyNN
>   nCbZwNQAGEVW0yEAlfjPqWu5djA6eSYGvCyL/zUYbWKqOxeUfiIyImr0otn+TaMK8qOMmNjD
>   /09xkpMQ3R5F97FlDUITiwdjzjNYNKDpBeg5i13qdiy/+7vWA3x6oqAFbtTPc909BCrnKiML
>   eiQhCV+KTZGzZ4MwX7IyL4C3F8dkS1udj+tEageui7JVq7fhqhXDxsDYSNpKMRI97483hVKO
>   cPDkNP116J3geQvC1hYUlztgMepatIQI2G8LV/IGFyENLCbKjLV2cH3ZqW8RKZUjOVVsR2wp
>   DmaH1XiPjSFiznmSRSvPftQgyGcORxUoJu9fQp1CWj/UNLmbQW2Md92jTEswbw4nGjKNXMaM
>   TVnd0NNr7uQ7T5Xg/llGmxB6GZlIveAmyqD8+bYLZMWu+NxAitoj+Ja/Gg6y7xN4SBEXvN1n
>   zHertxvo1GgieSPzjtnUBxTqjdEno6LvENiObnH+ZlEQ3rL4BUN7WCIARQQu9RlEsHvu7xXy
>   tXXl6L8MjZC/8jU/csBHcfULs2HPWElMRroHj7UEQQEQSSqNWHZm0xSjvWS+meJoZg9r5jmg
>   IAORaNDVFwpCvMaDVxoHNkDIJd0RDwoi7Cag9AT5XqishbRXt5VsY7dVvKdGvjvLDeZjb9ZZ
>   xoH27/4LIITNpHl1ExmcFV1gIPKG0/IV9BXviJhdhM0oFlK8HVmUm0z3EflZhms4XAJE/60g
>   wU2ihB/Yek28Dfs4lE3JkfFpScqkUkxn8nlji6VcDLrMKiwWoRWATLut0ctKpP7Xxp1bQqqk
>   E1kMzfEWqtRj6Z6emBtkgPcopxPGfhTTaBfZB8QxPeXZ+gn0FhGqyWnw1NH6vXBCZd4iAQqd
>   puspWpa2w1/dN41ObDQJK1Rw1hQmK2Ovyyo1uMwwAMEJEYN6mSSeCEWt0wSK7kmPDGl/uht6
>   QyEhjtCd3IAV/ssovJ26Ew9P/6MwD7n075GMkqxLfCQL7uFu2jcks6FWkk/1l4TmElf47d30
>   d0vc06VV000yLuRFhIJNdfNKA1PbspS8mTTcjiKsenX3Z11OIC9HPjyTeCSrKYUnl6kHAExE
>   oQQ78QBG4Og313CIcj7LL4K1xMt6R/vJFWECvRJZR2KnC0Go8G50J94w41dKisBDmV6NCW9/
>   qzXqRMygPqfQNc2ZW8XX4seOXIwXc26mihZsGxBDTm3yO0ZzgeC4iT4piTRCjn8ctViaO2VZ
>   RNqFNG55zE//7Kqhl7Q95WNb139YNFutPfM5PkcqpLBDOlbCfF5skHBi8xWX3Oje3DAHMTzJ
>   JXqbYQoK9vuBTLyVl25liJwTMrrOtupBraHjBuuRotOtoSfmjc5OpyTDDYbTi1tvPpL2699a
>   g0fK84hfAXzugA6caz5PwqZ3dWya2erNTZSCfJYyLPpNPRs0yMwY7rimzMbRZYgwrzvmXM=
> IronPort-HdrOrdr: A9a23:t4aaUaA+6hlJPGHlHejssceALOonbusQ8zAX/mp6ICYlFvCwvc
>   aogfgdyFvIkz4XQn4tgpStP6OHTHPa+/dOkPUsFJqrQQWOghrPEKhM9o3nqgeQeRHW1ukY7q
>   t4drg7NduYNykAse/b+w++KtA63Z283ZvAv4q+815JTRt2L4FMhj0JdDqzNm1TaE14CYEiFJ
>   yaj/A32gaIXXgMdMy0Cj0kUoH41qT2vav8bRQLChIh4gXmt0LW1JfAHxKV3ggTXlp0qN9Imw
>   a18DDR3amtv+q2zRXRzQbonu5rseH8wdhODtHksLlzFhzQjG+TFeFccozHnCsyp9io80tvuO
>   akmXtOX/hb2jf0dmGxrQCF4XiZ7B8er1vryVqZhnWmhMziWVsBert8ub4=
> X-IronPort-Anti-Spam-Filtered: true
> X-IronPort-AV: E=McAfee;i="6200,9189,9973"; a="261790135"
> X-IronPort-AV: E=Sophos;i="5.82,271,1613462400";
>     d="scan'208";a="261790135"
> X-Amp-Result: SKIPPED(no attachment in message)
> X-Amp-File-Uploaded: False
> Received: from vger.kernel.org ([23.128.96.18])
>    by mtab.intel.com with ESMTP; 03 May 2021 14:36:07 -0700
> Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
>          id S229846AbhECVg4 (ORCPT <rfc822;bingbu.cao@linux.intel.com>
>          + 29 others); Mon, 3 May 2021 17:36:56 -0400
> Received: from mga18.intel.com ([134.134.136.126]:6730 "EHLO mga18.intel.com"
>          rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
>          id S229764AbhECVgw (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
>          Mon, 3 May 2021 17:36:52 -0400
> IronPort-SDR: 1csj0DEIMG/4QUsFu3BmUbvFdT7jBonCObhRz0qmqggHKb35l90RoafgGQbJRBABy2bIFj1oyz
>   QE6rWT57N1bw==
> X-IronPort-AV: E=McAfee;i="6200,9189,9973"; a="185312154"
> X-IronPort-AV: E=Sophos;i="5.82,271,1613462400";
>     d="scan'208";a="185312154"
> Received: from fmsmga008.fm.intel.com ([10.253.24.58])
>    by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 May 2021 14:35:57 -0700
> IronPort-SDR: /v7zWWgeCpbkRPEz3SQB9i9+7fEAMEScp2HIQ7R0gaTFo8YKVYigkoddcEJ3InSW9wC39KNvod
>   e9jWlQq2QaDQ==
> X-IronPort-AV: E=Sophos;i="5.82,271,1613462400";
>     d="scan'208";a="428548807"
> Received: from rhweight-mobl2.amr.corp.intel.com (HELO rhweight-mobl2.ra.intel.com) ([10.212.218.202])
>    by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 May 2021 14:35:56 -0700
> From:   Russ Weight <russell.h.weight@intel.com>
> To:     mdf@kernel.org, linux-fpga@vger.kernel.org,
>          linux-kernel@vger.kernel.org
> Cc:     trix@redhat.com, lgoncalv@redhat.com, yilun.xu@intel.com,
>          hao.wu@intel.com, matthew.gerlach@intel.com,
>          richard.gong@intel.com, Russ Weight <russell.h.weight@intel.com>
> Subject: [PATCH v12 3/7] fpga: sec-mgr: expose sec-mgr update status
> Date:   Mon,  3 May 2021 14:35:42 -0700
> Message-Id: <20210503213546.316439-4-russell.h.weight@intel.com>
> X-Mailer: git-send-email 2.25.1
> In-Reply-To: <20210503213546.316439-1-russell.h.weight@intel.com>
> References: <20210503213546.316439-1-russell.h.weight@intel.com>
> MIME-Version: 1.0
> Content-Transfer-Encoding: 8bit
> Precedence: bulk
> List-ID: <linux-kernel.vger.kernel.org>
> X-Mailing-List: linux-kernel@vger.kernel.org
> 
> Extend the FPGA Security Manager class driver to
> include an update/status sysfs node that can be polled
> and read to monitor the progress of an ongoing secure
> update. Sysfs_notify() is used to signal transitions
> between different phases of the update process.
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> ---
> v12:
>    - Updated Date and KernelVersion fields in ABI documentation
>    - Changed syntax of sec_mgr_prog_str[] array definition from:
> 	"idle",			/* FPGA_SEC_PROG_IDLE */
>      to:
> 	[FPGA_SEC_PROG_IDLE]	    = "idle",
> v11:
>    - No change
> v10:
>    - Rebased to 5.12-rc2 next
>    - Updated Date and KernelVersion in ABI documentation
> v9:
>    - Updated Date and KernelVersion in ABI documentation
> v8:
>    - No change
> v7:
>    - Changed Date in documentation file to December 2020
> v6:
>    - No change
> v5:
>    - Use new function sysfs_emit() in the status_show() function
> v4:
>    - Changed from "Intel FPGA Security Manager" to FPGA Security Manager"
>      and removed unnecessary references to "Intel".
>    - Changed: iops -> sops, imgr -> smgr, IFPGA_ -> FPGA_, ifpga_ to fpga_
> v3:
>    - Use a local variable to read progress once in status_show()
>    - Use dev_err to report invalid progress status
> v2:
>    - Bumped documentation date and version
>    - Changed progress state "read_file" to "reading"
> ---
>   .../ABI/testing/sysfs-class-fpga-sec-mgr      | 11 +++++
>   drivers/fpga/fpga-sec-mgr.c                   | 42 +++++++++++++++++--
>   2 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> index 36d1b6ba8d76..b962ad2cf18d 100644
> --- a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> @@ -16,3 +16,14 @@ Description:	Write only. Write the filename of an image
>   		BMC images, BMC firmware, Static Region images,
>   		and Root Entry Hashes, and to cancel Code Signing
>   		Keys (CSK).
> +
> +What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/status
> +Date:		June 2021
> +KernelVersion:	5.14
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read-only. Returns a string describing the current
> +		status of an update. The string will be one of the
> +		following: idle, reading, preparing, writing,
> +		programming. Userspace code can poll on this file,
> +		as it will be signaled by sysfs_notify() on each
> +		state change.
> diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
> index fe82feda6b3c..dec52c68fe16 100644
> --- a/drivers/fpga/fpga-sec-mgr.c
> +++ b/drivers/fpga/fpga-sec-mgr.c
> @@ -23,6 +23,13 @@ struct fpga_sec_mgr_devres {
>   
>   #define to_sec_mgr(d) container_of(d, struct fpga_sec_mgr, dev)
>   
> +static void update_progress(struct fpga_sec_mgr *smgr,
> +			    enum fpga_sec_prog new_progress)
> +{
> +	smgr->progress = new_progress;
> +	sysfs_notify(&smgr->dev.kobj, "update", "status");
> +}
> +
>   static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
>   			       enum fpga_sec_err err_code)
>   {
> @@ -33,7 +40,7 @@ static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
>   static void progress_complete(struct fpga_sec_mgr *smgr)
>   {
>   	mutex_lock(&smgr->lock);
> -	smgr->progress = FPGA_SEC_PROG_IDLE;
> +	update_progress(smgr, FPGA_SEC_PROG_IDLE);
>   	complete_all(&smgr->update_done);
>   	mutex_unlock(&smgr->lock);
>   }
> @@ -61,14 +68,14 @@ static void fpga_sec_mgr_update(struct work_struct *work)
>   		goto release_fw_exit;
>   	}
>   
> -	smgr->progress = FPGA_SEC_PROG_PREPARING;
> +	update_progress(smgr, FPGA_SEC_PROG_PREPARING);
>   	ret = smgr->sops->prepare(smgr);
>   	if (ret != FPGA_SEC_ERR_NONE) {
>   		fpga_sec_dev_error(smgr, ret);
>   		goto modput_exit;
>   	}
>   
> -	smgr->progress = FPGA_SEC_PROG_WRITING;
> +	update_progress(smgr, FPGA_SEC_PROG_WRITING);
>   	while (smgr->remaining_size) {
>   		ret = smgr->sops->write_blk(smgr, offset);
>   		if (ret != FPGA_SEC_ERR_NONE) {
> @@ -79,7 +86,7 @@ static void fpga_sec_mgr_update(struct work_struct *work)
>   		offset = fw->size - smgr->remaining_size;
>   	}
>   
> -	smgr->progress = FPGA_SEC_PROG_PROGRAMMING;
> +	update_progress(smgr, FPGA_SEC_PROG_PROGRAMMING);
>   	ret = smgr->sops->poll_complete(smgr);
>   	if (ret != FPGA_SEC_ERR_NONE)
>   		fpga_sec_dev_error(smgr, ret);
> @@ -107,6 +114,32 @@ static void fpga_sec_mgr_update(struct work_struct *work)
>   	progress_complete(smgr);
>   }
>   
> +static const char * const sec_mgr_prog_str[] = {
> +	[FPGA_SEC_PROG_IDLE]	    = "idle",
> +	[FPGA_SEC_PROG_READING]	    = "reading",
> +	[FPGA_SEC_PROG_PREPARING]   = "preparing",
> +	[FPGA_SEC_PROG_WRITING]	    = "writing",
> +	[FPGA_SEC_PROG_PROGRAMMING] = "programming"
> +};
> +
> +static ssize_t
> +status_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
> +	const char *status = "unknown-status";
> +	enum fpga_sec_prog progress;
> +
> +	progress = smgr->progress;
> +	if (progress < FPGA_SEC_PROG_MAX)
> +		status = sec_mgr_prog_str[progress];
> +	else
> +		dev_err(dev, "Invalid status during secure update: %d\n",
> +			progress);
> +
> +	return sysfs_emit(buf, "%s\n", status);
> +}
> +static DEVICE_ATTR_RO(status);
> +
>   static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>   			      const char *buf, size_t count)
>   {
> @@ -141,6 +174,7 @@ static DEVICE_ATTR_WO(filename);
>   
>   static struct attribute *sec_mgr_update_attrs[] = {
>   	&dev_attr_filename.attr,
> +	&dev_attr_status.attr,
>   	NULL,
>   };
>   
> 

Re: [PATCH v1 1/2] perf header: Support HYBRID_TOPOLOGY feature

[Jiri Olsa]

On Fri, Apr 30, 2021 at 03:46:01PM +0800, Jin Yao wrote:

SNIP

> +static int write_hybrid_topology(struct feat_fd *ff,
> +				 struct evlist *evlist __maybe_unused)
> +{
> +	struct hybrid_topology *tp;
> +	int ret;
> +	u32 i;
> +
> +	tp = hybrid_topology__new();
> +	if (!tp)
> +		return -1;
> +
> +	ret = do_write(ff, &tp->nr, sizeof(u32));
> +	if (ret < 0)
> +		goto err;
> +
> +	for (i = 0; i < tp->nr; i++) {
> +		struct hybrid_topology_node *n = &tp->nodes[i];
> +
> +		ret = do_write_string(ff, n->pmu_name);
> +		if (ret < 0)
> +			goto err;
> +
> +		ret = do_write_string(ff, n->cpus);
> +		if (ret < 0)
> +			goto err;
> +	}
> +
> +	ret = 0;
> +
> +err:
> +	hybrid_topology__delete(tp);
> +	return ret;
> +}
> +
>  static int write_dir_format(struct feat_fd *ff,
>  			    struct evlist *evlist __maybe_unused)
>  {
> @@ -1623,6 +1657,19 @@ static void print_clock_data(struct feat_fd *ff, FILE *fp)
>  		    clockid_name(clockid));
>  }
>  
> +static void print_hybrid_topology(struct feat_fd *ff, FILE *fp)
> +{
> +	int i;
> +	struct hybrid_node *n;
> +
> +	for (i = 0; i < ff->ph->env.nr_hybrid_nodes; i++) {
> +		n = &ff->ph->env.hybrid_nodes[i];
> +
> +		fprintf(fp, "# %s cpu list : ", n->pmu_name);
> +		cpu_map__fprintf(n->map, fp);

do you plan to do anything else with n->map in the future?
because right now you could just print the stored cpus string no?
it should be already in the cpumask shape

jirka

Re: [PATCH v1 1/2] perf header: Support HYBRID_TOPOLOGY feature

[Jiri Olsa]

On Fri, Apr 30, 2021 at 03:46:01PM +0800, Jin Yao wrote:
> It would be useful to let user know the hybrid topology.
> Adding HYBRID_TOPOLOGY feature in header to indicate the
> core cpus and the atom cpus.
> 
> With this patch,
> 
> For the perf.data generated on hybrid platform,
> reports the hybrid cpu list.
> 
>   root@otcpl-adl-s-2:~# perf report --header-only -I
>   ...
>   # cpu_core cpu list : 0-15
>   # cpu_atom cpu list : 16-23

hum, should we print 'hybrid:' or something to make
sure its not confused with something else? like

  # hybrid cpu_core cpu list : 0-15
  # hybrid cpu_atom cpu list : 16-23

jirka

Re: [PATCH v1 2/2] perf header: Support hybrid CPU_PMU_CAPS

[Jiri Olsa]

On Fri, Apr 30, 2021 at 03:46:02PM +0800, Jin Yao wrote:
> On hybrid platform, it may have several cpu pmus, such as,
> "cpu_core" and "cpu_atom". The CPU_PMU_CAPS feature in perf
> header needs to be improved to support multiple cpu pmus.
> 
> The new layout in header is defined as:
> 
> <nr_caps>
> <caps string>
> <caps string>
> <pmu name>
> <nr of rest pmus>

not sure why is the 'nr of rest pmus' needed

the current format is:

        u32 nr_cpu_pmu_caps;
        {
                char    name[];
                char    value[];
        } [nr_cpu_pmu_caps]


I guess we could extend it to:

        u32 nr_cpu_pmu_caps;
        {
                char    name[];
                char    value[];
        } [nr_cpu_pmu_caps]
	char pmu_name[]

        u32 nr_cpu_pmu_caps;
        {
                char    name[];
                char    value[];
        } [nr_cpu_pmu_caps]
	char pmu_name[]

	...

and we could detect the old format by checking that there's no
pmu name.. but maybe I'm missing something, I did not check deeply,
please let me know

also would be great to move the format change and storing hybrid
pmus in separate patches

thanks,
jirka

Re: [PATCH v1 1/2] perf header: Support HYBRID_TOPOLOGY feature

[Arnaldo Carvalho de Melo]

Em Tue, May 04, 2021 at 04:56:44PM +0200, Jiri Olsa escreveu:
> On Fri, Apr 30, 2021 at 03:46:01PM +0800, Jin Yao wrote:
> > It would be useful to let user know the hybrid topology.
> > Adding HYBRID_TOPOLOGY feature in header to indicate the
> > core cpus and the atom cpus.

> > With this patch,

> > For the perf.data generated on hybrid platform,
> > reports the hybrid cpu list.

> >   root@otcpl-adl-s-2:~# perf report --header-only -I
> >   ...
> >   # cpu_core cpu list : 0-15
> >   # cpu_atom cpu list : 16-23

> hum, should we print 'hybrid:' or something to make
> sure its not confused with something else? like
 
>   # hybrid cpu_core cpu list : 0-15
>   # hybrid cpu_atom cpu list : 16-23

But this _core/_atom already got to be enough? I disagreed with that
naming, but neverthless having one or the other present in an output is
a clear mark of this hybrid topology.

I.e having that extra hybrid string that wouldn't add information to the
output.

IMHO.

- Arnaldo

Re: [PATCH v1 1/2] perf header: Support HYBRID_TOPOLOGY feature

[Jiri Olsa]

On Tue, May 04, 2021 at 04:28:33PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, May 04, 2021 at 04:56:44PM +0200, Jiri Olsa escreveu:
> > On Fri, Apr 30, 2021 at 03:46:01PM +0800, Jin Yao wrote:
> > > It would be useful to let user know the hybrid topology.
> > > Adding HYBRID_TOPOLOGY feature in header to indicate the
> > > core cpus and the atom cpus.
> 
> > > With this patch,
> 
> > > For the perf.data generated on hybrid platform,
> > > reports the hybrid cpu list.
> 
> > >   root@otcpl-adl-s-2:~# perf report --header-only -I
> > >   ...
> > >   # cpu_core cpu list : 0-15
> > >   # cpu_atom cpu list : 16-23
> 
> > hum, should we print 'hybrid:' or something to make
> > sure its not confused with something else? like
>  
> >   # hybrid cpu_core cpu list : 0-15
> >   # hybrid cpu_atom cpu list : 16-23
> 
> But this _core/_atom already got to be enough? I disagreed with that
> naming, but neverthless having one or the other present in an output is
> a clear mark of this hybrid topology.
> 
> I.e having that extra hybrid string that wouldn't add information to the
> output.

sure when you know that cpu_core/cpu_atom are hybrid pmus ;-)
and I guess other arch will come with other names 

jirka

> 
> IMHO.
> 
> - Arnaldo
> 

Re: [PATCH v1 1/2] perf header: Support HYBRID_TOPOLOGY feature

[Arnaldo Carvalho de Melo]

Em Tue, May 04, 2021 at 09:37:34PM +0200, Jiri Olsa escreveu:
> On Tue, May 04, 2021 at 04:28:33PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, May 04, 2021 at 04:56:44PM +0200, Jiri Olsa escreveu:
> > > On Fri, Apr 30, 2021 at 03:46:01PM +0800, Jin Yao wrote:
> > > > It would be useful to let user know the hybrid topology.
> > > > Adding HYBRID_TOPOLOGY feature in header to indicate the
> > > > core cpus and the atom cpus.
> > 
> > > > With this patch,
> > 
> > > > For the perf.data generated on hybrid platform,
> > > > reports the hybrid cpu list.
> > 
> > > >   root@otcpl-adl-s-2:~# perf report --header-only -I
> > > >   ...
> > > >   # cpu_core cpu list : 0-15
> > > >   # cpu_atom cpu list : 16-23
> > 
> > > hum, should we print 'hybrid:' or something to make
> > > sure its not confused with something else? like
> >  
> > >   # hybrid cpu_core cpu list : 0-15
> > >   # hybrid cpu_atom cpu list : 16-23
> > 
> > But this _core/_atom already got to be enough? I disagreed with that
> > naming, but neverthless having one or the other present in an output is
> > a clear mark of this hybrid topology.
> > 
> > I.e having that extra hybrid string that wouldn't add information to the
> > output.
> 
> sure when you know that cpu_core/cpu_atom are hybrid pmus ;-)
> and I guess other arch will come with other names 

Yeah, its too Intel centric, I thought they would come up with
cpu_big/cpu_little and map it to core/atom on Intel and whatever other
BIG/little arches come up with.

Perhaps:

root@otcpl-adl-s-2:~# perf report --header-only -I
...
# hybrid cpu system:
# cpu_core cpu list : 0-15
# cpu_atom cpu list : 16-23

?

- Arnaldo

Re: [PATCH v1 1/2] perf header: Support HYBRID_TOPOLOGY feature

[Jiri Olsa]

On Wed, May 05, 2021 at 10:49:23AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, May 04, 2021 at 09:37:34PM +0200, Jiri Olsa escreveu:
> > On Tue, May 04, 2021 at 04:28:33PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, May 04, 2021 at 04:56:44PM +0200, Jiri Olsa escreveu:
> > > > On Fri, Apr 30, 2021 at 03:46:01PM +0800, Jin Yao wrote:
> > > > > It would be useful to let user know the hybrid topology.
> > > > > Adding HYBRID_TOPOLOGY feature in header to indicate the
> > > > > core cpus and the atom cpus.
> > > 
> > > > > With this patch,
> > > 
> > > > > For the perf.data generated on hybrid platform,
> > > > > reports the hybrid cpu list.
> > > 
> > > > >   root@otcpl-adl-s-2:~# perf report --header-only -I
> > > > >   ...
> > > > >   # cpu_core cpu list : 0-15
> > > > >   # cpu_atom cpu list : 16-23
> > > 
> > > > hum, should we print 'hybrid:' or something to make
> > > > sure its not confused with something else? like
> > >  
> > > >   # hybrid cpu_core cpu list : 0-15
> > > >   # hybrid cpu_atom cpu list : 16-23
> > > 
> > > But this _core/_atom already got to be enough? I disagreed with that
> > > naming, but neverthless having one or the other present in an output is
> > > a clear mark of this hybrid topology.
> > > 
> > > I.e having that extra hybrid string that wouldn't add information to the
> > > output.
> > 
> > sure when you know that cpu_core/cpu_atom are hybrid pmus ;-)
> > and I guess other arch will come with other names 
> 
> Yeah, its too Intel centric, I thought they would come up with
> cpu_big/cpu_little and map it to core/atom on Intel and whatever other
> BIG/little arches come up with.
> 
> Perhaps:
> 
> root@otcpl-adl-s-2:~# perf report --header-only -I
> ...
> # hybrid cpu system:
> # cpu_core cpu list : 0-15
> # cpu_atom cpu list : 16-23
> 
> ?

'hybrid pmus' would sounds better to me,
but as long as there's hybrid in there I'm good ;-)

thanks,
jirka

> 
> - Arnaldo
> 

Re: [PATCH v1 1/2] perf header: Support HYBRID_TOPOLOGY feature

[Jin, Yao]

Hi Jiri,

On 5/4/2021 10:54 PM, Jiri Olsa wrote:
> On Fri, Apr 30, 2021 at 03:46:01PM +0800, Jin Yao wrote:
> 
> SNIP
> 
>> +static int write_hybrid_topology(struct feat_fd *ff,
>> +				 struct evlist *evlist __maybe_unused)
>> +{
>> +	struct hybrid_topology *tp;
>> +	int ret;
>> +	u32 i;
>> +
>> +	tp = hybrid_topology__new();
>> +	if (!tp)
>> +		return -1;
>> +
>> +	ret = do_write(ff, &tp->nr, sizeof(u32));
>> +	if (ret < 0)
>> +		goto err;
>> +
>> +	for (i = 0; i < tp->nr; i++) {
>> +		struct hybrid_topology_node *n = &tp->nodes[i];
>> +
>> +		ret = do_write_string(ff, n->pmu_name);
>> +		if (ret < 0)
>> +			goto err;
>> +
>> +		ret = do_write_string(ff, n->cpus);
>> +		if (ret < 0)
>> +			goto err;
>> +	}
>> +
>> +	ret = 0;
>> +
>> +err:
>> +	hybrid_topology__delete(tp);
>> +	return ret;
>> +}
>> +
>>   static int write_dir_format(struct feat_fd *ff,
>>   			    struct evlist *evlist __maybe_unused)
>>   {
>> @@ -1623,6 +1657,19 @@ static void print_clock_data(struct feat_fd *ff, FILE *fp)
>>   		    clockid_name(clockid));
>>   }
>>   
>> +static void print_hybrid_topology(struct feat_fd *ff, FILE *fp)
>> +{
>> +	int i;
>> +	struct hybrid_node *n;
>> +
>> +	for (i = 0; i < ff->ph->env.nr_hybrid_nodes; i++) {
>> +		n = &ff->ph->env.hybrid_nodes[i];
>> +
>> +		fprintf(fp, "# %s cpu list : ", n->pmu_name);
>> +		cpu_map__fprintf(n->map, fp);
> 
> do you plan to do anything else with n->map in the future?
> because right now you could just print the stored cpus string no?
> it should be already in the cpumask shape
> 
> jirka
> 

Yes, you are right, we don't need to use n->map at least now.

Following code should be much simpler.

+struct hybrid_node {
+       char    *pmu_name;
+       char    *cpus;
+};

+static int process_hybrid_topology(struct feat_fd *ff,
+                                  void *data __maybe_unused)
+{
+       struct hybrid_node *nodes, *n;
+       u32 nr, i;
+
+       /* nr nodes */
+       if (do_read_u32(ff, &nr))
+               return -1;
+
+       nodes = zalloc(sizeof(*nodes) * nr);
+       if (!nodes)
+               return -ENOMEM;
+
+       for (i = 0; i < nr; i++) {
+               n = &nodes[i];
+
+               n->pmu_name = do_read_string(ff);
+               if (!n->pmu_name)
+                       goto error;
+
+               n->cpus = do_read_string(ff);
+               if (!n->cpus)
+                       goto error;
+       }
+
+       ff->ph->env.nr_hybrid_nodes = nr;
+       ff->ph->env.hybrid_nodes = nodes;
+       return 0;
+
+error:
...

+static void print_hybrid_topology(struct feat_fd *ff, FILE *fp)
+{
+       int i;
+       struct hybrid_node *n;
+
+       for (i = 0; i < ff->ph->env.nr_hybrid_nodes; i++) {
+               n = &ff->ph->env.hybrid_nodes[i];
+               fprintf(fp, "# %s cpu list : %s\n", n->pmu_name, n->cpus);
+       }
+}

Thanks
Jin Yao

Re: [PATCH v1 1/2] perf header: Support HYBRID_TOPOLOGY feature

[Jin, Yao]

Hi Arnaldo,

On 5/5/2021 9:49 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, May 04, 2021 at 09:37:34PM +0200, Jiri Olsa escreveu:
>> On Tue, May 04, 2021 at 04:28:33PM -0300, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, May 04, 2021 at 04:56:44PM +0200, Jiri Olsa escreveu:
>>>> On Fri, Apr 30, 2021 at 03:46:01PM +0800, Jin Yao wrote:
>>>>> It would be useful to let user know the hybrid topology.
>>>>> Adding HYBRID_TOPOLOGY feature in header to indicate the
>>>>> core cpus and the atom cpus.
>>>
>>>>> With this patch,
>>>
>>>>> For the perf.data generated on hybrid platform,
>>>>> reports the hybrid cpu list.
>>>
>>>>>    root@otcpl-adl-s-2:~# perf report --header-only -I
>>>>>    ...
>>>>>    # cpu_core cpu list : 0-15
>>>>>    # cpu_atom cpu list : 16-23
>>>
>>>> hum, should we print 'hybrid:' or something to make
>>>> sure its not confused with something else? like
>>>   
>>>>    # hybrid cpu_core cpu list : 0-15
>>>>    # hybrid cpu_atom cpu list : 16-23
>>>
>>> But this _core/_atom already got to be enough? I disagreed with that
>>> naming, but neverthless having one or the other present in an output is
>>> a clear mark of this hybrid topology.
>>>
>>> I.e having that extra hybrid string that wouldn't add information to the
>>> output.
>>
>> sure when you know that cpu_core/cpu_atom are hybrid pmus ;-)
>> and I guess other arch will come with other names
> 
> Yeah, its too Intel centric, I thought they would come up with
> cpu_big/cpu_little and map it to core/atom on Intel and whatever other
> BIG/little arches come up with.
> 
> Perhaps:
> 
> root@otcpl-adl-s-2:~# perf report --header-only -I
> ...
> # hybrid cpu system:
> # cpu_core cpu list : 0-15
> # cpu_atom cpu list : 16-23
> 
> ?
>

That's a good idea! :)

On my ADL test machine, it's now output like:

# hybrid cpu system:
# cpu_core cpu list : 0-15
# cpu_atom cpu list : 16-23

Thanks
Jin Yao

> - Arnaldo
> 

Re: [PATCH v1 1/2] perf header: Support HYBRID_TOPOLOGY feature

[Jin, Yao]

Hi Jiri,

On 5/6/2021 4:28 AM, Jiri Olsa wrote:
> On Wed, May 05, 2021 at 10:49:23AM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Tue, May 04, 2021 at 09:37:34PM +0200, Jiri Olsa escreveu:
>>> On Tue, May 04, 2021 at 04:28:33PM -0300, Arnaldo Carvalho de Melo wrote:
>>>> Em Tue, May 04, 2021 at 04:56:44PM +0200, Jiri Olsa escreveu:
>>>>> On Fri, Apr 30, 2021 at 03:46:01PM +0800, Jin Yao wrote:
>>>>>> It would be useful to let user know the hybrid topology.
>>>>>> Adding HYBRID_TOPOLOGY feature in header to indicate the
>>>>>> core cpus and the atom cpus.
>>>>
>>>>>> With this patch,
>>>>
>>>>>> For the perf.data generated on hybrid platform,
>>>>>> reports the hybrid cpu list.
>>>>
>>>>>>    root@otcpl-adl-s-2:~# perf report --header-only -I
>>>>>>    ...
>>>>>>    # cpu_core cpu list : 0-15
>>>>>>    # cpu_atom cpu list : 16-23
>>>>
>>>>> hum, should we print 'hybrid:' or something to make
>>>>> sure its not confused with something else? like
>>>>   
>>>>>    # hybrid cpu_core cpu list : 0-15
>>>>>    # hybrid cpu_atom cpu list : 16-23
>>>>
>>>> But this _core/_atom already got to be enough? I disagreed with that
>>>> naming, but neverthless having one or the other present in an output is
>>>> a clear mark of this hybrid topology.
>>>>
>>>> I.e having that extra hybrid string that wouldn't add information to the
>>>> output.
>>>
>>> sure when you know that cpu_core/cpu_atom are hybrid pmus ;-)
>>> and I guess other arch will come with other names
>>
>> Yeah, its too Intel centric, I thought they would come up with
>> cpu_big/cpu_little and map it to core/atom on Intel and whatever other
>> BIG/little arches come up with.
>>
>> Perhaps:
>>
>> root@otcpl-adl-s-2:~# perf report --header-only -I
>> ...
>> # hybrid cpu system:
>> # cpu_core cpu list : 0-15
>> # cpu_atom cpu list : 16-23
>>
>> ?
> 
> 'hybrid pmus' would sounds better to me,
> but as long as there's hybrid in there I'm good ;-)
> 
> thanks,
> jirka
> 

:)

Thanks
Jin Yao

>>
>> - Arnaldo
>>
> 

Re: [PATCH v1 2/2] perf header: Support hybrid CPU_PMU_CAPS

[Jin, Yao]

Hi Jiri,

On 5/4/2021 11:07 PM, Jiri Olsa wrote:
> On Fri, Apr 30, 2021 at 03:46:02PM +0800, Jin Yao wrote:
>> On hybrid platform, it may have several cpu pmus, such as,
>> "cpu_core" and "cpu_atom". The CPU_PMU_CAPS feature in perf
>> header needs to be improved to support multiple cpu pmus.
>>
>> The new layout in header is defined as:
>>
>> <nr_caps>
>> <caps string>
>> <caps string>
>> <pmu name>
>> <nr of rest pmus>
> 
> not sure why is the 'nr of rest pmus' needed
> 

The 'nr of rest pmus' indicates the remaining pmus which are waiting for process.

For example,

<nr_caps>
<caps string>
"cpu_core"
1
<nr_caps>
<caps string>
"cpu_atom"
0

When we see '0' in data file processing, we know all the pmu have been processed yet.

> the current format is:
> 
>          u32 nr_cpu_pmu_caps;
>          {
>                  char    name[];
>                  char    value[];
>          } [nr_cpu_pmu_caps]
> 
> 
> I guess we could extend it to:
> 
>          u32 nr_cpu_pmu_caps;
>          {
>                  char    name[];
>                  char    value[];
>          } [nr_cpu_pmu_caps]
> 	char pmu_name[]
> 
>          u32 nr_cpu_pmu_caps;
>          {
>                  char    name[];
>                  char    value[];
>          } [nr_cpu_pmu_caps]
> 	char pmu_name[]
> 
> 	...
> 
> and we could detect the old format by checking that there's no
> pmu name.. but maybe I'm missing something, I did not check deeply,
> please let me know
>

Actually we do the same thing, but I just add an extra 'nr of rest pmus' after the pmu_name. The 
purpose of 'nr of rest pmus' is when we see '0' at 'nr of rest pmus', we know that all pmus have 
been processed.

Otherwise, we have to continue reading data file till we find something incorrect and then finally 
drop the last read data.

So is this solution acceptable?

> also would be great to move the format change and storing hybrid
> pmus in separate patches
> 

Maybe we have to put the storing and processing into one patch.

Say patch 1 contains the format change and storing hybrid pmus. And patch 2 contains the processing 
for the new format. If the repo only contains the patch 1, I'm afraid that may introduce the 
compatible issue.

Thanks
Jin Yao

> thanks,
> jirka
> 

Re: [PATCH v1 2/2] perf header: Support hybrid CPU_PMU_CAPS

[Jiri Olsa]

On Thu, May 06, 2021 at 12:59:08PM +0800, Jin, Yao wrote:
> Hi Jiri,
> 
> On 5/4/2021 11:07 PM, Jiri Olsa wrote:
> > On Fri, Apr 30, 2021 at 03:46:02PM +0800, Jin Yao wrote:
> > > On hybrid platform, it may have several cpu pmus, such as,
> > > "cpu_core" and "cpu_atom". The CPU_PMU_CAPS feature in perf
> > > header needs to be improved to support multiple cpu pmus.
> > > 
> > > The new layout in header is defined as:
> > > 
> > > <nr_caps>
> > > <caps string>
> > > <caps string>
> > > <pmu name>
> > > <nr of rest pmus>
> > 
> > not sure why is the 'nr of rest pmus' needed
> > 
> 
> The 'nr of rest pmus' indicates the remaining pmus which are waiting for process.
> 
> For example,
> 
> <nr_caps>
> <caps string>
> "cpu_core"
> 1
> <nr_caps>
> <caps string>
> "cpu_atom"
> 0
> 
> When we see '0' in data file processing, we know all the pmu have been processed yet.
> 
> > the current format is:
> > 
> >          u32 nr_cpu_pmu_caps;
> >          {
> >                  char    name[];
> >                  char    value[];
> >          } [nr_cpu_pmu_caps]
> > 
> > 
> > I guess we could extend it to:
> > 
> >          u32 nr_cpu_pmu_caps;
> >          {
> >                  char    name[];
> >                  char    value[];
> >          } [nr_cpu_pmu_caps]
> > 	char pmu_name[]
> > 
> >          u32 nr_cpu_pmu_caps;
> >          {
> >                  char    name[];
> >                  char    value[];
> >          } [nr_cpu_pmu_caps]
> > 	char pmu_name[]
> > 
> > 	...
> > 
> > and we could detect the old format by checking that there's no
> > pmu name.. but maybe I'm missing something, I did not check deeply,
> > please let me know
> > 
> 
> Actually we do the same thing, but I just add an extra 'nr of rest pmus'
> after the pmu_name. The purpose of 'nr of rest pmus' is when we see '0' at
> 'nr of rest pmus', we know that all pmus have been processed.
> 
> Otherwise, we have to continue reading data file till we find something
> incorrect and then finally drop the last read data.

you have the size of the feature data right? I think we use
it in other cases to check if there are more data

> 
> So is this solution acceptable?
> 
> > also would be great to move the format change and storing hybrid
> > pmus in separate patches
> > 
> 
> Maybe we have to put the storing and processing into one patch.
> 
> Say patch 1 contains the format change and storing hybrid pmus. And patch 2
> contains the processing for the new format. If the repo only contains the
> patch 1, I'm afraid that may introduce the compatible issue.

maybe you can have change of caps format in one patch
and storing/processing hybrid pmus in another?

jirka

Re: [PATCH v1 2/2] perf header: Support hybrid CPU_PMU_CAPS

[Jin, Yao]

Hi Jiri,

On 5/6/2021 9:22 PM, Jiri Olsa wrote:
> On Thu, May 06, 2021 at 12:59:08PM +0800, Jin, Yao wrote:
>> Hi Jiri,
>>
>> On 5/4/2021 11:07 PM, Jiri Olsa wrote:
>>> On Fri, Apr 30, 2021 at 03:46:02PM +0800, Jin Yao wrote:
>>>> On hybrid platform, it may have several cpu pmus, such as,
>>>> "cpu_core" and "cpu_atom". The CPU_PMU_CAPS feature in perf
>>>> header needs to be improved to support multiple cpu pmus.
>>>>
>>>> The new layout in header is defined as:
>>>>
>>>> <nr_caps>
>>>> <caps string>
>>>> <caps string>
>>>> <pmu name>
>>>> <nr of rest pmus>
>>>
>>> not sure why is the 'nr of rest pmus' needed
>>>
>>
>> The 'nr of rest pmus' indicates the remaining pmus which are waiting for process.
>>
>> For example,
>>
>> <nr_caps>
>> <caps string>
>> "cpu_core"
>> 1
>> <nr_caps>
>> <caps string>
>> "cpu_atom"
>> 0
>>
>> When we see '0' in data file processing, we know all the pmu have been processed yet.
>>
>>> the current format is:
>>>
>>>           u32 nr_cpu_pmu_caps;
>>>           {
>>>                   char    name[];
>>>                   char    value[];
>>>           } [nr_cpu_pmu_caps]
>>>
>>>
>>> I guess we could extend it to:
>>>
>>>           u32 nr_cpu_pmu_caps;
>>>           {
>>>                   char    name[];
>>>                   char    value[];
>>>           } [nr_cpu_pmu_caps]
>>> 	char pmu_name[]
>>>
>>>           u32 nr_cpu_pmu_caps;
>>>           {
>>>                   char    name[];
>>>                   char    value[];
>>>           } [nr_cpu_pmu_caps]
>>> 	char pmu_name[]
>>>
>>> 	...
>>>
>>> and we could detect the old format by checking that there's no
>>> pmu name.. but maybe I'm missing something, I did not check deeply,
>>> please let me know
>>>
>>
>> Actually we do the same thing, but I just add an extra 'nr of rest pmus'
>> after the pmu_name. The purpose of 'nr of rest pmus' is when we see '0' at
>> 'nr of rest pmus', we know that all pmus have been processed.
>>
>> Otherwise, we have to continue reading data file till we find something
>> incorrect and then finally drop the last read data.
> 
> you have the size of the feature data right? I think we use
> it in other cases to check if there are more data
> 

The challenge for us is if we need to compatible with the old perf.data which was generated by old 
perf tool.

For the old perf.data, the layout in header is:

nr of caps
caps string 1
caps string 2
...
caps string N

It doesn't carry with any other fields such as size of caps data.

To be compatible with old perf.data, so I have to extend the layout to:

nr of caps for pmu 1
caps string 1
caps string 2
...
caps string N
name of pmu 1
nr of rest pmus

nr of caps for pmu2
caps string 1
caps string 2
...
caps string N
name of pmu 2
nr of rest pmus

When the new perf tool detects the string such as "cpu_", it can know that it's the pmu name field 
in new perf.data, otherwise it's the old perf.data.

If we add new field such as "size" to the layout, I'm afraid the new perf tool can not process the 
old perf.data correctly.

If we don't need to support old perf.data, that makes things easy.

>>
>> So is this solution acceptable?
>>
>>> also would be great to move the format change and storing hybrid
>>> pmus in separate patches
>>>
>>
>> Maybe we have to put the storing and processing into one patch.
>>
>> Say patch 1 contains the format change and storing hybrid pmus. And patch 2
>> contains the processing for the new format. If the repo only contains the
>> patch 1, I'm afraid that may introduce the compatible issue.
> 
> maybe you can have change of caps format in one patch
> and storing/processing hybrid pmus in another?
> 

But there is no data structure defined in header.h for each feature.

It directly uses do_write/do_write_string in 'write()' ops to write the feature data.

So for the new layout, as I mentioned above, if we change the layout to

nr of caps for pmu 1
caps string 1
caps string 2
...
caps string N
"cpu"
0

We need to call do_write/do_write_string, actually it's the storing method. So I don't understand 
well for having changes of caps format in one patch, I'm sorry about that. :(

Thanks
Jin Yao

> jirka
> 

Re: [PATCH v1 2/2] perf header: Support hybrid CPU_PMU_CAPS

[Jiri Olsa]

On Thu, May 06, 2021 at 10:43:39PM +0800, Jin, Yao wrote:

SNIP

> > > 'nr of rest pmus', we know that all pmus have been processed.
> > > 
> > > Otherwise, we have to continue reading data file till we find something
> > > incorrect and then finally drop the last read data.
> > 
> > you have the size of the feature data right? I think we use
> > it in other cases to check if there are more data
> > 
> 
> The challenge for us is if we need to compatible with the old perf.data
> which was generated by old perf tool.
> 
> For the old perf.data, the layout in header is:
> 
> nr of caps
> caps string 1
> caps string 2
> ...
> caps string N
> 
> It doesn't carry with any other fields such as size of caps data.
> 
> To be compatible with old perf.data, so I have to extend the layout to:
> 
> nr of caps for pmu 1
> caps string 1
> caps string 2
> ...
> caps string N
> name of pmu 1
> nr of rest pmus
> 
> nr of caps for pmu2
> caps string 1
> caps string 2
> ...
> caps string N
> name of pmu 2
> nr of rest pmus
> 
> When the new perf tool detects the string such as "cpu_", it can know that
> it's the pmu name field in new perf.data, otherwise it's the old perf.data.

what if the cap string starts with 'cpu_' ?

I think it might be better to create new feature that
stores caps for multiple pmus in generic way

> 
> If we add new field such as "size" to the layout, I'm afraid the new perf
> tool can not process the old perf.data correctly.
> 
> If we don't need to support old perf.data, that makes things easy.

we need to

jirka

Re: [PATCH v1 2/2] perf header: Support hybrid CPU_PMU_CAPS

[Jin, Yao]

Hi Jiri,

On 5/10/2021 9:11 PM, Jiri Olsa wrote:
> On Thu, May 06, 2021 at 10:43:39PM +0800, Jin, Yao wrote:
> 
> SNIP
> 
>>>> 'nr of rest pmus', we know that all pmus have been processed.
>>>>
>>>> Otherwise, we have to continue reading data file till we find something
>>>> incorrect and then finally drop the last read data.
>>>
>>> you have the size of the feature data right? I think we use
>>> it in other cases to check if there are more data
>>>
>>
>> The challenge for us is if we need to compatible with the old perf.data
>> which was generated by old perf tool.
>>
>> For the old perf.data, the layout in header is:
>>
>> nr of caps
>> caps string 1
>> caps string 2
>> ...
>> caps string N
>>
>> It doesn't carry with any other fields such as size of caps data.
>>
>> To be compatible with old perf.data, so I have to extend the layout to:
>>
>> nr of caps for pmu 1
>> caps string 1
>> caps string 2
>> ...
>> caps string N
>> name of pmu 1
>> nr of rest pmus
>>
>> nr of caps for pmu2
>> caps string 1
>> caps string 2
>> ...
>> caps string N
>> name of pmu 2
>> nr of rest pmus
>>
>> When the new perf tool detects the string such as "cpu_", it can know that
>> it's the pmu name field in new perf.data, otherwise it's the old perf.data.
> 
> what if the cap string starts with 'cpu_' ?
> 

I just assume the cap string would not have 'cpu_' string. Yes, I agree, that's not a very good 
solution.

> I think it might be better to create new feature that
> stores caps for multiple pmus in generic way
>

Yes, creating a new feature in header (such as "HYBRID_CPU_PMU_CAPS") is a better way.

>>
>> If we add new field such as "size" to the layout, I'm afraid the new perf
>> tool can not process the old perf.data correctly.
>>
>> If we don't need to support old perf.data, that makes things easy.
> 
> we need to
> 

Yes, we need to. I'm now preparing v3.

Thanks
Jin Yao

> jirka
>