Re: [RFC PATCH perf/core v2 14/16] perf probe: Add group name support

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

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