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 S1753183AbbGSKSy (ORCPT <rfc822;w@1wt.eu>);
	Sun, 19 Jul 2015 06:18:54 -0400
Received: from mail-pd0-f170.google.com ([209.85.192.170]:34056 "EHLO
	mail-pd0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753067AbbGSKSw (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sun, 19 Jul 2015 06:18:52 -0400
Date: Sun, 19 Jul 2015 19:16:33 +0900
From: Namhyung Kim <namhyung@kernel.org>
To: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
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
Message-ID: <20150719101633.GA25163@danjae.kornet>
References: <20150715091352.8915.87480.stgit@localhost.localdomain>
 <20150715091530.8915.91220.stgit@localhost.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20150715091530.8915.91220.stgit@localhost.localdomain>
User-Agent: Mutt/1.5.23+89 (0255b37be491) (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

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

What do you think?


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