Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 31 Jul 2012 16:21:21 +0800
From:      David Xu <listlog2011@gmail.com>
To:        Giovanni Trematerra <gianni@freebsd.org>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org, Konstantin Belousov <kib@freebsd.org>, David Xu <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:  <50179581.9070805@gmail.com>
In-Reply-To: <CACfq090si0g7QG9UR%2Bmb%2BO5M4KH0=WPfWnF5NzZjYzKBp8hQ5Q@mail.gmail.com>
References:  <201207310548.q6V5mZHf091624@svn.freebsd.org> <CACfq090si0g7QG9UR%2Bmb%2BO5M4KH0=WPfWnF5NzZjYzKBp8hQ5Q@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help
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.

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.





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