| 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: <aOTOEcNOiUiEJ9tz@google.com>
Date: Tue, 7 Oct 2025 17:23:45 +0900
From: Namhyung Kim <namhyung@...nel.org>
To: "Li, Tianyou" <tianyou.li@...el.com>
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 v5] perf tools c2c: Add annotation support to perf c2c
report
Hello,
On Fri, Oct 03, 2025 at 07:44:34PM +0800, Li, Tianyou wrote:
> Hi Namhyung,
>
> Appreciated for your review comments. Sorry for the delayed response. I am
> on National Holiday so check email late. My response inlined for your
> consideration.
>
> Regards,
>
> Tianyou
>
>
> On 10/3/2025 1:05 PM, Namhyung Kim wrote:
> > Hello,
> >
> > On Tue, Sep 30, 2025 at 08:39:00PM +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.
> > >
> > > 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>
> > > ---
[SNIP]
> > > @@ -2980,7 +3056,8 @@ static int setup_coalesce(const char *coalesce, bool no_source)
> > > else if (c2c.display == DISPLAY_SNP_PEER)
> > > sort_str = "tot_peer";
> > > - if (asprintf(&c2c.cl_resort, "offset,%s", sort_str) < 0)
> > > + /* add 'symbol' sort key to make sure hpp_list->sym get updated */
> > > + if (asprintf(&c2c.cl_resort, "offset,%s,symbol", sort_str) < 0)
> > I think it's better to just process the input rather than enforcing it.
> > It seems the default value will have 'iaddr' and so 'symbol as well.
>
>
> Sorry I am not so clear about 'so symbol as well'. Did you mean we can check
> the 'dim == &dim_iaddr' instead of 'dim == &dim_symbol' to make sure
> hpp_list->sym = 1? If so, do we need to check the coalesce set to default
> 'iaddr' or not, otherwise we need to append the 'iaddr' in addition to the
> user specific one?
I meant you have 'iaddr' in the default sort keys and it will include
'symbol' in the output. So annotation will be enabled by default.
>
>
> >
> > > return -ENOMEM;
> > > pr_debug("coalesce sort fields: %s\n", c2c.cl_sort);
> > > @@ -3006,6 +3083,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 +3111,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"),
> > Please update documentation with the new options.
>
>
> Noted, will do in patch v6.
>
>
> >
> > > + OPT_STRING(0, "addr2line", &addr2line_path, "path",
> > > + "addr2line binary to use for line numbers"),
> > Do you really need this?
>
>
> In my use scenarios of c2c tool, I did not use this addr2line tool. If this
> was not quite necessary, I will remove it from patch v6.
Yes, please.
>
>
> >
> > > OPT_PARENT(c2c_options),
> > > OPT_END()
> > > };
> > > @@ -3040,6 +3124,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 +3142,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 +3246,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(NULL)) {
> > > + 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 +3287,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 +3357,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..a9d56e67454d 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_AL_ADDR);
> > > /*
> > > * 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_al_addr = NO_INITIAL_AL_ADDR;
> > > 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_al_addr = 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_al_addr != NO_INITIAL_AL_ADDR) {
> > > + struct disasm_line *dl = find_disasm_line(sym, init_al_addr, 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;
> > > + }
> > Can you please split annotate changes from c2c change? I think you can
> > start with annotation support in c2c. And add INITIAL_ADDR later so
> > that we can discuss the issue separately. Maybe we don't need the ADDR
> > change. Do you have any concrete usecase where default annotate is not
> > enough for c2c?
>
>
> Sure, I will split the patch into 2 patches. I use c2c extensively for my
> day-to-day performance work, the INITIAL_ADDR would be very helpful to
> located to the code where the iaddr was indicated in the cacheline browser.
> Otherwise, probably I need to copy the iaddr from the cacheline browser, get
> into the annotation browser, press 'o' to show the view with addresses in
> disassemble view, and manually find the iaddr match since the search only
> match string for disassembly code. The code highlight with INITIAL_ADDR
> would quickly allow me to navigate the contended lines of code from
> different functions showed in the cacheline browser, plus with 's' and 'T',
> I can get to the point more conveniently.
>
>
> Agreed to discuss it separately, looking forward to hearing your thoughts.
Thanks for your understanding!
Namhyung
Powered by blists - more mailing lists