Re: [PATCH v1 09/25] perf pmu: Make the loading of formats lazy

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Wed, Aug 23, 2023 at 01:08:12AM -0700, Ian Rogers escreveu:
> The sysfs format files are loaded eagerly in a PMU. Add a flag so that
> we create the format but only load the contents when necessary.
> 
> Reduce the size of the value in struct perf_pmu_format and avoid holes
> so there is no additional space requirement.
> 
> For "perf stat -e cycles true" this reduces the number of openat calls
> from 648 to 573 (about 12%). The benchmark pmu scan speed is improved
> by roughly 5%.
> 
> Before: $ perf bench internals pmu-scan
> Computing performance of sysfs PMU event scan for 100 times
>   Average core PMU scanning took: 1061.100 usec (+- 9.965 usec)
>   Average PMU scanning took: 4725.300 usec (+- 260.599 usec)
> 
> After: $ perf bench internals pmu-scan
> Computing performance of sysfs PMU event scan for 100 times
>   Average core PMU scanning took: 989.170 usec (+- 6.873 usec)
>   Average PMU scanning took: 4520.960 usec (+- 251.272 usec)
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/pmu.c |   2 +-
>  tools/perf/util/pmu.c  | 140 +++++++++++++++++++++++++++--------------
>  tools/perf/util/pmu.h  |   5 +-
>  tools/perf/util/pmu.y  |  20 +++---
>  4 files changed, 102 insertions(+), 65 deletions(-)
> 
> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> index 2c1c349a42e2..c204ed1f1a8b 100644
> --- a/tools/perf/tests/pmu.c
> +++ b/tools/perf/tests/pmu.c
> @@ -171,7 +171,7 @@ static int test__pmu(struct test_suite *test __maybe_unused, int subtest __maybe
>  	}
>  
>  	pmu->name = strdup("perf-pmu-test");
> -	ret = perf_pmu__format_parse(pmu, fd);
> +	ret = perf_pmu__format_parse(pmu, fd, /*eager_load=*/true);
>  	if (ret)
>  		goto out;
>  
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 42f3249994ab..7c3de51bab08 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -40,6 +40,10 @@ struct perf_pmu perf_pmu__fake;
>   * value=PERF_PMU_FORMAT_VALUE_CONFIG and bits 0 to 7 will be set.
>   */
>  struct perf_pmu_format {
> +	/** @list: Element on list within struct perf_pmu. */
> +	struct list_head list;
> +	/** @bits: Which config bits are set by this format value. */
> +	DECLARE_BITMAP(bits, PERF_PMU_FORMAT_BITS);
>  	/** @name: The modifier/file name. */
>  	char *name;
>  	/**
> @@ -47,18 +51,75 @@ struct perf_pmu_format {
>  	 * are from PERF_PMU_FORMAT_VALUE_CONFIG to
>  	 * PERF_PMU_FORMAT_VALUE_CONFIG_END.
>  	 */
> -	int value;
> -	/** @bits: Which config bits are set by this format value. */
> -	DECLARE_BITMAP(bits, PERF_PMU_FORMAT_BITS);
> -	/** @list: Element on list within struct perf_pmu. */
> -	struct list_head list;
> +	u16 value;
> +	/** @loaded: Has the contents been loaded/parsed. */
> +	bool loaded;
>  };
>  
> +static struct perf_pmu_format *perf_pmu__new_format(struct list_head *list, char *name)
> +{
> +	struct perf_pmu_format *format;
> +
> +	format = zalloc(sizeof(*format));
> +	if (!format)
> +		return NULL;
> +
> +	format->name = strdup(name);

We need to check this for failure, later you assume it is non-NULL,
calling strlen() on it, etc.

I applied all the previous patches, will review the others later.

- Arnaldo

> +	list_add_tail(&format->list, list);
> +	return format;
> +}
> +
> +/* Called at the end of parsing a format. */
> +void perf_pmu_format__set_value(void *vformat, int config, unsigned long *bits)
> +{
> +	struct perf_pmu_format *format = vformat;
> +
> +	format->value = config;
> +	memcpy(format->bits, bits, sizeof(format->bits));
> +}
> +
> +static void __perf_pmu_format__load(struct perf_pmu_format *format, FILE *file)
> +{
> +	void *scanner;
> +	int ret;
> +
> +	ret = perf_pmu_lex_init(&scanner);
> +	if (ret)
> +		return;
> +
> +	perf_pmu_set_in(file, scanner);
> +	ret = perf_pmu_parse(format, scanner);
> +	perf_pmu_lex_destroy(scanner);
> +	format->loaded = true;
> +}
> +
> +static void perf_pmu_format__load(struct perf_pmu *pmu, struct perf_pmu_format *format)
> +{
> +	char path[PATH_MAX];
> +	FILE *file = NULL;
> +
> +	if (format->loaded)
> +		return;
> +
> +	if (!perf_pmu__pathname_scnprintf(path, sizeof(path), pmu->name, "format"))
> +		return;
> +
> +	assert(strlen(path) + strlen(format->name) + 2 < sizeof(path));
> +	strcat(path, "/");
> +	strcat(path, format->name);
> +
> +	file = fopen(path, "r");
> +	if (!file)
> +		return;
> +	__perf_pmu_format__load(format, file);
> +	fclose(file);
> +}
> +
>  /*
>   * Parse & process all the sysfs attributes located under
>   * the directory specified in 'dir' parameter.
>   */
> -int perf_pmu__format_parse(struct perf_pmu *pmu, int dirfd)
> +int perf_pmu__format_parse(struct perf_pmu *pmu, int dirfd, bool eager_load)
>  {
>  	struct dirent *evt_ent;
>  	DIR *format_dir;
> @@ -68,37 +129,35 @@ int perf_pmu__format_parse(struct perf_pmu *pmu, int dirfd)
>  	if (!format_dir)
>  		return -EINVAL;
>  
> -	while (!ret && (evt_ent = readdir(format_dir))) {
> +	while ((evt_ent = readdir(format_dir)) != NULL) {
> +		struct perf_pmu_format *format;
>  		char *name = evt_ent->d_name;
> -		int fd;
> -		void *scanner;
> -		FILE *file;
>  
>  		if (!strcmp(name, ".") || !strcmp(name, ".."))
>  			continue;
>  
> -
> -		ret = -EINVAL;
> -		fd = openat(dirfd, name, O_RDONLY);
> -		if (fd < 0)
> -			break;
> -
> -		file = fdopen(fd, "r");
> -		if (!file) {
> -			close(fd);
> +		format = perf_pmu__new_format(&pmu->format, name);
> +		if (!format) {
> +			ret = -ENOMEM;
>  			break;
>  		}
>  
> -		ret = perf_pmu_lex_init(&scanner);
> -		if (ret) {
> +		if (eager_load) {
> +			FILE *file;
> +			int fd = openat(dirfd, name, O_RDONLY);
> +
> +			if (fd < 0) {
> +				ret = -errno;
> +				break;
> +			}
> +			file = fdopen(fd, "r");
> +			if (!file) {
> +				close(fd);
> +				break;
> +			}
> +			__perf_pmu_format__load(format, file);
>  			fclose(file);
> -			break;
>  		}
> -
> -		perf_pmu_set_in(file, scanner);
> -		ret = perf_pmu_parse(&pmu->format, name, scanner);
> -		perf_pmu_lex_destroy(scanner);
> -		fclose(file);
>  	}
>  
>  	closedir(format_dir);
> @@ -119,7 +178,7 @@ static int pmu_format(struct perf_pmu *pmu, int dirfd, const char *name)
>  		return 0;
>  
>  	/* it'll close the fd */
> -	if (perf_pmu__format_parse(pmu, fd))
> +	if (perf_pmu__format_parse(pmu, fd, /*eager_load=*/false))
>  		return -1;
>  
>  	return 0;
> @@ -962,13 +1021,15 @@ void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu)
>  	if (pmu == &perf_pmu__fake)
>  		return;
>  
> -	list_for_each_entry(format, &pmu->format, list)
> +	list_for_each_entry(format, &pmu->format, list) {
> +		perf_pmu_format__load(pmu, format);
>  		if (format->value >= PERF_PMU_FORMAT_VALUE_CONFIG_END) {
>  			pr_warning("WARNING: '%s' format '%s' requires 'perf_event_attr::config%d'"
>  				   "which is not supported by this version of perf!\n",
>  				   pmu->name, format->name, format->value);
>  			return;
>  		}
> +	}
>  }
>  
>  bool evsel__is_aux_event(const struct evsel *evsel)
> @@ -1041,6 +1102,7 @@ int perf_pmu__format_type(struct perf_pmu *pmu, const char *name)
>  	if (!format)
>  		return -1;
>  
> +	perf_pmu_format__load(pmu, format);
>  	return format->value;
>  }
>  
> @@ -1177,7 +1239,7 @@ static int pmu_config_term(struct perf_pmu *pmu,
>  		free(pmu_term);
>  		return -EINVAL;
>  	}
> -
> +	perf_pmu_format__load(pmu, format);
>  	switch (format->value) {
>  	case PERF_PMU_FORMAT_VALUE_CONFIG:
>  		vp = &attr->config;
> @@ -1403,24 +1465,6 @@ struct perf_pmu_alias *perf_pmu__find_alias(struct perf_pmu *pmu, const char *ev
>  
>  	return NULL;
>  }
> -
> -int perf_pmu__new_format(struct list_head *list, char *name,
> -			 int config, unsigned long *bits)
> -{
> -	struct perf_pmu_format *format;
> -
> -	format = zalloc(sizeof(*format));
> -	if (!format)
> -		return -ENOMEM;
> -
> -	format->name = strdup(name);
> -	format->value = config;
> -	memcpy(format->bits, bits, sizeof(format->bits));
> -
> -	list_add_tail(&format->list, list);
> -	return 0;
> -}
> -
>  static void perf_pmu__del_formats(struct list_head *formats)
>  {
>  	struct perf_pmu_format *fmt, *tmp;
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index c4268053c979..675c9b97f7bf 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -227,9 +227,8 @@ int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
>  			  struct perf_pmu_info *info);
>  struct perf_pmu_alias *perf_pmu__find_alias(struct perf_pmu *pmu, const char *event);
>  
> -int perf_pmu__new_format(struct list_head *list, char *name,
> -			 int config, unsigned long *bits);
> -int perf_pmu__format_parse(struct perf_pmu *pmu, int dirfd);
> +int perf_pmu__format_parse(struct perf_pmu *pmu, int dirfd, bool eager_load);
> +void perf_pmu_format__set_value(void *format, int config, unsigned long *bits);
>  bool perf_pmu__has_format(const struct perf_pmu *pmu, const char *name);
>  
>  bool is_pmu_core(const char *name);
> diff --git a/tools/perf/util/pmu.y b/tools/perf/util/pmu.y
> index d861a5bfa3bd..600c8c158c8e 100644
> --- a/tools/perf/util/pmu.y
> +++ b/tools/perf/util/pmu.y
> @@ -1,6 +1,5 @@
>  %define api.pure full
> -%parse-param {struct list_head *format}
> -%parse-param {char *name}
> +%parse-param {void *format}
>  %parse-param {void *scanner}
>  %lex-param {void* scanner}
>  
> @@ -21,7 +20,7 @@ do { \
>                  YYABORT; \
>  } while (0)
>  
> -static void perf_pmu_error(struct list_head *list, char *name, void *scanner, char const *msg);
> +static void perf_pmu_error(void *format, void *scanner, const char *msg);
>  
>  static void perf_pmu__set_format(unsigned long *bits, long from, long to)
>  {
> @@ -59,16 +58,12 @@ format_term
>  format_term:
>  PP_CONFIG ':' bits
>  {
> -	ABORT_ON(perf_pmu__new_format(format, name,
> -				      PERF_PMU_FORMAT_VALUE_CONFIG,
> -				      $3));
> +	perf_pmu_format__set_value(format, PERF_PMU_FORMAT_VALUE_CONFIG, $3);
>  }
>  |
>  PP_CONFIG PP_VALUE ':' bits
>  {
> -	ABORT_ON(perf_pmu__new_format(format, name,
> -				      $2,
> -				      $4));
> +	perf_pmu_format__set_value(format, $2, $4);
>  }
>  
>  bits:
> @@ -95,9 +90,8 @@ PP_VALUE
>  
>  %%
>  
> -static void perf_pmu_error(struct list_head *list __maybe_unused,
> -		    char *name __maybe_unused,
> -		    void *scanner __maybe_unused,
> -		    char const *msg __maybe_unused)
> +static void perf_pmu_error(void *format __maybe_unused,
> +			   void *scanner __maybe_unused,
> +			   const char *msg __maybe_unused)
>  {
>  }
> -- 
> 2.42.0.rc1.204.g551eb34607-goog
> 

-- 

- Arnaldo