| Message ID | 20240321160300.1635121-5-irogers@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | dso/dsos memory savings and clean up | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 >
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; > > +}
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)
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(-)