[(resend)patch 0/7] Notify page fault call chain

[(resend)patch 0/7] Notify page fault call chain

[Anil S Keshavamurthy]

Hi Andrew,

Currently in the do_page_fault() code path, we call
notify_die(DIE_PAGE_FAULT, ...) to notify the page fault. 
The only interested components for this page fault 
notifications  are  Kprobes  and/or  kdb. Since 
notify_die() is highly overloaded, this  page  fault  
notification  is  currently  being  sent  to  other
components registered with register_die_notifier()  
which  uses  the  same die_chain to loop for all 
the registered components.

In order to optimize the do_page_fault() code path, 
this critical page fault notification is now moved 
to different call chain and the test results conducted
by Robin Holt showed great improvements.

Patches for i386, x86_64, ia64, powerpc and sparc64 follows this mail.

Please apply.

Thanks,
Anil Keshavamurthy

--

[(resend)patch 1/7] Notify page fault call chain for i386

[Anil S Keshavamurthy]

[-- Attachment #1: notify_page_fault_i386.patch --]
[-- Type: text/plain, Size: 3603 bytes --]

Overloading of page fault notification with the
notify_die() has performance issues(since the
only interested components for page fault is
kprobes and/or kdb) and hence this patch introduces 
the new notifier call chain exclusively for page 
fault notifications their by avoiding notifying
unnecessary components in the do_page_fault() code
path.

Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>

---
 arch/i386/kernel/traps.c  |   12 ++++++++++++
 arch/i386/mm/fault.c      |    4 ++--
 include/asm-i386/kdebug.h |   15 +++++++++++++++
 3 files changed, 29 insertions(+), 2 deletions(-)

Index: linux-2.6.17-rc1-mm3/arch/i386/kernel/traps.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/arch/i386/kernel/traps.c
+++ linux-2.6.17-rc1-mm3/arch/i386/kernel/traps.c
@@ -93,6 +93,7 @@ asmlinkage void machine_check(void);
 
 static int kstack_depth_to_print = 24;
 ATOMIC_NOTIFIER_HEAD(i386die_chain);
+ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);
 
 int register_die_notifier(struct notifier_block *nb)
 {
@@ -107,6 +108,17 @@ int unregister_die_notifier(struct notif
 }
 EXPORT_SYMBOL(unregister_die_notifier);
 
+int register_page_fault_notifier(struct notifier_block *nb)
+{
+	vmalloc_sync_all();
+	return atomic_notifier_chain_register(&notify_page_fault_chain, nb);
+}
+
+int unregister_page_fault_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_unregister(&notify_page_fault_chain, nb);
+}
+
 static inline int valid_stack_ptr(struct thread_info *tinfo, void *p)
 {
 	return	p > (void *)tinfo &&
Index: linux-2.6.17-rc1-mm3/arch/i386/mm/fault.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/arch/i386/mm/fault.c
+++ linux-2.6.17-rc1-mm3/arch/i386/mm/fault.c
@@ -321,7 +321,7 @@ fastcall void __kprobes do_page_fault(st
 	if (unlikely(address >= TASK_SIZE)) {
 		if (!(error_code & 0x0000000d) && vmalloc_fault(address) >= 0)
 			return;
-		if (notify_die(DIE_PAGE_FAULT, "page fault", regs, error_code, 14,
+		if (notify_page_fault(DIE_PAGE_FAULT, "page fault", regs, error_code, 14,
 						SIGSEGV) == NOTIFY_STOP)
 			return;
 		/*
@@ -331,7 +331,7 @@ fastcall void __kprobes do_page_fault(st
 		goto bad_area_nosemaphore;
 	}
 
-	if (notify_die(DIE_PAGE_FAULT, "page fault", regs, error_code, 14,
+	if (notify_page_fault(DIE_PAGE_FAULT, "page fault", regs, error_code, 14,
 					SIGSEGV) == NOTIFY_STOP)
 		return;
 
Index: linux-2.6.17-rc1-mm3/include/asm-i386/kdebug.h
===================================================================
--- linux-2.6.17-rc1-mm3.orig/include/asm-i386/kdebug.h
+++ linux-2.6.17-rc1-mm3/include/asm-i386/kdebug.h
@@ -19,7 +19,10 @@ struct die_args {
 
 extern int register_die_notifier(struct notifier_block *);
 extern int unregister_die_notifier(struct notifier_block *);
+extern int register_page_fault_notifier(struct notifier_block *);
+extern int unregister_page_fault_notifier(struct notifier_block *);
 extern struct atomic_notifier_head i386die_chain;
+extern struct atomic_notifier_head notify_page_fault_chain;
 
 
 /* Grossly misnamed. */
@@ -53,4 +56,16 @@ static inline int notify_die(enum die_va
 	return atomic_notifier_call_chain(&i386die_chain, val, &args);
 }
 
+static inline int notify_page_fault(enum die_val val, const char *str,
+			struct pt_regs *regs, long err, int trap, int sig)
+{
+	struct die_args args = {
+		.regs = regs,
+		.str = str,
+		.err = err,
+		.trapnr = trap,
+		.signr = sig
+	};
+	return atomic_notifier_call_chain(&notify_page_fault_chain, val, &args);
+}
 #endif

--

[(resend)patch 2/7] Notify page fault call chain for x86_64

[Anil S Keshavamurthy]

[-- Attachment #1: notify_page_fault_x86_64.patch --]
[-- Type: text/plain, Size: 3721 bytes --]

Overloading of page fault notification with the
notify_die() has performance issues(since the
only interested components for page fault is
kprobes and/or kdb) and hence this patch introduces 
the new notifier call chain exclusively for page 
fault notifications their by avoiding notifying
unnecessary components in the do_page_fault() code
path.

Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>

---
 arch/x86_64/kernel/traps.c  |   12 ++++++++++++
 arch/x86_64/mm/fault.c      |    4 ++--
 include/asm-x86_64/kdebug.h |   16 ++++++++++++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

Index: linux-2.6.17-rc1-mm3/arch/x86_64/kernel/traps.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/arch/x86_64/kernel/traps.c
+++ linux-2.6.17-rc1-mm3/arch/x86_64/kernel/traps.c
@@ -71,6 +71,7 @@ asmlinkage void machine_check(void);
 asmlinkage void spurious_interrupt_bug(void);
 
 ATOMIC_NOTIFIER_HEAD(die_chain);
+ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);
 
 int register_die_notifier(struct notifier_block *nb)
 {
@@ -85,6 +86,17 @@ int unregister_die_notifier(struct notif
 }
 EXPORT_SYMBOL(unregister_die_notifier);
 
+int register_page_fault_notifier(struct notifier_block *nb)
+{
+	vmalloc_sync_all();
+	return atomic_notifier_chain_register(&notify_page_fault_chain, nb);
+}
+
+int unregister_page_fault_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_unregister(&notify_page_fault_chain, nb);
+}
+
 static inline void conditional_sti(struct pt_regs *regs)
 {
 	if (regs->eflags & X86_EFLAGS_IF)
Index: linux-2.6.17-rc1-mm3/arch/x86_64/mm/fault.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/arch/x86_64/mm/fault.c
+++ linux-2.6.17-rc1-mm3/arch/x86_64/mm/fault.c
@@ -348,7 +348,7 @@ asmlinkage void __kprobes do_page_fault(
 			if (vmalloc_fault(address) >= 0)
 				return;
 		}
-		if (notify_die(DIE_PAGE_FAULT, "page fault", regs, error_code, 14,
+		if (notify_page_fault(DIE_PAGE_FAULT, "page fault", regs, error_code, 14,
 						SIGSEGV) == NOTIFY_STOP)
 			return;
 		/*
@@ -358,7 +358,7 @@ asmlinkage void __kprobes do_page_fault(
 		goto bad_area_nosemaphore;
 	}
 
-	if (notify_die(DIE_PAGE_FAULT, "page fault", regs, error_code, 14,
+	if (notify_page_fault(DIE_PAGE_FAULT, "page fault", regs, error_code, 14,
 					SIGSEGV) == NOTIFY_STOP)
 		return;
 
Index: linux-2.6.17-rc1-mm3/include/asm-x86_64/kdebug.h
===================================================================
--- linux-2.6.17-rc1-mm3.orig/include/asm-x86_64/kdebug.h
+++ linux-2.6.17-rc1-mm3/include/asm-x86_64/kdebug.h
@@ -15,7 +15,10 @@ struct die_args {
 
 extern int register_die_notifier(struct notifier_block *);
 extern int unregister_die_notifier(struct notifier_block *);
+extern int register_page_fault_notifier(struct notifier_block *);
+extern int unregister_page_fault_notifier(struct notifier_block *);
 extern struct atomic_notifier_head die_chain;
+extern struct atomic_notifier_head notify_page_fault_chain;
 
 /* Grossly misnamed. */
 enum die_val {
@@ -47,6 +50,19 @@ static inline int notify_die(enum die_va
 	return atomic_notifier_call_chain(&die_chain, val, &args);
 } 
 
+static inline int notify_page_fault(enum die_val val, const char *str,
+			struct pt_regs *regs, long err, int trap, int sig)
+{
+	struct die_args args = {
+		.regs = regs,
+		.str = str,
+		.err = err,
+		.trapnr = trap,
+		.signr = sig
+	};
+	return atomic_notifier_call_chain(&notify_page_fault_chain, val, &args);
+}
+
 extern int printk_address(unsigned long address);
 extern void die(const char *,struct pt_regs *,long);
 extern void __die(const char *,struct pt_regs *,long);

--

[(resend)patch 3/7] Notify page fault call chain for ia64

[Anil S Keshavamurthy]

[-- Attachment #1: notify_page_fault_ia64.patch --]
[-- Type: text/plain, Size: 3186 bytes --]

Overloading of page fault notification with the
notify_die() has performance issues(since the
only interested components for page fault is
kprobes and/or kdb) and hence this patch introduces 
the new notifier call chain exclusively for page 
fault notifications their by avoiding notifying
unnecessary components in the do_page_fault() code
path.

Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>

---
 arch/ia64/kernel/traps.c  |   11 +++++++++++
 arch/ia64/mm/fault.c      |    2 +-
 include/asm-ia64/kdebug.h |   16 ++++++++++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

Index: linux-2.6.17-rc1-mm3/arch/ia64/kernel/traps.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/arch/ia64/kernel/traps.c
+++ linux-2.6.17-rc1-mm3/arch/ia64/kernel/traps.c
@@ -31,6 +31,7 @@ fpswa_interface_t *fpswa_interface;
 EXPORT_SYMBOL(fpswa_interface);
 
 ATOMIC_NOTIFIER_HEAD(ia64die_chain);
+ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);
 
 int
 register_die_notifier(struct notifier_block *nb)
@@ -46,6 +47,16 @@ unregister_die_notifier(struct notifier_
 }
 EXPORT_SYMBOL_GPL(unregister_die_notifier);
 
+int register_page_fault_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_register(&notify_page_fault_chain, nb);
+}
+
+int unregister_page_fault_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_unregister(&notify_page_fault_chain, nb);
+}
+
 void __init
 trap_init (void)
 {
Index: linux-2.6.17-rc1-mm3/arch/ia64/mm/fault.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/arch/ia64/mm/fault.c
+++ linux-2.6.17-rc1-mm3/arch/ia64/mm/fault.c
@@ -84,7 +84,7 @@ ia64_do_page_fault (unsigned long addres
 	/*
 	 * This is to handle the kprobes on user space access instructions
 	 */
-	if (notify_die(DIE_PAGE_FAULT, "page fault", regs, code, TRAP_BRKPT,
+	if (notify_page_fault(DIE_PAGE_FAULT, "page fault", regs, code, TRAP_BRKPT,
 					SIGSEGV) == NOTIFY_STOP)
 		return;
 
Index: linux-2.6.17-rc1-mm3/include/asm-ia64/kdebug.h
===================================================================
--- linux-2.6.17-rc1-mm3.orig/include/asm-ia64/kdebug.h
+++ linux-2.6.17-rc1-mm3/include/asm-ia64/kdebug.h
@@ -40,7 +40,10 @@ struct die_args {
 
 extern int register_die_notifier(struct notifier_block *);
 extern int unregister_die_notifier(struct notifier_block *);
+extern int register_page_fault_notifier(struct notifier_block *);
+extern int unregister_page_fault_notifier(struct notifier_block *);
 extern struct atomic_notifier_head ia64die_chain;
+extern struct atomic_notifier_head notify_page_fault_chain;
 
 enum die_val {
 	DIE_BREAK = 1,
@@ -86,4 +89,17 @@ static inline int notify_die(enum die_va
 	return atomic_notifier_call_chain(&ia64die_chain, val, &args);
 }
 
+static inline int notify_page_fault(enum die_val val, char *str, struct pt_regs *regs,
+			     long err, int trap, int sig)
+{
+	struct die_args args = {
+		.regs   = regs,
+		.str    = str,
+		.err    = err,
+		.trapnr = trap,
+		.signr  = sig
+	};
+
+	return atomic_notifier_call_chain(&notify_page_fault_chain, val, &args);
+}
 #endif

--

[(resend)patch 4/7] Notify page fault call chain for powerpc

[Anil S Keshavamurthy]

[-- Attachment #1: notify_page_fault_powerpc.patch --]
[-- Type: text/plain, Size: 3213 bytes --]

Overloading of page fault notification with the
notify_die() has performance issues(since the
only interested components for page fault is
kprobes and/or kdb) and hence this patch introduces 
the new notifier call chain exclusively for page 
fault notifications their by avoiding notifying
unnecessary components in the do_page_fault() code
path.

Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>

---
 arch/powerpc/kernel/traps.c  |   11 +++++++++++
 arch/powerpc/mm/fault.c      |    2 +-
 include/asm-powerpc/kdebug.h |    9 +++++++++
 3 files changed, 21 insertions(+), 1 deletion(-)

Index: linux-2.6.17-rc1-mm3/arch/powerpc/kernel/traps.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/arch/powerpc/kernel/traps.c
+++ linux-2.6.17-rc1-mm3/arch/powerpc/kernel/traps.c
@@ -75,6 +75,7 @@ EXPORT_SYMBOL(__debugger_fault_handler);
 #endif
 
 ATOMIC_NOTIFIER_HEAD(powerpc_die_chain);
+ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);
 
 int register_die_notifier(struct notifier_block *nb)
 {
@@ -88,6 +89,16 @@ int unregister_die_notifier(struct notif
 }
 EXPORT_SYMBOL(unregister_die_notifier);
 
+int register_page_fault_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_register(&notify_page_fault_chain, nb);
+}
+
+int unregister_page_fault_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_unregister(&notify_page_fault_chain, nb);
+}
+
 /*
  * Trap & Exception support
  */
Index: linux-2.6.17-rc1-mm3/arch/powerpc/mm/fault.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/arch/powerpc/mm/fault.c
+++ linux-2.6.17-rc1-mm3/arch/powerpc/mm/fault.c
@@ -142,7 +142,7 @@ int __kprobes do_page_fault(struct pt_re
 	is_write = error_code & ESR_DST;
 #endif /* CONFIG_4xx || CONFIG_BOOKE */
 
-	if (notify_die(DIE_PAGE_FAULT, "page_fault", regs, error_code,
+	if (notify_page_fault(DIE_PAGE_FAULT, "page_fault", regs, error_code,
 				11, SIGSEGV) == NOTIFY_STOP)
 		return 0;
 
Index: linux-2.6.17-rc1-mm3/include/asm-powerpc/kdebug.h
===================================================================
--- linux-2.6.17-rc1-mm3.orig/include/asm-powerpc/kdebug.h
+++ linux-2.6.17-rc1-mm3/include/asm-powerpc/kdebug.h
@@ -18,7 +18,10 @@ struct die_args {
 
 extern int register_die_notifier(struct notifier_block *);
 extern int unregister_die_notifier(struct notifier_block *);
+extern int register_page_fault_notifier(struct notifier_block *);
+extern int unregister_page_fault_notifier(struct notifier_block *);
 extern struct atomic_notifier_head powerpc_die_chain;
+extern struct atomic_notifier_head notify_page_fault_chain;
 
 /* Grossly misnamed. */
 enum die_val {
@@ -36,5 +39,11 @@ static inline int notify_die(enum die_va
 	return atomic_notifier_call_chain(&powerpc_die_chain, val, &args);
 }
 
+static inline int notify_page_fault(enum die_val val,char *str,struct pt_regs *regs,long err,int trap, int sig)
+{
+	struct die_args args = { .regs=regs, .str=str, .err=err, .trapnr=trap,.signr=sig };
+	return atomic_notifier_call_chain(&notify_page_fault_chain, val, &args);
+}
+
 #endif /* __KERNEL__ */
 #endif /* _ASM_POWERPC_KDEBUG_H */

--

[(resend)patch 5/7] Notify page fault call chain for sparc64

[Anil S Keshavamurthy]

[-- Attachment #1: notify_page_fault_sparc64.patch --]
[-- Type: text/plain, Size: 3303 bytes --]

Overloading of page fault notification with the
notify_die() has performance issues(since the
only interested components for page fault is
kprobes and/or kdb) and hence this patch introduces 
the new notifier call chain exclusively for page 
fault notifications their by avoiding notifying
unnecessary components in the do_page_fault() code
path.

Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
---
 arch/sparc64/kernel/traps.c  |   11 +++++++++++
 arch/sparc64/mm/fault.c      |    2 +-
 include/asm-sparc64/kdebug.h |   16 ++++++++++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

Index: linux-2.6.17-rc1-mm3/arch/sparc64/kernel/traps.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/arch/sparc64/kernel/traps.c
+++ linux-2.6.17-rc1-mm3/arch/sparc64/kernel/traps.c
@@ -44,6 +44,7 @@
 #endif
 
 ATOMIC_NOTIFIER_HEAD(sparc64die_chain);
+ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);
 
 int register_die_notifier(struct notifier_block *nb)
 {
@@ -57,6 +58,16 @@ int unregister_die_notifier(struct notif
 }
 EXPORT_SYMBOL(unregister_die_notifier);
 
+int register_page_fault_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_register(&notify_page_fault_chain, nb);
+}
+
+int unregister_page_fault_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_unregister(&notify_page_fault_chain, nb);
+}
+
 /* When an irrecoverable trap occurs at tl > 0, the trap entry
  * code logs the trap state registers at every level in the trap
  * stack.  It is found at (pt_regs + sizeof(pt_regs)) and the layout
Index: linux-2.6.17-rc1-mm3/arch/sparc64/mm/fault.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/arch/sparc64/mm/fault.c
+++ linux-2.6.17-rc1-mm3/arch/sparc64/mm/fault.c
@@ -263,7 +263,7 @@ asmlinkage void __kprobes do_sparc64_fau
 
 	fault_code = get_thread_fault_code();
 
-	if (notify_die(DIE_PAGE_FAULT, "page_fault", regs,
+	if (notify_page_fault(DIE_PAGE_FAULT, "page_fault", regs,
 		       fault_code, 0, SIGSEGV) == NOTIFY_STOP)
 		return;
 
Index: linux-2.6.17-rc1-mm3/include/asm-sparc64/kdebug.h
===================================================================
--- linux-2.6.17-rc1-mm3.orig/include/asm-sparc64/kdebug.h
+++ linux-2.6.17-rc1-mm3/include/asm-sparc64/kdebug.h
@@ -17,7 +17,10 @@ struct die_args {
 
 extern int register_die_notifier(struct notifier_block *);
 extern int unregister_die_notifier(struct notifier_block *);
+extern int register_page_fault_notifier(struct notifier_block *);
+extern int unregister_page_fault_notifier(struct notifier_block *);
 extern struct atomic_notifier_head sparc64die_chain;
+extern struct atomic_notifier_head notify_page_fault_chain;
 
 extern void bad_trap(struct pt_regs *, long);
 
@@ -46,4 +49,17 @@ static inline int notify_die(enum die_va
 	return atomic_notifier_call_chain(&sparc64die_chain, val, &args);
 }
 
+static inline int notify_page_fault(enum die_val val,char *str, struct pt_regs *regs,
+			     long err, int trap, int sig)
+{
+	struct die_args args = { .regs		= regs,
+				 .str		= str,
+				 .err		= err,
+				 .trapnr	= trap,
+				 .signr		= sig };
+
+	return atomic_notifier_call_chain(&notify_page_fault_chain, val, &args);
+}
+
+
 #endif

--

[(resend)patch 6/7] Kprobes registers for notify page fault

[Anil S Keshavamurthy]

[-- Attachment #1: notify_page_fault_kprobes.patch --]
[-- Type: text/plain, Size: 1025 bytes --]

Kprobes now registers for the page fault notifications.

Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>

---
 kernel/kprobes.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: linux-2.6.17-rc1-mm3/kernel/kprobes.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/kernel/kprobes.c
+++ linux-2.6.17-rc1-mm3/kernel/kprobes.c
@@ -544,6 +544,11 @@ static struct notifier_block kprobe_exce
 	.priority = 0x7fffffff /* we need to notified first */
 };
 
+static struct notifier_block kprobe_page_fault_nb = {
+	.notifier_call = kprobe_exceptions_notify,
+	.priority = 0x7fffffff /* we need to notified first */
+};
+
 int __kprobes register_jprobe(struct jprobe *jp)
 {
 	/* Todo: Verify probepoint is a function entry point */
@@ -654,6 +659,9 @@ static int __init init_kprobes(void)
 	if (!err)
 		err = register_die_notifier(&kprobe_exceptions_nb);
 
+	if (!err)
+		err = register_page_fault_notifier(&kprobe_page_fault_nb);
+
 	return err;
 }
 

--

[(resend)patch 7/7] Kprobes - Register for page fault notify on active probes

[Anil S Keshavamurthy]

[-- Attachment #1: kprobes_noitfy_register.patch --]
[-- Type: text/plain, Size: 2939 bytes --]

With this patch Kprobes now registers for page fault notifications only
when their is an active probe registered. Once all the active probes are
unregistered their is no need to be notified of page faults and kprobes
unregisters itself from the page fault notifications. Hence we
will have ZERO side effects when no probes are active.

Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
---
 kernel/kprobes.c |   25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

Index: linux-2.6.17-rc1-mm3/kernel/kprobes.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/kernel/kprobes.c
+++ linux-2.6.17-rc1-mm3/kernel/kprobes.c
@@ -47,11 +47,17 @@
 
 static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
 static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
+static atomic_t kprobe_count;
 
 DEFINE_MUTEX(kprobe_mutex);		/* Protects kprobe_table */
 DEFINE_SPINLOCK(kretprobe_lock);	/* Protects kretprobe_inst_table */
 static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
 
+static struct notifier_block kprobe_page_fault_nb = {
+	.notifier_call = kprobe_exceptions_notify,
+	.priority = 0x7fffffff /* we need to notified first */
+};
+
 #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
 /*
  * kprobe->ainsn.insn points to the copy of the instruction to be
@@ -464,6 +470,8 @@ static int __kprobes __register_kprobe(s
 	old_p = get_kprobe(p->addr);
 	if (old_p) {
 		ret = register_aggr_kprobe(old_p, p);
+		if (!ret)
+			atomic_inc(&kprobe_count);
 		goto out;
 	}
 
@@ -474,6 +482,9 @@ static int __kprobes __register_kprobe(s
 	hlist_add_head_rcu(&p->hlist,
 		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
 
+	if(atomic_add_return(1, &kprobe_count) == 2)
+		register_page_fault_notifier(&kprobe_page_fault_nb);
+
   	arch_arm_kprobe(p);
 
 out:
@@ -523,9 +534,13 @@ valid_p:
 		cleanup_p = 0;
 	}
 
+	if(atomic_add_return(-1, &kprobe_count) == 1)
+		unregister_page_fault_notifier(&kprobe_page_fault_nb);
+	else
+		synchronize_rcu();
+
 	mutex_unlock(&kprobe_mutex);
 
-	synchronize_sched();
 	if (p->mod_refcounted &&
 	    (mod = module_text_address((unsigned long)p->addr)))
 		module_put(mod);
@@ -544,10 +559,6 @@ static struct notifier_block kprobe_exce
 	.priority = 0x7fffffff /* we need to notified first */
 };
 
-static struct notifier_block kprobe_page_fault_nb = {
-	.notifier_call = kprobe_exceptions_notify,
-	.priority = 0x7fffffff /* we need to notified first */
-};
 
 int __kprobes register_jprobe(struct jprobe *jp)
 {
@@ -654,14 +665,12 @@ static int __init init_kprobes(void)
 		INIT_HLIST_HEAD(&kprobe_table[i]);
 		INIT_HLIST_HEAD(&kretprobe_inst_table[i]);
 	}
+	atomic_set(&kprobe_count, 0);
 
 	err = arch_init_kprobes();
 	if (!err)
 		err = register_die_notifier(&kprobe_exceptions_nb);
 
-	if (!err)
-		err = register_page_fault_notifier(&kprobe_page_fault_nb);
-
 	return err;
 }
 

--

Re: [(resend)patch 0/7] Notify page fault call chain

[Keith Owens]

Anil S Keshavamurthy (on Wed, 19 Apr 2006 15:14:19 -0700) wrote:
>Hi Andrew,
>
>Currently in the do_page_fault() code path, we call
>notify_die(DIE_PAGE_FAULT, ...) to notify the page fault. 
>The only interested components for this page fault 
>notifications  are  Kprobes  and/or  kdb.

Only kprobes.  kdb does not care about page faults.

Re: [(resend)patch 3/7] Notify page fault call chain for ia64

[Keith Owens]

Anil S Keshavamurthy (on Wed, 19 Apr 2006 15:14:22 -0700) wrote:
>Overloading of page fault notification with the
>notify_die() has performance issues(since the
>only interested components for page fault is
>kprobes and/or kdb) and hence this patch introduces 
>the new notifier call chain exclusively for page 
>fault notifications their by avoiding notifying
>unnecessary components in the do_page_fault() code
>path.
>
>Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
>
>---
> arch/ia64/kernel/traps.c  |   11 +++++++++++
>===================================================================
>--- linux-2.6.17-rc1-mm3.orig/arch/ia64/kernel/traps.c
>+++ linux-2.6.17-rc1-mm3/arch/ia64/kernel/traps.c
>@@ -31,6 +31,7 @@ fpswa_interface_t *fpswa_interface;
> EXPORT_SYMBOL(fpswa_interface);
> 
> ATOMIC_NOTIFIER_HEAD(ia64die_chain);
>+ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);
> 
> int
> register_die_notifier(struct notifier_block *nb)
>@@ -46,6 +47,16 @@ unregister_die_notifier(struct notifier_
> }
> EXPORT_SYMBOL_GPL(unregister_die_notifier);
> 
>+int register_page_fault_notifier(struct notifier_block *nb)
>+{
>+	return atomic_notifier_chain_register(&notify_page_fault_chain, nb);
>+}
>+
>+int unregister_page_fault_notifier(struct notifier_block *nb)
>+{
>+	return atomic_notifier_chain_unregister(&notify_page_fault_chain, nb);
>+}
>+

Why is register_page_fault_notifier defined in traps.c?  Surely it
should be in mm/fault.c, which is the only place that uses the chain.

> trap_init (void)
> {
>Index: linux-2.6.17-rc1-mm3/arch/ia64/mm/fault.c
>===================================================================
>--- linux-2.6.17-rc1-mm3.orig/arch/ia64/mm/fault.c
>+++ linux-2.6.17-rc1-mm3/arch/ia64/mm/fault.c
>@@ -84,7 +84,7 @@ ia64_do_page_fault (unsigned long addres
> 	/*
> 	 * This is to handle the kprobes on user space access instructions
> 	 */
>-	if (notify_die(DIE_PAGE_FAULT, "page fault", regs, code, TRAP_BRKPT,
>+	if (notify_page_fault(DIE_PAGE_FAULT, "page fault", regs, code, TRAP_BRKPT,
> 					SIGSEGV) == NOTIFY_STOP)
> 		return;

Since this is a critical path, please remove all references to
notify_page_fault() and its register functions when CONFIG_KPROBES=n.

Re: [(resend)patch 7/7] Kprobes - Register for page fault notify on active probes

[Ananth N Mavinakayanahalli]

On Wed, Apr 19, 2006 at 03:14:26PM -0700, Anil S Keshavamurthy wrote:
> With this patch Kprobes now registers for page fault notifications only
> when their is an active probe registered. Once all the active probes are
> unregistered their is no need to be notified of page faults and kprobes
> unregisters itself from the page fault notifications. Hence we
> will have ZERO side effects when no probes are active.

This patch isn't complete yet... comments below.
 
> Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> ---
>  kernel/kprobes.c |   25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> Index: linux-2.6.17-rc1-mm3/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.17-rc1-mm3.orig/kernel/kprobes.c
> +++ linux-2.6.17-rc1-mm3/kernel/kprobes.c
> @@ -47,11 +47,17 @@
>  
>  static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
>  static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
> +static atomic_t kprobe_count;
>  
>  DEFINE_MUTEX(kprobe_mutex);		/* Protects kprobe_table */
>  DEFINE_SPINLOCK(kretprobe_lock);	/* Protects kretprobe_inst_table */
>  static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
>  
> +static struct notifier_block kprobe_page_fault_nb = {
> +	.notifier_call = kprobe_exceptions_notify,
> +	.priority = 0x7fffffff /* we need to notified first */
> +};
> +
>  #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
>  /*
>   * kprobe->ainsn.insn points to the copy of the instruction to be
> @@ -464,6 +470,8 @@ static int __kprobes __register_kprobe(s
>  	old_p = get_kprobe(p->addr);
>  	if (old_p) {
>  		ret = register_aggr_kprobe(old_p, p);
> +		if (!ret)
> +			atomic_inc(&kprobe_count);
>  		goto out;
>  	}
>  
> @@ -474,6 +482,9 @@ static int __kprobes __register_kprobe(s
>  	hlist_add_head_rcu(&p->hlist,
>  		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
>  
> +	if(atomic_add_return(1, &kprobe_count) == 2)
	^^^
	if (..) please, here and elsewhere

This will not work as desired for i386 (i386 no longer has a kprobe on the
trampoline) and sparc64 (no retprobe support).

> +		register_page_fault_notifier(&kprobe_page_fault_nb);
> +
>    	arch_arm_kprobe(p);
>  
>  out:
> @@ -523,9 +534,13 @@ valid_p:
>  		cleanup_p = 0;
>  	}
>  
> +	if(atomic_add_return(-1, &kprobe_count) == 1)
> +		unregister_page_fault_notifier(&kprobe_page_fault_nb);

Same here - i386 and sparc64 need different checks.

> +	else
> +		synchronize_rcu();

Why has this changed from synchronize_sched()? This *must* be
synchronize_sched() since, by definition it'll ensure that all
non-preemptive sections are completed. In contrast, synchronize_rcu()
will just enure rcu_read_lock() sections are complete.

As of now, synchronize_sched() is aliased to synchronize_rcu() but I am
told its scheduled to change in the future.

Please revert this back to synchronize_sched().

Thanks,
Ananth

> +
>  	mutex_unlock(&kprobe_mutex);
>  
> -	synchronize_sched();
>  	if (p->mod_refcounted &&
>  	    (mod = module_text_address((unsigned long)p->addr)))
>  		module_put(mod);
> @@ -544,10 +559,6 @@ static struct notifier_block kprobe_exce
>  	.priority = 0x7fffffff /* we need to notified first */
>  };
>  
> -static struct notifier_block kprobe_page_fault_nb = {
> -	.notifier_call = kprobe_exceptions_notify,
> -	.priority = 0x7fffffff /* we need to notified first */
> -};
>  
>  int __kprobes register_jprobe(struct jprobe *jp)
>  {
> @@ -654,14 +665,12 @@ static int __init init_kprobes(void)
>  		INIT_HLIST_HEAD(&kprobe_table[i]);
>  		INIT_HLIST_HEAD(&kretprobe_inst_table[i]);
>  	}
> +	atomic_set(&kprobe_count, 0);
>  
>  	err = arch_init_kprobes();
>  	if (!err)
>  		err = register_die_notifier(&kprobe_exceptions_nb);
>  
> -	if (!err)
> -		err = register_page_fault_notifier(&kprobe_page_fault_nb);
> -
>  	return err;
>  }
>  
> 
> --

Re: [(resend)patch 0/7] Notify page fault call chain

[Keshavamurthy Anil S]

On Thu, Apr 20, 2006 at 10:27:17AM +1000, Keith Owens wrote:
> Anil S Keshavamurthy (on Wed, 19 Apr 2006 15:14:19 -0700) wrote:
> >Hi Andrew,
> >
> >Currently in the do_page_fault() code path, we call
> >notify_die(DIE_PAGE_FAULT, ...) to notify the page fault. 
> >The only interested components for this page fault 
> >notifications  are  Kprobes  and/or  kdb.
> 
> Only kprobes.  kdb does not care about page faults.

Oh.good to know this.

thanks,
Anil

Re: [(resend)patch 3/7] Notify page fault call chain for ia64

[Keshavamurthy Anil S]

On Thu, Apr 20, 2006 at 12:18:25PM +1000, Keith Owens wrote:
> Why is register_page_fault_notifier defined in traps.c?  Surely it
> should be in mm/fault.c, which is the only place that uses the chain.
Ah..that is because I blindly followed {register/unregister}_die_notifier()
implementation. It can moved to mm/faults.c and I will rework my patch.

> 
> > trap_init (void)
> > {
> >Index: linux-2.6.17-rc1-mm3/arch/ia64/mm/fault.c
> >===================================================================
> >--- linux-2.6.17-rc1-mm3.orig/arch/ia64/mm/fault.c
> >+++ linux-2.6.17-rc1-mm3/arch/ia64/mm/fault.c
> >@@ -84,7 +84,7 @@ ia64_do_page_fault (unsigned long addres
> > 	/*
> > 	 * This is to handle the kprobes on user space access instructions
> > 	 */
> >-	if (notify_die(DIE_PAGE_FAULT, "page fault", regs, code, TRAP_BRKPT,
> >+	if (notify_page_fault(DIE_PAGE_FAULT, "page fault", regs, code, TRAP_BRKPT,
> > 					SIGSEGV) == NOTIFY_STOP)
> > 		return;
> 
> Since this is a critical path, please remove all references to
> notify_page_fault() and its register functions when CONFIG_KPROBES=n.
Yes, I agree with you and thanks for your feedback.

-Anil

Re: [(resend)patch 7/7] Kprobes - Register for page fault notify on active probes

[Keshavamurthy Anil S]

On Thu, Apr 20, 2006 at 09:27:35AM +0530, Ananth N Mavinakayanahalli wrote:
> On Wed, Apr 19, 2006 at 03:14:26PM -0700, Anil S Keshavamurthy wrote:
> > With this patch Kprobes now registers for page fault notifications only
> > when their is an active probe registered. Once all the active probes are
> > unregistered their is no need to be notified of page faults and kprobes
> > unregisters itself from the page fault notifications. Hence we
> > will have ZERO side effects when no probes are active.
> 
> This patch isn't complete yet... comments below.
>  
> > Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> > ---
> >  kernel/kprobes.c |   25 +++++++++++++++++--------
> >  1 file changed, 17 insertions(+), 8 deletions(-)
> > 
> > Index: linux-2.6.17-rc1-mm3/kernel/kprobes.c
> > ===================================================================
> > --- linux-2.6.17-rc1-mm3.orig/kernel/kprobes.c
> > +++ linux-2.6.17-rc1-mm3/kernel/kprobes.c
> > @@ -47,11 +47,17 @@
> >  
> >  static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
> >  static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
> > +static atomic_t kprobe_count;
> >  
> >  DEFINE_MUTEX(kprobe_mutex);		/* Protects kprobe_table */
> >  DEFINE_SPINLOCK(kretprobe_lock);	/* Protects kretprobe_inst_table */
> >  static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
> >  
> > +static struct notifier_block kprobe_page_fault_nb = {
> > +	.notifier_call = kprobe_exceptions_notify,
> > +	.priority = 0x7fffffff /* we need to notified first */
> > +};
> > +
> >  #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
> >  /*
> >   * kprobe->ainsn.insn points to the copy of the instruction to be
> > @@ -464,6 +470,8 @@ static int __kprobes __register_kprobe(s
> >  	old_p = get_kprobe(p->addr);
> >  	if (old_p) {
> >  		ret = register_aggr_kprobe(old_p, p);
> > +		if (!ret)
> > +			atomic_inc(&kprobe_count);
> >  		goto out;
> >  	}
> >  
> > @@ -474,6 +482,9 @@ static int __kprobes __register_kprobe(s
> >  	hlist_add_head_rcu(&p->hlist,
> >  		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
> >  
> > +	if(atomic_add_return(1, &kprobe_count) == 2)
> 	^^^
> 	if (..) please, here and elsewhere
> 
> This will not work as desired for i386 (i386 no longer has a kprobe on the
> trampoline) and sparc64 (no retprobe support).
Instead of hardcoding value 2, #define KPROBE_ARCH_INITIAL_COUNT x
should do the trick.
> 
> > +		register_page_fault_notifier(&kprobe_page_fault_nb);
> > +
> >    	arch_arm_kprobe(p);
> >  
> >  out:
> > @@ -523,9 +534,13 @@ valid_p:
> >  		cleanup_p = 0;
> >  	}
> >  
> > +	if(atomic_add_return(-1, &kprobe_count) == 1)
> > +		unregister_page_fault_notifier(&kprobe_page_fault_nb);
> 
> Same here - i386 and sparc64 need different checks.
Yup, will take care in my next version.

> 
> > +	else
> > +		synchronize_rcu();
> 
> As of now, synchronize_sched() is aliased to synchronize_rcu() but I am
> told its scheduled to change in the future.
> 
> Please revert this back to synchronize_sched().
> 
I will revert it back in my take2 :)

Thanks again for your valuable feedback.

-Anil Keshavamurthy