[PATCH 0/4] [GIT PULL] memory saving ring buffer for tip/tracing/ftrace

[PATCH 0/4] [GIT PULL] memory saving ring buffer for tip/tracing/ftrace

[Steven Rostedt]

Ingo,

This patch series implements a better memory management by the
ring buffer. It only allocates the buffers for the online CPUS, as well
as keeps the buffers to a minimum size until they are used.

Please pull the latest tip/tracing/ftrace tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/tracing/ftrace


Steven Rostedt (4):
      tracing: keep ring buffer to minimum size till used
      tracing: expand the ring buffers when an event is activated
      tracing: fix trace_wait to know to wait on all cpus or just one
      ring-buffer: only allocate buffers for online cpus

----
 kernel/trace/ring_buffer.c  |  266 +++++++++++++++++++++++++++++++++++--------
 kernel/trace/trace.c        |  118 +++++++++++++++----
 kernel/trace/trace.h        |    3 +
 kernel/trace/trace_events.c |    8 ++
 4 files changed, 319 insertions(+), 76 deletions(-)

-- 

[PATCH 1/4] tracing: keep ring buffer to minimum size till used

[Steven Rostedt]

[-- Attachment #1: 0001-tracing-keep-ring-buffer-to-minimum-size-till-used.patch --]
[-- Type: text/plain, Size: 5038 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Impact: less memory impact on systems not using tracer

When the kernel boots up that has tracing configured, it allocates
the default size of the ring buffer. This currently happens to be
1.4Megs per possible CPU. This is quite a bit of wasted memory if
the system is never using the tracer.

The current solution is to keep the ring buffers to a minimum size
until the user uses them. Once a tracer is piped into the current_tracer
the ring buffer will be expanded to the default size. If the user
changes the size of the ring buffer, it will take the size given
by the user immediately.

If the user adds a "ftrace=" to the kernel command line, then the ring
buffers will be set to the default size on initialization.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/trace.c |   79 +++++++++++++++++++++++++++++++++++++------------
 1 files changed, 59 insertions(+), 20 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 4c97947..0c1dc18 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -45,6 +45,12 @@ unsigned long __read_mostly	tracing_max_latency;
 unsigned long __read_mostly	tracing_thresh;
 
 /*
+ * On boot up, the ring buffer is set to the minimum size, so that
+ * we do not waste memory on systems that are not using tracing.
+ */
+static int ring_buffer_expanded;
+
+/*
  * We need to change this state when a selftest is running.
  * A selftest will lurk into the ring-buffer to count the
  * entries inserted during the selftest although some concurrent
@@ -128,6 +134,8 @@ static int __init set_ftrace(char *str)
 {
 	strncpy(bootup_tracer_buf, str, BOOTUP_TRACER_SIZE);
 	default_bootup_tracer = bootup_tracer_buf;
+	/* We are using ftrace early, expand it */
+	ring_buffer_expanded = 1;
 	return 1;
 }
 __setup("ftrace=", set_ftrace);
@@ -2315,6 +2323,40 @@ int tracer_init(struct tracer *t, struct trace_array *tr)
 	return t->init(tr);
 }
 
+static int tracing_resize_ring_buffer(unsigned long size)
+{
+	int ret;
+
+	/*
+	 * If kernel or user changes the size of the ring buffer
+	 * it get completed.
+	 */
+	ring_buffer_expanded = 1;
+
+	ret = ring_buffer_resize(global_trace.buffer, size);
+	if (ret < 0)
+		return ret;
+
+	ret = ring_buffer_resize(max_tr.buffer, size);
+	if (ret < 0) {
+		int r;
+
+		r = ring_buffer_resize(global_trace.buffer,
+				       global_trace.entries);
+		if (r < 0) {
+			/* AARGH! We are left with different
+			 * size max buffer!!!! */
+			WARN_ON(1);
+			tracing_disabled = 1;
+		}
+		return ret;
+	}
+
+	global_trace.entries = size;
+
+	return ret;
+}
+
 struct trace_option_dentry;
 
 static struct trace_option_dentry *
@@ -2330,6 +2372,13 @@ static int tracing_set_tracer(const char *buf)
 	struct tracer *t;
 	int ret = 0;
 
+	if (!ring_buffer_expanded) {
+		ret = tracing_resize_ring_buffer(trace_buf_size);
+		if (ret < 0)
+			return ret;
+		ret = 0;
+	}
+
 	mutex_lock(&trace_types_lock);
 	for (t = trace_types; t; t = t->next) {
 		if (strcmp(t->name, buf) == 0)
@@ -2903,28 +2952,11 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 	val <<= 10;
 
 	if (val != global_trace.entries) {
-		ret = ring_buffer_resize(global_trace.buffer, val);
+		ret = tracing_resize_ring_buffer(val);
 		if (ret < 0) {
 			cnt = ret;
 			goto out;
 		}
-
-		ret = ring_buffer_resize(max_tr.buffer, val);
-		if (ret < 0) {
-			int r;
-			cnt = ret;
-			r = ring_buffer_resize(global_trace.buffer,
-					       global_trace.entries);
-			if (r < 0) {
-				/* AARGH! We are left with different
-				 * size max buffer!!!! */
-				WARN_ON(1);
-				tracing_disabled = 1;
-			}
-			goto out;
-		}
-
-		global_trace.entries = val;
 	}
 
 	filp->f_pos += cnt;
@@ -3916,6 +3948,7 @@ void ftrace_dump(void)
 __init static int tracer_alloc_buffers(void)
 {
 	struct trace_array_cpu *data;
+	int ring_buf_size;
 	int i;
 	int ret = -ENOMEM;
 
@@ -3928,12 +3961,18 @@ __init static int tracer_alloc_buffers(void)
 	if (!alloc_cpumask_var(&tracing_reader_cpumask, GFP_KERNEL))
 		goto out_free_tracing_cpumask;
 
+	/* To save memory, keep the ring buffer size to its minimum */
+	if (ring_buffer_expanded)
+		ring_buf_size = trace_buf_size;
+	else
+		ring_buf_size = 1;
+
 	cpumask_copy(tracing_buffer_mask, cpu_possible_mask);
 	cpumask_copy(tracing_cpumask, cpu_all_mask);
 	cpumask_clear(tracing_reader_cpumask);
 
 	/* TODO: make the number of buffers hot pluggable with CPUS */
-	global_trace.buffer = ring_buffer_alloc(trace_buf_size,
+	global_trace.buffer = ring_buffer_alloc(ring_buf_size,
 						   TRACE_BUFFER_FLAGS);
 	if (!global_trace.buffer) {
 		printk(KERN_ERR "tracer: failed to allocate ring buffer!\n");
@@ -3944,7 +3983,7 @@ __init static int tracer_alloc_buffers(void)
 
 
 #ifdef CONFIG_TRACER_MAX_TRACE
-	max_tr.buffer = ring_buffer_alloc(trace_buf_size,
+	max_tr.buffer = ring_buffer_alloc(ring_buf_size,
 					     TRACE_BUFFER_FLAGS);
 	if (!max_tr.buffer) {
 		printk(KERN_ERR "tracer: failed to allocate max ring buffer!\n");
-- 
1.6.1.3

-- 

[PATCH 2/4] tracing: expand the ring buffers when an event is activated

[Steven Rostedt]

[-- Attachment #1: 0002-tracing-expand-the-ring-buffers-when-an-event-is-ac.patch --]
[-- Type: text/plain, Size: 2441 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

To save memory, the tracer ring buffers are set to a minimum.
The activating of a trace expands the ring buffer size. This patch
adds this expanding, when an event is activated.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/trace.c        |   20 ++++++++++++++++++++
 kernel/trace/trace.h        |    3 +++
 kernel/trace/trace_events.c |    8 ++++++++
 3 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 0c1dc18..35ee63a 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2357,6 +2357,26 @@ static int tracing_resize_ring_buffer(unsigned long size)
 	return ret;
 }
 
+/**
+ * tracing_update_buffers - used by tracing facility to expand ring buffers
+ *
+ * To save on memory when the tracing is never used on a system with it
+ * configured in. The ring buffers are set to a minimum size. But once
+ * a user starts to use the tracing facility, then they need to grow
+ * to their default size.
+ *
+ * This function is to be called when a tracer is about to be used.
+ */
+int tracing_update_buffers(void)
+{
+	int ret = 0;
+
+	if (!ring_buffer_expanded)
+		ret = tracing_resize_ring_buffer(trace_buf_size);
+
+	return ret;
+}
+
 struct trace_option_dentry;
 
 static struct trace_option_dentry *
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index c5e1d88..336324d 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -737,6 +737,9 @@ static inline void trace_branch_disable(void)
 }
 #endif /* CONFIG_BRANCH_TRACER */
 
+/* set ring buffers to default size if not already done so */
+int tracing_update_buffers(void);
+
 /* trace event type bit fields, not numeric */
 enum {
 	TRACE_EVENT_TYPE_PRINTF		= 1,
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 769dfd0..ca624df 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -141,6 +141,10 @@ ftrace_event_write(struct file *file, const char __user *ubuf,
 	if (!cnt || cnt < 0)
 		return 0;
 
+	ret = tracing_update_buffers();
+	if (ret < 0)
+		return ret;
+
 	ret = get_user(ch, ubuf++);
 	if (ret)
 		return ret;
@@ -331,6 +335,10 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	if (ret < 0)
 		return ret;
 
+	ret = tracing_update_buffers();
+	if (ret < 0)
+		return ret;
+
 	switch (val) {
 	case 0:
 	case 1:
-- 
1.6.1.3

-- 

[PATCH 3/4] tracing: fix trace_wait to know to wait on all cpus or just one

[Steven Rostedt]

[-- Attachment #1: 0003-tracing-fix-trace_wait-to-know-to-wait-on-all-cpus.patch --]
[-- Type: text/plain, Size: 1722 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Impact: fix to task live locking on reading trace_pipe on one CPU

The same code is used for both trace_pipe (all CPUS) and the per_cpu
trace_pipe file. When there is no data to read, it will check for
signals and wait on the trace wait queue.

The problem happens with the per_cpu wait. The trace_wait code checks
all CPUs. Thus, if there's data in another CPU buffer, then it will
exit the wait, without checking for signals or waiting on the wait queue.

It would then try to read the empty buffer, and since that will just
return nothing, then it will try to wait again. Unfortunately, that will
again fail due to there still being data in the other buffers. This
ends up with a live lock for the task.

This patch fixes the trace_wait to be aware that the iterator may only
be waiting on a single buffer.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/trace.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 35ee63a..e60f4be 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1666,6 +1666,19 @@ static int trace_empty(struct trace_iterator *iter)
 {
 	int cpu;
 
+	/* If we are looking at one CPU buffer, only check that one */
+	if (iter->cpu_file != TRACE_PIPE_ALL_CPU) {
+		cpu = iter->cpu_file;
+		if (iter->buffer_iter[cpu]) {
+			if (!ring_buffer_iter_empty(iter->buffer_iter[cpu]))
+				return 0;
+		} else {
+			if (!ring_buffer_empty_cpu(iter->tr->buffer, cpu))
+				return 0;
+		}
+		return 1;
+	}
+
 	for_each_tracing_cpu(cpu) {
 		if (iter->buffer_iter[cpu]) {
 			if (!ring_buffer_iter_empty(iter->buffer_iter[cpu]))
-- 
1.6.1.3

-- 

[PATCH 4/4] ring-buffer: only allocate buffers for online cpus

[Steven Rostedt]

[-- Attachment #1: 0004-ring-buffer-only-allocate-buffers-for-online-cpus.patch --]
[-- Type: text/plain, Size: 16965 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Impact: save on memory

Currently, a ring buffer was allocated for each "possible_cpus". On
some systems, this is the same as NR_CPUS. Thus, if a system defined
NR_CPUS = 64 but it only had 1 CPU, we could have possibly 63 useless
ring buffers taking up space. With a default buffer of 3 megs, this
could be quite drastic.

This patch changes the ring buffer code to only allocate ring buffers
for online CPUs.  If a CPU goes off line, we do not free the buffer.
This is because the user may still have trace data in that buffer
that they would like to look at.

Perhaps in the future we could add code to delete a ring buffer if
the CPU is offline and the ring buffer becomes empty.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/ring_buffer.c |  266 +++++++++++++++++++++++++++++++++++--------
 kernel/trace/trace.c       |    6 -
 2 files changed, 216 insertions(+), 56 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 1788584..d07c288 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -16,6 +16,7 @@
 #include <linux/init.h>
 #include <linux/hash.h>
 #include <linux/list.h>
+#include <linux/cpu.h>
 #include <linux/fs.h>
 
 #include "trace.h"
@@ -301,6 +302,10 @@ struct ring_buffer {
 	struct mutex			mutex;
 
 	struct ring_buffer_per_cpu	**buffers;
+
+#ifdef CONFIG_HOTPLUG
+	struct notifier_block		cpu_notify;
+#endif
 };
 
 struct ring_buffer_iter {
@@ -459,6 +464,11 @@ static void rb_free_cpu_buffer(struct ring_buffer_per_cpu *cpu_buffer)
  */
 extern int ring_buffer_page_too_big(void);
 
+#ifdef CONFIG_HOTPLUG
+static int __cpuinit rb_cpu_notify(struct notifier_block *self,
+				   unsigned long action, void *hcpu);
+#endif
+
 /**
  * ring_buffer_alloc - allocate a new ring_buffer
  * @size: the size in bytes per cpu that is needed.
@@ -496,7 +506,8 @@ struct ring_buffer *ring_buffer_alloc(unsigned long size, unsigned flags)
 	if (buffer->pages == 1)
 		buffer->pages++;
 
-	cpumask_copy(buffer->cpumask, cpu_possible_mask);
+	get_online_cpus();
+	cpumask_copy(buffer->cpumask, cpu_online_mask);
 	buffer->cpus = nr_cpu_ids;
 
 	bsize = sizeof(void *) * nr_cpu_ids;
@@ -512,6 +523,13 @@ struct ring_buffer *ring_buffer_alloc(unsigned long size, unsigned flags)
 			goto fail_free_buffers;
 	}
 
+#ifdef CONFIG_HOTPLUG
+	buffer->cpu_notify.notifier_call = rb_cpu_notify;
+	buffer->cpu_notify.priority = 0;
+	register_cpu_notifier(&buffer->cpu_notify);
+#endif
+
+	put_online_cpus();
 	mutex_init(&buffer->mutex);
 
 	return buffer;
@@ -525,6 +543,7 @@ struct ring_buffer *ring_buffer_alloc(unsigned long size, unsigned flags)
 
  fail_free_cpumask:
 	free_cpumask_var(buffer->cpumask);
+	put_online_cpus();
 
  fail_free_buffer:
 	kfree(buffer);
@@ -541,9 +560,17 @@ ring_buffer_free(struct ring_buffer *buffer)
 {
 	int cpu;
 
+	get_online_cpus();
+
+#ifdef CONFIG_HOTPLUG
+	unregister_cpu_notifier(&buffer->cpu_notify);
+#endif
+
 	for_each_buffer_cpu(buffer, cpu)
 		rb_free_cpu_buffer(buffer->buffers[cpu]);
 
+	put_online_cpus();
+
 	free_cpumask_var(buffer->cpumask);
 
 	kfree(buffer);
@@ -649,16 +676,15 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size)
 		return size;
 
 	mutex_lock(&buffer->mutex);
+	get_online_cpus();
 
 	nr_pages = DIV_ROUND_UP(size, BUF_PAGE_SIZE);
 
 	if (size < buffer_size) {
 
 		/* easy case, just free pages */
-		if (RB_WARN_ON(buffer, nr_pages >= buffer->pages)) {
-			mutex_unlock(&buffer->mutex);
-			return -1;
-		}
+		if (RB_WARN_ON(buffer, nr_pages >= buffer->pages))
+			goto out_fail;
 
 		rm_pages = buffer->pages - nr_pages;
 
@@ -677,10 +703,8 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size)
 	 * add these pages to the cpu_buffers. Otherwise we just free
 	 * them all and return -ENOMEM;
 	 */
-	if (RB_WARN_ON(buffer, nr_pages <= buffer->pages)) {
-		mutex_unlock(&buffer->mutex);
-		return -1;
-	}
+	if (RB_WARN_ON(buffer, nr_pages <= buffer->pages))
+		goto out_fail;
 
 	new_pages = nr_pages - buffer->pages;
 
@@ -705,13 +729,12 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size)
 		rb_insert_pages(cpu_buffer, &pages, new_pages);
 	}
 
-	if (RB_WARN_ON(buffer, !list_empty(&pages))) {
-		mutex_unlock(&buffer->mutex);
-		return -1;
-	}
+	if (RB_WARN_ON(buffer, !list_empty(&pages)))
+		goto out_fail;
 
  out:
 	buffer->pages = nr_pages;
+	put_online_cpus();
 	mutex_unlock(&buffer->mutex);
 
 	return size;
@@ -721,8 +744,18 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size)
 		list_del_init(&bpage->list);
 		free_buffer_page(bpage);
 	}
+	put_online_cpus();
 	mutex_unlock(&buffer->mutex);
 	return -ENOMEM;
+
+	/*
+	 * Something went totally wrong, and we are too paranoid
+	 * to even clean up the mess.
+	 */
+ out_fail:
+	put_online_cpus();
+	mutex_unlock(&buffer->mutex);
+	return -1;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_resize);
 
@@ -1528,11 +1561,15 @@ void ring_buffer_record_disable_cpu(struct ring_buffer *buffer, int cpu)
 {
 	struct ring_buffer_per_cpu *cpu_buffer;
 
+	get_online_cpus();
+
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
-		return;
+		goto out;
 
 	cpu_buffer = buffer->buffers[cpu];
 	atomic_inc(&cpu_buffer->record_disabled);
+ out:
+	put_online_cpus();
 }
 EXPORT_SYMBOL_GPL(ring_buffer_record_disable_cpu);
 
@@ -1548,11 +1585,15 @@ void ring_buffer_record_enable_cpu(struct ring_buffer *buffer, int cpu)
 {
 	struct ring_buffer_per_cpu *cpu_buffer;
 
+	get_online_cpus();
+
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
-		return;
+		goto out;
 
 	cpu_buffer = buffer->buffers[cpu];
 	atomic_dec(&cpu_buffer->record_disabled);
+ out:
+	put_online_cpus();
 }
 EXPORT_SYMBOL_GPL(ring_buffer_record_enable_cpu);
 
@@ -1564,12 +1605,19 @@ EXPORT_SYMBOL_GPL(ring_buffer_record_enable_cpu);
 unsigned long ring_buffer_entries_cpu(struct ring_buffer *buffer, int cpu)
 {
 	struct ring_buffer_per_cpu *cpu_buffer;
+	unsigned long ret = 0;
+
+	get_online_cpus();
 
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
-		return 0;
+		goto out;
 
 	cpu_buffer = buffer->buffers[cpu];
-	return cpu_buffer->entries;
+	ret = cpu_buffer->entries;
+ out:
+	put_online_cpus();
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_entries_cpu);
 
@@ -1581,12 +1629,19 @@ EXPORT_SYMBOL_GPL(ring_buffer_entries_cpu);
 unsigned long ring_buffer_overrun_cpu(struct ring_buffer *buffer, int cpu)
 {
 	struct ring_buffer_per_cpu *cpu_buffer;
+	unsigned long ret = 0;
+
+	get_online_cpus();
 
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
-		return 0;
+		goto out;
 
 	cpu_buffer = buffer->buffers[cpu];
-	return cpu_buffer->overrun;
+	ret = cpu_buffer->overrun;
+ out:
+	put_online_cpus();
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_overrun_cpu);
 
@@ -1603,12 +1658,16 @@ unsigned long ring_buffer_entries(struct ring_buffer *buffer)
 	unsigned long entries = 0;
 	int cpu;
 
+	get_online_cpus();
+
 	/* if you care about this being correct, lock the buffer */
 	for_each_buffer_cpu(buffer, cpu) {
 		cpu_buffer = buffer->buffers[cpu];
 		entries += cpu_buffer->entries;
 	}
 
+	put_online_cpus();
+
 	return entries;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_entries);
@@ -1626,12 +1685,16 @@ unsigned long ring_buffer_overruns(struct ring_buffer *buffer)
 	unsigned long overruns = 0;
 	int cpu;
 
+	get_online_cpus();
+
 	/* if you care about this being correct, lock the buffer */
 	for_each_buffer_cpu(buffer, cpu) {
 		cpu_buffer = buffer->buffers[cpu];
 		overruns += cpu_buffer->overrun;
 	}
 
+	put_online_cpus();
+
 	return overruns;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_overruns);
@@ -1663,9 +1726,14 @@ static void rb_iter_reset(struct ring_buffer_iter *iter)
  */
 void ring_buffer_iter_reset(struct ring_buffer_iter *iter)
 {
-	struct ring_buffer_per_cpu *cpu_buffer = iter->cpu_buffer;
+	struct ring_buffer_per_cpu *cpu_buffer;
 	unsigned long flags;
 
+	if (!iter)
+		return;
+
+	cpu_buffer = iter->cpu_buffer;
+
 	spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
 	rb_iter_reset(iter);
 	spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
@@ -1900,9 +1968,6 @@ rb_buffer_peek(struct ring_buffer *buffer, int cpu, u64 *ts)
 	struct buffer_page *reader;
 	int nr_loops = 0;
 
-	if (!cpumask_test_cpu(cpu, buffer->cpumask))
-		return NULL;
-
 	cpu_buffer = buffer->buffers[cpu];
 
  again:
@@ -2028,13 +2093,21 @@ struct ring_buffer_event *
 ring_buffer_peek(struct ring_buffer *buffer, int cpu, u64 *ts)
 {
 	struct ring_buffer_per_cpu *cpu_buffer = buffer->buffers[cpu];
-	struct ring_buffer_event *event;
+	struct ring_buffer_event *event = NULL;
 	unsigned long flags;
 
+	get_online_cpus();
+
+	if (!cpumask_test_cpu(cpu, buffer->cpumask))
+		goto out;
+
 	spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
 	event = rb_buffer_peek(buffer, cpu, ts);
 	spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
 
+ out:
+	put_online_cpus();
+
 	return event;
 }
 
@@ -2071,24 +2144,31 @@ ring_buffer_iter_peek(struct ring_buffer_iter *iter, u64 *ts)
 struct ring_buffer_event *
 ring_buffer_consume(struct ring_buffer *buffer, int cpu, u64 *ts)
 {
-	struct ring_buffer_per_cpu *cpu_buffer = buffer->buffers[cpu];
-	struct ring_buffer_event *event;
+	struct ring_buffer_per_cpu *cpu_buffer;
+	struct ring_buffer_event *event = NULL;
 	unsigned long flags;
 
+	/* might be called in atomic */
+	preempt_disable();
+
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
-		return NULL;
+		goto out;
 
+	cpu_buffer = buffer->buffers[cpu];
 	spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
 
 	event = rb_buffer_peek(buffer, cpu, ts);
 	if (!event)
-		goto out;
+		goto out_unlock;
 
 	rb_advance_reader(cpu_buffer);
 
- out:
+ out_unlock:
 	spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
 
+ out:
+	preempt_enable();
+
 	return event;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_consume);
@@ -2109,15 +2189,17 @@ struct ring_buffer_iter *
 ring_buffer_read_start(struct ring_buffer *buffer, int cpu)
 {
 	struct ring_buffer_per_cpu *cpu_buffer;
-	struct ring_buffer_iter *iter;
+	struct ring_buffer_iter *iter = NULL;
 	unsigned long flags;
 
+	get_online_cpus();
+
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
-		return NULL;
+		goto out;
 
 	iter = kmalloc(sizeof(*iter), GFP_KERNEL);
 	if (!iter)
-		return NULL;
+		goto out;
 
 	cpu_buffer = buffer->buffers[cpu];
 
@@ -2132,6 +2214,9 @@ ring_buffer_read_start(struct ring_buffer *buffer, int cpu)
 	__raw_spin_unlock(&cpu_buffer->lock);
 	spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
 
+ out:
+	put_online_cpus();
+
 	return iter;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_read_start);
@@ -2224,9 +2309,13 @@ void ring_buffer_reset_cpu(struct ring_buffer *buffer, int cpu)
 {
 	struct ring_buffer_per_cpu *cpu_buffer = buffer->buffers[cpu];
 	unsigned long flags;
+	int resched;
+
+	/* Can't use get_online_cpus because this can be in atomic */
+	resched = ftrace_preempt_disable();
 
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
-		return;
+		goto out;
 
 	spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
 
@@ -2237,6 +2326,8 @@ void ring_buffer_reset_cpu(struct ring_buffer *buffer, int cpu)
 	__raw_spin_unlock(&cpu_buffer->lock);
 
 	spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
+ out:
+	ftrace_preempt_enable(resched);
 }
 EXPORT_SYMBOL_GPL(ring_buffer_reset_cpu);
 
@@ -2246,10 +2337,16 @@ EXPORT_SYMBOL_GPL(ring_buffer_reset_cpu);
  */
 void ring_buffer_reset(struct ring_buffer *buffer)
 {
+	int resched;
 	int cpu;
 
+	/* Can't use get_online_cpus because this can be in atomic */
+	resched = ftrace_preempt_disable();
+
 	for_each_buffer_cpu(buffer, cpu)
 		ring_buffer_reset_cpu(buffer, cpu);
+
+	ftrace_preempt_enable(resched);
 }
 EXPORT_SYMBOL_GPL(ring_buffer_reset);
 
@@ -2262,12 +2359,17 @@ int ring_buffer_empty(struct ring_buffer *buffer)
 	struct ring_buffer_per_cpu *cpu_buffer;
 	int cpu;
 
+	get_online_cpus();
+
 	/* yes this is racy, but if you don't like the race, lock the buffer */
 	for_each_buffer_cpu(buffer, cpu) {
 		cpu_buffer = buffer->buffers[cpu];
 		if (!rb_per_cpu_empty(cpu_buffer))
 			return 0;
 	}
+
+	put_online_cpus();
+
 	return 1;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_empty);
@@ -2280,12 +2382,20 @@ EXPORT_SYMBOL_GPL(ring_buffer_empty);
 int ring_buffer_empty_cpu(struct ring_buffer *buffer, int cpu)
 {
 	struct ring_buffer_per_cpu *cpu_buffer;
+	int ret = 1;
+
+	get_online_cpus();
 
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
-		return 1;
+		goto out;
 
 	cpu_buffer = buffer->buffers[cpu];
-	return rb_per_cpu_empty(cpu_buffer);
+	ret = rb_per_cpu_empty(cpu_buffer);
+
+ out:
+	put_online_cpus();
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_empty_cpu);
 
@@ -2304,32 +2414,37 @@ int ring_buffer_swap_cpu(struct ring_buffer *buffer_a,
 {
 	struct ring_buffer_per_cpu *cpu_buffer_a;
 	struct ring_buffer_per_cpu *cpu_buffer_b;
+	int ret = -EINVAL;
+
+	get_online_cpus();
 
 	if (!cpumask_test_cpu(cpu, buffer_a->cpumask) ||
 	    !cpumask_test_cpu(cpu, buffer_b->cpumask))
-		return -EINVAL;
+		goto out;
 
 	/* At least make sure the two buffers are somewhat the same */
 	if (buffer_a->pages != buffer_b->pages)
-		return -EINVAL;
+		goto out;
+
+	ret = -EAGAIN;
 
 	if (ring_buffer_flags != RB_BUFFERS_ON)
-		return -EAGAIN;
+		goto out;
 
 	if (atomic_read(&buffer_a->record_disabled))
-		return -EAGAIN;
+		goto out;
 
 	if (atomic_read(&buffer_b->record_disabled))
-		return -EAGAIN;
+		goto out;
 
 	cpu_buffer_a = buffer_a->buffers[cpu];
 	cpu_buffer_b = buffer_b->buffers[cpu];
 
 	if (atomic_read(&cpu_buffer_a->record_disabled))
-		return -EAGAIN;
+		goto out;
 
 	if (atomic_read(&cpu_buffer_b->record_disabled))
-		return -EAGAIN;
+		goto out;
 
 	/*
 	 * We can't do a synchronize_sched here because this
@@ -2349,7 +2464,11 @@ int ring_buffer_swap_cpu(struct ring_buffer *buffer_a,
 	atomic_dec(&cpu_buffer_a->record_disabled);
 	atomic_dec(&cpu_buffer_b->record_disabled);
 
-	return 0;
+	ret = 0;
+out:
+	put_online_cpus();
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_swap_cpu);
 
@@ -2464,27 +2583,32 @@ int ring_buffer_read_page(struct ring_buffer *buffer,
 	u64 save_timestamp;
 	int ret = -1;
 
+	get_online_cpus();
+
+	if (!cpumask_test_cpu(cpu, buffer->cpumask))
+		goto out;
+
 	/*
 	 * If len is not big enough to hold the page header, then
 	 * we can not copy anything.
 	 */
 	if (len <= BUF_PAGE_HDR_SIZE)
-		return -1;
+		goto out;
 
 	len -= BUF_PAGE_HDR_SIZE;
 
 	if (!data_page)
-		return -1;
+		goto out;
 
 	bpage = *data_page;
 	if (!bpage)
-		return -1;
+		goto out;
 
 	spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
 
 	reader = rb_get_reader_page(cpu_buffer);
 	if (!reader)
-		goto out;
+		goto out_unlock;
 
 	event = rb_reader_event(cpu_buffer);
 
@@ -2506,7 +2630,7 @@ int ring_buffer_read_page(struct ring_buffer *buffer,
 		unsigned int size;
 
 		if (full)
-			goto out;
+			goto out_unlock;
 
 		if (len > (commit - read))
 			len = (commit - read);
@@ -2514,7 +2638,7 @@ int ring_buffer_read_page(struct ring_buffer *buffer,
 		size = rb_event_length(event);
 
 		if (len < size)
-			goto out;
+			goto out_unlock;
 
 		/* save the current timestamp, since the user will need it */
 		save_timestamp = cpu_buffer->read_stamp;
@@ -2553,9 +2677,12 @@ int ring_buffer_read_page(struct ring_buffer *buffer,
 	}
 	ret = read;
 
- out:
+ out_unlock:
 	spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
 
+ out:
+	put_online_cpus();
+
 	return ret;
 }
 
@@ -2629,3 +2756,42 @@ static __init int rb_init_debugfs(void)
 }
 
 fs_initcall(rb_init_debugfs);
+
+#ifdef CONFIG_HOTPLUG
+static int __cpuinit rb_cpu_notify(struct notifier_block *self,
+				   unsigned long action, void *hcpu)
+{
+	struct ring_buffer *buffer =
+		container_of(self, struct ring_buffer, cpu_notify);
+	long cpu = (long)hcpu;
+
+	switch (action) {
+	case CPU_UP_PREPARE:
+	case CPU_UP_PREPARE_FROZEN:
+		if (cpu_isset(cpu, *buffer->cpumask))
+			return NOTIFY_OK;
+
+		buffer->buffers[cpu] =
+			rb_allocate_cpu_buffer(buffer, cpu);
+		if (!buffer->buffers[cpu]) {
+			WARN(1, "failed to allocate ring buffer on CPU %ld\n",
+			     cpu);
+			return NOTIFY_OK;
+		}
+		smp_wmb();
+		cpu_set(cpu, *buffer->cpumask);
+		break;
+	case CPU_DOWN_PREPARE:
+	case CPU_DOWN_PREPARE_FROZEN:
+		/*
+		 * Do nothing.
+		 *  If we were to free the buffer, then the user would
+		 *  lose any trace that was in the buffer.
+		 */
+		break;
+	default:
+		break;
+	}
+	return NOTIFY_OK;
+}
+#endif
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index e60f4be..14c98f6 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1805,17 +1805,11 @@ __tracing_open(struct inode *inode, struct file *file)
 
 			iter->buffer_iter[cpu] =
 				ring_buffer_read_start(iter->tr->buffer, cpu);
-
-			if (!iter->buffer_iter[cpu])
-				goto fail_buffer;
 		}
 	} else {
 		cpu = iter->cpu_file;
 		iter->buffer_iter[cpu] =
 				ring_buffer_read_start(iter->tr->buffer, cpu);
-
-		if (!iter->buffer_iter[cpu])
-			goto fail;
 	}
 
 	/* TODO stop tracer */
-- 
1.6.1.3

-- 

Re: [PATCH 1/4] tracing: keep ring buffer to minimum size till used

[Andrew Morton]

On Wed, 11 Mar 2009 22:37:21 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <srostedt@redhat.com>
> 
> Impact: less memory impact on systems not using tracer
> 
> When the kernel boots up that has tracing configured, it allocates
> the default size of the ring buffer. This currently happens to be
> 1.4Megs per possible CPU. This is quite a bit of wasted memory if
> the system is never using the tracer.
> 
> The current solution is to keep the ring buffers to a minimum size
> until the user uses them. Once a tracer is piped into the current_tracer
> the ring buffer will be expanded to the default size. If the user
> changes the size of the ring buffer, it will take the size given
> by the user immediately.
> 
> If the user adds a "ftrace=" to the kernel command line, then the ring
> buffers will be set to the default size on initialization.
> 
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
>  kernel/trace/trace.c |   79 +++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 59 insertions(+), 20 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 4c97947..0c1dc18 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -45,6 +45,12 @@ unsigned long __read_mostly	tracing_max_latency;
>  unsigned long __read_mostly	tracing_thresh;
>  
>  /*
> + * On boot up, the ring buffer is set to the minimum size, so that
> + * we do not waste memory on systems that are not using tracing.
> + */
> +static int ring_buffer_expanded;
> +
> +/*
>   * We need to change this state when a selftest is running.
>   * A selftest will lurk into the ring-buffer to count the
>   * entries inserted during the selftest although some concurrent
> @@ -128,6 +134,8 @@ static int __init set_ftrace(char *str)
>  {
>  	strncpy(bootup_tracer_buf, str, BOOTUP_TRACER_SIZE);
>  	default_bootup_tracer = bootup_tracer_buf;
> +	/* We are using ftrace early, expand it */
> +	ring_buffer_expanded = 1;
>  	return 1;
>  }
>  __setup("ftrace=", set_ftrace);
> @@ -2315,6 +2323,40 @@ int tracer_init(struct tracer *t, struct trace_array *tr)
>  	return t->init(tr);
>  }
>  
> +static int tracing_resize_ring_buffer(unsigned long size)
> +{
> +	int ret;
> +
> +	/*
> +	 * If kernel or user changes the size of the ring buffer
> +	 * it get completed.
> +	 */

That comment needs help.

> +	ring_buffer_expanded = 1;
> +
> +	ret = ring_buffer_resize(global_trace.buffer, size);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ring_buffer_resize(max_tr.buffer, size);
> +	if (ret < 0) {
> +		int r;
> +
> +		r = ring_buffer_resize(global_trace.buffer,
> +				       global_trace.entries);
> +		if (r < 0) {
> +			/* AARGH! We are left with different
> +			 * size max buffer!!!! */

So does that one, but at least it's understandable ;)

> +			WARN_ON(1);
> +			tracing_disabled = 1;
> +		}
> +		return ret;
> +	}
> +
> +	global_trace.entries = size;
> +
> +	return ret;
> +}
> +
>  struct trace_option_dentry;
>  
>  static struct trace_option_dentry *
> @@ -2330,6 +2372,13 @@ static int tracing_set_tracer(const char *buf)
>  	struct tracer *t;
>  	int ret = 0;
>  
> +	if (!ring_buffer_expanded) {
> +		ret = tracing_resize_ring_buffer(trace_buf_size);
> +		if (ret < 0)
> +			return ret;
> +		ret = 0;
> +	}

This would look a bit less racy were it to happen inside the lock.

>  	mutex_lock(&trace_types_lock);
>  	for (t = trace_types; t; t = t->next) {
>  		if (strcmp(t->name, buf) == 0)

Re: [PATCH 4/4] ring-buffer: only allocate buffers for online cpus

[Andrew Morton]

On Wed, 11 Mar 2009 22:37:24 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <srostedt@redhat.com>
> 
> Impact: save on memory
> 
> Currently, a ring buffer was allocated for each "possible_cpus". On
> some systems, this is the same as NR_CPUS. Thus, if a system defined
> NR_CPUS = 64 but it only had 1 CPU, we could have possibly 63 useless
> ring buffers taking up space. With a default buffer of 3 megs, this
> could be quite drastic.
> 
> This patch changes the ring buffer code to only allocate ring buffers
> for online CPUs.  If a CPU goes off line, we do not free the buffer.
> This is because the user may still have trace data in that buffer
> that they would like to look at.
> 
> Perhaps in the future we could add code to delete a ring buffer if
> the CPU is offline and the ring buffer becomes empty.
> 
> ...
>
> +#ifdef CONFIG_HOTPLUG
> +#ifdef CONFIG_HOTPLUG
> +#ifdef CONFIG_HOTPLUG
> +#ifdef CONFIG_HOTPLUG
> +#ifdef CONFIG_HOTPLUG

CONFIG_HOTPLUG_CPU

Re: [PATCH 0/4] [GIT PULL] memory saving ring buffer for tip/tracing/ftrace

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 1172 bytes --]

On Wed, 11 Mar 2009 22:37:20 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Ingo,
> 
> This patch series implements a better memory management by the
> ring buffer. It only allocates the buffers for the online CPUS, as well
> as keeps the buffers to a minimum size until they are used.
> 
> Please pull the latest tip/tracing/ftrace tree, which can be found at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> tip/tracing/ftrace
> 
> 
> Steven Rostedt (4):
>       tracing: keep ring buffer to minimum size till used
>       tracing: expand the ring buffers when an event is activated
>       tracing: fix trace_wait to know to wait on all cpus or just one
>       ring-buffer: only allocate buffers for online cpus
> 

How difficult is it to apply this to 2.6.27? Fedora is still on that
kernel and it would be nice if I could get my memory back on a
distribution kernel. :)

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 4/4] ring-buffer: only allocate buffers for online cpus

[KOSAKI Motohiro]

> From: Steven Rostedt <srostedt@redhat.com>
> 
> Impact: save on memory
> 
> Currently, a ring buffer was allocated for each "possible_cpus". On
> some systems, this is the same as NR_CPUS. Thus, if a system defined
> NR_CPUS = 64 but it only had 1 CPU, we could have possibly 63 useless
> ring buffers taking up space. With a default buffer of 3 megs, this
> could be quite drastic.
> 
> This patch changes the ring buffer code to only allocate ring buffers
> for online CPUs.  If a CPU goes off line, we do not free the buffer.
> This is because the user may still have trace data in that buffer
> that they would like to look at.
> 
> Perhaps in the future we could add code to delete a ring buffer if
> the CPU is offline and the ring buffer becomes empty.

I don't like this patch.
your [1/4] and [2/4] already solve Pierre's problem.

using online cpu (not possible cpu) increase performance overhead 
and messiness.
but nobody get benefit ;)

Re: [PATCH 0/4] [GIT PULL] memory saving ring buffer for tip/tracing/ftrace

[KOSAKI Motohiro]

> How difficult is it to apply this to 2.6.27? Fedora is still on that
> kernel and it would be nice if I could get my memory back on a
> distribution kernel. :)

Difficult.
but you can use "trace_entries=1" kernel boot parameter :)

Re: [PATCH 0/4] [GIT PULL] memory saving ring buffer for tip/tracing/ftrace

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]

On Thu, 12 Mar 2009 16:34:55 +0900 (JST)
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> > How difficult is it to apply this to 2.6.27? Fedora is still on that
> > kernel and it would be nice if I could get my memory back on a
> > distribution kernel. :)
> 
> Difficult.
> but you can use "trace_entries=1" kernel boot parameter :)
> 

Ah. I got a tip for trace_buf_size=1, but that didn't work. Your
version was much better. :)

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/4] [GIT PULL] memory saving ring buffer for tip/tracing/ftrace

[Neil Horman]

On Thu, Mar 12, 2009 at 08:07:58AM +0100, Pierre Ossman wrote:
> On Wed, 11 Mar 2009 22:37:20 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > Ingo,
> > 
> > This patch series implements a better memory management by the
> > ring buffer. It only allocates the buffers for the online CPUS, as well
> > as keeps the buffers to a minimum size until they are used.
> > 
> > Please pull the latest tip/tracing/ftrace tree, which can be found at:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> > tip/tracing/ftrace
> > 
> > 
> > Steven Rostedt (4):
> >       tracing: keep ring buffer to minimum size till used
> >       tracing: expand the ring buffers when an event is activated
> >       tracing: fix trace_wait to know to wait on all cpus or just one
> >       ring-buffer: only allocate buffers for online cpus
> > 
> 
> How difficult is it to apply this to 2.6.27? Fedora is still on that
> kernel and it would be nice if I could get my memory back on a
> distribution kernel. :)

I'm looking into it currently.  I'll post a test kernel when I have something
for you.
Neil

Re: [PATCH 0/4] [GIT PULL] memory saving ring buffer for tip/tracing/ftrace

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]

On Thu, 12 Mar 2009 06:47:21 -0400
Neil Horman <nhorman@redhat.com> wrote:

> On Thu, Mar 12, 2009 at 08:07:58AM +0100, Pierre Ossman wrote:
> > 
> > How difficult is it to apply this to 2.6.27? Fedora is still on that
> > kernel and it would be nice if I could get my memory back on a
> > distribution kernel. :)
> 
> I'm looking into it currently.  I'll post a test kernel when I have something
> for you.

F9 kernel if possible, as that's what the test machine is running.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/4] [GIT PULL] memory saving ring buffer for tip/tracing/ftrace

[Neil Horman]

On Thu, Mar 12, 2009 at 01:12:07PM +0100, Pierre Ossman wrote:
> On Thu, 12 Mar 2009 06:47:21 -0400
> Neil Horman <nhorman@redhat.com> wrote:
> 
> > On Thu, Mar 12, 2009 at 08:07:58AM +0100, Pierre Ossman wrote:
> > > 
> > > How difficult is it to apply this to 2.6.27? Fedora is still on that
> > > kernel and it would be nice if I could get my memory back on a
> > > distribution kernel. :)
> > 
> > I'm looking into it currently.  I'll post a test kernel when I have something
> > for you.
> 
> F9 kernel if possible, as that's what the test machine is running.
> 
I doubt I'll get an update into F9, given that it will go end of life soon, but
I'll try
Neil

> Rgds
> -- 
>      -- Pierre Ossman
> 
>   WARNING: This correspondence is being monitored by the
>   Swedish government. Make sure your server uses encryption
>   for SMTP traffic and consider using PGP for end-to-end
>   encryption.

Re: [PATCH 1/4] tracing: keep ring buffer to minimum size till used

[Steven Rostedt]

On Wed, 11 Mar 2009, Andrew Morton wrote:

> On Wed, 11 Mar 2009 22:37:21 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: Steven Rostedt <srostedt@redhat.com>
> > 
> > Impact: less memory impact on systems not using tracer
> > 
> > When the kernel boots up that has tracing configured, it allocates
> > the default size of the ring buffer. This currently happens to be
> > 1.4Megs per possible CPU. This is quite a bit of wasted memory if
> > the system is never using the tracer.
> > 
> > The current solution is to keep the ring buffers to a minimum size
> > until the user uses them. Once a tracer is piped into the current_tracer
> > the ring buffer will be expanded to the default size. If the user
> > changes the size of the ring buffer, it will take the size given
> > by the user immediately.
> > 
> > If the user adds a "ftrace=" to the kernel command line, then the ring
> > buffers will be set to the default size on initialization.
> > 
> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> > ---
> >  kernel/trace/trace.c |   79 +++++++++++++++++++++++++++++++++++++------------
> >  1 files changed, 59 insertions(+), 20 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 4c97947..0c1dc18 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -45,6 +45,12 @@ unsigned long __read_mostly	tracing_max_latency;
> >  unsigned long __read_mostly	tracing_thresh;
> >  
> >  /*
> > + * On boot up, the ring buffer is set to the minimum size, so that
> > + * we do not waste memory on systems that are not using tracing.
> > + */
> > +static int ring_buffer_expanded;
> > +
> > +/*
> >   * We need to change this state when a selftest is running.
> >   * A selftest will lurk into the ring-buffer to count the
> >   * entries inserted during the selftest although some concurrent
> > @@ -128,6 +134,8 @@ static int __init set_ftrace(char *str)
> >  {
> >  	strncpy(bootup_tracer_buf, str, BOOTUP_TRACER_SIZE);
> >  	default_bootup_tracer = bootup_tracer_buf;
> > +	/* We are using ftrace early, expand it */
> > +	ring_buffer_expanded = 1;
> >  	return 1;
> >  }
> >  __setup("ftrace=", set_ftrace);
> > @@ -2315,6 +2323,40 @@ int tracer_init(struct tracer *t, struct trace_array *tr)
> >  	return t->init(tr);
> >  }
> >  
> > +static int tracing_resize_ring_buffer(unsigned long size)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * If kernel or user changes the size of the ring buffer
> > +	 * it get completed.
> > +	 */
> 
> That comment needs help.

Hmm, I wonder what kind of drugs I was on when I wrote that ;-)
Seriously, I think that was a cut and paste error. Looks like I some
how killed a line :-/

> 
> > +	ring_buffer_expanded = 1;
> > +
> > +	ret = ring_buffer_resize(global_trace.buffer, size);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = ring_buffer_resize(max_tr.buffer, size);
> > +	if (ret < 0) {
> > +		int r;
> > +
> > +		r = ring_buffer_resize(global_trace.buffer,
> > +				       global_trace.entries);
> > +		if (r < 0) {
> > +			/* AARGH! We are left with different
> > +			 * size max buffer!!!! */
> 
> So does that one, but at least it's understandable ;)
> 
> > +			WARN_ON(1);
> > +			tracing_disabled = 1;
> > +		}
> > +		return ret;
> > +	}
> > +
> > +	global_trace.entries = size;
> > +
> > +	return ret;
> > +}
> > +
> >  struct trace_option_dentry;
> >  
> >  static struct trace_option_dentry *
> > @@ -2330,6 +2372,13 @@ static int tracing_set_tracer(const char *buf)
> >  	struct tracer *t;
> >  	int ret = 0;
> >  
> > +	if (!ring_buffer_expanded) {
> > +		ret = tracing_resize_ring_buffer(trace_buf_size);
> > +		if (ret < 0)
> > +			return ret;
> > +		ret = 0;
> > +	}
> 
> This would look a bit less racy were it to happen inside the lock.

Agreed.

Thanks, I'll go make the updates.

-- Steve

> 
> >  	mutex_lock(&trace_types_lock);
> >  	for (t = trace_types; t; t = t->next) {
> >  		if (strcmp(t->name, buf) == 0)
> 
> 

Re: [PATCH 4/4] ring-buffer: only allocate buffers for online cpus

[Steven Rostedt]

On Wed, 11 Mar 2009, Andrew Morton wrote:

> On Wed, 11 Mar 2009 22:37:24 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: Steven Rostedt <srostedt@redhat.com>
> > 
> > Impact: save on memory
> > 
> > Currently, a ring buffer was allocated for each "possible_cpus". On
> > some systems, this is the same as NR_CPUS. Thus, if a system defined
> > NR_CPUS = 64 but it only had 1 CPU, we could have possibly 63 useless
> > ring buffers taking up space. With a default buffer of 3 megs, this
> > could be quite drastic.
> > 
> > This patch changes the ring buffer code to only allocate ring buffers
> > for online CPUs.  If a CPU goes off line, we do not free the buffer.
> > This is because the user may still have trace data in that buffer
> > that they would like to look at.
> > 
> > Perhaps in the future we could add code to delete a ring buffer if
> > the CPU is offline and the ring buffer becomes empty.
> > 
> > ...
> >
> > +#ifdef CONFIG_HOTPLUG
> > +#ifdef CONFIG_HOTPLUG
> > +#ifdef CONFIG_HOTPLUG
> > +#ifdef CONFIG_HOTPLUG
> > +#ifdef CONFIG_HOTPLUG
> 
> CONFIG_HOTPLUG_CPU

Oops, well I did have both in my config to 'y'.

Thanks, will fix.

-- Steve

Re: [PATCH 4/4] ring-buffer: only allocate buffers for online cpus

[Steven Rostedt]

On Thu, 12 Mar 2009, KOSAKI Motohiro wrote:

> > From: Steven Rostedt <srostedt@redhat.com>
> > 
> > Impact: save on memory
> > 
> > Currently, a ring buffer was allocated for each "possible_cpus". On
> > some systems, this is the same as NR_CPUS. Thus, if a system defined
> > NR_CPUS = 64 but it only had 1 CPU, we could have possibly 63 useless
> > ring buffers taking up space. With a default buffer of 3 megs, this
> > could be quite drastic.
> > 
> > This patch changes the ring buffer code to only allocate ring buffers
> > for online CPUs.  If a CPU goes off line, we do not free the buffer.
> > This is because the user may still have trace data in that buffer
> > that they would like to look at.
> > 
> > Perhaps in the future we could add code to delete a ring buffer if
> > the CPU is offline and the ring buffer becomes empty.
> 
> I don't like this patch.
> your [1/4] and [2/4] already solve Pierre's problem.
> 
> using online cpu (not possible cpu) increase performance overhead 
> and messiness.
> but nobody get benefit ;)

Well, the fact that you can have 15 buffers for non existent CPUs is a big 
benefit. And the overhead was only on the read side, not the write, and 
very limited in overhead for that matter.

But, looking at this, I realized I can get rid of all the 
"get_online_cpus". I originally had the CPU_DOWN_PREPARE remove the 
buffer. But I found it highly annoying during tests, that I lose my data 
when I brought down a CPU. Thus, I removed the code to free the buffer and 
replaced it with the comment explaining this.

The get_online_cpus is to prevent the race where we might remove a buffer. 
But since we do not do that anymore, those get_online_cpus are pretty 
useless.

I'll update the code in a bit.

-- Steve

Re: [PATCH 0/4] [GIT PULL] memory saving ring buffer for tip/tracing/ftrace

[Steven Rostedt]

On Thu, 12 Mar 2009, Pierre Ossman wrote:

> On Thu, 12 Mar 2009 16:34:55 +0900 (JST)
> KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
> 
> > > How difficult is it to apply this to 2.6.27? Fedora is still on that
> > > kernel and it would be nice if I could get my memory back on a
> > > distribution kernel. :)
> > 
> > Difficult.
> > but you can use "trace_entries=1" kernel boot parameter :)
> > 
> 
> Ah. I got a tip for trace_buf_size=1, but that didn't work. Your
> version was much better. :)

Yeah, the name changed within the versions.

-- Steve

Re: [PATCH 1/4] tracing: keep ring buffer to minimum size till used

[Frederic Weisbecker]

On Wed, Mar 11, 2009 at 10:37:21PM -0400, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> Impact: less memory impact on systems not using tracer
> 
> When the kernel boots up that has tracing configured, it allocates
> the default size of the ring buffer. This currently happens to be
> 1.4Megs per possible CPU. This is quite a bit of wasted memory if
> the system is never using the tracer.
> 
> The current solution is to keep the ring buffers to a minimum size
> until the user uses them. Once a tracer is piped into the current_tracer
> the ring buffer will be expanded to the default size. If the user
> changes the size of the ring buffer, it will take the size given
> by the user immediately.
> 
> If the user adds a "ftrace=" to the kernel command line, then the ring
> buffers will be set to the default size on initialization.
> 
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
>  kernel/trace/trace.c |   79 +++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 59 insertions(+), 20 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 4c97947..0c1dc18 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -45,6 +45,12 @@ unsigned long __read_mostly	tracing_max_latency;
>  unsigned long __read_mostly	tracing_thresh;
>  
>  /*
> + * On boot up, the ring buffer is set to the minimum size, so that
> + * we do not waste memory on systems that are not using tracing.
> + */
> +static int ring_buffer_expanded;
> +
> +/*
>   * We need to change this state when a selftest is running.
>   * A selftest will lurk into the ring-buffer to count the
>   * entries inserted during the selftest although some concurrent
> @@ -128,6 +134,8 @@ static int __init set_ftrace(char *str)
>  {
>  	strncpy(bootup_tracer_buf, str, BOOTUP_TRACER_SIZE);
>  	default_bootup_tracer = bootup_tracer_buf;
> +	/* We are using ftrace early, expand it */
> +	ring_buffer_expanded = 1;
>  	return 1;
>  }
>  __setup("ftrace=", set_ftrace);
> @@ -2315,6 +2323,40 @@ int tracer_init(struct tracer *t, struct trace_array *tr)
>  	return t->init(tr);
>  }
>  
> +static int tracing_resize_ring_buffer(unsigned long size)
> +{
> +	int ret;
> +
> +	/*
> +	 * If kernel or user changes the size of the ring buffer
> +	 * it get completed.
> +	 */

Indeed :-)

I just wonder about one thing.
Sometimes I use trace_printk to trace some events that come on bootup.
The only solution I see that could avoid me this minimum size would be
to use ftrace=nop that I sometimes use to avoid ftrace selftests.

I guess that few developers will be aware of this trick so may be
it could be worth it to explain it on the ftrace documentation and
on kernel.h (though a simple ftrace_printk as a parameter would be
more obvious, or ftrace=printk).

Anyway this patch is a very good thing, especially on embeeded systems.

Frederic.

> +	ring_buffer_expanded = 1;
> +
> +	ret = ring_buffer_resize(global_trace.buffer, size);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ring_buffer_resize(max_tr.buffer, size);
> +	if (ret < 0) {
> +		int r;
> +
> +		r = ring_buffer_resize(global_trace.buffer,
> +				       global_trace.entries);
> +		if (r < 0) {
> +			/* AARGH! We are left with different
> +			 * size max buffer!!!! */
> +			WARN_ON(1);
> +			tracing_disabled = 1;
> +		}
> +		return ret;
> +	}
> +
> +	global_trace.entries = size;
> +
> +	return ret;
> +}
> +
>  struct trace_option_dentry;
>  
>  static struct trace_option_dentry *
> @@ -2330,6 +2372,13 @@ static int tracing_set_tracer(const char *buf)
>  	struct tracer *t;
>  	int ret = 0;
>  
> +	if (!ring_buffer_expanded) {
> +		ret = tracing_resize_ring_buffer(trace_buf_size);
> +		if (ret < 0)
> +			return ret;
> +		ret = 0;
> +	}
> +
>  	mutex_lock(&trace_types_lock);
>  	for (t = trace_types; t; t = t->next) {
>  		if (strcmp(t->name, buf) == 0)
> @@ -2903,28 +2952,11 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
>  	val <<= 10;
>  
>  	if (val != global_trace.entries) {
> -		ret = ring_buffer_resize(global_trace.buffer, val);
> +		ret = tracing_resize_ring_buffer(val);
>  		if (ret < 0) {
>  			cnt = ret;
>  			goto out;
>  		}
> -
> -		ret = ring_buffer_resize(max_tr.buffer, val);
> -		if (ret < 0) {
> -			int r;
> -			cnt = ret;
> -			r = ring_buffer_resize(global_trace.buffer,
> -					       global_trace.entries);
> -			if (r < 0) {
> -				/* AARGH! We are left with different
> -				 * size max buffer!!!! */
> -				WARN_ON(1);
> -				tracing_disabled = 1;
> -			}
> -			goto out;
> -		}
> -
> -		global_trace.entries = val;
>  	}
>  
>  	filp->f_pos += cnt;
> @@ -3916,6 +3948,7 @@ void ftrace_dump(void)
>  __init static int tracer_alloc_buffers(void)
>  {
>  	struct trace_array_cpu *data;
> +	int ring_buf_size;
>  	int i;
>  	int ret = -ENOMEM;
>  
> @@ -3928,12 +3961,18 @@ __init static int tracer_alloc_buffers(void)
>  	if (!alloc_cpumask_var(&tracing_reader_cpumask, GFP_KERNEL))
>  		goto out_free_tracing_cpumask;
>  
> +	/* To save memory, keep the ring buffer size to its minimum */
> +	if (ring_buffer_expanded)
> +		ring_buf_size = trace_buf_size;
> +	else
> +		ring_buf_size = 1;
> +
>  	cpumask_copy(tracing_buffer_mask, cpu_possible_mask);
>  	cpumask_copy(tracing_cpumask, cpu_all_mask);
>  	cpumask_clear(tracing_reader_cpumask);
>  
>  	/* TODO: make the number of buffers hot pluggable with CPUS */
> -	global_trace.buffer = ring_buffer_alloc(trace_buf_size,
> +	global_trace.buffer = ring_buffer_alloc(ring_buf_size,
>  						   TRACE_BUFFER_FLAGS);
>  	if (!global_trace.buffer) {
>  		printk(KERN_ERR "tracer: failed to allocate ring buffer!\n");
> @@ -3944,7 +3983,7 @@ __init static int tracer_alloc_buffers(void)
>  
>  
>  #ifdef CONFIG_TRACER_MAX_TRACE
> -	max_tr.buffer = ring_buffer_alloc(trace_buf_size,
> +	max_tr.buffer = ring_buffer_alloc(ring_buf_size,
>  					     TRACE_BUFFER_FLAGS);
>  	if (!max_tr.buffer) {
>  		printk(KERN_ERR "tracer: failed to allocate max ring buffer!\n");
> -- 
> 1.6.1.3
> 
> -- 

Re: [PATCH 1/4] tracing: keep ring buffer to minimum size till used

[Steven Rostedt]

On Thu, 12 Mar 2009, Frederic Weisbecker wrote:
> >  
> > +static int tracing_resize_ring_buffer(unsigned long size)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * If kernel or user changes the size of the ring buffer
> > +	 * it get completed.
> > +	 */
> 
> Indeed :-)

That's my imitation of Shakespeare ;-)

> 
> I just wonder about one thing.
> Sometimes I use trace_printk to trace some events that come on bootup.
> The only solution I see that could avoid me this minimum size would be
> to use ftrace=nop that I sometimes use to avoid ftrace selftests.

"Give that man a cigar!" Yep, you hit the jackpot!  I was anticipating
this question and you even gave the same answer I would. That is to
use "ftrace=nop" on the command line ;-)

> 
> I guess that few developers will be aware of this trick so may be
> it could be worth it to explain it on the ftrace documentation and
> on kernel.h (though a simple ftrace_printk as a parameter would be
> more obvious, or ftrace=printk).

Any ftrace=foobar would work. Even if foobar was not a tracer. So using 
ftrace=foobar literally, would work.

> 
> Anyway this patch is a very good thing, especially on embeeded systems.

Thanks,

-- Steve

Re: [PATCH 4/4] ring-buffer: only allocate buffers for online cpus

[KOSAKI Motohiro]

> > I don't like this patch.
> > your [1/4] and [2/4] already solve Pierre's problem.
> > 
> > using online cpu (not possible cpu) increase performance overhead 
> > and messiness.
> > but nobody get benefit ;)
> 
> Well, the fact that you can have 15 buffers for non existent CPUs is a big 
> benefit. And the overhead was only on the read side, not the write, and 
> very limited in overhead for that matter.
> 
> But, looking at this, I realized I can get rid of all the 
> "get_online_cpus". I originally had the CPU_DOWN_PREPARE remove the 
> buffer. But I found it highly annoying during tests, that I lose my data 
> when I brought down a CPU. Thus, I removed the code to free the buffer and 
> replaced it with the comment explaining this.
> 
> The get_online_cpus is to prevent the race where we might remove a buffer. 
> But since we do not do that anymore, those get_online_cpus are pretty 
> useless.
> 
> I'll update the code in a bit.

Great!

from mission critical area view, CPU down event is very important.
I don't hope to lost trace data of downed cpu.
and no get_online_cpus() don't cause annoy overhead.

I love to see your next patch :)