Changed blk trace msgs to directly use relay buffer

PATCH In review Alan D. Brunelle 2 participants 6 messages May 27, 2008 – May 28, 2008 Block layer

Share & tools

Alan D. Brunelle

author

2008-05-27 14:36 UTC #0 lore raw

PATCH 1/1 Changed blk trace msgs to directly use relay buffer

From 43c8ea2b78f31d7ccd349384a9a2084e787aafc1 Mon Sep 17 00:00:00 2001
From: Alan D. Brunelle <[email protected]>
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 <[email protected]>

---
 block/blktrace.c             |   42 ++++++++++++++++++++++++++----------------
 include/linux/blktrace_api.h |    2 +-
 2 files changed, 27 insertions(+), 17 deletions(-)

block/blktrace.c +26 ~~-16~~

index 20e11f3..38e6b83 100644
@@ -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);

include/linux/blktrace_api.h +1 -1

index b7cd8f1..3228caa 100644
@@ -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

Signature · 1 line

1.5.4.3

Jens Axboe

2008-05-28 12:13 UTC #1 lore raw

Quoted 7 lines · Alan D. Brunelle

From 43c8ea2b78f31d7ccd349384a9a2084e787aafc1 Mon Sep 17 00:00:00 2001
From: Alan D. Brunelle <[email protected]>
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.

block/blktrace.c +12 -3

index 20e11f3..7ae87cc 100644
@@ -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);

include/linux/blktrace_api.h +2 -1

index b7cd8f1..e3ef903 100644
@@ -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

Signature · 1 line

Jens Axboe

Alan D. Brunelle

author

2008-05-28 12:26 UTC #2 lore raw

Quoted 7 lines · Jens Axboe

Quoted 5 lines · Alan D. Brunelle
From 43c8ea2b78f31d7ccd349384a9a2084e787aafc1 Mon Sep 17 00:00:00 2001
From: Alan D. Brunelle <[email protected]>
Date: Tue, 27 May 2008 10:32:36 -0400
Subject: [PATCH] Changed blk trace msgs to directly use relay buffer

Quoted 6 lines

Quoted 2 lines
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

Jens Axboe

2008-05-28 13:00 UTC #3 lore raw

Quoted 22 lines · Alan D. Brunelle

Quoted 7 lines · Jens Axboe
Quoted 5 lines · Alan D. Brunelle
From 43c8ea2b78f31d7ccd349384a9a2084e787aafc1 Mon Sep 17 00:00:00 2001
From: Alan D. Brunelle <[email protected]>
Date: Tue, 27 May 2008 10:32:36 -0400
Subject: [PATCH] Changed blk trace msgs to directly use relay buffer
Quoted 6 lines
Quoted 2 lines
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.]

Quoted 2 lines

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?

Signature · 1 line

Jens Axboe

Alan D. Brunelle

author

2008-05-28 13:22 UTC #4 lore raw

Quoted 31 lines · Jens Axboe

Quoted 20 lines · Alan D. Brunelle
Quoted 7 lines · Jens Axboe
Quoted 5 lines · Alan D. Brunelle
From 43c8ea2b78f31d7ccd349384a9a2084e787aafc1 Mon Sep 17 00:00:00 2001
From: Alan D. Brunelle <[email protected]>
Date: Tue, 27 May 2008 10:32:36 -0400
Subject: [PATCH] Changed blk trace msgs to directly use relay buffer
Quoted 5 lines
Quoted 2 lines
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.]
Quoted 2 lines
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.

Quoted 7 lines

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

Jens Axboe

2008-05-28 13:28 UTC #5 lore raw

Quoted 37 lines · Alan D. Brunelle

Quoted 36 lines · Jens Axboe
Quoted 20 lines · Alan D. Brunelle
Quoted 7 lines · Jens Axboe
Quoted 5 lines · Alan D. Brunelle
From 43c8ea2b78f31d7ccd349384a9a2084e787aafc1 Mon Sep 17 00:00:00 2001
From: Alan D. Brunelle <[email protected]>
Date: Tue, 27 May 2008 10:32:36 -0400
Subject: [PATCH] Changed blk trace msgs to directly use relay buffer
Quoted 5 lines
Quoted 2 lines
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.]
Quoted 2 lines
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.

Quoted 5 lines

Quoted 2 lines
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).

Signature · 1 line

Jens Axboe