From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752645AbdJFSyZ (ORCPT <rfc822;w@1wt.eu>);
        Fri, 6 Oct 2017 14:54:25 -0400
Received: from mail.kernel.org ([198.145.29.99]:32870 "EHLO mail.kernel.org"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1752577AbdJFSyY (ORCPT <rfc822;Linux-kernel@vger.kernel.org>);
        Fri, 6 Oct 2017 14:54:24 -0400
DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 05A22217C5
Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org
Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=acme@kernel.org
Date: Fri, 6 Oct 2017 15:54:22 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: "Jin, Yao" <yao.jin@linux.intel.com>
Cc: jolsa@kernel.org, peterz@infradead.org, mingo@redhat.com,
        alexander.shishkin@linux.intel.com, Linux-kernel@vger.kernel.org,
        ak@linux.intel.com, kan.liang@intel.com, yao.jin@intel.com
Subject: Re: [PATCH v1 1/4] perf annotate: create a new hists to manage
 multiple events samples
Message-ID: <20171006185421.GB28623@kernel.org>
References: <1502878716-30817-1-git-send-email-yao.jin@linux.intel.com>
 <1502878716-30817-2-git-send-email-yao.jin@linux.intel.com>
 <20171005132155.GM25388@kernel.org>
 <7d4d7dd4-3bab-a63b-c666-89f7ccd7ace0@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7d4d7dd4-3bab-a63b-c666-89f7ccd7ace0@linux.intel.com>
X-Url: http://acmel.wordpress.com
User-Agent: Mutt/1.9.0 (2017-09-02)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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?

> > 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