Messages in this thread | | | Date | Thu, 18 Apr 2024 11:35:11 -0400 | Subject | Re: [RFC PATCH v1] perf pmu: Assume sysfs events are always lowercase | From | "Liang, Kan" <> |
| |
On 2024-04-13 12:08 a.m., Ian Rogers wrote: > Perf event names aren't case sensitive. For sysfs events the entire > directory of events is read then iterated comparing names in a case > insensitive way, most often to see if an event is present. > > Consider: > $ perf stat -e inst_retired.any true > > The event inst_retired.any may be present in any PMU, so every PMU's > sysfs events are loaded and then searched with strcasecmp to see if > any match. This event is only present on the cpu PMU as a json event > so a lot of events were loaded from sysfs unnecessarily just to prove > an event didn't exist there. > > This change avoids loading all the events by assuming sysfs event > names are always lowercase.
From what I searched in the kernel, it looks like all the sysfs event names (from different ARCHs and devices) are lowercase. It should not break the existing usages. I don't see a reason why a event name must be uppercase. However, I think we need to add something to guarantee the assumption.
Could you please update the doc to describe the assumption? Documentation/ABI/testing/sysfs-bus-event_source-devices-events So everybody can follow the rule.
I think a perf test is required as well. Maybe we can extend the test__pmu_events() to do the name check.
Thanks, Kan
> It then uses file exists and only loads > the events when the desired event is present. > > For the example above, the number of openat calls measured by perf > trace on a tigerlake laptop goes from 325 down to 255. The reduction > will be larger for machines with many PMUs, particularly replicated > uncore PMUs. > > Make pmu_aliases_parse early return when aliases are loaded, ensure > the function is called before all uses of the aliases list. > > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/pmu.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c > index ab30f22eaf10..ce72c99e4f61 100644 > --- a/tools/perf/util/pmu.c > +++ b/tools/perf/util/pmu.c > @@ -425,9 +425,16 @@ static struct perf_pmu_alias *perf_pmu__find_alias(struct perf_pmu *pmu, > { > struct perf_pmu_alias *alias; > > - if (load && !pmu->sysfs_aliases_loaded) > - pmu_aliases_parse(pmu); > + if (load && !pmu->sysfs_aliases_loaded) { > + char event_file_name[FILENAME_MAX + 8]; > > + scnprintf(event_file_name, sizeof(event_file_name), "events/%s", name); > + for (size_t i = 7, n = 7 + strlen(name); i < n; i++) > + event_file_name[i] = tolower(event_file_name[i]); > + > + if (perf_pmu__file_exists(pmu, event_file_name)) > + pmu_aliases_parse(pmu); > + } > list_for_each_entry(alias, &pmu->aliases, list) { > if (!strcasecmp(alias->name, name)) > return alias; > @@ -605,6 +612,9 @@ static int pmu_aliases_parse(struct perf_pmu *pmu) > size_t len; > int fd, dir_fd; > > + if (pmu->sysfs_aliases_loaded) > + return 0; > + > len = perf_pmu__event_source_devices_scnprintf(path, sizeof(path)); > if (!len) > return 0; > @@ -1689,9 +1699,7 @@ size_t perf_pmu__num_events(struct perf_pmu *pmu) > { > size_t nr; > > - if (!pmu->sysfs_aliases_loaded) > - pmu_aliases_parse(pmu); > - > + pmu_aliases_parse(pmu); > nr = pmu->sysfs_aliases; > > if (pmu->cpu_aliases_added) > @@ -1750,6 +1758,7 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus, > struct strbuf sb; > > strbuf_init(&sb, /*hint=*/ 0); > + pmu_aliases_parse(pmu); > pmu_add_cpu_aliases(pmu); > list_for_each_entry(event, &pmu->aliases, list) { > size_t buf_used; > @@ -2154,6 +2163,7 @@ const char *perf_pmu__name_from_config(struct perf_pmu *pmu, u64 config) > if (!pmu) > return NULL; > > + pmu_aliases_parse(pmu); > pmu_add_cpu_aliases(pmu); > list_for_each_entry(event, &pmu->aliases, list) { > struct perf_event_attr attr = {.config = 0,};
| |