: perf: reduce data loss when profiling highly parallel CPU bound workloads

tools/perf/builtin-record.c +13 -1

@@ -329,7 +469,7 @@ static int record__mmap_evlist(struct record *rec,
 
 	if (perf_evlist__mmap_ex(evlist, opts->mmap_pages,
 				 opts->auxtrace_mmap_pages,
-				 opts->auxtrace_snapshot_mode) < 0) {
+				 opts->auxtrace_snapshot_mode, opts->nr_cblocks) < 0) {
 		if (errno == EPERM) {
 			pr_err("Permission error mapping pages.\n"
 			       "Consider increasing "

@@ -513,6 +653,29 @@ static struct perf_event_header finished_round_event = {
 	.type = PERF_RECORD_FINISHED_ROUND,
 };
 
+#ifdef HAVE_AIO_SUPPORT
+static void record__mmap_read_sync(struct record *rec)
+{
+	int i;
+	struct perf_evlist *evlist = rec->evlist;
+	struct perf_mmap *maps = evlist->mmap;
+
+	if (!rec->opts.nr_cblocks)
+		return;
+
+	for (i = 0; i < evlist->nr_mmaps; i++) {
+		struct perf_mmap *map = &maps[i];

On Tue, Oct 09, 2018 at 11:58:53AM +0300, Alexey Budankov wrote:

SNIP

Quoted 25 lines

tools/perf/builtin-record.c +13 -1

@@ -329,7 +469,7 @@ static int record__mmap_evlist(struct record *rec,
 
 	if (perf_evlist__mmap_ex(evlist, opts->mmap_pages,
 				 opts->auxtrace_mmap_pages,
-				 opts->auxtrace_snapshot_mode) < 0) {
+				 opts->auxtrace_snapshot_mode, opts->nr_cblocks) < 0) {
 		if (errno == EPERM) {
 			pr_err("Permission error mapping pages.\n"
 			       "Consider increasing "

@@ -513,6 +653,29 @@ static struct perf_event_header finished_round_event = {
 	.type = PERF_RECORD_FINISHED_ROUND,
 };
 
+#ifdef HAVE_AIO_SUPPORT
+static void record__mmap_read_sync(struct record *rec)
+{
+	int i;
+	struct perf_evlist *evlist = rec->evlist;
+	struct perf_mmap *maps = evlist->mmap;
+
+	if (!rec->opts.nr_cblocks)
+		return;
+
+	for (i = 0; i < evlist->nr_mmaps; i++) {
+		struct perf_mmap *map = &maps[i];

tools/perf/builtin-record.c static struct perf_event_header finished_round_event = { · line 677

Quoted 10 lines

tools/perf/builtin-record.c +10 -0

+
+		if (map->base)
+			record__aio_sync(map);
+	}
+}
+#else
+static void record__mmap_read_sync(struct record *rec __maybe_unused)
+{
+}
+#endif

please move this up so we have just one 'ifdef HAVE_AIO_SUPPORT' block

and change the name to record__aio_mmap_read_sync, so it's obvious it's aio related, like the rest of the functions you added

thanks,
jirka

tools/perf/builtin-record.c +5 -0

@@ -1378,6 +1566,22 @@ static int parse_clockid(const struct option *opt, const char *str, int unset)
 	return -1;
 }
 
+#ifdef HAVE_AIO_SUPPORT
+static int record__aio_parse(const struct option *opt,
+			     const char *str __maybe_unused,
+			     int unset)
+{

On Tue, Oct 09, 2018 at 11:58:53AM +0300, Alexey Budankov wrote:

SNIP

Quoted 13 lines

tools/perf/builtin-record.c +9 -0

@@ -1378,6 +1566,22 @@ static int parse_clockid(const struct option *opt, const char *str, int unset)
 	return -1;
 }
 
+#ifdef HAVE_AIO_SUPPORT
+static int record__aio_parse(const struct option *opt,
+			     const char *str __maybe_unused,
+			     int unset)
+{
+	struct record_opts *opts = (struct record_opts *)opt->value;
+
+	if (unset)
+		opts->nr_cblocks = 0;

tools/perf/builtin-record.c static int parse_clockid(const struct option *opt, const char *str, int unset) · line 1583

Quoted 6 lines

tools/perf/builtin-record.c +6 -0

+	else
+		opts->nr_cblocks = 1;
+
+	return 0;
+}
+#endif

same, please move this up to have just one 'ifdef HAVE_AIO_SUPPORT' block

thanks,
jirka

(quoted diff) +71 -0

+#ifdef HAVE_AIO_SUPPORT
+int perf_mmap__aio_push(struct perf_mmap *md, void *to,
+			int push(void *to, struct aiocb *cblock, void *buf, size_t size, off_t off),
+			off_t *off)
+{
+	u64 head = perf_mmap__read_head(md);
+	unsigned char *data = md->base + page_size;
+	unsigned long size, size0 = 0;
+	void *buf;
+	int rc = 0;
+
+	rc = perf_mmap__read_init(md);
+	if (rc < 0)
+		return (rc == -EAGAIN) ? 0 : -1;
+
+	/*
+	 * md->base data is copied into md->data buffer to
+	 * release space in the kernel buffer as fast as possible,
+	 * thru perf_mmap__consume() below.
+	 *
+	 * That lets the kernel to proceed with storing more
+	 * profiling data into the kernel buffer earlier than other
+	 * per-cpu kernel buffers are handled.
+	 *
+	 * Coping can be done in two steps in case the chunk of
+	 * profiling data crosses the upper bound of the kernel buffer.
+	 * In this case we first move part of data from md->start
+	 * till the upper bound and then the reminder from the
+	 * beginning of the kernel buffer till the end of
+	 * the data chunk.
+	 */
+
+	size = md->end - md->start;
+
+	if ((md->start & md->mask) + size != (md->end & md->mask)) {
+		buf = &data[md->start & md->mask];
+		size = md->mask + 1 - (md->start & md->mask);
+		md->start += size;
+		memcpy(md->aio.data, buf, size);
+		size0 = size;
+	}
+
+	buf = &data[md->start & md->mask];
+	size = md->end - md->start;
+	md->start += size;
+	memcpy(md->aio.data + size0, buf, size);
+
+	/*
+	 * Increment md->refcount to guard md->data buffer
+	 * from premature deallocation because md object can be
+	 * released earlier than aio write request started
+	 * on mmap->data is complete.
+	 *
+	 * perf_mmap__put() is done at record__aio_complete()
+	 * after started request completion.
+	 */
+	perf_mmap__get(md);
+
+	md->prev = head;
+	perf_mmap__consume(md);
+
+	rc = push(to, &(md->aio.cblock), md->aio.data, size0 + size, *off);
+	if (!rc) {
+		*off += size0 + size;
+	} else {
+		/*
+		 * Decrement md->refcount back if aio write
+		 * operation failed to start.
+		 */
+		perf_mmap__put(md);
+	}

On Tue, Oct 09, 2018 at 11:58:53AM +0300, Alexey Budankov wrote:

SNIP

Quoted 71 lines

(quoted diff) +71 -0

+#ifdef HAVE_AIO_SUPPORT
+int perf_mmap__aio_push(struct perf_mmap *md, void *to,
+			int push(void *to, struct aiocb *cblock, void *buf, size_t size, off_t off),
+			off_t *off)
+{
+	u64 head = perf_mmap__read_head(md);
+	unsigned char *data = md->base + page_size;
+	unsigned long size, size0 = 0;
+	void *buf;
+	int rc = 0;
+
+	rc = perf_mmap__read_init(md);
+	if (rc < 0)
+		return (rc == -EAGAIN) ? 0 : -1;
+
+	/*
+	 * md->base data is copied into md->data buffer to
+	 * release space in the kernel buffer as fast as possible,
+	 * thru perf_mmap__consume() below.
+	 *
+	 * That lets the kernel to proceed with storing more
+	 * profiling data into the kernel buffer earlier than other
+	 * per-cpu kernel buffers are handled.
+	 *
+	 * Coping can be done in two steps in case the chunk of
+	 * profiling data crosses the upper bound of the kernel buffer.
+	 * In this case we first move part of data from md->start
+	 * till the upper bound and then the reminder from the
+	 * beginning of the kernel buffer till the end of
+	 * the data chunk.
+	 */
+
+	size = md->end - md->start;
+
+	if ((md->start & md->mask) + size != (md->end & md->mask)) {
+		buf = &data[md->start & md->mask];
+		size = md->mask + 1 - (md->start & md->mask);
+		md->start += size;
+		memcpy(md->aio.data, buf, size);
+		size0 = size;
+	}
+
+	buf = &data[md->start & md->mask];
+	size = md->end - md->start;
+	md->start += size;
+	memcpy(md->aio.data + size0, buf, size);
+
+	/*
+	 * Increment md->refcount to guard md->data buffer
+	 * from premature deallocation because md object can be
+	 * released earlier than aio write request started
+	 * on mmap->data is complete.
+	 *
+	 * perf_mmap__put() is done at record__aio_complete()
+	 * after started request completion.
+	 */
+	perf_mmap__get(md);
+
+	md->prev = head;
+	perf_mmap__consume(md);
+
+	rc = push(to, &(md->aio.cblock), md->aio.data, size0 + size, *off);
+	if (!rc) {
+		*off += size0 + size;
+	} else {
+		/*
+		 * Decrement md->refcount back if aio write
+		 * operation failed to start.
+		 */
+		perf_mmap__put(md);
+	}

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+	return rc;
+}
+#else
+int perf_mmap__aio_push(struct perf_mmap *md __maybe_unused, void *to __maybe_unused,
+	int push(void *to, struct aiocb *cblock, void *buf, size_t size, off_t off) __maybe_unused,
+	off_t *off __maybe_unused)
+{
+	return 0;
+}

I think you need to put this one in the header as static inline otherwise it'd still appear in the NO_AIO=1 build, like:
[jolsa@krava perf]$ make NO_AIO=1
...
[jolsa@krava perf]$ nm -D perf | grep perf_mmap__aio_push 00000000004c2be0 T perf_mmap__aio_push

change below makes it disappear completely>
jirka

---

tools/perf/util/mmap.c +0 -7

index b176d88b3fcb..8c7516696891 100644
@@ -445,13 +445,6 @@ int perf_mmap__aio_push(struct perf_mmap *md, void *to,
 
 	return rc;
 }
-#else
-int perf_mmap__aio_push(struct perf_mmap *md __maybe_unused, void *to __maybe_unused,
-	int push(void *to, struct aiocb *cblock, void *buf, size_t size, off_t off) __maybe_unused,
-	off_t *off __maybe_unused)
-{
-	return 0;
-}
 #endif
 
 /*

tools/perf/util/mmap.h +4 -0

index 04ff4d2ffdbe..3ccf8c925002 100644
@@ -105,10 +105,21 @@ union perf_event *perf_mmap__read_event(struct perf_mmap *map);
 
 int perf_mmap__push(struct perf_mmap *md, void *to,
 		    int push(struct perf_mmap *map, void *to, void *buf, size_t size));
+
+#ifdef HAVE_AIO_SUPPORT
 int perf_mmap__aio_push(struct perf_mmap *md, void *to,
 			int push(void *to, struct aiocb *cblock, void *buf, size_t size, off_t off),
 			off_t *off);
 
+#else
+static inline int perf_mmap__aio_push(struct perf_mmap *md __maybe_unused, void *to __maybe_unused,

Trace file offset is read once before mmaps iterating loop and written back after all performance data enqueued for aio writing. Trace file offset is incremented linearly after every successful aio write operation.

record__aio_sync() blocks till completion of started AIO operation and then proceeds.

record__mmap_read_sync() implements a barrier for all incomplete aio write requests.

Signed-off-by Alexey Budankov <[email protected]>

---
Changes in v12:
- implemented record__aio_get/set_pos(), record__aio_enabled()
- implemented simple --aio option
Changes in v11:
- replacing the both lseek() syscalls in every loop iteration by the only
two syscalls just before and after the loop at record__mmap_read_evlist() and advancing *in-flight* off file pos value at perf_mmap__aio_push()
Changes in v10:
- avoided lseek() setting file pos back in case of record__aio_write() failure
- compacted code selecting between serial and AIO streaming
- optimized call places of record__mmap_read_sync()
Changes in v9:
- enable AIO streaming only when --aio-cblocks option is specified explicitly Changes in v8:
- split AIO completion check into separate record__aio_complete()
Changes in v6:
- handled errno == EAGAIN case from aio_write();
Changes in v5:
- data loss metrics decreased from 25% to 2x in trialed configuration;
- avoided nanosleep() prior calling aio_suspend();
- switched to per cpu multi record__aio_sync() aio
- record_mmap_read_sync() now does global barrier just before
switching trace file or collection stop;
- resolved livelock on perf record -e intel_pt// -- dd if=/dev/zero of=/dev/null count=100000 Changes in v4:
- converted void *bf to struct perf_mmap *md in signatures
- written comment in perf_mmap__push() just before perf_mmap__get();
- written comment in record__mmap_read_sync() on possible restarting
of aio_write() operation and releasing perf_mmap object after all;
- added perf_mmap__put() for the cases of failed aio_write();
Changes in v3:
- written comments about nanosleep(0.5ms) call prior aio_suspend()
to cope with intrusiveness of its implementation in glibc;
- written comments about rationale behind coping profiling data
into mmap->data buffer;

---
 tools/perf/Documentation/perf-record.txt |   5 +
 tools/perf/builtin-record.c              | 220 ++++++++++++++++++++++++++++++-
 tools/perf/perf.h                        |   1 +
 tools/perf/util/evlist.c                 |   6 +-
 tools/perf/util/evlist.h                 |   2 +-
 tools/perf/util/mmap.c                   |  86 +++++++++++-
 tools/perf/util/mmap.h                   |   5 +
 7 files changed, 316 insertions(+), 9 deletions(-)

tools/perf/Documentation/perf-record.txt +0 -0

index 246dee081efd..5cedb3e75434 100644

Quoted 66 lines · Alexey Budankov

Trace file offset is read once before mmaps iterating loop and written back after all performance data enqueued for aio writing. Trace file offset is incremented linearly after every successful aio write operation.

record__aio_sync() blocks till completion of started AIO operation and then proceeds.

record__mmap_read_sync() implements a barrier for all incomplete aio write requests.

Signed-off-by Alexey Budankov <[email protected]>

---
Changes in v12:
- implemented record__aio_get/set_pos(), record__aio_enabled()
- implemented simple --aio option
Changes in v11:
- replacing the both lseek() syscalls in every loop iteration by the only
two syscalls just before and after the loop at record__mmap_read_evlist() and advancing *in-flight* off file pos value at perf_mmap__aio_push()
Changes in v10:
- avoided lseek() setting file pos back in case of record__aio_write() failure
- compacted code selecting between serial and AIO streaming
- optimized call places of record__mmap_read_sync()
Changes in v9:
- enable AIO streaming only when --aio-cblocks option is specified explicitly Changes in v8:
- split AIO completion check into separate record__aio_complete()
Changes in v6:
- handled errno == EAGAIN case from aio_write();
Changes in v5:
- data loss metrics decreased from 25% to 2x in trialed configuration;
- avoided nanosleep() prior calling aio_suspend();
- switched to per cpu multi record__aio_sync() aio
- record_mmap_read_sync() now does global barrier just before
switching trace file or collection stop;
- resolved livelock on perf record -e intel_pt// -- dd if=/dev/zero of=/dev/null count=100000 Changes in v4:
- converted void *bf to struct perf_mmap *md in signatures
- written comment in perf_mmap__push() just before perf_mmap__get();
- written comment in record__mmap_read_sync() on possible restarting
of aio_write() operation and releasing perf_mmap object after all;
- added perf_mmap__put() for the cases of failed aio_write();
Changes in v3:
- written comments about nanosleep(0.5ms) call prior aio_suspend()
to cope with intrusiveness of its implementation in glibc;
- written comments about rationale behind coping profiling data
into mmap->data buffer;

---
 tools/perf/Documentation/perf-record.txt |   5 +
 tools/perf/builtin-record.c              | 220 ++++++++++++++++++++++++++++++-
 tools/perf/perf.h                        |   1 +
 tools/perf/util/evlist.c                 |   6 +-
 tools/perf/util/evlist.h                 |   2 +-
 tools/perf/util/mmap.c                   |  86 +++++++++++-
 tools/perf/util/mmap.h                   |   5 +
 7 files changed, 316 insertions(+), 9 deletions(-)

tools/perf/Documentation/perf-record.txt +0 -0

index 246dee081efd..5cedb3e75434 100644
@@ -435,6 +435,11 @@ Specify vmlinux path which has debuginfo.
 --buildid-all::
 Record build-id of all DSOs regardless whether it's actually hit or not.
 

tools/perf/Documentation/perf-record.txt Specify vmlinux path which has debuginfo. · line 439

Quoted 2 lines

tools/perf/Documentation/perf-record.txt +2 -0

+--aio::
+Enable asynchronous (Posix AIO) trace writing mode. 

nit, there's an extra whitespace at the end of above line, making the 'git am' to not apply your patch

thanks,
jirka

tools/perf/builtin-record.c +7 -4

@@ -1568,15 +1596,18 @@ static int parse_clockid(const struct option *opt, const char *str, int unset)
 
 #ifdef HAVE_AIO_SUPPORT
 static int record__aio_parse(const struct option *opt,
-			     const char *str __maybe_unused,
+			     const char *str,
 			     int unset)
 {
 	struct record_opts *opts = (struct record_opts *)opt->value;
 
-	if (unset)
+	if (unset) {
 		opts->nr_cblocks = 0;
-	else
-		opts->nr_cblocks = 1;
+	} else {
+		opts->nr_cblocks = strtol(str, NULL, 0);
+		if (!opts->nr_cblocks)
+			opts->nr_cblocks = 1;
+	}
 
 	return 0;
 }

tools/perf/builtin-record.c +2 -0

+	OPT_CALLBACK(0, "aio-cblocks", &record.opts,
+		     "n", "Use <n> control blocks in asynchronous trace writing mode (max: 4)",
 		     record__aio_parse),

On Tue, Oct 09, 2018 at 11:59:31AM +0300, Alexey Budankov wrote:

tools/perf/builtin-record.c static struct option __record_options[] = { · line 1890

Quoted 7 lines

tools/perf/builtin-record.c +0 -2

@@ -1886,8 +1917,8 @@ static struct option __record_options[] = {
 	OPT_BOOLEAN(0, "dry-run", &dry_run,
 		    "Parse options then exit"),
 #ifdef HAVE_AIO_SUPPORT
-	OPT_CALLBACK_NOOPT(0, "aio", &record.opts,
-		     NULL, "Enable asynchronous trace writing mode",

hum, why removing --aio option? I though we will have:

perf record --aio .... # enables aio with default settings
perf record --aio-cblocks=3 ... # enabled aio with aio-cblocks=3

tools/perf/builtin-record.c +1 -0

+				if (perf_mmap__aio_push(map, rec, idx, record__aio_pushfn, &off) != 0) {
 					record__aio_set_pos(trace_fd, off);
 					rc = -1;
 					goto out;

On Tue, Oct 09, 2018 at 11:59:31AM +0300, Alexey Budankov wrote:

SNIP

tools/perf/builtin-record.c static int record__mmap_read_evlist(struct record *rec, struct perf_evlist *evli · line 737

Quoted 12 lines

tools/perf/builtin-record.c +2 -2

@@ -709,12 +729,13 @@ static int record__mmap_read_evlist(struct record *rec, struct perf_evlist *evli
 					goto out;
 				}
 			} else {
+				int idx;
 				/*
 				 * Call record__aio_sync() to wait till map->data buffer
 				 * becomes available after previous aio write request.
 				 */
-				record__aio_sync(map);
-				if (perf_mmap__aio_push(map, rec, record__aio_pushfn, &off) != 0) {
+				idx = record__aio_sync(map, false);

looks like record__aio_sync can return -1,
how is it not breaking the perf_mmap__aio_push call below?

Quoted 5 lines

tools/perf/builtin-record.c +1 -0

+				if (perf_mmap__aio_push(map, rec, idx, record__aio_pushfn, &off) != 0) {
 					record__aio_set_pos(trace_fd, off);
 					rc = -1;
 					goto out;

SNIP