| Message ID | 20221222160328.3639989-3-james.clark@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf cs_etm: Basic support for virtual/kernel timestamps | expand |
On Thu, Dec 22, 2022 at 04:03:22PM +0000, James Clark wrote: > Remove some code that duplicates existing methods. This requires that > some consts are removed because one of the existing helper methods takes > a struct perf_pmu instead of a name which has a non const name field. > But except for the tests, the strings were already non const. > > No functional changes. > > Signed-off-by: James Clark <james.clark@arm.com> > --- > tools/perf/tests/evsel-roundtrip-name.c | 3 +- > tools/perf/tests/parse-events.c | 3 +- > tools/perf/util/cputopo.c | 9 +----- > tools/perf/util/pmu-hybrid.c | 27 +++------------- > tools/perf/util/pmu-hybrid.h | 2 +- > tools/perf/util/pmu.c | 42 +++++++------------------ > tools/perf/util/pmu.h | 3 +- > 7 files changed, 24 insertions(+), 65 deletions(-) > > diff --git a/tools/perf/tests/evsel-roundtrip-name.c b/tools/perf/tests/evsel-roundtrip-name.c > index e94fed901992..9bccf177f481 100644 > --- a/tools/perf/tests/evsel-roundtrip-name.c > +++ b/tools/perf/tests/evsel-roundtrip-name.c > @@ -103,8 +103,9 @@ static int test__perf_evsel__roundtrip_name_test(struct test_suite *test __maybe > int subtest __maybe_unused) > { > int err = 0, ret = 0; > + char cpu_atom[] = "cpu_atom"; > > - if (perf_pmu__has_hybrid() && perf_pmu__hybrid_mounted("cpu_atom")) > + if (perf_pmu__has_hybrid() && perf_pmu__hybrid_mounted(cpu_atom)) After change the parameter 'name' to non const type in function perf_pmu__hybrid_mounted(), at here we still can pass string "cpu_atom" without warning, right? If so, we don't need to define a local variable 'cpu_atom'. [...] > -bool perf_pmu__hybrid_mounted(const char *name) > +bool perf_pmu__hybrid_mounted(char *name) > { > - char path[PATH_MAX]; > - const char *sysfs; > - FILE *file; > - int n, cpu; > + int cpu; > + struct perf_pmu pmu = {.name = name}; > > if (strncmp(name, "cpu_", 4)) > return false; > > - sysfs = sysfs__mountpoint(); > - if (!sysfs) > - return false; > - > - snprintf(path, PATH_MAX, CPUS_TEMPLATE_CPU, sysfs, name); > - if (!file_available(path)) > - return false; > - > - file = fopen(path, "r"); > - if (!file) > - return false; > - > - n = fscanf(file, "%u", &cpu); > - fclose(file); > - if (n <= 0) > - return false; > - > - return true; > + return perf_pmu__scan_file(&pmu, "cpus", "%u", &cpu) > 0; It's not necessarily to change the parameter 'name' to non const type, we can handle this case in two different ways. The first option is to refine the code with using the function perf_pmu__pathname_scnprintf() which is introduced in patch 01, thus we can keep using fopen() and fscanf() to read out value from 'cpus' entry. Another option is to define a local array 'pmu_name[PATH_MAX]' and then copy the input string to this array, something like: bool perf_pmu__hybrid_mounted(const char *name) { char pmu_name[PATH_MAX]; struct perf_pmu pmu; int cpu; strncpy(pmu_name, name, sizeof(pmu_name)); pmu.name = pmu_name; return perf_pmu__scan_file(&pmu, "cpus", "%u", &cpu) > 0; } We even can consider to dynamically allocate buffer and copy string from 'name' to the allocated buffer. > } > > struct perf_pmu *perf_pmu__find_hybrid_pmu(const char *name) > diff --git a/tools/perf/util/pmu-hybrid.h b/tools/perf/util/pmu-hybrid.h > index 2b186c26a43e..11db6ef4a376 100644 > --- a/tools/perf/util/pmu-hybrid.h > +++ b/tools/perf/util/pmu-hybrid.h > @@ -13,7 +13,7 @@ extern struct list_head perf_pmu__hybrid_pmus; > #define perf_pmu__for_each_hybrid_pmu(pmu) \ > list_for_each_entry(pmu, &perf_pmu__hybrid_pmus, hybrid_list) > > -bool perf_pmu__hybrid_mounted(const char *name); > +bool perf_pmu__hybrid_mounted(char *name); > > struct perf_pmu *perf_pmu__find_hybrid_pmu(const char *name); > bool perf_pmu__is_hybrid(const char *name); > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c > index 827c1a6bb99a..faaeec1e15aa 100644 > --- a/tools/perf/util/pmu.c > +++ b/tools/perf/util/pmu.c > @@ -570,45 +570,29 @@ static void pmu_read_sysfs(void) > closedir(dir); > } > > -static struct perf_cpu_map *__pmu_cpumask(const char *path) > -{ > - FILE *file; > - struct perf_cpu_map *cpus; > - > - file = fopen(path, "r"); > - if (!file) > - return NULL; > - > - cpus = perf_cpu_map__read(file); > - fclose(file); > - return cpus; > -} > - > /* > * Uncore PMUs have a "cpumask" file under sysfs. CPU PMUs (e.g. on arm/arm64) > * may have a "cpus" file. > */ > #define SYS_TEMPLATE_ID "./bus/event_source/devices/%s/identifier" > -#define CPUS_TEMPLATE_UNCORE "%s/bus/event_source/devices/%s/cpumask" > > -static struct perf_cpu_map *pmu_cpumask(const char *name) > +static struct perf_cpu_map *pmu_cpumask(char *name) > { > - char path[PATH_MAX]; > struct perf_cpu_map *cpus; > - const char *sysfs = sysfs__mountpoint(); > const char *templates[] = { > - CPUS_TEMPLATE_UNCORE, > - CPUS_TEMPLATE_CPU, > + "cpumask", > + "cpus", > NULL > }; > const char **template; > - > - if (!sysfs) > - return NULL; > + struct perf_pmu pmu = {.name = name}; Here we also can define a local array and copy string from 'name' to the local array. This can allow us to provide a friendly function and caller doesn't need to explictly pass non const string. Thanks, Leo > + FILE *file; > > for (template = templates; *template; template++) { > - snprintf(path, PATH_MAX, *template, sysfs, name); > - cpus = __pmu_cpumask(path); > + file = perf_pmu__open_file(&pmu, *template); > + if (!file) > + continue; > + cpus = perf_cpu_map__read(file); > if (cpus) > return cpus; > } > @@ -616,16 +600,14 @@ static struct perf_cpu_map *pmu_cpumask(const char *name) > return NULL; > } > > -static bool pmu_is_uncore(const char *name) > +static bool pmu_is_uncore(char *name) > { > char path[PATH_MAX]; > - const char *sysfs; > > if (perf_pmu__hybrid_mounted(name)) > return false; > > - sysfs = sysfs__mountpoint(); > - snprintf(path, PATH_MAX, CPUS_TEMPLATE_UNCORE, sysfs, name); > + perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "cpumask"); > return file_available(path); > } > > @@ -1736,7 +1718,7 @@ bool pmu_have_event(const char *pname, const char *name) > return false; > } > > -static FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name) > +FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name) > { > char path[PATH_MAX]; > > diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h > index 2f2bb0286e2a..8f39e2d17fb1 100644 > --- a/tools/perf/util/pmu.h > +++ b/tools/perf/util/pmu.h > @@ -2,6 +2,7 @@ > #ifndef __PMU_H > #define __PMU_H > > +#include <bits/types/FILE.h> > #include <linux/bitmap.h> > #include <linux/compiler.h> > #include <linux/perf_event.h> > @@ -22,7 +23,6 @@ enum { > }; > > #define PERF_PMU_FORMAT_BITS 64 > -#define CPUS_TEMPLATE_CPU "%s/bus/event_source/devices/%s/cpus" > #define MAX_PMU_NAME_LEN 128 > > struct perf_event_attr; > @@ -261,5 +261,6 @@ char *pmu_find_alias_name(const char *name); > int perf_pmu__event_source_devices_scnprintf(char *pathname, size_t size); > int perf_pmu__pathname_scnprintf(char *buf, size_t size, > const char *pmu_name, const char *filename); > +FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name); > > #endif /* __PMU_H */ > -- > 2.25.1 >
On Fri, Dec 23, 2022 at 11:45:21AM +0800, Leo Yan wrote: [...] > > @@ -103,8 +103,9 @@ static int test__perf_evsel__roundtrip_name_test(struct test_suite *test __maybe > > int subtest __maybe_unused) > > { > > int err = 0, ret = 0; > > + char cpu_atom[] = "cpu_atom"; > > > > - if (perf_pmu__has_hybrid() && perf_pmu__hybrid_mounted("cpu_atom")) > > + if (perf_pmu__has_hybrid() && perf_pmu__hybrid_mounted(cpu_atom)) > > After change the parameter 'name' to non const type in function > perf_pmu__hybrid_mounted(), at here we still can pass string "cpu_atom" > without warning, right? If so, we don't need to define a local > variable 'cpu_atom'. Correct for above statement, I did experiment and confirmed building failure after change the parameter to non const type and directly pass string "cpu_atom": tests/evsel-roundtrip-name.c: In function ‘test__perf_evsel__roundtrip_name_test’: tests/evsel-roundtrip-name.c:108:64: error: passing argument 1 of ‘perf_pmu__hybrid_mounted’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] 108 | if (perf_pmu__has_hybrid() && perf_pmu__hybrid_mounted("cpu_atom")) | ^~~~~~~~~~ But I still suggest we can keep const type for the parameter for perf_pmu__hybrid_mounted(), this is more friendly for its callers without define local strings. Thanks, Leo
On 23/12/2022 03:45, Leo Yan wrote: > On Thu, Dec 22, 2022 at 04:03:22PM +0000, James Clark wrote: >> Remove some code that duplicates existing methods. This requires that >> some consts are removed because one of the existing helper methods takes >> a struct perf_pmu instead of a name which has a non const name field. >> But except for the tests, the strings were already non const. >> >> No functional changes. >> >> Signed-off-by: James Clark <james.clark@arm.com> >> --- >> tools/perf/tests/evsel-roundtrip-name.c | 3 +- >> tools/perf/tests/parse-events.c | 3 +- >> tools/perf/util/cputopo.c | 9 +----- >> tools/perf/util/pmu-hybrid.c | 27 +++------------- >> tools/perf/util/pmu-hybrid.h | 2 +- >> tools/perf/util/pmu.c | 42 +++++++------------------ >> tools/perf/util/pmu.h | 3 +- >> 7 files changed, 24 insertions(+), 65 deletions(-) >> >> diff --git a/tools/perf/tests/evsel-roundtrip-name.c b/tools/perf/tests/evsel-roundtrip-name.c >> index e94fed901992..9bccf177f481 100644 >> --- a/tools/perf/tests/evsel-roundtrip-name.c >> +++ b/tools/perf/tests/evsel-roundtrip-name.c >> @@ -103,8 +103,9 @@ static int test__perf_evsel__roundtrip_name_test(struct test_suite *test __maybe >> int subtest __maybe_unused) >> { >> int err = 0, ret = 0; >> + char cpu_atom[] = "cpu_atom"; >> >> - if (perf_pmu__has_hybrid() && perf_pmu__hybrid_mounted("cpu_atom")) >> + if (perf_pmu__has_hybrid() && perf_pmu__hybrid_mounted(cpu_atom)) > > After change the parameter 'name' to non const type in function > perf_pmu__hybrid_mounted(), at here we still can pass string "cpu_atom" > without warning, right? If so, we don't need to define a local > variable 'cpu_atom'. > > [...] > >> -bool perf_pmu__hybrid_mounted(const char *name) >> +bool perf_pmu__hybrid_mounted(char *name) >> { >> - char path[PATH_MAX]; >> - const char *sysfs; >> - FILE *file; >> - int n, cpu; >> + int cpu; >> + struct perf_pmu pmu = {.name = name}; >> >> if (strncmp(name, "cpu_", 4)) >> return false; >> >> - sysfs = sysfs__mountpoint(); >> - if (!sysfs) >> - return false; >> - >> - snprintf(path, PATH_MAX, CPUS_TEMPLATE_CPU, sysfs, name); >> - if (!file_available(path)) >> - return false; >> - >> - file = fopen(path, "r"); >> - if (!file) >> - return false; >> - >> - n = fscanf(file, "%u", &cpu); >> - fclose(file); >> - if (n <= 0) >> - return false; >> - >> - return true; >> + return perf_pmu__scan_file(&pmu, "cpus", "%u", &cpu) > 0; > > It's not necessarily to change the parameter 'name' to non const type, > we can handle this case in two different ways. > > The first option is to refine the code with using the function > perf_pmu__pathname_scnprintf() which is introduced in patch 01, thus > we can keep using fopen() and fscanf() to read out value from 'cpus' > entry. > > Another option is to define a local array 'pmu_name[PATH_MAX]' and > then copy the input string to this array, something like: > > bool perf_pmu__hybrid_mounted(const char *name) > { > char pmu_name[PATH_MAX]; > struct perf_pmu pmu; > int cpu; > > strncpy(pmu_name, name, sizeof(pmu_name)); > pmu.name = pmu_name; > > return perf_pmu__scan_file(&pmu, "cpus", "%u", &cpu) > 0; > } > > We even can consider to dynamically allocate buffer and copy string from > 'name' to the allocated buffer. > I went with the string copy way and put the consts back in V3. One minor difference is that I had to use strlcpy() instead of strncpy() to avoid a compiler warning about the destination buffer being the same size. James [...] >> -static struct perf_cpu_map *pmu_cpumask(const char *name) >> +static struct perf_cpu_map *pmu_cpumask(char *name) >> { >> - char path[PATH_MAX]; >> struct perf_cpu_map *cpus; >> - const char *sysfs = sysfs__mountpoint(); >> const char *templates[] = { >> - CPUS_TEMPLATE_UNCORE, >> - CPUS_TEMPLATE_CPU, >> + "cpumask", >> + "cpus", >> NULL >> }; >> const char **template; >> - >> - if (!sysfs) >> - return NULL; >> + struct perf_pmu pmu = {.name = name}; > > Here we also can define a local array and copy string from 'name' to > the local array. This can allow us to provide a friendly function and > caller doesn't need to explictly pass non const string. > Done
diff --git a/tools/perf/tests/evsel-roundtrip-name.c b/tools/perf/tests/evsel-roundtrip-name.c index e94fed901992..9bccf177f481 100644 --- a/tools/perf/tests/evsel-roundtrip-name.c +++ b/tools/perf/tests/evsel-roundtrip-name.c @@ -103,8 +103,9 @@ static int test__perf_evsel__roundtrip_name_test(struct test_suite *test __maybe int subtest __maybe_unused) { int err = 0, ret = 0; + char cpu_atom[] = "cpu_atom"; - if (perf_pmu__has_hybrid() && perf_pmu__hybrid_mounted("cpu_atom")) + if (perf_pmu__has_hybrid() && perf_pmu__hybrid_mounted(cpu_atom)) return perf_evsel__name_array_test(evsel__hw_names, 2); err = perf_evsel__name_array_test(evsel__hw_names, 1); diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c index 71a5cb343311..5857125202bf 100644 --- a/tools/perf/tests/parse-events.c +++ b/tools/perf/tests/parse-events.c @@ -1608,8 +1608,9 @@ static int test__hybrid_group_modifier1(struct evlist *evlist) static int test__hybrid_raw1(struct evlist *evlist) { struct evsel *evsel = evlist__first(evlist); + char cpu_atom[] = "cpu_atom"; - if (!perf_pmu__hybrid_mounted("cpu_atom")) { + if (!perf_pmu__hybrid_mounted(cpu_atom)) { TEST_ASSERT_VAL("wrong number of entries", 1 == evlist->core.nr_entries); TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type); TEST_ASSERT_VAL("wrong config", 0x1a == evsel->core.attr.config); diff --git a/tools/perf/util/cputopo.c b/tools/perf/util/cputopo.c index 1a3ff6449158..e08797c3cdbc 100644 --- a/tools/perf/util/cputopo.c +++ b/tools/perf/util/cputopo.c @@ -422,8 +422,6 @@ void numa_topology__delete(struct numa_topology *tp) static int load_hybrid_node(struct hybrid_topology_node *node, struct perf_pmu *pmu) { - const char *sysfs; - char path[PATH_MAX]; char *buf = NULL, *p; FILE *fp; size_t len = 0; @@ -432,12 +430,7 @@ static int load_hybrid_node(struct hybrid_topology_node *node, if (!node->pmu_name) return -1; - sysfs = sysfs__mountpoint(); - if (!sysfs) - goto err; - - snprintf(path, PATH_MAX, CPUS_TEMPLATE_CPU, sysfs, pmu->name); - fp = fopen(path, "r"); + fp = perf_pmu__open_file(pmu, "cpus"); if (!fp) goto err; diff --git a/tools/perf/util/pmu-hybrid.c b/tools/perf/util/pmu-hybrid.c index f51ccaac60ee..317c612cc41d 100644 --- a/tools/perf/util/pmu-hybrid.c +++ b/tools/perf/util/pmu-hybrid.c @@ -18,34 +18,15 @@ LIST_HEAD(perf_pmu__hybrid_pmus); -bool perf_pmu__hybrid_mounted(const char *name) +bool perf_pmu__hybrid_mounted(char *name) { - char path[PATH_MAX]; - const char *sysfs; - FILE *file; - int n, cpu; + int cpu; + struct perf_pmu pmu = {.name = name}; if (strncmp(name, "cpu_", 4)) return false; - sysfs = sysfs__mountpoint(); - if (!sysfs) - return false; - - snprintf(path, PATH_MAX, CPUS_TEMPLATE_CPU, sysfs, name); - if (!file_available(path)) - return false; - - file = fopen(path, "r"); - if (!file) - return false; - - n = fscanf(file, "%u", &cpu); - fclose(file); - if (n <= 0) - return false; - - return true; + return perf_pmu__scan_file(&pmu, "cpus", "%u", &cpu) > 0; } struct perf_pmu *perf_pmu__find_hybrid_pmu(const char *name) diff --git a/tools/perf/util/pmu-hybrid.h b/tools/perf/util/pmu-hybrid.h index 2b186c26a43e..11db6ef4a376 100644 --- a/tools/perf/util/pmu-hybrid.h +++ b/tools/perf/util/pmu-hybrid.h @@ -13,7 +13,7 @@ extern struct list_head perf_pmu__hybrid_pmus; #define perf_pmu__for_each_hybrid_pmu(pmu) \ list_for_each_entry(pmu, &perf_pmu__hybrid_pmus, hybrid_list) -bool perf_pmu__hybrid_mounted(const char *name); +bool perf_pmu__hybrid_mounted(char *name); struct perf_pmu *perf_pmu__find_hybrid_pmu(const char *name); bool perf_pmu__is_hybrid(const char *name); diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index 827c1a6bb99a..faaeec1e15aa 100644 --- a/tools/perf/util/pmu.c +++ b/tools/perf/util/pmu.c @@ -570,45 +570,29 @@ static void pmu_read_sysfs(void) closedir(dir); } -static struct perf_cpu_map *__pmu_cpumask(const char *path) -{ - FILE *file; - struct perf_cpu_map *cpus; - - file = fopen(path, "r"); - if (!file) - return NULL; - - cpus = perf_cpu_map__read(file); - fclose(file); - return cpus; -} - /* * Uncore PMUs have a "cpumask" file under sysfs. CPU PMUs (e.g. on arm/arm64) * may have a "cpus" file. */ #define SYS_TEMPLATE_ID "./bus/event_source/devices/%s/identifier" -#define CPUS_TEMPLATE_UNCORE "%s/bus/event_source/devices/%s/cpumask" -static struct perf_cpu_map *pmu_cpumask(const char *name) +static struct perf_cpu_map *pmu_cpumask(char *name) { - char path[PATH_MAX]; struct perf_cpu_map *cpus; - const char *sysfs = sysfs__mountpoint(); const char *templates[] = { - CPUS_TEMPLATE_UNCORE, - CPUS_TEMPLATE_CPU, + "cpumask", + "cpus", NULL }; const char **template; - - if (!sysfs) - return NULL; + struct perf_pmu pmu = {.name = name}; + FILE *file; for (template = templates; *template; template++) { - snprintf(path, PATH_MAX, *template, sysfs, name); - cpus = __pmu_cpumask(path); + file = perf_pmu__open_file(&pmu, *template); + if (!file) + continue; + cpus = perf_cpu_map__read(file); if (cpus) return cpus; } @@ -616,16 +600,14 @@ static struct perf_cpu_map *pmu_cpumask(const char *name) return NULL; } -static bool pmu_is_uncore(const char *name) +static bool pmu_is_uncore(char *name) { char path[PATH_MAX]; - const char *sysfs; if (perf_pmu__hybrid_mounted(name)) return false; - sysfs = sysfs__mountpoint(); - snprintf(path, PATH_MAX, CPUS_TEMPLATE_UNCORE, sysfs, name); + perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "cpumask"); return file_available(path); } @@ -1736,7 +1718,7 @@ bool pmu_have_event(const char *pname, const char *name) return false; } -static FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name) +FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name) { char path[PATH_MAX]; diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h index 2f2bb0286e2a..8f39e2d17fb1 100644 --- a/tools/perf/util/pmu.h +++ b/tools/perf/util/pmu.h @@ -2,6 +2,7 @@ #ifndef __PMU_H #define __PMU_H +#include <bits/types/FILE.h> #include <linux/bitmap.h> #include <linux/compiler.h> #include <linux/perf_event.h> @@ -22,7 +23,6 @@ enum { }; #define PERF_PMU_FORMAT_BITS 64 -#define CPUS_TEMPLATE_CPU "%s/bus/event_source/devices/%s/cpus" #define MAX_PMU_NAME_LEN 128 struct perf_event_attr; @@ -261,5 +261,6 @@ char *pmu_find_alias_name(const char *name); int perf_pmu__event_source_devices_scnprintf(char *pathname, size_t size); int perf_pmu__pathname_scnprintf(char *buf, size_t size, const char *pmu_name, const char *filename); +FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name); #endif /* __PMU_H */
Remove some code that duplicates existing methods. This requires that some consts are removed because one of the existing helper methods takes a struct perf_pmu instead of a name which has a non const name field. But except for the tests, the strings were already non const. No functional changes. Signed-off-by: James Clark <james.clark@arm.com> --- tools/perf/tests/evsel-roundtrip-name.c | 3 +- tools/perf/tests/parse-events.c | 3 +- tools/perf/util/cputopo.c | 9 +----- tools/perf/util/pmu-hybrid.c | 27 +++------------- tools/perf/util/pmu-hybrid.h | 2 +- tools/perf/util/pmu.c | 42 +++++++------------------ tools/perf/util/pmu.h | 3 +- 7 files changed, 24 insertions(+), 65 deletions(-)