Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753360AbbGTEso (ORCPT ); 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 ); 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 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 CC: Arnaldo Carvalho de Melo , Peter Zijlstra , linux-kernel@vger.kernel.org, Adrian Hunter , Ingo Molnar , Paul Mackerras , Jiri Olsa , Borislav Petkov , Hemant Kumar 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: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4566 Lines: 137 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_: ? - Give just a name on a userspace binary perf probe -x --add = -> probe_: - Give a pair of group and name on a userspace binary perf probe -x --add := -> probe_: - Set an sdt event on a userspace binary perf probe -x --add %: # or %sdt_ ? -> sdt_: - Set an cached event on a userspace binary perf probe -x --add %: # or %probe_ ? -> probe_: Thank you, > >> >> Signed-off-by: Masami Hiramatsu >> --- >> 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; >> /* >> * >> - * 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/