From owner-svn-src-head@FreeBSD.ORG Thu Aug 2 08:13:03 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 898A0106566C; Thu, 2 Aug 2012 08:13:03 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail02.syd.optusnet.com.au (mail02.syd.optusnet.com.au [211.29.132.183]) by mx1.freebsd.org (Postfix) with ESMTP id 1866F8FC0A; Thu, 2 Aug 2012 08:13:02 +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 mail02.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q728CxPc010824 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 2 Aug 2012 18:13:01 +1000 Date: Thu, 2 Aug 2012 18:12:59 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: davidxu@freebsd.org In-Reply-To: <5019ED93.8060802@gmail.com> Message-ID: <20120802173441.X1291@besplex.bde.org> References: <201207310548.q6V5mZHf091624@svn.freebsd.org> <50179581.9070805@gmail.com> <20120802051805.P3345@besplex.bde.org> <5019ED93.8060802@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, Bruce Evans , svn-src-head@freebsd.org Subject: Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys 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: Thu, 02 Aug 2012 08:13:03 -0000 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.c 27 Mar 2004 19:50:22 -0000 1.171 % +++ sys_pipe.c 13 Aug 2009 11:33:08 -0000 % @@ -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 -0000 1.189 % +++ uipc_socket.c 26 Aug 2009 22:49:12 -0000 % @@ -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 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) { % - if (events & % - (POLLIN | POLLINIGNEOF | POLLPRI | POLLRDNORM | % - POLLRDBAND)) { % + if (events & (POLLIN | POLLPRI | POLLRDNORM | POLLRDBAND)) { Continue killing POLLINIGNEOF. % SOCKBUF_LOCK(&so->so_rcv); % selrecord(td, &so->so_rcv.sb_sel); Bruce