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

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

[Anil S Keshavamurthy]

Currently in the do_page_fault() code path, we call
notify_die(DIE_PAGE_FAULT, ...) to notify the page fault. 
Since notify_die() is highly overloaded, this page fault  
notification is currently being sent to all the components
registered   with  register_die_notification()  which  uses  the  same
die_chain to loop for all the registered components which is unnecessary.

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 showed great improvements.

And the kprobes which is interested in this notifications, now registers
onto this new call chain only when it need to, i.e Kprobes now registers
for page fault notification only when their are an active probes and
unregisters from this page fault notification when no probes are active.

I have incorporated all the feedback given by Ananth and Keith and everyone,
and thanks for all the review feedback.

Andrew, please apply this patch to your mm tree and I would gladly fix
if their are any other issues.

thanks,
Anil Keshavamurthy

--

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

[Anil S Keshavamurthy]

[-- Attachment #1: notify_page_fault_x86_64.patch --]
[-- Type: text/plain, Size: 3335 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.

Changes since previous version:
1) Moved the implementaion of register/unregister_page_fault_notifier()
call to arch/xxx/mm/fault.c
2) Added #ifdef CONFIG_KPROBES around this register/unregister calls
as currently kprobes is the only potential user.

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

---
 arch/x86_64/mm/fault.c      |   39 +++++++++++++++++++++++++++++++++++++--
 include/asm-x86_64/kdebug.h |    2 ++
 2 files changed, 39 insertions(+), 2 deletions(-)

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
@@ -41,6 +41,41 @@
 #define PF_RSVD	(1<<3)
 #define PF_INSTR	(1<<4)
 
+#ifdef CONFIG_KPROBES
+ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);
+
+/* Hook to register for page fault notifications */
+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 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);
+}
+#else
+static inline int notify_page_fault(enum die_val val, const char *str,
+			struct pt_regs *regs, long err, int trap, int sig)
+{
+	return NOTIFY_DONE;
+}
+#endif
+
 void bust_spinlocks(int yes)
 {
 	int loglevel_save = console_loglevel;
@@ -348,7 +383,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 +393,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,6 +15,8 @@ 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;
 
 /* Grossly misnamed. */

--

[(take 2)patch 2/7] Notify page fault call chain for i386

[Anil S Keshavamurthy]

[-- Attachment #1: notify_page_fault_i386.patch --]
[-- Type: text/plain, Size: 3328 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.

Changes since previous version:
1) Moved the implementaion of register/unregister_page_fault_notifier()
call to arch/xxx/mm/fault.c
2) Added #ifdef CONFIG_KPROBES around this register/unregister calls
as currently kprobes is the only potential user.

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

---
 arch/i386/mm/fault.c      |   38 ++++++++++++++++++++++++++++++++++++--
 include/asm-i386/kdebug.h |    2 ++
 2 files changed, 38 insertions(+), 2 deletions(-)

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
@@ -30,6 +30,40 @@
 
 extern void die(const char *,struct pt_regs *,long);
 
+#ifdef CONFIG_KPROBES
+ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);
+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 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);
+}
+#else
+static inline int notify_page_fault(enum die_val val, const char *str,
+			struct pt_regs *regs, long err, int trap, int sig)
+{
+	return NOTIFY_DONE;
+}
+#endif
+
+
 /*
  * Unlock any spinlocks which will prevent us from getting the
  * message out 
@@ -321,7 +355,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 +365,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,6 +19,8 @@ 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;
 
 

--

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

[Anil S Keshavamurthy]

[-- Attachment #1: notify_page_fault_ia64.patch --]
[-- Type: text/plain, Size: 3089 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.

Changes since previous version:
1) Moved the implementaion of register/unregister_page_fault_notifier()
call to arch/xxx/mm/fault.c
2) Added #ifdef CONFIG_KPROBES around this register/unregister calls
as currently kprobes is the only potential user.

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

---
 arch/ia64/mm/fault.c      |   36 +++++++++++++++++++++++++++++++++++-
 include/asm-ia64/kdebug.h |    2 ++
 2 files changed, 37 insertions(+), 1 deletion(-)

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
@@ -19,6 +19,40 @@
 
 extern void die (char *, struct pt_regs *, long);
 
+#ifdef CONFIG_KPROBES
+ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);
+
+/* Hook to register for page fault notifications */
+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);
+}
+
+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);
+}
+#else
+static inline int notify_page_fault(enum die_val val, const char *str,
+			struct pt_regs *regs, long err, int trap, int sig)
+{
+	return NOTIFY_DONE;
+}
+#endif
+
 /*
  * Return TRUE if ADDRESS points at a page in the kernel's mapped segment
  * (inside region 5, on ia64) and that page is present.
@@ -84,7 +118,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,6 +40,8 @@ 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;
 
 enum die_val {

--

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

[Anil S Keshavamurthy]

[-- Attachment #1: notify_page_fault_powerpc.patch --]
[-- Type: text/plain, Size: 3101 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.

Changes since previous version:
1) Moved the implementaion of register/unregister_page_fault_notifier()
call to arch/xxx/mm/fault.c
2) Added #ifdef CONFIG_KPROBES around this register/unregister calls
as currently kprobes is the only potential user.

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


---
 arch/powerpc/mm/fault.c      |   36 +++++++++++++++++++++++++++++++++++-
 include/asm-powerpc/kdebug.h |    2 ++
 2 files changed, 37 insertions(+), 1 deletion(-)

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
@@ -40,6 +40,40 @@
 #include <asm/kdebug.h>
 #include <asm/siginfo.h>
 
+#ifdef CONFIG_KPROBES
+ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);
+
+/* Hook to register for page fault notifications */
+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);
+}
+
+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);
+}
+#else
+static inline int notify_page_fault(enum die_val val, const char *str,
+			struct pt_regs *regs, long err, int trap, int sig)
+{
+	return NOTIFY_DONE;
+}
+#endif
+
 /*
  * Check whether the instruction at regs->nip is a store using
  * an update addressing form which will update r1.
@@ -142,7 +176,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,6 +18,8 @@ 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;
 
 /* Grossly misnamed. */

--

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

[Anil S Keshavamurthy]

[-- Attachment #1: notify_page_fault_sparc64.patch --]
[-- Type: text/plain, Size: 3124 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.

Changes since previous version:
1) Moved the implementaion of register/unregister_page_fault_notifier()
call to arch/xxx/mm/fault.c
2) Added #ifdef CONFIG_KPROBES around this register/unregister calls
as currently kprobes is the only potential user.

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


---
 arch/sparc64/mm/fault.c      |   36 +++++++++++++++++++++++++++++++++++-
 include/asm-sparc64/kdebug.h |    2 ++
 2 files changed, 37 insertions(+), 1 deletion(-)

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
@@ -31,6 +31,40 @@
 #include <asm/kdebug.h>
 #include <asm/mmu_context.h>
 
+#ifdef CONFIG_KPROBES
+ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);
+
+/* Hook to register for page fault notifications */
+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);
+}
+
+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);
+}
+#else
+static inline int notify_page_fault(enum die_val val, const char *str,
+			struct pt_regs *regs, long err, int trap, int sig)
+{
+	return NOTIFY_DONE;
+}
+#endif
+
 /*
  * To debug kernel to catch accesses to certain virtual/physical addresses.
  * Mode = 0 selects physical watchpoints, mode = 1 selects virtual watchpoints.
@@ -263,7 +297,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,6 +17,8 @@ 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 void bad_trap(struct pt_regs *, long);

--

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

[Anil S Keshavamurthy]

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

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

--

Re: [(take 2)patch 6/7] Kprobes registers for notify page fault

[Keshavamurthy Anil S]

On Thu, Apr 20, 2006 at 04:25:02PM -0700, Anil S Keshavamurthy wrote:
> ---

Ha..I had missed the description again.


Patch Description:
Kprobes now registers for page fault notifications.

Signed-off-by: Anil S Keshavamurthy <anil.s.keshavmurthy@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;
>  }
>  
> 
> --
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [(take 2)patch 6/7] Kprobes registers for notify page fault

[Keith Owens]

Anil S Keshavamurthy (on Thu, 20 Apr 2006 16:25:02 -0700) wrote:
>---
> 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;
> }
> 

The rest of the patches look OK, but this one does not.  init_kprobes()
registers the main kprobe exception handler, not the page fault
handler.

Now that there is a dedicated page fault handler, instead of being a
subcase of notify_die(), it might be better to delete DIE_PAGE_FAULT
completely.  That can be done in this patch set or in some follow on
patches.

Re: [(take 2)patch 6/7] Kprobes registers for notify page fault

[Keshavamurthy Anil S]

On Fri, Apr 21, 2006 at 10:14:04AM +1000, Keith Owens wrote:
> Anil S Keshavamurthy (on Thu, 20 Apr 2006 16:25:02 -0700) wrote:
> >---
> >@@ -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;
> > }
> > 
> 
> The rest of the patches look OK, but this one does not.  init_kprobes()
> registers the main kprobe exception handler, not the page fault
> handler.
I am registering for register_page_fault_notifier() and as you can see
I am not deleting the old register_die_notifier() which is also required
for getting notified on int3/break and single-step traps. 
So no issues here.

> 
> Now that there is a dedicated page fault handler, instead of being a
> subcase of notify_die(), it might be better to delete DIE_PAGE_FAULT
> completely.  That can be done in this patch set or in some follow on
> patches.
It can be done as a follow up (cleanup patch) and if you see the whole
whole DIE_XXXX is grossly missnamed. I did not want to address two many
things in one patch.

thanks,
-Anil Keshavamurthy

Re: [(take 2)patch 6/7] Kprobes registers for notify page fault

[Keith Owens]

Keshavamurthy Anil S (on Thu, 20 Apr 2006 17:38:47 -0700) wrote:
>On Fri, Apr 21, 2006 at 10:14:04AM +1000, Keith Owens wrote:
>> Anil S Keshavamurthy (on Thu, 20 Apr 2006 16:25:02 -0700) wrote:
>> >---
>> >@@ -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;
>> > }
>> > 
>> 
>> The rest of the patches look OK, but this one does not.  init_kprobes()
>> registers the main kprobe exception handler, not the page fault
>> handler.
>I am registering for register_page_fault_notifier() and as you can see
>I am not deleting the old register_die_notifier() which is also required
>for getting notified on int3/break and single-step traps. 
>So no issues here.

Patch 7 conditionally registers a page fault handler, plus an
unregister when there are no user space probes.  Why does patch 6
unconditionally register a page fault handler?

Re: [(take 2)patch 6/7] Kprobes registers for notify page fault

[Keshavamurthy Anil S]

On Fri, Apr 21, 2006 at 10:53:29AM +1000, Keith Owens wrote:
> Keshavamurthy Anil S (on Thu, 20 Apr 2006 17:38:47 -0700) wrote:
> >On Fri, Apr 21, 2006 at 10:14:04AM +1000, Keith Owens wrote:
> >> Anil S Keshavamurthy (on Thu, 20 Apr 2006 16:25:02 -0700) wrote:
> >> >---
> >> >@@ -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;
> >> > }
> >> > 
> >> 
> >> The rest of the patches look OK, but this one does not.  init_kprobes()
> >> registers the main kprobe exception handler, not the page fault
> >> handler.
> >I am registering for register_page_fault_notifier() and as you can see
> >I am not deleting the old register_die_notifier() which is also required
> >for getting notified on int3/break and single-step traps. 
> >So no issues here.
> 
> Patch 7 conditionally registers a page fault handler, plus an
> unregister when there are no user space probes.  Why does patch 6
> unconditionally register a page fault handler?
Patch 7 is the improvement of patch 6, so in other words the patch
7 removes the patch 6's unconditional registration and adds
conditional registration support. 

-Anil

[(take 2)patch 7/7] Notify page fault call chain

[Anil S Keshavamurthy]

[-- Attachment #1: kprobes_noitfy_register.patch --]
[-- Type: text/plain, Size: 5753 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>
---
 include/asm-i386/kprobes.h    |    1 +
 include/asm-ia64/kprobes.h    |    1 +
 include/asm-powerpc/kprobes.h |    2 ++
 include/asm-sparc64/kprobes.h |    1 +
 include/asm-x86_64/kprobes.h  |    1 +
 kernel/kprobes.c              |   30 +++++++++++++++++++++++-------
 6 files changed, 29 insertions(+), 7 deletions(-)

Index: linux-2.6.17-rc1-mm3/include/asm-i386/kprobes.h
===================================================================
--- linux-2.6.17-rc1-mm3.orig/include/asm-i386/kprobes.h
+++ linux-2.6.17-rc1-mm3/include/asm-i386/kprobes.h
@@ -44,6 +44,7 @@ typedef u8 kprobe_opcode_t;
 
 #define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
 #define ARCH_SUPPORTS_KRETPROBES
+#define  ARCH_INACTIVE_KPROBE_COUNT 0
 
 void arch_remove_kprobe(struct kprobe *p);
 void kretprobe_trampoline(void);
Index: linux-2.6.17-rc1-mm3/include/asm-ia64/kprobes.h
===================================================================
--- linux-2.6.17-rc1-mm3.orig/include/asm-ia64/kprobes.h
+++ linux-2.6.17-rc1-mm3/include/asm-ia64/kprobes.h
@@ -82,6 +82,7 @@ struct kprobe_ctlblk {
 #define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
 
 #define ARCH_SUPPORTS_KRETPROBES
+#define  ARCH_INACTIVE_KPROBE_COUNT 1
 
 #define SLOT0_OPCODE_SHIFT	(37)
 #define SLOT1_p1_OPCODE_SHIFT	(37 - (64-46))
Index: linux-2.6.17-rc1-mm3/include/asm-powerpc/kprobes.h
===================================================================
--- linux-2.6.17-rc1-mm3.orig/include/asm-powerpc/kprobes.h
+++ linux-2.6.17-rc1-mm3/include/asm-powerpc/kprobes.h
@@ -50,6 +50,8 @@ typedef unsigned int kprobe_opcode_t;
 			IS_TWI(instr) || IS_TDI(instr))
 
 #define ARCH_SUPPORTS_KRETPROBES
+#define  ARCH_INACTIVE_KPROBE_COUNT 1
+
 void kretprobe_trampoline(void);
 extern void arch_remove_kprobe(struct kprobe *p);
 
Index: linux-2.6.17-rc1-mm3/include/asm-sparc64/kprobes.h
===================================================================
--- linux-2.6.17-rc1-mm3.orig/include/asm-sparc64/kprobes.h
+++ linux-2.6.17-rc1-mm3/include/asm-sparc64/kprobes.h
@@ -13,6 +13,7 @@ typedef u32 kprobe_opcode_t;
 
 #define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
 #define arch_remove_kprobe(p)	do {} while (0)
+#define  ARCH_INACTIVE_KPROBE_COUNT 0
 
 /* Architecture specific copy of original instruction*/
 struct arch_specific_insn {
Index: linux-2.6.17-rc1-mm3/include/asm-x86_64/kprobes.h
===================================================================
--- linux-2.6.17-rc1-mm3.orig/include/asm-x86_64/kprobes.h
+++ linux-2.6.17-rc1-mm3/include/asm-x86_64/kprobes.h
@@ -43,6 +43,7 @@ typedef u8 kprobe_opcode_t;
 
 #define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
 #define ARCH_SUPPORTS_KRETPROBES
+#define  ARCH_INACTIVE_KPROBE_COUNT 1
 
 void kretprobe_trampoline(void);
 extern void arch_remove_kprobe(struct kprobe *p);
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,10 @@ 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) == \
+				(ARCH_INACTIVE_KPROBE_COUNT + 1))
+		register_page_fault_notifier(&kprobe_page_fault_nb);
+
   	arch_arm_kprobe(p);
 
 out:
@@ -537,6 +549,16 @@ valid_p:
 		}
 		arch_remove_kprobe(p);
 	}
+
+	/* 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;
 }
 
 static struct notifier_block kprobe_exceptions_nb = {
@@ -544,10 +566,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 +672,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: [(take 2)patch 7/7] Notify page fault call chain

[bibo mao]

I have some questions about page fault call chain.
1) Can kprobe_exceptions_notify be divided into two sub-functions, one 
is for die call chain, which handles DIE_BREAK/DIE_FAULT trap, the other 
is specially for DIE_PAGE_FAULT trap.

2) page_fault_notifier is conditionally registered/unregistered in this 
patch, notify_page_fault(DIE_PAGE_FAULT...) is unconditionally called in 
  ia64_do_page_fault() function. I do not know whether it is possible to 
unconditionally register page_fault_notifier() call chain, but 
conditionally call notify_page_fault(DIE_PAGE_FAULT...) function.

3) I do know whether there are other conditions like kdb/kgdb which need
call page fault call chain when page fault happens. If only kprobe need 
handle page fault, then I think kprobe_exceptions_notify can be called 
directly in ia64_do_page_fault() function. Of course,  the call chain 
method is more convenient and easy to extend for other fault causes(like 
kdb/kgdb).

thanks
bibo,mao

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.
> 
> Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> ---
>  include/asm-i386/kprobes.h    |    1 +
>  include/asm-ia64/kprobes.h    |    1 +
>  include/asm-powerpc/kprobes.h |    2 ++
>  include/asm-sparc64/kprobes.h |    1 +
>  include/asm-x86_64/kprobes.h  |    1 +
>  kernel/kprobes.c              |   30 +++++++++++++++++++++++-------
>  6 files changed, 29 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6.17-rc1-mm3/include/asm-i386/kprobes.h
> ===================================================================
> --- linux-2.6.17-rc1-mm3.orig/include/asm-i386/kprobes.h
> +++ linux-2.6.17-rc1-mm3/include/asm-i386/kprobes.h
> @@ -44,6 +44,7 @@ typedef u8 kprobe_opcode_t;
>  
>  #define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
>  #define ARCH_SUPPORTS_KRETPROBES
> +#define  ARCH_INACTIVE_KPROBE_COUNT 0
>  
>  void arch_remove_kprobe(struct kprobe *p);
>  void kretprobe_trampoline(void);
> Index: linux-2.6.17-rc1-mm3/include/asm-ia64/kprobes.h
> ===================================================================
> --- linux-2.6.17-rc1-mm3.orig/include/asm-ia64/kprobes.h
> +++ linux-2.6.17-rc1-mm3/include/asm-ia64/kprobes.h
> @@ -82,6 +82,7 @@ struct kprobe_ctlblk {
>  #define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
>  
>  #define ARCH_SUPPORTS_KRETPROBES
> +#define  ARCH_INACTIVE_KPROBE_COUNT 1
>  
>  #define SLOT0_OPCODE_SHIFT	(37)
>  #define SLOT1_p1_OPCODE_SHIFT	(37 - (64-46))
> Index: linux-2.6.17-rc1-mm3/include/asm-powerpc/kprobes.h
> ===================================================================
> --- linux-2.6.17-rc1-mm3.orig/include/asm-powerpc/kprobes.h
> +++ linux-2.6.17-rc1-mm3/include/asm-powerpc/kprobes.h
> @@ -50,6 +50,8 @@ typedef unsigned int kprobe_opcode_t;
>  			IS_TWI(instr) || IS_TDI(instr))
>  
>  #define ARCH_SUPPORTS_KRETPROBES
> +#define  ARCH_INACTIVE_KPROBE_COUNT 1
> +
>  void kretprobe_trampoline(void);
>  extern void arch_remove_kprobe(struct kprobe *p);
>  
> Index: linux-2.6.17-rc1-mm3/include/asm-sparc64/kprobes.h
> ===================================================================
> --- linux-2.6.17-rc1-mm3.orig/include/asm-sparc64/kprobes.h
> +++ linux-2.6.17-rc1-mm3/include/asm-sparc64/kprobes.h
> @@ -13,6 +13,7 @@ typedef u32 kprobe_opcode_t;
>  
>  #define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
>  #define arch_remove_kprobe(p)	do {} while (0)
> +#define  ARCH_INACTIVE_KPROBE_COUNT 0
>  
>  /* Architecture specific copy of original instruction*/
>  struct arch_specific_insn {
> Index: linux-2.6.17-rc1-mm3/include/asm-x86_64/kprobes.h
> ===================================================================
> --- linux-2.6.17-rc1-mm3.orig/include/asm-x86_64/kprobes.h
> +++ linux-2.6.17-rc1-mm3/include/asm-x86_64/kprobes.h
> @@ -43,6 +43,7 @@ typedef u8 kprobe_opcode_t;
>  
>  #define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
>  #define ARCH_SUPPORTS_KRETPROBES
> +#define  ARCH_INACTIVE_KPROBE_COUNT 1
>  
>  void kretprobe_trampoline(void);
>  extern void arch_remove_kprobe(struct kprobe *p);
> 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,10 @@ 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) == \
> +				(ARCH_INACTIVE_KPROBE_COUNT + 1))
> +		register_page_fault_notifier(&kprobe_page_fault_nb);
> +
>    	arch_arm_kprobe(p);
>  
>  out:
> @@ -537,6 +549,16 @@ valid_p:
>  		}
>  		arch_remove_kprobe(p);
>  	}
> +
> +	/* 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;
>  }
>  
>  static struct notifier_block kprobe_exceptions_nb = {
> @@ -544,10 +566,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 +672,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;
>  }
>  
> 
> --
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [(take 2)patch 7/7] Notify page fault call chain

[Keith Owens]

bibo mao (on Mon, 24 Apr 2006 17:19:01 +0800) wrote:
>I have some questions about page fault call chain.
>1) Can kprobe_exceptions_notify be divided into two sub-functions, one 
>is for die call chain, which handles DIE_BREAK/DIE_FAULT trap, the other 
>is specially for DIE_PAGE_FAULT trap.

I asked the same question, Anil said that would/could be done in a
later set of patches, but did not want to change too much code in one
go.

>2) page_fault_notifier is conditionally registered/unregistered in this 
>patch, notify_page_fault(DIE_PAGE_FAULT...) is unconditionally called in 
>  ia64_do_page_fault() function. I do not know whether it is possible to 
>unconditionally register page_fault_notifier() call chain, but 
>conditionally call notify_page_fault(DIE_PAGE_FAULT...) function.

Only by putting extra code at the site of notify_page_fault().  That
would introduce a race against kprobes unregistering a user space
probe.  The race is probably safe, but why risk it?

>3) I do know whether there are other conditions like kdb/kgdb which need
>call page fault call chain when page fault happens. If only kprobe need 
>handle page fault, then I think kprobe_exceptions_notify can be called 
>directly in ia64_do_page_fault() function. Of course,  the call chain 
>method is more convenient and easy to extend for other fault causes(like 
>kdb/kgdb).

Only kprobes needs the page fault handler.

Calling kprobe_exceptions_notify() directly would work but again it
introduces races.  The register and unregister are atomic, and remember
how long it took us to agree on how to make atomic register and
unregister race safe.  Using the existing notify chain code gives us
race safe register, call and unregister without having to verify that
any hand written replacement code is also race safe.

You would need to demonstrate that any hand crafted code was race safe
and had enough speed improvement to make the coding and debugging effort
worthwhile.

Re: [(take 2)patch 7/7] Notify page fault call chain

[Keshavamurthy Anil S]

On Tue, Apr 25, 2006 at 11:10:59AM +1000, Keith Owens wrote:
> bibo mao (on Mon, 24 Apr 2006 17:19:01 +0800) wrote:
> >I have some questions about page fault call chain.
> >1) Can kprobe_exceptions_notify be divided into two sub-functions, one 
> >is for die call chain, which handles DIE_BREAK/DIE_FAULT trap, the other 
> >is specially for DIE_PAGE_FAULT trap.
> 
> I asked the same question, Anil said that would/could be done in a
> later set of patches, but did not want to change too much code in one
> go.

I don't think it is necessary to split kprobe_exception_notify(). All it is 
having is a simple switch case and based on the swich case it takes appropriate
actions.

> 
> >2) page_fault_notifier is conditionally registered/unregistered in this 
> >patch, notify_page_fault(DIE_PAGE_FAULT...) is unconditionally called in 
> >  ia64_do_page_fault() function. I do not know whether it is possible to 
> >unconditionally register page_fault_notifier() call chain, but 
> >conditionally call notify_page_fault(DIE_PAGE_FAULT...) function.
> 
> Only by putting extra code at the site of notify_page_fault().  That
> would introduce a race against kprobes unregistering a user space
> probe.  The race is probably safe, but why risk it?

Their is no race while we call unregister_page_fault_notifier(), as we unregister
only after the last probe is removed and after synchronized_sched() is done which
guarantees that all the probes have completly finished executing.
> 
> >3) I do know whether there are other conditions like kdb/kgdb which need
> >call page fault call chain when page fault happens. If only kprobe need 
> >handle page fault, then I think kprobe_exceptions_notify can be called 
> >directly in ia64_do_page_fault() function. Of course,  the call chain 
> >method is more convenient and easy to extend for other fault causes(like 
> >kdb/kgdb).
> 
> Only kprobes needs the page fault handler.
> 
> Calling kprobe_exceptions_notify() directly would work but again it
> introduces races.  The register and unregister are atomic, and remember

Calling kprobe_exception_notify() directly can be made to work with
out any races if needed. Not sure if calling directly  will have 
any significant performance improvement over current non-overloaded
call chain notifications.

-Anil

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

[Robin Holt]

Anil,

This set definitely improves things.  My timings from last week must
have been off.  I think I may have still had the notify_die() call in
the fault path.  This week, I see a 35 nSec slowdown between with/without
KRPOBES.  Last week, I thought they were roughly equivalent.


Thanks,
Robin Holt

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

[Keshavamurthy Anil S]

On Mon, Apr 24, 2006 at 02:28:24PM -0500, Robin Holt wrote:
> Anil,
> 
> This set definitely improves things.  My timings from last week must
> have been off.  I think I may have still had the notify_die() call in
> the fault path.  This week, I see a 35 nSec slowdown between with/without
> KRPOBES.  Last week, I thought they were roughly equivalent.
The non-overloaded call chain notification with dynamic registeration/unregistration 
is much better than earlier one. But if you still want to improve the 35 nSec
slowdown, then the only other alternative is to eliminate the call chain and 
try calling kprobe_exceptions_notify() directly with the kprobe_running() around it.
i.e
static inline int notify_page_fault(enum die_val val, const char *str,
                        struct pt_regs *regs, long err, int trap, int sig)
{
	if(kprobe_running())
	{
        	struct die_args args = {
               	 .regs = regs,
               	 .str = str,
               	 .err = err,
               	 .trapnr = trap,
               	 .signr = sig

		return kprobe_exceptions_notify(NULL, &die_args, DIE_PAGE_FAULT);
	}
}

Anyone has any other comments/suggestion?

-thanks,
Anil

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

[Robin Holt]

On Mon, Apr 24, 2006 at 11:01:16PM -0700, Keshavamurthy Anil S wrote:
> On Mon, Apr 24, 2006 at 02:28:24PM -0500, Robin Holt wrote:
> > This set definitely improves things.  My timings from last week must
> > have been off.  I think I may have still had the notify_die() call in
> > the fault path.  This week, I see a 35 nSec slowdown between with/without
> > KRPOBES.  Last week, I thought they were roughly equivalent.
> The non-overloaded call chain notification with dynamic registeration/unregistration 
> is much better than earlier one. But if you still want to improve the 35 nSec
> slowdown, then the only other alternative is to eliminate the call chain and 
> try calling kprobe_exceptions_notify() directly with the kprobe_running() around it.
> i.e
> static inline int notify_page_fault(enum die_val val, const char *str,
>                         struct pt_regs *regs, long err, int trap, int sig)

If we do that, can we rename notify_page_fault to something with kprobes in it?