Received-SPF: pass (google.com: domain of linux-kernel+bounces-180115-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <b205e882-a9dc-44fd-bbc3-53089433eae5@oracle.com>
Date: Wed, 15 May 2024 10:09:46 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1] perf pmu: Count sys and cpuid json events separately
To: Ian Rogers <irogers@google.com>, Jia He <justin.he@arm.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>,
        Kan Liang <kan.liang@linux.intel.com>,
        James Clark <james.clark@arm.com>, linux-perf-users@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20240511003601.2666907-1-irogers@google.com>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <20240511003601.2666907-1-irogers@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?cFFNSEU2Y3pya05meWZNdEdCaDFaZjdYcy9nMFYrZ3RydXVsOTZFMUFYYnRn?=
 =?utf-8?B?aVF4dWRaMjh6eGM1b2JtL0xRZ0pkNDdjQlVyWk5KUkg5RGxjVHFaWHMrbWp6?=
 =?utf-8?B?ODhQbWVkZWpka3VYaXJuT3AvNHZPc21aT1RkckxrRW5MbTNvc2dLZ1Z2SWtp?=
 =?utf-8?B?aUlSa2VFdnFaVXNYY0FYL0ZCMmF1aWhPZmtUbDZMazZjTjc4dGx4T3Fvcm5n?=
 =?utf-8?B?amN1M3RlSG9EL0VyVC9DOVlpOEJKMzRoMEEyb1JJWkRQYzZWNldTOFpGQWlK?=
 =?utf-8?B?SytkODEvdktRbkNnWnJ5TktpTjc4Q0J4c201VzlrbmpxU1lVeGIvQkdoQWIy?=
 =?utf-8?B?bXNhK0hMbEF2bXgxWFA1aWp4YmNJM3dzTXZWUmhTR2dzWG9McGkzWG5ocVVO?=
 =?utf-8?B?eURLY2FVbWg4T0RlNXJNcTVuZTYxYkdncnlRTnZsc01vYkFlMnh2VGxYR2t4?=
 =?utf-8?B?S2JPMUllYkRwT1FkM1ljNEphWUFyTmplNUcycHk3cDcyeXRrZ2phTHVCTW9n?=
 =?utf-8?B?WUExWmMwWU5sMkhtYnlzSXJySGRCbG8rZ3Z0MFFVbkJ1UUM0eURPeG51allC?=
 =?utf-8?B?QXAzWGJkSGJpbmxtelpyM1BsYlY5K0Yvdm1WaGw2Nzc3MENmcFVIZFUzS2JY?=
 =?utf-8?B?ZHhGUUJ3ejYrblZhakpEU0NvenN4dUdpRk9VdWc0emhUdDJYNFFSSUVYUko0?=
 =?utf-8?B?VDhnY2cvNHltSEJHemxzOGVmb0NKbjQ3NU9GNXNDSTJBUGNObXd4ZXBHMm5Q?=
 =?utf-8?B?V3QwNDg1c2hnNHlhejViZW9SK0M3Ynl1cTNhZVMxRWRDZk5VNy9ENGZpbzEw?=
 =?utf-8?B?UTR0R0p4ZENGbGlpRC9uMmJUdTFuZVZnTk9hSU5kUXBhZlV6SHNHN3JUN05h?=
 =?utf-8?B?ZGt4THVXNWlxazV1R0RsSG84aThCQzd1T1IxL3BwOW82Nzl2U0RsbUlOTTZk?=
 =?utf-8?B?OWVNOFdOMmJkSjdoeG5zUHp0Y09HMkpSSW5pRnlENTBKVWQ3MXgwVjUvYUlk?=
 =?utf-8?B?QVZGWWZmMS80VFlWbUcrSVVLazJ0NnJ0Rmpta0xubXlidzFrM25ZQzl5T3Jo?=
 =?utf-8?B?aUttd1RmeTFEQzFiR2FkYmUvaW1wdmJvc0Y2czQxQWU4aXZYKzRiZ0pqdE5o?=
 =?utf-8?B?L2hNUEFUTzlxMmUxc1dWY1U4Y1NOZ1l1ZTVKZEp5UjVOZXRxK2U1RUxVamJm?=
 =?utf-8?B?WHhsK3NlSGFjQlpVQUN0Q0E1Z2todGkxS1pwTTdhL3NzbHJMbFh0cEdZT016?=
 =?utf-8?B?Y3VGRkNESWYxYVdFT2NKNGMxYktvaDVBN1Y2WTVoYXJsZjR4TExHVjVBOE04?=
 =?utf-8?B?RHVnNVRFWGh6VHlmeE05NlY0TDhzUnJpU0ZIUXF6MkQzRnlzOTAvc0x0eUtX?=
 =?utf-8?B?MXFyRWova295dnJLdW1kaGVMV3NreDlVR1dvemxldUZsWUcxWVhHQVA4dVJB?=
 =?utf-8?B?L05vTkkvWEJkTktwNHFvcXh3ZzV0U1Q0MEVRS2RrWUZSaTM5c3dRbU5TdmhX?=
 =?utf-8?B?NGFxeFluUTVDRWwrNEorVFFQcGFId1F0YW5Yd2lUT2ZvRDlGTGZsN3g1K1lJ?=
 =?utf-8?B?b05kT3kycUVDbGR3dCtFcVV4SUExOVU2Z1B1TzU4R1FtRHVlWHUvTGZEaXlz?=
 =?utf-8?B?V2Q1bDBES1pBTGkzVkJTZ1ptNWhSRzV5OXcyNW1zaDAvVC9vaVFoYkQ3MXlO?=
 =?utf-8?B?M0VIT3FGbVZuQjhodFl3TFlGMkFLY0QyY05QdUJyc1B3dkw4WFJ2OEdzZFhw?=
 =?utf-8?B?VnB6ZmFVSDhQQ1N0ZkR4VW9iYy9jbSs0YzFlaDhvNG1vSW5KY3lkQkRtUmNV?=
 =?utf-8?B?RlVtTUpaTDAvRnpnUkVEQm1aTTV3SWhubldIMXozeEorVG5BbC90TG1mRHpS?=
 =?utf-8?B?bE1ySVhLL0Q3Y1BMR2RrcU8zdm1PSnpIMmtBWVNrYW4vckFQTUg3bUIyZFpJ?=
 =?utf-8?B?azFLdDdhbHhQSHY0Umh0dFBMT0gxS09PN2JYYTVML2lFeVp1eWRpSU9lREt2?=
 =?utf-8?B?YkFxcHI0NjU1clRVUTFlaTY5U0JSMVlJNnVsVlZ3ZFlEanRmSWxPQTM3L3pF?=
 =?utf-8?B?YkRlMUUrQTNleUpRS1Y2NCtBRzEvaDVtbnNhRGZEc3NicDRnd0VOMm1jOVVQ?=
 =?utf-8?B?dS9ralhLVTdXNVk5V1FwdTFrdUVjSjN3QlNCWEJHQnZIOGJkRGE4M0NBcnlz?=
 =?utf-8?B?aWc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	axUY2MLq1FSvF303RDC5EdueXiH1kILfTNQPUlNEpclHsRTT2HmBqbAzibiTelQs76L5xG1iKZA/Ocx/ac1VkMnZprF3zJfzeMgTxcn2E3CsL8+0FV9JEhGiyrR0L7/Fben5NR+VK7f1+koLl2Nr+KJgXrrI9vXh3FUpPdyC96QNq3Zfk8PQT3J9vajbLhpi5WyYpgufx7KgDTXFt0yWVcbaYyLSnlheISmLaV5HnxUXR7q4d9oc9nC1MVt81twk5xhsbMHm7Ntkb5iahGrdOOeCpHncCcQhY+rO60JjFcbp/5atsCHKYFsEimXinADD4uUrYZtQCB+Z9MRkgfGWBNJpc6WJSeGv18VNyDC3J2oaFw088gZiSDy8WNaun274mANkqTOH72pYrrixdLZWSy+9xdwxLftcjTDRuD3LZT/hetYUC+hdBEU2O2Dl+LYdlvWN2chQ5JZ0antvmR+EiqtZZiwmAGdmI3xbK2WrhKyjrYrc0hWsnL5myHlWyDbmXiyepmObYH8uIBMsrFC0rPSmI6PZ1jBLOJauOHnXzMavof4TNZWuV5y9iG7YyGnpPRMDJJWJqIiRsYp7EBJbYSrMfAc4/VboxD6Cu/wrlww=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 271e038a-7764-49e5-8357-08dc74f97193
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 May 2024 16:09:48.2025
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Iq3w0qKmxcQ3gyQJcIW8SaXkFx8NA33p4xNcSkcPAnU4UirhXf3xSUrBC9HjjVLEgG2dnjr+TdFCF1JrblI7ng==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4274
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-05-15_10,2024-05-15_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0
 mlxlogscore=999 spamscore=0 suspectscore=0 mlxscore=0 malwarescore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2405150114
X-Proofpoint-GUID: CteOgCWnGx-UEskEUcjS0du_llLtX-IN
X-Proofpoint-ORIG-GUID: CteOgCWnGx-UEskEUcjS0du_llLtX-IN

On 10/05/2024 18:36, Ian Rogers wrote:
> Sys events are eagerly loaded as each event has a compat option that
> may mean the event is or isn't associated with the PMU. These
> shouldn't be counted as loaded_json_events as that is used for json
> events matching the CPUID that may or may not have been loaded. The
> mismatch causes issues on ARM64 that uses sys events.
> 
> Reported-by: Jia He <justin.he@arm.com>
> Closes: https://urldefense.com/v3/__https://lore.kernel.org/lkml/20240510024729.1075732-1-justin.he@arm.com/__;!!ACWV5N9M2RV99hQ!KVhaRPLqS7T1s6p506Gv0zFNdlTR1exCUrXM3UIDr0EdrRFwrzM3W9ntkxw42jNeG01P7H78r0c-nz8FVQQ$
> Fixes: e6ff1eed3584 ("perf pmu: Lazily add JSON events")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>   tools/perf/util/pmu.c | 70 ++++++++++++++++++++++++++++++-------------
>   tools/perf/util/pmu.h |  6 ++--
>   2 files changed, 53 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index b3b072feef02..888ce9912275 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -36,6 +36,18 @@ struct perf_pmu perf_pmu__fake = {
>   
>   #define UNIT_MAX_LEN	31 /* max length for event unit name */
>   
> +enum event_source {
> +	/* An event loaded from /sys/devices/<pmu>/events. */
> +	EVENT_SRC_SYSFS,
> +	/* An event loaded from a CPUID matched json file. */
> +	EVENT_SRC_CPU_JSON,
> +	/*
> +	 * An event loaded from a /sys/devices/<pmu>/identifier matched json
> +	 * file.
> +	 */
> +	EVENT_SRC_SYS_JSON,

If a json sys event aliases to a EVENT_SRC_SYSFS event, this is 
considered a EVENT_SRC_SYS_JSON event source, right?

> +};
> +
>   /**
>    * struct perf_pmu_alias - An event either read from sysfs or builtin in
>    * pmu-events.c, created by parsing the pmu-events json files.
> @@ -521,7 +533,7 @@ static int update_alias(const struct pmu_event *pe,
>   
>   static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
>   				const char *desc, const char *val, FILE *val_fd,
> -				const struct pmu_event *pe)
> +			        const struct pmu_event *pe, enum event_source src)
>   {
>   	struct perf_pmu_alias *alias;
>   	int ret;
> @@ -574,25 +586,30 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
>   		}
>   		snprintf(alias->unit, sizeof(alias->unit), "%s", unit);
>   	}
> -	if (!pe) {
> -		/* Update an event from sysfs with json data. */
> -		struct update_alias_data data = {
> -			.pmu = pmu,
> -			.alias = alias,
> -		};
> -
> +	switch (src) {
> +	default:
> +	case EVENT_SRC_SYSFS:
>   		alias->from_sysfs = true;
>   		if (pmu->events_table) {
> +			/* Update an event from sysfs with json data. */
> +			struct update_alias_data data = {
> +				.pmu = pmu,
> +				.alias = alias,
> +			};
>   			if (pmu_events_table__find_event(pmu->events_table, pmu, name,
>   							 update_alias, &data) == 0)
> -				pmu->loaded_json_aliases++;
> +				pmu->cpu_json_aliases++;

seems strange that matching for EVENT_SRC_SYSFS increases 
pmu->cpu_json_aliases

>   		}
> -	}
> -
> -	if (!pe)
>   		pmu->sysfs_aliases++;
> -	else
> -		pmu->loaded_json_aliases++;
> +		break;
> +	case  EVENT_SRC_CPU_JSON:
> +		pmu->cpu_json_aliases++;
> +		break;
> +	case  EVENT_SRC_SYS_JSON:
> +		pmu->sys_json_aliases++;
> +		break;
> +
> +	}
>   	list_add_tail(&alias->list, &pmu->aliases);
>   	return 0;
>   }
> @@ -653,7 +670,8 @@ static int __pmu_aliases_parse(struct perf_pmu *pmu, int events_dir_fd)
>   		}
>   
>   		if (perf_pmu__new_alias(pmu, name, /*desc=*/ NULL,
> -					/*val=*/ NULL, file, /*pe=*/ NULL) < 0)
> +					/*val=*/ NULL, file, /*pe=*/ NULL,
> +					EVENT_SRC_SYSFS) < 0)
>   			pr_debug("Cannot set up %s\n", name);
>   		fclose(file);
>   	}
> @@ -946,7 +964,8 @@ static int pmu_add_cpu_aliases_map_callback(const struct pmu_event *pe,
>   {
>   	struct perf_pmu *pmu = vdata;
>   
> -	perf_pmu__new_alias(pmu, pe->name, pe->desc, pe->event, /*val_fd=*/ NULL, pe);
> +	perf_pmu__new_alias(pmu, pe->name, pe->desc, pe->event, /*val_fd=*/ NULL,
> +			    pe, EVENT_SRC_CPU_JSON);
>   	return 0;
>   }
>   
> @@ -981,13 +1000,14 @@ static int pmu_add_sys_aliases_iter_fn(const struct pmu_event *pe,
>   		return 0;
>   
>   	if (pmu_uncore_alias_match(pe->pmu, pmu->name) &&
> -			pmu_uncore_identifier_match(pe->compat, pmu->id)) {
> +	    pmu_uncore_identifier_match(pe->compat, pmu->id)) {
>   		perf_pmu__new_alias(pmu,
>   				pe->name,
>   				pe->desc,
>   				pe->event,
>   				/*val_fd=*/ NULL,
> -				pe);
> +				pe,
> +				EVENT_SRC_SYS_JSON);
>   	}
>   
>   	return 0;
> @@ -1082,6 +1102,12 @@ struct perf_pmu *perf_pmu__lookup(struct list_head *pmus, int dirfd, const char
>   	pmu->max_precise = pmu_max_precise(dirfd, pmu);
>   	pmu->alias_name = pmu_find_alias_name(pmu, dirfd);
>   	pmu->events_table = perf_pmu__find_events_table(pmu);
> +	/*
> +	 * Load the sys json events/aliases when loading the PMU as each event
> +	 * may have a different compat regular expression. We therefore can't
> +	 * know the number of sys json events/aliases without computing the
> +	 * regular expressions for them all.
> +	 */
>   	pmu_add_sys_aliases(pmu);
>   	list_add_tail(&pmu->list, pmus);
>   
> @@ -1739,12 +1765,14 @@ size_t perf_pmu__num_events(struct perf_pmu *pmu)
>   	size_t nr;
>   
>   	pmu_aliases_parse(pmu);
> -	nr = pmu->sysfs_aliases;
> +	nr = pmu->sysfs_aliases + pmu->sys_json_aliases;;
>   
>   	if (pmu->cpu_aliases_added)
> -		 nr += pmu->loaded_json_aliases;
> +		 nr += pmu->cpu_json_aliases;
>   	else if (pmu->events_table)
> -		nr += pmu_events_table__num_events(pmu->events_table, pmu) - pmu->loaded_json_aliases;
> +		nr += pmu_events_table__num_events(pmu->events_table, pmu) - pmu->cpu_json_aliases;
> +	else
> +		assert(pmu->cpu_json_aliases == 0);
>   
>   	return pmu->selectable ? nr + 1 : nr;
>   }
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 561716aa2b25..b2d3fd291f02 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -123,8 +123,10 @@ struct perf_pmu {
>   	const struct pmu_events_table *events_table;
>   	/** @sysfs_aliases: Number of sysfs aliases loaded. */
>   	uint32_t sysfs_aliases;
> -	/** @sysfs_aliases: Number of json event aliases loaded. */
> -	uint32_t loaded_json_aliases;
> +	/** @cpu_json_aliases: Number of json event aliases loaded specific to the CPUID. */
> +	uint32_t cpu_json_aliases;

it seems strange that we allow a pmu to have both cpu_json_aliases and 
sys_json_aliases count. A union could be used, but that would complicate 
things.

> +	/** @sys_json_aliases: Number of json event aliases loaded matching the PMU's identifier. */
> +	uint32_t sys_json_aliases;
>   	/** @sysfs_aliases_loaded: Are sysfs aliases loaded from disk? */
>   	bool sysfs_aliases_loaded;
>   	/**