From owner-svn-src-all@FreeBSD.ORG Tue Jul 31 11:36:55 2012 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0C924106564A; Tue, 31 Jul 2012 11:36:55 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail28.syd.optusnet.com.au (mail28.syd.optusnet.com.au [211.29.133.169]) by mx1.freebsd.org (Postfix) with ESMTP id 55A288FC08; Tue, 31 Jul 2012 11:36:54 +0000 (UTC) Received: from c122-106-171-246.carlnfd1.nsw.optusnet.com.au (c122-106-171-246.carlnfd1.nsw.optusnet.com.au [122.106.171.246]) by mail28.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q6VBaiiV019129 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 31 Jul 2012 21:36:46 +1000 Date: Tue, 31 Jul 2012 21:36:44 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: David Xu In-Reply-To: <50179581.9070805@gmail.com> Message-ID: <20120731210253.T1970@besplex.bde.org> References: <201207310548.q6V5mZHf091624@svn.freebsd.org> <50179581.9070805@gmail.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: src-committers@FreeBSD.org, Giovanni Trematerra , svn-src-all@FreeBSD.org, Konstantin Belousov , bde@FreeBSD.org, svn-src-head@FreeBSD.org Subject: Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 31 Jul 2012 11:36:55 -0000 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 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