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 S1753360AbbGTEso (ORCPT <rfc822;w@1wt.eu>);
	Mon, 20 Jul 2015 00:48:44 -0400
Received: from mail7.hitachi.co.jp ([133.145.228.42]:49800 "EHLO
	mail7.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753003AbbGTEsn (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 20 Jul 2015 00:48:43 -0400
Message-ID: <55AC7DA4.80301@hitachi.com>
Date: Mon, 20 Jul 2015 13:48:36 +0900
From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Organization: Hitachi, Ltd., Japan
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0
MIME-Version: 1.0
To: Namhyung Kim <namhyung@kernel.org>
CC: Arnaldo Carvalho de Melo <acme@kernel.org>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>, linux-kernel@vger.kernel.org,
        Adrian Hunter <adrian.hunter@intel.com>,
        Ingo Molnar <mingo@redhat.com>, Paul Mackerras <paulus@samba.org>,
        Jiri Olsa <jolsa@kernel.org>, Borislav Petkov <bp@suse.de>,
        Hemant Kumar <hemant@linux.vnet.ibm.com>
Subject: Re: [RFC PATCH perf/core v2 14/16] perf probe: Add group name support
References: <20150715091352.8915.87480.stgit@localhost.localdomain> <20150715091530.8915.91220.stgit@localhost.localdomain> <20150719101633.GA25163@danjae.kornet>
In-Reply-To: <20150719101633.GA25163@danjae.kornet>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2015/07/19 19:16, Namhyung Kim wrote:
> On Wed, Jul 15, 2015 at 06:15:30PM +0900, Masami Hiramatsu wrote:
>> Allow user to set group name for adding new event.
>> Note that this can easily shot yourself in the foot.
>> E.g. Existing group name can conflict with other events.
>> Especially, using the group name reserved for kernel
>> modules can break something when loading/unloading
>> modules.
> 
> Yes, I agree that this can be dangerous.  How about enforcing
> [ku]probes to make the directory of dynamic events safely?

What the safety issue would you afraid?

>  I think
> it'd be better putting all dynamic events in a single directory -
> e.g. $tracefs/events/probe/.  Any events lack group name are created
> in the directory.  Any events have group name create subdirectories as
> group name under the directory.  The perf tools (and others too)
> should be changed to lookup the directory after the usual location.

That will be possible, but includes a big change on event namespace,
e.g. how we'll show the events by perf-list? Even if we can avoid
namespace conflict on tracefs, perf-list event namespace is still
fragile.

> What do you think?

I think there are 2 purposes of probe-event, one is just additional
debug points, another is an extensible event-set. The former will not
any namespace problem, we just add it into new namespace. But latter
requires to be treated as a part of existing (in-kernel) events.
And (userspace)SDT is clearly the latter one.

However, avoiding the conflict of namespace is also important, how
about simply using sdt_<PROVIDER>:<NAME> ?

- Give just a name on a userspace binary
  perf probe -x <BIN> --add <NAME>=<PROBEDEF>
   -> probe_<BIN>:<NAME>
- Give a pair of group and name on a userspace binary
  perf probe -x <BIN> --add <GRP>:<NAME>=<PROBEDEF>
   -> probe_<GRP>:<NAME>
- Set an sdt event on a userspace binary
  perf probe -x <BIN> --add %<PROV>:<NAME>   # or %sdt_<PROV> ?
   -> sdt_<PROV>:<NAME>
- Set an cached event on a userspace binary
  perf probe -x <BIN> --add %<GRP>:<NAME>    # or %probe_<GRP> ?
   -> probe_<GRP>:<NAME>

Thank you,

> 
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> ---
>>  tools/perf/util/probe-event.c |   23 ++++++++++++++---------
>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index 262f9d3..c19a380 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -1141,10 +1141,8 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
>>  	bool file_spec = false;
>>  	/*
>>  	 * <Syntax>
>> -	 * perf probe [EVENT=]SRC[:LN|;PTN]
>> -	 * perf probe [EVENT=]FUNC[@SRC][+OFFS|%return|:LN|;PAT]
>> -	 *
>> -	 * TODO:Group name support
>> +	 * perf probe [GRP:][EVENT=]SRC[:LN|;PTN]
> 
> Shouldn't it be
>                       [[GRP:]EVENT=]
> 
> ?
> 
>> +	 * perf probe [GRP:][EVENT=]FUNC[@SRC][+OFFS|%return|:LN|;PAT]
> 
> Ditto.
> 
> Thanks,
> Namhyung
> 
> 
>>  	 */
>>  	if (!arg)
>>  		return -EINVAL;
>> @@ -1153,11 +1151,19 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
>>  	if (ptr && *ptr == '=') {	/* Event name */
>>  		*ptr = '\0';
>>  		tmp = ptr + 1;
>> -		if (strchr(arg, ':')) {
>> -			semantic_error("Group name is not supported yet.\n");
>> -			return -ENOTSUP;
>> -		}
>> +		ptr = strchr(arg, ':');
>> +		if (ptr) {
>> +			*ptr = '\0';
>> +			if (!is_c_func_name(arg))
>> +				goto not_fname;
>> +			pev->group = strdup(arg);
>> +			if (!pev->group)
>> +				return -ENOMEM;
>> +			arg = ptr + 1;
>> +		} else
>> +			pev->group = NULL;
>>  		if (!is_c_func_name(arg)) {
>> +not_fname:
>>  			semantic_error("%s is bad for event name -it must "
>>  				       "follow C symbol-naming rule.\n", arg);
>>  			return -EINVAL;
>> @@ -1165,7 +1171,6 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
>>  		pev->event = strdup(arg);
>>  		if (pev->event == NULL)
>>  			return -ENOMEM;
>> -		pev->group = NULL;
>>  		arg = tmp;
>>  	}
>>  
>>
>>
> 


-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com