Re: [PATCH] bpf is now blocking again with and without timeout
On 21 Jan 2015, at 5:50 am, Simon Mages mages.si...@googlemail.com wrote:
Sorry, i did not test the Patch well enough.
It is still broken, but in a different way.
I think tedu forgot in his patch to reset d-bd_rdStart. From my point
of view it
should be zero after sleeping in this case. Because if you read again after a
successful read the timeout wont be processed because of:
/*
* If there's a timeout, bd_rdStart is tagged when we start the read.
* we can then figure out when we're done reading.
*/
if (d-bd_rtout != -1 d-bd_rdStart == 0)
d-bd_rdStart = ticks;
else
d-bd_rdStart = 0;
And zero is all the time smaller then the elapsed time in the second read.
This would fix it:
if (elapsed d-bd_rtout) {
error = tsleep(d, PRINET|PCATCH, bpf,
d-bd_rtout - elapsed);
+ d-bd_rdStart = 0;
} else
error = EWOULDBLOCK;
yes, that makes sense to me.
ill commit it when im back at work (tuesday the 27th around 11am gmt+10) unless
someone objects. or beats me too it.
dlg
BR
Simon
2015-01-07 9:11 GMT+01:00, Simon Mages mages.si...@googlemail.com:
I tested the patch and its working.
I have a small test program already. I create a regression test with it.
I'll post the diff here.
Am 06.01.2015 04:19 schrieb Philip Guenther guent...@gmail.com:
[(@#*$(*# control-enter keybinding]
On Mon, Jan 5, 2015 at 7:15 PM, Philip Guenther guent...@gmail.com
wrote:
On Mon, Jan 5, 2015 at 11:01 AM, Ted Unangst t...@tedunangst.com
wrote:
...
In the regular timeout case, I'm not sure what you're changing. There
is a problem here though. If we're already close to the timeout
expiring, we shouldn't sleep the full timeout, only the time left
since we began the read.
Yes, that was what I was trying to convey in my reply to Mages's
earlier post on this bpf issue.
Your diff looks correct to me, though untested.
Mages, do you have code this can be tested against? Is there
something you could contribute to form a regress test we could place
under /usr/src/regress/net/ to verify that we got this right and to
catch breakage in the future?
Philip Guenther
Re: [PATCH] bpf is now blocking again with and without timeout
Is everything right with my regression test?
2015-01-21 15:28 GMT+01:00, Simon Mages mages.si...@googlemail.com:
btw. here is my regression test for bpf:
Index: regress/sys/net/Makefile
===
RCS file: /home/cvs/src/regress/sys/net/Makefile,v
retrieving revision 1.6
diff -u -p -r1.6 Makefile
--- regress/sys/net/Makefile 12 Jul 2014 21:41:49 - 1.6
+++ regress/sys/net/Makefile 21 Jan 2015 13:54:05 -
@@ -1,6 +1,6 @@
#$OpenBSD: Makefile,v 1.6 2014/07/12 21:41:49 bluhm Exp $
-SUBDIR +=pf_divert pf_forward pf_fragment
+SUBDIR +=pf_divert pf_forward pf_fragment bpf_features
.MAIN: regress
Index: regress/sys/net/bpf_features/Makefile
===
RCS file: regress/sys/net/bpf_features/Makefile
diff -N regress/sys/net/bpf_features/Makefile
--- /dev/null 1 Jan 1970 00:00:00 -
+++ regress/sys/net/bpf_features/Makefile 21 Jan 2015 14:21:19 -
@@ -0,0 +1,46 @@
+.MAIN: all
+
+CDIAGFLAGS = -std=c99 -Werror -Wall -Wstrict-prototypes \
+ -Wmissing-prototypes -Wno-main -Wno-uninitialized \
+ -Wbad-function-cast -Wcast-align -Wcast-qual \
+ -Wextra -Wmissing-declarations -Wpointer-arith -Wshadow \
+ -Wsign-compare -Wuninitialized -Wunused -Wno-unused-parameter \
+ -Wnested-externs -Wunreachable-code -Winline \
+ -Wdisabled-optimization -Wconversion -Wfloat-equal -Wswitch \
+ -Wswitch-default -Wtrigraphs -Wsequence-point -Wimplicit \
+WARNINGS = yes
+depend: bpf_read_blocking bpf_read_timeout bpf_read_async
bpf_read_timeout_loop
+
+bpf_read_blocking: bpf_read_blocking.c
+ ${CC} ${CFLAGS} -o bpf_read_blocking bpf_read_blocking.c
+
+bpf_read_timeout: bpf_read_timeout.c
+ ${CC} ${CFLAGS} -o bpf_read_timeout bpf_read_timeout.c
+
+bpf_read_async: bpf_read_async.c
+ ${CC} ${CFLAGS} -o bpf_read_async bpf_read_async.c
+
+bpf_read_timeout_loop: bpf_read_timeout_loop.c
+ ${CC} ${CFLAGS} -o bpf_read_timeout_loop bpf_read_timeout_loop.c
+
+TARGETS += blocking timeout async timeoutloop
+
+run-regress-blocking: bpf_read_blocking
+ ./bpf_read_blocking
+
+run-regress-timeout: bpf_read_timeout
+ ./bpf_read_timeout
+
+run-regress-async: bpf_read_async
+ @/sbin/ping -c 10 127.0.0.1 /dev/null 21
+ ./bpf_read_async
+
+run-regress-timeoutloop: bpf_read_timeout_loop
+ ./bpf_read_timeout_loop
+
+REGRESS_TARGETS =${TARGETS:S/^/run-regress-/}
+
+CLEANFILES +=bpf_read_timeout bpf_read_blocking
bpf_read_async \
+ bpf_read_timeout_loop
+
+.include bsd.regress.mk
Index: regress/sys/net/bpf_features/bpf_read_async.c
===
RCS file: regress/sys/net/bpf_features/bpf_read_async.c
diff -N regress/sys/net/bpf_features/bpf_read_async.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ regress/sys/net/bpf_features/bpf_read_async.c 21 Jan 2015 12:32:01
-
@@ -0,0 +1,86 @@
+#include stdlib.h
+#include stdio.h
+#include fcntl.h
+#include unistd.h
+#include signal.h
+#include string.h
+#include errno.h
+#include err.h
+
+#include sys/types.h
+#include sys/time.h
+#include sys/ioctl.h
+#include sys/socket.h
+#include net/bpf.h
+#include net/ethertypes.h
+#include netinet/in.h
+#include net/if.h
+#include sys/time.h
+
+#define BUFFFER_SIZE 32786
+u_char buffer[BUFFFER_SIZE];
+u_int buf_size = BUFFFER_SIZE;
+
+struct bpf_program bpf_machine = {
+ 2,
+ (struct bpf_insn []){
+ BPF_STMT(BPF_LD+BPF_W+BPF_LEN, 8),
+ BPF_STMT(BPF_RET+BPF_K, 8),
+ },
+};
+struct ifreq interface;
+struct sigaction sigact;
+int fd, out, pid, flag;
+int async = 1;
+struct sigaction action;
+
+void handler(int);
+
+int
+main(void)
+{
+ action.sa_handler = handler;
+ sigemptyset(action.sa_mask);
+ action.sa_flags = 0;
+ if (sigaction(SIGIO, action, NULL) 0)
+ err(1, sigaction);
+
+ pid = getpid();
+ if (setpgrp(0, 0) 0)
+ err(1, setpgrp);
+
+ sigemptyset(action.sa_mask);
+ if (sigprocmask(SIG_SETMASK, action.sa_mask, NULL) 0)
+ err(1, sigprocmask);
+
+ if ((fd = open(/dev/bpf9, O_RDONLY)) 0)
+ err(1, open);
+
+ if (ioctl(fd, BIOCSBLEN, buf_size) 0)
+ err(1, BIOCSBLEN);
+
+ strlcpy(interface.ifr_name, lo0, sizeof(interface.ifr_name));
+ if(ioctl(fd, BIOCSETIF, interface) 0)
+ err(1, BIOCSETIF);
+
+ if (ioctl(fd, FIOSETOWN, pid) 0)
+ err(1, FIOSETOWN);
+ if (ioctl(fd, FIOASYNC, async) 0)
+ err(1, FIOASYNC);
+
+ if (ioctl(fd, BIOCSETF, bpf_machine) 0)
+ err(1, BIOCSETF);
+
+ out = read(fd, buffer, sizeof(buffer));
+ if (out 0)
+ err(1,
Re: [PATCH] bpf is now blocking again with and without timeout
I tested the patch and its working. I have a small test program already. I create a regression test with it. I'll post the diff here. Am 06.01.2015 04:19 schrieb Philip Guenther guent...@gmail.com: [(@#*$(*# control-enter keybinding] On Mon, Jan 5, 2015 at 7:15 PM, Philip Guenther guent...@gmail.com wrote: On Mon, Jan 5, 2015 at 11:01 AM, Ted Unangst t...@tedunangst.com wrote: ... In the regular timeout case, I'm not sure what you're changing. There is a problem here though. If we're already close to the timeout expiring, we shouldn't sleep the full timeout, only the time left since we began the read. Yes, that was what I was trying to convey in my reply to Mages's earlier post on this bpf issue. Your diff looks correct to me, though untested. Mages, do you have code this can be tested against? Is there something you could contribute to form a regress test we could place under /usr/src/regress/net/ to verify that we got this right and to catch breakage in the future? Philip Guenther
Re: [PATCH] bpf is now blocking again with and without timeout
[(@#*$(*# control-enter keybinding] On Mon, Jan 5, 2015 at 7:15 PM, Philip Guenther guent...@gmail.com wrote: On Mon, Jan 5, 2015 at 11:01 AM, Ted Unangst t...@tedunangst.com wrote: ... In the regular timeout case, I'm not sure what you're changing. There is a problem here though. If we're already close to the timeout expiring, we shouldn't sleep the full timeout, only the time left since we began the read. Yes, that was what I was trying to convey in my reply to Mages's earlier post on this bpf issue. Your diff looks correct to me, though untested. Mages, do you have code this can be tested against? Is there something you could contribute to form a regress test we could place under /usr/src/regress/net/ to verify that we got this right and to catch breakage in the future? Philip Guenther
Re: [PATCH] bpf is now blocking again with and without timeout
On Mon, Jan 5, 2015 at 11:01 AM, Ted Unangst t...@tedunangst.com wrote:
...
In the regular timeout case, I'm not sure what you're changing. There
is a problem here though. If we're already close to the timeout
expiring, we shouldn't sleep the full timeout, only the time left
since we began the read.
Looking at the current code, the comparison seems reversed. If we've
just started reading, ticks == rdStart, and so adding the timeout will
always be greater, not less, than the current ticks.
I believe this diff addresses all the above issues. The current code
works correctly for the rtout == 0 case, but sleeping for only the
remaining timeout is much easier done by splitting it.
Anyone else have thoughts? I get confused by this code every time I
read it, perhaps because bugs obscure what it's trying to do and what
is doing. Am I correct?
Index: bpf.c
===
RCS file: /cvs/src/sys/net/bpf.c,v
retrieving revision 1.113
diff -u -p -r1.113 bpf.c
--- bpf.c 16 Dec 2014 18:30:04 - 1.113
+++ bpf.c 5 Jan 2015 18:49:11 -
@@ -430,10 +430,13 @@ bpfread(dev_t dev, struct uio *uio, int
if (d-bd_rtout == -1) {
/* User requested non-blocking I/O */
error = EWOULDBLOCK;
+ } else if (d-bd_rtout == 0) {
+ error = tsleep(d, PRINET|PCATCH, bpf, 0);
} else {
- if ((d-bd_rdStart + d-bd_rtout) ticks) {
- error = tsleep((caddr_t)d, PRINET|PCATCH,
bpf,
- d-bd_rtout);
+ int elapsed = ticks - d-bd_rdStart;
+ if (elapsed d-bd_rtout) {
+ error = tsleep(d, PRINET|PCATCH, bpf,
+ d-bd_rtout - elapsed);
} else
error = EWOULDBLOCK;
}
Re: [PATCH] bpf is now blocking again with and without timeout
On Sun, Jan 04, 2015 at 21:06, Mages Simon wrote:
I restored the functionality according to the manpage.
That means that read() on bpf is blocking again. If a
timeout is set read() will block until the timeout is
over.
Maybe asynchronous is also broken, i will look into that later.
Thanks, but I think your diff is wrong. Here's one that should be
better.
If FIONBIO is set to 0, the timeout is set to 0, which should be
infinite. You're right this needs a special case, but it shouldn't
just wait some amount of time. It should wait forever, as requested.
In the regular timeout case, I'm not sure what you're changing. There
is a problem here though. If we're already close to the timeout
expiring, we shouldn't sleep the full timeout, only the time left
since we began the read.
Looking at the current code, the comparison seems reversed. If we've
just started reading, ticks == rdStart, and so adding the timeout will
always be greater, not less, than the current ticks.
I believe this diff addresses all the above issues. The current code
works correctly for the rtout == 0 case, but sleeping for only the
remaining timeout is much easier done by splitting it.
Anyone else have thoughts? I get confused by this code every time I
read it, perhaps because bugs obscure what it's trying to do and what
is doing. Am I correct?
Index: bpf.c
===
RCS file: /cvs/src/sys/net/bpf.c,v
retrieving revision 1.113
diff -u -p -r1.113 bpf.c
--- bpf.c 16 Dec 2014 18:30:04 - 1.113
+++ bpf.c 5 Jan 2015 18:49:11 -
@@ -430,10 +430,13 @@ bpfread(dev_t dev, struct uio *uio, int
if (d-bd_rtout == -1) {
/* User requested non-blocking I/O */
error = EWOULDBLOCK;
+ } else if (d-bd_rtout == 0) {
+ error = tsleep(d, PRINET|PCATCH, bpf, 0);
} else {
- if ((d-bd_rdStart + d-bd_rtout) ticks) {
- error = tsleep((caddr_t)d, PRINET|PCATCH, bpf,
- d-bd_rtout);
+ int elapsed = ticks - d-bd_rdStart;
+ if (elapsed d-bd_rtout) {
+ error = tsleep(d, PRINET|PCATCH, bpf,
+ d-bd_rtout - elapsed);
} else
error = EWOULDBLOCK;
}
