Re: [PATCH 34/41] perf evlist: Add 'system_wide' option

[Adrian Hunter <adrian.hunter@intel.com>]

On 15/07/2014 6:03 p.m., Arnaldo Carvalho de Melo wrote:
> Em Mon, Jul 14, 2014 at 01:02:58PM +0300, Adrian Hunter escreveu:
>> Add an option to cause a selected event
>> to be opened always without a pid when
>> configured by perf_evsel__config().
>>
>> This is needed when using the sched_switch
>> tracepoint to follow object code execution.
>> sched_switch occurs before the task
>> switch and so it cannot record it in a
>> context limited to that task.  Note
>> that also means that sched_switch is
>> useless when capturing data per-thread,
>> as is the 'context-switches' software
>> event for the same reason.
>
> clever, but humm, need to be judicious when allocating things like the
> pollfd, I think, more below...
>
> ... after going thru the whole patch, its not clear how pollfd usage
> takes into account the fact that some of the evsels don't have
> thread_map->nr entries, but just one, will continue reading...
>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>   tools/perf/util/evlist.c | 45 +++++++++++++++++++++++++++++++++++++--------
>>   tools/perf/util/evsel.c  | 31 ++++++++++++++++++++++++++-----
>>   tools/perf/util/evsel.h  |  1 +
>>   3 files changed, 64 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index 282e83e..c295b7b 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -265,17 +265,27 @@ int perf_evlist__add_newtp(struct perf_evlist *evlist,
>>   	return 0;
>>   }
>>
>> +static int perf_evlist__nr_threads(struct perf_evlist *evlist,
>> +				   struct perf_evsel *evsel)
>> +{
>> +	if (evsel->system_wide)
>> +		return 1;
>> +	else
>> +		return thread_map__nr(evlist->threads);
>> +}
>> +
>>   void perf_evlist__disable(struct perf_evlist *evlist)
>>   {
>>   	int cpu, thread;
>>   	struct perf_evsel *pos;
>>   	int nr_cpus = cpu_map__nr(evlist->cpus);
>> -	int nr_threads = thread_map__nr(evlist->threads);
>> +	int nr_threads;
>>
>>   	for (cpu = 0; cpu < nr_cpus; cpu++) {
>>   		evlist__for_each(evlist, pos) {
>>   			if (!perf_evsel__is_group_leader(pos) || !pos->fd)
>>   				continue;
>> +			nr_threads = perf_evlist__nr_threads(evlist, pos);
>>   			for (thread = 0; thread < nr_threads; thread++)
>>   				ioctl(FD(pos, cpu, thread),
>>   				      PERF_EVENT_IOC_DISABLE, 0);
>> @@ -288,12 +298,13 @@ void perf_evlist__enable(struct perf_evlist *evlist)
>>   	int cpu, thread;
>>   	struct perf_evsel *pos;
>>   	int nr_cpus = cpu_map__nr(evlist->cpus);
>> -	int nr_threads = thread_map__nr(evlist->threads);
>> +	int nr_threads;
>>
>>   	for (cpu = 0; cpu < nr_cpus; cpu++) {
>>   		evlist__for_each(evlist, pos) {
>>   			if (!perf_evsel__is_group_leader(pos) || !pos->fd)
>>   				continue;
>> +			nr_threads = perf_evlist__nr_threads(evlist, pos);
>>   			for (thread = 0; thread < nr_threads; thread++)
>>   				ioctl(FD(pos, cpu, thread),
>>   				      PERF_EVENT_IOC_ENABLE, 0);
>> @@ -305,12 +316,14 @@ int perf_evlist__disable_event(struct perf_evlist *evlist,
>>   			       struct perf_evsel *evsel)
>>   {
>>   	int cpu, thread, err;
>> +	int nr_cpus = cpu_map__nr(evlist->cpus);
>> +	int nr_threads = perf_evlist__nr_threads(evlist, evsel);
>>
>>   	if (!evsel->fd)
>>   		return 0;
>>
>> -	for (cpu = 0; cpu < evlist->cpus->nr; cpu++) {
>> -		for (thread = 0; thread < evlist->threads->nr; thread++) {
>> +	for (cpu = 0; cpu < nr_cpus; cpu++) {
>> +		for (thread = 0; thread < nr_threads; thread++) {
>>   			err = ioctl(FD(evsel, cpu, thread),
>>   				    PERF_EVENT_IOC_DISABLE, 0);
>>   			if (err)
>> @@ -324,12 +337,14 @@ int perf_evlist__enable_event(struct perf_evlist *evlist,
>>   			      struct perf_evsel *evsel)
>>   {
>>   	int cpu, thread, err;
>> +	int nr_cpus = cpu_map__nr(evlist->cpus);
>> +	int nr_threads = perf_evlist__nr_threads(evlist, evsel);
>>
>>   	if (!evsel->fd)
>>   		return -EINVAL;
>>
>> -	for (cpu = 0; cpu < evlist->cpus->nr; cpu++) {
>> -		for (thread = 0; thread < evlist->threads->nr; thread++) {
>> +	for (cpu = 0; cpu < nr_cpus; cpu++) {
>> +		for (thread = 0; thread < nr_threads; thread++) {
>>   			err = ioctl(FD(evsel, cpu, thread),
>>   				    PERF_EVENT_IOC_ENABLE, 0);
>>   			if (err)
>
> All the above seems ok, the threads (x axis) array is flat when syswide, ok.
>
>> @@ -343,7 +358,16 @@ static int perf_evlist__alloc_pollfd(struct perf_evlist *evlist)
>>   {
>>   	int nr_cpus = cpu_map__nr(evlist->cpus);
>>   	int nr_threads = thread_map__nr(evlist->threads);
>> -	int nfds = nr_cpus * nr_threads * evlist->nr_entries;
>> +	int nfds = 0;
>> +	struct perf_evsel *evsel;
>> +
>> +	list_for_each_entry(evsel, &evlist->entries, node) {
>> +		if (evsel->system_wide)
>> +			nfds += nr_cpus;
>> +		else
>> +			nfds += nr_cpus * nr_threads;
>> +	}
>
> But here looks tricky, will look how the evlist->pollfd is used...
>
>> +
>>   	evlist->pollfd = malloc(sizeof(struct pollfd) * nfds);
>>   	return evlist->pollfd != NULL ? 0 : -ENOMEM;
>>   }
>> @@ -636,7 +660,12 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
>>   	struct perf_evsel *evsel;
>>
>>   	evlist__for_each(evlist, evsel) {
>> -		int fd = FD(evsel, cpu, thread);
>> +		int fd;
>> +
>> +		if (evsel->system_wide && thread)
>> +			continue;
>
> Discard after thread 0, i.e. the first, ok.
>
>> +
>> +		fd = FD(evsel, cpu, thread);
>>
>>   		if (*output == -1) {
>>   			*output = fd;
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index 90f58cd..7540b5f 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -691,6 +691,10 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
>>   int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
>>   {
>>   	int cpu, thread;
>> +
>> +	if (evsel->system_wide)
>> +		nthreads = 1;
>
> flatten it, ok
>
>> +
>>   	evsel->fd = xyarray__new(ncpus, nthreads, sizeof(int));
>>
>>   	if (evsel->fd) {
>> @@ -709,6 +713,9 @@ static int perf_evsel__run_ioctl(struct perf_evsel *evsel, int ncpus, int nthrea
>>   {
>>   	int cpu, thread;
>>
>> +	if (evsel->system_wide)
>> +		nthreads = 1;
>> +
>
> Ditto.
>
>>   	for (cpu = 0; cpu < ncpus; cpu++) {
>>   		for (thread = 0; thread < nthreads; thread++) {
>>   			int fd = FD(evsel, cpu, thread),
>> @@ -739,6 +746,9 @@ int perf_evsel__enable(struct perf_evsel *evsel, int ncpus, int nthreads)
>>
>>   int perf_evsel__alloc_id(struct perf_evsel *evsel, int ncpus, int nthreads)
>>   {
>> +	if (evsel->system_wide)
>> +		nthreads = 1;
>> +
>>   	evsel->sample_id = xyarray__new(ncpus, nthreads, sizeof(struct perf_sample_id));
>
> These are per evsel, so can be flattened when we get to it,
> evlist->pollfd is per evlist, so mixes syswide with !syswide,  that is why I'm scratching my head at this
> point...

pollfd is not a 2-dimensional array.  It is just a 1-dimensional array.
So it just needs to be the right length.

>
>>   	if (evsel->sample_id == NULL)
>>   		return -ENOMEM;
>> @@ -783,6 +793,9 @@ void perf_evsel__close_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
>>   {
>>   	int cpu, thread;
>>
>> +	if (evsel->system_wide)
>> +		nthreads = 1;
>> +
>>   	for (cpu = 0; cpu < ncpus; cpu++)
>>   		for (thread = 0; thread < nthreads; ++thread) {
>>   			close(FD(evsel, cpu, thread));
>> @@ -871,6 +884,9 @@ int __perf_evsel__read(struct perf_evsel *evsel,
>>   	int cpu, thread;
>>   	struct perf_counts_values *aggr = &evsel->counts->aggr, count;
>>
>> +	if (evsel->system_wide)
>> +		nthreads = 1;
>> +
>>   	aggr->val = aggr->ena = aggr->run = 0;
>>
>>   	for (cpu = 0; cpu < ncpus; cpu++) {
>> @@ -993,13 +1009,18 @@ static size_t perf_event_attr__fprintf(struct perf_event_attr *attr, FILE *fp)
>>   static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>>   			      struct thread_map *threads)
>>   {
>> -	int cpu, thread;
>> +	int cpu, thread, nthreads;
>>   	unsigned long flags = 0;
>>   	int pid = -1, err;
>>   	enum { NO_CHANGE, SET_TO_MAX, INCREASED_MAX } set_rlimit = NO_CHANGE;
>>
>> +	if (evsel->system_wide)
>> +		nthreads = 1;
>> +	else
>> +		nthreads = threads->nr;
>> +
>>   	if (evsel->fd == NULL &&
>> -	    perf_evsel__alloc_fd(evsel, cpus->nr, threads->nr) < 0)
>> +	    perf_evsel__alloc_fd(evsel, cpus->nr, nthreads) < 0)
>>   		return -ENOMEM;
>>
>>   	if (evsel->cgrp) {
>> @@ -1021,10 +1042,10 @@ retry_sample_id:
>>
>>   	for (cpu = 0; cpu < cpus->nr; cpu++) {
>>
>> -		for (thread = 0; thread < threads->nr; thread++) {
>> +		for (thread = 0; thread < nthreads; thread++) {
>>   			int group_fd;
>>
>> -			if (!evsel->cgrp)
>> +			if (!evsel->cgrp && !evsel->system_wide)
>>   				pid = threads->map[thread];
>>
>>   			group_fd = get_group_fd(evsel, cpu, thread);
>> @@ -1094,7 +1115,7 @@ out_close:
>>   			close(FD(evsel, cpu, thread));
>>   			FD(evsel, cpu, thread) = -1;
>>   		}
>> -		thread = threads->nr;
>> +		thread = nthreads;
>>   	} while (--cpu >= 0);
>>   	return err;
>>   }
>> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
>> index d7f93ce..dbb2a0d 100644
>> --- a/tools/perf/util/evsel.h
>> +++ b/tools/perf/util/evsel.h
>> @@ -85,6 +85,7 @@ struct perf_evsel {
>>   	bool 			needs_swap;
>>   	bool			no_aux_samples;
>>   	bool			immediate;
>> +	bool			system_wide;
>>   	/* parse modifier helper */
>>   	int			exclude_GH;
>>   	int			nr_members;
>> --
>> 1.8.3.2