From: Namhyung Kim <namhyung@kernel.org>
To: Ian Rogers <irogers@google.com>
Cc: "Peter Zijlstra" <peterz@infradead.org>,
"Ingo Molnar" <mingo@redhat.com>,
"Arnaldo Carvalho de Melo" <acme@kernel.org>,
"Mark Rutland" <mark.rutland@arm.com>,
"Alexander Shishkin" <alexander.shishkin@linux.intel.com>,
"Jiri Olsa" <jolsa@kernel.org>,
"Adrian Hunter" <adrian.hunter@intel.com>,
"James Clark" <james.clark@arm.com>,
"Athira Rajeev" <atrajeev@linux.vnet.ibm.com>,
"Colin Ian King" <colin.i.king@gmail.com>,
"Ahelenia Ziemiańska" <nabijaczleweli@nabijaczleweli.xyz>,
"Leo Yan" <leo.yan@linux.dev>, "Song Liu" <song@kernel.org>,
"Miguel Ojeda" <ojeda@kernel.org>,
"Liam Howlett" <liam.howlett@oracle.com>,
"Ilkka Koskinen" <ilkka@os.amperecomputing.com>,
"Ben Gainey" <ben.gainey@arm.com>,
"K Prateek Nayak" <kprateek.nayak@amd.com>,
"Kan Liang" <kan.liang@linux.intel.com>,
"Yanteng Si" <siyanteng@loongson.cn>,
"Ravi Bangoria" <ravi.bangoria@amd.com>,
"Sun Haiyong" <sunhaiyong@loongson.cn>,
"Changbin Du" <changbin.du@huawei.com>,
"Masami Hiramatsu" <mhiramat@kernel.org>,
zhaimingbing <zhaimingbing@cmss.chinamobile.com>,
"Paran Lee" <p4ranlee@gmail.com>, "Li Dong" <lidong@vivo.com>,
elfring@users.sourceforge.net, "Andi Kleen" <ak@linux.intel.com>,
"Markus Elfring" <Markus.Elfring@web.de>,
"Chengen Du" <chengen.du@canonical.com>,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
bpf@vger.kernel.org
Subject: Re: [PATCH v2 04/13] perf dsos: Add dsos__for_each_dso
Date: Fri, 22 Mar 2024 13:43:10 -0700 [thread overview]
Message-ID: <CAM9d7ciUwKrHsk3GencSDCRDEP0oUX6H99-uRmL=zf4gCgtdHQ@mail.gmail.com> (raw)
In-Reply-To: <20240321160300.1635121-5-irogers@google.com>
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
>
next prev parent reply other threads:[~2024-03-22 20:50 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-21 16:02 [PATCH v2 00/13] dso/dsos memory savings and clean up Ian Rogers
2024-03-21 16:02 ` [PATCH v2 01/13] perf dso: Reorder variables to save space in struct dso Ian Rogers
2024-03-21 20:28 ` Arnaldo Carvalho de Melo
2024-03-21 16:02 ` [PATCH v2 02/13] perf dsos: Attempt to better abstract dsos internals Ian Rogers
2024-03-21 16:02 ` [PATCH v2 03/13] perf dsos: Tidy reference counting and locking Ian Rogers
2024-03-21 16:02 ` [PATCH v2 04/13] perf dsos: Add dsos__for_each_dso Ian Rogers
2024-03-22 20:43 ` Namhyung Kim [this message]
2024-03-22 20:54 ` Namhyung Kim
2024-03-21 16:02 ` [PATCH v2 05/13] perf dso: Move dso functions out of dsos Ian Rogers
2024-03-21 16:02 ` [PATCH v2 06/13] perf dsos: Switch more loops to dsos__for_each_dso Ian Rogers
2024-03-21 16:02 ` [PATCH v2 07/13] perf dsos: Switch backing storage to array from rbtree/list Ian Rogers
2024-03-21 16:02 ` [PATCH v2 08/13] perf dsos: Remove __dsos__addnew Ian Rogers
2024-03-21 16:02 ` [PATCH v2 09/13] perf dsos: Remove __dsos__findnew_link_by_longname_id Ian Rogers
2024-03-21 16:02 ` [PATCH v2 10/13] perf dsos: Switch hand code to bsearch Ian Rogers
2024-03-21 16:02 ` [PATCH v2 11/13] perf dso: Add reference count checking and accessor functions Ian Rogers
2024-03-21 16:02 ` [PATCH v2 12/13] perf dso: Reference counting related fixes Ian Rogers
2024-03-25 17:22 ` Markus Elfring
2024-03-21 16:03 ` [PATCH v2 13/13] perf dso: Use container_of to avoid a pointer in dso_data Ian Rogers
2024-03-25 21:03 ` [PATCH v2 00/13] dso/dsos memory savings and clean up Namhyung Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAM9d7ciUwKrHsk3GencSDCRDEP0oUX6H99-uRmL=zf4gCgtdHQ@mail.gmail.com' \
--to=namhyung@kernel.org \
--cc=Markus.Elfring@web.de \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=ak@linux.intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=atrajeev@linux.vnet.ibm.com \
--cc=ben.gainey@arm.com \
--cc=bpf@vger.kernel.org \
--cc=changbin.du@huawei.com \
--cc=chengen.du@canonical.com \
--cc=colin.i.king@gmail.com \
--cc=elfring@users.sourceforge.net \
--cc=ilkka@os.amperecomputing.com \
--cc=irogers@google.com \
--cc=james.clark@arm.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=kprateek.nayak@amd.com \
--cc=leo.yan@linux.dev \
--cc=liam.howlett@oracle.com \
--cc=lidong@vivo.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mhiramat@kernel.org \
--cc=mingo@redhat.com \
--cc=nabijaczleweli@nabijaczleweli.xyz \
--cc=ojeda@kernel.org \
--cc=p4ranlee@gmail.com \
--cc=peterz@infradead.org \
--cc=ravi.bangoria@amd.com \
--cc=siyanteng@loongson.cn \
--cc=song@kernel.org \
--cc=sunhaiyong@loongson.cn \
--cc=zhaimingbing@cmss.chinamobile.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).