Re: [PATCH v6 3/3] perf tools c2c: Highlight the contention line in the annotate browser

From: Namhyung Kim
Date: Fri Oct 10 2025 - 09:09:16 EST

Next message: Peng Fan: "Re: [PATCH 6/6] remoteproc: core: Consolidate bool flags into 1-bit bitfields"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v1 7/7] regulator: s2mps11: add support for S2MPS16 regulators"
In reply to: Tianyou Li: "[PATCH v6 3/3] perf tools c2c: Highlight the contention line in the annotate browser"
Next in thread: Tianyou Li: "[PATCH v7 1/2] perf tools c2c: Add annotation support to perf c2c report"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hello,

On Fri, Oct 10, 2025 at 04:35:49PM +0800, Tianyou Li wrote:
> Add support to highlight the contention line in the annotate browser,
> use 'TAB'/'UNTAB' to refocus to the contention line.
>
> Signed-off-by: Tianyou Li <tianyou.li@xxxxxxxxx>
> Reviewed-by: Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx>
> Reviewed-by: Thomas Falcon <thomas.falcon@xxxxxxxxx>
> Reviewed-by: Jiebin Sun <jiebin.sun@xxxxxxxxx>
> Reviewed-by: Pan Deng <pan.deng@xxxxxxxxx>
> Reviewed-by: Zhiguo Zhou <zhiguo.zhou@xxxxxxxxx>
> Reviewed-by: Wangyang Guo <wangyang.guo@xxxxxxxxx>
> Tested-by: Ravi Bangoria <ravi.bangoria@xxxxxxx>
> ---
> tools/perf/builtin-annotate.c | 2 +-
> tools/perf/builtin-c2c.c | 6 +++-
> tools/perf/ui/browsers/annotate.c | 52 ++++++++++++++++++++++++++++---
> tools/perf/ui/browsers/hists.c | 2 +-
> tools/perf/util/hist.h | 6 ++--
> 5 files changed, 59 insertions(+), 9 deletions(-)
>
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 646f43b0f7c4..d89796648bec 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_AL_ADDR);

I think it's too long. How about NO_ADDR or ZERO_ADDR?

>
> switch (key) {
> case -1:
> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index 878913115b45..7ee3c8a3f66c 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -2586,6 +2586,7 @@ static int perf_c2c__toggle_annotation(struct hist_browser *browser)
> struct symbol *sym = NULL;
> struct annotated_source *src = NULL;
> struct c2c_hist_entry *c2c_he = NULL;
> + u64 al_addr = NO_INITIAL_AL_ADDR;
>
> if (!perf_c2c__has_annotation(he->hists->hpp_list)) {
> ui_browser__help_window(&browser->b, "No annotation support");
> @@ -2609,8 +2610,11 @@ static int perf_c2c__toggle_annotation(struct hist_browser *browser)
> return 0;
> }
>
> + if (he->mem_info)
> + al_addr = mem_info__iaddr(he->mem_info)->al_addr;
> +
> c2c_he = container_of(he, struct c2c_hist_entry, he);
> - return hist_entry__tui_annotate(he, c2c_he->evsel, NULL);
> + return hist_entry__tui_annotate(he, c2c_he->evsel, NULL, al_addr);

I think it's better to add sample info to annotation like Ravi said.
How about adding this? (not tested)

--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -336,6 +336,8 @@ static int process_sample_event(const struct perf_tool *tool __maybe_unused,
c2c_he__set_node(c2c_he, sample);

hists__inc_nr_samples(&c2c_hists->hists, he->filtered);
+ if (perf_c2c__has_annotation(c2c_hists->hists.hpp_list))
+ addr_map_symbol__inc_samples(mem_info__iaddr(mi), sample, evsel);
ret = hist_entry__append_callchain(he, sample);

if (!ret) {

> }
>
> static void c2c_browser__update_nr_entries(struct hist_browser *hb)
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 32da310b3b62..1de9bb88c379 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -300,6 +300,13 @@ static void disasm_rb_tree__insert(struct annotate_browser *browser,
> rb_insert_color(&al->rb_node, root);
> }
>
> +static void disasm_rb_tree__insert_if_empty(struct annotate_browser *browser,
> + struct annotation_line *al)
> +{
> + if (rb_first(&browser->entries) == NULL)
> + disasm_rb_tree__insert(browser, al);
> +}
> +
> static void annotate_browser__set_top(struct annotate_browser *browser,
> struct annotation_line *pos, u32 idx)
> {
> @@ -396,6 +403,22 @@ static struct annotation_line *annotate_browser__find_new_asm_line(
> return NULL;
> }
>
> +static struct annotation_line *annotate_browser__find_al_addr(struct annotate_browser *browser,
> + u64 al_addr)

We have annotated_source__get_line().

> +{
> + struct symbol *sym = browser->he->ms.sym;
> + struct list_head *head = browser->b.entries;
> + struct annotation_line *al;
> +
> + /* find an annotation line in the new list with the same al_addr */
> + list_for_each_entry(al, head, node) {
> + if (sym->start + al->offset == al_addr)
> + return al;
> + }
> + /* There are no asm lines */
> + return NULL;
> +}
> +
> static struct annotation_line *annotate_browser__find_next_asm_line(
> struct annotate_browser *browser,
> struct annotation_line *al)
> @@ -605,7 +628,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
> @@ -897,6 +920,12 @@ static int annotate_browser__run(struct annotate_browser *browser,
>
> annotate_browser__calc_percent(browser, evsel);
>
> + if (browser->curr_hot == NULL && browser->selection != NULL) {
> + disasm_rb_tree__insert_if_empty(browser, browser->selection);
> + browser->curr_hot = rb_first(&browser->entries);
> + browser->b.use_navkeypressed = false;
> + }

Then it can be like this.

if (browser->selection != NULL)
browser->curr_hot = &selection->rb_node;

> +
> if (browser->curr_hot) {
> annotate_browser__set_rb_top(browser, browser->curr_hot);
> browser->b.navkeypressed = false;
> @@ -1003,6 +1032,8 @@ static int annotate_browser__run(struct annotate_browser *browser,
> nd = annotate_browser__rb_node_by_idx_asm(browser, idx_asm_nd);
> browser->curr_hot = annotate_browser__rb_node_by_idx_asm(browser,
> idx_asm_curr_hot);
> + disasm_rb_tree__insert_if_empty(browser,
> + rb_entry(nd, struct annotation_line, rb_node));

I feel like it should call annotate_browser__calc_percent() after
annotate_browser__toggle_source(). Then just updating curr_hot would
work.

Thanks,
Namhyung

> }
> annotate__scnprintf_title(hists, title, sizeof(title));
> annotate_browser__show(browser, title, help);
> @@ -1139,19 +1170,19 @@ 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 al_addr)
> {
> /* 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, al_addr);
> }
>
> 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 al_addr)
> {
> struct symbol *sym = ms->sym;
> struct annotation *notes = symbol__annotation(sym);
> @@ -1221,6 +1252,19 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
> if (annotate_opts.hide_src_code)
> ui_browser__init_asm_mode(&browser.b);
>
> + /*
> + * If al_addr 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 (al_addr != NO_INITIAL_AL_ADDR) {
> + struct annotation_line *al = annotate_browser__find_al_addr(&browser, al_addr);
> +
> + browser.selection = al;
> + }
> +
> ret = annotate_browser__run(&browser, evsel, hbt);
>
> debuginfo__delete(browser.dbg);
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 487c0b08c003..c34ddc4ca13f 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_AL_ADDR);
> /*
> * offer option to annotate the other branch source or target
> * (if they exists) when returning from annotate
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index c64005278687..9542cf43bd2a 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_AL_ADDR 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 al_addr);
>
> int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
> - struct hist_browser_timer *hbt);
> + struct hist_browser_timer *hbt, u64 ad_addr);
>
> 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
>

Next message: Peng Fan: "Re: [PATCH 6/6] remoteproc: core: Consolidate bool flags into 1-bit bitfields"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v1 7/7] regulator: s2mps11: add support for S2MPS16 regulators"
In reply to: Tianyou Li: "[PATCH v6 3/3] perf tools c2c: Highlight the contention line in the annotate browser"
Next in thread: Tianyou Li: "[PATCH v7 1/2] perf tools c2c: Add annotation support to perf c2c report"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]