[PATCH v2 0/5] perf session: Extend reader object to allow multiple readers

[PATCH v2 0/5] perf session: Extend reader object to allow multiple readers

[Alexey Bayduraev]

Changes in v2:
- introduced struct decomp_data suggested by Jiri Olsa
- removed unnecessary [PATCH v1 1/6]
- removed unnecessary extra line in [PATCH v2 4/5]
- removed unnecessary reader_state.eof flag in [PATCH v2 5/5]

Patch set adds state info and decompressor object into reader object
that made possible to split reader__process_events function into three
logical parts: init/exit, map/unmap and single event reader which are
used in events reader loop. This approach allows reading multiple trace
files at the same time. 

The design and implementation are based on the prototype [1], [2].

The patch set was separated from [3] and already was rewieved by
Namhyung Kim and Riccardo Mancini. The difference from [3] is that
this patch set keeps reader object allocation on the stack.

Tested:

tools/perf/perf record -o prof.data -- matrix.gcc.g.O3
tools/perf/perf record -o prof.data -z -- matrix.gcc.g.O3
tools/perf/perf report -i prof.data
tools/perf/perf report -i prof.data --call-graph=callee
tools/perf/perf report -i prof.data --stdio --header
tools/perf/perf report -i prof.data -D --header

[1] git clone https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git -b perf/record_threads
[2] https://lore.kernel.org/lkml/20180913125450.21342-1-jolsa@kernel.org/
[3] https://lore.kernel.org/lkml/cover.1629186429.git.alexey.v.bayduraev@linux.intel.com/

Alexey Bayduraev (5):
  perf session: Introduce reader_state in reader object
  perf session: Introduce decompressor in reader object
  perf session: Move init/exit code to separate functions
  perf session: Move map/unmap to separate function
  perf session: Load single file for analysis

 tools/perf/util/session.c | 215 +++++++++++++++++++++++++-------------
 tools/perf/util/session.h |  10 +-
 2 files changed, 151 insertions(+), 74 deletions(-)

-- 
2.19.0

[PATCH v2 1/5] perf session: Introduce reader_state in reader object

[Alexey Bayduraev]

We need all the state info about reader in separate object to load data
from multiple files, so we can keep multiple readers at the same time.
Adding struct reader_state and adding all items that need to be kept.

Suggested-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Namhyung Kim <namhyung@gmail.com>
Reviewed-by: Riccardo Mancini <rickyman7@gmail.com>
Tested-by: Riccardo Mancini <rickyman7@gmail.com>
Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
---
 tools/perf/util/session.c | 74 +++++++++++++++++++++++----------------
 1 file changed, 43 insertions(+), 31 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 069c2cfdd3be..f29b106b1b17 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2165,41 +2165,52 @@ typedef s64 (*reader_cb_t)(struct perf_session *session,
 			   union perf_event *event,
 			   u64 file_offset);
 
+struct reader_state {
+	char	*mmaps[NUM_MMAPS];
+	size_t	 mmap_size;
+	int	 mmap_idx;
+	char	*mmap_cur;
+	u64	 file_pos;
+	u64	 file_offset;
+	u64	 data_size;
+	u64	 head;
+};
+
 struct reader {
 	int		 fd;
 	u64		 data_size;
 	u64		 data_offset;
 	reader_cb_t	 process;
 	bool		 in_place_update;
+	struct reader_state state;
 };
 
 static int
 reader__process_events(struct reader *rd, struct perf_session *session,
 		       struct ui_progress *prog)
 {
-	u64 data_size = rd->data_size;
-	u64 head, page_offset, file_offset, file_pos, size;
-	int err = 0, mmap_prot, mmap_flags, map_idx = 0;
-	size_t	mmap_size;
-	char *buf, *mmaps[NUM_MMAPS];
+	struct reader_state *st = &rd->state;
+	u64 page_offset, size;
+	int err = 0, mmap_prot, mmap_flags;
+	char *buf, **mmaps = st->mmaps;
 	union perf_event *event;
 	s64 skip;
 
 	page_offset = page_size * (rd->data_offset / page_size);
-	file_offset = page_offset;
-	head = rd->data_offset - page_offset;
+	st->file_offset = page_offset;
+	st->head = rd->data_offset - page_offset;
 
-	ui_progress__init_size(prog, data_size, "Processing events...");
+	ui_progress__init_size(prog, rd->data_size, "Processing events...");
 
-	data_size += rd->data_offset;
+	st->data_size = rd->data_size + rd->data_offset;
 
-	mmap_size = MMAP_SIZE;
-	if (mmap_size > data_size) {
-		mmap_size = data_size;
+	st->mmap_size = MMAP_SIZE;
+	if (st->mmap_size > st->data_size) {
+		st->mmap_size = st->data_size;
 		session->one_mmap = true;
 	}
 
-	memset(mmaps, 0, sizeof(mmaps));
+	memset(mmaps, 0, sizeof(st->mmaps));
 
 	mmap_prot  = PROT_READ;
 	mmap_flags = MAP_SHARED;
@@ -2211,35 +2222,36 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 		mmap_flags = MAP_PRIVATE;
 	}
 remap:
-	buf = mmap(NULL, mmap_size, mmap_prot, mmap_flags, rd->fd,
-		   file_offset);
+	buf = mmap(NULL, st->mmap_size, mmap_prot, mmap_flags, rd->fd,
+		   st->file_offset);
 	if (buf == MAP_FAILED) {
 		pr_err("failed to mmap file\n");
 		err = -errno;
 		goto out;
 	}
-	mmaps[map_idx] = buf;
-	map_idx = (map_idx + 1) & (ARRAY_SIZE(mmaps) - 1);
-	file_pos = file_offset + head;
+	mmaps[st->mmap_idx] = st->mmap_cur = buf;
+	st->mmap_idx = (st->mmap_idx + 1) & (ARRAY_SIZE(st->mmaps) - 1);
+	st->file_pos = st->file_offset + st->head;
 	if (session->one_mmap) {
 		session->one_mmap_addr = buf;
-		session->one_mmap_offset = file_offset;
+		session->one_mmap_offset = st->file_offset;
 	}
 
 more:
-	event = fetch_mmaped_event(head, mmap_size, buf, session->header.needs_swap);
+	event = fetch_mmaped_event(st->head, st->mmap_size, st->mmap_cur,
+				   session->header.needs_swap);
 	if (IS_ERR(event))
 		return PTR_ERR(event);
 
 	if (!event) {
-		if (mmaps[map_idx]) {
-			munmap(mmaps[map_idx], mmap_size);
-			mmaps[map_idx] = NULL;
+		if (mmaps[st->mmap_idx]) {
+			munmap(mmaps[st->mmap_idx], st->mmap_size);
+			mmaps[st->mmap_idx] = NULL;
 		}
 
-		page_offset = page_size * (head / page_size);
-		file_offset += page_offset;
-		head -= page_offset;
+		page_offset = page_size * (st->head / page_size);
+		st->file_offset += page_offset;
+		st->head -= page_offset;
 		goto remap;
 	}
 
@@ -2248,9 +2260,9 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 	skip = -EINVAL;
 
 	if (size < sizeof(struct perf_event_header) ||
-	    (skip = rd->process(session, event, file_pos)) < 0) {
+	    (skip = rd->process(session, event, st->file_pos)) < 0) {
 		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d [%s]\n",
-		       file_offset + head, event->header.size,
+		       st->file_offset + st->head, event->header.size,
 		       event->header.type, strerror(-skip));
 		err = skip;
 		goto out;
@@ -2259,8 +2271,8 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 	if (skip)
 		size += skip;
 
-	head += size;
-	file_pos += size;
+	st->head += size;
+	st->file_pos += size;
 
 	err = __perf_session__process_decomp_events(session);
 	if (err)
@@ -2271,7 +2283,7 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 	if (session_done())
 		goto out;
 
-	if (file_pos < data_size)
+	if (st->file_pos < st->data_size)
 		goto more;
 
 out:
-- 
2.19.0

[PATCH v2 2/5] perf session: Introduce decompressor in reader object

[Alexey Bayduraev]

Introducing decompressor data structure with pointers to decomp
objects and to zstd object. We cannot just move session->zstd_data to
decomp_data as session->zstd_data is used not only for decompression.
Adding decompressor data object to reader object and introducing
active_decomp into perf_session object to select current decompressor.
Thus decompression could be executed separately for each data file.

Acked-by: Namhyung Kim <namhyung@gmail.com>
Reviewed-by: Riccardo Mancini <rickyman7@gmail.com>
Tested-by: Riccardo Mancini <rickyman7@gmail.com>
Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
---
 tools/perf/util/session.c | 36 ++++++++++++++++++++++++------------
 tools/perf/util/session.h | 10 ++++++++--
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index f29b106b1b17..50f2ec523ae0 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -44,7 +44,7 @@ static int perf_session__process_compressed_event(struct perf_session *session,
 	size_t decomp_size, src_size;
 	u64 decomp_last_rem = 0;
 	size_t mmap_len, decomp_len = session->header.env.comp_mmap_len;
-	struct decomp *decomp, *decomp_last = session->decomp_last;
+	struct decomp *decomp, *decomp_last = session->active_decomp->decomp_last;
 
 	if (decomp_last) {
 		decomp_last_rem = decomp_last->size - decomp_last->head;
@@ -71,7 +71,7 @@ static int perf_session__process_compressed_event(struct perf_session *session,
 	src = (void *)event + sizeof(struct perf_record_compressed);
 	src_size = event->pack.header.size - sizeof(struct perf_record_compressed);
 
-	decomp_size = zstd_decompress_stream(&(session->zstd_data), src, src_size,
+	decomp_size = zstd_decompress_stream(session->active_decomp->zstd_decomp, src, src_size,
 				&(decomp->data[decomp_last_rem]), decomp_len - decomp_last_rem);
 	if (!decomp_size) {
 		munmap(decomp, mmap_len);
@@ -81,12 +81,12 @@ static int perf_session__process_compressed_event(struct perf_session *session,
 
 	decomp->size += decomp_size;
 
-	if (session->decomp == NULL) {
-		session->decomp = decomp;
-		session->decomp_last = decomp;
+	if (session->active_decomp->decomp == NULL) {
+		session->active_decomp->decomp = decomp;
+		session->active_decomp->decomp_last = decomp;
 	} else {
-		session->decomp_last->next = decomp;
-		session->decomp_last = decomp;
+		session->active_decomp->decomp_last->next = decomp;
+		session->active_decomp->decomp_last = decomp;
 	}
 
 	pr_debug("decomp (B): %zd to %zd\n", src_size, decomp_size);
@@ -197,6 +197,8 @@ struct perf_session *__perf_session__new(struct perf_data *data,
 
 	session->repipe = repipe;
 	session->tool   = tool;
+	session->decomp_data.zstd_decomp = &session->zstd_data;
+	session->active_decomp = &session->decomp_data;
 	INIT_LIST_HEAD(&session->auxtrace_index);
 	machines__init(&session->machines);
 	ordered_events__init(&session->ordered_events,
@@ -276,11 +278,11 @@ static void perf_session__delete_threads(struct perf_session *session)
 	machine__delete_threads(&session->machines.host);
 }
 
-static void perf_session__release_decomp_events(struct perf_session *session)
+static void perf_decomp__release_events(struct decomp *next)
 {
-	struct decomp *next, *decomp;
+	struct decomp *decomp;
 	size_t mmap_len;
-	next = session->decomp;
+
 	do {
 		decomp = next;
 		if (decomp == NULL)
@@ -299,7 +301,7 @@ void perf_session__delete(struct perf_session *session)
 	auxtrace_index__free(&session->auxtrace_index);
 	perf_session__destroy_kernel_maps(session);
 	perf_session__delete_threads(session);
-	perf_session__release_decomp_events(session);
+	perf_decomp__release_events(session->decomp_data.decomp);
 	perf_env__exit(&session->header.env);
 	machines__exit(&session->machines);
 	if (session->data) {
@@ -2117,7 +2119,7 @@ static int __perf_session__process_decomp_events(struct perf_session *session)
 {
 	s64 skip;
 	u64 size, file_pos = 0;
-	struct decomp *decomp = session->decomp_last;
+	struct decomp *decomp = session->active_decomp->decomp_last;
 
 	if (!decomp)
 		return 0;
@@ -2183,6 +2185,8 @@ struct reader {
 	reader_cb_t	 process;
 	bool		 in_place_update;
 	struct reader_state state;
+	struct zstd_data    zstd_data;
+	struct decomp_data  decomp_data;
 };
 
 static int
@@ -2212,6 +2216,10 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 
 	memset(mmaps, 0, sizeof(st->mmaps));
 
+	if (zstd_init(&rd->zstd_data, 0))
+		return -1;
+	rd->decomp_data.zstd_decomp = &rd->zstd_data;
+
 	mmap_prot  = PROT_READ;
 	mmap_flags = MAP_SHARED;
 
@@ -2255,6 +2263,7 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 		goto remap;
 	}
 
+	session->active_decomp = &rd->decomp_data;
 	size = event->header.size;
 
 	skip = -EINVAL;
@@ -2287,6 +2296,9 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 		goto more;
 
 out:
+	session->active_decomp = &session->decomp_data;
+	perf_decomp__release_events(rd->decomp_data.decomp);
+	zstd_fini(&rd->zstd_data);
 	return err;
 }
 
diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
index 5d8bd14a0a39..46c854292ad6 100644
--- a/tools/perf/util/session.h
+++ b/tools/perf/util/session.h
@@ -20,6 +20,12 @@ struct thread;
 struct auxtrace;
 struct itrace_synth_opts;
 
+struct decomp_data {
+	struct decomp	 *decomp;
+	struct decomp	 *decomp_last;
+	struct zstd_data *zstd_decomp;
+};
+
 struct perf_session {
 	struct perf_header	header;
 	struct machines		machines;
@@ -39,8 +45,8 @@ struct perf_session {
 	u64			bytes_transferred;
 	u64			bytes_compressed;
 	struct zstd_data	zstd_data;
-	struct decomp		*decomp;
-	struct decomp		*decomp_last;
+	struct decomp_data	decomp_data;
+	struct decomp_data	*active_decomp;
 };
 
 struct decomp {
-- 
2.19.0

[PATCH v2 3/5] perf session: Move init/exit code to separate functions

[Alexey Bayduraev]

Separating init/exit code into reader__init and reader__exit functions.

Suggested-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Namhyung Kim <namhyung@gmail.com>
Reviewed-by: Riccardo Mancini <rickyman7@gmail.com>
Tested-by: Riccardo Mancini <rickyman7@gmail.com>
Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
---
 tools/perf/util/session.c | 42 ++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 50f2ec523ae0..53cd7a3b5efd 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2190,28 +2190,23 @@ struct reader {
 };
 
 static int
-reader__process_events(struct reader *rd, struct perf_session *session,
-		       struct ui_progress *prog)
+reader__init(struct reader *rd, bool *one_mmap)
 {
 	struct reader_state *st = &rd->state;
-	u64 page_offset, size;
-	int err = 0, mmap_prot, mmap_flags;
-	char *buf, **mmaps = st->mmaps;
-	union perf_event *event;
-	s64 skip;
+	u64 page_offset;
+	char **mmaps = st->mmaps;
 
 	page_offset = page_size * (rd->data_offset / page_size);
 	st->file_offset = page_offset;
 	st->head = rd->data_offset - page_offset;
 
-	ui_progress__init_size(prog, rd->data_size, "Processing events...");
-
 	st->data_size = rd->data_size + rd->data_offset;
 
 	st->mmap_size = MMAP_SIZE;
 	if (st->mmap_size > st->data_size) {
 		st->mmap_size = st->data_size;
-		session->one_mmap = true;
+		if (one_mmap)
+			*one_mmap = true;
 	}
 
 	memset(mmaps, 0, sizeof(st->mmaps));
@@ -2220,6 +2215,27 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 		return -1;
 	rd->decomp_data.zstd_decomp = &rd->zstd_data;
 
+	return 0;
+}
+
+static void
+reader__exit(struct reader *rd)
+{
+	perf_decomp__release_events(rd->decomp_data.decomp);
+	zstd_fini(&rd->zstd_data);
+}
+
+static int
+reader__process_events(struct reader *rd, struct perf_session *session,
+		       struct ui_progress *prog)
+{
+	struct reader_state *st = &rd->state;
+	u64 page_offset, size;
+	int err = 0, mmap_prot, mmap_flags;
+	char *buf, **mmaps = st->mmaps;
+	union perf_event *event;
+	s64 skip;
+
 	mmap_prot  = PROT_READ;
 	mmap_flags = MAP_SHARED;
 
@@ -2297,8 +2313,6 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 
 out:
 	session->active_decomp = &session->decomp_data;
-	perf_decomp__release_events(rd->decomp_data.decomp);
-	zstd_fini(&rd->zstd_data);
 	return err;
 }
 
@@ -2330,6 +2344,9 @@ static int __perf_session__process_events(struct perf_session *session)
 
 	ui_progress__init_size(&prog, rd.data_size, "Processing events...");
 
+	err = reader__init(&rd, &session->one_mmap);
+	if (err)
+		goto out_err;
 	err = reader__process_events(&rd, session, &prog);
 	if (err)
 		goto out_err;
@@ -2352,6 +2369,7 @@ static int __perf_session__process_events(struct perf_session *session)
 	ordered_events__reinit(&session->ordered_events);
 	auxtrace__free_events(session);
 	session->one_mmap = false;
+	reader__exit(&rd);
 	return err;
 }
 
-- 
2.19.0

[PATCH v2 4/5] perf session: Move map/unmap to separate function

[Alexey Bayduraev]

Moving mapping and unmapping code into reader__mmap, so the mmap
code is located together. Moving head/file_offset computation into
reader__mmap function, so all the offset computation is located
together and in one place only.

Suggested-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Namhyung Kim <namhyung@gmail.com>
Reviewed-by: Riccardo Mancini <rickyman7@gmail.com>
Tested-by: Riccardo Mancini <rickyman7@gmail.com>
Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
---
 tools/perf/util/session.c | 61 ++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 26 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 53cd7a3b5efd..6c825e4a9dfe 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2193,13 +2193,9 @@ static int
 reader__init(struct reader *rd, bool *one_mmap)
 {
 	struct reader_state *st = &rd->state;
-	u64 page_offset;
 	char **mmaps = st->mmaps;
 
-	page_offset = page_size * (rd->data_offset / page_size);
-	st->file_offset = page_offset;
-	st->head = rd->data_offset - page_offset;
-
+	st->head = rd->data_offset;
 	st->data_size = rd->data_size + rd->data_offset;
 
 	st->mmap_size = MMAP_SIZE;
@@ -2226,15 +2222,12 @@ reader__exit(struct reader *rd)
 }
 
 static int
-reader__process_events(struct reader *rd, struct perf_session *session,
-		       struct ui_progress *prog)
+reader__mmap(struct reader *rd, struct perf_session *session)
 {
 	struct reader_state *st = &rd->state;
-	u64 page_offset, size;
-	int err = 0, mmap_prot, mmap_flags;
+	int mmap_prot, mmap_flags;
 	char *buf, **mmaps = st->mmaps;
-	union perf_event *event;
-	s64 skip;
+	u64 page_offset;
 
 	mmap_prot  = PROT_READ;
 	mmap_flags = MAP_SHARED;
@@ -2245,20 +2238,45 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 		mmap_prot  |= PROT_WRITE;
 		mmap_flags = MAP_PRIVATE;
 	}
-remap:
+
+	if (mmaps[st->mmap_idx]) {
+		munmap(mmaps[st->mmap_idx], st->mmap_size);
+		mmaps[st->mmap_idx] = NULL;
+	}
+
+	page_offset = page_size * (st->head / page_size);
+	st->file_offset += page_offset;
+	st->head -= page_offset;
+
 	buf = mmap(NULL, st->mmap_size, mmap_prot, mmap_flags, rd->fd,
 		   st->file_offset);
 	if (buf == MAP_FAILED) {
 		pr_err("failed to mmap file\n");
-		err = -errno;
-		goto out;
+		return -errno;
 	}
 	mmaps[st->mmap_idx] = st->mmap_cur = buf;
 	st->mmap_idx = (st->mmap_idx + 1) & (ARRAY_SIZE(st->mmaps) - 1);
 	st->file_pos = st->file_offset + st->head;
+	return 0;
+}
+
+static int
+reader__process_events(struct reader *rd, struct perf_session *session,
+		       struct ui_progress *prog)
+{
+	struct reader_state *st = &rd->state;
+	u64 size;
+	int err = 0;
+	union perf_event *event;
+	s64 skip;
+
+remap:
+	err = reader__mmap(rd, session);
+	if (err)
+		goto out;
 	if (session->one_mmap) {
-		session->one_mmap_addr = buf;
-		session->one_mmap_offset = st->file_offset;
+		session->one_mmap_addr   = rd->state.mmap_cur;
+		session->one_mmap_offset = rd->state.file_offset;
 	}
 
 more:
@@ -2267,17 +2285,8 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 	if (IS_ERR(event))
 		return PTR_ERR(event);
 
-	if (!event) {
-		if (mmaps[st->mmap_idx]) {
-			munmap(mmaps[st->mmap_idx], st->mmap_size);
-			mmaps[st->mmap_idx] = NULL;
-		}
-
-		page_offset = page_size * (st->head / page_size);
-		st->file_offset += page_offset;
-		st->head -= page_offset;
+	if (!event)
 		goto remap;
-	}
 
 	session->active_decomp = &rd->decomp_data;
 	size = event->header.size;
-- 
2.19.0

[PATCH v2 5/5] perf session: Load single file for analysis

[Alexey Bayduraev]

Adding reader EOF return code and moving the check of EOF state to
reader__mmap. Adding reader OK and NODATA return codes to simplify
the code and separating reading code of single event into
reader__read_event function. Introducing read_event/remap loop
in __perf_session__process_events.

Suggested-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Namhyung Kim <namhyung@gmail.com>
Reviewed-by: Riccardo Mancini <rickyman7@gmail.com>
Tested-by: Riccardo Mancini <rickyman7@gmail.com>
Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
---
 tools/perf/util/session.c | 74 +++++++++++++++++++++++++--------------
 1 file changed, 47 insertions(+), 27 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 6c825e4a9dfe..1915714747a1 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2167,6 +2167,12 @@ typedef s64 (*reader_cb_t)(struct perf_session *session,
 			   union perf_event *event,
 			   u64 file_offset);
 
+enum {
+	READER_OK,
+	READER_NODATA,
+	READER_EOF,
+};
+
 struct reader_state {
 	char	*mmaps[NUM_MMAPS];
 	size_t	 mmap_size;
@@ -2229,6 +2235,9 @@ reader__mmap(struct reader *rd, struct perf_session *session)
 	char *buf, **mmaps = st->mmaps;
 	u64 page_offset;
 
+	if (st->file_pos >= st->data_size)
+		return READER_EOF;
+
 	mmap_prot  = PROT_READ;
 	mmap_flags = MAP_SHARED;
 
@@ -2257,36 +2266,26 @@ reader__mmap(struct reader *rd, struct perf_session *session)
 	mmaps[st->mmap_idx] = st->mmap_cur = buf;
 	st->mmap_idx = (st->mmap_idx + 1) & (ARRAY_SIZE(st->mmaps) - 1);
 	st->file_pos = st->file_offset + st->head;
-	return 0;
+	return READER_OK;
 }
 
 static int
-reader__process_events(struct reader *rd, struct perf_session *session,
-		       struct ui_progress *prog)
+reader__read_event(struct reader *rd, struct perf_session *session,
+		   struct ui_progress *prog)
 {
 	struct reader_state *st = &rd->state;
-	u64 size;
-	int err = 0;
+	int err = READER_OK;
 	union perf_event *event;
+	u64 size;
 	s64 skip;
 
-remap:
-	err = reader__mmap(rd, session);
-	if (err)
-		goto out;
-	if (session->one_mmap) {
-		session->one_mmap_addr   = rd->state.mmap_cur;
-		session->one_mmap_offset = rd->state.file_offset;
-	}
-
-more:
 	event = fetch_mmaped_event(st->head, st->mmap_size, st->mmap_cur,
 				   session->header.needs_swap);
 	if (IS_ERR(event))
 		return PTR_ERR(event);
 
 	if (!event)
-		goto remap;
+		return READER_NODATA;
 
 	session->active_decomp = &rd->decomp_data;
 	size = event->header.size;
@@ -2308,18 +2307,12 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 	st->head += size;
 	st->file_pos += size;
 
-	err = __perf_session__process_decomp_events(session);
-	if (err)
-		goto out;
+	skip = __perf_session__process_decomp_events(session);
+	if (skip)
+		err = skip;
 
 	ui_progress__update(prog, size);
 
-	if (session_done())
-		goto out;
-
-	if (st->file_pos < st->data_size)
-		goto more;
-
 out:
 	session->active_decomp = &session->decomp_data;
 	return err;
@@ -2356,9 +2349,36 @@ static int __perf_session__process_events(struct perf_session *session)
 	err = reader__init(&rd, &session->one_mmap);
 	if (err)
 		goto out_err;
-	err = reader__process_events(&rd, session, &prog);
-	if (err)
+
+	err = reader__mmap(&rd, session);
+	if (err < 0) {
 		goto out_err;
+	} else if (err == READER_EOF) {
+		err = -EINVAL;
+		goto out_err;
+	}
+
+	if (session->one_mmap) {
+		session->one_mmap_addr   = rd.state.mmap_cur;
+		session->one_mmap_offset = rd.state.file_offset;
+	}
+
+	while (true) {
+		if (session_done())
+			break;
+
+		err = reader__read_event(&rd, session, &prog);
+		if (err < 0) {
+			goto out_err;
+		} else if (err == READER_NODATA) {
+			err = reader__mmap(&rd, session);
+			if (err < 0)
+				goto out_err;
+			else if (err == READER_EOF)
+				break;
+		}
+	}
+
 	/* do the final flush for ordered samples */
 	err = ordered_events__flush(oe, OE_FLUSH__FINAL);
 	if (err)
-- 
2.19.0

Re: [PATCH v2 1/5] perf session: Introduce reader_state in reader object

[Arnaldo Carvalho de Melo]

Em Tue, Oct 05, 2021 at 01:26:58PM +0300, Alexey Bayduraev escreveu:
> We need all the state info about reader in separate object to load data
> from multiple files, so we can keep multiple readers at the same time.
> Adding struct reader_state and adding all items that need to be kept.

Why not pass 'struct reader' instead? "reader_state" looks too vague,
isn't the existing state the reader state? i.e. things like 'fd',
'data_size', etc in 'struct reader'.

Can we find better names to avoid this confusion?

- Arnaldo
 
> Suggested-by: Jiri Olsa <jolsa@kernel.org>
> Acked-by: Namhyung Kim <namhyung@gmail.com>
> Reviewed-by: Riccardo Mancini <rickyman7@gmail.com>
> Tested-by: Riccardo Mancini <rickyman7@gmail.com>
> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
> ---
>  tools/perf/util/session.c | 74 +++++++++++++++++++++++----------------
>  1 file changed, 43 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 069c2cfdd3be..f29b106b1b17 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -2165,41 +2165,52 @@ typedef s64 (*reader_cb_t)(struct perf_session *session,
>  			   union perf_event *event,
>  			   u64 file_offset);
>  
> +struct reader_state {
> +	char	*mmaps[NUM_MMAPS];
> +	size_t	 mmap_size;
> +	int	 mmap_idx;
> +	char	*mmap_cur;
> +	u64	 file_pos;
> +	u64	 file_offset;
> +	u64	 data_size;
> +	u64	 head;
> +};
> +
>  struct reader {
>  	int		 fd;
>  	u64		 data_size;
>  	u64		 data_offset;
>  	reader_cb_t	 process;
>  	bool		 in_place_update;
> +	struct reader_state state;
>  };
>  
>  static int
>  reader__process_events(struct reader *rd, struct perf_session *session,
>  		       struct ui_progress *prog)
>  {
> -	u64 data_size = rd->data_size;
> -	u64 head, page_offset, file_offset, file_pos, size;
> -	int err = 0, mmap_prot, mmap_flags, map_idx = 0;
> -	size_t	mmap_size;
> -	char *buf, *mmaps[NUM_MMAPS];
> +	struct reader_state *st = &rd->state;
> +	u64 page_offset, size;
> +	int err = 0, mmap_prot, mmap_flags;
> +	char *buf, **mmaps = st->mmaps;
>  	union perf_event *event;
>  	s64 skip;
>  
>  	page_offset = page_size * (rd->data_offset / page_size);
> -	file_offset = page_offset;
> -	head = rd->data_offset - page_offset;
> +	st->file_offset = page_offset;
> +	st->head = rd->data_offset - page_offset;
>  
> -	ui_progress__init_size(prog, data_size, "Processing events...");
> +	ui_progress__init_size(prog, rd->data_size, "Processing events...");
>  
> -	data_size += rd->data_offset;
> +	st->data_size = rd->data_size + rd->data_offset;
>  
> -	mmap_size = MMAP_SIZE;
> -	if (mmap_size > data_size) {
> -		mmap_size = data_size;
> +	st->mmap_size = MMAP_SIZE;
> +	if (st->mmap_size > st->data_size) {
> +		st->mmap_size = st->data_size;
>  		session->one_mmap = true;
>  	}
>  
> -	memset(mmaps, 0, sizeof(mmaps));
> +	memset(mmaps, 0, sizeof(st->mmaps));
>  
>  	mmap_prot  = PROT_READ;
>  	mmap_flags = MAP_SHARED;
> @@ -2211,35 +2222,36 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>  		mmap_flags = MAP_PRIVATE;
>  	}
>  remap:
> -	buf = mmap(NULL, mmap_size, mmap_prot, mmap_flags, rd->fd,
> -		   file_offset);
> +	buf = mmap(NULL, st->mmap_size, mmap_prot, mmap_flags, rd->fd,
> +		   st->file_offset);
>  	if (buf == MAP_FAILED) {
>  		pr_err("failed to mmap file\n");
>  		err = -errno;
>  		goto out;
>  	}
> -	mmaps[map_idx] = buf;
> -	map_idx = (map_idx + 1) & (ARRAY_SIZE(mmaps) - 1);
> -	file_pos = file_offset + head;
> +	mmaps[st->mmap_idx] = st->mmap_cur = buf;
> +	st->mmap_idx = (st->mmap_idx + 1) & (ARRAY_SIZE(st->mmaps) - 1);
> +	st->file_pos = st->file_offset + st->head;
>  	if (session->one_mmap) {
>  		session->one_mmap_addr = buf;
> -		session->one_mmap_offset = file_offset;
> +		session->one_mmap_offset = st->file_offset;
>  	}
>  
>  more:
> -	event = fetch_mmaped_event(head, mmap_size, buf, session->header.needs_swap);
> +	event = fetch_mmaped_event(st->head, st->mmap_size, st->mmap_cur,
> +				   session->header.needs_swap);
>  	if (IS_ERR(event))
>  		return PTR_ERR(event);
>  
>  	if (!event) {
> -		if (mmaps[map_idx]) {
> -			munmap(mmaps[map_idx], mmap_size);
> -			mmaps[map_idx] = NULL;
> +		if (mmaps[st->mmap_idx]) {
> +			munmap(mmaps[st->mmap_idx], st->mmap_size);
> +			mmaps[st->mmap_idx] = NULL;
>  		}
>  
> -		page_offset = page_size * (head / page_size);
> -		file_offset += page_offset;
> -		head -= page_offset;
> +		page_offset = page_size * (st->head / page_size);
> +		st->file_offset += page_offset;
> +		st->head -= page_offset;
>  		goto remap;
>  	}
>  
> @@ -2248,9 +2260,9 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>  	skip = -EINVAL;
>  
>  	if (size < sizeof(struct perf_event_header) ||
> -	    (skip = rd->process(session, event, file_pos)) < 0) {
> +	    (skip = rd->process(session, event, st->file_pos)) < 0) {
>  		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d [%s]\n",
> -		       file_offset + head, event->header.size,
> +		       st->file_offset + st->head, event->header.size,
>  		       event->header.type, strerror(-skip));
>  		err = skip;
>  		goto out;
> @@ -2259,8 +2271,8 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>  	if (skip)
>  		size += skip;
>  
> -	head += size;
> -	file_pos += size;
> +	st->head += size;
> +	st->file_pos += size;
>  
>  	err = __perf_session__process_decomp_events(session);
>  	if (err)
> @@ -2271,7 +2283,7 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>  	if (session_done())
>  		goto out;
>  
> -	if (file_pos < data_size)
> +	if (st->file_pos < st->data_size)
>  		goto more;
>  
>  out:
> -- 
> 2.19.0

Re: [PATCH v2 2/5] perf session: Introduce decompressor in reader object

[Arnaldo Carvalho de Melo]

Em Tue, Oct 05, 2021 at 01:26:59PM +0300, Alexey Bayduraev escreveu:
> Introducing decompressor data structure with pointers to decomp
> objects and to zstd object. We cannot just move session->zstd_data to
> decomp_data as session->zstd_data is used not only for decompression.
> Adding decompressor data object to reader object and introducing
> active_decomp into perf_session object to select current decompressor.
> Thus decompression could be executed separately for each data file.
> 
> Acked-by: Namhyung Kim <namhyung@gmail.com>
> Reviewed-by: Riccardo Mancini <rickyman7@gmail.com>
> Tested-by: Riccardo Mancini <rickyman7@gmail.com>
> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
> ---
>  tools/perf/util/session.c | 36 ++++++++++++++++++++++++------------
>  tools/perf/util/session.h | 10 ++++++++--
>  2 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index f29b106b1b17..50f2ec523ae0 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -44,7 +44,7 @@ static int perf_session__process_compressed_event(struct perf_session *session,
>  	size_t decomp_size, src_size;
>  	u64 decomp_last_rem = 0;
>  	size_t mmap_len, decomp_len = session->header.env.comp_mmap_len;
> -	struct decomp *decomp, *decomp_last = session->decomp_last;
> +	struct decomp *decomp, *decomp_last = session->active_decomp->decomp_last;
>  
>  	if (decomp_last) {
>  		decomp_last_rem = decomp_last->size - decomp_last->head;
> @@ -71,7 +71,7 @@ static int perf_session__process_compressed_event(struct perf_session *session,
>  	src = (void *)event + sizeof(struct perf_record_compressed);
>  	src_size = event->pack.header.size - sizeof(struct perf_record_compressed);
>  
> -	decomp_size = zstd_decompress_stream(&(session->zstd_data), src, src_size,
> +	decomp_size = zstd_decompress_stream(session->active_decomp->zstd_decomp, src, src_size,
>  				&(decomp->data[decomp_last_rem]), decomp_len - decomp_last_rem);
>  	if (!decomp_size) {
>  		munmap(decomp, mmap_len);
> @@ -81,12 +81,12 @@ static int perf_session__process_compressed_event(struct perf_session *session,
>  
>  	decomp->size += decomp_size;
>  
> -	if (session->decomp == NULL) {
> -		session->decomp = decomp;
> -		session->decomp_last = decomp;
> +	if (session->active_decomp->decomp == NULL) {
> +		session->active_decomp->decomp = decomp;
> +		session->active_decomp->decomp_last = decomp;
>  	} else {
> -		session->decomp_last->next = decomp;
> -		session->decomp_last = decomp;
> +		session->active_decomp->decomp_last->next = decomp;
> +		session->active_decomp->decomp_last = decomp;
>  	}

This one is invariant, can it be put after the if/else block, please?

	session->active_decomp->decomp_last = decomp;
>  
>  	pr_debug("decomp (B): %zd to %zd\n", src_size, decomp_size);
> @@ -197,6 +197,8 @@ struct perf_session *__perf_session__new(struct perf_data *data,
>  
>  	session->repipe = repipe;
>  	session->tool   = tool;
> +	session->decomp_data.zstd_decomp = &session->zstd_data;
> +	session->active_decomp = &session->decomp_data;
>  	INIT_LIST_HEAD(&session->auxtrace_index);
>  	machines__init(&session->machines);
>  	ordered_events__init(&session->ordered_events,
> @@ -276,11 +278,11 @@ static void perf_session__delete_threads(struct perf_session *session)
>  	machine__delete_threads(&session->machines.host);
>  }
>  
> -static void perf_session__release_decomp_events(struct perf_session *session)
> +static void perf_decomp__release_events(struct decomp *next)
>  {
> -	struct decomp *next, *decomp;
> +	struct decomp *decomp;
>  	size_t mmap_len;
> -	next = session->decomp;
> +
>  	do {
>  		decomp = next;
>  		if (decomp == NULL)
> @@ -299,7 +301,7 @@ void perf_session__delete(struct perf_session *session)
>  	auxtrace_index__free(&session->auxtrace_index);
>  	perf_session__destroy_kernel_maps(session);
>  	perf_session__delete_threads(session);
> -	perf_session__release_decomp_events(session);
> +	perf_decomp__release_events(session->decomp_data.decomp);
>  	perf_env__exit(&session->header.env);
>  	machines__exit(&session->machines);
>  	if (session->data) {
> @@ -2117,7 +2119,7 @@ static int __perf_session__process_decomp_events(struct perf_session *session)
>  {
>  	s64 skip;
>  	u64 size, file_pos = 0;
> -	struct decomp *decomp = session->decomp_last;
> +	struct decomp *decomp = session->active_decomp->decomp_last;
>  
>  	if (!decomp)
>  		return 0;
> @@ -2183,6 +2185,8 @@ struct reader {
>  	reader_cb_t	 process;
>  	bool		 in_place_update;
>  	struct reader_state state;
> +	struct zstd_data    zstd_data;
> +	struct decomp_data  decomp_data;
>  };
>  
>  static int
> @@ -2212,6 +2216,10 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>  
>  	memset(mmaps, 0, sizeof(st->mmaps));
>  
> +	if (zstd_init(&rd->zstd_data, 0))
> +		return -1;
> +	rd->decomp_data.zstd_decomp = &rd->zstd_data;
> +
>  	mmap_prot  = PROT_READ;
>  	mmap_flags = MAP_SHARED;
>  
> @@ -2255,6 +2263,7 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>  		goto remap;
>  	}
>  
> +	session->active_decomp = &rd->decomp_data;
>  	size = event->header.size;
>  
>  	skip = -EINVAL;
> @@ -2287,6 +2296,9 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>  		goto more;
>  
>  out:
> +	session->active_decomp = &session->decomp_data;
> +	perf_decomp__release_events(rd->decomp_data.decomp);
> +	zstd_fini(&rd->zstd_data);
>  	return err;
>  }
>  
> diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
> index 5d8bd14a0a39..46c854292ad6 100644
> --- a/tools/perf/util/session.h
> +++ b/tools/perf/util/session.h
> @@ -20,6 +20,12 @@ struct thread;
>  struct auxtrace;
>  struct itrace_synth_opts;
>  
> +struct decomp_data {
> +	struct decomp	 *decomp;
> +	struct decomp	 *decomp_last;
> +	struct zstd_data *zstd_decomp;
> +};

using _data like above is too vague, and I saw that in some places you
used "perf_decomp_", wouldn't the above be better described as "struct
perf_decomp"?

> +
>  struct perf_session {
>  	struct perf_header	header;
>  	struct machines		machines;
> @@ -39,8 +45,8 @@ struct perf_session {
>  	u64			bytes_transferred;
>  	u64			bytes_compressed;
>  	struct zstd_data	zstd_data;
> -	struct decomp		*decomp;
> -	struct decomp		*decomp_last;
> +	struct decomp_data	decomp_data;
> +	struct decomp_data	*active_decomp;
>  };
>  
>  struct decomp {
> -- 
> 2.19.0

-- 

- Arnaldo

Re: [PATCH v2 5/5] perf session: Load single file for analysis

[Arnaldo Carvalho de Melo]

Em Tue, Oct 05, 2021 at 01:27:02PM +0300, Alexey Bayduraev escreveu:
> Adding reader EOF return code and moving the check of EOF state to
> reader__mmap. Adding reader OK and NODATA return codes to simplify
> the code and separating reading code of single event into
> reader__read_event function. Introducing read_event/remap loop
> in __perf_session__process_events.

You are listing a series of changes done into just one cset, isn't it
possible to break this down to ease review (now and when looking for
bugs in the future :) )?

- Arnaldo
 
> Suggested-by: Jiri Olsa <jolsa@kernel.org>
> Acked-by: Namhyung Kim <namhyung@gmail.com>
> Reviewed-by: Riccardo Mancini <rickyman7@gmail.com>
> Tested-by: Riccardo Mancini <rickyman7@gmail.com>
> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
> ---
>  tools/perf/util/session.c | 74 +++++++++++++++++++++++++--------------
>  1 file changed, 47 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 6c825e4a9dfe..1915714747a1 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -2167,6 +2167,12 @@ typedef s64 (*reader_cb_t)(struct perf_session *session,
>  			   union perf_event *event,
>  			   u64 file_offset);
>  
> +enum {
> +	READER_OK,
> +	READER_NODATA,
> +	READER_EOF,
> +};
> +
>  struct reader_state {
>  	char	*mmaps[NUM_MMAPS];
>  	size_t	 mmap_size;
> @@ -2229,6 +2235,9 @@ reader__mmap(struct reader *rd, struct perf_session *session)
>  	char *buf, **mmaps = st->mmaps;
>  	u64 page_offset;
>  
> +	if (st->file_pos >= st->data_size)
> +		return READER_EOF;
> +
>  	mmap_prot  = PROT_READ;
>  	mmap_flags = MAP_SHARED;
>  
> @@ -2257,36 +2266,26 @@ reader__mmap(struct reader *rd, struct perf_session *session)
>  	mmaps[st->mmap_idx] = st->mmap_cur = buf;
>  	st->mmap_idx = (st->mmap_idx + 1) & (ARRAY_SIZE(st->mmaps) - 1);
>  	st->file_pos = st->file_offset + st->head;
> -	return 0;
> +	return READER_OK;
>  }
>  
>  static int
> -reader__process_events(struct reader *rd, struct perf_session *session,
> -		       struct ui_progress *prog)
> +reader__read_event(struct reader *rd, struct perf_session *session,
> +		   struct ui_progress *prog)
>  {
>  	struct reader_state *st = &rd->state;
> -	u64 size;
> -	int err = 0;
> +	int err = READER_OK;
>  	union perf_event *event;
> +	u64 size;
>  	s64 skip;
>  
> -remap:
> -	err = reader__mmap(rd, session);
> -	if (err)
> -		goto out;
> -	if (session->one_mmap) {
> -		session->one_mmap_addr   = rd->state.mmap_cur;
> -		session->one_mmap_offset = rd->state.file_offset;
> -	}
> -
> -more:
>  	event = fetch_mmaped_event(st->head, st->mmap_size, st->mmap_cur,
>  				   session->header.needs_swap);
>  	if (IS_ERR(event))
>  		return PTR_ERR(event);
>  
>  	if (!event)
> -		goto remap;
> +		return READER_NODATA;
>  
>  	session->active_decomp = &rd->decomp_data;
>  	size = event->header.size;
> @@ -2308,18 +2307,12 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>  	st->head += size;
>  	st->file_pos += size;
>  
> -	err = __perf_session__process_decomp_events(session);
> -	if (err)
> -		goto out;
> +	skip = __perf_session__process_decomp_events(session);
> +	if (skip)
> +		err = skip;
>  
>  	ui_progress__update(prog, size);
>  
> -	if (session_done())
> -		goto out;
> -
> -	if (st->file_pos < st->data_size)
> -		goto more;
> -
>  out:
>  	session->active_decomp = &session->decomp_data;
>  	return err;
> @@ -2356,9 +2349,36 @@ static int __perf_session__process_events(struct perf_session *session)
>  	err = reader__init(&rd, &session->one_mmap);
>  	if (err)
>  		goto out_err;
> -	err = reader__process_events(&rd, session, &prog);
> -	if (err)
> +
> +	err = reader__mmap(&rd, session);
> +	if (err < 0) {
>  		goto out_err;
> +	} else if (err == READER_EOF) {
> +		err = -EINVAL;
> +		goto out_err;
> +	}
> +
> +	if (session->one_mmap) {
> +		session->one_mmap_addr   = rd.state.mmap_cur;
> +		session->one_mmap_offset = rd.state.file_offset;
> +	}
> +
> +	while (true) {
> +		if (session_done())
> +			break;
> +
> +		err = reader__read_event(&rd, session, &prog);
> +		if (err < 0) {
> +			goto out_err;
> +		} else if (err == READER_NODATA) {
> +			err = reader__mmap(&rd, session);
> +			if (err < 0)
> +				goto out_err;
> +			else if (err == READER_EOF)
> +				break;
> +		}
> +	}
> +
>  	/* do the final flush for ordered samples */
>  	err = ordered_events__flush(oe, OE_FLUSH__FINAL);
>  	if (err)
> -- 
> 2.19.0

-- 

- Arnaldo