From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757525Ab0EGRzc (ORCPT ); Fri, 7 May 2010 13:55:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49135 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755596Ab0EGRza (ORCPT ); Fri, 7 May 2010 13:55:30 -0400 Message-ID: <4BE453A1.1060003@redhat.com> Date: Fri, 07 May 2010 13:53:37 -0400 From: Masami Hiramatsu User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.9) Gecko/20100330 Fedora/3.0.4-1.fc11 Thunderbird/3.0.4 MIME-Version: 1.0 To: Srikar Dronamraju CC: Peter Zijlstra , Ingo Molnar , Mel Gorman , Randy Dunlap , Linus Torvalds , Roland McGrath , "H. Peter Anvin" , Ananth N Mavinakayanahalli , Oleg Nesterov , Mark Wielaard , Mathieu Desnoyers , LKML , Jim Keniston , Frederic Weisbecker , "Frank Ch. Eigler" , Andrew Morton , "Paul E. McKenney" Subject: Re: [PATCH v3 10/10] perf: perf interface for uprobes. References: <20100506180139.28877.81699.sendpatchset@localhost6.localdomain6> <20100506180340.28877.25709.sendpatchset@localhost6.localdomain6> <4BE34D1D.2020100@redhat.com> <20100507022403.GH7426@linux.vnet.ibm.com> In-Reply-To: <20100507022403.GH7426@linux.vnet.ibm.com> X-Enigmail-Version: 1.0.1 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Srikar Dronamraju wrote: >> >> Could you also rebase your patch on the latest tip tree? :) >> (and remove die() too) > > I was aware that I have to rebase the patch to the latest tip. > However I wanted to get a working set out. > Please expect the rebased version in the next version of the posting. > >> >>> Here is a terminal snapshot of placing, using and removing a probe on a >>> process with pid 3329 (corresponding to zsh) >>> >>> [ Probing a function in the executable using function name ] >>> ------------------------------------------------------------- >>> [root@ABCD]# perf probe -p 3329 zfree@zsh >> >> Hmm, I'm not sure why we have to specify both of PID and exec name. >> Is that possible to give a symbol as below? (omit exec name) >> >> # perf probe -p 3329 zfree > > Yes, If the symbol name belongs to th execfile then we can omit the > execfile part. However for symbols outside the execname, the > execfile(library file) is a must(atleast for now). Good!:) >>> Added new event: >>> probe:p_3329_zfree (on 0x446420) >> >> Hmm, the event name should be simpler, as like as zfree_3329. >> or, we might better make a new event group for each process, as like as >> 'probe_3329:zfree' >> > > Either zfree_3329 or having a new event group for each process is > possible. I think adding new groups make controlling those event easier. (e.g. deleting all probes on exited process) >> Hmm, if user gave --list with -p, what happened? >> We need to check a bad combination and warn it. >> > > Currently it would ignore -p option. i.e --list overrides. > We could either warn, or error out. What do you suggest? > >>> >>> + if (session.pid) >>> + goto end_dwarf; >>> + >> >> Again, it must be checked that the combination of -p option and dwarf requirement. >> The latest code has perf_probe_event_need_dwarf(), so you can check it easier. >> > > Okay, > >> [...] >>> + else >>> + free(pp->function); >>> + pp->function = zalloc(sizeof(char *) * 20); >>> + if (!pp->function) >>> + die("Failed to allocate memory by zalloc."); >>> + e_snprintf(pp->function, 20, "0x%llx", vaddr); >>> +} >> >> Well, after rebasing, you'll need to remove die()s from here and >> make it returns errors. >> > > Okay. > > >> [...] >>> @@ -383,7 +450,11 @@ int synthesize_trace_kprobe_event(struct probe_point *pp) >>> pp->probes[0] = buf = zalloc(MAX_CMDLEN); >>> if (!buf) >>> die("Failed to allocate memory by zalloc."); >>> - ret = e_snprintf(buf, MAX_CMDLEN, "%s+%d", pp->function, pp->offset); >>> + if (pp->offset) >>> + ret = e_snprintf(buf, MAX_CMDLEN, "%s+%d", pp->function, >>> + pp->offset); >>> + else >>> + ret = e_snprintf(buf, MAX_CMDLEN, "%s", pp->function); >>> if (ret <= 0) >>> goto error; >>> len = ret; >> >> Isn't it a cleanup ? > > Can you please elaborate? symbol+offset is always allowed even if offset == 0, so I think this change isn't needed. I guessed it is just for cleanup. >>> +void add_trace_uprobe_events(struct probe_point *probes, int nr_probes, >>> + bool force_add, pid_t pid) >>> +{ >>> + int fd = open_uprobe_events(O_RDWR, O_APPEND); >>> + add_trace_probe_events(fd, probes, nr_probes, force_add, pid); >>> +} >> >> Please close fd in the function which opened it. >> > > Okay. > >> >>> memset(&sym, 0, sizeof(sym)); >>> - if (!self->kernel) { >>> + if (self->kernel || symbol_conf.sort_by_name) >>> + self->adjust_symbols = 0; >>> + else { >>> self->adjust_symbols = (ehdr.e_type == ET_EXEC || >>> elf_section_by_name(elf, &ehdr, &shdr, >>> ".gnu.prelink_undo", >>> NULL) != NULL); >>> - } else self->adjust_symbols = 0; >>> + } >>> >>> elf_symtab__for_each_symbol(syms, nr_syms, idx, sym) { >>> struct symbol *f; >> >> This one changes the symbol.c, so I think you'd better make a >> separate patch for this change. > > Okay, I will separate it in the next version. > Thanks Masami for the review. Thank you! -- Masami Hiramatsu e-mail: mhiramat@redhat.com