public inbox for systemtap@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Linux Markers 0.4 (+dynamic probe loader) for 2.6.17
@ 2006-09-20 23:45 Mathieu Desnoyers
       [not found] ` <4511D92A.3090204@goop.org>
  2006-09-21  4:54 ` Ingo Molnar
  0 siblings, 2 replies; 5+ messages in thread
From: Mathieu Desnoyers @ 2006-09-20 23:45 UTC (permalink / raw)
  To: Martin Bligh, Frank Ch. Eigler, Masami Hiramatsu, prasanna,
	Andrew Morton, Ingo Molnar, Mathieu Desnoyers, Paul Mundt,
	linux-kernel, Jes Sorensen, Tom Zanussi, Richard J Moore,
	Michel Dagenais, Christoph Hellwig, Greg Kroah-Hartman,
	Thomas Gleixner, William Cohen, ltt-dev, systemtap, Alan Cox

Hi,

I thought about it a little more, and here is still another enhanced version of
the Linux Markers. It also allows the possibility to insert an inlined function
inside the MARKERS surrounded by a uncontidional jump.

I just made what will be the marker loader module : it works with the
"JUMP over CALL" flavor of the marker to provide dynamic loading and unloading
of a probe (using insmod/rmmod). I included the code at the bottom of this
email, with the Makefile and a small module example. As the marker loader uses
special flags, it has to be built out of tree. 
Hint : use "make TARGET=subsys_mark1" to build marker-loader.ko.

Comments/ideas are welcome.

Mathieu

---BEGIN---

--- a/arch/i386/Kconfig
+++ b/arch/i386/Kconfig
@@ -1082,6 +1082,8 @@ config KPROBES
 	  for kernel debugging, non-intrusive instrumentation and testing.
 	  If in doubt, say "N".
 
+source "kernel/Kconfig.marker"
+
 source "ltt/Kconfig"
 
 endmenu
--- /dev/null
+++ b/include/linux/marker.h
@@ -0,0 +1,98 @@
+/*****************************************************************************
+ * marker.h
+ *
+ * Code markup for dynamic and static tracing.
+ *
+ * Example :
+ *
+ * MARK(subsystem_event, "%d %s", someint, somestring);
+ * Where :
+ * - Subsystem is the name of your subsystem.
+ * - event is the name of the event to mark.
+ * - "%d %s" is the formatted string for printk.
+ * - someint is an integer.
+ * - somestring is a char *.
+ * - subsystem_event must be unique thorough the kernel!
+ *
+ * Update : Dynamically overridable function call based on marker mechanism
+ *          from Frank Ch. Eigler <fche@redhat.com>.
+ * Notes :
+ * To remove a probe :
+ * * set the function pointer back to __mark_empty_function
+ * * call synchronize_sched() to wait for all the functions to return
+ * * unload the module containing the function
+ *
+ * (C) Copyright 2006 Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
+ *
+ * This file is released under the GPLv2.
+ * See the file COPYING for more details.
+ */
+
+#ifdef CONFIG_MARK_SYMBOL
+#define MARK_SYM(name)	__asm__ ("__mark_kprobe_" #name ":")
+#else 
+#define MARK_SYM(name)
+#endif
+
+
+#ifdef CONFIG_MARK_JUMP_CALL
+#define MARK_JUMP_CALL_PROTOTYPE(name) \
+	static void \
+		(*__mark_##name##_call)(const char *fmt, ...) \
+		asm ("__mark_"#name"_call") = \
+			__mark_empty_function
+#define MARK_JUMP_CALL(name, format, args...) \
+	do { \
+		preempt_disable(); \
+		(void) (__mark_##name##_call(format, ## args)); \
+		preempt_enable_no_resched(); \
+	} while(0)
+#else
+#define MARK_JUMP_CALL_PROTOTYPE(name)
+#define MARK_JUMP_CALL(name, format, args...)
+#endif
+
+#ifdef CONFIG_MARK_JUMP_INLINE
+#define MARK_JUMP_INLINE(name, format, args...) \
+		(void) (__mark_##name##_inline(format, ## args))
+#else
+#define MARK_JUMP_INLINE(name, format, args...)
+#endif
+
+#define MARK_JUMP(name, format, args...) \
+	do { \
+		__label__ over_label, call_label, inline_label; \
+		volatile static void *__mark_##name##_jump_over \
+				asm ("__mark_"#name"_jump_over") = \
+					&&over_label; \
+		volatile static void *__mark_##name##_jump_call \
+				asm ("__mark_"#name"_jump_call") \
+				__attribute__((unused)) =  \
+					&&call_label; \
+		volatile static void *__mark_##name##_jump_inline \
+				asm ("__mark_"#name"_jump_inline") \
+				__attribute__((unused)) =  \
+					&&inline_label; \
+		MARK_JUMP_CALL_PROTOTYPE(name); \
+		goto *__mark_##name##_jump_over; \
+call_label: \
+		MARK_JUMP_CALL(name, format, ## args); \
+		goto over_label; \
+inline_label: \
+		MARK_JUMP_INLINE(name, format, ## args); \
+over_label: \
+		do {} while(0); \
+	} while(0)
+
+#define MARK(name, format, args...) \
+	do { \
+		__mark_check_format(format, ## args); \
+		MARK_SYM(name); \
+		MARK_JUMP(name, format, ## args); \
+	} while(0)
+
+static inline __attribute__ ((format (printf, 1, 2)))
+void __mark_check_format(const char *fmt, ...)
+{ }
+
+extern void __mark_empty_function(const char *fmt, ...);
--- a/init/main.c
+++ b/init/main.c
@@ -737,3 +737,10 @@ static int init(void * unused)
 
 	panic("No init found.  Try passing init= option to kernel.");
 }
+
+#ifdef CONFIG_MARK_JUMP
+void __mark_empty_function(const char *fmt, ...)
+{
+}
+EXPORT_SYMBOL(__mark_empty_function);
+#endif
--- /dev/null
+++ b/kernel/Kconfig.marker
@@ -0,0 +1,24 @@
+# Code markers configuration
+
+menu "Marker configuration"
+
+
+config MARK_SYMBOL
+	bool "Replace markers with symbols"
+	default n
+	help
+	  Put symbols in place of markers, useful for kprobe.
+
+config MARK_JUMP_CALL
+	bool "Replace markers with a jump over an inactive function call"
+	default n
+	help
+	  Put a jump over a call in place of markers.
+
+config MARK_JUMP_INLINE
+	bool "Replace markers with a jump over an inline function"
+	default n
+	help
+	  Put a jump over an inline function.
+
+endmenu

---END---



---MARKER LOADER---
---BEGIN---

/* marker-loader.c
 *
 * Marker Loader
 *
 * Loads a function at a marker call site.
 *
 * (C) Copyright 2006 Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
 *
 * This file is released under the GPLv2.
 * See the file COPYING for more details.
 */

#include <linux/marker.h>
#include <linux/module.h>
#include <linux/kallsyms.h>

/* function to install */
void do_mark1(const char *format, int value)
{
	printk("value is %d\n", value);
}

static void *saved_over;

static void **target_mark_call;
static void **target_mark_jump_over;
static void **target_mark_jump_call;
static void **target_mark_jump_inline;

void show_symbol_pointers(void)
{
	printk("Marker loader : Loading symbols...\n");
	printk("  %s %p %p\n", __stringify(CALL), target_mark_call,
		target_mark_call?*target_mark_call:0x0);
	printk("  %s %p %p\n", __stringify(JUMP_OVER), target_mark_jump_over,
		target_mark_jump_over?*target_mark_jump_over:0x0);
	printk("  %s %p %p\n", __stringify(JUMP_CALL), target_mark_jump_call,
		target_mark_jump_call?*target_mark_jump_call:0x0);
	printk("  %s %p %p\n", __stringify(JUMP_INLINE), target_mark_jump_inline,
		target_mark_jump_inline?*target_mark_jump_inline:0x0);
}

int mark_install_hook(void)
{
	target_mark_call = (void**)kallsyms_lookup_name(__stringify(CALL));
	target_mark_jump_over = (void**)kallsyms_lookup_name(__stringify(JUMP_OVER));
	target_mark_jump_call = (void**)kallsyms_lookup_name(__stringify(JUMP_CALL));
	target_mark_jump_inline = (void**)kallsyms_lookup_name(__stringify(JUMP_INLINE));

	show_symbol_pointers();
	
	if(!(target_mark_call && target_mark_jump_over && target_mark_jump_call && 
		target_mark_jump_inline)) {
		printk("Target symbols missing in kallsyms.\n");
		return -EPERM;
	}
	
	printk("Installing hook\n");
	*target_mark_call = (void*)do_mark1;
	saved_over = *target_mark_jump_over;
	*target_mark_jump_over = *target_mark_jump_call;

	return 0;
}

int init_module(void)
{
	return mark_install_hook();
}

void cleanup_module(void)
{
	printk("Removing hook\n");
	*target_mark_jump_over = saved_over;
	*target_mark_call = __mark_empty_function;

	/* Wait for instrumentation functions to return before quitting */
	synchronize_sched();
}

MODULE_LICENSE("GPL");
MODULE_AUTHOR("Mathieu Desnoyers");
MODULE_DESCRIPTION("Marker Loader");

---END---


-- MARKER LOADER Makefile ---
---BEGIN---

export MARKER_CFLAGS

MARKER_CFLAGS	:= -DCALL="__mark_$(TARGET)_call" -DJUMP_OVER="__mark_$(TARGET)_jump_over"
MARKER_CFLAGS	+= -DJUMP_CALL="__mark_$(TARGET)_jump_call" -DJUMP_INLINE="__mark_$(TARGET)_jump_inline"

EXTRA_CFLAGS	+= $(MARKER_CFLAGS)

ifneq ($(KERNELRELEASE),)

obj-m += marker-loader.o

else
	KERNELDIR ?= /lib/modules/$(shell uname -r)/build
	PWD := $(shell pwd)
	KERNELRELEASE = $(shell cat $(KERNELDIR)/$(KBUILD_OUTPUT)/include/linux/version.h | sed -n 's/.*UTS_RELEASE.*\"\(.*\)\".*/\1/p')
ifneq ($(INSTALL_MOD_PATH),)
	DEPMOD_OPT := -b $(INSTALL_MOD_PATH)
endif

default:
	$(MAKE) -C $(KERNELDIR) M=$(PWD) modules

modules_install:
	$(MAKE) -C $(KERNELDIR) M=$(PWD) modules_install
	if [ -f $(KERNELDIR)/$(KBUILD_OUTPUT)/System.map ] ; then /sbin/depmod -ae -F $(KERNELDIR)/$(KBUILD_OUTPUT)/System.map $(DEPMOD_OPT) $(KERNELRELEASE) ; fi


clean:
	$(MAKE) -C $(KERNELDIR) M=$(PWD) clean
endif

---END---

--Marker test program--
---BEGIN---
/* test-mark.c
 *
 */

#include <linux/marker.h>
#include <linux/module.h>
#include <linux/proc_fs.h>
#include <linux/sched.h>

int x=7;

struct proc_dir_entry *pentry = NULL;

static int my_open(struct inode *inode, struct file *file)
{
	MARK(subsys_mark1, "%d", 1);
	MARK(subsys_mark2, "%d %s", 2, "blah2");
	MARK(subsys_mark3, "%d %s", x, "blah3");

	return -EPERM;
}


static struct file_operations my_operations = {
        .open = my_open,
};

int init_module(void)
{
       pentry = create_proc_entry("testmark", 0444, NULL);
        if(pentry)
                pentry->proc_fops = &my_operations;
	return 0;
}

void cleanup_module(void)
{
	remove_proc_entry("testmark", NULL);
}

MODULE_LICENSE("GPL");
MODULE_AUTHOR("Mathieu Desnoyers");
MODULE_DESCRIPTION("Marker Test");

---END---





OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Linux Markers 0.4 (+dynamic probe loader) for 2.6.17
       [not found] ` <4511D92A.3090204@goop.org>
@ 2006-09-21  2:04   ` Mathieu Desnoyers
       [not found]     ` <45123C7D.3080309@goop.org>
  0 siblings, 1 reply; 5+ messages in thread
From: Mathieu Desnoyers @ 2006-09-21  2:04 UTC (permalink / raw)
  To: Jeremy Fitzhardinge
  Cc: Martin Bligh, Frank Ch. Eigler, Masami Hiramatsu, prasanna,
	Andrew Morton, Ingo Molnar, Paul Mundt, linux-kernel,
	Jes Sorensen, Tom Zanussi, Richard J Moore, Michel Dagenais,
	Christoph Hellwig, Greg Kroah-Hartman, Thomas Gleixner,
	William Cohen, ltt-dev, systemtap, Alan Cox

Hi Jeremy,

Thanks for the insightful comments, see below :

* Jeremy Fitzhardinge (jeremy@goop.org) wrote:
> Mathieu Desnoyers wrote:
> >+#define MARK_SYM(name)	__asm__ ("__mark_kprobe_" #name ":")
> >  
> 
> This will need to be "asm volatile()" so that gcc doesn't get rid of it 
> altogether.  Also, there's nothing to make gcc keep this in any 
> particular place in the instruction stream, since it doesn't have any 
> data dependencies on anything else.  A "memory" clobber might help, but 
> ideally it would have some explicit data dependency on an important 
> value at that point.
> 

Yup, good catch. I have not seen gcc removing this asm in my objdump however, by
I guess we cannot be sure. This MARK_SYM() is only useful for kprobe
insertion : I don't use it myself for the jump markup stuff. I don't know how
relevant it is for kprobes users for the symbol to be at a specific location,
as they don't know themself what data they are interested in and they simply
don't want to modify the instruction stream. I fact, if the asm volatile
modifies the instruction stream, it would be an unwanted side-effect :(

> >+#else 
> >+#define MARK_SYM(name)
> >+#endif
> >+
> >+
> >+#ifdef CONFIG_MARK_JUMP_CALL
> >+#define MARK_JUMP_CALL_PROTOTYPE(name) \
> >+	static void \
> >+		(*__mark_##name##_call)(const char *fmt, ...) \
> >+		asm ("__mark_"#name"_call") = \
> >+			__mark_empty_function
> >+#define MARK_JUMP_CALL(name, format, args...) \
> >+	do { \
> >+		preempt_disable(); \
> >+		(void) (__mark_##name##_call(format, ## args)); \
> >  
> 
> (*foo)(args) is the preferred style for calling function pointers, since 
> it makes the pointerness explicit.  Though in this case there's enough 
> other complexity that the function call syntax is pretty much irrelevant 
> here.
> 

Ok, will fix


> >+		preempt_enable_no_resched(); \
> >+	} while(0)
> >+#else
> >+#define MARK_JUMP_CALL_PROTOTYPE(name)
> >+#define MARK_JUMP_CALL(name, format, args...)
> >+#endif
> >+
> >+#ifdef CONFIG_MARK_JUMP_INLINE
> >+#define MARK_JUMP_INLINE(name, format, args...) \
> >+		(void) (__mark_##name##_inline(format, ## args))
> >+#else
> >+#define MARK_JUMP_INLINE(name, format, args...)
> >+#endif
> >+
> >+#define MARK_JUMP(name, format, args...) \
> >+	do { \
> >+		__label__ over_label, call_label, inline_label; \
> >+		volatile static void *__mark_##name##_jump_over \
> >+				asm ("__mark_"#name"_jump_over") = \
> >+					&&over_label; \
> >+		volatile static void *__mark_##name##_jump_call \
> >+				asm ("__mark_"#name"_jump_call") \
> >+				__attribute__((unused)) =  \
> >+					&&call_label; \
> >+		volatile static void *__mark_##name##_jump_inline \
> >+				asm ("__mark_"#name"_jump_inline") \
> >+				__attribute__((unused)) =  \
> >+					&&inline_label; \
> >+		MARK_JUMP_CALL_PROTOTYPE(name); \
> >+		goto *__mark_##name##_jump_over; \
> >+call_label: \
> >+		MARK_JUMP_CALL(name, format, ## args); \
> >+		goto over_label; \
> >+inline_label: \
> >+		MARK_JUMP_INLINE(name, format, ## args); \
> >+over_label: \
> >+		do {} while(0); \
> >+	} while(0)
> >  
> 
> I have to admit I haven't been following all this tracing stuff, but 
> this is pretty opaque.  What's it trying to achieve?  Hm, OK, I think I 
> see what you're getting at here - see below.
> 

Default behavior : load "over_label" and jump to its address.
I can override dynamically the behavior by setting the jump target to either
call_label or inline_label.

The call_label is reponsible for calling an empty function by default. The
function pointer can be overridden.


> >+
> >+#define MARK(name, format, args...) \
> >+	do { \
> >+		__mark_check_format(format, ## args); \
> >+		MARK_SYM(name); \
> >+		MARK_JUMP(name, format, ## args); \
> >+	} while(0)
> >  
> 
> Does this assume that the symbol injected by MARK_SYM() will label the 
> MARK_JUMP() code?  Because it won't.
> 

No, MARK_SYM() is only useful for kprobes, unrelated to MARK_JUMP.

> >	
> >	printk("Installing hook\n");
> >	*target_mark_call = (void*)do_mark1;
> >	saved_over = *target_mark_jump_over;
> >	*target_mark_jump_over = *target_mark_jump_call;
> >  
> 
> So the point of this is to set up the new function, then update the 
> jumpover to point to it, in a way that's SMP safe?  This assumes two things:
> 
>   1. that your pointer updates are atomic

Yes : the pointer is aligned and all of the architectures that I know write
pointer-sized information atomically when it is aligned.

>   2. that these writes don't get reordered
> 

It doesn't matter :) You are absolutely right, they can get reordered, and the
fact is : we don't care. The function above sets the *target_mark_call before
the *target_mark_jump_over, so that the function pointer is set up before the
jump can call it. But imagine the inverse : the will be able to the function
call before the function call handler is set up. It really doesn't matter
because the function pointer is always pointing to a valid function : either the
"empty" default function or the inserted one.

> 
> 
> >	return 0;
> >}
> >
> >int init_module(void)
> >{
> >	return mark_install_hook();
> >}
> >
> >void cleanup_module(void)
> >{
> >	printk("Removing hook\n");
> >	*target_mark_jump_over = saved_over;
> >	*target_mark_call = __mark_empty_function;
> >
> >	/* Wait for instrumentation functions to return before quitting */
> >	synchronize_sched();
> >}
> >  
> 
> What if multiple people hook onto the same mark?  Are you assuming LIFO?
> 

That's the "proof of concept module" part. I plan to do a kernel/marker.c that
will deal with all marker activation from a centralized, coherent mechanism. The
functions that will be called will simply sit in modules.

Before activating a probe, the marker.ko module will 1- take proper locking
and 2- verify that the function pointer and jmp target are at their default
values, otherwise the "install" operation fails, increment the probe-module
refcount and then set them up.

Setting them back to disabled is always the same operation : setting back the
default jmp and call values, synchronize_sched() and release the probe-module
refcount.

The marker.ko module will also deal with inline jump selection, which is the
same case as presented here, but without the function pointer, module refcount
and synchronization.

Thanks,

Mathieu


OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Linux Markers 0.4 (+dynamic probe loader) for 2.6.17
  2006-09-20 23:45 [PATCH] Linux Markers 0.4 (+dynamic probe loader) for 2.6.17 Mathieu Desnoyers
       [not found] ` <4511D92A.3090204@goop.org>
@ 2006-09-21  4:54 ` Ingo Molnar
  2006-09-21 12:28   ` Roman Zippel
  1 sibling, 1 reply; 5+ messages in thread
From: Ingo Molnar @ 2006-09-21  4:54 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: Martin Bligh, Frank Ch. Eigler, Masami Hiramatsu, prasanna,
	Andrew Morton, Paul Mundt, linux-kernel, Jes Sorensen,
	Tom Zanussi, Richard J Moore, Michel Dagenais, Christoph Hellwig,
	Greg Kroah-Hartman, Thomas Gleixner, William Cohen, ltt-dev,
	systemtap, Alan Cox


* Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:

> +menu "Marker configuration"

> +config MARK_SYMBOL
> +	bool "Replace markers with symbols"
> +	  Put symbols in place of markers, useful for kprobe.
> +
> +config MARK_JUMP_CALL
> +	bool "Replace markers with a jump over an inactive function call"
> +	  Put a jump over a call in place of markers.
> +
> +config MARK_JUMP_INLINE
> +	bool "Replace markers with a jump over an inline function"
> +	  Put a jump over an inline function.

This patch still has the fundamental failure of offering 3 annotation 
methods instead of offering _one_ annotation method. I mention it again, 
distros want to have _one_ method they enable, not: "oh, by the way, LTT 
requires MARK_JUMP_CALL, so no matter how low-overhead MARK_SYMBOL is, 
you have to enable MARK_JUMP_CALL anyway".

We have to face it, tracing is a very optional infrastructure, thus it 
has to be _very low (preferably zero in most cases) overhead when 
offered by a kernel binary but kept inactive by the user_ and thus you 
/have to/ program on the edge to get it into the upstream kernel.

It wont be easy to achieve this, and you'll have to work with the other 
tracing projects (and upstream kernel folks) to get one unified markup 
mechanism agreed on, but nevertheless it's possible technologically.

and the only acceptable near-zero-overhead markup scheme proposed so far 
(and suggested by me all along) is the symbol based markup method. 
Symbol based markup also has the advantage that the coupling between the 
kernel and the tracer moves to the symbol space (from the binary 
instruction-stream space), and thus the in-kernel implementation of it 
becomes alot more flexible. Flexibility of the upstream kernel design is 
another thing that we require for 'very optional' features.

Yes, LTT will probably have to embrace kprobes/SystemTap to insert the 
tracepoints themselves, but that's the price we get for uniformity, and 
that's the price you get for _having the markers maintained upstream_.

If after that point upstream cannot optimize kprobes performance to a 
sufficient level, /then/ can we think about /perhaps/ allowing direct 
calls generated into the kernel image. But that decision /must/ be 
driven by distributions and customers. Until then, kprobes based marking 
and tracing will be 'good enough'.

It affects all tracers: SystemTap/LKST has to adapt to such a scheme 
too, because currently there's no markup scheme in the kernel. So this 
is not something 'against' LTT, but something /for/ a unified landscape 
of tracers. (and as i mentioned it before, it will be easy for you to 
offer a simple "LTT speedup patch", which distros and the upstream 
kernel can consider separately. But it must be /optional/.)

So far i have not seen any real arguments against this simple but 
fundamental upstream requirement which i pointed out for v0.1 already.

	Ingo

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Linux Markers 0.4 (+dynamic probe loader) for 2.6.17
  2006-09-21  4:54 ` Ingo Molnar
@ 2006-09-21 12:28   ` Roman Zippel
  0 siblings, 0 replies; 5+ messages in thread
From: Roman Zippel @ 2006-09-21 12:28 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Mathieu Desnoyers, Martin Bligh, Frank Ch. Eigler,
	Masami Hiramatsu, prasanna, Andrew Morton, Paul Mundt,
	linux-kernel, Jes Sorensen, Tom Zanussi, Richard J Moore,
	Michel Dagenais, Christoph Hellwig, Greg Kroah-Hartman,
	Thomas Gleixner, William Cohen, ltt-dev, systemtap, Alan Cox

Hi,

On Thu, 21 Sep 2006, Ingo Molnar wrote:

> It affects all tracers: SystemTap/LKST has to adapt to such a scheme 
> too, because currently there's no markup scheme in the kernel. So this 
> is not something 'against' LTT, but something /for/ a unified landscape 
> of tracers. (and as i mentioned it before, it will be easy for you to 
> offer a simple "LTT speedup patch", which distros and the upstream 
> kernel can consider separately. But it must be /optional/.)

Out of curiosity: How exactly would it hurt this unifiation, if you left 
some of the implementation details simply to the archs?

> So far i have not seen any real arguments against this simple but 
> fundamental upstream requirement which i pointed out for v0.1 already.

It's funny, after reality sets in, I'll get exactly what I asked for in 
the first place, now I only have to figure out a way to do this without 
getting insulted by almost everyone...

bye, Roman

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Linux Markers 0.4 (+dynamic probe loader) for 2.6.17
       [not found]     ` <45123C7D.3080309@goop.org>
@ 2006-09-21 14:46       ` Mathieu Desnoyers
  0 siblings, 0 replies; 5+ messages in thread
From: Mathieu Desnoyers @ 2006-09-21 14:46 UTC (permalink / raw)
  To: Jeremy Fitzhardinge
  Cc: Martin Bligh, Frank Ch. Eigler, Masami Hiramatsu, prasanna,
	Andrew Morton, Ingo Molnar, Paul Mundt, linux-kernel,
	Jes Sorensen, Tom Zanussi, Richard J Moore, Michel Dagenais,
	Christoph Hellwig, Greg Kroah-Hartman, Thomas Gleixner,
	William Cohen, ltt-dev, systemtap, Alan Cox

* Jeremy Fitzhardinge (jeremy@goop.org) wrote:
> Mathieu Desnoyers wrote:
> >Yup, good catch. I have not seen gcc removing this asm in my objdump 
> >however, by
> >I guess we cannot be sure. This MARK_SYM() is only useful for kprobe
> >insertion : I don't use it myself for the jump markup stuff. I don't know 
> >how
> >relevant it is for kprobes users for the symbol to be at a specific 
> >location,
> >as they don't know themself what data they are interested in and they 
> >simply
> >don't want to modify the instruction stream. I fact, if the asm volatile
> >modifies the instruction stream, it would be an unwanted side-effect :(
> >  
> 
> "asm volatile" isn't documented to do anything other than prevent the 
> asm from being removed altogether.  It doesn't prevent it from being 
> moved elsewhere, and it doesn't imply any ordering dependency with the 
> code around it.  So I don't think it will change the generated code, but 
> I also don't think it will be all that useful unless there's something 
> to actually make sure it's in a particular place - and that may change 
> codegen because it may force the compiler to not eliminate/reorder/move 
> the point at which you want the label.
> 
> Something like this might do it:
> 
>    #define MARK_SYM(label)						\
>    	do {							\
>    		__label__ here;					\
>    	  here: asm volatile(#label " = %0" : : "m" (*&&here));	\
>    	} while(0)
>      
> 
> This at least gives the compiler a C-level label to hang the asm from.
> 
Ok, let's do that then. Thanks for the hint.


> >It doesn't matter :) You are absolutely right, they can get reordered, and 
> >the
> >fact is : we don't care. The function above sets the *target_mark_call 
> >before
> >the *target_mark_jump_over, so that the function pointer is set up before 
> >the
> >jump can call it. But imagine the inverse : the will be able to the 
> >function
> >call before the function call handler is set up. It really doesn't matter
> >because the function pointer is always pointing to a valid function : 
> >either the
> >"empty" default function or the inserted one.
> >  
> 
> Does the local indirect jump really help?  Wouldn't you do just as well 
> with the call?

Taking a function call, even if it is an empty function, will also imply the
cost of setting up the stack. I think it will be more costly than a load+jump.

> It's a jump out of line, but if it points to the null 
> function, it's likely to be in cache, and reducing the number of 
> indirect targets within a few instructions will help the CPU keep its 
> branch target prediction in order (modern Intel chips don't like having 
> too many indirect jumps within a cache line, for example).
> 

Good point. However, as my tests pointed out, it seems less costly to loop
doing out of line jumps than to loop doing predicted branches. Weird, but it
seems to be the case. We should however compare the speed of the jump vs stack
setup and call to empty function.


> It's a pity you can't make these all direct jumps; I guess patching the 
> instruction stream on an SMP system on the fly is too tricky...
> 

This is my basic concern : teams have been working on this full-time for a few
years without success, why would I succeed at doing faset portable
code-modifying branching code in less than that ? I think that the first thing
to achieve is to provide a fast+portable way of dealing with markers and then
the architecture specific improvements will come. As my marking mechanism is
generic enough to do any symbol marking of assembly, it will be easily
customizable per architecture.


> (Though on x86 you could do something like make the default case 5 bytes 
> of nops.  Then to patch it, you could patch in an int3 on the first 
> byte, put the relative address in the other 4 bytes, then patch the int3 
> back to the call/jump.  The int3 handler would look to see if the fault 
> address is a kernel hook point, and if so, spin waiting for the *eip to 
> go to a call/jump, then resume the instruction.)
> 

Yes, many optimisations can be thought of, for many architectures. What I miss
in your idea is where the function call will be ? Probably jumped-over by a
goto after the nops (so that the compiler will put the function call rarely-used
part of the function) ?

The problem with your approach is that : as we are in preemptible code, there
can be an arbitrary thread running in the NOPs, scheduled out and stopped. It
must not come back and iret in the middle of your addresses. The same problem
exists for interrupt handlers.

Regards,

Mathieu


OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2006-09-21 14:46 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-09-20 23:45 [PATCH] Linux Markers 0.4 (+dynamic probe loader) for 2.6.17 Mathieu Desnoyers
     [not found] ` <4511D92A.3090204@goop.org>
2006-09-21  2:04   ` Mathieu Desnoyers
     [not found]     ` <45123C7D.3080309@goop.org>
2006-09-21 14:46       ` Mathieu Desnoyers
2006-09-21  4:54 ` Ingo Molnar
2006-09-21 12:28   ` Roman Zippel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).