Re: [PATCH 1/2] perf tests: Fix switch tracking test for P4

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

On 01/06/17 16:11, Arnaldo Carvalho de Melo wrote:
> Em Fri, May 26, 2017 at 02:31:40PM +0200, Jiri Olsa escreveu:
>> The switch tracking test keeps failing on P4 cpu,
>> when NMI watchdog is enabled.
>>
>> The reason is that P4 pmu uses substitute event for cycles
>> when it's already taken (in our case by NMI watchdog), but
>> this event does not give even results like cycles, and we
>> could end up with no samples at all for our short
>> measuring period.

Did you consider increasing the measuring period?

>>
>> Fixing this by using "instructions:u" event instead,
>> which seems to be stable enough.
> 
> The original author of this test entry is Adrian, so would be nice for
> him to take a look and give his Ack, Adrian?
> 
> - Arnaldo
>  
>> Cc: Michael Petlan <mpetlan@redhat.com>
>> Link: http://lkml.kernel.org/n/tip-4s8vo7skneszacdckv7uiog3@git.kernel.org
>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>> ---
>>  tools/perf/tests/switch-tracking.c | 27 +++++++++++++++++++++++++--
>>  1 file changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/tests/switch-tracking.c b/tools/perf/tests/switch-tracking.c
>> index 65474fd80da7..e519819ea2e5 100644
>> --- a/tools/perf/tests/switch-tracking.c
>> +++ b/tools/perf/tests/switch-tracking.c
>> @@ -10,6 +10,7 @@
>>  #include "thread_map.h"
>>  #include "cpumap.h"
>>  #include "tests.h"
>> +#include "header.h"
>>  
>>  static int spin_sleep(void)
>>  {
>> @@ -298,6 +299,27 @@ static int process_events(struct perf_evlist *evlist,
>>  	return ret;
>>  }
>>  
>> +static const char *get_hw_counter(void)
>> +{
>> +	const char *counter = "cycles:u";
>> +	char *cpuid;
>> +
>> +	cpuid = get_cpuid_str();
>> +
>> +	/*
>> +	 * P4 pmu uses substitute event for cycles if it's already
>> +	 * taken, but it does not give even results like cycles,
>> +	 * and we could end up with no samples at all for our short
>> +	 * measuring period. Using "instructions:u" event instead,
>> +	 * which seems to be stable enough.
>> +	 */
>> +	if (!strcmp("GenuineIntel-15-4", cpuid))

Why just model 4?  Isn't all family 15 P4?

>> +		counter = "instructions:u";
>> +
>> +	pr_debug("using '%s' HW counter");

tests/switch-tracking.c: In function ‘get_hw_counter’:
tests/switch-tracking.c:319:2: error: format ‘%s’ expects a matching ‘char
*’ argument [-Werror=format=]


>> +	return counter;
>> +}
>> +
>>  /**
>>   * test__switch_tracking - test using sched_switch and tracking events.
>>   *
>> @@ -308,6 +330,7 @@ static int process_events(struct perf_evlist *evlist,
>>   */
>>  int test__switch_tracking(int subtest __maybe_unused)
>>  {
>> +	const char *hw_counter = get_hw_counter();
>>  	const char *sched_switch = "sched:sched_switch";
>>  	struct switch_tracking switch_tracking = { .tids = NULL, };
>>  	struct record_opts opts = {
>> @@ -357,9 +380,9 @@ int test__switch_tracking(int subtest __maybe_unused)
>>  	cpu_clocks_evsel = perf_evlist__last(evlist);
>>  
>>  	/* Second event */
>> -	err = parse_events(evlist, "cycles:u", NULL);
>> +	err = parse_events(evlist, hw_counter, NULL);
>>  	if (err) {
>> -		pr_debug("Failed to parse event cycles:u\n");
>> +		pr_debug("Failed to parse event %s\n", hw_counter);
>>  		goto out_err;
>>  	}
>>  
>> -- 
>> 2.9.4
>