* Re: [PATCH] selftests/ftrace: Extend multiple_kprobes.tc to add multiple consecutive probes in a function [not found] <20230112095600.37665-1-akanksha@linux.ibm.com> @ 2023-01-12 13:21 ` Naveen N. Rao 2023-01-12 15:51 ` Masami Hiramatsu 0 siblings, 1 reply; 8+ messages in thread From: Naveen N. Rao @ 2023-01-12 13:21 UTC (permalink / raw) To: Akanksha J N, linux-kernel, linux-kselftest, linux-trace-kernel Cc: mhiramat, rostedt, shuah Akanksha J N 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. > Extend multiple kprobes test to add kprobes on first 256 bytes within a > function, to be able to test potential issues with kprobes on > successive instructions. > The '|| true' 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> > --- > .../selftests/ftrace/test.d/kprobe/multiple_kprobes.tc | 4 ++++ > 1 file changed, 4 insertions(+) Thanks for adding this test! > > diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc > index be754f5bcf79..f005c2542baa 100644 > --- a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc > @@ -25,6 +25,10 @@ if [ $L -ne 256 ]; then > exit_fail > fi > > +for i in `seq 0 255`; do > + echo p $FUNCTION_FORK+${i} >> kprobe_events || true > +done > + > cat kprobe_events >> $testlog > > echo 1 > events/kprobes/enable Thinking about this more, I wonder if we should add an explicit fork after enabling the events, similar to kprobe_args.tc: ( echo "forked" ) That will ensure we hit all the probes we added. With that change: Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> - Naveen ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] selftests/ftrace: Extend multiple_kprobes.tc to add multiple consecutive probes in a function 2023-01-12 13:21 ` [PATCH] selftests/ftrace: Extend multiple_kprobes.tc to add multiple consecutive probes in a function Naveen N. Rao @ 2023-01-12 15:51 ` Masami Hiramatsu 2023-01-13 9:29 ` Naveen N. Rao 0 siblings, 1 reply; 8+ messages in thread From: Masami Hiramatsu @ 2023-01-12 15:51 UTC (permalink / raw) To: Naveen N. Rao Cc: Akanksha J N, linux-kernel, linux-kselftest, linux-trace-kernel, mhiramat, rostedt, shuah On Thu, 12 Jan 2023 18:51:14 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > Akanksha J N 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. > > Extend multiple kprobes test to add kprobes on first 256 bytes within a > > function, to be able to test potential issues with kprobes on > > successive instructions. What is the purpose of that test? If you intended to add a kprobe events with some offset so that it becomes ftrace-based kprobe, it should be a different test case, because - This is a test case for checking multiple (at least 256) kprobe events can be defined and enabled. - If you want to check the ftrace-based kprobe, it should be near the function entry, maybe within 16 bytes or so. - Also, you don't need to enable it at once (and should not for this case). > > The '|| true' 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. Can you add another test case for that? (and send it to the MLs which Cc'd to this mail) e.g. for i in `seq 0 16`; do echo p:testprobe $FUNCTION_FORK+${i} >> kprobe_events || continue echo 1 > events/kprobes/testprobe/enable ( echo "forked" ) echo 0 > events/kprobes/testprobe/enable echo > kprobe_events done BTW, after we introduce the fprobe event (https://lore.kernel.org/linux-trace-kernel/166792255429.919356.14116090269057513181.stgit@devnote3/) that test case may be update to check fprobe events. Thank you, > > > > Signed-off-by: Akanksha J N <akanksha@linux.ibm.com> > > --- > > .../selftests/ftrace/test.d/kprobe/multiple_kprobes.tc | 4 ++++ > > 1 file changed, 4 insertions(+) > > Thanks for adding this test! > > > > > diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc > > index be754f5bcf79..f005c2542baa 100644 > > --- a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc > > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc > > @@ -25,6 +25,10 @@ if [ $L -ne 256 ]; then > > exit_fail > > fi > > > > +for i in `seq 0 255`; do > > + echo p $FUNCTION_FORK+${i} >> kprobe_events || true > > +done > > + > > cat kprobe_events >> $testlog > > > > echo 1 > events/kprobes/enable > > Thinking about this more, I wonder if we should add an explicit fork > after enabling the events, similar to kprobe_args.tc: > ( echo "forked" ) > > That will ensure we hit all the probes we added. With that change: > Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > > > - Naveen -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] selftests/ftrace: Extend multiple_kprobes.tc to add multiple consecutive probes in a function 2023-01-12 15:51 ` Masami Hiramatsu @ 2023-01-13 9:29 ` Naveen N. Rao 2023-01-13 15:21 ` Masami Hiramatsu 0 siblings, 1 reply; 8+ messages in thread From: Naveen N. Rao @ 2023-01-13 9:29 UTC (permalink / raw) To: Masami Hiramatsu Cc: Akanksha J N, linux-kernel, linux-kselftest, linux-trace-kernel, rostedt, shuah Masami Hiramatsu wrote: > On Thu, 12 Jan 2023 18:51:14 +0530 > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > >> Akanksha J N 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. >> > Extend multiple kprobes test to add kprobes on first 256 bytes within a >> > function, to be able to test potential issues with kprobes on >> > successive instructions. > > What is the purpose of that test? If you intended to add a kprobe events > with some offset so that it becomes ftrace-based kprobe, it should be > a different test case, because This is a follow up to: http://lore.kernel.org/1664530538.ke6dp49pwh.naveen@linux.ibm.com The intent is to add consecutive probes covering KPROBES_ON_FTRACE, vanilla trap-based kprobes as well as optprobes to ensure all of those and their interactions are good. > > - This is a test case for checking multiple (at least 256) kprobe events > can be defined and enabled. > > - If you want to check the ftrace-based kprobe, it should be near the > function entry, maybe within 16 bytes or so. > > - Also, you don't need to enable it at once (and should not for this case). > >> > The '|| true' 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. > > Can you add another test case for that? (and send it to the MLs which Cc'd > to this mail) > e.g. > > for i in `seq 0 16`; do > echo p:testprobe $FUNCTION_FORK+${i} >> kprobe_events || continue > echo 1 > events/kprobes/testprobe/enable > ( echo "forked" ) > echo 0 > events/kprobes/testprobe/enable > echo > kprobe_events > done The current test to add multiple kprobes within a function also falls under the purview of multiple_kprobes.tc, but it can be split into a separate multiple_kprobes_func.tc if you think that will be better. > > > BTW, after we introduce the fprobe event (https://lore.kernel.org/linux-trace-kernel/166792255429.919356.14116090269057513181.stgit@devnote3/) that test case may be > update to check fprobe events. Indeed, I suppose that can be a separate test. Thanks, Naveen > > Thank you, > >> > >> > Signed-off-by: Akanksha J N <akanksha@linux.ibm.com> >> > --- >> > .../selftests/ftrace/test.d/kprobe/multiple_kprobes.tc | 4 ++++ >> > 1 file changed, 4 insertions(+) >> >> Thanks for adding this test! >> >> > >> > diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc >> > index be754f5bcf79..f005c2542baa 100644 >> > --- a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc >> > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc >> > @@ -25,6 +25,10 @@ if [ $L -ne 256 ]; then >> > exit_fail >> > fi >> > >> > +for i in `seq 0 255`; do >> > + echo p $FUNCTION_FORK+${i} >> kprobe_events || true >> > +done >> > + >> > cat kprobe_events >> $testlog >> > >> > echo 1 > events/kprobes/enable >> >> Thinking about this more, I wonder if we should add an explicit fork >> after enabling the events, similar to kprobe_args.tc: >> ( echo "forked" ) >> >> That will ensure we hit all the probes we added. With that change: >> Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> >> >> >> - Naveen > > > -- > Masami Hiramatsu (Google) <mhiramat@kernel.org> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] selftests/ftrace: Extend multiple_kprobes.tc to add multiple consecutive probes in a function 2023-01-13 9:29 ` Naveen N. Rao @ 2023-01-13 15:21 ` Masami Hiramatsu 2023-01-16 8:32 ` Naveen N. Rao 0 siblings, 1 reply; 8+ messages in thread From: Masami Hiramatsu @ 2023-01-13 15:21 UTC (permalink / raw) To: Naveen N. Rao Cc: Akanksha J N, linux-kernel, linux-kselftest, linux-trace-kernel, rostedt, shuah Hi Naveen, On Fri, 13 Jan 2023 14:59:51 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > Masami Hiramatsu wrote: > > On Thu, 12 Jan 2023 18:51:14 +0530 > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > > > >> Akanksha J N 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. > >> > Extend multiple kprobes test to add kprobes on first 256 bytes within a > >> > function, to be able to test potential issues with kprobes on > >> > successive instructions. > > > > What is the purpose of that test? If you intended to add a kprobe events > > with some offset so that it becomes ftrace-based kprobe, it should be > > a different test case, because > > This is a follow up to: > http://lore.kernel.org/1664530538.ke6dp49pwh.naveen@linux.ibm.com > > The intent is to add consecutive probes covering KPROBES_ON_FTRACE, > vanilla trap-based kprobes as well as optprobes to ensure all of those > and their interactions are good. Hmm, that should be implemented for each architecture with specific knowledge instead of random offset, so that we can ensure the kprobe is on ftrace/optimized or using trap. Also, it should check the debugfs/kprobes/list file. > > > > > - This is a test case for checking multiple (at least 256) kprobe events > > can be defined and enabled. > > > > - If you want to check the ftrace-based kprobe, it should be near the > > function entry, maybe within 16 bytes or so. > > > > - Also, you don't need to enable it at once (and should not for this case). > > > >> > The '|| true' 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. > > > > Can you add another test case for that? (and send it to the MLs which Cc'd > > to this mail) > > e.g. > > > > for i in `seq 0 16`; do > > echo p:testprobe $FUNCTION_FORK+${i} >> kprobe_events || continue > > echo 1 > events/kprobes/testprobe/enable > > ( echo "forked" ) > > echo 0 > events/kprobes/testprobe/enable > > echo > kprobe_events > > done > > The current test to add multiple kprobes within a function also falls > under the purview of multiple_kprobes.tc, but it can be split into a > separate multiple_kprobes_func.tc if you think that will be better. > Yes, please make it separate, this test case is for checking whether the ftrace can define/enable/disable multiple kprobe events. Not for checking kprobe with different types, nor checking interactions among different types of kprobes. (BTW, if you want to test optprobe on x86, you can not put the probes within the jump instruction (+5 bytes). It will unoptimize existing optimized kprobe in that case) And do you really need to run "multiple" kprobes at once? I think what you need is 'kprobe_opt_types.tc'. > > > > > > BTW, after we introduce the fprobe event (https://lore.kernel.org/linux-trace-kernel/166792255429.919356.14116090269057513181.stgit@devnote3/) that test case may be > > update to check fprobe events. > > Indeed, I suppose that can be a separate test. Thank you, > > > Thanks, > Naveen > > > > > Thank you, > > > >> > > >> > Signed-off-by: Akanksha J N <akanksha@linux.ibm.com> > >> > --- > >> > .../selftests/ftrace/test.d/kprobe/multiple_kprobes.tc | 4 ++++ > >> > 1 file changed, 4 insertions(+) > >> > >> Thanks for adding this test! > >> > >> > > >> > diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc > >> > index be754f5bcf79..f005c2542baa 100644 > >> > --- a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc > >> > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc > >> > @@ -25,6 +25,10 @@ if [ $L -ne 256 ]; then > >> > exit_fail > >> > fi > >> > > >> > +for i in `seq 0 255`; do > >> > + echo p $FUNCTION_FORK+${i} >> kprobe_events || true > >> > +done > >> > + > >> > cat kprobe_events >> $testlog > >> > > >> > echo 1 > events/kprobes/enable > >> > >> Thinking about this more, I wonder if we should add an explicit fork > >> after enabling the events, similar to kprobe_args.tc: > >> ( echo "forked" ) > >> > >> That will ensure we hit all the probes we added. With that change: > >> Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > >> > >> > >> - Naveen > > > > > > -- > > Masami Hiramatsu (Google) <mhiramat@kernel.org> > > -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] selftests/ftrace: Extend multiple_kprobes.tc to add multiple consecutive probes in a function 2023-01-13 15:21 ` Masami Hiramatsu @ 2023-01-16 8:32 ` Naveen N. Rao 2023-01-19 23:55 ` Masami Hiramatsu 0 siblings, 1 reply; 8+ messages in thread From: Naveen N. Rao @ 2023-01-16 8:32 UTC (permalink / raw) To: Masami Hiramatsu Cc: Akanksha J N, linux-kernel, linux-kselftest, linux-trace-kernel, rostedt, shuah Masami Hiramatsu wrote: > Hi Naveen, > > On Fri, 13 Jan 2023 14:59:51 +0530 > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > >> Masami Hiramatsu wrote: >> > On Thu, 12 Jan 2023 18:51:14 +0530 >> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: >> > >> >> Akanksha J N 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. >> >> > Extend multiple kprobes test to add kprobes on first 256 bytes within a >> >> > function, to be able to test potential issues with kprobes on >> >> > successive instructions. >> > >> > What is the purpose of that test? If you intended to add a kprobe events >> > with some offset so that it becomes ftrace-based kprobe, it should be >> > a different test case, because >> >> This is a follow up to: >> http://lore.kernel.org/1664530538.ke6dp49pwh.naveen@linux.ibm.com >> >> The intent is to add consecutive probes covering KPROBES_ON_FTRACE, >> vanilla trap-based kprobes as well as optprobes to ensure all of those >> and their interactions are good. > > Hmm, that should be implemented for each architecture with specific > knowledge instead of random offset, so that we can ensure the kprobe > is on ftrace/optimized or using trap. Also, it should check the > debugfs/kprobes/list file. ... > >> >> > >> > - This is a test case for checking multiple (at least 256) kprobe events >> > can be defined and enabled. >> > >> > - If you want to check the ftrace-based kprobe, it should be near the >> > function entry, maybe within 16 bytes or so. >> > >> > - Also, you don't need to enable it at once (and should not for this case). >> > >> >> > The '|| true' 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. >> > >> > Can you add another test case for that? (and send it to the MLs which Cc'd >> > to this mail) >> > e.g. >> > >> > for i in `seq 0 16`; do >> > echo p:testprobe $FUNCTION_FORK+${i} >> kprobe_events || continue >> > echo 1 > events/kprobes/testprobe/enable >> > ( echo "forked" ) >> > echo 0 > events/kprobes/testprobe/enable >> > echo > kprobe_events >> > done >> >> The current test to add multiple kprobes within a function also falls >> under the purview of multiple_kprobes.tc, but it can be split into a >> separate multiple_kprobes_func.tc if you think that will be better. >> > > Yes, please make it separate, this test case is for checking whether > the ftrace can define/enable/disable multiple kprobe events. Not for > checking kprobe with different types, nor checking interactions among > different types of kprobes. > > (BTW, if you want to test optprobe on x86, you can not put the probes > within the jump instruction (+5 bytes). It will unoptimize existing > optimized kprobe in that case) Ok, I can see why we won't be able to optimize any of the probes on x86 with this approach. But, we should be able to do so on powerpc and arm, the only other architectures supporting OPTPROBES at this time. For x86, we may have to extend the test to check kprobes/list. Crucially, I think trying to place a probe at each byte can still exercize interactions across KPROBES_ON_FTRACE and normal kprobes, so this test is still a good start. In addition, we get to ensure that kprobes infrastructure is rejecting placing probes at non-instruction boundaries. > > And do you really need to run "multiple" kprobes at once? > I think what you need is 'kprobe_opt_types.tc'. Yes, enabling those probes is a good stress test to ensure we are only accepting valid probe locations. multiple_kprobe_types.tc ? :) Thanks, Naveen ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] selftests/ftrace: Extend multiple_kprobes.tc to add multiple consecutive probes in a function 2023-01-16 8:32 ` Naveen N. Rao @ 2023-01-19 23:55 ` Masami Hiramatsu 2023-01-25 7:09 ` Naveen N. Rao 0 siblings, 1 reply; 8+ messages in thread From: Masami Hiramatsu @ 2023-01-19 23:55 UTC (permalink / raw) To: Naveen N. Rao Cc: Akanksha J N, linux-kernel, linux-kselftest, linux-trace-kernel, rostedt, shuah Hi Naveen, On Mon, 16 Jan 2023 14:02:04 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > Masami Hiramatsu wrote: > > Hi Naveen, > > > > On Fri, 13 Jan 2023 14:59:51 +0530 > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > > > >> Masami Hiramatsu wrote: > >> > On Thu, 12 Jan 2023 18:51:14 +0530 > >> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > >> > > >> >> Akanksha J N 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. > >> >> > Extend multiple kprobes test to add kprobes on first 256 bytes within a > >> >> > function, to be able to test potential issues with kprobes on > >> >> > successive instructions. > >> > > >> > What is the purpose of that test? If you intended to add a kprobe events > >> > with some offset so that it becomes ftrace-based kprobe, it should be > >> > a different test case, because > >> > >> This is a follow up to: > >> http://lore.kernel.org/1664530538.ke6dp49pwh.naveen@linux.ibm.com > >> > >> The intent is to add consecutive probes covering KPROBES_ON_FTRACE, > >> vanilla trap-based kprobes as well as optprobes to ensure all of those > >> and their interactions are good. > > > > Hmm, that should be implemented for each architecture with specific > > knowledge instead of random offset, so that we can ensure the kprobe > > is on ftrace/optimized or using trap. Also, it should check the > > debugfs/kprobes/list file. > > ... > > > > >> > >> > > >> > - This is a test case for checking multiple (at least 256) kprobe events > >> > can be defined and enabled. > >> > > >> > - If you want to check the ftrace-based kprobe, it should be near the > >> > function entry, maybe within 16 bytes or so. > >> > > >> > - Also, you don't need to enable it at once (and should not for this case). > >> > > >> >> > The '|| true' 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. > >> > > >> > Can you add another test case for that? (and send it to the MLs which Cc'd > >> > to this mail) > >> > e.g. > >> > > >> > for i in `seq 0 16`; do > >> > echo p:testprobe $FUNCTION_FORK+${i} >> kprobe_events || continue > >> > echo 1 > events/kprobes/testprobe/enable > >> > ( echo "forked" ) > >> > echo 0 > events/kprobes/testprobe/enable > >> > echo > kprobe_events > >> > done > >> > >> The current test to add multiple kprobes within a function also falls > >> under the purview of multiple_kprobes.tc, but it can be split into a > >> separate multiple_kprobes_func.tc if you think that will be better. > >> > > > > Yes, please make it separate, this test case is for checking whether > > the ftrace can define/enable/disable multiple kprobe events. Not for > > checking kprobe with different types, nor checking interactions among > > different types of kprobes. > > > > (BTW, if you want to test optprobe on x86, you can not put the probes > > within the jump instruction (+5 bytes). It will unoptimize existing > > optimized kprobe in that case) > > Ok, I can see why we won't be able to optimize any of the probes on x86 > with this approach. But, we should be able to do so on powerpc and arm, > the only other architectures supporting OPTPROBES at this time. For x86, > we may have to extend the test to check kprobes/list. Are there any instruction type specific limitation on those arch for using optprobe? I guess the 'call' (branch with link register) will not able to be optimized because it leaves the trampoline address on the stack. > > Crucially, I think trying to place a probe at each byte can still > exercize interactions across KPROBES_ON_FTRACE and normal kprobes, so > this test is still a good start. In addition, we get to ensure that > kprobes infrastructure is rejecting placing probes at non-instruction > boundaries. The interfere between probes can be happen between kprobes and optprobe (*only on x86*), but not with KPORBES_ON_FTRACE. The ftrace replaced NOP will be handled as one instruction. > > And do you really need to run "multiple" kprobes at once? > > I think what you need is 'kprobe_opt_types.tc'. > > Yes, enabling those probes is a good stress test to ensure we are only > accepting valid probe locations. > > multiple_kprobe_types.tc ? :) Please don't mixed it with the concept of 'multiple' probe test. It is different that - kprobes can put probes on each instruction boundary. - kprobes can allocate and enable multiple probes at the same time. What the multiple_kprobes.tc tests is the latter one. (This is the reason why it chooses different functions so as not to interfere with each other.) Thank you, > > > Thanks, > Naveen > -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] selftests/ftrace: Extend multiple_kprobes.tc to add multiple consecutive probes in a function 2023-01-19 23:55 ` Masami Hiramatsu @ 2023-01-25 7:09 ` Naveen N. Rao 2023-01-28 1:16 ` Masami Hiramatsu 0 siblings, 1 reply; 8+ messages in thread From: Naveen N. Rao @ 2023-01-25 7:09 UTC (permalink / raw) To: Masami Hiramatsu Cc: Akanksha J N, linux-kernel, linux-kselftest, linux-trace-kernel, rostedt, shuah Hi Masami, Masami Hiramatsu wrote: >> > >> > Yes, please make it separate, this test case is for checking whether >> > the ftrace can define/enable/disable multiple kprobe events. Not for >> > checking kprobe with different types, nor checking interactions among >> > different types of kprobes. >> > >> > (BTW, if you want to test optprobe on x86, you can not put the probes >> > within the jump instruction (+5 bytes). It will unoptimize existing >> > optimized kprobe in that case) >> >> Ok, I can see why we won't be able to optimize any of the probes on x86 >> with this approach. But, we should be able to do so on powerpc and arm, >> the only other architectures supporting OPTPROBES at this time. For x86, >> we may have to extend the test to check kprobes/list. > > Are there any instruction type specific limitation on those arch for > using optprobe? I guess the 'call' (branch with link register) will not > able to be optimized because it leaves the trampoline address on the > stack. Yes, at least on powerpc, we only optimize ALU instructions and do not optimize load/store instructions, among many others. This is the reason we try to put a probe uptil 256 offset into a function in the proposed test, which will almost certainly catch an instruction that can be optimized. > >> >> Crucially, I think trying to place a probe at each byte can still >> exercize interactions across KPROBES_ON_FTRACE and normal kprobes, so >> this test is still a good start. In addition, we get to ensure that >> kprobes infrastructure is rejecting placing probes at non-instruction >> boundaries. > > The interfere between probes can be happen between kprobes and optprobe > (*only on x86*), but not with KPORBES_ON_FTRACE. The ftrace replaced NOP > will be handled as one instruction. Yes. > >> > And do you really need to run "multiple" kprobes at once? >> > I think what you need is 'kprobe_opt_types.tc'. >> >> Yes, enabling those probes is a good stress test to ensure we are only >> accepting valid probe locations. >> >> multiple_kprobe_types.tc ? :) > > Please don't mixed it with the concept of 'multiple' probe test. > It is different that > - kprobes can put probes on each instruction boundary. > - kprobes can allocate and enable multiple probes at the same time. > > What the multiple_kprobes.tc tests is the latter one. > (This is the reason why it chooses different functions so as not to > interfere with each other.) Ok, I was coming from the point of view that both tests end up installing "multiple" kprobes, but I do see your point. How about adding two new tests: 1. The same test as has been proposed in this thread: trying to add a kprobe at every byte within $FUNCTION_FORK upto an offset of 256 bytes. We can probably call it kprobe_insn_boundary.tc 2. A new test to ensure we can add different kprobe types (kprobe_opt_types.tc). This test will need to enable and check if each probe has been optimized or not and needs arch-specific knowledge so that we can take care of x86. Would that be ok? Thanks, Naveen ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] selftests/ftrace: Extend multiple_kprobes.tc to add multiple consecutive probes in a function 2023-01-25 7:09 ` Naveen N. Rao @ 2023-01-28 1:16 ` Masami Hiramatsu 0 siblings, 0 replies; 8+ messages in thread From: Masami Hiramatsu @ 2023-01-28 1:16 UTC (permalink / raw) To: Naveen N. Rao Cc: Akanksha J N, linux-kernel, linux-kselftest, linux-trace-kernel, rostedt, shuah On Wed, 25 Jan 2023 12:39:36 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > Hi Masami, > > Masami Hiramatsu wrote: > >> > > >> > Yes, please make it separate, this test case is for checking whether > >> > the ftrace can define/enable/disable multiple kprobe events. Not for > >> > checking kprobe with different types, nor checking interactions among > >> > different types of kprobes. > >> > > >> > (BTW, if you want to test optprobe on x86, you can not put the probes > >> > within the jump instruction (+5 bytes). It will unoptimize existing > >> > optimized kprobe in that case) > >> > >> Ok, I can see why we won't be able to optimize any of the probes on x86 > >> with this approach. But, we should be able to do so on powerpc and arm, > >> the only other architectures supporting OPTPROBES at this time. For x86, > >> we may have to extend the test to check kprobes/list. > > > > Are there any instruction type specific limitation on those arch for > > using optprobe? I guess the 'call' (branch with link register) will not > > able to be optimized because it leaves the trampoline address on the > > stack. > > Yes, at least on powerpc, we only optimize ALU instructions and do not > optimize load/store instructions, among many others. This is the reason > we try to put a probe uptil 256 offset into a function in the proposed > test, which will almost certainly catch an instruction that can be > optimized. > > > > >> > >> Crucially, I think trying to place a probe at each byte can still > >> exercize interactions across KPROBES_ON_FTRACE and normal kprobes, so > >> this test is still a good start. In addition, we get to ensure that > >> kprobes infrastructure is rejecting placing probes at non-instruction > >> boundaries. > > > > The interfere between probes can be happen between kprobes and optprobe > > (*only on x86*), but not with KPORBES_ON_FTRACE. The ftrace replaced NOP > > will be handled as one instruction. > > Yes. > > > > >> > And do you really need to run "multiple" kprobes at once? > >> > I think what you need is 'kprobe_opt_types.tc'. > >> > >> Yes, enabling those probes is a good stress test to ensure we are only > >> accepting valid probe locations. > >> > >> multiple_kprobe_types.tc ? :) > > > > Please don't mixed it with the concept of 'multiple' probe test. > > It is different that > > - kprobes can put probes on each instruction boundary. > > - kprobes can allocate and enable multiple probes at the same time. > > > > What the multiple_kprobes.tc tests is the latter one. > > (This is the reason why it chooses different functions so as not to > > interfere with each other.) > > Ok, I was coming from the point of view that both tests end up > installing "multiple" kprobes, but I do see your point. > > How about adding two new tests: > 1. The same test as has been proposed in this thread: trying to add a > kprobe at every byte within $FUNCTION_FORK upto an offset of 256 bytes. > We can probably call it kprobe_insn_boundary.tc OK. > 2. A new test to ensure we can add different kprobe types > (kprobe_opt_types.tc). This test will need to enable and check if each > probe has been optimized or not and needs arch-specific knowledge so > that we can take care of x86. OK, this should be only for x86. > > Would that be ok? Yes, this sounds good to me. Thank you! > > > Thanks, > Naveen > -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-01-28 1:16 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20230112095600.37665-1-akanksha@linux.ibm.com> 2023-01-12 13:21 ` [PATCH] selftests/ftrace: Extend multiple_kprobes.tc to add multiple consecutive probes in a function Naveen N. Rao 2023-01-12 15:51 ` Masami Hiramatsu 2023-01-13 9:29 ` Naveen N. Rao 2023-01-13 15:21 ` Masami Hiramatsu 2023-01-16 8:32 ` Naveen N. Rao 2023-01-19 23:55 ` Masami Hiramatsu 2023-01-25 7:09 ` Naveen N. Rao 2023-01-28 1:16 ` Masami Hiramatsu
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.