From owner-svn-src-all@FreeBSD.ORG Thu Dec 1 11:48:52 2011 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0B0A2106567B; Thu, 1 Dec 2011 11:48:52 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id 4AF0A8FC1D; Thu, 1 Dec 2011 11:48:50 +0000 (UTC) Received: from alf.home (alf.kiev.zoral.com.ua [10.1.1.177]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id pB1Bmkww083375 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 1 Dec 2011 13:48:46 +0200 (EET) (envelope-from kostikbel@gmail.com) Received: from alf.home (kostik@localhost [127.0.0.1]) by alf.home (8.14.5/8.14.5) with ESMTP id pB1Bmk3H082751; Thu, 1 Dec 2011 13:48:46 +0200 (EET) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by alf.home (8.14.5/8.14.5/Submit) id pB1Bmkl3082750; Thu, 1 Dec 2011 13:48:46 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: alf.home: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 1 Dec 2011 13:48:46 +0200 From: Kostik Belousov To: Giovanni Trematerra Message-ID: <20111201114846.GI50300@deviant.kiev.zoral.com.ua> References: <201110051656.p95Gu6Cw020744@svn.freebsd.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="LAfrv7inJS3zgkKF" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-3.9 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: src-committers@freebsd.org, peterjeremy@acm.org, John Baldwin , svn-src-all@freebsd.org, Attilio Rao , svn-src-head@freebsd.org, Jeff Roberson Subject: Re: svn commit: r226042 - in head/sys: kern sys X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Dec 2011 11:48:52 -0000 --LAfrv7inJS3zgkKF Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Nov 30, 2011 at 10:01:12PM +0100, Giovanni Trematerra wrote: > On Wed, Oct 5, 2011 at 6:56 PM, Konstantin Belousov wro= te: > > Author: kib > > Date: Wed Oct =9A5 16:56:06 2011 > > New Revision: 226042 > > URL: http://svn.freebsd.org/changeset/base/226042 > > > > Log: > > =9ASupply unique (st_dev, st_ino) value pair for the fstat(2) done on t= he pipes. > > > > =9AReviewed by: =9Ajhb, Peter Jeremy > > =9AMFC after: =9A =9A2 weeks > > > > Modified: > > =9Ahead/sys/kern/sys_pipe.c > > =9Ahead/sys/sys/pipe.h > > >=20 > Hi Konstantin, > unfortunately your commit introduces a performance penalty of about 3% > documented below in a real workload. > I guess that fstat(2) on the pipes is seldom used so we could just be lazy > and alloc a new unr number inside pipe_stat instead of during pipe creati= on. > In that case if an application doesn't need fstat(2) on the pipes it > won't be charged > the cost of alloc_unr/free_unr for the fake inode number. > The patch I propose, furthermore, fix a panic in the case alloc_unr > failed to allocate > a new unr number and return -1. This is because ino_t is unsigned and the= test > pipe_ino > 0 into pipeclose would be true, calling then free_unr when > it hasn't to. > The proposed patch was tested with a regression test code that you can fi= nd here >=20 > http://www.trematerra.net/patches/pipe-fstat.c >=20 > Feel free to add it under tools/regression/pipe/ >=20 > Here the proposed patch: >=20 > http://www.trematerra.net/patches/lazy_inoalloc.diff >=20 > Here the report of the benchmark: >=20 > Configuration > 10.0-CURRENT updated to r22807. > kern.smp.disabled=3D1 in /boot/loader.conf > kernel config GENERIC without debugging options. >=20 > The first result of any test was discarded and not reported here. >=20 > here the result of three executions of > # make -s buildkernel > note that I managed to compile the same identical source files > for all the tests. >=20 > r22807 with r226042 reverted (time in seconds) > 527, 527, 527 >=20 > r22807 (time in seconds) > 544, 544, 544 >=20 > r22807M with the proposed patch (time in seconds) > 527, 528, 528 >=20 > ministat output: >=20 > x r22807 with r226042 reverted > + r22807 > * r22807M with the proposed patch > +------------------------------------------------------------------------= ------+ > |+ * = x| > |* * = x| > ||__A_M| = A| > +------------------------------------------------------------------------= ------+ > N Min Max Median Avg Stdd= ev > x 3 544 544 544 544 = 0 > + 3 527 527 527 527 = 0 > Difference at 95.0% confidence > -17 +/- 0 > -3.125% +/- 0% > (Student's t, pooled s =3D 0) > * 3 527 528 528 527.66667 0.577350= 27 > Difference at 95.0% confidence > -16.3333 +/- 0.925333 > -3.00245% +/- 0.170098% > (Student's t, pooled s =3D 0.408248) >=20 > -- > Gianni Thank you for looking at this. I committed the test, and the fix for the call to free_unr(-1). Regarding the lazy allocation of the inode number, I agree with the idea, but have some reservations against the implementation. If several threads call fstat(2) on the same pipe which inode is not yet initialized, then I see a race in the patch. The easiest workaround is to cover the inode allocation with the pipe lock. Also, I find the return of ENOMEM from fstat(2) somewhat questionable. The error code is not documented as allowed for the syscall. I prefer to not fail the fstat(2) if lazy allocation failed, but return some fake value for inode instead. Updated patch is below. diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c index 2b6eb66..19fc98f 100644 --- a/sys/kern/sys_pipe.c +++ b/sys/kern/sys_pipe.c @@ -569,12 +569,7 @@ pipe_create(pipe, backing) /* If we're not backing this pipe, no need to do anything. */ error =3D 0; } - if (error =3D=3D 0) { - pipe->pipe_ino =3D alloc_unr(pipeino_unr); - if (pipe->pipe_ino =3D=3D -1) - /* pipeclose will clear allocated kva */ - error =3D ENOMEM; - } + pipe->pipe_ino =3D -1; return (error); } =20 @@ -1398,16 +1393,40 @@ pipe_stat(fp, ub, active_cred, td) struct ucred *active_cred; struct thread *td; { - struct pipe *pipe =3D fp->f_data; + struct pipe *pipe; + int new_unr; #ifdef MAC int error; +#endif =20 + pipe =3D fp->f_data; PIPE_LOCK(pipe); +#ifdef MAC error =3D mac_pipe_check_stat(active_cred, pipe->pipe_pair); - PIPE_UNLOCK(pipe); - if (error) + if (error) { + PIPE_UNLOCK(pipe); return (error); + } #endif + /* + * Lazily allocate an inode number for the pipe. Most pipe + * users do not call stat(2) on the pipe, which means that + * postponing the inode allocation until it is must be returned to + * userland is useful. If alloc_unr failed, assign st_ino + * zero instead of returning an error. + * Special pipe_ino values: + * -1 - not yet initialized; + * 0 - alloc_unr failed, return 0 as st_ino forever. + */ + if (pipe->pipe_ino =3D=3D (ino_t)-1) { + new_unr =3D alloc_unr(pipeino_unr); + if (new_unr !=3D -1) + pipe->pipe_ino =3D new_unr; + else + pipe->pipe_ino =3D 0; + } + PIPE_UNLOCK(pipe); + bzero(ub, sizeof(*ub)); ub->st_mode =3D S_IFIFO; ub->st_blksize =3D PAGE_SIZE; --LAfrv7inJS3zgkKF Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.18 (FreeBSD) iEYEARECAAYFAk7XaZ4ACgkQC3+MBN1Mb4j0LQCgixhFoA6HSPTmR9Y7kKjZRbsJ 1REAoL60t5QFAcrCNxcENgYb306HMy2m =AIsa -----END PGP SIGNATURE----- --LAfrv7inJS3zgkKF--