linux-kernel - Re: [RFC PATCH v1] perf pmu: Assume sysfs events are always lowercase

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 18 Apr 2024 11:35:11 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Ian Rogers <irogers@...gle.com>, Peter Zijlstra <peterz@...radead.org>,
 Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
 James Clark <james.clark@....com>, linux-perf-users@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v1] perf pmu: Assume sysfs events are always lowercase



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@...gle.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,};