Messages in this thread Patch in this message | | | Subject | Re: [PATCH] perf annotate: Fix sample events lost in stdio mode | From | Yang Jihong <> | Date | Fri, 12 Mar 2021 15:18:52 +0800 |
| |
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
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(¬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); > } > > 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. > Thanks, > Namhyung > . > Thanks, Yang .
| |