From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5897 invoked by alias); 30 Oct 2006 14:07:07 -0000 Received: (qmail 5600 invoked by uid 22791); 30 Oct 2006 14:07:05 -0000 X-Spam-Status: No, hits=0.9 required=5.0 tests=AWL,BAYES_05,TW_DJ,TW_JP,UNPARSEABLE_RELAY X-Spam-Check-By: sourceware.org Received: from mail7.hitachi.co.jp (HELO mail7.hitachi.co.jp) (133.145.228.42) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 30 Oct 2006 14:06:52 +0000 Received: from mlsv9.hitachi.co.jp (unknown [133.145.228.16]) by mail7.hitachi.co.jp (Postfix) with ESMTP id 59AAC37AE6 for ; Mon, 30 Oct 2006 23:06:49 +0900 (JST) Received: from mfilter-s1.hitachi.co.jp by mlsv9.hitachi.co.jp (8.12.11/8.12.11) id k9UE6nLr029757; Mon, 30 Oct 2006 23:06:49 +0900 Received: from vshuts2.hitachi.co.jp (unverified) by mfilter-s1.hitachi.co.jp (Content Technologies SMTPRS 4.3.17) with SMTP id ; Mon, 30 Oct 2006 23:06:48 +0900 Received: from hsdlgw92.sdl.hitachi.co.jp ([133.144.7.20]) by vshuts2.hitachi.co.jp with SMTP id M2006103023064724304 ; Mon, 30 Oct 2006 23:06:48 +0900 Received: from vgate2.sdl.hitachi.co.jp by hsdlgw92.sdl.hitachi.co.jp (8.13.1/3.7W06092911) id k9UE6kMl005907; Mon, 30 Oct 2006 23:06:47 +0900 Received: from maila.sdl.hitachi.co.jp ([133.144.14.196]) by vgate2.sdl.hitachi.co.jp (SAVSMTP 3.1.1.32) with SMTP id M2006103023064616981 ; Mon, 30 Oct 2006 23:06:46 +0900 Received: from [127.0.0.1] ([10.232.9.172]) by maila.sdl.hitachi.co.jp (8.13.1/3.7W04031011) with ESMTP id k9UE6kg4026127; Mon, 30 Oct 2006 23:06:46 +0900 Message-ID: <454606E0.3050703@hitachi.com> Date: Mon, 30 Oct 2006 14:07:00 -0000 From: Masami Hiramatsu Organization: Systems Development Lab., Hitachi, Ltd., Japan User-Agent: Thunderbird 1.5.0.7 (Windows/20060909) MIME-Version: 1.0 To: Keshavamurthy@bambi.jf.intel.com, Anil S Cc: Ananth N Mavinakayanahalli , Prasanna S Panchamukhi , Ingo Molnar , SystemTAP , Satoshi Oshima , Hideo Aoki , Yumiko Sugita Subject: Re: [PATCH 2/5][djprobe] djprobe core patch References: <45338593.6090207@hitachi.com> <45373F28.1070305@hitachi.com> <20061027233941.GB10018@bambi.jf.intel.com> In-Reply-To: <20061027233941.GB10018@bambi.jf.intel.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Mailing-List: contact systemtap-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Post: List-Help: , Sender: systemtap-owner@sourceware.org X-SW-Source: 2006-q4/txt/msg00276.txt.bz2 Hi Anil, Thank you for your advice. Keshavamurthy@bambi.jf.intel.com wrote: > Hi Masami, > Sorry for the delayed response. My comments are inline. > > On Thu, Oct 19, 2006 at 06:02:32PM +0900, Masami Hiramatsu wrote: >> I also updated API reference. >> >> API Reference >> ------------- >> The Djprobe API includes "register_djprobe" function, >> "unregister_djprobe" function and "commit_djprobes" function. >> Here are specifications for these functions and the >> associated probe handlers. > > I am curious as to why you are introducing a new interface > for registering djprobes. Can't this be done by modifying the > existing register_kprobe()/unregister_kprobe() and under the > covers you can choose to implement djprobes. There were two reasons. 1) I thought that should be waste of memory. As you can see, the djprobe structure doesn't have its kprobe nor instruction buffer, because it is just an interface structure. Instead of that, it has an pointer to the djprobe_instance structure which is true instance of the djprobe. This "instance" has the kprobe structure and instruction buffer. So, I could keep the djprobe structure small. 2) The other reason is from difference of the usage between djprobe and kprobe. Kprobe users have to ensure that the target address points to the 1st byte of instruction. On the other hand, djprobe users additionally have to count the length of the target instructions, and ensure those instructions are relocatable. Therefore, I provided those special interfaces. If you never mind that the jump-based-kprobe needs two kprobe structures per one probe point (one is for the interface, another is for the instance -- for deferred releasing), I can integrate these interfaces. > The benifit of hiding djprobes under kprobes would be > > 1)Users of kernel probe need not have to maintain two > version of their instrumentation code (one with kprobes > and one with djprobes). Indeed. (I think users have to prepare additional code which counts the length and checks whether the instructions are relocatable or not.) > 2)If for some reason djprobes fails (might be because there exists > a kprobes in that djprobe address + size range), then the user of the > probe interface has to try kprobes in order to succeed the registration. > However if you choose to implement djprobes under the covers of kprobes, then you can > transparently support the insertion of the probes, through aggregate kprobes instead > of failing the djprobes. I agree. This advantage is important. > Also with this approach, today's systemtap can easily take advantage of djprobes, > since djprobes is implemented under the kprobes implementation. Since you are targeting > your code for mainline, don't worry about kabi of kprobes interface. Thanks, I'll try it. > Oaky, some more comments. > 1) I did not see where you check for whether the > target instruction of the djprobe is relocatable. > Are you currently assuming that the target instruction > is simply relocatable? In this version, that check must be done in the user-space, because parsing and analyzing CISC instructions are too hard to be done in the kernel code. Oshima-san is working for developing this check routine. > 2) You are not fully populating the pt_regs which the probe handler receives, > ( you have bogus values in eflags, eip, esp, etc) so I am not sure whether > this is a must for instrumentation code who might need these values. OK, I'll fix that. > [...] >> +int __kprobes djprobe_kprobe(struct kprobe *kp) > Did not see where you are using this function. This function is called from kernel/kprobe.c:register_kprobe() >> + >> +static __always_inline >> + struct djprobe_instance *__create_djprobe_instance(struct arch_djprobe_param >> + *param) >> +{ >> + struct djprobe_instance *djpi; >> + void * addr = (void *)djprobe_param_address(param); >> + /* allocate a new instance */ >> + djpi = kcalloc(1, sizeof(struct djprobe_instance), GFP_ATOMIC); > I think you can use GFP_KERNEL. Indeed. >> + if (djpi == NULL) { >> + goto out; >> + } >> + >> + /* allocate stub */ >> + djpi->stub.insn = __get_insn_slot(&djprobe_insn_pages); >> + if (djpi->stub.insn == NULL) { >> + __free_djprobe_instance(djpi); > kfree(djpi) should do. Thanks. I'll fix it. > [...] >> + >> + if ((ret = in_kprobes_functions((long)addr)) != 0) >> + return ret; >> + >> + mutex_lock(&djprobe_mutex); >> + >> + /* check confliction with other djprobes */ >> + djpi = __get_djprobe_instance(addr, len); >> + if (djpi) { >> + if (djpi->kp.addr == addr) { >> + djp->inst = djpi; /* add to another instance */ >> + list_add_rcu(&djp->plist, &djpi->plist); >> + } else { >> + ret = -EEXIST; /* other djprobes were inserted */ >> + } >> + goto out; >> + } >> + /* check confliction with kprobes */ >> + for (i = 1; i < len; i++) { >> + if (get_kprobe((void *)((long)addr + i))) { >> + ret = -EEXIST; /* a kprobes were inserted */ >> + goto out; >> + } >> + } > You need similar check in the kprobes registration, as we don;t want > to insert kprobes on the djprobe where you have jump target address. I checked it in the kernel/kprobes.c as below: > --- linux-2.6.19-rc1-mm1.orig/kernel/kprobes.c 2006-10-16 22:04:22.000000000 +0900 > +++ linux-2.6.19-rc1-mm1/kernel/kprobes.c 2006-10-16 22:06:22.000000000 +0900 [...] > @@ -582,6 +583,16 @@ > probed_mod = NULL; > } > > +#ifdef CONFIG_DJPROBE > + if (!djprobe_kprobe(p)) { > + mutex_lock(&djprobe_mutex); > + if (__get_djprobe_instance(p->addr, 1) != NULL) { This __get_djprobe_instance() returns the djprobe_instance which partially or fully covers the specified region. > + mutex_unlock(&djprobe_mutex); > + return -EEXIST; > + } > + } > +#endif /* CONFIG_DJPROBE */ > + Best regards, -- Masami HIRAMATSU Linux Technology Center Hitachi, Ltd., Systems Development Laboratory E-mail: masami.hiramatsu.pt@hitachi.com