Re: Filter option should follow a tracer option

[Jack Henschel <jackdev@mailbox.org>]

[-- Attachment #1.1: Type: text/plain, Size: 7012 bytes --]

On 08/23/2017 09:07 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Aug 23, 2017 at 03:40:41PM +0200, Jack Henschel escreveu:
>> On 08/23/2017 03:22 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Wed, Aug 23, 2017 at 02:11:16PM +0200, Jack Henschel escreveu:
>>>>> Adrian Hunter <adrian.hunter@intel.com> hat am 23. August 2017 um 12:33 geschrieben:
>>>>> On 23/08/17 11:51, Jack Henschel wrote:
>>>>>>> Adrian Hunter <adrian.hunter@intel.com> hat am 23. August 2017 um 08:06 geschrieben:
>>>>>>> On 22/08/17 22:00, Arnaldo Carvalho de Melo wrote:
>>>>>>>> Em Fri, Aug 18, 2017 at 11:43:51AM +0200, Jack Henschel escreveu:
>>>>>>>>> I'm using perf version 4.9.30 with kernel 4.9.30-2+deb9u3 on Debian 9 Stretch.
>>>
>>>>>>> It looks like either your CPU does not support Intel PT at all, or it does not support address filtering.
>>>
>>>>>> My CPU (Xeon CPU E5-2680 v4) supports Intel PT (intel_pt flag is
>>>>>> present in /proc/cpuinfo), but this is not an issue with Intel PT.
>>>>>> Here are some more examples (with various syntaxes):
>>>
>>>>> I think that is Broadwell which doesn't have address filtering.
>>>> Indeed, it is Broadwell.
>>>  
>>>>>>> Have you checked the value of /sys/bus/event_source/devices/intel_pt/nr_addr_filters ?
>>>>>> I don't have that file:
>>>>>
>>>>> Which definitely means address filtering is not supported.  That is
>>>>> mentioned in the man page for 'perf record'.
>>>
>>>> Ok, thanks for the clarification. It is mentioned in the perf record
>>>> man-page, but the perf error message could be a little bit more
>>>> specific for this issue. (e.g.
>>>> /sys/bus/event_source/devices/intel_pt/nr_addr_filters not present ->
>>>> "CPU does not support address filtering").
>>>
>>> Right, I'll get that into some __strerror() function. Will CC you when
>>> that is done.
>>>
>>> E.g. __strerror() function: perf_evsel__open_strerror(), that helps the
>>> user wrt errors returned by the perf_evsel__open() function:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/evsel.c?h=perf/core#n2669
>>
>> Hi,
>>
>> I just hacked together a quick patch for this, too :-)
>>
>> Since you are 
>> a) the maintainer of affected files (as I have just found out with the get_maintainer.pl script) 
>> and 
>> b) this is my first kernel patch
>> maybe you can give me some feedback on it. I did my best to follow all kernel coding and contributing guidelines.
> 
> Ok, thank you in advance.
> 
> Even in these cases, its always good to CC the development mailing list,
> which in this case is linux-kernel@vger.kernel.org, because even I'm
> being the maintainer of tools/perf/ in general there are areas where
> other people, like Jiri Olsa, here CCed have more experience or are the
> primary authors.
Ok, thanks for the hint. :-)

> Having said that, from a quick look, I was expecting a even more precise
> explanation, one tied to intel PT, where that file Adrian pointed out,
> would have its existence checked
> (/sys/bus/event_source/devices/intel_pt/nr_addr_filters).
I would have made the error message more explicit, but as far as I can see this does not necessarily have anything to with intel_pt (other hardware such as Intel RTIT may provide address filtering, too):

set_filter (util/parse-events.c) calls perf_pmu__scan to find available PMUs. 
perf_pmu__scan (util/pmu.c) is in this case (pmu == NULL) just a wrapper over pmu_read_sysfs.
pmu_read_sysfs (util/pmu.c) in turn just reads all available entries from EVENT_SOURCE_DEVICE_PATH (/sys/bus/event_source/devices/).

=> Not necessarily Intel PT specific.

> Because people that know that their CPU has Intel PT, say having a
> broadwell, like you, would maybe think that they could use hw filters,
> when only some later chips have that feature.
> 
> Adrian, is this something that started from some specific revision that
> we could point out in the message?

I spent yesterdays afternoon digging through the Intel Webpage to find out which chips / micro-architecture supports which feature, but so far the only real resource I have found is Andi Kleen's blog post on Intel PT:

CPU 	                                    Support
Broadwell (5th generation Core, Xeon v4)    More overhead. No fine grained timing.
Skylake (6th generation Core, Xeon v5) 	    Fine grained timing. Address filtering.
Goldmont (Apollo Lake, Denverton) 	    Fine grained timing. Address filtering.

http://halobates.de/blog/p/410

Greetings
Jack


> - Arnaldo
>  
>> (see attached)
>>
>> Greetings
>> Jack
> 
>> From 3143fb373b16eb6e8f674cca7127801e2809e4c0 Mon Sep 17 00:00:00 2001
>> From: Jack Henschel <jackdev@mailbox.org>
>> Date: Wed, 23 Aug 2017 15:20:40 +0200
>> Subject: [PATCH] perf events parse: Improve error message for address filters
>>
>> This patch improves the error message of the perf events parser
>> when the hardware does not support address filters.
>>
>> Previously, the perf returned the following error:
>>> --filter option should follow a -e tracepoint or HW tracer option
>> This implies there is some syntax error present in the command line,
>> which is not true. Rather, notify the user that the CPU does not have
>> support for this feature.
>>
>> Signed-off-by: Jack Henschel <jackdev@mailbox.org>
>> ---
>>  tools/perf/util/parse-events.c | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index 01e779b91c8e..7f9e3906fb81 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -1833,8 +1833,11 @@ static int set_filter(struct perf_evsel *evsel, const void *arg)
>>  	int nr_addr_filters = 0;
>>  	struct perf_pmu *pmu = NULL;
>>  
>> -	if (evsel == NULL)
>> -		goto err;
>> +	if (evsel == NULL) {
>> +		fprintf(stderr,
>> +			"--filter option should follow a -e tracepoint or HW tracer option\n");
>> +		return -1;
>> +	}
>>  
>>  	if (evsel->attr.type == PERF_TYPE_TRACEPOINT) {
>>  		if (perf_evsel__append_tp_filter(evsel, str) < 0) {
>> @@ -1856,8 +1859,11 @@ static int set_filter(struct perf_evsel *evsel, const void *arg)
>>  		perf_pmu__scan_file(pmu, "nr_addr_filters",
>>  				    "%d", &nr_addr_filters);
>>  
>> -	if (!nr_addr_filters)
>> -		goto err;
>> +	if (!nr_addr_filters) {
>> +		fprintf(stderr,
>> +			"This CPU does not support address filtering\n");
>> +		return -1;
>> +	}
>>  
>>  	if (perf_evsel__append_addr_filter(evsel, str) < 0) {
>>  		fprintf(stderr,
>> @@ -1866,12 +1872,6 @@ static int set_filter(struct perf_evsel *evsel, const void *arg)
>>  	}
>>  
>>  	return 0;
>> -
>> -err:
>> -	fprintf(stderr,
>> -		"--filter option should follow a -e tracepoint or HW tracer option\n");
>> -
>> -	return -1;
>>  }
>>  
>>  int parse_filter(const struct option *opt, const char *str,
>> -- 
>> 2.14.1
>>
> 
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 862 bytes --]