Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 11 Aug 2021 21:23:25 +0200
From:   Jiri Olsa <jolsa@redhat.com>
To:     Jin Yao <yao.jin@linux.intel.com>
Cc:     acme@kernel.org, jolsa@kernel.org, peterz@infradead.org,
        mingo@redhat.com, alexander.shishkin@linux.intel.com,
        Linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        ak@linux.intel.com, kan.liang@intel.com, yao.jin@intel.com,
        rickyman7@gmail.com, Kan Liang <kan.liang@linux.intel.com>
Subject: Re: [PATCH v4 1/2] perf pmu: Add PMU alias support
Message-ID: <YRQjreot69DL0xVV@krava>
References: <20210811024827.9483-1-yao.jin@linux.intel.com>
 <20210811024827.9483-2-yao.jin@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210811024827.9483-2-yao.jin@linux.intel.com>
Precedence: bulk

On Wed, Aug 11, 2021 at 10:48:26AM +0800, Jin Yao wrote:

SNIP

>  				if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true, false))
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index fc683bc41715..796a4be752f4 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -946,6 +946,18 @@ perf_pmu__get_default_config(struct perf_pmu *pmu __maybe_unused)
>  	return NULL;
>  }
>  
> +char * __weak
> +pmu_find_real_name(const char *name)
> +{
> +	return strdup(name);
> +}

hm, why does this need to return already strdup? it forces you
to add all those goto below.. could just return name and keep
the 'pmu->name = strdup(name);' below?

that should make the change simpler

jirka

> +
> +char * __weak
> +pmu_find_alias_name(const char *name __maybe_unused)
> +{
> +	return NULL;
> +}
> +
>  static int pmu_max_precise(const char *name)
>  {
>  	char path[PATH_MAX];
> @@ -959,19 +971,25 @@ static int pmu_max_precise(const char *name)
>  	return max_precise;
>  }
>  
> -static struct perf_pmu *pmu_lookup(const char *name)
> +static struct perf_pmu *pmu_lookup(const char *lookup_name)
>  {
> -	struct perf_pmu *pmu;
> +	struct perf_pmu *pmu = NULL;
>  	LIST_HEAD(format);
>  	LIST_HEAD(aliases);
>  	__u32 type;
> -	bool is_hybrid = perf_pmu__hybrid_mounted(name);
> +	bool is_hybrid;
> +	char *name = pmu_find_real_name(lookup_name);
> +
> +	if (!name)
> +		return NULL;
> +
> +	is_hybrid = perf_pmu__hybrid_mounted(name);
>  
>  	/*
>  	 * Check pmu name for hybrid and the pmu may be invalid in sysfs
>  	 */
>  	if (!strncmp(name, "cpu_", 4) && !is_hybrid)
> -		return NULL;
> +		goto out;
>  
>  	/*
>  	 * The pmu data we store & need consists of the pmu
> @@ -979,23 +997,24 @@ static struct perf_pmu *pmu_lookup(const char *name)
>  	 * now.
>  	 */
>  	if (pmu_format(name, &format))
> -		return NULL;
> +		goto out;
>  
>  	/*
>  	 * Check the type first to avoid unnecessary work.
>  	 */
>  	if (pmu_type(name, &type))
> -		return NULL;
> +		goto out;
>  
>  	if (pmu_aliases(name, &aliases))
> -		return NULL;
> +		goto out;
>  
>  	pmu = zalloc(sizeof(*pmu));
>  	if (!pmu)
> -		return NULL;
> +		goto out;
>  
>  	pmu->cpus = pmu_cpumask(name);
> -	pmu->name = strdup(name);
> +	pmu->name = name;
> +	pmu->alias_name = pmu_find_alias_name(name);
>  	pmu->type = type;
>  	pmu->is_uncore = pmu_is_uncore(name);
>  	if (pmu->is_uncore)
> @@ -1017,6 +1036,10 @@ static struct perf_pmu *pmu_lookup(const char *name)
>  
>  	pmu->default_config = perf_pmu__get_default_config(pmu);
>  
> +out:
> +	if (!pmu)
> +		free(name);
> +
>  	return pmu;
>  }
>  
> @@ -1025,7 +1048,8 @@ static struct perf_pmu *pmu_find(const char *name)
>  	struct perf_pmu *pmu;
>  
>  	list_for_each_entry(pmu, &pmus, list)
> -		if (!strcmp(pmu->name, name))
> +		if (!strcmp(pmu->name, name) ||
> +		    (pmu->alias_name && !strcmp(pmu->alias_name, name)))
>  			return pmu;
>  
>  	return NULL;
> @@ -1920,6 +1944,9 @@ bool perf_pmu__has_hybrid(void)
>  
>  int perf_pmu__match(char *pattern, char *name, char *tok)
>  {
> +	if (!name)
> +		return -1;
> +
>  	if (fnmatch(pattern, name, 0))
>  		return -1;
>  
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 926da483a141..f6ca9f6a06ef 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -21,6 +21,7 @@ enum {
>  #define PERF_PMU_FORMAT_BITS 64
>  #define EVENT_SOURCE_DEVICE_PATH "/bus/event_source/devices/"
>  #define CPUS_TEMPLATE_CPU	"%s/bus/event_source/devices/%s/cpus"
> +#define MAX_PMU_NAME_LEN 128
>  
>  struct perf_event_attr;
>  
> @@ -32,6 +33,7 @@ struct perf_pmu_caps {
>  
>  struct perf_pmu {
>  	char *name;
> +	char *alias_name;	/* PMU alias name */
>  	char *id;
>  	__u32 type;
>  	bool selectable;
> @@ -135,4 +137,7 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
>  bool perf_pmu__has_hybrid(void);
>  int perf_pmu__match(char *pattern, char *name, char *tok);
>  
> +char *pmu_find_real_name(const char *name);
> +char *pmu_find_alias_name(const char *name);
> +
>  #endif /* __PMU_H */
> -- 
> 2.17.1
>