Changes since v1:
* Add 3 refactor commits for sysfs reading around pmu.c as suggested by Arnaldo here [1] * The dependency on [2] has now reached mainline so is no longer blocking * Rebase on perf/core
[1]: https://lore.kernel.org/all/YnqVqq5QW%2Fb14oPZ@kernel.org/ [2]: https://lore.kernel.org/all/20220503123537.1003035-1-german.gomez@arm.com/
German Gomez (4): perf pmu: Add function to check if a pmu file exists perf cs_etm: Keep separate symbols for ETMv4 and ETE parameters perf cs_etm: Record ts_source in AUXTRACE_INFO for ETMv4 and ETE perf cs_etm: Set the time field in the synthetic samples
James Clark (3): perf: Remove duplication around EVENT_SOURCE_DEVICE_PATH perf: Use perf_pmu__open_file() and perf_pmu__scan_file() perf: Remove remaining duplication of bus/event_source/devices/...
tools/perf/arch/arm/util/auxtrace.c | 5 +- tools/perf/arch/arm/util/cs-etm.c | 91 +++++++++++- tools/perf/arch/x86/util/pmu.c | 12 +- tools/perf/tests/evsel-roundtrip-name.c | 3 +- tools/perf/tests/parse-events.c | 3 +- tools/perf/util/cputopo.c | 9 +- tools/perf/util/cs-etm-base.c | 34 +++-- tools/perf/util/cs-etm.c | 86 +++++++++-- tools/perf/util/cs-etm.h | 13 +- tools/perf/util/pmu-hybrid.c | 27 +--- tools/perf/util/pmu-hybrid.h | 2 +- tools/perf/util/pmu.c | 183 ++++++++++-------------- tools/perf/util/pmu.h | 10 +- 13 files changed, 292 insertions(+), 186 deletions(-)
base-commit: 573de010917836f198a4e579d40674991659668b
The pattern for accessing EVENT_SOURCE_DEVICE_PATH is duplicated in a few places, so add two utility functions to cover it. Also just use perf_pmu__scan_file() instead of pmu_type() which already does the same thing.
No functional changes.
Signed-off-by: James Clark james.clark@arm.com --- tools/perf/arch/arm/util/auxtrace.c | 5 +- tools/perf/arch/x86/util/pmu.c | 12 +-- tools/perf/util/pmu.c | 110 +++++++++++----------------- tools/perf/util/pmu.h | 5 +- 4 files changed, 49 insertions(+), 83 deletions(-)
diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm/util/auxtrace.c index deeb163999ce..3cb4a6a16112 100644 --- a/tools/perf/arch/arm/util/auxtrace.c +++ b/tools/perf/arch/arm/util/auxtrace.c @@ -55,17 +55,16 @@ static struct perf_pmu **find_all_arm_spe_pmus(int *nr_spes, int *err)
static struct perf_pmu **find_all_hisi_ptt_pmus(int *nr_ptts, int *err) { - const char *sysfs = sysfs__mountpoint(); struct perf_pmu **hisi_ptt_pmus = NULL; struct dirent *dent; char path[PATH_MAX]; DIR *dir = NULL; int idx = 0;
- snprintf(path, PATH_MAX, "%s" EVENT_SOURCE_DEVICE_PATH, sysfs); + perf_pmu__event_source_devices_scnprintf(path, PATH_MAX); dir = opendir(path); if (!dir) { - pr_err("can't read directory '%s'\n", EVENT_SOURCE_DEVICE_PATH); + pr_err("can't read directory '%s'\n", path); *err = -EINVAL; return NULL; } diff --git a/tools/perf/arch/x86/util/pmu.c b/tools/perf/arch/x86/util/pmu.c index 74d69db1ea99..e3456d4b9063 100644 --- a/tools/perf/arch/x86/util/pmu.c +++ b/tools/perf/arch/x86/util/pmu.c @@ -15,8 +15,6 @@ #include "../../../util/pmu.h" #include "../../../util/fncache.h"
-#define TEMPLATE_ALIAS "%s/bus/event_source/devices/%s/alias" - struct pmu_alias { char *name; char *alias; @@ -72,18 +70,14 @@ static int setup_pmu_alias_list(void) char path[PATH_MAX]; DIR *dir; struct dirent *dent; - const char *sysfs = sysfs__mountpoint(); struct pmu_alias *pmu_alias; char buf[MAX_PMU_NAME_LEN]; FILE *file; int ret = -ENOMEM;
- if (!sysfs) + if (!perf_pmu__event_source_devices_scnprintf(path, PATH_MAX)) return -1;
- snprintf(path, PATH_MAX, - "%s" EVENT_SOURCE_DEVICE_PATH, sysfs); - dir = opendir(path); if (!dir) return -errno; @@ -93,9 +87,7 @@ static int setup_pmu_alias_list(void) !strcmp(dent->d_name, "..")) continue;
- snprintf(path, PATH_MAX, - TEMPLATE_ALIAS, sysfs, dent->d_name); - + perf_pmu__pathname_scnprintf(path, PATH_MAX, dent->d_name, "alias"); if (!file_available(path)) continue;
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index 2bdeb89352e7..827c1a6bb99a 100644 --- a/tools/perf/util/pmu.c +++ b/tools/perf/util/pmu.c @@ -107,14 +107,10 @@ int perf_pmu__format_parse(char *dir, struct list_head *head) static int pmu_format(const char *name, struct list_head *format) { char path[PATH_MAX]; - const char *sysfs = sysfs__mountpoint();
- if (!sysfs) + if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "format")) return -1;
- snprintf(path, PATH_MAX, - "%s" EVENT_SOURCE_DEVICE_PATH "%s/format", sysfs, name); - if (!file_available(path)) return 0;
@@ -513,14 +509,10 @@ static int pmu_aliases_parse(char *dir, struct list_head *head) static int pmu_aliases(const char *name, struct list_head *head) { char path[PATH_MAX]; - const char *sysfs = sysfs__mountpoint();
- if (!sysfs) + if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "events")) return -1;
- snprintf(path, PATH_MAX, - "%s/bus/event_source/devices/%s/events", sysfs, name); - if (!file_available(path)) return 0;
@@ -554,52 +546,16 @@ static int pmu_alias_terms(struct perf_pmu_alias *alias, return 0; }
-/* - * Reading/parsing the default pmu type value, which should be - * located at: - * /sys/bus/event_source/devices/<dev>/type as sysfs attribute. - */ -static int pmu_type(const char *name, __u32 *type) -{ - char path[PATH_MAX]; - FILE *file; - int ret = 0; - const char *sysfs = sysfs__mountpoint(); - - if (!sysfs) - return -1; - - snprintf(path, PATH_MAX, - "%s" EVENT_SOURCE_DEVICE_PATH "%s/type", sysfs, name); - - if (access(path, R_OK) < 0) - return -1; - - file = fopen(path, "r"); - if (!file) - return -EINVAL; - - if (1 != fscanf(file, "%u", type)) - ret = -1; - - fclose(file); - return ret; -} - /* Add all pmus in sysfs to pmu list: */ static void pmu_read_sysfs(void) { char path[PATH_MAX]; DIR *dir; struct dirent *dent; - const char *sysfs = sysfs__mountpoint();
- if (!sysfs) + if (!perf_pmu__event_source_devices_scnprintf(path, PATH_MAX)) return;
- snprintf(path, PATH_MAX, - "%s" EVENT_SOURCE_DEVICE_PATH, sysfs); - dir = opendir(path); if (!dir) return; @@ -696,14 +652,9 @@ static char *pmu_id(const char *name) static int is_arm_pmu_core(const char *name) { char path[PATH_MAX]; - const char *sysfs = sysfs__mountpoint();
- if (!sysfs) + if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "cpus")) return 0; - - /* Look for cpu sysfs (specific to arm) */ - scnprintf(path, PATH_MAX, "%s/bus/event_source/devices/%s/cpus", - sysfs, name); return file_available(path); }
@@ -969,11 +920,8 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name) return NULL;
/* - * Check the type first to avoid unnecessary work. + * Check the aliases first to avoid unnecessary work. */ - if (pmu_type(name, &type)) - return NULL; - if (pmu_aliases(name, &aliases)) return NULL;
@@ -983,9 +931,14 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name)
pmu->cpus = pmu_cpumask(name); pmu->name = strdup(name); + if (!pmu->name) goto err;
+ /* Read type, and ensure that 1 value (type) is successfully assigned */ + if (perf_pmu__scan_file(pmu, "type", "%u", &type) != 1) + goto err; + alias_name = pmu_find_alias_name(name); if (alias_name) { pmu->alias_name = strdup(alias_name); @@ -1786,16 +1739,11 @@ bool pmu_have_event(const char *pname, const char *name) static FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name) { char path[PATH_MAX]; - const char *sysfs;
- sysfs = sysfs__mountpoint(); - if (!sysfs) + if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, pmu->name, name) || + !file_available(path)) return NULL;
- snprintf(path, PATH_MAX, - "%s" EVENT_SOURCE_DEVICE_PATH "%s/%s", sysfs, pmu->name, name); - if (!file_available(path)) - return NULL; return fopen(path, "r"); }
@@ -1849,7 +1797,6 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu) { struct stat st; char caps_path[PATH_MAX]; - const char *sysfs = sysfs__mountpoint(); DIR *caps_dir; struct dirent *evt_ent;
@@ -1858,12 +1805,9 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
pmu->nr_caps = 0;
- if (!sysfs) + if (!perf_pmu__pathname_scnprintf(caps_path, PATH_MAX, pmu->name, "caps")) return -1;
- snprintf(caps_path, PATH_MAX, - "%s" EVENT_SOURCE_DEVICE_PATH "%s/caps", sysfs, pmu->name); - if (stat(caps_path, &st) < 0) { pmu->caps_initialized = true; return 0; /* no error if caps does not exist */ @@ -1993,3 +1937,31 @@ int perf_pmu__cpus_match(struct perf_pmu *pmu, struct perf_cpu_map *cpus, *ucpus_ptr = unmatched_cpus; return 0; } + +int perf_pmu__event_source_devices_scnprintf(char *pathname, size_t size) +{ + const char *sysfs = sysfs__mountpoint(); + + if (!sysfs) + return 0; + return scnprintf(pathname, size, "%s/bus/event_source/devices/", sysfs); +} + +/* + * Fill 'buf' with the path to a file or folder in 'pmu_name' in + * sysfs. For example if pmu_name = "cs_etm" and 'filename' = "format" + * then pathname will be filled with + * "/sys/bus/event_source/devices/cs_etm/format" + * + * Return 0 if the sysfs mountpoint couldn't be found or if no + * characters were written. + */ +int perf_pmu__pathname_scnprintf(char *buf, size_t size, + const char *pmu_name, const char *filename) +{ + char base_path[PATH_MAX]; + + if (!perf_pmu__event_source_devices_scnprintf(base_path, PATH_MAX)) + return 0; + return scnprintf(buf, size, "%s%s/%s", base_path, pmu_name, filename); +} diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h index 69ca0004f94f..2f2bb0286e2a 100644 --- a/tools/perf/util/pmu.h +++ b/tools/perf/util/pmu.h @@ -22,7 +22,6 @@ enum { };
#define PERF_PMU_FORMAT_BITS 64 -#define EVENT_SOURCE_DEVICE_PATH "/bus/event_source/devices/" #define CPUS_TEMPLATE_CPU "%s/bus/event_source/devices/%s/cpus" #define MAX_PMU_NAME_LEN 128
@@ -259,4 +258,8 @@ int perf_pmu__cpus_match(struct perf_pmu *pmu, struct perf_cpu_map *cpus,
char *pmu_find_real_name(const char *name); 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); + #endif /* __PMU_H */
On Thu, Dec 22, 2022 at 04:03:21PM +0000, James Clark wrote:
The pattern for accessing EVENT_SOURCE_DEVICE_PATH is duplicated in a few places, so add two utility functions to cover it. Also just use perf_pmu__scan_file() instead of pmu_type() which already does the same thing.
No functional changes.
After I read the article: https://lwn.net/Articles/69419/, good to see this patch uses scnprintf() to replace snprintf().
Signed-off-by: James Clark james.clark@arm.com
tools/perf/arch/arm/util/auxtrace.c | 5 +- tools/perf/arch/x86/util/pmu.c | 12 +-- tools/perf/util/pmu.c | 110 +++++++++++----------------- tools/perf/util/pmu.h | 5 +- 4 files changed, 49 insertions(+), 83 deletions(-)
diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm/util/auxtrace.c index deeb163999ce..3cb4a6a16112 100644 --- a/tools/perf/arch/arm/util/auxtrace.c +++ b/tools/perf/arch/arm/util/auxtrace.c @@ -55,17 +55,16 @@ static struct perf_pmu **find_all_arm_spe_pmus(int *nr_spes, int *err) static struct perf_pmu **find_all_hisi_ptt_pmus(int *nr_ptts, int *err) {
- const char *sysfs = sysfs__mountpoint(); struct perf_pmu **hisi_ptt_pmus = NULL; struct dirent *dent; char path[PATH_MAX]; DIR *dir = NULL; int idx = 0;
- snprintf(path, PATH_MAX, "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
- perf_pmu__event_source_devices_scnprintf(path, PATH_MAX);
Nitpick: since 'path' is an array, a good practice is to use 'sizeof(path)' rather than 'PATH_MAX' for passing the length.
dir = opendir(path); if (!dir) {
pr_err("can't read directory '%s'\n", EVENT_SOURCE_DEVICE_PATH);
pr_err("can't read directory '%s'\n", path);diff --git a/tools/perf/arch/x86/util/pmu.c b/tools/perf/arch/x86/util/pmu.c index 74d69db1ea99..e3456d4b9063 100644 --- a/tools/perf/arch/x86/util/pmu.c +++ b/tools/perf/arch/x86/util/pmu.c @@ -15,8 +15,6 @@ #include "../../../util/pmu.h" #include "../../../util/fncache.h" -#define TEMPLATE_ALIAS "%s/bus/event_source/devices/%s/alias"
struct pmu_alias { char *name; char *alias; @@ -72,18 +70,14 @@ static int setup_pmu_alias_list(void) char path[PATH_MAX]; DIR *dir; struct dirent *dent;
- const char *sysfs = sysfs__mountpoint(); struct pmu_alias *pmu_alias; char buf[MAX_PMU_NAME_LEN]; FILE *file; int ret = -ENOMEM;
- if (!sysfs)
- if (!perf_pmu__event_source_devices_scnprintf(path, PATH_MAX)) return -1;
- snprintf(path, PATH_MAX,
"%s" EVENT_SOURCE_DEVICE_PATH, sysfs);- dir = opendir(path); if (!dir) return -errno;
@@ -93,9 +87,7 @@ static int setup_pmu_alias_list(void) !strcmp(dent->d_name, "..")) continue;
snprintf(path, PATH_MAX,TEMPLATE_ALIAS, sysfs, dent->d_name);
perf_pmu__pathname_scnprintf(path, PATH_MAX, dent->d_name, "alias");diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index 2bdeb89352e7..827c1a6bb99a 100644 --- a/tools/perf/util/pmu.c +++ b/tools/perf/util/pmu.c @@ -107,14 +107,10 @@ int perf_pmu__format_parse(char *dir, struct list_head *head) static int pmu_format(const char *name, struct list_head *format) { char path[PATH_MAX];
- const char *sysfs = sysfs__mountpoint();
- if (!sysfs)
- if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "format")) return -1;
- snprintf(path, PATH_MAX,
"%s" EVENT_SOURCE_DEVICE_PATH "%s/format", sysfs, name);- if (!file_available(path)) return 0;
@@ -513,14 +509,10 @@ static int pmu_aliases_parse(char *dir, struct list_head *head) static int pmu_aliases(const char *name, struct list_head *head) { char path[PATH_MAX];
- const char *sysfs = sysfs__mountpoint();
- if (!sysfs)
- if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "events")) return -1;
- snprintf(path, PATH_MAX,
"%s/bus/event_source/devices/%s/events", sysfs, name);- if (!file_available(path)) return 0;
@@ -554,52 +546,16 @@ static int pmu_alias_terms(struct perf_pmu_alias *alias, return 0; } -/*
- Reading/parsing the default pmu type value, which should be
- located at:
- /sys/bus/event_source/devices/<dev>/type as sysfs attribute.
- */
-static int pmu_type(const char *name, __u32 *type) -{
- char path[PATH_MAX];
- FILE *file;
- int ret = 0;
- const char *sysfs = sysfs__mountpoint();
- if (!sysfs)
return -1;- snprintf(path, PATH_MAX,
"%s" EVENT_SOURCE_DEVICE_PATH "%s/type", sysfs, name);- if (access(path, R_OK) < 0)
return -1;- file = fopen(path, "r");
- if (!file)
return -EINVAL;- if (1 != fscanf(file, "%u", type))
ret = -1;- fclose(file);
- return ret;
-}
/* Add all pmus in sysfs to pmu list: */ static void pmu_read_sysfs(void) { char path[PATH_MAX]; DIR *dir; struct dirent *dent;
- const char *sysfs = sysfs__mountpoint();
- if (!sysfs)
- if (!perf_pmu__event_source_devices_scnprintf(path, PATH_MAX)) return;
- snprintf(path, PATH_MAX,
"%s" EVENT_SOURCE_DEVICE_PATH, sysfs);- dir = opendir(path); if (!dir) return;
@@ -696,14 +652,9 @@ static char *pmu_id(const char *name) static int is_arm_pmu_core(const char *name) { char path[PATH_MAX];
- const char *sysfs = sysfs__mountpoint();
- if (!sysfs)
- if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "cpus")) return 0;
- /* Look for cpu sysfs (specific to arm) */
- scnprintf(path, PATH_MAX, "%s/bus/event_source/devices/%s/cpus",
sysfs, name);} @@ -969,11 +920,8 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name) return NULL; /*
* Check the type first to avoid unnecessary work.
* Check the aliases first to avoid unnecessary work.
- if (pmu_type(name, &type))
return NULL;- if (pmu_aliases(name, &aliases)) return NULL;
@@ -983,9 +931,14 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name) pmu->cpus = pmu_cpumask(name); pmu->name = strdup(name);
- if (!pmu->name) goto err;
- /* Read type, and ensure that 1 value (type) is successfully assigned */
Maybe I don't understand well, seems to me a better comment is:
/* Read type, and ensure that type value is successfully assigned (return 1) */
The rest looks good to me. With addressing above two comments:
Reviewed-by: Leo Yan leo.yan@linaro.org
- if (perf_pmu__scan_file(pmu, "type", "%u", &type) != 1)
goto err;- alias_name = pmu_find_alias_name(name); if (alias_name) { pmu->alias_name = strdup(alias_name);
@@ -1786,16 +1739,11 @@ bool pmu_have_event(const char *pname, const char *name) static FILE *perf_pmu__open_file(struct perf_pmu *pmu, const char *name) { char path[PATH_MAX];
- const char *sysfs;
- sysfs = sysfs__mountpoint();
- if (!sysfs)
- if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, pmu->name, name) ||
!file_available(path))
- snprintf(path, PATH_MAX,
"%s" EVENT_SOURCE_DEVICE_PATH "%s/%s", sysfs, pmu->name, name);- if (!file_available(path))
return NULL;} @@ -1849,7 +1797,6 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu) { struct stat st; char caps_path[PATH_MAX];
- const char *sysfs = sysfs__mountpoint(); DIR *caps_dir; struct dirent *evt_ent;
@@ -1858,12 +1805,9 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu) pmu->nr_caps = 0;
- if (!sysfs)
- if (!perf_pmu__pathname_scnprintf(caps_path, PATH_MAX, pmu->name, "caps")) return -1;
- snprintf(caps_path, PATH_MAX,
"%s" EVENT_SOURCE_DEVICE_PATH "%s/caps", sysfs, pmu->name);- if (stat(caps_path, &st) < 0) { pmu->caps_initialized = true; return 0; /* no error if caps does not exist */
@@ -1993,3 +1937,31 @@ int perf_pmu__cpus_match(struct perf_pmu *pmu, struct perf_cpu_map *cpus, *ucpus_ptr = unmatched_cpus; return 0; }
+int perf_pmu__event_source_devices_scnprintf(char *pathname, size_t size) +{
- const char *sysfs = sysfs__mountpoint();
- if (!sysfs)
return 0;- return scnprintf(pathname, size, "%s/bus/event_source/devices/", sysfs);
+}
+/*
- Fill 'buf' with the path to a file or folder in 'pmu_name' in
- sysfs. For example if pmu_name = "cs_etm" and 'filename' = "format"
- then pathname will be filled with
- "/sys/bus/event_source/devices/cs_etm/format"
- Return 0 if the sysfs mountpoint couldn't be found or if no
- characters were written.
- */
+int perf_pmu__pathname_scnprintf(char *buf, size_t size,
const char *pmu_name, const char *filename)+{
- char base_path[PATH_MAX];
- if (!perf_pmu__event_source_devices_scnprintf(base_path, PATH_MAX))
return 0;- return scnprintf(buf, size, "%s%s/%s", base_path, pmu_name, filename);
+} diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h index 69ca0004f94f..2f2bb0286e2a 100644 --- a/tools/perf/util/pmu.h +++ b/tools/perf/util/pmu.h @@ -22,7 +22,6 @@ enum { }; #define PERF_PMU_FORMAT_BITS 64 -#define EVENT_SOURCE_DEVICE_PATH "/bus/event_source/devices/" #define CPUS_TEMPLATE_CPU "%s/bus/event_source/devices/%s/cpus" #define MAX_PMU_NAME_LEN 128 @@ -259,4 +258,8 @@ int perf_pmu__cpus_match(struct perf_pmu *pmu, struct perf_cpu_map *cpus, char *pmu_find_real_name(const char *name); 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);#endif /* __PMU_H */
2.25.1
On 23/12/2022 02:39, Leo Yan wrote:
On Thu, Dec 22, 2022 at 04:03:21PM +0000, James Clark wrote:
The pattern for accessing EVENT_SOURCE_DEVICE_PATH is duplicated in a few places, so add two utility functions to cover it. Also just use perf_pmu__scan_file() instead of pmu_type() which already does the same thing.
No functional changes.
After I read the article: https://lwn.net/Articles/69419/, good to see this patch uses scnprintf() to replace snprintf().
Signed-off-by: James Clark james.clark@arm.com
tools/perf/arch/arm/util/auxtrace.c | 5 +- tools/perf/arch/x86/util/pmu.c | 12 +-- tools/perf/util/pmu.c | 110 +++++++++++----------------- tools/perf/util/pmu.h | 5 +- 4 files changed, 49 insertions(+), 83 deletions(-)
diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm/util/auxtrace.c index deeb163999ce..3cb4a6a16112 100644 --- a/tools/perf/arch/arm/util/auxtrace.c +++ b/tools/perf/arch/arm/util/auxtrace.c @@ -55,17 +55,16 @@ static struct perf_pmu **find_all_arm_spe_pmus(int *nr_spes, int *err) static struct perf_pmu **find_all_hisi_ptt_pmus(int *nr_ptts, int *err) {
- const char *sysfs = sysfs__mountpoint(); struct perf_pmu **hisi_ptt_pmus = NULL; struct dirent *dent; char path[PATH_MAX]; DIR *dir = NULL; int idx = 0;
- snprintf(path, PATH_MAX, "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
- perf_pmu__event_source_devices_scnprintf(path, PATH_MAX);
Nitpick: since 'path' is an array, a good practice is to use 'sizeof(path)' rather than 'PATH_MAX' for passing the length.
Done
[...]
@@ -983,9 +931,14 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name) pmu->cpus = pmu_cpumask(name); pmu->name = strdup(name);
- if (!pmu->name) goto err;
- /* Read type, and ensure that 1 value (type) is successfully assigned */
Maybe I don't understand well, seems to me a better comment is:
/* Read type, and ensure that type value is successfully assigned (return 1) */
Changed it in V3. I was trying to portray that the function returns how many variables were successfully assigned, which is slightly different to 1 == success. But it's probably clearer your way.
The rest looks good to me. With addressing above two comments:
Reviewed-by: Leo Yan leo.yan@linaro.org
Thanks for the review.
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)) 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 */
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",
- 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);@@ -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",
- 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
Use the new perf_pmu__pathname_scnprintf() instead. No functional changes.
Signed-off-by: James Clark james.clark@arm.com --- tools/perf/util/pmu.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index faaeec1e15aa..15b852b3c401 100644 --- a/tools/perf/util/pmu.c +++ b/tools/perf/util/pmu.c @@ -574,8 +574,6 @@ static void pmu_read_sysfs(void) * 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" - static struct perf_cpu_map *pmu_cpumask(char *name) { struct perf_cpu_map *cpus; @@ -616,9 +614,9 @@ static char *pmu_id(const char *name) char path[PATH_MAX], *str; size_t len;
- snprintf(path, PATH_MAX, SYS_TEMPLATE_ID, name); + perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "identifier");
- if (sysfs__read_str(path, &str, &len) < 0) + if (filename__read_str(path, &str, &len) < 0) return NULL;
str[len - 1] = 0; /* remove line feed */ @@ -864,16 +862,11 @@ pmu_find_alias_name(const char *name __maybe_unused) return NULL; }
-static int pmu_max_precise(const char *name) +static int pmu_max_precise(struct perf_pmu *pmu) { - char path[PATH_MAX]; int max_precise = -1;
- scnprintf(path, PATH_MAX, - "bus/event_source/devices/%s/caps/max_precise", - name); - - sysfs__read_int(path, &max_precise); + perf_pmu__scan_file(pmu, "caps/max_precise", "%d", &max_precise); return max_precise; }
@@ -932,7 +925,7 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name) pmu->is_uncore = pmu_is_uncore(name); if (pmu->is_uncore) pmu->id = pmu_id(name); - pmu->max_precise = pmu_max_precise(name); + pmu->max_precise = pmu_max_precise(pmu); pmu_add_cpu_aliases(&aliases, pmu); pmu_add_sys_aliases(&aliases, pmu);
On Thu, Dec 22, 2022 at 04:03:23PM +0000, James Clark wrote:
Use the new perf_pmu__pathname_scnprintf() instead. No functional changes.
Signed-off-by: James Clark james.clark@arm.com
Reviewed-by: Leo Yan leo.yan@linaro.org
tools/perf/util/pmu.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index faaeec1e15aa..15b852b3c401 100644 --- a/tools/perf/util/pmu.c +++ b/tools/perf/util/pmu.c @@ -574,8 +574,6 @@ static void pmu_read_sysfs(void)
- 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"
static struct perf_cpu_map *pmu_cpumask(char *name) { struct perf_cpu_map *cpus; @@ -616,9 +614,9 @@ static char *pmu_id(const char *name) char path[PATH_MAX], *str; size_t len;
- snprintf(path, PATH_MAX, SYS_TEMPLATE_ID, name);
- perf_pmu__pathname_scnprintf(path, PATH_MAX, name, "identifier");
- if (sysfs__read_str(path, &str, &len) < 0)
- if (filename__read_str(path, &str, &len) < 0) return NULL;
str[len - 1] = 0; /* remove line feed */ @@ -864,16 +862,11 @@ pmu_find_alias_name(const char *name __maybe_unused) return NULL; } -static int pmu_max_precise(const char *name) +static int pmu_max_precise(struct perf_pmu *pmu) {
- char path[PATH_MAX]; int max_precise = -1;
- scnprintf(path, PATH_MAX,
"bus/event_source/devices/%s/caps/max_precise",name);- sysfs__read_int(path, &max_precise);
- perf_pmu__scan_file(pmu, "caps/max_precise", "%d", &max_precise); return max_precise;
} @@ -932,7 +925,7 @@ static struct perf_pmu *pmu_lookup(const char *lookup_name) pmu->is_uncore = pmu_is_uncore(name); if (pmu->is_uncore) pmu->id = pmu_id(name);
- pmu->max_precise = pmu_max_precise(name);
- pmu->max_precise = pmu_max_precise(pmu); pmu_add_cpu_aliases(&aliases, pmu); pmu_add_sys_aliases(&aliases, pmu);
2.25.1
From: German Gomez german.gomez@arm.com
Add a utility function perf_pmu__file_exists() to check if a given pmu file exists in the sysfs filesystem.
Signed-off-by: German Gomez german.gomez@arm.com Signed-off-by: James Clark james.clark@arm.com --- tools/perf/util/pmu.c | 14 ++++++++++++++ tools/perf/util/pmu.h | 2 ++ 2 files changed, 16 insertions(+)
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index 15b852b3c401..b72b2d892949 100644 --- a/tools/perf/util/pmu.c +++ b/tools/perf/util/pmu.c @@ -1739,6 +1739,20 @@ int perf_pmu__scan_file(struct perf_pmu *pmu, const char *name, const char *fmt, return ret; }
+bool perf_pmu__file_exists(struct perf_pmu *pmu, const char *name) +{ + char path[PATH_MAX]; + struct stat statbuf; + + if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, pmu->name, name)) + return false; + + if (!file_available(path)) + return false; + + return stat(path, &statbuf) == 0; +} + static int perf_pmu__new_caps(struct list_head *list, char *name, char *value) { struct perf_pmu_caps *caps = zalloc(sizeof(*caps)); diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h index 8f39e2d17fb1..c1d138fe9602 100644 --- a/tools/perf/util/pmu.h +++ b/tools/perf/util/pmu.h @@ -230,6 +230,8 @@ bool pmu_have_event(const char *pname, const char *name);
int perf_pmu__scan_file(struct perf_pmu *pmu, const char *name, const char *fmt, ...) __scanf(3, 4);
+bool perf_pmu__file_exists(struct perf_pmu *pmu, const char *name); + int perf_pmu__test(void);
struct perf_event_attr *perf_pmu__get_default_config(struct perf_pmu *pmu);
On Thu, Dec 22, 2022 at 04:03:24PM +0000, James Clark wrote:
From: German Gomez german.gomez@arm.com
Add a utility function perf_pmu__file_exists() to check if a given pmu file exists in the sysfs filesystem.
Signed-off-by: German Gomez german.gomez@arm.com Signed-off-by: James Clark james.clark@arm.com
tools/perf/util/pmu.c | 14 ++++++++++++++ tools/perf/util/pmu.h | 2 ++ 2 files changed, 16 insertions(+)
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index 15b852b3c401..b72b2d892949 100644 --- a/tools/perf/util/pmu.c +++ b/tools/perf/util/pmu.c @@ -1739,6 +1739,20 @@ int perf_pmu__scan_file(struct perf_pmu *pmu, const char *name, const char *fmt, return ret; } +bool perf_pmu__file_exists(struct perf_pmu *pmu, const char *name) +{
- char path[PATH_MAX];
- struct stat statbuf;
- if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, pmu->name, name))
return false;- if (!file_available(path))
return false;- return stat(path, &statbuf) == 0;
Can we simply return the returned value from file_available() and skip calling stat()? Because file_available() invokes access() to detect if a file is existed or not, so here calling stat() is redundant.
Thanks, Leo
+}
static int perf_pmu__new_caps(struct list_head *list, char *name, char *value) { struct perf_pmu_caps *caps = zalloc(sizeof(*caps)); diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h index 8f39e2d17fb1..c1d138fe9602 100644 --- a/tools/perf/util/pmu.h +++ b/tools/perf/util/pmu.h @@ -230,6 +230,8 @@ bool pmu_have_event(const char *pname, const char *name); int perf_pmu__scan_file(struct perf_pmu *pmu, const char *name, const char *fmt, ...) __scanf(3, 4); +bool perf_pmu__file_exists(struct perf_pmu *pmu, const char *name);
int perf_pmu__test(void); struct perf_event_attr *perf_pmu__get_default_config(struct perf_pmu *pmu); -- 2.25.1
On 23/12/2022 06:26, Leo Yan wrote:
On Thu, Dec 22, 2022 at 04:03:24PM +0000, James Clark wrote:
From: German Gomez german.gomez@arm.com
Add a utility function perf_pmu__file_exists() to check if a given pmu file exists in the sysfs filesystem.
Signed-off-by: German Gomez german.gomez@arm.com Signed-off-by: James Clark james.clark@arm.com
tools/perf/util/pmu.c | 14 ++++++++++++++ tools/perf/util/pmu.h | 2 ++ 2 files changed, 16 insertions(+)
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index 15b852b3c401..b72b2d892949 100644 --- a/tools/perf/util/pmu.c +++ b/tools/perf/util/pmu.c @@ -1739,6 +1739,20 @@ int perf_pmu__scan_file(struct perf_pmu *pmu, const char *name, const char *fmt, return ret; } +bool perf_pmu__file_exists(struct perf_pmu *pmu, const char *name) +{
- char path[PATH_MAX];
- struct stat statbuf;
- if (!perf_pmu__pathname_scnprintf(path, PATH_MAX, pmu->name, name))
return false;- if (!file_available(path))
return false;- return stat(path, &statbuf) == 0;
Can we simply return the returned value from file_available() and skip calling stat()? Because file_available() invokes access() to detect if a file is existed or not, so here calling stat() is redundant.
Yep that works. Fixed in V3.
From: German Gomez german.gomez@arm.com
Previously, adding a new parameter at the end of ETMv4 meant adding it somewhere in the middle of ETE, which is not supported by the current header version.
Signed-off-by: German Gomez german.gomez@arm.com Signed-off-by: James Clark james.clark@arm.com --- tools/perf/arch/arm/util/cs-etm.c | 43 ++++++++++++++++++++++++++----- tools/perf/util/cs-etm-base.c | 32 +++++++++++++++++------ tools/perf/util/cs-etm.c | 12 ++++----- tools/perf/util/cs-etm.h | 11 +++++++- 4 files changed, 76 insertions(+), 22 deletions(-)
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index a346d5f3dafa..b526ffe550a5 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -53,7 +53,15 @@ static const char * const metadata_etmv4_ro[] = { [CS_ETMV4_TRCIDR2] = "trcidr/trcidr2", [CS_ETMV4_TRCIDR8] = "trcidr/trcidr8", [CS_ETMV4_TRCAUTHSTATUS] = "mgmt/trcauthstatus", - [CS_ETE_TRCDEVARCH] = "mgmt/trcdevarch" +}; + +static const char * const metadata_ete_ro[] = { + [CS_ETE_TRCIDR0] = "trcidr/trcidr0", + [CS_ETE_TRCIDR1] = "trcidr/trcidr1", + [CS_ETE_TRCIDR2] = "trcidr/trcidr2", + [CS_ETE_TRCIDR8] = "trcidr/trcidr8", + [CS_ETE_TRCAUTHSTATUS] = "mgmt/trcauthstatus", + [CS_ETE_TRCDEVARCH] = "mgmt/trcdevarch", };
static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu); @@ -617,7 +625,7 @@ static bool cs_etm_is_ete(struct auxtrace_record *itr, int cpu) { struct cs_etm_recording *ptr = container_of(itr, struct cs_etm_recording, itr); struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu; - int trcdevarch = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETE_TRCDEVARCH]); + int trcdevarch = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_ete_ro[CS_ETE_TRCDEVARCH]);
/* * ETE if ARCHVER is 5 (ARCHVER is 4 for ETM) and ARCHPART is 0xA13. @@ -648,6 +656,31 @@ static void cs_etm_save_etmv4_header(__u64 data[], struct auxtrace_record *itr, metadata_etmv4_ro[CS_ETMV4_TRCAUTHSTATUS]); }
+static void cs_etm_save_ete_header(__u64 data[], struct auxtrace_record *itr, int cpu) +{ + struct cs_etm_recording *ptr = container_of(itr, struct cs_etm_recording, itr); + struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu; + + /* Get trace configuration register */ + data[CS_ETE_TRCCONFIGR] = cs_etmv4_get_config(itr); + /* Get traceID from the framework */ + data[CS_ETE_TRCTRACEIDR] = coresight_get_trace_id(cpu); + /* Get read-only information from sysFS */ + data[CS_ETE_TRCIDR0] = cs_etm_get_ro(cs_etm_pmu, cpu, + metadata_ete_ro[CS_ETE_TRCIDR0]); + data[CS_ETE_TRCIDR1] = cs_etm_get_ro(cs_etm_pmu, cpu, + metadata_ete_ro[CS_ETE_TRCIDR1]); + data[CS_ETE_TRCIDR2] = cs_etm_get_ro(cs_etm_pmu, cpu, + metadata_ete_ro[CS_ETE_TRCIDR2]); + data[CS_ETE_TRCIDR8] = cs_etm_get_ro(cs_etm_pmu, cpu, + metadata_ete_ro[CS_ETE_TRCIDR8]); + data[CS_ETE_TRCAUTHSTATUS] = cs_etm_get_ro(cs_etm_pmu, cpu, + metadata_ete_ro[CS_ETE_TRCAUTHSTATUS]); + /* ETE uses the same registers as ETMv4 plus TRCDEVARCH */ + data[CS_ETE_TRCDEVARCH] = cs_etm_get_ro(cs_etm_pmu, cpu, + metadata_ete_ro[CS_ETE_TRCDEVARCH]); +} + static void cs_etm_get_metadata(int cpu, u32 *offset, struct auxtrace_record *itr, struct perf_record_auxtrace_info *info) @@ -661,11 +694,7 @@ static void cs_etm_get_metadata(int cpu, u32 *offset, /* first see what kind of tracer this cpu is affined to */ if (cs_etm_is_ete(itr, cpu)) { magic = __perf_cs_ete_magic; - /* ETE uses the same registers as ETMv4 plus TRCDEVARCH */ - cs_etm_save_etmv4_header(&info->priv[*offset], itr, cpu); - info->priv[*offset + CS_ETE_TRCDEVARCH] = - cs_etm_get_ro(cs_etm_pmu, cpu, - metadata_etmv4_ro[CS_ETE_TRCDEVARCH]); + cs_etm_save_ete_header(&info->priv[*offset], itr, cpu);
/* How much space was used */ increment = CS_ETE_PRIV_MAX; diff --git a/tools/perf/util/cs-etm-base.c b/tools/perf/util/cs-etm-base.c index 597542410854..282042c6e944 100644 --- a/tools/perf/util/cs-etm-base.c +++ b/tools/perf/util/cs-etm-base.c @@ -36,7 +36,20 @@ static const char * const cs_etmv4_priv_fmts[] = { [CS_ETMV4_TRCIDR2] = " TRCIDR2 %llx\n", [CS_ETMV4_TRCIDR8] = " TRCIDR8 %llx\n", [CS_ETMV4_TRCAUTHSTATUS] = " TRCAUTHSTATUS %llx\n", - [CS_ETE_TRCDEVARCH] = " TRCDEVARCH %llx\n" +}; + +static const char * const cs_ete_priv_fmts[] = { + [CS_ETM_MAGIC] = " Magic number %llx\n", + [CS_ETM_CPU] = " CPU %lld\n", + [CS_ETM_NR_TRC_PARAMS] = " NR_TRC_PARAMS %llx\n", + [CS_ETE_TRCCONFIGR] = " TRCCONFIGR %llx\n", + [CS_ETE_TRCTRACEIDR] = " TRCTRACEIDR %llx\n", + [CS_ETE_TRCIDR0] = " TRCIDR0 %llx\n", + [CS_ETE_TRCIDR1] = " TRCIDR1 %llx\n", + [CS_ETE_TRCIDR2] = " TRCIDR2 %llx\n", + [CS_ETE_TRCIDR8] = " TRCIDR8 %llx\n", + [CS_ETE_TRCAUTHSTATUS] = " TRCAUTHSTATUS %llx\n", + [CS_ETE_TRCDEVARCH] = " TRCDEVARCH %llx\n", };
static const char * const param_unk_fmt = @@ -96,19 +109,22 @@ static int cs_etm__print_cpu_metadata_v1(u64 *val, int *offset) else fprintf(stdout, cs_etm_priv_fmts[j], val[i]); } - } else if (magic == __perf_cs_etmv4_magic || magic == __perf_cs_ete_magic) { - /* - * ETE and ETMv4 can be printed in the same block because the number of parameters - * is saved and they share the list of parameter names. ETE is also only supported - * in V1 files. - */ + } else if (magic == __perf_cs_etmv4_magic) { for (j = 0; j < total_params; j++, i++) { /* if newer record - could be excess params */ - if (j >= CS_ETE_PRIV_MAX) + if (j >= CS_ETMV4_PRIV_MAX) fprintf(stdout, param_unk_fmt, j, val[i]); else fprintf(stdout, cs_etmv4_priv_fmts[j], val[i]); } + } else if (magic == __perf_cs_ete_magic) { + for (j = 0; j < total_params; j++, i++) { + /* if newer record - could be excess params */ + if (j >= CS_ETE_PRIV_MAX) + fprintf(stdout, param_unk_fmt, j, val[i]); + else + fprintf(stdout, cs_ete_priv_fmts[j], val[i]); + } } else { /* failure - note bad magic value and error out */ fprintf(stdout, magic_unk_fmt, magic); diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 33303d03c2fa..879576d5f899 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -464,12 +464,12 @@ static void cs_etm__set_trace_param_ete(struct cs_etm_trace_params *t_params, u64 **metadata = etm->metadata;
t_params[idx].protocol = CS_ETM_PROTO_ETE; - t_params[idx].ete.reg_idr0 = metadata[idx][CS_ETMV4_TRCIDR0]; - t_params[idx].ete.reg_idr1 = metadata[idx][CS_ETMV4_TRCIDR1]; - t_params[idx].ete.reg_idr2 = metadata[idx][CS_ETMV4_TRCIDR2]; - t_params[idx].ete.reg_idr8 = metadata[idx][CS_ETMV4_TRCIDR8]; - t_params[idx].ete.reg_configr = metadata[idx][CS_ETMV4_TRCCONFIGR]; - t_params[idx].ete.reg_traceidr = metadata[idx][CS_ETMV4_TRCTRACEIDR]; + t_params[idx].ete.reg_idr0 = metadata[idx][CS_ETE_TRCIDR0]; + t_params[idx].ete.reg_idr1 = metadata[idx][CS_ETE_TRCIDR1]; + t_params[idx].ete.reg_idr2 = metadata[idx][CS_ETE_TRCIDR2]; + t_params[idx].ete.reg_idr8 = metadata[idx][CS_ETE_TRCIDR8]; + t_params[idx].ete.reg_configr = metadata[idx][CS_ETE_TRCCONFIGR]; + t_params[idx].ete.reg_traceidr = metadata[idx][CS_ETE_TRCTRACEIDR]; t_params[idx].ete.reg_devarch = metadata[idx][CS_ETE_TRCDEVARCH]; }
diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h index 5da50d5dae6b..c5925428afa9 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -82,7 +82,16 @@ enum { * added in header V1 */ enum { - CS_ETE_TRCDEVARCH = CS_ETMV4_PRIV_MAX, + /* Dynamic, configurable parameters */ + CS_ETE_TRCCONFIGR = CS_ETM_COMMON_BLK_MAX_V1, + CS_ETE_TRCTRACEIDR, + /* RO, taken from sysFS */ + CS_ETE_TRCIDR0, + CS_ETE_TRCIDR1, + CS_ETE_TRCIDR2, + CS_ETE_TRCIDR8, + CS_ETE_TRCAUTHSTATUS, + CS_ETE_TRCDEVARCH, CS_ETE_PRIV_MAX };
On Thu, 22 Dec 2022 at 16:07, James Clark james.clark@arm.com wrote:
From: German Gomez german.gomez@arm.com
Previously, adding a new parameter at the end of ETMv4 meant adding it somewhere in the middle of ETE, which is not supported by the current header version.
Signed-off-by: German Gomez german.gomez@arm.com Signed-off-by: James Clark james.clark@arm.com
tools/perf/arch/arm/util/cs-etm.c | 43 ++++++++++++++++++++++++++----- tools/perf/util/cs-etm-base.c | 32 +++++++++++++++++------ tools/perf/util/cs-etm.c | 12 ++++----- tools/perf/util/cs-etm.h | 11 +++++++- 4 files changed, 76 insertions(+), 22 deletions(-)
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index a346d5f3dafa..b526ffe550a5 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -53,7 +53,15 @@ static const char * const metadata_etmv4_ro[] = { [CS_ETMV4_TRCIDR2] = "trcidr/trcidr2", [CS_ETMV4_TRCIDR8] = "trcidr/trcidr8", [CS_ETMV4_TRCAUTHSTATUS] = "mgmt/trcauthstatus",
[CS_ETE_TRCDEVARCH] = "mgmt/trcdevarch"+};
+static const char * const metadata_ete_ro[] = {
[CS_ETE_TRCIDR0] = "trcidr/trcidr0",[CS_ETE_TRCIDR1] = "trcidr/trcidr1",[CS_ETE_TRCIDR2] = "trcidr/trcidr2",[CS_ETE_TRCIDR8] = "trcidr/trcidr8",[CS_ETE_TRCAUTHSTATUS] = "mgmt/trcauthstatus",[CS_ETE_TRCDEVARCH] = "mgmt/trcdevarch",};
static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu); @@ -617,7 +625,7 @@ static bool cs_etm_is_ete(struct auxtrace_record *itr, int cpu) { struct cs_etm_recording *ptr = container_of(itr, struct cs_etm_recording, itr); struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
int trcdevarch = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETE_TRCDEVARCH]);
int trcdevarch = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_ete_ro[CS_ETE_TRCDEVARCH]); /* * ETE if ARCHVER is 5 (ARCHVER is 4 for ETM) and ARCHPART is 0xA13.@@ -648,6 +656,31 @@ static void cs_etm_save_etmv4_header(__u64 data[], struct auxtrace_record *itr, metadata_etmv4_ro[CS_ETMV4_TRCAUTHSTATUS]); }
+static void cs_etm_save_ete_header(__u64 data[], struct auxtrace_record *itr, int cpu) +{
struct cs_etm_recording *ptr = container_of(itr, struct cs_etm_recording, itr);struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;/* Get trace configuration register */data[CS_ETE_TRCCONFIGR] = cs_etmv4_get_config(itr);/* Get traceID from the framework */data[CS_ETE_TRCTRACEIDR] = coresight_get_trace_id(cpu);/* Get read-only information from sysFS */data[CS_ETE_TRCIDR0] = cs_etm_get_ro(cs_etm_pmu, cpu,metadata_ete_ro[CS_ETE_TRCIDR0]);data[CS_ETE_TRCIDR1] = cs_etm_get_ro(cs_etm_pmu, cpu,metadata_ete_ro[CS_ETE_TRCIDR1]);data[CS_ETE_TRCIDR2] = cs_etm_get_ro(cs_etm_pmu, cpu,metadata_ete_ro[CS_ETE_TRCIDR2]);data[CS_ETE_TRCIDR8] = cs_etm_get_ro(cs_etm_pmu, cpu,metadata_ete_ro[CS_ETE_TRCIDR8]);data[CS_ETE_TRCAUTHSTATUS] = cs_etm_get_ro(cs_etm_pmu, cpu,metadata_ete_ro[CS_ETE_TRCAUTHSTATUS]);/* ETE uses the same registers as ETMv4 plus TRCDEVARCH */data[CS_ETE_TRCDEVARCH] = cs_etm_get_ro(cs_etm_pmu, cpu,metadata_ete_ro[CS_ETE_TRCDEVARCH]);+}
static void cs_etm_get_metadata(int cpu, u32 *offset, struct auxtrace_record *itr, struct perf_record_auxtrace_info *info) @@ -661,11 +694,7 @@ static void cs_etm_get_metadata(int cpu, u32 *offset, /* first see what kind of tracer this cpu is affined to */ if (cs_etm_is_ete(itr, cpu)) { magic = __perf_cs_ete_magic;
/* ETE uses the same registers as ETMv4 plus TRCDEVARCH */cs_etm_save_etmv4_header(&info->priv[*offset], itr, cpu);info->priv[*offset + CS_ETE_TRCDEVARCH] =cs_etm_get_ro(cs_etm_pmu, cpu,metadata_etmv4_ro[CS_ETE_TRCDEVARCH]);
cs_etm_save_ete_header(&info->priv[*offset], itr, cpu); /* How much space was used */ increment = CS_ETE_PRIV_MAX;diff --git a/tools/perf/util/cs-etm-base.c b/tools/perf/util/cs-etm-base.c index 597542410854..282042c6e944 100644 --- a/tools/perf/util/cs-etm-base.c +++ b/tools/perf/util/cs-etm-base.c @@ -36,7 +36,20 @@ static const char * const cs_etmv4_priv_fmts[] = { [CS_ETMV4_TRCIDR2] = " TRCIDR2 %llx\n", [CS_ETMV4_TRCIDR8] = " TRCIDR8 %llx\n", [CS_ETMV4_TRCAUTHSTATUS] = " TRCAUTHSTATUS %llx\n",
[CS_ETE_TRCDEVARCH] = " TRCDEVARCH %llx\n"+};
+static const char * const cs_ete_priv_fmts[] = {
[CS_ETM_MAGIC] = " Magic number %llx\n",[CS_ETM_CPU] = " CPU %lld\n",[CS_ETM_NR_TRC_PARAMS] = " NR_TRC_PARAMS %llx\n",[CS_ETE_TRCCONFIGR] = " TRCCONFIGR %llx\n",[CS_ETE_TRCTRACEIDR] = " TRCTRACEIDR %llx\n",[CS_ETE_TRCIDR0] = " TRCIDR0 %llx\n",[CS_ETE_TRCIDR1] = " TRCIDR1 %llx\n",[CS_ETE_TRCIDR2] = " TRCIDR2 %llx\n",[CS_ETE_TRCIDR8] = " TRCIDR8 %llx\n",[CS_ETE_TRCAUTHSTATUS] = " TRCAUTHSTATUS %llx\n",[CS_ETE_TRCDEVARCH] = " TRCDEVARCH %llx\n",};
static const char * const param_unk_fmt = @@ -96,19 +109,22 @@ static int cs_etm__print_cpu_metadata_v1(u64 *val, int *offset) else fprintf(stdout, cs_etm_priv_fmts[j], val[i]); }
} else if (magic == __perf_cs_etmv4_magic || magic == __perf_cs_ete_magic) {/** ETE and ETMv4 can be printed in the same block because the number of parameters* is saved and they share the list of parameter names. ETE is also only supported* in V1 files.*/
} else if (magic == __perf_cs_etmv4_magic) { for (j = 0; j < total_params; j++, i++) { /* if newer record - could be excess params */
if (j >= CS_ETE_PRIV_MAX)
if (j >= CS_ETMV4_PRIV_MAX) fprintf(stdout, param_unk_fmt, j, val[i]); else fprintf(stdout, cs_etmv4_priv_fmts[j], val[i]); }} else if (magic == __perf_cs_ete_magic) {for (j = 0; j < total_params; j++, i++) {/* if newer record - could be excess params */if (j >= CS_ETE_PRIV_MAX)fprintf(stdout, param_unk_fmt, j, val[i]);elsefprintf(stdout, cs_ete_priv_fmts[j], val[i]);} } else { /* failure - note bad magic value and error out */ fprintf(stdout, magic_unk_fmt, magic);diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 33303d03c2fa..879576d5f899 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -464,12 +464,12 @@ static void cs_etm__set_trace_param_ete(struct cs_etm_trace_params *t_params, u64 **metadata = etm->metadata;
t_params[idx].protocol = CS_ETM_PROTO_ETE;
t_params[idx].ete.reg_idr0 = metadata[idx][CS_ETMV4_TRCIDR0];t_params[idx].ete.reg_idr1 = metadata[idx][CS_ETMV4_TRCIDR1];t_params[idx].ete.reg_idr2 = metadata[idx][CS_ETMV4_TRCIDR2];t_params[idx].ete.reg_idr8 = metadata[idx][CS_ETMV4_TRCIDR8];t_params[idx].ete.reg_configr = metadata[idx][CS_ETMV4_TRCCONFIGR];t_params[idx].ete.reg_traceidr = metadata[idx][CS_ETMV4_TRCTRACEIDR];
t_params[idx].ete.reg_idr0 = metadata[idx][CS_ETE_TRCIDR0];t_params[idx].ete.reg_idr1 = metadata[idx][CS_ETE_TRCIDR1];t_params[idx].ete.reg_idr2 = metadata[idx][CS_ETE_TRCIDR2];t_params[idx].ete.reg_idr8 = metadata[idx][CS_ETE_TRCIDR8];t_params[idx].ete.reg_configr = metadata[idx][CS_ETE_TRCCONFIGR];t_params[idx].ete.reg_traceidr = metadata[idx][CS_ETE_TRCTRACEIDR]; t_params[idx].ete.reg_devarch = metadata[idx][CS_ETE_TRCDEVARCH];}
diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h index 5da50d5dae6b..c5925428afa9 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -82,7 +82,16 @@ enum {
- added in header V1
*/ enum {
CS_ETE_TRCDEVARCH = CS_ETMV4_PRIV_MAX,
/* Dynamic, configurable parameters */CS_ETE_TRCCONFIGR = CS_ETM_COMMON_BLK_MAX_V1,CS_ETE_TRCTRACEIDR,/* RO, taken from sysFS */CS_ETE_TRCIDR0,CS_ETE_TRCIDR1,CS_ETE_TRCIDR2,CS_ETE_TRCIDR8,CS_ETE_TRCAUTHSTATUS,CS_ETE_TRCDEVARCH, CS_ETE_PRIV_MAX};
-- 2.25.1
Reviewed-by: Mike Leach mike.leach@linaro.org
From: German Gomez german.gomez@arm.com
Read the value of ts_source exposed by the driver and store it in the ETMv4 and ETE header. If the interface doesn't exist (such as in older Kernels), defaults to a safe value of -1.
Signed-off-by: German Gomez german.gomez@arm.com Signed-off-by: James Clark james.clark@arm.com --- tools/perf/arch/arm/util/cs-etm.c | 48 +++++++++++++++++++++++++++++++ tools/perf/util/cs-etm-base.c | 2 ++ tools/perf/util/cs-etm.h | 2 ++ 3 files changed, 52 insertions(+)
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index b526ffe550a5..481e170cd3f1 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -53,6 +53,7 @@ static const char * const metadata_etmv4_ro[] = { [CS_ETMV4_TRCIDR2] = "trcidr/trcidr2", [CS_ETMV4_TRCIDR8] = "trcidr/trcidr8", [CS_ETMV4_TRCAUTHSTATUS] = "mgmt/trcauthstatus", + [CS_ETMV4_TS_SOURCE] = "ts_source", };
static const char * const metadata_ete_ro[] = { @@ -62,6 +63,7 @@ static const char * const metadata_ete_ro[] = { [CS_ETE_TRCIDR8] = "trcidr/trcidr8", [CS_ETE_TRCAUTHSTATUS] = "mgmt/trcauthstatus", [CS_ETE_TRCDEVARCH] = "mgmt/trcdevarch", + [CS_ETE_TS_SOURCE] = "ts_source", };
static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu); @@ -613,6 +615,32 @@ static int cs_etm_get_ro(struct perf_pmu *pmu, int cpu, const char *path) return val; }
+static int cs_etm_get_ro_signed(struct perf_pmu *pmu, int cpu, const char *path) +{ + char pmu_path[PATH_MAX]; + int scan; + int val = 0; + + /* Get RO metadata from sysfs */ + snprintf(pmu_path, PATH_MAX, "cpu%d/%s", cpu, path); + + scan = perf_pmu__scan_file(pmu, pmu_path, "%d", &val); + if (scan != 1) + pr_err("%s: error reading: %s\n", __func__, pmu_path); + + return val; +} + +static bool cs_etm_pmu_path_exists(struct perf_pmu *pmu, int cpu, const char *path) +{ + char pmu_path[PATH_MAX]; + + /* Get RO metadata from sysfs */ + snprintf(pmu_path, PATH_MAX, "cpu%d/%s", cpu, path); + + return perf_pmu__file_exists(pmu, pmu_path); +} + #define TRCDEVARCH_ARCHPART_SHIFT 0 #define TRCDEVARCH_ARCHPART_MASK GENMASK(11, 0) #define TRCDEVARCH_ARCHPART(x) (((x) & TRCDEVARCH_ARCHPART_MASK) >> TRCDEVARCH_ARCHPART_SHIFT) @@ -654,6 +682,16 @@ static void cs_etm_save_etmv4_header(__u64 data[], struct auxtrace_record *itr, metadata_etmv4_ro[CS_ETMV4_TRCIDR8]); data[CS_ETMV4_TRCAUTHSTATUS] = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TRCAUTHSTATUS]); + + /* Kernels older than 5.19 may not expose ts_source */ + if (cs_etm_pmu_path_exists(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TS_SOURCE])) + data[CS_ETMV4_TS_SOURCE] = (__u64) cs_etm_get_ro_signed(cs_etm_pmu, cpu, + metadata_etmv4_ro[CS_ETMV4_TS_SOURCE]); + else { + pr_warning("[%03d] pmu file 'ts_source' not found. Fallback to safe value (-1)\n", + cpu); + data[CS_ETMV4_TS_SOURCE] = (__u64) -1; + } }
static void cs_etm_save_ete_header(__u64 data[], struct auxtrace_record *itr, int cpu) @@ -679,6 +717,16 @@ static void cs_etm_save_ete_header(__u64 data[], struct auxtrace_record *itr, in /* ETE uses the same registers as ETMv4 plus TRCDEVARCH */ data[CS_ETE_TRCDEVARCH] = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_ete_ro[CS_ETE_TRCDEVARCH]); + + /* Kernels older than 5.19 may not expose ts_source */ + if (cs_etm_pmu_path_exists(cs_etm_pmu, cpu, metadata_ete_ro[CS_ETE_TS_SOURCE])) + data[CS_ETE_TS_SOURCE] = (__u64) cs_etm_get_ro_signed(cs_etm_pmu, cpu, + metadata_ete_ro[CS_ETE_TS_SOURCE]); + else { + pr_warning("[%03d] pmu file 'ts_source' not found. Fallback to safe value (-1)\n", + cpu); + data[CS_ETE_TS_SOURCE] = (__u64) -1; + } }
static void cs_etm_get_metadata(int cpu, u32 *offset, diff --git a/tools/perf/util/cs-etm-base.c b/tools/perf/util/cs-etm-base.c index 282042c6e944..5f48b756c4cf 100644 --- a/tools/perf/util/cs-etm-base.c +++ b/tools/perf/util/cs-etm-base.c @@ -36,6 +36,7 @@ static const char * const cs_etmv4_priv_fmts[] = { [CS_ETMV4_TRCIDR2] = " TRCIDR2 %llx\n", [CS_ETMV4_TRCIDR8] = " TRCIDR8 %llx\n", [CS_ETMV4_TRCAUTHSTATUS] = " TRCAUTHSTATUS %llx\n", + [CS_ETMV4_TS_SOURCE] = " TS_SOURCE %lld\n", };
static const char * const cs_ete_priv_fmts[] = { @@ -50,6 +51,7 @@ static const char * const cs_ete_priv_fmts[] = { [CS_ETE_TRCIDR8] = " TRCIDR8 %llx\n", [CS_ETE_TRCAUTHSTATUS] = " TRCAUTHSTATUS %llx\n", [CS_ETE_TRCDEVARCH] = " TRCDEVARCH %llx\n", + [CS_ETE_TS_SOURCE] = " TS_SOURCE %lld\n", };
static const char * const param_unk_fmt = diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h index c5925428afa9..ad790930bcbc 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -71,6 +71,7 @@ enum { CS_ETMV4_TRCIDR2, CS_ETMV4_TRCIDR8, CS_ETMV4_TRCAUTHSTATUS, + CS_ETMV4_TS_SOURCE, CS_ETMV4_PRIV_MAX, };
@@ -92,6 +93,7 @@ enum { CS_ETE_TRCIDR8, CS_ETE_TRCAUTHSTATUS, CS_ETE_TRCDEVARCH, + CS_ETE_TS_SOURCE, CS_ETE_PRIV_MAX };
Hi,
There was a discussion some time ago in one of our coresight regular dev meetings about this.
Can we just use only the necessary bits that TS source needs and leave the remaining bits from the 64 as unused for future expansion - i..e use this as a bitfield rather than have 64 bits occupied for what is effectively a 2 bit value.
Perhaps call the full value something other than TS_SOURCE and have a TS_SOURCE field with a known safe unset value.
Thanks
Mike
On Thu, 22 Dec 2022 at 16:07, James Clark james.clark@arm.com wrote:
From: German Gomez german.gomez@arm.com
Read the value of ts_source exposed by the driver and store it in the ETMv4 and ETE header. If the interface doesn't exist (such as in older Kernels), defaults to a safe value of -1.
Signed-off-by: German Gomez german.gomez@arm.com Signed-off-by: James Clark james.clark@arm.com
tools/perf/arch/arm/util/cs-etm.c | 48 +++++++++++++++++++++++++++++++ tools/perf/util/cs-etm-base.c | 2 ++ tools/perf/util/cs-etm.h | 2 ++ 3 files changed, 52 insertions(+)
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index b526ffe550a5..481e170cd3f1 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -53,6 +53,7 @@ static const char * const metadata_etmv4_ro[] = { [CS_ETMV4_TRCIDR2] = "trcidr/trcidr2", [CS_ETMV4_TRCIDR8] = "trcidr/trcidr8", [CS_ETMV4_TRCAUTHSTATUS] = "mgmt/trcauthstatus",
[CS_ETMV4_TS_SOURCE] = "ts_source",};
static const char * const metadata_ete_ro[] = { @@ -62,6 +63,7 @@ static const char * const metadata_ete_ro[] = { [CS_ETE_TRCIDR8] = "trcidr/trcidr8", [CS_ETE_TRCAUTHSTATUS] = "mgmt/trcauthstatus", [CS_ETE_TRCDEVARCH] = "mgmt/trcdevarch",
[CS_ETE_TS_SOURCE] = "ts_source",};
static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu); @@ -613,6 +615,32 @@ static int cs_etm_get_ro(struct perf_pmu *pmu, int cpu, const char *path) return val; }
+static int cs_etm_get_ro_signed(struct perf_pmu *pmu, int cpu, const char *path) +{
char pmu_path[PATH_MAX];int scan;int val = 0;/* Get RO metadata from sysfs */snprintf(pmu_path, PATH_MAX, "cpu%d/%s", cpu, path);scan = perf_pmu__scan_file(pmu, pmu_path, "%d", &val);if (scan != 1)pr_err("%s: error reading: %s\n", __func__, pmu_path);return val;+}
+static bool cs_etm_pmu_path_exists(struct perf_pmu *pmu, int cpu, const char *path) +{
char pmu_path[PATH_MAX];/* Get RO metadata from sysfs */snprintf(pmu_path, PATH_MAX, "cpu%d/%s", cpu, path);return perf_pmu__file_exists(pmu, pmu_path);+}
#define TRCDEVARCH_ARCHPART_SHIFT 0 #define TRCDEVARCH_ARCHPART_MASK GENMASK(11, 0) #define TRCDEVARCH_ARCHPART(x) (((x) & TRCDEVARCH_ARCHPART_MASK) >> TRCDEVARCH_ARCHPART_SHIFT) @@ -654,6 +682,16 @@ static void cs_etm_save_etmv4_header(__u64 data[], struct auxtrace_record *itr, metadata_etmv4_ro[CS_ETMV4_TRCIDR8]); data[CS_ETMV4_TRCAUTHSTATUS] = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TRCAUTHSTATUS]);
/* Kernels older than 5.19 may not expose ts_source */if (cs_etm_pmu_path_exists(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TS_SOURCE]))data[CS_ETMV4_TS_SOURCE] = (__u64) cs_etm_get_ro_signed(cs_etm_pmu, cpu,metadata_etmv4_ro[CS_ETMV4_TS_SOURCE]);else {pr_warning("[%03d] pmu file 'ts_source' not found. Fallback to safe value (-1)\n",cpu);data[CS_ETMV4_TS_SOURCE] = (__u64) -1;}}
static void cs_etm_save_ete_header(__u64 data[], struct auxtrace_record *itr, int cpu) @@ -679,6 +717,16 @@ static void cs_etm_save_ete_header(__u64 data[], struct auxtrace_record *itr, in /* ETE uses the same registers as ETMv4 plus TRCDEVARCH */ data[CS_ETE_TRCDEVARCH] = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_ete_ro[CS_ETE_TRCDEVARCH]);
/* Kernels older than 5.19 may not expose ts_source */if (cs_etm_pmu_path_exists(cs_etm_pmu, cpu, metadata_ete_ro[CS_ETE_TS_SOURCE]))data[CS_ETE_TS_SOURCE] = (__u64) cs_etm_get_ro_signed(cs_etm_pmu, cpu,metadata_ete_ro[CS_ETE_TS_SOURCE]);else {pr_warning("[%03d] pmu file 'ts_source' not found. Fallback to safe value (-1)\n",cpu);data[CS_ETE_TS_SOURCE] = (__u64) -1;}}
static void cs_etm_get_metadata(int cpu, u32 *offset, diff --git a/tools/perf/util/cs-etm-base.c b/tools/perf/util/cs-etm-base.c index 282042c6e944..5f48b756c4cf 100644 --- a/tools/perf/util/cs-etm-base.c +++ b/tools/perf/util/cs-etm-base.c @@ -36,6 +36,7 @@ static const char * const cs_etmv4_priv_fmts[] = { [CS_ETMV4_TRCIDR2] = " TRCIDR2 %llx\n", [CS_ETMV4_TRCIDR8] = " TRCIDR8 %llx\n", [CS_ETMV4_TRCAUTHSTATUS] = " TRCAUTHSTATUS %llx\n",
[CS_ETMV4_TS_SOURCE] = " TS_SOURCE %lld\n",};
static const char * const cs_ete_priv_fmts[] = { @@ -50,6 +51,7 @@ static const char * const cs_ete_priv_fmts[] = { [CS_ETE_TRCIDR8] = " TRCIDR8 %llx\n", [CS_ETE_TRCAUTHSTATUS] = " TRCAUTHSTATUS %llx\n", [CS_ETE_TRCDEVARCH] = " TRCDEVARCH %llx\n",
[CS_ETE_TS_SOURCE] = " TS_SOURCE %lld\n",};
static const char * const param_unk_fmt = diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h index c5925428afa9..ad790930bcbc 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -71,6 +71,7 @@ enum { CS_ETMV4_TRCIDR2, CS_ETMV4_TRCIDR8, CS_ETMV4_TRCAUTHSTATUS,
CS_ETMV4_TS_SOURCE, CS_ETMV4_PRIV_MAX,};
@@ -92,6 +93,7 @@ enum { CS_ETE_TRCIDR8, CS_ETE_TRCAUTHSTATUS, CS_ETE_TRCDEVARCH,
CS_ETE_TS_SOURCE, CS_ETE_PRIV_MAX};
-- 2.25.1
On 23/12/2022 09:28, Mike Leach wrote:
Hi,
There was a discussion some time ago in one of our coresight regular dev meetings about this.
Can we just use only the necessary bits that TS source needs and leave the remaining bits from the 64 as unused for future expansion - i..e use this as a bitfield rather than have 64 bits occupied for what is effectively a 2 bit value.
Perhaps call the full value something other than TS_SOURCE and have a TS_SOURCE field with a known safe unset value.
If we did that, there wouldn't be any mechanism to detect if new values that were added were the value 0, or just not set/implemented/saved in the file. The current implementation of CS_ETM_NR_TRC_PARAMS allows us to add new fields, and detect if they exist or not without bumping the header version for each new sub field.
In this change HAS_PARAM() has been used to do this, but that can't be expanded to sub fields in the same 64 bits. I don't think space or performance are worth the extra complexity of dividing it up. And because this is just one block saved once in the header, so I'm not sure if it's worth it in this case. It would also make it harder to read the values on the raw dump mode.
James
Thanks
Mike
On Thu, 22 Dec 2022 at 16:07, James Clark james.clark@arm.com wrote:
From: German Gomez german.gomez@arm.com
Read the value of ts_source exposed by the driver and store it in the ETMv4 and ETE header. If the interface doesn't exist (such as in older Kernels), defaults to a safe value of -1.
Signed-off-by: German Gomez german.gomez@arm.com Signed-off-by: James Clark james.clark@arm.com
tools/perf/arch/arm/util/cs-etm.c | 48 +++++++++++++++++++++++++++++++ tools/perf/util/cs-etm-base.c | 2 ++ tools/perf/util/cs-etm.h | 2 ++ 3 files changed, 52 insertions(+)
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index b526ffe550a5..481e170cd3f1 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -53,6 +53,7 @@ static const char * const metadata_etmv4_ro[] = { [CS_ETMV4_TRCIDR2] = "trcidr/trcidr2", [CS_ETMV4_TRCIDR8] = "trcidr/trcidr8", [CS_ETMV4_TRCAUTHSTATUS] = "mgmt/trcauthstatus",
[CS_ETMV4_TS_SOURCE] = "ts_source",};
static const char * const metadata_ete_ro[] = { @@ -62,6 +63,7 @@ static const char * const metadata_ete_ro[] = { [CS_ETE_TRCIDR8] = "trcidr/trcidr8", [CS_ETE_TRCAUTHSTATUS] = "mgmt/trcauthstatus", [CS_ETE_TRCDEVARCH] = "mgmt/trcdevarch",
[CS_ETE_TS_SOURCE] = "ts_source",};
static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu); @@ -613,6 +615,32 @@ static int cs_etm_get_ro(struct perf_pmu *pmu, int cpu, const char *path) return val; }
+static int cs_etm_get_ro_signed(struct perf_pmu *pmu, int cpu, const char *path) +{
char pmu_path[PATH_MAX];int scan;int val = 0;/* Get RO metadata from sysfs */snprintf(pmu_path, PATH_MAX, "cpu%d/%s", cpu, path);scan = perf_pmu__scan_file(pmu, pmu_path, "%d", &val);if (scan != 1)pr_err("%s: error reading: %s\n", __func__, pmu_path);return val;+}
+static bool cs_etm_pmu_path_exists(struct perf_pmu *pmu, int cpu, const char *path) +{
char pmu_path[PATH_MAX];/* Get RO metadata from sysfs */snprintf(pmu_path, PATH_MAX, "cpu%d/%s", cpu, path);return perf_pmu__file_exists(pmu, pmu_path);+}
#define TRCDEVARCH_ARCHPART_SHIFT 0 #define TRCDEVARCH_ARCHPART_MASK GENMASK(11, 0) #define TRCDEVARCH_ARCHPART(x) (((x) & TRCDEVARCH_ARCHPART_MASK) >> TRCDEVARCH_ARCHPART_SHIFT) @@ -654,6 +682,16 @@ static void cs_etm_save_etmv4_header(__u64 data[], struct auxtrace_record *itr, metadata_etmv4_ro[CS_ETMV4_TRCIDR8]); data[CS_ETMV4_TRCAUTHSTATUS] = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TRCAUTHSTATUS]);
/* Kernels older than 5.19 may not expose ts_source */if (cs_etm_pmu_path_exists(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TS_SOURCE]))data[CS_ETMV4_TS_SOURCE] = (__u64) cs_etm_get_ro_signed(cs_etm_pmu, cpu,metadata_etmv4_ro[CS_ETMV4_TS_SOURCE]);else {pr_warning("[%03d] pmu file 'ts_source' not found. Fallback to safe value (-1)\n",cpu);data[CS_ETMV4_TS_SOURCE] = (__u64) -1;}}
static void cs_etm_save_ete_header(__u64 data[], struct auxtrace_record *itr, int cpu) @@ -679,6 +717,16 @@ static void cs_etm_save_ete_header(__u64 data[], struct auxtrace_record *itr, in /* ETE uses the same registers as ETMv4 plus TRCDEVARCH */ data[CS_ETE_TRCDEVARCH] = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_ete_ro[CS_ETE_TRCDEVARCH]);
/* Kernels older than 5.19 may not expose ts_source */if (cs_etm_pmu_path_exists(cs_etm_pmu, cpu, metadata_ete_ro[CS_ETE_TS_SOURCE]))data[CS_ETE_TS_SOURCE] = (__u64) cs_etm_get_ro_signed(cs_etm_pmu, cpu,metadata_ete_ro[CS_ETE_TS_SOURCE]);else {pr_warning("[%03d] pmu file 'ts_source' not found. Fallback to safe value (-1)\n",cpu);data[CS_ETE_TS_SOURCE] = (__u64) -1;}}
static void cs_etm_get_metadata(int cpu, u32 *offset, diff --git a/tools/perf/util/cs-etm-base.c b/tools/perf/util/cs-etm-base.c index 282042c6e944..5f48b756c4cf 100644 --- a/tools/perf/util/cs-etm-base.c +++ b/tools/perf/util/cs-etm-base.c @@ -36,6 +36,7 @@ static const char * const cs_etmv4_priv_fmts[] = { [CS_ETMV4_TRCIDR2] = " TRCIDR2 %llx\n", [CS_ETMV4_TRCIDR8] = " TRCIDR8 %llx\n", [CS_ETMV4_TRCAUTHSTATUS] = " TRCAUTHSTATUS %llx\n",
[CS_ETMV4_TS_SOURCE] = " TS_SOURCE %lld\n",};
static const char * const cs_ete_priv_fmts[] = { @@ -50,6 +51,7 @@ static const char * const cs_ete_priv_fmts[] = { [CS_ETE_TRCIDR8] = " TRCIDR8 %llx\n", [CS_ETE_TRCAUTHSTATUS] = " TRCAUTHSTATUS %llx\n", [CS_ETE_TRCDEVARCH] = " TRCDEVARCH %llx\n",
[CS_ETE_TS_SOURCE] = " TS_SOURCE %lld\n",};
static const char * const param_unk_fmt = diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h index c5925428afa9..ad790930bcbc 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -71,6 +71,7 @@ enum { CS_ETMV4_TRCIDR2, CS_ETMV4_TRCIDR8, CS_ETMV4_TRCAUTHSTATUS,
CS_ETMV4_TS_SOURCE, CS_ETMV4_PRIV_MAX,};
@@ -92,6 +93,7 @@ enum { CS_ETE_TRCIDR8, CS_ETE_TRCAUTHSTATUS, CS_ETE_TRCDEVARCH,
CS_ETE_TS_SOURCE, CS_ETE_PRIV_MAX};
-- 2.25.1
On 03/01/2023 11:49, James Clark wrote:
On 23/12/2022 09:28, Mike Leach wrote:
Hi,
There was a discussion some time ago in one of our coresight regular dev meetings about this.
Can we just use only the necessary bits that TS source needs and leave the remaining bits from the 64 as unused for future expansion - i..e use this as a bitfield rather than have 64 bits occupied for what is effectively a 2 bit value.
Perhaps call the full value something other than TS_SOURCE and have a TS_SOURCE field with a known safe unset value.
If we did that, there wouldn't be any mechanism to detect if new values that were added were the value 0, or just not set/implemented/saved in the file. The current implementation of CS_ETM_NR_TRC_PARAMS allows us to add new fields, and detect if they exist or not without bumping the header version for each new sub field.
In this change HAS_PARAM() has been used to do this, but that can't be expanded to sub fields in the same 64 bits. I don't think space or performance are worth the extra complexity of dividing it up. And because this is just one block saved once in the header, so I'm not sure if it's worth it in this case. It would also make it harder to read the values on the raw dump mode.
I think it is fine to use the entire field for the time source, given the header can be extended with new fields, without breaking compatibility for future additions.
Suzuki
James
From: German Gomez german.gomez@arm.com
If virtual timestamps are detected, set sample time field accordingly, otherwise warn the user that the samples will not include the time data.
| Test notes (FEAT_TRF platform) | | $ ./perf record -e cs_etm//u -a -- sleep 4 | $ ./perf script --fields +time | perf 422 [000] 163.375100: 1 branches:uH: 0 [unknown] ([unknown]) | perf 422 [000] 163.375100: 1 branches:uH: ffffb8009544 ioctl+0x14 (/lib/aarch64-linux-gnu/libc-2.27.so) | perf 422 [000] 163.375100: 1 branches:uH: aaaaab6bebf4 perf_evsel__run_ioctl+0x90 (/home/german/linux/tools/perf/perf) | [...] | perf 422 [000] 167.393100: 1 branches:uH: aaaaab6bda00 __xyarray__entry+0x74 (/home/german/linux/tools/perf/perf) | perf 422 [000] 167.393099: 1 branches:uH: aaaaab6bda0c __xyarray__entry+0x80 (/home/german/linux/tools/perf/perf) | perf 422 [000] 167.393099: 1 branches:uH: ffffb8009538 ioctl+0x8 (/lib/aarch64-linux-gnu/libc-2.27.so) | | The time from the first sample to the last sample is 4 seconds
Signed-off-by: German Gomez german.gomez@arm.com Signed-off-by: James Clark james.clark@arm.com --- tools/perf/util/cs-etm.c | 74 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 70 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 879576d5f899..57a381eaaa6a 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -35,6 +35,7 @@ #include "tool.h" #include "thread.h" #include "thread-stack.h" +#include "tsc.h" #include <tools/libc_compat.h> #include "util/synthetic-events.h"
@@ -46,10 +47,12 @@ struct cs_etm_auxtrace { struct perf_session *session; struct machine *machine; struct thread *unknown_thread; + struct perf_tsc_conversion tc;
u8 timeless_decoding; u8 snapshot_mode; u8 data_queued; + u8 has_virtual_ts; /* Virtual/Kernel timestamps in the trace. */
int num_cpu; u64 latest_kernel_timestamp; @@ -1161,6 +1164,22 @@ static void cs_etm__copy_insn(struct cs_etm_queue *etmq, sample->insn_len, (void *)sample->insn); }
+static inline void cs_etm__resolve_sample_time(struct cs_etm_queue *etmq, + struct cs_etm_traceid_queue *tidq, + u64 *time) +{ + struct cs_etm_auxtrace *etm = etmq->etm; + struct cs_etm_packet_queue *packet_queue = &tidq->packet_queue; + + if (etm->timeless_decoding) + *time = 0; + else if (etm->has_virtual_ts) + *time = tsc_to_perf_time(packet_queue->cs_timestamp, &etm->tc); + else + *time = etm->latest_kernel_timestamp; + +} + static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq, struct cs_etm_traceid_queue *tidq, u64 addr, u64 period) @@ -1174,8 +1193,9 @@ static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq, event->sample.header.misc = cs_etm__cpu_mode(etmq, addr); event->sample.header.size = sizeof(struct perf_event_header);
- if (!etm->timeless_decoding) - sample.time = etm->latest_kernel_timestamp; + /* Set time field based con etm auxtrace config. */ + cs_etm__resolve_sample_time(etmq, tidq, &sample.time); + sample.ip = addr; sample.pid = tidq->pid; sample.tid = tidq->tid; @@ -1232,8 +1252,9 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq, event->sample.header.misc = cs_etm__cpu_mode(etmq, ip); event->sample.header.size = sizeof(struct perf_event_header);
- if (!etm->timeless_decoding) - sample.time = etm->latest_kernel_timestamp; + /* Set time field based con etm auxtrace config. */ + cs_etm__resolve_sample_time(etmq, tidq, &sample.time); + sample.ip = ip; sample.pid = tidq->pid; sample.tid = tidq->tid; @@ -2746,12 +2767,42 @@ static int cs_etm__queue_aux_records(struct perf_session *session) return 0; }
+#define HAS_PARAM(j, type, param) (metadata[(j)][CS_ETM_NR_TRC_PARAMS] <= \ + (CS_##type##_##param - CS_ETM_COMMON_BLK_MAX_V1)) + +/* + * Loop through the ETMs and complain if we find at least one where ts_source != 1 (virtual + * timestamps). + */ +static bool cs_etm__has_virtual_ts(u64 **metadata, int num_cpu) +{ + int j; + + for (j = 0; j < num_cpu; j++) { + switch (metadata[j][CS_ETM_MAGIC]) { + case __perf_cs_etmv4_magic: + if (HAS_PARAM(j, ETMV4, TS_SOURCE) || metadata[j][CS_ETMV4_TS_SOURCE] != 1) + return false; + break; + case __perf_cs_ete_magic: + if (HAS_PARAM(j, ETE, TS_SOURCE) || metadata[j][CS_ETE_TS_SOURCE] != 1) + return false; + break; + default: + /* Unknown / unsupported magic number. */ + return false; + } + } + return true; +} + int cs_etm__process_auxtrace_info_full(union perf_event *event, struct perf_session *session) { struct perf_record_auxtrace_info *auxtrace_info = &event->auxtrace_info; struct cs_etm_auxtrace *etm = NULL; struct int_node *inode; + struct perf_record_time_conv *tc = &session->time_conv; int event_header_size = sizeof(struct perf_event_header); int total_size = auxtrace_info->header.size; int priv_size = 0; @@ -2886,6 +2937,12 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event, etm->auxtrace_type = auxtrace_info->type; etm->timeless_decoding = cs_etm__is_timeless_decoding(etm);
+ /* Use virtual timestamps if all ETMs report ts_source = 1 */ + etm->has_virtual_ts = cs_etm__has_virtual_ts(metadata, num_cpu); + + if (!etm->has_virtual_ts) + ui__warning("Virtual timestamps are not enabled, or not supported by the traced system.\n\nThe time field of the samples will not be set.\n\n"); + etm->auxtrace.process_event = cs_etm__process_event; etm->auxtrace.process_auxtrace_event = cs_etm__process_auxtrace_event; etm->auxtrace.flush_events = cs_etm__flush_events; @@ -2915,6 +2972,15 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event, goto err_delete_thread; }
+ etm->tc.time_shift = tc->time_shift; + etm->tc.time_mult = tc->time_mult; + etm->tc.time_zero = tc->time_zero; + if (event_contains(*tc, time_cycles)) { + etm->tc.time_cycles = tc->time_cycles; + etm->tc.time_mask = tc->time_mask; + etm->tc.cap_user_time_zero = tc->cap_user_time_zero; + etm->tc.cap_user_time_short = tc->cap_user_time_short; + } err = cs_etm__synth_events(etm, session); if (err) goto err_delete_thread;
-
James Clark -
Leo Yan -
Mike Leach -
Suzuki K Poulose