Re: [PATCH v2 04/13] perf dsos: Add dsos__for_each_dso

[Namhyung Kim <namhyung@kernel.org>]

Hi Ian,

On Thu, Mar 21, 2024 at 9:03 AM Ian Rogers <irogers@google.com> wrote:
>
> To better abstract the dsos internals, add dsos__for_each_dso that
> does a callback on each dso. This also means the read lock can be
> correctly held.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-inject.c | 25 +++++++-----
>  tools/perf/util/build-id.c  | 76 ++++++++++++++++++++-----------------
>  tools/perf/util/dsos.c      | 16 ++++++++
>  tools/perf/util/dsos.h      |  8 +---
>  tools/perf/util/machine.c   | 40 +++++++++++--------
>  5 files changed, 100 insertions(+), 65 deletions(-)
>
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index ef73317e6ae7..ce5e28eaad90 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -1187,23 +1187,28 @@ static int synthesize_build_id(struct perf_inject *inject, struct dso *dso, pid_
>                                                process_build_id, machine);
>  }
>
> +static int guest_session__add_build_ids_cb(struct dso *dso, void *data)
> +{
> +       struct guest_session *gs = data;
> +       struct perf_inject *inject = container_of(gs, struct perf_inject, guest_session);
> +
> +       if (!dso->has_build_id)
> +               return 0;
> +
> +       return synthesize_build_id(inject, dso, gs->machine_pid);
> +

An unnecessary newline.

> +}
> +
>  static int guest_session__add_build_ids(struct guest_session *gs)
>  {
>         struct perf_inject *inject = container_of(gs, struct perf_inject, guest_session);
> -       struct machine *machine = &gs->session->machines.host;
> -       struct dso *dso;
> -       int ret;
>
>         /* Build IDs will be put in the Build ID feature section */
>         perf_header__set_feat(&inject->session->header, HEADER_BUILD_ID);
>
> -       dsos__for_each_with_build_id(dso, &machine->dsos.head) {
> -               ret = synthesize_build_id(inject, dso, gs->machine_pid);
> -               if (ret)
> -                       return ret;
> -       }
> -
> -       return 0;
> +       return dsos__for_each_dso(&gs->session->machines.host.dsos,
> +                                 guest_session__add_build_ids_cb,
> +                                 gs);
>  }
>
>  static int guest_session__ksymbol_event(struct perf_tool *tool,
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index a617b1917e6b..a6d3c253f19f 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -327,48 +327,56 @@ static int write_buildid(const char *name, size_t name_len, struct build_id *bid
>         return write_padded(fd, name, name_len + 1, len);
>  }
>
> -static int machine__write_buildid_table(struct machine *machine,
> -                                       struct feat_fd *fd)
> +struct machine__write_buildid_table_cb_args {
> +       struct machine *machine;
> +       struct feat_fd *fd;
> +       u16 kmisc, umisc;
> +};
> +
> +static int machine__write_buildid_table_cb(struct dso *dso, void *data)
>  {
> -       int err = 0;
> -       struct dso *pos;
> -       u16 kmisc = PERF_RECORD_MISC_KERNEL,
> -           umisc = PERF_RECORD_MISC_USER;
> +       struct machine__write_buildid_table_cb_args *args = data;
> +       const char *name;
> +       size_t name_len;
> +       bool in_kernel = false;
>
> -       if (!machine__is_host(machine)) {
> -               kmisc = PERF_RECORD_MISC_GUEST_KERNEL;
> -               umisc = PERF_RECORD_MISC_GUEST_USER;
> -       }
> +       if (!dso->has_build_id)
> +               return 0;
>
> -       dsos__for_each_with_build_id(pos, &machine->dsos.head) {
> -               const char *name;
> -               size_t name_len;
> -               bool in_kernel = false;
> +       if (!dso->hit && !dso__is_vdso(dso))
> +               return 0;
>
> -               if (!pos->hit && !dso__is_vdso(pos))
> -                       continue;
> +       if (dso__is_vdso(dso)) {
> +               name = dso->short_name;
> +               name_len = dso->short_name_len;
> +       } else if (dso__is_kcore(dso)) {
> +               name = args->machine->mmap_name;
> +               name_len = strlen(name);
> +       } else {
> +               name = dso->long_name;
> +               name_len = dso->long_name_len;
> +       }
>
> -               if (dso__is_vdso(pos)) {
> -                       name = pos->short_name;
> -                       name_len = pos->short_name_len;
> -               } else if (dso__is_kcore(pos)) {
> -                       name = machine->mmap_name;
> -                       name_len = strlen(name);
> -               } else {
> -                       name = pos->long_name;
> -                       name_len = pos->long_name_len;
> -               }
> +       in_kernel = dso->kernel || is_kernel_module(name, PERF_RECORD_MISC_CPUMODE_UNKNOWN);
> +       return write_buildid(name, name_len, &dso->bid, args->machine->pid,
> +                            in_kernel ? args->kmisc : args->umisc, args->fd);
> +}
>
> -               in_kernel = pos->kernel ||
> -                               is_kernel_module(name,
> -                                       PERF_RECORD_MISC_CPUMODE_UNKNOWN);
> -               err = write_buildid(name, name_len, &pos->bid, machine->pid,
> -                                   in_kernel ? kmisc : umisc, fd);
> -               if (err)
> -                       break;
> +static int machine__write_buildid_table(struct machine *machine, struct feat_fd *fd)
> +{
> +       struct machine__write_buildid_table_cb_args args = {
> +               .machine = machine,
> +               .fd = fd,
> +               .kmisc = PERF_RECORD_MISC_KERNEL,
> +               .umisc = PERF_RECORD_MISC_USER,
> +       };
> +
> +       if (!machine__is_host(machine)) {
> +               args.kmisc = PERF_RECORD_MISC_GUEST_KERNEL;
> +               args.umisc = PERF_RECORD_MISC_GUEST_USER;
>         }
>
> -       return err;
> +       return dsos__for_each_dso(&machine->dsos, machine__write_buildid_table_cb, &args);
>  }
>
>  int perf_session__write_buildid_table(struct perf_session *session,
> diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
> index d269e09005a7..d43f64939b12 100644
> --- a/tools/perf/util/dsos.c
> +++ b/tools/perf/util/dsos.c
> @@ -433,3 +433,19 @@ struct dso *dsos__find_kernel_dso(struct dsos *dsos)
>         up_read(&dsos->lock);
>         return res;
>  }
> +
> +int dsos__for_each_dso(struct dsos *dsos, int (*cb)(struct dso *dso, void *data), void *data)
> +{
> +       struct dso *dso;
> +
> +       down_read(&dsos->lock);
> +       list_for_each_entry(dso, &dsos->head, node) {
> +               int err;
> +
> +               err = cb(dso, data);
> +               if (err)
> +                       return err;

Please break and return the error to release the lock.

Thanks,
Namhyung


> +       }
> +       up_read(&dsos->lock);
> +       return 0;
> +}
> diff --git a/tools/perf/util/dsos.h b/tools/perf/util/dsos.h
> index a7c7f723c5ff..317a263f0e37 100644
> --- a/tools/perf/util/dsos.h
> +++ b/tools/perf/util/dsos.h
> @@ -23,12 +23,6 @@ struct dsos {
>         struct rw_semaphore lock;
>  };
>
> -#define dsos__for_each_with_build_id(pos, head)        \
> -       list_for_each_entry(pos, head, node)    \
> -               if (!pos->has_build_id)         \
> -                       continue;               \
> -               else
> -
>  void dsos__init(struct dsos *dsos);
>  void dsos__exit(struct dsos *dsos);
>
> @@ -55,4 +49,6 @@ struct dso *dsos__findnew_module_dso(struct dsos *dsos, struct machine *machine,
>
>  struct dso *dsos__find_kernel_dso(struct dsos *dsos);
>
> +int dsos__for_each_dso(struct dsos *dsos, int (*cb)(struct dso *dso, void *data), void *data);
> +
>  #endif /* __PERF_DSOS */
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index a06b030fba54..bd153bc2c8da 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1562,16 +1562,14 @@ int machine__create_kernel_maps(struct machine *machine)
>         return ret;
>  }
>
> -static bool machine__uses_kcore(struct machine *machine)
> +static int machine__uses_kcore_cb(struct dso *dso, void *data __maybe_unused)
>  {
> -       struct dso *dso;
> -
> -       list_for_each_entry(dso, &machine->dsos.head, node) {
> -               if (dso__is_kcore(dso))
> -                       return true;
> -       }
> +       return dso__is_kcore(dso) ? 1 : 0;
> +}
>
> -       return false;
> +static bool machine__uses_kcore(struct machine *machine)
> +{
> +       return dsos__for_each_dso(&machine->dsos, machine__uses_kcore_cb, NULL) != 0 ? true : false;
>  }
>
>  static bool perf_event__is_extra_kernel_mmap(struct machine *machine,
> @@ -3137,16 +3135,28 @@ char *machine__resolve_kernel_addr(void *vmachine, unsigned long long *addrp, ch
>         return sym->name;
>  }
>
> +struct machine__for_each_dso_cb_args {
> +       struct machine *machine;
> +       machine__dso_t fn;
> +       void *priv;
> +};
> +
> +static int machine__for_each_dso_cb(struct dso *dso, void *data)
> +{
> +       struct machine__for_each_dso_cb_args *args = data;
> +
> +       return args->fn(dso, args->machine, args->priv);
> +}
> +
>  int machine__for_each_dso(struct machine *machine, machine__dso_t fn, void *priv)
>  {
> -       struct dso *pos;
> -       int err = 0;
> +       struct machine__for_each_dso_cb_args args = {
> +               .machine = machine,
> +               .fn = fn,
> +               .priv = priv,
> +       };
>
> -       list_for_each_entry(pos, &machine->dsos.head, node) {
> -               if (fn(pos, machine, priv))
> -                       err = -1;
> -       }
> -       return err;
> +       return dsos__for_each_dso(&machine->dsos, machine__for_each_dso_cb, &args);
>  }
>
>  int machine__for_each_kernel_map(struct machine *machine, machine__map_t fn, void *priv)
> --
> 2.44.0.396.g6e790dbe36-goog
>