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

[Masami Hiramatsu]

On Thu, 22 Jun 2017 22:33:25 +0530
"Naveen N. Rao" <naveen.n.rao@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxxx>
> > 
> 


-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>