linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] A couple of small updates/fixes for kprobes tracer
@ 2017-06-21 18:50 Naveen N. Rao
  2017-06-21 18:50 ` [PATCH 1/2] trace/kprobes: Sanitize derived event names Naveen N. Rao
  2017-06-21 18:50 ` [PATCH 2/2] selftests/ftrace: Update multiple kprobes test for powerpc Naveen N. Rao
  0 siblings, 2 replies; 15+ messages in thread
From: Naveen N. Rao @ 2017-06-21 18:50 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Ananth N Mavinakayanahalli, Masami Hiramatsu, Michael Ellerman,
	linux-kernel, linuxppc-dev

Two simple updates for kprobes tracer:
- the first patch is a convenience and allows to probe module symbols
  as well as any dot symbols (necessary on powerpc64 elfv1) without
  having to provide a name for the probepoint.
- the second patch updates the newly added multiple_kprobes.tc test
  case for powerpc.

Thanks,
Naveen

Naveen N. Rao (2):
  trace/kprobes: Sanitize derived event names
  selftests/ftrace: Update multiple kprobes test for powerpc

 kernel/trace/trace_kprobe.c                                      | 9 +++++++++
 tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc | 8 ++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

-- 
2.13.1

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

* [PATCH 1/2] trace/kprobes: Sanitize derived event names
  2017-06-21 18:50 [PATCH 0/2] A couple of small updates/fixes for kprobes tracer Naveen N. Rao
@ 2017-06-21 18:50 ` Naveen N. Rao
  2017-06-22  9:29   ` Masami Hiramatsu
  2017-06-21 18:50 ` [PATCH 2/2] selftests/ftrace: Update multiple kprobes test for powerpc Naveen N. Rao
  1 sibling, 1 reply; 15+ messages in thread
From: Naveen N. Rao @ 2017-06-21 18:50 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Ananth N Mavinakayanahalli, Masami Hiramatsu, Michael Ellerman,
	linux-kernel, linuxppc-dev

When we derive event names, convert some expected symbols (such as ':'
used to specify module:name and '.' present in some symbols) into
underscores so that the event name is not rejected.

Before this patch:
    # echo 'p kobject_example:foo_store' > kprobe_events
    trace_kprobe: Failed to allocate trace_probe.(-22)
    -sh: write error: Invalid argument

After this patch:
    # echo 'p kobject_example:foo_store' > kprobe_events
    # cat kprobe_events
    p:kprobes/p_kobject_example_foo_store_0 kobject_example:foo_store

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 kernel/trace/trace_kprobe.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index c129fca6ec99..44fd819aa33d 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -598,6 +598,14 @@ static struct notifier_block trace_kprobe_module_nb = {
 	.priority = 1	/* Invoked after kprobe module callback */
 };
 
+/* Convert certain expected symbols into '_' when generating event names */
+static inline void sanitize_event_name(char *name)
+{
+	while (*name++ != '\0')
+		if (*name == ':' || *name == '.')
+			*name = '_';
+}
+
 static int create_trace_kprobe(int argc, char **argv)
 {
 	/*
@@ -740,6 +748,7 @@ static int create_trace_kprobe(int argc, char **argv)
 		else
 			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_0x%p",
 				 is_return ? 'r' : 'p', addr);
+		sanitize_event_name(buf);
 		event = buf;
 	}
 	tk = alloc_trace_kprobe(group, event, addr, symbol, offset, maxactive,
-- 
2.13.1

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

* [PATCH 2/2] selftests/ftrace: Update multiple kprobes test for powerpc
  2017-06-21 18:50 [PATCH 0/2] A couple of small updates/fixes for kprobes tracer Naveen N. Rao
  2017-06-21 18:50 ` [PATCH 1/2] trace/kprobes: Sanitize derived event names Naveen N. Rao
@ 2017-06-21 18:50 ` Naveen N. Rao
  2017-06-22  9:07   ` Masami Hiramatsu
  1 sibling, 1 reply; 15+ messages in thread
From: Naveen N. Rao @ 2017-06-21 18:50 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Ananth N Mavinakayanahalli, Masami Hiramatsu, Michael Ellerman,
	linux-kernel, linuxppc-dev

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.

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.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

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 f4d1ff785d67..d209c071b2c0 100644
--- a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
@@ -2,16 +2,16 @@
 # description: Register/unregister many kprobe events
 
 # ftrace fentry skip size depends on the machine architecture.
-# Currently HAVE_KPROBES_ON_FTRACE defined on x86 and powerpc
+# Currently HAVE_KPROBES_ON_FTRACE defined on x86 and powerpc64le
 case `uname -m` in
   x86_64|i[3456]86) OFFS=5;;
-  ppc*) OFFS=4;;
+  ppc64le) OFFS=8;;
   *) OFFS=0;;
 esac
 
 echo "Setup up to 256 kprobes"
-grep t /proc/kallsyms | cut -f3 -d" " | grep -v .*\\..* | \
-head -n 256 | while read i; do echo p ${i}+${OFFS} ; done > kprobe_events ||:
+grep t /proc/kallsyms | cut -f3 -d" " | head -n 256 | \
+while read i; do echo p ${i}+${OFFS} ; done > kprobe_events ||:
 
 echo 1 > events/kprobes/enable
 echo 0 > events/kprobes/enable
-- 
2.13.1

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

* Re: [PATCH 2/2] selftests/ftrace: Update multiple kprobes test for powerpc
  2017-06-21 18:50 ` [PATCH 2/2] selftests/ftrace: Update multiple kprobes test for powerpc Naveen N. Rao
@ 2017-06-22  9:07   ` Masami Hiramatsu
  2017-06-22 17:03     ` Naveen N. Rao
  0 siblings, 1 reply; 15+ messages in thread
From: Masami Hiramatsu @ 2017-06-22  9:07 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Steven Rostedt, Ananth N Mavinakayanahalli, Masami Hiramatsu,
	Michael Ellerman, linux-kernel, linuxppc-dev

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.
So if the Powerpc Elfv1 use dot prefix, that is OK, in that case,
could you update the filter as "^.*\\..*" ?

Thank you,

> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> 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 f4d1ff785d67..d209c071b2c0 100644
> --- a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
> @@ -2,16 +2,16 @@
>  # description: Register/unregister many kprobe events
>  
>  # ftrace fentry skip size depends on the machine architecture.
> -# Currently HAVE_KPROBES_ON_FTRACE defined on x86 and powerpc
> +# Currently HAVE_KPROBES_ON_FTRACE defined on x86 and powerpc64le
>  case `uname -m` in
>    x86_64|i[3456]86) OFFS=5;;
> -  ppc*) OFFS=4;;
> +  ppc64le) OFFS=8;;
>    *) OFFS=0;;
>  esac
>  
>  echo "Setup up to 256 kprobes"
> -grep t /proc/kallsyms | cut -f3 -d" " | grep -v .*\\..* | \
> -head -n 256 | while read i; do echo p ${i}+${OFFS} ; done > kprobe_events ||:
> +grep t /proc/kallsyms | cut -f3 -d" " | head -n 256 | \
> +while read i; do echo p ${i}+${OFFS} ; done > kprobe_events ||:
>  
>  echo 1 > events/kprobes/enable
>  echo 0 > events/kprobes/enable
> -- 
> 2.13.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

* Re: [PATCH 1/2] trace/kprobes: Sanitize derived event names
  2017-06-21 18:50 ` [PATCH 1/2] trace/kprobes: Sanitize derived event names Naveen N. Rao
@ 2017-06-22  9:29   ` Masami Hiramatsu
  2017-06-22 19:03     ` Naveen N. Rao
  0 siblings, 1 reply; 15+ messages in thread
From: Masami Hiramatsu @ 2017-06-22  9:29 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Steven Rostedt, Ananth N Mavinakayanahalli, Masami Hiramatsu,
	Michael Ellerman, linux-kernel, linuxppc-dev

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

> When we derive event names, convert some expected symbols (such as ':'
> used to specify module:name and '.' present in some symbols) into
> underscores so that the event name is not rejected.

Oops, ok, this is my mistake.

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

This must be marked as bugfix for stable trees.

Could you also add a testcase for this (module name) bug?

MODNAME=`lsmod | head -n 2 | tail -n 1 | cut -f 1 -d " "`
FUNCNAME=`grep -m 1 "\\[$MODNAME\\]" /proc/kallsyms | xargs | cut -f 3 -d " "`

May gives you a target name :)

Thank you,

> 
> Before this patch:
>     # echo 'p kobject_example:foo_store' > kprobe_events
>     trace_kprobe: Failed to allocate trace_probe.(-22)
>     -sh: write error: Invalid argument
> 
> After this patch:
>     # echo 'p kobject_example:foo_store' > kprobe_events
>     # cat kprobe_events
>     p:kprobes/p_kobject_example_foo_store_0 kobject_example:foo_store
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  kernel/trace/trace_kprobe.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index c129fca6ec99..44fd819aa33d 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -598,6 +598,14 @@ static struct notifier_block trace_kprobe_module_nb = {
>  	.priority = 1	/* Invoked after kprobe module callback */
>  };
>  
> +/* Convert certain expected symbols into '_' when generating event names */
> +static inline void sanitize_event_name(char *name)
> +{
> +	while (*name++ != '\0')
> +		if (*name == ':' || *name == '.')
> +			*name = '_';
> +}
> +
>  static int create_trace_kprobe(int argc, char **argv)
>  {
>  	/*
> @@ -740,6 +748,7 @@ static int create_trace_kprobe(int argc, char **argv)
>  		else
>  			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_0x%p",
>  				 is_return ? 'r' : 'p', addr);
> +		sanitize_event_name(buf);
>  		event = buf;
>  	}
>  	tk = alloc_trace_kprobe(group, event, addr, symbol, offset, maxactive,
> -- 
> 2.13.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

* Re: [PATCH 2/2] selftests/ftrace: Update multiple kprobes test for powerpc
  2017-06-22  9:07   ` Masami Hiramatsu
@ 2017-06-22 17:03     ` Naveen N. Rao
  2017-06-23 17:30       ` Masami Hiramatsu
  0 siblings, 1 reply; 15+ messages in thread
From: Naveen N. Rao @ 2017-06-22 17:03 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Steven Rostedt, Ananth N Mavinakayanahalli, Michael Ellerman,
	linux-kernel, linuxppc-dev

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.

Thanks for the review!
- Naveen


> So if the Powerpc Elfv1 use dot prefix, that is OK, in that case,
> could you update the filter as "^.*\\..*" ?
> 
> Thank you,
> 
> > 
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > 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 f4d1ff785d67..d209c071b2c0 100644
> > --- a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
> > @@ -2,16 +2,16 @@
> >  # description: Register/unregister many kprobe events
> >  
> >  # ftrace fentry skip size depends on the machine architecture.
> > -# Currently HAVE_KPROBES_ON_FTRACE defined on x86 and powerpc
> > +# Currently HAVE_KPROBES_ON_FTRACE defined on x86 and powerpc64le
> >  case `uname -m` in
> >    x86_64|i[3456]86) OFFS=5;;
> > -  ppc*) OFFS=4;;
> > +  ppc64le) OFFS=8;;
> >    *) OFFS=0;;
> >  esac
> >  
> >  echo "Setup up to 256 kprobes"
> > -grep t /proc/kallsyms | cut -f3 -d" " | grep -v .*\\..* | \
> > -head -n 256 | while read i; do echo p ${i}+${OFFS} ; done > kprobe_events ||:
> > +grep t /proc/kallsyms | cut -f3 -d" " | head -n 256 | \
> > +while read i; do echo p ${i}+${OFFS} ; done > kprobe_events ||:
> >  
> >  echo 1 > events/kprobes/enable
> >  echo 0 > events/kprobes/enable
> > -- 
> > 2.13.1
> > 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>
> 

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

* Re: [PATCH 1/2] trace/kprobes: Sanitize derived event names
  2017-06-22  9:29   ` Masami Hiramatsu
@ 2017-06-22 19:03     ` Naveen N. Rao
  2017-06-23 17:30       ` Masami Hiramatsu
  0 siblings, 1 reply; 15+ messages in thread
From: Naveen N. Rao @ 2017-06-22 19:03 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Steven Rostedt, Ananth N Mavinakayanahalli, Michael Ellerman,
	linux-kernel, linuxppc-dev

On 2017/06/22 06:29PM, Masami Hiramatsu wrote:
> On Thu, 22 Jun 2017 00:20:27 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > When we derive event names, convert some expected symbols (such as ':'
> > used to specify module:name and '.' present in some symbols) into
> > underscores so that the event name is not rejected.
> 
> Oops, ok, this is my mistake.
> 
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
> 
> This must be marked as bugfix for stable trees.
> 
> Could you also add a testcase for this (module name) bug?
> 
> MODNAME=`lsmod | head -n 2 | tail -n 1 | cut -f 1 -d " "`
> FUNCNAME=`grep -m 1 "\\[$MODNAME\\]" /proc/kallsyms | xargs | cut -f 3 -d " "`
> 
> May gives you a target name :)

Sure. Here is a test.

Thanks for the review,
Naveen

-
[PATCH] selftests/ftrace: Add a test to probe module functions

Add a kprobes test to ensure that we are able to add a probe on a
module function using 'p <mod>:<func>' format, without having to
specify a probe name.

Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 .../testing/selftests/ftrace/test.d/kprobe/probe_module.tc | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc b/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc
new file mode 100644
index 000000000000..ea7657041ba6
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc
@@ -0,0 +1,14 @@
+#!/bin/sh
+# description: Kprobe dynamic event - probing module
+
+[ -f kprobe_events ] || exit_unsupported # this is configurable
+
+echo 0 > events/enable
+echo > kprobe_events
+export MOD=`lsmod | head -n 2 | tail -n 1 | cut -f1 -d" "`
+export FUNC=`grep -m 1 ".* t .*\\[$MOD\\]" /proc/kallsyms | xargs | cut -f3 -d" "`
+[ "x" != "x$MOD" -a "y" != "y$FUNC" ] || exit_untested
+echo p $MOD:$FUNC > kprobe_events
+grep $MOD kprobe_events
+echo > kprobe_events
+clear_trace
-- 
2.13.1

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

* Re: [PATCH 2/2] selftests/ftrace: Update multiple kprobes test for powerpc
  2017-06-22 17:03     ` Naveen N. Rao
@ 2017-06-23 17:30       ` Masami Hiramatsu
  2017-06-24 11:06         ` Masami Hiramatsu
  0 siblings, 1 reply; 15+ messages in thread
From: Masami Hiramatsu @ 2017-06-23 17:30 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Steven Rostedt, Ananth N Mavinakayanahalli, Michael Ellerman,
	linux-kernel, linuxppc-dev

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.

Thank you,

> 
> Thanks for the review!
> - Naveen
> 
> 
> > So if the Powerpc Elfv1 use dot prefix, that is OK, in that case,
> > could you update the filter as "^.*\\..*" ?
> > 
> > Thank you,
> > 
> > > 
> > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > > ---
> > >  tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > 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 f4d1ff785d67..d209c071b2c0 100644
> > > --- a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
> > > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
> > > @@ -2,16 +2,16 @@
> > >  # description: Register/unregister many kprobe events
> > >  
> > >  # ftrace fentry skip size depends on the machine architecture.
> > > -# Currently HAVE_KPROBES_ON_FTRACE defined on x86 and powerpc
> > > +# Currently HAVE_KPROBES_ON_FTRACE defined on x86 and powerpc64le
> > >  case `uname -m` in
> > >    x86_64|i[3456]86) OFFS=5;;
> > > -  ppc*) OFFS=4;;
> > > +  ppc64le) OFFS=8;;
> > >    *) OFFS=0;;
> > >  esac
> > >  
> > >  echo "Setup up to 256 kprobes"
> > > -grep t /proc/kallsyms | cut -f3 -d" " | grep -v .*\\..* | \
> > > -head -n 256 | while read i; do echo p ${i}+${OFFS} ; done > kprobe_events ||:
> > > +grep t /proc/kallsyms | cut -f3 -d" " | head -n 256 | \
> > > +while read i; do echo p ${i}+${OFFS} ; done > kprobe_events ||:
> > >  
> > >  echo 1 > events/kprobes/enable
> > >  echo 0 > events/kprobes/enable
> > > -- 
> > > 2.13.1
> > > 
> > 
> > 
> > -- 
> > Masami Hiramatsu <mhiramat@kernel.org>
> > 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

* Re: [PATCH 1/2] trace/kprobes: Sanitize derived event names
  2017-06-22 19:03     ` Naveen N. Rao
@ 2017-06-23 17:30       ` Masami Hiramatsu
  0 siblings, 0 replies; 15+ messages in thread
From: Masami Hiramatsu @ 2017-06-23 17:30 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Steven Rostedt, Ananth N Mavinakayanahalli, Michael Ellerman,
	linux-kernel, linuxppc-dev

On Fri, 23 Jun 2017 00:33:45 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> On 2017/06/22 06:29PM, Masami Hiramatsu wrote:
> > On Thu, 22 Jun 2017 00:20:27 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> > > When we derive event names, convert some expected symbols (such as ':'
> > > used to specify module:name and '.' present in some symbols) into
> > > underscores so that the event name is not rejected.
> > 
> > Oops, ok, this is my mistake.
> > 
> > Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
> > 
> > This must be marked as bugfix for stable trees.
> > 
> > Could you also add a testcase for this (module name) bug?
> > 
> > MODNAME=`lsmod | head -n 2 | tail -n 1 | cut -f 1 -d " "`
> > FUNCNAME=`grep -m 1 "\\[$MODNAME\\]" /proc/kallsyms | xargs | cut -f 3 -d " "`
> > 
> > May gives you a target name :)
> 
> Sure. Here is a test.
> 
> Thanks for the review,
> Naveen
> 
> -
> [PATCH] selftests/ftrace: Add a test to probe module functions
> 
> Add a kprobes test to ensure that we are able to add a probe on a
> module function using 'p <mod>:<func>' format, without having to
> specify a probe name.
> 
> Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Perfect! :)

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

Thanks!

> ---
>  .../testing/selftests/ftrace/test.d/kprobe/probe_module.tc | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>  create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc b/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc
> new file mode 100644
> index 000000000000..ea7657041ba6
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc
> @@ -0,0 +1,14 @@
> +#!/bin/sh
> +# description: Kprobe dynamic event - probing module
> +
> +[ -f kprobe_events ] || exit_unsupported # this is configurable
> +
> +echo 0 > events/enable
> +echo > kprobe_events
> +export MOD=`lsmod | head -n 2 | tail -n 1 | cut -f1 -d" "`
> +export FUNC=`grep -m 1 ".* t .*\\[$MOD\\]" /proc/kallsyms | xargs | cut -f3 -d" "`
> +[ "x" != "x$MOD" -a "y" != "y$FUNC" ] || exit_untested
> +echo p $MOD:$FUNC > kprobe_events
> +grep $MOD kprobe_events
> +echo > kprobe_events
> +clear_trace
> -- 
> 2.13.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

* Re: [PATCH 2/2] selftests/ftrace: Update multiple kprobes test for powerpc
  2017-06-23 17:30       ` Masami Hiramatsu
@ 2017-06-24 11:06         ` Masami Hiramatsu
  2017-06-28  9:28           ` Naveen N. Rao
  0 siblings, 1 reply; 15+ messages in thread
From: Masami Hiramatsu @ 2017-06-24 11:06 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Steven Rostedt, Ananth N Mavinakayanahalli, Michael Ellerman,
	linux-kernel, Masami Hiramatsu, linuxppc-dev

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

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 " "`
+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
-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

* Re: [PATCH 2/2] selftests/ftrace: Update multiple kprobes test for powerpc
  2017-06-24 11:06         ` Masami Hiramatsu
@ 2017-06-28  9:28           ` Naveen N. Rao
  2017-06-28 14:16             ` Masami Hiramatsu
  0 siblings, 1 reply; 15+ messages in thread
From: Naveen N. Rao @ 2017-06-28  9:28 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Steven Rostedt, Ananth N Mavinakayanahalli, Michael Ellerman,
	linux-kernel, linuxppc-dev

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

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

* Re: [PATCH 2/2] selftests/ftrace: Update multiple kprobes test for powerpc
  2017-06-28  9:28           ` Naveen N. Rao
@ 2017-06-28 14:16             ` Masami Hiramatsu
  2017-06-28 18:43               ` Naveen N. Rao
  0 siblings, 1 reply; 15+ messages in thread
From: Masami Hiramatsu @ 2017-06-28 14:16 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Steven Rostedt, Ananth N Mavinakayanahalli, Michael Ellerman,
	linux-kernel, linuxppc-dev

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
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

* Re: [PATCH 2/2] selftests/ftrace: Update multiple kprobes test for powerpc
  2017-06-28 14:16             ` Masami Hiramatsu
@ 2017-06-28 18:43               ` Naveen N. Rao
  2017-06-29  0:57                 ` Masami Hiramatsu
  0 siblings, 1 reply; 15+ messages in thread
From: Naveen N. Rao @ 2017-06-28 18:43 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Steven Rostedt, Ananth N Mavinakayanahalli, Michael Ellerman,
	linux-kernel, linuxppc-dev

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

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

* Re: [PATCH 2/2] selftests/ftrace: Update multiple kprobes test for powerpc
  2017-06-28 18:43               ` Naveen N. Rao
@ 2017-06-29  0:57                 ` Masami Hiramatsu
  2017-06-29 13:08                   ` Naveen N. Rao
  0 siblings, 1 reply; 15+ messages in thread
From: Masami Hiramatsu @ 2017-06-29  0:57 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Steven Rostedt, Ananth N Mavinakayanahalli, Michael Ellerman,
	linux-kernel, linuxppc-dev

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,



-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

* Re: [PATCH 2/2] selftests/ftrace: Update multiple kprobes test for powerpc
  2017-06-29  0:57                 ` Masami Hiramatsu
@ 2017-06-29 13:08                   ` Naveen N. Rao
  0 siblings, 0 replies; 15+ messages in thread
From: Naveen N. Rao @ 2017-06-29 13:08 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Steven Rostedt, Ananth N Mavinakayanahalli, Michael Ellerman,
	linux-kernel, linuxppc-dev

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>
> 

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

end of thread, other threads:[~2017-06-29 13:09 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-21 18:50 [PATCH 0/2] A couple of small updates/fixes for kprobes tracer Naveen N. Rao
2017-06-21 18:50 ` [PATCH 1/2] trace/kprobes: Sanitize derived event names Naveen N. Rao
2017-06-22  9:29   ` Masami Hiramatsu
2017-06-22 19:03     ` Naveen N. Rao
2017-06-23 17:30       ` Masami Hiramatsu
2017-06-21 18:50 ` [PATCH 2/2] selftests/ftrace: Update multiple kprobes test for powerpc Naveen N. Rao
2017-06-22  9:07   ` Masami Hiramatsu
2017-06-22 17:03     ` Naveen N. Rao
2017-06-23 17:30       ` Masami Hiramatsu
2017-06-24 11:06         ` Masami Hiramatsu
2017-06-28  9:28           ` Naveen N. Rao
2017-06-28 14:16             ` Masami Hiramatsu
2017-06-28 18:43               ` Naveen N. Rao
2017-06-29  0:57                 ` Masami Hiramatsu
2017-06-29 13:08                   ` Naveen N. Rao

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