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

["bibo,mao" <bibo.mao@intel.com>]

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.

thanks
bibo,mao

Masami Hiramatsu wrote:
> Hi,
> 
> Here is the patch which enables kprobe-booster on
> the preemptive kernel.
> 
> When we are unregistering a kprobe-booster, we can't release
> its buffer immediately on the preemptive kernel, because
> some processes might be preempted on the buffer.
> The freeze_processes() and thaw_processes() functions can
> clean those processes up from the buffer. However, the
> processing of those functions takes a long time.
> So, this patch introduces the garbage collection mechanism
> of insn_slot. It also introduces the "dirty" flag to
> free_insn_slot because of efficiency.
> 
> The "clean" instruction slots (dirty flag is cleared) are
> released immediately. But the "dirty" slots which are used
> by boosted kprobes, are marked as garbages.
> collect_garbage_slots() will be invoked to release "dirty"
> slots if 1) there are more than INSNS_PER_PAGE garbage slots
> or 2) there are no unused slots.
> 
> Thanks,
> --
> Masami HIRAMATSU
> 2nd Research Dept.
> Hitachi, Ltd., Systems Development Laboratory
> E-mail: masami.hiramatsu.pt@hitachi.com
> 
> 
> ---
>  arch/i386/kernel/kprobes.c    |    4 -
>  arch/ia64/kernel/kprobes.c    |    2
>  arch/powerpc/kernel/kprobes.c |    2
>  arch/s390/kernel/kprobes.c    |    2
>  arch/x86_64/kernel/kprobes.c  |    2
>  include/linux/kprobes.h       |    2
>  kernel/kprobes.c              |  101 +++++++++++++++++++++++++++++++++---------
>  7 files changed, 87 insertions(+), 28 deletions(-)
> 
> Index: linux-2.6.19-rc1-mm1/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.19-rc1-mm1.orig/kernel/kprobes.c  2006-10-16 10:40:02.000000000 +0900
> +++ linux-2.6.19-rc1-mm1/kernel/kprobes.c       2006-10-16 21:50:44.000000000 +0900
> @@ -38,6 +38,7 @@
>  #include <linux/module.h>
>  #include <linux/moduleloader.h>
>  #include <linux/kallsyms.h>
> +#include <linux/sched.h>
>  #include <asm-generic/sections.h>
>  #include <asm/cacheflush.h>
>  #include <asm/errno.h>
> @@ -83,9 +84,12 @@
>         kprobe_opcode_t *insns;         /* Page of instruction slots */
>         char slot_used[INSNS_PER_PAGE];
>         int nused;
> +       int ngarbage;
>  };
> 
>  static struct hlist_head kprobe_insn_pages;
> +static int kprobe_garbage_slots;
> +static int collect_garbage_slots(void);
> 
>  /**
>   * get_insn_slot() - Find a slot on an executable page for an instruction.
> @@ -96,6 +100,7 @@
>         struct kprobe_insn_page *kip;
>         struct hlist_node *pos;
> 
> +      retry:
>         hlist_for_each(pos, &kprobe_insn_pages) {
>                 kip = hlist_entry(pos, struct kprobe_insn_page, hlist);
>                 if (kip->nused < INSNS_PER_PAGE) {
> @@ -112,7 +117,11 @@
>                 }
>         }
> 
> -       /* All out of space.  Need to allocate a new page. Use slot 0.*/
> +       /* If there are any garbage slots, collect it and try again. */
> +       if (kprobe_garbage_slots && collect_garbage_slots() == 0) {
I am not familiar with freeze_processes/thaw_process, but I only think that
it will bring performance downgrade greatly for the moment.I think kmalloc
method is better than collect_garbage_slots.
> +               goto retry;
> +       }
> +       /* All out of space.  Need to allocate a new page. Use slot 0. */
>         kip = kmalloc(sizeof(struct kprobe_insn_page), GFP_KERNEL);
>         if (!kip) {
>                 return NULL;
> @@ -133,10 +142,70 @@
>         memset(kip->slot_used, 0, INSNS_PER_PAGE);
>         kip->slot_used[0] = 1;
>         kip->nused = 1;
> +       kip->ngarbage = 0;
>         return kip->insns;
>  }
> 
> -void __kprobes free_insn_slot(kprobe_opcode_t *slot)
> +/* Return 1 if all garbages are collected, otherwise 0. */
> +static int __kprobes collect_one_slot(struct kprobe_insn_page *kip, int idx)
> +{
> +       kip->slot_used[idx] = 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);
> +                       return 1;
> +               } else {
> +                       module_free(NULL, kip->insns);
> +                       kfree(kip);
> +               }
> +       }
> +       return 0;
> +}
> +
> +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. 
> +               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.
> +                               goto collected;
> +               }
> +       }
> +      collected:
> +       kprobe_garbage_slots = 0;
> +       ret = 0;
> +#if defined(CONFIG_PREEMPT) && defined(CONFIG_PM)
> +      thaw_all:
> +       thaw_processes();
> +#endif
> +       return ret;
> +}
> +
> +void __kprobes free_insn_slot(kprobe_opcode_t * slot, int dirty)
>  {
>         struct kprobe_insn_page *kip;
>         struct hlist_node *pos;
> @@ -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.
> +                       } else {
> +                               collect_one_slot(kip, i);
> +                               break;
>                         }
> -                       return;
>                 }
>         }
> +       if (dirty && (++kprobe_garbage_slots > INSNS_PER_PAGE)) {
> +               collect_garbage_slots();
> +       }
>  }
>  #endif
> 
> Index: linux-2.6.19-rc1-mm1/arch/i386/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.19-rc1-mm1.orig/arch/i386/kernel/kprobes.c        2006-10-16 
> 10:40:00.000000000 +0900
> +++ linux-2.6.19-rc1-mm1/arch/i386/kernel/kprobes.c     2006-10-16 
> 21:43:03.000000000 +0900
> @@ -184,7 +184,7 @@
>  void __kprobes arch_remove_kprobe(struct kprobe *p)
>  {
>         mutex_lock(&kprobe_mutex);
> -       free_insn_slot(p->ainsn.insn);
> +       free_insn_slot(p->ainsn.insn, (p->ainsn.boostable == 1));
>         mutex_unlock(&kprobe_mutex);
>  }
> 
> @@ -333,7 +333,7 @@
>                 return 1;
> 
>  ss_probe:
> -#ifndef CONFIG_PREEMPT
> +#if !defined(CONFIG_PREEMPT) || defined(CONFIG_PM)
>         if (p->ainsn.boostable == 1 && !p->post_handler){
>                 /* Boost up -- we can execute copied instructions directly */
>                 reset_current_kprobe();
> Index: linux-2.6.19-rc1-mm1/arch/ia64/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.19-rc1-mm1.orig/arch/ia64/kernel/kprobes.c        2006-10-16 
> 10:40:00.000000000 +0900
> +++ linux-2.6.19-rc1-mm1/arch/ia64/kernel/kprobes.c     2006-10-16 
> 10:54:09.000000000 +0900
> @@ -481,7 +481,7 @@
>  void __kprobes arch_remove_kprobe(struct kprobe *p)
>  {
>         mutex_lock(&kprobe_mutex);
> -       free_insn_slot(p->ainsn.insn);
> +       free_insn_slot(p->ainsn.insn, 0);
>         mutex_unlock(&kprobe_mutex);
>  }
>  /*
> Index: linux-2.6.19-rc1-mm1/arch/powerpc/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.19-rc1-mm1.orig/arch/powerpc/kernel/kprobes.c     2006-10-16 
> 10:40:00.000000000 +0900
> +++ linux-2.6.19-rc1-mm1/arch/powerpc/kernel/kprobes.c  2006-10-16 
> 10:54:09.000000000 +0900
> @@ -85,7 +85,7 @@
>  void __kprobes arch_remove_kprobe(struct kprobe *p)
>  {
>         mutex_lock(&kprobe_mutex);
> -       free_insn_slot(p->ainsn.insn);
> +       free_insn_slot(p->ainsn.insn, 0);
>         mutex_unlock(&kprobe_mutex);
>  }
> 
> Index: linux-2.6.19-rc1-mm1/arch/s390/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.19-rc1-mm1.orig/arch/s390/kernel/kprobes.c        2006-10-16 
> 10:40:00.000000000 +0900
> +++ linux-2.6.19-rc1-mm1/arch/s390/kernel/kprobes.c     2006-10-16 
> 10:54:09.000000000 +0900
> @@ -200,7 +200,7 @@
>  void __kprobes arch_remove_kprobe(struct kprobe *p)
>  {
>         mutex_lock(&kprobe_mutex);
> -       free_insn_slot(p->ainsn.insn);
> +       free_insn_slot(p->ainsn.insn, 0);
>         mutex_unlock(&kprobe_mutex);
>  }
> 
> Index: linux-2.6.19-rc1-mm1/arch/x86_64/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.19-rc1-mm1.orig/arch/x86_64/kernel/kprobes.c      2006-10-16 
> 10:40:00.000000000 +0900
> +++ linux-2.6.19-rc1-mm1/arch/x86_64/kernel/kprobes.c   2006-10-16 
> 10:54:09.000000000 +0900
> @@ -224,7 +224,7 @@
>  void __kprobes arch_remove_kprobe(struct kprobe *p)
>  {
>         mutex_lock(&kprobe_mutex);
> -       free_insn_slot(p->ainsn.insn);
> +       free_insn_slot(p->ainsn.insn, 0);
>         mutex_unlock(&kprobe_mutex);
>  }
> 
> Index: linux-2.6.19-rc1-mm1/include/linux/kprobes.h
> ===================================================================
> --- linux-2.6.19-rc1-mm1.orig/include/linux/kprobes.h   2006-10-16 
> 10:40:02.000000000 +0900
> +++ linux-2.6.19-rc1-mm1/include/linux/kprobes.h        2006-10-16 
> 21:43:07.000000000 +0900
> @@ -165,7 +165,7 @@
>  extern int arch_init_kprobes(void);
>  extern void show_registers(struct pt_regs *regs);
>  extern kprobe_opcode_t *get_insn_slot(void);
> -extern void free_insn_slot(kprobe_opcode_t *slot);
> +extern void free_insn_slot(kprobe_opcode_t *slot, int dirty);
>  extern void kprobes_inc_nmissed_count(struct kprobe *p);
> 
>  /* Get the kprobe at this addr (if any) - called with preemption disabled */
>