Messages in this thread | | | From | Ian Rogers <> | Date | Thu, 2 Nov 2023 16:55:50 -0700 | Subject | Re: [PATCH] perf test: Simplify object code reading test |
| |
On Thu, Nov 2, 2023 at 4:40 PM Namhyung Kim <namhyung@kernel.org> 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@linux.ibm.com> > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > 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 >
| |