Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.
On Wednesday 31 August 2005 01:54, Christoph Lameter wrote: > Certainly not a big effect (if we make sure the compiler knows that > this test mostly fails and insure that the variable is in > __mostly_read) Currently neither, but that could be easily fixed. > but this is a frequently executed code path and the code > is there without purpose if CONFIG_KPROBES is off. Well if you really worry about it then it might be better to do some dynamic code patching to make generic and distribution kernels too. > It wont get too bad unless lots of other people have similar ideas about > fixing their race conditions using similar methods. But we will be setting > a bad precedent if we allow this. Agreed on the general direction, but I didn't see an alternative for kprobes for this. Well actually the hook could be maybe right now moved into the part before kernel oops which is much less frequently executed, but then it'll likely move up again once kprobes support user space probes. -Andi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.
From: Andi Kleen <[EMAIL PROTECTED]> Subject: Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured. Date: Wed, 31 Aug 2005 01:38:08 +0200 > On Wednesday 31 August 2005 01:05, Luck, Tony wrote: > > >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. > > > > I can (eventually) wrap this call inside the #ifdef CONFIG_KPROBES. > > At least the original die notifiers were designed as a generic debugger > interface, not a kprobes specific thing. So I don't think it's a good idea. Me neither, I think a way too big deal is being made about about this by the ia64 folks. Just put the dang hook in there unconditionally already :-) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.
On Wed, 31 Aug 2005, Andi Kleen wrote: > Also with the inline the test should be essentially a single test of > a global variable and jump. Hardly a big performance issue, no? There are multiple effects of this code. - Additional cacheline in use in the page fault handler increasing the cache foot print. - There are registers in use for checking the global variable. - The compilers will reserve registers for the code that is never executed which may affect other elements of performance. From the register perspective a function call may be better on ia64. Certainly not a big effect (if we make sure the compiler knows that this test mostly fails and insure that the variable is in __mostly_read) but this is a frequently executed code path and the code is there without purpose if CONFIG_KPROBES is off. It wont get too bad unless lots of other people have similar ideas about fixing their race conditions using similar methods. But we will be setting a bad precedent if we allow this. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.
On Wednesday 31 August 2005 01:05, Luck, Tony wrote: > >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. > > I can (eventually) wrap this call inside the #ifdef CONFIG_KPROBES. At least the original die notifiers were designed as a generic debugger interface, not a kprobes specific thing. So I don't think it's a good idea. Given most debuggers don't need the early page fault hook and it's mostly needed for a special case in kprobes, but it doesn't seem nice to only offer a subset of the hooks with specific config options. Also with the inline the test should be essentially a single test of a global variable and jump. Hardly a big performance issue, no? -Andi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.
>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. I can (eventually) wrap this call inside the #ifdef CONFIG_KPROBES. But I'd like to keep following leads on making the overhead as low as possible for those people that do have KPROBES configured (which may be most people if OS distributors ship kernels with this enabled). -Tony - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.
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 from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.
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 from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.
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_FAILED0x0006 /* 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, .
Re: [PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.
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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re:[PATCH] Only process_die notifier in ia64_do_page_fault if KPROBES is configured.
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 from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
