Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1643788 > unrolled thread

[PATCH 7/7] perf tools: add feature header record to pipe-mode

Started byDavid Carrillo-Cisneros <davidcc@google.com>
First post2017-05-18 06:20 +0200
Last post2017-05-18 20:30 +0200
Articles 9 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 7/7] perf tools: add feature header record to pipe-mode David Carrillo-Cisneros <davidcc@google.com> - 2017-05-18 06:20 +0200
    Re: [PATCH 7/7] perf tools: add feature header record to pipe-mode Jiri Olsa <jolsa@redhat.com> - 2017-05-18 18:20 +0200
      Re: [PATCH 7/7] perf tools: add feature header record to pipe-mode David Carrillo-Cisneros <davidcc@google.com> - 2017-05-18 20:40 +0200
    Re: [PATCH 7/7] perf tools: add feature header record to pipe-mode Jiri Olsa <jolsa@redhat.com> - 2017-05-18 18:20 +0200
      Re: [PATCH 7/7] perf tools: add feature header record to pipe-mode David Carrillo-Cisneros <davidcc@google.com> - 2017-05-18 20:30 +0200
    Re: [PATCH 7/7] perf tools: add feature header record to pipe-mode Jiri Olsa <jolsa@redhat.com> - 2017-05-18 18:20 +0200
    Re: [PATCH 7/7] perf tools: add feature header record to pipe-mode Jiri Olsa <jolsa@redhat.com> - 2017-05-18 18:20 +0200
    Re: [PATCH 7/7] perf tools: add feature header record to pipe-mode Jiri Olsa <jolsa@redhat.com> - 2017-05-18 18:20 +0200
      Re: [PATCH 7/7] perf tools: add feature header record to pipe-mode David Carrillo-Cisneros <davidcc@google.com> - 2017-05-18 20:30 +0200

#1643788 — [PATCH 7/7] perf tools: add feature header record to pipe-mode

FromDavid Carrillo-Cisneros <davidcc@google.com>
Date2017-05-18 06:20 +0200
Subject[PATCH 7/7] perf tools: add feature header record to pipe-mode
Message-ID<tIl4S-4Ep-19@gated-at.bofh.it>
Add header record types to pipe-mode, reusing the functions
used in file-mode and leveraging the new struct feat_fd.

Add the perf_event__synthesize_feature event call back to
process the new header records.

Before this patch:

  $ perf record -o - -e cycles -c 100000 sleep 1 | perf report --stdio
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.000 MB - ]
  ...

After this patch:
  $ perf record -o - -e cycles -c 100000 sleep 1 | perf report --stdio
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.000 MB - ]
  # hostname : lphh7
  # os release : 4.11.0-dbx-up_perf
  # perf version : 4.11.rc6.g6277c80
  # arch : x86_64
  # nrcpus online : 72
  # nrcpus avail : 72
  # cpudesc : Intel(R) Xeon(R) CPU E5-2696 v3 @ 2.30GHz
  # cpuid : GenuineIntel,6,63,2
  # total memory : 263457192 kB
  # cmdline : /root/perf record -o - -e cycles -c 100000 sleep 1
  # HEADER_CPU_TOPOLOGY info available, use -I to display
  # HEADER_NUMA_TOPOLOGY info available, use -I to display
  # pmu mappings: intel_bts = 6, uncore_imc_4 = 22, uncore_sbox_1 = 47, uncore_cbox_5 = 33, uncore_ha_0 = 16, uncore_cbox
   Percent |      Source code & Disassembly of kcore for cycles (9 samples)
  ...

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 tools/perf/builtin-annotate.c |   1 +
 tools/perf/builtin-inject.c   |   1 +
 tools/perf/builtin-record.c   |   7 ++
 tools/perf/builtin-report.c   |   1 +
 tools/perf/util/event.c       |  13 +++
 tools/perf/util/event.h       |  19 ++++
 tools/perf/util/header.c      | 224 ++++++++++++++++++++++++++++++++++--------
 tools/perf/util/header.h      |   9 ++
 tools/perf/util/session.c     |  12 +++
 tools/perf/util/tool.h        |   3 +-
 10 files changed, 248 insertions(+), 42 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index ce44edc30c71..ffe28002dc4f 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -398,6 +398,7 @@ int cmd_annotate(int argc, const char **argv)
 			.attr	= perf_event__process_attr,
 			.build_id = perf_event__process_build_id,
 			.tracing_data   = perf_event__process_tracing_data,
+			.feature	= perf_event__process_feature,
 			.ordered_events = true,
 			.ordering_requires_timestamps = true,
 		},
diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index ea8db38eedd1..2b8032908fb2 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -770,6 +770,7 @@ int cmd_inject(int argc, const char **argv)
 			.finished_round	= perf_event__repipe_oe_synth,
 			.build_id	= perf_event__repipe_op2_synth,
 			.id_index	= perf_event__repipe_op2_synth,
+			.feature	= perf_event__repipe_op2_synth,
 		},
 		.input_name  = "-",
 		.samples = LIST_HEAD_INIT(inject.samples),
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ee7d0a82ccd0..a1bcb72b4195 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -799,6 +799,13 @@ static int record__synthesize(struct record *rec, bool tail)
 		return 0;
 
 	if (file->is_pipe) {
+		err = perf_event__synthesize_features(
+			tool, session, rec->evlist, process_synthesized_event);
+		if (err < 0) {
+			pr_err("Couldn't synthesize features.\n");
+			return err;
+		}
+
 		err = perf_event__synthesize_attrs(tool, session,
 						   process_synthesized_event);
 		if (err < 0) {
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 08c90d65a252..5a6917410469 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -717,6 +717,7 @@ int cmd_report(int argc, const char **argv)
 			.id_index	 = perf_event__process_id_index,
 			.auxtrace_info	 = perf_event__process_auxtrace_info,
 			.auxtrace	 = perf_event__process_auxtrace,
+			.feature	 = perf_event__process_feature,
 			.ordered_events	 = true,
 			.ordering_requires_timestamps = true,
 		},
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 142835c0ca0a..cef1322d2993 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -56,6 +56,19 @@ static const char *perf_event__names[] = {
 	[PERF_RECORD_STAT_ROUND]		= "STAT_ROUND",
 	[PERF_RECORD_EVENT_UPDATE]		= "EVENT_UPDATE",
 	[PERF_RECORD_TIME_CONV]			= "TIME_CONV",
+	[PERF_RECORD_HEADER_HOSTNAME]		= "HOSTNAME",
+	[PERF_RECORD_HEADER_OSRELEASE]		= "OSRELEASE",
+	[PERF_RECORD_HEADER_VERSION]		= "VERSION",
+	[PERF_RECORD_HEADER_ARCH]		= "ARCH",
+	[PERF_RECORD_HEADER_NRCPUS]		= "NRCPUS",
+	[PERF_RECORD_HEADER_CPUDESC]		= "CPUDESC",
+	[PERF_RECORD_HEADER_CPUID]		= "CPUID",
+	[PERF_RECORD_HEADER_TOTAL_MEM]		= "TOTAL_MEM",
+	[PERF_RECORD_HEADER_CMDLINE]		= "CMDLINE",
+	[PERF_RECORD_HEADER_EVENT_DESC]		= "EVENT_DESC",
+	[PERF_RECORD_HEADER_CPU_TOPOLOGY]	= "CPU_TOPOLOGY",
+	[PERF_RECORD_HEADER_NUMA_TOPOLOGY]	= "NUMA_TOPOLOGY",
+	[PERF_RECORD_HEADER_PMU_MAPPINGS]	= "PMU_MAPPINGS",
 };
 
 static const char *perf_ns__names[] = {
diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
index db2de6413518..d404f50260f8 100644
--- a/tools/perf/util/event.h
+++ b/tools/perf/util/event.h
@@ -244,6 +244,19 @@ enum perf_user_event_type { /* above any possible kernel type */
 	PERF_RECORD_STAT_ROUND			= 77,
 	PERF_RECORD_EVENT_UPDATE		= 78,
 	PERF_RECORD_TIME_CONV			= 79,
+	PERF_RECORD_HEADER_HOSTNAME		= 80,
+	PERF_RECORD_HEADER_OSRELEASE		= 81,
+	PERF_RECORD_HEADER_VERSION		= 82,
+	PERF_RECORD_HEADER_ARCH			= 83,
+	PERF_RECORD_HEADER_NRCPUS		= 84,
+	PERF_RECORD_HEADER_CPUDESC		= 85,
+	PERF_RECORD_HEADER_CPUID		= 86,
+	PERF_RECORD_HEADER_TOTAL_MEM		= 87,
+	PERF_RECORD_HEADER_CMDLINE		= 88,
+	PERF_RECORD_HEADER_EVENT_DESC		= 89,
+	PERF_RECORD_HEADER_CPU_TOPOLOGY		= 90,
+	PERF_RECORD_HEADER_NUMA_TOPOLOGY	= 91,
+	PERF_RECORD_HEADER_PMU_MAPPINGS		= 92,
 	PERF_RECORD_HEADER_MAX
 };
 
@@ -488,6 +501,11 @@ struct time_conv_event {
 	u64 time_zero;
 };
 
+struct feature_event {
+	struct perf_event_header header;
+	char data[]; /* size bytes of raw data specific to the feature */
+};
+
 union perf_event {
 	struct perf_event_header	header;
 	struct mmap_event		mmap;
@@ -518,6 +536,7 @@ union perf_event {
 	struct stat_event		stat;
 	struct stat_round_event		stat_round;
 	struct time_conv_event		time_conv;
+	struct feature_event		feat;
 };
 
 void perf_event__print_totals(void);
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 981f5e9685e2..f6ce7ca80c12 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -11,6 +11,7 @@
 #include <linux/list.h>
 #include <linux/kernel.h>
 #include <linux/bitops.h>
+#include <linux/stringify.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <sys/utsname.h>
@@ -32,6 +33,7 @@
 #include "data.h"
 #include <api/fs/fs.h>
 #include "asm/bug.h"
+#include "tool.h"
 
 #include "sane_ctype.h"
 
@@ -2107,42 +2109,80 @@ struct feature_ops {
 	int (*process)(struct feat_fd *fd, void *data);
 	const char *name;
 	bool full_only;
+	int record_type;
 };
 
-#define FEAT_OPA(n, func) \
-	[n] = { .name = #n, .write = write_##func, .print = print_##func }
-#define FEAT_OPP(n, func) \
-	[n] = { .name = #n, .write = write_##func, .print = print_##func, \
-		.process = process_##func }
-#define FEAT_OPF(n, func) \
-	[n] = { .name = #n, .write = write_##func, .print = print_##func, \
-		.process = process_##func, .full_only = true }
+#define FEAT_OPP(n, func, __full_only) \
+	[HEADER_##n] = {					\
+		.name = __stringify(HEADER_##n),		\
+		.write = write_##func,				\
+		.print = print_##func,				\
+		.full_only = __full_only,			\
+		.process = process_##func,			\
+		.record_type = PERF_RECORD_HEADER_##n		\
+	}
+
+#define FEAT_OPN(n, func, __full_only) \
+	[HEADER_##n] = {					\
+		.name = __stringify(HEADER_##n),		\
+		.write = write_##func,				\
+		.print = print_##func,				\
+		.full_only = __full_only,			\
+		.process = process_##func			\
+	}
 
 /* feature_ops not implemented: */
 #define print_tracing_data	NULL
 #define print_build_id		NULL
 
+#define process_branch_stack	NULL
+#define process_stat		NULL
+
+
 static const struct feature_ops feat_ops[HEADER_LAST_FEATURE] = {
-	FEAT_OPP(HEADER_TRACING_DATA,	tracing_data),
-	FEAT_OPP(HEADER_BUILD_ID,	build_id),
-	FEAT_OPP(HEADER_HOSTNAME,	hostname),
-	FEAT_OPP(HEADER_OSRELEASE,	osrelease),
-	FEAT_OPP(HEADER_VERSION,	version),
-	FEAT_OPP(HEADER_ARCH,		arch),
-	FEAT_OPP(HEADER_NRCPUS,		nrcpus),
-	FEAT_OPP(HEADER_CPUDESC,	cpudesc),
-	FEAT_OPP(HEADER_CPUID,		cpuid),
-	FEAT_OPP(HEADER_TOTAL_MEM,	total_mem),
-	FEAT_OPP(HEADER_EVENT_DESC,	event_desc),
-	FEAT_OPP(HEADER_CMDLINE,	cmdline),
-	FEAT_OPF(HEADER_CPU_TOPOLOGY,	cpu_topology),
-	FEAT_OPF(HEADER_NUMA_TOPOLOGY,	numa_topology),
-	FEAT_OPA(HEADER_BRANCH_STACK,	branch_stack),
-	FEAT_OPP(HEADER_PMU_MAPPINGS,	pmu_mappings),
-	FEAT_OPP(HEADER_GROUP_DESC,	group_desc),
-	FEAT_OPP(HEADER_AUXTRACE,	auxtrace),
-	FEAT_OPA(HEADER_STAT,		stat),
-	FEAT_OPF(HEADER_CACHE,		cache),
+	FEAT_OPP(TRACING_DATA,	tracing_data,	false),
+	FEAT_OPP(BUILD_ID,	build_id,	false),
+	FEAT_OPP(HOSTNAME,	hostname,	false),
+	FEAT_OPP(OSRELEASE,	osrelease,	false),
+	FEAT_OPP(VERSION,	version,	false),
+	FEAT_OPP(ARCH,		arch,		false),
+	FEAT_OPP(NRCPUS,	nrcpus,		false),
+	FEAT_OPP(CPUDESC,	cpudesc,	false),
+	FEAT_OPP(CPUID,		cpuid,		false),
+	FEAT_OPP(TOTAL_MEM,	total_mem,	false),
+	FEAT_OPP(EVENT_DESC,	event_desc,	false),
+	FEAT_OPP(CMDLINE,	cmdline,	false),
+	FEAT_OPP(CPU_TOPOLOGY,	cpu_topology,	true),
+	FEAT_OPP(NUMA_TOPOLOGY,	numa_topology,	true),
+	FEAT_OPN(BRANCH_STACK,	branch_stack,	false),
+	FEAT_OPP(PMU_MAPPINGS,	pmu_mappings,	false),
+	FEAT_OPN(GROUP_DESC,	group_desc,	false),
+	FEAT_OPN(AUXTRACE,	auxtrace,	false),
+	FEAT_OPN(STAT,		stat,		false),
+	FEAT_OPN(CACHE,		cache,		true),
+};
+
+/*
+ * we use a mapping table to go from record type to feature header
+ * because we have no guarantee that new record types may not be added
+ * after the feature header.
+ */
+#define REC2FEAT(a)	[PERF_RECORD_HEADER_##a] = HEADER_##a
+
+static const int rec2feat[PERF_RECORD_HEADER_MAX] = {
+	REC2FEAT(HOSTNAME),
+	REC2FEAT(OSRELEASE),
+	REC2FEAT(VERSION),
+	REC2FEAT(ARCH),
+	REC2FEAT(NRCPUS),
+	REC2FEAT(CPUDESC),
+	REC2FEAT(CPUID),
+	REC2FEAT(TOTAL_MEM),
+	REC2FEAT(EVENT_DESC),
+	REC2FEAT(CMDLINE),
+	REC2FEAT(CPU_TOPOLOGY),
+	REC2FEAT(NUMA_TOPOLOGY),
+	REC2FEAT(PMU_MAPPINGS),
 };
 
 struct header_print_data {
@@ -2218,33 +2258,33 @@ int perf_header__fprintf_info(struct perf_session *session, FILE *fp, bool full)
 	return 0;
 }
 
-static int do_write_feat(struct feat_fd *fd, struct perf_header *h, int type,
+static int do_write_feat(struct feat_fd *fdd, int type,
 			 struct perf_file_section **p,
 			 struct perf_evlist *evlist)
 {
 	int err;
 	int ret = 0;
 
-	if (perf_header__has_feat(h, type)) {
+	if (perf_header__has_feat(fdd->ph, type)) {
 		if (!feat_ops[type].write)
 			return -1;
 
-		if (fd->buf) {
-			pr_err("do_write_feat to memory buffer\n");
+		if (fdd->buf) {
+			pr_debug("Called do_write_feat for memory buffer\n");
 			return -1;
 		}
-		(*p)->offset = lseek(fd->fd, 0, SEEK_CUR);
+		(*p)->offset = lseek(fdd->fd, 0, SEEK_CUR);
 
-		err = feat_ops[type].write(fd, evlist);
+		err = feat_ops[type].write(fdd, evlist);
 		if (err < 0) {
 			pr_debug("failed to write feature %s\n", feat_ops[type].name);
 
 			/* undo anything written */
-			lseek(fd->fd, (*p)->offset, SEEK_SET);
+			lseek(fdd->fd, (*p)->offset, SEEK_SET);
 
 			return -1;
 		}
-		(*p)->size = lseek(fd->fd, 0, SEEK_CUR) - (*p)->offset;
+		(*p)->size = lseek(fdd->fd, 0, SEEK_CUR) - (*p)->offset;
 		(*p)++;
 	}
 	return ret;
@@ -2261,10 +2301,6 @@ static int perf_header__adds_write(struct perf_header *header,
 	int feat;
 	int err;
 
-	/*
-	 * may write more than needed due to dropped feature, but
-	 * this is okay, reader will skip the mising entries
-	 */
 	fdd = (struct feat_fd){
 		.fd  = fd,
 		.ph = header,
@@ -2284,7 +2320,7 @@ static int perf_header__adds_write(struct perf_header *header,
 	lseek(fd, sec_start + sec_size, SEEK_SET);
 
 	for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
-		if (do_write_feat(&fdd, header, feat, &p, evlist))
+		if (do_write_feat(&fdd, feat, &p, evlist))
 			perf_header__clear_feat(header, feat);
 	}
 
@@ -2941,6 +2977,112 @@ int perf_event__synthesize_attr(struct perf_tool *tool,
 	return err;
 }
 
+int perf_event__synthesize_features(struct perf_tool *tool,
+				    struct perf_session *session,
+				    struct perf_evlist *evlist,
+				    perf_event__handler_t process)
+{
+	struct perf_header *header = &session->header;
+	struct feat_fd fdd;
+	struct feature_event *fe;
+	size_t sz, sz_hdr;
+	int feat, ret;
+
+	sz_hdr = sizeof(fe->header);
+	sz = sizeof(union perf_event);
+	/* get a nice alignment */
+	sz = PERF_ALIGN(sz, getpagesize());
+
+	memset(&fdd, 0, sizeof(fdd));
+
+	fdd.buf = malloc(sz);
+	if (!fdd.buf)
+		return -ENOMEM;
+
+	fdd.size = sz - sz_hdr;
+
+	for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
+		/*
+		 * those are already written as:
+		 * - PERF_RECORD_HEADER_TRACING_DATA
+		 * - PERF_RECORD_HEADER_BUILD_ID
+		 */
+		if (feat == HEADER_TRACING_DATA || feat == HEADER_BUILD_ID)
+			continue;
+
+		if (!feat_ops[feat].record_type) {
+			pr_debug("No record type for header :%d\n", feat);
+			continue;
+		}
+
+		fdd.offset = sizeof(*fe);
+
+		ret = feat_ops[feat].write(&fdd, evlist);
+		if (ret || fdd.offset <= (ssize_t)sizeof(*fe)) {
+			pr_debug("Error writing feature\n");
+			continue;
+		}
+
+		/* fdd.buf may have changed due to realloc in do_write() */
+		fe = fdd.buf;
+		memset(fe, 0, sizeof(*fe));
+
+		fe->header.type = feat_ops[feat].record_type;
+		fe->header.size = fdd.offset;
+
+		process(tool, fdd.buf, NULL, NULL);
+	}
+	free(fdd.buf);
+	return 0;
+}
+
+int perf_event__process_feature(struct perf_tool *tool,
+				union perf_event *event,
+				struct perf_session *session __maybe_unused)
+{
+	struct feat_fd fd = { .fd = 0 };
+	int type = event->header.type;
+	int feat;
+
+	if (type < 0 || type >= PERF_RECORD_HEADER_MAX) {
+		pr_warning("invalid record type %d\n", type);
+		return 0;
+	}
+	feat = rec2feat[type];
+	if (feat == HEADER_RESERVED)
+		return -1;
+
+	if (feat > HEADER_LAST_FEATURE)
+		return 0;
+
+	if (!feat_ops[feat].process)
+		return 0;
+
+	/*
+	 * no print routine
+	 */
+	if (!feat_ops[feat].print)
+		return 0;
+
+	fd.buf  = (void *)event;
+	fd.buf += sizeof(event->header);
+	fd.size = event->header.size - sizeof(event->header);
+	fd.ph = &session->header;
+
+	if (!feat_ops[feat].full_only || tool->show_full_info) {
+		if (feat_ops[feat].process) {
+			if (feat_ops[feat].process(&fd, NULL))
+				return -1;
+		}
+		feat_ops[feat].print(&fd, stdout);
+	} else {
+		fprintf(stdout, "# %s info available, use -I to display\n",
+			feat_ops[feat].name);
+	}
+
+	return 0;
+}
+
 static struct event_update_event *
 event_update_event__new(size_t size, u64 type, u64 id)
 {
diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
index 86ab723cbee4..fbe1a4690627 100644
--- a/tools/perf/util/header.h
+++ b/tools/perf/util/header.h
@@ -102,6 +102,15 @@ int perf_header__process_sections(struct perf_header *header, int fd,
 
 int perf_header__fprintf_info(struct perf_session *s, FILE *fp, bool full);
 
+int perf_event__synthesize_features(struct perf_tool *tool,
+				    struct perf_session *session,
+				    struct perf_evlist *evlist,
+				    perf_event__handler_t process);
+
+int perf_event__process_feature(struct perf_tool *tool,
+				union perf_event *event,
+				struct perf_session *session);
+
 int perf_event__synthesize_attr(struct perf_tool *tool,
 				struct perf_event_attr *attr, u32 ids, u64 *id,
 				perf_event__handler_t process);
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 3041c6b98191..ef788b91239e 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -256,6 +256,14 @@ static int process_event_stub(struct perf_tool *tool __maybe_unused,
 	return 0;
 }
 
+static int process_feature_stub(struct perf_tool *tool __maybe_unused,
+				union perf_event *event __maybe_unused,
+				struct perf_session *session __maybe_unused)
+{
+	dump_printf(": unhandled!\n");
+	return 0;
+}
+
 static int process_finished_round_stub(struct perf_tool *tool __maybe_unused,
 				       union perf_event *event __maybe_unused,
 				       struct ordered_events *oe __maybe_unused)
@@ -427,6 +435,8 @@ void perf_tool__fill_defaults(struct perf_tool *tool)
 		tool->stat_round = process_stat_round_stub;
 	if (tool->time_conv == NULL)
 		tool->time_conv = process_event_op2_stub;
+	if (tool->feature == NULL)
+		tool->feature = process_feature_stub;
 }
 
 static void swap_sample_id_all(union perf_event *event, void *data)
@@ -1370,6 +1380,8 @@ static s64 perf_session__process_user_event(struct perf_session *session,
 	case PERF_RECORD_TIME_CONV:
 		session->time_conv = event->time_conv;
 		return tool->time_conv(tool, event, session);
+	case PERF_RECORD_HEADER_HOSTNAME ... PERF_RECORD_HEADER_PMU_MAPPINGS:
+		return tool->feature(tool, event, session);
 	default:
 		return -EINVAL;
 	}
diff --git a/tools/perf/util/tool.h b/tools/perf/util/tool.h
index 795b2be7b51f..70cb36e6b098 100644
--- a/tools/perf/util/tool.h
+++ b/tools/perf/util/tool.h
@@ -63,7 +63,8 @@ struct perf_tool {
 			cpu_map,
 			stat_config,
 			stat,
-			stat_round;
+			stat_round,
+			feature;
 	event_op3	auxtrace;
 	bool		ordered_events;
 	bool		ordering_requires_timestamps;
-- 
2.13.0.303.g4ebf302169-goog

[toc] | [next] | [standalone]

#1644719

FromJiri Olsa <jolsa@redhat.com>
Date2017-05-18 18:20 +0200
Message-ID<tIwjD-58a-3@gated-at.bofh.it>
In reply to#1643788
On Wed, May 17, 2017 at 09:16:02PM -0700, David Carrillo-Cisneros wrote:
> Add header record types to pipe-mode, reusing the functions
> used in file-mode and leveraging the new struct feat_fd.
> 
> Add the perf_event__synthesize_feature event call back to
> process the new header records.
> 
> Before this patch:
> 
>   $ perf record -o - -e cycles -c 100000 sleep 1 | perf report --stdio
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.000 MB - ]
>   ...
> 
> After this patch:
>   $ perf record -o - -e cycles -c 100000 sleep 1 | perf report --stdio
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.000 MB - ]
>   # hostname : lphh7
>   # os release : 4.11.0-dbx-up_perf
>   # perf version : 4.11.rc6.g6277c80
>   # arch : x86_64
>   # nrcpus online : 72
>   # nrcpus avail : 72
>   # cpudesc : Intel(R) Xeon(R) CPU E5-2696 v3 @ 2.30GHz
>   # cpuid : GenuineIntel,6,63,2
>   # total memory : 263457192 kB
>   # cmdline : /root/perf record -o - -e cycles -c 100000 sleep 1
>   # HEADER_CPU_TOPOLOGY info available, use -I to display
>   # HEADER_NUMA_TOPOLOGY info available, use -I to display
>   # pmu mappings: intel_bts = 6, uncore_imc_4 = 22, uncore_sbox_1 = 47, uncore_cbox_5 = 33, uncore_ha_0 = 16, uncore_cbox
>    Percent |      Source code & Disassembly of kcore for cycles (9 samples)
>   ...

thanks a lot for doing this, comments comming shortly

jirka

[toc] | [prev] | [next] | [standalone]

#1644829

FromDavid Carrillo-Cisneros <davidcc@google.com>
Date2017-05-18 20:40 +0200
Message-ID<tIyv7-6vH-3@gated-at.bofh.it>
In reply to#1644719
On Thu, May 18, 2017 at 9:12 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Wed, May 17, 2017 at 09:16:02PM -0700, David Carrillo-Cisneros wrote:
>> Add header record types to pipe-mode, reusing the functions
>> used in file-mode and leveraging the new struct feat_fd.
>>
>> Add the perf_event__synthesize_feature event call back to
>> process the new header records.
>>
>> Before this patch:
>>
>>   $ perf record -o - -e cycles -c 100000 sleep 1 | perf report --stdio
>>   [ perf record: Woken up 1 times to write data ]
>>   [ perf record: Captured and wrote 0.000 MB - ]
>>   ...
>>
>> After this patch:
>>   $ perf record -o - -e cycles -c 100000 sleep 1 | perf report --stdio
>>   [ perf record: Woken up 1 times to write data ]
>>   [ perf record: Captured and wrote 0.000 MB - ]
>>   # hostname : lphh7
>>   # os release : 4.11.0-dbx-up_perf
>>   # perf version : 4.11.rc6.g6277c80
>>   # arch : x86_64
>>   # nrcpus online : 72
>>   # nrcpus avail : 72
>>   # cpudesc : Intel(R) Xeon(R) CPU E5-2696 v3 @ 2.30GHz
>>   # cpuid : GenuineIntel,6,63,2
>>   # total memory : 263457192 kB
>>   # cmdline : /root/perf record -o - -e cycles -c 100000 sleep 1
>>   # HEADER_CPU_TOPOLOGY info available, use -I to display
>>   # HEADER_NUMA_TOPOLOGY info available, use -I to display
>>   # pmu mappings: intel_bts = 6, uncore_imc_4 = 22, uncore_sbox_1 = 47, uncore_cbox_5 = 33, uncore_ha_0 = 16, uncore_cbox
>>    Percent |      Source code & Disassembly of kcore for cycles (9 samples)
>>   ...
>
> thanks a lot for doing this, comments comming shortly
>
thanks a lot for the review. I'll do the suggested changes and send a
2nd version.

David

[toc] | [prev] | [next] | [standalone]

#1644720

FromJiri Olsa <jolsa@redhat.com>
Date2017-05-18 18:20 +0200
Message-ID<tIwjD-58a-5@gated-at.bofh.it>
In reply to#1643788
On Wed, May 17, 2017 at 09:16:02PM -0700, David Carrillo-Cisneros wrote:

SNIP

>  static const char *perf_ns__names[] = {
> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
> index db2de6413518..d404f50260f8 100644
> --- a/tools/perf/util/event.h
> +++ b/tools/perf/util/event.h
> @@ -244,6 +244,19 @@ enum perf_user_event_type { /* above any possible kernel type */
>  	PERF_RECORD_STAT_ROUND			= 77,
>  	PERF_RECORD_EVENT_UPDATE		= 78,
>  	PERF_RECORD_TIME_CONV			= 79,
> +	PERF_RECORD_HEADER_HOSTNAME		= 80,
> +	PERF_RECORD_HEADER_OSRELEASE		= 81,
> +	PERF_RECORD_HEADER_VERSION		= 82,
> +	PERF_RECORD_HEADER_ARCH			= 83,
> +	PERF_RECORD_HEADER_NRCPUS		= 84,
> +	PERF_RECORD_HEADER_CPUDESC		= 85,
> +	PERF_RECORD_HEADER_CPUID		= 86,
> +	PERF_RECORD_HEADER_TOTAL_MEM		= 87,
> +	PERF_RECORD_HEADER_CMDLINE		= 88,
> +	PERF_RECORD_HEADER_EVENT_DESC		= 89,
> +	PERF_RECORD_HEADER_CPU_TOPOLOGY		= 90,
> +	PERF_RECORD_HEADER_NUMA_TOPOLOGY	= 91,
> +	PERF_RECORD_HEADER_PMU_MAPPINGS		= 92,
>  	PERF_RECORD_HEADER_MAX
>  };

SNIP

> +
> +/*
> + * we use a mapping table to go from record type to feature header
> + * because we have no guarantee that new record types may not be added
> + * after the feature header.
> + */
> +#define REC2FEAT(a)	[PERF_RECORD_HEADER_##a] = HEADER_##a
> +
> +static const int rec2feat[PERF_RECORD_HEADER_MAX] = {
> +	REC2FEAT(HOSTNAME),
> +	REC2FEAT(OSRELEASE),
> +	REC2FEAT(VERSION),
> +	REC2FEAT(ARCH),
> +	REC2FEAT(NRCPUS),
> +	REC2FEAT(CPUDESC),
> +	REC2FEAT(CPUID),
> +	REC2FEAT(TOTAL_MEM),
> +	REC2FEAT(EVENT_DESC),
> +	REC2FEAT(CMDLINE),
> +	REC2FEAT(CPU_TOPOLOGY),
> +	REC2FEAT(NUMA_TOPOLOGY),
> +	REC2FEAT(PMU_MAPPINGS),
>  };

hum, how about instead of adding all those new events we add
just one: PERF_RECORD_FEATURE, which would carry the id of the
feature, like:

struct feature_event {
	struct perf_event_header	header;
	u64	id;
	char	data[]; /* size bytes of raw data specific to the feature */
};


this way we could also omit the reverse event to feature map

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1644821

FromDavid Carrillo-Cisneros <davidcc@google.com>
Date2017-05-18 20:30 +0200
Message-ID<tIylr-6r1-3@gated-at.bofh.it>
In reply to#1644720
On Thu, May 18, 2017 at 9:13 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Wed, May 17, 2017 at 09:16:02PM -0700, David Carrillo-Cisneros wrote:
>
> SNIP
>
>>  static const char *perf_ns__names[] = {
>> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
>> index db2de6413518..d404f50260f8 100644
>> --- a/tools/perf/util/event.h
>> +++ b/tools/perf/util/event.h
>> @@ -244,6 +244,19 @@ enum perf_user_event_type { /* above any possible kernel type */
>>       PERF_RECORD_STAT_ROUND                  = 77,
>>       PERF_RECORD_EVENT_UPDATE                = 78,
>>       PERF_RECORD_TIME_CONV                   = 79,
>> +     PERF_RECORD_HEADER_HOSTNAME             = 80,
>> +     PERF_RECORD_HEADER_OSRELEASE            = 81,
>> +     PERF_RECORD_HEADER_VERSION              = 82,
>> +     PERF_RECORD_HEADER_ARCH                 = 83,
>> +     PERF_RECORD_HEADER_NRCPUS               = 84,
>> +     PERF_RECORD_HEADER_CPUDESC              = 85,
>> +     PERF_RECORD_HEADER_CPUID                = 86,
>> +     PERF_RECORD_HEADER_TOTAL_MEM            = 87,
>> +     PERF_RECORD_HEADER_CMDLINE              = 88,
>> +     PERF_RECORD_HEADER_EVENT_DESC           = 89,
>> +     PERF_RECORD_HEADER_CPU_TOPOLOGY         = 90,
>> +     PERF_RECORD_HEADER_NUMA_TOPOLOGY        = 91,
>> +     PERF_RECORD_HEADER_PMU_MAPPINGS         = 92,
>>       PERF_RECORD_HEADER_MAX
>>  };
>
> SNIP
>
>> +
>> +/*
>> + * we use a mapping table to go from record type to feature header
>> + * because we have no guarantee that new record types may not be added
>> + * after the feature header.
>> + */
>> +#define REC2FEAT(a)  [PERF_RECORD_HEADER_##a] = HEADER_##a
>> +
>> +static const int rec2feat[PERF_RECORD_HEADER_MAX] = {
>> +     REC2FEAT(HOSTNAME),
>> +     REC2FEAT(OSRELEASE),
>> +     REC2FEAT(VERSION),
>> +     REC2FEAT(ARCH),
>> +     REC2FEAT(NRCPUS),
>> +     REC2FEAT(CPUDESC),
>> +     REC2FEAT(CPUID),
>> +     REC2FEAT(TOTAL_MEM),
>> +     REC2FEAT(EVENT_DESC),
>> +     REC2FEAT(CMDLINE),
>> +     REC2FEAT(CPU_TOPOLOGY),
>> +     REC2FEAT(NUMA_TOPOLOGY),
>> +     REC2FEAT(PMU_MAPPINGS),
>>  };
>
> hum, how about instead of adding all those new events we add
> just one: PERF_RECORD_FEATURE, which would carry the id of the
> feature, like:
>
> struct feature_event {
>         struct perf_event_header        header;
>         u64     id;
>         char    data[]; /* size bytes of raw data specific to the feature */
> };
>
>
> this way we could also omit the reverse event to feature map

I think that'd work. I'll try that.

[toc] | [prev] | [next] | [standalone]

#1644721

FromJiri Olsa <jolsa@redhat.com>
Date2017-05-18 18:20 +0200
Message-ID<tIwjE-58a-11@gated-at.bofh.it>
In reply to#1643788
On Wed, May 17, 2017 at 09:16:02PM -0700, David Carrillo-Cisneros wrote:

SNIP

> -		(*p)->offset = lseek(fd->fd, 0, SEEK_CUR);
> +		(*p)->offset = lseek(fdd->fd, 0, SEEK_CUR);
>  
> -		err = feat_ops[type].write(fd, evlist);
> +		err = feat_ops[type].write(fdd, evlist);
>  		if (err < 0) {
>  			pr_debug("failed to write feature %s\n", feat_ops[type].name);
>  
>  			/* undo anything written */
> -			lseek(fd->fd, (*p)->offset, SEEK_SET);
> +			lseek(fdd->fd, (*p)->offset, SEEK_SET);
>  
>  			return -1;
>  		}
> -		(*p)->size = lseek(fd->fd, 0, SEEK_CUR) - (*p)->offset;
> +		(*p)->size = lseek(fdd->fd, 0, SEEK_CUR) - (*p)->offset;
>  		(*p)++;
>  	}
>  	return ret;
> @@ -2261,10 +2301,6 @@ static int perf_header__adds_write(struct perf_header *header,
>  	int feat;
>  	int err;
>  
> -	/*
> -	 * may write more than needed due to dropped feature, but
> -	 * this is okay, reader will skip the mising entries
> -	 */

you added this comment in earlier patch, if you don't want it,
remove it in the original patch

jirka

[toc] | [prev] | [next] | [standalone]

#1644724

FromJiri Olsa <jolsa@redhat.com>
Date2017-05-18 18:20 +0200
Message-ID<tIwjE-58a-17@gated-at.bofh.it>
In reply to#1643788
On Wed, May 17, 2017 at 09:16:02PM -0700, David Carrillo-Cisneros wrote:

SNIP

>  struct header_print_data {
> @@ -2218,33 +2258,33 @@ int perf_header__fprintf_info(struct perf_session *session, FILE *fp, bool full)
>  	return 0;
>  }
>  
> -static int do_write_feat(struct feat_fd *fd, struct perf_header *h, int type,
> +static int do_write_feat(struct feat_fd *fdd, int type,
>  			 struct perf_file_section **p,
>  			 struct perf_evlist *evlist)
>  {
>  	int err;
>  	int ret = 0;
>  
> -	if (perf_header__has_feat(h, type)) {
> +	if (perf_header__has_feat(fdd->ph, type)) {

please do these changes in the earlier patch that introduced
the feat_fd as parameter in do_write_feat function

thanks,
jirka

>  		if (!feat_ops[type].write)
>  			return -1;
>  
> -		if (fd->buf) {
> -			pr_err("do_write_feat to memory buffer\n");
> +		if (fdd->buf) {
> +			pr_debug("Called do_write_feat for memory buffer\n");
>  			return -1;
>  		}
> -		(*p)->offset = lseek(fd->fd, 0, SEEK_CUR);
> +		(*p)->offset = lseek(fdd->fd, 0, SEEK_CUR);
>  
> -		err = feat_ops[type].write(fd, evlist);
> +		err = feat_ops[type].write(fdd, evlist);
>  		if (err < 0) {
>  			pr_debug("failed to write feature %s\n", feat_ops[type].name);
>  
>  			/* undo anything written */
> -			lseek(fd->fd, (*p)->offset, SEEK_SET);
> +			lseek(fdd->fd, (*p)->offset, SEEK_SET);
>  
>  			return -1;
>  		}
> -		(*p)->size = lseek(fd->fd, 0, SEEK_CUR) - (*p)->offset;
> +		(*p)->size = lseek(fdd->fd, 0, SEEK_CUR) - (*p)->offset;
>  		(*p)++;
>  	}
>  	return ret;
> @@ -2261,10 +2301,6 @@ static int perf_header__adds_write(struct perf_header *header,
>  	int feat;
>  	int err;
>  
> -	/*
> -	 * may write more than needed due to dropped feature, but
> -	 * this is okay, reader will skip the mising entries
> -	 */
>  	fdd = (struct feat_fd){
>  		.fd  = fd,
>  		.ph = header,
> @@ -2284,7 +2320,7 @@ static int perf_header__adds_write(struct perf_header *header,
>  	lseek(fd, sec_start + sec_size, SEEK_SET);
>  
>  	for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
> -		if (do_write_feat(&fdd, header, feat, &p, evlist))
> +		if (do_write_feat(&fdd, feat, &p, evlist))
>  			perf_header__clear_feat(header, feat);

SNIP

[toc] | [prev] | [next] | [standalone]

#1644728

FromJiri Olsa <jolsa@redhat.com>
Date2017-05-18 18:20 +0200
Message-ID<tIwjE-58a-25@gated-at.bofh.it>
In reply to#1643788
On Wed, May 17, 2017 at 09:16:02PM -0700, David Carrillo-Cisneros wrote:

SNIP

>  
> @@ -2107,42 +2109,80 @@ struct feature_ops {
>  	int (*process)(struct feat_fd *fd, void *data);
>  	const char *name;
>  	bool full_only;
> +	int record_type;
>  };
>  
> -#define FEAT_OPA(n, func) \
> -	[n] = { .name = #n, .write = write_##func, .print = print_##func }
> -#define FEAT_OPP(n, func) \
> -	[n] = { .name = #n, .write = write_##func, .print = print_##func, \
> -		.process = process_##func }
> -#define FEAT_OPF(n, func) \
> -	[n] = { .name = #n, .write = write_##func, .print = print_##func, \
> -		.process = process_##func, .full_only = true }
> +#define FEAT_OPP(n, func, __full_only) \
> +	[HEADER_##n] = {					\
> +		.name = __stringify(HEADER_##n),		\
> +		.write = write_##func,				\
> +		.print = print_##func,				\
> +		.full_only = __full_only,			\
> +		.process = process_##func,			\
> +		.record_type = PERF_RECORD_HEADER_##n		\
> +	}
> +
> +#define FEAT_OPN(n, func, __full_only) \
> +	[HEADER_##n] = {					\
> +		.name = __stringify(HEADER_##n),		\
> +		.write = write_##func,				\
> +		.print = print_##func,				\
> +		.full_only = __full_only,			\
> +		.process = process_##func			\
> +	}

you're adding record_type, which should not be a reason
to rename FEAT_* macros.. please do that in the separate
patch and state reason in changelog

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1644822

FromDavid Carrillo-Cisneros <davidcc@google.com>
Date2017-05-18 20:30 +0200
Message-ID<tIylr-6r1-9@gated-at.bofh.it>
In reply to#1644728
On Thu, May 18, 2017 at 9:13 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Wed, May 17, 2017 at 09:16:02PM -0700, David Carrillo-Cisneros wrote:
>
> SNIP
>
>>
>> @@ -2107,42 +2109,80 @@ struct feature_ops {
>>       int (*process)(struct feat_fd *fd, void *data);
>>       const char *name;
>>       bool full_only;
>> +     int record_type;
>>  };
>>
>> -#define FEAT_OPA(n, func) \
>> -     [n] = { .name = #n, .write = write_##func, .print = print_##func }
>> -#define FEAT_OPP(n, func) \
>> -     [n] = { .name = #n, .write = write_##func, .print = print_##func, \
>> -             .process = process_##func }
>> -#define FEAT_OPF(n, func) \
>> -     [n] = { .name = #n, .write = write_##func, .print = print_##func, \
>> -             .process = process_##func, .full_only = true }
>> +#define FEAT_OPP(n, func, __full_only) \
>> +     [HEADER_##n] = {                                        \
>> +             .name = __stringify(HEADER_##n),                \
>> +             .write = write_##func,                          \
>> +             .print = print_##func,                          \
>> +             .full_only = __full_only,                       \
>> +             .process = process_##func,                      \
>> +             .record_type = PERF_RECORD_HEADER_##n           \
>> +     }
>> +
>> +#define FEAT_OPN(n, func, __full_only) \
>> +     [HEADER_##n] = {                                        \
>> +             .name = __stringify(HEADER_##n),                \
>> +             .write = write_##func,                          \
>> +             .print = print_##func,                          \
>> +             .full_only = __full_only,                       \
>> +             .process = process_##func                       \
>> +     }
>
> you're adding record_type, which should not be a reason
> to rename FEAT_* macros.. please do that in the separate
> patch and state reason in changelog

will do

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web