* [PATCH] perf annotate: Fix sample events lost in stdio mode @ 2021-03-06 8:28 Yang Jihong 2021-03-11 8:48 ` Yang Jihong 0 siblings, 1 reply; 10+ messages in thread From: Yang Jihong @ 2021-03-06 8:28 UTC (permalink / raw) To: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa, namhyung, yao.jin, gustavoars, mliska, linux-kernel Cc: yangjihong1, zhangjinhao2 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. 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; + struct sym_hist_entry addr[]; }; -- 2.30.GIT ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: Fix sample events lost in stdio mode 2021-03-06 8:28 [PATCH] perf annotate: Fix sample events lost in stdio mode Yang Jihong @ 2021-03-11 8:48 ` Yang Jihong 2021-03-11 14:42 ` Namhyung Kim 0 siblings, 1 reply; 10+ messages in thread From: Yang Jihong @ 2021-03-11 8:48 UTC (permalink / raw) To: peterz, mingo, acme, mark.rutland, alexander.shishkin, jolsa, namhyung, yao.jin, gustavoars, mliska, linux-kernel Cc: zhangjinhao2 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. > > 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; > + > struct sym_hist_entry addr[]; > }; > > Please check whether this solution is feasible, look forward to your review. Thanks, Yang ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: Fix sample events lost in stdio mode 2021-03-11 8:48 ` Yang Jihong @ 2021-03-11 14:42 ` Namhyung Kim 2021-03-12 3:24 ` Yang Jihong 0 siblings, 1 reply; 10+ messages in thread From: Namhyung Kim @ 2021-03-11 14:42 UTC (permalink / raw) To: Yang Jihong Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Yao Jin, gustavoars, mliska, linux-kernel, zhangjinhao2 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.. > > > > 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. > > + > > 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); } } } Thanks, Namhyung ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: Fix sample events lost in stdio mode 2021-03-11 14:42 ` Namhyung Kim @ 2021-03-12 3:24 ` Yang Jihong 2021-03-12 5:49 ` Namhyung Kim 0 siblings, 1 reply; 10+ messages in thread From: Yang Jihong @ 2021-03-12 3:24 UTC (permalink / raw) To: Namhyung Kim Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Yao Jin, gustavoars, mliska, linux-kernel, zhangjinhao2 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. 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. > Thanks, > Namhyung > . > Thanks, Yang . ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: Fix sample events lost in stdio mode 2021-03-12 3:24 ` Yang Jihong @ 2021-03-12 5:49 ` Namhyung Kim 2021-03-12 7:18 ` Yang Jihong 0 siblings, 1 reply; 10+ messages in thread From: Namhyung Kim @ 2021-03-12 5:49 UTC (permalink / raw) To: Yang Jihong Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Yao Jin, gustavoars, mliska, linux-kernel, zhangjinhao2 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: Fix sample events lost in stdio mode 2021-03-12 5:49 ` Namhyung Kim @ 2021-03-12 7:18 ` Yang Jihong 2021-03-12 8:39 ` Namhyung Kim 0 siblings, 1 reply; 10+ messages in thread From: Yang Jihong @ 2021-03-12 7:18 UTC (permalink / raw) To: Namhyung Kim Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Yao Jin, gustavoars, mliska, linux-kernel, zhangjinhao2 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 . ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: Fix sample events lost in stdio mode 2021-03-12 7:18 ` Yang Jihong @ 2021-03-12 8:39 ` Namhyung Kim 2021-03-12 10:20 ` Yang Jihong 0 siblings, 1 reply; 10+ messages in thread From: Namhyung Kim @ 2021-03-12 8:39 UTC (permalink / raw) To: Yang Jihong Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Yao Jin, gustavoars, mliska, linux-kernel, zhangjinhao2 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: Fix sample events lost in stdio mode 2021-03-12 8:39 ` Namhyung Kim @ 2021-03-12 10:20 ` Yang Jihong 2021-03-13 2:00 ` Yang Jihong 0 siblings, 1 reply; 10+ messages in thread From: Yang Jihong @ 2021-03-12 10:20 UTC (permalink / raw) To: Namhyung Kim Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Yao Jin, gustavoars, mliska, linux-kernel, zhangjinhao2 Hello, On 2021/3/12 16:39, Namhyung Kim wrote: > 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. :) > OK, :) > >>>>> 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). > OK, I'll submit a v3 patch based on the "changing the sort order" solution. Thanks, Yang > Thanks, > Namhyung > . > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: Fix sample events lost in stdio mode 2021-03-12 10:20 ` Yang Jihong @ 2021-03-13 2:00 ` Yang Jihong 2021-03-13 2:23 ` Yang Jihong 0 siblings, 1 reply; 10+ messages in thread From: Yang Jihong @ 2021-03-13 2:00 UTC (permalink / raw) To: Namhyung Kim Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Yao Jin, gustavoars, mliska, linux-kernel, zhangjinhao2 Hello, Namhyung On 2021/3/12 18:20, Yang Jihong wrote: > Hello, > > On 2021/3/12 16:39, Namhyung Kim wrote: >> 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. :) >> > OK, :) >> >>>>>> 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). >> > OK, I'll submit a v3 patch based on the "changing the sort order" solution. > I have submitted the v3 patch, look forward to your review: https://lore.kernel.org/patchwork/patch/1394619/ > Thanks, > Yang >> Thanks, >> Namhyung Thanks, Yang >> . >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: Fix sample events lost in stdio mode 2021-03-13 2:00 ` Yang Jihong @ 2021-03-13 2:23 ` Yang Jihong 0 siblings, 0 replies; 10+ messages in thread From: Yang Jihong @ 2021-03-13 2:23 UTC (permalink / raw) To: Namhyung Kim Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Yao Jin, gustavoars, mliska, linux-kernel, zhangjinhao2 Hello, On 2021/3/13 10:00, Yang Jihong wrote: > Hello, Namhyung > On 2021/3/12 18:20, Yang Jihong wrote: >> Hello, >> >> On 2021/3/12 16:39, Namhyung Kim wrote: >>> 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. :) >>> >> OK, :) >>> >>>>>>> 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). >>> >> OK, I'll submit a v3 patch based on the "changing the sort order" >> solution. >> > I have submitted the v3 patch, look forward to your review: > https://lore.kernel.org/patchwork/patch/1394619/ > The first line of comments in the v3 patch is not empty and has been modified. For details, see v4 patch: https://lore.kernel.org/patchwork/patch/1394623/ Please review the new patch :) >> Thanks, >> Yang >>> Thanks, >>> Namhyung > Thanks, > Yang >>> . >>> Thanks, Yang ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-03-13 2:24 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-06 8:28 [PATCH] perf annotate: Fix sample events lost in stdio mode Yang Jihong 2021-03-11 8:48 ` Yang Jihong 2021-03-11 14:42 ` Namhyung Kim 2021-03-12 3:24 ` Yang Jihong 2021-03-12 5:49 ` Namhyung Kim 2021-03-12 7:18 ` Yang Jihong 2021-03-12 8:39 ` Namhyung Kim 2021-03-12 10:20 ` Yang Jihong 2021-03-13 2:00 ` Yang Jihong 2021-03-13 2:23 ` Yang Jihong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).