Re: [PATCH v3] perf test tsc: Fix error message report when not supported.

From: Adrian Hunter
Date: Fri Apr 08 2022 - 03:59:26 EST


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@xxxxxxxxxxx>

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@xxxxxxxxxxx>

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


Otherwise you can add:

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


> ---
> 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,
> +};