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

From: Ian Rogers
Date: Wed Aug 23 2023 - 22:24:56 EST

Next message: Baolin Wang: "Re: [PATCH 7/9] mm/compaction: factor out code to test if we should run compaction for target order"
Previous message: Mario Limonciello: "Re: [PATCH V3 0/7] AMD Pstate Preferred Core"
In reply to: Arnaldo Carvalho de Melo: "Re: [PATCH v1 09/25] perf pmu: Make the loading of formats lazy"
Next in thread: Ian Rogers: "[PATCH v1 07/25] perf pmu: Avoid passing format list to perf_pmu__format_bits"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Aug 23, 2023 at 4:54 AM Arnaldo Carvalho de Melo
<acme@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > ---
> > 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.

Thanks, Arnaldo I'll fix this and rebase on to tmp.perf-tools-next for v2.

> - 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);

The unchecked strdup was a copy-and-paste from here.

Thanks,
Ian

> > - 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

Next message: Baolin Wang: "Re: [PATCH 7/9] mm/compaction: factor out code to test if we should run compaction for target order"
Previous message: Mario Limonciello: "Re: [PATCH V3 0/7] AMD Pstate Preferred Core"
In reply to: Arnaldo Carvalho de Melo: "Re: [PATCH v1 09/25] perf pmu: Make the loading of formats lazy"
Next in thread: Ian Rogers: "[PATCH v1 07/25] perf pmu: Avoid passing format list to perf_pmu__format_bits"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]