[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <50921564-d444-4825-b580-94276833a372@intel.com>
Date: Sun, 7 Sep 2025 22:53:31 +0800
From: "Li, Tianyou" <tianyou.li@...el.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
CC: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Namhyung Kim <namhyung@...nel.org>, 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>, Kan Liang <kan.liang@...ux.intel.com>,
<wangyang.guo@...el.com>, <pan.deng@...el.com>, <zhiguo.zhou@...el.com>,
<jiebin.sun@...el.com>, <thomas.falcon@...el.com>, <dapeng1.mi@...el.com>,
<linux-perf-users@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] perf tools c2c: Add annotation support to perf c2c
report
On 9/3/2025 9:50 PM, Arnaldo Carvalho de Melo wrote:
> On Tue, Aug 19, 2025 at 04:00:14PM +0800, Tianyou Li wrote:
>> Perf c2c report currently specified the code address and source:line
>> information in the cacheline browser, while it is lack of annotation
>> support like perf report to directly show the disassembly code for
>> the particular symbol shared that same cacheline. This patches add
>> a key 'a' binding to the cacheline browser which reuse the annotation
>> browser to show the disassembly view for easier analysis of cacheline
>> contentions. By default, the 'TAB' key navigate to the code address
>> where the contentions detected.
> There were changes in that codebase recently, can you please consider
> rebasing your work?
>
> This is something I really want to see in place, almost did it myself at
> some point :-)
>
> Please use what is in:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=tmp.perf-tools-next
Thanks Arnaldo for your comments. Sorry for the late response due to
other errands.
I have rebased the code on top of tmp.perf-tools-next branch, sent out
as v3 patch.
Looking forward for your review comments, appreciated.
Regards,
Tianyou
> - Arnaldo
>
>> Signed-off-by: Tianyou Li <tianyou.li@...el.com>
>> Reviewed-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
>> Reviewed-by: Thomas Falcon <thomas.falcon@...el.com>
>> Reviewed-by: Jiebin Sun <jiebin.sun@...el.com>
>> Reviewed-by: Pan Deng <pan.deng@...el.com>
>> Reviewed-by: Zhiguo Zhou <zhiguo.zhou@...el.com>
>> Reviewed-by: Wangyang Guo <wangyang.guo@...el.com>
>> ---
>> tools/perf/builtin-annotate.c | 2 +-
>> tools/perf/builtin-c2c.c | 124 ++++++++++++++++++++++++++++--
>> tools/perf/ui/browsers/annotate.c | 30 ++++++--
>> tools/perf/ui/browsers/hists.c | 2 +-
>> tools/perf/util/annotate.c | 2 +-
>> tools/perf/util/annotate.h | 4 +-
>> tools/perf/util/hist.h | 6 +-
>> 7 files changed, 153 insertions(+), 17 deletions(-)
>>
>> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
>> index 5d57d2913f3d..8c896fbe76b7 100644
>> --- a/tools/perf/builtin-annotate.c
>> +++ b/tools/perf/builtin-annotate.c
>> @@ -519,7 +519,7 @@ static void hists__find_annotations(struct hists *hists,
>> /* skip missing symbols */
>> nd = rb_next(nd);
>> } else if (use_browser == 1) {
>> - key = hist_entry__tui_annotate(he, evsel, NULL);
>> + key = hist_entry__tui_annotate(he, evsel, NULL, NO_INITIAL_IP);
>>
>> switch (key) {
>> case -1:
>> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
>> index 9e9ff471ddd1..f753ec50b967 100644
>> --- a/tools/perf/builtin-c2c.c
>> +++ b/tools/perf/builtin-c2c.c
>> @@ -45,6 +45,8 @@
>> #include "pmus.h"
>> #include "string2.h"
>> #include "util/util.h"
>> +#include "util/symbol.h"
>> +#include "util/annotate.h"
>>
>> struct c2c_hists {
>> struct hists hists;
>> @@ -62,6 +64,7 @@ struct compute_stats {
>>
>> struct c2c_hist_entry {
>> struct c2c_hists *hists;
>> + struct evsel *evsel;
>> struct c2c_stats stats;
>> unsigned long *cpuset;
>> unsigned long *nodeset;
>> @@ -225,6 +228,12 @@ he__get_c2c_hists(struct hist_entry *he,
>> return hists;
>> }
>>
>> +static void c2c_he__set_evsel(struct c2c_hist_entry *c2c_he,
>> + struct evsel *evsel)
>> +{
>> + c2c_he->evsel = evsel;
>> +}
>> +
>> static void c2c_he__set_cpu(struct c2c_hist_entry *c2c_he,
>> struct perf_sample *sample)
>> {
>> @@ -334,6 +343,7 @@ static int process_sample_event(const struct perf_tool *tool __maybe_unused,
>>
>> c2c_he__set_cpu(c2c_he, sample);
>> c2c_he__set_node(c2c_he, sample);
>> + c2c_he__set_evsel(c2c_he, evsel);
>>
>> hists__inc_nr_samples(&c2c_hists->hists, he->filtered);
>> ret = hist_entry__append_callchain(he, sample);
>> @@ -371,6 +381,7 @@ static int process_sample_event(const struct perf_tool *tool __maybe_unused,
>>
>> c2c_he__set_cpu(c2c_he, sample);
>> c2c_he__set_node(c2c_he, sample);
>> + c2c_he__set_evsel(c2c_he, evsel);
>>
>> hists__inc_nr_samples(&c2c_hists->hists, he->filtered);
>> ret = hist_entry__append_callchain(he, sample);
>> @@ -2606,6 +2617,28 @@ c2c_cacheline_browser__new(struct hists *hists, struct hist_entry *he)
>> return browser;
>> }
>>
>> +static int perf_c2c__toggle_annotation(struct hist_browser *browser)
>> +{
>> + struct hist_entry *he = browser->he_selection;
>> + struct symbol *sym = NULL;
>> + struct c2c_hist_entry *c2c_he = NULL;
>> +
>> + if (!he) {
>> + ui_browser__help_window(&browser->b, "No entry selected for annotation");
>> + return 0;
>> + }
>> + sym = (&he->ms)->sym;
>> +
>> + if (sym == NULL) {
>> + ui_browser__help_window(&browser->b, "Can not annotate, no symbol found");
>> + return 0;
>> + }
>> +
>> + symbol__hists(sym, 0);
>> + c2c_he = container_of(he, struct c2c_hist_entry, he);
>> + return hist_entry__tui_annotate(he, c2c_he->evsel, NULL, he->ip);
>> +}
>> +
>> static int perf_c2c__browse_cacheline(struct hist_entry *he)
>> {
>> struct c2c_hist_entry *c2c_he;
>> @@ -2617,6 +2650,7 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
>> " ENTER Toggle callchains (if present) \n"
>> " n Toggle Node details info \n"
>> " s Toggle full length of symbol and source line columns \n"
>> + " a Toggle annotation view \n"
>> " q Return back to cacheline list \n";
>>
>> if (!he)
>> @@ -2651,6 +2685,9 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
>> c2c.node_info = (c2c.node_info + 1) % 3;
>> setup_nodes_header();
>> break;
>> + case 'a':
>> + perf_c2c__toggle_annotation(browser);
>> + break;
>> case 'q':
>> goto out;
>> case '?':
>> @@ -2989,6 +3026,11 @@ static int setup_coalesce(const char *coalesce, bool no_source)
>> return 0;
>> }
>>
>> +static bool perf_c2c__has_annotation(void)
>> +{
>> + return use_browser == 1;
>> +}
>> +
>> static int perf_c2c__report(int argc, const char **argv)
>> {
>> struct itrace_synth_opts itrace_synth_opts = {
>> @@ -3006,6 +3048,8 @@ static int perf_c2c__report(int argc, const char **argv)
>> const char *display = NULL;
>> const char *coalesce = NULL;
>> bool no_source = false;
>> + const char *disassembler_style = NULL, *objdump_path = NULL, *addr2line_path = NULL;
>> +
>> const struct option options[] = {
>> OPT_STRING('k', "vmlinux", &symbol_conf.vmlinux_name,
>> "file", "vmlinux pathname"),
>> @@ -3033,6 +3077,12 @@ static int perf_c2c__report(int argc, const char **argv)
>> OPT_BOOLEAN(0, "stitch-lbr", &c2c.stitch_lbr,
>> "Enable LBR callgraph stitching approach"),
>> OPT_BOOLEAN(0, "double-cl", &chk_double_cl, "Detect adjacent cacheline false sharing"),
>> + OPT_STRING('M', "disassembler-style", &disassembler_style, "disassembler style",
>> + "Specify disassembler style (e.g. -M intel for intel syntax)"),
>> + OPT_STRING(0, "objdump", &objdump_path, "path",
>> + "objdump binary to use for disassembly and annotations"),
>> + OPT_STRING(0, "addr2line", &addr2line_path, "path",
>> + "addr2line binary to use for line numbers"),
>> OPT_PARENT(c2c_options),
>> OPT_END()
>> };
>> @@ -3040,6 +3090,12 @@ static int perf_c2c__report(int argc, const char **argv)
>> const char *output_str, *sort_str = NULL;
>> struct perf_env *env;
>>
>> + annotation_options__init();
>> +
>> + err = hists__init();
>> + if (err < 0)
>> + goto out;
>> +
>> argc = parse_options(argc, argv, options, report_c2c_usage,
>> PARSE_OPT_STOP_AT_NON_OPTION);
>> if (argc)
>> @@ -3052,6 +3108,36 @@ static int perf_c2c__report(int argc, const char **argv)
>> if (c2c.stats_only)
>> c2c.use_stdio = true;
>>
>> + /**
>> + * Annotation related options
>> + * disassembler_style, objdump_path, addr2line_path
>> + * are set in the c2c_options, so we can use them here.
>> + */
>> + if (disassembler_style) {
>> + annotate_opts.disassembler_style = strdup(disassembler_style);
>> + if (!annotate_opts.disassembler_style) {
>> + err = -ENOMEM;
>> + pr_err("Failed to allocate memory for annotation options\n");
>> + goto out;
>> + }
>> + }
>> + if (objdump_path) {
>> + annotate_opts.objdump_path = strdup(objdump_path);
>> + if (!annotate_opts.objdump_path) {
>> + err = -ENOMEM;
>> + pr_err("Failed to allocate memory for annotation options\n");
>> + goto out;
>> + }
>> + }
>> + if (addr2line_path) {
>> + symbol_conf.addr2line_path = strdup(addr2line_path);
>> + if (!symbol_conf.addr2line_path) {
>> + err = -ENOMEM;
>> + pr_err("Failed to allocate memory for annotation options\n");
>> + goto out;
>> + }
>> + }
>> +
>> err = symbol__validate_sym_arguments();
>> if (err)
>> goto out;
>> @@ -3126,6 +3212,38 @@ 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;
>> +
>> + /*
>> + * Only in the TUI browser we are doing integrated annotation,
>> + * so don't allocate extra space that won't be used in the stdio
>> + * implementation.
>> + */
>> + if (perf_c2c__has_annotation()) {
>> + int ret = symbol__annotation_init();
>> +
>> + if (ret < 0)
>> + goto out_mem2node;
>> + /*
>> + * For searching by name on the "Browse map details".
>> + * providing it only in verbose mode not to bloat too
>> + * much struct symbol.
>> + */
>> + if (verbose > 0) {
>> + /*
>> + * XXX: Need to provide a less kludgy way to ask for
>> + * more space per symbol, the u32 is for the index on
>> + * the ui browser.
>> + * See symbol__browser_index.
>> + */
>> + symbol_conf.priv_size += sizeof(u32);
>> + }
>> + annotation_config__init();
>> + }
>> +
>> if (symbol__init(env) < 0)
>> goto out_mem2node;
>>
>> @@ -3135,11 +3253,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);
>> @@ -3210,6 +3323,7 @@ static int perf_c2c__report(int argc, const char **argv)
>> out_session:
>> perf_session__delete(session);
>> out:
>> + annotation_options__exit();
>> return err;
>> }
>>
>> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
>> index 183902dac042..7eb659c76b53 100644
>> --- a/tools/perf/ui/browsers/annotate.c
>> +++ b/tools/perf/ui/browsers/annotate.c
>> @@ -557,7 +557,7 @@ static bool annotate_browser__callq(struct annotate_browser *browser,
>> target_ms.map = ms->map;
>> target_ms.sym = dl->ops.target.sym;
>> annotation__unlock(notes);
>> - symbol__tui_annotate(&target_ms, evsel, hbt);
>> + symbol__tui_annotate(&target_ms, evsel, hbt, NO_INITIAL_IP);
>> sym_title(ms->sym, ms->map, title, sizeof(title), annotate_opts.percent_type);
>> ui_browser__show_title(&browser->b, title);
>> return true;
>> @@ -814,6 +814,11 @@ static int annotate_browser__run(struct annotate_browser *browser,
>>
>> annotate_browser__calc_percent(browser, evsel);
>>
>> + if (browser->curr_hot == NULL && browser->selection) {
>> + disasm_rb_tree__insert(browser, browser->selection);
>> + browser->curr_hot = rb_last(&browser->entries);
>> + }
>> +
>> if (browser->curr_hot) {
>> annotate_browser__set_rb_top(browser, browser->curr_hot);
>> browser->b.navkeypressed = false;
>> @@ -1033,27 +1038,28 @@ static int annotate_browser__run(struct annotate_browser *browser,
>> }
>>
>> int map_symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
>> - struct hist_browser_timer *hbt)
>> + struct hist_browser_timer *hbt, u64 init_ip)
>> {
>> - return symbol__tui_annotate(ms, evsel, hbt);
>> + return symbol__tui_annotate(ms, evsel, hbt, init_ip);
>> }
>>
>> int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
>> - struct hist_browser_timer *hbt)
>> + struct hist_browser_timer *hbt, u64 init_ip)
>> {
>> /* reset abort key so that it can get Ctrl-C as a key */
>> SLang_reset_tty();
>> SLang_init_tty(0, 0, 0);
>> SLtty_set_suspend_state(true);
>>
>> - return map_symbol__tui_annotate(&he->ms, evsel, hbt);
>> + return map_symbol__tui_annotate(&he->ms, evsel, hbt, init_ip);
>> }
>>
>> int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
>> - struct hist_browser_timer *hbt)
>> + struct hist_browser_timer *hbt, u64 init_ip)
>> {
>> struct symbol *sym = ms->sym;
>> struct annotation *notes = symbol__annotation(sym);
>> + struct disasm_line *dl = NULL;
>> struct annotate_browser browser = {
>> .b = {
>> .refresh = annotate_browser__refresh,
>> @@ -1093,6 +1099,18 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
>> }
>> }
>>
>> + /*
>> + * If init_ip is set, it means that there should be a line
>> + * intentionally selected, not based on the percentages
>> + * which caculated by the event sampling. In this case, we
>> + * convey this information into the browser selection, where
>> + * the selection in other cases should be empty.
>> + */
>> + if (init_ip != NO_INITIAL_IP) {
>> + dl = find_disasm_line(sym, init_ip, false);
>> + browser.selection = &dl->al;
>> + }
>> +
>> ui_helpline__push("Press ESC to exit");
>>
>> browser.b.width = notes->src->widths.max_line_len;
>> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
>> index d9d3fb44477a..eec1b5c12a28 100644
>> --- a/tools/perf/ui/browsers/hists.c
>> +++ b/tools/perf/ui/browsers/hists.c
>> @@ -2484,7 +2484,7 @@ do_annotate(struct hist_browser *browser, struct popup_action *act)
>> else
>> evsel = hists_to_evsel(browser->hists);
>>
>> - err = map_symbol__tui_annotate(&act->ms, evsel, browser->hbt);
>> + err = map_symbol__tui_annotate(&act->ms, evsel, browser->hbt, NO_INITIAL_IP);
>> he = hist_browser__selected_entry(browser);
>> /*
>> * offer option to annotate the other branch source or target
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 0dd475a744b6..682100196134 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -2544,7 +2544,7 @@ int annotate_get_insn_location(struct arch *arch, struct disasm_line *dl,
>> return 0;
>> }
>>
>> -static struct disasm_line *find_disasm_line(struct symbol *sym, u64 ip,
>> +struct disasm_line *find_disasm_line(struct symbol *sym, u64 ip,
>> bool allow_update)
>> {
>> struct disasm_line *dl;
>> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
>> index 8b5131d257b0..c4c897745698 100644
>> --- a/tools/perf/util/annotate.h
>> +++ b/tools/perf/util/annotate.h
>> @@ -170,6 +170,8 @@ static inline struct disasm_line *disasm_line(struct annotation_line *al)
>> return al ? container_of(al, struct disasm_line, al) : NULL;
>> }
>>
>> +struct disasm_line *find_disasm_line(struct symbol *sym, u64 ip,
>> + bool allow_update);
>> /*
>> * Is this offset in the same function as the line it is used?
>> * asm functions jump to other functions, for instance.
>> @@ -473,7 +475,7 @@ int hist_entry__tty_annotate2(struct hist_entry *he, struct evsel *evsel);
>>
>> #ifdef HAVE_SLANG_SUPPORT
>> int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
>> - struct hist_browser_timer *hbt);
>> + struct hist_browser_timer *hbt, u64 init_ip);
>> #else
>> static inline int symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
>> struct evsel *evsel __maybe_unused,
>> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
>> index 70438d03ca9c..aca1e3151bcc 100644
>> --- a/tools/perf/util/hist.h
>> +++ b/tools/perf/util/hist.h
>> @@ -713,11 +713,13 @@ struct block_hist {
>> #include "../ui/keysyms.h"
>> void attr_to_script(char *buf, struct perf_event_attr *attr);
>>
>> +#define NO_INITIAL_IP 0
>> +
>> int map_symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
>> - struct hist_browser_timer *hbt);
>> + struct hist_browser_timer *hbt, u64 init_ip);
>>
>> int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
>> - struct hist_browser_timer *hbt);
>> + struct hist_browser_timer *hbt, u64 init_ip);
>>
>> int evlist__tui_browse_hists(struct evlist *evlist, const char *help, struct hist_browser_timer *hbt,
>> float min_pcnt, struct perf_env *env, bool warn_lost_event);
>> --
>> 2.47.1
>>
Powered by blists - more mailing lists