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
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
Hi,
On Thu, Mar 11, 2021 at 5:48 PM Yang Jihong <[email protected]> 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
Hello, Namhyung
On 2021/3/11 22:42, Namhyung Kim wrote:
> Hi,
>
> On Thu, Mar 11, 2021 at 5:48 PM Yang Jihong <[email protected]> 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
.
Hi,
On Fri, Mar 12, 2021 at 12:24 PM Yang Jihong <[email protected]> wrote:
>
> Hello, Namhyung
>
> On 2021/3/11 22:42, Namhyung Kim wrote:
> > Hi,
> >
> > On Thu, Mar 11, 2021 at 5:48 PM Yang Jihong <[email protected]> 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
Hello,
On 2021/3/12 13:49, Namhyung Kim wrote:
> Hi,
>
> On Fri, Mar 12, 2021 at 12:24 PM Yang Jihong <[email protected]> wrote:
>>
>> Hello, Namhyung
>>
>> On 2021/3/11 22:42, Namhyung Kim wrote:
>>> Hi,
>>>
>>> On Thu, Mar 11, 2021 at 5:48 PM Yang Jihong <[email protected]> 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
.
On Fri, Mar 12, 2021 at 4:19 PM Yang Jihong <[email protected]> wrote:
>
>
> Hello,
> On 2021/3/12 13:49, Namhyung Kim wrote:
> > Hi,
> >
> > On Fri, Mar 12, 2021 at 12:24 PM Yang Jihong <[email protected]> wrote:
> >>
> >> Hello, Namhyung
> >>
> >> On 2021/3/11 22:42, Namhyung Kim wrote:
> >>> Hi,
> >>>
> >>> On Thu, Mar 11, 2021 at 5:48 PM Yang Jihong <[email protected]> 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
Hello,
On 2021/3/12 16:39, Namhyung Kim wrote:
> On Fri, Mar 12, 2021 at 4:19 PM Yang Jihong <[email protected]> wrote:
>>
>>
>> Hello,
>> On 2021/3/12 13:49, Namhyung Kim wrote:
>>> Hi,
>>>
>>> On Fri, Mar 12, 2021 at 12:24 PM Yang Jihong <[email protected]> wrote:
>>>>
>>>> Hello, Namhyung
>>>>
>>>> On 2021/3/11 22:42, Namhyung Kim wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, Mar 11, 2021 at 5:48 PM Yang Jihong <[email protected]> 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
> .
>
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 <[email protected]>
>> wrote:
>>>
>>>
>>> Hello,
>>> On 2021/3/12 13:49, Namhyung Kim wrote:
>>>> Hi,
>>>>
>>>> On Fri, Mar 12, 2021 at 12:24 PM Yang Jihong
>>>> <[email protected]> wrote:
>>>>>
>>>>> Hello, Namhyung
>>>>>
>>>>> On 2021/3/11 22:42, Namhyung Kim wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Thu, Mar 11, 2021 at 5:48 PM Yang Jihong
>>>>>> <[email protected]> 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
>> .
>>
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 <[email protected]>
>>> wrote:
>>>>
>>>>
>>>> Hello,
>>>> On 2021/3/12 13:49, Namhyung Kim wrote:
>>>>> Hi,
>>>>>
>>>>> On Fri, Mar 12, 2021 at 12:24 PM Yang Jihong
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>> Hello, Namhyung
>>>>>>
>>>>>> On 2021/3/11 22:42, Namhyung Kim wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Thu, Mar 11, 2021 at 5:48 PM Yang Jihong
>>>>>>> <[email protected]> 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