Re: [PATCH] tracing: Allow changing default ring buffer size for ftrace instances.

[Steven Rostedt <rostedt@goodmis.org>]

On Wed,  6 Nov 2013 16:43:02 -0800
Bharath Ravi <rbharath@google.com> wrote:

> It is often memory efficient to start instances off with a smaller ring
> buffer size than the current default. This is particularly true on
> systems with many cores, or when multiple ftrace instances are created,
> where the current (higher) default value results in allocation failures.
> 
> To allow instances to start off with custom ring buffer sizes, a new
> debugfs file named "instance_default_buffer_size_kb" is added in the

I don't mind the patch, but I hate the interface.
"instance_default_buffer_size_kb", can we get a bigger name?

"instance_buffer_size_kb" may be sufficient, even though I don't even
care for that.

I wonder if we add it to the tracing/options directory, if that would
be better. Grant you, this would be the first option that's not an on
off switch, but I'm not sure that's a bad thing.

Have any better ideas?

-- Steve

> root ftrace directory. A size in kilobytes may be written to this. Any
> ftrace instances created will be initialized with ring buffers of the
> size specified in this file.
> 
> This change also modifies allocate_trace_buffer() to use the "size"
> parameter while setting set_buffer_entries(), instead of the actual ring
> ring buffer size (which is "size" rounded up to the nearest page size).
> This makes it consistent with the way writing to the buffer_size_kb
> files works (using ring_buffer_resize()), and ensures that the
> buffer_size_kb value of a newly created instance matches that written
> to the instance_default_buffer_size file, instead of the rounded up
> value.
> 
> Tested: Booted into a kernel with these changes, verified reading and
> writing to the new file. Ensured that new ftrace instances created
> used the size specified in instance_default_buffer_size_kb as their
> initial ring buffer size.
> 
> Signed-off-by: Bharath Ravi <rbharath@google.com>
> ---
>  kernel/trace/trace.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 54 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 7974ba2..d10afea 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -296,6 +296,14 @@ int tracing_is_enabled(void)
>  
>  static unsigned long		trace_buf_size = TRACE_BUF_SIZE_DEFAULT;
>  
> +/*
> + * instance_default_buf_size is the size in bytes that is allocated for an
> + * instance's buffer when it is first created. This is initially set to
> + * TRACE_BUF_SIZE_DEFAULT but may be modified by writing to the
> + * "instance_default_buffer_size_kb" file.
> + */
> +static unsigned long instance_default_buf_size = TRACE_BUF_SIZE_DEFAULT;
> +
>  /* trace_types holds a link list of available tracers. */
>  static struct tracer		*trace_types __read_mostly;
>  
> @@ -3165,6 +3173,48 @@ static const struct file_operations show_traces_fops = {
>  	.llseek		= seq_lseek,
>  };
>  
> +static ssize_t
> +instance_default_buffer_size_read(struct file *filp, char __user *ubuf,
> +				  size_t cnt, loff_t *ppos)
> +{
> +	int r = 0;
> +	char buf[64];
> +	ssize_t ret;
> +
> +	r = sprintf(buf, "%lu\n", instance_default_buf_size >> 10);
> +	ret = simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
> +
> +	return ret;
> +}
> +
> +static ssize_t
> +instance_default_buffer_size_write(struct file *filp, const char __user *ubuf,
> +				   size_t cnt, loff_t *ppos)
> +{
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtoul_from_user(ubuf, cnt, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* ubuf must have at least 1 entry */
> +	if (!val)
> +		return -EINVAL;
> +
> +	/* val is in KB */
> +	instance_default_buf_size = val << 10;
> +
> +	return cnt;
> +}
> +
> +static const struct file_operations instance_default_buffer_size_fops = {
> +	.open		= tracing_open_generic,
> +	.read		= instance_default_buffer_size_read,
> +	.write		= instance_default_buffer_size_write,
> +	.llseek		= generic_file_llseek,
> +};
> +
>  /*
>   * The tracer itself will not take this lock, but still we want
>   * to provide a consistent cpumask to user-space:
> @@ -5880,8 +5930,7 @@ allocate_trace_buffer(struct trace_array *tr, struct trace_buffer *buf, int size
>  	}
>  
>  	/* Allocate the first page for all buffers */
> -	set_buffer_entries(&tr->trace_buffer,
> -			   ring_buffer_size(tr->trace_buffer.buffer, 0));
> +	set_buffer_entries(&tr->trace_buffer, size);
>  
>  	return 0;
>  }
> @@ -5947,7 +5996,7 @@ static int new_instance_create(const char *name)
>  	INIT_LIST_HEAD(&tr->systems);
>  	INIT_LIST_HEAD(&tr->events);
>  
> -	if (allocate_trace_buffers(tr, trace_buf_size) < 0)
> +	if (allocate_trace_buffers(tr, instance_default_buf_size) < 0)
>  		goto out_free_tr;
>  
>  	tr->dir = debugfs_create_dir(name, trace_instance_dir);
> @@ -6177,6 +6226,8 @@ static __init int tracer_init_debugfs(void)
>  	trace_create_file("dyn_ftrace_total_info", 0444, d_tracer,
>  			&ftrace_update_tot_cnt, &tracing_dyn_info_fops);
>  #endif
> +	trace_create_file("instance_default_buffer_size_kb", 0444, d_tracer,
> +			NULL, &instance_default_buffer_size_fops);
>  
>  	create_trace_instances(d_tracer);
>