From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2642 invoked by alias); 6 Oct 2005 19:57:43 -0000 Mailing-List: contact systemtap-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Post: List-Help: , Sender: systemtap-owner@sources.redhat.com Received: (qmail 2606 invoked by uid 22791); 6 Oct 2005 19:57:40 -0000 Date: Thu, 06 Oct 2005 19:57:00 -0000 From: Keshavamurthy Anil S To: Masami Hiramatsu Cc: Keshavamurthy Anil S , systemtap@sources.redhat.com, Satoshi Oshima , Hideo Aoki , sugita@sdl.hitachi.co.jp Subject: Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes Message-ID: <20051006125717.A18501@unix-os.sc.intel.com> Reply-To: Keshavamurthy Anil S References: <433BE533.9080501@sdl.hitachi.co.jp> <20051003162917.A16966@unix-os.sc.intel.com> <434490F0.3030400@sdl.hitachi.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5.1i In-Reply-To: <434490F0.3030400@sdl.hitachi.co.jp>; from hiramatu@sdl.hitachi.co.jp on Thu, Oct 06, 2005 at 11:50:24AM +0900 X-Scanned-By: MIMEDefang 2.52 on 10.3.253.9 X-SW-Source: 2005-q4/txt/msg00015.txt.bz2 On Thu, Oct 06, 2005 at 11:50:24AM +0900, Masami Hiramatsu wrote: > Hi, Anil > > Keshavamurthy Anil S wrote: > > On Thu, Sep 29, 2005 at 05:59:31AM -0700, Masami Hiramatsu wrote: > > > > I see your patch has lots of line wrap and hard to review. > > Would you say that my patch has wrong indentation? Or would the > way to separate the patch be wrong? > I would like to correct wrong points. I guess it is to do with your mailer, I have seen something like this when I used to send my mails from MS outlook and now I have moved to mutt and don't have any problems. > > > With great difficulty, I have reviewed your code, > > please see my comments below. > > Thanks a lot! Your are very welcome. > > > Main highlights: > > 1) You code does not work if a new CPU comes online or existing cpu goes offline > > I have no machine which can plug/unplug CPUs. So I can not test > that feature. But I will try to develop a patch that djprobe can > work with CPU-Hotplug. > Would you have that machine? If I develop a patch, would you help > me to test? To test this you don;t have to have a machine which can plug/unplug CPUs, just enable CONFIG_HOTPLUG_CPU and when this kernel boots, you see /sys/devices/system/cpu/cpuX/online file. echo'ing '0' onto this file will offline the cpu and echo'ing '1' onto this file will bring the cpu online there by affecting cpu_online_map on which you are depending on. > > > 2) suggestion: Create djprobe.c and djprobe.h files instead of pushing them into > existing files > > Djprobe uses kprobes' internal function (__get_insn_slot()) to > allocate stub code buffers. And Kprobe has to check whether the > code at the insertion address was already modified by djprobe. > So we need to apply some patches to kprobes.c. > Additionally, I think the djprobe is one of the probes, so it > should be included in kprobes.c. This is easier to use for other > developers. I understand that you need some infrastructural changes to existing Kprobes and I have no problems you modifying the existing kprobes to accommodate djprobes, what I ment was the core djprobe logic can go into separate file for better readability and for some architecture which does not yet support djprobe can exclude djprobe.c file from compiling. > > > 3) Use good hashing algorithm to search djprobe instances. > > Hash_list is useful if a node has only one key value. Unfortunately, > a djprobe has a range of address as key value. Humm..This is not a excuse for not to use hashing algorithm :-) > __get_djprobe_instance() function is used for searching overlapping > of address ranges. So hash_list is not useful for djprobe. > And, I think the hash list is not so efficient for performance > improvement for the djprobe. Because, the djprobe needs to search > its instance only when it registers probes and executes deferred > processing. It does NOT search when the probe was hit. With the Djprobe, the number of probes that can be inserted will move to the order of few thousands. And for every single register or unregister djprobes call, searching this thousands of entries is not at all an efficient way to implement something in the kernel. Also, I see the same linear search happening inside work_check_djprobe_instance(). As I understand you are scheduling this function on all the cpus and inside this function you are doing a linear search for djprobe instances that too holding a spin lock and thus making other thread executing this function on different cpus to wait untill you finish serial search on this cpu. Hence my suggestion to look into optimizing this search. > > > 4)Handle the case where ARCH does not support djprobes transparently. > > I think the transparency of djprobe means that the source code > which uses the djprobe does not need any modification when ARCH > does not support djprobes. From this point of view, current code > works transparently. > > But current implementation is not so good. I will design it again. Okay, thanks. > > > 5) optimize djprobe data struct > > Exactly. I know it is not optimized enough. I think I can reduce > the size of djprobe data structure by moving some members of it > to arguments of register_djprobe() function. > What would you think about it? Yes, that would be better and you can store those extra arguments in the djprobe instance structure for later use. > > > 6) Not convinced how you are atomically updating 5 Bytes (jmp inst ) > guaranteeing that > > none of the other processor are executing near those address. > > The half of the code which guarantees that none of the other > processor are executing near those address is included in the > other (arch-depend) patch. Djprobe makes a bypass route from > copy of original codes which will be replaced by jmp code. After > all processors finished executing the original codes (and it will > execute the bypass route next time) djprobe inserts jmp code. You > can see this detail in the answer of 8th Question in Q&A text > (Answer of Q:How does the djprobe guarantee no threads and no > processors are executing the modifying area?). I will look at this again and will get back to you later. Thanks, Anil Keshavamurthy