[Patch 2/5][Djprobe]Djprobe Coexist with Kprobes

[Patch 2/5][Djprobe]Djprobe Coexist with Kprobes

[Masami Hiramatsu]

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

Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes

[Ananth N Mavinakayanahalli]

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

Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes

[Masami Hiramatsu]

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

Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes

[Keshavamurthy Anil S]

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

Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes

[Masami Hiramatsu]

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

Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes

[Keshavamurthy Anil S]

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

Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes

[Masami Hiramatsu]

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

Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes

[Masami Hiramatsu]

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

Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes

[Ananth N Mavinakayanahalli]

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

Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes

[Keshavamurthy Anil S]

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

Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes

[Masami Hiramatsu]

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

Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes

[Keshavamurthy Anil S]

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