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 S1754013AbbG0OFx (ORCPT <rfc822;w@1wt.eu>);
	Mon, 27 Jul 2015 10:05:53 -0400
Received: from mail-pa0-f47.google.com ([209.85.220.47]:36160 "EHLO
	mail-pa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753832AbbG0OFu (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 27 Jul 2015 10:05:50 -0400
Date: Mon, 27 Jul 2015 23:03:20 +0900
From: Namhyung Kim <namhyung@kernel.org>
To: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>,
        Hemant Kumar <hemant@linux.vnet.ibm.com>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>, linux-kernel@vger.kernel.org,
        Adrian Hunter <adrian.hunter@intel.com>,
        Ingo Molnar <mingo@redhat.com>, Jiri Olsa <jolsa@kernel.org>,
        Borislav Petkov <bp@suse.de>
Subject: Re: Re: Re: [RFC PATCH perf/core v2 00/16] perf-probe --cache and
 SDT support
Message-ID: <20150727140320.GF22022@danjae.kornet>
References: <20150715091352.8915.87480.stgit@localhost.localdomain>
 <55A7215F.40803@linux.vnet.ibm.com>
 <55A874C6.5030202@hitachi.com>
 <55AFA4E2.4040801@linux.vnet.ibm.com>
 <55B0E872.8030206@hitachi.com>
 <20150723140127.GD3152@kernel.org>
 <55B11555.9060100@hitachi.com>
 <20150724075519.GA19672@sejong>
 <20150724155237.GA300@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20150724155237.GA300@kernel.org>
User-Agent: Mutt/1.5.23+102 (2ca89bed6448) (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Arnaldo,

On Fri, Jul 24, 2015 at 12:52:37PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jul 24, 2015 at 04:55:19PM +0900, Namhyung Kim escreveu:
> > On Fri, Jul 24, 2015 at 01:24:53AM +0900, Masami Hiramatsu wrote:
> > > On 2015/07/23 23:01, Arnaldo Carvalho de Melo wrote:
> > > > Em Thu, Jul 23, 2015 at 10:13:22PM +0900, Masami Hiramatsu escreveu:
> 
> > > The following patterns we've discussed.
> > > 
> > >  - <provider>:<name>
> > > 	simple, but could easily clash with others.
> > >  - probe_<provider>:<name>
> > >  - sdt_<provider>:<name>
> > > 	also simple and similar to current solution. but fragile against
> > > 	clash among SDTs.
> > >  - probe_<binary>:<provider>_<name>
> > > 	also simple, but if provider or/and name has '_', it is hard to
> > > 	split the provider and name. and fragile against clash among SDTs too.
> > >  - <provider>_<buildid>/<name>
> > > 	possible, but ugly since buildid is a random long xdigits(maybe cut up
> > > 	to 8 or 12 bytes).
>  
> > As I said, we might allow name clashes as they're rare.  I don't want
> > to make it complex just for an uncommon case.  I think such a
> > duplicate name is fine as long as 'perf list' indicates it and 'perf
> > record' enable them all.
> 
> I made some comments about enabling it all by default, look below.

OK.


>  
> > If we agreed to extend the event format, I'd like to keep it simple
> > and to make it optional to add more info (separated by colon?).
> 
> Reading this again after writing what is below: my suggestion is to use
> @, see rationale below.

I'm fine with using @.


>  
> > Maybe something like below.  Suppose we have 3 SDT events with a same
> > name:
> > 
> >  /some/where/dir1/libfoo1.so (build-id: 0x1234...) -->  foo:bar
> >  /some/where/dir2/libfoo1.so (build-id: 0x5678...) -->  foo:bar
> >  /some/where/dir2/libfoo2.so (build-id: 0xabcd...) -->  foo:bar
> > 
> > So perf list shows the single name, but also says it has 3 events.
> > 
> >   $ perf list sdt_foo:bar
> >   
> >   sdt_foo:bar (total 3 events)            [User SDT event]
> 
> I would show what desambiguates them in non verbose mode, i.e., the
> above would be:
> 
>    $ perf list sdt_foo:bar
> 
>    sdt_foo:bar:dir1/libfoo1.so   [User SDT event]
>    sdt_foo:bar:dir2/libfoo1.so   [User SDT event]
>    sdt_foo:bar:libfoo2.so        [User SDT event]

Then it should use @ here too.


> 
>  The -v one would should both the full path and the buildid, but this
> is just polishing up the default output a bit to make it more
> informative.

Fair enough.


> 
> 	Now what should be the default when one does:
> 
>    perf record -e sdt_foo:bar
> 
>         Will it enable all events or bail out and state that multiple
> events with that name matches, requiring a '--all-matches' to really
> apply it to all events with the same name?
> 
> 	Humm, this probably will not be that common, so perhaps just
> use all matches by default while telling the user that all those places
> were used and if the user wants just one of them, be more precise,
> adding somehow a disambiguator.

Either is fine to me.  Mayb we can add a config option to select the
default bahavior.. :)


> 
> 	That would be something like this:
> 
>     perf record -e sdt_foo:bar:0x1234
> 
> 	Or perhaps:
> 
>     perf record -e sdt_foo:bar@0x1234
> 
> 	Because in this case the 'at' meaning of '@' makes sense, i.e.
> use the std_foo:bar event at the DSO with a 0x1234 buildid?

IMHO @ looks perfect for pathnames but I don't know about build-id as
it can be thought as some address.  Anyway I still think @ is a good
choice though. ;-)


> 
> 	Additionally, for people that don't want to mess with buildids
> because its environment is deemed well controlled and this works and is
> unambiguous, looking at the LD_LIBRARY_PATH or equivalent:

Ah, good idea.


> 
>     perf record -e sdt_foo:bar@libfoo2
> 
> 	Full paths could be used as well.
> > 
> >   $ perf list -v sdt_foo:bar
> >   
> >   sdt_foo:bar:libfoo1.so:0x1234...        [User SDT event]
> >   sdt_foo:bar:libfoo1.so:0x5678...        [User SDT event]
> >   sdt_foo:bar:libfoo2.so:0xabcd...        [User SDT event]
> 
> > 
> > Now perf record can accept any of these forms..
> > 
> >   # record all 3 events
> >   $ perf record -e 'sdt_foo:bar'
> > 
> >   # record 2 events from libfoo1.so
> >   $ perf record -e 'sdt_foo:bar:libfoo1.so'
> > 
> >   # record only 1 event
> >   $ perf record -e 'sdt_foo:bar:libfoo1.so:0x1234...'
> > 
> > 
> > What do you think?
> 
> If nothing prevents using @ with the meaning of "event at LOCATION"
> where LOCATION is a buildid (noticed because it starts with 0x) or
> a library name or pathname, then that looks more natural.

Agreed.

Thanks,
Namhyung