From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752115Ab3KKVwP (ORCPT ); Mon, 11 Nov 2013 16:52:15 -0500 Received: from cdptpa-outbound-snat.email.rr.com ([107.14.166.226]:63580 "EHLO cdptpa-oedge-vip.email.rr.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751531Ab3KKVwG (ORCPT ); Mon, 11 Nov 2013 16:52:06 -0500 Date: Mon, 11 Nov 2013 16:52:04 -0500 From: Steven Rostedt To: Bharath Ravi Cc: Vaibhav Nagarnaik , David Sharp , Laurent Chavey , linux-kernel@vger.kernel.org Subject: Re: [PATCH] tracing: Allow changing default ring buffer size for ftrace instances. Message-ID: <20131111165204.513a72cd@gandalf.local.home> In-Reply-To: <1383784982-27482-1-git-send-email-rbharath@google.com> References: <1383784982-27482-1-git-send-email-rbharath@google.com> X-Mailer: Claws Mail 3.9.2 (GTK+ 2.24.20; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-RR-Connecting-IP: 107.14.168.142:25 X-Cloudmark-Score: 0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 6 Nov 2013 16:43:02 -0800 Bharath Ravi 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 > --- > 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); >