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

Groups > linux.kernel > #1647756 > unrolled thread

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

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 v2 13/13] perf tools: add feature header record to pipe-mode David Carrillo-Cisneros <davidcc@google.com> - 2017-05-23 10:00 +0200
    Re: [PATCH v2 13/13] perf tools: add feature header record to  pipe-mode Namhyung Kim <namhyung@kernel.org> - 2017-05-24 17:50 +0200
    Re: [PATCH v2 13/13] perf tools: add feature header record to  pipe-mode Jiri Olsa <jolsa@redhat.com> - 2017-05-25 10:10 +0200
    Re: [PATCH v2 13/13] perf tools: add feature header record to  pipe-mode Jiri Olsa <jolsa@redhat.com> - 2017-05-25 10:20 +0200
      Re: [PATCH v2 13/13] perf tools: add feature header record to pipe-mode David Carrillo-Cisneros <davidcc@google.com> - 2017-06-06 03:40 +0200
        Re: [PATCH v2 13/13] perf tools: add feature header record to  pipe-mode Jiri Olsa <jolsa@redhat.com> - 2017-06-06 13:10 +0200
          Re: [PATCH v2 13/13] perf tools: add feature header record to pipe-mode David Carrillo-Cisneros <davidcc@google.com> - 2017-06-06 20:20 +0200
    Re: [PATCH v2 13/13] perf tools: add feature header record to  pipe-mode Jiri Olsa <jolsa@redhat.com> - 2017-05-25 10:20 +0200
    Re: [PATCH v2 13/13] perf tools: add feature header record to  pipe-mode Jiri Olsa <jolsa@redhat.com> - 2017-05-25 10:20 +0200
      Re: [PATCH v2 13/13] perf tools: add feature header record to  pipe-mode Jiri Olsa <jolsa@redhat.com> - 2017-06-06 13:10 +0200
    Re: [PATCH v2 13/13] perf tools: add feature header record to  pipe-mode Jiri Olsa <jolsa@redhat.com> - 2017-05-25 10:20 +0200
    Re: [PATCH v2 13/13] perf tools: add feature header record to  pipe-mode Jiri Olsa <jolsa@redhat.com> - 2017-05-25 10:20 +0200

#1647756 — [PATCH v2 13/13] perf tools: add feature header record to pipe-mode

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 sleep 1 | perf report --stdio --header
  [ 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 sleep 1 | perf report --stdio --header
  # ========
  # captured on: Mon May 22 16:33:43 2017
  # ========
  #
  # hostname : my_hostname
  # 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
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.000 MB - ]
  ...

Support added for the subcommands: report, inject, annotate and script.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 tools/perf/Documentation/perf.data-file-format.txt |  10 +-
 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/builtin-script.c                        |   1 +
 tools/perf/util/event.c                            |   1 +
 tools/perf/util/event.h                            |   8 ++
 tools/perf/util/header.c                           | 109 ++++++++++++++++++++-
 tools/perf/util/header.h                           |   9 ++
 tools/perf/util/session.c                          |  12 +++
 tools/perf/util/tool.h                             |   3 +-
 12 files changed, 157 insertions(+), 6 deletions(-)

diff --git a/tools/perf/Documentation/perf.data-file-format.txt b/tools/perf/Documentation/perf.data-file-format.txt
index fa2a9132f0a9..a1d5c4303592 100644
--- a/tools/perf/Documentation/perf.data-file-format.txt
+++ b/tools/perf/Documentation/perf.data-file-format.txt
@@ -398,6 +398,11 @@ struct auxtrace_error_event {
 	char msg[MAX_AUXTRACE_ERROR_MSG];
 };
 
+	PERF_RECORD_HEADER_FEATURE		= 80,
+
+Describes a header feature. These are records used in pipe-mode that
+contain information that otherwise would be in perf.data file's header.
+
 Event types
 
 Define the event attributes with their IDs.
@@ -422,8 +427,9 @@ struct perf_pipe_file_header {
 };
 
 The information about attrs, data, and event_types is instead in the
-synthesized events PERF_RECORD_ATTR, PERF_RECORD_HEADER_TRACING_DATA and
-PERF_RECORD_HEADER_EVENT_TYPE that are generated by perf record in pipe-mode.
+synthesized events PERF_RECORD_ATTR, PERF_RECORD_HEADER_TRACING_DATA,
+PERF_RECORD_HEADER_EVENT_TYPE, and PERF_RECORD_HEADER_FEATURE
+that are generated by perf record in pipe-mode.
 
 
 References:
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 7620d708c78b..33881ca7dde6 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -718,6 +718,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/builtin-script.c b/tools/perf/builtin-script.c
index 0934d0885614..3f97eeba8105 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -2397,6 +2397,7 @@ int cmd_script(int argc, const char **argv)
 			.attr		 = process_attr,
 			.event_update   = perf_event__process_event_update,
 			.tracing_data	 = perf_event__process_tracing_data,
+			.feature	 = perf_event__process_feature,
 			.build_id	 = perf_event__process_build_id,
 			.id_index	 = perf_event__process_id_index,
 			.auxtrace_info	 = perf_event__process_auxtrace_info,
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 142835c0ca0a..b23b02e21c92 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -56,6 +56,7 @@ 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_FEATURE]		= "FEATURE",
 };
 
 static const char *perf_ns__names[] = {
diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
index db2de6413518..c09c7a6f7663 100644
--- a/tools/perf/util/event.h
+++ b/tools/perf/util/event.h
@@ -244,6 +244,7 @@ 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_FEATURE		= 80,
 	PERF_RECORD_HEADER_MAX
 };
 
@@ -488,6 +489,12 @@ struct time_conv_event {
 	u64 time_zero;
 };
 
+struct feature_event {
+	struct perf_event_header header;
+	u64 header_id;
+	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 +525,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 7aa84a02b9bd..cb79a205e566 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -33,6 +33,7 @@
 #include "data.h"
 #include <api/fs/fs.h>
 #include "asm/bug.h"
+#include "tool.h"
 
 #include "sane_ctype.h"
 
@@ -2233,14 +2234,14 @@ 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 *fd, 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(fd->ph, type)) {
 		if (!feat_ops[type].write)
 			return -1;
 
@@ -2295,7 +2296,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);
 	}
 
@@ -2952,6 +2953,108 @@ 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) {
+		if (!feat_ops[feat].has_record) {
+			pr_debug("No record header feature 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_id = feat;
+		fe->header.type = PERF_RECORD_HEADER_FEATURE;
+		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 };
+	struct feature_event *fe = (struct feature_event *)event;
+	int type = fe->header.type;
+	u64 feat = fe->header_id;
+
+	if (type < 0 || type >= PERF_RECORD_HEADER_MAX) {
+		pr_warning("invalid record type %d\n", type);
+		return 0;
+	}
+	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 *)fe->data;
+	fd.size = event->header.size - sizeof(event->header);
+	fd.ph = &session->header;
+
+	if (!tool->show_feat_hdr)
+		return 0;
+
+	if (!feat_ops[feat].full_only ||
+	    tool->show_feat_hdr >= SHOW_FEAT_HEADER_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 9d8dcd5eb727..f7a16ee527b8 100644
--- a/tools/perf/util/header.h
+++ b/tools/perf/util/header.h
@@ -101,6 +101,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..ffde0dafed6f 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_FEATURE:
+		return tool->feature(tool, event, session);
 	default:
 		return -EINVAL;
 	}
diff --git a/tools/perf/util/tool.h b/tools/perf/util/tool.h
index baeca808dfda..d549e50db397 100644
--- a/tools/perf/util/tool.h
+++ b/tools/perf/util/tool.h
@@ -69,7 +69,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.219.gdb65acc882-goog

[toc] | [next] | [standalone]

#1649715 — Re: [PATCH v2 13/13] perf tools: add feature header record to pipe-mode

On Tue, May 23, 2017 at 12:48:53AM -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 sleep 1 | perf report --stdio --header
>   [ 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 sleep 1 | perf report --stdio --header
>   # ========
>   # captured on: Mon May 22 16:33:43 2017
>   # ========
>   #
>   # hostname : my_hostname
>   # 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
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.000 MB - ]
>   ...
> 
> Support added for the subcommands: report, inject, annotate and script.
> 
> Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
> ---

[SNIP]
> +struct feature_event {
> +	struct perf_event_header header;
> +	u64 header_id;

s/header_id/feat_id/ ?


> +	char data[]; /* size bytes of raw data specific to the feature */
> +};
> +
>  union perf_event {
>  	struct perf_event_header	header;
>  	struct mmap_event		mmap;

[SNIP]
> +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 };
> +	struct feature_event *fe = (struct feature_event *)event;
> +	int type = fe->header.type;
> +	u64 feat = fe->header_id;
> +
> +	if (type < 0 || type >= PERF_RECORD_HEADER_MAX) {
> +		pr_warning("invalid record type %d\n", type);
> +		return 0;
> +	}
> +	if (feat == HEADER_RESERVED)
> +		return -1;
> +
> +	if (feat > HEADER_LAST_FEATURE)
> +		return 0;
> +
> +	if (!feat_ops[feat].process)
> +		return 0;

Checking here..

> +
> +	/*
> +	 * no print routine
> +	 */
> +	if (!feat_ops[feat].print)
> +		return 0;
> +
> +	fd.buf  = (void *)fe->data;
> +	fd.size = event->header.size - sizeof(event->header);
> +	fd.ph = &session->header;
> +
> +	if (!tool->show_feat_hdr)
> +		return 0;
> +
> +	if (!feat_ops[feat].full_only ||
> +	    tool->show_feat_hdr >= SHOW_FEAT_HEADER_FULL_INFO) {
> +		if (feat_ops[feat].process) {

.. and here again.

Thanks,
Namhyung


> +			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;
> +}

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

#1650274 — Re: [PATCH v2 13/13] perf tools: add feature header record to pipe-mode

On Tue, May 23, 2017 at 12:48:53AM -0700, David Carrillo-Cisneros wrote:

SNIP

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 3041c6b98191..ffde0dafed6f 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;

no need to define new one, you can use process_event_op2_stub

jirka

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

#1650279 — Re: [PATCH v2 13/13] perf tools: add feature header record to pipe-mode

On Tue, May 23, 2017 at 12:48:53AM -0700, David Carrillo-Cisneros wrote:

SNIP

> +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) {
> +		if (!feat_ops[feat].has_record) {
> +			pr_debug("No record header feature 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() */

right, so how's ensured the data never cross the maximum event size (0xffff) ?

I think do_write should have some check on that

jirka

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

#1658306

On Thu, May 25, 2017 at 1:10 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Tue, May 23, 2017 at 12:48:53AM -0700, David Carrillo-Cisneros wrote:
>
> SNIP
>
>> +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) {
>> +             if (!feat_ops[feat].has_record) {
>> +                     pr_debug("No record header feature 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() */
>
> right, so how's ensured the data never cross the maximum event size (0xffff) ?
>
> I think do_write should have some check on that

do_write reallocates ff->buff when it's not large enough.

>
> jirka

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

#1658636 — Re: [PATCH v2 13/13] perf tools: add feature header record to pipe-mode

On Mon, Jun 05, 2017 at 06:32:50PM -0700, David Carrillo-Cisneros wrote:
> On Thu, May 25, 2017 at 1:10 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> > On Tue, May 23, 2017 at 12:48:53AM -0700, David Carrillo-Cisneros wrote:
> >
> > SNIP
> >
> >> +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) {
> >> +             if (!feat_ops[feat].has_record) {
> >> +                     pr_debug("No record header feature 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() */
> >
> > right, so how's ensured the data never cross the maximum event size (0xffff) ?
> >
> > I think do_write should have some check on that
> 
> do_write reallocates ff->buff when it's not large enough.

and what if it's bigger than 0xffff?

jirka

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

#1659030

On Tue, Jun 6, 2017 at 4:04 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Mon, Jun 05, 2017 at 06:32:50PM -0700, David Carrillo-Cisneros wrote:
>> On Thu, May 25, 2017 at 1:10 AM, Jiri Olsa <jolsa@redhat.com> wrote:
>> > On Tue, May 23, 2017 at 12:48:53AM -0700, David Carrillo-Cisneros wrote:
>> >
>> > SNIP
>> >
>> >> +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) {
>> >> +             if (!feat_ops[feat].has_record) {
>> >> +                     pr_debug("No record header feature 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() */
>> >
>> > right, so how's ensured the data never cross the maximum event size (0xffff) ?
>> >
>> > I think do_write should have some check on that
>>
>> do_write reallocates ff->buff when it's not large enough.
>
> and what if it's bigger than 0xffff?
>
Oh yeah, I'll add that check.

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

#1650281 — Re: [PATCH v2 13/13] perf tools: add feature header record to pipe-mode

On Tue, May 23, 2017 at 12:48:53AM -0700, David Carrillo-Cisneros wrote:

SNIP

> +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 };
> +	struct feature_event *fe = (struct feature_event *)event;
> +	int type = fe->header.type;
> +	u64 feat = fe->header_id;
> +
> +	if (type < 0 || type >= PERF_RECORD_HEADER_MAX) {
> +		pr_warning("invalid record type %d\n", type);
> +		return 0;
> +	}
> +	if (feat == HEADER_RESERVED)
> +		return -1;
> +
> +	if (feat > HEADER_LAST_FEATURE)
> +		return 0;

I think we should warn in here

> +
> +	if (!feat_ops[feat].process)
> +		return 0;
> +
> +	/*
> +	 * no print routine
> +	 */

superfluous comment

> +	if (!feat_ops[feat].print)
> +		return 0;
> +
> +	fd.buf  = (void *)fe->data;
> +	fd.size = event->header.size - sizeof(event->header);
> +	fd.ph = &session->header;
> +
> +	if (!tool->show_feat_hdr)
> +		return 0;

some of the features could provide data for processing,
should we call process unconditionaly and check this
just before calling print?

thanks,
jirka

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

#1650283 — Re: [PATCH v2 13/13] perf tools: add feature header record to pipe-mode

On Tue, May 23, 2017 at 12:48:53AM -0700, David Carrillo-Cisneros wrote:

SNIP

> +	for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
> +		if (!feat_ops[feat].has_record) {
> +			pr_debug("No record header feature 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));

no need to wipe it out if you set all the members just below
(apart from header.misc, which is not used)

jirka

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

#1658637 — Re: [PATCH v2 13/13] perf tools: add feature header record to pipe-mode

On Tue, Jun 06, 2017 at 12:57:53AM +0000, David Carrillo-Cisneros wrote:
> On Thu, May 25, 2017 at 1:09 AM Jiri Olsa <jolsa@redhat.com> wrote:
> 
> > On Tue, May 23, 2017 at 12:48:53AM -0700, David Carrillo-Cisneros wrote:
> >
> > SNIP
> >
> > > +     for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
> > > +             if (!feat_ops[feat].has_record) {
> > > +                     pr_debug("No record header feature 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));
> >
> > no need to wipe it out if you set all the members just below
> > (apart from header.misc, which is not used)
> >
> 
> what if that changes one day? isn't it better to zero to avoid hard to find
> bugs?

ok

jirka

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

#1650284 — Re: [PATCH v2 13/13] perf tools: add feature header record to pipe-mode

On Tue, May 23, 2017 at 12:48:53AM -0700, David Carrillo-Cisneros wrote:

SNIP

> +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());

we have page_size global variable for this

jirka

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

#1650285 — Re: [PATCH v2 13/13] perf tools: add feature header record to pipe-mode

On Tue, May 23, 2017 at 12:48:53AM -0700, David Carrillo-Cisneros wrote:

SNIP

>  };
>  
> @@ -488,6 +489,12 @@ struct time_conv_event {
>  	u64 time_zero;
>  };
>  
> +struct feature_event {
> +	struct perf_event_header header;
> +	u64 header_id;
> +	char data[]; /* size bytes of raw data specific to the feature */
> +};

please align the members, like it's in structs around

struct feature_event {
	struct perf_event_header	header;
	u64				header_id;
	char				data[];
}

thanks,
jirka

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web