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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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