Received-SPF: pass (google.com: domain of linux-kernel+bounces-150398-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <550e5ca5-f28c-48e1-b86e-9de3458a5737@linux.intel.com>
Date: Thu, 18 Apr 2024 11:35:11 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH v1] perf pmu: Assume sysfs events are always lowercase
To: Ian Rogers <irogers@google.com>, Peter Zijlstra <peterz@infradead.org>,
 Ingo Molnar <mingo@redhat.com>, Arnaldo Carvalho de Melo <acme@kernel.org>,
 Namhyung Kim <namhyung@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
 Alexander Shishkin <alexander.shishkin@linux.intel.com>,
 Jiri Olsa <jolsa@kernel.org>, Adrian Hunter <adrian.hunter@intel.com>,
 James Clark <james.clark@arm.com>, linux-perf-users@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240413040812.4042051-1-irogers@google.com>
Content-Language: en-US
From: "Liang, Kan" <kan.liang@linux.intel.com>
In-Reply-To: <20240413040812.4042051-1-irogers@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


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,};