Re: [PATCH] Linux Markers 0.4 (+dynamic probe loader) for 2.6.17

[Jeremy Fitzhardinge <jeremy@goop.org>]

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.

> +#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.

> +		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.

> +
> +#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.

> 	
> 	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
   2. that these writes don't get reordered

1 might be safe, but I don't think its guaranteed for all 
architectures.  2 is not true without some explicit barriers in there.  
You'll probably need some in MARK_JUMP too.


> 	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?

    J