Re:[PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.

Re:[PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.

[Rusty Lynch]

>-----Original Message-----
>From: linux-ia64-owner@vger.kernel.org [mailto:linux-ia64-
>owner@vger.kernel.org] On Behalf Of Christoph Lameter
>Sent: Thursday, August 25, 2005 1:14 PM
>To: linux-ia64@vger.kernel.org
>Cc: linux-mm@kvack.org; prasanna@in.ibm.com
>Subject: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.
>
>ia64_do_page_fault is a path critical for system performance. 
>The code to call notify_die() should not be compiled into that critical path 
>if the system is not configured to use KPROBES.

Just to be sure everyone understands the overhead involved, kprobes only 
registers a single notifier.  If kprobes is disabled (CONFIG_KPROBES is
off) then the overhead on a page fault is the overhead to execute an empty
notifier chain.

The debate over wrapping this notification call chain by a #define has
surfaced in the past for other architectures (and also when the initial ia64
kprobe patches were submitted to the MM tree), and when the dust settled
the notifier chain was left unwrapped.

If the consensus is that executing an empty notifier chain introduces a
measurable performance hit, then I think:
* A new config option should be introduced to disable this hook and then
  let CONFIG_KPROBES depend on this new option since other kernel components
  could choose to use this hook.
* The patch should do the same for all architectures, not just ia64

    --rusty

>Signed-off-by: Christoph Lameter <clameter@sgi.com>
>
>Index: linux-2.6.13-rc7/arch/ia64/mm/fault.c
>===================================================================
>--- linux-2.6.13-rc7.orig/arch/ia64/mm/fault.c	2005-08-23
>20:39:14.000000000 -0700
>+++ linux-2.6.13-rc7/arch/ia64/mm/fault.c	2005-08-25 13:04:57.000000000 - 0700
>@@ -103,12 +103,16 @@ ia64_do_page_fault (unsigned long addres
> 		goto bad_area_no_up;
> #endif
>
>+#ifdef CONFIG_KPROBES
> 	/*
>-	 * This is to handle the kprobes on user space access instructions
>+	 * This is to handle the kprobes on user space access instructions.
>+	 * This is a path criticial for system performance. So only
>+	 * process this notifier if we are compiled with kprobes support.
> 	 */
> 	if (notify_die(DIE_PAGE_FAULT, "page fault", regs, code, TRAP_BRKPT,
> 					SIGSEGV) == NOTIFY_STOP)
> 		return;
>+#endif
>
> 	down_read(&mm->mmap_sem);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re:[PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.

[Christoph Lameter]

On Fri, 26 Aug 2005, Rusty Lynch wrote:

> Just to be sure everyone understands the overhead involved, kprobes only 
> registers a single notifier.  If kprobes is disabled (CONFIG_KPROBES is
> off) then the overhead on a page fault is the overhead to execute an empty
> notifier chain.

Its the overhead of using registers to pass parameters, performing a 
function call that does nothing etc. A waste of computing resources. All 
of that unconditionally in a performance critical execution path that 
is executed a gazillion times for an optional feature that I frankly 
find not useful at all and that is disabled by default.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.

[Andi Kleen]

On Saturday 27 August 2005 01:05, Christoph Lameter wrote:
> On Fri, 26 Aug 2005, Rusty Lynch wrote:
> > Just to be sure everyone understands the overhead involved, kprobes only
> > registers a single notifier.  If kprobes is disabled (CONFIG_KPROBES is
> > off) then the overhead on a page fault is the overhead to execute an
> > empty notifier chain.
>
> Its the overhead of using registers to pass parameters, performing a
> function call that does nothing etc. A waste of computing resources. All
> of that unconditionally in a performance critical execution path that
> is executed a gazillion times for an optional feature that I frankly
> find not useful at all and that is disabled by default.

In the old days notifier_call_chain used to be inline. Then someone looking
at code size out of lined it. Perhaps it should be inlined again or notifier.h
could supply a special faster inline version for time critical code.

Then it would be simple if (global_var != NULL) { ... } in the fast path.
In addition the call chain could be declared __read_mostly.

I suspect with these changes Christoph's concerns would go away, right?

-Andi
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.

[Rusty Lynch]

On Sat, Aug 27, 2005 at 02:24:25AM +0200, Andi Kleen wrote:
> On Saturday 27 August 2005 01:05, Christoph Lameter wrote:
> > On Fri, 26 Aug 2005, Rusty Lynch wrote:
> > > Just to be sure everyone understands the overhead involved, kprobes only
> > > registers a single notifier.  If kprobes is disabled (CONFIG_KPROBES is
> > > off) then the overhead on a page fault is the overhead to execute an
> > > empty notifier chain.
> >
> > Its the overhead of using registers to pass parameters, performing a
> > function call that does nothing etc. A waste of computing resources. All
> > of that unconditionally in a performance critical execution path that
> > is executed a gazillion times for an optional feature that I frankly
> > find not useful at all and that is disabled by default.
> 
> In the old days notifier_call_chain used to be inline. Then someone looking
> at code size out of lined it. Perhaps it should be inlined again or notifier.h
> could supply a special faster inline version for time critical code.
> 
> Then it would be simple if (global_var != NULL) { ... } in the fast path.
> In addition the call chain could be declared __read_mostly.
> 
> I suspect with these changes Christoph's concerns would go away, right?
> 
> -Andi

So, assuming inlining the notifier_call_chain would address Christoph's
conserns, is the following patch something like what you are sugesting?  
This would make all the kdebug.h::notify_die() calls use the inline version. 

(WARNING:  The following has only been tested on ia64.)

 include/asm-i386/kdebug.h    |    2 +-
 include/asm-ia64/kdebug.h    |    2 +-
 include/asm-ppc64/kdebug.h   |    2 +-
 include/asm-sparc64/kdebug.h |    2 +-
 include/asm-x86_64/kdebug.h  |    2 +-
 include/linux/notifier.h     |   18 ++++++++++++++++++
 kernel/sys.c                 |   14 +-------------
 7 files changed, 24 insertions(+), 18 deletions(-)

Index: linux-2.6.13/include/linux/notifier.h
===================================================================
--- linux-2.6.13.orig/include/linux/notifier.h
+++ linux-2.6.13/include/linux/notifier.h
@@ -72,5 +72,23 @@ extern int notifier_call_chain(struct no
 #define CPU_DOWN_FAILED		0x0006 /* CPU (unsigned)v NOT going down */
 #define CPU_DEAD		0x0007 /* CPU (unsigned)v dead */
 
+static inline int fast_notifier_call_chain(struct notifier_block **n,
+					   unsigned long val, void *v)
+{
+	int ret=NOTIFY_DONE;
+	struct notifier_block *nb = *n;
+
+	while(nb)
+	{
+		ret=nb->notifier_call(nb,val,v);
+		if(ret&NOTIFY_STOP_MASK)
+		{
+			return ret;
+		}
+		nb=nb->next;
+	}
+	return ret;
+}
+
 #endif /* __KERNEL__ */
 #endif /* _LINUX_NOTIFIER_H */
Index: linux-2.6.13/kernel/sys.c
===================================================================
--- linux-2.6.13.orig/kernel/sys.c
+++ linux-2.6.13/kernel/sys.c
@@ -169,19 +169,7 @@ EXPORT_SYMBOL(notifier_chain_unregister)
  
 int notifier_call_chain(struct notifier_block **n, unsigned long val, void *v)
 {
-	int ret=NOTIFY_DONE;
-	struct notifier_block *nb = *n;
-
-	while(nb)
-	{
-		ret=nb->notifier_call(nb,val,v);
-		if(ret&NOTIFY_STOP_MASK)
-		{
-			return ret;
-		}
-		nb=nb->next;
-	}
-	return ret;
+	return fast_notifier_call_chain(n, val, v);
 }
 
 EXPORT_SYMBOL(notifier_call_chain);
Index: linux-2.6.13/include/asm-ia64/kdebug.h
===================================================================
--- linux-2.6.13.orig/include/asm-ia64/kdebug.h
+++ linux-2.6.13/include/asm-ia64/kdebug.h
@@ -55,7 +55,7 @@ static inline int notify_die(enum die_va
 		.signr  = sig
 	};
 
-	return notifier_call_chain(&ia64die_chain, val, &args);
+	return fast_notifier_call_chain(&ia64die_chain, val, &args);
 }
 
 #endif
Index: linux-2.6.13/include/asm-i386/kdebug.h
===================================================================
--- linux-2.6.13.orig/include/asm-i386/kdebug.h
+++ linux-2.6.13/include/asm-i386/kdebug.h
@@ -44,7 +44,7 @@ enum die_val {
 static inline int notify_die(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 notifier_call_chain(&i386die_chain, val, &args);
+	return fast_notifier_call_chain(&i386die_chain, val, &args);
 }
 
 #endif
Index: linux-2.6.13/include/asm-ppc64/kdebug.h
===================================================================
--- linux-2.6.13.orig/include/asm-ppc64/kdebug.h
+++ linux-2.6.13/include/asm-ppc64/kdebug.h
@@ -37,7 +37,7 @@ enum die_val {
 static inline int notify_die(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 notifier_call_chain(&ppc64_die_chain, val, &args);
+	return fast_notifier_call_chain(&ppc64_die_chain, val, &args);
 }
 
 #endif
Index: linux-2.6.13/include/asm-sparc64/kdebug.h
===================================================================
--- linux-2.6.13.orig/include/asm-sparc64/kdebug.h
+++ linux-2.6.13/include/asm-sparc64/kdebug.h
@@ -46,7 +46,7 @@ static inline int notify_die(enum die_va
 				 .trapnr	= trap,
 				 .signr		= sig };
 
-	return notifier_call_chain(&sparc64die_chain, val, &args);
+	return fast_notifier_call_chain(&sparc64die_chain, val, &args);
 }
 
 #endif
Index: linux-2.6.13/include/asm-x86_64/kdebug.h
===================================================================
--- linux-2.6.13.orig/include/asm-x86_64/kdebug.h
+++ linux-2.6.13/include/asm-x86_64/kdebug.h
@@ -38,7 +38,7 @@ enum die_val { 
 static inline int notify_die(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 notifier_call_chain(&die_chain, val, &args); 
+	return fast_notifier_call_chain(&die_chain, val, &args);
 } 
 
 extern int printk_address(unsigned long address);
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.

[Andi Kleen]

On Tuesday 30 August 2005 02:19, Rusty Lynch wrote:

>
> So, assuming inlining the notifier_call_chain would address Christoph's
> conserns, is the following patch something like what you are sugesting?

Yes.

Well in theory you could make fast and slow notify_die too, but that's
probably not worth it.

-Andi
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.

[Christoph Lameter]

On Mon, 29 Aug 2005, Rusty Lynch wrote:

> So, assuming inlining the notifier_call_chain would address Christoph's
> conserns, is the following patch something like what you are sugesting?  
> This would make all the kdebug.h::notify_die() calls use the inline version. 

Please do not generate any code if the feature cannot ever be 
used (CONFIG_KPROBES off). With this patch we still have lots of 
unnecessary code being executed on each page fault.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.

[Matthew Wilcox]

On Mon, Aug 29, 2005 at 05:19:05PM -0700, Rusty Lynch wrote:
> So, assuming inlining the notifier_call_chain would address Christoph's
> conserns, is the following patch something like what you are sugesting?  
> This would make all the kdebug.h::notify_die() calls use the inline version. 

I think we need something more like this ...

include/linux/notifier.h:
+static inline int notifier_call_chain(struct notifier_block **n,
+					unsigned long val, void *v)
+{
+	if (n)
+		return __notifier_call_chain(n, val, v);
+	return NOTIFY_DONE;
+}
kernel/sys.c:
-int notifier_call_chain(struct notifier_block **n, unsigned long val, void *v)
+int __notifier_call_chain(struct notifier_block **n, unsigned long val, void *v)
-EXPORT_SYMBOL(notifier_call_chain);
+EXPORT_SYMBOL(__notifier_call_chain);

That way everyone gets both the quick test and the global size reduction.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.

[Christoph Lameter]

On Tue, 30 Aug 2005, Matthew Wilcox wrote:

> On Mon, Aug 29, 2005 at 05:19:05PM -0700, Rusty Lynch wrote:
> > So, assuming inlining the notifier_call_chain would address Christoph's
> > conserns, is the following patch something like what you are sugesting?  
> > This would make all the kdebug.h::notify_die() calls use the inline version. 
> 
> I think we need something more like this ...
> 
> include/linux/notifier.h:
> +static inline int notifier_call_chain(struct notifier_block **n,
> +					unsigned long val, void *v)
> +{
> +	if (n)
> +		return __notifier_call_chain(n, val, v);
> +	return NOTIFY_DONE;
> +}
> kernel/sys.c:
> -int notifier_call_chain(struct notifier_block **n, unsigned long val, void *v)
> +int __notifier_call_chain(struct notifier_block **n, unsigned long val, void *v)
> -EXPORT_SYMBOL(notifier_call_chain);
> +EXPORT_SYMBOL(__notifier_call_chain);
> 
> That way everyone gets both the quick test and the global size reduction.

And then do

#ifndef CONFIG_KPROBES

#define ia64die_chain 0

#endif

in include/asm-ia64/kdebug.h?

Otherwise we still check a notifier chain that cannot ever be 
activated.

But then the patch is essentially the same as the last one I proposed.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>