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 Content-Length: 4787 Lines: 108 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