Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 1 Aug 2012 19:57:12 +0200
From:      Giovanni Trematerra <gianni@freebsd.org>
To:        davidxu@freebsd.org
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Konstantin Belousov <kib@freebsd.org>, bde@freebsd.org
Subject:   Re: svn commit: r238936 - in head/sys: fs/fifofs kern sys
Message-ID:  <CACfq091K=1DUOee4LorPPSoEV8KnT570GGXDA01mHDrV12kKiQ@mail.gmail.com>
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, Jul 31, 2012 at 10:21 AM, David Xu <listlog2011@gmail.com> 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:
>>>
>>> Author: davidxu
>>> Date: Tue Jul 31 05:48:35 2012
>>> New Revision: 238936
>>> URL: http://svn.freebsd.org/changeset/base/238936
>>>
>>> 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.
>>>    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.
>>>    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.
>>>    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.
>>>
>> 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
>>
>> http://lists.freebsd.org/pipermail/freebsd-arch/2012-January/012131.html
>> http://lists.freebsd.org/pipermail/freebsd-arch/2012-February/012314.html
>>
>> Thank you
>>
>> --
>> Gianni
>>
> 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.

This is on Vanilla 9.0-RELEASE where new fifo implementation weren't backported

FreeBSD bombay 9.0-RELEASE FreeBSD 9.0-RELEASE #3: Tue Dec 27 21:59:00
UTC 2011     root@build9x64.pcbsd.org:/usr/obj/builds/i386/pcbsd-build90/fbsd-source/9.0/sys/GENERIC
 i386

[gianni@bombay] /usr/src/tools/regression/poll#./pipepoll
1..20
ok 1      Pipe state 4: expected 0; got 0
ok 2      Pipe state 5: expected POLLIN; got POLLIN
ok 3      Pipe state 6: expected POLLIN | POLLHUP; got POLLIN | POLLHUP
not ok 4  Pipe state 6a: expected POLLHUP; got POLLIN | POLLHUP
ok 5      Sock state 4: expected 0; got 0
ok 6      Sock state 5: expected POLLIN; got POLLIN
ok 7      Sock state 6: expected POLLIN | POLLHUP; got POLLIN | POLLHUP
not ok 8  Sock state 6a: expected POLLHUP; got POLLIN | POLLHUP
ok 9      FIFO state 0: expected 0; got 0
ok 10     FIFO state 1: expected 0; got 0
ok 11     FIFO state 2: expected POLLIN; got POLLIN
ok 12     FIFO state 2a: expected 0; got 0
not ok 13 FIFO state 3: expected POLLHUP; got POLLIN | POLLHUP
ok 14     FIFO state 4: expected 0; got 0
ok 15     FIFO state 5: expected POLLIN; got POLLIN
ok 16     FIFO state 6: expected POLLIN | POLLHUP; got POLLIN | POLLHUP
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.

>
> 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().
>

Ops, right. So a pointy hat to me.

Thank you

--
Gianni



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACfq091K=1DUOee4LorPPSoEV8KnT570GGXDA01mHDrV12kKiQ>