From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19508 invoked by alias); 27 Oct 2006 23:34:51 -0000 Received: (qmail 19490 invoked by uid 22791); 27 Oct 2006 23:34:49 -0000 X-Spam-Status: No, hits=0.7 required=5.0 tests=AWL,BAYES_00,NO_DNS_FOR_FROM,TW_DJ,TW_JP,UNPARSEABLE_RELAY X-Spam-Check-By: sourceware.org Received: from mga09.intel.com (HELO mga09.intel.com) (134.134.136.24) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 27 Oct 2006 23:34:48 +0000 Received: from orsmga001.jf.intel.com ([10.7.209.18]) by mga09.intel.com with ESMTP; 27 Oct 2006 16:34:46 -0700 Received: from bambi.jf.intel.com ([134.134.19.188]) by orsmga001.jf.intel.com with ESMTP; 27 Oct 2006 16:34:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: i="4.09,366,1157353200"; d="scan'208"; a="151843361:sNHT20770981" Received: from bambi.jf.intel.com (bambi.jf.intel.com [127.0.0.1]) by bambi.jf.intel.com (8.13.7/8.13.7) with ESMTP id k9RNdgVX019840; Fri, 27 Oct 2006 16:39:42 -0700 Received: (from askeshav@localhost) by bambi.jf.intel.com (8.13.7/8.13.7/Submit) id k9RNdfQC019839; Fri, 27 Oct 2006 16:39:41 -0700 Date: Fri, 27 Oct 2006 23:34:00 -0000 From: Keshavamurthy@bambi.jf.intel.com, Anil S To: Masami Hiramatsu Cc: "Keshavamurthy, Anil S" , Ananth N Mavinakayanahalli , Prasanna S Panchamukhi , Ingo Molnar , SystemTAP , Satoshi Oshima , Hideo Aoki , Yumiko Sugita Subject: Re: [PATCH 2/5][djprobe] djprobe core patch Message-ID: <20061027233941.GB10018@bambi.jf.intel.com> Reply-To: Keshavamurthy@bambi.jf.intel.com, Anil S References: <45338593.6090207@hitachi.com> <45373F28.1070305@hitachi.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <45373F28.1070305@hitachi.com> User-Agent: Mutt/1.4.2.1i X-IsSubscribed: yes 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/msg00269.txt.bz2 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. 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). 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. 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. 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? 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. [...] > +int __kprobes djprobe_kprobe(struct kprobe *kp) Did not see where you are using this function. > +{ > + return kp->pre_handler == __djprobe_pre_handler; > +} [...] > + > +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. > + 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. > + djpi = NULL; > + goto out; > + } > + > + arch_prepare_djprobe_instance(djpi, param); [...] > + > + 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. regards, Anil Keshavamurthy