[PATCH] perf parse: Allow names to start with digits

[PATCH] perf parse: Allow names to start with digits

[Dominique Martinet]

Tracepoints can start with digits, although we don't have many of these:

$ rg -g '*.h' '\bTRACE_EVENT\([0-9]'
net/mac802154/trace.h
53:TRACE_EVENT(802154_drv_return_int,
...

net/ieee802154/trace.h
66:TRACE_EVENT(802154_rdev_add_virtual_intf,
...

include/trace/events/9p.h
124:TRACE_EVENT(9p_client_req,
...

Just allow names to start with digits too so e.g. perf probe -e '9p:*'
works

Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
 tools/perf/util/parse-events.l | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 5b6e4b5249cf..4133d6950d29 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -211,7 +211,7 @@ bpf_source	[^,{}]+\.c[a-zA-Z0-9._]*
 num_dec		[0-9]+
 num_hex		0x[a-fA-F0-9]+
 num_raw_hex	[a-fA-F0-9]+
-name		[a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
+name		[a-zA-Z0-9_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
 name_tag	[\'][a-zA-Z_*?\[\]][a-zA-Z0-9_*?\-,\.\[\]:=]*[\']
 name_minus	[a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
 drv_cfg_term	[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
-- 
2.36.1

Re: [PATCH] perf parse: Allow names to start with digits

[Dominique Martinet]

Hello,

just making sure my mail didn't get lost -- would anyone have time to
look at this?

I don't mind if it's slow or another solution is taken, I'd just like to
be able to use 9p probes with perf eventually :)

Thanks!

Dominique Martinet wrote on Sun, Jun 12, 2022 at 03:15:08PM +0900:
> Tracepoints can start with digits, although we don't have many of these:
> 
> $ rg -g '*.h' '\bTRACE_EVENT\([0-9]'
> net/mac802154/trace.h
> 53:TRACE_EVENT(802154_drv_return_int,
> ...
> 
> net/ieee802154/trace.h
> 66:TRACE_EVENT(802154_rdev_add_virtual_intf,
> ...
> 
> include/trace/events/9p.h
> 124:TRACE_EVENT(9p_client_req,
> ...
> 
> Just allow names to start with digits too so e.g. perf probe -e '9p:*'
> works
> 
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
>  tools/perf/util/parse-events.l | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 5b6e4b5249cf..4133d6950d29 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -211,7 +211,7 @@ bpf_source	[^,{}]+\.c[a-zA-Z0-9._]*
>  num_dec		[0-9]+
>  num_hex		0x[a-fA-F0-9]+
>  num_raw_hex	[a-fA-F0-9]+
> -name		[a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
> +name		[a-zA-Z0-9_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
>  name_tag	[\'][a-zA-Z_*?\[\]][a-zA-Z0-9_*?\-,\.\[\]:=]*[\']
>  name_minus	[a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
>  drv_cfg_term	[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?

Re: [PATCH] perf parse: Allow names to start with digits

[Arnaldo Carvalho de Melo]

Em Sat, Jul 02, 2022 at 06:49:31PM +0900, Dominique Martinet escreveu:
> Hello,
> 
> just making sure my mail didn't get lost -- would anyone have time to
> look at this?
> 
> I don't mind if it's slow or another solution is taken, I'd just like to
> be able to use 9p probes with perf eventually :)

Jiri, seems ok, can you please review?

Thanks,

- Arnaldo
 
> Thanks!
> 
> Dominique Martinet wrote on Sun, Jun 12, 2022 at 03:15:08PM +0900:
> > Tracepoints can start with digits, although we don't have many of these:
> > 
> > $ rg -g '*.h' '\bTRACE_EVENT\([0-9]'
> > net/mac802154/trace.h
> > 53:TRACE_EVENT(802154_drv_return_int,
> > ...
> > 
> > net/ieee802154/trace.h
> > 66:TRACE_EVENT(802154_rdev_add_virtual_intf,
> > ...
> > 
> > include/trace/events/9p.h
> > 124:TRACE_EVENT(9p_client_req,
> > ...
> > 
> > Just allow names to start with digits too so e.g. perf probe -e '9p:*'
> > works
> > 
> > Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> > ---
> >  tools/perf/util/parse-events.l | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> > index 5b6e4b5249cf..4133d6950d29 100644
> > --- a/tools/perf/util/parse-events.l
> > +++ b/tools/perf/util/parse-events.l
> > @@ -211,7 +211,7 @@ bpf_source	[^,{}]+\.c[a-zA-Z0-9._]*
> >  num_dec		[0-9]+
> >  num_hex		0x[a-fA-F0-9]+
> >  num_raw_hex	[a-fA-F0-9]+
> > -name		[a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
> > +name		[a-zA-Z0-9_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
> >  name_tag	[\'][a-zA-Z_*?\[\]][a-zA-Z0-9_*?\-,\.\[\]:=]*[\']
> >  name_minus	[a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
> >  drv_cfg_term	[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?

-- 

- Arnaldo

Re: [PATCH] perf parse: Allow names to start with digits

[Jiri Olsa]

On Sat, Jul 02, 2022 at 09:24:24AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sat, Jul 02, 2022 at 06:49:31PM +0900, Dominique Martinet escreveu:
> > Hello,
> > 
> > just making sure my mail didn't get lost -- would anyone have time to
> > look at this?
> > 
> > I don't mind if it's slow or another solution is taken, I'd just like to
> > be able to use 9p probes with perf eventually :)
> 
> Jiri, seems ok, can you please review?

ah right, sorry, I remember looking on that before,
but forgot to respond

> 
> Thanks,
> 
> - Arnaldo
>  
> > Thanks!
> > 
> > Dominique Martinet wrote on Sun, Jun 12, 2022 at 03:15:08PM +0900:
> > > Tracepoints can start with digits, although we don't have many of these:
> > > 
> > > $ rg -g '*.h' '\bTRACE_EVENT\([0-9]'
> > > net/mac802154/trace.h
> > > 53:TRACE_EVENT(802154_drv_return_int,
> > > ...
> > > 
> > > net/ieee802154/trace.h
> > > 66:TRACE_EVENT(802154_rdev_add_virtual_intf,
> > > ...
> > > 
> > > include/trace/events/9p.h
> > > 124:TRACE_EVENT(9p_client_req,
> > > ...
> > > 
> > > Just allow names to start with digits too so e.g. perf probe -e '9p:*'
> > > works
> > > 
> > > Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> > > ---
> > >  tools/perf/util/parse-events.l | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> > > index 5b6e4b5249cf..4133d6950d29 100644
> > > --- a/tools/perf/util/parse-events.l
> > > +++ b/tools/perf/util/parse-events.l
> > > @@ -211,7 +211,7 @@ bpf_source	[^,{}]+\.c[a-zA-Z0-9._]*
> > >  num_dec		[0-9]+
> > >  num_hex		0x[a-fA-F0-9]+
> > >  num_raw_hex	[a-fA-F0-9]+
> > > -name		[a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
> > > +name		[a-zA-Z0-9_*?\[\]][a-zA-Z0-9_*?.\[\]!]*

I thought it'd clash with events like cpu/event=3/,
but lexer check numbers first, so we're fine there

it smells like it could break some events, but I couldn't
find any case of that

could you please at least add tests to tests/parse-events.c
for such case?

thanks,
jirka

> > >  name_tag	[\'][a-zA-Z_*?\[\]][a-zA-Z0-9_*?\-,\.\[\]:=]*[\']
> > >  name_minus	[a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
> > >  drv_cfg_term	[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
> 
> -- 
> 
> - Arnaldo

Re: [PATCH] perf parse: Allow names to start with digits

[Dominique Martinet]

Jiri Olsa wrote on Sat, Jul 02, 2022 at 05:48:03PM +0200:
> > > > -name		[a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
> > > > +name		[a-zA-Z0-9_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
> 
> I thought it'd clash with events like cpu/event=3/,
> but lexer check numbers first, so we're fine there
> 
> it smells like it could break some events, but I couldn't
> find any case of that
> 
> could you please at least add tests to tests/parse-events.c
> for such case?

hmm, I'd love to but on my system I have no events that start with a
number unless I manually modprobe 9pnet (or 802154 stuff), so tests
would start failing for most people and I don't think we want that...

I have confirmed the following 'perf test "event parsing"' works iff
patch is applied and 9pnet loaded:
---- 
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 459afdb256a1..3d069fe9eb79 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -1941,6 +1941,11 @@ static const struct evlist_test test__events[] = {
 		.check = test__exclusive_group,
 		/* 7 */
 	},
+	{
+		.name  = "9p:9p_client_req",
+		.check = test__checkevent_tracepoint,
+		/* 8 */
+	},
 };
 
 static const struct evlist_test test__events_pmu[] = {
----

not sure if there'd be any other way of testing, there's nothing else in
'perf list' that starts with a number.
--
Dominique

Re: [PATCH] perf parse: Allow names to start with digits

[Jiri Olsa]

On Sun, Jul 03, 2022 at 08:51:14AM +0900, Dominique Martinet wrote:
> Jiri Olsa wrote on Sat, Jul 02, 2022 at 05:48:03PM +0200:
> > > > > -name		[a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
> > > > > +name		[a-zA-Z0-9_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
> > 
> > I thought it'd clash with events like cpu/event=3/,
> > but lexer check numbers first, so we're fine there
> > 
> > it smells like it could break some events, but I couldn't
> > find any case of that
> > 
> > could you please at least add tests to tests/parse-events.c
> > for such case?
> 
> hmm, I'd love to but on my system I have no events that start with a
> number unless I manually modprobe 9pnet (or 802154 stuff), so tests
> would start failing for most people and I don't think we want that...
> 
> I have confirmed the following 'perf test "event parsing"' works iff
> patch is applied and 9pnet loaded:
> ---- 
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index 459afdb256a1..3d069fe9eb79 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -1941,6 +1941,11 @@ static const struct evlist_test test__events[] = {
>  		.check = test__exclusive_group,
>  		/* 7 */
>  	},
> +	{
> +		.name  = "9p:9p_client_req",
> +		.check = test__checkevent_tracepoint,
> +		/* 8 */
> +	},
>  };
>  
>  static const struct evlist_test test__events_pmu[] = {
> ----
> 
> not sure if there'd be any other way of testing, there's nothing else in
> 'perf list' that starts with a number.

maybe we could do it same way we did for fake pmu events like below

jirka


---
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 3d069fe9eb79..0dc1fbc80447 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -2058,7 +2058,7 @@ static int test_event(const struct evlist_test *e)
 		return TEST_FAIL;
 	}
 	parse_events_error__init(&err);
-	ret = parse_events(evlist, e->name, &err);
+	ret = __parse_events(evlist, e->name, &err, NULL, true);
 	if (ret) {
 		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
 			 e->name, ret, err.str);
@@ -2087,7 +2087,7 @@ static int test_event_fake_pmu(const char *str)
 
 	parse_events_error__init(&err);
 	perf_pmu__test_parse_init();
-	ret = __parse_events(evlist, str, &err, &perf_pmu__fake);
+	ret = __parse_events(evlist, str, &err, &perf_pmu__fake, false);
 	if (ret) {
 		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
 			 str, ret, err.str);
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index f13368569d8b..27541d4d6025 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -812,7 +812,7 @@ static int check_parse_id(const char *id, struct parse_events_error *error,
 	for (cur = strchr(dup, '@') ; cur; cur = strchr(++cur, '@'))
 		*cur = '/';
 
-	ret = __parse_events(evlist, dup, error, fake_pmu);
+	ret = __parse_events(evlist, dup, error, fake_pmu, false);
 	free(dup);
 
 	evlist__delete(evlist);
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index ce499c5da8d7..73771e22cb39 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -476,7 +476,7 @@ struct evsel *evsel__clone(struct evsel *orig)
 /*
  * Returns pointer with encoded error via <linux/err.h> interface.
  */
-struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx)
+struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx, bool format)
 {
 	struct evsel *evsel = zalloc(perf_evsel__object.size);
 	int err = -ENOMEM;
@@ -493,14 +493,18 @@ struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx)
 		if (asprintf(&evsel->name, "%s:%s", sys, name) < 0)
 			goto out_free;
 
-		evsel->tp_format = trace_event__tp_format(sys, name);
-		if (IS_ERR(evsel->tp_format)) {
-			err = PTR_ERR(evsel->tp_format);
-			goto out_free;
+		if (format) {
+			evsel->tp_format = trace_event__tp_format(sys, name);
+			if (IS_ERR(evsel->tp_format)) {
+				err = PTR_ERR(evsel->tp_format);
+				goto out_free;
+			}
+			attr.config = evsel->tp_format->id;
+		} else {
+			attr.config = (__u64) -1;
 		}
 
 		event_attr_init(&attr);
-		attr.config = evsel->tp_format->id;
 		attr.sample_period = 1;
 		evsel__init(evsel, &attr, idx);
 	}
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 73ea48e94079..255919658b57 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -224,7 +224,7 @@ static inline struct evsel *evsel__new(struct perf_event_attr *attr)
 }
 
 struct evsel *evsel__clone(struct evsel *orig);
-struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx);
+struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx, bool format);
 
 int copy_config_terms(struct list_head *dst, struct list_head *src);
 void free_config_terms(struct list_head *config_terms);
@@ -234,7 +234,7 @@ void free_config_terms(struct list_head *config_terms);
  */
 static inline struct evsel *evsel__newtp(const char *sys, const char *name)
 {
-	return evsel__newtp_idx(sys, name, 0);
+	return evsel__newtp_idx(sys, name, 0, true);
 }
 
 struct evsel *evsel__new_cycles(bool precise, __u32 type, __u64 config);
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 8f7baeabc5cf..8b066a0058f3 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -1417,7 +1417,7 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
 	}
 	pr_debug("Parsing metric events '%s'\n", events.buf);
 	parse_events_error__init(&parse_error);
-	ret = __parse_events(parsed_evlist, events.buf, &parse_error, fake_pmu);
+	ret = __parse_events(parsed_evlist, events.buf, &parse_error, fake_pmu, false);
 	if (ret) {
 		parse_events_error__print(&parse_error, events.buf);
 		goto err_out;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 7ed235740431..914d87fbad5b 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -576,12 +576,14 @@ static void tracepoint_error(struct parse_events_error *e, int err,
 	parse_events_error__handle(e, 0, strdup(str), strdup(help));
 }
 
-static int add_tracepoint(struct list_head *list, int *idx,
+static int add_tracepoint(struct parse_events_state *parse_state,
+			  struct list_head *list,
 			  const char *sys_name, const char *evt_name,
 			  struct parse_events_error *err,
 			  struct list_head *head_config)
 {
-	struct evsel *evsel = evsel__newtp_idx(sys_name, evt_name, (*idx)++);
+	struct evsel *evsel = evsel__newtp_idx(sys_name, evt_name, parse_state->idx++,
+					       !parse_state->fake_tp);
 
 	if (IS_ERR(evsel)) {
 		tracepoint_error(err, PTR_ERR(evsel), sys_name, evt_name);
@@ -600,7 +602,8 @@ static int add_tracepoint(struct list_head *list, int *idx,
 	return 0;
 }
 
-static int add_tracepoint_multi_event(struct list_head *list, int *idx,
+static int add_tracepoint_multi_event(struct parse_events_state *parse_state,
+				      struct list_head *list,
 				      const char *sys_name, const char *evt_name,
 				      struct parse_events_error *err,
 				      struct list_head *head_config)
@@ -634,7 +637,7 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,
 
 		found++;
 
-		ret = add_tracepoint(list, idx, sys_name, evt_ent->d_name,
+		ret = add_tracepoint(parse_state, list, sys_name, evt_ent->d_name,
 				     err, head_config);
 	}
 
@@ -648,19 +651,21 @@ static int add_tracepoint_multi_event(struct list_head *list, int *idx,
 	return ret;
 }
 
-static int add_tracepoint_event(struct list_head *list, int *idx,
+static int add_tracepoint_event(struct parse_events_state *parse_state,
+				struct list_head *list,
 				const char *sys_name, const char *evt_name,
 				struct parse_events_error *err,
 				struct list_head *head_config)
 {
 	return strpbrk(evt_name, "*?") ?
-	       add_tracepoint_multi_event(list, idx, sys_name, evt_name,
+	       add_tracepoint_multi_event(parse_state, list, sys_name, evt_name,
 					  err, head_config) :
-	       add_tracepoint(list, idx, sys_name, evt_name,
+	       add_tracepoint(parse_state, list, sys_name, evt_name,
 			      err, head_config);
 }
 
-static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
+static int add_tracepoint_multi_sys(struct parse_events_state *parse_state,
+				    struct list_head *list,
 				    const char *sys_name, const char *evt_name,
 				    struct parse_events_error *err,
 				    struct list_head *head_config)
@@ -686,7 +691,7 @@ static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
 		if (!strglobmatch(events_ent->d_name, sys_name))
 			continue;
 
-		ret = add_tracepoint_event(list, idx, events_ent->d_name,
+		ret = add_tracepoint_event(parse_state, list, events_ent->d_name,
 					   evt_name, err, head_config);
 	}
 
@@ -723,7 +728,7 @@ static int add_bpf_event(const char *group, const char *event, int fd, struct bp
 	pr_debug("add bpf event %s:%s and attach bpf program %d\n",
 		 group, event, fd);
 
-	err = parse_events_add_tracepoint(&new_evsels, &parse_state->idx, group,
+	err = parse_events_add_tracepoint(parse_state, &new_evsels, group,
 					  event, parse_state->error,
 					  param->head_config);
 	if (err) {
@@ -1420,7 +1425,8 @@ static int get_config_chgs(struct perf_pmu *pmu, struct list_head *head_config,
 	return 0;
 }
 
-int parse_events_add_tracepoint(struct list_head *list, int *idx,
+int parse_events_add_tracepoint(struct parse_events_state *parse_state,
+				struct list_head *list,
 				const char *sys, const char *event,
 				struct parse_events_error *err,
 				struct list_head *head_config)
@@ -1434,10 +1440,10 @@ int parse_events_add_tracepoint(struct list_head *list, int *idx,
 	}
 
 	if (strpbrk(sys, "*?"))
-		return add_tracepoint_multi_sys(list, idx, sys, event,
+		return add_tracepoint_multi_sys(parse_state, list, sys, event,
 						err, head_config);
 	else
-		return add_tracepoint_event(list, idx, sys, event,
+		return add_tracepoint_event(parse_state, list, sys, event,
 					    err, head_config);
 }
 
@@ -2332,7 +2338,8 @@ static int parse_events__with_hybrid_pmu(struct parse_events_state *parse_state,
 }
 
 int __parse_events(struct evlist *evlist, const char *str,
-		   struct parse_events_error *err, struct perf_pmu *fake_pmu)
+		   struct parse_events_error *err,
+		   struct perf_pmu *fake_pmu, bool fake_tp)
 {
 	struct parse_events_state parse_state = {
 		.list	  = LIST_HEAD_INIT(parse_state.list),
@@ -2341,6 +2348,7 @@ int __parse_events(struct evlist *evlist, const char *str,
 		.evlist	  = evlist,
 		.stoken	  = PE_START_EVENTS,
 		.fake_pmu = fake_pmu,
+		.fake_tp  = fake_tp,
 	};
 	int ret;
 
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index a38b8b160e80..bd2fb9a354b4 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -34,12 +34,12 @@ const char *event_type(int type);
 int parse_events_option(const struct option *opt, const char *str, int unset);
 int parse_events_option_new_evlist(const struct option *opt, const char *str, int unset);
 int __parse_events(struct evlist *evlist, const char *str, struct parse_events_error *error,
-		   struct perf_pmu *fake_pmu);
+		   struct perf_pmu *fake_pmu, bool fake_tp);
 
 static inline int parse_events(struct evlist *evlist, const char *str,
 			       struct parse_events_error *err)
 {
-	return __parse_events(evlist, str, err, NULL);
+	return __parse_events(evlist, str, err, NULL, false);
 }
 
 int parse_events_terms(struct list_head *terms, const char *str);
@@ -140,6 +140,7 @@ struct parse_events_state {
 	struct list_head	  *terms;
 	int			   stoken;
 	struct perf_pmu		  *fake_pmu;
+	bool			   fake_tp;
 	char			  *hybrid_pmu_name;
 };
 
@@ -163,7 +164,8 @@ void parse_events__clear_array(struct parse_events_array *a);
 int parse_events__modifier_event(struct list_head *list, char *str, bool add);
 int parse_events__modifier_group(struct list_head *list, char *event_mod);
 int parse_events_name(struct list_head *list, const char *name);
-int parse_events_add_tracepoint(struct list_head *list, int *idx,
+int parse_events_add_tracepoint(struct parse_events_state *parse_state,
+				struct list_head *list,
 				const char *sys, const char *event,
 				struct parse_events_error *error,
 				struct list_head *head_config);
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index be8c51770051..83ccbf433482 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -625,7 +625,7 @@ tracepoint_name opt_event_config
 	if (error)
 		error->idx = @1.first_column;
 
-	err = parse_events_add_tracepoint(list, &parse_state->idx, $1.sys, $1.event,
+	err = parse_events_add_tracepoint(parse_state, list, $1.sys, $1.event,
 					error, $2);
 
 	parse_events_terms__delete($2);

Re: [PATCH] perf parse: Allow names to start with digits

[Dominique Martinet]

Jiri Olsa wrote on Sun, Jul 03, 2022 at 03:46:33PM +0200:
> > not sure if there'd be any other way of testing, there's nothing else in
> > 'perf list' that starts with a number.
> 
> maybe we could do it same way we did for fake pmu events like below

hmm, I'll have to defer to you on that honestly.

It looks good to me though and I've tested your diff (test succeeds even
with 9pnet module unloaded).

Just a note though, this makes test no longer checks the sys event
directories exist for all other tests in these arrays (test__events,
test__events_pmu and test__hybrid_events); if we have guarantees the
probes exist I believe at least a few should keep checking the format
path correctly.
It might be worth adding a check_format flag to test_event() and add a
new list that doesn't check formats just for 9p?

If you're ok with that I can resend this as three patches: my original
patch, a patch with your diff and test_event() keeping current
behaviour, and a last patch adding that last flag and testing 9p without
format check.

(and if you don't think it's worth checking probe existence same thing
but even simpler)
--
Dominique

Re: [PATCH] perf parse: Allow names to start with digits

[Jiri Olsa]

On Mon, Jul 04, 2022 at 08:58:45AM +0900, Dominique Martinet wrote:
> Jiri Olsa wrote on Sun, Jul 03, 2022 at 03:46:33PM +0200:
> > > not sure if there'd be any other way of testing, there's nothing else in
> > > 'perf list' that starts with a number.
> > 
> > maybe we could do it same way we did for fake pmu events like below
> 
> hmm, I'll have to defer to you on that honestly.
> 
> It looks good to me though and I've tested your diff (test succeeds even
> with 9pnet module unloaded).
> 
> Just a note though, this makes test no longer checks the sys event
> directories exist for all other tests in these arrays (test__events,
> test__events_pmu and test__hybrid_events); if we have guarantees the
> probes exist I believe at least a few should keep checking the format
> path correctly.
> It might be worth adding a check_format flag to test_event() and add a
> new list that doesn't check formats just for 9p?

format provides only id which we don't check for tracepoints

> 
> If you're ok with that I can resend this as three patches: my original
> patch, a patch with your diff and test_event() keeping current
> behaviour, and a last patch adding that last flag and testing 9p without
> format check.
> 
> (and if you don't think it's worth checking probe existence same thing
> but even simpler)

I have that patch split into 2 separated changes,
I'll try to send it later today

jirka

Re: [PATCH] perf parse: Allow names to start with digits

[Ian Rogers]

On Sat, Jun 11, 2022 at 11:16 PM Dominique Martinet
<asmadeus@codewreck.org> wrote:
>
> Tracepoints can start with digits, although we don't have many of these:
>
> $ rg -g '*.h' '\bTRACE_EVENT\([0-9]'
> net/mac802154/trace.h
> 53:TRACE_EVENT(802154_drv_return_int,
> ...
>
> net/ieee802154/trace.h
> 66:TRACE_EVENT(802154_rdev_add_virtual_intf,
> ...
>
> include/trace/events/9p.h
> 124:TRACE_EVENT(9p_client_req,
> ...
>
> Just allow names to start with digits too so e.g. perf probe -e '9p:*'
> works
>
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
>  tools/perf/util/parse-events.l | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 5b6e4b5249cf..4133d6950d29 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -211,7 +211,7 @@ bpf_source  [^,{}]+\.c[a-zA-Z0-9._]*
>  num_dec                [0-9]+
>  num_hex                0x[a-fA-F0-9]+
>  num_raw_hex    [a-fA-F0-9]+
> -name           [a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
> +name           [a-zA-Z0-9_*?\[\]][a-zA-Z0-9_*?.\[\]!]*

Perhaps this would be cleaner as:
name           [a-zA-Z0-9_*?\[\]!]+
except that would allow a name to start with an exclamation. Would
that be an issue?

Thanks,
Ian

>  name_tag       [\'][a-zA-Z_*?\[\]][a-zA-Z0-9_*?\-,\.\[\]:=]*[\']
>  name_minus     [a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
>  drv_cfg_term   [a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
> --
> 2.36.1
>

Re: [PATCH] perf parse: Allow names to start with digits

[Dominique Martinet]

Hi Jiri, Ian,

Jiri Olsa wrote on Mon, Jul 04, 2022 at 01:25:28PM +0200:
> > If you're ok with that I can resend this as three patches: my original
> > patch, a patch with your diff and test_event() keeping current
> > behaviour, and a last patch adding that last flag and testing 9p without
> > format check.
> > 
> > (and if you don't think it's worth checking probe existence same thing
> > but even simpler)
> 
> I have that patch split into 2 separated changes,
> I'll try to send it later today

It's been a while (I had totally forgotten), but I don't think I saw
this patch.

For reminder you've requested that I add some test for a tracepoint
starting with digits e.g. 9p:9p_client_res but there's nothing commonly
available to use there, so you added a way to only check without
checking if a backing tracepoint exist, but I see no trace of the update
you sent here:
https://lore.kernel.org/all/YsGduWiTvkM2/tHv@krava/

Should I take it, do the split you suggested and send it together with a
resend of my original patch and new test?


Ian Rogers wrote on Mon, Jul 04, 2022 at 02:39:05PM -0700:
> > diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> > index 5b6e4b5249cf..4133d6950d29 100644
> > --- a/tools/perf/util/parse-events.l
> > +++ b/tools/perf/util/parse-events.l
> > @@ -211,7 +211,7 @@ bpf_source  [^,{}]+\.c[a-zA-Z0-9._]*
> >  num_dec                [0-9]+
> >  num_hex                0x[a-fA-F0-9]+
> >  num_raw_hex    [a-fA-F0-9]+
> > -name           [a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
> > +name           [a-zA-Z0-9_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
> 
> Perhaps this would be cleaner as:
> name           [a-zA-Z0-9_*?\[\]!]+
> except that would allow a name to start with an exclamation. Would
> that be an issue?

Sorry for the lack of reply -- I have no opinion on this as long as we
can use digits.
I can't imagine any probe starting with . or !, but that does not seem
to create any ambiguity with the rest of the grammar that I can see
either so I think it'd be fine, but I'm not comfortable deciding.


Thanks,
--
Dominique

Re: [PATCH] perf parse: Allow names to start with digits

[Jiri Olsa]

On Mon, Oct 10, 2022 at 02:38:54PM +0900, Dominique Martinet wrote:
> Hi Jiri, Ian,
> 
> Jiri Olsa wrote on Mon, Jul 04, 2022 at 01:25:28PM +0200:
> > > If you're ok with that I can resend this as three patches: my original
> > > patch, a patch with your diff and test_event() keeping current
> > > behaviour, and a last patch adding that last flag and testing 9p without
> > > format check.
> > > 
> > > (and if you don't think it's worth checking probe existence same thing
> > > but even simpler)
> > 
> > I have that patch split into 2 separated changes,
> > I'll try to send it later today
> 
> It's been a while (I had totally forgotten), but I don't think I saw
> this patch.
> 
> For reminder you've requested that I add some test for a tracepoint
> starting with digits e.g. 9p:9p_client_res but there's nothing commonly
> available to use there, so you added a way to only check without
> checking if a backing tracepoint exist, but I see no trace of the update
> you sent here:
> https://lore.kernel.org/all/YsGduWiTvkM2/tHv@krava/
> 
> Should I take it, do the split you suggested and send it together with a
> resend of my original patch and new test?

ah, it fell through the cracks, sry.. if you could resend it,
that'd be great

thanks,
jirka

> 
> 
> Ian Rogers wrote on Mon, Jul 04, 2022 at 02:39:05PM -0700:
> > > diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> > > index 5b6e4b5249cf..4133d6950d29 100644
> > > --- a/tools/perf/util/parse-events.l
> > > +++ b/tools/perf/util/parse-events.l
> > > @@ -211,7 +211,7 @@ bpf_source  [^,{}]+\.c[a-zA-Z0-9._]*
> > >  num_dec                [0-9]+
> > >  num_hex                0x[a-fA-F0-9]+
> > >  num_raw_hex    [a-fA-F0-9]+
> > > -name           [a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
> > > +name           [a-zA-Z0-9_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
> > 
> > Perhaps this would be cleaner as:
> > name           [a-zA-Z0-9_*?\[\]!]+
> > except that would allow a name to start with an exclamation. Would
> > that be an issue?
> 
> Sorry for the lack of reply -- I have no opinion on this as long as we
> can use digits.
> I can't imagine any probe starting with . or !, but that does not seem
> to create any ambiguity with the rest of the grammar that I can see
> either so I think it'd be fine, but I'm not comfortable deciding.
> 
> 
> Thanks,
> --
> Dominique