Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   Jiri Olsa <olsajiri@gmail.com>
Date:   Mon, 23 May 2022 11:04:36 +0200
To:     Ravi Bangoria <ravi.bangoria@amd.com>
Cc:     acme@kernel.org, irogers@google.com, peterz@infradead.org,
        rrichter@amd.com, mingo@redhat.com, mark.rutland@arm.com,
        namhyung@kernel.org, tglx@linutronix.de, bp@alien8.de,
        james.clark@arm.com, leo.yan@linaro.org, kan.liang@linux.intel.com,
        ak@linux.intel.com, eranian@google.com, like.xu.linux@gmail.com,
        x86@kernel.org, linux-perf-users@vger.kernel.org,
        linux-kernel@vger.kernel.org, sandipan.das@amd.com,
        ananth.narayan@amd.com, kim.phillips@amd.com,
        santosh.shukla@amd.com
Subject: Re: [PATCH v4 2/5] perf header: Parse non-cpu pmu capabilities
Message-ID: <YotOJL0dJmKUQnlI@krava>
References: <20220523033945.1612-1-ravi.bangoria@amd.com>
 <20220523033945.1612-3-ravi.bangoria@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220523033945.1612-3-ravi.bangoria@amd.com>
Precedence: bulk

On Mon, May 23, 2022 at 09:09:42AM +0530, Ravi Bangoria wrote:

SNIP

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index a27132e5a5ef..b4505dbb9f4a 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1580,6 +1580,67 @@ static int write_hybrid_cpu_pmu_caps(struct feat_fd *ff,
>  	return 0;
>  }
>  
> +/*
> + * File format:
> + *
> + * struct {
> + *	u32 nr_pmus;
> + *	struct {
> + *		char pmu_name[];
> + *		u32 nr_caps;
> + *		struct {
> + *			char name[];
> + *			char value[];
> + *		} [nr_caps];
> + *	} [nr_pmus];
> + * };
> + */
> +static int write_pmu_caps(struct feat_fd *ff, struct evlist *evlist __maybe_unused)
> +{
> +	struct perf_pmu_caps *caps = NULL;
> +	struct perf_pmu *pmu = NULL;
> +	u32 nr_pmus = 0;
> +	int ret;
> +
> +	while ((pmu = perf_pmu__scan(pmu))) {
> +		if (!pmu->name || !strncmp(pmu->name, "cpu", 3) ||

should we check for the hybrid names as well?
there's a helper perf_pmu__is_hybrid,
aybe you can use that

jirka


> +		    perf_pmu__caps_parse(pmu) <= 0)
> +			continue;
> +		nr_pmus++;
> +	}
> +
> +	ret = do_write(ff, &nr_pmus, sizeof(nr_pmus));
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!nr_pmus)
> +		return 0;
> +
> +	while ((pmu = perf_pmu__scan(pmu))) {
> +		if (!pmu->name || !strncmp(pmu->name, "cpu", 3) || !pmu->nr_caps)
> +			continue;
> +
> +		ret = do_write_string(ff, pmu->name);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = do_write(ff, &pmu->nr_caps, sizeof(pmu->nr_caps));
> +		if (ret < 0)
> +			return ret;
> +
> +		list_for_each_entry(caps, &pmu->caps, list) {
> +			ret = do_write_string(ff, caps->name);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = do_write_string(ff, caps->value);
> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static void print_hostname(struct feat_fd *ff, FILE *fp)
>  {
>  	fprintf(fp, "# hostname : %s\n", ff->ph->env.hostname);
> @@ -2209,6 +2270,32 @@ static void print_mem_topology(struct feat_fd *ff, FILE *fp)
>  	}
>  }
>  
> +static void print_pmu_caps(struct feat_fd *ff, FILE *fp)
> +{
> +	struct env_pmu_caps *env_pmu_caps = ff->ph->env.env_pmu_caps;
> +	int nr_pmus_with_caps = ff->ph->env.nr_pmus_with_caps;
> +	const char *delimiter = "";
> +	char **ptr;
> +	int i;
> +	u32 j;
> +
> +	if (!nr_pmus_with_caps)
> +		return;
> +
> +	for (i = 0; i < nr_pmus_with_caps; i++) {
> +		fprintf(fp, "# %s pmu capabilities: ", env_pmu_caps[i].pmu_name);
> +
> +		ptr = env_pmu_caps[i].pmu_caps;
> +
> +		delimiter = "";
> +		for (j = 0; j < env_pmu_caps[i].nr_caps; j++) {
> +			fprintf(fp, "%s%s", delimiter, ptr[j]);
> +			delimiter = ", ";
> +		}
> +		fprintf(fp, "\n");
> +	}
> +}
> +
>  static int __event_process_build_id(struct perf_record_header_build_id *bev,
>  				    char *filename,
>  				    struct perf_session *session)
> @@ -3319,6 +3406,103 @@ static int process_hybrid_cpu_pmu_caps(struct feat_fd *ff,
>  	return ret;
>  }
>  
> +static int __process_pmu_caps(struct feat_fd *ff, struct env_pmu_caps *env_pmu_caps)
> +{
> +	u32 nr_caps = env_pmu_caps->nr_caps;
> +	int name_size, value_size;
> +	char *name, *value, *ptr;
> +	u32 i;
> +
> +	env_pmu_caps->pmu_caps = zalloc(sizeof(char *) * nr_caps);
> +	if (!env_pmu_caps->pmu_caps)
> +		return -1;
> +
> +	for (i = 0; i < nr_caps; i++) {
> +		name = do_read_string(ff);
> +		if (!name)
> +			goto error;
> +
> +		value = do_read_string(ff);
> +		if (!value)
> +			goto free_name;
> +
> +		name_size = strlen(name);
> +		value_size = strlen(value);
> +		ptr = zalloc(sizeof(char) * (name_size + value_size + 2));
> +		if (!ptr)
> +			goto free_value;
> +
> +		memcpy(ptr, name, name_size);
> +		ptr[name_size] = '=';
> +		memcpy(ptr + name_size + 1, value, value_size);
> +		env_pmu_caps->pmu_caps[i] = ptr;
> +
> +		free(value);
> +		free(name);
> +	}
> +	return 0;
> +
> +free_value:
> +	free(value);
> +free_name:
> +	free(name);
> +error:
> +	for (; i > 0; i--)
> +		free(env_pmu_caps->pmu_caps[i - 1]);
> +	free(env_pmu_caps->pmu_caps);
> +	return -1;
> +}
> +
> +static int process_pmu_caps(struct feat_fd *ff, void *data __maybe_unused)
> +{
> +	struct env_pmu_caps *env_pmu_caps;
> +	u32 nr_pmus;
> +	u32 i, j;
> +
> +	ff->ph->env.nr_pmus_with_caps = 0;
> +	ff->ph->env.env_pmu_caps = NULL;
> +
> +	if (do_read_u32(ff, &nr_pmus))
> +		return -1;
> +
> +	if (!nr_pmus)
> +		return 0;
> +
> +	env_pmu_caps = zalloc(sizeof(struct env_pmu_caps) * nr_pmus);
> +	if (!env_pmu_caps)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nr_pmus; i++) {
> +		env_pmu_caps[i].pmu_name = do_read_string(ff);
> +		if (!env_pmu_caps[i].pmu_name)
> +			goto error;
> +
> +		if (do_read_u32(ff, &env_pmu_caps[i].nr_caps))
> +			goto free_pmu_name;
> +
> +		if (!__process_pmu_caps(ff, &env_pmu_caps[i]))
> +			continue;
> +
> +free_pmu_name:
> +		free(env_pmu_caps[i].pmu_name);
> +		goto error;
> +	}
> +
> +	ff->ph->env.nr_pmus_with_caps = nr_pmus;
> +	ff->ph->env.env_pmu_caps = env_pmu_caps;
> +	return 0;
> +
> +error:
> +	for (; i > 0; i--) {
> +		free(env_pmu_caps[i - 1].pmu_name);
> +		for (j = 0; j < env_pmu_caps[i - 1].nr_caps; j++)
> +			free(env_pmu_caps[i - 1].pmu_caps[j]);
> +		free(env_pmu_caps[i - 1].pmu_caps);
> +	}
> +	free(env_pmu_caps);
> +	return -1;
> +}
> +
>  #define FEAT_OPR(n, func, __full_only) \
>  	[HEADER_##n] = {					\
>  		.name	    = __stringify(n),			\
> @@ -3382,6 +3566,7 @@ const struct perf_header_feature_ops feat_ops[HEADER_LAST_FEATURE] = {
>  	FEAT_OPR(CLOCK_DATA,	clock_data,	false),
>  	FEAT_OPN(HYBRID_TOPOLOGY,	hybrid_topology,	true),
>  	FEAT_OPR(HYBRID_CPU_PMU_CAPS,	hybrid_cpu_pmu_caps,	false),
> +	FEAT_OPR(PMU_CAPS,	pmu_caps,	false),
>  };
>  
>  struct header_print_data {
> diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
> index 0eb4bc29a5a4..e9a067bb8b9e 100644
> --- a/tools/perf/util/header.h
> +++ b/tools/perf/util/header.h
> @@ -47,6 +47,7 @@ enum {
>  	HEADER_CLOCK_DATA,
>  	HEADER_HYBRID_TOPOLOGY,
>  	HEADER_HYBRID_CPU_PMU_CAPS,
> +	HEADER_PMU_CAPS,
>  	HEADER_LAST_FEATURE,
>  	HEADER_FEAT_BITS	= 256,
>  };
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 9a1c7e63e663..8d599acb7569 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1890,16 +1890,22 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
>  	const char *sysfs = sysfs__mountpoint();
>  	DIR *caps_dir;
>  	struct dirent *evt_ent;
> -	int nr_caps = 0;
> +
> +	if (pmu->caps_initialized)
> +		return pmu->nr_caps;
>  
>  	if (!sysfs)
>  		return -1;
>  
> +	pmu->nr_caps = 0;
> +
>  	snprintf(caps_path, PATH_MAX,
>  		 "%s" EVENT_SOURCE_DEVICE_PATH "%s/caps", sysfs, pmu->name);
>  
> -	if (stat(caps_path, &st) < 0)
> +	if (stat(caps_path, &st) < 0) {
> +		pmu->caps_initialized = true;
>  		return 0;	/* no error if caps does not exist */
> +	}
>  
>  	caps_dir = opendir(caps_path);
>  	if (!caps_dir)
> @@ -1926,13 +1932,14 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
>  			continue;
>  		}
>  
> -		nr_caps++;
> +		pmu->nr_caps++;
>  		fclose(file);
>  	}
>  
>  	closedir(caps_dir);
>  
> -	return nr_caps;
> +	pmu->caps_initialized = true;
> +	return pmu->nr_caps;
>  }
>  
>  void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 541889fa9f9c..4b45fd8da5a3 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -46,6 +46,8 @@ struct perf_pmu {
>  	struct perf_cpu_map *cpus;
>  	struct list_head format;  /* HEAD struct perf_pmu_format -> list */
>  	struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
> +	bool caps_initialized;
> +	u32 nr_caps;
>  	struct list_head caps;    /* HEAD struct perf_pmu_caps -> list */
>  	struct list_head list;    /* ELEM */
>  	struct list_head hybrid_list;
> -- 
> 2.31.1
>