linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] tracing/probes: allow no event name input when create group
@ 2022-05-29  3:34 Linyu Yuan
  2022-05-29  3:34 ` [PATCH v2 1/2] tracing: auto generate event name when create a group of events Linyu Yuan
  2022-05-29  3:34 ` [PATCH v2 2/2] tracing: eprobe: remove duplicate is_good_name() operation Linyu Yuan
  0 siblings, 2 replies; 5+ messages in thread
From: Linyu Yuan @ 2022-05-29  3:34 UTC (permalink / raw)
  To: Steven Rostedt, Masami Hiramatsu, Tom Zanussi, Ingo Molnar
  Cc: linux-kernel, Linyu Yuan

take kprobe event as example, when create a group of events,
p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS],
according to this format, we must input EVENT name,

this change allow only GRP/ input, EVENT name auto generate from KSYM,
p[:[GRP/][EVENT]] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS]

siliar change apply to eprobe and uprobe.

V1: https://lore.kernel.org/lkml/1651397651-30454-1-git-send-email-quic_linyyuan@quicinc.com/T/#m33b0665941d9caeeb118afa84db4016321cea9a6
V2: fix remove comment in V1 patch1,
    remove v1 patch2 as it is NACK.

Linyu Yuan (2):
  tracing: auto generate event name when create a group of events
  tracing: eprobe: remove duplicate is_good_name() operation

 Documentation/trace/kprobetrace.rst  |  8 ++++----
 Documentation/trace/uprobetracer.rst |  8 ++++----
 kernel/trace/trace_dynevent.c        |  2 +-
 kernel/trace/trace_eprobe.c          | 24 ++++++++++++------------
 kernel/trace/trace_kprobe.c          | 19 ++++++++++++-------
 kernel/trace/trace_probe.c           |  9 ++++++++-
 kernel/trace/trace_probe.h           |  4 ++++
 kernel/trace/trace_uprobe.c          | 15 ++++++++++-----
 8 files changed, 55 insertions(+), 34 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 1/2] tracing: auto generate event name when create a group of events
  2022-05-29  3:34 [PATCH v2 0/2] tracing/probes: allow no event name input when create group Linyu Yuan
@ 2022-05-29  3:34 ` Linyu Yuan
  2022-05-30  0:47   ` Masami Hiramatsu
  2022-05-29  3:34 ` [PATCH v2 2/2] tracing: eprobe: remove duplicate is_good_name() operation Linyu Yuan
  1 sibling, 1 reply; 5+ messages in thread
From: Linyu Yuan @ 2022-05-29  3:34 UTC (permalink / raw)
  To: Steven Rostedt, Masami Hiramatsu, Tom Zanussi, Ingo Molnar
  Cc: linux-kernel, Linyu Yuan

Currently when create a specific group of trace events,
take kprobe event as example, user must use the following format:
p:GRP/EVENT [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS],
which means user must enter EVENT name, one example is: echo
'p:usb_gadget/config_usb_cfg_link config_usb_cfg_link $arg1' >>
kprobe_events, it is not simple if there are too many entries
because the event name is same as symbol name.

This change allows user to specify no EVENT name, format changed as:
p:GRP/ [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS],
it will generate event name automatically and one example is:
echo 'p:usb_gadget/ config_usb_cfg_link $arg1' >> kprobe_events.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: fix review comments in V1:
    change TP_ENAME_EMPTTY to TP_ENAME_EMPTY,
    add some space,
    document the macros return by traceprobe_parse_event_name(),
    updatea commit description.

 Documentation/trace/kprobetrace.rst  |  8 ++++----
 Documentation/trace/uprobetracer.rst |  8 ++++----
 kernel/trace/trace_dynevent.c        |  2 +-
 kernel/trace/trace_eprobe.c          | 20 ++++++++++++--------
 kernel/trace/trace_kprobe.c          | 19 ++++++++++++-------
 kernel/trace/trace_probe.c           |  9 ++++++++-
 kernel/trace/trace_probe.h           |  4 ++++
 kernel/trace/trace_uprobe.c          | 15 ++++++++++-----
 8 files changed, 55 insertions(+), 30 deletions(-)

diff --git a/Documentation/trace/kprobetrace.rst b/Documentation/trace/kprobetrace.rst
index b175d88..4274cc6 100644
--- a/Documentation/trace/kprobetrace.rst
+++ b/Documentation/trace/kprobetrace.rst
@@ -28,10 +28,10 @@ Synopsis of kprobe_events
 -------------------------
 ::
 
-  p[:[GRP/]EVENT] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS]	: Set a probe
-  r[MAXACTIVE][:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS]	: Set a return probe
-  p:[GRP/]EVENT] [MOD:]SYM[+0]%return [FETCHARGS]	: Set a return probe
-  -:[GRP/]EVENT						: Clear a probe
+  p[:[GRP/][EVENT]] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS]	: Set a probe
+  r[MAXACTIVE][:[GRP/][EVENT]] [MOD:]SYM[+0] [FETCHARGS]	: Set a return probe
+  p[:[GRP/][EVENT]] [MOD:]SYM[+0]%return [FETCHARGS]	: Set a return probe
+  -:[GRP/][EVENT]						: Clear a probe
 
  GRP		: Group name. If omitted, use "kprobes" for it.
  EVENT		: Event name. If omitted, the event name is generated
diff --git a/Documentation/trace/uprobetracer.rst b/Documentation/trace/uprobetracer.rst
index a8e5938..3a1797d7 100644
--- a/Documentation/trace/uprobetracer.rst
+++ b/Documentation/trace/uprobetracer.rst
@@ -26,10 +26,10 @@ Synopsis of uprobe_tracer
 -------------------------
 ::
 
-  p[:[GRP/]EVENT] PATH:OFFSET [FETCHARGS] : Set a uprobe
-  r[:[GRP/]EVENT] PATH:OFFSET [FETCHARGS] : Set a return uprobe (uretprobe)
-  p[:[GRP/]EVENT] PATH:OFFSET%return [FETCHARGS] : Set a return uprobe (uretprobe)
-  -:[GRP/]EVENT                           : Clear uprobe or uretprobe event
+  p[:[GRP/][EVENT]] PATH:OFFSET [FETCHARGS] : Set a uprobe
+  r[:[GRP/][EVENT]] PATH:OFFSET [FETCHARGS] : Set a return uprobe (uretprobe)
+  p[:[GRP/][EVENT]] PATH:OFFSET%return [FETCHARGS] : Set a return uprobe (uretprobe)
+  -:[GRP/][EVENT]                           : Clear uprobe or uretprobe event
 
   GRP           : Group name. If omitted, "uprobes" is the default value.
   EVENT         : Event name. If omitted, the event name is generated based
diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
index e34e8182..033248d 100644
--- a/kernel/trace/trace_dynevent.c
+++ b/kernel/trace/trace_dynevent.c
@@ -101,7 +101,7 @@ int dyn_event_release(const char *raw_command, struct dyn_event_operations *type
 		event = p + 1;
 		*p = '\0';
 	}
-	if (event[0] == '\0') {
+	if (!system && event[0] == '\0') {
 		ret = -EINVAL;
 		goto out;
 	}
diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 7d44785..13cd7fc 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -125,6 +125,7 @@ static bool eprobe_dyn_event_match(const char *system, const char *event,
 	 * We match the following:
 	 *  event only			- match all eprobes with event name
 	 *  system and event only	- match all system/event probes
+	 *  system only			- match all system probes
 	 *
 	 * The below has the above satisfied with more arguments:
 	 *
@@ -143,7 +144,7 @@ static bool eprobe_dyn_event_match(const char *system, const char *event,
 		return false;
 
 	/* Must match the event name */
-	if (strcmp(trace_probe_name(&ep->tp), event) != 0)
+	if (event[0] != '\0' && strcmp(trace_probe_name(&ep->tp), event) != 0)
 		return false;
 
 	/* No arguments match all */
@@ -848,7 +849,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 {
 	/*
 	 * Argument syntax:
-	 *      e[:[GRP/]ENAME] SYSTEM.EVENT [FETCHARGS]
+	 *      e[:[GRP/][ENAME]] SYSTEM.EVENT [FETCHARGS]
 	 * Fetch args:
 	 *  <name>=$<field>[:TYPE]
 	 */
@@ -858,6 +859,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 	struct trace_eprobe *ep = NULL;
 	char buf1[MAX_EVENT_NAME_LEN];
 	char buf2[MAX_EVENT_NAME_LEN];
+	char grp_buf[MAX_EVENT_NAME_LEN];
 	int ret = 0;
 	int i;
 
@@ -866,14 +868,17 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 
 	trace_probe_log_init("event_probe", argc, argv);
 
+	ret = TP_ENAME_EMPTY;
 	event = strchr(&argv[0][1], ':');
 	if (event) {
 		event++;
-		ret = traceprobe_parse_event_name(&event, &group, buf1,
+		ret = traceprobe_parse_event_name(&event, &group, grp_buf,
 						  event - argv[0]);
-		if (ret)
+		if (ret < 0)
 			goto parse_error;
-	} else {
+	}
+
+	if (ret == TP_ENAME_EMPTY || ret == TP_ENAME_GROUP) {
 		strscpy(buf1, argv[1], MAX_EVENT_NAME_LEN);
 		sanitize_event_name(buf1);
 		event = buf1;
@@ -882,9 +887,8 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 		goto parse_error;
 
 	sys_event = argv[1];
-	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2,
-					  sys_event - argv[1]);
-	if (ret || !sys_name)
+	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
+	if (ret != TP_ENAME_GROUP_EVENT)
 		goto parse_error;
 	if (!is_good_name(sys_event) || !is_good_name(sys_name))
 		goto parse_error;
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 47cebef..55822b6 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -163,7 +163,8 @@ static bool trace_kprobe_match(const char *system, const char *event,
 {
 	struct trace_kprobe *tk = to_trace_kprobe(ev);
 
-	return strcmp(trace_probe_name(&tk->tp), event) == 0 &&
+	return (event[0] == '\0' ||
+		strcmp(trace_probe_name(&tk->tp), event) == 0) &&
 	    (!system || strcmp(trace_probe_group_name(&tk->tp), system) == 0) &&
 	    trace_kprobe_match_command_head(tk, argc, argv);
 }
@@ -708,11 +709,11 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 	/*
 	 * Argument syntax:
 	 *  - Add kprobe:
-	 *      p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS]
+	 *      p[:[GRP/][EVENT]] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS]
 	 *  - Add kretprobe:
-	 *      r[MAXACTIVE][:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS]
+	 *      r[MAXACTIVE][:[GRP/][EVENT]] [MOD:]KSYM[+0] [FETCHARGS]
 	 *    Or
-	 *      p:[GRP/]EVENT] [MOD:]KSYM[+0]%return [FETCHARGS]
+	 *      p[:[GRP/][EVENT]] [MOD:]KSYM[+0]%return [FETCHARGS]
 	 *
 	 * Fetch args:
 	 *  $retval	: fetch return value
@@ -739,6 +740,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 	long offset = 0;
 	void *addr = NULL;
 	char buf[MAX_EVENT_NAME_LEN];
+	char grp_buf[MAX_EVENT_NAME_LEN];
 	unsigned int flags = TPARG_FL_KERNEL;
 
 	switch (argv[0][0]) {
@@ -832,12 +834,15 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 	}
 
 	trace_probe_log_set_index(0);
+	ret = TP_ENAME_EMPTY;
 	if (event) {
-		ret = traceprobe_parse_event_name(&event, &group, buf,
+		ret = traceprobe_parse_event_name(&event, &group, grp_buf,
 						  event - argv[0]);
-		if (ret)
+		if (ret < 0)
 			goto parse_error;
-	} else {
+	}
+
+	if (ret == TP_ENAME_EMPTY || ret == TP_ENAME_GROUP) {
 		/* Make a new event name */
 		if (symbol)
 			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_%s_%ld",
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 80863c6..7fd50ab 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -257,6 +257,9 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
 	}
 	len = strlen(event);
 	if (len == 0) {
+		if (slash)
+			return TP_ENAME_GROUP;
+
 		trace_probe_log_err(offset, NO_EVENT_NAME);
 		return -EINVAL;
 	} else if (len > MAX_EVENT_NAME_LEN) {
@@ -267,7 +270,11 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
 		trace_probe_log_err(offset, BAD_EVENT_NAME);
 		return -EINVAL;
 	}
-	return 0;
+
+	if (slash)
+		return TP_ENAME_GROUP_EVENT;
+
+	return TP_ENAME_EVENT;
 }
 
 #define PARAM_MAX_STACK (THREAD_SIZE / sizeof(unsigned long))
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 92cc149..7390669 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -364,6 +364,10 @@ extern void traceprobe_free_probe_arg(struct probe_arg *arg);
 extern int traceprobe_split_symbol_offset(char *symbol, long *offset);
 int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
 				char *buf, int offset);
+#define TP_ENAME_GROUP_EVENT	0 /* GRP/EVENT format, user defined group and event name */
+#define TP_ENAME_EVENT		1 /* EVENT format, user defined event name, default group */
+#define TP_ENAME_GROUP		2 /* GRP/ format, user defined group name, auto event name */
+#define TP_ENAME_EMPTY		3 /* default group and auto event name */
 
 enum probe_print_type {
 	PROBE_PRINT_NORMAL,
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 9711589..bc32361 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -312,7 +312,8 @@ static bool trace_uprobe_match(const char *system, const char *event,
 {
 	struct trace_uprobe *tu = to_trace_uprobe(ev);
 
-	return strcmp(trace_probe_name(&tu->tp), event) == 0 &&
+	return (event[0] == '\0' ||
+		strcmp(trace_probe_name(&tu->tp), event) == 0) &&
 	   (!system || strcmp(trace_probe_group_name(&tu->tp), system) == 0) &&
 	   trace_uprobe_match_command_head(tu, argc, argv);
 }
@@ -532,7 +533,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
 
 /*
  * Argument syntax:
- *  - Add uprobe: p|r[:[GRP/]EVENT] PATH:OFFSET[%return][(REF)] [FETCHARGS]
+ *  - Add uprobe: p|r[:[GRP/][EVENT]] PATH:OFFSET[%return][(REF)] [FETCHARGS]
  */
 static int __trace_uprobe_create(int argc, const char **argv)
 {
@@ -540,6 +541,7 @@ static int __trace_uprobe_create(int argc, const char **argv)
 	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
 	char *arg, *filename, *rctr, *rctr_end, *tmp;
 	char buf[MAX_EVENT_NAME_LEN];
+	char grp_buf[MAX_EVENT_NAME_LEN];
 	enum probe_print_type ptype;
 	struct path path;
 	unsigned long offset, ref_ctr_offset;
@@ -644,12 +646,15 @@ static int __trace_uprobe_create(int argc, const char **argv)
 
 	/* setup a probe */
 	trace_probe_log_set_index(0);
+	ret = TP_ENAME_EMPTY;
 	if (event) {
-		ret = traceprobe_parse_event_name(&event, &group, buf,
+		ret = traceprobe_parse_event_name(&event, &group, grp_buf,
 						  event - argv[0]);
-		if (ret)
+		if (ret < 0)
 			goto fail_address_parse;
-	} else {
+	}
+
+	if (ret == TP_ENAME_EMPTY || ret == TP_ENAME_GROUP) {
 		char *tail;
 		char *ptr;
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v2 2/2] tracing: eprobe: remove duplicate is_good_name() operation
  2022-05-29  3:34 [PATCH v2 0/2] tracing/probes: allow no event name input when create group Linyu Yuan
  2022-05-29  3:34 ` [PATCH v2 1/2] tracing: auto generate event name when create a group of events Linyu Yuan
@ 2022-05-29  3:34 ` Linyu Yuan
  1 sibling, 0 replies; 5+ messages in thread
From: Linyu Yuan @ 2022-05-29  3:34 UTC (permalink / raw)
  To: Steven Rostedt, Masami Hiramatsu, Tom Zanussi, Ingo Molnar
  Cc: linux-kernel, Linyu Yuan

traceprobe_parse_event_name() already validate group and event name,
there is no need to call is_good_name() after it.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: drop v1 change as it is NACK.
    add it to remove duplicate is_good_name().

 kernel/trace/trace_eprobe.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 13cd7fc..2ee041d 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -883,15 +883,11 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 		sanitize_event_name(buf1);
 		event = buf1;
 	}
-	if (!is_good_name(event) || !is_good_name(group))
-		goto parse_error;
 
 	sys_event = argv[1];
 	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
 	if (ret != TP_ENAME_GROUP_EVENT)
 		goto parse_error;
-	if (!is_good_name(sys_event) || !is_good_name(sys_name))
-		goto parse_error;
 
 	mutex_lock(&event_mutex);
 	event_call = find_and_get_event(sys_name, sys_event);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 1/2] tracing: auto generate event name when create a group of events
  2022-05-29  3:34 ` [PATCH v2 1/2] tracing: auto generate event name when create a group of events Linyu Yuan
@ 2022-05-30  0:47   ` Masami Hiramatsu
  2022-05-30  1:28     ` Linyu Yuan
  0 siblings, 1 reply; 5+ messages in thread
From: Masami Hiramatsu @ 2022-05-30  0:47 UTC (permalink / raw)
  To: Linyu Yuan
  Cc: Steven Rostedt, Tom Zanussi, Ingo Molnar, linux-kernel,
	Tzvetomir Stoyanov

Hi Linyu,

On Sun, 29 May 2022 11:34:53 +0800
Linyu Yuan <quic_linyyuan@quicinc.com> wrote:

> Currently when create a specific group of trace events,
> take kprobe event as example, user must use the following format:
> p:GRP/EVENT [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS],
> which means user must enter EVENT name, one example is: echo
> 'p:usb_gadget/config_usb_cfg_link config_usb_cfg_link $arg1' >>
> kprobe_events, it is not simple if there are too many entries
> because the event name is same as symbol name.
> 
> This change allows user to specify no EVENT name, format changed as:
> p:GRP/ [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS],
> it will generate event name automatically and one example is:
> echo 'p:usb_gadget/ config_usb_cfg_link $arg1' >> kprobe_events.
> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v2: fix review comments in V1:
>     change TP_ENAME_EMPTTY to TP_ENAME_EMPTY,

Thanks for update! And I think this return code is a bit redundant
because those cases can be rephrased as follows;

TP_ENAME_GROUP_EVENT : event != NULL && group != (default)
TP_ENAME_GROUP : event == NULL && group != (default)
TP_ENAME_EVENT : event != NULL && group == (default)
TP_ENAME_EMPTY : event == NULL && group == (default)

What about this (e.g. trace_kprobe.c)?

	if (event) {
		/* event and group will be updated in this function */
		ret = traceprobe_parse_event_name(&event, &group, gbuf,
							event - argv[0]);
		if (ret)
			goto parse_error;
	}

	if (!event) {
		/* Make a new event name */
		if (symbol)
			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_%s_%ld",
				is_return ? 'r' : 'p', symbol, offset);
		else
			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_0x%p",
				is_return ? 'r' : 'p', addr);
		sanitize_event_name(buf);
		event = buf;
	}

This makes the code clearer.

>     add some space,
>     document the macros return by traceprobe_parse_event_name(),
>     updatea commit description.
> 
>  Documentation/trace/kprobetrace.rst  |  8 ++++----
>  Documentation/trace/uprobetracer.rst |  8 ++++----
>  kernel/trace/trace_dynevent.c        |  2 +-
>  kernel/trace/trace_eprobe.c          | 20 ++++++++++++--------
>  kernel/trace/trace_kprobe.c          | 19 ++++++++++++-------
>  kernel/trace/trace_probe.c           |  9 ++++++++-
>  kernel/trace/trace_probe.h           |  4 ++++
>  kernel/trace/trace_uprobe.c          | 15 ++++++++++-----
>  8 files changed, 55 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/trace/kprobetrace.rst b/Documentation/trace/kprobetrace.rst
> index b175d88..4274cc6 100644
> --- a/Documentation/trace/kprobetrace.rst
> +++ b/Documentation/trace/kprobetrace.rst
> @@ -28,10 +28,10 @@ Synopsis of kprobe_events
>  -------------------------
>  ::
>  
> -  p[:[GRP/]EVENT] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS]	: Set a probe
> -  r[MAXACTIVE][:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS]	: Set a return probe
> -  p:[GRP/]EVENT] [MOD:]SYM[+0]%return [FETCHARGS]	: Set a return probe
> -  -:[GRP/]EVENT						: Clear a probe
> +  p[:[GRP/][EVENT]] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS]	: Set a probe
> +  r[MAXACTIVE][:[GRP/][EVENT]] [MOD:]SYM[+0] [FETCHARGS]	: Set a return probe
> +  p[:[GRP/][EVENT]] [MOD:]SYM[+0]%return [FETCHARGS]	: Set a return probe
> +  -:[GRP/][EVENT]						: Clear a probe


Could you also update 'readme_msg' in kernel/trace/trace.c in this patch?
e.g. 

#if defined(CONFIG_KPROBE_EVENTS) || defined(CONFIG_UPROBE_EVENTS)
        "\t  accepts: event-definitions (one definition per line)\n"
        "\t   Format: p[:[<group>/][<event>]] <place> [<args>]\n"
        "\t           r[maxactive][:[<group>/][<event>]] <place> [<args>]\n"
#ifdef CONFIG_HIST_TRIGGERS
        "\t           s:[synthetic/]<event> <field> [<field>]\n"
#endif
        "\t           e[:[<group>/][<event>]] <attached-group>.<attached-event> [<args>]\n"
        "\t           -:[<group>/]<event>\n"


And also, could you add *another patch* to update below testcases about
dynevent to ensure this feature is working? 

tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc
and tools/testing/selftests/ftrace/test.d/dynevent/add_remove_eprobe.tc

e.g. 

if grep -q '\[<event>\]' README then
  (add your test...)
fi


Thank you,


>  
>   GRP		: Group name. If omitted, use "kprobes" for it.
>   EVENT		: Event name. If omitted, the event name is generated
> diff --git a/Documentation/trace/uprobetracer.rst b/Documentation/trace/uprobetracer.rst
> index a8e5938..3a1797d7 100644
> --- a/Documentation/trace/uprobetracer.rst
> +++ b/Documentation/trace/uprobetracer.rst
> @@ -26,10 +26,10 @@ Synopsis of uprobe_tracer
>  -------------------------
>  ::
>  
> -  p[:[GRP/]EVENT] PATH:OFFSET [FETCHARGS] : Set a uprobe
> -  r[:[GRP/]EVENT] PATH:OFFSET [FETCHARGS] : Set a return uprobe (uretprobe)
> -  p[:[GRP/]EVENT] PATH:OFFSET%return [FETCHARGS] : Set a return uprobe (uretprobe)
> -  -:[GRP/]EVENT                           : Clear uprobe or uretprobe event
> +  p[:[GRP/][EVENT]] PATH:OFFSET [FETCHARGS] : Set a uprobe
> +  r[:[GRP/][EVENT]] PATH:OFFSET [FETCHARGS] : Set a return uprobe (uretprobe)
> +  p[:[GRP/][EVENT]] PATH:OFFSET%return [FETCHARGS] : Set a return uprobe (uretprobe)
> +  -:[GRP/][EVENT]                           : Clear uprobe or uretprobe event
>  
>    GRP           : Group name. If omitted, "uprobes" is the default value.
>    EVENT         : Event name. If omitted, the event name is generated based
> diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
> index e34e8182..033248d 100644
> --- a/kernel/trace/trace_dynevent.c
> +++ b/kernel/trace/trace_dynevent.c
> @@ -101,7 +101,7 @@ int dyn_event_release(const char *raw_command, struct dyn_event_operations *type
>  		event = p + 1;
>  		*p = '\0';
>  	}
> -	if (event[0] == '\0') {
> +	if (!system && event[0] == '\0') {
>  		ret = -EINVAL;
>  		goto out;
>  	}
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 7d44785..13cd7fc 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -125,6 +125,7 @@ static bool eprobe_dyn_event_match(const char *system, const char *event,
>  	 * We match the following:
>  	 *  event only			- match all eprobes with event name
>  	 *  system and event only	- match all system/event probes
> +	 *  system only			- match all system probes
>  	 *
>  	 * The below has the above satisfied with more arguments:
>  	 *
> @@ -143,7 +144,7 @@ static bool eprobe_dyn_event_match(const char *system, const char *event,
>  		return false;
>  
>  	/* Must match the event name */
> -	if (strcmp(trace_probe_name(&ep->tp), event) != 0)
> +	if (event[0] != '\0' && strcmp(trace_probe_name(&ep->tp), event) != 0)
>  		return false;
>  
>  	/* No arguments match all */
> @@ -848,7 +849,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>  {
>  	/*
>  	 * Argument syntax:
> -	 *      e[:[GRP/]ENAME] SYSTEM.EVENT [FETCHARGS]
> +	 *      e[:[GRP/][ENAME]] SYSTEM.EVENT [FETCHARGS]
>  	 * Fetch args:
>  	 *  <name>=$<field>[:TYPE]
>  	 */
> @@ -858,6 +859,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>  	struct trace_eprobe *ep = NULL;
>  	char buf1[MAX_EVENT_NAME_LEN];
>  	char buf2[MAX_EVENT_NAME_LEN];
> +	char grp_buf[MAX_EVENT_NAME_LEN];
>  	int ret = 0;
>  	int i;
>  
> @@ -866,14 +868,17 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>  
>  	trace_probe_log_init("event_probe", argc, argv);
>  
> +	ret = TP_ENAME_EMPTY;
>  	event = strchr(&argv[0][1], ':');
>  	if (event) {
>  		event++;
> -		ret = traceprobe_parse_event_name(&event, &group, buf1,
> +		ret = traceprobe_parse_event_name(&event, &group, grp_buf,
>  						  event - argv[0]);
> -		if (ret)
> +		if (ret < 0)
>  			goto parse_error;
> -	} else {
> +	}
> +
> +	if (ret == TP_ENAME_EMPTY || ret == TP_ENAME_GROUP) {
>  		strscpy(buf1, argv[1], MAX_EVENT_NAME_LEN);
>  		sanitize_event_name(buf1);
>  		event = buf1;
> @@ -882,9 +887,8 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>  		goto parse_error;
>  
>  	sys_event = argv[1];
> -	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2,
> -					  sys_event - argv[1]);
> -	if (ret || !sys_name)
> +	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
> +	if (ret != TP_ENAME_GROUP_EVENT)
>  		goto parse_error;
>  	if (!is_good_name(sys_event) || !is_good_name(sys_name))
>  		goto parse_error;
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 47cebef..55822b6 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -163,7 +163,8 @@ static bool trace_kprobe_match(const char *system, const char *event,
>  {
>  	struct trace_kprobe *tk = to_trace_kprobe(ev);
>  
> -	return strcmp(trace_probe_name(&tk->tp), event) == 0 &&
> +	return (event[0] == '\0' ||
> +		strcmp(trace_probe_name(&tk->tp), event) == 0) &&
>  	    (!system || strcmp(trace_probe_group_name(&tk->tp), system) == 0) &&
>  	    trace_kprobe_match_command_head(tk, argc, argv);
>  }
> @@ -708,11 +709,11 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>  	/*
>  	 * Argument syntax:
>  	 *  - Add kprobe:
> -	 *      p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS]
> +	 *      p[:[GRP/][EVENT]] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS]
>  	 *  - Add kretprobe:
> -	 *      r[MAXACTIVE][:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS]
> +	 *      r[MAXACTIVE][:[GRP/][EVENT]] [MOD:]KSYM[+0] [FETCHARGS]
>  	 *    Or
> -	 *      p:[GRP/]EVENT] [MOD:]KSYM[+0]%return [FETCHARGS]
> +	 *      p[:[GRP/][EVENT]] [MOD:]KSYM[+0]%return [FETCHARGS]
>  	 *
>  	 * Fetch args:
>  	 *  $retval	: fetch return value
> @@ -739,6 +740,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>  	long offset = 0;
>  	void *addr = NULL;
>  	char buf[MAX_EVENT_NAME_LEN];
> +	char grp_buf[MAX_EVENT_NAME_LEN];
>  	unsigned int flags = TPARG_FL_KERNEL;
>  
>  	switch (argv[0][0]) {
> @@ -832,12 +834,15 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>  	}
>  
>  	trace_probe_log_set_index(0);
> +	ret = TP_ENAME_EMPTY;
>  	if (event) {
> -		ret = traceprobe_parse_event_name(&event, &group, buf,
> +		ret = traceprobe_parse_event_name(&event, &group, grp_buf,
>  						  event - argv[0]);
> -		if (ret)
> +		if (ret < 0)
>  			goto parse_error;
> -	} else {
> +	}
> +
> +	if (ret == TP_ENAME_EMPTY || ret == TP_ENAME_GROUP) {
>  		/* Make a new event name */
>  		if (symbol)
>  			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_%s_%ld",
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 80863c6..7fd50ab 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -257,6 +257,9 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
>  	}
>  	len = strlen(event);
>  	if (len == 0) {
> +		if (slash)
> +			return TP_ENAME_GROUP;
> +
>  		trace_probe_log_err(offset, NO_EVENT_NAME);
>  		return -EINVAL;
>  	} else if (len > MAX_EVENT_NAME_LEN) {
> @@ -267,7 +270,11 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
>  		trace_probe_log_err(offset, BAD_EVENT_NAME);
>  		return -EINVAL;
>  	}
> -	return 0;
> +
> +	if (slash)
> +		return TP_ENAME_GROUP_EVENT;
> +
> +	return TP_ENAME_EVENT;
>  }
>  
>  #define PARAM_MAX_STACK (THREAD_SIZE / sizeof(unsigned long))
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 92cc149..7390669 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -364,6 +364,10 @@ extern void traceprobe_free_probe_arg(struct probe_arg *arg);
>  extern int traceprobe_split_symbol_offset(char *symbol, long *offset);
>  int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
>  				char *buf, int offset);
> +#define TP_ENAME_GROUP_EVENT	0 /* GRP/EVENT format, user defined group and event name */
> +#define TP_ENAME_EVENT		1 /* EVENT format, user defined event name, default group */
> +#define TP_ENAME_GROUP		2 /* GRP/ format, user defined group name, auto event name */
> +#define TP_ENAME_EMPTY		3 /* default group and auto event name */
>  
>  enum probe_print_type {
>  	PROBE_PRINT_NORMAL,
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 9711589..bc32361 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -312,7 +312,8 @@ static bool trace_uprobe_match(const char *system, const char *event,
>  {
>  	struct trace_uprobe *tu = to_trace_uprobe(ev);
>  
> -	return strcmp(trace_probe_name(&tu->tp), event) == 0 &&
> +	return (event[0] == '\0' ||
> +		strcmp(trace_probe_name(&tu->tp), event) == 0) &&
>  	   (!system || strcmp(trace_probe_group_name(&tu->tp), system) == 0) &&
>  	   trace_uprobe_match_command_head(tu, argc, argv);
>  }
> @@ -532,7 +533,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
>  
>  /*
>   * Argument syntax:
> - *  - Add uprobe: p|r[:[GRP/]EVENT] PATH:OFFSET[%return][(REF)] [FETCHARGS]
> + *  - Add uprobe: p|r[:[GRP/][EVENT]] PATH:OFFSET[%return][(REF)] [FETCHARGS]
>   */
>  static int __trace_uprobe_create(int argc, const char **argv)
>  {
> @@ -540,6 +541,7 @@ static int __trace_uprobe_create(int argc, const char **argv)
>  	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
>  	char *arg, *filename, *rctr, *rctr_end, *tmp;
>  	char buf[MAX_EVENT_NAME_LEN];
> +	char grp_buf[MAX_EVENT_NAME_LEN];
>  	enum probe_print_type ptype;
>  	struct path path;
>  	unsigned long offset, ref_ctr_offset;
> @@ -644,12 +646,15 @@ static int __trace_uprobe_create(int argc, const char **argv)
>  
>  	/* setup a probe */
>  	trace_probe_log_set_index(0);
> +	ret = TP_ENAME_EMPTY;
>  	if (event) {
> -		ret = traceprobe_parse_event_name(&event, &group, buf,
> +		ret = traceprobe_parse_event_name(&event, &group, grp_buf,
>  						  event - argv[0]);
> -		if (ret)
> +		if (ret < 0)
>  			goto fail_address_parse;
> -	} else {
> +	}
> +
> +	if (ret == TP_ENAME_EMPTY || ret == TP_ENAME_GROUP) {
>  		char *tail;
>  		char *ptr;
>  
> -- 
> 2.7.4
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 1/2] tracing: auto generate event name when create a group of events
  2022-05-30  0:47   ` Masami Hiramatsu
@ 2022-05-30  1:28     ` Linyu Yuan
  0 siblings, 0 replies; 5+ messages in thread
From: Linyu Yuan @ 2022-05-30  1:28 UTC (permalink / raw)
  To: Masami Hiramatsu (Google)
  Cc: Steven Rostedt, Tom Zanussi, Ingo Molnar, linux-kernel,
	Tzvetomir Stoyanov (VMware)


On 5/30/2022 8:47 AM, Masami Hiramatsu (Google) wrote:
> Hi Linyu,
>
> On Sun, 29 May 2022 11:34:53 +0800
> Linyu Yuan <quic_linyyuan@quicinc.com> wrote:
>
>> Currently when create a specific group of trace events,
>> take kprobe event as example, user must use the following format:
>> p:GRP/EVENT [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS],
>> which means user must enter EVENT name, one example is: echo
>> 'p:usb_gadget/config_usb_cfg_link config_usb_cfg_link $arg1' >>
>> kprobe_events, it is not simple if there are too many entries
>> because the event name is same as symbol name.
>>
>> This change allows user to specify no EVENT name, format changed as:
>> p:GRP/ [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS],
>> it will generate event name automatically and one example is:
>> echo 'p:usb_gadget/ config_usb_cfg_link $arg1' >> kprobe_events.
>>
>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>> ---
>> v2: fix review comments in V1:
>>      change TP_ENAME_EMPTTY to TP_ENAME_EMPTY,
> Thanks for update! And I think this return code is a bit redundant
> because those cases can be rephrased as follows;
>
> TP_ENAME_GROUP_EVENT : event != NULL && group != (default)
> TP_ENAME_GROUP : event == NULL && group != (default)
> TP_ENAME_EVENT : event != NULL && group == (default)
> TP_ENAME_EMPTY : event == NULL && group == (default)
>
> What about this (e.g. trace_kprobe.c)?
>
> 	if (event) {
> 		/* event and group will be updated in this function */
> 		ret = traceprobe_parse_event_name(&event, &group, gbuf,
> 							event - argv[0]);
> 		if (ret)
> 			goto parse_error;
> 	}
>
> 	if (!event) {
> 		/* Make a new event name */
> 		if (symbol)
> 			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_%s_%ld",
> 				is_return ? 'r' : 'p', symbol, offset);
> 		else
> 			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_0x%p",
> 				is_return ? 'r' : 'p', addr);
> 		sanitize_event_name(buf);
> 		event = buf;
> 	}
>
> This makes the code clearer.
good idea, will follow it.
>
>>      add some space,
>>      document the macros return by traceprobe_parse_event_name(),
>>      updatea commit description.
>>
>>   Documentation/trace/kprobetrace.rst  |  8 ++++----
>>   Documentation/trace/uprobetracer.rst |  8 ++++----
>>   kernel/trace/trace_dynevent.c        |  2 +-
>>   kernel/trace/trace_eprobe.c          | 20 ++++++++++++--------
>>   kernel/trace/trace_kprobe.c          | 19 ++++++++++++-------
>>   kernel/trace/trace_probe.c           |  9 ++++++++-
>>   kernel/trace/trace_probe.h           |  4 ++++
>>   kernel/trace/trace_uprobe.c          | 15 ++++++++++-----
>>   8 files changed, 55 insertions(+), 30 deletions(-)
>>
>> diff --git a/Documentation/trace/kprobetrace.rst b/Documentation/trace/kprobetrace.rst
>> index b175d88..4274cc6 100644
>> --- a/Documentation/trace/kprobetrace.rst
>> +++ b/Documentation/trace/kprobetrace.rst
>> @@ -28,10 +28,10 @@ Synopsis of kprobe_events
>>   -------------------------
>>   ::
>>   
>> -  p[:[GRP/]EVENT] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS]	: Set a probe
>> -  r[MAXACTIVE][:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS]	: Set a return probe
>> -  p:[GRP/]EVENT] [MOD:]SYM[+0]%return [FETCHARGS]	: Set a return probe
>> -  -:[GRP/]EVENT						: Clear a probe
>> +  p[:[GRP/][EVENT]] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS]	: Set a probe
>> +  r[MAXACTIVE][:[GRP/][EVENT]] [MOD:]SYM[+0] [FETCHARGS]	: Set a return probe
>> +  p[:[GRP/][EVENT]] [MOD:]SYM[+0]%return [FETCHARGS]	: Set a return probe
>> +  -:[GRP/][EVENT]						: Clear a probe
>
> Could you also update 'readme_msg' in kernel/trace/trace.c in this patch?
> e.g.
>
> #if defined(CONFIG_KPROBE_EVENTS) || defined(CONFIG_UPROBE_EVENTS)
>          "\t  accepts: event-definitions (one definition per line)\n"
>          "\t   Format: p[:[<group>/][<event>]] <place> [<args>]\n"
>          "\t           r[maxactive][:[<group>/][<event>]] <place> [<args>]\n"
> #ifdef CONFIG_HIST_TRIGGERS
>          "\t           s:[synthetic/]<event> <field> [<field>]\n"
> #endif
>          "\t           e[:[<group>/][<event>]] <attached-group>.<attached-event> [<args>]\n"
>          "\t           -:[<group>/]<event>\n"
sure.
>
> And also, could you add *another patch* to update below testcases about
> dynevent to ensure this feature is working?
>
> tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc
> and tools/testing/selftests/ftrace/test.d/dynevent/add_remove_eprobe.tc
>
> e.g.
>
> if grep -q '\[<event>\]' README then
>    (add your test...)
> fi
thanks, will do it.
>
>
> Thank you,
>
>
>>   
>>    GRP		: Group name. If omitted, use "kprobes" for it.
>>    EVENT		: Event name. If omitted, the event name is generated
>> diff --git a/Documentation/trace/uprobetracer.rst b/Documentation/trace/uprobetracer.rst
>> index a8e5938..3a1797d7 100644
>> --- a/Documentation/trace/uprobetracer.rst
>> +++ b/Documentation/trace/uprobetracer.rst
>> @@ -26,10 +26,10 @@ Synopsis of uprobe_tracer
>>   -------------------------
>>   ::
>>   
>> -  p[:[GRP/]EVENT] PATH:OFFSET [FETCHARGS] : Set a uprobe
>> -  r[:[GRP/]EVENT] PATH:OFFSET [FETCHARGS] : Set a return uprobe (uretprobe)
>> -  p[:[GRP/]EVENT] PATH:OFFSET%return [FETCHARGS] : Set a return uprobe (uretprobe)
>> -  -:[GRP/]EVENT                           : Clear uprobe or uretprobe event
>> +  p[:[GRP/][EVENT]] PATH:OFFSET [FETCHARGS] : Set a uprobe
>> +  r[:[GRP/][EVENT]] PATH:OFFSET [FETCHARGS] : Set a return uprobe (uretprobe)
>> +  p[:[GRP/][EVENT]] PATH:OFFSET%return [FETCHARGS] : Set a return uprobe (uretprobe)
>> +  -:[GRP/][EVENT]                           : Clear uprobe or uretprobe event
>>   
>>     GRP           : Group name. If omitted, "uprobes" is the default value.
>>     EVENT         : Event name. If omitted, the event name is generated based
>> diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
>> index e34e8182..033248d 100644
>> --- a/kernel/trace/trace_dynevent.c
>> +++ b/kernel/trace/trace_dynevent.c
>> @@ -101,7 +101,7 @@ int dyn_event_release(const char *raw_command, struct dyn_event_operations *type
>>   		event = p + 1;
>>   		*p = '\0';
>>   	}
>> -	if (event[0] == '\0') {
>> +	if (!system && event[0] == '\0') {
>>   		ret = -EINVAL;
>>   		goto out;
>>   	}
>> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
>> index 7d44785..13cd7fc 100644
>> --- a/kernel/trace/trace_eprobe.c
>> +++ b/kernel/trace/trace_eprobe.c
>> @@ -125,6 +125,7 @@ static bool eprobe_dyn_event_match(const char *system, const char *event,
>>   	 * We match the following:
>>   	 *  event only			- match all eprobes with event name
>>   	 *  system and event only	- match all system/event probes
>> +	 *  system only			- match all system probes
>>   	 *
>>   	 * The below has the above satisfied with more arguments:
>>   	 *
>> @@ -143,7 +144,7 @@ static bool eprobe_dyn_event_match(const char *system, const char *event,
>>   		return false;
>>   
>>   	/* Must match the event name */
>> -	if (strcmp(trace_probe_name(&ep->tp), event) != 0)
>> +	if (event[0] != '\0' && strcmp(trace_probe_name(&ep->tp), event) != 0)
>>   		return false;
>>   
>>   	/* No arguments match all */
>> @@ -848,7 +849,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>>   {
>>   	/*
>>   	 * Argument syntax:
>> -	 *      e[:[GRP/]ENAME] SYSTEM.EVENT [FETCHARGS]
>> +	 *      e[:[GRP/][ENAME]] SYSTEM.EVENT [FETCHARGS]
>>   	 * Fetch args:
>>   	 *  <name>=$<field>[:TYPE]
>>   	 */
>> @@ -858,6 +859,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>>   	struct trace_eprobe *ep = NULL;
>>   	char buf1[MAX_EVENT_NAME_LEN];
>>   	char buf2[MAX_EVENT_NAME_LEN];
>> +	char grp_buf[MAX_EVENT_NAME_LEN];
>>   	int ret = 0;
>>   	int i;
>>   
>> @@ -866,14 +868,17 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>>   
>>   	trace_probe_log_init("event_probe", argc, argv);
>>   
>> +	ret = TP_ENAME_EMPTY;
>>   	event = strchr(&argv[0][1], ':');
>>   	if (event) {
>>   		event++;
>> -		ret = traceprobe_parse_event_name(&event, &group, buf1,
>> +		ret = traceprobe_parse_event_name(&event, &group, grp_buf,
>>   						  event - argv[0]);
>> -		if (ret)
>> +		if (ret < 0)
>>   			goto parse_error;
>> -	} else {
>> +	}
>> +
>> +	if (ret == TP_ENAME_EMPTY || ret == TP_ENAME_GROUP) {
>>   		strscpy(buf1, argv[1], MAX_EVENT_NAME_LEN);
>>   		sanitize_event_name(buf1);
>>   		event = buf1;
>> @@ -882,9 +887,8 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>>   		goto parse_error;
>>   
>>   	sys_event = argv[1];
>> -	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2,
>> -					  sys_event - argv[1]);
>> -	if (ret || !sys_name)
>> +	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
>> +	if (ret != TP_ENAME_GROUP_EVENT)
>>   		goto parse_error;
>>   	if (!is_good_name(sys_event) || !is_good_name(sys_name))
>>   		goto parse_error;
>> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
>> index 47cebef..55822b6 100644
>> --- a/kernel/trace/trace_kprobe.c
>> +++ b/kernel/trace/trace_kprobe.c
>> @@ -163,7 +163,8 @@ static bool trace_kprobe_match(const char *system, const char *event,
>>   {
>>   	struct trace_kprobe *tk = to_trace_kprobe(ev);
>>   
>> -	return strcmp(trace_probe_name(&tk->tp), event) == 0 &&
>> +	return (event[0] == '\0' ||
>> +		strcmp(trace_probe_name(&tk->tp), event) == 0) &&
>>   	    (!system || strcmp(trace_probe_group_name(&tk->tp), system) == 0) &&
>>   	    trace_kprobe_match_command_head(tk, argc, argv);
>>   }
>> @@ -708,11 +709,11 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>>   	/*
>>   	 * Argument syntax:
>>   	 *  - Add kprobe:
>> -	 *      p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS]
>> +	 *      p[:[GRP/][EVENT]] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS]
>>   	 *  - Add kretprobe:
>> -	 *      r[MAXACTIVE][:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS]
>> +	 *      r[MAXACTIVE][:[GRP/][EVENT]] [MOD:]KSYM[+0] [FETCHARGS]
>>   	 *    Or
>> -	 *      p:[GRP/]EVENT] [MOD:]KSYM[+0]%return [FETCHARGS]
>> +	 *      p[:[GRP/][EVENT]] [MOD:]KSYM[+0]%return [FETCHARGS]
>>   	 *
>>   	 * Fetch args:
>>   	 *  $retval	: fetch return value
>> @@ -739,6 +740,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>>   	long offset = 0;
>>   	void *addr = NULL;
>>   	char buf[MAX_EVENT_NAME_LEN];
>> +	char grp_buf[MAX_EVENT_NAME_LEN];
>>   	unsigned int flags = TPARG_FL_KERNEL;
>>   
>>   	switch (argv[0][0]) {
>> @@ -832,12 +834,15 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>>   	}
>>   
>>   	trace_probe_log_set_index(0);
>> +	ret = TP_ENAME_EMPTY;
>>   	if (event) {
>> -		ret = traceprobe_parse_event_name(&event, &group, buf,
>> +		ret = traceprobe_parse_event_name(&event, &group, grp_buf,
>>   						  event - argv[0]);
>> -		if (ret)
>> +		if (ret < 0)
>>   			goto parse_error;
>> -	} else {
>> +	}
>> +
>> +	if (ret == TP_ENAME_EMPTY || ret == TP_ENAME_GROUP) {
>>   		/* Make a new event name */
>>   		if (symbol)
>>   			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_%s_%ld",
>> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
>> index 80863c6..7fd50ab 100644
>> --- a/kernel/trace/trace_probe.c
>> +++ b/kernel/trace/trace_probe.c
>> @@ -257,6 +257,9 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
>>   	}
>>   	len = strlen(event);
>>   	if (len == 0) {
>> +		if (slash)
>> +			return TP_ENAME_GROUP;
>> +
>>   		trace_probe_log_err(offset, NO_EVENT_NAME);
>>   		return -EINVAL;
>>   	} else if (len > MAX_EVENT_NAME_LEN) {
>> @@ -267,7 +270,11 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
>>   		trace_probe_log_err(offset, BAD_EVENT_NAME);
>>   		return -EINVAL;
>>   	}
>> -	return 0;
>> +
>> +	if (slash)
>> +		return TP_ENAME_GROUP_EVENT;
>> +
>> +	return TP_ENAME_EVENT;
>>   }
>>   
>>   #define PARAM_MAX_STACK (THREAD_SIZE / sizeof(unsigned long))
>> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
>> index 92cc149..7390669 100644
>> --- a/kernel/trace/trace_probe.h
>> +++ b/kernel/trace/trace_probe.h
>> @@ -364,6 +364,10 @@ extern void traceprobe_free_probe_arg(struct probe_arg *arg);
>>   extern int traceprobe_split_symbol_offset(char *symbol, long *offset);
>>   int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
>>   				char *buf, int offset);
>> +#define TP_ENAME_GROUP_EVENT	0 /* GRP/EVENT format, user defined group and event name */
>> +#define TP_ENAME_EVENT		1 /* EVENT format, user defined event name, default group */
>> +#define TP_ENAME_GROUP		2 /* GRP/ format, user defined group name, auto event name */
>> +#define TP_ENAME_EMPTY		3 /* default group and auto event name */
>>   
>>   enum probe_print_type {
>>   	PROBE_PRINT_NORMAL,
>> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
>> index 9711589..bc32361 100644
>> --- a/kernel/trace/trace_uprobe.c
>> +++ b/kernel/trace/trace_uprobe.c
>> @@ -312,7 +312,8 @@ static bool trace_uprobe_match(const char *system, const char *event,
>>   {
>>   	struct trace_uprobe *tu = to_trace_uprobe(ev);
>>   
>> -	return strcmp(trace_probe_name(&tu->tp), event) == 0 &&
>> +	return (event[0] == '\0' ||
>> +		strcmp(trace_probe_name(&tu->tp), event) == 0) &&
>>   	   (!system || strcmp(trace_probe_group_name(&tu->tp), system) == 0) &&
>>   	   trace_uprobe_match_command_head(tu, argc, argv);
>>   }
>> @@ -532,7 +533,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
>>   
>>   /*
>>    * Argument syntax:
>> - *  - Add uprobe: p|r[:[GRP/]EVENT] PATH:OFFSET[%return][(REF)] [FETCHARGS]
>> + *  - Add uprobe: p|r[:[GRP/][EVENT]] PATH:OFFSET[%return][(REF)] [FETCHARGS]
>>    */
>>   static int __trace_uprobe_create(int argc, const char **argv)
>>   {
>> @@ -540,6 +541,7 @@ static int __trace_uprobe_create(int argc, const char **argv)
>>   	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
>>   	char *arg, *filename, *rctr, *rctr_end, *tmp;
>>   	char buf[MAX_EVENT_NAME_LEN];
>> +	char grp_buf[MAX_EVENT_NAME_LEN];
>>   	enum probe_print_type ptype;
>>   	struct path path;
>>   	unsigned long offset, ref_ctr_offset;
>> @@ -644,12 +646,15 @@ static int __trace_uprobe_create(int argc, const char **argv)
>>   
>>   	/* setup a probe */
>>   	trace_probe_log_set_index(0);
>> +	ret = TP_ENAME_EMPTY;
>>   	if (event) {
>> -		ret = traceprobe_parse_event_name(&event, &group, buf,
>> +		ret = traceprobe_parse_event_name(&event, &group, grp_buf,
>>   						  event - argv[0]);
>> -		if (ret)
>> +		if (ret < 0)
>>   			goto fail_address_parse;
>> -	} else {
>> +	}
>> +
>> +	if (ret == TP_ENAME_EMPTY || ret == TP_ENAME_GROUP) {
>>   		char *tail;
>>   		char *ptr;
>>   
>> -- 
>> 2.7.4
>>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-05-30  1:28 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-29  3:34 [PATCH v2 0/2] tracing/probes: allow no event name input when create group Linyu Yuan
2022-05-29  3:34 ` [PATCH v2 1/2] tracing: auto generate event name when create a group of events Linyu Yuan
2022-05-30  0:47   ` Masami Hiramatsu
2022-05-30  1:28     ` Linyu Yuan
2022-05-29  3:34 ` [PATCH v2 2/2] tracing: eprobe: remove duplicate is_good_name() operation Linyu Yuan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).