linux-kernel - Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aff0e63b-aca3-f116-6df5-41a8382f1fea@linux.intel.com>
Date:   Mon, 9 Oct 2017 09:40:56 +0800
From:   "Jin, Yao" <yao.jin@...ux.intel.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     jolsa@...nel.org, peterz@...radead.org, mingo@...hat.com,
        alexander.shishkin@...ux.intel.com, Linux-kernel@...r.kernel.org,
        ak@...ux.intel.com, kan.liang@...el.com, yao.jin@...el.com
Subject: Re: [PATCH v1 1/4] perf annotate: create a new hists to manage
 multiple events samples



On 10/7/2017 2:54 AM, Arnaldo Carvalho de Melo wrote:
> Em Sat, Oct 07, 2017 at 12:31:37AM +0800, Jin, Yao escreveu:
>> On 10/5/2017 9:21 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Wed, Aug 16, 2017 at 06:18:33PM +0800, Jin Yao escreveu:
>>>> An issue is found during using perf annotate.
> 
>>>> perf record -e cycles,branches ...
>>>> perf annotate main --stdio
> 
>>>> The result only shows cycles. It should show both cycles and
>>>> branches on the left side. It works with "--group", but need
>>>> this to work even without groups.
>>>
>>> Right, for --group we know that we'll be reading all the counters at
>>> each sample, so it all works and we can use the current design.
> 
>>> When we're not using groups tho, each sample has just one of the events
>>> and we end up with separate views.
> 
>>> Note tho that since the annotation buckets are kept per 'struct symbol'
>>> instance, this problem should be present only in the hist_entry based
>>> views, i.e. 'perf report' and 'perf top', right?
>  
>> Yes, it seems to be in hist_entry based view.
> 
> Ok.
> 
> But note that your initial statement of the problem:
> 
> <quote Ji, Yao>
> An issue is found during using perf annotate.
> 
> perf record -e cycles,branches ...
> perf annotate main --stdio
> 
> The result only shows cycles. It should show both cycles and
> branches on the left side. It works with "--group", but need
> this to work even without groups.
> </>
> 
> Can be solved right now, its just a matter of accessing the other
> buckets in a given symbol, just like we have for --group.
> 
> The only problem is in presenting a list of symbols which can be
> annotated, we have them for each evsel, and you, rightly, want to show
> the list of all symbols in all evsels.
> 
> Ok?
> 

Hi Arnaldo, I just feel there might be another problem when displaying the result.

I will talk about this in below.

>>> I.e. all struct hist_entry->ms.sym instances point to the same stuct
>>> symbol and thus will use the same annotation histogram buckets, i.e.
>>> symbol__annotation(hist_entry->ms.sym) point to the same 'struct
>>> annotation' instance, and then its a matter of passing this pointer to
>>> annotation__histogram(notes, IDX) where this IDX is perf_evsel->idx.
> 
>>> I wonder if we can't just add a new rb_node in struct hist_entry and
>>> have it in two rb_trees, i.e. in two 'struct hists' instances, one per
>>> evsel and one per evlist.
> 
>> Currently we just have per-evsel hists. This idea will create a new per-evlist hists. 
>  
>> struct perf_evlist {
>> 	......
>> 	struct hists	hists;
>> };
>  
>> And let the hist_entry be linked in both per-evsel hists and per-evlist hists. 
>  
>> Please correct me if my understanding is wrong for this idea.
> 
> Yes, do you see problems with trying to do it this way? At a first sight
> seems like it will reuse more code, no?
> 
> I.e. in hists__findnew_entry(), when not finding an existing hist_entry
> in the per-evsel hists you end up calling hist_entry__new(), right here
> you'll add it to the evsel->evlist->hists, and when we want to go from
> a hist_entry to the evlist it is in we'll use:
> 
>   hists_to_evsel(he->hists)->evlist
> 
> Right?
> 
> - Arnaldo
> 

With this method, we can get the events from per-evlist hists. 

For example, when printing the symbol annotate of 'cycles', we can also get the 'branches' from per-evlist hists. So we can print the values of both 'cycles' and 'branches' in annotate view of 'cycles'. 

The problem might be, once the annotate view of 'cycles' being printed, the view of 'branches' will be printed in next. Maybe they are the same symbol (e.g. function A), so duplicated.

The problem is that we only sort the per-evsel hists, but we don't sort the per-evlist hists.

>From my personal opinion, we may need a new sorted hists for multiple events. That will avoid the symbol duplication.

Maybe I misunderstand something, please correct me if I'm wrong.

Thanks
Jin Yao