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

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

[Alexey Bayduraev]

Changes in v3:
- removed struct reader_state in [PATCH v3 1/8]
- fixed repeating code in [PATCH v3 2/8]
- split [PATCH v2 4/5] to [PATCH v3 4/8], [PATCH v3 5/8]
- split [PATCH v2 5/5] to [PATCH v3 6/8] - [PATCH v3 8/8]

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 moves state info and decompressor object into reader object
that made possible to split reader__process_events function into three
logical parts: init, 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].

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 (8):
  perf session: Move all state items to reader object
  perf session: Introduce decompressor in reader object
  perf session: Move init/release code to separate functions
  perf session: Move map code to separate function
  perf session: Move unmap code to reader__mmap
  perf session: Move event read code to separate function
  perf session: Introduce reader return codes
  perf session: Introduce reader EOF function

 tools/perf/util/session.c | 192 ++++++++++++++++++++++++++------------
 tools/perf/util/session.h |  10 +-
 2 files changed, 140 insertions(+), 62 deletions(-)

-- 
2.19.0

[PATCH v3 1/8] perf session: Move all state items to 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.
Moving all items that need to be kept from reader__process_events to
the reader object. Introducing mmap_cur to keep current mapping.

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 | 63 ++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 28 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 069c2cfdd3be..bf73e8c1f15c 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2171,6 +2171,13 @@ struct reader {
 	u64		 data_offset;
 	reader_cb_t	 process;
 	bool		 in_place_update;
+	char		 *mmaps[NUM_MMAPS];
+	size_t		 mmap_size;
+	int		 mmap_idx;
+	char		 *mmap_cur;
+	u64		 file_pos;
+	u64		 file_offset;
+	u64		 head;
 };
 
 static int
@@ -2178,28 +2185,27 @@ 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];
+	u64 page_offset, size;
+	int err = 0, mmap_prot, mmap_flags;
+	char *buf, **mmaps = rd->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;
+	rd->file_offset = page_offset;
+	rd->head = rd->data_offset - page_offset;
 
 	ui_progress__init_size(prog, data_size, "Processing events...");
 
 	data_size += rd->data_offset;
 
-	mmap_size = MMAP_SIZE;
-	if (mmap_size > data_size) {
-		mmap_size = data_size;
+	rd->mmap_size = MMAP_SIZE;
+	if (rd->mmap_size > data_size) {
+		rd->mmap_size = data_size;
 		session->one_mmap = true;
 	}
 
-	memset(mmaps, 0, sizeof(mmaps));
+	memset(mmaps, 0, sizeof(rd->mmaps));
 
 	mmap_prot  = PROT_READ;
 	mmap_flags = MAP_SHARED;
@@ -2211,35 +2217,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, rd->mmap_size, mmap_prot, mmap_flags, rd->fd,
+		   rd->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[rd->mmap_idx] = rd->mmap_cur = buf;
+	rd->mmap_idx = (rd->mmap_idx + 1) & (ARRAY_SIZE(rd->mmaps) - 1);
+	rd->file_pos = rd->file_offset + rd->head;
 	if (session->one_mmap) {
 		session->one_mmap_addr = buf;
-		session->one_mmap_offset = file_offset;
+		session->one_mmap_offset = rd->file_offset;
 	}
 
 more:
-	event = fetch_mmaped_event(head, mmap_size, buf, session->header.needs_swap);
+	event = fetch_mmaped_event(rd->head, rd->mmap_size, rd->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[rd->mmap_idx]) {
+			munmap(mmaps[rd->mmap_idx], rd->mmap_size);
+			mmaps[rd->mmap_idx] = NULL;
 		}
 
-		page_offset = page_size * (head / page_size);
-		file_offset += page_offset;
-		head -= page_offset;
+		page_offset = page_size * (rd->head / page_size);
+		rd->file_offset += page_offset;
+		rd->head -= page_offset;
 		goto remap;
 	}
 
@@ -2248,9 +2255,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, rd->file_pos)) < 0) {
 		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d [%s]\n",
-		       file_offset + head, event->header.size,
+		       rd->file_offset + rd->head, event->header.size,
 		       event->header.type, strerror(-skip));
 		err = skip;
 		goto out;
@@ -2259,8 +2266,8 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 	if (skip)
 		size += skip;
 
-	head += size;
-	file_pos += size;
+	rd->head += size;
+	rd->file_pos += size;
 
 	err = __perf_session__process_decomp_events(session);
 	if (err)
@@ -2271,7 +2278,7 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 	if (session_done())
 		goto out;
 
-	if (file_pos < data_size)
+	if (rd->file_pos < data_size)
 		goto more;
 
 out:
-- 
2.19.0

[PATCH v3 2/8] 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 | 39 +++++++++++++++++++++++++--------------
 tools/perf/util/session.h | 10 ++++++++--
 2 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index bf73e8c1f15c..c3930c49da7a 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,13 +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;
-	} else {
-		session->decomp_last->next = decomp;
-		session->decomp_last = decomp;
-	}
+	if (session->active_decomp->decomp == NULL)
+		session->active_decomp->decomp = decomp;
+	else
+		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 +196,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 +277,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 +300,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 +2118,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;
@@ -2178,6 +2179,8 @@ struct reader {
 	u64		 file_pos;
 	u64		 file_offset;
 	u64		 head;
+	struct zstd_data   zstd_data;
+	struct decomp_data decomp_data;
 };
 
 static int
@@ -2207,6 +2210,10 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 
 	memset(mmaps, 0, sizeof(rd->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;
 
@@ -2250,6 +2257,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;
@@ -2282,6 +2290,7 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 		goto more;
 
 out:
+	session->active_decomp = &session->decomp_data;
 	return err;
 }
 
@@ -2334,6 +2343,8 @@ static int __perf_session__process_events(struct perf_session *session)
 	 */
 	ordered_events__reinit(&session->ordered_events);
 	auxtrace__free_events(session);
+	perf_decomp__release_events(rd.decomp_data.decomp);
+	zstd_fini(&rd.zstd_data);
 	session->one_mmap = false;
 	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 v3 3/8] perf session: Move init/release code to separate functions

[Alexey Bayduraev]

Separating init/release code into reader__init and reader__release_decomp
functions. Removing a duplicate call to ui_progress__init_size, the same
call can be found in __perf_session__process_events. For multiple traces
ui_progress should be initialized by total size before reader__init calls.

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 | 44 +++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index c3930c49da7a..00417b0d29ec 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2184,28 +2184,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)
 {
 	u64 data_size = rd->data_size;
-	u64 page_offset, size;
-	int err = 0, mmap_prot, mmap_flags;
-	char *buf, **mmaps = rd->mmaps;
-	union perf_event *event;
-	s64 skip;
+	u64 page_offset;
+	char **mmaps = rd->mmaps;
 
 	page_offset = page_size * (rd->data_offset / page_size);
 	rd->file_offset = page_offset;
 	rd->head = rd->data_offset - page_offset;
 
-	ui_progress__init_size(prog, data_size, "Processing events...");
-
 	data_size += rd->data_offset;
 
 	rd->mmap_size = MMAP_SIZE;
 	if (rd->mmap_size > data_size) {
 		rd->mmap_size = data_size;
-		session->one_mmap = true;
+		if (one_mmap)
+			*one_mmap = true;
 	}
 
 	memset(mmaps, 0, sizeof(rd->mmaps));
@@ -2214,6 +2209,30 @@ 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__release_decomp(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)
+{
+	u64 page_offset, size;
+	int err = 0, mmap_prot, mmap_flags;
+	char *buf, **mmaps = rd->mmaps;
+	union perf_event *event;
+	s64 skip;
+
+	err = reader__init(rd, &session->one_mmap);
+	if (err)
+		goto out;
+
 	mmap_prot  = PROT_READ;
 	mmap_flags = MAP_SHARED;
 
@@ -2286,7 +2305,7 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 	if (session_done())
 		goto out;
 
-	if (rd->file_pos < data_size)
+	if (rd->file_pos < rd->data_size + rd->data_offset)
 		goto more;
 
 out:
@@ -2343,8 +2362,7 @@ static int __perf_session__process_events(struct perf_session *session)
 	 */
 	ordered_events__reinit(&session->ordered_events);
 	auxtrace__free_events(session);
-	perf_decomp__release_events(rd.decomp_data.decomp);
-	zstd_fini(&rd.zstd_data);
+	reader__release_decomp(&rd);
 	session->one_mmap = false;
 	return err;
 }
-- 
2.19.0

[PATCH v3 4/8] perf session: Move map code to separate function

[Alexey Bayduraev]

Moving mapping code into separate reader__mmap function.

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 | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 00417b0d29ec..36faa2d598b2 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2220,18 +2220,10 @@ reader__release_decomp(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)
 {
-	u64 page_offset, size;
-	int err = 0, mmap_prot, mmap_flags;
+	int mmap_prot, mmap_flags;
 	char *buf, **mmaps = rd->mmaps;
-	union perf_event *event;
-	s64 skip;
-
-	err = reader__init(rd, &session->one_mmap);
-	if (err)
-		goto out;
 
 	mmap_prot  = PROT_READ;
 	mmap_flags = MAP_SHARED;
@@ -2242,13 +2234,12 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 		mmap_prot  |= PROT_WRITE;
 		mmap_flags = MAP_PRIVATE;
 	}
-remap:
+
 	buf = mmap(NULL, rd->mmap_size, mmap_prot, mmap_flags, rd->fd,
 		   rd->file_offset);
 	if (buf == MAP_FAILED) {
 		pr_err("failed to mmap file\n");
-		err = -errno;
-		goto out;
+		return -errno;
 	}
 	mmaps[rd->mmap_idx] = rd->mmap_cur = buf;
 	rd->mmap_idx = (rd->mmap_idx + 1) & (ARRAY_SIZE(rd->mmaps) - 1);
@@ -2258,6 +2249,28 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 		session->one_mmap_offset = rd->file_offset;
 	}
 
+	return 0;
+}
+
+static int
+reader__process_events(struct reader *rd, struct perf_session *session,
+		       struct ui_progress *prog)
+{
+	u64 page_offset, size;
+	int err = 0;
+	char **mmaps = rd->mmaps;
+	union perf_event *event;
+	s64 skip;
+
+	err = reader__init(rd, &session->one_mmap);
+	if (err)
+		goto out;
+
+remap:
+	err = reader__mmap(rd, session);
+	if (err)
+		goto out;
+
 more:
 	event = fetch_mmaped_event(rd->head, rd->mmap_size, rd->mmap_cur,
 				   session->header.needs_swap);
-- 
2.19.0

[PATCH v3 5/8] perf session: Move unmap code to reader__mmap

[Alexey Bayduraev]

Moving 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 | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 36faa2d598b2..6289fcafdc86 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2187,13 +2187,9 @@ static int
 reader__init(struct reader *rd, bool *one_mmap)
 {
 	u64 data_size = rd->data_size;
-	u64 page_offset;
 	char **mmaps = rd->mmaps;
 
-	page_offset = page_size * (rd->data_offset / page_size);
-	rd->file_offset = page_offset;
-	rd->head = rd->data_offset - page_offset;
-
+	rd->head = rd->data_offset;
 	data_size += rd->data_offset;
 
 	rd->mmap_size = MMAP_SIZE;
@@ -2224,6 +2220,7 @@ reader__mmap(struct reader *rd, struct perf_session *session)
 {
 	int mmap_prot, mmap_flags;
 	char *buf, **mmaps = rd->mmaps;
+	u64 page_offset;
 
 	mmap_prot  = PROT_READ;
 	mmap_flags = MAP_SHARED;
@@ -2235,6 +2232,15 @@ reader__mmap(struct reader *rd, struct perf_session *session)
 		mmap_flags = MAP_PRIVATE;
 	}
 
+	if (mmaps[rd->mmap_idx]) {
+		munmap(mmaps[rd->mmap_idx], rd->mmap_size);
+		mmaps[rd->mmap_idx] = NULL;
+	}
+
+	page_offset = page_size * (rd->head / page_size);
+	rd->file_offset += page_offset;
+	rd->head -= page_offset;
+
 	buf = mmap(NULL, rd->mmap_size, mmap_prot, mmap_flags, rd->fd,
 		   rd->file_offset);
 	if (buf == MAP_FAILED) {
@@ -2256,9 +2262,8 @@ static int
 reader__process_events(struct reader *rd, struct perf_session *session,
 		       struct ui_progress *prog)
 {
-	u64 page_offset, size;
+	u64 size;
 	int err = 0;
-	char **mmaps = rd->mmaps;
 	union perf_event *event;
 	s64 skip;
 
@@ -2277,17 +2282,8 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 	if (IS_ERR(event))
 		return PTR_ERR(event);
 
-	if (!event) {
-		if (mmaps[rd->mmap_idx]) {
-			munmap(mmaps[rd->mmap_idx], rd->mmap_size);
-			mmaps[rd->mmap_idx] = NULL;
-		}
-
-		page_offset = page_size * (rd->head / page_size);
-		rd->file_offset += page_offset;
-		rd->head -= page_offset;
+	if (!event)
 		goto remap;
-	}
 
 	session->active_decomp = &rd->decomp_data;
 	size = event->header.size;
-- 
2.19.0

[PATCH v3 6/8] perf session: Move event read code to separate function

[Alexey Bayduraev]

Separating reading code of single event into reader__read_event
function.

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 | 44 ++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 6289fcafdc86..6b255b0b23e0 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2259,31 +2259,21 @@ reader__mmap(struct reader *rd, struct perf_session *session)
 }
 
 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)
 {
 	u64 size;
 	int err = 0;
 	union perf_event *event;
 	s64 skip;
 
-	err = reader__init(rd, &session->one_mmap);
-	if (err)
-		goto out;
-
-remap:
-	err = reader__mmap(rd, session);
-	if (err)
-		goto out;
-
-more:
 	event = fetch_mmaped_event(rd->head, rd->mmap_size, rd->mmap_cur,
 				   session->header.needs_swap);
 	if (IS_ERR(event))
 		return PTR_ERR(event);
 
 	if (!event)
-		goto remap;
+		return 1;
 
 	session->active_decomp = &rd->decomp_data;
 	size = event->header.size;
@@ -2311,6 +2301,33 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 
 	ui_progress__update(prog, size);
 
+out:
+	session->active_decomp = &session->decomp_data;
+	return err;
+}
+
+static int
+reader__process_events(struct reader *rd, struct perf_session *session,
+		       struct ui_progress *prog)
+{
+	int err;
+
+	err = reader__init(rd, &session->one_mmap);
+	if (err)
+		goto out;
+
+remap:
+	err = reader__mmap(rd, session);
+	if (err)
+		goto out;
+
+more:
+	err = reader__read_event(rd, session, prog);
+	if (err < 0)
+		goto out;
+	else if (err == 1)
+		goto remap;
+
 	if (session_done())
 		goto out;
 
@@ -2318,7 +2335,6 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 		goto more;
 
 out:
-	session->active_decomp = &session->decomp_data;
 	return err;
 }
 
-- 
2.19.0

Re: [PATCH v3 6/8] perf session: Move event read code to separate function

[Jiri Olsa]

On Thu, Oct 07, 2021 at 01:25:41PM +0300, Alexey Bayduraev wrote:

SNIP

>  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)
>  {
>  	u64 size;
>  	int err = 0;
>  	union perf_event *event;
>  	s64 skip;
>  
> -	err = reader__init(rd, &session->one_mmap);
> -	if (err)
> -		goto out;
> -
> -remap:
> -	err = reader__mmap(rd, session);
> -	if (err)
> -		goto out;
> -
> -more:
>  	event = fetch_mmaped_event(rd->head, rd->mmap_size, rd->mmap_cur,
>  				   session->header.needs_swap);
>  	if (IS_ERR(event))
>  		return PTR_ERR(event);
>  
>  	if (!event)
> -		goto remap;
> +		return 1;
>  
>  	session->active_decomp = &rd->decomp_data;
>  	size = event->header.size;
> @@ -2311,6 +2301,33 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>  
>  	ui_progress__update(prog, size);
>  
> +out:
> +	session->active_decomp = &session->decomp_data;
> +	return err;
> +}
> +
> +static int
> +reader__process_events(struct reader *rd, struct perf_session *session,
> +		       struct ui_progress *prog)
> +{
> +	int err;
> +
> +	err = reader__init(rd, &session->one_mmap);
> +	if (err)
> +		goto out;
> +
> +remap:
> +	err = reader__mmap(rd, session);
> +	if (err)
> +		goto out;
> +
> +more:
> +	err = reader__read_event(rd, session, prog);
> +	if (err < 0)
> +		goto out;
> +	else if (err == 1)
> +		goto remap;
> +
>  	if (session_done())
>  		goto out;
>  
> @@ -2318,7 +2335,6 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>  		goto more;
>  
>  out:
> -	session->active_decomp = &session->decomp_data;

active_decomp should be set/unset within reader__process_events,
not just for single event read, right?

jirka

>  	return err;
>  }
>  
> -- 
> 2.19.0
> 

Re: [PATCH v3 6/8] perf session: Move event read code to separate function

[Bayduraev, Alexey V]

On 08.10.2021 10:33, Jiri Olsa wrote:
> On Thu, Oct 07, 2021 at 01:25:41PM +0300, Alexey Bayduraev wrote:
> 
> SNIP
> 
>>  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)
>>  {
>>  	u64 size;
>>  	int err = 0;
>>  	union perf_event *event;
>>  	s64 skip;
>>  
>> -	err = reader__init(rd, &session->one_mmap);
>> -	if (err)
>> -		goto out;
>> -
>> -remap:
>> -	err = reader__mmap(rd, session);
>> -	if (err)
>> -		goto out;
>> -
>> -more:
>>  	event = fetch_mmaped_event(rd->head, rd->mmap_size, rd->mmap_cur,
>>  				   session->header.needs_swap);
>>  	if (IS_ERR(event))
>>  		return PTR_ERR(event);
>>  
>>  	if (!event)
>> -		goto remap;
>> +		return 1;
>>  
>>  	session->active_decomp = &rd->decomp_data;
>>  	size = event->header.size;
>> @@ -2311,6 +2301,33 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>>  
>>  	ui_progress__update(prog, size);
>>  
>> +out:
>> +	session->active_decomp = &session->decomp_data;
>> +	return err;
>> +}
>> +
>> +static int
>> +reader__process_events(struct reader *rd, struct perf_session *session,
>> +		       struct ui_progress *prog)
>> +{
>> +	int err;
>> +
>> +	err = reader__init(rd, &session->one_mmap);
>> +	if (err)
>> +		goto out;
>> +
>> +remap:
>> +	err = reader__mmap(rd, session);
>> +	if (err)
>> +		goto out;
>> +
>> +more:
>> +	err = reader__read_event(rd, session, prog);
>> +	if (err < 0)
>> +		goto out;
>> +	else if (err == 1)
>> +		goto remap;
>> +
>>  	if (session_done())
>>  		goto out;
>>  
>> @@ -2318,7 +2335,6 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>>  		goto more;
>>  
>>  out:
>> -	session->active_decomp = &session->decomp_data;
> 
> active_decomp should be set/unset within reader__process_events,
> not just for single event read, right?

No, it should be set before perf_session__process_event/process_decomp_events
and unset after these calls. So active_decomp setting/unsetting is moved in
this patch to the reader__read_event function. This is necessary for multiple
trace reader because it could call reader__read_event in round-robin manner.

Regards,
Alexey

> 
> jirka
> 
>>  	return err;
>>  }
>>  
>> -- 
>> 2.19.0
>>
> 

Re: [PATCH v3 6/8] perf session: Move event read code to separate function

[Jiri Olsa]

On Fri, Oct 08, 2021 at 11:42:18AM +0300, Bayduraev, Alexey V wrote:
> 
> 
> On 08.10.2021 10:33, Jiri Olsa wrote:
> > On Thu, Oct 07, 2021 at 01:25:41PM +0300, Alexey Bayduraev wrote:
> > 
> > SNIP
> > 
> >>  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)
> >>  {
> >>  	u64 size;
> >>  	int err = 0;
> >>  	union perf_event *event;
> >>  	s64 skip;
> >>  
> >> -	err = reader__init(rd, &session->one_mmap);
> >> -	if (err)
> >> -		goto out;
> >> -
> >> -remap:
> >> -	err = reader__mmap(rd, session);
> >> -	if (err)
> >> -		goto out;
> >> -
> >> -more:
> >>  	event = fetch_mmaped_event(rd->head, rd->mmap_size, rd->mmap_cur,
> >>  				   session->header.needs_swap);
> >>  	if (IS_ERR(event))
> >>  		return PTR_ERR(event);
> >>  
> >>  	if (!event)
> >> -		goto remap;
> >> +		return 1;
> >>  
> >>  	session->active_decomp = &rd->decomp_data;
> >>  	size = event->header.size;
> >> @@ -2311,6 +2301,33 @@ reader__process_events(struct reader *rd, struct perf_session *session,
> >>  
> >>  	ui_progress__update(prog, size);
> >>  
> >> +out:
> >> +	session->active_decomp = &session->decomp_data;
> >> +	return err;
> >> +}
> >> +
> >> +static int
> >> +reader__process_events(struct reader *rd, struct perf_session *session,
> >> +		       struct ui_progress *prog)
> >> +{
> >> +	int err;
> >> +
> >> +	err = reader__init(rd, &session->one_mmap);
> >> +	if (err)
> >> +		goto out;
> >> +
> >> +remap:
> >> +	err = reader__mmap(rd, session);
> >> +	if (err)
> >> +		goto out;
> >> +
> >> +more:
> >> +	err = reader__read_event(rd, session, prog);
> >> +	if (err < 0)
> >> +		goto out;
> >> +	else if (err == 1)
> >> +		goto remap;
> >> +
> >>  	if (session_done())
> >>  		goto out;
> >>  
> >> @@ -2318,7 +2335,6 @@ reader__process_events(struct reader *rd, struct perf_session *session,
> >>  		goto more;
> >>  
> >>  out:
> >> -	session->active_decomp = &session->decomp_data;
> > 
> > active_decomp should be set/unset within reader__process_events,
> > not just for single event read, right?
> 
> No, it should be set before perf_session__process_event/process_decomp_events
> and unset after these calls. So active_decomp setting/unsetting is moved in
> this patch to the reader__read_event function. This is necessary for multiple
> trace reader because it could call reader__read_event in round-robin manner.

hum, is that code already in? I can't see this happening in current code

jirka

> 
> Regards,
> Alexey
> 
> > 
> > jirka
> > 
> >>  	return err;
> >>  }
> >>  
> >> -- 
> >> 2.19.0
> >>
> > 
> 

Re: [PATCH v3 6/8] perf session: Move event read code to separate function

[Bayduraev, Alexey V]

On 08.10.2021 17:38, Jiri Olsa wrote:
> On Fri, Oct 08, 2021 at 11:42:18AM +0300, Bayduraev, Alexey V wrote:
>>
>>
>> On 08.10.2021 10:33, Jiri Olsa wrote:
>>> On Thu, Oct 07, 2021 at 01:25:41PM +0300, Alexey Bayduraev wrote:
>>>
>>> SNIP
>>>
>>>>  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)

SNIP

>>>
>>> active_decomp should be set/unset within reader__process_events,
>>> not just for single event read, right?
>>
>> No, it should be set before perf_session__process_event/process_decomp_events
>> and unset after these calls. So active_decomp setting/unsetting is moved in
>> this patch to the reader__read_event function. This is necessary for multiple
>> trace reader because it could call reader__read_event in round-robin manner.
> 
> hum, is that code already in? I can't see this happening in current code

Probably I don't understand the question. In [PATCH v3 2/8] I introduced 
active_decomp pointer in perf_session. It is initialized by a pointer to the 
decompressor object in perf_session. In reader__process_events it is set to 
the reader decompressor object. And it is reset to the session decompressor 
object at exit. In this case we do not need to reset it after each 
perf_session__process_event because this code reads events in loop with 
constant reader object. Maybe setting of active_decomp should be at the 
entrance to the reader__process_events, not before reader__process_events, 
in [PATCH v3 2/8]. All this code is new.

In this patch I separates single event reading and moves setting/resetting
of active_decomp before/after perf_session__process_event because this is 
necessary for multiple trace reader. 

Regards,
Alexey

> 
> jirka
> 
>>
>> Regards,
>> Alexey
>>
>>>
>>> jirka
>>>
>>>>  	return err;
>>>>  }
>>>>  
>>>> -- 
>>>> 2.19.0
>>>>
>>>
>>
> 

Re: [PATCH v3 6/8] perf session: Move event read code to separate function

[Bayduraev, Alexey V]

On 11.10.2021 12:08, Bayduraev, Alexey V wrote:
> 
> On 08.10.2021 17:38, Jiri Olsa wrote:
>> On Fri, Oct 08, 2021 at 11:42:18AM +0300, Bayduraev, Alexey V wrote:
>>>
>>>
>>> On 08.10.2021 10:33, Jiri Olsa wrote:
>>>> On Thu, Oct 07, 2021 at 01:25:41PM +0300, Alexey Bayduraev wrote:
>>>>
>>>> SNIP
>>>>
>>>>>  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)
> 
> SNIP
> 
>>>>
>>>> active_decomp should be set/unset within reader__process_events,
>>>> not just for single event read, right?
>>>
>>> No, it should be set before perf_session__process_event/process_decomp_events
>>> and unset after these calls. So active_decomp setting/unsetting is moved in
>>> this patch to the reader__read_event function. This is necessary for multiple
>>> trace reader because it could call reader__read_event in round-robin manner.
>>
>> hum, is that code already in? I can't see this happening in current code
> 
> Probably I don't understand the question. In [PATCH v3 2/8] I introduced 
> active_decomp pointer in perf_session. It is initialized by a pointer to the 
> decompressor object in perf_session. In reader__process_events it is set to 
> the reader decompressor object. And it is reset to the session decompressor 
> object at exit. In this case we do not need to reset it after each 
> perf_session__process_event because this code reads events in loop with 
> constant reader object. Maybe setting of active_decomp should be at the 
> entrance to the reader__process_events, not before reader__process_events, 
> in [PATCH v3 2/8]. All this code is new.

We set active_decomp for perf_session__process_event (rd->process() in our
case) and for __perf_session__process_decomp_events, active_decomp is not 
necessary for other parts of reader__process_events.

Regards,
Alexey

> 
> In this patch I separates single event reading and moves setting/resetting
> of active_decomp before/after perf_session__process_event because this is 
> necessary for multiple trace reader. 
> 
> Regards,
> Alexey
> 
>>
>> jirka
>>
>>>
>>> Regards,
>>> Alexey
>>>
>>>>
>>>> jirka
>>>>
>>>>>  	return err;
>>>>>  }
>>>>>  
>>>>> -- 
>>>>> 2.19.0
>>>>>
>>>>
>>>
>>

Re: [PATCH v3 6/8] perf session: Move event read code to separate function

[Jiri Olsa]

On Mon, Oct 11, 2021 at 12:53:30PM +0300, Bayduraev, Alexey V wrote:
> 
> 
> On 11.10.2021 12:08, Bayduraev, Alexey V wrote:
> > 
> > On 08.10.2021 17:38, Jiri Olsa wrote:
> >> On Fri, Oct 08, 2021 at 11:42:18AM +0300, Bayduraev, Alexey V wrote:
> >>>
> >>>
> >>> On 08.10.2021 10:33, Jiri Olsa wrote:
> >>>> On Thu, Oct 07, 2021 at 01:25:41PM +0300, Alexey Bayduraev wrote:
> >>>>
> >>>> SNIP
> >>>>
> >>>>>  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)
> > 
> > SNIP
> > 
> >>>>
> >>>> active_decomp should be set/unset within reader__process_events,
> >>>> not just for single event read, right?
> >>>
> >>> No, it should be set before perf_session__process_event/process_decomp_events
> >>> and unset after these calls. So active_decomp setting/unsetting is moved in
> >>> this patch to the reader__read_event function. This is necessary for multiple
> >>> trace reader because it could call reader__read_event in round-robin manner.
> >>
> >> hum, is that code already in? I can't see this happening in current code
> > 
> > Probably I don't understand the question. In [PATCH v3 2/8] I introduced 
> > active_decomp pointer in perf_session. It is initialized by a pointer to the 
> > decompressor object in perf_session. In reader__process_events it is set to 
> > the reader decompressor object. And it is reset to the session decompressor 
> > object at exit. In this case we do not need to reset it after each 
> > perf_session__process_event because this code reads events in loop with 
> > constant reader object. Maybe setting of active_decomp should be at the 
> > entrance to the reader__process_events, not before reader__process_events, 
> > in [PATCH v3 2/8]. All this code is new.
> 
> We set active_decomp for perf_session__process_event (rd->process() in our
> case) and for __perf_session__process_decomp_events, active_decomp is not 
> necessary for other parts of reader__process_events.

so what I see in the code is:

__perf_session__process_events
{
	struct reader rd;

	reader__process_events(rd)
	{
		reader__read_event(rd)
		{
->			session->active_decomp = &rd->decomp_data;
			rd->process(...
->			session->active_decomp = &session->decomp_data;
		}

	}
}


we set session->active_decomp for each event that we process
and I don't understand why we can't do that just once in
__perf_session__process_events, so it'd be like:

__perf_session__process_events
{
	struct reader rd;

->	session->active_decomp = &rd->decomp_data;

	reader__process_events(rd)
	{
		reader__read_event(rd)
		{
			rd->process(...
		}

	}

->	session->active_decomp = &session->decomp_data;
}


or within reader__process_events if it's more convenient

jirka

> 
> Regards,
> Alexey
> 
> > 
> > In this patch I separates single event reading and moves setting/resetting
> > of active_decomp before/after perf_session__process_event because this is 
> > necessary for multiple trace reader. 
> > 
> > Regards,
> > Alexey
> > 
> >>
> >> jirka
> >>
> >>>
> >>> Regards,
> >>> Alexey
> >>>
> >>>>
> >>>> jirka
> >>>>
> >>>>>  	return err;
> >>>>>  }
> >>>>>  
> >>>>> -- 
> >>>>> 2.19.0
> >>>>>
> >>>>
> >>>
> >>
> 

Re: [PATCH v3 6/8] perf session: Move event read code to separate function

[Bayduraev, Alexey V]

On 11.10.2021 16:21, Jiri Olsa wrote:
> On Mon, Oct 11, 2021 at 12:53:30PM +0300, Bayduraev, Alexey V wrote:
>>
>>
>> On 11.10.2021 12:08, Bayduraev, Alexey V wrote:
>>>
>>> On 08.10.2021 17:38, Jiri Olsa wrote:
>>>> On Fri, Oct 08, 2021 at 11:42:18AM +0300, Bayduraev, Alexey V wrote:
>>>>>
>>>>>
>>>>> On 08.10.2021 10:33, Jiri Olsa wrote:
>>>>>> On Thu, Oct 07, 2021 at 01:25:41PM +0300, Alexey Bayduraev wrote:
>>>>>>
>>>>>> SNIP
>>>>>>
>>>>>>>  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)
>>>
>>> SNIP
>>>
>>>>>>
>>>>>> active_decomp should be set/unset within reader__process_events,
>>>>>> not just for single event read, right?
>>>>>
>>>>> No, it should be set before perf_session__process_event/process_decomp_events
>>>>> and unset after these calls. So active_decomp setting/unsetting is moved in
>>>>> this patch to the reader__read_event function. This is necessary for multiple
>>>>> trace reader because it could call reader__read_event in round-robin manner.
>>>>
>>>> hum, is that code already in? I can't see this happening in current code
>>>
>>> Probably I don't understand the question. In [PATCH v3 2/8] I introduced 
>>> active_decomp pointer in perf_session. It is initialized by a pointer to the 
>>> decompressor object in perf_session. In reader__process_events it is set to 
>>> the reader decompressor object. And it is reset to the session decompressor 
>>> object at exit. In this case we do not need to reset it after each 
>>> perf_session__process_event because this code reads events in loop with 
>>> constant reader object. Maybe setting of active_decomp should be at the 
>>> entrance to the reader__process_events, not before reader__process_events, 
>>> in [PATCH v3 2/8]. All this code is new.
>>
>> We set active_decomp for perf_session__process_event (rd->process() in our
>> case) and for __perf_session__process_decomp_events, active_decomp is not 
>> necessary for other parts of reader__process_events.
> 
> so what I see in the code is:
> 
> __perf_session__process_events
> {
> 	struct reader rd;
> 
> 	reader__process_events(rd)
> 	{
> 		reader__read_event(rd)
> 		{
> ->			session->active_decomp = &rd->decomp_data;
> 			rd->process(...
> ->			session->active_decomp = &session->decomp_data;
> 		}
> 
> 	}
> }
> 
> 
> we set session->active_decomp for each event that we process
> and I don't understand why we can't do that just once in
> __perf_session__process_events, so it'd be like:
> 
> __perf_session__process_events
> {
> 	struct reader rd;
> 
> ->	session->active_decomp = &rd->decomp_data;
> 
> 	reader__process_events(rd)
> 	{
> 		reader__read_event(rd)
> 		{
> 			rd->process(...
> 		}
> 
> 	}
> 
> ->	session->active_decomp = &session->decomp_data;
> }
> 
> 
> or within reader__process_events if it's more convenient

Now I got it, thanks ;)

With your suggestion, for multiple trace reader, we should always 
remember to switch active_decomp when switching the reader object, 
just passing the current reader pointer to the reader__read_event 
function will not be enough. I thought it would be better to hide 
such details in the reader__read_event function.

Of course, I can move setting of active_decomp outside of 
reader__read_event if this is better from your point of view.

Regards,
Alexey

> 
> jirka
> 
>>
>> Regards,
>> Alexey
>>
>>>
>>> In this patch I separates single event reading and moves setting/resetting
>>> of active_decomp before/after perf_session__process_event because this is 
>>> necessary for multiple trace reader. 
>>>
>>> Regards,
>>> Alexey
>>>
>>>>
>>>> jirka
>>>>
>>>>>
>>>>> Regards,
>>>>> Alexey
>>>>>
>>>>>>
>>>>>> jirka
>>>>>>
>>>>>>>  	return err;
>>>>>>>  }
>>>>>>>  
>>>>>>> -- 
>>>>>>> 2.19.0
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
> 

Re: [PATCH v3 6/8] perf session: Move event read code to separate function

[Jiri Olsa]

On Mon, Oct 11, 2021 at 07:40:33PM +0300, Bayduraev, Alexey V wrote:
> 
> 
> On 11.10.2021 16:21, Jiri Olsa wrote:
> > On Mon, Oct 11, 2021 at 12:53:30PM +0300, Bayduraev, Alexey V wrote:
> >>
> >>
> >> On 11.10.2021 12:08, Bayduraev, Alexey V wrote:
> >>>
> >>> On 08.10.2021 17:38, Jiri Olsa wrote:
> >>>> On Fri, Oct 08, 2021 at 11:42:18AM +0300, Bayduraev, Alexey V wrote:
> >>>>>
> >>>>>
> >>>>> On 08.10.2021 10:33, Jiri Olsa wrote:
> >>>>>> On Thu, Oct 07, 2021 at 01:25:41PM +0300, Alexey Bayduraev wrote:
> >>>>>>
> >>>>>> SNIP
> >>>>>>
> >>>>>>>  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)
> >>>
> >>> SNIP
> >>>
> >>>>>>
> >>>>>> active_decomp should be set/unset within reader__process_events,
> >>>>>> not just for single event read, right?
> >>>>>
> >>>>> No, it should be set before perf_session__process_event/process_decomp_events
> >>>>> and unset after these calls. So active_decomp setting/unsetting is moved in
> >>>>> this patch to the reader__read_event function. This is necessary for multiple
> >>>>> trace reader because it could call reader__read_event in round-robin manner.
> >>>>
> >>>> hum, is that code already in? I can't see this happening in current code
> >>>
> >>> Probably I don't understand the question. In [PATCH v3 2/8] I introduced 
> >>> active_decomp pointer in perf_session. It is initialized by a pointer to the 
> >>> decompressor object in perf_session. In reader__process_events it is set to 
> >>> the reader decompressor object. And it is reset to the session decompressor 
> >>> object at exit. In this case we do not need to reset it after each 
> >>> perf_session__process_event because this code reads events in loop with 
> >>> constant reader object. Maybe setting of active_decomp should be at the 
> >>> entrance to the reader__process_events, not before reader__process_events, 
> >>> in [PATCH v3 2/8]. All this code is new.
> >>
> >> We set active_decomp for perf_session__process_event (rd->process() in our
> >> case) and for __perf_session__process_decomp_events, active_decomp is not 
> >> necessary for other parts of reader__process_events.
> > 
> > so what I see in the code is:
> > 
> > __perf_session__process_events
> > {
> > 	struct reader rd;
> > 
> > 	reader__process_events(rd)
> > 	{
> > 		reader__read_event(rd)
> > 		{
> > ->			session->active_decomp = &rd->decomp_data;
> > 			rd->process(...
> > ->			session->active_decomp = &session->decomp_data;
> > 		}
> > 
> > 	}
> > }
> > 
> > 
> > we set session->active_decomp for each event that we process
> > and I don't understand why we can't do that just once in
> > __perf_session__process_events, so it'd be like:
> > 
> > __perf_session__process_events
> > {
> > 	struct reader rd;
> > 
> > ->	session->active_decomp = &rd->decomp_data;
> > 
> > 	reader__process_events(rd)
> > 	{
> > 		reader__read_event(rd)
> > 		{
> > 			rd->process(...
> > 		}
> > 
> > 	}
> > 
> > ->	session->active_decomp = &session->decomp_data;
> > }
> > 
> > 
> > or within reader__process_events if it's more convenient
> 
> Now I got it, thanks ;)
> 
> With your suggestion, for multiple trace reader, we should always 
> remember to switch active_decomp when switching the reader object, 
> just passing the current reader pointer to the reader__read_event 
> function will not be enough. I thought it would be better to hide 
> such details in the reader__read_event function.
> 
> Of course, I can move setting of active_decomp outside of 
> reader__read_event if this is better from your point of view.

at the moment it's not necessary to set/unset it for each event,
let's do that when it's really needed in the following changes
for threaded perf record with explanation

thanks,
jirka

> 
> Regards,
> Alexey
> 
> > 
> > jirka
> > 
> >>
> >> Regards,
> >> Alexey
> >>
> >>>
> >>> In this patch I separates single event reading and moves setting/resetting
> >>> of active_decomp before/after perf_session__process_event because this is 
> >>> necessary for multiple trace reader. 
> >>>
> >>> Regards,
> >>> Alexey
> >>>
> >>>>
> >>>> jirka
> >>>>
> >>>>>
> >>>>> Regards,
> >>>>> Alexey
> >>>>>
> >>>>>>
> >>>>>> jirka
> >>>>>>
> >>>>>>>  	return err;
> >>>>>>>  }
> >>>>>>>  
> >>>>>>> -- 
> >>>>>>> 2.19.0
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
> > 
> 

[PATCH v3 7/8] perf session: Introduce reader return codes

[Alexey Bayduraev]

Adding reader READER_OK and READER_NODATA return codes to make
the code more clear.

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 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 6b255b0b23e0..7d88c651ffd7 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2258,12 +2258,17 @@ reader__mmap(struct reader *rd, struct perf_session *session)
 	return 0;
 }
 
+enum {
+	READER_OK,
+	READER_NODATA,
+};
+
 static int
 reader__read_event(struct reader *rd, struct perf_session *session,
 		   struct ui_progress *prog)
 {
 	u64 size;
-	int err = 0;
+	int err = READER_OK;
 	union perf_event *event;
 	s64 skip;
 
@@ -2273,7 +2278,7 @@ reader__read_event(struct reader *rd, struct perf_session *session,
 		return PTR_ERR(event);
 
 	if (!event)
-		return 1;
+		return READER_NODATA;
 
 	session->active_decomp = &rd->decomp_data;
 	size = event->header.size;
@@ -2325,7 +2330,7 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 	err = reader__read_event(rd, session, prog);
 	if (err < 0)
 		goto out;
-	else if (err == 1)
+	else if (err == READER_NODATA)
 		goto remap;
 
 	if (session_done())
-- 
2.19.0

[PATCH v3 8/8] perf session: Introduce reader EOF function

[Alexey Bayduraev]

Introducing a function to check end-of-file status.

Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
---
 tools/perf/util/session.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 7d88c651ffd7..f74e153231fa 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2311,6 +2311,12 @@ reader__read_event(struct reader *rd, struct perf_session *session,
 	return err;
 }
 
+static inline bool
+reader__eof(struct reader *rd)
+{
+	return (rd->file_pos >= rd->data_size + rd->data_offset);
+}
+
 static int
 reader__process_events(struct reader *rd, struct perf_session *session,
 		       struct ui_progress *prog)
@@ -2336,7 +2342,7 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 	if (session_done())
 		goto out;
 
-	if (rd->file_pos < rd->data_size + rd->data_offset)
+	if (!reader__eof(rd))
 		goto more;
 
 out:
-- 
2.19.0