Received-SPF: pass (google.com: domain of linux-kernel+bounces-107976-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240319055115.4063940-1-namhyung@kernel.org> <20240319055115.4063940-23-namhyung@kernel.org>
In-Reply-To: <20240319055115.4063940-23-namhyung@kernel.org>
From: Namhyung Kim <namhyung@kernel.org>
Date: Tue, 19 Mar 2024 11:05:04 -0700
Message-ID: <CAM9d7cjhuxB_iWPPWgFxK1_oBaYqF=MgthV=EBG0HeyJ9-ONXg@mail.gmail.com>
Subject: Re: [PATCH 22/23] perf annotate-data: Add a cache for global variable types
To: Arnaldo Carvalho de Melo <acme@kernel.org>, Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>, Adrian Hunter <adrian.hunter@intel.com>, 
	Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@kernel.org>, 
	LKML <linux-kernel@vger.kernel.org>, linux-perf-users@vger.kernel.org, 
	Linus Torvalds <torvalds@linux-foundation.org>, Stephane Eranian <eranian@google.com>, 
	Masami Hiramatsu <mhiramat@kernel.org>, linux-toolchains@vger.kernel.org, 
	linux-trace-devel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Mar 18, 2024 at 10:56=E2=80=AFPM Namhyung Kim <namhyung@kernel.org>=
 wrote:
>
> They are often searched by many different places.  Let's add a cache
> for them to reduce the duplicate DWARF access.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/annotate-data.c | 107 +++++++++++++++++++++++++++++++-
>  tools/perf/util/annotate-data.h |   7 +++
>  tools/perf/util/dso.c           |   2 +
>  tools/perf/util/dso.h           |   6 +-
>  4 files changed, 118 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-d=
ata.c
> index 633fe125fcd8..4b3184b7c799 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -433,6 +433,91 @@ static struct type_state_stack *findnew_stack_state(=
struct type_state *state,
>         return stack;
>  }
>
> +/* Maintain a cache for quick global variable lookup */
> +struct global_var_entry {
> +       struct rb_node node;
> +       char *name;
> +       u64 start;
> +       u64 end;
> +       u64 die_offset;
> +};
> +
> +static int global_var_cmp(const void *_key, const struct rb_node *node)
> +{
> +       const u64 addr =3D (uintptr_t)_key;
> +       struct global_var_entry *gvar;
> +
> +       gvar =3D rb_entry(node, struct global_var_entry, node);
> +
> +       if (gvar->start <=3D addr && addr < gvar->end)
> +               return 0;
> +       return gvar->start > addr ? -1 : 1;
> +}
> +
> +static bool global_var_less(struct rb_node *node_a, const struct rb_node=
 *node_b)
> +{
> +       struct global_var_entry *gvar_a, *gvar_b;
> +
> +       gvar_a =3D rb_entry(node_a, struct global_var_entry, node);
> +       gvar_b =3D rb_entry(node_b, struct global_var_entry, node);
> +
> +       return gvar_a->start < gvar_b->start;
> +}
> +
> +static struct global_var_entry *global_var__find(struct data_loc_info *d=
loc, u64 addr)
> +{
> +       struct dso *dso =3D map__dso(dloc->ms->map);
> +       struct rb_node *node;
> +
> +       node =3D rb_find((void *)addr, &dso->global_vars, global_var_cmp)=
;

It seems to cause a build error on 32-bit systems.  It needs one
more cast to suppress the "pointer cast w/ different size" warning.

    node =3D rb_find(void *)(uintptr_tr)addr, ...);

Thanks,
Namhyung


> +       if (node =3D=3D NULL)
> +               return NULL;
> +
> +       return rb_entry(node, struct global_var_entry, node);
> +}
> +
> +static bool global_var__add(struct data_loc_info *dloc, u64 addr,
> +                           const char *name, Dwarf_Die *type_die)
> +{
> +       struct dso *dso =3D map__dso(dloc->ms->map);
> +       struct global_var_entry *gvar;
> +       Dwarf_Word size;
> +
> +       if (dwarf_aggregate_size(type_die, &size) < 0)
> +               return false;
> +
> +       gvar =3D malloc(sizeof(*gvar));
> +       if (gvar =3D=3D NULL)
> +               return false;
> +
> +       gvar->name =3D strdup(name);
> +       if (gvar->name =3D=3D NULL) {
> +               free(gvar);
> +               return false;
> +       }
> +
> +       gvar->start =3D addr;
> +       gvar->end =3D addr + size;
> +       gvar->die_offset =3D dwarf_dieoffset(type_die);
> +
> +       rb_add(&gvar->node, &dso->global_vars, global_var_less);
> +       return true;
> +}
> +
> +void global_var_type__tree_delete(struct rb_root *root)
> +{
> +       struct global_var_entry *gvar;
> +
> +       while (!RB_EMPTY_ROOT(root)) {
> +               struct rb_node *node =3D rb_first(root);
> +
> +               rb_erase(node, root);
> +               gvar =3D rb_entry(node, struct global_var_entry, node);
> +               free(gvar->name);
> +               free(gvar);
> +       }
> +}
> +
>  static bool get_global_var_info(struct data_loc_info *dloc, u64 addr,
>                                 const char **var_name, int *var_offset)
>  {
> @@ -467,14 +552,25 @@ static bool get_global_var_type(Dwarf_Die *cu_die, =
struct data_loc_info *dloc,
>         u64 pc;
>         int offset;
>         bool is_pointer =3D false;
> -       const char *var_name;
> +       const char *var_name =3D NULL;
> +       struct global_var_entry *gvar;
>         Dwarf_Die var_die;
>
> +       gvar =3D global_var__find(dloc, var_addr);
> +       if (gvar) {
> +               if (!dwarf_offdie(dloc->di->dbg, gvar->die_offset, type_d=
ie))
> +                       return false;
> +
> +               *var_offset =3D var_addr - gvar->start;
> +               return true;
> +       }
> +
>         /* Try to get the variable by address first */
>         if (die_find_variable_by_addr(cu_die, var_addr, &var_die, &offset=
) &&
>             check_variable(&var_die, type_die, offset, is_pointer) =3D=3D=
 0) {
> +               var_name =3D dwarf_diename(&var_die);
>                 *var_offset =3D offset;
> -               return true;
> +               goto ok;
>         }
>
>         if (!get_global_var_info(dloc, var_addr, &var_name, var_offset))
> @@ -485,9 +581,14 @@ static bool get_global_var_type(Dwarf_Die *cu_die, s=
truct data_loc_info *dloc,
>         /* Try to get the name of global variable */
>         if (die_find_variable_at(cu_die, var_name, pc, &var_die) &&
>             check_variable(&var_die, type_die, *var_offset, is_pointer) =
=3D=3D 0)
> -               return true;
> +               goto ok;
>
>         return false;
> +
> +ok:
> +       /* The address should point to the start of the variable */
> +       global_var__add(dloc, var_addr - *var_offset, var_name, type_die)=
;
> +       return true;
>  }
>
>  /**
> diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-d=
ata.h
> index 1b5a152163b5..fe1e53d6e8c7 100644
> --- a/tools/perf/util/annotate-data.h
> +++ b/tools/perf/util/annotate-data.h
> @@ -153,6 +153,9 @@ int annotated_data_type__update_samples(struct annota=
ted_data_type *adt,
>  /* Release all data type information in the tree */
>  void annotated_data_type__tree_delete(struct rb_root *root);
>
> +/* Release all global variable information in the tree */
> +void global_var_type__tree_delete(struct rb_root *root);
> +
>  #else /* HAVE_DWARF_SUPPORT */
>
>  static inline struct annotated_data_type *
> @@ -175,6 +178,10 @@ static inline void annotated_data_type__tree_delete(=
struct rb_root *root __maybe
>  {
>  }
>
> +static inline void global_var_type__tree_delete(struct rb_root *root __m=
aybe_unused)
> +{
> +}
> +
>  #endif /* HAVE_DWARF_SUPPORT */
>
>  #endif /* _PERF_ANNOTATE_DATA_H */
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 22fd5fa806ed..6e2a7198b382 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -1329,6 +1329,7 @@ struct dso *dso__new_id(const char *name, struct ds=
o_id *id)
>                 dso->inlined_nodes =3D RB_ROOT_CACHED;
>                 dso->srclines =3D RB_ROOT_CACHED;
>                 dso->data_types =3D RB_ROOT;
> +               dso->global_vars =3D RB_ROOT;
>                 dso->data.fd =3D -1;
>                 dso->data.status =3D DSO_DATA_STATUS_UNKNOWN;
>                 dso->symtab_type =3D DSO_BINARY_TYPE__NOT_FOUND;
> @@ -1373,6 +1374,7 @@ void dso__delete(struct dso *dso)
>         dso->symbol_names_len =3D 0;
>         zfree(&dso->symbol_names);
>         annotated_data_type__tree_delete(&dso->data_types);
> +       global_var_type__tree_delete(&dso->global_vars);
>
>         if (dso->short_name_allocated) {
>                 zfree((char **)&dso->short_name);
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index ce9f3849a773..2cdcd1e2ef8b 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -154,7 +154,8 @@ struct dso {
>         size_t           symbol_names_len;
>         struct rb_root_cached inlined_nodes;
>         struct rb_root_cached srclines;
> -       struct rb_root  data_types;
> +       struct rb_root   data_types;
> +       struct rb_root   global_vars;
>
>         struct {
>                 u64             addr;
> @@ -411,4 +412,7 @@ int dso__strerror_load(struct dso *dso, char *buf, si=
ze_t buflen);
>
>  void reset_fd_limit(void);
>
> +u64 dso__find_global_type(struct dso *dso, u64 addr);
> +u64 dso__findnew_global_type(struct dso *dso, u64 addr, u64 offset);
> +
>  #endif /* __PERF_DSO */
> --
> 2.44.0.291.gc1ea87d7ee-goog
>
>