Re: [PATCH] perf test: Simplify object code reading test

[Ian Rogers]

On Thu, Nov 2, 2023 at 4:40 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> It tries cycles (or cpu-clock on s390) event with exclude_kernel bit to
> open.  But other arch on a VM can fail with the hardware event and need
> to fallback to the software event in the same way.
>
> So let's get rid of the cpuid check and use generic fallback mechanism
> using an array of event candidates.  Now event in the odd index excludes
> the kernel so use that for the return value.
>
> Cc: Thomas Richter <tmricht@xxxxxxxxxxxxx>
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
>  tools/perf/tests/code-reading.c | 76 ++++++++++-----------------------
>  1 file changed, 23 insertions(+), 53 deletions(-)
>
> diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
> index 3af81012014e..047ba297c6fa 100644
> --- a/tools/perf/tests/code-reading.c
> +++ b/tools/perf/tests/code-reading.c
> @@ -511,38 +511,6 @@ static void fs_something(void)
>         }
>  }
>
> -#ifdef __s390x__
> -#include "header.h" // for get_cpuid()
> -#endif
> -
> -static const char *do_determine_event(bool excl_kernel)
> -{
> -       const char *event = excl_kernel ? "cycles:u" : "cycles";
> -
> -#ifdef __s390x__
> -       char cpuid[128], model[16], model_c[16], cpum_cf_v[16];
> -       unsigned int family;
> -       int ret, cpum_cf_a;
> -
> -       if (get_cpuid(cpuid, sizeof(cpuid)))
> -               goto out_clocks;
> -       ret = sscanf(cpuid, "%*[^,],%u,%[^,],%[^,],%[^,],%x", &family, model_c,
> -                    model, cpum_cf_v, &cpum_cf_a);
> -       if (ret != 5)            /* Not available */
> -               goto out_clocks;
> -       if (excl_kernel && (cpum_cf_a & 4))
> -               return event;
> -       if (!excl_kernel && (cpum_cf_a & 2))
> -               return event;
> -
> -       /* Fall through: missing authorization */
> -out_clocks:
> -       event = excl_kernel ? "cpu-clock:u" : "cpu-clock";
> -
> -#endif
> -       return event;
> -}
> -
>  static void do_something(void)
>  {
>         fs_something();
> @@ -583,8 +551,10 @@ static int do_test_code_reading(bool try_kcore)
>         int err = -1, ret;
>         pid_t pid;
>         struct map *map;
> -       bool have_vmlinux, have_kcore, excl_kernel = false;
> +       bool have_vmlinux, have_kcore;
>         struct dso *dso;
> +       const char *events[] = { "cycles", "cycles:u", "cpu-clock", "cpu-clock:u", NULL };
> +       int evidx = 0;
>
>         pid = getpid();
>
> @@ -618,7 +588,7 @@ static int do_test_code_reading(bool try_kcore)
>
>         /* No point getting kernel events if there is no kernel object */
>         if (!have_vmlinux && !have_kcore)
> -               excl_kernel = true;
> +               evidx++;
>
>         threads = thread_map__new_by_tid(pid);
>         if (!threads) {
> @@ -646,7 +616,7 @@ static int do_test_code_reading(bool try_kcore)
>                 goto out_put;
>         }
>
> -       while (1) {
> +       while (events[evidx]) {
>                 const char *str;
>
>                 evlist = evlist__new();
> @@ -657,7 +627,7 @@ static int do_test_code_reading(bool try_kcore)
>
>                 perf_evlist__set_maps(&evlist->core, cpus, threads);
>
> -               str = do_determine_event(excl_kernel);
> +               str = events[evidx];
>                 pr_debug("Parsing event '%s'\n", str);
>                 ret = parse_event(evlist, str);
>                 if (ret < 0) {
> @@ -675,32 +645,32 @@ static int do_test_code_reading(bool try_kcore)
>
>                 ret = evlist__open(evlist);
>                 if (ret < 0) {
> -                       if (!excl_kernel) {
> -                               excl_kernel = true;
> -                               /*
> -                                * Both cpus and threads are now owned by evlist
> -                                * and will be freed by following perf_evlist__set_maps
> -                                * call. Getting reference to keep them alive.
> -                                */
> -                               perf_cpu_map__get(cpus);
> -                               perf_thread_map__get(threads);
> -                               perf_evlist__set_maps(&evlist->core, NULL, NULL);
> -                               evlist__delete(evlist);
> -                               evlist = NULL;
> -                               continue;
> -                       }
> +                       evidx++;
>
> -                       if (verbose > 0) {
> +                       if (events[evidx] == NULL && verbose > 0) {
>                                 char errbuf[512];
>                                 evlist__strerror_open(evlist, errno, errbuf, sizeof(errbuf));
>                                 pr_debug("perf_evlist__open() failed!\n%s\n", errbuf);
>                         }
>
> -                       goto out_put;
> +                       /*
> +                        * Both cpus and threads are now owned by evlist
> +                        * and will be freed by following perf_evlist__set_maps
> +                        * call. Getting reference to keep them alive.
> +                        */
> +                       perf_cpu_map__get(cpus);
> +                       perf_thread_map__get(threads);
> +                       perf_evlist__set_maps(&evlist->core, NULL, NULL);
> +                       evlist__delete(evlist);
> +                       evlist = NULL;
> +                       continue;
>                 }
>                 break;
>         }
>
> +       if (events[evidx] == NULL)
> +               goto out_put;
> +
>         ret = evlist__mmap(evlist, UINT_MAX);
>         if (ret < 0) {
>                 pr_debug("evlist__mmap failed\n");
> @@ -721,7 +691,7 @@ static int do_test_code_reading(bool try_kcore)
>                 err = TEST_CODE_READING_NO_KERNEL_OBJ;
>         else if (!have_vmlinux && !try_kcore)
>                 err = TEST_CODE_READING_NO_VMLINUX;
> -       else if (excl_kernel)
> +       else if (evidx % 2)
>                 err = TEST_CODE_READING_NO_ACCESS;

Perhaps it would be more intention revealing to do:

if (strstr(events[evidx], ":u"))

rather than the "evidx % 2".

Thanks,
Ian

>         else
>                 err = TEST_CODE_READING_OK;
> --
> 2.42.0.869.gea05f2083d-goog
>