From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15322 invoked by alias); 22 Dec 2005 06:07:09 -0000 Received: (qmail 14601 invoked by uid 22791); 22 Dec 2005 06:07:08 -0000 X-Spam-Status: No, hits=-0.9 required=5.0 tests=AWL,BAYES_00,DNS_FROM_RFC_POST X-Spam-Check-By: sourceware.org Received: from fmr17.intel.com (HELO orsfmr002.jf.intel.com) (134.134.136.16) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 22 Dec 2005 06:07:07 +0000 Received: from orsfmr100.jf.intel.com (orsfmr100.jf.intel.com [10.7.209.16]) by orsfmr002.jf.intel.com (8.12.10/8.12.10/d: major-outer.mc,v 1.1 2004/09/17 17:50:56 root Exp $) with ESMTP id jBM66vbo013511; Thu, 22 Dec 2005 06:06:59 GMT Received: from pdsmsxvs01.pd.intel.com (pdsmsxvs01.pd.intel.com [172.16.12.122]) by orsfmr100.jf.intel.com (8.12.10/8.12.10/d: major-inner.mc,v 1.2 2004/09/17 18:05:01 root Exp $) with SMTP id jBM66kgD025798; Thu, 22 Dec 2005 06:06:57 GMT Received: from pdsmsx331.ccr.corp.intel.com ([172.16.12.58]) by pdsmsxvs01.pd.intel.com (SAVSMTP 3.1.7.47) with SMTP id M2005122214065613757 ; Thu, 22 Dec 2005 14:06:56 +0800 Received: from pdsmsx404.ccr.corp.intel.com ([172.16.12.64]) by pdsmsx331.ccr.corp.intel.com with Microsoft SMTPSVC(6.0.3790.211); Thu, 22 Dec 2005 14:06:55 +0800 X-MimeOLE: Produced By Microsoft Exchange V6.5.7226.0 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="GB2312" Content-Transfer-Encoding: quoted-printable Subject: RE: Review patches of user space kprobe Date: Thu, 22 Dec 2005 07:14:00 -0000 Message-ID: <8126E4F969BA254AB43EA03C59F44E840447BDB3@pdsmsx404> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: Review patches of user space kprobe Thread-Index: AcYGumyfmpnCeRg0SeiuZNrn3vO/CQAAy8uw From: "Zhang, Yanmin" To: "Vara Prasad" Cc: , , "Keshavamurthy, Anil S" , "Mao, Bibo" X-OriginalArrivalTime: 22 Dec 2005 06:06:55.0943 (UTC) FILETIME=[E94E0D70:01C606BD] X-Scanned-By: MIMEDefang 2.52 on 10.7.209.16 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: 2005-q4/txt/msg00543.txt.bz2 Thanks for your info. I go through the patches 1 by 1. Perhaps later patche= s already fix the problems in prior patches, so my comments might be incorr= ect. :) Yanmin >>-----Original Message----- >>From: systemtap-owner@sourceware.org [mailto:systemtap-owner@sourceware.o= rg] On Behalf Of Vara Prasad >>Sent: 2005=C4=EA12=D4=C222=C8=D5 13:41 >>To: Zhang, Yanmin >>Cc: prasanna@in.ibm.com; systemtap@sources.redhat.com; Keshavamurthy, Ani= l S; Mao, Bibo >>Subject: Re: Review patches of user space kprobe >> >>Hi Yanmin, >> >>I appreciate your comments. Prasanna the author of the patch is on >>vacation until the month end, he will address your comments once he is >>back in Jan. Please feel free to finish the review of the 3rd patch as >>well in the mean time. >> >>Thanks, >>Vara Prasad >> >>Zhang, Yanmin wrote: >> >>>Below inline are the comments for patch 2/3. >>> >>>Yanmin >>> >>> >>> >>>>>Signed-of-by: Prasanna S Panchamukhi >>>>> >>>>> >>>>>--- >>>>> >>>>>linux-2.6.13-prasanna/include/linux/kprobes.h | 2 >>>>>linux-2.6.13-prasanna/kernel/kprobes.c | 113 >>>>> >>>>> >>>++++++++++++++++++++++++++ >>> >>> >>>>>2 files changed, 115 insertions(+) >>>>> >>>>>diff -puN kernel/kprobes.c~kprobes_userspace_probes-readpages >>>>> >>>>> >>>kernel/kprobes.c >>> >>> >>>>>--- linux-2.6.13/kernel/kprobes.c~kprobes_userspace_probes-readpages >>>>> >>>>> >>>2005-09-14 11:01:18.495513696 +0530 >>> >>> >>>>>+++ linux-2.6.13-prasanna/kernel/kprobes.c 2005-09-14 >>>>> >>>>> >>>11:01:18.550505336 +0530 >>> >>> >>>>>@@ -652,6 +652,109 @@ static struct uprobe_module *get_module_ >>>>>} >>>>> >>>>>/* >>>>>+ * Check if the given offset lies within given page range. >>>>>+ */ >>>>>+static int find_page_probe(unsigned long offset, unsigned long >>>>> >>>>> >>>page_start) >>> >>> >>>>>+{ >>>>>+ unsigned long page_end =3D page_start + PAGE_SIZE; >>>>>+ if (offset >=3D page_start && offset < page_end) >>>>>+ return 1; >>>>>+ return 0; >>>>>+} >>>>>+ >>>>>+/* >>>>>+ * This function handles the readpages of all modules that have >>>>> >>>>> >>>active probes >>> >>> >>>>>+ * in them. Here, we first call the original readpages() of this >>>>>+ * inode / address_space to actually read the pages into memory. >>>>> >>>>> >>>Then, we will >>> >>> >>>>>+ * insert all the probes that are specified in this pages before >>>>> >>>>> >>>returning. >>> >>> >>>>>+ */ >>>>>+static int up_readpages(struct file *file, struct address_space >>>>> >>>>> >>>*mapping, >>> >>> >>>>>+ struct list_head *pages, unsigned nr_pages) >>>>>+{ >>>>>+ int retval =3D 0; >>>>>+ struct page *page; >>>>>+ struct uprobe_module *m; >>>>>+ struct uprobe *up =3D NULL; >>>>>+ struct hlist_node *node; >>>>>+ >>>>>+ m =3D get_module_by_inode(file->f_dentry->d_inode); >>>>> >>>>> >>>There is a race condition between unregister_userspace_probe and here. >>>If a thread jumps to the beginning of function up_readpages while >>>another thread calls unregister_userspace_probe to delete the um, the >>>first thread might return error. >>> >>> >>> >>> >>>>>+ if (!m) { >>>>>+ printk("up_readpages: major problem. we don't \ >>>>>+ have mod for this >>>>> >>>>> >>>!!!\n"); >>> >>> >>>>>+ return -EINVAL; >>>>>+ } >>>>>+ >>>>>+ /* call original readpages() */ >>>>>+ retval =3D m->ori_a_ops->readpages(file, mapping, pages, >>>>> >>>>> >>>nr_pages); >>> >>> >>>>>+ if (retval >=3D 0) { >>>>>+ hlist_for_each_entry(up, node, &m->ulist_head, ulist) { >>>>>+ /* >>>>>+ * TODO: Walk through readpages page list and >>>>> >>>>> >>>get >>> >>> >>>>>+ * pages with probes instead of find_get_page(). >>>>>+ */ >>>>>+ if ((page =3D find_get_page(mapping, >>>>>+ up->offset >> PAGE_CACHE_SHIFT)) !=3D >>>>> >>>>> >>>NULL) { >>> >>> >>>>>+ if (find_page_probe >>>>>+ (up->offset >> PAGE_CACHE_SHIFT, >>>>>+ page->index << PAGE_CACHE_SHIFT)) { >>>>>+ up->page =3D page; >>>>>+ if (!map_uprobe_page(up, 0)) { >>>>>+ lock_page(up->page); >>>>> >>>>> >>>The first patch doesn't do lock_page before calling insert_probe_page. >>>Why does this patch do so? It's inconsistent. >>> >>> >>> >>> >>>>>+ insert_probe_page(up); >>>>>+ unmap_uprobe_page(up); >>>>>+ unlock_page(up->page); >>>>>+ } >>>>>+ } >>>>>+ page_cache_release(up->page); >>>>>+ } >>>>>+ } >>>>>+ } >>>>>+ return retval; >>>>>+} >>>>>+ >>>>>+/* >>>>>+ * This function handles the readpage of all modules that have active >>>>> >>>>> >>>probes >>> >>> >>>>>+ * in them. Here, we first call the original readpage() of this >>>>>+ * inode / address_space to actually read the page into memory. Then, >>>>> >>>>> >>>we will >>> >>> >>>>>+ * insert all the probes that are specified in this page before >>>>> >>>>> >>>returning. >>> >>> >>>>>+ */ >>>>>+int up_readpage(struct file *file, struct page *page) >>>>>+{ >>>>>+ int retval =3D 0; >>>>>+ struct uprobe_module *m; >>>>>+ struct uprobe *up =3D NULL; >>>>>+ int kprobe_page_mapped =3D 0; >>>>>+ struct hlist_node *node; >>>>>+ >>>>>+ m =3D get_module_by_inode(file->f_dentry->d_inode); >>>>> >>>>> >>>The same race condition like above function. >>> >>> >>> >>> >>>>>+ if (!m) { >>>>>+ printk("up_readpage: major problem. we don't have mod >>>>> >>>>> >>>for this !!!\n"); >>> >>> >>>>>+ return -EINVAL; >>>>>+ } >>>>>+ >>>>>+ /* call original readpage() */ >>>>>+ retval =3D m->ori_a_ops->readpage(file, page); >>>>>+ if (retval >=3D 0) { >>>>>+ hlist_for_each_entry(up, node, &m->ulist_head, ulist) { >>>>>+ if (find_page_probe (up->offset >> >>>>> >>>>> >>>PAGE_CACHE_SHIFT, >>> >>> >>>>>+ page->index << >>>>> >>>>> >>>PAGE_CACHE_SHIFT)) { >>> >>> >>>>>+ up->page =3D page; >>>>>+ if (!map_uprobe_page(up, >>>>> >>>>> >>>kprobe_page_mapped)) { >>> >>> >>>>>+ lock_page(up->page); >>>>> >>>>> >>>Same inconsistent issue. >>> >>> >>> >>> >>>>>+ kprobe_page_mapped =3D 1; >>>>>+ retval =3D insert_probe_page(up); >>>>>+ } >>>>>+ } >>>>>+ } >>>>>+ if (kprobe_page_mapped) { >>>>> >>>>> >>>The logic here is incorrect. If there are many uprobes at the same page, >>>up just points to the last one. How about others? >>> >>> >>> >>> >>> >>>>>+ unmap_uprobe_page(up); >>>>>+ unlock_page(up->page); >>>>>+ } >>>>>+ } >>>>>+ return retval; >>>>>+} >>>>>+ >>>>> >>>>> >>> >>> >>> >>> >>