Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220406100609.65239-1-chengdongli@tencent.com> <ce2c3c4f-3751-312a-5fa6-0f98330067fe@intel.com>
In-Reply-To: <ce2c3c4f-3751-312a-5fa6-0f98330067fe@intel.com>
From:   Bryton Lee <brytonlee01@gmail.com>
Date:   Thu, 7 Apr 2022 19:34:51 +0800
Message-ID: <CAC2pzGe0Kn9Vzyx7S5_5A338shYG-DXgY=CS38zpJYk3m3_N7A@mail.gmail.com>
Subject: Re: [PATCH v2] perf test tsc: Fix error message report when not supported.
To:     Adrian Hunter <adrian.hunter@intel.com>
Cc:     linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        peterz@infradead.org, mingo@redhat.com,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        mark.rutland@arm.com, alexander.shishkin@linux.intel.com,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>, ak@linux.intel.com,
        likexu@tencent.com, chengdongli@tencent.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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