Re: [PATCH v1 20/23] perf pmus: Split pmus list into core and uncore

["Liang, Kan" <kan.liang@linux.intel.com>]

On 2023-05-17 10:58 a.m., Ian Rogers wrote:
> Split the pmus list into core and uncore. This will later allow for
> the core and uncore pmus to be populated separately.

I think the "uncore pmus" here means the other non-core PMUs, right? If
so, I think the "uncore pmus" is misleading, since we have a dedicated
uncore driver/PMU. Maybe we can use "other pmus" instead.

Thanks,
Kan
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/pmus.c | 52 ++++++++++++++++++++++++++++++------------
>  1 file changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index 2fb28e583366..dd029a810147 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -12,13 +12,19 @@
>  #include "pmu.h"
>  #include "print-events.h"
>  
> -static LIST_HEAD(pmus);
> +static LIST_HEAD(core_pmus);
> +static LIST_HEAD(uncore_pmus);
>  
>  void perf_pmus__destroy(void)
>  {
>  	struct perf_pmu *pmu, *tmp;
>  
> -	list_for_each_entry_safe(pmu, tmp, &pmus, list) {
> +	list_for_each_entry_safe(pmu, tmp, &core_pmus, list) {
> +		list_del(&pmu->list);
> +
> +		perf_pmu__delete(pmu);
> +	}
> +	list_for_each_entry_safe(pmu, tmp, &uncore_pmus, list) {
>  		list_del(&pmu->list);
>  
>  		perf_pmu__delete(pmu);
> @@ -29,7 +35,12 @@ static struct perf_pmu *pmu_find(const char *name)
>  {
>  	struct perf_pmu *pmu;
>  
> -	list_for_each_entry(pmu, &pmus, list) {
> +	list_for_each_entry(pmu, &core_pmus, list) {
> +		if (!strcmp(pmu->name, name) ||
> +		    (pmu->alias_name && !strcmp(pmu->alias_name, name)))
> +			return pmu;
> +	}
> +	list_for_each_entry(pmu, &uncore_pmus, list) {
>  		if (!strcmp(pmu->name, name) ||
>  		    (pmu->alias_name && !strcmp(pmu->alias_name, name)))
>  			return pmu;
> @@ -53,7 +64,7 @@ struct perf_pmu *perf_pmus__find(const char *name)
>  		return pmu;
>  
>  	dirfd = perf_pmu__event_source_devices_fd();
> -	pmu = perf_pmu__lookup(&pmus, dirfd, name);
> +	pmu = perf_pmu__lookup(is_pmu_core(name) ? &core_pmus : &uncore_pmus, dirfd, name);
>  	close(dirfd);
>  
>  	return pmu;
> @@ -72,7 +83,7 @@ static struct perf_pmu *perf_pmu__find2(int dirfd, const char *name)
>  	if (pmu)
>  		return pmu;
>  
> -	return perf_pmu__lookup(&pmus, dirfd, name);
> +	return perf_pmu__lookup(is_pmu_core(name) ? &core_pmus : &uncore_pmus, dirfd, name);
>  }
>  
>  /* Add all pmus in sysfs to pmu list: */
> @@ -93,7 +104,7 @@ static void pmu_read_sysfs(void)
>  	while ((dent = readdir(dir))) {
>  		if (!strcmp(dent->d_name, ".") || !strcmp(dent->d_name, ".."))
>  			continue;
> -		/* add to static LIST_HEAD(pmus): */
> +		/* add to static LIST_HEAD(core_pmus) or LIST_HEAD(uncore_pmus): */
>  		perf_pmu__find2(fd, dent->d_name);
>  	}
>  
> @@ -104,24 +115,37 @@ struct perf_pmu *perf_pmus__find_by_type(unsigned int type)
>  {
>  	struct perf_pmu *pmu;
>  
> -	list_for_each_entry(pmu, &pmus, list)
> +	list_for_each_entry(pmu, &core_pmus, list) {
>  		if (pmu->type == type)
>  			return pmu;
> -
> +	}
> +	list_for_each_entry(pmu, &uncore_pmus, list) {
> +		if (pmu->type == type)
> +			return pmu;
> +	}
>  	return NULL;
>  }
>  
> +/*
> + * pmu iterator: If pmu is NULL, we start at the begin, otherwise return the
> + * next pmu. Returns NULL on end.
> + */
>  struct perf_pmu *perf_pmus__scan(struct perf_pmu *pmu)
>  {
> -	/*
> -	 * pmu iterator: If pmu is NULL, we start at the begin,
> -	 * otherwise return the next pmu. Returns NULL on end.
> -	 */
> +	bool use_core_pmus = !pmu || pmu->is_core;
> +
>  	if (!pmu) {
>  		pmu_read_sysfs();
> -		pmu = list_prepare_entry(pmu, &pmus, list);
> +		pmu = list_prepare_entry(pmu, &core_pmus, list);
> +	}
> +	if (use_core_pmus) {
> +		list_for_each_entry_continue(pmu, &core_pmus, list)
> +			return pmu;
> +
> +		pmu = NULL;
> +		pmu = list_prepare_entry(pmu, &uncore_pmus, list);
>  	}
> -	list_for_each_entry_continue(pmu, &pmus, list)
> +	list_for_each_entry_continue(pmu, &uncore_pmus, list)
>  		return pmu;
>  	return NULL;
>  }