| 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: <CAP-5=fVK3g-8oMoBUAHSR=sufQrU_Xed22Xf0Eu6JGbhDu9e5w@mail.gmail.com>
Date: Thu, 25 Jul 2024 20:41:14 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, 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 v2] perf annotate: Cache debuginfo for data type profiling
On Thu, Jul 25, 2024 at 6:57 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> In find_data_type(), it creates and deletes a debug info whenver it
> tries to find data type for a sample. This is inefficient and it most
> likely accesses the same binary again and again.
>
> Let's add a single entry cache the debug info structure for the last DSO.
> Depending on sample data, it usually gives me 2~3x (and sometimes more)
> speed ups.
>
> Note that this will introduce a little difference in the output due to
> the order of checking stack operations. It used to check the stack ops
> before checking the availability of debug info but I moved it after the
> symbol check. So it'll report stack operations in DSOs without debug
> info as unknown. But I think it's ok and better to have the checking
> near the caching logic.
>
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> ---
> v2) add an assertion for the single-threaded case.
>
> tools/perf/util/annotate-data.c | 15 ++------------
> tools/perf/util/annotate-data.h | 2 +-
> tools/perf/util/annotate.c | 35 +++++++++++++++++++++++++++++++++
> tools/perf/util/annotate.h | 2 ++
> tools/perf/util/session.c | 2 ++
> 5 files changed, 42 insertions(+), 14 deletions(-)
>
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index 734acdd8c4b7..f125ac5f0bda 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -1345,16 +1345,9 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
> */
> struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
> {
> - struct annotated_data_type *result = NULL;
> struct dso *dso = map__dso(dloc->ms->map);
> Dwarf_Die type_die;
>
> - dloc->di = debuginfo__new(dso__long_name(dso));
> - if (dloc->di == NULL) {
> - pr_debug_dtp("cannot get the debug info\n");
> - return NULL;
> - }
> -
> /*
> * The type offset is the same as instruction offset by default.
> * But when finding a global variable, the offset won't be valid.
> @@ -1364,13 +1357,9 @@ struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
> dloc->fbreg = -1;
>
> if (find_data_type_die(dloc, &type_die) < 0)
> - goto out;
> -
> - result = dso__findnew_data_type(dso, &type_die);
> + return NULL;
>
> -out:
> - debuginfo__delete(dloc->di);
> - return result;
> + return dso__findnew_data_type(dso, &type_die);
> }
>
> static int alloc_data_type_histograms(struct annotated_data_type *adt, int nr_entries)
> diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
> index 992b7ce4bd11..37a1a3b68e0b 100644
> --- a/tools/perf/util/annotate-data.h
> +++ b/tools/perf/util/annotate-data.h
> @@ -123,9 +123,9 @@ struct data_loc_info {
> u64 var_addr;
> u8 cpumode;
> struct annotated_op_loc *op;
> + struct debuginfo *di;
>
> /* These are used internally */
> - struct debuginfo *di;
> int fbreg;
> bool fb_cfa;
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index a2ee4074f768..2ef119539336 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -25,6 +25,7 @@
> #include "srcline.h"
> #include "units.h"
> #include "debug.h"
> +#include "debuginfo.h"
> #include "annotate.h"
> #include "annotate-data.h"
> #include "evsel.h"
> @@ -2326,6 +2327,20 @@ u64 annotate_calc_pcrel(struct map_symbol *ms, u64 ip, int offset,
> return map__rip_2objdump(ms->map, addr);
> }
>
> +static struct debuginfo_cache {
> + struct dso *dso;
> + struct debuginfo *dbg;
> +} di_cache;
> +
> +void debuginfo_cache__delete(void)
> +{
> + dso__put(di_cache.dso);
> + di_cache.dso = NULL;
> +
> + debuginfo__delete(di_cache.dbg);
> + di_cache.dbg = NULL;
> +}
> +
> /**
> * hist_entry__get_data_type - find data type for given hist entry
> * @he: hist entry
> @@ -2360,6 +2375,25 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
> return NULL;
> }
>
> + /*
> + * It needs to protect di_cache as well as data_types and global_vars in
> + * DSO when enabling multi-threading.
Perhaps:
/*
* di_cache holds a pair of values, but code below assumes
di_cache.dso can be compared/updated and di_cache.dbg can be
read/updated independently from each other. That assumption only holds
in single threaded code.
*/
Thanks,
Ian
> + */
> + assert(perf_singlethreaded);
> +
> + if (map__dso(ms->map) != di_cache.dso) {
> + dso__put(di_cache.dso);
> + di_cache.dso = dso__get(map__dso(ms->map));
> +
> + debuginfo__delete(di_cache.dbg);
> + di_cache.dbg = debuginfo__new(dso__long_name(di_cache.dso));
> + }
> +
> + if (di_cache.dbg == NULL) {
> + ann_data_stat.no_dbginfo++;
> + return NULL;
> + }
> +
> /* Make sure it has the disasm of the function */
> if (symbol__annotate(ms, evsel, &arch) < 0) {
> ann_data_stat.no_insn++;
> @@ -2404,6 +2438,7 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
> .ip = ms->sym->start + dl->al.offset,
> .cpumode = he->cpumode,
> .op = op_loc,
> + .di = di_cache.dbg,
> };
>
> if (!op_loc->mem_ref && op_loc->segment == INSN_SEG_NONE)
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 9ba772f46270..27d9540604ef 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -543,4 +543,6 @@ struct annotated_basic_block {
> int annotate_get_basic_blocks(struct symbol *sym, s64 src, s64 dst,
> struct list_head *head);
>
> +void debuginfo_cache__delete(void);
> +
> #endif /* __PERF_ANNOTATE_H */
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 5596bed1b8c8..f9072e003367 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -36,6 +36,7 @@
> #include "util.h"
> #include "arch/common.h"
> #include "units.h"
> +#include "annotate.h"
> #include <internal/lib.h>
>
> #ifdef HAVE_ZSTD_SUPPORT
> @@ -304,6 +305,7 @@ void perf_session__delete(struct perf_session *session)
> return;
> auxtrace__free(session);
> auxtrace_index__free(&session->auxtrace_index);
> + debuginfo_cache__delete();
> perf_session__destroy_kernel_maps(session);
> perf_decomp__release_events(session->decomp_data.decomp);
> perf_env__exit(&session->header.env);
> --
> 2.46.0.rc1.232.g9752f9e123-goog
>
Powered by blists - more mailing lists