| 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: <5ea9bf13-c78d-43ef-ae3b-5ded69f75afc@intel.com>
Date: Tue, 30 Sep 2025 19:41:24 +0800
From: "Li, Tianyou" <tianyou.li@...el.com>
To: Namhyung Kim <namhyung@...nel.org>
CC: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...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 v4] perf tools c2c: Add annotation support to perf c2c
report
On 9/29/2025 4:07 PM, Namhyung Kim wrote:
> Hello,
>
> On Sun, Sep 28, 2025 at 04:16:16PM +0800, Li, Tianyou wrote:
>> Rebased with latest perf-tools-next. Looking forward to your review
>> comments. Thanks.
> Sorry for the delay, I was on vacation.
Very appreciated for your time to review the patch. Believes you have a
great vacation.
>
>> On 9/28/2025 5:02 PM, 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.
>>>
>>> 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 | 130 ++++++++++++++++++++++++++++--
>>> tools/perf/ui/browsers/annotate.c | 41 +++++++++-
>>> tools/perf/ui/browsers/hists.c | 2 +-
>>> tools/perf/util/annotate.c | 2 +-
>>> tools/perf/util/annotate.h | 2 +
>>> tools/perf/util/hist.h | 6 +-
>>> 7 files changed, 171 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
>>> index 646f43b0f7c4..f977e97a9c96 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..bf2136d062ef 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);
>>> @@ -2550,6 +2561,35 @@ static void perf_c2c__hists_fprintf(FILE *out, struct perf_session *session)
>>> }
>>> #ifdef HAVE_SLANG_SUPPORT
>>> +
>>> +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;
>>> + struct annotated_source *src = NULL;
>>> +
>>> + if (he == NULL) {
>>> + 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;
>>> + }
>>> +
>>> + src = symbol__hists(sym, 0);
>>> + if (src == NULL) {
>>> + ui_browser__help_window(&browser->b, "Failed to initialize annotation source");
>>> + return 0;
>>> + }
>>> +
>>> + c2c_he = container_of(he, struct c2c_hist_entry, he);
>>> + return hist_entry__tui_annotate(he, c2c_he->evsel, NULL, he->ip);
> I'm skeptical about using he->ip. An hist_entry can collapse multiple
> samples with different IP in a symbol (even if hpp_list has symbol sort
> key). That means he->ip cannot represent the entry is from the specific
> point in the function. This might lead users to an inaccurate place in
> the annotation browser. I'd recommend not passing IP.
Understood. Thanks. What if we use cacheline browser's he->mem_info to
get the al_addr? I will change the code accordingly for your review.
>>> +}
>>> +
>>> static void c2c_browser__update_nr_entries(struct hist_browser *hb)
>>> {
>>> u64 nr_entries = 0;
>>> @@ -2617,6 +2657,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 +2692,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 +3033,11 @@ static int setup_coalesce(const char *coalesce, bool no_source)
>>> return 0;
>>> }
>>> +static bool perf_c2c__has_annotation(void)
>>> +{
>>> + return use_browser == 1;
> Please check if it has symbol dimension in the c2c_hists->list like in
> the ui__has_annotation(). Maybe you need to add this in the
> c2c_hists__init_sort().
>
> if (dim == &dim_symbol)
> hpp_list->sym = 1;
>
> Thanks,
> Namhyung
Agreed, will do. I am thinking to add the 'symbol' sort key into
cl_resort so it will not impact the 'dcacheline' sort key in the
c2c_hists__init and the cl_sort sort key which also controls the cl
output. In this way it may need to have a hpp_list parameter to indicate
the caller from c2c_browser or cacheline_browser.
>>> +}
>>> +
>>> static int perf_c2c__report(int argc, const char **argv)
>>> {
>>> struct itrace_synth_opts itrace_synth_opts = {
>>> @@ -3006,6 +3055,7 @@ 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 +3083,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 +3096,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 +3114,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 +3218,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 +3259,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 +3329,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 8fe699f98542..63d0e28fb991 100644
>>> --- a/tools/perf/ui/browsers/annotate.c
>>> +++ b/tools/perf/ui/browsers/annotate.c
>>> @@ -605,7 +605,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);
>>> - __hist_entry__tui_annotate(browser->he, &target_ms, evsel, hbt);
>>> + __hist_entry__tui_annotate(browser->he, &target_ms, evsel, hbt, NO_INITIAL_IP);
>>> /*
>>> * The annotate_browser above changed the title with the target function
>>> @@ -864,6 +864,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
>>> const char *help = "Press 'h' for help on key bindings";
>>> int delay_secs = hbt ? hbt->refresh : 0;
>>> char *br_cntr_text = NULL;
>>> + u64 init_ip = NO_INITIAL_IP;
>>> char title[256];
>>> int key;
>>> @@ -873,6 +874,13 @@ static int annotate_browser__run(struct annotate_browser *browser,
>>> annotate_browser__calc_percent(browser, evsel);
>>> + /* the selection are intentionally even not from the sample percentage */
>>> + if (browser->entries.rb_node == NULL && browser->selection) {
>>> + init_ip = sym->start + browser->selection->offset;
>>> + 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;
>>> @@ -973,6 +981,18 @@ static int annotate_browser__run(struct annotate_browser *browser,
>>> ui_helpline__puts(help);
>>> annotate__scnprintf_title(hists, title, sizeof(title));
>>> annotate_browser__show(browser, title, help);
>>> + /* Previous RB tree may not valid, need refresh according to new entries*/
>>> + if (init_ip != NO_INITIAL_IP) {
>>> + struct disasm_line *dl = find_disasm_line(sym, init_ip, true);
>>> +
>>> + browser->curr_hot = NULL;
>>> + browser->entries.rb_node = NULL;
>>> + if (dl != NULL) {
>>> + disasm_rb_tree__insert(browser, &dl->al);
>>> + browser->curr_hot = rb_last(&browser->entries);
>>> + }
>>> + nd = browser->curr_hot;
>>> + }
>>> continue;
>>> case 'o':
>>> annotate_opts.use_offset = !annotate_opts.use_offset;
>>> @@ -1106,22 +1126,23 @@ static int annotate_browser__run(struct annotate_browser *browser,
>>> }
>>> 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 __hist_entry__tui_annotate(he, &he->ms, evsel, hbt);
>>> + return __hist_entry__tui_annotate(he, &he->ms, evsel, hbt, init_ip);
>>> }
>>> int __hist_entry__tui_annotate(struct hist_entry *he, 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,
>>> @@ -1173,6 +1194,18 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
>>> browser.he = &annotate_he;
>>> }
>>> + /*
>>> + * 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");
>>> if (annotate_opts.code_with_type) {
>>> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
>>> index 487c0b08c003..3675a703de11 100644
>>> --- a/tools/perf/ui/browsers/hists.c
>>> +++ b/tools/perf/ui/browsers/hists.c
>>> @@ -2485,7 +2485,7 @@ do_annotate(struct hist_browser *browser, struct popup_action *act)
>>> evsel = hists_to_evsel(browser->hists);
>>> he = hist_browser__selected_entry(browser);
>>> - err = __hist_entry__tui_annotate(he, &act->ms, evsel, browser->hbt);
>>> + err = __hist_entry__tui_annotate(he, &act->ms, evsel, browser->hbt, NO_INITIAL_IP);
>>> /*
>>> * offer option to annotate the other branch source or target
>>> * (if they exists) when returning from annotate
>>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>>> index c9b220d9f924..937effbeda69 100644
>>> --- a/tools/perf/util/annotate.c
>>> +++ b/tools/perf/util/annotate.c
>>> @@ -2622,7 +2622,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 eaf6c8aa7f47..bbe67588bbdd 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.
>>> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
>>> index c64005278687..e544e1795f19 100644
>>> --- a/tools/perf/util/hist.h
>>> +++ b/tools/perf/util/hist.h
>>> @@ -713,12 +713,14 @@ struct block_hist {
>>> #include "../ui/keysyms.h"
>>> void attr_to_script(char *buf, struct perf_event_attr *attr);
>>> +#define NO_INITIAL_IP 0
>>> +
>>> int __hist_entry__tui_annotate(struct hist_entry *he, 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);
Powered by blists - more mailing lists