From owner-svn-src-head@FreeBSD.ORG Wed Aug 26 07:09:04 2009 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id AEC76106568C; Wed, 26 Aug 2009 07:09:04 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail01.syd.optusnet.com.au (mail01.syd.optusnet.com.au [211.29.132.182]) by mx1.freebsd.org (Postfix) with ESMTP id 455508FC23; Wed, 26 Aug 2009 07:09:03 +0000 (UTC) Received: from c122-106-152-1.carlnfd1.nsw.optusnet.com.au (c122-106-152-1.carlnfd1.nsw.optusnet.com.au [122.106.152.1]) by mail01.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id n7Q78v7R026854 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 26 Aug 2009 17:08:59 +1000 Date: Wed, 26 Aug 2009 17:08:57 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Jilles Tjoelker In-Reply-To: <20090825210815.GA8792@stack.nl> Message-ID: <20090826163931.C41472@delplex.bde.org> References: <200908231244.n7NCiFgc061095@svn.freebsd.org> <20090823213759.GA55039@stack.nl> <20090825031355.B729@besplex.bde.org> <20090825210815.GA8792@stack.nl> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Konstantin Belousov , Bruce Evans Subject: Re: svn commit: r196460 - head/sys/kern X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 Aug 2009 07:09:04 -0000 On Tue, 25 Aug 2009, Jilles Tjoelker wrote: > On Tue, Aug 25, 2009 at 04:07:11AM +1000, Bruce Evans wrote: >> On Sun, 23 Aug 2009, Jilles Tjoelker wrote: > >>> I think poll on fifos should instead be fixed by closing the >>> half-connection corresponding to writing from fi_readsock to >>> fi_writesock. I have tried this out, see attached patch. With the patch, >>> pipepoll only gives "expected POLLHUP; got POLLIN | POLLHUP" errors and >>> an error in fifo case 6b caused by our distinction between new and old >>> readers. > >> This sort of worked for me, but has several problems: > >> % Index: sys/fs/fifofs/fifo_vnops.c >> % =================================================================== >> % --- sys/fs/fifofs/fifo_vnops.c (revision 196459) >> % +++ sys/fs/fifofs/fifo_vnops.c (working copy) >> % @@ -193,6 +193,10 @@ >> % goto fail2; >> % fip->fi_writesock = wso; >> % error = soconnect2(wso, rso); >> % + if (error == 0) >> % + error = soshutdown(rso, SHUT_WR); >> % + if (error == 0) >> % + error = soshutdown(wso, SHUT_RD); >> % if (error) { >> % (void)soclose(wso); >> % fail2: > >> The second soshutdown() is only harmful. I see the following state changes >> - the first soshutdown() sets SBS_CANTRCVMORE on rso like you would expect, >> and also sets SBS_CANTSENDMORE on wso. This gives the desired state. >> - the second soshutdown() then clears SBS_CANTRCVMORE on rso (without >> the first soshutdown() it leaves both flags clear in both directions). >> This clobbers the desired state. The failure shows in just one of my >> uncommitted regression tests (when there is a writer and there was >> a reader, poll() returns POLLOUT for the writer, but should return >> POLLHUP; the missing SBS_CANTRCVMORE on rso prevents it ever returning >> POLLHUP for writers). > >> After removing the second soshutdown() and fixing a spurious POLLIN (see >> below), all my tests pass. > > I have removed the second shutdown, it is not necessary. The second shutdown became harmless for me when I fixed the clobbering of sb_state. Does it have any effect? >> % Index: sys/kern/uipc_socket.c >> % =================================================================== >> % --- sys/kern/uipc_socket.c (revision 196469) >> % +++ sys/kern/uipc_socket.c (working copy) >> % @@ -2898,11 +2898,13 @@ >> % if (so->so_oobmark || (so->so_rcv.sb_state & SBS_RCVATMARK)) >> % revents |= events & (POLLPRI | POLLRDBAND); >> % >> % - if ((events & POLLINIGNEOF) == 0) >> % - if (so->so_rcv.sb_state & SBS_CANTRCVMORE) >> % - revents |= POLLHUP; >> % - if (so->so_snd.sb_state & SBS_CANTSENDMORE) >> % - revents |= POLLHUP; >> % + if ((events & POLLINIGNEOF) == 0) { > >> Old problems become larger: > >> I don't like POLLINIGNEOF (for input) affecting POLLHUP for output. This >> seems to cause no problems for fifos, at least when the kernel sets >> POLLINIGNEOF, but it is hard to understand even why it doesn't cause >> problems, and kib@ wants POLLINIGNEOF to remain user-settable, so the >> complications might remain exported to userland for for fifos and >> sockets, where they are harder to document and understand. > > I do not like userland POLLINIGNEOF either. I think programs can do fine > with the standard functionality (closing and reopening a fifo to reset > the POLLHUP state). > >> % + revents |= events & (POLLIN | POLLRDNORM); > >> This gives spurious POLLINs when POLLHUP is also returned, and thus defeats >> the point of soreadable_data() being different from soreadable(). Tests >> 6a-6d show the spurious POLLIN. I don't understand how tests 6a and 6c-6d >> passed for you. > > Same problem here. I think kib@ wants to keep this in 8.x for the sake > of buggy programs that do not check for POLLHUP. I suppose we can do it > properly in 9.x. But it may break non-buggy programs, and doesn't help for buggy programs like gdb. (gdb exits immediately when it sees POLLHUP, despite POLLIN also being set and input being queued. gdb used to work before it was converted to use poll(). Then it saw a ready input fd and had to read() it to detect EOF.) I now have better tests for POLLOUT, and have tested the buggy case for select some more (test 9, and 6b (?) in current): if POLLINIGNEOF is not applied for select(), then test 9 passes as expected. Test 9 only covers a fifo in initial state with a reader and no writers. This is EOF for read() and input-ready for select(), but neither POLLIN nor POLLHUP for poll(). Test 6b is for essentially the initial state for read() and select(), but for poll() the state is too-sticky hangup (POSIX) or back to the initial state (Linux-2.6.10/FreeBSD-current). Bruce