From owner-svn-src-head@FreeBSD.ORG Wed Aug 1 17:57:20 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 31C49106566C; Wed, 1 Aug 2012 17:57:20 +0000 (UTC) (envelope-from giovanni.trematerra@gmail.com) Received: from mail-qc0-f182.google.com (mail-qc0-f182.google.com [209.85.216.182]) by mx1.freebsd.org (Postfix) with ESMTP id 708818FC0A; Wed, 1 Aug 2012 17:57:19 +0000 (UTC) Received: by qcsg15 with SMTP id g15so5774494qcs.13 for ; Wed, 01 Aug 2012 10:57:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=jrXvnx8wQLij0U6Wznu486GuMCraEN4i5pq5y7itcy0=; b=IO5iBbN9toSVYwvy3X4biOa9Qy1lpV2WUxv5EGM8/8+vaIC3AIUz/yOOd5hvC2qcMc aAIhK2ABvQh9doJaliEIKKGu9UkgQdOsgXwch/i5eUOGfH44OcssNOD63wGTlGhj4RD/ a1pTMj2zqSrh+xG1Eqln84vAJpxTgvYSR6Vuul3dr2xHHWDltXMMOZnCgck+VB4Ahrkr +5vhrdjs8uX1EMqcymCgRGrULOX8EgArqjrYHVMuHDx45D/yvzgM2+V5qLkpr+QWRrxy 7qmjyDa7IaydO/VsanMZZYOb5BA8x7/RQTQMnX8fiy5kxN0+4YoyDTLt0j1jvkPaIj4B hJvg== MIME-Version: 1.0 Received: by 10.224.26.195 with SMTP id f3mr24923779qac.71.1343843832790; Wed, 01 Aug 2012 10:57:12 -0700 (PDT) Sender: giovanni.trematerra@gmail.com Received: by 10.229.59.169 with HTTP; Wed, 1 Aug 2012 10:57:12 -0700 (PDT) In-Reply-To: <50179581.9070805@gmail.com> References: <201207310548.q6V5mZHf091624@svn.freebsd.org> <50179581.9070805@gmail.com> Date: Wed, 1 Aug 2012 19:57:12 +0200 X-Google-Sender-Auth: de3WDTBcgRNDzxivqJfyMB4ck-c Message-ID: From: Giovanni Trematerra To: davidxu@freebsd.org Content-Type: text/plain; charset=UTF-8 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Konstantin Belousov , bde@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: Wed, 01 Aug 2012 17:57:20 -0000 On Tue, Jul 31, 2012 at 10:21 AM, David Xu wrote: > On 2012/7/31 15:22, Giovanni Trematerra wrote: >> >> On Tue, Jul 31, 2012 at 7:48 AM, David Xu 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