Re: [PATCH v3 05/11] perf evlist: Introduce aux perf evlist

[Arnaldo Carvalho de Melo]

Em Mon, May 23, 2016 at 07:13:42AM +0000, Wang Nan escreveu:
> Introduce auxiliary perf evlist. Such evlists created by perf_evlist__new_aux()
> using an existing evlist. A 'parent' pointer points to the template.

You must define template above, otherwise I will not have how to compare
the patch _intention_ to what the patch _does_.

That could help understanding the next paragraph, where it seems two
evlists are at play, but I don't know which is which:
 
> A 'evlist->parent' pointer is added to 'struct perf_evlist' and points to the
> evlist itself for normal evlists (so in this patch changing 'evlist' to
> 'evlist->parent' won't causes any error).

After reading the patch I understand that you're paving the way to allow
that ,when reading the mmaps in an evlist, instead of reading from its
mmaps, read from a "parent", i.e. at some point evlist->evlist !=
evlist, but why, just by reading this commit I can't figure it out :-\
 
> Following commits uses auxiliary evlist as container of 'struct perf_mmap',
> creates auxiliary evlist for overwritable events, allow them to create
> separated mmaps. To achieve this goal, this patch carefully changes 'evlist'

To begin with, _why_ do we need two evlists, you need to make this
clear.

> to 'evlist->parent' in all functions in the patch of 'perf_evlist__mmap_ex',
> except 'evlist->mmap' related operations, to make sure all evlist modifications
> like pollfd and event id hash tables goes to original evlist.

I applied the patches before this one.

- Arnaldo
 
> Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
> Cc: He Kuang <hekuang@xxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> Cc: Zefan Li <lizefan@xxxxxxxxxx>
> Cc: pi3orama@xxxxxxx
> ---
>  tools/perf/util/evlist.c | 31 +++++++++++++++++++++----------
>  tools/perf/util/evlist.h |  3 +++
>  2 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 1305910..af0bea7 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -45,6 +45,7 @@ void perf_evlist__init(struct perf_evlist *evlist, struct cpu_map *cpus,
>  	fdarray__init(&evlist->pollfd, 64);
>  	evlist->workload.pid = -1;
>  	evlist->backward = false;
> +	evlist->parent = evlist;
>  }
>  
>  struct perf_evlist *perf_evlist__new(void)
> @@ -994,7 +995,7 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
>  {
>  	struct perf_evsel *evsel;
>  
> -	evlist__for_each(evlist, evsel) {
> +	evlist__for_each(evlist->parent, evsel) {
>  		int fd;
>  
>  		if (evsel->system_wide && thread)
> @@ -1021,16 +1022,16 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
>  		 * Therefore don't add it for polling.
>  		 */
>  		if (!evsel->system_wide &&
> -		    __perf_evlist__add_pollfd(evlist, fd, idx) < 0) {
> +		    __perf_evlist__add_pollfd(evlist->parent, fd, idx) < 0) {
>  			perf_evlist__mmap_put(evlist, idx);
>  			return -1;
>  		}
>  
>  		if (evsel->attr.read_format & PERF_FORMAT_ID) {
> -			if (perf_evlist__id_add_fd(evlist, evsel, cpu, thread,
> +			if (perf_evlist__id_add_fd(evlist->parent, evsel, cpu, thread,
>  						   fd) < 0)
>  				return -1;
> -			perf_evlist__set_sid_idx(evlist, evsel, idx, cpu,
> +			perf_evlist__set_sid_idx(evlist->parent, evsel, idx, cpu,
>  						 thread);
>  		}
>  	}
> @@ -1071,13 +1072,13 @@ static int perf_evlist__mmap_per_thread(struct perf_evlist *evlist,
>  					struct mmap_params *mp)
>  {
>  	int thread;
> -	int nr_threads = thread_map__nr(evlist->threads);
> +	int nr_threads = thread_map__nr(evlist->parent->threads);
>  
>  	pr_debug2("perf event ring buffer mmapped per thread\n");
>  	for (thread = 0; thread < nr_threads; thread++) {
>  		int output = -1;
>  
> -		auxtrace_mmap_params__set_idx(&mp->auxtrace_mp, evlist, thread,
> +		auxtrace_mmap_params__set_idx(&mp->auxtrace_mp, evlist->parent, thread,
>  					      false);
>  
>  		if (perf_evlist__mmap_per_evsel(evlist, thread, mp, 0, thread,
> @@ -1216,8 +1217,8 @@ int perf_evlist__mmap_ex(struct perf_evlist *evlist, unsigned int pages,
>  			 bool auxtrace_overwrite)
>  {
>  	struct perf_evsel *evsel;
> -	const struct cpu_map *cpus = evlist->cpus;
> -	const struct thread_map *threads = evlist->threads;
> +	const struct cpu_map *cpus = evlist->parent->cpus;
> +	const struct thread_map *threads = evlist->parent->threads;
>  	struct mmap_params mp = {
>  		.prot = PROT_READ | (overwrite ? 0 : PROT_WRITE),
>  	};
> @@ -1225,7 +1226,7 @@ int perf_evlist__mmap_ex(struct perf_evlist *evlist, unsigned int pages,
>  	if (evlist->mmap == NULL && perf_evlist__alloc_mmap(evlist) < 0)
>  		return -ENOMEM;
>  
> -	if (evlist->pollfd.entries == NULL && perf_evlist__alloc_pollfd(evlist) < 0)
> +	if (evlist->parent->pollfd.entries == NULL && perf_evlist__alloc_pollfd(evlist->parent) < 0)
>  		return -ENOMEM;
>  
>  	evlist->overwrite = overwrite;
> @@ -1236,7 +1237,7 @@ int perf_evlist__mmap_ex(struct perf_evlist *evlist, unsigned int pages,
>  	auxtrace_mmap_params__init(&mp.auxtrace_mp, evlist->mmap_len,
>  				   auxtrace_pages, auxtrace_overwrite);
>  
> -	evlist__for_each(evlist, evsel) {
> +	evlist__for_each(evlist->parent, evsel) {
>  		if ((evsel->attr.read_format & PERF_FORMAT_ID) &&
>  		    evsel->sample_id == NULL &&
>  		    perf_evsel__alloc_id(evsel, cpu_map__nr(cpus), threads->nr) < 0)
> @@ -1893,3 +1894,13 @@ perf_evlist__find_evsel_by_str(struct perf_evlist *evlist,
>  
>  	return NULL;
>  }
> +
> +struct perf_evlist *perf_evlist__new_aux(struct perf_evlist *parent)
> +{
> +	struct perf_evlist *evlist = zalloc(sizeof(*evlist));
> +
> +	if (evlist != NULL)
> +		perf_evlist__init(evlist, parent->cpus, parent->threads);
> +	evlist->parent = parent->parent;
> +	return evlist;
> +}
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index d740fb8..0505012 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -60,6 +60,7 @@ struct perf_evlist {
>  	struct perf_evsel *selected;
>  	struct events_stats stats;
>  	struct perf_env	*env;
> +	struct perf_evlist *parent;
>  };
>  
>  struct perf_evsel_str_handler {
> @@ -70,6 +71,8 @@ struct perf_evsel_str_handler {
>  struct perf_evlist *perf_evlist__new(void);
>  struct perf_evlist *perf_evlist__new_default(void);
>  struct perf_evlist *perf_evlist__new_dummy(void);
> +struct perf_evlist *perf_evlist__new_aux(struct perf_evlist *);
> +
>  void perf_evlist__init(struct perf_evlist *evlist, struct cpu_map *cpus,
>  		       struct thread_map *threads);
>  void perf_evlist__exit(struct perf_evlist *evlist);
> -- 
> 1.8.3.4