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

From: Arnaldo Carvalho de Melo
Date: Fri Sep 04 2015 - 09:29:36 EST

Next message: RaphaÃl Beamonte: "Re: [PATCH 04/15] perf tools: Move tracing_path interface into api/fs/tracing_path.c"
Previous message: Jiri Kosina: "Re: [PATCH 1/1] Corsair Vengeance K90 driver"
In reply to: Adrian Hunter: "[PATCH] perf tools: Fix gaps propagating maps"
Next in thread: Adrian Hunter: "Re: [PATCH] perf tools: Fix gaps propagating maps"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: RaphaÃl Beamonte: "Re: [PATCH 04/15] perf tools: Move tracing_path interface into api/fs/tracing_path.c"
Previous message: Jiri Kosina: "Re: [PATCH 1/1] Corsair Vengeance K90 driver"
In reply to: Adrian Hunter: "[PATCH] perf tools: Fix gaps propagating maps"
Next in thread: Adrian Hunter: "Re: [PATCH] perf tools: Fix gaps propagating maps"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]