[2/2] selftests/ftrace: Update multiple kprobes test for powerpc

On Sat, 24 Jun 2017 02:30:21 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Thu, 22 Jun 2017 22:33:25 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > On 2017/06/22 06:07PM, Masami Hiramatsu wrote:
> > > On Thu, 22 Jun 2017 00:20:28 +0530
> > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > 
> > > > KPROBES_ON_FTRACE is only available on powerpc64le. Update comment to
> > > > clarify this.
> > > > 
> > > > Also, we should use an offset of 8 to ensure that the probe does not
> > > > fall on ftrace location. The current offset of 4 will fall before the
> > > > function local entry point and won't fire, while an offset of 12 or 16
> > > > will fall on ftrace location. Offset 8 is currently guaranteed to not be
> > > > the ftrace location.
> > > 
> > > OK, these part seems good to me.
> > > 
> > > > 
> > > > Finally, do not filter out symbols with a dot. Powerpc Elfv1 uses dot
> > > > prefix for all functions and this prevents us from testing some of those
> > > > symbols. Furthermore, with the patch to derive event names properly in
> > > > the presence of ':' and '.', such names are accepted by kprobe_events
> > > > and constitutes a good test for those symbols.
> > > 
> > > Hmm, the reason why I added such filter was to avoid symbols including
> > > gcc-generated suffixes like as .constprop or .isra etc.
> > 
> > I see.
> > 
> > I do wonder -- is there a problem if we try probing those symbols? On my 
> > local x86 vm, I don't see an issue probing it especially with the 
> > previous patch to enable probing with symbols having a '.' or ':'.
> > 
> > Furthermore, since this is for testing kprobe_events, I feel it is good 
> > to try probing those symbols too to catch any weird errors we may hit.
> 
> Yes, and that is not what this testcase is aiming to. That testcase should
> be a separated one, with correct error handling.

Hi Naveen,

Here is the testcase which I meant above. This may help if there is any
regression related to this specific issue.

Thank you,

-----

selftests: ftrace: Add a testcase for kprobe event naming

From: Masami Hiramatsu <mhiramat@kernel.org>

Add a testcase for kprobe event naming. This testcase
checks whether the kprobe events can automatically ganerate
its event name on normal function and dot-suffixed function.
Also it checks whether the kprobe events can correctly
define new event with given event name and group name.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 .../ftrace/test.d/kprobe/kprobe_eventname.tc       |   28 ++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc

On 2017/06/24 08:06PM, Masami Hiramatsu wrote:
> On Sat, 24 Jun 2017 02:30:21 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > On Thu, 22 Jun 2017 22:33:25 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> > > On 2017/06/22 06:07PM, Masami Hiramatsu wrote:
> > > > On Thu, 22 Jun 2017 00:20:28 +0530
> > > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > > 
> > > > > KPROBES_ON_FTRACE is only available on powerpc64le. Update comment to
> > > > > clarify this.
> > > > > 
> > > > > Also, we should use an offset of 8 to ensure that the probe does not
> > > > > fall on ftrace location. The current offset of 4 will fall before the
> > > > > function local entry point and won't fire, while an offset of 12 or 16
> > > > > will fall on ftrace location. Offset 8 is currently guaranteed to not be
> > > > > the ftrace location.
> > > > 
> > > > OK, these part seems good to me.
> > > > 
> > > > > 
> > > > > Finally, do not filter out symbols with a dot. Powerpc Elfv1 uses dot
> > > > > prefix for all functions and this prevents us from testing some of those
> > > > > symbols. Furthermore, with the patch to derive event names properly in
> > > > > the presence of ':' and '.', such names are accepted by kprobe_events
> > > > > and constitutes a good test for those symbols.
> > > > 
> > > > Hmm, the reason why I added such filter was to avoid symbols including
> > > > gcc-generated suffixes like as .constprop or .isra etc.
> > > 
> > > I see.
> > > 
> > > I do wonder -- is there a problem if we try probing those symbols? On my 
> > > local x86 vm, I don't see an issue probing it especially with the 
> > > previous patch to enable probing with symbols having a '.' or ':'.
> > > 
> > > Furthermore, since this is for testing kprobe_events, I feel it is good 
> > > to try probing those symbols too to catch any weird errors we may hit.
> > 
> > Yes, and that is not what this testcase is aiming to. That testcase should
> > be a separated one, with correct error handling.

Ok, I will re-send the patch by changing the pattern.

> 
> Hi Naveen,
> 
> Here is the testcase which I meant above. This may help if there is any
> regression related to this specific issue.

Nice!
I tested this and there are a few small issues on powerpc...

> 
> Thank you,
> 
> -----
> 
> selftests: ftrace: Add a testcase for kprobe event naming
> 
> From: Masami Hiramatsu <mhiramat@kernel.org>
> 
> Add a testcase for kprobe event naming. This testcase
> checks whether the kprobe events can automatically ganerate
> its event name on normal function and dot-suffixed function.
> Also it checks whether the kprobe events can correctly
> define new event with given event name and group name.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  .../ftrace/test.d/kprobe/kprobe_eventname.tc       |   28 ++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
> new file mode 100644
> index 0000000..d259031
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
> @@ -0,0 +1,28 @@
> +#!/bin/sh
> +# description: Kprobe event auto/manual naming
> +
> +disable_events
> +echo > kprobe_events
> +
> +:;: "Add an event on function without name" ;:
> +
> +FUNC=`grep -m 10 " [tT] [^.]*$" /proc/kallsyms | tail -n 1 | cut -f 3 -d " "`

On powerpc, this always ends up using a blacklisted function. So, I 
think we need a way to find a function that is not black listed.  
However, one of the issues is that debugfs does not show all the address 
ranges that are blacklisted. I am coming up with a way to address that 
and will post patches once I have it working.

With those patches, we should be able to select symbols that are not 
blacklisted.

> +echo p $FUNC > kprobe_events
> +test -d events/kprobes/p_${FUNC}_0 || exit_failure
> +
> +:;: "Add an event on function with new name" ;:
> +
> +echo p:event1 $FUNC > kprobe_events
> +test -d events/kprobes/event1 || exit_failure
> +
> +:;: "Add an event on function with new name and group" ;:
> +
> +echo p:kprobes2/event2 $FUNC > kprobe_events
> +test -d events/kprobes2/event2 || exit_failure
> +
> +:;: "Add an event on dot function without name" ;:
> +
> +FUNC=`grep -m 10 " [tT] .*\..*$" /proc/kallsyms | tail -n 1 | cut -f 3 -d " "`
> +echo p $FUNC > kprobe_events
> +EVENT=`grep $FUNC kprobe_events | cut -f 1 -d " " | cut -f 2 -d:` || exit_failure
> +test -d events/$EVENT || exit_failure

Probably good to add 'echo > kprobe_events' at the end just to clean up?


Thanks,
Naveen

On Wed, 28 Jun 2017 14:58:46 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> On 2017/06/24 08:06PM, Masami Hiramatsu wrote:
> > On Sat, 24 Jun 2017 02:30:21 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > > On Thu, 22 Jun 2017 22:33:25 +0530
> > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > 
> > > > On 2017/06/22 06:07PM, Masami Hiramatsu wrote:
> > > > > On Thu, 22 Jun 2017 00:20:28 +0530
> > > > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > > > 
> > > > > > KPROBES_ON_FTRACE is only available on powerpc64le. Update comment to
> > > > > > clarify this.
> > > > > > 
> > > > > > Also, we should use an offset of 8 to ensure that the probe does not
> > > > > > fall on ftrace location. The current offset of 4 will fall before the
> > > > > > function local entry point and won't fire, while an offset of 12 or 16
> > > > > > will fall on ftrace location. Offset 8 is currently guaranteed to not be
> > > > > > the ftrace location.
> > > > > 
> > > > > OK, these part seems good to me.
> > > > > 
> > > > > > 
> > > > > > Finally, do not filter out symbols with a dot. Powerpc Elfv1 uses dot
> > > > > > prefix for all functions and this prevents us from testing some of those
> > > > > > symbols. Furthermore, with the patch to derive event names properly in
> > > > > > the presence of ':' and '.', such names are accepted by kprobe_events
> > > > > > and constitutes a good test for those symbols.
> > > > > 
> > > > > Hmm, the reason why I added such filter was to avoid symbols including
> > > > > gcc-generated suffixes like as .constprop or .isra etc.
> > > > 
> > > > I see.
> > > > 
> > > > I do wonder -- is there a problem if we try probing those symbols? On my 
> > > > local x86 vm, I don't see an issue probing it especially with the 
> > > > previous patch to enable probing with symbols having a '.' or ':'.
> > > > 
> > > > Furthermore, since this is for testing kprobe_events, I feel it is good 
> > > > to try probing those symbols too to catch any weird errors we may hit.
> > > 
> > > Yes, and that is not what this testcase is aiming to. That testcase should
> > > be a separated one, with correct error handling.
> 
> Ok, I will re-send the patch by changing the pattern.
> 
> > 
> > Hi Naveen,
> > 
> > Here is the testcase which I meant above. This may help if there is any
> > regression related to this specific issue.
> 
> Nice!
> I tested this and there are a few small issues on powerpc...
> 
> > 
> > Thank you,
> > 
> > -----
> > 
> > selftests: ftrace: Add a testcase for kprobe event naming
> > 
> > From: Masami Hiramatsu <mhiramat@kernel.org>
> > 
> > Add a testcase for kprobe event naming. This testcase
> > checks whether the kprobe events can automatically ganerate
> > its event name on normal function and dot-suffixed function.
> > Also it checks whether the kprobe events can correctly
> > define new event with given event name and group name.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  .../ftrace/test.d/kprobe/kprobe_eventname.tc       |   28 ++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
> > 
> > diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
> > new file mode 100644
> > index 0000000..d259031
> > --- /dev/null
> > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
> > @@ -0,0 +1,28 @@
> > +#!/bin/sh
> > +# description: Kprobe event auto/manual naming
> > +
> > +disable_events
> > +echo > kprobe_events
> > +
> > +:;: "Add an event on function without name" ;:
> > +
> > +FUNC=`grep -m 10 " [tT] [^.]*$" /proc/kallsyms | tail -n 1 | cut -f 3 -d " "`
> 
> On powerpc, this always ends up using a blacklisted function. So, I 
> think we need a way to find a function that is not black listed.  

Hmm, if we increase the -m argument, like -m 100, is that still
in a blacklisted function?

> However, one of the issues is that debugfs does not show all the address 
> ranges that are blacklisted. I am coming up with a way to address that 
> and will post patches once I have it working.

Would you find that is only on powerpc or generic issue?

> 
> With those patches, we should be able to select symbols that are not 
> blacklisted.

Or, maybe we can use generic function, like "schedule" or "vfs_read"
etc.

> 
> > +echo p $FUNC > kprobe_events
> > +test -d events/kprobes/p_${FUNC}_0 || exit_failure
> > +
> > +:;: "Add an event on function with new name" ;:
> > +
> > +echo p:event1 $FUNC > kprobe_events
> > +test -d events/kprobes/event1 || exit_failure
> > +
> > +:;: "Add an event on function with new name and group" ;:
> > +
> > +echo p:kprobes2/event2 $FUNC > kprobe_events
> > +test -d events/kprobes2/event2 || exit_failure
> > +
> > +:;: "Add an event on dot function without name" ;:
> > +
> > +FUNC=`grep -m 10 " [tT] .*\..*$" /proc/kallsyms | tail -n 1 | cut -f 3 -d " "`
> > +echo p $FUNC > kprobe_events
> > +EVENT=`grep $FUNC kprobe_events | cut -f 1 -d " " | cut -f 2 -d:` || exit_failure
> > +test -d events/$EVENT || exit_failure
> 
> Probably good to add 'echo > kprobe_events' at the end just to clean up?

Ah, good catch!

Thanks!

> 
> 
> Thanks,
> Naveen
>

On 2017/06/28 11:16PM, Masami Hiramatsu wrote:
> > > diff --git 
> > > a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc 
> > > b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
> > > new file mode 100644
> > > index 0000000..d259031
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
> > > @@ -0,0 +1,28 @@
> > > +#!/bin/sh
> > > +# description: Kprobe event auto/manual naming
> > > +
> > > +disable_events
> > > +echo > kprobe_events
> > > +
> > > +:;: "Add an event on function without name" ;:
> > > +
> > > +FUNC=`grep -m 10 " [tT] [^.]*$" /proc/kallsyms | tail -n 1 | cut -f 3 -d " "`
> > 
> > On powerpc, this always ends up using a blacklisted function. So, I 
> > think we need a way to find a function that is not black listed.  
> 
> Hmm, if we increase the -m argument, like -m 100, is that still
> in a blacklisted function?

Yes, most of the initial symbols are exception vectors which are 
blacklisted.

> 
> > However, one of the issues is that debugfs does not show all the address 
> > ranges that are blacklisted. I am coming up with a way to address that 
> > and will post patches once I have it working.
> 
> Would you find that is only on powerpc or generic issue?

I meant the address _ranges_ that are blacklisted such as the ones with 
__kprobes annotation and __entry_text and so on.

> 
> > 
> > With those patches, we should be able to select symbols that are not 
> > blacklisted.
> 
> Or, maybe we can use generic function, like "schedule" or "vfs_read"
> etc.

Yes, I think this will be good for the generic test, but may not help 
selecting a dot symbol on powerpc.

Thanks,
Naveen

On Thu, 29 Jun 2017 00:13:24 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> On 2017/06/28 11:16PM, Masami Hiramatsu wrote:
> > > > diff --git 
> > > > a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc 
> > > > b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
> > > > new file mode 100644
> > > > index 0000000..d259031
> > > > --- /dev/null
> > > > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
> > > > @@ -0,0 +1,28 @@
> > > > +#!/bin/sh
> > > > +# description: Kprobe event auto/manual naming
> > > > +
> > > > +disable_events
> > > > +echo > kprobe_events
> > > > +
> > > > +:;: "Add an event on function without name" ;:
> > > > +
> > > > +FUNC=`grep -m 10 " [tT] [^.]*$" /proc/kallsyms | tail -n 1 | cut -f 3 -d " "`
> > > 
> > > On powerpc, this always ends up using a blacklisted function. So, I 
> > > think we need a way to find a function that is not black listed.  
> > 
> > Hmm, if we increase the -m argument, like -m 100, is that still
> > in a blacklisted function?
> 
> Yes, most of the initial symbols are exception vectors which are 
> blacklisted.

Hmm, then how about this? :)

grep _stext -A 1000 | grep -m 10 " [tT] [^.]*$" /proc/kallsyms

> 
> > 
> > > However, one of the issues is that debugfs does not show all the address 
> > > ranges that are blacklisted. I am coming up with a way to address that 
> > > and will post patches once I have it working.
> > 
> > Would you find that is only on powerpc or generic issue?
> 
> I meant the address _ranges_ that are blacklisted such as the ones with 
> __kprobes annotation and __entry_text and so on.

I see, but we can also check the address by comparing the address
of symbols, which also can be retrieved from kallsyms.
Since the test case is also applied to stable kernel, I don't want
to make it depending on some special kernel tweaks.

> > 
> > > 
> > > With those patches, we should be able to select symbols that are not 
> > > blacklisted.
> > 
> > Or, maybe we can use generic function, like "schedule" or "vfs_read"
> > etc.
> 
> Yes, I think this will be good for the generic test, but may not help 
> selecting a dot symbol on powerpc.

Right, and it depends on what gcc version and option is specified.
So, maybe we can skip the test if there is no such symbols.

I intended to test the symbols with some dot-suffix, but it seems
ppc64 has dot-started symbols, right? If there is no dot-suffixed
symbols, I think this test should skip the test case.

Thank you,

On 2017/06/29 09:57AM, Masami Hiramatsu wrote:
> On Thu, 29 Jun 2017 00:13:24 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > On 2017/06/28 11:16PM, Masami Hiramatsu wrote:
> > > > > diff --git 
> > > > > a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc 
> > > > > b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
> > > > > new file mode 100644
> > > > > index 0000000..d259031
> > > > > --- /dev/null
> > > > > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
> > > > > @@ -0,0 +1,28 @@
> > > > > +#!/bin/sh
> > > > > +# description: Kprobe event auto/manual naming
> > > > > +
> > > > > +disable_events
> > > > > +echo > kprobe_events
> > > > > +
> > > > > +:;: "Add an event on function without name" ;:
> > > > > +
> > > > > +FUNC=`grep -m 10 " [tT] [^.]*$" /proc/kallsyms | tail -n 1 | cut -f 3 -d " "`
> > > > 
> > > > On powerpc, this always ends up using a blacklisted function. So, I 
> > > > think we need a way to find a function that is not black listed.  
> > > 
> > > Hmm, if we increase the -m argument, like -m 100, is that still
> > > in a blacklisted function?
> > 
> > Yes, most of the initial symbols are exception vectors which are 
> > blacklisted.
> 
> Hmm, then how about this? :)
> 
> grep _stext -A 1000 | grep -m 10 " [tT] [^.]*$" /proc/kallsyms

That just happened to block the test ;)
I think your suggestion to use schedule/vfs_read is probably the best 
option. I will update your patch and post it.

> 
> > 
> > > 
> > > > However, one of the issues is that debugfs does not show all the address 
> > > > ranges that are blacklisted. I am coming up with a way to address that 
> > > > and will post patches once I have it working.
> > > 
> > > Would you find that is only on powerpc or generic issue?
> > 
> > I meant the address _ranges_ that are blacklisted such as the ones with 
> > __kprobes annotation and __entry_text and so on.
> 
> I see, but we can also check the address by comparing the address
> of symbols, which also can be retrieved from kallsyms.

Yes, but this is very platform specific and will result in a lot of 
checks, arm64 especially.

> Since the test case is also applied to stable kernel, I don't want
> to make it depending on some special kernel tweaks.

Sure, makes sense. For now, I think it's best to keep the test simple.  
But, we can enhance these later to consider the blacklist.

Thanks,
Naveen

> 
> > > 
> > > > 
> > > > With those patches, we should be able to select symbols that are not 
> > > > blacklisted.
> > > 
> > > Or, maybe we can use generic function, like "schedule" or "vfs_read"
> > > etc.
> > 
> > Yes, I think this will be good for the generic test, but may not help 
> > selecting a dot symbol on powerpc.
> 
> Right, and it depends on what gcc version and option is specified.
> So, maybe we can skip the test if there is no such symbols.
> 
> I intended to test the symbols with some dot-suffix, but it seems
> ppc64 has dot-started symbols, right? If there is no dot-suffixed
> symbols, I think this test should skip the test case.
> 
> Thank you,
> 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>
>