Re: [RFC][PATCH][kprobe] enabling booster on the preemptible kernel, take 2

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.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