[PATCH 0/2] perf tests: Fix Convert perf time to TSC test for hybrid

[PATCH 0/2] perf tests: Fix Convert perf time to TSC test for hybrid

[Adrian Hunter]

Hi

Here are a couple more minor non-urgent fixes.


Adrian Hunter (2):
      perf tests: Stop Convert perf time to TSC test opening events twice
      perf tests: Fix Convert perf time to TSC test for hybrid

 tools/perf/tests/perf-time-to-tsc.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)


Regards
Adrian

[PATCH 1/2] perf tests: Stop Convert perf time to TSC test opening events twice

[Adrian Hunter]

Do not call evlist__open() twice.

Fixes: 5bb017d4b97a ("perf test: Fix error message for test case 71 on s390, where it is not supported")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/tests/perf-time-to-tsc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
index 4ad0dfbc8b21..8d6d60173693 100644
--- a/tools/perf/tests/perf-time-to-tsc.c
+++ b/tools/perf/tests/perf-time-to-tsc.c
@@ -123,11 +123,14 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 		evsel->core.attr.enable_on_exec = 0;
 	}
 
-	if (evlist__open(evlist) == -ENOENT) {
-		err = TEST_SKIP;
+	ret = evlist__open(evlist);
+	if (ret < 0) {
+		if (ret == -ENOENT)
+			err = TEST_SKIP;
+		else
+			pr_debug("evlist__open() failed\n");
 		goto out_err;
 	}
-	CHECK__(evlist__open(evlist));
 
 	CHECK__(evlist__mmap(evlist, UINT_MAX));
 
-- 
2.25.1

Re: [PATCH 1/2] perf tests: Stop Convert perf time to TSC test opening events twice

[Liang, Kan]

On 2022-07-13 8:34 a.m., Adrian Hunter wrote:
> Do not call evlist__open() twice.
> 
> Fixes: 5bb017d4b97a ("perf test: Fix error message for test case 71 on s390, where it is not supported")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>> ---
>  tools/perf/tests/perf-time-to-tsc.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
> index 4ad0dfbc8b21..8d6d60173693 100644
> --- a/tools/perf/tests/perf-time-to-tsc.c
> +++ b/tools/perf/tests/perf-time-to-tsc.c
> @@ -123,11 +123,14 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  		evsel->core.attr.enable_on_exec = 0;
>  	}
>  
> -	if (evlist__open(evlist) == -ENOENT) {
> -		err = TEST_SKIP;
> +	ret = evlist__open(evlist);
> +	if (ret < 0) {
> +		if (ret == -ENOENT)
> +			err = TEST_SKIP;
> +		else
> +			pr_debug("evlist__open() failed\n");

The error message for the evlist__open is slightly different with the
old one, "evlist__open(evlist) failed!". Not sure if anyone care about it.

Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan

>  		goto out_err;
>  	}
> -	CHECK__(evlist__open(evlist));
>  
>  	CHECK__(evlist__mmap(evlist, UINT_MAX));
>  

[PATCH 2/2] perf tests: Fix Convert perf time to TSC test for hybrid

[Adrian Hunter]

The test does not always correctly determine the number of events for
hybrids, nor allow for more than 1 evsel when parsing.

Fix by iterating the events actually created and getting the correct
evsel for the events processed.

Fixes: d9da6f70eb23 ("perf tests: Support 'Convert perf time to TSC' test for hybrid")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/tests/perf-time-to-tsc.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
index 8d6d60173693..7c7d20fc503a 100644
--- a/tools/perf/tests/perf-time-to-tsc.c
+++ b/tools/perf/tests/perf-time-to-tsc.c
@@ -20,8 +20,6 @@
 #include "tsc.h"
 #include "mmap.h"
 #include "tests.h"
-#include "pmu.h"
-#include "pmu-hybrid.h"
 
 /*
  * Except x86_64/i386 and Arm64, other archs don't support TSC in perf.  Just
@@ -106,18 +104,8 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 
 	evlist__config(evlist, &opts, NULL);
 
-	evsel = evlist__first(evlist);
-
-	evsel->core.attr.comm = 1;
-	evsel->core.attr.disabled = 1;
-	evsel->core.attr.enable_on_exec = 0;
-
-	/*
-	 * For hybrid "cycles:u", it creates two events.
-	 * Init the second evsel here.
-	 */
-	if (perf_pmu__has_hybrid() && perf_pmu__hybrid_mounted("cpu_atom")) {
-		evsel = evsel__next(evsel);
+	/* For hybrid "cycles:u", it creates two events */
+	evlist__for_each_entry(evlist, evsel) {
 		evsel->core.attr.comm = 1;
 		evsel->core.attr.disabled = 1;
 		evsel->core.attr.enable_on_exec = 0;
@@ -170,10 +158,12 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 				goto next_event;
 
 			if (strcmp(event->comm.comm, comm1) == 0) {
+				CHECK_NOT_NULL__(evsel = evlist__event2evsel(evlist, event));
 				CHECK__(evsel__parse_sample(evsel, event, &sample));
 				comm1_time = sample.time;
 			}
 			if (strcmp(event->comm.comm, comm2) == 0) {
+				CHECK_NOT_NULL__(evsel = evlist__event2evsel(evlist, event));
 				CHECK__(evsel__parse_sample(evsel, event, &sample));
 				comm2_time = sample.time;
 			}
-- 
2.25.1

Re: [PATCH 2/2] perf tests: Fix Convert perf time to TSC test for hybrid

[Liang, Kan]

On 2022-07-13 8:34 a.m., Adrian Hunter wrote:
> The test does not always correctly determine the number of events for
> hybrids, nor allow for more than 1 evsel when parsing.
> 
> Fix by iterating the events actually created and getting the correct
> evsel for the events processed.
> 

Yes, we cannot always assume there are two events for hybrid.

> Fixes: d9da6f70eb23 ("perf tests: Support 'Convert perf time to TSC' test for hybrid")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>


Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan
> ---
>  tools/perf/tests/perf-time-to-tsc.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
> index 8d6d60173693..7c7d20fc503a 100644
> --- a/tools/perf/tests/perf-time-to-tsc.c
> +++ b/tools/perf/tests/perf-time-to-tsc.c
> @@ -20,8 +20,6 @@
>  #include "tsc.h"
>  #include "mmap.h"
>  #include "tests.h"
> -#include "pmu.h"
> -#include "pmu-hybrid.h"
>  
>  /*
>   * Except x86_64/i386 and Arm64, other archs don't support TSC in perf.  Just
> @@ -106,18 +104,8 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  
>  	evlist__config(evlist, &opts, NULL);
>  
> -	evsel = evlist__first(evlist);
> -
> -	evsel->core.attr.comm = 1;
> -	evsel->core.attr.disabled = 1;
> -	evsel->core.attr.enable_on_exec = 0;
> -
> -	/*
> -	 * For hybrid "cycles:u", it creates two events.
> -	 * Init the second evsel here.
> -	 */
> -	if (perf_pmu__has_hybrid() && perf_pmu__hybrid_mounted("cpu_atom")) {
> -		evsel = evsel__next(evsel);
> +	/* For hybrid "cycles:u", it creates two events */
> +	evlist__for_each_entry(evlist, evsel) {
>  		evsel->core.attr.comm = 1;
>  		evsel->core.attr.disabled = 1;
>  		evsel->core.attr.enable_on_exec = 0;
> @@ -170,10 +158,12 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  				goto next_event;
>  
>  			if (strcmp(event->comm.comm, comm1) == 0) {
> +				CHECK_NOT_NULL__(evsel = evlist__event2evsel(evlist, event));
>  				CHECK__(evsel__parse_sample(evsel, event, &sample));
>  				comm1_time = sample.time;
>  			}
>  			if (strcmp(event->comm.comm, comm2) == 0) {
> +				CHECK_NOT_NULL__(evsel = evlist__event2evsel(evlist, event));
>  				CHECK__(evsel__parse_sample(evsel, event, &sample));
>  				comm2_time = sample.time;
>  			}

Re: [PATCH 2/2] perf tests: Fix Convert perf time to TSC test for hybrid

[Arnaldo Carvalho de Melo]

Em Wed, Jul 13, 2022 at 09:05:21AM -0400, Liang, Kan escreveu:
> 
> 
> On 2022-07-13 8:34 a.m., Adrian Hunter wrote:
> > The test does not always correctly determine the number of events for
> > hybrids, nor allow for more than 1 evsel when parsing.
> > 
> > Fix by iterating the events actually created and getting the correct
> > evsel for the events processed.
> > 
> 
> Yes, we cannot always assume there are two events for hybrid.
> 
> > Fixes: d9da6f70eb23 ("perf tests: Support 'Convert perf time to TSC' test for hybrid")
> > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> 
> 
> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks, applied.

- Arnaldo

 
> Thanks,
> Kan
> > ---
> >  tools/perf/tests/perf-time-to-tsc.c | 18 ++++--------------
> >  1 file changed, 4 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
> > index 8d6d60173693..7c7d20fc503a 100644
> > --- a/tools/perf/tests/perf-time-to-tsc.c
> > +++ b/tools/perf/tests/perf-time-to-tsc.c
> > @@ -20,8 +20,6 @@
> >  #include "tsc.h"
> >  #include "mmap.h"
> >  #include "tests.h"
> > -#include "pmu.h"
> > -#include "pmu-hybrid.h"
> >  
> >  /*
> >   * Except x86_64/i386 and Arm64, other archs don't support TSC in perf.  Just
> > @@ -106,18 +104,8 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >  
> >  	evlist__config(evlist, &opts, NULL);
> >  
> > -	evsel = evlist__first(evlist);
> > -
> > -	evsel->core.attr.comm = 1;
> > -	evsel->core.attr.disabled = 1;
> > -	evsel->core.attr.enable_on_exec = 0;
> > -
> > -	/*
> > -	 * For hybrid "cycles:u", it creates two events.
> > -	 * Init the second evsel here.
> > -	 */
> > -	if (perf_pmu__has_hybrid() && perf_pmu__hybrid_mounted("cpu_atom")) {
> > -		evsel = evsel__next(evsel);
> > +	/* For hybrid "cycles:u", it creates two events */
> > +	evlist__for_each_entry(evlist, evsel) {
> >  		evsel->core.attr.comm = 1;
> >  		evsel->core.attr.disabled = 1;
> >  		evsel->core.attr.enable_on_exec = 0;
> > @@ -170,10 +158,12 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >  				goto next_event;
> >  
> >  			if (strcmp(event->comm.comm, comm1) == 0) {
> > +				CHECK_NOT_NULL__(evsel = evlist__event2evsel(evlist, event));
> >  				CHECK__(evsel__parse_sample(evsel, event, &sample));
> >  				comm1_time = sample.time;
> >  			}
> >  			if (strcmp(event->comm.comm, comm2) == 0) {
> > +				CHECK_NOT_NULL__(evsel = evlist__event2evsel(evlist, event));
> >  				CHECK__(evsel__parse_sample(evsel, event, &sample));
> >  				comm2_time = sample.time;
> >  			}

-- 

- Arnaldo