From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751954AbeEDPse (ORCPT ); Fri, 4 May 2018 11:48:34 -0400 Received: from mail.kernel.org ([198.145.29.99]:56788 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751710AbeEDPsc (ORCPT ); Fri, 4 May 2018 11:48:32 -0400 Date: Sat, 5 May 2018 00:48:28 +0900 From: Masami Hiramatsu To: Steven Rostedt Cc: linux-kernel@vger.kernel.org, Ingo Molnar , Namhyung Kim , Tom Zanussi , Arnaldo Carvalho de Melo , linux-trace-users@vger.kernel.org, linux-kselftest@vger.kernel.org, shuah@kernel.org, Ravi Bangoria Subject: Re: [PATCH v7 00/16] tracing: probeevent: Improve fetcharg features Message-Id: <20180505004828.9b75b6802472f09b0d2de5b8@kernel.org> In-Reply-To: <20180503181137.6d82d897@gandalf.local.home> References: <152465856498.26224.16969986455942749517.stgit@devbox> <20180503181137.6d82d897@gandalf.local.home> X-Mailer: Sylpheed 3.5.1 (GTK+ 2.24.31; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 3 May 2018 18:11:37 -0400 Steven Rostedt wrote: > On Wed, 25 Apr 2018 21:16:06 +0900 > Masami Hiramatsu wrote: > > > Hi, > > > > This is the 7th version of the fetch-arg improvement series. > > This includes variable changes on fetcharg framework like, > > > > - Add fetcharg testcases (syntax, argN, symbol, string and array) > > and probepoint testcase. > > - Rewrite fetcharg framework with fetch_insn, switch-case based > > instead of function pointer. > > - Add "symbol" type support, which shows symbol+offset instead of > > address value. > > - Add "$argN" fetcharg, which fetches function parameters. > > (currently only for x86-64) > > - Add array type support (including string arrary :) ) , > > which enables to get fixed length array from probe-events. > > - Add array type support for perf-probe, so that user can > > dump partial array entries. > > > > V6 is here: > > https://lkml.org/lkml/2018/3/17/75 > > > > Changes from the v6 are here: > > [6/16] - Fix to return an error if failed to fetch string and > > fill zero-length data_loc in error case. > > [11/16] - Update document for restructured text. > > [15/16] - Fix README test. > > [16/16] - Add type-casting description (and note) to documentation. > > > > And rebased on the latest Steve's ftrace/core branch. > > > > Hi Masami, > > I skimmed through the patches and it they appear fine. I've applied > them and started playing a little. > Thanks! > I've been thinking my function based events, and thought instead I > would make them part of the kprobe infrastructure. Just have a slightly > different format, and instead of being p: or r: be f: And keep the > format I was suggesting. > > What do you think? Unifying the user knob is great! But if so, the format also should not be so different, I mean user can use it same way. And maybe "f:" will be non-sense. "p:" with "function+0" allows user to use "function type casting", that is enough I think. So the syntax will be p[:EVENT] SYM[(CAST)|+OFFS] [FETCHARG] And here is an example; p:myevent vfs_read(void *file, char *buf, size_t count, void *pos) $arg1 $arg2 In this case inside '()' will be analyzed and packed as something like "reference type" data and it is used when converting "$argN". And maybe we can provide $args special variable to record all arguments (it can be available only when the (CAST) is given). This gives the user a consistent model; if you just give a symbol the arguments may not be correctly translated. but if you give a type-casting information, it will be much better. > > Also, when looking at the kprobe code, I was looking at this function: > > > /* Ftrace callback handler for kprobes -- called under preepmt disabed */ > > void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > > struct ftrace_ops *ops, struct pt_regs *regs) > > { > > struct kprobe *p; > > struct kprobe_ctlblk *kcb; > > > > /* Preempt is disabled by ftrace */ > > p = get_kprobe((kprobe_opcode_t *)ip); > > if (unlikely(!p) || kprobe_disabled(p)) > > return; > > > > kcb = get_kprobe_ctlblk(); > > if (kprobe_running()) { > > kprobes_inc_nmissed_count(p); > > } else { > > unsigned long orig_ip = regs->ip; > > /* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */ > > regs->ip = ip + sizeof(kprobe_opcode_t); > > > > /* To emulate trap based kprobes, preempt_disable here */ > > preempt_disable(); > > __this_cpu_write(current_kprobe, p); > > kcb->kprobe_status = KPROBE_HIT_ACTIVE; > > if (!p->pre_handler || !p->pre_handler(p, regs)) { > > __skip_singlestep(p, regs, kcb, orig_ip); > > preempt_enable_no_resched(); > > This preemption disabling and enabling looks rather strange. Looking at > git blame, it appears this was added for jprobes. Can we remove it now > that jprobes is going away? No, that is not for jprobes but for compatibility with kprobe's user handler. Since this transformation is done silently, user can not change their handler for ftrace case. So we need to keep this condition same as original kprobes. And anyway, for using smp_processor_id() for accessing per-cpu, we should disable preemption, correct? Thanks, > > > } > > /* > > * If pre_handler returns !0, it sets regs->ip and > > * resets current kprobe, and keep preempt count +1. > > */ > > } > > } > > -- Steve -- Masami Hiramatsu From mboxrd@z Thu Jan 1 00:00:00 1970 From: mhiramat at kernel.org (Masami Hiramatsu) Date: Sat, 5 May 2018 00:48:28 +0900 Subject: [PATCH v7 00/16] tracing: probeevent: Improve fetcharg features In-Reply-To: <20180503181137.6d82d897@gandalf.local.home> References: <152465856498.26224.16969986455942749517.stgit@devbox> <20180503181137.6d82d897@gandalf.local.home> Message-ID: <20180505004828.9b75b6802472f09b0d2de5b8@kernel.org> On Thu, 3 May 2018 18:11:37 -0400 Steven Rostedt wrote: > On Wed, 25 Apr 2018 21:16:06 +0900 > Masami Hiramatsu wrote: > > > Hi, > > > > This is the 7th version of the fetch-arg improvement series. > > This includes variable changes on fetcharg framework like, > > > > - Add fetcharg testcases (syntax, argN, symbol, string and array) > > and probepoint testcase. > > - Rewrite fetcharg framework with fetch_insn, switch-case based > > instead of function pointer. > > - Add "symbol" type support, which shows symbol+offset instead of > > address value. > > - Add "$argN" fetcharg, which fetches function parameters. > > (currently only for x86-64) > > - Add array type support (including string arrary :) ) , > > which enables to get fixed length array from probe-events. > > - Add array type support for perf-probe, so that user can > > dump partial array entries. > > > > V6 is here: > > https://lkml.org/lkml/2018/3/17/75 > > > > Changes from the v6 are here: > > [6/16] - Fix to return an error if failed to fetch string and > > fill zero-length data_loc in error case. > > [11/16] - Update document for restructured text. > > [15/16] - Fix README test. > > [16/16] - Add type-casting description (and note) to documentation. > > > > And rebased on the latest Steve's ftrace/core branch. > > > > Hi Masami, > > I skimmed through the patches and it they appear fine. I've applied > them and started playing a little. > Thanks! > I've been thinking my function based events, and thought instead I > would make them part of the kprobe infrastructure. Just have a slightly > different format, and instead of being p: or r: be f: And keep the > format I was suggesting. > > What do you think? Unifying the user knob is great! But if so, the format also should not be so different, I mean user can use it same way. And maybe "f:" will be non-sense. "p:" with "function+0" allows user to use "function type casting", that is enough I think. So the syntax will be p[:EVENT] SYM[(CAST)|+OFFS] [FETCHARG] And here is an example; p:myevent vfs_read(void *file, char *buf, size_t count, void *pos) $arg1 $arg2 In this case inside '()' will be analyzed and packed as something like "reference type" data and it is used when converting "$argN". And maybe we can provide $args special variable to record all arguments (it can be available only when the (CAST) is given). This gives the user a consistent model; if you just give a symbol the arguments may not be correctly translated. but if you give a type-casting information, it will be much better. > > Also, when looking at the kprobe code, I was looking at this function: > > > /* Ftrace callback handler for kprobes -- called under preepmt disabed */ > > void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > > struct ftrace_ops *ops, struct pt_regs *regs) > > { > > struct kprobe *p; > > struct kprobe_ctlblk *kcb; > > > > /* Preempt is disabled by ftrace */ > > p = get_kprobe((kprobe_opcode_t *)ip); > > if (unlikely(!p) || kprobe_disabled(p)) > > return; > > > > kcb = get_kprobe_ctlblk(); > > if (kprobe_running()) { > > kprobes_inc_nmissed_count(p); > > } else { > > unsigned long orig_ip = regs->ip; > > /* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */ > > regs->ip = ip + sizeof(kprobe_opcode_t); > > > > /* To emulate trap based kprobes, preempt_disable here */ > > preempt_disable(); > > __this_cpu_write(current_kprobe, p); > > kcb->kprobe_status = KPROBE_HIT_ACTIVE; > > if (!p->pre_handler || !p->pre_handler(p, regs)) { > > __skip_singlestep(p, regs, kcb, orig_ip); > > preempt_enable_no_resched(); > > This preemption disabling and enabling looks rather strange. Looking at > git blame, it appears this was added for jprobes. Can we remove it now > that jprobes is going away? No, that is not for jprobes but for compatibility with kprobe's user handler. Since this transformation is done silently, user can not change their handler for ftrace case. So we need to keep this condition same as original kprobes. And anyway, for using smp_processor_id() for accessing per-cpu, we should disable preemption, correct? Thanks, > > > } > > /* > > * If pre_handler returns !0, it sets regs->ip and > > * resets current kprobe, and keep preempt count +1. > > */ > > } > > } > > -- Steve -- Masami Hiramatsu -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: mhiramat@kernel.org (Masami Hiramatsu) Date: Sat, 5 May 2018 00:48:28 +0900 Subject: [PATCH v7 00/16] tracing: probeevent: Improve fetcharg features In-Reply-To: <20180503181137.6d82d897@gandalf.local.home> References: <152465856498.26224.16969986455942749517.stgit@devbox> <20180503181137.6d82d897@gandalf.local.home> Message-ID: <20180505004828.9b75b6802472f09b0d2de5b8@kernel.org> Content-Type: text/plain; charset="UTF-8" Message-ID: <20180504154828.0C01btWok9qU4vy7IxySPoAJK4gxhEUfU48Tfp2nld8@z> On Thu, 3 May 2018 18:11:37 -0400 Steven Rostedt wrote: > On Wed, 25 Apr 2018 21:16:06 +0900 > Masami Hiramatsu wrote: > > > Hi, > > > > This is the 7th version of the fetch-arg improvement series. > > This includes variable changes on fetcharg framework like, > > > > - Add fetcharg testcases (syntax, argN, symbol, string and array) > > and probepoint testcase. > > - Rewrite fetcharg framework with fetch_insn, switch-case based > > instead of function pointer. > > - Add "symbol" type support, which shows symbol+offset instead of > > address value. > > - Add "$argN" fetcharg, which fetches function parameters. > > (currently only for x86-64) > > - Add array type support (including string arrary :) ) , > > which enables to get fixed length array from probe-events. > > - Add array type support for perf-probe, so that user can > > dump partial array entries. > > > > V6 is here: > > https://lkml.org/lkml/2018/3/17/75 > > > > Changes from the v6 are here: > > [6/16] - Fix to return an error if failed to fetch string and > > fill zero-length data_loc in error case. > > [11/16] - Update document for restructured text. > > [15/16] - Fix README test. > > [16/16] - Add type-casting description (and note) to documentation. > > > > And rebased on the latest Steve's ftrace/core branch. > > > > Hi Masami, > > I skimmed through the patches and it they appear fine. I've applied > them and started playing a little. > Thanks! > I've been thinking my function based events, and thought instead I > would make them part of the kprobe infrastructure. Just have a slightly > different format, and instead of being p: or r: be f: And keep the > format I was suggesting. > > What do you think? Unifying the user knob is great! But if so, the format also should not be so different, I mean user can use it same way. And maybe "f:" will be non-sense. "p:" with "function+0" allows user to use "function type casting", that is enough I think. So the syntax will be p[:EVENT] SYM[(CAST)|+OFFS] [FETCHARG] And here is an example; p:myevent vfs_read(void *file, char *buf, size_t count, void *pos) $arg1 $arg2 In this case inside '()' will be analyzed and packed as something like "reference type" data and it is used when converting "$argN". And maybe we can provide $args special variable to record all arguments (it can be available only when the (CAST) is given). This gives the user a consistent model; if you just give a symbol the arguments may not be correctly translated. but if you give a type-casting information, it will be much better. > > Also, when looking at the kprobe code, I was looking at this function: > > > /* Ftrace callback handler for kprobes -- called under preepmt disabed */ > > void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > > struct ftrace_ops *ops, struct pt_regs *regs) > > { > > struct kprobe *p; > > struct kprobe_ctlblk *kcb; > > > > /* Preempt is disabled by ftrace */ > > p = get_kprobe((kprobe_opcode_t *)ip); > > if (unlikely(!p) || kprobe_disabled(p)) > > return; > > > > kcb = get_kprobe_ctlblk(); > > if (kprobe_running()) { > > kprobes_inc_nmissed_count(p); > > } else { > > unsigned long orig_ip = regs->ip; > > /* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */ > > regs->ip = ip + sizeof(kprobe_opcode_t); > > > > /* To emulate trap based kprobes, preempt_disable here */ > > preempt_disable(); > > __this_cpu_write(current_kprobe, p); > > kcb->kprobe_status = KPROBE_HIT_ACTIVE; > > if (!p->pre_handler || !p->pre_handler(p, regs)) { > > __skip_singlestep(p, regs, kcb, orig_ip); > > preempt_enable_no_resched(); > > This preemption disabling and enabling looks rather strange. Looking at > git blame, it appears this was added for jprobes. Can we remove it now > that jprobes is going away? No, that is not for jprobes but for compatibility with kprobe's user handler. Since this transformation is done silently, user can not change their handler for ftrace case. So we need to keep this condition same as original kprobes. And anyway, for using smp_processor_id() for accessing per-cpu, we should disable preemption, correct? Thanks, > > > } > > /* > > * If pre_handler returns !0, it sets regs->ip and > > * resets current kprobe, and keep preempt count +1. > > */ > > } > > } > > -- Steve -- Masami Hiramatsu -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html