Date: Thu, 02 Aug 2012 20:29:34 +0800 From: David Xu <listlog2011@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: src-committers@freebsd.org, Giovanni Trematerra <gianni@freebsd.org>, svn-src-all@freebsd.org, Konstantin Belousov <kib@freebsd.org>, davidxu@freebsd.org, svn-src-head@freebsd.org, bde@freebsd.org Subject: Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys Message-ID: <501A72AE.30309@gmail.com> In-Reply-To: <20120802173441.X1291@besplex.bde.org> References: <201207310548.q6V5mZHf091624@svn.freebsd.org> <CACfq090si0g7QG9UR%2Bmb%2BO5M4KH0=WPfWnF5NzZjYzKBp8hQ5Q@mail.gmail.com> <50179581.9070805@gmail.com> <CACfq091K=1DUOee4LorPPSoEV8KnT570GGXDA01mHDrV12kKiQ@mail.gmail.com> <20120802051805.P3345@besplex.bde.org> <5019ED93.8060802@gmail.com> <20120802173441.X1291@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2012/8/2 16:12, Bruce Evans wrote:
> 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
>
I think you can kill POLLINIGNEOF at all, I have grepped, and there is
no external user,
only pipe and socket code use it internally. The POLLINIGNEOF is
confusing because
it has same prefix with POLLIN, POLLOUT and other POLL flags.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?501A72AE.30309>