Re: [PATCH] kprobe-booster: boosting multi-probe

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

Hi Marami,
I refresh kprobe boost patch as follows, in this patch post_handler is 
seperated from break_handler. And I think booster can be enabled even 
when preempt is disabled.

--- 2.6.16-rc6-mm1.org/kernel/kprobes.c	2006-03-13 12:25:18.000000000 +0800
+++ 2.6.16-rc6-mm1/kernel/kprobes.c	2006-03-13 12:57:28.000000000 +0800
@@ -368,16 +368,15 @@ static inline void copy_kprobe(struct kp
  */
  static int __kprobes add_new_kprobe(struct kprobe *old_p, struct 
kprobe *p)
  {
-        struct kprobe *kp;
-
  	if (p->break_handler) {
-		list_for_each_entry_rcu(kp, &old_p->list, list) {
-			if (kp->break_handler)
-				return -EEXIST;
-		}
+		if (old_p->break_handler)
+			return -EEXIST;
  		list_add_tail_rcu(&p->list, &old_p->list);
+		old_p->break_handler = aggr_break_handler;
  	} else
  		list_add_rcu(&p->list, &old_p->list);
+	if (p->post_handler && !old_p->post_handler)
+		old_p->post_handler = aggr_post_handler;
  	return 0;
  }

@@ -390,9 +389,12 @@ static inline void add_aggr_kprobe(struc
  	copy_kprobe(p, ap);
  	ap->addr = p->addr;
  	ap->pre_handler = aggr_pre_handler;
-	ap->post_handler = aggr_post_handler;
  	ap->fault_handler = aggr_fault_handler;
-	ap->break_handler = aggr_break_handler;
+	if (p->post_handler)
+		ap->post_handler = aggr_post_handler;
+	if (p->break_handler)
+		ap->break_handler = aggr_break_handler;
+

  	INIT_LIST_HEAD(&ap->list);
  	list_add_rcu(&p->list, &ap->list);
@@ -537,6 +539,18 @@ valid_p:
  		}
  		arch_remove_kprobe(p);
  	}
+	else{
+		mutex_lock(&kprobe_mutex);
+		if (p->break_handler)
+			old_p->break_handler = NULL;
+		if (p->post_handler){
+			list_for_each_entry_rcu(list_p, &old_p->list, list){
+				if (list_p->post_handler) break;
+				old_p->post_handler = NULL;
+			}
+		}
+		mutex_unlock(&kprobe_mutex);
+	}
  }

  static struct notifier_block kprobe_exceptions_nb = {
--- 2.6.16-rc6-mm1.org/arch/i386/kernel/kprobes.c	2006-03-13 
12:41:01.000000000 +0800
+++ 2.6.16-rc6-mm1/arch/i386/kernel/kprobes.c	2006-03-13 
13:00:21.000000000 +0800
@@ -205,9 +205,6 @@ static int __kprobes kprobe_handler(stru
  	int ret = 0;
  	kprobe_opcode_t *addr;
  	struct kprobe_ctlblk *kcb;
-#ifdef CONFIG_PREEMPT
-	unsigned pre_preempt_count = preempt_count();
-#endif /* CONFIG_PREEMPT */

  	addr = (kprobe_opcode_t *)(regs->eip - sizeof(kprobe_opcode_t));

@@ -293,22 +290,14 @@ static int __kprobes kprobe_handler(stru
  		/* handler has already set things up, so skip ss setup */
  		return 1;

-	if (p->ainsn.boostable == 1 &&
-#ifdef CONFIG_PREEMPT
-	    !(pre_preempt_count) && /*
-				       * This enables booster when the direct
-				       * execution path aren't preempted.
-				       */
-#endif /* CONFIG_PREEMPT */
-	    !p->post_handler && !p->break_handler ) {
+ss_probe:
+	if (p->ainsn.boostable == 1 && !p->post_handler) {
  		/* Boost up -- we can execute copied instructions directly */
  		reset_current_kprobe();
  		regs->eip = (unsigned long)p->ainsn.insn;
  		preempt_enable_no_resched();
  		return 1;
  	}
-
-ss_probe:
  	prepare_singlestep(p, regs);
  	kcb->kprobe_status = KPROBE_HIT_SS;
  	return 1;


Masami Hiramatsu wrote:
> Hi, Prasanna
> 
> Prasanna S Panchamukhi wrote:
>  > Masami,
>  >
>  > Please see few coding style issues inline below marked with "^^^".
> 
> Thank you. I fixed those issues.
> 
>  > Thanks
>  > Prasanna
>  >> +static inline void boost_aggr_kprobe(struct kprobe *ap)
>  >> +{
>  >> +        struct kprobe *kp;
>  >> +    if (ap->post_handler || ap->break_handler) {
>  >       ^^^^^^^^^^^
>  > Could you please, leave a line after local variables as shown below
>  >         struct kprobe *kp;
>  >
>  >       if (ap->post_handler || ap->break_handler) {
>  >
>  >
>  >>                      kfree(old_p);
>  >>              }
>  >>              arch_remove_kprobe(p);
>  >> +    } else {
>  >> +            boost_aggr_kprobe(old_p);
>  >>      }
>  > ^^^^^^^^^
>  > This does not look good, could you please remove the "{" for else
>  > condition, since it is just a single line, as shown below
>  >       else
>  >               boost_aggr_kprobe(old_p);
> 
> 
> --
> Masami HIRAMATSU
> 2nd Research Dept.
> Hitachi, Ltd., Systems Development Laboratory
> E-mail: hiramatu@sdl.hitachi.co.jp
> 
>  kprobes.c |   28 ++++++++++++++++++++++++----
>  1 files changed, 24 insertions(+), 4 deletions(-)
> diff -Narup a/kernel/kprobes.c b/kernel/kprobes.c
> --- a/kernel/kprobes.c  2006-02-15 10:48:32.000000000 +0900
> +++ b/kernel/kprobes.c  2006-02-17 19:03:21.000000000 +0900
> @@ -370,6 +370,11 @@ static int __kprobes add_new_kprobe(stru
>  {
>          struct kprobe *kp;
> 
> +       if (p->post_handler || p->break_handler) { /* For more 
> boostability */
> +               old_p->post_handler = aggr_post_handler;
> +               old_p->break_handler = aggr_break_handler;
> +       }
> +
>         if (p->break_handler) {
>                 list_for_each_entry_rcu(kp, &old_p->list, list) {
>                         if (kp->break_handler)
> @@ -390,16 +395,30 @@ static inline void add_aggr_kprobe(struc
>         copy_kprobe(p, ap);
>         ap->addr = p->addr;
>         ap->pre_handler = aggr_pre_handler;
> -       ap->post_handler = aggr_post_handler;
>         ap->fault_handler = aggr_fault_handler;
> -       ap->break_handler = aggr_break_handler;
> -
> +       if (p->post_handler || p->break_handler) { /* For more 
> boostability */
> +               ap->post_handler = aggr_post_handler;
> +               ap->break_handler = aggr_break_handler;
> +       }
>         INIT_LIST_HEAD(&ap->list);
>         list_add_rcu(&p->list, &ap->list);
> 
>         hlist_replace_rcu(&p->hlist, &ap->hlist);
>  }
> 
> +static inline void boost_aggr_kprobe(struct kprobe *ap)
> +{
> +       struct kprobe *kp;
> +       if (ap->post_handler || ap->break_handler) {
> +               list_for_each_entry_rcu(kp, &ap->list, list) {
> +                       if (kp->post_handler || kp->break_handler)
> +                               return;
> +               }
> +       }
> +       ap->post_handler = NULL;
> +       ap->break_handler = NULL;
> +}
> +
>  /*
>   * This is the second or subsequent kprobe at the address - handle
>   * the intricacies
> @@ -536,7 +555,8 @@ valid_p:
>                         kfree(old_p);
>                 }
>                 arch_remove_kprobe(p);
> -       }
> +       } else
> +               boost_aggr_kprobe(old_p);
>  }
> 
>  static struct notifier_block kprobe_exceptions_nb = {
>