linux-kernel - Re: [PATCH v2] perf test tsc: Fix error message report when not supported.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 7 Apr 2022 15:34:32 +0300
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Bryton Lee <brytonlee01@...il.com>
Cc:     linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        peterz@...radead.org, mingo@...hat.com,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        mark.rutland@....com, alexander.shishkin@...ux.intel.com,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>, ak@...ux.intel.com,
        likexu@...cent.com, chengdongli@...cent.com
Subject: Re: [PATCH v2] perf test tsc: Fix error message report when not
 supported.

On 07/04/2022 14.34, Bryton Lee wrote:
> On Thu, Apr 7, 2022 at 2:59 PM Adrian Hunter <adrian.hunter@...el.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@...cent.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,
>>> +};
>>