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

From: Namhyung Kim
Date: Fri Mar 22 2024 - 16:50:57 EST

Next message: Guenter Roeck: "Re: [PATCH v3 07/11] mm: vmalloc: Offload free_vmap_area_lock lock"
Previous message: Andrew Morton: "Re: [PATCH v3 12/12] mm/gup: Handle hugetlb in the generic follow_page_mask code"
In reply to: Ian Rogers: "[PATCH v2 04/13] perf dsos: Add dsos__for_each_dso"
Next in thread: Namhyung Kim: "Re: [PATCH v2 04/13] perf dsos: Add dsos__for_each_dso"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Ian,

On Thu, Mar 21, 2024 at 9:03 AM Ian Rogers <irogers@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
>

Next message: Guenter Roeck: "Re: [PATCH v3 07/11] mm: vmalloc: Offload free_vmap_area_lock lock"
Previous message: Andrew Morton: "Re: [PATCH v3 12/12] mm/gup: Handle hugetlb in the generic follow_page_mask code"
In reply to: Ian Rogers: "[PATCH v2 04/13] perf dsos: Add dsos__for_each_dso"
Next in thread: Namhyung Kim: "Re: [PATCH v2 04/13] perf dsos: Add dsos__for_each_dso"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]