Re: [PATCH] perf tools: Fix gaps propagating maps

[Arnaldo Carvalho de Melo <acme@redhat.com>]

Em Fri, Sep 04, 2015 at 03:15:54PM +0300, Adrian Hunter escreveu:
> A perf_evsel is a selected event containing the perf_event_attr
> that is passed to perf_event_open().  A perf_evlist is a collection
> of perf_evsel's.  A perf_evlist also has lists of cpus and threads
> (pids) on which to open the event.  These lists are called 'maps'
> and this patch is about how those 'maps' are propagated from the
>> perf_evlist to the perf_evsels.

Can't this be broken up in multiple patches, for instance this:

 int perf_evlist__create_maps(struct perf_evlist *evlist, struct
 target *target)
 {
+     if (evlist->threads || evlist->cpus)
+             return -1;
+

Looks like a fix that could be separated. Also FOO__propagate(.., false)
to do the opposite of propagate seems confusing, how about
FOO__unpropagate() if that verb exists? :-)


Also, when unpropagating, you do:

             if (evsel->cpus == evlist->cpus) {
                     cpu_map__put(evsel->cpus);
                     evsel->cpus = NULL;
             }

What if the PMU code _set_ it to the same cpus as in evlist->cpus, but
now we're unpropagating to set to another CPU, in this case we will be
changing the PMU setting with a new one. I.e. when a PMU sets it it
should be sticky, no?

I.e. we would have to know, in the evsel, if evsel->cpus was set by the
PMU or any other future entity expecting this behaviour, so that we
don't touch it, i.e. testing (evsel->cpus != evlist->cpus) when
unpropagating doesn't seem to cut, right?

- Arnaldo

 
> Originally perf_evsels did not have their own thread 'maps', and
> their cpu 'maps' were only used for checking.  Then threads were
> added by:
> 
> 	578e91ec04d0 ("perf evlist: Propagate thread maps through the evlist")
> 
> And then 'perf record' was changed to open events using the 'maps'
> from perf_evsel not perf_evlist anymore, by:
> 
> 	d988d5ee6478 ("perf evlist: Open event on evsel cpus and threads")
> 
> That exposed situations where the 'maps' were not getting propagated
> correctly, such as when perf_evsel are added to the perf_evlist later.
> This patch ensures 'maps' get propagated in that case, and also if 'maps'
> are subsequently changed or set to NULL by perf_evlist__set_maps()
> and perf_evlist__create_syswide_maps().
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  tools/perf/util/evlist.c | 129 ++++++++++++++++++++++++++++++++---------------
>  tools/perf/util/evlist.h |   1 +
>  2 files changed, 88 insertions(+), 42 deletions(-)
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index d51a5200c8af..00128cf7655b 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -124,8 +124,49 @@ void perf_evlist__delete(struct perf_evlist *evlist)
>  	free(evlist);
>  }
>  
> +static void __perf_evlist__propagate_maps(struct perf_evlist *evlist,
> +					  struct perf_evsel *evsel,
> +					  bool propagate)
> +{
> +	if (propagate) {
> +		/*
> +		 * We already have cpus for evsel (via PMU sysfs) so
> +		 * keep it, if there's no target cpu list defined.
> +		 */
> +		if ((!evsel->cpus || evlist->has_user_cpus) &&
> +		    evsel->cpus != evlist->cpus) {
> +			cpu_map__put(evsel->cpus);
> +			evsel->cpus = cpu_map__get(evlist->cpus);
> +		}
> +
> +		if (!evsel->threads)
> +			evsel->threads = thread_map__get(evlist->threads);
> +	} else {
> +		if (evsel->cpus == evlist->cpus) {
> +			cpu_map__put(evsel->cpus);
> +			evsel->cpus = NULL;
> +		}
> +
> +		if (evsel->threads == evlist->threads) {
> +			thread_map__put(evsel->threads);
> +			evsel->threads = NULL;
> +		}
> +	}
> +}
> +
> +static void perf_evlist__propagate_maps(struct perf_evlist *evlist,
> +					bool propagate)
> +{
> +	struct perf_evsel *evsel;
> +
> +	evlist__for_each(evlist, evsel)
> +		__perf_evlist__propagate_maps(evlist, evsel, propagate);
> +}
> +
>  void perf_evlist__add(struct perf_evlist *evlist, struct perf_evsel *entry)
>  {
> +	__perf_evlist__propagate_maps(evlist, entry, true);
> +
>  	entry->evlist = evlist;
>  	list_add_tail(&entry->node, &evlist->entries);
>  	entry->idx = evlist->nr_entries;
> @@ -140,6 +181,10 @@ void perf_evlist__splice_list_tail(struct perf_evlist *evlist,
>  				   int nr_entries)
>  {
>  	bool set_id_pos = !evlist->nr_entries;
> +	struct perf_evsel *evsel;
> +
> +	__evlist__for_each(list, evsel)
> +		__perf_evlist__propagate_maps(evlist, evsel, true);
>  
>  	list_splice_tail(list, &evlist->entries);
>  	evlist->nr_entries += nr_entries;
> @@ -1103,34 +1148,11 @@ int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages,
>  	return perf_evlist__mmap_ex(evlist, pages, overwrite, 0, false);
>  }
>  
> -static int perf_evlist__propagate_maps(struct perf_evlist *evlist,
> -				       bool has_user_cpus)
> -{
> -	struct perf_evsel *evsel;
> -
> -	evlist__for_each(evlist, evsel) {
> -		/*
> -		 * We already have cpus for evsel (via PMU sysfs) so
> -		 * keep it, if there's no target cpu list defined.
> -		 */
> -		if (evsel->cpus && has_user_cpus)
> -			cpu_map__put(evsel->cpus);
> -
> -		if (!evsel->cpus || has_user_cpus)
> -			evsel->cpus = cpu_map__get(evlist->cpus);
> -
> -		evsel->threads = thread_map__get(evlist->threads);
> -
> -		if ((evlist->cpus && !evsel->cpus) ||
> -		    (evlist->threads && !evsel->threads))
> -			return -ENOMEM;
> -	}
> -
> -	return 0;
> -}
> -
>  int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)
>  {
> +	if (evlist->threads || evlist->cpus)
> +		return -1;
> +
>  	evlist->threads = thread_map__new_str(target->pid, target->tid,
>  					      target->uid);
>  
> @@ -1145,7 +1167,11 @@ int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target)
>  	if (evlist->cpus == NULL)
>  		goto out_delete_threads;
>  
> -	return perf_evlist__propagate_maps(evlist, !!target->cpu_list);
> +	evlist->has_user_cpus = !!target->cpu_list;
> +
> +	perf_evlist__propagate_maps(evlist, true);
> +
> +	return 0;
>  
>  out_delete_threads:
>  	thread_map__put(evlist->threads);
> @@ -1157,17 +1183,32 @@ int perf_evlist__set_maps(struct perf_evlist *evlist,
>  			  struct cpu_map *cpus,
>  			  struct thread_map *threads)
>  {
> -	if (evlist->cpus)
> -		cpu_map__put(evlist->cpus);
> +	/*
> +	 * First 'un-propagate' the current maps which allows the propagation to
> +	 * work correctly even when changing the maps or setting them to NULL.
> +	 */
> +	perf_evlist__propagate_maps(evlist, false);
>  
> -	evlist->cpus = cpus;
> +	/*
> +	 * Allow for the possibility that one or another of the maps isn't being
> +	 * changed i.e. don't put it.  Note we are assuming the maps that are
> +	 * being applied are brand new and evlist is taking ownership of the
> +	 * original reference count of 1.  If that is not the case it is up to
> +	 * the caller to increase the reference count.
> +	 */
> +	if (cpus != evlist->cpus) {
> +		cpu_map__put(evlist->cpus);
> +		evlist->cpus = cpus;
> +	}
>  
> -	if (evlist->threads)
> +	if (threads != evlist->threads) {
>  		thread_map__put(evlist->threads);
> +		evlist->threads = threads;
> +	}
>  
> -	evlist->threads = threads;
> +	perf_evlist__propagate_maps(evlist, true);
>  
> -	return perf_evlist__propagate_maps(evlist, false);
> +	return 0;
>  }
>  
>  int perf_evlist__apply_filters(struct perf_evlist *evlist, struct perf_evsel **err_evsel)
> @@ -1387,6 +1428,8 @@ void perf_evlist__close(struct perf_evlist *evlist)
>  
>  static int perf_evlist__create_syswide_maps(struct perf_evlist *evlist)
>  {
> +	struct cpu_map	  *cpus;
> +	struct thread_map *threads;
>  	int err = -ENOMEM;
>  
>  	/*
> @@ -1398,20 +1441,22 @@ static int perf_evlist__create_syswide_maps(struct perf_evlist *evlist)
>  	 * error, and we may not want to do that fallback to a
>  	 * default cpu identity map :-\
>  	 */
> -	evlist->cpus = cpu_map__new(NULL);
> -	if (evlist->cpus == NULL)
> +	cpus = cpu_map__new(NULL);
> +	if (!cpus)
>  		goto out;
>  
> -	evlist->threads = thread_map__new_dummy();
> -	if (evlist->threads == NULL)
> -		goto out_free_cpus;
> +	threads = thread_map__new_dummy();
> +	if (!threads)
> +		goto out_put;
>  
> -	err = 0;
> +	err = perf_evlist__set_maps(evlist, cpus, threads);
> +	if (err)
> +		goto out_put;
>  out:
>  	return err;
> -out_free_cpus:
> -	cpu_map__put(evlist->cpus);
> -	evlist->cpus = NULL;
> +out_put:
> +	cpu_map__put(cpus);
> +	thread_map__put(threads);
>  	goto out;
>  }
>  
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index b39a6198f4ac..2dd5715dfef6 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -42,6 +42,7 @@ struct perf_evlist {
>  	int		 nr_mmaps;
>  	bool		 overwrite;
>  	bool		 enabled;
> +	bool		 has_user_cpus;
>  	size_t		 mmap_len;
>  	int		 id_pos;
>  	int		 is_pos;
> -- 
> 1.9.1