[PATCH ftrace/for-next ] tracing/kprobes: Replace startup test with selftest script

[PATCH ftrace/for-next ] tracing/kprobes: Replace startup test with selftest script

[Masami Hiramatsu]

Replace the kprobe_tracer's startup test with two selftest scripts.
These test cases are testing that the kprobe_event can accept a
kprobe event with $stack related arguments and a kretprobe event
with $retval argument.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 kernel/trace/trace_kprobe.c                        |  142 --------------------
 .../selftests/ftrace/test.d/kprobe/kprobe_args.tc  |   16 ++
 .../ftrace/test.d/kprobe/kretprobe_args.tc         |   15 ++
 3 files changed, 31 insertions(+), 142 deletions(-)
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args.tc
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_args.tc

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 282f6e4..1018f77 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1352,145 +1352,3 @@ static __init int init_kprobe_trace(void)
 }
 fs_initcall(init_kprobe_trace);
 
-
-#ifdef CONFIG_FTRACE_STARTUP_TEST
-
-/*
- * The "__used" keeps gcc from removing the function symbol
- * from the kallsyms table.
- */
-static __used int kprobe_trace_selftest_target(int a1, int a2, int a3,
-					       int a4, int a5, int a6)
-{
-	return a1 + a2 + a3 + a4 + a5 + a6;
-}
-
-static struct ftrace_event_file *
-find_trace_probe_file(struct trace_kprobe *tk, struct trace_array *tr)
-{
-	struct ftrace_event_file *file;
-
-	list_for_each_entry(file, &tr->events, list)
-		if (file->event_call == &tk->tp.call)
-			return file;
-
-	return NULL;
-}
-
-/*
- * Nobody but us can call enable_trace_kprobe/disable_trace_kprobe at this
- * stage, we can do this lockless.
- */
-static __init int kprobe_trace_self_tests_init(void)
-{
-	int ret, warn = 0;
-	int (*target)(int, int, int, int, int, int);
-	struct trace_kprobe *tk;
-	struct ftrace_event_file *file;
-
-	if (tracing_is_disabled())
-		return -ENODEV;
-
-	target = kprobe_trace_selftest_target;
-
-	pr_info("Testing kprobe tracing: ");
-
-	ret = traceprobe_command("p:testprobe kprobe_trace_selftest_target "
-				  "$stack $stack0 +0($stack)",
-				  create_trace_kprobe);
-	if (WARN_ON_ONCE(ret)) {
-		pr_warn("error on probing function entry.\n");
-		warn++;
-	} else {
-		/* Enable trace point */
-		tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
-		if (WARN_ON_ONCE(tk == NULL)) {
-			pr_warn("error on getting new probe.\n");
-			warn++;
-		} else {
-			file = find_trace_probe_file(tk, top_trace_array());
-			if (WARN_ON_ONCE(file == NULL)) {
-				pr_warn("error on getting probe file.\n");
-				warn++;
-			} else
-				enable_trace_kprobe(tk, file);
-		}
-	}
-
-	ret = traceprobe_command("r:testprobe2 kprobe_trace_selftest_target "
-				  "$retval", create_trace_kprobe);
-	if (WARN_ON_ONCE(ret)) {
-		pr_warn("error on probing function return.\n");
-		warn++;
-	} else {
-		/* Enable trace point */
-		tk = find_trace_kprobe("testprobe2", KPROBE_EVENT_SYSTEM);
-		if (WARN_ON_ONCE(tk == NULL)) {
-			pr_warn("error on getting 2nd new probe.\n");
-			warn++;
-		} else {
-			file = find_trace_probe_file(tk, top_trace_array());
-			if (WARN_ON_ONCE(file == NULL)) {
-				pr_warn("error on getting probe file.\n");
-				warn++;
-			} else
-				enable_trace_kprobe(tk, file);
-		}
-	}
-
-	if (warn)
-		goto end;
-
-	ret = target(1, 2, 3, 4, 5, 6);
-
-	/* Disable trace points before removing it */
-	tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
-	if (WARN_ON_ONCE(tk == NULL)) {
-		pr_warn("error on getting test probe.\n");
-		warn++;
-	} else {
-		file = find_trace_probe_file(tk, top_trace_array());
-		if (WARN_ON_ONCE(file == NULL)) {
-			pr_warn("error on getting probe file.\n");
-			warn++;
-		} else
-			disable_trace_kprobe(tk, file);
-	}
-
-	tk = find_trace_kprobe("testprobe2", KPROBE_EVENT_SYSTEM);
-	if (WARN_ON_ONCE(tk == NULL)) {
-		pr_warn("error on getting 2nd test probe.\n");
-		warn++;
-	} else {
-		file = find_trace_probe_file(tk, top_trace_array());
-		if (WARN_ON_ONCE(file == NULL)) {
-			pr_warn("error on getting probe file.\n");
-			warn++;
-		} else
-			disable_trace_kprobe(tk, file);
-	}
-
-	ret = traceprobe_command("-:testprobe", create_trace_kprobe);
-	if (WARN_ON_ONCE(ret)) {
-		pr_warn("error on deleting a probe.\n");
-		warn++;
-	}
-
-	ret = traceprobe_command("-:testprobe2", create_trace_kprobe);
-	if (WARN_ON_ONCE(ret)) {
-		pr_warn("error on deleting a probe.\n");
-		warn++;
-	}
-
-end:
-	release_all_trace_kprobes();
-	if (warn)
-		pr_cont("NG: Some tests are failed. Please check them.\n");
-	else
-		pr_cont("OK\n");
-	return 0;
-}
-
-late_initcall(kprobe_trace_self_tests_init);
-
-#endif
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args.tc
new file mode 100644
index 0000000..a603d3f
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args.tc
@@ -0,0 +1,16 @@
+#!/bin/sh
+# description: Kprobe dynamic event with arguments
+
+[ -f kprobe_events ] || exit_unsupported # this is configurable
+
+echo 0 > events/enable
+echo > kprobe_events
+echo 'p:testprobe do_fork $stack $stack0 +0($stack)' > kprobe_events
+grep testprobe kprobe_events
+test -d events/kprobes/testprobe
+echo 1 > events/kprobes/testprobe/enable
+( echo "forked")
+echo 0 > events/kprobes/testprobe/enable
+echo "-:testprobe" >> kprobe_events
+test -d events/kprobes/testprobe && exit 1 || exit 0
+
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_args.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_args.tc
new file mode 100644
index 0000000..283c29e
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_args.tc
@@ -0,0 +1,15 @@
+#!/bin/sh
+# description: Kretprobe dynamic event with arguments
+
+[ -f kprobe_events ] || exit_unsupported # this is configurable
+
+echo 0 > events/enable
+echo > kprobe_events
+echo 'r:testprobe2 do_fork $retval' > kprobe_events
+grep testprobe2 kprobe_events
+test -d events/kprobes/testprobe2
+echo 1 > events/kprobes/testprobe2/enable
+( echo "forked")
+echo 0 > events/kprobes/testprobe2/enable
+echo '-:testprobe2' >> kprobe_events
+test -d events/kprobes/testprobe2 && exit 1 || exit 0

Re: [PATCH ftrace/for-next ] tracing/kprobes: Replace startup test with selftest script

[Steven Rostedt]

On Mon, 06 Oct 2014 11:48:06 +0000
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> Replace the kprobe_tracer's startup test with two selftest scripts.
> These test cases are testing that the kprobe_event can accept a
> kprobe event with $stack related arguments and a kretprobe event
> with $retval argument.

Can't we keep both? I have boxes I run my own tests with and enables
these start up tests in the kernel. I don't plan on testing on all
theses boxes using the scripts in the kernel.

Having a self test in the kernel itself can be useful too.

-- Steve

> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
>  kernel/trace/trace_kprobe.c                        |  142 --------------------
>  .../selftests/ftrace/test.d/kprobe/kprobe_args.tc  |   16 ++
>  .../ftrace/test.d/kprobe/kretprobe_args.tc         |   15 ++
>  3 files changed, 31 insertions(+), 142 deletions(-)
>  create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args.tc
>  create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_args.tc
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 282f6e4..1018f77 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -1352,145 +1352,3 @@ static __init int init_kprobe_trace(void)
>  }
>  fs_initcall(init_kprobe_trace);
>  
> -
> -#ifdef CONFIG_FTRACE_STARTUP_TEST
> -
> -/*
> - * The "__used" keeps gcc from removing the function symbol
> - * from the kallsyms table.
> - */
> -static __used int kprobe_trace_selftest_target(int a1, int a2, int a3,
> -					       int a4, int a5, int a6)
> -{
> -	return a1 + a2 + a3 + a4 + a5 + a6;
> -}
> -
> -static struct ftrace_event_file *
> -find_trace_probe_file(struct trace_kprobe *tk, struct trace_array *tr)
> -{
> -	struct ftrace_event_file *file;
> -
> -	list_for_each_entry(file, &tr->events, list)
> -		if (file->event_call == &tk->tp.call)
> -			return file;
> -
> -	return NULL;
> -}
> -
> -/*
> - * Nobody but us can call enable_trace_kprobe/disable_trace_kprobe at this
> - * stage, we can do this lockless.
> - */
> -static __init int kprobe_trace_self_tests_init(void)
> -{
> -	int ret, warn = 0;
> -	int (*target)(int, int, int, int, int, int);
> -	struct trace_kprobe *tk;
> -	struct ftrace_event_file *file;
> -
> -	if (tracing_is_disabled())
> -		return -ENODEV;
> -
> -	target = kprobe_trace_selftest_target;
> -
> -	pr_info("Testing kprobe tracing: ");
> -
> -	ret = traceprobe_command("p:testprobe kprobe_trace_selftest_target "
> -				  "$stack $stack0 +0($stack)",
> -				  create_trace_kprobe);
> -	if (WARN_ON_ONCE(ret)) {
> -		pr_warn("error on probing function entry.\n");
> -		warn++;
> -	} else {
> -		/* Enable trace point */
> -		tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
> -		if (WARN_ON_ONCE(tk == NULL)) {
> -			pr_warn("error on getting new probe.\n");
> -			warn++;
> -		} else {
> -			file = find_trace_probe_file(tk, top_trace_array());
> -			if (WARN_ON_ONCE(file == NULL)) {
> -				pr_warn("error on getting probe file.\n");
> -				warn++;
> -			} else
> -				enable_trace_kprobe(tk, file);
> -		}
> -	}
> -
> -	ret = traceprobe_command("r:testprobe2 kprobe_trace_selftest_target "
> -				  "$retval", create_trace_kprobe);
> -	if (WARN_ON_ONCE(ret)) {
> -		pr_warn("error on probing function return.\n");
> -		warn++;
> -	} else {
> -		/* Enable trace point */
> -		tk = find_trace_kprobe("testprobe2", KPROBE_EVENT_SYSTEM);
> -		if (WARN_ON_ONCE(tk == NULL)) {
> -			pr_warn("error on getting 2nd new probe.\n");
> -			warn++;
> -		} else {
> -			file = find_trace_probe_file(tk, top_trace_array());
> -			if (WARN_ON_ONCE(file == NULL)) {
> -				pr_warn("error on getting probe file.\n");
> -				warn++;
> -			} else
> -				enable_trace_kprobe(tk, file);
> -		}
> -	}
> -
> -	if (warn)
> -		goto end;
> -
> -	ret = target(1, 2, 3, 4, 5, 6);
> -
> -	/* Disable trace points before removing it */
> -	tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
> -	if (WARN_ON_ONCE(tk == NULL)) {
> -		pr_warn("error on getting test probe.\n");
> -		warn++;
> -	} else {
> -		file = find_trace_probe_file(tk, top_trace_array());
> -		if (WARN_ON_ONCE(file == NULL)) {
> -			pr_warn("error on getting probe file.\n");
> -			warn++;
> -		} else
> -			disable_trace_kprobe(tk, file);
> -	}
> -
> -	tk = find_trace_kprobe("testprobe2", KPROBE_EVENT_SYSTEM);
> -	if (WARN_ON_ONCE(tk == NULL)) {
> -		pr_warn("error on getting 2nd test probe.\n");
> -		warn++;
> -	} else {
> -		file = find_trace_probe_file(tk, top_trace_array());
> -		if (WARN_ON_ONCE(file == NULL)) {
> -			pr_warn("error on getting probe file.\n");
> -			warn++;
> -		} else
> -			disable_trace_kprobe(tk, file);
> -	}
> -
> -	ret = traceprobe_command("-:testprobe", create_trace_kprobe);
> -	if (WARN_ON_ONCE(ret)) {
> -		pr_warn("error on deleting a probe.\n");
> -		warn++;
> -	}
> -
> -	ret = traceprobe_command("-:testprobe2", create_trace_kprobe);
> -	if (WARN_ON_ONCE(ret)) {
> -		pr_warn("error on deleting a probe.\n");
> -		warn++;
> -	}
> -
> -end:
> -	release_all_trace_kprobes();
> -	if (warn)
> -		pr_cont("NG: Some tests are failed. Please check them.\n");
> -	else
> -		pr_cont("OK\n");
> -	return 0;
> -}
> -
> -late_initcall(kprobe_trace_self_tests_init);
> -
> -#endif
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args.tc
> new file mode 100644
> index 0000000..a603d3f
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args.tc
> @@ -0,0 +1,16 @@
> +#!/bin/sh
> +# description: Kprobe dynamic event with arguments
> +
> +[ -f kprobe_events ] || exit_unsupported # this is configurable
> +
> +echo 0 > events/enable
> +echo > kprobe_events
> +echo 'p:testprobe do_fork $stack $stack0 +0($stack)' > kprobe_events
> +grep testprobe kprobe_events
> +test -d events/kprobes/testprobe
> +echo 1 > events/kprobes/testprobe/enable
> +( echo "forked")
> +echo 0 > events/kprobes/testprobe/enable
> +echo "-:testprobe" >> kprobe_events
> +test -d events/kprobes/testprobe && exit 1 || exit 0
> +
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_args.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_args.tc
> new file mode 100644
> index 0000000..283c29e
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_args.tc
> @@ -0,0 +1,15 @@
> +#!/bin/sh
> +# description: Kretprobe dynamic event with arguments
> +
> +[ -f kprobe_events ] || exit_unsupported # this is configurable
> +
> +echo 0 > events/enable
> +echo > kprobe_events
> +echo 'r:testprobe2 do_fork $retval' > kprobe_events
> +grep testprobe2 kprobe_events
> +test -d events/kprobes/testprobe2
> +echo 1 > events/kprobes/testprobe2/enable
> +( echo "forked")
> +echo 0 > events/kprobes/testprobe2/enable
> +echo '-:testprobe2' >> kprobe_events
> +test -d events/kprobes/testprobe2 && exit 1 || exit 0
> 

Re: [PATCH ftrace/for-next ] tracing/kprobes: Replace startup test with selftest script

[Masami Hiramatsu]

(2014/10/07 7:33), Steven Rostedt wrote:
> On Mon, 06 Oct 2014 11:48:06 +0000
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> Replace the kprobe_tracer's startup test with two selftest scripts.
>> These test cases are testing that the kprobe_event can accept a
>> kprobe event with $stack related arguments and a kretprobe event
>> with $retval argument.
> 
> Can't we keep both? I have boxes I run my own tests with and enables
> these start up tests in the kernel. I don't plan on testing on all
> theses boxes using the scripts in the kernel.
> 
> Having a self test in the kernel itself can be useful too.

Hmm, deprecating the test is acceptable, but I think it is just
a dead weight that if we have both of them forever in the kernel.
Of course, if that feature is fundamentally related to booting up
the kernel, we need to keep them in boot up code. But if it is
possible to run after booting up, I think we'd better to move it
under kselftest, since we can do more investigation after booting.

Thank you,

> 
> -- Steve
> 
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> ---
>>  kernel/trace/trace_kprobe.c                        |  142 --------------------
>>  .../selftests/ftrace/test.d/kprobe/kprobe_args.tc  |   16 ++
>>  .../ftrace/test.d/kprobe/kretprobe_args.tc         |   15 ++
>>  3 files changed, 31 insertions(+), 142 deletions(-)
>>  create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args.tc
>>  create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_args.tc
>>
>> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
>> index 282f6e4..1018f77 100644
>> --- a/kernel/trace/trace_kprobe.c
>> +++ b/kernel/trace/trace_kprobe.c
>> @@ -1352,145 +1352,3 @@ static __init int init_kprobe_trace(void)
>>  }
>>  fs_initcall(init_kprobe_trace);
>>  
>> -
>> -#ifdef CONFIG_FTRACE_STARTUP_TEST
>> -
>> -/*
>> - * The "__used" keeps gcc from removing the function symbol
>> - * from the kallsyms table.
>> - */
>> -static __used int kprobe_trace_selftest_target(int a1, int a2, int a3,
>> -					       int a4, int a5, int a6)
>> -{
>> -	return a1 + a2 + a3 + a4 + a5 + a6;
>> -}
>> -
>> -static struct ftrace_event_file *
>> -find_trace_probe_file(struct trace_kprobe *tk, struct trace_array *tr)
>> -{
>> -	struct ftrace_event_file *file;
>> -
>> -	list_for_each_entry(file, &tr->events, list)
>> -		if (file->event_call == &tk->tp.call)
>> -			return file;
>> -
>> -	return NULL;
>> -}
>> -
>> -/*
>> - * Nobody but us can call enable_trace_kprobe/disable_trace_kprobe at this
>> - * stage, we can do this lockless.
>> - */
>> -static __init int kprobe_trace_self_tests_init(void)
>> -{
>> -	int ret, warn = 0;
>> -	int (*target)(int, int, int, int, int, int);
>> -	struct trace_kprobe *tk;
>> -	struct ftrace_event_file *file;
>> -
>> -	if (tracing_is_disabled())
>> -		return -ENODEV;
>> -
>> -	target = kprobe_trace_selftest_target;
>> -
>> -	pr_info("Testing kprobe tracing: ");
>> -
>> -	ret = traceprobe_command("p:testprobe kprobe_trace_selftest_target "
>> -				  "$stack $stack0 +0($stack)",
>> -				  create_trace_kprobe);
>> -	if (WARN_ON_ONCE(ret)) {
>> -		pr_warn("error on probing function entry.\n");
>> -		warn++;
>> -	} else {
>> -		/* Enable trace point */
>> -		tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
>> -		if (WARN_ON_ONCE(tk == NULL)) {
>> -			pr_warn("error on getting new probe.\n");
>> -			warn++;
>> -		} else {
>> -			file = find_trace_probe_file(tk, top_trace_array());
>> -			if (WARN_ON_ONCE(file == NULL)) {
>> -				pr_warn("error on getting probe file.\n");
>> -				warn++;
>> -			} else
>> -				enable_trace_kprobe(tk, file);
>> -		}
>> -	}
>> -
>> -	ret = traceprobe_command("r:testprobe2 kprobe_trace_selftest_target "
>> -				  "$retval", create_trace_kprobe);
>> -	if (WARN_ON_ONCE(ret)) {
>> -		pr_warn("error on probing function return.\n");
>> -		warn++;
>> -	} else {
>> -		/* Enable trace point */
>> -		tk = find_trace_kprobe("testprobe2", KPROBE_EVENT_SYSTEM);
>> -		if (WARN_ON_ONCE(tk == NULL)) {
>> -			pr_warn("error on getting 2nd new probe.\n");
>> -			warn++;
>> -		} else {
>> -			file = find_trace_probe_file(tk, top_trace_array());
>> -			if (WARN_ON_ONCE(file == NULL)) {
>> -				pr_warn("error on getting probe file.\n");
>> -				warn++;
>> -			} else
>> -				enable_trace_kprobe(tk, file);
>> -		}
>> -	}
>> -
>> -	if (warn)
>> -		goto end;
>> -
>> -	ret = target(1, 2, 3, 4, 5, 6);
>> -
>> -	/* Disable trace points before removing it */
>> -	tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
>> -	if (WARN_ON_ONCE(tk == NULL)) {
>> -		pr_warn("error on getting test probe.\n");
>> -		warn++;
>> -	} else {
>> -		file = find_trace_probe_file(tk, top_trace_array());
>> -		if (WARN_ON_ONCE(file == NULL)) {
>> -			pr_warn("error on getting probe file.\n");
>> -			warn++;
>> -		} else
>> -			disable_trace_kprobe(tk, file);
>> -	}
>> -
>> -	tk = find_trace_kprobe("testprobe2", KPROBE_EVENT_SYSTEM);
>> -	if (WARN_ON_ONCE(tk == NULL)) {
>> -		pr_warn("error on getting 2nd test probe.\n");
>> -		warn++;
>> -	} else {
>> -		file = find_trace_probe_file(tk, top_trace_array());
>> -		if (WARN_ON_ONCE(file == NULL)) {
>> -			pr_warn("error on getting probe file.\n");
>> -			warn++;
>> -		} else
>> -			disable_trace_kprobe(tk, file);
>> -	}
>> -
>> -	ret = traceprobe_command("-:testprobe", create_trace_kprobe);
>> -	if (WARN_ON_ONCE(ret)) {
>> -		pr_warn("error on deleting a probe.\n");
>> -		warn++;
>> -	}
>> -
>> -	ret = traceprobe_command("-:testprobe2", create_trace_kprobe);
>> -	if (WARN_ON_ONCE(ret)) {
>> -		pr_warn("error on deleting a probe.\n");
>> -		warn++;
>> -	}
>> -
>> -end:
>> -	release_all_trace_kprobes();
>> -	if (warn)
>> -		pr_cont("NG: Some tests are failed. Please check them.\n");
>> -	else
>> -		pr_cont("OK\n");
>> -	return 0;
>> -}
>> -
>> -late_initcall(kprobe_trace_self_tests_init);
>> -
>> -#endif
>> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args.tc
>> new file mode 100644
>> index 0000000..a603d3f
>> --- /dev/null
>> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args.tc
>> @@ -0,0 +1,16 @@
>> +#!/bin/sh
>> +# description: Kprobe dynamic event with arguments
>> +
>> +[ -f kprobe_events ] || exit_unsupported # this is configurable
>> +
>> +echo 0 > events/enable
>> +echo > kprobe_events
>> +echo 'p:testprobe do_fork $stack $stack0 +0($stack)' > kprobe_events
>> +grep testprobe kprobe_events
>> +test -d events/kprobes/testprobe
>> +echo 1 > events/kprobes/testprobe/enable
>> +( echo "forked")
>> +echo 0 > events/kprobes/testprobe/enable
>> +echo "-:testprobe" >> kprobe_events
>> +test -d events/kprobes/testprobe && exit 1 || exit 0
>> +
>> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_args.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_args.tc
>> new file mode 100644
>> index 0000000..283c29e
>> --- /dev/null
>> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_args.tc
>> @@ -0,0 +1,15 @@
>> +#!/bin/sh
>> +# description: Kretprobe dynamic event with arguments
>> +
>> +[ -f kprobe_events ] || exit_unsupported # this is configurable
>> +
>> +echo 0 > events/enable
>> +echo > kprobe_events
>> +echo 'r:testprobe2 do_fork $retval' > kprobe_events
>> +grep testprobe2 kprobe_events
>> +test -d events/kprobes/testprobe2
>> +echo 1 > events/kprobes/testprobe2/enable
>> +( echo "forked")
>> +echo 0 > events/kprobes/testprobe2/enable
>> +echo '-:testprobe2' >> kprobe_events
>> +test -d events/kprobes/testprobe2 && exit 1 || exit 0
>>
> 
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH ftrace/for-next ] tracing/kprobes: Replace startup test with selftest script

[Steven Rostedt]

On Tue, 07 Oct 2014 15:00:28 +0900
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> (2014/10/07 7:33), Steven Rostedt wrote:
> > On Mon, 06 Oct 2014 11:48:06 +0000
> > Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> > 
> >> Replace the kprobe_tracer's startup test with two selftest scripts.
> >> These test cases are testing that the kprobe_event can accept a
> >> kprobe event with $stack related arguments and a kretprobe event
> >> with $retval argument.
> > 
> > Can't we keep both? I have boxes I run my own tests with and enables
> > these start up tests in the kernel. I don't plan on testing on all
> > theses boxes using the scripts in the kernel.
> > 
> > Having a self test in the kernel itself can be useful too.
> 
> Hmm, deprecating the test is acceptable, but I think it is just
> a dead weight that if we have both of them forever in the kernel.
> Of course, if that feature is fundamentally related to booting up
> the kernel, we need to keep them in boot up code. But if it is
> possible to run after booting up, I think we'd better to move it
> under kselftest, since we can do more investigation after booting.
> 

I'm just saying that it is more likely to have this test run if it's in
the kernel than in userspace. But as you say, we can debug it better if
there's a userspace tool that can run too. This is why I'm saying we
should keep both. I think they are both useful for different reasons.
Keeping it in the kernel as a config option will give it more exposure,
and keeping it in the tools/testing directory gives us a way to debug
it if there an issue should arise.

Both of these have valid reasons staying in the kernel and I don't see
either as dead weight. Is there a maintenance issue with keeping it in
the kernel? There doesn't seem to be much done to it. It seems
untouched for over a year, and that was to add support for multiple
buffers.

-- Steve

Re: Re: [PATCH ftrace/for-next ] tracing/kprobes: Replace startup test with selftest script

[Masami Hiramatsu]

(2014/10/08 1:05), Steven Rostedt wrote:
> On Tue, 07 Oct 2014 15:00:28 +0900
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> (2014/10/07 7:33), Steven Rostedt wrote:
>>> On Mon, 06 Oct 2014 11:48:06 +0000
>>> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
>>>
>>>> Replace the kprobe_tracer's startup test with two selftest scripts.
>>>> These test cases are testing that the kprobe_event can accept a
>>>> kprobe event with $stack related arguments and a kretprobe event
>>>> with $retval argument.
>>>
>>> Can't we keep both? I have boxes I run my own tests with and enables
>>> these start up tests in the kernel. I don't plan on testing on all
>>> theses boxes using the scripts in the kernel.
>>>
>>> Having a self test in the kernel itself can be useful too.
>>
>> Hmm, deprecating the test is acceptable, but I think it is just
>> a dead weight that if we have both of them forever in the kernel.
>> Of course, if that feature is fundamentally related to booting up
>> the kernel, we need to keep them in boot up code. But if it is
>> possible to run after booting up, I think we'd better to move it
>> under kselftest, since we can do more investigation after booting.
>>
> 
> I'm just saying that it is more likely to have this test run if it's in
> the kernel than in userspace. But as you say, we can debug it better if
> there's a userspace tool that can run too. This is why I'm saying we
> should keep both. I think they are both useful for different reasons.
> Keeping it in the kernel as a config option will give it more exposure,
> and keeping it in the tools/testing directory gives us a way to debug
> it if there an issue should arise.

OK, I can keep both in the kernel at this point. I just expected when we
have kselftest which becomes good enough and widely used, then it is more
likely to be run than the startup selftest, because the startup selftest
requires kernel configuration but kselftest is completely add-on test
(so that we can run it on distro kernel too :)
However, yes, it is just my guess.

> Both of these have valid reasons staying in the kernel and I don't see
> either as dead weight. Is there a maintenance issue with keeping it in
> the kernel? There doesn't seem to be much done to it. It seems
> untouched for over a year, and that was to add support for multiple
> buffers.

Keeping it has no issue. But it's much easier to expand the test
in userspace than the kernel code. I'll add more feature tests in
kselftest, but not in this code. This means that this startup
test code will get behind.

Thank you,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH ftrace/for-next ] tracing/kprobes: Replace startup test with selftest script

[Steven Rostedt]

On Wed, 08 Oct 2014 10:59:49 +0900
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
 
> > Both of these have valid reasons staying in the kernel and I don't see
> > either as dead weight. Is there a maintenance issue with keeping it in
> > the kernel? There doesn't seem to be much done to it. It seems
> > untouched for over a year, and that was to add support for multiple
> > buffers.
> 
> Keeping it has no issue. But it's much easier to expand the test
> in userspace than the kernel code. I'll add more feature tests in
> kselftest, but not in this code. This means that this startup
> test code will get behind.

And that's exactly what I expect you to do. I have lots of tests to
test ftrace, but what gets tested at kernel startup is just a bare
minimum, and that's all it needs to be. I don't expect you to extend
the start up self tests. That should be only done for the scripts. But
we have this start up test and I don't see a reason to get rid of it.
If anything, it gives me warm fuzzies in my stomach when I see it
pass :-)

The start up tests in the kernel should really just be the basic of the
basic tests, that give a small sanity check that a change didn't
totally screw things up.

Can you send a new patch that doesn't remove the start up test?

Thanks!

-- Steve

Re: Re: [PATCH ftrace/for-next ] tracing/kprobes: Replace startup test with selftest script

[Masami Hiramatsu]

(2014/10/08 11:20), Steven Rostedt wrote:
> On Wed, 08 Oct 2014 10:59:49 +0900
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
>  
>>> Both of these have valid reasons staying in the kernel and I don't see
>>> either as dead weight. Is there a maintenance issue with keeping it in
>>> the kernel? There doesn't seem to be much done to it. It seems
>>> untouched for over a year, and that was to add support for multiple
>>> buffers.
>>
>> Keeping it has no issue. But it's much easier to expand the test
>> in userspace than the kernel code. I'll add more feature tests in
>> kselftest, but not in this code. This means that this startup
>> test code will get behind.
> 
> And that's exactly what I expect you to do. I have lots of tests to
> test ftrace, but what gets tested at kernel startup is just a bare
> minimum, and that's all it needs to be. I don't expect you to extend
> the start up self tests. That should be only done for the scripts. But
> we have this start up test and I don't see a reason to get rid of it.
> If anything, it gives me warm fuzzies in my stomach when I see it
> pass :-)
> 
> The start up tests in the kernel should really just be the basic of the
> basic tests, that give a small sanity check that a change didn't
> totally screw things up.
> 
> Can you send a new patch that doesn't remove the start up test?

OK, I'll send it asap :)

Thank you,


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com