Re: [PATCH 0/2] perf probe fixes for ppc64le

[Balbir Singh]

On Thu, 2016-04-07 at 14:56 +0530, Naveen N. Rao wrote:
> On 2016/04/07 06:19PM, Balbir Singh wrote:
> >Â
> >Â
> > On 06/04/16 22:32, Naveen N. Rao wrote:
> > >Â
> > > This patchset fixes three issues found with perf probe on ppc64le:
> > > 1. 'perf test kallsyms' failure on ppc64le (reported by Michael
> > > Ellerman). This was due to the symbols being fixed up during symbol
> > > table load. This is fixed in patch 2 by delaying symbol fixup until
> > > later.
> > > 2. perf probe function offset was being calculated from the local entry
> > > point (LEP), which does not match user expectation when trying to look
> > > at function disassembly output (reported by Ananth N). This is fixed for
> > > kallsyms in patch 1 and for symbol table in patch 2.
> > I think the bit where the offset is w.r.t LEP when using a name, but w.r.t
> > GEP when using function+offset can be confusing.
> Thanks for your review!
>Â
> The rationale for this is actually from the end-user perspective. TheÂ
> two use cases we are considering are:
> 1. User just wants to probe at function entry point:
>Â	# perf probe _do_fork
>Â
> In this case, the user most definitely needs the local entry point,Â
> without which the probe won't be hit. So, for this case, weÂ
> automatically insert the probe at the LEP.
>Â
> [We really only want to alter perf probe behavior in this case only, butÂ
> we were incorrectly changing the behavior of perf with the belowÂ
> scenario as well.]
>Â
> 2. User wants to probe at a specific location. In this case, the userÂ
> most likely starts by looking at the function disassembly. For instance:
>Â	# objdump -S -d vmlinux.bak | grep -A100 \<_do_fork\>:
>Â	c0000000000b6a00 <_do_fork>:
>Â		ÂÂÂÂÂÂunsigned long stack_start,
>Â		ÂÂÂÂÂÂunsigned long stack_size,
>Â		ÂÂÂÂÂÂint __user *parent_tidptr,
>Â		ÂÂÂÂÂÂint __user *child_tidptr,
>Â		ÂÂÂÂÂÂunsigned long tls)
>Â	{
>Â	c0000000000b6a00:	f7 00 4c 3cÂ	addisÂÂÂr2,r12,247
>Â	c0000000000b6a04:	00 86 42 38Â	addiÂÂÂÂr2,r2,-31232
>Â	c0000000000b6a08:	a6 02 08 7cÂ	mflrÂÂÂÂr0
>Â	c0000000000b6a0c:	d0 ff 41 fbÂ	stdÂÂÂÂÂr26,-48(r1)
>Â	c0000000000b6a10:	26 80 90 7dÂ	mfocrfÂÂr12,8
>Â	...<snip>...
>Â		if (!(clone_flags & CLONE_UNTRACED)) {
>Â	c0000000000b6a54:	e3 4f c7 7bÂ	rldicl. r7,r30,41,63
>Â	c0000000000b6a58:	2c 00 82 40Â	bneÂÂÂÂÂc0000000000b6a84 <_do_fork+0x84>
>Â			if (clone_flags & CLONE_VFORK)
>Â	c0000000000b6a5c:	e3 97 c8 7bÂ	rldicl. r8,r30,50,63
>Â	c0000000000b6a60:	a0 01 82 41Â	beqÂÂÂÂÂc0000000000b6c00 <_do_fork+0x200>
>Â	c0000000000b6a64:	20 00 20 39Â	liÂÂÂÂÂÂr9,32
>Â				trace = PTRACE_EVENT_VFORK;
>Â	c0000000000b6a68:	02 00 80 3bÂ	liÂÂÂÂÂÂr28,2
>Â	c0000000000b6a6c:	10 02 4d e9Â	ldÂÂÂÂÂÂr10,528(r13)
>Â
> If the user wants to probe at _do_fork+0x54, he'd do:
>Â	# perf probe _do_fork+0x54
>Â
> With the earlier approach, we would insert the probe at _do_fork+0x5cÂ
> (0x54 from the LEP) instead, which is incorrect.
>Â
> In reality, user would probably just use debuginfo:
>Â	# perf probe -L _do_fork
>Â	<_do_fork@/root/linus/kernel/fork.c:0>
>Â	ÂÂÂÂÂÂ0ÂÂlong _do_fork(unsigned long clone_flags,
>Â			ÂÂÂÂÂÂunsigned long stack_start,
>Â			ÂÂÂÂÂÂunsigned long stack_size,
>Â			ÂÂÂÂÂÂint __user *parent_tidptr,
>Â			ÂÂÂÂÂÂint __user *child_tidptr,
>Â			ÂÂÂÂÂÂunsigned long tls)
>Â	ÂÂÂÂÂÂ6ÂÂ{
>Â			struct task_struct *p;
>Â	ÂÂÂÂÂÂ8ÂÂÂÂÂÂÂÂÂint trace = 0;
>Â			long nr;
>Â		Â
>Â			/*
>Â			Â* Determine whether and which event to report to ptracer.ÂÂWhen
>Â			Â* called from kernel_thread or CLONE_UNTRACED is explicitly
>Â			Â* requested, no event is reported; otherwise, report if the event
>Â			Â* for the type of forking is enabled.
>Â			Â*/
>Â	ÂÂÂÂÂ17ÂÂÂÂÂÂÂÂÂif (!(clone_flags & CLONE_UNTRACED)) {
>Â	ÂÂÂÂÂ18ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (clone_flags & CLONE_VFORK)
>Â	ÂÂÂÂÂ19ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtrace = PTRACE_EVENT_VFORK;
>Â	ÂÂÂÂÂ20ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂelse if ((clone_flags & CSIGNAL) != SIGCHLD)
>Â	ÂÂÂÂÂ21ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtrace = PTRACE_EVENT_CLONE;
>Â
>Â	# perf probe _do_fork:17
>Â
> In this case, perf chooses the right address based on DWARF. The currentÂ
> patchset matches the behavior of perf without debuginfo with this.


I agree what I worry is that perf probe _do_fork sets a breakpoint after
perf probe _do_fork+0x4. I am not sure if there is an easy solution to
the problem.Â

Balbir