This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH] kprobe-booster: boosting multi-probe
- From: "bibo,mao" <bibo dot mao at intel dot com>
- To: Masami Hiramatsu <hiramatu at sdl dot hitachi dot co dot jp>
- Cc: "bibo,mao" <bibo dot mao at intel dot com>, prasanna at in dot ibm dot com, systemtap at sources dot redhat dot com, Ananth N Mavinakayanahalli <ananth at in dot ibm dot com>, "Keshavamurthy, Anil S" <anil dot s dot keshavamurthy at intel dot com>, Jim Keniston <jkenisto at us dot ibm dot com>, Satoshi Oshima <soshima at redhat dot com>, Yumiko Sugita <sugita at sdl dot hitachi dot co dot jp>, Hideo Aoki <haoki at redhat dot com>, Maneesh Soni <maneesh at in dot ibm dot com>
- Date: Tue, 14 Mar 2006 10:07:37 +0800
- Subject: Re: [PATCH] kprobe-booster: boosting multi-probe
- References: <43F5C22C.5050702@sdl.hitachi.co.jp> <44151136.7020101@intel.com> <441578BF.6090001@sdl.hitachi.co.jp>
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;