relay-file-read-start-pos-fix.patch

This patch fixes a bug in the relay read interface causing the number of consumed bytes to be set incorrectly.

Hi David and Tom,

Quoted 2 lines · David Wilder

This patch fixes a bug in the relay read interface causing the number of consumed bytes to be set incorrectly.

Thank you. Your patch fixes one of my concerns.
However there is another bug I found.
When I use relayfs with "overwrite" mode, read() still set incorrect number of consumed bytes.
I tried to fix that. Please review it.

Signed-off-by Masami Hiramatsu <[email protected]>

---
 kernel/relay.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Index: linux-2.6.22-rc4-mm2/kernel/relay.c
===================================================================

kernel/relay.c +6 -2

@@ -812,7 +812,10 @@
 	}
 
 	buf->bytes_consumed += bytes_consumed;
-	read_subbuf = read_pos / buf->chan->subbuf_size;
+	if (!read_pos)
+		read_subbuf = buf->subbufs_consumed;
+	else
+		read_subbuf = read_pos / buf->chan->subbuf_size;
 	if (buf->bytes_consumed + buf->padding[read_subbuf] == subbuf_size) {
 		if ((read_subbuf == buf->subbufs_produced % n_subbufs) &&
 		    (buf->offset == subbuf_size))

@@ -841,8 +844,9 @@
 	}
 
 	if (unlikely(produced - consumed >= n_subbufs)) {
-		consumed = (produced / n_subbufs) * n_subbufs;
+		consumed = produced - n_subbufs + 1;
 		buf->subbufs_consumed = consumed;
+		buf->bytes_consumed = 0;
 	}
 
 	produced = (produced % n_subbufs) * subbuf_size + buf->offset;

Could you send more info on how to reproduce the problem you're seeing? And does this patch fix it?

P.S.
I attached my patch (relay-file-read-overwrite-mode-fix.patch) which fixed the problem pointed in previous mail.

Signed-off-by Masami Hiramatsu <[email protected]>

---
 kernel/relay.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Index: linux-2.6.22-rc4-mm2/kernel/relay.c
===================================================================

kernel/relay.c +6 -2

@@ -812,7 +812,10 @@
 	}
 
 	buf->bytes_consumed += bytes_consumed;
-	read_subbuf = read_pos / buf->chan->subbuf_size;
+	if (!read_pos)
+		read_subbuf = buf->subbufs_consumed % n_subbufs;
+	else
+		read_subbuf = read_pos / buf->chan->subbuf_size;
 	if (buf->bytes_consumed + buf->padding[read_subbuf] == subbuf_size) {
 		if ((read_subbuf == buf->subbufs_produced % n_subbufs) &&
 		    (buf->offset == subbuf_size))

@@ -841,8 +844,9 @@
 	}
 
 	if (unlikely(produced - consumed >= n_subbufs)) {
-		consumed = (produced / n_subbufs) * n_subbufs;
+		consumed = produced - n_subbufs + 1;
 		buf->subbufs_consumed = consumed;
+		buf->bytes_consumed = 0;
 	}
 
 	produced = (produced % n_subbufs) * subbuf_size + buf->offset;

Hi Tom,

Quoted 4 lines · Tom Zanussi

Could you send more info on how to reproduce the problem you're seeing? And does this patch fix it?

Sure, I'll explain how to reproduce it.

Since current SystemTap is not supporting "overwrite" mode, you need to apply a patch before trying to reproduce it. I already posted the patch to bugzilla. You can get it from below.
http://sourceware.org/bugzilla/attachment.cgi?id=1896&action=view

Here is an example script (fillup.stp).
----
global counter=0

probe timer.ms(1) {
       counter++;
       printf("%08d : %020d\n", counter, gettimeofday_ns());
}

----

First of all, run the script with -O (overwrite mode) flag. (For simplify my explanation, I also use -m flag here.)

$ stap -O fillup.stp -m fillup
Soon after starting, press ^\(Ctrl+\) to detach from it.

The script writes 32 bytes dummy data per 1 milli-second, so it writes about 32k bytes per 1 second.
And the default size of relay channel of systemtap is 512kB which contains 4 subbufs (each size of subbufs is 128kB). Thus, it fills the relay channel at about 16 seconds and wraparounds because it uses overwrite mode.

So, wait more than 16 seconds (for example, 18 sec), read the relay channel and count the line number. And repeat it.

$ while true; do sleep 18; \
cat /sys/kernel/debug/systemtap/fillup/trace0 | wc -l; done

Ideally, it will show the number from 12288(=3*128k/32) to 16384(=4*128k/32).

However, without my patch, it shows;
4793
5721
9818
9817
9819
13912
0
0
780
1625
5723
5721

And, with my patch;
15742
13273
14901
12430
14056
15682
13215
14840
12370
13996
15624
13154

So, I think my patch (which )fixes the problem.

Thanks,

P.S.
I attached my patch (relay-file-read-overwrite-mode-fix.patch) which fixed the problem pointed in previous mail.

Signed-off-by Masami Hiramatsu <[email protected]>

---
kernel/relay.c | 8 ++++++--

1 file changed, 6 insertions(+), 2 deletions(-)

Index: linux-2.6.22-rc4-mm2/kernel/relay.c
===================================================================

kernel/relay.c +6 -2

@@ -812,7 +812,10 @@
 	}
 
 	buf->bytes_consumed += bytes_consumed;
-	read_subbuf = read_pos / buf->chan->subbuf_size;
+	if (!read_pos)
+		read_subbuf = buf->subbufs_consumed % n_subbufs;
+	else
+		read_subbuf = read_pos / buf->chan->subbuf_size;
 	if (buf->bytes_consumed + buf->padding[read_subbuf] == subbuf_size) {
 		if ((read_subbuf == buf->subbufs_produced % n_subbufs) &&
 		    (buf->offset == subbuf_size))

@@ -841,8 +844,9 @@
 	}
 
 	if (unlikely(produced - consumed >= n_subbufs)) {
-		consumed = (produced / n_subbufs) * n_subbufs;
+		consumed = produced - n_subbufs + 1;
 		buf->subbufs_consumed = consumed;
+		buf->bytes_consumed = 0;
 	}
 
 	produced = (produced % n_subbufs) * subbuf_size + buf->offset;

Hi David and Tom,

Quoted 2 lines · David Wilder

This patch fixes a bug in the relay read interface causing the number of consumed bytes to be set incorrectly.

Thank you. Your patch fixes one of my concerns.
However there is another bug I found.
When I use relayfs with "overwrite" mode, read() still set incorrect number of consumed bytes.
I tried to fix that. Please review it.

Signed-off-by Masami Hiramatsu <[email protected]>

---
 kernel/relay.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Index: linux-2.6.22-rc4-mm2/kernel/relay.c
===================================================================

kernel/relay.c +4 -1

+	else
+		read_subbuf = read_pos / buf->chan->subbuf_size;
 	if (buf->bytes_consumed + buf->padding[read_subbuf] == subbuf_size) {
 		if ((read_subbuf == buf->subbufs_produced % n_subbufs) &&
 		    (buf->offset == subbuf_size))

@@ -841,8 +844,9 @@
 	}
 
 	if (unlikely(produced - consumed >= n_subbufs)) {
-		consumed = (produced / n_subbufs) * n_subbufs;
+		consumed = produced - n_subbufs + 1;
 		buf->subbufs_consumed = consumed;
+		buf->bytes_consumed = 0;
 	}
 
 	produced = (produced % n_subbufs) * subbuf_size + buf->offset;

Hi,

I haven't had a chance to test it myself yet, but it looks ok to me, except for one problem noted below...

Thanks for fixing it.

Quoted 11 lines

Signed-off-by Masami Hiramatsu <[email protected]>

---
 kernel/relay.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Index: linux-2.6.22-rc4-mm2/kernel/relay.c
===================================================================

kernel/relay.c line 816

Quoted 6 lines

kernel/relay.c +2 -1

@@ -812,7 +812,10 @@
 	}
 
 	buf->bytes_consumed += bytes_consumed;
-	read_subbuf = read_pos / buf->chan->subbuf_size;
+	if (!read_pos)
+		read_subbuf = buf->subbufs_consumed;

I think this should be instead: