Messages in this thread | | | From | Namhyung Kim <> | Date | Fri, 12 Mar 2021 17:39:57 +0900 | Subject | Re: [PATCH] perf annotate: Fix sample events lost in stdio mode |
| |
On Fri, Mar 12, 2021 at 4:19 PM Yang Jihong <yangjihong1@huawei.com> wrote: > > > Hello, > On 2021/3/12 13:49, Namhyung Kim wrote: > > 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. > > > 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
Yeah, so this is for a symbol.
> > 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
This should be for each IP (ideally it should be per instruction).
> > By bugs, do you mean there's something wrong?
No. I think we were saying about different things. :)
> >>> 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); > > } > > > > > 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(¬es->src->cycles_hist); > - zfree(¬es->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.
I prefer changing the sort order (and removing the zfree and comments).
Thanks, Namhyung
| |