Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys
On Thu, 2 Aug 2012, David Xu wrote:
On 2012/8/2 3:29, Bruce Evans wrote:
On Wed, 1 Aug 2012, Giovanni Trematerra wrote:
...
[gianni@bombay] /usr/src/tools/regression/poll#./pipepoll
1..20
not ok 17 FIFO state 6a: expected POLLHUP; got POLLIN | POLLHUP
not ok 18 FIFO state 6b: poll result 0 expected 1. expected POLLHUP; got 0
not ok 19 FIFO state 6c: expected POLLHUP; got POLLIN | POLLHUP
not ok 20 FIFO state 6d: expected POLLHUP; got POLLIN | POLLHUP
As you can see, sub-tests 6c and 6d failed too on 9. So it's not a problem
with
new code though is irrelevant wrt the commit.
The failure is very differnt. Failure to clear POLLIN in 6a, 6c and 6d
is a normal bug in FreeBSD.
I have attached a patch to fix it, it should make the regression tool happy.
Is it worth to commit ?
This is your patch quoted inline:
% Index: sys_pipe.c
% ===
% --- sys_pipe.c(revision 238936)
% +++ sys_pipe.c(working copy)
% @@ -1447,7 +1447,6 @@
%
% if ((events POLLINIGNEOF) == 0) {
% if (rpipe-pipe_state PIPE_EOF) {
% - revents |= (events (POLLIN | POLLRDNORM));
% if (wpipe-pipe_present != PIPE_ACTIVE ||
% (wpipe-pipe_state PIPE_EOF))
% revents |= POLLHUP;
My old patches use this:
% Index: sys_pipe.c
% ===
% RCS file: /home/ncvs/src/sys/kern/sys_pipe.c,v
% retrieving revision 1.171
% diff -u -2 -r1.171 sys_pipe.c
% --- sys_pipe.c27 Mar 2004 19:50:22 - 1.171
% +++ sys_pipe.c13 Aug 2009 11:33:08 -
% @@ -1296,6 +1295,5 @@
% if (events (POLLIN | POLLRDNORM))
% if ((rpipe-pipe_state PIPE_DIRECTW) ||
% - (rpipe-pipe_buffer.cnt 0) ||
% - (rpipe-pipe_state PIPE_EOF))
% + (rpipe-pipe_buffer.cnt 0))
% revents |= events (POLLIN | POLLRDNORM);
%
I'm not sure if there is any difference. The pipe code seems to have
been changed to be more like the socket code.
I made similar patches for sockets (to set POLLHUP on hangup (now in
-current) and to not set POLLIN on hangup unless there is still data
to be read). I started killing POLLINIGNEOF for sockets. -current
added it for nameless pipes instead :-(. With the new fifo
implementation, POLLINIGNEOF is even more of a mistake for sockets,
but more needed for pipes since named pipes are fifos.
% Index: uipc_socket.c
% ===
% RCS file: /home/ncvs/src/sys/kern/uipc_socket.c,v
% retrieving revision 1.189
% diff -u -2 -r1.189 uipc_socket.c
% --- uipc_socket.c 24 Jun 2004 04:28:30 - 1.189
% +++ uipc_socket.c 26 Aug 2009 22:49:12 -
% @@ -1862,4 +1861,9 @@
% }
%
% +#define soreadabledata(so) \
% +(((so)-so_rcv.sb_cc 0 \
% + (so)-so_rcv.sb_cc = (so)-so_rcv.sb_lowat) || \
% + !TAILQ_EMPTY((so)-so_comp) || (so)-so_error)
% +
% int
% sopoll(struct socket *so, int events, struct ucred *active_cred,
-current already has this in a header (don't count hangup as data).
But -current still sets POLLIN for compatibility later, except in the
bogus POLLINIGNEOF case. It only uses the above change to avoid
setting POLLIN initially for hangup in the POLLINIGNEOF case.
% @@ -1869,12 +1873,7 @@
%
% if (events (POLLIN | POLLRDNORM))
% - if (soreadable(so))
% + if (soreadabledata(so))
% revents |= events (POLLIN | POLLRDNORM);
Make POLLIN actually track data, not disconnection.
%
% - if (events POLLINIGNEOF)
% - if (so-so_rcv.sb_cc = so-so_rcv.sb_lowat ||
% - !TAILQ_EMPTY(so-so_comp) || so-so_error)
% - revents |= POLLINIGNEOF;
% -
% if (events (POLLOUT | POLLWRNORM))
% if (sowriteable(so))
Start killing this.
% @@ -1885,8 +1884,15 @@
% revents |= events (POLLPRI | POLLRDBAND);
%
% + if ((events POLLINIGNEOF) == 0) {
% + if (so-so_rcv.sb_state SBS_CANTRCVMORE) {
% + if (so-so_snd.sb_state SBS_CANTSENDMORE)
% + revents |= POLLHUP;
% + else
% + revents |= events (POLLIN | POLLRDNORM);
% + }
% + }
% +
Don't completely kill POLLINIGNEOF. I forget how the 2 socket state EOF
flags work. Both this and -current set POLLHUP iff both socket state
EOF flags are set. Then this never sets POLLIN, while -current always
sets it. Both set POLLIN of SBS_CANTRCVMORE is set but SBS_CANTSENDMORE
is not set. Note that POLLIN has already been set if there is actual data,
so any setting of it here is bogus, but there seems to be a problem
when only SBS_CANTRCVMORE is set. Then !SBS_CANTSENDMORE implies that
output is still possible, so we must be able to
Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys
On 2012/8/2 16:12, Bruce Evans wrote:
On Thu, 2 Aug 2012, David Xu wrote:
On 2012/8/2 3:29, Bruce Evans wrote:
On Wed, 1 Aug 2012, Giovanni Trematerra wrote:
...
[gianni@bombay] /usr/src/tools/regression/poll#./pipepoll
1..20
not ok 17 FIFO state 6a: expected POLLHUP; got POLLIN | POLLHUP
not ok 18 FIFO state 6b: poll result 0 expected 1. expected
POLLHUP; got 0
not ok 19 FIFO state 6c: expected POLLHUP; got POLLIN | POLLHUP
not ok 20 FIFO state 6d: expected POLLHUP; got POLLIN | POLLHUP
As you can see, sub-tests 6c and 6d failed too on 9. So it's not a
problem with
new code though is irrelevant wrt the commit.
The failure is very differnt. Failure to clear POLLIN in 6a, 6c and 6d
is a normal bug in FreeBSD.
I have attached a patch to fix it, it should make the regression tool
happy.
Is it worth to commit ?
This is your patch quoted inline:
% Index: sys_pipe.c
% ===
% --- sys_pipe.c(revision 238936)
% +++ sys_pipe.c(working copy)
% @@ -1447,7 +1447,6 @@
% % if ((events POLLINIGNEOF) == 0) {
% if (rpipe-pipe_state PIPE_EOF) {
% -revents |= (events (POLLIN | POLLRDNORM));
% if (wpipe-pipe_present != PIPE_ACTIVE ||
% (wpipe-pipe_state PIPE_EOF))
% revents |= POLLHUP;
My old patches use this:
% Index: sys_pipe.c
% ===
% RCS file: /home/ncvs/src/sys/kern/sys_pipe.c,v
% retrieving revision 1.171
% diff -u -2 -r1.171 sys_pipe.c
% --- sys_pipe.c27 Mar 2004 19:50:22 -1.171
% +++ sys_pipe.c13 Aug 2009 11:33:08 -
% @@ -1296,6 +1295,5 @@
% if (events (POLLIN | POLLRDNORM))
% if ((rpipe-pipe_state PIPE_DIRECTW) ||
% -(rpipe-pipe_buffer.cnt 0) ||
% -(rpipe-pipe_state PIPE_EOF))
% +(rpipe-pipe_buffer.cnt 0))
% revents |= events (POLLIN | POLLRDNORM);
%
I'm not sure if there is any difference. The pipe code seems to have
been changed to be more like the socket code.
I made similar patches for sockets (to set POLLHUP on hangup (now in
-current) and to not set POLLIN on hangup unless there is still data
to be read). I started killing POLLINIGNEOF for sockets. -current
added it for nameless pipes instead :-(. With the new fifo
implementation, POLLINIGNEOF is even more of a mistake for sockets,
but more needed for pipes since named pipes are fifos.
% Index: uipc_socket.c
% ===
% RCS file: /home/ncvs/src/sys/kern/uipc_socket.c,v
% retrieving revision 1.189
% diff -u -2 -r1.189 uipc_socket.c
% --- uipc_socket.c24 Jun 2004 04:28:30 -1.189
% +++ uipc_socket.c26 Aug 2009 22:49:12 -
% @@ -1862,4 +1861,9 @@
% }
% % +#definesoreadabledata(so) \
% +(((so)-so_rcv.sb_cc 0 \
% +(so)-so_rcv.sb_cc = (so)-so_rcv.sb_lowat) || \
% +!TAILQ_EMPTY((so)-so_comp) || (so)-so_error)
% +
% int
% sopoll(struct socket *so, int events, struct ucred *active_cred,
-current already has this in a header (don't count hangup as data).
But -current still sets POLLIN for compatibility later, except in the
bogus POLLINIGNEOF case. It only uses the above change to avoid
setting POLLIN initially for hangup in the POLLINIGNEOF case.
% @@ -1869,12 +1873,7 @@
% % if (events (POLLIN | POLLRDNORM))
% -if (soreadable(so))
% +if (soreadabledata(so))
% revents |= events (POLLIN | POLLRDNORM);
Make POLLIN actually track data, not disconnection.
% % -if (events POLLINIGNEOF)
% -if (so-so_rcv.sb_cc = so-so_rcv.sb_lowat ||
% -!TAILQ_EMPTY(so-so_comp) || so-so_error)
% -revents |= POLLINIGNEOF;
% -
% if (events (POLLOUT | POLLWRNORM))
% if (sowriteable(so))
Start killing this.
% @@ -1885,8 +1884,15 @@
% revents |= events (POLLPRI | POLLRDBAND);
% % +if ((events POLLINIGNEOF) == 0) {
% +if (so-so_rcv.sb_state SBS_CANTRCVMORE) {
% +if (so-so_snd.sb_state SBS_CANTSENDMORE)
% +revents |= POLLHUP;
% +else
% +revents |= events (POLLIN | POLLRDNORM);
% +}
% +}
% +
Don't completely kill POLLINIGNEOF. I forget how the 2 socket state EOF
flags work. Both this and -current set POLLHUP iff both socket state
EOF flags are set. Then this never sets POLLIN, while -current always
sets it. Both set POLLIN of SBS_CANTRCVMORE is set but SBS_CANTSENDMORE
is not set. Note that POLLIN has already been set if there is actual
data,
so any setting of it here is bogus, but there seems to be a problem
when only SBS_CANTRCVMORE is set. Then !SBS_CANTSENDMORE implies that
output is still possible, so we must be able to return POLLOUT, but
POLLOUT is incompatible with POLLHUP so we can't set POLLHUP. We
apparently set POLLIN to fake this partial EOF.
% if (revents == 0) {
Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys
On Thu, 2 Aug 2012, David Xu wrote: On 2012/8/2 16:12, Bruce Evans wrote: ... I made similar patches for sockets (to set POLLHUP on hangup (now in -current) and to not set POLLIN on hangup unless there is still data to be read). I started killing POLLINIGNEOF for sockets. -current added it for nameless pipes instead :-(. With the new fifo implementation, POLLINIGNEOF is even more of a mistake for sockets, but more needed for pipes since named pipes are fifos. ... I think you can kill POLLINIGNEOF at all, I have grepped, and there is no external user, only pipe and socket code use it internally. The POLLINIGNEOF is confusing because it has same prefix with POLLIN, POLLOUT and other POLL flags. Did you grep all of google for it :-). All of ports should be enough. Uses of it in the kernel are certainly gone, but it was intentionally put in the user API to previde a workaround for the policy that was hard-coded in the kernel. The policy changed slightly, and you could set POLLINIGNEOF to either go back to the old policy or get the new policy for more cases. Hopefully this was never actually used except for testing and termporary workarounds. But it was expanded to work on nameless pipes as well as fifos and sockets. Bruce ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys
On 2012/8/2 22:17, Bruce Evans wrote: On Thu, 2 Aug 2012, David Xu wrote: On 2012/8/2 16:12, Bruce Evans wrote: ... I made similar patches for sockets (to set POLLHUP on hangup (now in -current) and to not set POLLIN on hangup unless there is still data to be read). I started killing POLLINIGNEOF for sockets. -current added it for nameless pipes instead :-(. With the new fifo implementation, POLLINIGNEOF is even more of a mistake for sockets, but more needed for pipes since named pipes are fifos. ... I think you can kill POLLINIGNEOF at all, I have grepped, and there is no external user, only pipe and socket code use it internally. The POLLINIGNEOF is confusing because it has same prefix with POLLIN, POLLOUT and other POLL flags. Did you grep all of google for it :-). All of ports should be enough. Uses of it in the kernel are certainly gone, but it was intentionally put in the user API to previde a workaround for the policy that was hard-coded in the kernel. The policy changed slightly, and you could set POLLINIGNEOF to either go back to the old policy or get the new policy for more cases. Hopefully this was never actually used except for testing and termporary workarounds. But it was expanded to work on nameless pipes as well as fifos and sockets. Bruce I don't know it is used by some ports. :-) Anyway, if people don't agree my patches, it is not a problem to me, because I always can apply them locally, though named pipe may be less useful from offical release. Thanks, ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys
On Tue, Jul 31, 2012 at 10:21 AM, David Xu listlog2...@gmail.com wrote: On 2012/7/31 15:22, Giovanni Trematerra wrote: On Tue, Jul 31, 2012 at 7:48 AM, David Xu davi...@freebsd.org wrote: Author: davidxu Date: Tue Jul 31 05:48:35 2012 New Revision: 238936 URL: http://svn.freebsd.org/changeset/base/238936 Log: I am comparing current pipe code with the one in 8.3-STABLE r236165, I found 8.3 is a history BSD version using socket to implement FIFO pipe, it uses per-file seqcount to compare with writer generation stored in per-pipe object. The concept is after all writers are gone, the pipe enters next generation, all old readers have not closed the pipe should get the indication that the pipe is disconnected, result is they should get EPIPE, SIGPIPE or get POLLHUP in poll(). But newcomer should not know that previous writters were gone, it should treat it as a fresh session. I am trying to bring back FIFO pipe to history behavior. It is still unclear that if single EOF flag can represent SBS_CANTSENDMORE and SBS_CANTRCVMORE which socket-based version is using, but I have run the poll regression test in tool directory, output is same as the one on 8.3-STABLE now. I think the output not ok 18 FIFO state 6b: poll result 0 expected 1. expected POLLHUP; got 0 might be bogus, because newcomer should not know that old writers were gone. I got the same behavior on Linux. Our implementation always return POLLIN for disconnected pipe even it should return POLLHUP, but I think it is not wise to remove POLLIN for compatible reason, this is our history behavior. I'm sorry but I'm failing to understand the reason for this change. Can you point me out a test that confirm that the change is needed. The only thing I see is an increase in the memory footprint for the pipes. There was a lot of discussions on this topic on -arch mailing list http://lists.freebsd.org/pipermail/freebsd-arch/2012-January/012131.html http://lists.freebsd.org/pipermail/freebsd-arch/2012-February/012314.html Thank you -- Gianni The old code broke some history semantic of FIFO pipe, you can try the test tool /usr/src/tools/regression/poll/pipepoll, try it before and after my commit, also compare the result with 8.3-STABLE, without this commit, both sub-tests 6c and 6d failed. This is on Vanilla 9.0-RELEASE where new fifo implementation weren't backported FreeBSD bombay 9.0-RELEASE FreeBSD 9.0-RELEASE #3: Tue Dec 27 21:59:00 UTC 2011 r...@build9x64.pcbsd.org:/usr/obj/builds/i386/pcbsd-build90/fbsd-source/9.0/sys/GENERIC i386 [gianni@bombay] /usr/src/tools/regression/poll#./pipepoll 1..20 ok 1 Pipe state 4: expected 0; got 0 ok 2 Pipe state 5: expected POLLIN; got POLLIN ok 3 Pipe state 6: expected POLLIN | POLLHUP; got POLLIN | POLLHUP not ok 4 Pipe state 6a: expected POLLHUP; got POLLIN | POLLHUP ok 5 Sock state 4: expected 0; got 0 ok 6 Sock state 5: expected POLLIN; got POLLIN ok 7 Sock state 6: expected POLLIN | POLLHUP; got POLLIN | POLLHUP not ok 8 Sock state 6a: expected POLLHUP; got POLLIN | POLLHUP ok 9 FIFO state 0: expected 0; got 0 ok 10 FIFO state 1: expected 0; got 0 ok 11 FIFO state 2: expected POLLIN; got POLLIN ok 12 FIFO state 2a: expected 0; got 0 not ok 13 FIFO state 3: expected POLLHUP; got POLLIN | POLLHUP ok 14 FIFO state 4: expected 0; got 0 ok 15 FIFO state 5: expected POLLIN; got POLLIN ok 16 FIFO state 6: expected POLLIN | POLLHUP; got POLLIN | POLLHUP not ok 17 FIFO state 6a: expected POLLHUP; got POLLIN | POLLHUP not ok 18 FIFO state 6b: poll result 0 expected 1. expected POLLHUP; got 0 not ok 19 FIFO state 6c: expected POLLHUP; got POLLIN | POLLHUP not ok 20 FIFO state 6d: expected POLLHUP; got POLLIN | POLLHUP As you can see, sub-tests 6c and 6d failed too on 9. So it's not a problem with new code though is irrelevant wrt the commit. I think old code did not mimic original code correctly, in 8.3-STABLE code, seqcount is stored in each file, writer generation detection is based on each copy of seqcount, but your code stored single copy of seqcount in fifoinfo object which is store as vnode data, and made the writer generation flag global by setting PIPE_SAMEWGEN in pipe object and used this flag to determine if it should return POLLHUP/POLLIN or not, this is wrong, for example: when there is no writer but have old readers, new incoming reader will executes: line 174 and 175: fip-fi_seqcount = fip-fi_wgen - fip-fi_writers; FIFO_WPDWGEN(fip, fpipe); this causes fi_seqcount to be equal to fi_wgen because fi_writer is zero, and FIFO_WPDWGEN() turns on flag PIPE_SAMEWGEN. When PIPE_SAMEWGEN is on, pipe_poll() ignores EOF, this breaks old readers, it causes old reader to get nothing while it should get POLLHUP from poll(). Ops, right. So a pointy hat to me. Thank you -- Gianni
Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys
On Wed, 1 Aug 2012, Giovanni Trematerra wrote: On Tue, Jul 31, 2012 at 10:21 AM, David Xu listlog2...@gmail.com wrote: ... The old code broke some history semantic of FIFO pipe, you can try the test tool /usr/src/tools/regression/poll/pipepoll, try it before and after my commit, also compare the result with 8.3-STABLE, without this commit, both sub-tests 6c and 6d failed. This is on Vanilla 9.0-RELEASE where new fifo implementation weren't backported FreeBSD bombay 9.0-RELEASE FreeBSD 9.0-RELEASE #3: Tue Dec 27 21:59:00 UTC 2011 r...@build9x64.pcbsd.org:/usr/obj/builds/i386/pcbsd-build90/fbsd-source/9.0/sys/GENERIC i386 [gianni@bombay] /usr/src/tools/regression/poll#./pipepoll 1..20 not ok 17 FIFO state 6a: expected POLLHUP; got POLLIN | POLLHUP not ok 18 FIFO state 6b: poll result 0 expected 1. expected POLLHUP; got 0 not ok 19 FIFO state 6c: expected POLLHUP; got POLLIN | POLLHUP not ok 20 FIFO state 6d: expected POLLHUP; got POLLIN | POLLHUP As you can see, sub-tests 6c and 6d failed too on 9. So it's not a problem with new code though is irrelevant wrt the commit. The failure is very differnt. Failure to clear POLLIN in 6a, 6c and 6d is a normal bug in FreeBSD. Failure to set POLLHUP is apparently a normal bug in FreeBSD (I didn't know about this one). But now, POLLHUP isn't set for 6c or 6d, and consequentially, POLLIN isn't set either. The behaviour is strangely version-depending: - FreeBSD-9 (ref9-amd64): as above (select 6c and 6d pass) - FreeBSD-10 (ref10-amd64 Feb 19 2012): as above - FreeBSD-10 (ref10-i386 Jul 10 2012): not ok 17 FIFO state 6a: expected POLLHUP; got POLLIN | POLLHUP not ok 18 FIFO state 6b: poll result 0 expected 1. expected POLLHUP; got 0 not ok 19 FIFO state 6c: poll result 0 expected 1. expected POLLHUP; got 0 not ok 20 FIFO state 6d: poll result 0 expected 1. expected POLLHUP; got 0 Actually, this is not strange. The difference between the FreeBSD-10's sys_pipe.c is your new pipe implementation. Bruce ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys
On 2012/8/2 3:29, Bruce Evans wrote:
On Wed, 1 Aug 2012, Giovanni Trematerra wrote:
On Tue, Jul 31, 2012 at 10:21 AM, David Xu listlog2...@gmail.com
wrote:
...
The old code broke some history semantic of FIFO pipe, you can try
the test
tool /usr/src/tools/regression/poll/pipepoll, try it before and
after my
commit, also compare the result with 8.3-STABLE, without this commit,
both sub-tests 6c and 6d failed.
This is on Vanilla 9.0-RELEASE where new fifo implementation weren't
backported
FreeBSD bombay 9.0-RELEASE FreeBSD 9.0-RELEASE #3: Tue Dec 27 21:59:00
UTC 2011
r...@build9x64.pcbsd.org:/usr/obj/builds/i386/pcbsd-build90/fbsd-source/9.0/sys/GENERIC
i386
[gianni@bombay] /usr/src/tools/regression/poll#./pipepoll
1..20
not ok 17 FIFO state 6a: expected POLLHUP; got POLLIN | POLLHUP
not ok 18 FIFO state 6b: poll result 0 expected 1. expected POLLHUP;
got 0
not ok 19 FIFO state 6c: expected POLLHUP; got POLLIN | POLLHUP
not ok 20 FIFO state 6d: expected POLLHUP; got POLLIN | POLLHUP
As you can see, sub-tests 6c and 6d failed too on 9. So it's not a
problem with
new code though is irrelevant wrt the commit.
The failure is very differnt. Failure to clear POLLIN in 6a, 6c and 6d
is a normal bug in FreeBSD.
I have attached a patch to fix it, it should make the regression tool happy.
Is it worth to commit ?
Index: sys_pipe.c
===
--- sys_pipe.c (revision 238936)
+++ sys_pipe.c (working copy)
@@ -1447,7 +1447,6 @@
if ((events POLLINIGNEOF) == 0) {
if (rpipe-pipe_state PIPE_EOF) {
- revents |= (events (POLLIN | POLLRDNORM));
if (wpipe-pipe_present != PIPE_ACTIVE ||
(wpipe-pipe_state PIPE_EOF))
revents |= POLLHUP;
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys
On Tue, Jul 31, 2012 at 7:48 AM, David Xu davi...@freebsd.org wrote: Author: davidxu Date: Tue Jul 31 05:48:35 2012 New Revision: 238936 URL: http://svn.freebsd.org/changeset/base/238936 Log: I am comparing current pipe code with the one in 8.3-STABLE r236165, I found 8.3 is a history BSD version using socket to implement FIFO pipe, it uses per-file seqcount to compare with writer generation stored in per-pipe object. The concept is after all writers are gone, the pipe enters next generation, all old readers have not closed the pipe should get the indication that the pipe is disconnected, result is they should get EPIPE, SIGPIPE or get POLLHUP in poll(). But newcomer should not know that previous writters were gone, it should treat it as a fresh session. I am trying to bring back FIFO pipe to history behavior. It is still unclear that if single EOF flag can represent SBS_CANTSENDMORE and SBS_CANTRCVMORE which socket-based version is using, but I have run the poll regression test in tool directory, output is same as the one on 8.3-STABLE now. I think the output not ok 18 FIFO state 6b: poll result 0 expected 1. expected POLLHUP; got 0 might be bogus, because newcomer should not know that old writers were gone. I got the same behavior on Linux. Our implementation always return POLLIN for disconnected pipe even it should return POLLHUP, but I think it is not wise to remove POLLIN for compatible reason, this is our history behavior. I'm sorry but I'm failing to understand the reason for this change. Can you point me out a test that confirm that the change is needed. The only thing I see is an increase in the memory footprint for the pipes. There was a lot of discussions on this topic on -arch mailing list http://lists.freebsd.org/pipermail/freebsd-arch/2012-January/012131.html http://lists.freebsd.org/pipermail/freebsd-arch/2012-February/012314.html Thank you -- Gianni ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys
On 2012/7/31 15:22, Giovanni Trematerra wrote: On Tue, Jul 31, 2012 at 7:48 AM, David Xu davi...@freebsd.org wrote: Author: davidxu Date: Tue Jul 31 05:48:35 2012 New Revision: 238936 URL: http://svn.freebsd.org/changeset/base/238936 Log: I am comparing current pipe code with the one in 8.3-STABLE r236165, I found 8.3 is a history BSD version using socket to implement FIFO pipe, it uses per-file seqcount to compare with writer generation stored in per-pipe object. The concept is after all writers are gone, the pipe enters next generation, all old readers have not closed the pipe should get the indication that the pipe is disconnected, result is they should get EPIPE, SIGPIPE or get POLLHUP in poll(). But newcomer should not know that previous writters were gone, it should treat it as a fresh session. I am trying to bring back FIFO pipe to history behavior. It is still unclear that if single EOF flag can represent SBS_CANTSENDMORE and SBS_CANTRCVMORE which socket-based version is using, but I have run the poll regression test in tool directory, output is same as the one on 8.3-STABLE now. I think the output not ok 18 FIFO state 6b: poll result 0 expected 1. expected POLLHUP; got 0 might be bogus, because newcomer should not know that old writers were gone. I got the same behavior on Linux. Our implementation always return POLLIN for disconnected pipe even it should return POLLHUP, but I think it is not wise to remove POLLIN for compatible reason, this is our history behavior. I'm sorry but I'm failing to understand the reason for this change. Can you point me out a test that confirm that the change is needed. The only thing I see is an increase in the memory footprint for the pipes. There was a lot of discussions on this topic on -arch mailing list http://lists.freebsd.org/pipermail/freebsd-arch/2012-January/012131.html http://lists.freebsd.org/pipermail/freebsd-arch/2012-February/012314.html Thank you -- Gianni The old code broke some history semantic of FIFO pipe, you can try the test tool /usr/src/tools/regression/poll/pipepoll, try it before and after my commit, also compare the result with 8.3-STABLE, without this commit, both sub-tests 6c and 6d failed. I think old code did not mimic original code correctly, in 8.3-STABLE code, seqcount is stored in each file, writer generation detection is based on each copy of seqcount, but your code stored single copy of seqcount in fifoinfo object which is store as vnode data, and made the writer generation flag global by setting PIPE_SAMEWGEN in pipe object and used this flag to determine if it should return POLLHUP/POLLIN or not, this is wrong, for example: when there is no writer but have old readers, new incoming reader will executes: line 174 and 175: fip-fi_seqcount = fip-fi_wgen - fip-fi_writers; FIFO_WPDWGEN(fip, fpipe); this causes fi_seqcount to be equal to fi_wgen because fi_writer is zero, and FIFO_WPDWGEN() turns on flag PIPE_SAMEWGEN. When PIPE_SAMEWGEN is on, pipe_poll() ignores EOF, this breaks old readers, it causes old reader to get nothing while it should get POLLHUP from poll(). The new incoming reader should get nothing, so I think sub-tests 6b is wrong. ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys
On Tue, 31 Jul 2012, David Xu wrote: On 2012/7/31 15:22, Giovanni Trematerra wrote: On Tue, Jul 31, 2012 at 7:48 AM, David Xu davi...@freebsd.org wrote: Log: I am comparing current pipe code with the one in 8.3-STABLE r236165, I found 8.3 is a history BSD version using socket to implement FIFO pipe, it uses per-file seqcount to compare with writer generation stored in per-pipe object. The concept is after all writers are gone, the pipe enters next generation, all old readers have not closed the pipe should get the indication that the pipe is disconnected, result is they should get EPIPE, SIGPIPE or get POLLHUP in poll(). But newcomer should not know that previous writters were gone, it should treat it as a fresh session. Good commit message. Almost worth quoting in 3 followups :-). I wrote most of the code and forgotten some details, and the above made them clear. I am trying to bring back FIFO pipe to history behavior. It is still unclear that if single EOF flag can represent SBS_CANTSENDMORE and SBS_CANTRCVMORE which socket-based version is using, but I have run the poll regression test in tool directory, output is same as the one on 8.3-STABLE now. Not very historic. Only FreeBSD-8 (maybe 9?) did that. I think the output not ok 18 FIFO state 6b: poll result 0 expected 1. expected POLLHUP; got 0 might be bogus, because newcomer should not know that old writers were gone. I got the same behavior on Linux. 6b is intentionally different from Linux. I forget if it is to reduce races with readers or just to simply the implementation and understanding it. New readers simply joing old readers with a hangup set for all if they manage to open the fifo (necessarily using O_NONBLOCK) after the hangup but before the old readers go away. Since this seems to increase races, I may remember it backwards Our implementation always return POLLIN for disconnected pipe even it should return POLLHUP, but I think it is not wise to remove POLLIN for compatible reason, this is our history behavior. This is historical back to FreeBSD-3 (earlier versions didn't have poll()). I think it is just a bug. POLLHUP was unimplemented for most file types before FreeBSD-8, and setting POLLIN works around this for most callers. I tried to get it fixed for at least fifos when I fixed POLLHUP for some file types. No one uses fifos, so they are safer to fix than sockets :-). I'm sorry but I'm failing to understand the reason for this change. Can you point me out a test that confirm that the change is needed. The only thing I see is an increase in the memory footprint for the pipes. There was a lot of discussions on this topic on -arch mailing list Many poll regression tests fail. http://lists.freebsd.org/pipermail/freebsd-arch/2012-January/012131.html http://lists.freebsd.org/pipermail/freebsd-arch/2012-February/012314.html There are also a lot of old PRs about this for poll() (not for your new fifo implementation). I think the PRs are mentioned in these threads. The old code broke some history semantic of FIFO pipe, you can try the test tool /usr/src/tools/regression/poll/pipepoll, try it before and after my commit, also compare the result with 8.3-STABLE, without this commit, both sub-tests 6c and 6d failed. I think old code did not mimic original code correctly, in 8.3-STABLE code, seqcount is stored in each file, writer generation detection is based on each copy of seqcount, but your code stored single copy of seqcount in fifoinfo object which is store as vnode data, and made the writer generation flag global by setting PIPE_SAMEWGEN in pipe object and used this flag to determine if it should return POLLHUP/POLLIN or not, this is wrong, for example: when there is no writer but have old readers, new incoming reader will executes: line 174 and 175: fip-fi_seqcount = fip-fi_wgen - fip-fi_writers; FIFO_WPDWGEN(fip, fpipe); this causes fi_seqcount to be equal to fi_wgen because fi_writer is zero, and FIFO_WPDWGEN() turns on flag PIPE_SAMEWGEN. When PIPE_SAMEWGEN is on, pipe_poll() ignores EOF, this breaks old readers, it causes old reader to get nothing while it should get POLLHUP from poll(). The new incoming reader should get nothing, so I think sub-tests 6b is wrong. Luckily I have forgotten the details for fifos and never understood them all for nameless pipes, so you get to fix it :-). Bruce ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
