From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1289 invoked by alias); 10 Jul 2009 20:30:42 -0000 Received: (qmail 1279 invoked by uid 22791); 10 Jul 2009 20:30:39 -0000 X-SWARE-Spam-Status: No, hits=-0.7 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_64,KAM_STOCKGEN,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 10 Jul 2009 20:30:31 +0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n6AKUTvr025264 for ; Fri, 10 Jul 2009 16:30:29 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n6AKURGa003796 for ; Fri, 10 Jul 2009 16:30:28 -0400 Received: from [10.16.2.46] (dhcp-100-2-46.bos.redhat.com [10.16.2.46]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id n6AKUOiI029036; Fri, 10 Jul 2009 16:30:25 -0400 Message-ID: <4A57A58B.8010909@redhat.com> Date: Fri, 10 Jul 2009 20:30:00 -0000 From: Masami Hiramatsu User-Agent: Thunderbird 2.0.0.21 (X11/20090320) MIME-Version: 1.0 To: Li Zefan CC: Ingo Molnar , Steven Rostedt , lkml , systemtap , kvm , DLE , Ananth N Mavinakayanahalli , Christoph Hellwig , Frederic Weisbecker , Tom Zanussi Subject: Re: [PATCH -tip -v11 08/11] tracing: add kprobe-based event tracer References: <20090709202220.13223.97114.stgit@localhost.localdomain> <20090709202309.13223.9431.stgit@localhost.localdomain> <4A56EA17.8000508@cn.fujitsu.com> In-Reply-To: <4A56EA17.8000508@cn.fujitsu.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact systemtap-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: systemtap-owner@sourceware.org X-SW-Source: 2009-q3/txt/msg00085.txt.bz2 Li Zefan wrote: >> +static __kprobes unsigned long fetch_memory(struct pt_regs *regs, void *addr) >> +{ >> + unsigned long retval; > > need a space after local variable declarations. > >> + if (probe_kernel_address(addr, retval)) >> + return 0; >> + return retval; >> +} >> + >> +static __kprobes unsigned long fetch_argument(struct pt_regs *regs, void *num) >> +{ >> + return regs_get_argument_nth(regs, (unsigned int)((unsigned long)num)); >> +} >> + >> +static __kprobes unsigned long fetch_retvalue(struct pt_regs *regs, >> + void *dummy) >> +{ >> + return regs_return_value(regs); >> +} >> + >> +static __kprobes unsigned long fetch_ip(struct pt_regs *regs, void *dummy) >> +{ >> + return instruction_pointer(regs); >> +} >> + >> +/* Memory fetching by symbol */ >> +struct symbol_cache { >> + char *symbol; >> + long offset; >> + unsigned long addr; >> +}; >> + >> +static unsigned long update_symbol_cache(struct symbol_cache *sc) >> +{ >> + sc->addr = (unsigned long)kallsyms_lookup_name(sc->symbol); >> + if (sc->addr) >> + sc->addr += sc->offset; >> + return sc->addr; >> +} >> + >> +static void free_symbol_cache(struct symbol_cache *sc) >> +{ >> + kfree(sc->symbol); >> + kfree(sc); >> +} >> + >> +static struct symbol_cache *alloc_symbol_cache(const char *sym, long offset) >> +{ >> + struct symbol_cache *sc; > > ditto. > > and in some other places Agreed, I'll fix all of it. >> +static int probes_seq_show(struct seq_file *m, void *v) >> +{ >> + struct trace_probe *tp = v; >> + int i, ret; >> + char buf[MAX_ARGSTR_LEN + 1]; >> + >> + if (tp == NULL) >> + return 0; >> + > > redundant check. tp won't be NULL. Sure. >> +static ssize_t probes_write(struct file *file, const char __user *buffer, >> + size_t count, loff_t *ppos) >> +{ >> + char *kbuf, *tmp; >> + int ret; >> + size_t done; >> + size_t size; >> + >> + if (!count || count < 0) >> + return 0; > > count is unsigned, so won't < 0. Also I don't think you > need to treat (count == 0) specially. Ah, right. That was another redundant check too... >> + >> + kbuf = kmalloc(WRITE_BUFSIZE, GFP_KERNEL); > > should be kmalloc(WRITE_BUFSIZE+1), or... > >> + if (!kbuf) >> + return -ENOMEM; >> + >> + ret = done = 0; >> + do { >> + size = count - done; >> + if (size > WRITE_BUFSIZE) >> + size = WRITE_BUFSIZE; > > if (size >= WRITE_BUFSIZE) > size = WRITE_BUFSIZE - 1; Hm, I'll take this, because WRITE_BUFSIZE should be the size of buffer. >> + if (copy_from_user(kbuf, buffer + done, size)) { >> + ret = -EFAULT; >> + goto out; >> + } >> + kbuf[size] = '\0'; >> + tmp = strchr(kbuf, '\n'); >> + if (!tmp) { >> + pr_warning("Line length is too long: " >> + "Should be less than %d.", WRITE_BUFSIZE); >> + ret = -EINVAL; >> + goto out; >> + } Hmm, here, there will be a case that the last line is terminated without a new line... >> + *tmp = '\0'; >> + size = tmp - kbuf + 1; >> + done += size; >> + /* Remove comments */ >> + tmp = strchr(kbuf, '#'); >> + if (tmp) >> + *tmp = '\0'; >> + >> + ret = command_trace_probe(kbuf); >> + if (ret) >> + goto out; >> + >> + } while (done < count); >> + ret = done; >> +out: >> + kfree(kbuf); >> + return ret; >> +} > > ... > >> +enum print_line_t >> +print_kretprobe_event(struct trace_iterator *iter, int flags) >> +{ >> + struct kretprobe_trace_entry *field; >> + struct trace_seq *s = &iter->seq; >> + int i; >> + >> + trace_assign_type(field, iter->ent); >> + >> + if (!seq_print_ip_sym(s, field->ret_ip, flags | TRACE_ITER_SYM_OFFSET)) >> + goto partial; >> + > > can't we use %pF? > >> + if (!trace_seq_puts(s, " <- ")) >> + goto partial; >> + >> + if (!seq_print_ip_sym(s, field->func, flags & ~TRACE_ITER_SYM_OFFSET)) >> + goto partial; >> + > > and $pf? > >> + if (!trace_seq_puts(s, ":")) >> + goto partial; >> + > > so all the above: > > trace_seq_puts(s, "%pF <- %pf:", (void *)field->ret_ip, > (void *)field->func); Hmm, I'd like to use seq_print_ip_sym() rather than %pF/%pf, because there is a difference between them when ftrace's sym-addr option is set. Set a probe and a return probe on vfs_read, like echo p vfs_read > kprobe_events echo r vfs_read >> kprobe_events And if we use seq_print_ip_sym() and sym-addr is 1, we'll see below output; <...>-1908 [001] 875089.743395: vfs_read+0x0/0x102 : <...>-1908 [001] 875089.743398: sys_read+0x47/0x70 <- vfs_read : On the other hand, if we use trace_seq_printf("%pf...), then: <...>-1861 [001] 873504.331268: vfs_read+0x0/0x102 : <...>-1861 [001] 873504.331272: sys_read+0x47/0x70 <- vfs_read: In this case, we can't see the real address of those symbols. Thank you for review my patch! -- Masami Hiramatsu Software Engineer Hitachi Computer Products (America), Inc. Software Solutions Division e-mail: mhiramat@redhat.com