Date: Tue, 31 Jul 2012 21:36:44 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: David Xu <davidxu@FreeBSD.org> Cc: src-committers@FreeBSD.org, Giovanni Trematerra <gianni@FreeBSD.org>, svn-src-all@FreeBSD.org, Konstantin Belousov <kib@FreeBSD.org>, bde@FreeBSD.org, svn-src-head@FreeBSD.org Subject: Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys Message-ID: <20120731210253.T1970@besplex.bde.org> 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, 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 <davidxu@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120731210253.T1970>