[PATCH] Kprobe:multi kprobe posthandler for booster

[PATCH] Kprobe:multi kprobe posthandler for booster

[mao, bibo]

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

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

Re: [PATCH] Kprobe:multi kprobe posthandler for booster

[Andrew Morton]

"mao, bibo" <bibo.mao@intel.com> wrote:
>
>   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.

OK...  But you didn't provide me with sufficient information with which I
can gauge the seriousness of this problem.  Show-stopper bug?  Minor
performance problem?  Hard to tell, and I do need that information to be
able to judge whether patches should do into -rc or into the next kernel
cycle, as well as being backported into -stable.

I currently have four patches:

kprobe-boost-2byte-opcodes-on-i386.patch
kprobe-fix-resume-execution-on-i386.patch
kprobemulti-kprobe-posthandler-for-booster.patch
kprobe-cleanup-for-vm_mask-judgement.patch

I _think_ the first two are 2.6.17 material and the latter two are 2.6.18
material.  Could others please comment?

Re: [PATCH] Kprobe:multi kprobe posthandler for booster

[Prasanna S Panchamukhi]

Hi Andrew,

Please find the details below.

On Wed, Apr 26, 2006 at 02:55:08PM -0700, Andrew Morton wrote:
> "mao, bibo" <bibo.mao@intel.com> wrote:
> >
> >   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.
> 
> OK...  But you didn't provide me with sufficient information with which I
> can gauge the seriousness of this problem.  Show-stopper bug?  Minor
> performance problem?  Hard to tell, and I do need that information to be
> able to judge whether patches should do into -rc or into the next kernel
> cycle, as well as being backported into -stable.
> 

Yes, sorry for not providing enough information.

> I currently have four patches:
> 
> kprobe-boost-2byte-opcodes-on-i386.patch
> kprobe-fix-resume-execution-on-i386.patch
> kprobemulti-kprobe-posthandler-for-booster.patch
> kprobe-cleanup-for-vm_mask-judgement.patch
> 
> I _think_ the first two are 2.6.17 material and the latter two are 2.6.18
> material.  Could others please comment?

kprobe-cleanup-for-vm_mask-judgement.patch
This patch is a minor cleanup patch and can go into 2.6.17.

kprobe-fix-resume-execution-on-i386.patch
This patch is a minor bug fix and can go into 2.6.17. This patch
needs to be reviewed and tested before it can make into the mainline.

kprobe-boost-2byte-opcodes-on-i386.patch
kprobemulti-kprobe-posthandler-for-booster.patch
These two patches solve minor performance problem and can go into 2.6.18, but
they need more reivew and testing before they can make into mainline.

Also the patches does not contain enough explanation, hence request
all of you to provide more explanation along with the patch.

Plesae let me know if you need more information.

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

Re: [PATCH] Kprobe:multi kprobe posthandler for booster

[Masami Hiramatsu]

Hi, Andrew

Andrew Morton wrote:
> OK...  But you didn't provide me with sufficient information with which I
> can gauge the seriousness of this problem.  Show-stopper bug?  Minor
> performance problem?  Hard to tell, and I do need that information to be
> able to judge whether patches should do into -rc or into the next kernel
> cycle, as well as being backported into -stable.

I'm sorry not to provide detail information.
I described it below.

> I currently have four patches:
> 
> kprobe-boost-2byte-opcodes-on-i386.patch

This is an enhancement patch of kprobe-booster on i386.
In the previous patch
(http://marc.theaimsgroup.com/?l=linux-kernel&m=114104159911612&w=4),
I decided that the first booster patch didn't boost the 2bytes
opcodes and prefixes, because of simplicity of the patch.

In this patch, I identified "boostable" 2 byte instructions as below;
- Conditional jumps can not be boost.
- WRMSR, RDMSR, CPUID, RSM, and RDPMC may have unexpected side effects.
  Thus these can not be boost.
- Extended Groups are including some undefined opcodes. Thus
  these can not be boost.
- Invalid opcode, UD2 and other undefined opcodes can not be boost.
- other defined opcodes can be boost.

I also identified prefixes again:
- Address size prefix(0x67) and CS segment override(0x2e) may change
  valid address range. So these can not be boost.
- If there is other prefixes, there is a chance to boost it.

> kprobe-fix-resume-execution-on-i386.patch

This patch enables kprobe's resume_execution() to handle
iret(0xcf) and absolute-call(0x9a) correctly.
It handles those operations as below;
- iret
 After the "iret" was executed, the value of eip register
was restored from the stack. Thus resume_execution() handles
it as same as "ret".
- absolute-call
 The target address of the "absolute-call" is specified as an
absolute address. Thus no need to fix eip.


Best regards,

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

RE: [PATCH] Kprobe:multi kprobe posthandler for booster

[Mao, Bibo]

Andrew,

This patch solves minor performance problem, it is based on http://marc.theaimsgroup.com/?l=linux-kernel&m=114104159911612&w=4, Kprobe-booster can take effect only when post_handler is NULL, however when there are multiple kprobes on the same probepoint, post_handler of aggr_kprobe will be set. This patch modifies this, post_handler of aggr_kprobe will be set only if there exist one kprobe whose post_hander is not NULL.

kprobe-cleanup-for-vm_mask-judgement.patch is irrelative with booster kprobe.

Thanks
bibo,mao

>-----Original Message-----
>From: Andrew Morton [mailto:akpm@osdl.org]
>Sent: 2006年4月27日 5:55
>To: Mao, Bibo
>Cc: systemtap@sources.redhat.com
>Subject: Re: [PATCH] Kprobe:multi kprobe posthandler for booster
>
>"mao, bibo" <bibo.mao@intel.com> wrote:
>>
>>   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.
>
>OK...  But you didn't provide me with sufficient information with which I
>can gauge the seriousness of this problem.  Show-stopper bug?  Minor
>performance problem?  Hard to tell, and I do need that information to be
>able to judge whether patches should do into -rc or into the next kernel
>cycle, as well as being backported into -stable.
>
>I currently have four patches:
>
>kprobe-boost-2byte-opcodes-on-i386.patch
>kprobe-fix-resume-execution-on-i386.patch
>kprobemulti-kprobe-posthandler-for-booster.patch
>kprobe-cleanup-for-vm_mask-judgement.patch
>
>I _think_ the first two are 2.6.17 material and the latter two are 2.6.18
>material.  Could others please comment?