Message-ID: <CAP-5=fWS=tZs_-G72p8sHnJY6AdRttQ+2swuo=HZ=hjU-ZjP4A@mail.gmail.com>
Date:   Wed, 23 Aug 2023 19:23:52 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        James Clark <james.clark@....com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        John Garry <john.g.garry@...cle.com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        Jing Zhang <renyu.zj@...ux.alibaba.com>,
        Ravi Bangoria <ravi.bangoria@....com>,
        Rob Herring <robh@...nel.org>,
        Gaosheng Cui <cuigaosheng1@...wei.com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 09/25] perf pmu: Make the loading of formats lazy

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

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

Powered by blists - more mailing lists