[PATCH] Changed blk trace msgs to directly use relay buffer

[PATCH] Changed blk trace msgs to directly use relay buffer

[Alan D. Brunelle]

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



[-- Attachment #2: 0001-Changed-blk-trace-msgs-to-directly-use-relay-buffer.patch --]
[-- Type: text/x-diff, Size: 2924 bytes --]

From 43c8ea2b78f31d7ccd349384a9a2084e787aafc1 Mon Sep 17 00:00:00 2001
From: Alan D. Brunelle <alan.brunelle@hp.com>
Date: Tue, 27 May 2008 10:32:36 -0400
Subject: [PATCH] Changed blk trace msgs to directly use relay buffer

Allows for SMP-usage without corruption, and removes an extra copy at
the expense of copying extra bytes. Reduced message size from 1024 to 128.

Signed-off-by: Alan D. Brunelle <alan.brunelle@hp.com>
---
 block/blktrace.c             |   42 ++++++++++++++++++++++++++----------------
 include/linux/blktrace_api.h |    2 +-
 2 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/block/blktrace.c b/block/blktrace.c
index 20e11f3..38e6b83 100644
--- a/block/blktrace.c
+++ b/block/blktrace.c
@@ -27,6 +27,20 @@
 
 static unsigned int blktrace_seq __read_mostly = 1;
 
+static inline void note_header(struct blk_io_trace *t, struct blk_trace *bt,
+				pid_t pid, int action, size_t len)
+{
+	const int cpu = smp_processor_id();
+
+	t->magic = BLK_IO_TRACE_MAGIC | BLK_IO_TRACE_VERSION;
+	t->time = ktime_to_ns(ktime_get());
+	t->device = bt->dev;
+	t->action = action;
+	t->pid = pid;
+	t->cpu = cpu;
+	t->pdu_len = len;
+}
+
 /*
  * Send out a notify message.
  */
@@ -37,15 +51,7 @@ static void trace_note(struct blk_trace *bt, pid_t pid, int action,
 
 	t = relay_reserve(bt->rchan, sizeof(*t) + len);
 	if (t) {
-		const int cpu = smp_processor_id();
-
-		t->magic = BLK_IO_TRACE_MAGIC | BLK_IO_TRACE_VERSION;
-		t->time = ktime_to_ns(ktime_get());
-		t->device = bt->dev;
-		t->action = action;
-		t->pid = pid;
-		t->cpu = cpu;
-		t->pdu_len = len;
+		note_header(t, bt, pid, action, len);
 		memcpy((void *) t + sizeof(*t), data, len);
 	}
 }
@@ -77,15 +83,19 @@ static void trace_note_time(struct blk_trace *bt)
 
 void __trace_note_message(struct blk_trace *bt, const char *fmt, ...)
 {
-	int n;
-	va_list args;
-	static char bt_msg_buf[BLK_TN_MAX_MSG];
+	struct blk_io_trace *t;
 
-	va_start(args, fmt);
-	n = vscnprintf(bt_msg_buf, BLK_TN_MAX_MSG, fmt, args);
-	va_end(args);
+	t = relay_reserve(bt->rchan, sizeof(*t) + BLK_TN_MAX_MSG);
+	if (t) {
+		va_list args;
+		char *bt_msg_buf = (void *) t + sizeof(*t);
+
+		va_start(args, fmt);
+		vscnprintf(bt_msg_buf, BLK_TN_MAX_MSG, fmt, args);
+		va_end(args);
 
-	trace_note(bt, 0, BLK_TN_MESSAGE, bt_msg_buf, n);
+		note_header(t, bt, 0, BLK_TN_MESSAGE, BLK_TN_MAX_MSG);
+	}
 }
 EXPORT_SYMBOL_GPL(__trace_note_message);
 
diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index b7cd8f1..3228caa 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -172,7 +172,7 @@ extern void __trace_note_message(struct blk_trace *, const char *fmt, ...);
 		if (unlikely(bt))					\
 			__trace_note_message(bt, fmt, ##__VA_ARGS__);	\
 	} while (0)
-#define BLK_TN_MAX_MSG		1024
+#define BLK_TN_MAX_MSG		128
 
 /**
  * blk_add_trace_rq - Add a trace for a request oriented action
-- 
1.5.4.3

Re: [PATCH] Changed blk trace msgs to directly use relay buffer

[Jens Axboe]

On Tue, May 27 2008, Alan D. Brunelle wrote:

> From 43c8ea2b78f31d7ccd349384a9a2084e787aafc1 Mon Sep 17 00:00:00 2001
> From: Alan D. Brunelle <alan.brunelle@hp.com>
> Date: Tue, 27 May 2008 10:32:36 -0400
> Subject: [PATCH] Changed blk trace msgs to directly use relay buffer
> 
> Allows for SMP-usage without corruption, and removes an extra copy at
> the expense of copying extra bytes. Reduced message size from 1024 to 128.

Or, alternatively, something like the below. Then we don't
unconditionally reserve and copy 128 bytes for each message, at the
cost 128 bytes per-cpu per trace.

diff --git a/block/blktrace.c b/block/blktrace.c
index 20e11f3..7ae87cc 100644
--- a/block/blktrace.c
+++ b/block/blktrace.c
@@ -79,13 +79,16 @@ void __trace_note_message(struct blk_trace *bt, const char *fmt, ...)
 {
 	int n;
 	va_list args;
-	static char bt_msg_buf[BLK_TN_MAX_MSG];
+	char *buf;
 
+	preempt_disable();
+	buf = per_cpu_ptr(bt->msg_data, smp_processor_id());
 	va_start(args, fmt);
-	n = vscnprintf(bt_msg_buf, BLK_TN_MAX_MSG, fmt, args);
+	n = vscnprintf(buf, BLK_TN_MAX_MSG, fmt, args);
 	va_end(args);
 
-	trace_note(bt, 0, BLK_TN_MESSAGE, bt_msg_buf, n);
+	trace_note(bt, 0, BLK_TN_MESSAGE, buf, n);
+	preempt_enable();
 }
 EXPORT_SYMBOL_GPL(__trace_note_message);
 
@@ -246,6 +249,7 @@ static void blk_trace_cleanup(struct blk_trace *bt)
 	debugfs_remove(bt->dropped_file);
 	blk_remove_tree(bt->dir);
 	free_percpu(bt->sequence);
+	free_percpu(bt->msg_data);
 	kfree(bt);
 }
 
@@ -360,6 +364,10 @@ int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	if (!bt->sequence)
 		goto err;
 
+	bt->msg_data = __alloc_percpu(BLK_TN_MAX_MSG);
+	if (!bt->msg_data)
+		goto err;
+
 	ret = -ENOENT;
 	dir = blk_create_tree(buts->name);
 	if (!dir)
@@ -406,6 +414,7 @@ err:
 		if (bt->dropped_file)
 			debugfs_remove(bt->dropped_file);
 		free_percpu(bt->sequence);
+		free_percpu(bt->msg_data);
 		if (bt->rchan)
 			relay_close(bt->rchan);
 		kfree(bt);
diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index b7cd8f1..e3ef903 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -121,6 +121,7 @@ struct blk_trace {
 	int trace_state;
 	struct rchan *rchan;
 	unsigned long *sequence;
+	unsigned char *msg_data;
 	u16 act_mask;
 	u64 start_lba;
 	u64 end_lba;
@@ -172,7 +173,7 @@ extern void __trace_note_message(struct blk_trace *, const char *fmt, ...);
 		if (unlikely(bt))					\
 			__trace_note_message(bt, fmt, ##__VA_ARGS__);	\
 	} while (0)
-#define BLK_TN_MAX_MSG		1024
+#define BLK_TN_MAX_MSG		128
 
 /**
  * blk_add_trace_rq - Add a trace for a request oriented action

-- 
Jens Axboe

Re: [PATCH] Changed blk trace msgs to directly use relay buffer

[Alan D. Brunelle]

Jens Axboe wrote:
> On Tue, May 27 2008, Alan D. Brunelle wrote:
> 
>> From 43c8ea2b78f31d7ccd349384a9a2084e787aafc1 Mon Sep 17 00:00:00 2001
>> From: Alan D. Brunelle <alan.brunelle@hp.com>
>> Date: Tue, 27 May 2008 10:32:36 -0400
>> Subject: [PATCH] Changed blk trace msgs to directly use relay buffer
>>
>> Allows for SMP-usage without corruption, and removes an extra copy at
>> the expense of copying extra bytes. Reduced message size from 1024 to 128.
> 
> Or, alternatively, something like the below. Then we don't
> unconditionally reserve and copy 128 bytes for each message, at the
> cost 128 bytes per-cpu per trace.

I looked into something like this, but thought the added complexity
wasn't worth it. Besides the extra per-cpu stuff, you also have an extra
memcopy involved - in my patch you print directly into the relay buffer.
I figure that /if/ copying (128-msg_size) extra bytes is too much, one
could always shrink the 128 down further. [I would think 64 bytes is
probably ok.]

I'd bet that the reduced complexity, and skipping the extra memcopy more
than offsets having to copy a few extra bytes...

Alan

Re: [PATCH] Changed blk trace msgs to directly use relay buffer

[Jens Axboe]

On Wed, May 28 2008, Alan D. Brunelle wrote:
> Jens Axboe wrote:
> > On Tue, May 27 2008, Alan D. Brunelle wrote:
> > 
> >> From 43c8ea2b78f31d7ccd349384a9a2084e787aafc1 Mon Sep 17 00:00:00 2001
> >> From: Alan D. Brunelle <alan.brunelle@hp.com>
> >> Date: Tue, 27 May 2008 10:32:36 -0400
> >> Subject: [PATCH] Changed blk trace msgs to directly use relay buffer
> >>
> >> Allows for SMP-usage without corruption, and removes an extra copy at
> >> the expense of copying extra bytes. Reduced message size from 1024 to 128.
> > 
> > Or, alternatively, something like the below. Then we don't
> > unconditionally reserve and copy 128 bytes for each message, at the
> > cost 128 bytes per-cpu per trace.
> 
> I looked into something like this, but thought the added complexity
> wasn't worth it. Besides the extra per-cpu stuff, you also have an
> extra memcopy involved - in my patch you print directly into the relay
> buffer.  I figure that /if/ copying (128-msg_size) extra bytes is too
> much, one could always shrink the 128 down further. [I would think 64
> bytes is probably ok.]
> 
> I'd bet that the reduced complexity, and skipping the extra memcopy
> more than offsets having to copy a few extra bytes...

The complexity is the same imho, both versions are fairly trivial.
I wasn't out to optimize this in a memory copy sense. To me the most
precious resource is the data stream to the app, and 128 bytes
is probably 6 times larger than the normal message would be. With
the actual trace structure, we are down to about 3 times the byte
size.

So it was just an idea, I don't care much either way. With 128 bytes,
we could just put the buffer on the stack (and still do the copy to
the relay buffer). The per-cpu buffers has the advantage that we
could grow the size easily if we wanted to.

So, given everything, which do you prefer?

-- 
Jens Axboe

Re: [PATCH] Changed blk trace msgs to directly use relay buffer

[Alan D. Brunelle]

Jens Axboe wrote:
> On Wed, May 28 2008, Alan D. Brunelle wrote:
>> Jens Axboe wrote:
>>> On Tue, May 27 2008, Alan D. Brunelle wrote:
>>>
>>>> From 43c8ea2b78f31d7ccd349384a9a2084e787aafc1 Mon Sep 17 00:00:00 2001
>>>> From: Alan D. Brunelle <alan.brunelle@hp.com>
>>>> Date: Tue, 27 May 2008 10:32:36 -0400
>>>> Subject: [PATCH] Changed blk trace msgs to directly use relay buffer
>>>>
>>>> Allows for SMP-usage without corruption, and removes an extra copy at
>>>> the expense of copying extra bytes. Reduced message size from 1024 to 128.
>>> Or, alternatively, something like the below. Then we don't
>>> unconditionally reserve and copy 128 bytes for each message, at the
>>> cost 128 bytes per-cpu per trace.
>> I looked into something like this, but thought the added complexity
>> wasn't worth it. Besides the extra per-cpu stuff, you also have an
>> extra memcopy involved - in my patch you print directly into the relay
>> buffer.  I figure that /if/ copying (128-msg_size) extra bytes is too
>> much, one could always shrink the 128 down further. [I would think 64
>> bytes is probably ok.]
>>
>> I'd bet that the reduced complexity, and skipping the extra memcopy
>> more than offsets having to copy a few extra bytes...
> 
> The complexity is the same imho, both versions are fairly trivial.
> I wasn't out to optimize this in a memory copy sense. To me the most
> precious resource is the data stream to the app, and 128 bytes
> is probably 6 times larger than the normal message would be. With
> the actual trace structure, we are down to about 3 times the byte
> size.
> 
> So it was just an idea, I don't care much either way. With 128 bytes,
> we could just put the buffer on the stack (and still do the copy to
> the relay buffer). The per-cpu buffers has the advantage that we
> could grow the size easily if we wanted to.
> 
> So, given everything, which do you prefer?
> 

Given your prioritizing of relay-copying over kernel-copying, I think
you're solution is better (and more flexible going forward).

Alan

Re: [PATCH] Changed blk trace msgs to directly use relay buffer

[Jens Axboe]

On Wed, May 28 2008, Alan D. Brunelle wrote:
> Jens Axboe wrote:
> > On Wed, May 28 2008, Alan D. Brunelle wrote:
> >> Jens Axboe wrote:
> >>> On Tue, May 27 2008, Alan D. Brunelle wrote:
> >>>
> >>>> From 43c8ea2b78f31d7ccd349384a9a2084e787aafc1 Mon Sep 17 00:00:00 2001
> >>>> From: Alan D. Brunelle <alan.brunelle@hp.com>
> >>>> Date: Tue, 27 May 2008 10:32:36 -0400
> >>>> Subject: [PATCH] Changed blk trace msgs to directly use relay buffer
> >>>>
> >>>> Allows for SMP-usage without corruption, and removes an extra copy at
> >>>> the expense of copying extra bytes. Reduced message size from 1024 to 128.
> >>> Or, alternatively, something like the below. Then we don't
> >>> unconditionally reserve and copy 128 bytes for each message, at the
> >>> cost 128 bytes per-cpu per trace.
> >> I looked into something like this, but thought the added complexity
> >> wasn't worth it. Besides the extra per-cpu stuff, you also have an
> >> extra memcopy involved - in my patch you print directly into the relay
> >> buffer.  I figure that /if/ copying (128-msg_size) extra bytes is too
> >> much, one could always shrink the 128 down further. [I would think 64
> >> bytes is probably ok.]
> >>
> >> I'd bet that the reduced complexity, and skipping the extra memcopy
> >> more than offsets having to copy a few extra bytes...
> > 
> > The complexity is the same imho, both versions are fairly trivial.
> > I wasn't out to optimize this in a memory copy sense. To me the most
> > precious resource is the data stream to the app, and 128 bytes
> > is probably 6 times larger than the normal message would be. With
> > the actual trace structure, we are down to about 3 times the byte
> > size.
> > 
> > So it was just an idea, I don't care much either way. With 128 bytes,
> > we could just put the buffer on the stack (and still do the copy to
> > the relay buffer). The per-cpu buffers has the advantage that we
> > could grow the size easily if we wanted to.
> > 
> > So, given everything, which do you prefer?
> > 
> 
> Given your prioritizing of relay-copying over kernel-copying, I think
> you're solution is better (and more flexible going forward).

OK good, it's committed and going upstream soon(ish).

-- 
Jens Axboe