[PATCH 0/4] perf record: Cleanups and mmap-based output

[PATCH 0/4] perf record: Cleanups and mmap-based output

[David Ahern]

I know Jiri is working on cleanups of the output file, but had this
sitting around for a couple of weeks now. Might as well push it out
for the next baseline. The cleanups of perf-record can be taken
independently.

Ingo: I took a look at leveraging the copy_user_nocache and was not able
      to get it to work. I won't have time to come back to it for a while.
      Given that the mmap output already improves perf-trace a lot I would
      like to get the option into 3.13 and come back to the optimization
      later.

David Ahern (4):
  perf record: Refactor feature handling into a separate function
  perf record: Remove advance_output function
  perf record: Remove post_processing_offset variable
  perf record: mmap output file - v3

 tools/perf/Documentation/perf-record.txt |   5 ++
 tools/perf/builtin-record.c              | 137 ++++++++++++++++++++++++++-----
 2 files changed, 123 insertions(+), 19 deletions(-)

-- 
1.8.3.4 (Apple Git-47)

[PATCH 1/4] perf record: Refactor feature handling into a separate function

[David Ahern]

Code move only. No logic changes.

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
---
 tools/perf/builtin-record.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ea4c04f7437e..2932069ad7a8 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -342,9 +342,28 @@ out:
 	return rc;
 }
 
+static void perf_record__init_features(struct perf_record *rec)
+{
+	struct perf_evlist *evsel_list = rec->evlist;
+	struct perf_session *session = rec->session;
+	int feat;
+
+	for (feat = HEADER_FIRST_FEATURE; feat < HEADER_LAST_FEATURE; feat++)
+		perf_header__set_feat(&session->header, feat);
+
+	if (rec->no_buildid)
+		perf_header__clear_feat(&session->header, HEADER_BUILD_ID);
+
+	if (!have_tracepoints(&evsel_list->entries))
+		perf_header__clear_feat(&session->header, HEADER_TRACING_DATA);
+
+	if (!rec->opts.branch_stack)
+		perf_header__clear_feat(&session->header, HEADER_BRANCH_STACK);
+}
+
 static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
 {
-	int err, feat;
+	int err;
 	unsigned long waking = 0;
 	const bool forks = argc > 0;
 	struct machine *machine;
@@ -371,17 +390,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
 
 	rec->session = session;
 
-	for (feat = HEADER_FIRST_FEATURE; feat < HEADER_LAST_FEATURE; feat++)
-		perf_header__set_feat(&session->header, feat);
-
-	if (rec->no_buildid)
-		perf_header__clear_feat(&session->header, HEADER_BUILD_ID);
-
-	if (!have_tracepoints(&evsel_list->entries))
-		perf_header__clear_feat(&session->header, HEADER_TRACING_DATA);
-
-	if (!rec->opts.branch_stack)
-		perf_header__clear_feat(&session->header, HEADER_BRANCH_STACK);
+	perf_record__init_features(rec);
 
 	if (forks) {
 		err = perf_evlist__prepare_workload(evsel_list, &opts->target,
-- 
1.8.3.4 (Apple Git-47)

[PATCH 2/4] perf record: Remove advance_output function

[David Ahern]

1 line function with only 1 user; might as well embed directly.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
---
 tools/perf/builtin-record.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 2932069ad7a8..19c4db6bdd6f 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -77,11 +77,6 @@ struct perf_record {
 	off_t			post_processing_offset;
 };
 
-static void advance_output(struct perf_record *rec, size_t size)
-{
-	rec->bytes_written += size;
-}
-
 static int write_output(struct perf_record *rec, void *buf, size_t size)
 {
 	struct perf_data_file *file = &rec->file;
@@ -461,7 +456,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
 				pr_err("Couldn't record tracing data.\n");
 				goto out_delete_session;
 			}
-			advance_output(rec, err);
+			rec->bytes_written += err;
 		}
 	}
 
-- 
1.8.3.4 (Apple Git-47)

[PATCH 3/4] perf record: Remove post_processing_offset variable

[David Ahern]

Duplicates the data_offset from header in the session.

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Stephane Eranian <eranian@google.com>
---
 tools/perf/builtin-record.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 19c4db6bdd6f..15280b5e5574 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -74,7 +74,6 @@ struct perf_record {
 	bool			no_buildid;
 	bool			no_buildid_cache;
 	long			samples;
-	off_t			post_processing_offset;
 };
 
 static int write_output(struct perf_record *rec, void *buf, size_t size)
@@ -247,13 +246,14 @@ static int process_buildids(struct perf_record *rec)
 {
 	struct perf_data_file *file  = &rec->file;
 	struct perf_session *session = rec->session;
+	u64 start = session->header.data_offset;
 
 	u64 size = lseek(file->fd, 0, SEEK_CUR);
 	if (size == 0)
 		return 0;
 
-	return __perf_session__process_events(session, rec->post_processing_offset,
-					      size - rec->post_processing_offset,
+	return __perf_session__process_events(session, start,
+					      size - start,
 					      size, &build_id__mark_dso_hit_ops);
 }
 
@@ -429,8 +429,6 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
 		goto out_delete_session;
 	}
 
-	rec->post_processing_offset = lseek(file->fd, 0, SEEK_CUR);
-
 	machine = &session->machines.host;
 
 	if (file->is_pipe) {
-- 
1.8.3.4 (Apple Git-47)

[PATCH 4/4] perf record: mmap output file - v3

[David Ahern]

When recording raw_syscalls for the entire system, e.g.,
    perf record -e raw_syscalls:*,sched:sched_switch -a -- sleep 1

you end up with a negative feedback loop as perf itself calls write() fairly
often. This patch handles the problem by mmap'ing the file in chunks of 64M at
a time and copies events from the event buffers to the file avoiding write
system calls.

Before (with write syscall):

    perf record -o /tmp/perf.data -e raw_syscalls:*,sched:sched_switch -a -- sleep 1
    [ perf record: Woken up 0 times to write data ]
    [ perf record: Captured and wrote 81.843 MB /tmp/perf.data (~3575786 samples) ]

After (using mmap):

    perf record -o /tmp/perf.data -e raw_syscalls:*,sched:sched_switch -a -- sleep 1
    [ perf record: Woken up 31 times to write data ]
    [ perf record: Captured and wrote 8.203 MB /tmp/perf.data (~358388 samples) ]

In addition to perf-trace benefits using mmap lowers the overhead of
perf-record. For example,

  perf stat -i -- perf record -g -o /tmp/perf.data openssl speed aes

shows a drop in time, CPU cycles, and instructions all drop by more than a
factor of 3. Jiri also ran a test that showed a big improvement.

v3: Removed use of bytes_at_mmap_start at the stat() that set it
    Added user option to control the size of the mmap for writing file.

v2: Removed msync call before munmap per Jiri's suggestion

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Stephane Eranian <eranian@google.com>

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 tools/perf/Documentation/perf-record.txt |  5 ++
 tools/perf/builtin-record.c              | 97 ++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 052f7c4dc00c..5cd305eb1698 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -201,6 +201,11 @@ abort events and some memory events in precise mode on modern Intel CPUs.
 --transaction::
 Record transaction flags for transaction related events.
 
+--out-pages=::
+	Number of pages to mmap while writing data to file (must be a power of two).
+	Specification can be appended with unit character - B/K/M/G. The
+	size is rounded up to have nearest pages power of two value.
+
 SEE ALSO
 --------
 linkperf:perf-stat[1], linkperf:perf-list[1]
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 15280b5e5574..3cf563eb7896 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -30,6 +30,9 @@
 #include <sched.h>
 #include <sys/mman.h>
 
+/* output file mmap'ed N chunks at a time */
+#define MMAP_OUTPUT_SIZE   (64*1024*1024)
+
 #ifndef HAVE_ON_EXIT_SUPPORT
 #ifndef ATEXIT_MAX
 #define ATEXIT_MAX 32
@@ -65,6 +68,14 @@ static void __handle_on_exit_funcs(void)
 struct perf_record {
 	struct perf_tool	tool;
 	struct perf_record_opts	opts;
+
+	/* for MMAP based file writes */
+	void			*mmap_addr;
+	u64			mmap_offset;     /* current location within mmap */
+	unsigned int		mmap_out_pages;  /* user configurable option */
+	size_t			mmap_out_size;   /* size of mmap segments */
+	bool			use_mmap;
+
 	u64			bytes_written;
 	struct perf_data_file	file;
 	struct perf_evlist	*evlist;
@@ -76,10 +87,68 @@ struct perf_record {
 	long			samples;
 };
 
+static int do_mmap_output(struct perf_record *rec, void *buf, size_t size)
+{
+	struct perf_data_file *file = &rec->file;
+	u64 remaining;
+	off_t offset;
+
+	if (rec->mmap_addr == NULL) {
+do_mmap:
+		offset = rec->session->header.data_offset + rec->bytes_written;
+		if (offset < (ssize_t) rec->mmap_out_size) {
+			rec->mmap_offset = offset;
+			offset = 0;
+		} else
+			rec->mmap_offset = 0;
+
+		/* extend file to include a new mmap segment */
+		if (ftruncate(file->fd, offset + rec->mmap_out_size) != 0) {
+			pr_err("ftruncate failed\n");
+			return -1;
+		}
+
+		rec->mmap_addr = mmap(NULL, rec->mmap_out_size,
+				      PROT_WRITE | PROT_READ, MAP_SHARED,
+				      file->fd, offset);
+
+		if (rec->mmap_addr == MAP_FAILED) {
+			pr_err("mmap failed: %d: %s\n", errno, strerror(errno));
+			/* reset file size */
+			ftruncate(file->fd, offset);
+			return -1;
+		}
+	}
+
+	remaining = rec->mmap_out_size - rec->mmap_offset;
+
+	if (size > remaining) {
+		memcpy(rec->mmap_addr + rec->mmap_offset, buf, remaining);
+		rec->bytes_written += remaining;
+
+		size -= remaining;
+		buf  += remaining;
+
+		munmap(rec->mmap_addr, rec->mmap_out_size);
+		goto do_mmap;
+	}
+
+	if (size) {
+		memcpy(rec->mmap_addr + rec->mmap_offset, buf, size);
+		rec->bytes_written += size;
+		rec->mmap_offset += size;
+	}
+
+	return 0;
+}
+
 static int write_output(struct perf_record *rec, void *buf, size_t size)
 {
 	struct perf_data_file *file = &rec->file;
 
+	if (rec->use_mmap)
+		return do_mmap_output(rec, buf, size);
+
 	while (size) {
 		int ret = write(file->fd, buf, size);
 
@@ -429,6 +498,12 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
 		goto out_delete_session;
 	}
 
+	if (!file->is_pipe && rec->mmap_out_size) {
+		if (rec->mmap_out_pages)
+			rec->mmap_out_size = rec->mmap_out_pages * page_size;
+		rec->use_mmap = true;
+	}
+
 	machine = &session->machines.host;
 
 	if (file->is_pipe) {
@@ -544,6 +619,24 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
 		}
 	}
 
+	if (rec->use_mmap) {
+		off_t len = rec->session->header.data_offset + rec->bytes_written;
+		int fd = rec->file.fd;
+
+		rec->use_mmap = false;
+		munmap(rec->mmap_addr, rec->mmap_out_size);
+		rec->mmap_addr = NULL;
+
+		if (ftruncate(fd, len) != 0)
+			pr_err("ftruncate failed\n");
+
+		/*
+		 * Set output pointer to end of file
+		 * eg., needed for buildid processing
+		 */
+		lseek(fd, len, SEEK_SET);
+	}
+
 	if (quiet || signr == SIGUSR1)
 		return 0;
 
@@ -805,6 +898,7 @@ static struct perf_record record = {
 			.uses_mmap   = true,
 		},
 	},
+	.mmap_out_size = MMAP_OUTPUT_SIZE,
 };
 
 #define CALLCHAIN_HELP "setup and enables call-graph (stack chain/backtrace) recording: "
@@ -891,6 +985,9 @@ const struct option record_options[] = {
 		    "sample by weight (on special events only)"),
 	OPT_BOOLEAN(0, "transaction", &record.opts.sample_transaction,
 		    "sample transaction flags (special events only)"),
+	OPT_CALLBACK(0, "out-pages", &record.mmap_out_pages, "pages",
+		     "number of pages to use for output chunks.",
+		     perf_evlist__parse_mmap_pages),
 	OPT_END()
 };
 
-- 
1.8.3.4 (Apple Git-47)

Re: [PATCH 4/4] perf record: mmap output file - v3

[Ingo Molnar]

* David Ahern <dsahern@gmail.com> wrote:

> +--out-pages=::
> +	Number of pages to mmap while writing data to file (must be a power of two).
> +	Specification can be appended with unit character - B/K/M/G. The
> +	size is rounded up to have nearest pages power of two value.

So why doesn't the code automatically round down (or up) to the next power 
of 2 limit? We use computers to solve problems, not to introduce 
additional ones! ;-)

> +/* output file mmap'ed N chunks at a time */
> +#define MMAP_OUTPUT_SIZE   (64*1024*1024)

I suspect the --out-pages help text should mention that the default is 
64M?

>  struct perf_record {
>  	struct perf_tool	tool;
>  	struct perf_record_opts	opts;
> +
> +	/* for MMAP based file writes */
> +	void			*mmap_addr;
> +	u64			mmap_offset;     /* current location within mmap */
> +	unsigned int		mmap_out_pages;  /* user configurable option */
> +	size_t			mmap_out_size;   /* size of mmap segments */
> +	bool			use_mmap;

So I think it makes sense for such a group of fields to get its own 
record.mmap sub-structure and be referenced via:

	rec->mmap.addr
	rec->mmap.offset
	rec->mmap.out_pages
	...

Such sub-structures make the semantic grouping easier to see, etc.

> +static int do_mmap_output(struct perf_record *rec, void *buf, size_t size)
> +{
> +	struct perf_data_file *file = &rec->file;
> +	u64 remaining;
> +	off_t offset;
> +
> +	if (rec->mmap_addr == NULL) {
> +do_mmap:
> +		offset = rec->session->header.data_offset + rec->bytes_written;
> +		if (offset < (ssize_t) rec->mmap_out_size) {
> +			rec->mmap_offset = offset;
> +			offset = 0;
> +		} else
> +			rec->mmap_offset = 0;

(Nit: unbalanced curly braces.)

> +
> +		/* extend file to include a new mmap segment */
> +		if (ftruncate(file->fd, offset + rec->mmap_out_size) != 0) {
> +			pr_err("ftruncate failed\n");
> +			return -1;
> +		}
> +
> +		rec->mmap_addr = mmap(NULL, rec->mmap_out_size,
> +				      PROT_WRITE | PROT_READ, MAP_SHARED,
> +				      file->fd, offset);
> +
> +		if (rec->mmap_addr == MAP_FAILED) {
> +			pr_err("mmap failed: %d: %s\n", errno, strerror(errno));
> +			/* reset file size */
> +			ftruncate(file->fd, offset);
> +			return -1;
> +		}
> +	}

I think this branch should move into its well-named helper inline 
function.

> +
> +	remaining = rec->mmap_out_size - rec->mmap_offset;
> +
> +	if (size > remaining) {
> +		memcpy(rec->mmap_addr + rec->mmap_offset, buf, remaining);
> +		rec->bytes_written += remaining;
> +
> +		size -= remaining;
> +		buf  += remaining;
> +
> +		munmap(rec->mmap_addr, rec->mmap_out_size);
> +		goto do_mmap;
> +	}
> +
> +	if (size) {
> +		memcpy(rec->mmap_addr + rec->mmap_offset, buf, size);
> +		rec->bytes_written += size;
> +		rec->mmap_offset += size;
> +	}

One-one line of comment that explains what these two branches do would be 
nice.

>  static int write_output(struct perf_record *rec, void *buf, size_t size)
>  {
>  	struct perf_data_file *file = &rec->file;
>  
> +	if (rec->use_mmap)
> +		return do_mmap_output(rec, buf, size);
> +
>  	while (size) {
>  		int ret = write(file->fd, buf, size);

I think to make it symmetric, the !use_mmap case should be moved into a 
helper inline function as well. That way write_output() is just a 
meta-function calling handlers, not a mixture of real logic and 
demultiplexing of operations ...

> @@ -429,6 +498,12 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
>  		goto out_delete_session;
>  	}
>  
> +	if (!file->is_pipe && rec->mmap_out_size) {
> +		if (rec->mmap_out_pages)
> +			rec->mmap_out_size = rec->mmap_out_pages * page_size;
> +		rec->use_mmap = true;
> +	}

This should move into a separate inline as well, named mmap_init() or so.

> @@ -544,6 +619,24 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
>  		}
>  	}
>  
> +	if (rec->use_mmap) {
> +		off_t len = rec->session->header.data_offset + rec->bytes_written;
> +		int fd = rec->file.fd;
> +
> +		rec->use_mmap = false;
> +		munmap(rec->mmap_addr, rec->mmap_out_size);
> +		rec->mmap_addr = NULL;
> +
> +		if (ftruncate(fd, len) != 0)
> +			pr_err("ftruncate failed\n");
> +
> +		/*
> +		 * Set output pointer to end of file
> +		 * eg., needed for buildid processing
> +		 */
> +		lseek(fd, len, SEEK_SET);
> +	}

This should move into an inline as well. We really hate large, mixed-role 
functions! :-)

> +	OPT_CALLBACK(0, "out-pages", &record.mmap_out_pages, "pages",
> +		     "number of pages to use for output chunks.",
> +		     perf_evlist__parse_mmap_pages),

Nit: the short explanation here doesn't mention it at all to the user that 
these 'out pages' are used in mmap.

Shouldn't it say:

		     "number of pages mmap()ed for output chunks."

?

Also, what happens if a user sets it to zero?

Thanks,

	Ingo

Re: [PATCH 1/4] perf record: Refactor feature handling into a separate function

[Ingo Molnar]

* David Ahern <dsahern@gmail.com> wrote:

> Code move only. No logic changes.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung.kim@lge.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Stephane Eranian <eranian@google.com>
> ---
>  tools/perf/builtin-record.c | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index ea4c04f7437e..2932069ad7a8 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -342,9 +342,28 @@ out:
>  	return rc;
>  }
>  
> +static void perf_record__init_features(struct perf_record *rec)
> +{
> +	struct perf_evlist *evsel_list = rec->evlist;
> +	struct perf_session *session = rec->session;
> +	int feat;
> +
> +	for (feat = HEADER_FIRST_FEATURE; feat < HEADER_LAST_FEATURE; feat++)
> +		perf_header__set_feat(&session->header, feat);
> +
> +	if (rec->no_buildid)
> +		perf_header__clear_feat(&session->header, HEADER_BUILD_ID);
> +
> +	if (!have_tracepoints(&evsel_list->entries))
> +		perf_header__clear_feat(&session->header, HEADER_TRACING_DATA);
> +
> +	if (!rec->opts.branch_stack)
> +		perf_header__clear_feat(&session->header, HEADER_BRANCH_STACK);
> +}
> +
>  static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
>  {
> -	int err, feat;
> +	int err;
>  	unsigned long waking = 0;
>  	const bool forks = argc > 0;
>  	struct machine *machine;
> @@ -371,17 +390,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
>  
>  	rec->session = session;
>  
> -	for (feat = HEADER_FIRST_FEATURE; feat < HEADER_LAST_FEATURE; feat++)
> -		perf_header__set_feat(&session->header, feat);
> -
> -	if (rec->no_buildid)
> -		perf_header__clear_feat(&session->header, HEADER_BUILD_ID);
> -
> -	if (!have_tracepoints(&evsel_list->entries))
> -		perf_header__clear_feat(&session->header, HEADER_TRACING_DATA);
> -
> -	if (!rec->opts.branch_stack)
> -		perf_header__clear_feat(&session->header, HEADER_BRANCH_STACK);
> +	perf_record__init_features(rec);
>  
>  	if (forks) {
>  		err = perf_evlist__prepare_workload(evsel_list, &opts->target,
> -- 
> 1.8.3.4 (Apple Git-47)

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH 2/4] perf record: Remove advance_output function

[Ingo Molnar]

* David Ahern <dsahern@gmail.com> wrote:

> 1 line function with only 1 user; might as well embed directly.
> 
> Suggested-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung.kim@lge.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Stephane Eranian <eranian@google.com>
> ---
>  tools/perf/builtin-record.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 2932069ad7a8..19c4db6bdd6f 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -77,11 +77,6 @@ struct perf_record {
>  	off_t			post_processing_offset;
>  };
>  
> -static void advance_output(struct perf_record *rec, size_t size)
> -{
> -	rec->bytes_written += size;
> -}
> -
>  static int write_output(struct perf_record *rec, void *buf, size_t size)
>  {
>  	struct perf_data_file *file = &rec->file;
> @@ -461,7 +456,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
>  				pr_err("Couldn't record tracing data.\n");
>  				goto out_delete_session;
>  			}
> -			advance_output(rec, err);
> +			rec->bytes_written += err;
>  		}
>  	}

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH 3/4] perf record: Remove post_processing_offset variable

[Ingo Molnar]

* David Ahern <dsahern@gmail.com> wrote:

> Duplicates the data_offset from header in the session.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Stephane Eranian <eranian@google.com>
> ---
>  tools/perf/builtin-record.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 19c4db6bdd6f..15280b5e5574 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -74,7 +74,6 @@ struct perf_record {
>  	bool			no_buildid;
>  	bool			no_buildid_cache;
>  	long			samples;
> -	off_t			post_processing_offset;
>  };
>  
>  static int write_output(struct perf_record *rec, void *buf, size_t size)
> @@ -247,13 +246,14 @@ static int process_buildids(struct perf_record *rec)
>  {
>  	struct perf_data_file *file  = &rec->file;
>  	struct perf_session *session = rec->session;
> +	u64 start = session->header.data_offset;
>  
>  	u64 size = lseek(file->fd, 0, SEEK_CUR);
>  	if (size == 0)
>  		return 0;
>  
> -	return __perf_session__process_events(session, rec->post_processing_offset,
> -					      size - rec->post_processing_offset,
> +	return __perf_session__process_events(session, start,
> +					      size - start,
>  					      size, &build_id__mark_dso_hit_ops);
>  }
>  
> @@ -429,8 +429,6 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
>  		goto out_delete_session;
>  	}
>  
> -	rec->post_processing_offset = lseek(file->fd, 0, SEEK_CUR);
> -
>  	machine = &session->machines.host;
>  
>  	if (file->is_pipe) {
> -- 
> 1.8.3.4 (Apple Git-47)

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH 0/4] perf record: Cleanups and mmap-based output

[Ingo Molnar]

* David Ahern <dsahern@gmail.com> wrote:

> I know Jiri is working on cleanups of the output file, but had this
> sitting around for a couple of weeks now. Might as well push it out
> for the next baseline. The cleanups of perf-record can be taken
> independently.
> 
> Ingo: I took a look at leveraging the copy_user_nocache and was not able
>       to get it to work. I won't have time to come back to it for a while.
>       Given that the mmap output already improves perf-trace a lot I would
>       like to get the option into 3.13 and come back to the optimization
>       later.

That's OK - your feature is already a big step forward!

1-3 look good to me and 4/4 needs some minor cleanups (see my review 
mail).

Thanks,

	Ingo

Re: [PATCH 0/4] perf record: Cleanups and mmap-based output

[Jiri Olsa]

On Wed, Nov 06, 2013 at 11:41:33AM -0700, David Ahern wrote:
> I know Jiri is working on cleanups of the output file, but had this
> sitting around for a couple of weeks now. Might as well push it out
> for the next baseline. The cleanups of perf-record can be taken
> independently.

np, I'll rebase once your change is in

jirka

Re: [PATCH 0/4] perf record: Cleanups and mmap-based output

[Jiri Olsa]

On Wed, Nov 06, 2013 at 11:41:33AM -0700, David Ahern wrote:
> I know Jiri is working on cleanups of the output file, but had this
> sitting around for a couple of weeks now. Might as well push it out
> for the next baseline. The cleanups of perf-record can be taken
> independently.
> 
> Ingo: I took a look at leveraging the copy_user_nocache and was not able
>       to get it to work. I won't have time to come back to it for a while.
>       Given that the mmap output already improves perf-trace a lot I would
>       like to get the option into 3.13 and come back to the optimization
>       later.
> 
> David Ahern (4):
>   perf record: Refactor feature handling into a separate function
>   perf record: Remove advance_output function
>   perf record: Remove post_processing_offset variable
>   perf record: mmap output file - v3
> 
>  tools/perf/Documentation/perf-record.txt |   5 ++
>  tools/perf/builtin-record.c              | 137 ++++++++++++++++++++++++++-----
>  2 files changed, 123 insertions(+), 19 deletions(-)
> 

hi,
got this on F19:

  CC       builtin-record.o
builtin-record.c: In function ‘do_mmap_output’:
builtin-record.c:118:13: error: ignoring return value of ‘ftruncate’, declared with attribute warn_unused_result [-Werror=unused-result]
    ftruncate(file->fd, offset);
             ^
cc1: all warnings being treated as errors


jirka

[tip:perf/core] perf record: Refactor feature handling into a separate function

[tip-bot for David Ahern]

Commit-ID:  57706abc19afc60f0b629af839d2ebee17739f59
Gitweb:     http://git.kernel.org/tip/57706abc19afc60f0b629af839d2ebee17739f59
Author:     David Ahern <dsahern@gmail.com>
AuthorDate: Wed, 6 Nov 2013 11:41:34 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 7 Nov 2013 10:42:26 -0300

perf record: Refactor feature handling into a separate function

Code move only. No logic changes.

Signed-off-by: David Ahern <dsahern@gmail.com>
Acked-by: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1383763297-27066-2-git-send-email-dsahern@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ea4c04f..2932069 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -342,9 +342,28 @@ out:
 	return rc;
 }
 
+static void perf_record__init_features(struct perf_record *rec)
+{
+	struct perf_evlist *evsel_list = rec->evlist;
+	struct perf_session *session = rec->session;
+	int feat;
+
+	for (feat = HEADER_FIRST_FEATURE; feat < HEADER_LAST_FEATURE; feat++)
+		perf_header__set_feat(&session->header, feat);
+
+	if (rec->no_buildid)
+		perf_header__clear_feat(&session->header, HEADER_BUILD_ID);
+
+	if (!have_tracepoints(&evsel_list->entries))
+		perf_header__clear_feat(&session->header, HEADER_TRACING_DATA);
+
+	if (!rec->opts.branch_stack)
+		perf_header__clear_feat(&session->header, HEADER_BRANCH_STACK);
+}
+
 static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
 {
-	int err, feat;
+	int err;
 	unsigned long waking = 0;
 	const bool forks = argc > 0;
 	struct machine *machine;
@@ -371,17 +390,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
 
 	rec->session = session;
 
-	for (feat = HEADER_FIRST_FEATURE; feat < HEADER_LAST_FEATURE; feat++)
-		perf_header__set_feat(&session->header, feat);
-
-	if (rec->no_buildid)
-		perf_header__clear_feat(&session->header, HEADER_BUILD_ID);
-
-	if (!have_tracepoints(&evsel_list->entries))
-		perf_header__clear_feat(&session->header, HEADER_TRACING_DATA);
-
-	if (!rec->opts.branch_stack)
-		perf_header__clear_feat(&session->header, HEADER_BRANCH_STACK);
+	perf_record__init_features(rec);
 
 	if (forks) {
 		err = perf_evlist__prepare_workload(evsel_list, &opts->target,

[tip:perf/core] perf record: Remove advance_output function

[tip-bot for David Ahern]

Commit-ID:  f34b9001f9a2f6fa41d3582fe515d194cc86bfb2
Gitweb:     http://git.kernel.org/tip/f34b9001f9a2f6fa41d3582fe515d194cc86bfb2
Author:     David Ahern <dsahern@gmail.com>
AuthorDate: Wed, 6 Nov 2013 11:41:35 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 7 Nov 2013 10:43:15 -0300

perf record: Remove advance_output function

1 line function with only 1 user; might as well embed directly.

Signed-off-by: David Ahern <dsahern@gmail.com>
Suggested-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1383763297-27066-3-git-send-email-dsahern@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 2932069..19c4db6 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -77,11 +77,6 @@ struct perf_record {
 	off_t			post_processing_offset;
 };
 
-static void advance_output(struct perf_record *rec, size_t size)
-{
-	rec->bytes_written += size;
-}
-
 static int write_output(struct perf_record *rec, void *buf, size_t size)
 {
 	struct perf_data_file *file = &rec->file;
@@ -461,7 +456,7 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
 				pr_err("Couldn't record tracing data.\n");
 				goto out_delete_session;
 			}
-			advance_output(rec, err);
+			rec->bytes_written += err;
 		}
 	}
 

[tip:perf/core] perf record: Remove post_processing_offset variable

[tip-bot for David Ahern]

Commit-ID:  7ab75cffd6a1b2195944b8522673522f09e7fcb0
Gitweb:     http://git.kernel.org/tip/7ab75cffd6a1b2195944b8522673522f09e7fcb0
Author:     David Ahern <dsahern@gmail.com>
AuthorDate: Wed, 6 Nov 2013 11:41:36 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 7 Nov 2013 11:01:59 -0300

perf record: Remove post_processing_offset variable

Duplicates the data_offset from header in the session.

Signed-off-by: David Ahern <dsahern@gmail.com>
Acked-by: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1383763297-27066-4-git-send-email-dsahern@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 19c4db6..15280b5 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -74,7 +74,6 @@ struct perf_record {
 	bool			no_buildid;
 	bool			no_buildid_cache;
 	long			samples;
-	off_t			post_processing_offset;
 };
 
 static int write_output(struct perf_record *rec, void *buf, size_t size)
@@ -247,13 +246,14 @@ static int process_buildids(struct perf_record *rec)
 {
 	struct perf_data_file *file  = &rec->file;
 	struct perf_session *session = rec->session;
+	u64 start = session->header.data_offset;
 
 	u64 size = lseek(file->fd, 0, SEEK_CUR);
 	if (size == 0)
 		return 0;
 
-	return __perf_session__process_events(session, rec->post_processing_offset,
-					      size - rec->post_processing_offset,
+	return __perf_session__process_events(session, start,
+					      size - start,
 					      size, &build_id__mark_dso_hit_ops);
 }
 
@@ -429,8 +429,6 @@ static int __cmd_record(struct perf_record *rec, int argc, const char **argv)
 		goto out_delete_session;
 	}
 
-	rec->post_processing_offset = lseek(file->fd, 0, SEEK_CUR);
-
 	machine = &session->machines.host;
 
 	if (file->is_pipe) {

Re: [PATCH 0/4] perf record: Cleanups and mmap-based output

[David Ahern]

On 11/7/13, 6:21 AM, Jiri Olsa wrote:
>
>    CC       builtin-record.o
> builtin-record.c: In function ‘do_mmap_output’:
> builtin-record.c:118:13: error: ignoring return value of ‘ftruncate’, declared with attribute warn_unused_result [-Werror=unused-result]
>      ftruncate(file->fd, offset);
>               ^
> cc1: all warnings being treated as errors
>

oops, fixed that and forgot to fold into the patch. Will take care of it 
on next round.

David

Re: [PATCH 4/4] perf record: mmap output file - v3

[David Ahern]

On 11/7/13, 1:03 AM, Ingo Molnar wrote:
>
> * David Ahern <dsahern@gmail.com> wrote:
>
>> +--out-pages=::
>> +	Number of pages to mmap while writing data to file (must be a power of two).
>> +	Specification can be appended with unit character - B/K/M/G. The
>> +	size is rounded up to have nearest pages power of two value.
>
> So why doesn't the code automatically round down (or up) to the next power
> of 2 limit? We use computers to solve problems, not to introduce
> additional ones! ;-)

sure. It reuses perf_evlist__parse_mmap_pages which rounds so I will 
update the description.



>> +static int do_mmap_output(struct perf_record *rec, void *buf, size_t size)
>> +{
>> +	struct perf_data_file *file = &rec->file;
>> +	u64 remaining;
>> +	off_t offset;
>> +
>> +	if (rec->mmap_addr == NULL) {
>> +do_mmap:
>> +		offset = rec->session->header.data_offset + rec->bytes_written;
>> +		if (offset < (ssize_t) rec->mmap_out_size) {
>> +			rec->mmap_offset = offset;
>> +			offset = 0;
>> +		} else
>> +			rec->mmap_offset = 0;
>
> (Nit: unbalanced curly braces.)

I believe checkpatch.pl complains, but will add.


>> +	OPT_CALLBACK(0, "out-pages", &record.mmap_out_pages, "pages",
>> +		     "number of pages to use for output chunks.",
>> +		     perf_evlist__parse_mmap_pages),
>
> Nit: the short explanation here doesn't mention it at all to the user that
> these 'out pages' are used in mmap.
>
> Shouldn't it say:
>
> 		     "number of pages mmap()ed for output chunks."
>
> ?
>
> Also, what happens if a user sets it to zero?

I was intending to use that as a way of disabling the mmap output - go 
back to write(). I'll update the documentation.

Ack on all of the other comments.

David

Re: [PATCH 4/4] perf record: mmap output file - v3

[Ingo Molnar]

* David Ahern <dsahern@gmail.com> wrote:

> >>+do_mmap:
> >>+		offset = rec->session->header.data_offset + rec->bytes_written;
> >>+		if (offset < (ssize_t) rec->mmap_out_size) {
> >>+			rec->mmap_offset = offset;
> >>+			offset = 0;
> >>+		} else
> >>+			rec->mmap_offset = 0;
> >
> >(Nit: unbalanced curly braces.)
> 
> I believe checkpatch.pl complains, but will add.

Hm, if that is so then please report that to the checkpatch folks, the 
warning is bogus and causes worse code to be written.

Thanks,

	Ingo