[RFC][Patch 1/4] kprobe fast unregistration

[RFC][Patch 1/4] kprobe fast unregistration

[Masami Hiramatsu]

Hi Ananth,

Here is a series of patches which introduce two kinds of interfaces
for speeding up unregistering process of kprobes.

Currently, the unregister_*probe() function takes a long time to
unregister specified probe because it waits the rcu synchronization
after each unregistration. So we need to introduce new method for
unregistering a lot of probes at once.

I'd like to suggest introducing following two interfaces for this issue.
The first interface is unregister_*probe_fast(). This removes
breakpoint instruction from kernel code and adds specified probe
to the unregistering list.
The second interface is commit_kprobes(). This waits the rcu
synchronization and clean up each probe on the unregistering list.
This patch also adds a list_head member to the kprobe data structure
for linking to the unregistering list.

If using these interfaces, the probe handlers of unregistering probes
might be called after unregister_*probe_fast() is called. So, you MUST
call commit_kprobes() after calling unregisnter_*probe_fast() for all
probes.

It is safe even if several threads unregister probes simultaneously,
because the unregistration process is protected by kprobe_lock mutex.
If one thread calls commit_kprobes(), it will cleanup not only its probes
but also the other thread's probes. And it is transparently done.
If there is no unregistering probes, commit_kprobes() do nothing.

Here is an example code.
--
struct kprobes *p;
for_each_probe(p) {
	unregister_kprobe_fast(p);
}
commit_kprobes();
--

Best regards,

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

---
 This patch introduces unregister_kprobe_fast() and commit_kprobes().

 arch/i386/kernel/kprobes.c    |    2
 arch/ia64/kernel/kprobes.c    |    2
 arch/powerpc/kernel/kprobes.c |    2
 arch/s390/kernel/kprobes.c    |    2
 arch/x86_64/kernel/kprobes.c  |    2
 include/linux/kprobes.h       |   11 +++
 kernel/kprobes.c              |  138 +++++++++++++++++++++++++++++++-----------
 7 files changed, 115 insertions(+), 44 deletions(-)

Index: linux-2.6.21-rc4-mm1/include/linux/kprobes.h
===================================================================
--- linux-2.6.21-rc4-mm1.orig/include/linux/kprobes.h
+++ linux-2.6.21-rc4-mm1/include/linux/kprobes.h
@@ -68,6 +68,9 @@ struct kprobe {
 	/* list of kprobes for multi-handler support */
 	struct list_head list;

+	/* list of kprobes for fast unregistering support */
+	struct list_head commit_list;
+
 	/* Indicates that the corresponding module has been ref counted */
 	unsigned int mod_refcounted;

@@ -190,6 +193,8 @@ static inline struct kprobe_ctlblk *get_

 int register_kprobe(struct kprobe *p);
 void unregister_kprobe(struct kprobe *p);
+void unregister_kprobe_fast(struct kprobe *p);
+void commit_kprobes(void);
 int setjmp_pre_handler(struct kprobe *, struct pt_regs *);
 int longjmp_break_handler(struct kprobe *, struct pt_regs *);
 int register_jprobe(struct jprobe *p);
@@ -220,6 +225,9 @@ static inline int register_kprobe(struct
 static inline void unregister_kprobe(struct kprobe *p)
 {
 }
+static inline void unregister_kprobe_fast(struct kprobe *p)
+{
+}
 static inline int register_jprobe(struct jprobe *p)
 {
 	return -ENOSYS;
@@ -240,5 +248,8 @@ static inline void unregister_kretprobe(
 static inline void kprobe_flush_task(struct task_struct *tk)
 {
 }
+static inline void commit_kprobes(void)
+{
+}
 #endif				/* CONFIG_KPROBES */
 #endif				/* _LINUX_KPROBES_H */
Index: linux-2.6.21-rc4-mm1/kernel/kprobes.c
===================================================================
--- linux-2.6.21-rc4-mm1.orig/kernel/kprobes.c
+++ linux-2.6.21-rc4-mm1/kernel/kprobes.c
@@ -62,6 +62,8 @@
 static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
 static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
 static atomic_t kprobe_count;
+static LIST_HEAD(clean_probe_list);
+static LIST_HEAD(dirty_probe_list);

 DEFINE_MUTEX(kprobe_mutex);		/* Protects kprobe_table */
 DEFINE_SPINLOCK(kretprobe_lock);	/* Protects kretprobe_inst_table */
@@ -584,6 +586,8 @@ static int __kprobes __register_kprobe(s
 	}

 	p->nmissed = 0;
+	/* Initialize multiprobe list for __unregister_kprobe_bottom() */
+	INIT_LIST_HEAD(&p->list);
 	mutex_lock(&kprobe_mutex);
 	old_p = get_kprobe(p->addr);
 	if (old_p) {
@@ -596,6 +600,7 @@ static int __kprobes __register_kprobe(s
 	if ((ret = arch_prepare_kprobe(p)) != 0)
 		goto out;

+	INIT_LIST_HEAD(&p->commit_list);
 	INIT_HLIST_NODE(&p->hlist);
 	hlist_add_head_rcu(&p->hlist,
 		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
@@ -620,80 +625,143 @@ int __kprobes register_kprobe(struct kpr
 		(unsigned long)__builtin_return_address(0));
 }

-void __kprobes unregister_kprobe(struct kprobe *p)
+/*
+ * Unregister a kprobe without a scheduler synchronization.
+ * You have to invoke commit_kprobes before releasing the kprobe.
+ */
+static int __kprobes __unregister_kprobe_top(struct kprobe *p)
 {
-	struct module *mod;
 	struct kprobe *old_p, *list_p;
-	int cleanup_p;
+	int cleanup;

-	mutex_lock(&kprobe_mutex);
 	old_p = get_kprobe(p->addr);
 	if (unlikely(!old_p)) {
-		mutex_unlock(&kprobe_mutex);
-		return;
+		return -EINVAL;
 	}
 	if (p != old_p) {
 		list_for_each_entry_rcu(list_p, &old_p->list, list)
 			if (list_p == p)
 			/* kprobe p is a valid probe */
 				goto valid_p;
-		mutex_unlock(&kprobe_mutex);
-		return;
+		return -EINVAL;
 	}
 valid_p:
 	if ((old_p == p) || ((old_p->pre_handler == aggr_pre_handler) &&
 		(p->list.next == &old_p->list) &&
 		(p->list.prev == &old_p->list))) {
-		/* Only probe on the hash list */
-		arch_disarm_kprobe(p);
-		hlist_del_rcu(&old_p->hlist);
-		cleanup_p = 1;
+		/* Only this probe on the hash list */
+  		arch_disarm_kprobe(p);
+  		hlist_del_rcu(&old_p->hlist);
+		cleanup = 1;
 	} else {
+		if (p->break_handler)
+			old_p->break_handler = NULL;
+		if (p->post_handler){
+			list_for_each_entry_rcu(list_p, &old_p->list, list){
+				if ((list_p != p) && (list_p->post_handler)){
+					goto noclean;
+				}
+			}
+			old_p->post_handler = NULL;
+		}
+noclean:
 		list_del_rcu(&p->list);
-		cleanup_p = 0;
+		cleanup = 0;
 	}

-	mutex_unlock(&kprobe_mutex);
+	return cleanup;
+}
+
+static void __kprobes __unregister_kprobe_bottom(struct kprobe *p, int cleanup)
+{
+	struct module *mod;
+	struct kprobe *old_p;

-	synchronize_sched();
 	if (p->mod_refcounted &&
 	    (mod = module_text_address((unsigned long)p->addr)))
 		module_put(mod);

-	if (cleanup_p) {
-		if (p != old_p) {
+	if (cleanup) {
+		if (!list_empty(&p->list)) {
+			/* "p" is the last child of an aggr_kprobe */
+			old_p = list_entry(p->list.next, struct kprobe, list);
 			list_del_rcu(&p->list);
 			kfree(old_p);
 		}
 		arch_remove_kprobe(p);
-	} else {
-		mutex_lock(&kprobe_mutex);
-		if (p->break_handler)
-			old_p->break_handler = NULL;
-		if (p->post_handler){
-			list_for_each_entry_rcu(list_p, &old_p->list, list){
-				if (list_p->post_handler){
-					cleanup_p = 2;
-					break;
-				}
-			}
-			if (cleanup_p == 0)
-				old_p->post_handler = NULL;
-		}
-		mutex_unlock(&kprobe_mutex);
 	}

 	/* Call unregister_page_fault_notifier()
 	 * if no probes are active
 	 */
-	mutex_lock(&kprobe_mutex);
 	if (atomic_add_return(-1, &kprobe_count) == \
 				ARCH_INACTIVE_KPROBE_COUNT)
 		unregister_page_fault_notifier(&kprobe_page_fault_nb);
-	mutex_unlock(&kprobe_mutex);
 	return;
 }

+/*
+ * Commit to optimize kprobes and remove optimized kprobes.
+ */
+void __kprobes commit_kprobes(void)
+{
+	struct kprobe *kp;
+	/* Protect from unregistering probes while waiting on synchronize_sched()*/
+	mutex_lock(&kprobe_mutex);
+
+	if (!list_empty(&clean_probe_list) ||
+	    !list_empty(&dirty_probe_list)) {
+		/* synchronize for cleaning up running probes */
+		synchronize_sched();
+
+		while (!list_empty(&clean_probe_list)) {
+			kp = list_entry(clean_probe_list.next, struct kprobe,
+					commit_list);
+			list_del_init(&kp->commit_list);
+			__unregister_kprobe_bottom(kp, 0);
+		}
+		while (!list_empty(&dirty_probe_list)) {
+			kp = list_entry(dirty_probe_list.next, struct kprobe,
+					commit_list);
+			list_del_init(&kp->commit_list);
+			__unregister_kprobe_bottom(kp, 1);
+		}
+	}
+	mutex_unlock(&kprobe_mutex);
+	return ;
+}
+
+void __kprobes unregister_kprobe_fast(struct kprobe *p)
+{
+	int cleanup;
+
+	mutex_lock(&kprobe_mutex);
+	cleanup = __unregister_kprobe_top(p);
+	if (cleanup == 0) {
+		list_add(&p->commit_list, &clean_probe_list);
+	} else if (cleanup == 1) {
+		list_add(&p->commit_list, &dirty_probe_list);
+	}
+	mutex_unlock(&kprobe_mutex);
+}
+
+void __kprobes unregister_kprobe(struct kprobe *p)
+{
+	int cleanup;
+
+	mutex_lock(&kprobe_mutex);
+	cleanup = __unregister_kprobe_top(p);
+	mutex_unlock(&kprobe_mutex);
+
+	if (cleanup < 0) return ;
+
+	synchronize_sched();
+
+	mutex_lock(&kprobe_mutex);
+	__unregister_kprobe_bottom(p, cleanup);
+	mutex_unlock(&kprobe_mutex);
+}
+
 static struct notifier_block kprobe_exceptions_nb = {
 	.notifier_call = kprobe_exceptions_notify,
 	.priority = 0x7fffffff /* we need to be notified first */
@@ -929,6 +997,8 @@ module_init(init_kprobes);

 EXPORT_SYMBOL_GPL(register_kprobe);
 EXPORT_SYMBOL_GPL(unregister_kprobe);
+EXPORT_SYMBOL_GPL(unregister_kprobe_fast);
+EXPORT_SYMBOL_GPL(commit_kprobes);
 EXPORT_SYMBOL_GPL(register_jprobe);
 EXPORT_SYMBOL_GPL(unregister_jprobe);
 EXPORT_SYMBOL_GPL(jprobe_return);
Index: linux-2.6.21-rc4-mm1/arch/i386/kernel/kprobes.c
===================================================================
--- linux-2.6.21-rc4-mm1.orig/arch/i386/kernel/kprobes.c
+++ linux-2.6.21-rc4-mm1/arch/i386/kernel/kprobes.c
@@ -183,9 +183,7 @@ void __kprobes arch_disarm_kprobe(struct

 void __kprobes arch_remove_kprobe(struct kprobe *p)
 {
-	mutex_lock(&kprobe_mutex);
 	free_insn_slot(p->ainsn.insn, (p->ainsn.boostable == 1));
-	mutex_unlock(&kprobe_mutex);
 }

 static void __kprobes save_previous_kprobe(struct kprobe_ctlblk *kcb)
Index: linux-2.6.21-rc4-mm1/arch/ia64/kernel/kprobes.c
===================================================================
--- linux-2.6.21-rc4-mm1.orig/arch/ia64/kernel/kprobes.c
+++ linux-2.6.21-rc4-mm1/arch/ia64/kernel/kprobes.c
@@ -570,9 +570,7 @@ void __kprobes arch_disarm_kprobe(struct

 void __kprobes arch_remove_kprobe(struct kprobe *p)
 {
-	mutex_lock(&kprobe_mutex);
 	free_insn_slot(p->ainsn.insn, 0);
-	mutex_unlock(&kprobe_mutex);
 }
 /*
  * We are resuming execution after a single step fault, so the pt_regs
Index: linux-2.6.21-rc4-mm1/arch/powerpc/kernel/kprobes.c
===================================================================
--- linux-2.6.21-rc4-mm1.orig/arch/powerpc/kernel/kprobes.c
+++ linux-2.6.21-rc4-mm1/arch/powerpc/kernel/kprobes.c
@@ -84,9 +84,7 @@ void __kprobes arch_disarm_kprobe(struct

 void __kprobes arch_remove_kprobe(struct kprobe *p)
 {
-	mutex_lock(&kprobe_mutex);
 	free_insn_slot(p->ainsn.insn, 0);
-	mutex_unlock(&kprobe_mutex);
 }

 static void __kprobes prepare_singlestep(struct kprobe *p, struct pt_regs *regs)
Index: linux-2.6.21-rc4-mm1/arch/s390/kernel/kprobes.c
===================================================================
--- linux-2.6.21-rc4-mm1.orig/arch/s390/kernel/kprobes.c
+++ linux-2.6.21-rc4-mm1/arch/s390/kernel/kprobes.c
@@ -218,9 +218,7 @@ void __kprobes arch_disarm_kprobe(struct

 void __kprobes arch_remove_kprobe(struct kprobe *p)
 {
-	mutex_lock(&kprobe_mutex);
 	free_insn_slot(p->ainsn.insn, 0);
-	mutex_unlock(&kprobe_mutex);
 }

 static void __kprobes prepare_singlestep(struct kprobe *p, struct pt_regs *regs)
Index: linux-2.6.21-rc4-mm1/arch/x86_64/kernel/kprobes.c
===================================================================
--- linux-2.6.21-rc4-mm1.orig/arch/x86_64/kernel/kprobes.c
+++ linux-2.6.21-rc4-mm1/arch/x86_64/kernel/kprobes.c
@@ -223,9 +223,7 @@ void __kprobes arch_disarm_kprobe(struct

 void __kprobes arch_remove_kprobe(struct kprobe *p)
 {
-	mutex_lock(&kprobe_mutex);
 	free_insn_slot(p->ainsn.insn, 0);
-	mutex_unlock(&kprobe_mutex);
 }

 static void __kprobes save_previous_kprobe(struct kprobe_ctlblk *kcb)

[RFC][Patch 2/4] kretprobe fast unregisteration

[Masami Hiramatsu]

This patch introduces unregister_kretprobe_fast() for kretprobe.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

---

 include/linux/kprobes.h |   12 +++++++++++-
 kernel/kprobes.c        |   44 ++++++++++++++++++++++++++++----------------
 2 files changed, 39 insertions(+), 17 deletions(-)

Index: linux-2.6.21-rc4-mm1/include/linux/kprobes.h
===================================================================
--- linux-2.6.21-rc4-mm1.orig/include/linux/kprobes.h
+++ linux-2.6.21-rc4-mm1/include/linux/kprobes.h
@@ -202,7 +202,14 @@ void unregister_jprobe(struct jprobe *p)
 void jprobe_return(void);

 int register_kretprobe(struct kretprobe *rp);
-void unregister_kretprobe(struct kretprobe *rp);
+static inline void unregister_kretprobe(struct kretprobe *rp)
+{
+	unregister_kprobe(&rp->kp);
+}
+static inline void unregister_kretprobe_fast(struct kretprobe *rp)
+{
+	unregister_kprobe_fast(&rp->kp);
+}

 struct kretprobe_instance *get_free_rp_inst(struct kretprobe *rp);
 void add_rp_inst(struct kretprobe_instance *ri);
@@ -245,6 +252,9 @@ static inline int register_kretprobe(str
 static inline void unregister_kretprobe(struct kretprobe *rp)
 {
 }
+static inline void unregister_kretprobe_fast(struct kretprobe *rp)
+{
+}
 static inline void kprobe_flush_task(struct task_struct *tk)
 {
 }
Index: linux-2.6.21-rc4-mm1/kernel/kprobes.c
===================================================================
--- linux-2.6.21-rc4-mm1.orig/kernel/kprobes.c
+++ linux-2.6.21-rc4-mm1/kernel/kprobes.c
@@ -74,6 +74,8 @@ static struct notifier_block kprobe_page
 	.priority = 0x7fffffff /* we need to notified first */
 };

+static int __kprobes is_kretprobe(struct kprobe *p);
+
 #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
 /*
  * kprobe->ainsn.insn points to the copy of the instruction to be
@@ -462,6 +464,20 @@ static inline void free_rp_inst(struct k
 	}
 }

+static void __kprobes cleanup_rp_inst(struct kretprobe *rp)
+{
+	unsigned long flags;
+	struct kretprobe_instance *ri;
+	/* No race here */
+	spin_lock_irqsave(&kretprobe_lock, flags);
+	while ((ri = get_used_rp_inst(rp)) != NULL) {
+		ri->rp = NULL;
+		hlist_del(&ri->uflist);
+	}
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
+	free_rp_inst(rp);
+}
+
 /*
  * Keep all fields in the kprobe consistent
  */
@@ -697,6 +713,9 @@ static void __kprobes __unregister_kprob
 	if (atomic_add_return(-1, &kprobe_count) == \
 				ARCH_INACTIVE_KPROBE_COUNT)
 		unregister_page_fault_notifier(&kprobe_page_fault_nb);
+	if (is_kretprobe(p)) {
+		cleanup_rp_inst(container_of(p, struct kretprobe, kp));
+	}
 	return;
 }

@@ -841,6 +860,11 @@ int __kprobes register_kretprobe(struct
 	return ret;
 }

+static int __kprobes is_kretprobe(struct kprobe *p)
+{
+	return p->pre_handler == pre_handler_kretprobe;
+}
+
 #else /* ARCH_SUPPORTS_KRETPROBES */

 int __kprobes register_kretprobe(struct kretprobe *rp)
@@ -854,24 +878,13 @@ static int __kprobes pre_handler_kretpro
 	return 0;
 }

-#endif /* ARCH_SUPPORTS_KRETPROBES */
-
-void __kprobes unregister_kretprobe(struct kretprobe *rp)
+static int __kprobes is_kretprobe(struct kprobe *p)
 {
-	unsigned long flags;
-	struct kretprobe_instance *ri;
-
-	unregister_kprobe(&rp->kp);
-	/* No race here */
-	spin_lock_irqsave(&kretprobe_lock, flags);
-	while ((ri = get_used_rp_inst(rp)) != NULL) {
-		ri->rp = NULL;
-		hlist_del(&ri->uflist);
-	}
-	spin_unlock_irqrestore(&kretprobe_lock, flags);
-	free_rp_inst(rp);
+	return 0;
 }

+#endif /* ARCH_SUPPORTS_KRETPROBES */
+
 static int __init init_kprobes(void)
 {
 	int i, err = 0;
@@ -1003,4 +1016,3 @@ EXPORT_SYMBOL_GPL(register_jprobe);
 EXPORT_SYMBOL_GPL(unregister_jprobe);
 EXPORT_SYMBOL_GPL(jprobe_return);
 EXPORT_SYMBOL_GPL(register_kretprobe);
-EXPORT_SYMBOL_GPL(unregister_kretprobe);

[RFC][Patch 3/4] jprobe fast unregisteration

[Masami Hiramatsu]

This patch introduces unregister_jprobe_fast() for jprobe.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

---
 This patch introduces unregister_jprobe_fast() for jprobe.

 include/linux/kprobes.h |   12 +++++++++++-
 kernel/kprobes.c        |    6 ------
 2 files changed, 11 insertions(+), 7 deletions(-)

Index: linux-2.6.21-rc4-mm1/include/linux/kprobes.h
===================================================================
--- linux-2.6.21-rc4-mm1.orig/include/linux/kprobes.h
+++ linux-2.6.21-rc4-mm1/include/linux/kprobes.h
@@ -198,7 +198,14 @@ void commit_kprobes(void);
 int setjmp_pre_handler(struct kprobe *, struct pt_regs *);
 int longjmp_break_handler(struct kprobe *, struct pt_regs *);
 int register_jprobe(struct jprobe *p);
-void unregister_jprobe(struct jprobe *p);
+static inline void unregister_jprobe(struct jprobe *jp)
+{
+	unregister_kprobe(&jp->kp);
+}
+static inline void unregister_jprobe_fast(struct jprobe *jp)
+{
+	unregister_kprobe_fast(&jp->kp);
+}
 void jprobe_return(void);

 int register_kretprobe(struct kretprobe *rp);
@@ -242,6 +249,9 @@ static inline int register_jprobe(struct
 static inline void unregister_jprobe(struct jprobe *p)
 {
 }
+static inline void unregister_jprobe_fast(struct jprobe *p)
+{
+}
 static inline void jprobe_return(void)
 {
 }
Index: linux-2.6.21-rc4-mm1/kernel/kprobes.c
===================================================================
--- linux-2.6.21-rc4-mm1.orig/kernel/kprobes.c
+++ linux-2.6.21-rc4-mm1/kernel/kprobes.c
@@ -797,11 +797,6 @@ int __kprobes register_jprobe(struct jpr
 		(unsigned long)__builtin_return_address(0));
 }

-void __kprobes unregister_jprobe(struct jprobe *jp)
-{
-	unregister_kprobe(&jp->kp);
-}
-
 #ifdef ARCH_SUPPORTS_KRETPROBES

 /*
@@ -1013,6 +1008,5 @@ EXPORT_SYMBOL_GPL(unregister_kprobe);
 EXPORT_SYMBOL_GPL(unregister_kprobe_fast);
 EXPORT_SYMBOL_GPL(commit_kprobes);
 EXPORT_SYMBOL_GPL(register_jprobe);
-EXPORT_SYMBOL_GPL(unregister_jprobe);
 EXPORT_SYMBOL_GPL(jprobe_return);
 EXPORT_SYMBOL_GPL(register_kretprobe);

[RFC][Patch 4/4] kprobes fast unregisteration documentation

[Masami Hiramatsu]

This patch adds the description of the fast unregisteration interfaces.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

---

 Documentation/kprobes.txt |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Index: linux-2.6.21-rc4-mm1/Documentation/kprobes.txt
===================================================================
--- linux-2.6.21-rc4-mm1.orig/Documentation/kprobes.txt
+++ linux-2.6.21-rc4-mm1/Documentation/kprobes.txt
@@ -36,6 +36,15 @@ registration function such as register_k
 the probe is to be inserted and what handler is to be called when
 the probe is hit.

+There are two kinds of unregistration functions. unregister_*probe()
+functions are useful to unregister a few probes. However, it will take
+several seconds to remove all probes when you unregister hundreds of
+probes. In contrast, unregister_*probe_fast() and commit_kprobes() are
+fast to remove hundreds of probes. unregister_*probe_fast() function
+removes probes, but doesn't cleanup it. So, don't forget to call
+commit_kprobes() for cleaning up when you finish to remove all probes
+by using unregister_*probe_fast().
+
 The next three subsections explain how the different types of
 probes work.  They explain certain things that you'll need to
 know in order to make the best use of Kprobes -- e.g., the
@@ -295,6 +304,27 @@ void unregister_kretprobe(struct kretpro
 Removes the specified probe.  The unregister function can be called
 at any time after the probe has been registered.

+4.5 unregister_*probe_fast
+
+#include <linux/kprobes.h>
+void unregister_kprobe_fast(struct kprobe *kp);
+void unregister_jprobe_fast(struct jprobe *jp);
+void unregister_kretprobe_fast(struct kretprobe *rp);
+
+Prepares to remove the specified probe, and add it to cleanup list.
+The unregister function can be called at any time after the probe has
+been registered.
+WARNING: you MUST call commit_kprobes() before freeing or reusing
+those probes.
+
+4.6 commit_kprobes
+
+#include <linux/kprobes.h>
+void commit_kprobes(void);
+
+Commits to removing all prepared probes. This function synchronizes
+scheduler for RCU safety, and unregisters all probes on the cleanup list.
+
 5. Kprobes Features and Limitations

 Kprobes allows multiple probes at the same address.  Currently,

Re: [RFC][Patch 1/4] kprobe fast unregistration

[Christoph Hellwig]

On Fri, Mar 23, 2007 at 11:43:48PM +0900, Masami Hiramatsu wrote:
> I'd like to suggest introducing following two interfaces for this issue.
> The first interface is unregister_*probe_fast(). This removes
> breakpoint instruction from kernel code and adds specified probe
> to the unregistering list.
> The second interface is commit_kprobes(). This waits the rcu
> synchronization and clean up each probe on the unregistering list.
> This patch also adds a list_head member to the kprobe data structure
> for linking to the unregistering list.
> 
> If using these interfaces, the probe handlers of unregistering probes
> might be called after unregister_*probe_fast() is called. So, you MUST
> call commit_kprobes() after calling unregisnter_*probe_fast() for all
> probes.

Speeding up the unregistration is a very good idea, but this interface
is rather horrible.  It's almost a receipe for users to get it wrong.

Instead just changes the register and unregister functions to take
a NULL-terminated array of probes that get registered and unregisters
at the same time (and renames them from *probe* to *probes*, please)

Re: [RFC][Patch 1/4] kprobe fast unregistration

[Keshavamurthy, Anil S]

On Fri, Mar 23, 2007 at 11:43:48PM +0900, Masami Hiramatsu wrote:
> Hi Ananth,
> 
> Here is a series of patches which introduce two kinds of interfaces
> for speeding up unregistering process of kprobes.
> 
> Currently, the unregister_*probe() function takes a long time to
> unregister specified probe because it waits the rcu synchronization
> after each unregistration. So we need to introduce new method for
> unregistering a lot of probes at once.
> 
> I'd like to suggest introducing following two interfaces for this issue.
> The first interface is unregister_*probe_fast(). This removes
> breakpoint instruction from kernel code and adds specified probe
> to the unregistering list.
> The second interface is commit_kprobes(). This waits the rcu
> synchronization and clean up each probe on the unregistering list.
> This patch also adds a list_head member to the kprobe data structure
> for linking to the unregistering list.
> 
> If using these interfaces, the probe handlers of unregistering probes
> might be called after unregister_*probe_fast() is called. So, you MUST
> call commit_kprobes() after calling unregisnter_*probe_fast() for all
> probes.
> 
> It is safe even if several threads unregister probes simultaneously,
> because the unregistration process is protected by kprobe_lock mutex.
> If one thread calls commit_kprobes(), it will cleanup not only its probes
> but also the other thread's probes. And it is transparently done.
> If there is no unregistering probes, commit_kprobes() do nothing.
> 
I agree with Christop that the interface is horrible and error prone.
However, I see the use case where people want to disable the probes quickly and
would like to reenable them again. Looking closely at your patch,
I think this can be acheived.
Here is my suggestion.

> Here is an example code.
> --
> struct kprobes *p;
> for_each_probe(p) {
> 	unregister_kprobe_fast(p);
        ^^^^^^^^^^^^^^^^^^^^^^
Change this to disable_kprobe(p), which is essentially the same as
what you have implemented. And also provide an opposite function
to reenable_kprobe(p) which enables the disabled probe again.
> }
> commit_kprobes();
  ^^^^^^^^^^^^^^
Change this to unregister_disabled_kprobes(), which essentially 
unregisters all the disabled probes.

Thanks,
Anil Keshavamurthy



> --
> 
> Best regards,
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> 
> ---
>  This patch introduces unregister_kprobe_fast() and commit_kprobes().
> 
>  arch/i386/kernel/kprobes.c    |    2
>  arch/ia64/kernel/kprobes.c    |    2
>  arch/powerpc/kernel/kprobes.c |    2
>  arch/s390/kernel/kprobes.c    |    2
>  arch/x86_64/kernel/kprobes.c  |    2
>  include/linux/kprobes.h       |   11 +++
>  kernel/kprobes.c              |  138 +++++++++++++++++++++++++++++++-----------
>  7 files changed, 115 insertions(+), 44 deletions(-)
> 
> Index: linux-2.6.21-rc4-mm1/include/linux/kprobes.h
> ===================================================================
> --- linux-2.6.21-rc4-mm1.orig/include/linux/kprobes.h
> +++ linux-2.6.21-rc4-mm1/include/linux/kprobes.h
> @@ -68,6 +68,9 @@ struct kprobe {
>  	/* list of kprobes for multi-handler support */
>  	struct list_head list;
> 
> +	/* list of kprobes for fast unregistering support */
> +	struct list_head commit_list;
> +
>  	/* Indicates that the corresponding module has been ref counted */
>  	unsigned int mod_refcounted;
> 
> @@ -190,6 +193,8 @@ static inline struct kprobe_ctlblk *get_
> 
>  int register_kprobe(struct kprobe *p);
>  void unregister_kprobe(struct kprobe *p);
> +void unregister_kprobe_fast(struct kprobe *p);
> +void commit_kprobes(void);
>  int setjmp_pre_handler(struct kprobe *, struct pt_regs *);
>  int longjmp_break_handler(struct kprobe *, struct pt_regs *);
>  int register_jprobe(struct jprobe *p);
> @@ -220,6 +225,9 @@ static inline int register_kprobe(struct
>  static inline void unregister_kprobe(struct kprobe *p)
>  {
>  }
> +static inline void unregister_kprobe_fast(struct kprobe *p)
> +{
> +}
>  static inline int register_jprobe(struct jprobe *p)
>  {
>  	return -ENOSYS;
> @@ -240,5 +248,8 @@ static inline void unregister_kretprobe(
>  static inline void kprobe_flush_task(struct task_struct *tk)
>  {
>  }
> +static inline void commit_kprobes(void)
> +{
> +}
>  #endif				/* CONFIG_KPROBES */
>  #endif				/* _LINUX_KPROBES_H */
> Index: linux-2.6.21-rc4-mm1/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.21-rc4-mm1.orig/kernel/kprobes.c
> +++ linux-2.6.21-rc4-mm1/kernel/kprobes.c
> @@ -62,6 +62,8 @@
>  static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
>  static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
>  static atomic_t kprobe_count;
> +static LIST_HEAD(clean_probe_list);
> +static LIST_HEAD(dirty_probe_list);
> 
>  DEFINE_MUTEX(kprobe_mutex);		/* Protects kprobe_table */
>  DEFINE_SPINLOCK(kretprobe_lock);	/* Protects kretprobe_inst_table */
> @@ -584,6 +586,8 @@ static int __kprobes __register_kprobe(s
>  	}
> 
>  	p->nmissed = 0;
> +	/* Initialize multiprobe list for __unregister_kprobe_bottom() */
> +	INIT_LIST_HEAD(&p->list);
>  	mutex_lock(&kprobe_mutex);
>  	old_p = get_kprobe(p->addr);
>  	if (old_p) {
> @@ -596,6 +600,7 @@ static int __kprobes __register_kprobe(s
>  	if ((ret = arch_prepare_kprobe(p)) != 0)
>  		goto out;
> 
> +	INIT_LIST_HEAD(&p->commit_list);
>  	INIT_HLIST_NODE(&p->hlist);
>  	hlist_add_head_rcu(&p->hlist,
>  		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
> @@ -620,80 +625,143 @@ int __kprobes register_kprobe(struct kpr
>  		(unsigned long)__builtin_return_address(0));
>  }
> 
> -void __kprobes unregister_kprobe(struct kprobe *p)
> +/*
> + * Unregister a kprobe without a scheduler synchronization.
> + * You have to invoke commit_kprobes before releasing the kprobe.
> + */
> +static int __kprobes __unregister_kprobe_top(struct kprobe *p)
>  {
> -	struct module *mod;
>  	struct kprobe *old_p, *list_p;
> -	int cleanup_p;
> +	int cleanup;
> 
> -	mutex_lock(&kprobe_mutex);
>  	old_p = get_kprobe(p->addr);
>  	if (unlikely(!old_p)) {
> -		mutex_unlock(&kprobe_mutex);
> -		return;
> +		return -EINVAL;
>  	}
>  	if (p != old_p) {
>  		list_for_each_entry_rcu(list_p, &old_p->list, list)
>  			if (list_p == p)
>  			/* kprobe p is a valid probe */
>  				goto valid_p;
> -		mutex_unlock(&kprobe_mutex);
> -		return;
> +		return -EINVAL;
>  	}
>  valid_p:
>  	if ((old_p == p) || ((old_p->pre_handler == aggr_pre_handler) &&
>  		(p->list.next == &old_p->list) &&
>  		(p->list.prev == &old_p->list))) {
> -		/* Only probe on the hash list */
> -		arch_disarm_kprobe(p);
> -		hlist_del_rcu(&old_p->hlist);
> -		cleanup_p = 1;
> +		/* Only this probe on the hash list */
> +  		arch_disarm_kprobe(p);
> +  		hlist_del_rcu(&old_p->hlist);
> +		cleanup = 1;
>  	} else {
> +		if (p->break_handler)
> +			old_p->break_handler = NULL;
> +		if (p->post_handler){
> +			list_for_each_entry_rcu(list_p, &old_p->list, list){
> +				if ((list_p != p) && (list_p->post_handler)){
> +					goto noclean;
> +				}
> +			}
> +			old_p->post_handler = NULL;
> +		}
> +noclean:
>  		list_del_rcu(&p->list);
> -		cleanup_p = 0;
> +		cleanup = 0;
>  	}
> 
> -	mutex_unlock(&kprobe_mutex);
> +	return cleanup;
> +}
> +
> +static void __kprobes __unregister_kprobe_bottom(struct kprobe *p, int cleanup)
> +{
> +	struct module *mod;
> +	struct kprobe *old_p;
> 
> -	synchronize_sched();
>  	if (p->mod_refcounted &&
>  	    (mod = module_text_address((unsigned long)p->addr)))
>  		module_put(mod);
> 
> -	if (cleanup_p) {
> -		if (p != old_p) {
> +	if (cleanup) {
> +		if (!list_empty(&p->list)) {
> +			/* "p" is the last child of an aggr_kprobe */
> +			old_p = list_entry(p->list.next, struct kprobe, list);
>  			list_del_rcu(&p->list);
>  			kfree(old_p);
>  		}
>  		arch_remove_kprobe(p);
> -	} else {
> -		mutex_lock(&kprobe_mutex);
> -		if (p->break_handler)
> -			old_p->break_handler = NULL;
> -		if (p->post_handler){
> -			list_for_each_entry_rcu(list_p, &old_p->list, list){
> -				if (list_p->post_handler){
> -					cleanup_p = 2;
> -					break;
> -				}
> -			}
> -			if (cleanup_p == 0)
> -				old_p->post_handler = NULL;
> -		}
> -		mutex_unlock(&kprobe_mutex);
>  	}
> 
>  	/* Call unregister_page_fault_notifier()
>  	 * if no probes are active
>  	 */
> -	mutex_lock(&kprobe_mutex);
>  	if (atomic_add_return(-1, &kprobe_count) == \
>  				ARCH_INACTIVE_KPROBE_COUNT)
>  		unregister_page_fault_notifier(&kprobe_page_fault_nb);
> -	mutex_unlock(&kprobe_mutex);
>  	return;
>  }
> 
> +/*
> + * Commit to optimize kprobes and remove optimized kprobes.
> + */
> +void __kprobes commit_kprobes(void)
> +{
> +	struct kprobe *kp;
> +	/* Protect from unregistering probes while waiting on synchronize_sched()*/
> +	mutex_lock(&kprobe_mutex);
> +
> +	if (!list_empty(&clean_probe_list) ||
> +	    !list_empty(&dirty_probe_list)) {
> +		/* synchronize for cleaning up running probes */
> +		synchronize_sched();
> +
> +		while (!list_empty(&clean_probe_list)) {
> +			kp = list_entry(clean_probe_list.next, struct kprobe,
> +					commit_list);
> +			list_del_init(&kp->commit_list);
> +			__unregister_kprobe_bottom(kp, 0);
> +		}
> +		while (!list_empty(&dirty_probe_list)) {
> +			kp = list_entry(dirty_probe_list.next, struct kprobe,
> +					commit_list);
> +			list_del_init(&kp->commit_list);
> +			__unregister_kprobe_bottom(kp, 1);
> +		}
> +	}
> +	mutex_unlock(&kprobe_mutex);
> +	return ;
> +}
> +
> +void __kprobes unregister_kprobe_fast(struct kprobe *p)
> +{
> +	int cleanup;
> +
> +	mutex_lock(&kprobe_mutex);
> +	cleanup = __unregister_kprobe_top(p);
> +	if (cleanup == 0) {
> +		list_add(&p->commit_list, &clean_probe_list);
> +	} else if (cleanup == 1) {
> +		list_add(&p->commit_list, &dirty_probe_list);
> +	}
> +	mutex_unlock(&kprobe_mutex);
> +}
> +
> +void __kprobes unregister_kprobe(struct kprobe *p)
> +{
> +	int cleanup;
> +
> +	mutex_lock(&kprobe_mutex);
> +	cleanup = __unregister_kprobe_top(p);
> +	mutex_unlock(&kprobe_mutex);
> +
> +	if (cleanup < 0) return ;
> +
> +	synchronize_sched();
> +
> +	mutex_lock(&kprobe_mutex);
> +	__unregister_kprobe_bottom(p, cleanup);
> +	mutex_unlock(&kprobe_mutex);
> +}
> +
>  static struct notifier_block kprobe_exceptions_nb = {
>  	.notifier_call = kprobe_exceptions_notify,
>  	.priority = 0x7fffffff /* we need to be notified first */
> @@ -929,6 +997,8 @@ module_init(init_kprobes);
> 
>  EXPORT_SYMBOL_GPL(register_kprobe);
>  EXPORT_SYMBOL_GPL(unregister_kprobe);
> +EXPORT_SYMBOL_GPL(unregister_kprobe_fast);
> +EXPORT_SYMBOL_GPL(commit_kprobes);
>  EXPORT_SYMBOL_GPL(register_jprobe);
>  EXPORT_SYMBOL_GPL(unregister_jprobe);
>  EXPORT_SYMBOL_GPL(jprobe_return);
> Index: linux-2.6.21-rc4-mm1/arch/i386/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.21-rc4-mm1.orig/arch/i386/kernel/kprobes.c
> +++ linux-2.6.21-rc4-mm1/arch/i386/kernel/kprobes.c
> @@ -183,9 +183,7 @@ void __kprobes arch_disarm_kprobe(struct
> 
>  void __kprobes arch_remove_kprobe(struct kprobe *p)
>  {
> -	mutex_lock(&kprobe_mutex);
>  	free_insn_slot(p->ainsn.insn, (p->ainsn.boostable == 1));
> -	mutex_unlock(&kprobe_mutex);
>  }
> 
>  static void __kprobes save_previous_kprobe(struct kprobe_ctlblk *kcb)
> Index: linux-2.6.21-rc4-mm1/arch/ia64/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.21-rc4-mm1.orig/arch/ia64/kernel/kprobes.c
> +++ linux-2.6.21-rc4-mm1/arch/ia64/kernel/kprobes.c
> @@ -570,9 +570,7 @@ void __kprobes arch_disarm_kprobe(struct
> 
>  void __kprobes arch_remove_kprobe(struct kprobe *p)
>  {
> -	mutex_lock(&kprobe_mutex);
>  	free_insn_slot(p->ainsn.insn, 0);
> -	mutex_unlock(&kprobe_mutex);
>  }
>  /*
>   * We are resuming execution after a single step fault, so the pt_regs
> Index: linux-2.6.21-rc4-mm1/arch/powerpc/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.21-rc4-mm1.orig/arch/powerpc/kernel/kprobes.c
> +++ linux-2.6.21-rc4-mm1/arch/powerpc/kernel/kprobes.c
> @@ -84,9 +84,7 @@ void __kprobes arch_disarm_kprobe(struct
> 
>  void __kprobes arch_remove_kprobe(struct kprobe *p)
>  {
> -	mutex_lock(&kprobe_mutex);
>  	free_insn_slot(p->ainsn.insn, 0);
> -	mutex_unlock(&kprobe_mutex);
>  }
> 
>  static void __kprobes prepare_singlestep(struct kprobe *p, struct pt_regs *regs)
> Index: linux-2.6.21-rc4-mm1/arch/s390/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.21-rc4-mm1.orig/arch/s390/kernel/kprobes.c
> +++ linux-2.6.21-rc4-mm1/arch/s390/kernel/kprobes.c
> @@ -218,9 +218,7 @@ void __kprobes arch_disarm_kprobe(struct
> 
>  void __kprobes arch_remove_kprobe(struct kprobe *p)
>  {
> -	mutex_lock(&kprobe_mutex);
>  	free_insn_slot(p->ainsn.insn, 0);
> -	mutex_unlock(&kprobe_mutex);
>  }
> 
>  static void __kprobes prepare_singlestep(struct kprobe *p, struct pt_regs *regs)
> Index: linux-2.6.21-rc4-mm1/arch/x86_64/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.21-rc4-mm1.orig/arch/x86_64/kernel/kprobes.c
> +++ linux-2.6.21-rc4-mm1/arch/x86_64/kernel/kprobes.c
> @@ -223,9 +223,7 @@ void __kprobes arch_disarm_kprobe(struct
> 
>  void __kprobes arch_remove_kprobe(struct kprobe *p)
>  {
> -	mutex_lock(&kprobe_mutex);
>  	free_insn_slot(p->ainsn.insn, 0);
> -	mutex_unlock(&kprobe_mutex);
>  }
> 
>  static void __kprobes save_previous_kprobe(struct kprobe_ctlblk *kcb)

RE: [RFC][Patch 1/4] kprobe fast unregistration

[Stone, Joshua I]

Keshavamurthy, Anil S wrote:
> I agree with Christop that the interface is horrible and error prone.
> However, I see the use case where people want to disable the probes
> quickly and would like to reenable them again. Looking closely at
> your patch, 
> I think this can be acheived.
> Here is my suggestion.
> 
>> Here is an example code.
>> --
>> struct kprobes *p;
>> for_each_probe(p) {
>> 	unregister_kprobe_fast(p);
>         ^^^^^^^^^^^^^^^^^^^^^^
> Change this to disable_kprobe(p), which is essentially the same as
> what you have implemented. And also provide an opposite function
> to reenable_kprobe(p) which enables the disabled probe again.
>> }
>> commit_kprobes();
>   ^^^^^^^^^^^^^^
> Change this to unregister_disabled_kprobes(), which essentially
> unregisters all the disabled probes.

The ability to disable/reenable kprobes would be an interesting
enhancement.  However, unregister_disabled_kprobes() shouldn't have a
global effect, because there might be a concurrent kprobes user that
disabled a probe with the intention of reenabling it later.


Josh Stone

Re: [RFC][Patch 1/4] kprobe fast unregistration

[Frank Ch. Eigler]

Hi -

Keshavamurthy, Anil S wrote:
> I agree with Christoph that the interface is horrible and error prone.

Really?  What possible problems can occur?  The worst that occurs to
me is that if someone forgets to call the commit function, the kprobes
will still be disabled, but memory won't be recycled for a while.  Is
this really so bad, considering that a kprobe_unregister is to imply a
commit?  Maybe if kprobe_register can also implied a commit, we can
bound the conceivable memory leak.

Would it be possible to allay even that concern with an automated
deferred/periodic commit?

- FChE

Re: [RFC][Patch 1/4] kprobe fast unregistration

[Keshavamurthy, Anil S]

On Fri, Mar 23, 2007 at 02:22:48PM -0400, Frank Ch. Eigler wrote:
> Hi -
> 
> Keshavamurthy, Anil S wrote:
> > I agree with Christoph that the interface is horrible and error prone.
> 
> Really?  What possible problems can occur?  The worst that occurs to
> me is that if someone forgets to call the commit function, the kprobes
> will still be disabled, but memory won't be recycled for a while.  Is
> this really so bad, considering that a kprobe_unregister is to imply a
> commit?  Maybe if kprobe_register can also implied a commit, we can
> bound the conceivable memory leak.
Yes, Have you looked at the code? If someone forgets to call the
commit function, the kprobe will be disabled and yes the memory won't
be recycled but the worst problem is that if the probe is on a module
function then that module can't be unloaded at all as the
register_kprobe() would have taken a reference on that module.

Hence, my suggestion would be to call them as disable_kprobe()
(instead of unregister_kprobes_fast() which is confusing and error prone)
and also to provide an opposite function to reenable_kprobe()
and finally provide unregister_disabled_kprobes() which is
essentially the same as commit_kprobes(). With this 
name changes the intentions of the new exported functions
will be clear for users and we don;t have to discard the hard work
that has gone into this patch.


> 
> Would it be possible to allay even that concern with an automated
> deferred/periodic commit?
> 
I would recomand that users call unregister_disabled_kprobes() explictly.

-Anil Keshavamurthy

Re: [RFC][Patch 1/4] kprobe fast unregistration

[Frank Ch. Eigler]

Hi -

"Keshavamurthy, Anil S" <anil.s.keshavamurthy@intel.com> writes:

> [...]
> > Really?  What possible problems can occur?  The worst that occurs
> > to me is that if someone forgets to call the commit function, the
> > kprobes will still be disabled, but memory won't be recycled for a
> > while.  [...]
>
> Yes, Have you looked at the code? 

A little, but we were talking more about the interface than the
implementation.

> If someone forgets to call the commit function, the kprobe will be
> disabled and yes the memory won't be recycled but the worst problem
> is that if the probe is on a module function then that module can't
> be unloaded at all [...]

I believe there is already a kprobes patch in the queue for
enumerating active probes in some /proc file.  Should a module be
locked into memory for such a reason, finding the culprit should not
be difficult.

> Hence, my suggestion would be to call them as disable_kprobe()
> (instead of unregister_kprobes_fast() which is confusing and error
> prone) and also to provide an opposite function to reenable_kprobe()
> and finally provide unregister_disabled_kprobes() which is
> essentially the same as commit_kprobes().

One problem with this idea is that if the unregister_fast()=disable()
is to become reversible, then the renamed commit_kprobes() will no
longer be indempotent.  There can no longer be a single system-wide
deferred-kprobe-cleanup list, since individual kprobes clients might
want to reinstate their probes in the future.

> > Would it be possible to allay even that concern with an automated
> > deferred/periodic commit?
> > 
> I would recomand that users call unregister_disabled_kprobes() explictly.

But this would solve both problems (memory leaks and outstanding
reference counts on modules).  In this variant,
unregister_kprobes_fast could replace unregister_kprobes outright, and
the (builtin deferred) commit function would need not be exported.

- FChE

Re: [RFC][Patch 1/4] kprobe fast unregistration

[Masami Hiramatsu]

Hi Christoph and Anil,

Thank you for your comments.

Christoph Hellwig wrote:
> Speeding up the unregistration is a very good idea, but this interface
> is rather horrible.  It's almost a receipe for users to get it wrong.
Keshavamurthy, Anil S wrote:
> I agree with Christop that the interface is horrible and error prone.

OK, I agree. I had chosen a confusable name.

> However, I see the use case where people want to disable the probes quickly and
> would like to reenable them again. Looking closely at your patch,
> I think this can be acheived.

Thank you.

> Here is my suggestion.
> 
>> Here is an example code.
>> --
>> struct kprobes *p;
>> for_each_probe(p) {
>> 	unregister_kprobe_fast(p);
>         ^^^^^^^^^^^^^^^^^^^^^^
> Change this to disable_kprobe(p), which is essentially the same as
> what you have implemented. And also provide an opposite function
> to reenable_kprobe(p) which enables the disabled probe again.

I'd like to change that to prepare_to_unregister_kprobe(p) instead of
disable_kprobe(p).

I think Josh and other people want interfaces to disable/reenable all
probes at once when the sysrq is pressed.
So, IMHO, these interfaces should use a global (and per-cpu?) flag which
controls whether kprobes calls user-defined handler or not, instead of
self-modifying.
For example,

if (p && p->pre_handler && kprobe_enable) {
                           ^^^^^^^^^^^^^
   ...
}

But, I think this would be another story.
I'd like to discuss this topic in other mails.

>> }
>> commit_kprobes();
>   ^^^^^^^^^^^^^^
> Change this to unregister_disabled_kprobes(), which essentially 
> unregisters all the disabled probes.

And also, I'd like to change it to unregister_prepared_kprobes().
What would you think about this idea?

Thanks,


> 
> Thanks,
> Anil Keshavamurthy
> 
-- 
Masami HIRAMATSU
Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [RFC][Patch 1/4] kprobe fast unregistration

[Keshavamurthy, Anil S]

On Mon, Mar 26, 2007 at 12:17:49PM +0900, Masami Hiramatsu wrote:
> Hi Christoph and Anil,
> 
> Thank you for your comments.
> 
> Christoph Hellwig wrote:
> > Speeding up the unregistration is a very good idea, but this interface
> > is rather horrible.  It's almost a receipe for users to get it wrong.
> Keshavamurthy, Anil S wrote:
> > I agree with Christop that the interface is horrible and error prone.
> 
> OK, I agree. I had chosen a confusable name.
Keep in mind that the the sequence of unregistering a 
probe should be same irrespecitve of whether user wants 
to unregister a single probe or user want to 
unregister more that one probe, i.e. you can not say 
use this call (unregister_kprobe() ) for unregistering 
a probe which is b.t.w slow and use this set of calls if you 
have more than one kprobes to unregister for faster unregistration.

thanks,
Anil Keshavamurthy

Re: [RFC][Patch 1/4] kprobe fast unregistration

[Masami Hiramatsu]

Hi Anil,

Keshavamurthy, Anil S wrote:
> On Mon, Mar 26, 2007 at 12:17:49PM +0900, Masami Hiramatsu wrote:
>> Hi Christoph and Anil,
>>
>> Thank you for your comments.
>>
>> Christoph Hellwig wrote:
>>> Speeding up the unregistration is a very good idea, but this interface
>>> is rather horrible.  It's almost a receipe for users to get it wrong.
>> Keshavamurthy, Anil S wrote:
>>> I agree with Christop that the interface is horrible and error prone.
>> OK, I agree. I had chosen a confusable name.
> Keep in mind that the the sequence of unregistering a 
> probe should be same irrespecitve of whether user wants 
> to unregister a single probe or user want to 
> unregister more that one probe, i.e. you can not say 
> use this call (unregister_kprobe() ) for unregistering 
> a probe which is b.t.w slow and use this set of calls if you 
> have more than one kprobes to unregister for faster unregistration.

Would you mean that we should integrate unregistration interface?
Or, would you mean that you prefer "disable_kprobe()" since that
name provides another meaning?

Thanks,

-- 
Masami HIRAMATSU
Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [RFC][Patch 1/4] kprobe fast unregistration

[Keshavamurthy, Anil S]

On Tue, Mar 27, 2007 at 12:24:09PM +0900, Masami Hiramatsu wrote:
> Would you mean that we should integrate unregistration interface?
Yes, that is correct. Ask youself how do I detect and warn 
users (who are doing the below code in their module) that 
there is a faster method available (don't say they have keep 
checking the Kprobe Documentation file)?

struct kprobe *p;
for_each_probe(p) {
	unregister_kprobe(p);
}

Hence you should either depricate the old interface ( so users are warned
and are forced to look at the new interface) or better would be to 
integrate your changes into existing unregister interface( even
if it means changing the current interface).
In the end you should aim at having just one fast way of unregistering 
the probes.

You can follow what Chirstoph suggested i.e modify 
the existing interface to take a NULL terminated array of probes, i.e
int __kprobes unregister_kprobes(struct kprobe **pp)

Or getrid of the current unregister_kprobe() and make it a two stage process.

struct kprobe *p;
for_each_probe(p) {
	prepare_unregister_kprobe(p);
}
claim_kprobes_struct(); //Same as your commit_kprobes(); 


I was also interested to see if we can also support enable/disable of probes and
was interested to see if your two staged approch can be used to enable this feature.
As you have stated we can discuss this in a separate email.

-thanks,
Anil

Re: [RFC][Patch 1/4] kprobe fast unregistration

[Keshavamurthy, Anil S]

On Sun, Mar 25, 2007 at 10:29:51PM -0400, Frank Ch. Eigler wrote:
> > Hence, my suggestion would be to call them as disable_kprobe()
> > (instead of unregister_kprobes_fast() which is confusing and error
> > prone) and also to provide an opposite function to reenable_kprobe()
> > and finally provide unregister_disabled_kprobes() which is
> > essentially the same as commit_kprobes().
> 
> One problem with this idea is that if the unregister_fast()=disable()
> is to become reversible, then the renamed commit_kprobes() will no
> longer be indempotent.  There can no longer be a single system-wide
> deferred-kprobe-cleanup list, since individual kprobes clients might
> want to reinstate their probes in the future.
True, worth investigating..

> 
> > > Would it be possible to allay even that concern with an automated
> > > deferred/periodic commit?
> > > 
> > I would recomand that users call unregister_disabled_kprobes() explictly.
> 
> But this would solve both problems (memory leaks and outstanding
> reference counts on modules).  In this variant,
> unregister_kprobes_fast could replace unregister_kprobes outright, and
> the (builtin deferred) commit function would need not be exported.
The only problem with deferred builtin commit funcion would be that
the module will have no way of knowing when is the right time to reuse
the kprobe data structure after calling unregister_kprobe(), hence exlict
call is always better.

-Anil

Re: [RFC][Patch 1/4] kprobe fast unregistration

[Christoph Hellwig]

On Fri, Mar 23, 2007 at 02:22:48PM -0400, Frank Ch. Eigler wrote:
> Really?  What possible problems can occur?  The worst that occurs to
> me is that if someone forgets to call the commit function, the kprobes
> will still be disabled, but memory won't be recycled for a while.

Exactly.  It's a very unintuitive interface, exposing implementation
details to the user.  The array variant otoh is obvious to the user:
gice me an array of probes to register/unregister and do it.

> Would it be possible to allay even that concern with an automated
> deferred/periodic commit?

That's even worse from the user perspective interface. 

Re: [RFC][Patch 1/4] kprobe fast unregistration

[Christoph Hellwig]

On Fri, Mar 23, 2007 at 11:12:22AM -0700, Stone, Joshua I wrote:
> The ability to disable/reenable kprobes would be an interesting
> enhancement.  However, unregister_disabled_kprobes() shouldn't have a
> global effect, because there might be a concurrent kprobes user that
> disabled a probe with the intention of reenabling it later.

enabling/disabling probes sounds like a wonderful additional feature.
I don't think we should expose it as a kernel interface, but rather
through debugfs so I can temporarily disable and then reenable probes
from the shell easily.