Date:   Fri, 12 Mar 2021 18:20:03 +0800
From:   Yang Jihong <yangjihong1@...wei.com>
To:     Namhyung Kim <namhyung@...nel.org>
CC:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Yao Jin <yao.jin@...ux.intel.com>, <gustavoars@...nel.org>,
        <mliska@...e.cz>, linux-kernel <linux-kernel@...r.kernel.org>,
        <zhangjinhao2@...wei.com>
Subject: Re: [PATCH] perf annotate: Fix sample events lost in stdio mode

Hello,

On 2021/3/12 16:39, Namhyung Kim wrote:
> On Fri, Mar 12, 2021 at 4:19 PM Yang Jihong <yangjihong1@...wei.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@...wei.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@...wei.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
> .
> 

Powered by blists - more mailing lists