Re: [PATCH] perf annotate: Fix sample events lost in stdio mode

From: Yang Jihong
Date: Fri Mar 12 2021 - 02:20:03 EST


Hello,
On 2021/3/12 13:49, Namhyung Kim wrote:
Hi,

On Fri, Mar 12, 2021 at 12:24 PM Yang Jihong <yangjihong1@xxxxxxxxxx> wrote:

Hello, Namhyung

On 2021/3/11 22:42, Namhyung Kim wrote:
Hi,

On Thu, Mar 11, 2021 at 5:48 PM Yang Jihong <yangjihong1@xxxxxxxxxx> wrote:

Hello,

On 2021/3/6 16:28, Yang Jihong wrote:
In hist__find_annotations function, since have a hist_entry per IP for the same
symbol, we free notes->src to signal already processed this symbol in stdio mode;
when annotate, entry will skipped if notes->src is NULL to avoid repeated output.

I'm not sure it's still true that we have a hist_entry per IP.
Afaik the default sort key is comm,dso,sym which means it should have a single
hist_entry for each symbol. It seems like an old comment..

Emm, yes, we have a hist_entry for per IP.
a member named "sym" in struct "hist_entry" points to symbol,
different IP may point to the same symbol.

Are you sure about this? It seems like a bug then.

Yes, now each IP corresponds to a hist_entry :)

Last week I found that some sample events were missing when perf annotate in stdio mode, so I went through the annotate code carefully.

The event handling process is as follows:
process_sample_event
evsel_add_sample
hists__add_entry
__hists__add_entry
hists__findnew_entry
hist_entry__new -> here allock new hist_entry

hist_entry__inc_addr_samples
symbol__inc_addr_samples
symbol__hists
annotated_source__new -> here alloc annotate soruce
annotated_source__alloc_histograms -> here alloc histograms

By bugs, do you mean there's something wrong?

The hist_entry struct is as follows:
struct hist_entry {
...
struct map_symbol ms;
...
};
struct map_symbol {
struct maps *maps;
struct map *map;
struct symbol *sym;
};


However, there is a problem, for example, run the following command:

# perf record -e branch-misses -e branch-instructions -a sleep 1

perf.data file contains different types of sample event.

If the same IP sample event exists in branch-misses and branch-instructions,
this event uses the same symbol. When annotate branch-misses events, notes->src
corresponding to this event is set to null, as a result, when annotate
branch-instructions events, this event is skipped and no annotate is output.

Solution of this patch is to add a u8 member to struct sym_hist and use a bit to
indicate whether the symbol has been processed.
Because different types of event correspond to different sym_hist, no conflict
occurs.
---
tools/perf/builtin-annotate.c | 22 ++++++++++++++--------
tools/perf/util/annotate.h | 4 ++++
2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index a23ba6bb99b6..c8c67892ae82 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -372,15 +372,21 @@ static void hists__find_annotations(struct hists *hists,
if (next != NULL)
nd = next;
} else {
- hist_entry__tty_annotate(he, evsel, ann);
+ struct sym_hist *h = annotated_source__histogram(notes->src,
+ evsel->idx);
+
+ if (h->processed == 0) {
+ hist_entry__tty_annotate(he, evsel, ann);
+
+ /*
+ * Since we have a hist_entry per IP for the same
+ * symbol, set processed flag of evsel in sym_hist
+ * to signal we already processed this symbol.
+ */
+ h->processed = 1;
+ }
+
nd = rb_next(nd);
- /*
- * Since we have a hist_entry per IP for the same
- * symbol, free he->ms.sym->src to signal we already
- * processed this symbol.
- */
- zfree(&notes->src->cycles_hist);
- zfree(&notes->src);
}
}
}
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 096cdaf21b01..89872bfdc958 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -228,6 +228,10 @@ void symbol__calc_percent(struct symbol *sym, struct evsel *evsel);
struct sym_hist {
u64 nr_samples;
u64 period;
+
+ u8 processed : 1, /* whether symbol has been processed, used for annotate */
+ __reserved : 7;

I think just a bool member is fine.

OK, I have submitted the v2 patch and changed to bool member, new patch
is as follows, look forward to your review:
https://lore.kernel.org/patchwork/patch/1393901/

+
struct sym_hist_entry addr[];
};


Please check whether this solution is feasible, look forward to your review.

What about this? (not tested)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index a23ba6bb99b6..a91fe45bd69f 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -374,13 +374,6 @@ static void hists__find_annotations(struct hists *hists,
} else {
hist_entry__tty_annotate(he, evsel, ann);
nd = rb_next(nd);
- /*
- * Since we have a hist_entry per IP for the same
- * symbol, free he->ms.sym->src to signal we already
- * processed this symbol.
- */
- zfree(&notes->src->cycles_hist);
- zfree(&notes->src);
}
}
}

This solution may have the following problem:
For example, if two sample events are in two different processes but in
the same symbol, repeated output may occur.
Therefore, a flag is required to indicate whether the symbol has been
processed to avoid repeated output.

Hmm.. ok. Yeah we don't care about the processes here.
Then we should remove it from the sort key like below:

@@ -624,6 +617,7 @@ int cmd_annotate(int argc, const char **argv)
if (setup_sorting(annotate.session->evlist) < 0)
usage_with_options(annotate_usage, options);
} else {
+ sort_order = "dso,symbol";
if (setup_sorting(NULL) < 0)
usage_with_options(annotate_usage, options);
}


Are you referring to this solution?
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -374,13 +374,6 @@ static void hists__find_annotations(struct hists *hists,
} else {
hist_entry__tty_annotate(he, evsel, ann);
nd = rb_next(nd);
- /*
- * Since we have a hist_entry per IP for the same
- * symbol, free he->ms.sym->src to signal we already
- * processed this symbol.
- */
- zfree(&notes->src->cycles_hist);
- zfree(&notes->src);
}
}
}
@@ -624,6 +617,7 @@ int cmd_annotate(int argc, const char **argv)
if (setup_sorting(annotate.session->evlist) < 0)
usage_with_options(annotate_usage, options);
} else {
+ sort_order = "dso,symbol";
if (setup_sorting(NULL) < 0)
usage_with_options(annotate_usage, options);
}
It seems to be a better solution without adding new member.
I just tested it and it works.

If we decide to use this solution, I'll resubmit a v3 patch.
Thanks,
Namhyung
.

Thanks,
Yang
.