[PATCH] kprobe-booster: boosting multi-probe

[PATCH] kprobe-booster: boosting multi-probe

[Masami Hiramatsu]

Hi,

I developed a patch to enhance boost-ability of multi-probes.
Please review it.

Currently, the boosting process of a kprobe is disabled by
inserting another probe to the same address.
This will be occurred easily, because both of a kprobe and
a kretprobe are inserted to the top of a function when a
SystemTap user would like to measure the processing time
of that function.

One of the reasons of this degradation is the aggregator kprobe
always registers four kind of handlers(pre_handler, post_handler,
break_handler and fault_handler) even if aggregated probes do NOT
have post_handlers and break_handler. The boosted kprobe can
NOT have post_handler and break_handler, because boosting process
skips post processing of probing to accelerate process.
Also, the break_handler is special handler. The execution path of
break_handler skips boosting routine. So, currently the probe that
has break_handler is not boosted.

Multi-probe booster
===================
- Boosting the probes which are inserted to the same address as
  far as those do not have post_handler and break_handler.
- Set post_handler and break_handler of the aggregator probe to
  NULL if the aggregated probe does not have post_handler and
  break_handler.


This patch is elementary against linux-2.6.16-rc2-mm1.
Before applying this patch, you must apply the patch posted by
Mao Bibo which can be accessed from following URL:
http://sources.redhat.com/bugzilla/show_bug.cgi?id=2294#c3

-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp

 kprobes.c |   27 ++++++++++++++++++++++++---
 1 files changed, 24 insertions(+), 3 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-16 16:56:12.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,6 +555,8 @@ valid_p:
 			kfree(old_p);
 		}
 		arch_remove_kprobe(p);
+	} else {
+		boost_aggr_kprobe(old_p);
 	}
 }

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

[Prasanna S Panchamukhi]

Masami,

Please see few coding style issues inline below marked with "^^^".

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);



-- 
Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Email: prasanna@in.ibm.com
Ph: 91-80-51776329

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

[Masami Hiramatsu]

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 = {

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

[bibo,mao]

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 = {
> 

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

[Masami Hiramatsu]

Hi, bibo

bibo,mao wrote:
> Hi Marami,
> I refresh kprobe boost patch as follows, in this patch post_handler is
> seperated from break_handler.

The part of the patch against kernel/kprobes.c seems good to me.
But I found a bug.

> @@ -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;
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^
When the head of the list does not have a post_handler, this code
always clear the aggr_kprobe's post_handler.

> +            }
> +        }
> +        mutex_unlock(&kprobe_mutex);
> +    }
>  }


And the next part is not comfortable to me.

> And I think booster can be enabled even
> when preempt is disabled.

I think your patch enables booster even when preemption is
enabled, and it may be dangerous. Some running processes can
be preempted by another process when it is executing the codes in
the instruction buffer. When the boosted-probe is deregistered, that
instruction buffer is removed. Then, those preempted processes
can't continue to run, because they can't back to the preempted address.
So, I think, for the safety, the booster should NOT be enabled when
preemption is enabled.
Please fix it.


-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp

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

[bibo,mao]

Masami Hiramatsu wrote:
> Hi, bibo
> 
> bibo,mao wrote:
>> Hi Marami,
>> I refresh kprobe boost patch as follows, in this patch post_handler is
>> seperated from break_handler.
> 
> The part of the patch against kernel/kprobes.c seems good to me.
> But I found a bug.
> 
>> @@ -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;
>                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> When the head of the list does not have a post_handler, this code
> always clear the aggr_kprobe's post_handler.
I have modified it.

> And the next part is not comfortable to me.
> 
> 
> I think your patch enables booster even when preemption is
> enabled, and it may be dangerous. Some running processes can
> be preempted by another process when it is executing the codes in
> the instruction buffer. When the boosted-probe is deregistered, that
> instruction buffer is removed. Then, those preempted processes
> can't continue to run, because they can't back to the preempted address.
> So, I think, for the safety, the booster should NOT be enabled when
> preemption is enabled.
> Please fix it.
It actually is one problem, now I fix it. But actually most time kprobe 
happen in preempt enable places, such as system call entry position, 
then booster function will lose its effect.



  arch/i386/kernel/kprobes.c |   14 ++------------
  kernel/kprobes.c           |   34 ++++++++++++++++++++++++++--------
  2 files changed, 28 insertions(+), 20 deletions(-)
--- 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-14 08:56:01.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,22 @@ 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){
+					cleanup_p = 2;
+					 break;
+				}
+			}
+			if (cleanup_p == 0)
+				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-14 
09:27:56.000000000 +0800
@@ -205,9 +205,7 @@ 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 +291,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 (pre_preempt_count && 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;

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

[Masami Hiramatsu]

Hi,

bibo,mao wrote:
> Masami Hiramatsu wrote:
>> I think your patch enables booster even when preemption is
>> enabled, and it may be dangerous. Some running processes can
>> be preempted by another process when it is executing the codes in
>> the instruction buffer. When the boosted-probe is deregistered, that
>> instruction buffer is removed. Then, those preempted processes
>> can't continue to run, because they can't back to the preempted address.
>> So, I think, for the safety, the booster should NOT be enabled when
>> preemption is enabled.
>> Please fix it.
> It actually is one problem, now I fix it. But actually most time kprobe
> happen in preempt enable places, such as system call entry position,
> then booster function will lose its effect.

Sure, I know. There are some solutions for it. One of the my idea
is to inhibit preemption in the instruction buffer by
checking interruption address at the interrupt handler.
However, I thought this should modify other kernel subsystem
and the patch would become too complex.
So, in the first release patch, I just disabled booster when
preemption is enabled.

After all, this patch seems good to me.
Thanks for the hack.


-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp

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

[Masami Hiramatsu]

Hi, bibo

Could you update your multi-booster patch against
the latest kernel and post it to LKML?
I think your implementation is better than mine.

Best regards,
-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp

[PATCH] kprobe-booster: boosting multi-probe

[bibo,mao]

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]

Andrew,
    If there are multi kprobes on the same probepoint, there
will be one extra aggr_kprobe on the head of kprobe list.
The aggr_kprobe has aggr_post_handler/aggr_break_handler
whether the other kprobe post_hander/break_handler is NULL
or not. This patch modifies this, only when there is one or
more kprobe in the list whose post_handler is not NULL,
post_handler of aggr_kprobe will be set as aggr_post_handler.

Signed-off-by: bibo, mao <bibo.mao@intel.com>

Thanks
bibo,mao

[-- Attachment #2: kprobe_booster_multiprobe.patch --]
[-- Type: text/x-patch, Size: 3096 bytes --]

diff -Nruap 2.6.17-rc1-mm3.org/arch/i386/kernel/kprobes.c 2.6.17-rc1-mm3/arch/i386/kernel/kprobes.c
--- 2.6.17-rc1-mm3.org/arch/i386/kernel/kprobes.c	2006-04-26 15:52:24.000000000 +0800
+++ 2.6.17-rc1-mm3/arch/i386/kernel/kprobes.c	2006-04-26 16:18:50.000000000 +0800
@@ -206,9 +206,7 @@ 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));
 
@@ -294,22 +292,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 (pre_preempt_count && 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;
diff -Nruap 2.6.17-rc1-mm3.org/kernel/kprobes.c 2.6.17-rc1-mm3/kernel/kprobes.c
--- 2.6.17-rc1-mm3.org/kernel/kprobes.c	2006-04-26 15:52:24.000000000 +0800
+++ 2.6.17-rc1-mm3/kernel/kprobes.c	2006-04-26 16:09:14.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,11 @@ 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);
@@ -536,6 +537,21 @@ valid_p:
 			kfree(old_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){
+					cleanup_p = 2;
+					break;
+				}
+			}
+			if (cleanup_p == 0)
+				old_p->post_handler = NULL;
+		}
+		mutex_unlock(&kprobe_mutex);
 	}
 }