All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] selftests/ftrace: Add tests for kprobes and optimized probes
@ 2023-04-18  9:55 Akanksha J N
  2023-04-18  9:55 ` [PATCH v2 1/2] selftests/ftrace: Add new test case which adds multiple consecutive probes in a function Akanksha J N
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Akanksha J N @ 2023-04-18  9:55 UTC (permalink / raw)
  To: linux-kselftest, linux-trace-kernel, linux-kernel
  Cc: rostedt, mhiramat, shuah, naveen.n.rao, akanksha

This patchset adds a stress test for kprobes and a test for checking
optimized probes.
The two tests are being added based on the below discussion:
https://lore.kernel.org/all/20230128101622.ce6f8e64d929e29d36b08b73@kernel.org/

Changelog:


* Add an explicit fork after enabling the events ( echo "forked" )
* Remove the extended test from multiple_kprobe_types.tc which adds 
  multiple consecutive probes in a function and add it as a 
  separate test case. 
* Add new test case which checks for optimized probes.

Akanksha J N (2):
  selftests/ftrace: Add new test case which adds multiple consecutive
    probes in a function
  selftests/ftrace: Add new test case which checks for optimized probes

 .../test.d/kprobe/kprobe_insn_boundary.tc     | 19 +++++++++++
 .../ftrace/test.d/kprobe/kprobe_opt_types.tc  | 34 +++++++++++++++++++
 2 files changed, 53 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_insn_boundary.tc
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc

-- 
2.31.1


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

* [PATCH v2 1/2] selftests/ftrace: Add new test case which adds multiple consecutive probes in a function
  2023-04-18  9:55 [PATCH v2 0/2] selftests/ftrace: Add tests for kprobes and optimized probes Akanksha J N
@ 2023-04-18  9:55 ` Akanksha J N
  2023-04-25  0:12   ` Masami Hiramatsu
  2023-04-18  9:55 ` [PATCH v2 2/2] selftests/ftrace: Add new test case which checks for optimized probes Akanksha J N
  2023-04-24 11:09 ` [PATCH v2 0/2] selftests/ftrace: Add tests for kprobes and " Naveen N. Rao
  2 siblings, 1 reply; 9+ messages in thread
From: Akanksha J N @ 2023-04-18  9:55 UTC (permalink / raw)
  To: linux-kselftest, linux-trace-kernel, linux-kernel
  Cc: rostedt, mhiramat, shuah, naveen.n.rao, akanksha

Commit 97f88a3d723162 ("powerpc/kprobes: Fix null pointer reference in
arch_prepare_kprobe()") fixed a recent kernel oops that was caused as
ftrace-based kprobe does not generate kprobe::ainsn::insn and it gets
set to NULL.
Add new test case kprobe_insn_boundary.tc which adds a
kprobe at every byte within $FUNCTION_FORK up to an offset of 256 bytes,
to be able to test potential issues with kprobes on
successive instructions.
The '|| continue' is added with the echo statement to ignore errors that
are caused by trying to add kprobes to non probeable lines and continue
with the test.
Signed-off-by: Akanksha J N <akanksha@linux.ibm.com>
---
 .../test.d/kprobe/kprobe_insn_boundary.tc     | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_insn_boundary.tc

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_insn_boundary.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_insn_boundary.tc
new file mode 100644
index 000000000000..4f7cc318f331
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_insn_boundary.tc
@@ -0,0 +1,19 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (C) 2023 Akanksha J N, IBM corporation
+# description: Register multiple kprobe events in a function
+# requires: kprobe_events
+
+for i in `seq 0 255`; do
+  echo p $FUNCTION_FORK+${i} >> kprobe_events || continue
+done
+
+cat kprobe_events >> $testlog
+
+echo 1 > events/kprobes/enable
+( echo "forked" )
+echo 0 > events/kprobes/enable
+echo > kprobe_events
+echo "Waiting for unoptimizing & freeing"
+sleep 5
+echo "Done"
-- 
2.31.1


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

* [PATCH v2 2/2] selftests/ftrace: Add new test case which checks for optimized probes
  2023-04-18  9:55 [PATCH v2 0/2] selftests/ftrace: Add tests for kprobes and optimized probes Akanksha J N
  2023-04-18  9:55 ` [PATCH v2 1/2] selftests/ftrace: Add new test case which adds multiple consecutive probes in a function Akanksha J N
@ 2023-04-18  9:55 ` Akanksha J N
  2023-04-25  0:10   ` Masami Hiramatsu
  2023-04-24 11:09 ` [PATCH v2 0/2] selftests/ftrace: Add tests for kprobes and " Naveen N. Rao
  2 siblings, 1 reply; 9+ messages in thread
From: Akanksha J N @ 2023-04-18  9:55 UTC (permalink / raw)
  To: linux-kselftest, linux-trace-kernel, linux-kernel
  Cc: rostedt, mhiramat, shuah, naveen.n.rao, akanksha

Add new test case kprobe_opt_types.tc which enables and checks
if each probe has been optimized in order to test potential issues with
optimized probes.
The '|| continue' is added with the echo statement to ignore errors that
are caused by trying to add kprobes to non probeable lines and continue
with the test.
Signed-off-by: Akanksha J N <akanksha@linux.ibm.com>
---
 .../ftrace/test.d/kprobe/kprobe_opt_types.tc  | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc
new file mode 100644
index 000000000000..54e4800b8a13
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc
@@ -0,0 +1,34 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (C) 2023 Akanksha J N, IBM corporation
+# description: Register/unregister optimized probe
+# requires: kprobe_events
+
+case `uname -m` in
+x86_64)
+;;
+arm*)
+;;
+ppc*)
+;;
+*)
+  echo "Please implement other architecture here"
+  exit_unsupported
+esac
+
+DEFAULT=$(cat /proc/sys/debug/kprobes-optimization)
+echo 1 > /proc/sys/debug/kprobes-optimization
+for i in `seq 0 255`; do
+        echo  "p:testprobe $FUNCTION_FORK+${i}" > kprobe_events || continue
+        echo 1 > events/kprobes/enable || continue
+        (echo "forked")
+        PROBE_TYPE=$(cat /sys/kernel/debug/kprobes/list | grep $FUNCTION_FORK | awk '{print $4}' | awk '{print substr($0,2,length($0)-2)}')
+        echo 0 > events/kprobes/enable
+        echo > kprobe_events
+        if [ $PROBE_TYPE = "OPTIMIZED" ]; then
+                echo "$DEFAULT" >  /proc/sys/debug/kprobes-optimization
+                exit_pass
+        fi
+done
+echo "$DEFAULT" >  /proc/sys/debug/kprobes-optimization
+echo "Done"
-- 
2.31.1


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

* Re: [PATCH v2 0/2] selftests/ftrace: Add tests for kprobes and optimized probes
  2023-04-18  9:55 [PATCH v2 0/2] selftests/ftrace: Add tests for kprobes and optimized probes Akanksha J N
  2023-04-18  9:55 ` [PATCH v2 1/2] selftests/ftrace: Add new test case which adds multiple consecutive probes in a function Akanksha J N
  2023-04-18  9:55 ` [PATCH v2 2/2] selftests/ftrace: Add new test case which checks for optimized probes Akanksha J N
@ 2023-04-24 11:09 ` Naveen N. Rao
  2 siblings, 0 replies; 9+ messages in thread
From: Naveen N. Rao @ 2023-04-24 11:09 UTC (permalink / raw)
  To: Akanksha J N, linux-kernel, linux-kselftest, linux-trace-kernel
  Cc: mhiramat, rostedt, shuah

Akanksha J N wrote:
> This patchset adds a stress test for kprobes and a test for checking
> optimized probes.
> The two tests are being added based on the below discussion:
> https://lore.kernel.org/all/20230128101622.ce6f8e64d929e29d36b08b73@kernel.org/
> 
> Changelog:
> 
> 
> * Add an explicit fork after enabling the events ( echo "forked" )
> * Remove the extended test from multiple_kprobe_types.tc which adds 
>   multiple consecutive probes in a function and add it as a 
>   separate test case. 
> * Add new test case which checks for optimized probes.
> 
> Akanksha J N (2):
>   selftests/ftrace: Add new test case which adds multiple consecutive
>     probes in a function
>   selftests/ftrace: Add new test case which checks for optimized probes
> 
>  .../test.d/kprobe/kprobe_insn_boundary.tc     | 19 +++++++++++
>  .../ftrace/test.d/kprobe/kprobe_opt_types.tc  | 34 +++++++++++++++++++
>  2 files changed, 53 insertions(+)
>  create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_insn_boundary.tc
>  create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc

This series looks good to me.
Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>



- Naveen


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

* Re: [PATCH v2 2/2] selftests/ftrace: Add new test case which checks for optimized probes
  2023-04-18  9:55 ` [PATCH v2 2/2] selftests/ftrace: Add new test case which checks for optimized probes Akanksha J N
@ 2023-04-25  0:10   ` Masami Hiramatsu
  2023-04-25  5:28     ` Naveen N. Rao
  0 siblings, 1 reply; 9+ messages in thread
From: Masami Hiramatsu @ 2023-04-25  0:10 UTC (permalink / raw)
  To: Akanksha J N
  Cc: linux-kselftest, linux-trace-kernel, linux-kernel, rostedt,
	mhiramat, shuah, naveen.n.rao

On Tue, 18 Apr 2023 15:25:57 +0530
Akanksha J N <akanksha@linux.ibm.com> wrote:

> Add new test case kprobe_opt_types.tc which enables and checks
> if each probe has been optimized in order to test potential issues with
> optimized probes.
> The '|| continue' is added with the echo statement to ignore errors that
> are caused by trying to add kprobes to non probeable lines and continue
> with the test.
> Signed-off-by: Akanksha J N <akanksha@linux.ibm.com>
> ---
>  .../ftrace/test.d/kprobe/kprobe_opt_types.tc  | 34 +++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc
> new file mode 100644
> index 000000000000..54e4800b8a13
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2023 Akanksha J N, IBM corporation
> +# description: Register/unregister optimized probe
> +# requires: kprobe_events
> +
> +case `uname -m` in
> +x86_64)
> +;;
> +arm*)
> +;;
> +ppc*)
> +;;
> +*)
> +  echo "Please implement other architecture here"
> +  exit_unsupported
> +esac
> +
> +DEFAULT=$(cat /proc/sys/debug/kprobes-optimization)
> +echo 1 > /proc/sys/debug/kprobes-optimization
> +for i in `seq 0 255`; do
> +        echo  "p:testprobe $FUNCTION_FORK+${i}" > kprobe_events || continue
> +        echo 1 > events/kprobes/enable || continue
> +        (echo "forked")
> +        PROBE_TYPE=$(cat /sys/kernel/debug/kprobes/list | grep $FUNCTION_FORK | awk '{print $4}' | awk '{print substr($0,2,length($0)-2)}')

I think we can make it simply;

PROBE=$(grep $FUNCTION_FORK /sys/kernel/debug/kprobes/list)

> +        echo 0 > events/kprobes/enable
> +        echo > kprobe_events
> +        if [ $PROBE_TYPE = "OPTIMIZED" ]; then

and

if echo $PROBE | grep -q OPTIMIZED; then

> +                echo "$DEFAULT" >  /proc/sys/debug/kprobes-optimization
> +                exit_pass
> +        fi
> +done
> +echo "$DEFAULT" >  /proc/sys/debug/kprobes-optimization
> +echo "Done"

Hmm, this test does NOT return any error. It always returns success.
I understand that optimization may not be possible within 256 bytes
from the beginning of the function. In that case, you can return
"unresolved", and not echoing "Done" but the reason why it is
unresolved.

Thank you,

> -- 
> 2.31.1
> 


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

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

* Re: [PATCH v2 1/2] selftests/ftrace: Add new test case which adds multiple consecutive probes in a function
  2023-04-18  9:55 ` [PATCH v2 1/2] selftests/ftrace: Add new test case which adds multiple consecutive probes in a function Akanksha J N
@ 2023-04-25  0:12   ` Masami Hiramatsu
  0 siblings, 0 replies; 9+ messages in thread
From: Masami Hiramatsu @ 2023-04-25  0:12 UTC (permalink / raw)
  To: Akanksha J N
  Cc: linux-kselftest, linux-trace-kernel, linux-kernel, rostedt,
	mhiramat, shuah, naveen.n.rao

On Tue, 18 Apr 2023 15:25:56 +0530
Akanksha J N <akanksha@linux.ibm.com> wrote:

> Commit 97f88a3d723162 ("powerpc/kprobes: Fix null pointer reference in
> arch_prepare_kprobe()") fixed a recent kernel oops that was caused as
> ftrace-based kprobe does not generate kprobe::ainsn::insn and it gets
> set to NULL.
> Add new test case kprobe_insn_boundary.tc which adds a
> kprobe at every byte within $FUNCTION_FORK up to an offset of 256 bytes,
> to be able to test potential issues with kprobes on
> successive instructions.
> The '|| continue' is added with the echo statement to ignore errors that
> are caused by trying to add kprobes to non probeable lines and continue
> with the test.

Hi Akanksha,

Thanks for adding test case. This looks good to me.

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

> Signed-off-by: Akanksha J N <akanksha@linux.ibm.com>
> ---
>  .../test.d/kprobe/kprobe_insn_boundary.tc     | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_insn_boundary.tc
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_insn_boundary.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_insn_boundary.tc
> new file mode 100644
> index 000000000000..4f7cc318f331
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_insn_boundary.tc
> @@ -0,0 +1,19 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2023 Akanksha J N, IBM corporation
> +# description: Register multiple kprobe events in a function
> +# requires: kprobe_events
> +
> +for i in `seq 0 255`; do
> +  echo p $FUNCTION_FORK+${i} >> kprobe_events || continue
> +done
> +
> +cat kprobe_events >> $testlog
> +
> +echo 1 > events/kprobes/enable
> +( echo "forked" )
> +echo 0 > events/kprobes/enable
> +echo > kprobe_events
> +echo "Waiting for unoptimizing & freeing"
> +sleep 5
> +echo "Done"
> -- 
> 2.31.1
> 


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

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

* Re: [PATCH v2 2/2] selftests/ftrace: Add new test case which checks for optimized probes
  2023-04-25  0:10   ` Masami Hiramatsu
@ 2023-04-25  5:28     ` Naveen N. Rao
  2023-04-25 14:41       ` Masami Hiramatsu
  0 siblings, 1 reply; 9+ messages in thread
From: Naveen N. Rao @ 2023-04-25  5:28 UTC (permalink / raw)
  To: Akanksha J N, Masami Hiramatsu
  Cc: linux-kernel, linux-kselftest, linux-trace-kernel, rostedt, shuah

Masami Hiramatsu wrote:
> On Tue, 18 Apr 2023 15:25:57 +0530
> Akanksha J N <akanksha@linux.ibm.com> wrote:
> 
>> Add new test case kprobe_opt_types.tc which enables and checks
>> if each probe has been optimized in order to test potential issues with
>> optimized probes.
>> The '|| continue' is added with the echo statement to ignore errors that
>> are caused by trying to add kprobes to non probeable lines and continue
>> with the test.
>> Signed-off-by: Akanksha J N <akanksha@linux.ibm.com>
>> ---
>>  .../ftrace/test.d/kprobe/kprobe_opt_types.tc  | 34 +++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>  create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc
>> 
>> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc
>> new file mode 100644
>> index 000000000000..54e4800b8a13
>> --- /dev/null
>> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc
>> @@ -0,0 +1,34 @@
>> +#!/bin/sh
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +# Copyright (C) 2023 Akanksha J N, IBM corporation
>> +# description: Register/unregister optimized probe
>> +# requires: kprobe_events
>> +
>> +case `uname -m` in
>> +x86_64)
>> +;;
>> +arm*)
>> +;;
>> +ppc*)
>> +;;
>> +*)
>> +  echo "Please implement other architecture here"
>> +  exit_unsupported
>> +esac
>> +
>> +DEFAULT=$(cat /proc/sys/debug/kprobes-optimization)
>> +echo 1 > /proc/sys/debug/kprobes-optimization
>> +for i in `seq 0 255`; do
>> +        echo  "p:testprobe $FUNCTION_FORK+${i}" > kprobe_events || continue
>> +        echo 1 > events/kprobes/enable || continue
>> +        (echo "forked")
>> +        PROBE_TYPE=$(cat /sys/kernel/debug/kprobes/list | grep $FUNCTION_FORK | awk '{print $4}' | awk '{print substr($0,2,length($0)-2)}')
> 
> I think we can make it simply;
> 
> PROBE=$(grep $FUNCTION_FORK /sys/kernel/debug/kprobes/list)
> 
>> +        echo 0 > events/kprobes/enable
>> +        echo > kprobe_events
>> +        if [ $PROBE_TYPE = "OPTIMIZED" ]; then
> 
> and
> 
> if echo $PROBE | grep -q OPTIMIZED; then
> 
>> +                echo "$DEFAULT" >  /proc/sys/debug/kprobes-optimization
>> +                exit_pass
>> +        fi
>> +done
>> +echo "$DEFAULT" >  /proc/sys/debug/kprobes-optimization
>> +echo "Done"
> 
> Hmm, this test does NOT return any error. It always returns success.

Good catch!

> I understand that optimization may not be possible within 256 bytes
> from the beginning of the function.

Is that true in practice? Looking at x86 and ppc64le, it looks like we 
will almost always be able to optimize at least one of the instructions 
within the first 256 bytes of kernel_clone(). That's one of the primary 
purposes of this test.

Are there valid reasons why we may not be able to optimize instructions?

> In that case, you can return
> "unresolved", and not echoing "Done" but the reason why it is
> unresolved.


- Naveen


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

* Re: [PATCH v2 2/2] selftests/ftrace: Add new test case which checks for optimized probes
  2023-04-25  5:28     ` Naveen N. Rao
@ 2023-04-25 14:41       ` Masami Hiramatsu
  2023-04-26 11:01         ` Naveen N. Rao
  0 siblings, 1 reply; 9+ messages in thread
From: Masami Hiramatsu @ 2023-04-25 14:41 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Akanksha J N, linux-kernel, linux-kselftest, linux-trace-kernel,
	rostedt, shuah

On Tue, 25 Apr 2023 10:58:30 +0530
"Naveen N. Rao" <naveen.n.rao@linux.ibm.com> wrote:

> Masami Hiramatsu wrote:
> > On Tue, 18 Apr 2023 15:25:57 +0530
> > Akanksha J N <akanksha@linux.ibm.com> wrote:
> > 
> >> Add new test case kprobe_opt_types.tc which enables and checks
> >> if each probe has been optimized in order to test potential issues with
> >> optimized probes.
> >> The '|| continue' is added with the echo statement to ignore errors that
> >> are caused by trying to add kprobes to non probeable lines and continue
> >> with the test.
> >> Signed-off-by: Akanksha J N <akanksha@linux.ibm.com>
> >> ---
> >>  .../ftrace/test.d/kprobe/kprobe_opt_types.tc  | 34 +++++++++++++++++++
> >>  1 file changed, 34 insertions(+)
> >>  create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc
> >> 
> >> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc
> >> new file mode 100644
> >> index 000000000000..54e4800b8a13
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc
> >> @@ -0,0 +1,34 @@
> >> +#!/bin/sh
> >> +# SPDX-License-Identifier: GPL-2.0-or-later
> >> +# Copyright (C) 2023 Akanksha J N, IBM corporation
> >> +# description: Register/unregister optimized probe
> >> +# requires: kprobe_events
> >> +
> >> +case `uname -m` in
> >> +x86_64)
> >> +;;
> >> +arm*)
> >> +;;
> >> +ppc*)
> >> +;;
> >> +*)
> >> +  echo "Please implement other architecture here"
> >> +  exit_unsupported
> >> +esac
> >> +
> >> +DEFAULT=$(cat /proc/sys/debug/kprobes-optimization)
> >> +echo 1 > /proc/sys/debug/kprobes-optimization
> >> +for i in `seq 0 255`; do
> >> +        echo  "p:testprobe $FUNCTION_FORK+${i}" > kprobe_events || continue
> >> +        echo 1 > events/kprobes/enable || continue
> >> +        (echo "forked")
> >> +        PROBE_TYPE=$(cat /sys/kernel/debug/kprobes/list | grep $FUNCTION_FORK | awk '{print $4}' | awk '{print substr($0,2,length($0)-2)}')
> > 
> > I think we can make it simply;
> > 
> > PROBE=$(grep $FUNCTION_FORK /sys/kernel/debug/kprobes/list)
> > 
> >> +        echo 0 > events/kprobes/enable
> >> +        echo > kprobe_events
> >> +        if [ $PROBE_TYPE = "OPTIMIZED" ]; then
> > 
> > and
> > 
> > if echo $PROBE | grep -q OPTIMIZED; then
> > 
> >> +                echo "$DEFAULT" >  /proc/sys/debug/kprobes-optimization
> >> +                exit_pass
> >> +        fi
> >> +done
> >> +echo "$DEFAULT" >  /proc/sys/debug/kprobes-optimization
> >> +echo "Done"
> > 
> > Hmm, this test does NOT return any error. It always returns success.
> 
> Good catch!
> 
> > I understand that optimization may not be possible within 256 bytes
> > from the beginning of the function.
> 
> Is that true in practice? Looking at x86 and ppc64le, it looks like we 
> will almost always be able to optimize at least one of the instructions 
> within the first 256 bytes of kernel_clone(). That's one of the primary 
> purposes of this test.

Yeah, usually it should not happen. But since we don't disassemble it,
we can not ensure that. So this depends on the compiler at last.

> 
> Are there valid reasons why we may not be able to optimize instructions?

For example, if the compiler starts inserting some checker instruction
on each instruction boundary for security, it may prevent optimizing
kprobes. Usually it should not happen (because it bloat up the kernel size)
but we cannot deny the possibility of such new feature as an option
in the future.

> 
> > In that case, you can return
> > "unresolved", and not echoing "Done" but the reason why it is
> > unresolved.

Even in that case, it can notify such case as "unresolved", then we
can notice it. (something like WARN_ON)

Thank you,

> 
> 
> - Naveen
> 


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

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

* Re: [PATCH v2 2/2] selftests/ftrace: Add new test case which checks for optimized probes
  2023-04-25 14:41       ` Masami Hiramatsu
@ 2023-04-26 11:01         ` Naveen N. Rao
  0 siblings, 0 replies; 9+ messages in thread
From: Naveen N. Rao @ 2023-04-26 11:01 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Akanksha J N, linux-kernel, linux-kselftest, linux-trace-kernel,
	rostedt, shuah

Masami Hiramatsu wrote:
> On Tue, 25 Apr 2023 10:58:30 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.ibm.com> wrote:
> 
>> Masami Hiramatsu wrote:
>> > On Tue, 18 Apr 2023 15:25:57 +0530
>> > Akanksha J N <akanksha@linux.ibm.com> wrote:
>> > 
>> >> Add new test case kprobe_opt_types.tc which enables and checks
>> >> if each probe has been optimized in order to test potential issues with
>> >> optimized probes.
>> >> The '|| continue' is added with the echo statement to ignore errors that
>> >> are caused by trying to add kprobes to non probeable lines and continue
>> >> with the test.
>> >> Signed-off-by: Akanksha J N <akanksha@linux.ibm.com>
>> >> ---
>> >>  .../ftrace/test.d/kprobe/kprobe_opt_types.tc  | 34 +++++++++++++++++++
>> >>  1 file changed, 34 insertions(+)
>> >>  create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc
>> >> 
>> >> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc
>> >> new file mode 100644
>> >> index 000000000000..54e4800b8a13
>> >> --- /dev/null
>> >> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_opt_types.tc
>> >> @@ -0,0 +1,34 @@
>> >> +#!/bin/sh
>> >> +# SPDX-License-Identifier: GPL-2.0-or-later
>> >> +# Copyright (C) 2023 Akanksha J N, IBM corporation
>> >> +# description: Register/unregister optimized probe
>> >> +# requires: kprobe_events
>> >> +
>> >> +case `uname -m` in
>> >> +x86_64)
>> >> +;;
>> >> +arm*)
>> >> +;;
>> >> +ppc*)
>> >> +;;
>> >> +*)
>> >> +  echo "Please implement other architecture here"
>> >> +  exit_unsupported
>> >> +esac
>> >> +
>> >> +DEFAULT=$(cat /proc/sys/debug/kprobes-optimization)
>> >> +echo 1 > /proc/sys/debug/kprobes-optimization
>> >> +for i in `seq 0 255`; do
>> >> +        echo  "p:testprobe $FUNCTION_FORK+${i}" > kprobe_events || continue
>> >> +        echo 1 > events/kprobes/enable || continue
>> >> +        (echo "forked")
>> >> +        PROBE_TYPE=$(cat /sys/kernel/debug/kprobes/list | grep $FUNCTION_FORK | awk '{print $4}' | awk '{print substr($0,2,length($0)-2)}')
>> > 
>> > I think we can make it simply;
>> > 
>> > PROBE=$(grep $FUNCTION_FORK /sys/kernel/debug/kprobes/list)
>> > 
>> >> +        echo 0 > events/kprobes/enable
>> >> +        echo > kprobe_events
>> >> +        if [ $PROBE_TYPE = "OPTIMIZED" ]; then
>> > 
>> > and
>> > 
>> > if echo $PROBE | grep -q OPTIMIZED; then
>> > 
>> >> +                echo "$DEFAULT" >  /proc/sys/debug/kprobes-optimization
>> >> +                exit_pass
>> >> +        fi
>> >> +done
>> >> +echo "$DEFAULT" >  /proc/sys/debug/kprobes-optimization
>> >> +echo "Done"
>> > 
>> > Hmm, this test does NOT return any error. It always returns success.
>> 
>> Good catch!
>> 
>> > I understand that optimization may not be possible within 256 bytes
>> > from the beginning of the function.
>> 
>> Is that true in practice? Looking at x86 and ppc64le, it looks like we 
>> will almost always be able to optimize at least one of the instructions 
>> within the first 256 bytes of kernel_clone(). That's one of the primary 
>> purposes of this test.
> 
> Yeah, usually it should not happen. But since we don't disassemble it,
> we can not ensure that. So this depends on the compiler at last.

Ok.

> 
>> 
>> Are there valid reasons why we may not be able to optimize instructions?
> 
> For example, if the compiler starts inserting some checker instruction
> on each instruction boundary for security, it may prevent optimizing
> kprobes. Usually it should not happen (because it bloat up the kernel size)
> but we cannot deny the possibility of such new feature as an option
> in the future.
> 
>> 
>> > In that case, you can return
>> > "unresolved", and not echoing "Done" but the reason why it is
>> > unresolved.
> 
> Even in that case, it can notify such case as "unresolved", then we
> can notice it. (something like WARN_ON)

Sure, exiting as "Unresolved" should help point out a potential issue 
with optimizing probes, rather than labeling this as a failure.


Thanks,
Naveen


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

end of thread, other threads:[~2023-04-26 11:02 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-18  9:55 [PATCH v2 0/2] selftests/ftrace: Add tests for kprobes and optimized probes Akanksha J N
2023-04-18  9:55 ` [PATCH v2 1/2] selftests/ftrace: Add new test case which adds multiple consecutive probes in a function Akanksha J N
2023-04-25  0:12   ` Masami Hiramatsu
2023-04-18  9:55 ` [PATCH v2 2/2] selftests/ftrace: Add new test case which checks for optimized probes Akanksha J N
2023-04-25  0:10   ` Masami Hiramatsu
2023-04-25  5:28     ` Naveen N. Rao
2023-04-25 14:41       ` Masami Hiramatsu
2023-04-26 11:01         ` Naveen N. Rao
2023-04-24 11:09 ` [PATCH v2 0/2] selftests/ftrace: Add tests for kprobes and " Naveen N. Rao

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.