Immediate Values for 2.6.23-rc4-mm1

Remove "static" from module_mutex and the modules list so it can be used by other builtin objects in the kernel. Otherwise, every code depending on the module list would have to be put in kernel/module.c. Since the immediate values depends on the module list but can be considered as logically different, it makes sense to implement them in their own file.

The alternative to this would be to disable preemption in code path that need such synchronization, so they can be protected against module unload by stop_machine(), but not being able to sleep within while needing such synchronization is limiting.

kernel/module.c +2 -2

@@ -64,8 +64,8 @@ extern int module_sysfs_initialized;
 
 /* List of modules, protected by module_mutex or preempt_disable
  * (add/delete uses stop_machine). */
-static DEFINE_MUTEX(module_mutex);
-static LIST_HEAD(modules);
+DEFINE_MUTEX(module_mutex);
+LIST_HEAD(modules);
 static DECLARE_MUTEX(notify_mutex);
 
 static BLOCKING_NOTIFIER_HEAD(module_notify_list);

Quoted 5 lines · Mathieu Desnoyers

Remove "static" from module_mutex and the modules list so it can be used by other builtin objects in the kernel. Otherwise, every code depending on the module list would have to be put in kernel/module.c. Since the immediate values depends on the module list but can be considered as logically different, it makes sense to implement them in their own file.

Quoted 6 lines · Alexey Dobriyan

Quoted 5 lines · Mathieu Desnoyers

Remove "static" from module_mutex and the modules list so it can be used by other builtin objects in the kernel. Otherwise, every code depending on the module list would have to be put in kernel/module.c. Since the immediate values depends on the module list but can be considered as logically different, it makes sense to implement them in their own file.

If I understand this code correctly, then changing immediate values needs some exclusion to avoid patching live code. You leave this to the user with some very unclear rules.

The result is a real mess that has nothing to do with the module mutex and list. These patches need a lot more work 8(

1) The immediate types are just kind of silly. See per-cpu for how it handles this already. DECLARE_IMMEDIATE(type, var) is probably enough.

2) immediate_if() needs an implementation before you introduce it. Your assumption that it's always unlikely seems non-orthogonal.

If we can change the compiler, here is what we could do:

Tell GCC to put NOPs that could be altered by a branch alternative to some specified code. We should be able to get the instruction pointers (think of inlines) to these nop/branch instructions so we can change them dynamically.

I suspect this would be inherently tricky. If someone is ready to do this and tells me "yes, it will be there in 1 month", I am more than ready to switch my markers to this and help, but since the core of my work is kernel tracing, I don't have the time nor the ressources to tackle this problem.

In the event that someone answers "we'll do this in the following 3 years", I might consider to change the if (immediate(var)) into an immediate_if (var) so we can later proceed to the change with simple ifdefs without rewriting all the kernel code that would use it.

3) immediate_set(), _immediate_set() and immediate_set_early()? No thanks! AFAICT you really want an "init_immediate(var, val)". This means "you can patch all the references now, they're not executing". Later on we could possibly have a super-stop-machine version which ensures noone's preempted and handles the concurrent case. Maybe.

4) With an "init" interface not a "set" interface, you don't need locking. Simpler.

Hope that helps,
Rusty.

Code patching of _live_ SMP code is allowed. This is why I went through all this trouble on i386.

Quoted 2 lines

2) immediate_if() needs an implementation before you introduce it. Your assumption that it's always unlikely seems non-orthogonal.

I could remove the unlikely(), no problem with that. Your point about this is valid. However, I would like to leave the immediate_if() there because it may become very useful if someday gcc permits to extract the address of a branch instruction (and to generate assembly that would not be reached without doing code patching).

Quoted 2 lines · Mathieu Desnoyers

Code patching of _live_ SMP code is allowed. This is why I went through all this trouble on i386.

Oh, I was pretty sure it wasn't. OK.

So now why three versions of immediate_set()? And why are you using my lock for exclusion? Against what?

Why not just have one immediate_set() which iterates through and fixes up all the references?

It can use an internal lock if you want to avoid
concurrent immediate_set() calls.

I understand the need for a "module_immediate_fixup()" but that can also use your internal lock.

Quoted 9 lines

Quoted 3 lines

2) immediate_if() needs an implementation before you introduce it. Your assumption that it's always unlikely seems non-orthogonal.

I could remove the unlikely(), no problem with that. Your point about this is valid. However, I would like to leave the immediate_if() there because it may become very useful if someday gcc permits to extract the address of a branch instruction (and to generate assembly that would not be reached without doing code patching).

Why is it easier to patch the sites now than later? Currently it's just churn. You could go back and find them when this mythical patch gets merged into this mythical future gcc version. It could well need a completely different macro style, like "cond_imm(var, code)".

Rusty.

Quoted 9 lines · * Rusty Russell ([email protected]) wrote:

Quoted 2 lines · Mathieu Desnoyers

Code patching of _live_ SMP code is allowed. This is why I went through all this trouble on i386.

Oh, I was pretty sure it wasn't. OK.

So now why three versions of immediate_set()? And why are you using my lock for exclusion? Against what?

If we need to patch code at boot time, when interrupts are still disabled (it happens when we parse the kernel arguments for instance), we cannot afford to use IPIs to call sync_core() on each cpu, using breakpoints/notifier chains could be tricky (because we are very early at boot and alternatives or paravirt may not have been applied yet).

_immediate_set() has been introduced because of the way immediate values are used by markers: the linux kernel markers already hold the module mutex when they need to update the immediate values. Taking the mutex twice makes no sence, so _immediate_set() is used when the caller already holds the module mutex.

Why not just have one immediate_set() which iterates through and fixes

Quoted 1 line

up all the references?

(reasons explained above)

Quoted 3 lines

It can use an internal lock if you want to avoid
concurrent immediate_set() calls.

An internal lock won't protect against modules load/unload race. We have to iterate on the module list.

Quoted 4 lines

Why is it easier to patch the sites now than later? Currently it's just churn. You could go back and find them when this mythical patch gets merged into this mythical future gcc version. It could well need a completely different macro style, like "cond_imm(var, code)".

Maybe you're right. My though was that if we have a way to express a strictly boolean if() statement that can later be optimized further by gcc using a jump rather than a conditionnal branch and currently emulate it by using a load immediate/test/branch, we might want to do so right now so we don't have to do a second code transition from
if (immediate_read(&var)) to immediate_if (&var) later. But you might be right in that the form could potentially change anyway when the implementation would come, although I don't see how.

Quoted 11 lines · Mathieu Desnoyers

Quoted 9 lines · * Rusty Russell ([email protected]) wrote:

Quoted 2 lines · Mathieu Desnoyers

Code patching of _live_ SMP code is allowed. This is why I went through all this trouble on i386.

Oh, I was pretty sure it wasn't. OK.

So now why three versions of immediate_set()? And why are you using my lock for exclusion? Against what?

Quoted 5 lines

If we need to patch code at boot time, when interrupts are still disabled (it happens when we parse the kernel arguments for instance), we cannot afford to use IPIs to call sync_core() on each cpu, using breakpoints/notifier chains could be tricky (because we are very early at boot and alternatives or paravirt may not have been applied yet).

Hi Mathieu,

Sure, but why is that the caller's problem? immediate_set() isn't fastpath, so why not make it do an "if (early_boot)" internally?

Quoted 9 lines

_immediate_set() has been introduced because of the way immediate values are used by markers: the linux kernel markers already hold the module mutex when they need to update the immediate values. Taking the mutex twice makes no sence, so _immediate_set() is used when the caller already holds the module mutex.

Why not just have one immediate_set() which iterates through and fixes

Quoted 1 line

up all the references?

Quoted 8 lines

(reasons explained above)

Quoted 3 lines

It can use an internal lock if you want to avoid
concurrent immediate_set() calls.

An internal lock won't protect against modules load/unload race. We have to iterate on the module list.

Sure, but it seems like that's fairly easy to do within module.c:

/* This updates all the immediates even though only one might have
* changed. But it's so rare it's not worth optimizing. */ void module_update_immediates(void)
{
mutex_lock(&module_mutex);
list_for_each_entry(mod, &modules, list) update_immediates(mod->immediate, mod->num_immediate); mutex_unlock(&module_mutex);
}

Then during module load you do:

update_immediates(mod->immediate, mod->num_immediate);

Your immediate_update() just becomes:

update_immediates(__start___immediate,

		  __stop___immediate - __start___immediate);

module_update_immediates();

update_immediates() can grab the immediate_mutex if you want.

Quoted 5 lines

Quoted 4 lines

Why is it easier to patch the sites now than later? Currently it's just churn. You could go back and find them when this mythical patch gets merged into this mythical future gcc version. It could well need a completely different macro style, like "cond_imm(var, code)".

Quoted 8 lines

Maybe you're right. My though was that if we have a way to express a strictly boolean if() statement that can later be optimized further by gcc using a jump rather than a conditionnal branch and currently emulate it by using a load immediate/test/branch, we might want to do so right now so we don't have to do a second code transition from
if (immediate_read(&var)) to immediate_if (&var) later. But you might be right in that the form could potentially change anyway when the implementation would come, although I don't see how.

I was thinking that we might find useful specific cases before we get GCC support, which archs can override with tricky asm if they wish.

Cheers,
Rusty.

Quoted 2 lines · * Rusty Russell ([email protected]) wrote:

Sure, but why is that the caller's problem? immediate_set() isn't fastpath, so why not make it do an "if (early_boot)" internally?

I see two reasons:
1 - early_boot, or anything that looks like this, does not exist currently (and the following reason might show why).
2 - If we use this, we cannot declare the early code with __init, so it will have to stay there forever insteaf of being removable once boot is over.

Therefore, I think it's better to stick to an immediate_set_early version.

Quoted 3 lines

I was thinking that we might find useful specific cases before we get GCC support, which archs can override with tricky asm if they wish.

The first useful case is the Linux Kernel Markers, which really needs a completely boolean if: active or inactive. That would be a good test case to get gcc support.

Quoted 11 lines · Mathieu Desnoyers

Quoted 2 lines · * Rusty Russell ([email protected]) wrote:

Sure, but why is that the caller's problem? immediate_set() isn't fastpath, so why not make it do an "if (early_boot)" internally?

I see two reasons:
1 - early_boot, or anything that looks like this, does not exist currently (and the following reason might show why).
2 - If we use this, we cannot declare the early code with __init, so it will have to stay there forever insteaf of being removable once boot is over.

Quoted 2 lines

Therefore, I think it's better to stick to an immediate_set_early version.

My problem is that I don't know when to use immediate_set_early! You say "early boot": what does that mean? Is it arch-specific? And why is it my problem anyway?

Since arch_immediate_update_early() is a memcpy, I don't really buy the bloat argument (it has some silly-looking optimization, but that should be removed anyway). I'd really rather see arch_immediate_update() examine an internal flag and do the memcpy if it's too early for the full algorithm. That's the only place which really knows, after all. You may need some external hook to update that internal "too early" flag, of course.

Alternatively, if you called it "immediate_init" then the semantics change slightly, but are more obvious (ie. only use this when the value isn't being accessed yet). But it can't be __init then anyway.

On an unrelated note, did you consider simply IPI-ing and doing the substitution with all CPUs stopped? If you only updated the immediate references to this particular var, it should be fast enough not to upset the RT guys, even.

Quoted 7 lines

Quoted 3 lines

I was thinking that we might find useful specific cases before we get GCC support, which archs can override with tricky asm if they wish.

The first useful case is the Linux Kernel Markers, which really needs a completely boolean if: active or inactive. That would be a good test case to get gcc support.

Well, you can do that in asm without gcc support. It's a little nasty:
since gcc will know nothing about the function call, it can't have side effects which are visible in this function, and you'll have to save and restore *all* regs if you decide to do the function call. But it's possible (a 5-byte nop gets changed to a call, the call does the pushes and sets the args regs, calls the function, then pops everything and rets).

Cheers,
Rusty.

Quoted 4 lines · * Rusty Russell ([email protected]) wrote:

Alternatively, if you called it "immediate_init" then the semantics change slightly, but are more obvious (ie. only use this when the value isn't being accessed yet). But it can't be __init then anyway.

I think your idea is good. immediate_init() could be used to update the immediate values at boot time _and_ at module load time, and we could use an architecture specific arch_immediate_update_init() to support it.

As for "when" to use this, it should be used at boot time when interrupts are still disabled, still running in UP. It can also be used at module load time before any of the module code is executed, as long as the module code pages are writable (which they always are, for now..). Therefore, the flag seems inappropriate for module load arch_immediate_update_init. It cannot be put in __init section neither though if we use it like this.

Quoted 5 lines

On an unrelated note, did you consider simply IPI-ing and doing the substitution with all CPUs stopped? If you only updated the immediate references to this particular var, it should be fast enough not to upset the RT guys, even.

Yes, I thought about this, but since I use immediate values in the kernel markers, which can be put in exception handlers (including nmi, mce handler), which cannot be disabled without important side-effects, I don't think trying to stop the CPUs is a workable solution.

Quoted 7 lines

Well, you can do that in asm without gcc support. It's a little nasty:
since gcc will know nothing about the function call, it can't have side effects which are visible in this function, and you'll have to save and restore *all* regs if you decide to do the function call. But it's possible (a 5-byte nop gets changed to a call, the call does the pushes and sets the args regs, calls the function, then pops everything and rets).

GCC support is required if we want to embed inline functions inside unlikely branches depending on immediate values (no function call there). It also permits passing local variables as arguments to the function call (stack setup), which would be tricky, instrumentation site specific and non portable if done in assembly.

Quoted 9 lines · Mathieu Desnoyers

Quoted 4 lines · * Rusty Russell ([email protected]) wrote:

Alternatively, if you called it "immediate_init" then the semantics change slightly, but are more obvious (ie. only use this when the value isn't being accessed yet). But it can't be __init then anyway.

I think your idea is good. immediate_init() could be used to update the immediate values at boot time _and_ at module load time, and we could use an architecture specific arch_immediate_update_init() to support it.

Right.

Quoted 7 lines

As for "when" to use this, it should be used at boot time when interrupts are still disabled, still running in UP. It can also be used at module load time before any of the module code is executed, as long as the module code pages are writable (which they always are, for now..). Therefore, the flag seems inappropriate for module load arch_immediate_update_init. It cannot be put in __init section neither though if we use it like this.

I think from a user's POV it would be nice to have a 1:1 mapping with normal initialization semantics (ie. it will work as long as you don't access this value until initialized). And I think this would be the case. eg:

        int foo_func(void)
        {
        	if (immediate_read(&some_immediate))
        		return 0;
        	...
        }
        
        int some_init(void)
        {
        	immediate_init(some_immediate, 0);
        	register_foo(foo_func);
        	...
        }

Quoted 7 lines

Quoted 6 lines

On an unrelated note, did you consider simply IPI-ing and doing the substitution with all CPUs stopped? If you only updated the immediate references to this particular var, it should be fast enough not to upset the RT guys, even.

Quoted 4 lines

Yes, I thought about this, but since I use immediate values in the kernel markers, which can be put in exception handlers (including nmi, mce handler), which cannot be disabled without important side-effects, I don't think trying to stop the CPUs is a workable solution.

OK, but can you justify the use of immediates within the nmi or mce handlers? They don't strike me as useful candidates for optimization.

Quoted 8 lines

Quoted 7 lines

Well, you can do that in asm without gcc support. It's a little nasty:
since gcc will know nothing about the function call, it can't have side effects which are visible in this function, and you'll have to save and restore *all* regs if you decide to do the function call. But it's possible (a 5-byte nop gets changed to a call, the call does the pushes and sets the args regs, calls the function, then pops everything and rets).

Quoted 5 lines

GCC support is required if we want to embed inline functions inside unlikely branches depending on immediate values (no function call there). It also permits passing local variables as arguments to the function call (stack setup), which would be tricky, instrumentation site specific and non portable if done in assembly.

Well if this is the slow path, you don't want inline anyway. But it would be horribly, horribly arch-specific, yes.

Rusty.

Quoted 26 lines · * Rusty Russell ([email protected]) wrote:

Quoted 9 lines · Mathieu Desnoyers

Quoted 4 lines · * Rusty Russell ([email protected]) wrote:

Alternatively, if you called it "immediate_init" then the semantics change slightly, but are more obvious (ie. only use this when the value isn't being accessed yet). But it can't be __init then anyway.

I think your idea is good. immediate_init() could be used to update the immediate values at boot time _and_ at module load time, and we could use an architecture specific arch_immediate_update_init() to support it.

Right.

Quoted 7 lines

As for "when" to use this, it should be used at boot time when interrupts are still disabled, still running in UP. It can also be used at module load time before any of the module code is executed, as long as the module code pages are writable (which they always are, for now..). Therefore, the flag seems inappropriate for module load arch_immediate_update_init. It cannot be put in __init section neither though if we use it like this.

I think from a user's POV it would be nice to have a 1:1 mapping with normal initialization semantics (ie. it will work as long as you don't access this value until initialized). And I think this would be the case. eg:

Quoted 14 lines

        int foo_func(void)
        {
        	if (immediate_read(&some_immediate))
        		return 0;
        	...
        }
        
        int some_init(void)
        {
        	immediate_init(some_immediate, 0);
        	register_foo(foo_func);
        	...
        }

There are other considerations that differs between the boot-time case and the general "init" case: the write-protection flag must be cleared-saved/restored when the kernel is running to patch read-only text, but we don't want to modify cr0 at early boot on i386 because paravirt is not executed yet (at boot time, pages are not write-protected yet).

And I am not sure that it buys us anything to create an immediate_init() when we can do exactly the same job with immediate_set. Yes, it might be a bit slower, but we are not on a fast path.

Quoted 2 lines

OK, but can you justify the use of immediates within the nmi or mce handlers? They don't strike me as useful candidates for optimization.

Yes, immediate values are used by the Linux Kernel Markers, which instrument many code paths, including functions called from nmi and mce contexts (including printk).

Quoted 49 lines · Mathieu Desnoyers

Quoted 26 lines · * Rusty Russell ([email protected]) wrote:

Quoted 9 lines · Mathieu Desnoyers

Quoted 4 lines · * Rusty Russell ([email protected]) wrote:

Alternatively, if you called it "immediate_init" then the semantics change slightly, but are more obvious (ie. only use this when the value isn't being accessed yet). But it can't be __init then anyway.

I think your idea is good. immediate_init() could be used to update the immediate values at boot time _and_ at module load time, and we could use an architecture specific arch_immediate_update_init() to support it.

Right.

Quoted 7 lines

As for "when" to use this, it should be used at boot time when interrupts are still disabled, still running in UP. It can also be used at module load time before any of the module code is executed, as long as the module code pages are writable (which they always are, for now..). Therefore, the flag seems inappropriate for module load arch_immediate_update_init. It cannot be put in __init section neither though if we use it like this.

I think from a user's POV it would be nice to have a 1:1 mapping with normal initialization semantics (ie. it will work as long as you don't access this value until initialized). And I think this would be the case. eg:

Quoted 14 lines

        int foo_func(void)
        {
        	if (immediate_read(&some_immediate))
        		return 0;
        	...
        }
        
        int some_init(void)
        {
        	immediate_init(some_immediate, 0);
        	register_foo(foo_func);
        	...
        }

There are other considerations that differs between the boot-time case and the general "init" case: the write-protection flag must be cleared-saved/restored when the kernel is running to patch read-only text, but we don't want to modify cr0 at early boot on i386 because paravirt is not executed yet (at boot time, pages are not write-protected yet).

Quoted 3 lines

And I am not sure that it buys us anything to create an immediate_init() when we can do exactly the same job with immediate_set. Yes, it might be a bit slower, but we are not on a fast path.

Good points. Well I'd say hiding it all behind a friendly "immediate_set()" interface is the best option then.

Quoted 6 lines

Quoted 2 lines

OK, but can you justify the use of immediates within the nmi or mce handlers? They don't strike me as useful candidates for optimization.

Yes, immediate values are used by the Linux Kernel Markers, which instrument many code paths, including functions called from nmi and mce contexts (including printk).

Is this really worth worrying about? Isn't there already a problem with printk() in nmi?

Cheers,
Rusty.

Quoted 2 lines

Good points. Well I'd say hiding it all behind a friendly "immediate_set()" interface is the best option then.

Then we can't benefit of the __init section to have the code removed after boot. I don't see the point in doing so.

Quoted 2 lines

Is this really worth worrying about? Isn't there already a problem with printk() in nmi?

printk() is just an example taken from my current instrumentation. Let's say I plan to instrument kmalloc() (which will happen someday): it's used in every context, including NMI.

init/Kconfig config CC_OPTIMIZE_FOR_SIZE · line 411

Quoted 2 lines

init/Kconfig +2 -0

+config IMMEDIATE
+	default y if !DISABLE_IMMEDIATE

It's still unclear to me why DISABLE_IMMEDIATE is needed. It would be better to make it just the default.

Quoted 8 lines

init/Kconfig +8 -0

+config DISABLE_IMMEDIATE
+	default y if EMBEDDED
+	bool "Disable immediate values" if EMBEDDED
+	depends on X86_32 || PPC || PPC64
+	help
+	  Disable code patching based immediate values for embedded systems. It
+	  consumes slightly more memory and requires to modify the instruction
+	  stream each time a variable is updated. 

init/Kconfig config PROC_KPAGEMAP · line 667

Quoted 2 lines

Should really be disabled for

init/Kconfig +1 -0

+	  embedded systems with read-only text.

There are no such on x86 at least And for other architectures like PPC it would be better to have a high level CONFIG_READ_ONLY_TEXT then that does the necessary changes internally. But I'm not sure it's even supporting r/o text.

-Andi

Quoted 6 lines · * Andi Kleen ([email protected]) wrote:

init/Kconfig config CC_OPTIMIZE_FOR_SIZE · line 411

Quoted 2 lines

init/Kconfig +2 -0

+config IMMEDIATE
+	default y if !DISABLE_IMMEDIATE

It's still unclear to me why DISABLE_IMMEDIATE is needed. It would be better to make it just the default.

It is actually the default on any non embedded configuration. Do you think we should make it default to on on embedded configs too ?

The idea here is to give embedded system developers incentives to create an optimized immediate value header for their architecture. I

fear that if it is not trivial to disable when they need to use ROM to put the kernel code (as kprobes is, meaning, with a single config option), they will refuse to event think about including an optimized immediate value header for their architecture.

And yes, having a CONFIG_READ_ONLY_TEXT makes sense, but it implies menu dependencies with not only immediate values but also kprobes, paravirt, alternatives, (am I missing others ?)

As long as we find a way for people to disable _all_ code patching in their kernel, I'm happy with that. But since every existing code patching mechanism can currently be disabled one by one, it makes sense to do the same for the immediate values. Having a global CONFIG_READ_ONLY_TEXT should IMHO come in a separate effort.

Quoted 10 lines · Mathieu Desnoyers

Quoted 6 lines · * Andi Kleen ([email protected]) wrote:

init/Kconfig config CC_OPTIMIZE_FOR_SIZE · line 411

Quoted 2 lines

init/Kconfig +2 -0

+config IMMEDIATE
+	default y if !DISABLE_IMMEDIATE

It's still unclear to me why DISABLE_IMMEDIATE is needed. It would be better to make it just the default.

It is actually the default on any non embedded configuration. Do you think we should make it default to on on embedded configs too ?

I would prefer to not have any config options at all and let the non converted architectures always use a asm-generic fallback.

Quoted 2 lines

The idea here is to give embedded system developers incentives to create an optimized immediate value header for their architecture. I

Sounds like a quite bogus way to do this.

Quoted 4 lines

fear that if it is not trivial to disable when they need to use ROM to put the kernel code (as kprobes is, meaning, with a single config option), they will refuse to event think about including an optimized immediate value header for their architecture.

#ifdef CONFIG_ARCH_SPECIFIC_READONLY
#include <asm-generic/generic-immediate.h>
#else
/* optimized implementation */
#endif

That's trivial.

Quoted 3 lines

And yes, having a CONFIG_READ_ONLY_TEXT makes sense, but it implies menu dependencies with not only immediate values but also kprobes, paravirt, alternatives, (am I missing others ?)

paravirt and alternatives are x86 only.

I don't think CONFIG_READ_ONLY_TEXT on x86 makes sense.

On other architectures they have to deal with kprobes, but they presumably do this already. Not really your problem I suspect.

Quoted 5 lines

As long as we find a way for people to disable _all_ code patching in their kernel, I'm happy with that. But since every existing code patching mechanism can currently be disabled one by one, it makes sense to do the same for the immediate values. Having a global CONFIG_READ_ONLY_TEXT should IMHO come in a separate effort.

You're clearly deep into overdesign territory here.

-Andi

Yeah, but then you would make immediate.o an obj-y (both for kernel/Makefile and arch/*/kernel/Makefile) and it would be built even on systems that would be configured not to use immediate values.

Since the breakpoint handler is useful both to kprobes and immediate values, it makes sense to make the required restore_interrupt() available through asm-i386/kdebug.h.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Christoph Hellwig <[email protected]>

CC [email protected]

CC [email protected]

CC [email protected]

CC [email protected]

---
 include/asm-i386/kdebug.h  |   12 ++++++++++++
 include/asm-i386/kprobes.h |    9 ---------
 2 files changed, 12 insertions(+), 9 deletions(-)

Index: linux-2.6-lttng/include/asm-i386/kdebug.h
===================================================================

include/asm-i386/kdebug.h +12 -0

@@ -6,6 +6,9 @@
  * from x86_64 architecture.
  */
 
+#include <asm/ptrace.h>
+#include <asm/system.h>
+
 struct pt_regs;
 
 /* Grossly misnamed. */

@@ -25,4 +28,13 @@ enum die_val {
 	DIE_PAGE_FAULT_NO_CONTEXT,
 };
 
+/* trap3/1 are intr gates for kprobes.  So, restore the status of IF,
+ * if necessary, before executing the original int3/1 (trap) handler.
+ */
+static inline void restore_interrupts(struct pt_regs *regs)
+{
+	if (regs->eflags & IF_MASK)
+		local_irq_enable();
+}
+
 #endif

Index: linux-2.6-lttng/include/asm-i386/kprobes.h
===================================================================

include/asm-i386/kprobes.h +0 -9

@@ -79,15 +79,6 @@ struct kprobe_ctlblk {
 	struct prev_kprobe prev_kprobe;
 };
 
-/* trap3/1 are intr gates for kprobes.  So, restore the status of IF,
- * if necessary, before executing the original int3/1 (trap) handler.
- */
-static inline void restore_interrupts(struct pt_regs *regs)
-{
-	if (regs->eflags & IF_MASK)
-		local_irq_enable();
-}
-
 extern int kprobe_exceptions_notify(struct notifier_block *self,
 				    unsigned long val, void *data);
 extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr);

Signature · 9 lines

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
-
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/

 Documentation/immediate.txt |  232 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 232 insertions(+)

Index: linux-2.6-lttng/Documentation/immediate.txt
===================================================================

Documentation/immediate.txt new file +4 -0

+Use immediate_set(&this_immediate) to set the immediate value.
+
+Use immediate_read(&this_immediate) to read the immediate value.
+

Documentation/immediate.txt new file +23 -0

+
+If you need to declare your own immediate types (for instance, a pointer to
+struct task_struct), use:
+
+DEFINE_IMMEDIATE_TYPE(struct task_struct*, immediate_task_struct_ptr_t);
+
+and declare your variable with:
+immediate_task_struct_ptr_t myptr;
+
+You can choose to set an initial static value to the immediate by using, for
+instance:
+
+immediate_task_struct_ptr_t myptr = IMMEDIATE_INIT(10);
+
+
+* Optimization for a given architecture
+
+One can implement optimized immediate values for a given architecture by
+replacing asm-$ARCH/immediate.h.
+
+* Performance improvement
+
+* Memory hit for a data-based branch

Documentation/immediate.txt new file +69 -0

+
+So:
+Base case:                      77.50 cycles
+instruction stream based test:  +7.8394 cycles
+L1 cache hit based test:        +12.1369 cycles
+Memory load based test:         +559.1066 cycles
+
+So let's say we have a ping flood coming at
+(14014 packets transmitted, 14014 received, 0% packet loss, time 1826ms)
+7674 packets per second. If we put 2 markers for irq entry/exit, it
+brings us to 15348 markers sites executed per second.
+
+(15348 exec/s) * (559 cycles/exec) / (3G cycles/s) = 0.0029
+We therefore have a 0.29% slowdown just on this case.
+
+Compared to this, the instruction stream based test will cause a
+slowdown of:
+
+(15348 exec/s) * (7.84 cycles/exec) / (3G cycles/s) = 0.00004
+For a 0.004% slowdown.
+
+If we plan to use this for memory allocation, spinlock, and all sort of
+very high event rate tracing, we can assume it will execute 10 to 100
+times more sites per second, which brings us to 0.4% slowdown with the
+instruction stream based test compared to 29% slowdown with the memory
+load based test on a system with high memory pressure.
+
+
+
+* Markers impact under heavy memory load
+
+Running a kernel with my LTTng instrumentation set, in a test that
+generates memory pressure (from userspace) by trashing L1 and L2 caches
+between calls to getppid() (note: syscall_trace is active and calls
+a marker upon syscall entry and syscall exit; markers are disarmed).
+This test is done in user-space, so there are some delays due to IRQs
+coming and to the scheduler. (UP 2.6.22-rc6-mm1 kernel, task with -20
+nice level)
+
+My first set of results : Linear cache trashing, turned out not to be
+very interesting, because it seems like the linearity of the memset on a
+full array is somehow detected and it does not "really" trash the
+caches.
+
+Now the most interesting result : Random walk L1 and L2 trashing
+surrounding a getppid() call.
+
+- Markers compiled out (but syscall_trace execution forced)
+number of tests : 10000
+No memory pressure
+Reading timestamps takes 108.033 cycles
+getppid : 1681.4 cycles
+With memory pressure
+Reading timestamps takes 102.938 cycles
+getppid : 15691.6 cycles
+
+
+- With the immediate values based markers:
+number of tests : 10000
+No memory pressure
+Reading timestamps takes 108.006 cycles
+getppid : 1681.84 cycles
+With memory pressure
+Reading timestamps takes 100.291 cycles
+getppid : 11793 cycles
+
+
+- With global variables based markers:
+number of tests : 10000

Documentation/immediate.txt new file +5 -0

+markers are compiled in or out. It seems that it aligns the function's
+data better to compile-in the markers in this case.
+
+But since the interesting comparison is between the immediate values and
+global variables based markers, and because they share the same memory

Documentation/immediate.txt new file +13 -0

+(sorry, my system is not setup to execute syscall_trace this time, but it will
+make the point anyway).
+
+No memory pressure
+Reading timestamps:     150.92 cycles,     std dev.    1.01 cycles
+getppid:               1462.09 cycles,     std dev.   18.87 cycles
+
+With memory pressure
+Reading timestamps:     578.22 cycles,     std dev.  269.51 cycles
+getppid:              17113.33 cycles,     std dev. 1655.92 cycles
+
+
+Now for memory read timing: (10 runs, branches per test: 100000)

Documentation/immediate.txt new file +18 -0

+Getppid without memory pressure:
+1462.09±18.87 - 150.92±1.01 = 1311.17±18.90 cycles
+
+Getppid with memory pressure:
+17113.33±1655.92 - 578.22±269.51 = 16535.11±1677.71 cycles
+
+Therefore, if we add 2 markers not based on immediate values to the getppid
+code, which would add 2 memory reads, we would add
+2 * 555.93±12.74 = 1111.86±25.48 cycles
+
+Therefore,
+
+1111.86±25.48 / 16535.11±1677.71 = 0.0672
+ relative error: sqrt(((25.48/1111.86)^2)+((1677.71/16535.11)^2))
+                     = 0.1040
+ absolute error: 0.1040 * 0.0672 = 0.0070
+
+Therefore: 0.0672±0.0070 * 100% = 6.72±0.70 %

What character is this that I cannot read (not displayed properly by my email client maybe)? <something> after 644.09 and before the +- symbol, repeated just before all of the +- symbols.

Quoted 19 lines

Documentation/immediate.txt new file +18 -0

+Getppid without memory pressure:
+1462.09±18.87 - 150.92±1.01 = 1311.17±18.90 cycles
+
+Getppid with memory pressure:
+17113.33±1655.92 - 578.22±269.51 = 16535.11±1677.71 cycles
+
+Therefore, if we add 2 markers not based on immediate values to the getppid
+code, which would add 2 memory reads, we would add
+2 * 555.93±12.74 = 1111.86±25.48 cycles
+
+Therefore,
+
+1111.86±25.48 / 16535.11±1677.71 = 0.0672
+ relative error: sqrt(((25.48/1111.86)^2)+((1677.71/16535.11)^2))
+                     = 0.1040
+ absolute error: 0.1040 * 0.0672 = 0.0070
+
+Therefore: 0.0672±0.0070 * 100% = 6.72±0.70 %

Documentation/immediate.txt line 232

Quoted 10 lines

Documentation/immediate.txt new file +10 -0

+
+We can therefore affirm that adding 2 markers to getppid, on a system with high
+memory pressure, would have a performance hit of at least 6.0% on the system
+call time, all within the uncertainty limits of these tests. The same applies to
+other kernel code paths. The smaller those code paths are, the highest the
+impact ratio will be.
+
+Therefore, not only is it interesting to use the immediate values to dynamically
+activate dormant code such as the markers, but I think it should also be
+considered as a replacement for many of the "read mostly" static variables.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

* Randy Dunlap ([email protected]) wrote:
[fixing the rest of the comments in the patch]

Quoted 4 lines

What character is this that I cannot read (not displayed properly by my email client maybe)? <something> after 644.09 and before the +- symbol, repeated just before all of the +- symbols.

This file seems to be UTF-8 and the mail has been send by quilt. Is it ok or should I change this to +/- just to make sure ?

Quoted 28 lines

Quoted 28 lines

Documentation/immediate.txt new file +28 -0

+Getppid without memory pressure:
+1462.09±18.87 - 150.92±1.01 = 1311.17±18.90 cycles
+
+Getppid with memory pressure:
+17113.33±1655.92 - 578.22±269.51 = 16535.11±1677.71 cycles
+
+Therefore, if we add 2 markers not based on immediate values to the getppid
+code, which would add 2 memory reads, we would add
+2 * 555.93±12.74 = 1111.86±25.48 cycles
+
+Therefore,
+
+1111.86±25.48 / 16535.11±1677.71 = 0.0672
+ relative error: sqrt(((25.48/1111.86)^2)+((1677.71/16535.11)^2))
+                     = 0.1040
+ absolute error: 0.1040 * 0.0672 = 0.0070
+
+Therefore: 0.0672±0.0070 * 100% = 6.72±0.70 %
+
+We can therefore affirm that adding 2 markers to getppid, on a system with high
+memory pressure, would have a performance hit of at least 6.0% on the system
+call time, all within the uncertainty limits of these tests. The same applies to
+other kernel code paths. The smaller those code paths are, the highest the
+impact ratio will be.
+
+Therefore, not only is it interesting to use the immediate values to dynamically
+activate dormant code such as the markers, but I think it should also be
+considered as a replacement for many of the "read mostly" static variables.