Re: [PATCH 08/17] perf record: Don't poll on overwrite channel

["Wangnan (F)" <wangnan0@huawei.com>]

On 2016/5/13 21:12, Arnaldo Carvalho de Melo wrote:
> Em Fri, May 13, 2016 at 07:56:05AM +0000, Wang Nan escreveu:
>> There's no need to receive events from overwritable ring buffer. Instead,
>> perf should make them run background until something happen. This patch
>> makes normal events from overwrite ring buffer ignored.
>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> Signed-off-by: He Kuang <hekuang@huawei.com>
>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Zefan Li <lizefan@huawei.com>
>> Cc: pi3orama@163.com
>> ---
>>   tools/perf/util/evlist.c | 23 +++++++++++++++++++----
>>   1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index abce588..f0b0457 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -461,9 +461,9 @@ int perf_evlist__alloc_pollfd(struct perf_evlist *evlist)
>>   	return 0;
>>   }
>>   
>> -static int __perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd, int idx)
>> +static int __perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd, int idx, short revent)
>>   {
>> -	int pos = fdarray__add(&evlist->pollfd, fd, POLLIN | POLLERR | POLLHUP);
>> +	int pos = fdarray__add(&evlist->pollfd, fd, revent | POLLERR | POLLHUP);
>>   	/*
>>   	 * Save the idx so that when we filter out fds POLLHUP'ed we can
>>   	 * close the associated evlist->mmap[] entry.
>> @@ -479,7 +479,7 @@ static int __perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd, int idx
>>   
>>   int perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd)
>>   {
>> -	return __perf_evlist__add_pollfd(evlist, fd, -1);
>> +	return __perf_evlist__add_pollfd(evlist, fd, -1, POLLIN);
>>   }
>>   
>>   static void perf_evlist__munmap_filtered(struct fdarray *fda, int fd)
>> @@ -1077,6 +1077,18 @@ perf_evlist__channel_complete(struct perf_evlist *evlist)
>>   	return 0;
>>   }
>>   
>> +static bool
>> +perf_evlist__should_poll(struct perf_evlist *evlist,
>> +			 struct perf_evsel *evsel,
>> +			 int channel)
>> +{
>> +	if (evsel->system_wide)
>> +		return false;
> So, what is the above doing in this patch? If we should not poll when in
> syswide mode, then this should be in a separate patch, unrelated to
> 'channels'.  No?

I think the name 'system_wide' is more or less missleading. It is not means
an event in 'perf record -a', but means "a selected event to be opened 
always
without a pid when configured by perf_evsel__config().". See bf8e8f4b8.

Here we use similary logic in existing perf_evlist__mmap_per_evsel. It never
poll system_wide evsel:

                 /*
                  * The system_wide flag causes a selected event to be 
opened
                  * always without a pid.  Consequently it will never get a
                  * POLLHUP, but it is used for tracking in combination with
                  * other events, so it should not need to be polled anyway.
                  * Therefore don't add it for polling.
                  */
                 if (!evsel->system_wide &&
                     __perf_evlist__add_pollfd(evlist, fd, idx) < 0) {
                         perf_evlist__mmap_put(evlist, idx);
                         return -1;
                 }

Thank you.