This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [RFC][PATCH][kprobe] enabling booster on the preemptible kernel, take 2
- From: Masami Hiramatsu <masami dot hiramatsu dot pt at hitachi dot com>
- To: "bibo,mao" <bibo dot mao at intel dot com>
- Cc: "Keshavamurthy, Anil S" <anil dot s dot keshavamurthy at intel dot com>, Ananth N Mavinakayanahalli <ananth at in dot ibm dot com>, Prasanna S Panchamukhi <prasanna at in dot ibm dot com>, Ingo Molnar <mingo at redhat dot com>, SystemTAP <systemtap at sources dot redhat dot com>, Satoshi Oshima <soshima at redhat dot com>, Hideo Aoki <haoki at redhat dot com>, Yumiko Sugita <yumiko dot sugita dot yf at hitachi dot com>
- Date: Mon, 30 Oct 2006 23:06:36 +0900
- Subject: Re: [RFC][PATCH][kprobe] enabling booster on the preemptible kernel, take 2
- Organization: Systems Development Lab., Hitachi, Ltd., Japan
- References: <45338593.6090207@hitachi.com> <45459D94.8060905@intel.com>
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