Re: [patch/RFC] make kernel vs user ip detection more portable in the kernel

[Philippe E. <ph...@wa...>]

Randolph Chung wrote:
> The attach patch makes it possible for arch-specific code to determine
> whether a certain address is in kernel space or in user space when
> adding a sample to oprofile. It also defines two new codes to denote
> detection of a kernel->user switch or a user->kernel switch.
> 
> This still needs some thought though, because by encoding these as new
> events, there is a chance for some uncertainty of whether a recorded
> address is in userspace or kernelspace when a new buffer is being filled
> in (if an address is stored before a switch is detected). 
> 
> any comments? this also requires corresponding userspace tool changes to
> work. together with some other changes this seems to work for some
> simple cases on parisc-linux now (2.5.54 kernel) 


Some minor point + a bug unless I miss something.


> +++ include/linux/oprofile.h	12 Jan 2003 08:41:33 -0000
> @@ -55,7 +55,8 @@ int oprofile_arch_init(struct oprofile_o
>   * Add a sample. This may be called from any context. Pass
>   * smp_processor_id() as cpu.
>   */
> -extern void oprofile_add_sample(unsigned long eip, unsigned long event, int cpu);
> +extern void oprofile_add_sample(unsigned long eip, unsigned int is_user, 
> +	unsigned long event, int cpu);


Mismatch of named parameter, it's is_kernel, annoying
if someone read the header to remember what is the
meaning of this parameter.



> Index: drivers/oprofile/buffer_sync.c
> ===================================================================
> RCS file: /var/cvs/linux-2.5/drivers/oprofile/buffer_sync.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 buffer_sync.c
> --- drivers/oprofile/buffer_sync.c	24 Dec 2002 22:43:07 -0000	1.6
> +++ drivers/oprofile/buffer_sync.c	12 Jan 2003 08:41:33 -0000
> @@ -199,8 +199,16 @@ static void add_cpu_switch(int i)
>  	last_cookie = ~0UL;
>  }
>  
> +static void add_kernel_ctx_switch(unsigned int in_kernel)


You mix int/unsigned int for in_kernel.



> -static void add_sample(struct mm_struct * mm, struct op_sample * s)
> +static void add_sample(struct mm_struct * mm, struct op_sample * s, int in_kernel)
>  {
> -	if (is_kernel(s->eip)) {
> +	if (in_kernel) {
>  		add_sample_entry(s->eip, s->event);
>  	} else if (mm) {
>  		add_us_sample(mm, s);
> @@ -319,21 +321,29 @@ static void sync_buffer(struct oprofile_
>  	struct mm_struct * mm = 0;
>  	struct task_struct * new;
>  	unsigned long cookie;
> +	int in_kernel = 1;
>  	int i;

1) cpu0 in kernel mode, cpu1 in user mode
2) buffer sync occur and finish with the global buffer in user mode
3) no switch occur: cpu0 continue to receive kernel samples
4) at next buffer sync cpu0 samples will be flushed w/o a needed
   kernel_switch code.

>   
>  	for (i=0; i < cpu_buf->pos; ++i) {
>  		struct op_sample * s = &cpu_buf->buffer[i];
>   
>  		if (is_ctx_switch(s->eip)) {
> -			new = (struct task_struct *)s->event;
> - 
> -			release_mm(mm);
> -			mm = take_task_mm(new);
> - 
> -			cookie = get_exec_dcookie(mm);
> -			add_ctx_switch(new->pid, cookie);
> +			if (s->event <= 1) {
> +				/* kernel/userspace switch */
> +				in_kernel = s->event;
> +				add_kernel_ctx_switch(s->event);
> +			} else {
> +				/* userspace context switch */
> +				new = (struct task_struct *)s->event;
> +
> +				release_mm(mm);
> +				mm = take_task_mm(new);
> +
> +				cookie = get_exec_dcookie(mm);
> +				add_user_ctx_switch(new->pid, cookie);
> +			}

hu, black magic, 0/1 integer else pointer.

> -void oprofile_add_sample(unsigned long eip, unsigned long event, int cpu)
> +void oprofile_add_sample(unsigned long eip, unsigned int is_kernel, 
> +	unsigned long event, int cpu)
>  {
>  	struct oprofile_cpu_buffer * cpu_buf = &cpu_buffer[cpu];
>  	struct task_struct * task;
>  
> +	/* force to 0 or 1 */

hey, explain rather why you need to force to 0/1 :)

> +	is_kernel = !!is_kernel;
> +

needed because 0/1/pointer but I don't see how we can
avoid that.

regards,
Phil