linux-kernel - Re: [PATCH] perf tools: Fix gaps propagating maps

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55E99FC6.5060807@intel.com>
Date:	Fri, 4 Sep 2015 16:42:30 +0300
From:	Adrian Hunter <adrian.hunter@...el.com>
To:	Arnaldo Carvalho de Melo <acme@...hat.com>
Cc:	Jiri Olsa <jolsa@...hat.com>, jolsa@...nel.org, mingo@...nel.org,
	kan.liang@...el.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf tools: Fix gaps propagating maps

On 04/09/15 16:28, Arnaldo Carvalho de Melo wrote:
> 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:

Ok, might not be until next week though.

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

Or you could just drop that chunk.

> 
> 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? :-)

Ok

> 
> 
> 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?

We are comparing the pointer, so that won't happen unless the PMU actually
does evsel->cpus = evlist->cpus which seems unlikely.

> 
> 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?

I think the pointer comparison covers that. i.e. the pointers won't be the
same even if the cpus are.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/