* [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes @ 2005-09-29 13:01 Masami Hiramatsu 2005-09-29 14:32 ` Ananth N Mavinakayanahalli 2005-10-03 23:29 ` Keshavamurthy Anil S 0 siblings, 2 replies; 12+ messages in thread From: Masami Hiramatsu @ 2005-09-29 13:01 UTC (permalink / raw) To: systemtap; +Cc: Satoshi Oshima, Hideo Aoki, sugita Hi, This patch is an architecture common code of djprobe. -- Masami HIRAMATSU 2nd Research Dept. Hitachi, Ltd., Systems Development Laboratory E-mail: hiramatu@sdl.hitachi.co.jp include/linux/kprobes.h | 56 ++++++++++ kernel/kprobes.c | 247 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 303 insertions(+) diff -Narup linux-2.6.13-mm1.djp.1/include/linux/kprobes.h linux-2.6.13-mm1.djp.2/include/linux/kprobes.h --- linux-2.6.13-mm1.djp.1/include/linux/kprobes.h 2005-09-05 19:11:21.000000000 +0900 +++ linux-2.6.13-mm1.djp.2/include/linux/kprobes.h 2005-09-12 14:05:36.000000000 +0900 @@ -28,6 +28,8 @@ * 2005-May Hien Nguyen <hien@us.ibm.com> and Jim Keniston * <jkenisto@us.ibm.com> and Prasanna S Panchamukhi * <prasanna@in.ibm.com> added function-return probes. + * 2005-Aug Masami HIRAMATSU <hiramatu@sdl.hitachi.co.jp> added direct + * jump probe (djprobe) interface to reduce overhead. */ #include <linux/config.h> #include <linux/list.h> @@ -141,6 +143,41 @@ struct kretprobe_instance { struct task_struct *task; }; +/* djprobe's instance (internal use)*/ +struct djprobe_instance { + struct djprobe *djp; +#ifdef ARCH_SUPPORTS_DJPROBES + struct arch_djprobe_stub stub; +#endif /* ARCH_SUPPORTS_DJPROBES */ + struct kprobe kp; + struct list_head list; /* list of djprobe_instances */ + cpumask_t checked_cpus; +}; +#define DJPI_EMPTY(djpi) (djpi->djp==NULL) +#define DJPI_CHECKED(djpi) (cpus_equal(djpi->checked_cpus, cpu_online_map)) + +struct djprobe; +typedef void (*djprobe_handler_t)(struct djprobe *, struct pt_regs *); +/* + * Direct Jump probe interface structure + */ +struct djprobe { +#ifndef ARCH_SUPPORTS_DJPROBES + struct kprobe kp; +#endif /* ARCH_SUPPORTS_DJPROBES */ + /* location of the probe point */ + void * addr; + + /* sum of length of the replacing codes */ + int size; + + /* probing handler (pre-executed) */ + djprobe_handler_t handler; + + /* pointer for instance */ + struct djprobe_instance * inst; +}; + #ifdef CONFIG_KPROBES /* Locks kprobe: irq must be disabled */ void lock_kprobes(void); @@ -182,6 +219,18 @@ struct kretprobe_instance *get_free_rp_i void add_rp_inst(struct kretprobe_instance *ri); void kprobe_flush_task(struct task_struct *tk); void recycle_rp_inst(struct kretprobe_instance *ri); + +#ifdef ARCH_SUPPORTS_DJPROBES +extern int arch_prepare_djprobe_instance(struct djprobe_instance *djpi, + unsigned long size); +extern int djprobe_bypass_handler(struct kprobe * kp, struct pt_regs * regs); +extern void arch_install_djprobe_instance(struct djprobe_instance *djpi); +extern void arch_uninstall_djprobe_instance(struct djprobe_instance *djpi); +#endif /* ARCH_SUPPORTS_DJPROBES */ + +int register_djprobe(struct djprobe *p); +void unregister_djprobe(struct djprobe *p); + #else /* CONFIG_KPROBES */ static inline int kprobe_running(void) { @@ -214,5 +263,12 @@ static inline void unregister_kretprobe( static inline void kprobe_flush_task(struct task_struct *tk) { } +static inline int register_djprobe(struct djprobe *p) +{ + return -ENOSYS; +} +static inline void unregister_djprobe(struct djprobe *p) +{ +} #endif /* CONFIG_KPROBES */ #endif /* _LINUX_KPROBES_H */ diff -Narup linux-2.6.13-mm1.djp.1/kernel/kprobes.c linux-2.6.13-mm1.djp.2/kernel/kprobes.c --- linux-2.6.13-mm1.djp.1/kernel/kprobes.c 2005-09-05 19:11:21.000000000 +0900 +++ linux-2.6.13-mm1.djp.2/kernel/kprobes.c 2005-09-28 20:10:26.000000000 +0900 @@ -30,6 +30,8 @@ * 2005-May Hien Nguyen <hien@us.ibm.com>, Jim Keniston * <jkenisto@us.ibm.com> and Prasanna S Panchamukhi * <prasanna@in.ibm.com> added function-return probes. + * 2005-Aug Masami HIRAMATSU <hiramatu@sdl.hitachi.co.jp> added direct + * jump probe (djprobe) interface to reduce overhead. */ #include <linux/kprobes.h> #include <linux/spinlock.h> @@ -75,6 +77,9 @@ struct kprobe_insn_page_list { static struct kprobe_insn_page_list kprobe_insn_pages = { HLIST_HEAD_INIT, MAX_INSN_SIZE}; +static struct djprobe_instance * + __kprobes get_djprobe_instance(void *addr, int size); + /** * __get_insn_slot() - Find a slot on an executable page for an instruction. * We allocate an executable page if there's no room on existing ones. @@ -474,6 +479,11 @@ int __kprobes register_kprobe(struct kpr if ((ret = in_kprobes_functions((unsigned long) p->addr)) != 0) return ret; +#ifdef ARCH_SUPPORTS_DJPROBES + if (p->pre_handler != djprobe_bypass_handler && + get_djprobe_instance(p->addr, 1) != NULL ) + return -EEXIST; +#endif if ((ret = arch_prepare_kprobe(p)) != 0) goto rm_kprobe; @@ -597,6 +607,238 @@ void __kprobes unregister_kretprobe(stru spin_unlock_irqrestore(&kprobe_lock, flags); } +#ifdef ARCH_SUPPORTS_DJPROBES +/* + * The djprobe do not refer instances list when probe function called. + * This list is operated on registering and unregistering djprobe. + */ +static LIST_HEAD(djprobe_list); +static DEFINE_SPINLOCK(djprobe_lock); +/* Instruction pages for djprobe's stub code */ +static struct kprobe_insn_page_list djprobe_insn_pages = { + HLIST_HEAD_INIT, 0}; + +static inline void __free_djprobe_instance(struct djprobe_instance *djpi) +{ + list_del(&djpi->list); + if (djpi->kp.addr) unregister_kprobe(&(djpi->kp)); + __free_insn_slot(&djprobe_insn_pages, djpi->stub.insn); + kfree(djpi); +} + +static inline struct djprobe_instance * + __get_djprobe_instance(void *addr, int size) +{ + struct djprobe_instance *djpi; + list_for_each_entry(djpi, &djprobe_list, list) { + if (((long)addr < (long)djpi->kp.addr + DJPI_ARCH_SIZE(djpi)) + && ((long)djpi->kp.addr < (long)addr + size)) { + return djpi; + } + } + return NULL; +} + +static struct djprobe_instance * + __kprobes get_djprobe_instance(void *addr, int size) +{ + struct djprobe_instance *djpi; + spin_lock(&djprobe_lock); + djpi = __get_djprobe_instance(addr, size); + spin_unlock(&djprobe_lock); + return djpi; +} + +#ifdef CONFIG_SMP +static void __kprobes work_check_djprobe_instances(void *data); +static DEFINE_PER_CPU(struct work_struct, djprobe_check_works); + +static inline void init_djprobe(void) +{ + int cpu; + struct work_struct *wk; + djprobe_insn_pages.insn_size = ARCH_STUB_SIZE; + for_each_cpu(cpu) { /* we should initialize all percpu data */ + wk = &per_cpu(djprobe_check_works, cpu); + INIT_WORK(wk, work_check_djprobe_instances, NULL); + } +} + +static void __kprobes work_check_djprobe_instances(void *data) +{ + struct list_head *pos; + struct djprobe_instance *djpi; + + spin_lock(&djprobe_lock); + list_for_each(pos, &djprobe_list) { + djpi = container_of(pos, struct djprobe_instance, list); + if (DJPI_CHECKED(djpi)) + continue; /* already checked */ + cpu_set(smp_processor_id(), djpi->checked_cpus); + if (DJPI_CHECKED(djpi)) { + if (DJPI_EMPTY(djpi)) { + pos = pos->prev; /* pos is going to be freed */ + __free_djprobe_instance(djpi); + } else { + arch_install_djprobe_instance(djpi); + } + } + } + spin_unlock(&djprobe_lock); +} + +static inline void schedule_check_djprobe_instances(void) +{ + int cpu; + struct work_struct *wk; + cpus_clear(djpi->checked_cpus); + for_each_online_cpu(cpu) { + wk = &per_cpu(djprobe_check_works, cpu); + if (list_empty(&wk->entry)) /* not scheduled yet */ + schedule_delayed_work_on(cpu, wk, 0); + } +} + +#define __install_djprobe_instance(djpi) \ + schedule_check_djprobe_instance(djpi) + +#define __uninstall_djprobe_instance(djpi) \ + schedule_check_djprobe_instance(djpi) + +#else +#define init_djprobe() \ + djprobe_insn_pages.insn_size = ARCH_STUB_SIZE; + +#define __install_djprobe_instance(djpi) \ + arch_install_djprobe_instance(djpi) + +#define __uninstall_djprobe_instance(djpi) \ + __free_djprobe_instance(djpi) + +#endif /*CONFIG_SMP*/ + +/* Use kprobe to check safety and install */ +static int __kprobes install_djprobe_instance(struct djprobe_instance *djpi) +{ + int ret; + ret = register_kprobe(&(djpi->kp)); + if (ret == 0) { + __install_djprobe_instance(djpi); + } + return ret; +} + +/* Use kprobe to check safety and release */ +static void __kprobes uninstall_djprobe_instance(struct djprobe_instance *djpi) +{ + arch_uninstall_djprobe_instance(djpi); + __uninstall_djprobe_instance(djpi); +} + +int __kprobes register_djprobe(struct djprobe * djp) +{ + struct djprobe_instance *djpi; + struct kprobe *kp; + int ret = 0, i; + + if (djp == NULL || djp->addr == NULL || + djp->size > ARCH_STUB_INSN_MAX || + djp->size < ARCH_STUB_INSN_MIN || + djp->inst != NULL) + return -EINVAL; + + if ((ret = in_kprobes_functions((unsigned long) djp->addr)) != 0) + return ret; + + spin_lock(&djprobe_lock); + /* check confliction with other djprobes */ + djpi = __get_djprobe_instance(djp->addr, djp->size); + if (djpi) { + if (djpi->kp.addr == djp->addr && DJPI_EMPTY(djpi)) { + djp->inst = djpi; + djpi->djp = djp; /*TODO: use list*/ + goto out; + } else { + ret = -EEXIST; /* a djprobe were inserted */ + goto out; + } + } + /* check confliction with kprobes */ + for ( i=0; i < djp->size; i++) { + kp = get_kprobe((void*)((long)djp->addr+i)); + if (kp != NULL) { + ret = -EEXIST; /* a kprobes were inserted */ + goto out; + } + } + /* make a new instance */ + djpi = kmalloc(sizeof(struct djprobe_instance),GFP_KERNEL); + if (djpi == NULL) { + ret = -ENOMEM; /* memory allocation error */ + goto out; + } + memset(djpi, 0, sizeof(struct djprobe_instance)); /* for kprobe */ + /* attach */ + djp->inst = djpi; + djpi->djp = djp; /*TODO: use list*/ + djpi->kp.addr = djp->addr; + INIT_LIST_HEAD(&djpi->list); + list_add(&djpi->list, &djprobe_list); + + /* prepare stub */ + djpi->stub.insn = __get_insn_slot(&djprobe_insn_pages); + if (djpi->stub.insn == NULL) { + kfree(djpi); + ret = -ENOMEM; /* memory allocation error */ + goto out; + } + djpi->kp.pre_handler = djprobe_bypass_handler; + arch_prepare_djprobe_instance(djpi, djp->size); /*TODO : remove size*/ + + ret = install_djprobe_instance(djpi); + if (ret < 0) { /* failed to install */ + djp->inst = NULL; + djpi->kp.addr = NULL; + __free_djprobe_instance(djpi); + } +out: + spin_unlock(&djprobe_lock); + return ret; +} + +void __kprobes unregister_djprobe(struct djprobe * djp) +{ + struct djprobe_instance *djpi; + if (djp == NULL || djp->inst == NULL) + return ; + + djpi = djp->inst; + spin_lock(&djprobe_lock); + djp->inst = NULL; + djpi->djp = NULL; /*TODO: use list*/ + if (DJPI_EMPTY(djpi)) { + uninstall_djprobe_instance(djpi); + } + spin_unlock(&djprobe_lock); +} + +#else /* ARCH_SUPPORTS_DJPROBES */ +int __kprobes register_djprobe(struct djprobe *p) +{ + if (p!=NULL) { + p->kp.addr = p->addr; + p->kp.pre_handler = (kprobe_pre_handler_t)p->handler; + return register_kprobe(&p->kp); + } + return -EINVAL; +} + +void __kprobes unregister_djprobe(struct djprobe *p) +{ + unregister_kprobe(&p->kp); +} +#endif /* ARCH_SUPPORTS_DJPROBES */ + static int __init init_kprobes(void) { int i, err = 0; @@ -608,6 +850,9 @@ static int __init init_kprobes(void) INIT_HLIST_HEAD(&kretprobe_inst_table[i]); } +#if defined(ARCH_SUPPORTS_DJPROBES) + init_djprobe(); +#endif err = arch_init_kprobes(); if (!err) err = register_die_notifier(&kprobe_exceptions_nb); @@ -624,4 +869,6 @@ EXPORT_SYMBOL_GPL(unregister_jprobe); EXPORT_SYMBOL_GPL(jprobe_return); EXPORT_SYMBOL_GPL(register_kretprobe); EXPORT_SYMBOL_GPL(unregister_kretprobe); +EXPORT_SYMBOL_GPL(register_djprobe); +EXPORT_SYMBOL_GPL(unregister_djprobe); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes 2005-09-29 13:01 [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes Masami Hiramatsu @ 2005-09-29 14:32 ` Ananth N Mavinakayanahalli 2005-10-03 0:36 ` Masami Hiramatsu 2005-10-17 13:47 ` Masami Hiramatsu 2005-10-03 23:29 ` Keshavamurthy Anil S 1 sibling, 2 replies; 12+ messages in thread From: Ananth N Mavinakayanahalli @ 2005-09-29 14:32 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: systemtap, Satoshi Oshima, Hideo Aoki, sugita On Thu, Sep 29, 2005 at 09:59:31PM +0900, Masami Hiramatsu wrote: Few comments on first glance... [...] > +int __kprobes register_djprobe(struct djprobe * djp) > +{ > + struct djprobe_instance *djpi; > + struct kprobe *kp; > + int ret = 0, i; > + > + if (djp == NULL || djp->addr == NULL || > + djp->size > ARCH_STUB_INSN_MAX || > + djp->size < ARCH_STUB_INSN_MIN || > + djp->inst != NULL) > + return -EINVAL; > + > + if ((ret = in_kprobes_functions((unsigned long) djp->addr)) != 0) > + return ret; > + > + spin_lock(&djprobe_lock); Please use _irqsave/_irqrestore versions at all places. > + /* check confliction with other djprobes */ > + djpi = __get_djprobe_instance(djp->addr, djp->size); > + if (djpi) { > + if (djpi->kp.addr == djp->addr && DJPI_EMPTY(djpi)) { > + djp->inst = djpi; > + djpi->djp = djp; /*TODO: use list*/ > + goto out; > + } else { > + ret = -EEXIST; /* a djprobe were inserted */ > + goto out; > + } > + } > + /* check confliction with kprobes */ > + for ( i=0; i < djp->size; i++) { > + kp = get_kprobe((void*)((long)djp->addr+i)); > + if (kp != NULL) { > + ret = -EEXIST; /* a kprobes were inserted */ > + goto out; > + } > + } > + /* make a new instance */ > + djpi = kmalloc(sizeof(struct djprobe_instance),GFP_KERNEL); You are under a spinlock... this kmalloc may sleep. > + if (djpi == NULL) { > + ret = -ENOMEM; /* memory allocation error */ > + goto out; > + } > + memset(djpi, 0, sizeof(struct djprobe_instance)); /* for kprobe */ > + /* attach */ > + djp->inst = djpi; > + djpi->djp = djp; /*TODO: use list*/ > + djpi->kp.addr = djp->addr; > + INIT_LIST_HEAD(&djpi->list); > + list_add(&djpi->list, &djprobe_list); > + > + /* prepare stub */ > + djpi->stub.insn = __get_insn_slot(&djprobe_insn_pages); > + if (djpi->stub.insn == NULL) { > + kfree(djpi); > + ret = -ENOMEM; /* memory allocation error */ > + goto out; > + } > + djpi->kp.pre_handler = djprobe_bypass_handler; > + arch_prepare_djprobe_instance(djpi, djp->size); /*TODO : remove size*/ > + > + ret = install_djprobe_instance(djpi); > + if (ret < 0) { /* failed to install */ > + djp->inst = NULL; > + djpi->kp.addr = NULL; > + __free_djprobe_instance(djpi); > + } > +out: > + spin_unlock(&djprobe_lock); > + return ret; > +} > + > +void __kprobes unregister_djprobe(struct djprobe * djp) > +{ > + struct djprobe_instance *djpi; > + if (djp == NULL || djp->inst == NULL) > + return ; > + > + djpi = djp->inst; > + spin_lock(&djprobe_lock); > + djp->inst = NULL; > + djpi->djp = NULL; /*TODO: use list*/ > + if (DJPI_EMPTY(djpi)) { > + uninstall_djprobe_instance(djpi); > + } > + spin_unlock(&djprobe_lock); > +} > + > +#else /* ARCH_SUPPORTS_DJPROBES */ > +int __kprobes register_djprobe(struct djprobe *p) > +{ > + if (p!=NULL) { Follow CodingStyle please! There are a few other places in the existing kprobes code that also need a CodingStyle cleanup, but that is for a later patch. Ananth ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes 2005-09-29 14:32 ` Ananth N Mavinakayanahalli @ 2005-10-03 0:36 ` Masami Hiramatsu 2005-10-17 13:47 ` Masami Hiramatsu 1 sibling, 0 replies; 12+ messages in thread From: Masami Hiramatsu @ 2005-10-03 0:36 UTC (permalink / raw) To: ananth; +Cc: systemtap, Satoshi Oshima, Hideo Aoki, sugita Hi, Ananth N Mavinakayanahalli wrote: > On Thu, Sep 29, 2005 at 09:59:31PM +0900, Masami Hiramatsu wrote: > > Few comments on first glance... Thank you! >>+ spin_lock(&djprobe_lock); > > > Please use _irqsave/_irqrestore versions at all places. OK, I will use it. >>+ /* make a new instance */ >>+ djpi = kmalloc(sizeof(struct djprobe_instance),GFP_KERNEL); > > > You are under a spinlock... this kmalloc may sleep. Oh, it's a my mistake. I will change to GFP_ATOMIC. > > Follow CodingStyle please! There are a few other places in the existing > kprobes code that also need a CodingStyle cleanup, but that is for a > later patch. OK, I will check my codes by using ./scripts/Lindent, and post patches again. Thanks, Best regards, -- Masami HIRAMATSU 2nd Research Dept. Hitachi, Ltd., Systems Development Laboratory E-mail: hiramatu@sdl.hitachi.co.jp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes 2005-09-29 14:32 ` Ananth N Mavinakayanahalli 2005-10-03 0:36 ` Masami Hiramatsu @ 2005-10-17 13:47 ` Masami Hiramatsu 2005-10-17 14:28 ` Ananth N Mavinakayanahalli 1 sibling, 1 reply; 12+ messages in thread From: Masami Hiramatsu @ 2005-10-17 13:47 UTC (permalink / raw) To: ananth; +Cc: systemtap, Satoshi Oshima, Hideo Aoki, sugita Hi, Ananth Ananth N Mavinakayanahalli wrote: > On Thu, Sep 29, 2005 at 09:59:31PM +0900, Masami Hiramatsu wrote: [snip] >>+ >>+ spin_lock(&djprobe_lock); > > > Please use _irqsave/_irqrestore versions at all places. I have a question about using spin_lock_irqsave/restore. Why it should be used at all places? The djprobe_lock is not refered from probe handlers. And register/unregister_djprobe() must not be called from any interrupt handlers. So, I think it does not cause deadlock even if we use simple spin_lock/unlock(). Best regards. -- Masami HIRAMATSU 2nd Research Dept. Hitachi, Ltd., Systems Development Laboratory E-mail: hiramatu@sdl.hitachi.co.jp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes 2005-10-17 13:47 ` Masami Hiramatsu @ 2005-10-17 14:28 ` Ananth N Mavinakayanahalli 0 siblings, 0 replies; 12+ messages in thread From: Ananth N Mavinakayanahalli @ 2005-10-17 14:28 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: ananth, systemtap, Satoshi Oshima, Hideo Aoki, sugita Masami Hiramatsu wrote: > Hi, Ananth > > Ananth N Mavinakayanahalli wrote: > >>On Thu, Sep 29, 2005 at 09:59:31PM +0900, Masami Hiramatsu wrote: > > [snip] > >>>+ >>>+ spin_lock(&djprobe_lock); >> >> >>Please use _irqsave/_irqrestore versions at all places. > > > I have a question about using spin_lock_irqsave/restore. > Why it should be used at all places? > > The djprobe_lock is not refered from probe handlers. > And register/unregister_djprobe() must not be called > from any interrupt handlers. So, I think it does not > cause deadlock even if we use simple spin_lock/unlock(). If the lock isn't referred from any handlers, we should be fine. Remember though that for historical reasons, kprobe handlers run with interrupts disabled only on i386. I think this is scheduled to change. We have seen atleast one other bug due to not using the irqsave/restore versions. Ananth ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes 2005-09-29 13:01 [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes Masami Hiramatsu 2005-09-29 14:32 ` Ananth N Mavinakayanahalli @ 2005-10-03 23:29 ` Keshavamurthy Anil S 2005-10-06 2:50 ` Masami Hiramatsu 1 sibling, 1 reply; 12+ messages in thread From: Keshavamurthy Anil S @ 2005-10-03 23:29 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: systemtap, Satoshi Oshima, Hideo Aoki, sugita On Thu, Sep 29, 2005 at 05:59:31AM -0700, Masami Hiramatsu wrote: I see your patch has lots of line wrap and hard to review. With great difficulty, I have reviewed your code, please see my comments below. Main highlights: 1) You code does not work if a new CPU comes online or existing cpu goes offline 2) suggestion: Create djprobe.c and djprobe.h files instead of pushing them into existing files 3) Use good hashing algorithm to search djprobe instances. 4)Handle the case where ARCH does not support djprobes transparently. 5) optimize djprobe data struct 6) Not convinced how you are atomically updating 5 Bytes (jmp inst ) guaranteeing that none of the other processor are executing near those address. -Anil Keshavamurthy Open Source Technology Center Intel Corp. E-mail: anil.s.keshavamurthy@intel.com > > Hi, > > This patch is an architecture common code of > djprobe. > > -- > Masami HIRAMATSU > 2nd Research Dept. > Hitachi, Ltd., Systems Development Laboratory > E-mail: hiramatu@sdl.hitachi.co.jp > > include/linux/kprobes.h | 56 ++++++++++ > kernel/kprobes.c | 247 > ++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 303 insertions(+) > > diff -Narup linux-2.6.13-mm1.djp.1/include/linux/kprobes.h > linux-2.6.13-mm1.djp.2/include/linux/kprobes.h > --- linux-2.6.13-mm1.djp.1/include/linux/kprobes.h 2005-09-05 > 19:11:21.000000000 +0900 > +++ linux-2.6.13-mm1.djp.2/include/linux/kprobes.h 2005-09-12 > 14:05:36.000000000 +0900 > @@ -28,6 +28,8 @@ > * 2005-May Hien Nguyen <hien@us.ibm.com> and Jim Keniston > * <jkenisto@us.ibm.com> and Prasanna S Panchamukhi > * <prasanna@in.ibm.com> added function-return probes. > + * 2005-Aug Masami HIRAMATSU <hiramatu@sdl.hitachi.co.jp> added > direct > + * jump probe (djprobe) interface to reduce overhead. > */ I think for clarity sake, it would be better to create new files called include/linux/djpropbes.h and kernel/djprobes.c and move all the arch independent djprobe functionality into those files. > #include <linux/config.h> > #include <linux/list.h> > @@ -141,6 +143,41 @@ struct kretprobe_instance { > struct task_struct *task; > }; > > +/* djprobe's instance (internal use)*/ > +struct djprobe_instance { > + struct djprobe *djp; > +#ifdef ARCH_SUPPORTS_DJPROBES > + struct arch_djprobe_stub stub; > +#endif /* ARCH_SUPPORTS_DJPROBES */ Move arch_djprobe_stub to the end of this data structure. > + struct kprobe kp; > + struct list_head list; /* list of djprobe_instances */ > + cpumask_t checked_cpus; > +}; > +#define DJPI_EMPTY(djpi) (djpi->djp==NULL) > +#define DJPI_CHECKED(djpi) (cpus_equal(djpi->checked_cpus, > cpu_online_map)) Humm...Your code will not work for CPU hotplug case as the cpu_online_map will change based on the online cpus. Also I think, suspend/resume on i386 will depend on cpu hotplug as they will be offlining all the cpus except BSP before suspending. > + > +struct djprobe; > +typedef void (*djprobe_handler_t)(struct djprobe *, struct pt_regs > *); > +/* > + * Direct Jump probe interface structure > + */ > +struct djprobe { > +#ifndef ARCH_SUPPORTS_DJPROBES > + struct kprobe kp; > +#endif /* ARCH_SUPPORTS_DJPROBES */ Try not to have ARCH_SUPPORTS_DJPROBES in the exported data structure, as this will break the binary compatibility. Handle the case where ARCH does not support DJPROBES transparently. > + /* location of the probe point */ > + void * addr; > + > + /* sum of length of the replacing codes */ > + int size; > + > + /* probing handler (pre-executed) */ > + djprobe_handler_t handler; > + > + /* pointer for instance */ > + struct djprobe_instance * inst; > +}; > + In the above djprobe struct, I see lots of repetition of fields, please optimize djprobe data structure as some of the fields defined for this structure are already defined in kprobes struct. > #ifdef CONFIG_KPROBES > /* Locks kprobe: irq must be disabled */ > void lock_kprobes(void); > @@ -182,6 +219,18 @@ struct kretprobe_instance *get_free_rp_i > void add_rp_inst(struct kretprobe_instance *ri); > void kprobe_flush_task(struct task_struct *tk); > void recycle_rp_inst(struct kretprobe_instance *ri); > + > +#ifdef ARCH_SUPPORTS_DJPROBES > +extern int arch_prepare_djprobe_instance(struct djprobe_instance > *djpi, > + unsigned long size); > +extern int djprobe_bypass_handler(struct kprobe * kp, struct pt_regs > * regs); > +extern void arch_install_djprobe_instance(struct djprobe_instance > *djpi); > +extern void arch_uninstall_djprobe_instance(struct djprobe_instance > *djpi); > +#endif /* ARCH_SUPPORTS_DJPROBES */ > + > +int register_djprobe(struct djprobe *p); > +void unregister_djprobe(struct djprobe *p); > + > #else /* CONFIG_KPROBES */ > static inline int kprobe_running(void) > { > @@ -214,5 +263,12 @@ static inline void unregister_kretprobe( > static inline void kprobe_flush_task(struct task_struct *tk) > { > } > +static inline int register_djprobe(struct djprobe *p) > +{ > + return -ENOSYS; > +} > +static inline void unregister_djprobe(struct djprobe *p) > +{ > +} > #endif /* CONFIG_KPROBES */ > #endif /* _LINUX_KPROBES_H */ > diff -Narup linux-2.6.13-mm1.djp.1/kernel/kprobes.c > linux-2.6.13-mm1.djp.2/kernel/kprobes.c > --- linux-2.6.13-mm1.djp.1/kernel/kprobes.c 2005-09-05 > 19:11:21.000000000 +0900 > +++ linux-2.6.13-mm1.djp.2/kernel/kprobes.c 2005-09-28 > 20:10:26.000000000 +0900 > @@ -30,6 +30,8 @@ > * 2005-May Hien Nguyen <hien@us.ibm.com>, Jim Keniston > * <jkenisto@us.ibm.com> and Prasanna S Panchamukhi > * <prasanna@in.ibm.com> added function-return probes. > + * 2005-Aug Masami HIRAMATSU <hiramatu@sdl.hitachi.co.jp> added > direct > + * jump probe (djprobe) interface to reduce overhead. > */ > #include <linux/kprobes.h> > #include <linux/spinlock.h> > @@ -75,6 +77,9 @@ struct kprobe_insn_page_list { > static struct kprobe_insn_page_list kprobe_insn_pages = { > HLIST_HEAD_INIT, MAX_INSN_SIZE}; > > +static struct djprobe_instance * > + __kprobes get_djprobe_instance(void *addr, int size); > + > /** > * __get_insn_slot() - Find a slot on an executable page for an > instruction. > * We allocate an executable page if there's no room on existing > ones. > @@ -474,6 +479,11 @@ int __kprobes register_kprobe(struct kpr > > if ((ret = in_kprobes_functions((unsigned long) p->addr)) != > 0) > return ret; > +#ifdef ARCH_SUPPORTS_DJPROBES > + if (p->pre_handler != djprobe_bypass_handler && > + get_djprobe_instance(p->addr, 1) != NULL ) > + return -EEXIST; > +#endif > if ((ret = arch_prepare_kprobe(p)) != 0) > goto rm_kprobe; > > @@ -597,6 +607,238 @@ void __kprobes unregister_kretprobe(stru > spin_unlock_irqrestore(&kprobe_lock, flags); > } > > +#ifdef ARCH_SUPPORTS_DJPROBES > +/* > + * The djprobe do not refer instances list when probe function > called. > + * This list is operated on registering and unregistering djprobe. > + */ > +static LIST_HEAD(djprobe_list); > +static DEFINE_SPINLOCK(djprobe_lock); > +/* Instruction pages for djprobe's stub code */ > +static struct kprobe_insn_page_list djprobe_insn_pages = { > + HLIST_HEAD_INIT, 0}; > + > +static inline void __free_djprobe_instance(struct djprobe_instance > *djpi) > +{ > + list_del(&djpi->list); > + if (djpi->kp.addr) unregister_kprobe(&(djpi->kp)); > + __free_insn_slot(&djprobe_insn_pages, djpi->stub.insn); > + kfree(djpi); > +} > + > +static inline struct djprobe_instance * > + __get_djprobe_instance(void *addr, int size) > +{ > + struct djprobe_instance *djpi; > + list_for_each_entry(djpi, &djprobe_list, list) { Use good hashing algorithms here instead of plain linked lists. > + if (((long)addr < (long)djpi->kp.addr + > DJPI_ARCH_SIZE(djpi)) > + && ((long)djpi->kp.addr < (long)addr + size)) { > + return djpi; > + } > + } > + return NULL; > +} > + > +static struct djprobe_instance * > + __kprobes get_djprobe_instance(void *addr, int size) > +{ > + struct djprobe_instance *djpi; > + spin_lock(&djprobe_lock); > + djpi = __get_djprobe_instance(addr, size); > + spin_unlock(&djprobe_lock); > + return djpi; > +} > + > +#ifdef CONFIG_SMP > +static void __kprobes work_check_djprobe_instances(void *data); > +static DEFINE_PER_CPU(struct work_struct, djprobe_check_works); > + > +static inline void init_djprobe(void) > +{ > + int cpu; > + struct work_struct *wk; > + djprobe_insn_pages.insn_size = ARCH_STUB_SIZE; > + for_each_cpu(cpu) { /* we should initialize all percpu data */ > + wk = &per_cpu(djprobe_check_works, cpu); > + INIT_WORK(wk, work_check_djprobe_instances, NULL); > + } > +} > + > +static void __kprobes work_check_djprobe_instances(void *data) > +{ I guess this function gets called on each cpu :-) > + struct list_head *pos; > + struct djprobe_instance *djpi; > + > + spin_lock(&djprobe_lock); With the above lock held, you are serialzing this function, which indeed gets scheduled on multiple cpus. > + list_for_each(pos, &djprobe_list) { Use good hashing algorithm here. > + djpi = container_of(pos, struct djprobe_instance, > list); > + if (DJPI_CHECKED(djpi)) DJPI_CHECKED() will fail if new cpu has come online or some existing cpu has gone offline. Djprobe must work in connjuction with CONFIG_CPU_HOTPUG > + continue; /* already checked */ > + cpu_set(smp_processor_id(), djpi->checked_cpus); > + if (DJPI_CHECKED(djpi)) { > + if (DJPI_EMPTY(djpi)) { > + pos = pos->prev; /* pos is going to be > freed */ > + __free_djprobe_instance(djpi); > + } else { > + arch_install_djprobe_instance(djpi); Humm..You are simply making a DJPI_CHECKED(djpi), which checks and tells that all the cpus have seen djpi and has cpu_set(smp_processor_id(), djpi_checked_cpus). But this does not guarantee that the other cpus are not executing the instruction where you are modifying 5 bytes by calling arch_install_djprobe_instances(djpi). You are trying to insert "jmp" opcode jmp address, i,e 5 bytes in arch_install_djprobe_instance() at kp.addr with out stopping all the other cpus from executing the instructions near that address. I am not convinced, please clarify how are you making sure you are updating 5 bytes automically. > + } > + } > + } > + spin_unlock(&djprobe_lock); > +} > + > +static inline void schedule_check_djprobe_instances(void) > +{ > + int cpu; > + struct work_struct *wk; > + cpus_clear(djpi->checked_cpus); > + for_each_online_cpu(cpu) { How do you account for new online CPUs? > + wk = &per_cpu(djprobe_check_works, cpu); > + if (list_empty(&wk->entry)) /* not scheduled yet */ > + schedule_delayed_work_on(cpu, wk, 0); > + } > +} > + > +#define __install_djprobe_instance(djpi) \ > + schedule_check_djprobe_instance(djpi) > + > +#define __uninstall_djprobe_instance(djpi) \ > + schedule_check_djprobe_instance(djpi) > + > +#else > +#define init_djprobe() \ > + djprobe_insn_pages.insn_size = ARCH_STUB_SIZE; > + > +#define __install_djprobe_instance(djpi) \ > + arch_install_djprobe_instance(djpi) > + > +#define __uninstall_djprobe_instance(djpi) \ > + __free_djprobe_instance(djpi) > + > +#endif /*CONFIG_SMP*/ > + > +/* Use kprobe to check safety and install */ > +static int __kprobes install_djprobe_instance(struct djprobe_instance > *djpi) > +{ > + int ret; > + ret = register_kprobe(&(djpi->kp)); > + if (ret == 0) { > + __install_djprobe_instance(djpi); > + } > + return ret; > +} > + > +/* Use kprobe to check safety and release */ > +static void __kprobes uninstall_djprobe_instance(struct > djprobe_instance *djpi) > +{ > + arch_uninstall_djprobe_instance(djpi); > + __uninstall_djprobe_instance(djpi); > +} > + > +int __kprobes register_djprobe(struct djprobe * djp) > +{ > + struct djprobe_instance *djpi; > + struct kprobe *kp; > + int ret = 0, i; > + > + if (djp == NULL || djp->addr == NULL || > + djp->size > ARCH_STUB_INSN_MAX || > + djp->size < ARCH_STUB_INSN_MIN || > + djp->inst != NULL) > + return -EINVAL; > + > + if ((ret = in_kprobes_functions((unsigned long) djp->addr)) != > 0) > + return ret; > + > + spin_lock(&djprobe_lock); > + /* check confliction with other djprobes */ > + djpi = __get_djprobe_instance(djp->addr, djp->size); > + if (djpi) { > + if (djpi->kp.addr == djp->addr && DJPI_EMPTY(djpi)) { > + djp->inst = djpi; > + djpi->djp = djp; /*TODO: use list*/ > + goto out; > + } else { > + ret = -EEXIST; /* a djprobe were inserted */ > + goto out; > + } > + } > + /* check confliction with kprobes */ > + for ( i=0; i < djp->size; i++) { > + kp = get_kprobe((void*)((long)djp->addr+i)); > + if (kp != NULL) { > + ret = -EEXIST; /* a kprobes were inserted */ > + goto out; > + } > + } This function is way too long. ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > + /* make a new instance */ > + djpi = kmalloc(sizeof(struct djprobe_instance),GFP_KERNEL); > + if (djpi == NULL) { > + ret = -ENOMEM; /* memory allocation error */ > + goto out; > + } > + memset(djpi, 0, sizeof(struct djprobe_instance)); /* for > kprobe */ > + /* attach */ > + djp->inst = djpi; > + djpi->djp = djp; /*TODO: use list*/ > + djpi->kp.addr = djp->addr; > + INIT_LIST_HEAD(&djpi->list); > + list_add(&djpi->list, &djprobe_list); +++++++ Move the above code into djprobe_make_new_instance() function > + > + /* prepare stub */ > + djpi->stub.insn = __get_insn_slot(&djprobe_insn_pages); > + if (djpi->stub.insn == NULL) { > + kfree(djpi); You are freeing djpi without removing from the list which you have inserted above. > + ret = -ENOMEM; /* memory allocation error */ > + goto out; > + } > + djpi->kp.pre_handler = djprobe_bypass_handler; > + arch_prepare_djprobe_instance(djpi, djp->size); /*TODO : > remove size*/ > + > + ret = install_djprobe_instance(djpi); > + if (ret < 0) { /* failed to install */ > + djp->inst = NULL; > + djpi->kp.addr = NULL; > + __free_djprobe_instance(djpi); > + } > +out: > + spin_unlock(&djprobe_lock); > + return ret; > +} > + > +void __kprobes unregister_djprobe(struct djprobe * djp) > +{ > + struct djprobe_instance *djpi; > + if (djp == NULL || djp->inst == NULL) > + return ; > + > + djpi = djp->inst; > + spin_lock(&djprobe_lock); > + djp->inst = NULL; > + djpi->djp = NULL; /*TODO: use list*/ > + if (DJPI_EMPTY(djpi)) { djpi->djp is set to NULL and why are you checking DJPI_EMPTY() again? > + uninstall_djprobe_instance(djpi); > + } > + spin_unlock(&djprobe_lock); > +} > + > +#else /* ARCH_SUPPORTS_DJPROBES */ > +int __kprobes register_djprobe(struct djprobe *p) > +{ > + if (p!=NULL) { > + p->kp.addr = p->addr; > + p->kp.pre_handler = (kprobe_pre_handler_t)p->handler; > + return register_kprobe(&p->kp); > + } > + return -EINVAL; > +} > + > +void __kprobes unregister_djprobe(struct djprobe *p) > +{ > + unregister_kprobe(&p->kp); > +} > +#endif /* ARCH_SUPPORTS_DJPROBES */ > + > static int __init init_kprobes(void) > { > int i, err = 0; > @@ -608,6 +850,9 @@ static int __init init_kprobes(void) > INIT_HLIST_HEAD(&kretprobe_inst_table[i]); > } > > +#if defined(ARCH_SUPPORTS_DJPROBES) > + init_djprobe(); > +#endif > err = arch_init_kprobes(); > if (!err) > err = register_die_notifier(&kprobe_exceptions_nb); > @@ -624,4 +869,6 @@ EXPORT_SYMBOL_GPL(unregister_jprobe); > EXPORT_SYMBOL_GPL(jprobe_return); > EXPORT_SYMBOL_GPL(register_kretprobe); > EXPORT_SYMBOL_GPL(unregister_kretprobe); > +EXPORT_SYMBOL_GPL(register_djprobe); > +EXPORT_SYMBOL_GPL(unregister_djprobe); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes 2005-10-03 23:29 ` Keshavamurthy Anil S @ 2005-10-06 2:50 ` Masami Hiramatsu 2005-10-06 19:57 ` Keshavamurthy Anil S 0 siblings, 1 reply; 12+ messages in thread From: Masami Hiramatsu @ 2005-10-06 2:50 UTC (permalink / raw) To: Keshavamurthy Anil S; +Cc: systemtap, Satoshi Oshima, Hideo Aoki, sugita Hi, Anil Keshavamurthy Anil S wrote: > On Thu, Sep 29, 2005 at 05:59:31AM -0700, Masami Hiramatsu wrote: > > I see your patch has lots of line wrap and hard to review. Would you say that my patch has wrong indentation? Or would the way to separate the patch be wrong? I would like to correct wrong points. > With great difficulty, I have reviewed your code, > please see my comments below. Thanks a lot! > Main highlights: > 1) You code does not work if a new CPU comes online or existing cpu goes offline I have no machine which can plug/unplug CPUs. So I can not test that feature. But I will try to develop a patch that djprobe can work with CPU-Hotplug. Would you have that machine? If I develop a patch, would you help me to test? > 2) suggestion: Create djprobe.c and djprobe.h files instead of pushing them into existing files Djprobe uses kprobes' internal function (__get_insn_slot()) to allocate stub code buffers. And Kprobe has to check whether the code at the insertion address was already modified by djprobe. So we need to apply some patches to kprobes.c. Additionally, I think the djprobe is one of the probes, so it should be included in kprobes.c. This is easier to use for other developers. > 3) Use good hashing algorithm to search djprobe instances. Hash_list is useful if a node has only one key value. Unfortunately, a djprobe has a range of address as key value. __get_djprobe_instance() function is used for searching overlapping of address ranges. So hash_list is not useful for djprobe. And, I think the hash list is not so efficient for performance improvement for the djprobe. Because, the djprobe needs to search its instance only when it registers probes and executes deferred processing. It does NOT search when the probe was hit. > 4)Handle the case where ARCH does not support djprobes transparently. I think the transparency of djprobe means that the source code which uses the djprobe does not need any modification when ARCH does not support djprobes. From this point of view, current code works transparently. But current implementation is not so good. I will design it again. > 5) optimize djprobe data struct Exactly. I know it is not optimized enough. I think I can reduce the size of djprobe data structure by moving some members of it to arguments of register_djprobe() function. What would you think about it? > 6) Not convinced how you are atomically updating 5 Bytes (jmp inst ) guaranteeing that > none of the other processor are executing near those address. The half of the code which guarantees that none of the other processor are executing near those address is included in the other (arch-depend) patch. Djprobe makes a bypass route from copy of original codes which will be replaced by jmp code. After all processors finished executing the original codes (and it will execute the bypass route next time) djprobe inserts jmp code. You can see this detail in the answer of 8th Question in Q&A text (Answer of Q:How does the djprobe guarantee no threads and no processors are executing the modifying area?). Best regards, Thanks -- Masami HIRAMATSU 2nd Research Dept. Hitachi, Ltd., Systems Development Laboratory E-mail: hiramatu@sdl.hitachi.co.jp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes 2005-10-06 2:50 ` Masami Hiramatsu @ 2005-10-06 19:57 ` Keshavamurthy Anil S 2005-10-13 10:12 ` Masami Hiramatsu 0 siblings, 1 reply; 12+ messages in thread From: Keshavamurthy Anil S @ 2005-10-06 19:57 UTC (permalink / raw) To: Masami Hiramatsu Cc: Keshavamurthy Anil S, systemtap, Satoshi Oshima, Hideo Aoki, sugita On Thu, Oct 06, 2005 at 11:50:24AM +0900, Masami Hiramatsu wrote: > Hi, Anil > > Keshavamurthy Anil S wrote: > > On Thu, Sep 29, 2005 at 05:59:31AM -0700, Masami Hiramatsu wrote: > > > > I see your patch has lots of line wrap and hard to review. > > Would you say that my patch has wrong indentation? Or would the > way to separate the patch be wrong? > I would like to correct wrong points. I guess it is to do with your mailer, I have seen something like this when I used to send my mails from MS outlook and now I have moved to mutt and don't have any problems. > > > With great difficulty, I have reviewed your code, > > please see my comments below. > > Thanks a lot! Your are very welcome. > > > Main highlights: > > 1) You code does not work if a new CPU comes online or existing cpu goes offline > > I have no machine which can plug/unplug CPUs. So I can not test > that feature. But I will try to develop a patch that djprobe can > work with CPU-Hotplug. > Would you have that machine? If I develop a patch, would you help > me to test? To test this you don;t have to have a machine which can plug/unplug CPUs, just enable CONFIG_HOTPLUG_CPU and when this kernel boots, you see /sys/devices/system/cpu/cpuX/online file. echo'ing '0' onto this file will offline the cpu and echo'ing '1' onto this file will bring the cpu online there by affecting cpu_online_map on which you are depending on. > > > 2) suggestion: Create djprobe.c and djprobe.h files instead of pushing them into > existing files > > Djprobe uses kprobes' internal function (__get_insn_slot()) to > allocate stub code buffers. And Kprobe has to check whether the > code at the insertion address was already modified by djprobe. > So we need to apply some patches to kprobes.c. > Additionally, I think the djprobe is one of the probes, so it > should be included in kprobes.c. This is easier to use for other > developers. I understand that you need some infrastructural changes to existing Kprobes and I have no problems you modifying the existing kprobes to accommodate djprobes, what I ment was the core djprobe logic can go into separate file for better readability and for some architecture which does not yet support djprobe can exclude djprobe.c file from compiling. > > > 3) Use good hashing algorithm to search djprobe instances. > > Hash_list is useful if a node has only one key value. Unfortunately, > a djprobe has a range of address as key value. Humm..This is not a excuse for not to use hashing algorithm :-) > __get_djprobe_instance() function is used for searching overlapping > of address ranges. So hash_list is not useful for djprobe. > And, I think the hash list is not so efficient for performance > improvement for the djprobe. Because, the djprobe needs to search > its instance only when it registers probes and executes deferred > processing. It does NOT search when the probe was hit. With the Djprobe, the number of probes that can be inserted will move to the order of few thousands. And for every single register or unregister djprobes call, searching this thousands of entries is not at all an efficient way to implement something in the kernel. Also, I see the same linear search happening inside work_check_djprobe_instance(). As I understand you are scheduling this function on all the cpus and inside this function you are doing a linear search for djprobe instances that too holding a spin lock and thus making other thread executing this function on different cpus to wait untill you finish serial search on this cpu. Hence my suggestion to look into optimizing this search. > > > 4)Handle the case where ARCH does not support djprobes transparently. > > I think the transparency of djprobe means that the source code > which uses the djprobe does not need any modification when ARCH > does not support djprobes. From this point of view, current code > works transparently. > > But current implementation is not so good. I will design it again. Okay, thanks. > > > 5) optimize djprobe data struct > > Exactly. I know it is not optimized enough. I think I can reduce > the size of djprobe data structure by moving some members of it > to arguments of register_djprobe() function. > What would you think about it? Yes, that would be better and you can store those extra arguments in the djprobe instance structure for later use. > > > 6) Not convinced how you are atomically updating 5 Bytes (jmp inst ) > guaranteeing that > > none of the other processor are executing near those address. > > The half of the code which guarantees that none of the other > processor are executing near those address is included in the > other (arch-depend) patch. Djprobe makes a bypass route from > copy of original codes which will be replaced by jmp code. After > all processors finished executing the original codes (and it will > execute the bypass route next time) djprobe inserts jmp code. You > can see this detail in the answer of 8th Question in Q&A text > (Answer of Q:How does the djprobe guarantee no threads and no > processors are executing the modifying area?). I will look at this again and will get back to you later. Thanks, Anil Keshavamurthy ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes 2005-10-06 19:57 ` Keshavamurthy Anil S @ 2005-10-13 10:12 ` Masami Hiramatsu 2005-10-19 3:58 ` Keshavamurthy Anil S 0 siblings, 1 reply; 12+ messages in thread From: Masami Hiramatsu @ 2005-10-13 10:12 UTC (permalink / raw) To: Keshavamurthy Anil S; +Cc: systemtap, Satoshi Oshima, Hideo Aoki, sugita Hi, Anil Keshavamurthy Anil S wrote: >>> I see your patch has lots of line wrap and hard to review. >> >>Would you say that my patch has wrong indentation? Or would the >>way to separate the patch be wrong? >>I would like to correct wrong points. > > I guess it is to do with your mailer, I have seen something like this when > I used to send my mails from MS outlook and now I have moved to mutt and > don't have any problems. I and my co-workers saw my emails in the systemtap archive: http://sourceware.org/ml/systemtap/2005-q3/msg00629.html This message has no line-wrapping. So I think it is not caused by my mailer. Please check the mails again. >>>Main highlights: >>>1) You code does not work if a new CPU comes online or existing cpu goes offline >> >>I have no machine which can plug/unplug CPUs. So I can not test >>that feature. But I will try to develop a patch that djprobe can >>work with CPU-Hotplug. >>Would you have that machine? If I develop a patch, would you help >>me to test? > > To test this you don;t have to have a machine which can plug/unplug CPUs, > just enable CONFIG_HOTPLUG_CPU and when this kernel boots, you see > /sys/devices/system/cpu/cpuX/online file. echo'ing '0' onto this file > will offline the cpu and echo'ing '1' onto this file will bring the cpu > online there by affecting cpu_online_map on which you are depending on. Thank you for that information. I'll try it and I'd like to make the CPU- hotplug support feature as an additional patch. >>>2) suggestion: Create djprobe.c and djprobe.h files instead of pushing them into >>existing files >> >>Djprobe uses kprobes' internal function (__get_insn_slot()) to >>allocate stub code buffers. And Kprobe has to check whether the >>code at the insertion address was already modified by djprobe. >>So we need to apply some patches to kprobes.c. >>Additionally, I think the djprobe is one of the probes, so it >>should be included in kprobes.c. This is easier to use for other >>developers. > > I understand that you need some infrastructural changes to existing > Kprobes and I have no problems you modifying the existing kprobes to > accommodate djprobes, what I ment was the core djprobe logic can go > into separate file for better readability and for some architecture > which does not yet support djprobe can exclude djprobe.c file from compiling. OK. I'll consider the possibility of that. >>>3) Use good hashing algorithm to search djprobe instances. >> >>Hash_list is useful if a node has only one key value. Unfortunately, >>a djprobe has a range of address as key value. > > Humm..This is not a excuse for not to use hashing algorithm :-) Ok, I understand that the thousands of probes cause problem with linear search. I misunderstood that scale. I will try to introduce a hash table. >>__get_djprobe_instance() function is used for searching overlapping >>of address ranges. So hash_list is not useful for djprobe. >>And, I think the hash list is not so efficient for performance >>improvement for the djprobe. Because, the djprobe needs to search >>its instance only when it registers probes and executes deferred >>processing. It does NOT search when the probe was hit. > > With the Djprobe, the number of probes that can > be inserted will move to the order of few thousands. And for every single > register or unregister djprobes call, searching this thousands of entries > is not at all an efficient way to implement something in the kernel. > > Also, I see the same linear search happening inside work_check_djprobe_instance(). > As I understand you are scheduling this function on all the cpus and inside this > function you are doing a linear search for djprobe instances that too holding > a spin lock and thus making other thread executing this function on different cpus to > wait untill you finish serial search on this cpu. Hence my suggestion to look into > optimizing this search. OK, that is a problem. I have an idea of "per-probe work"s to solve it. This will allocate a lot of works and insert it into the workqueues on each cpu. Is this acceptable? >>>5) optimize djprobe data struct >> >>Exactly. I know it is not optimized enough. I think I can reduce >>the size of djprobe data structure by moving some members of it >>to arguments of register_djprobe() function. >>What would you think about it? > > Yes, that would be better and you can store those > extra arguments in the djprobe instance structure for later use. OK, I will do that. -- Masami HIRAMATSU 2nd Research Dept. Hitachi, Ltd., Systems Development Laboratory E-mail: hiramatu@sdl.hitachi.co.jp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes 2005-10-13 10:12 ` Masami Hiramatsu @ 2005-10-19 3:58 ` Keshavamurthy Anil S 2005-10-19 13:30 ` Masami Hiramatsu 0 siblings, 1 reply; 12+ messages in thread From: Keshavamurthy Anil S @ 2005-10-19 3:58 UTC (permalink / raw) To: Masami Hiramatsu Cc: Keshavamurthy Anil S, systemtap, Satoshi Oshima, Hideo Aoki, sugita On Thu, Oct 13, 2005 at 07:12:09PM +0900, Masami Hiramatsu wrote: > Hi, Anil > > > > > > > Also, I see the same linear search happening inside work_check_djprobe_instance(). > > As I understand you are scheduling this function on all the cpus and inside this > > function you are doing a linear search for djprobe instances that too holding > > a spin lock and thus making other thread executing this function on different > cpus to > > wait untill you finish serial search on this cpu. Hence my suggestion to look into > > optimizing this search. > > OK, that is a problem. I have an idea of "per-probe work"s to solve it. > This will allocate a lot of works and insert it into the workqueues on each cpu. > Is this acceptable? How about calling flush_scheduled_work(). Will this work? Cheers, Anil ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes 2005-10-19 3:58 ` Keshavamurthy Anil S @ 2005-10-19 13:30 ` Masami Hiramatsu 2005-10-19 18:26 ` Keshavamurthy Anil S 0 siblings, 1 reply; 12+ messages in thread From: Masami Hiramatsu @ 2005-10-19 13:30 UTC (permalink / raw) To: Keshavamurthy Anil S; +Cc: systemtap, Satoshi Oshima, Hideo Aoki, sugita Hi, Anil Keshavamurthy Anil S wrote: > On Thu, Oct 13, 2005 at 07:12:09PM +0900, Masami Hiramatsu wrote: >>>Also, I see the same linear search happening inside work_check_djprobe_instance(). >>>As I understand you are scheduling this function on all the cpus and inside this >>>function you are doing a linear search for djprobe instances that too holding >>>a spin lock and thus making other thread executing this function on different >>>cpus to >>>wait untill you finish serial search on this cpu. Hence my suggestion to look into >>>optimizing this search. >> >>OK, that is a problem. I have an idea of "per-probe work"s to solve it. >>This will allocate a lot of works and insert it into the workqueues on each cpu. >>Is this acceptable? > > > How about calling flush_scheduled_work(). Will this work? > I think it will work. And I wrote it as a pseudo-code here. Does this fit your idea? --- register_djprobe() { down(&djprobe_mutex); /* avoid deadlock */ instance = create_instance(); register_kprobe(&instance->kp); /* schedule check routine */ for_each_other_cpu() schedule_work(per_cpu_works); flush_scheduled_work(); /* might sleep */ arch_install_instance(instance); up(&djprobe_mutex); } unregister_djprobe() { down(&djprobe_mutex); /* avoid deadlock */ arch_uninstall_instance(instance); /* schedule check routine */ for_each_other_cpu() schedule_work(per_cpu_works); flush_scheduled_work(); /* might sleep */ unregister_kprobe(&instance->kp); release_instance(instance); up(&djprobe_mutex); } --- Of course, this code would not block other processes, and not allocate any temporary works. But, if we use flush_scheduled_work(), register/unregister_djprobe() might sleep. In other words, those functions are no longer atomic. So, it would take a long time to register/unregister some probes at a time. I'm concerned about this point. What do you think about that? Thanks, -- Masami HIRAMATSU 2nd Research Dept. Hitachi, Ltd., Systems Development Laboratory E-mail: hiramatu@sdl.hitachi.co.jp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes 2005-10-19 13:30 ` Masami Hiramatsu @ 2005-10-19 18:26 ` Keshavamurthy Anil S 0 siblings, 0 replies; 12+ messages in thread From: Keshavamurthy Anil S @ 2005-10-19 18:26 UTC (permalink / raw) To: Masami Hiramatsu Cc: Keshavamurthy Anil S, systemtap, Satoshi Oshima, Hideo Aoki, sugita On Wed, Oct 19, 2005 at 10:30:04PM +0900, Masami Hiramatsu wrote: > Hi, Anil > > > > > How about calling flush_scheduled_work(). Will this work? > > > > I think it will work. And I wrote it as a pseudo-code here. > Does this fit your idea? Perfect, and the code looks lot simpler now and cpu hotplug safe too, :-) > > --- > register_djprobe() > { > down(&djprobe_mutex); /* avoid deadlock */ > > instance = create_instance(); > register_kprobe(&instance->kp); > > /* schedule check routine */ > for_each_other_cpu() > schedule_work(per_cpu_works); > flush_scheduled_work(); /* might sleep */ > > arch_install_instance(instance); > > up(&djprobe_mutex); > } > > unregister_djprobe() > { > down(&djprobe_mutex); /* avoid deadlock */ > > arch_uninstall_instance(instance); > > /* schedule check routine */ > for_each_other_cpu() > schedule_work(per_cpu_works); > flush_scheduled_work(); /* might sleep */ > > unregister_kprobe(&instance->kp); > release_instance(instance); > > up(&djprobe_mutex); > } > --- > > Of course, this code would not block other processes, and not > allocate any temporary works. Exactly. > But, if we use flush_scheduled_work(), register/unregister_djprobe() > might sleep. In other words, those functions are no longer atomic. Even though it is not automic, the above calls are synchronous i.e when register/unregister_djprobe() returns the probes are enabled / disabled. > So, it would take a long time to register/unregister some probes > at a time. > I'm concerned about this point. What do you think about that? This issue can be addressed with providing register/unregister_bulk_djprobe(), I think. so with this api, we can actually register/unregister multiple djprobes with just one flush_scheduled_work(). cheers, Anil ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2005-10-19 18:26 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2005-09-29 13:01 [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes Masami Hiramatsu 2005-09-29 14:32 ` Ananth N Mavinakayanahalli 2005-10-03 0:36 ` Masami Hiramatsu 2005-10-17 13:47 ` Masami Hiramatsu 2005-10-17 14:28 ` Ananth N Mavinakayanahalli 2005-10-03 23:29 ` Keshavamurthy Anil S 2005-10-06 2:50 ` Masami Hiramatsu 2005-10-06 19:57 ` Keshavamurthy Anil S 2005-10-13 10:12 ` Masami Hiramatsu 2005-10-19 3:58 ` Keshavamurthy Anil S 2005-10-19 13:30 ` Masami Hiramatsu 2005-10-19 18:26 ` Keshavamurthy Anil S
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).