| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aKZeRh3WqXCYyx1v@google.com>
Date: Wed, 20 Aug 2025 16:46:14 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: Ian Rogers <irogers@...gle.com>, Kan Liang <kan.liang@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v5 12/12] perf annotate: Use a hashmap to save type data
On Wed, Aug 20, 2025 at 06:37:18PM -0300, Arnaldo Carvalho de Melo wrote:
> On Fri, Aug 15, 2025 at 08:16:35PM -0700, Namhyung Kim wrote:
> > It can slowdown annotation browser if objdump is processing large DWARF
> > data. Let's add a hashmap to save the data type info for each line.
> >
> > Note that this is needed for TUI only because stdio only processes each
> > line once. TUI will display the same line whenever it refreshes the
> > screen.
> >
> > Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> > ---
> > tools/perf/ui/browsers/annotate.c | 34 ++++++++++++++++++++++++++++++-
> > tools/perf/util/annotate.c | 27 ++++++++++++++++++++++--
> > tools/perf/util/annotate.h | 2 ++
> > 3 files changed, 60 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> > index 9aa3c1ba22f52789..9677a3763a290a3d 100644
> > --- a/tools/perf/ui/browsers/annotate.c
> > +++ b/tools/perf/ui/browsers/annotate.c
> > @@ -6,6 +6,7 @@
> > #include "../../util/debug.h"
> > #include "../../util/debuginfo.h"
> > #include "../../util/dso.h"
> > +#include "../../util/hashmap.h"
> > #include "../../util/hist.h"
> > #include "../../util/sort.h"
> > #include "../../util/map.h"
> > @@ -15,6 +16,7 @@
> > #include "../../util/evlist.h"
> > #include "../../util/thread.h"
> > #include <inttypes.h>
> > +#include <linux/err.h>
> > #include <linux/kernel.h>
> > #include <linux/string.h>
> > #include <linux/zalloc.h>
> > @@ -36,6 +38,7 @@ struct annotate_browser {
> > struct hist_entry *he;
> > struct debuginfo *dbg;
> > struct evsel *evsel;
> > + struct hashmap *type_hash;
> > bool searching_backwards;
> > char search_bf[128];
> > };
> > @@ -43,6 +46,16 @@ struct annotate_browser {
> > /* A copy of target hist_entry for perf top. */
> > static struct hist_entry annotate_he;
> >
> > +static size_t type_hash(long key, void *ctx __maybe_unused)
> > +{
> > + return key;
> > +}
> > +
> > +static bool type_equal(long key1, long key2, void *ctx __maybe_unused)
> > +{
> > + return key1 == key2;
> > +}
> > +
> > static inline struct annotation *browser__annotation(struct ui_browser *browser)
> > {
> > struct map_symbol *ms = browser->priv;
> > @@ -130,6 +143,9 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
> > if (!browser->navkeypressed)
> > ops.width += 1;
> >
> > + if (!IS_ERR_OR_NULL(ab->type_hash))
> > + apd.type_hash = ab->type_hash;
> > +
> > annotation_line__write(al, notes, &ops, &apd);
> >
> > if (ops.current_entry)
> > @@ -1051,6 +1067,10 @@ static int annotate_browser__run(struct annotate_browser *browser,
> > annotate_opts.code_with_type ^= 1;
> > if (browser->dbg == NULL)
> > browser->dbg = dso__debuginfo(map__dso(ms->map));
> > + if (browser->type_hash == NULL) {
> > + browser->type_hash = hashmap__new(type_hash, type_equal,
> > + /*ctx=*/NULL);
> > + }
> > annotate_browser__show(&browser->b, title, help);
> > annotate_browser__debuginfo_warning(browser);
> > continue;
> > @@ -1145,8 +1165,10 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
> >
> > ui_helpline__push("Press ESC to exit");
> >
> > - if (annotate_opts.code_with_type)
> > + if (annotate_opts.code_with_type) {
> > browser.dbg = dso__debuginfo(dso);
> > + browser.type_hash = hashmap__new(type_hash, type_equal, /*ctx=*/NULL);
> > + }
> >
> > browser.b.width = notes->src->widths.max_line_len;
> > browser.b.nr_entries = notes->src->nr_entries;
> > @@ -1159,6 +1181,16 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
> > ret = annotate_browser__run(&browser, evsel, hbt);
> >
> > debuginfo__delete(browser.dbg);
> > +
> > + if (!IS_ERR_OR_NULL(browser.type_hash)) {
> > + struct hashmap_entry *cur;
> > + size_t bkt;
> > +
> > + hashmap__for_each_entry(browser.type_hash, cur, bkt)
> > + free(cur->pvalue);
>
> zfree(&cur->pvalue);
Yep. :)
>
> > + hashmap__free(browser.type_hash);
> > + }
> > +
> > if (not_annotated && !notes->src->tried_source)
> > annotated_source__purge(notes->src);
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index bea3457a00632fd7..77414e04d99bb4f2 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -1954,11 +1954,17 @@ int annotation_br_cntr_entry(char **str, int br_cntr_nr,
> > return -ENOMEM;
> > }
> >
> > +struct type_hash_entry {
> > + struct annotated_data_type *type;
> > + int offset;
> > +};
> > +
> > static int disasm_line__snprint_type_info(struct disasm_line *dl,
> > char *buf, int len,
> > struct annotation_print_data *apd)
> > {
> > - struct annotated_data_type *data_type;
> > + struct annotated_data_type *data_type = NULL;
> > + struct type_hash_entry *entry = NULL;
> > char member[256];
> > int offset = 0;
> > int printed;
> > @@ -1968,7 +1974,24 @@ static int disasm_line__snprint_type_info(struct disasm_line *dl,
> > if (!annotate_opts.code_with_type || apd->dbg == NULL)
> > return 1;
> >
> > - data_type = __hist_entry__get_data_type(apd->he, apd->arch, apd->dbg, dl, &offset);
> > + if (apd->type_hash) {
> > + hashmap__find(apd->type_hash, dl->al.offset, &entry);
> > + if (entry != NULL) {
> > + data_type = entry->type;
> > + offset = entry->offset;
> > + }
> > + }
> > + if (data_type == NULL)
> > + data_type = __hist_entry__get_data_type(apd->he, apd->arch, apd->dbg, dl, &offset);
>
>
> add space?
Sure.
>
> > + if (apd->type_hash && entry == NULL) {
> > + entry = malloc(sizeof(*entry));
>
> Is the 'entry' variable needed anywhere else? Not, so could be declared
> here to save a line at the start of the function. Or is it used in a
> later patch outside of this scope?
It was used in the earlier block to look up an existing entry.
Thanks,
Namhyung
>
> > + if (entry != NULL) {
> > + entry->type = data_type;
> > + entry->offset = offset;
> > + hashmap__add(apd->type_hash, dl->al.offset, entry);
> > + }
> > + }
> > +
> > if (!needs_type_info(data_type))
> > return 1;
> >
> > diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> > index 86e858f5bf173152..eaf6c8aa7f473959 100644
> > --- a/tools/perf/util/annotate.h
> > +++ b/tools/perf/util/annotate.h
> > @@ -204,6 +204,8 @@ struct annotation_print_data {
> > struct evsel *evsel;
> > struct arch *arch;
> > struct debuginfo *dbg;
> > + /* save data type info keyed by al->offset */
> > + struct hashmap *type_hash;
> > /* It'll be set in hist_entry__annotate_printf() */
> > int addr_fmt_width;
> > };
> > --
> > 2.50.1
> >
Powered by blists - more mailing lists