Messages in this thread | | | From | Namhyung Kim <> | Date | Fri, 12 Mar 2021 14:49:53 +0900 | Subject | Re: [PATCH] perf annotate: Fix sample events lost in stdio mode |
| |
Hi,
On Fri, Mar 12, 2021 at 12:24 PM Yang Jihong <yangjihong1@huawei.com> 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@huawei.com> 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.
> > 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(¬es->src->cycles_hist); > >>> - zfree(¬es->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(¬es->src->cycles_hist); > > - zfree(¬es->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); }
Thanks, Namhyung
| |