Date: Wed, 1 Aug 2012 19:57:12 +0200 From: Giovanni Trematerra <gianni@freebsd.org> To: davidxu@freebsd.org Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Konstantin Belousov <kib@freebsd.org>, bde@freebsd.org Subject: Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys Message-ID: <CACfq091K=1DUOee4LorPPSoEV8KnT570GGXDA01mHDrV12kKiQ@mail.gmail.com> In-Reply-To: <50179581.9070805@gmail.com> References: <201207310548.q6V5mZHf091624@svn.freebsd.org> <CACfq090si0g7QG9UR%2Bmb%2BO5M4KH0=WPfWnF5NzZjYzKBp8hQ5Q@mail.gmail.com> <50179581.9070805@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Jul 31, 2012 at 10:21 AM, David Xu <listlog2011@gmail.com> wrote: > On 2012/7/31 15:22, Giovanni Trematerra wrote: >> >> On Tue, Jul 31, 2012 at 7:48 AM, David Xu <davidxu@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 root@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACfq091K=1DUOee4LorPPSoEV8KnT570GGXDA01mHDrV12kKiQ>