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

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

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