Re: [PATCH v5] perf tools c2c: Add annotation support to perf c2c report

["Li, Tianyou" <tianyou.li@intel.com>]

Hi Namhyung,

Sorry for the delayed response, just came back from vacation. According 
to your review comments, I have sent patch v6 for your kindly review. 
Appreciated for your time.

Patch v6 rebased with latest perf-tools-next, removed the INIT_ADDR 
support, keeps the changes inside of the builtin-c2c.c, along with other 
code suggestions you've made. Basically split the annotate changes from 
the c2c changes.

I will work on the annotate changes according you and Ravi's 
suggestions, hopefully I could send that patch soon.

Regards,

Tianyou

On 10/7/2025 4:23 PM, Namhyung Kim wrote:
> 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@intel.com>
>>>> Reviewed-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>>>> Reviewed-by: Thomas Falcon <thomas.falcon@intel.com>
>>>> Reviewed-by: Jiebin Sun <jiebin.sun@intel.com>
>>>> Reviewed-by: Pan Deng <pan.deng@intel.com>
>>>> Reviewed-by: Zhiguo Zhou <zhiguo.zhou@intel.com>
>>>> Reviewed-by: Wangyang Guo <wangyang.guo@intel.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
>
>