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);
+
+}
+
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;
+ }
+ 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-googHi 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
>
On Fri, Mar 22, 2024 at 1:43 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> 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>
> > ---
[SNIP]
> > 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.
Hmm.. I saw this code was replaced by the next patch.
Then probably it'd be ok to leave it.
Thanks,
Namhyung
>
> > + }
> > + up_read(&dsos->lock);
> > + return 0;
> > +}
© 2016 - 2026 Red Hat, Inc.