Re: [PATCH 5/6] x86: patch all traced function calls using the int3-based framework

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 18 Oct 2013 16:27:24 +0200
Petr Mladek <pmladek@suse.cz> wrote:


> +/*
> + * We do not want to replace ftrace calls one by one because syncing all CPUs
> + * is quite expensive.
> + *
> + * On the other hand, we do not want to modify all calls at once because
> + * the buffers for text_poke_bp might be quite huge. Also we do not want
> + * to count the number of records before the allocation.
> + *
> + * Let's modify the call in batches defined by this define.
> + */
> +#define FTRACE_MAX_RECS_PATCH 8192
>  
> -static int finish_update(struct dyn_ftrace *rec, int enable)
> +static int ftrace_allocate_replace_buffers(unsigned long **addr, void **opcode)

I absolutely hate this. Current ftrace conversion does not need to
allocate at all. I want to keep it that way.

Now, what I was thinking is that the text_poke_bp() should have a
variant where you can pass in an iterator. That way we don't need to
copy the ftrace records to an array that text_poke_bp() uses to do the
conversion. Just let the iterator return the next address to use.

I'm all for merging the code (was on my TODO list, so thank you for
this :-) but one reason I didn't get to it yet, was because of not
doing the copy. To me, that's a cop out.

Anyway, this was really bad timing. I'm trying to get some other things
reviewed and into the kernel for 3.12 before it's too late, and next
week I'll be traveling for 10 days (Kernel Summit, followed by RT
summit). You may need to ping me again in a couple of weeks to review
this. Unless I get some time to do it in my travels.

Thanks,

-- Steve


>  {
> -	unsigned long ftrace_addr;
> -	int ret;
> -
> -	ret = ftrace_update_record(rec, enable);
> -
> -	ftrace_addr = get_ftrace_addr(rec);
> -
> -	switch (ret) {
> -	case FTRACE_UPDATE_IGNORE:
> -		return 0;
> -
> -	case FTRACE_UPDATE_MODIFY_CALL_REGS:
> -	case FTRACE_UPDATE_MODIFY_CALL:
> -	case FTRACE_UPDATE_MAKE_CALL:
> -		/* converting nop to call */
> -		return finish_update_call(rec, ftrace_addr);
> +	*addr = kmalloc_array(FTRACE_MAX_RECS_PATCH,
> +			      sizeof(*addr), GFP_KERNEL);
> +	if (!addr) {
> +		printk(KERN_ERR
> +		       "ftrace_replace_code: cannot allocate addr buffer\n");
> +		return -ENOMEM;
> +	}
>  
> -	case FTRACE_UPDATE_MAKE_NOP:
> -		/* converting a call to a nop */
> -		return finish_update_nop(rec);
> +	*opcode = kmalloc_array(FTRACE_MAX_RECS_PATCH,
> +				MCOUNT_INSN_SIZE, GFP_KERNEL);
> +	if (!opcode) {
> +		printk(KERN_ERR
> +		       "ftrace_replace_code: cannot allocate codes buffer\n");
> +		return -ENOMEM;
>  	}
>  
>  	return 0;
>  }
>