* [PATCH] relay-file-read-start-pos-fix.patch @ 2007-06-18 4:57 David Wilder 2007-06-19 3:44 ` Masami Hiramatsu 0 siblings, 1 reply; 8+ messages in thread From: David Wilder @ 2007-06-18 4:57 UTC (permalink / raw) To: linux-kernel; +Cc: systemtap [-- Attachment #1: Type: text/plain, Size: 104 bytes --] -- David Wilder IBM Linux Technology Center Beaverton, Oregon, USA dwilder@us.ibm.com (503)578-3789 [-- Attachment #2: relay-file-read-start-pos-fix.patch --] [-- Type: text/x-patch, Size: 907 bytes --] This patch fixes a bug in the relay read interface causing the number of consumed bytes to be set incorrectly. Signed-off-by: Tom Zanussi <zanussi@us.ibm.com> Signed-off-by: David Wilder <dwilder@us.ibm.com> kernel/relay.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/kernel/relay.c b/kernel/relay.c index 4311101..e61156e 100644 --- a/kernel/relay.c +++ b/kernel/relay.c @@ -899,7 +899,10 @@ static size_t relay_file_read_start_pos(size_t read_pos, size_t read_subbuf, padding, padding_start, padding_end; size_t subbuf_size = buf->chan->subbuf_size; size_t n_subbufs = buf->chan->n_subbufs; + size_t consumed = buf->subbufs_consumed % n_subbufs; + if (!read_pos) + read_pos = consumed * subbuf_size + buf->bytes_consumed; read_subbuf = read_pos / subbuf_size; padding = buf->padding[read_subbuf]; padding_start = (read_subbuf + 1) * subbuf_size - padding; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] relay-file-read-start-pos-fix.patch 2007-06-18 4:57 [PATCH] relay-file-read-start-pos-fix.patch David Wilder @ 2007-06-19 3:44 ` Masami Hiramatsu 2007-06-19 5:36 ` Tom Zanussi 2007-06-19 16:27 ` Tom Zanussi 0 siblings, 2 replies; 8+ messages in thread From: Masami Hiramatsu @ 2007-06-19 3:44 UTC (permalink / raw) To: David Wilder, Tom Zanussi Cc: linux-kernel, systemtap, Yumiko Sugita, Satoshi Oshima, Hideo Aoki Hi David and Tom, David Wilder wrote: > 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 <masami.hiramatsu.pt@hitachi.com> --- kernel/relay.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) Index: linux-2.6.22-rc4-mm2/kernel/relay.c =================================================================== --- linux-2.6.22-rc4-mm2.orig/kernel/relay.c 2007-06-13 20:22:02.000000000 +0900 +++ linux-2.6.22-rc4-mm2/kernel/relay.c 2007-06-18 23:00:54.000000000 +0900 @@ -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; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] relay-file-read-start-pos-fix.patch 2007-06-19 3:44 ` Masami Hiramatsu @ 2007-06-19 5:36 ` Tom Zanussi 2007-06-20 8:32 ` Masami Hiramatsu 2007-06-19 16:27 ` Tom Zanussi 1 sibling, 1 reply; 8+ messages in thread From: Tom Zanussi @ 2007-06-19 5:36 UTC (permalink / raw) To: Masami Hiramatsu Cc: David Wilder, linux-kernel, systemtap, Yumiko Sugita, Satoshi Oshima, Hideo Aoki On Tue, 2007-06-19 at 12:43 +0900, Masami Hiramatsu wrote: > Hi David and Tom, > > David Wilder wrote: > > 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. Hi, Could you send more info on how to reproduce the problem you're seeing? And does this patch fix it? Thanks, Tom > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> > > --- > kernel/relay.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > Index: linux-2.6.22-rc4-mm2/kernel/relay.c > =================================================================== > --- linux-2.6.22-rc4-mm2.orig/kernel/relay.c 2007-06-13 20:22:02.000000000 +0900 > +++ linux-2.6.22-rc4-mm2/kernel/relay.c 2007-06-18 23:00:54.000000000 +0900 > @@ -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; > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] relay-file-read-start-pos-fix.patch 2007-06-19 5:36 ` Tom Zanussi @ 2007-06-20 8:32 ` Masami Hiramatsu 2007-06-20 14:52 ` Tom Zanussi 2007-06-21 18:06 ` David Wilder 0 siblings, 2 replies; 8+ messages in thread From: Masami Hiramatsu @ 2007-06-20 8:32 UTC (permalink / raw) To: Tom Zanussi Cc: David Wilder, linux-kernel, systemtap, Yumiko Sugita, Satoshi Oshima, Hideo Aoki Hi Tom, Tom Zanussi wrote: > 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 <masami.hiramatsu.pt@hitachi.com> --- kernel/relay.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) Index: linux-2.6.22-rc4-mm2/kernel/relay.c =================================================================== --- linux-2.6.22-rc4-mm2.orig/kernel/relay.c 2007-06-13 20:22:02.000000000 +0900 +++ linux-2.6.22-rc4-mm2/kernel/relay.c 2007-06-20 10:53:06.000000000 +0900 @@ -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; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] relay-file-read-start-pos-fix.patch 2007-06-20 8:32 ` Masami Hiramatsu @ 2007-06-20 14:52 ` Tom Zanussi 2007-06-21 18:06 ` David Wilder 1 sibling, 0 replies; 8+ messages in thread From: Tom Zanussi @ 2007-06-20 14:52 UTC (permalink / raw) To: Masami Hiramatsu Cc: David Wilder, linux-kernel, systemtap, Yumiko Sugita, Satoshi Oshima, Hideo Aoki On Wed, 2007-06-20 at 17:31 +0900, Masami Hiramatsu wrote: [...] > 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 <masami.hiramatsu.pt@hitachi.com> > Hi, Thanks for sending the test case. I wrote my own program that tests for the same thing, and it also works fine. To make sure it doesn't break no-overwrite mode, I also ran several tests with blktrace and that looks good too. Thanks very much for analyzing the problem and providing the patch! Just to summarize - the problem this fixes is that in overwrite mode, the current read code doesn't pick up all the data it could if the whole buffer is filled - it will leave behind sub-buffers at the beginning. With this patch, the data in those sub-buffers is read as well. Acked-by: Tom Zanussi <zanussi@us.ibm.com> > --- > kernel/relay.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > Index: linux-2.6.22-rc4-mm2/kernel/relay.c > =================================================================== > --- linux-2.6.22-rc4-mm2.orig/kernel/relay.c 2007-06-13 20:22:02.000000000 +0900 > +++ linux-2.6.22-rc4-mm2/kernel/relay.c 2007-06-20 10:53:06.000000000 +0900 > @@ -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; > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] relay-file-read-start-pos-fix.patch 2007-06-20 8:32 ` Masami Hiramatsu 2007-06-20 14:52 ` Tom Zanussi @ 2007-06-21 18:06 ` David Wilder 1 sibling, 0 replies; 8+ messages in thread From: David Wilder @ 2007-06-21 18:06 UTC (permalink / raw) To: Masami Hiramatsu Cc: Tom Zanussi, linux-kernel, systemtap, Yumiko Sugita, Satoshi Oshima, Hideo Aoki Ack Works for me. Thanks. Note: Both Masami's patch and the relay-file-read-start-pos-fix.patch I posted earlier are required. Masami Hiramatsu wrote: >Hi Tom, > >Tom Zanussi wrote: > > >>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 <masami.hiramatsu.pt@hitachi.com> > >--- > kernel/relay.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > >Index: linux-2.6.22-rc4-mm2/kernel/relay.c >=================================================================== >--- linux-2.6.22-rc4-mm2.orig/kernel/relay.c 2007-06-13 20:22:02.000000000 +0900 >+++ linux-2.6.22-rc4-mm2/kernel/relay.c 2007-06-20 10:53:06.000000000 +0900 >@@ -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; > > > > > > > -- David Wilder IBM Linux Technology Center Beaverton, Oregon, USA dwilder@us.ibm.com (503)578-3789 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] relay-file-read-start-pos-fix.patch 2007-06-19 3:44 ` Masami Hiramatsu 2007-06-19 5:36 ` Tom Zanussi @ 2007-06-19 16:27 ` Tom Zanussi 2007-06-20 1:22 ` Masami Hiramatsu 1 sibling, 1 reply; 8+ messages in thread From: Tom Zanussi @ 2007-06-19 16:27 UTC (permalink / raw) To: Masami Hiramatsu Cc: David Wilder, linux-kernel, systemtap, Yumiko Sugita, Satoshi Oshima, Hideo Aoki On Tue, 2007-06-19 at 12:43 +0900, Masami Hiramatsu wrote: > Hi David and Tom, > > David Wilder wrote: > > 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. 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. > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> > > --- > kernel/relay.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > Index: linux-2.6.22-rc4-mm2/kernel/relay.c > =================================================================== > --- linux-2.6.22-rc4-mm2.orig/kernel/relay.c 2007-06-13 20:22:02.000000000 +0900 > +++ linux-2.6.22-rc4-mm2/kernel/relay.c 2007-06-18 23:00:54.000000000 +0900 > @@ -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: + read_subbuf = buf->subbufs_consumed % n_subbufs; Tom > + 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; > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] relay-file-read-start-pos-fix.patch 2007-06-19 16:27 ` Tom Zanussi @ 2007-06-20 1:22 ` Masami Hiramatsu 0 siblings, 0 replies; 8+ messages in thread From: Masami Hiramatsu @ 2007-06-20 1:22 UTC (permalink / raw) To: Tom Zanussi Cc: David Wilder, linux-kernel, systemtap, Yumiko Sugita, Satoshi Oshima, Hideo Aoki Tom Zanussi wrote: > Hi, > > I haven't had a chance to test it myself yet, but it looks ok to me, > except for one problem noted below... Hi, Thank you so much! I'm preparing how it can reproduce. I'll send it as soon as possible. > Thanks for fixing it. > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> >> >> --- >> kernel/relay.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> Index: linux-2.6.22-rc4-mm2/kernel/relay.c >> =================================================================== >> --- linux-2.6.22-rc4-mm2.orig/kernel/relay.c 2007-06-13 20:22:02.000000000 +0900 >> +++ linux-2.6.22-rc4-mm2/kernel/relay.c 2007-06-18 23:00:54.000000000 +0900 >> @@ -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: > > + read_subbuf = buf->subbufs_consumed % n_subbufs; Yes, you are right. Thank you again. -- Masami HIRAMATSU Linux Technology Center Hitachi, Ltd., Systems Development Laboratory E-mail: masami.hiramatsu.pt@hitachi.com ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2007-06-21 18:06 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-06-18 4:57 [PATCH] relay-file-read-start-pos-fix.patch David Wilder 2007-06-19 3:44 ` Masami Hiramatsu 2007-06-19 5:36 ` Tom Zanussi 2007-06-20 8:32 ` Masami Hiramatsu 2007-06-20 14:52 ` Tom Zanussi 2007-06-21 18:06 ` David Wilder 2007-06-19 16:27 ` Tom Zanussi 2007-06-20 1:22 ` Masami Hiramatsu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).