linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] perf test tsc: Fix error message report when not supported.
@ 2022-04-06 10:06 Chengdong Li
  2022-04-07  6:59 ` Adrian Hunter
  0 siblings, 1 reply; 10+ messages in thread
From: Chengdong Li @ 2022-04-06 10:06 UTC (permalink / raw)
  To: adrian.hunter, linux-kernel, linux-perf-users, peterz, mingo,
	acme, mark.rutland, alexander.shishkin, jolsa, namhyung
  Cc: ak, likexu, chengdongli

By default `perf test tsc` does not return the error message
when child process detected kernel does not support. Instead, child
process print error message to stderr, unfortunately the stderr is
redirected to /dev/null when verbose <= 0.

This patch did three things:
- returns TEST_SKIP to parent process instead of TEST_OK when
  perf_read_tsc_conversion() is not supported.
- add a new subtest of testing if TSC is supported on current
  architecture by moving exist code to a separate function.
- extended test suite definition to contain above two subtests.

Changes since v1 (thanks for the feedback from Adrian Hunter):
- rebase commit to current source.

Signed-off-by: Chengdong Li <chengdongli@tencent.com>
---
 tools/perf/tests/perf-time-to-tsc.c | 36 +++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
index d12d0ad81801..fc7c380af5a0 100644
--- a/tools/perf/tests/perf-time-to-tsc.c
+++ b/tools/perf/tests/perf-time-to-tsc.c
@@ -47,6 +47,17 @@
 	}					\
 }
 
+static int test__tsc_is_supported(struct test_suite *test __maybe_unused,
+				  int subtest __maybe_unused)
+{
+	if (!TSC_IS_SUPPORTED) {
+		pr_debug("Test not supported on this architecture");
+		return TEST_SKIP;
+	}
+
+	return TEST_OK;
+}
+
 /**
  * test__perf_time_to_tsc - test converting perf time to TSC.
  *
@@ -70,7 +81,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	struct perf_cpu_map *cpus = NULL;
 	struct evlist *evlist = NULL;
 	struct evsel *evsel = NULL;
-	int err = -1, ret, i;
+	int err = TEST_FAIL, ret, i;
 	const char *comm1, *comm2;
 	struct perf_tsc_conversion tc;
 	struct perf_event_mmap_page *pc;
@@ -79,10 +90,6 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	u64 test_time, comm1_time = 0, comm2_time = 0;
 	struct mmap *md;
 
-	if (!TSC_IS_SUPPORTED) {
-		pr_debug("Test not supported on this architecture");
-		return TEST_SKIP;
-	}
 
 	threads = thread_map__new(-1, getpid(), UINT_MAX);
 	CHECK_NOT_NULL__(threads);
@@ -124,8 +131,8 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	ret = perf_read_tsc_conversion(pc, &tc);
 	if (ret) {
 		if (ret == -EOPNOTSUPP) {
-			fprintf(stderr, " (not supported)");
-			return 0;
+			pr_debug("perf_read_tsc_conversion is not supported in current kernel");
+			err = TEST_SKIP;
 		}
 		goto out_err;
 	}
@@ -191,7 +198,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	    test_tsc >= comm2_tsc)
 		goto out_err;
 
-	err = 0;
+	err = TEST_OK;
 
 out_err:
 	evlist__delete(evlist);
@@ -200,4 +207,15 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	return err;
 }
 
-DEFINE_SUITE("Convert perf time to TSC", perf_time_to_tsc);
+static struct test_case time_to_tsc_tests[] = {
+	TEST_CASE_REASON("TSC support", tsc_is_supported,
+			 "This architecture does not support"),
+	TEST_CASE_REASON("Perf time to TSC", perf_time_to_tsc,
+			 "perf_read_tsc_conversion is not supported"),
+	{ .name = NULL, }
+};
+
+struct test_suite suite__perf_time_to_tsc = {
+	.desc = "Convert perf time to TSC",
+	.test_cases = time_to_tsc_tests,
+};
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] perf test tsc: Fix error message report when not supported.
  2022-04-06 10:06 [PATCH v2] perf test tsc: Fix error message report when not supported Chengdong Li
@ 2022-04-07  6:59 ` Adrian Hunter
  2022-04-07 11:34   ` Bryton Lee
  0 siblings, 1 reply; 10+ messages in thread
From: Adrian Hunter @ 2022-04-07  6:59 UTC (permalink / raw)
  To: Chengdong Li, adrian.hunter, linux-kernel, linux-perf-users,
	peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa,
	namhyung
  Cc: ak, likexu, chengdongli

On 06/04/2022 13.06, Chengdong Li wrote:
> By default `perf test tsc` does not return the error message
> when child process detected kernel does not support. Instead, child
> process print error message to stderr, unfortunately the stderr is
> redirected to /dev/null when verbose <= 0.
> 
> This patch did three things:
> - returns TEST_SKIP to parent process instead of TEST_OK when
>   perf_read_tsc_conversion() is not supported.
> - add a new subtest of testing if TSC is supported on current
>   architecture by moving exist code to a separate function.
> - extended test suite definition to contain above two subtests.
> 
> Changes since v1 (thanks for the feedback from Adrian Hunter):
> - rebase commit to current source.
> 
> Signed-off-by: Chengdong Li <chengdongli@tencent.com>
> ---
>  tools/perf/tests/perf-time-to-tsc.c | 36 +++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
> index d12d0ad81801..fc7c380af5a0 100644
> --- a/tools/perf/tests/perf-time-to-tsc.c
> +++ b/tools/perf/tests/perf-time-to-tsc.c
> @@ -47,6 +47,17 @@
>  	}					\
>  }
>  
> +static int test__tsc_is_supported(struct test_suite *test __maybe_unused,
> +				  int subtest __maybe_unused)
> +{
> +	if (!TSC_IS_SUPPORTED) {
> +		pr_debug("Test not supported on this architecture");

Message better ending with "\n" I think

> +		return TEST_SKIP;
> +	}
> +
> +	return TEST_OK;
> +}
> +
>  /**
>   * test__perf_time_to_tsc - test converting perf time to TSC.
>   *
> @@ -70,7 +81,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	struct perf_cpu_map *cpus = NULL;
>  	struct evlist *evlist = NULL;
>  	struct evsel *evsel = NULL;
> -	int err = -1, ret, i;
> +	int err = TEST_FAIL, ret, i;
>  	const char *comm1, *comm2;
>  	struct perf_tsc_conversion tc;
>  	struct perf_event_mmap_page *pc;
> @@ -79,10 +90,6 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	u64 test_time, comm1_time = 0, comm2_time = 0;
>  	struct mmap *md;
>  
> -	if (!TSC_IS_SUPPORTED) {
> -		pr_debug("Test not supported on this architecture");
> -		return TEST_SKIP;
> -	}
>  
>  	threads = thread_map__new(-1, getpid(), UINT_MAX);
>  	CHECK_NOT_NULL__(threads);
> @@ -124,8 +131,8 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	ret = perf_read_tsc_conversion(pc, &tc);
>  	if (ret) {
>  		if (ret == -EOPNOTSUPP) {
> -			fprintf(stderr, " (not supported)");
> -			return 0;
> +			pr_debug("perf_read_tsc_conversion is not supported in current kernel");
> +			err = TEST_SKIP;
>  		}
>  		goto out_err;
>  	}
> @@ -191,7 +198,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	    test_tsc >= comm2_tsc)
>  		goto out_err;
>  
> -	err = 0;
> +	err = TEST_OK;
>  
>  out_err:
>  	evlist__delete(evlist);
> @@ -200,4 +207,15 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	return err;
>  }
>  
> -DEFINE_SUITE("Convert perf time to TSC", perf_time_to_tsc);
> +static struct test_case time_to_tsc_tests[] = {
> +	TEST_CASE_REASON("TSC support", tsc_is_supported,
> +			 "This architecture does not support"),

The 2nd test case runs anyway, so I am not sure another
test case is needed?

> +	TEST_CASE_REASON("Perf time to TSC", perf_time_to_tsc,
> +			 "perf_read_tsc_conversion is not supported"),
> +	{ .name = NULL, }
> +};
> +
> +struct test_suite suite__perf_time_to_tsc = {
> +	.desc = "Convert perf time to TSC",
> +	.test_cases = time_to_tsc_tests,
> +};


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] perf test tsc: Fix error message report when not supported.
  2022-04-07  6:59 ` Adrian Hunter
@ 2022-04-07 11:34   ` Bryton Lee
  2022-04-07 12:34     ` Adrian Hunter
  0 siblings, 1 reply; 10+ messages in thread
From: Bryton Lee @ 2022-04-07 11:34 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: linux-kernel, linux-perf-users, peterz, mingo,
	Arnaldo Carvalho de Melo, mark.rutland, alexander.shishkin,
	Jiri Olsa, Namhyung Kim, ak, likexu, chengdongli

On Thu, Apr 7, 2022 at 2:59 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 06/04/2022 13.06, Chengdong Li wrote:
> > By default `perf test tsc` does not return the error message
> > when child process detected kernel does not support. Instead, child
> > process print error message to stderr, unfortunately the stderr is
> > redirected to /dev/null when verbose <= 0.
> >
> > This patch did three things:
> > - returns TEST_SKIP to parent process instead of TEST_OK when
> >   perf_read_tsc_conversion() is not supported.
> > - add a new subtest of testing if TSC is supported on current
> >   architecture by moving exist code to a separate function.
> > - extended test suite definition to contain above two subtests.
> >
> > Changes since v1 (thanks for the feedback from Adrian Hunter):
> > - rebase commit to current source.
> >
> > Signed-off-by: Chengdong Li <chengdongli@tencent.com>
> > ---
> >  tools/perf/tests/perf-time-to-tsc.c | 36 +++++++++++++++++++++--------
> >  1 file changed, 27 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
> > index d12d0ad81801..fc7c380af5a0 100644
> > --- a/tools/perf/tests/perf-time-to-tsc.c
> > +++ b/tools/perf/tests/perf-time-to-tsc.c
> > @@ -47,6 +47,17 @@
> >       }                                       \
> >  }
> >
> > +static int test__tsc_is_supported(struct test_suite *test __maybe_unused,
> > +                               int subtest __maybe_unused)
> > +{
> > +     if (!TSC_IS_SUPPORTED) {
> > +             pr_debug("Test not supported on this architecture");
>
> Message better ending with "\n" I think
OK, I will fix it.
>
> > +             return TEST_SKIP;
> > +     }
> > +
> > +     return TEST_OK;
> > +}
> > +
> >  /**
> >   * test__perf_time_to_tsc - test converting perf time to TSC.
> >   *
> > @@ -70,7 +81,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >       struct perf_cpu_map *cpus = NULL;
> >       struct evlist *evlist = NULL;
> >       struct evsel *evsel = NULL;
> > -     int err = -1, ret, i;
> > +     int err = TEST_FAIL, ret, i;
> >       const char *comm1, *comm2;
> >       struct perf_tsc_conversion tc;
> >       struct perf_event_mmap_page *pc;
> > @@ -79,10 +90,6 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >       u64 test_time, comm1_time = 0, comm2_time = 0;
> >       struct mmap *md;
> >
> > -     if (!TSC_IS_SUPPORTED) {
> > -             pr_debug("Test not supported on this architecture");
> > -             return TEST_SKIP;
> > -     }
> >
> >       threads = thread_map__new(-1, getpid(), UINT_MAX);
> >       CHECK_NOT_NULL__(threads);
> > @@ -124,8 +131,8 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >       ret = perf_read_tsc_conversion(pc, &tc);
> >       if (ret) {
> >               if (ret == -EOPNOTSUPP) {
> > -                     fprintf(stderr, " (not supported)");
> > -                     return 0;
> > +                     pr_debug("perf_read_tsc_conversion is not supported in current kernel");
> > +                     err = TEST_SKIP;
> >               }
> >               goto out_err;
> >       }
> > @@ -191,7 +198,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >           test_tsc >= comm2_tsc)
> >               goto out_err;
> >
> > -     err = 0;
> > +     err = TEST_OK;
> >
> >  out_err:
> >       evlist__delete(evlist);
> > @@ -200,4 +207,15 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >       return err;
> >  }
> >
> > -DEFINE_SUITE("Convert perf time to TSC", perf_time_to_tsc);
> > +static struct test_case time_to_tsc_tests[] = {
> > +     TEST_CASE_REASON("TSC support", tsc_is_supported,
> > +                      "This architecture does not support"),
>
> The 2nd test case runs anyway, so I am not sure another
> test case is needed?

Yes. But, I think there are two reasons to add two subtests: 1).
Original TSC support test is embedded in test__perf_time_to_tsc(), and
returns TEST_SKIP
if current architecture doesn't support it.  Thus there are two places
that return TEST_SKIP in test__perf_time_to_tsc() if we don't move TSC
support
to another subtest. 2). Current test_suite and test_case structs don't
support printing skip_reason if there is no subtest. To print skip
reason we need more than 1 subtests.

>
> > +     TEST_CASE_REASON("Perf time to TSC", perf_time_to_tsc,
> > +                      "perf_read_tsc_conversion is not supported"),
> > +     { .name = NULL, }
> > +};
> > +
> > +struct test_suite suite__perf_time_to_tsc = {
> > +     .desc = "Convert perf time to TSC",
> > +     .test_cases = time_to_tsc_tests,
> > +};
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] perf test tsc: Fix error message report when not supported.
  2022-04-07 11:34   ` Bryton Lee
@ 2022-04-07 12:34     ` Adrian Hunter
  2022-04-08  2:14       ` [PATCH v3] " Chengdong Li
  0 siblings, 1 reply; 10+ messages in thread
From: Adrian Hunter @ 2022-04-07 12:34 UTC (permalink / raw)
  To: Bryton Lee
  Cc: linux-kernel, linux-perf-users, peterz, mingo,
	Arnaldo Carvalho de Melo, mark.rutland, alexander.shishkin,
	Jiri Olsa, Namhyung Kim, ak, likexu, chengdongli

On 07/04/2022 14.34, Bryton Lee wrote:
> On Thu, Apr 7, 2022 at 2:59 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 06/04/2022 13.06, Chengdong Li wrote:
>>> By default `perf test tsc` does not return the error message
>>> when child process detected kernel does not support. Instead, child
>>> process print error message to stderr, unfortunately the stderr is
>>> redirected to /dev/null when verbose <= 0.
>>>
>>> This patch did three things:
>>> - returns TEST_SKIP to parent process instead of TEST_OK when
>>>   perf_read_tsc_conversion() is not supported.
>>> - add a new subtest of testing if TSC is supported on current
>>>   architecture by moving exist code to a separate function.
>>> - extended test suite definition to contain above two subtests.
>>>
>>> Changes since v1 (thanks for the feedback from Adrian Hunter):
>>> - rebase commit to current source.
>>>
>>> Signed-off-by: Chengdong Li <chengdongli@tencent.com>
>>> ---
>>>  tools/perf/tests/perf-time-to-tsc.c | 36 +++++++++++++++++++++--------
>>>  1 file changed, 27 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
>>> index d12d0ad81801..fc7c380af5a0 100644
>>> --- a/tools/perf/tests/perf-time-to-tsc.c
>>> +++ b/tools/perf/tests/perf-time-to-tsc.c
>>> @@ -47,6 +47,17 @@
>>>       }                                       \
>>>  }
>>>
>>> +static int test__tsc_is_supported(struct test_suite *test __maybe_unused,
>>> +                               int subtest __maybe_unused)
>>> +{
>>> +     if (!TSC_IS_SUPPORTED) {
>>> +             pr_debug("Test not supported on this architecture");
>>
>> Message better ending with "\n" I think
> OK, I will fix it.
>>
>>> +             return TEST_SKIP;
>>> +     }
>>> +
>>> +     return TEST_OK;
>>> +}
>>> +
>>>  /**
>>>   * test__perf_time_to_tsc - test converting perf time to TSC.
>>>   *
>>> @@ -70,7 +81,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>>>       struct perf_cpu_map *cpus = NULL;
>>>       struct evlist *evlist = NULL;
>>>       struct evsel *evsel = NULL;
>>> -     int err = -1, ret, i;
>>> +     int err = TEST_FAIL, ret, i;
>>>       const char *comm1, *comm2;
>>>       struct perf_tsc_conversion tc;
>>>       struct perf_event_mmap_page *pc;
>>> @@ -79,10 +90,6 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>>>       u64 test_time, comm1_time = 0, comm2_time = 0;
>>>       struct mmap *md;
>>>
>>> -     if (!TSC_IS_SUPPORTED) {
>>> -             pr_debug("Test not supported on this architecture");
>>> -             return TEST_SKIP;
>>> -     }
>>>
>>>       threads = thread_map__new(-1, getpid(), UINT_MAX);
>>>       CHECK_NOT_NULL__(threads);
>>> @@ -124,8 +131,8 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>>>       ret = perf_read_tsc_conversion(pc, &tc);
>>>       if (ret) {
>>>               if (ret == -EOPNOTSUPP) {
>>> -                     fprintf(stderr, " (not supported)");
>>> -                     return 0;
>>> +                     pr_debug("perf_read_tsc_conversion is not supported in current kernel");
>>> +                     err = TEST_SKIP;
>>>               }
>>>               goto out_err;
>>>       }
>>> @@ -191,7 +198,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>>>           test_tsc >= comm2_tsc)
>>>               goto out_err;
>>>
>>> -     err = 0;
>>> +     err = TEST_OK;
>>>
>>>  out_err:
>>>       evlist__delete(evlist);
>>> @@ -200,4 +207,15 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>>>       return err;
>>>  }
>>>
>>> -DEFINE_SUITE("Convert perf time to TSC", perf_time_to_tsc);
>>> +static struct test_case time_to_tsc_tests[] = {
>>> +     TEST_CASE_REASON("TSC support", tsc_is_supported,
>>> +                      "This architecture does not support"),
>>
>> The 2nd test case runs anyway, so I am not sure another
>> test case is needed?
> 
> Yes. But, I think there are two reasons to add two subtests: 1).
> Original TSC support test is embedded in test__perf_time_to_tsc(), and
> returns TEST_SKIP
> if current architecture doesn't support it.  Thus there are two places
> that return TEST_SKIP in test__perf_time_to_tsc() if we don't move TSC
> support
> to another subtest. 2). Current test_suite and test_case structs don't
> support printing skip_reason if there is no subtest. To print skip
> reason we need more than 1 subtests.

Ok, can that explanation be added to the commit message

> 
>>
>>> +     TEST_CASE_REASON("Perf time to TSC", perf_time_to_tsc,
>>> +                      "perf_read_tsc_conversion is not supported"),
>>> +     { .name = NULL, }
>>> +};
>>> +
>>> +struct test_suite suite__perf_time_to_tsc = {
>>> +     .desc = "Convert perf time to TSC",
>>> +     .test_cases = time_to_tsc_tests,
>>> +};
>>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3] perf test tsc: Fix error message report when not supported.
  2022-04-07 12:34     ` Adrian Hunter
@ 2022-04-08  2:14       ` Chengdong Li
  2022-04-08  7:58         ` Adrian Hunter
  0 siblings, 1 reply; 10+ messages in thread
From: Chengdong Li @ 2022-04-08  2:14 UTC (permalink / raw)
  To: adrian.hunter, linux-kernel, linux-perf-users, peterz, mingo,
	acme, mark.rutland, alexander.shishkin, jolsa, namhyung
  Cc: ak, likexu, chengdongli

By default `perf test tsc` does not return the error message
when the child process detected kernel does not support. Instead,
the child process print error message to stderr, unfortunately
the stderr is redirected to /dev/null when verbose <= 0.

This patch did three things:
- returns TEST_SKIP to the parent process instead of TEST_OK when
  perf_read_tsc_conversion() is not supported.
- add a new subtest of testing if TSC is supported on current
  architecture by moving exist code to a separate function.
  It avoids two places in test__perf_time_to_tsc() that return
  TEST_SKIP by doing this.
- extended test suite definition to contain above two subtests.
  Current test_suite and test_case structs do not support printing
  skip reason when the number of subtest less than 1. To print skip
  reason, it is necessary to extend current test suite definition.

Changes since v2:
- refine error message format.
- refine commit log message according to Adrian's review comments.

Changes since v1 (thanks for the feedback from Adrian Hunter):
- rebase commit to current source.

Signed-off-by: Chengdong Li <chengdongli@tencent.com>
---
 tools/perf/tests/perf-time-to-tsc.c | 36 +++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
index d12d0ad81801..cc6df49a65a1 100644
--- a/tools/perf/tests/perf-time-to-tsc.c
+++ b/tools/perf/tests/perf-time-to-tsc.c
@@ -47,6 +47,17 @@
 	}					\
 }
 
+static int test__tsc_is_supported(struct test_suite *test __maybe_unused,
+				  int subtest __maybe_unused)
+{
+	if (!TSC_IS_SUPPORTED) {
+		pr_debug("Test not supported on this architecture\n");
+		return TEST_SKIP;
+	}
+
+	return TEST_OK;
+}
+
 /**
  * test__perf_time_to_tsc - test converting perf time to TSC.
  *
@@ -70,7 +81,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	struct perf_cpu_map *cpus = NULL;
 	struct evlist *evlist = NULL;
 	struct evsel *evsel = NULL;
-	int err = -1, ret, i;
+	int err = TEST_FAIL, ret, i;
 	const char *comm1, *comm2;
 	struct perf_tsc_conversion tc;
 	struct perf_event_mmap_page *pc;
@@ -79,10 +90,6 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	u64 test_time, comm1_time = 0, comm2_time = 0;
 	struct mmap *md;
 
-	if (!TSC_IS_SUPPORTED) {
-		pr_debug("Test not supported on this architecture");
-		return TEST_SKIP;
-	}
 
 	threads = thread_map__new(-1, getpid(), UINT_MAX);
 	CHECK_NOT_NULL__(threads);
@@ -124,8 +131,8 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	ret = perf_read_tsc_conversion(pc, &tc);
 	if (ret) {
 		if (ret == -EOPNOTSUPP) {
-			fprintf(stderr, " (not supported)");
-			return 0;
+			pr_debug("perf_read_tsc_conversion is not supported in current kernel\n");
+			err = TEST_SKIP;
 		}
 		goto out_err;
 	}
@@ -191,7 +198,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	    test_tsc >= comm2_tsc)
 		goto out_err;
 
-	err = 0;
+	err = TEST_OK;
 
 out_err:
 	evlist__delete(evlist);
@@ -200,4 +207,15 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	return err;
 }
 
-DEFINE_SUITE("Convert perf time to TSC", perf_time_to_tsc);
+static struct test_case time_to_tsc_tests[] = {
+	TEST_CASE_REASON("TSC support", tsc_is_supported,
+			 "This architecture does not support"),
+	TEST_CASE_REASON("Perf time to TSC", perf_time_to_tsc,
+			 "perf_read_tsc_conversion is not supported"),
+	{ .name = NULL, }
+};
+
+struct test_suite suite__perf_time_to_tsc = {
+	.desc = "Convert perf time to TSC",
+	.test_cases = time_to_tsc_tests,
+};
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] perf test tsc: Fix error message report when not supported.
  2022-04-08  2:14       ` [PATCH v3] " Chengdong Li
@ 2022-04-08  7:58         ` Adrian Hunter
  2022-04-08  8:41           ` Bryton Lee
  0 siblings, 1 reply; 10+ messages in thread
From: Adrian Hunter @ 2022-04-08  7:58 UTC (permalink / raw)
  To: Chengdong Li, linux-kernel, linux-perf-users, peterz, mingo,
	acme, mark.rutland, alexander.shishkin, jolsa, namhyung
  Cc: ak, likexu, chengdongli

On 08/04/2022 5.14, Chengdong Li wrote:
> By default `perf test tsc` does not return the error message
> when the child process detected kernel does not support. Instead,
> the child process print error message to stderr, unfortunately
> the stderr is redirected to /dev/null when verbose <= 0.
> 
> This patch did three things:
> - returns TEST_SKIP to the parent process instead of TEST_OK when
>   perf_read_tsc_conversion() is not supported.
> - add a new subtest of testing if TSC is supported on current
>   architecture by moving exist code to a separate function.
>   It avoids two places in test__perf_time_to_tsc() that return
>   TEST_SKIP by doing this.
> - extended test suite definition to contain above two subtests.
>   Current test_suite and test_case structs do not support printing
>   skip reason when the number of subtest less than 1. To print skip
>   reason, it is necessary to extend current test suite definition.
> 
> Changes since v2:
> - refine error message format.
> - refine commit log message according to Adrian's review comments.
> 
> Changes since v1 (thanks for the feedback from Adrian Hunter):
> - rebase commit to current source.
> 
> Signed-off-by: Chengdong Li <chengdongli@tencent.com>

It is better to try to consistently use the same email address
for the same person for git logs, unless you have a sensible
reason not to.

So in this case it looks like you should use the same email
address for the "From:" and "Signed-off-by:".  Note you can
optionally put the "From:" line before the commit message e.g.

From: Chengdong Li <chengdongli@tencent.com>

By default `perf test tsc` does not return the error message
...


Otherwise you can add:

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>


> ---
>  tools/perf/tests/perf-time-to-tsc.c | 36 +++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
> index d12d0ad81801..cc6df49a65a1 100644
> --- a/tools/perf/tests/perf-time-to-tsc.c
> +++ b/tools/perf/tests/perf-time-to-tsc.c
> @@ -47,6 +47,17 @@
>  	}					\
>  }
>  
> +static int test__tsc_is_supported(struct test_suite *test __maybe_unused,
> +				  int subtest __maybe_unused)
> +{
> +	if (!TSC_IS_SUPPORTED) {
> +		pr_debug("Test not supported on this architecture\n");
> +		return TEST_SKIP;
> +	}
> +
> +	return TEST_OK;
> +}
> +
>  /**
>   * test__perf_time_to_tsc - test converting perf time to TSC.
>   *
> @@ -70,7 +81,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	struct perf_cpu_map *cpus = NULL;
>  	struct evlist *evlist = NULL;
>  	struct evsel *evsel = NULL;
> -	int err = -1, ret, i;
> +	int err = TEST_FAIL, ret, i;
>  	const char *comm1, *comm2;
>  	struct perf_tsc_conversion tc;
>  	struct perf_event_mmap_page *pc;
> @@ -79,10 +90,6 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	u64 test_time, comm1_time = 0, comm2_time = 0;
>  	struct mmap *md;
>  
> -	if (!TSC_IS_SUPPORTED) {
> -		pr_debug("Test not supported on this architecture");
> -		return TEST_SKIP;
> -	}
>  
>  	threads = thread_map__new(-1, getpid(), UINT_MAX);
>  	CHECK_NOT_NULL__(threads);
> @@ -124,8 +131,8 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	ret = perf_read_tsc_conversion(pc, &tc);
>  	if (ret) {
>  		if (ret == -EOPNOTSUPP) {
> -			fprintf(stderr, " (not supported)");
> -			return 0;
> +			pr_debug("perf_read_tsc_conversion is not supported in current kernel\n");
> +			err = TEST_SKIP;
>  		}
>  		goto out_err;
>  	}
> @@ -191,7 +198,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	    test_tsc >= comm2_tsc)
>  		goto out_err;
>  
> -	err = 0;
> +	err = TEST_OK;
>  
>  out_err:
>  	evlist__delete(evlist);
> @@ -200,4 +207,15 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	return err;
>  }
>  
> -DEFINE_SUITE("Convert perf time to TSC", perf_time_to_tsc);
> +static struct test_case time_to_tsc_tests[] = {
> +	TEST_CASE_REASON("TSC support", tsc_is_supported,
> +			 "This architecture does not support"),
> +	TEST_CASE_REASON("Perf time to TSC", perf_time_to_tsc,
> +			 "perf_read_tsc_conversion is not supported"),
> +	{ .name = NULL, }
> +};
> +
> +struct test_suite suite__perf_time_to_tsc = {
> +	.desc = "Convert perf time to TSC",
> +	.test_cases = time_to_tsc_tests,
> +};


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] perf test tsc: Fix error message report when not supported.
  2022-04-08  7:58         ` Adrian Hunter
@ 2022-04-08  8:41           ` Bryton Lee
  2022-04-08  8:47             ` [PATCH v4] " Chengdong Li
  0 siblings, 1 reply; 10+ messages in thread
From: Bryton Lee @ 2022-04-08  8:41 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: linux-kernel, linux-perf-users, peterz, mingo,
	Arnaldo Carvalho de Melo, mark.rutland, alexander.shishkin,
	Jiri Olsa, Namhyung Kim, ak, likexu, chengdongli

On Fri, Apr 8, 2022 at 3:58 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 08/04/2022 5.14, Chengdong Li wrote:
> > By default `perf test tsc` does not return the error message
> > when the child process detected kernel does not support. Instead,
> > the child process print error message to stderr, unfortunately
> > the stderr is redirected to /dev/null when verbose <= 0.
> >
> > This patch did three things:
> > - returns TEST_SKIP to the parent process instead of TEST_OK when
> >   perf_read_tsc_conversion() is not supported.
> > - add a new subtest of testing if TSC is supported on current
> >   architecture by moving exist code to a separate function.
> >   It avoids two places in test__perf_time_to_tsc() that return
> >   TEST_SKIP by doing this.
> > - extended test suite definition to contain above two subtests.
> >   Current test_suite and test_case structs do not support printing
> >   skip reason when the number of subtest less than 1. To print skip
> >   reason, it is necessary to extend current test suite definition.
> >
> > Changes since v2:
> > - refine error message format.
> > - refine commit log message according to Adrian's review comments.
> >
> > Changes since v1 (thanks for the feedback from Adrian Hunter):
> > - rebase commit to current source.
> >
> > Signed-off-by: Chengdong Li <chengdongli@tencent.com>
>
> It is better to try to consistently use the same email address
> for the same person for git logs, unless you have a sensible
> reason not to.
>

Sorry for the inconvenience. I can not use my company email account to send
patches via `git send-email` due to lack of email protocol support in
our email system.

> So in this case it looks like you should use the same email
> address for the "From:" and "Signed-off-by:".  Note you can
> optionally put the "From:" line before the commit message e.g.
>
> From: Chengdong Li <chengdongli@tencent.com>
>
> By default `perf test tsc` does not return the error message
> ...

Thanks for your guidance, I will resend this patch again.

>
> Otherwise you can add:
>
> Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>
>
>
> > ---
> >  tools/perf/tests/perf-time-to-tsc.c | 36 +++++++++++++++++++++--------
> >  1 file changed, 27 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
> > index d12d0ad81801..cc6df49a65a1 100644
> > --- a/tools/perf/tests/perf-time-to-tsc.c
> > +++ b/tools/perf/tests/perf-time-to-tsc.c
> > @@ -47,6 +47,17 @@
> >       }                                       \
> >  }
> >
> > +static int test__tsc_is_supported(struct test_suite *test __maybe_unused,
> > +                               int subtest __maybe_unused)
> > +{
> > +     if (!TSC_IS_SUPPORTED) {
> > +             pr_debug("Test not supported on this architecture\n");
> > +             return TEST_SKIP;
> > +     }
> > +
> > +     return TEST_OK;
> > +}
> > +
> >  /**
> >   * test__perf_time_to_tsc - test converting perf time to TSC.
> >   *
> > @@ -70,7 +81,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >       struct perf_cpu_map *cpus = NULL;
> >       struct evlist *evlist = NULL;
> >       struct evsel *evsel = NULL;
> > -     int err = -1, ret, i;
> > +     int err = TEST_FAIL, ret, i;
> >       const char *comm1, *comm2;
> >       struct perf_tsc_conversion tc;
> >       struct perf_event_mmap_page *pc;
> > @@ -79,10 +90,6 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >       u64 test_time, comm1_time = 0, comm2_time = 0;
> >       struct mmap *md;
> >
> > -     if (!TSC_IS_SUPPORTED) {
> > -             pr_debug("Test not supported on this architecture");
> > -             return TEST_SKIP;
> > -     }
> >
> >       threads = thread_map__new(-1, getpid(), UINT_MAX);
> >       CHECK_NOT_NULL__(threads);
> > @@ -124,8 +131,8 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >       ret = perf_read_tsc_conversion(pc, &tc);
> >       if (ret) {
> >               if (ret == -EOPNOTSUPP) {
> > -                     fprintf(stderr, " (not supported)");
> > -                     return 0;
> > +                     pr_debug("perf_read_tsc_conversion is not supported in current kernel\n");
> > +                     err = TEST_SKIP;
> >               }
> >               goto out_err;
> >       }
> > @@ -191,7 +198,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >           test_tsc >= comm2_tsc)
> >               goto out_err;
> >
> > -     err = 0;
> > +     err = TEST_OK;
> >
> >  out_err:
> >       evlist__delete(evlist);
> > @@ -200,4 +207,15 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >       return err;
> >  }
> >
> > -DEFINE_SUITE("Convert perf time to TSC", perf_time_to_tsc);
> > +static struct test_case time_to_tsc_tests[] = {
> > +     TEST_CASE_REASON("TSC support", tsc_is_supported,
> > +                      "This architecture does not support"),
> > +     TEST_CASE_REASON("Perf time to TSC", perf_time_to_tsc,
> > +                      "perf_read_tsc_conversion is not supported"),
> > +     { .name = NULL, }
> > +};
> > +
> > +struct test_suite suite__perf_time_to_tsc = {
> > +     .desc = "Convert perf time to TSC",
> > +     .test_cases = time_to_tsc_tests,
> > +};
>


-- 
Best Regards

Bryton.Lee

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v4] perf test tsc: Fix error message report when not supported.
  2022-04-08  8:41           ` Bryton Lee
@ 2022-04-08  8:47             ` Chengdong Li
  2022-04-08  9:16               ` Adrian Hunter
  0 siblings, 1 reply; 10+ messages in thread
From: Chengdong Li @ 2022-04-08  8:47 UTC (permalink / raw)
  To: adrian.hunter, linux-kernel, linux-perf-users, peterz, mingo,
	acme, mark.rutland, alexander.shishkin, jolsa, namhyung
  Cc: ak, likexu, chengdongli

From: Chengdong Li <chengdongli@tencent.com>

By default `perf test tsc` does not return the error message
when the child process detected kernel does not support. Instead,
the child process print error message to stderr, unfortunately
the stderr is redirected to /dev/null when verbose <= 0.

This patch did three things:
- returns TEST_SKIP to the parent process instead of TEST_OK when
  perf_read_tsc_conversion() is not supported.
- add a new subtest of testing if TSC is supported on current
  architecture by moving exist code to a separate function.
  It avoids two places in test__perf_time_to_tsc() that return
  TEST_SKIP by doing this.
- extended test suite definition to contain above two subtests.
  Current test_suite and test_case structs do not support printing
  skip reason when the number of subtest less than 1. To print skip
  reason, it is necessary to extend current test suite definition.

Changes since v3:
- refine commit message again to make it clear. 

Changes since v2:
- refine error message format.
- refine commit log message according to Adrian's review comments.

Changes since v1 (thanks for the feedback from Adrian Hunter):
- rebase commit to current source.

Signed-off-by: Chengdong Li <chengdongli@tencent.com>
---
 tools/perf/tests/perf-time-to-tsc.c | 36 +++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
index d12d0ad81801..cc6df49a65a1 100644
--- a/tools/perf/tests/perf-time-to-tsc.c
+++ b/tools/perf/tests/perf-time-to-tsc.c
@@ -47,6 +47,17 @@
 	}					\
 }
 
+static int test__tsc_is_supported(struct test_suite *test __maybe_unused,
+				  int subtest __maybe_unused)
+{
+	if (!TSC_IS_SUPPORTED) {
+		pr_debug("Test not supported on this architecture\n");
+		return TEST_SKIP;
+	}
+
+	return TEST_OK;
+}
+
 /**
  * test__perf_time_to_tsc - test converting perf time to TSC.
  *
@@ -70,7 +81,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	struct perf_cpu_map *cpus = NULL;
 	struct evlist *evlist = NULL;
 	struct evsel *evsel = NULL;
-	int err = -1, ret, i;
+	int err = TEST_FAIL, ret, i;
 	const char *comm1, *comm2;
 	struct perf_tsc_conversion tc;
 	struct perf_event_mmap_page *pc;
@@ -79,10 +90,6 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	u64 test_time, comm1_time = 0, comm2_time = 0;
 	struct mmap *md;
 
-	if (!TSC_IS_SUPPORTED) {
-		pr_debug("Test not supported on this architecture");
-		return TEST_SKIP;
-	}
 
 	threads = thread_map__new(-1, getpid(), UINT_MAX);
 	CHECK_NOT_NULL__(threads);
@@ -124,8 +131,8 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	ret = perf_read_tsc_conversion(pc, &tc);
 	if (ret) {
 		if (ret == -EOPNOTSUPP) {
-			fprintf(stderr, " (not supported)");
-			return 0;
+			pr_debug("perf_read_tsc_conversion is not supported in current kernel\n");
+			err = TEST_SKIP;
 		}
 		goto out_err;
 	}
@@ -191,7 +198,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	    test_tsc >= comm2_tsc)
 		goto out_err;
 
-	err = 0;
+	err = TEST_OK;
 
 out_err:
 	evlist__delete(evlist);
@@ -200,4 +207,15 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
 	return err;
 }
 
-DEFINE_SUITE("Convert perf time to TSC", perf_time_to_tsc);
+static struct test_case time_to_tsc_tests[] = {
+	TEST_CASE_REASON("TSC support", tsc_is_supported,
+			 "This architecture does not support"),
+	TEST_CASE_REASON("Perf time to TSC", perf_time_to_tsc,
+			 "perf_read_tsc_conversion is not supported"),
+	{ .name = NULL, }
+};
+
+struct test_suite suite__perf_time_to_tsc = {
+	.desc = "Convert perf time to TSC",
+	.test_cases = time_to_tsc_tests,
+};
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] perf test tsc: Fix error message report when not supported.
  2022-04-08  8:47             ` [PATCH v4] " Chengdong Li
@ 2022-04-08  9:16               ` Adrian Hunter
  2022-04-09 14:43                 ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 10+ messages in thread
From: Adrian Hunter @ 2022-04-08  9:16 UTC (permalink / raw)
  To: Chengdong Li, linux-kernel, linux-perf-users, peterz, mingo,
	acme, mark.rutland, alexander.shishkin, jolsa, namhyung
  Cc: ak, likexu, chengdongli

On 08/04/2022 11.47, Chengdong Li wrote:
> From: Chengdong Li <chengdongli@tencent.com>
> 
> By default `perf test tsc` does not return the error message
> when the child process detected kernel does not support. Instead,
> the child process print error message to stderr, unfortunately
> the stderr is redirected to /dev/null when verbose <= 0.
> 
> This patch did three things:
> - returns TEST_SKIP to the parent process instead of TEST_OK when
>   perf_read_tsc_conversion() is not supported.
> - add a new subtest of testing if TSC is supported on current
>   architecture by moving exist code to a separate function.
>   It avoids two places in test__perf_time_to_tsc() that return
>   TEST_SKIP by doing this.
> - extended test suite definition to contain above two subtests.
>   Current test_suite and test_case structs do not support printing
>   skip reason when the number of subtest less than 1. To print skip
>   reason, it is necessary to extend current test suite definition.
> 
> Changes since v3:
> - refine commit message again to make it clear. 
> 
> Changes since v2:
> - refine error message format.
> - refine commit log message according to Adrian's review comments.
> 
> Changes since v1 (thanks for the feedback from Adrian Hunter):
> - rebase commit to current source.
> 
> Signed-off-by: Chengdong Li <chengdongli@tencent.com>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  tools/perf/tests/perf-time-to-tsc.c | 36 +++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
> index d12d0ad81801..cc6df49a65a1 100644
> --- a/tools/perf/tests/perf-time-to-tsc.c
> +++ b/tools/perf/tests/perf-time-to-tsc.c
> @@ -47,6 +47,17 @@
>  	}					\
>  }
>  
> +static int test__tsc_is_supported(struct test_suite *test __maybe_unused,
> +				  int subtest __maybe_unused)
> +{
> +	if (!TSC_IS_SUPPORTED) {
> +		pr_debug("Test not supported on this architecture\n");
> +		return TEST_SKIP;
> +	}
> +
> +	return TEST_OK;
> +}
> +
>  /**
>   * test__perf_time_to_tsc - test converting perf time to TSC.
>   *
> @@ -70,7 +81,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	struct perf_cpu_map *cpus = NULL;
>  	struct evlist *evlist = NULL;
>  	struct evsel *evsel = NULL;
> -	int err = -1, ret, i;
> +	int err = TEST_FAIL, ret, i;
>  	const char *comm1, *comm2;
>  	struct perf_tsc_conversion tc;
>  	struct perf_event_mmap_page *pc;
> @@ -79,10 +90,6 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	u64 test_time, comm1_time = 0, comm2_time = 0;
>  	struct mmap *md;
>  
> -	if (!TSC_IS_SUPPORTED) {
> -		pr_debug("Test not supported on this architecture");
> -		return TEST_SKIP;
> -	}
>  
>  	threads = thread_map__new(-1, getpid(), UINT_MAX);
>  	CHECK_NOT_NULL__(threads);
> @@ -124,8 +131,8 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	ret = perf_read_tsc_conversion(pc, &tc);
>  	if (ret) {
>  		if (ret == -EOPNOTSUPP) {
> -			fprintf(stderr, " (not supported)");
> -			return 0;
> +			pr_debug("perf_read_tsc_conversion is not supported in current kernel\n");
> +			err = TEST_SKIP;
>  		}
>  		goto out_err;
>  	}
> @@ -191,7 +198,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	    test_tsc >= comm2_tsc)
>  		goto out_err;
>  
> -	err = 0;
> +	err = TEST_OK;
>  
>  out_err:
>  	evlist__delete(evlist);
> @@ -200,4 +207,15 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
>  	return err;
>  }
>  
> -DEFINE_SUITE("Convert perf time to TSC", perf_time_to_tsc);
> +static struct test_case time_to_tsc_tests[] = {
> +	TEST_CASE_REASON("TSC support", tsc_is_supported,
> +			 "This architecture does not support"),
> +	TEST_CASE_REASON("Perf time to TSC", perf_time_to_tsc,
> +			 "perf_read_tsc_conversion is not supported"),
> +	{ .name = NULL, }
> +};
> +
> +struct test_suite suite__perf_time_to_tsc = {
> +	.desc = "Convert perf time to TSC",
> +	.test_cases = time_to_tsc_tests,
> +};


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] perf test tsc: Fix error message report when not supported.
  2022-04-08  9:16               ` Adrian Hunter
@ 2022-04-09 14:43                 ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-04-09 14:43 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Chengdong Li, linux-kernel, linux-perf-users, peterz, mingo,
	mark.rutland, alexander.shishkin, jolsa, namhyung, ak, likexu,
	chengdongli

Em Fri, Apr 08, 2022 at 12:16:36PM +0300, Adrian Hunter escreveu:
> On 08/04/2022 11.47, Chengdong Li wrote:
> > From: Chengdong Li <chengdongli@tencent.com>
> > 
> > By default `perf test tsc` does not return the error message
> > when the child process detected kernel does not support. Instead,
> > the child process print error message to stderr, unfortunately
> > the stderr is redirected to /dev/null when verbose <= 0.
> > 
> > This patch did three things:
> > - returns TEST_SKIP to the parent process instead of TEST_OK when
> >   perf_read_tsc_conversion() is not supported.
> > - add a new subtest of testing if TSC is supported on current
> >   architecture by moving exist code to a separate function.
> >   It avoids two places in test__perf_time_to_tsc() that return
> >   TEST_SKIP by doing this.
> > - extended test suite definition to contain above two subtests.
> >   Current test_suite and test_case structs do not support printing
> >   skip reason when the number of subtest less than 1. To print skip
> >   reason, it is necessary to extend current test suite definition.
> > 
> > Changes since v3:
> > - refine commit message again to make it clear. 
> > 
> > Changes since v2:
> > - refine error message format.
> > - refine commit log message according to Adrian's review comments.
> > 
> > Changes since v1 (thanks for the feedback from Adrian Hunter):
> > - rebase commit to current source.
> > 
> > Signed-off-by: Chengdong Li <chengdongli@tencent.com>
> 
> Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

Thanks, applied.

- Arnaldo

 
> > ---
> >  tools/perf/tests/perf-time-to-tsc.c | 36 +++++++++++++++++++++--------
> >  1 file changed, 27 insertions(+), 9 deletions(-)
> > 
> > diff --git a/tools/perf/tests/perf-time-to-tsc.c b/tools/perf/tests/perf-time-to-tsc.c
> > index d12d0ad81801..cc6df49a65a1 100644
> > --- a/tools/perf/tests/perf-time-to-tsc.c
> > +++ b/tools/perf/tests/perf-time-to-tsc.c
> > @@ -47,6 +47,17 @@
> >  	}					\
> >  }
> >  
> > +static int test__tsc_is_supported(struct test_suite *test __maybe_unused,
> > +				  int subtest __maybe_unused)
> > +{
> > +	if (!TSC_IS_SUPPORTED) {
> > +		pr_debug("Test not supported on this architecture\n");
> > +		return TEST_SKIP;
> > +	}
> > +
> > +	return TEST_OK;
> > +}
> > +
> >  /**
> >   * test__perf_time_to_tsc - test converting perf time to TSC.
> >   *
> > @@ -70,7 +81,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >  	struct perf_cpu_map *cpus = NULL;
> >  	struct evlist *evlist = NULL;
> >  	struct evsel *evsel = NULL;
> > -	int err = -1, ret, i;
> > +	int err = TEST_FAIL, ret, i;
> >  	const char *comm1, *comm2;
> >  	struct perf_tsc_conversion tc;
> >  	struct perf_event_mmap_page *pc;
> > @@ -79,10 +90,6 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >  	u64 test_time, comm1_time = 0, comm2_time = 0;
> >  	struct mmap *md;
> >  
> > -	if (!TSC_IS_SUPPORTED) {
> > -		pr_debug("Test not supported on this architecture");
> > -		return TEST_SKIP;
> > -	}
> >  
> >  	threads = thread_map__new(-1, getpid(), UINT_MAX);
> >  	CHECK_NOT_NULL__(threads);
> > @@ -124,8 +131,8 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >  	ret = perf_read_tsc_conversion(pc, &tc);
> >  	if (ret) {
> >  		if (ret == -EOPNOTSUPP) {
> > -			fprintf(stderr, " (not supported)");
> > -			return 0;
> > +			pr_debug("perf_read_tsc_conversion is not supported in current kernel\n");
> > +			err = TEST_SKIP;
> >  		}
> >  		goto out_err;
> >  	}
> > @@ -191,7 +198,7 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >  	    test_tsc >= comm2_tsc)
> >  		goto out_err;
> >  
> > -	err = 0;
> > +	err = TEST_OK;
> >  
> >  out_err:
> >  	evlist__delete(evlist);
> > @@ -200,4 +207,15 @@ static int test__perf_time_to_tsc(struct test_suite *test __maybe_unused, int su
> >  	return err;
> >  }
> >  
> > -DEFINE_SUITE("Convert perf time to TSC", perf_time_to_tsc);
> > +static struct test_case time_to_tsc_tests[] = {
> > +	TEST_CASE_REASON("TSC support", tsc_is_supported,
> > +			 "This architecture does not support"),
> > +	TEST_CASE_REASON("Perf time to TSC", perf_time_to_tsc,
> > +			 "perf_read_tsc_conversion is not supported"),
> > +	{ .name = NULL, }
> > +};
> > +
> > +struct test_suite suite__perf_time_to_tsc = {
> > +	.desc = "Convert perf time to TSC",
> > +	.test_cases = time_to_tsc_tests,
> > +};

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-04-09 14:43 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-06 10:06 [PATCH v2] perf test tsc: Fix error message report when not supported Chengdong Li
2022-04-07  6:59 ` Adrian Hunter
2022-04-07 11:34   ` Bryton Lee
2022-04-07 12:34     ` Adrian Hunter
2022-04-08  2:14       ` [PATCH v3] " Chengdong Li
2022-04-08  7:58         ` Adrian Hunter
2022-04-08  8:41           ` Bryton Lee
2022-04-08  8:47             ` [PATCH v4] " Chengdong Li
2022-04-08  9:16               ` Adrian Hunter
2022-04-09 14:43                 ` Arnaldo Carvalho de Melo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).