From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6689 invoked by alias); 30 Oct 2006 14:07:14 -0000 Received: (qmail 6472 invoked by uid 22791); 30 Oct 2006 14:07:11 -0000 X-Spam-Status: No, hits=0.1 required=5.0 tests=AWL,BAYES_00,UNPARSEABLE_RELAY X-Spam-Check-By: sourceware.org Received: from mail9.hitachi.co.jp (HELO mail9.hitachi.co.jp) (133.145.228.44) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 30 Oct 2006 14:07:02 +0000 Received: from mlsv10.hitachi.co.jp (unknown [133.145.228.16]) by mail9.hitachi.co.jp (Postfix) with ESMTP id 29B7937C8D for ; Mon, 30 Oct 2006 23:07:00 +0900 (JST) Received: from mfilter-s5.hitachi.co.jp by mlsv10.hitachi.co.jp (8.12.11/8.12.11) id k9UE6xAO009731; Mon, 30 Oct 2006 23:06:59 +0900 Received: from vshuts5.hitachi.co.jp (unverified) by mfilter-s5.hitachi.co.jp (Content Technologies SMTPRS 4.3.17) with SMTP id ; Mon, 30 Oct 2006 23:06:59 +0900 Received: from hsdlgw92.sdl.hitachi.co.jp ([133.144.7.20]) by vshuts5.hitachi.co.jp with SMTP id M2006103023065903045 ; Mon, 30 Oct 2006 23:06:59 +0900 Received: from vgate2.sdl.hitachi.co.jp by hsdlgw92.sdl.hitachi.co.jp (8.13.1/3.7W06092911) id k9UE6wXi005929; Mon, 30 Oct 2006 23:06:58 +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 M2006103023065816982 ; Mon, 30 Oct 2006 23:06:58 +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 k9UE6wVl026136; Mon, 30 Oct 2006 23:06:58 +0900 Message-ID: <454606EC.1030807@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: "bibo,mao" Cc: "Keshavamurthy, Anil S" , Ananth N Mavinakayanahalli , Prasanna S Panchamukhi , Ingo Molnar , SystemTAP , Satoshi Oshima , Hideo Aoki , Yumiko Sugita Subject: Re: [RFC][PATCH][kprobe] enabling booster on the preemptible kernel, take 2 References: <45338593.6090207@hitachi.com> <45459D94.8060905@intel.com> In-Reply-To: <45459D94.8060905@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/msg00277.txt.bz2 Hi bibo, Thank you for your review! bibo,mao wrote: > This patch will boost kprobe on preemptible kernel, I think > it is deserved to waster some memory for better performance > by deferring memory free after freeze_processes. I think it doesn't waste memory so much, because it tries to reuse garbage memories before the kernel allocates an additional page. [...] >> +static int __kprobes collect_garbage_slots(void) >> +{ >> + struct kprobe_insn_page *kip; >> + struct hlist_node *pos, *next; >> + int ret = -1; >> + >> +#if defined(CONFIG_PREEMPT) && defined(CONFIG_PM) >> + /* Ensure no-one is preepmted on the garbages */ >> + if (freeze_processes() != 0) > I do not know whether there exists non-freezeable and preemptive kernel > thread, if there exist then this thread will not be frozen. In that case, freeze_processes() returns the positive value which means how many processes are not frozen. If freeze_processes() returns non-zero, this function aborts the garbage collection. >> + goto thaw_all; >> +#endif >> + hlist_for_each_safe(pos, next, &kprobe_insn_pages) { >> + int i; >> + kip = hlist_entry(pos, struct kprobe_insn_page, hlist); >> + if (kip->ngarbage == 0) >> + continue; >> + kip->ngarbage = 0; /* we will collect all >> garbages */ >> + for (i = 0; i < INSNS_PER_PAGE; i++) { >> + if (kip->slot_used[i] == -1 && >> + collect_one_slot(kip, i)) > if collect_one_slot executes kfree(kip) and return 0, then kernel will > continue > execute the for () loop sentence and access freed kip point by > kip->slot_used. Exactly, it's a bug. Thank you. I'll fix that. >> @@ -146,28 +215,18 @@ >> if (kip->insns <= slot && >> slot < kip->insns + (INSNS_PER_PAGE * >> MAX_INSN_SIZE)) { >> int i = (slot - kip->insns) / MAX_INSN_SIZE; >> - kip->slot_used[i] = 0; >> - kip->nused--; >> - if (kip->nused == 0) { >> - /* >> - * Page is no longer in use. Free it >> unless >> - * it's the last one. We keep the >> last one >> - * so as not to have to set it up >> again the >> - * next time somebody inserts a probe. >> - */ >> - hlist_del(&kip->hlist); >> - if (hlist_empty(&kprobe_insn_pages)) { >> - INIT_HLIST_NODE(&kip->hlist); >> - hlist_add_head(&kip->hlist, >> - &kprobe_insn_pages); >> - } else { >> - module_free(NULL, kip->insns); >> - kfree(kip); >> - } >> + if (dirty) { >> + kip->slot_used[i] = -1; >> + kip->ngarbage++; > it seems that break sentence is missing. Oh, it's my mistake. Thanks. >> + } else { >> + collect_one_slot(kip, i); >> + break; >> } >> - return; So, I will add a break here. Best regards, -- Masami HIRAMATSU Linux Technology Center Hitachi, Ltd., Systems Development Laboratory E-mail: masami.hiramatsu.pt@hitachi.com