[PATCH] perf annotate: Fix sample events lost in stdio mode

[PATCH] perf annotate: Fix sample events lost in stdio mode

[Yang Jihong]

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(&notes->src->cycles_hist);
-			zfree(&notes->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

Re: [PATCH] perf annotate: Fix sample events lost in stdio mode

[Yang Jihong]

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(&notes->src->cycles_hist);
> -			zfree(&notes->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

Re: [PATCH] perf annotate: Fix sample events lost in stdio mode

[Namhyung Kim]

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(&notes->src->cycles_hist);
> > -                     zfree(&notes->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(&notes->src->cycles_hist);
-                       zfree(&notes->src);
                }
        }
 }

Thanks,
Namhyung

Re: [PATCH] perf annotate: Fix sample events lost in stdio mode

[Yang Jihong]

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(&notes->src->cycles_hist);
>>> -                     zfree(&notes->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(&notes->src->cycles_hist);
> -                       zfree(&notes->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
.

Re: [PATCH] perf annotate: Fix sample events lost in stdio mode

[Namhyung Kim]

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(&notes->src->cycles_hist);
> >>> -                     zfree(&notes->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(&notes->src->cycles_hist);
> > -                       zfree(&notes->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

Re: [PATCH] perf annotate: Fix sample events lost in stdio mode

[Yang Jihong]

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(&notes->src->cycles_hist);
>>>>> -                     zfree(&notes->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(&notes->src->cycles_hist);
>>> -                       zfree(&notes->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(&notes->src->cycles_hist);
-                       zfree(&notes->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
.

Re: [PATCH] perf annotate: Fix sample events lost in stdio mode

[Namhyung Kim]

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(&notes->src->cycles_hist);
> >>> -                       zfree(&notes->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(&notes->src->cycles_hist);
> -                       zfree(&notes->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

Re: [PATCH] perf annotate: Fix sample events lost in stdio mode

[Yang Jihong]

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(&notes->src->cycles_hist);
>>>>> -                       zfree(&notes->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(&notes->src->cycles_hist);
>> -                       zfree(&notes->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
> .
> 

Re: [PATCH] perf annotate: Fix sample events lost in stdio mode

[Yang Jihong]

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(&notes->src->cycles_hist);
>>>>>> -                       zfree(&notes->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(&notes->src->cycles_hist);
>>> -                       zfree(&notes->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
>> .
>>

Re: [PATCH] perf annotate: Fix sample events lost in stdio mode

[Yang Jihong]

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(&notes->src->cycles_hist);
>>>>>>> -                       zfree(&notes->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(&notes->src->cycles_hist);
>>>> -                       zfree(&notes->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