| 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: <CAM9d7ci6h_f8WsRu3cyYPQJO8nuAXcsMX_T_+aLy_0t5_33tMA@mail.gmail.com>
Date: Thu, 1 Jun 2023 14:26:48 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Artem Savkov <asavkov@...hat.com>
Cc: linux-perf-users@...r.kernel.org,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
Adrian Hunter <adrian.hunter@...el.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] perf tools: allow running annotation browser from c2c-report
Hello,
On Wed, May 31, 2023 at 4:50 AM Artem Savkov <asavkov@...hat.com> wrote:
>
> Add a shortcut to run annotation browser for selected symbol from
> c2c report TUI.
>
> Signed-off-by: Artem Savkov <asavkov@...hat.com>
> ---
> tools/perf/builtin-c2c.c | 76 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 71 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index 05dfd98af170b..96e66289c2576 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -19,11 +19,13 @@
> #include <linux/zalloc.h>
> #include <asm/bug.h>
> #include <sys/param.h>
> +#include <sys/ttydefaults.h>
> #include "debug.h"
> #include "builtin.h"
> #include <perf/cpumap.h>
> #include <subcmd/pager.h>
> #include <subcmd/parse-options.h>
> +#include "map.h"
> #include "map_symbol.h"
> #include "mem-events.h"
> #include "session.h"
> @@ -43,6 +45,8 @@
> #include "ui/progress.h"
> #include "pmus.h"
> #include "string2.h"
> +#include "util/annotate.h"
> +#include "util/dso.h"
> #include "util/util.h"
>
> struct c2c_hists {
> @@ -79,6 +83,7 @@ struct c2c_hist_entry {
> * because of its callchain dynamic entry
> */
> struct hist_entry he;
> + struct evsel *evsel;
I'm not sure if it's needed. It seems c2c command doesn't collect
samples per evsel. It uses c2c.hists.hists for all evsels. Then it
might not be worth keeping an evsel in a c2c_hist_entry and
just use a random evsel in the evlist.
> };
>
> static char const *coalesce_default = "iaddr";
> @@ -111,6 +116,8 @@ struct perf_c2c {
> char *cl_sort;
> char *cl_resort;
> char *cl_output;
> +
> + struct annotation_options annotation_opts;
> };
>
> enum {
> @@ -326,7 +333,12 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused,
> c2c_he__set_cpu(c2c_he, sample);
> c2c_he__set_node(c2c_he, sample);
>
> + c2c_he->evsel = evsel;
> +
> hists__inc_nr_samples(&c2c_hists->hists, he->filtered);
> + if (use_browser == 1 && al.map != NULL && al.sym != NULL) {
> + hist_entry__inc_addr_samples(he, sample, c2c_he->evsel, al.addr);
> + }
> ret = hist_entry__append_callchain(he, sample);
>
> if (!ret) {
> @@ -363,7 +375,12 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused,
> c2c_he__set_cpu(c2c_he, sample);
> c2c_he__set_node(c2c_he, sample);
>
> + c2c_he->evsel = evsel;
> +
> hists__inc_nr_samples(&c2c_hists->hists, he->filtered);
> + if (use_browser == 1 && al.map != NULL && al.sym != NULL) {
> + hist_entry__inc_addr_samples(he, sample, c2c_he->evsel, al.addr);
> + }
> ret = hist_entry__append_callchain(he, sample);
> }
>
> @@ -2618,9 +2635,12 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
> struct c2c_hists *c2c_hists;
> struct c2c_cacheline_browser *cl_browser;
> struct hist_browser *browser;
> + struct map_symbol ms = { NULL, NULL, NULL };
> + int err = 0;
> int key = -1;
> static const char help[] =
> " ENTER Toggle callchains (if present) \n"
> + " a Annotate current symbol\n"
> " n Toggle Node details info \n"
> " s Toggle full length of symbol and source line columns \n"
> " q Return back to cacheline list \n";
> @@ -2650,6 +2670,44 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
> key = hist_browser__run(browser, "? - help", true, 0);
>
> switch (key) {
> + case 'a':
I think it's better to factor this code out to a function.
> + if (!browser->selection ||
> + !browser->selection->map ||
> + !browser->selection->map->dso ||
> + browser->selection->map->dso->annotate_warned) {
> + continue;
> + }
> +
> + ms.map = browser->selection->map;
> +
> + if (!browser->selection->sym) {
> + if (!browser->he_selection)
> + continue;
> +
> + ms.sym = symbol__new_unresolved(browser->he_selection->ip,
> + browser->selection->map);
> +
> + if (!ms.sym)
> + continue;
> + } else {
> + if (symbol__annotation(browser->selection->sym)->src == NULL) {
> + ui_browser__warning(&browser->b, 0,
> + "No samples for the \"%s\" symbol.\n\n",
> + browser->selection->sym->name);
> + continue;
> + }
> + ms.sym = browser->selection->sym;
> + }
> +
> + err = map_symbol__tui_annotate(&ms, c2c_he->evsel, browser->hbt, &c2c.annotation_opts);
> +
> + ui_browser__update_nr_entries(&browser->b, browser->hists->nr_entries);
c2c_browser__update_nr_entries() ?
> + if ((err == 'q' || err == CTRL('c')) && browser->he_selection->branch_info)
Why check branch_info?
> + continue;
> + if (err)
> + ui_browser__handle_resize(&browser->b);
> +
> + continue;
It'd be natural to use 'break' instead of 'continue' here.
> case 's':
> c2c.symbol_full = !c2c.symbol_full;
> break;
> @@ -3045,6 +3103,10 @@ static int perf_c2c__report(int argc, const char **argv)
> int err = 0;
> const char *output_str, *sort_str = NULL;
>
> + annotation_options__init(&c2c.annotation_opts);
> +
> + hists__init();
> +
> argc = parse_options(argc, argv, options, report_c2c_usage,
> PARSE_OPT_STOP_AT_NON_OPTION);
> if (argc)
> @@ -3118,6 +3180,14 @@ static int perf_c2c__report(int argc, const char **argv)
> if (err)
> goto out_mem2node;
>
> + if (c2c.use_stdio) {
> + use_browser = 0;
> + } else {
> + use_browser = 1;
> + symbol__annotation_init();
> + annotation_config__init(&c2c.annotation_opts);
> + }
> +
> if (symbol__init(&session->header.env) < 0)
> goto out_mem2node;
>
> @@ -3127,11 +3197,6 @@ static int perf_c2c__report(int argc, const char **argv)
> goto out_mem2node;
> }
>
> - if (c2c.use_stdio)
> - use_browser = 0;
> - else
> - use_browser = 1;
> -
> setup_browser(false);
>
> err = perf_session__process_events(session);
> @@ -3202,6 +3267,7 @@ static int perf_c2c__report(int argc, const char **argv)
> out_session:
> perf_session__delete(session);
> out:
> + annotation_options__init(&c2c.annotation_opts);
__exit() ?
Thanks,
Namhyung
> return err;
> }
>
> --
> 2.40.1
>
Powered by blists - more mailing lists