* [PATCH] kprobe-booster: boosting multi-probe @ 2006-02-17 9:06 Masami Hiramatsu 2006-02-17 9:51 ` Prasanna S Panchamukhi 0 siblings, 1 reply; 9+ messages in thread From: Masami Hiramatsu @ 2006-02-17 9:06 UTC (permalink / raw) To: systemtap, Prasanna S Panchamukhi, Ananth N Mavinakayanahalli, Keshavamurthy, Anil S, Jim Keniston Cc: Satoshi Oshima, Yumiko Sugita, Hideo Aoki, Maneesh Soni 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); } } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kprobe-booster: boosting multi-probe 2006-02-17 9:06 [PATCH] kprobe-booster: boosting multi-probe Masami Hiramatsu @ 2006-02-17 9:51 ` Prasanna S Panchamukhi 2006-02-17 12:31 ` Masami Hiramatsu 0 siblings, 1 reply; 9+ messages in thread From: Prasanna S Panchamukhi @ 2006-02-17 9:51 UTC (permalink / raw) To: Masami Hiramatsu Cc: systemtap, Ananth N Mavinakayanahalli, Keshavamurthy, Anil S, Jim Keniston, Satoshi Oshima, Yumiko Sugita, Hideo Aoki, Maneesh Soni 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kprobe-booster: boosting multi-probe 2006-02-17 9:51 ` Prasanna S Panchamukhi @ 2006-02-17 12:31 ` Masami Hiramatsu 2006-03-13 6:39 ` bibo,mao 0 siblings, 1 reply; 9+ messages in thread From: Masami Hiramatsu @ 2006-02-17 12:31 UTC (permalink / raw) To: prasanna Cc: systemtap, Ananth N Mavinakayanahalli, Keshavamurthy, Anil S, Jim Keniston, Satoshi Oshima, Yumiko Sugita, Hideo Aoki, Maneesh Soni 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 = { ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kprobe-booster: boosting multi-probe 2006-02-17 12:31 ` Masami Hiramatsu @ 2006-03-13 6:39 ` bibo,mao 2006-03-13 13:51 ` Masami Hiramatsu 0 siblings, 1 reply; 9+ messages in thread From: bibo,mao @ 2006-03-13 6:39 UTC (permalink / raw) To: Masami Hiramatsu Cc: prasanna, systemtap, Ananth N Mavinakayanahalli, Keshavamurthy, Anil S, Jim Keniston, Satoshi Oshima, Yumiko Sugita, Hideo Aoki, Maneesh Soni 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 = { > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kprobe-booster: boosting multi-probe 2006-03-13 6:39 ` bibo,mao @ 2006-03-13 13:51 ` Masami Hiramatsu 2006-03-14 2:16 ` bibo,mao 0 siblings, 1 reply; 9+ messages in thread From: Masami Hiramatsu @ 2006-03-13 13:51 UTC (permalink / raw) To: bibo,mao Cc: prasanna, systemtap, Ananth N Mavinakayanahalli, Keshavamurthy, Anil S, Jim Keniston, Satoshi Oshima, Yumiko Sugita, Hideo Aoki, Maneesh Soni 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kprobe-booster: boosting multi-probe 2006-03-13 13:51 ` Masami Hiramatsu @ 2006-03-14 2:16 ` bibo,mao 2006-03-15 13:38 ` Masami Hiramatsu 2006-04-26 7:51 ` Masami Hiramatsu 0 siblings, 2 replies; 9+ messages in thread From: bibo,mao @ 2006-03-14 2:16 UTC (permalink / raw) To: Masami Hiramatsu Cc: bibo,mao, prasanna, systemtap, Ananth N Mavinakayanahalli, Keshavamurthy, Anil S, Jim Keniston, Satoshi Oshima, Yumiko Sugita, Hideo Aoki, Maneesh Soni 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; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kprobe-booster: boosting multi-probe 2006-03-14 2:16 ` bibo,mao @ 2006-03-15 13:38 ` Masami Hiramatsu 2006-04-26 7:51 ` Masami Hiramatsu 1 sibling, 0 replies; 9+ messages in thread From: Masami Hiramatsu @ 2006-03-15 13:38 UTC (permalink / raw) To: bibo,mao Cc: prasanna, systemtap, Ananth N Mavinakayanahalli, Keshavamurthy, Anil S, Jim Keniston, Satoshi Oshima, Yumiko Sugita, Hideo Aoki, Maneesh Soni 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kprobe-booster: boosting multi-probe 2006-03-14 2:16 ` bibo,mao 2006-03-15 13:38 ` Masami Hiramatsu @ 2006-04-26 7:51 ` Masami Hiramatsu 2006-04-26 9:33 ` bibo,mao 1 sibling, 1 reply; 9+ messages in thread From: Masami Hiramatsu @ 2006-04-26 7:51 UTC (permalink / raw) To: bibo,mao Cc: prasanna, systemtap, Ananth N Mavinakayanahalli, Keshavamurthy, Anil S, Jim Keniston, Satoshi Oshima, Yumiko Sugita, Hideo Aoki, Maneesh Soni 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] kprobe-booster: boosting multi-probe 2006-04-26 7:51 ` Masami Hiramatsu @ 2006-04-26 9:33 ` bibo,mao 0 siblings, 0 replies; 9+ messages in thread From: bibo,mao @ 2006-04-26 9:33 UTC (permalink / raw) To: Andrew Morton, Ananth N Mavinakayanahalli, prasanna, Keshavamurthy, Anil S, Masami Hiramatsu, Jim Keniston Cc: systemtap, LKML [-- 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); } } ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2006-04-26 9:33 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2006-02-17 9:06 [PATCH] kprobe-booster: boosting multi-probe Masami Hiramatsu 2006-02-17 9:51 ` Prasanna S Panchamukhi 2006-02-17 12:31 ` Masami Hiramatsu 2006-03-13 6:39 ` bibo,mao 2006-03-13 13:51 ` Masami Hiramatsu 2006-03-14 2:16 ` bibo,mao 2006-03-15 13:38 ` Masami Hiramatsu 2006-04-26 7:51 ` Masami Hiramatsu 2006-04-26 9:33 ` bibo,mao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).