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

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: <57393C17.5070405@huawei.com>
Date:	Mon, 16 May 2016 11:18:47 +0800
From:	"Wangnan (F)" <wangnan0@...wei.com>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>
CC:	<arnaldo.melo@...il.com>, <linux-kernel@...r.kernel.org>,
	He Kuang <hekuang@...wei.com>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Namhyung Kim <namhyung@...nel.org>,
	Zefan Li <lizefan@...wei.com>, <pi3orama@....com>
Subject: Re: [PATCH 08/17] perf record: Don't poll on overwrite channel



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@...wei.com>
>> Signed-off-by: He Kuang <hekuang@...wei.com>
>> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
>> Cc: Jiri Olsa <jolsa@...nel.org>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
>> Cc: Namhyung Kim <namhyung@...nel.org>
>> Cc: Zefan Li <lizefan@...wei.com>
>> Cc: pi3orama@....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.