From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753232AbcDINoZ (ORCPT ); Sat, 9 Apr 2016 09:44:25 -0400 Received: from e23smtp07.au.ibm.com ([202.81.31.140]:48072 "EHLO e23smtp07.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751866AbcDINoX (ORCPT ); Sat, 9 Apr 2016 09:44:23 -0400 X-IBM-Helo: d23dlp02.au.ibm.com X-IBM-MailFrom: naveen.n.rao@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org Date: Sat, 9 Apr 2016 19:12:39 +0530 From: "Naveen N. Rao" To: Balbir Singh Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, Arnaldo Carvalho de Melo , Masami Hiramatsu , Thiago Jung Bauermann , Mark Wielaard Subject: Re: [PATCH 0/2] perf probe fixes for ppc64le Message-ID: <20160409134239.GL15993@naverao1-tp.in.ibm.com> References: <57061802.7020508@gmail.com> <20160407092636.GK15993@naverao1-tp.in.ibm.com> <1460098663.6475.6.camel@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1460098663.6475.6.camel@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16040913-0025-0000-0000-0000043B4BFE Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016/04/08 04:57PM, Balbir Singh wrote: > 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 > > ...... > > 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. I suppose this boils down to the quirkiness of ABIv2. Though, in reality, I don't think most users will notice. As I stated above, users will most likely start with the disassembly or debuginfo and this patch ensures there are actually no surprises there. - Naveen