Date: Thu, 1 Dec 2011 13:48:46 +0200 From: Kostik Belousov <kostikbel@gmail.com> To: Giovanni Trematerra <gianni@freebsd.org> Cc: src-committers@freebsd.org, peterjeremy@acm.org, John Baldwin <jhb@freebsd.org>, svn-src-all@freebsd.org, Attilio Rao <attilio@freebsd.org>, svn-src-head@freebsd.org, Jeff Roberson <jeff@freebsd.org> Subject: Re: svn commit: r226042 - in head/sys: kern sys Message-ID: <20111201114846.GI50300@deviant.kiev.zoral.com.ua> In-Reply-To: <CACfq090D6CgH=hbLxGwMaBpAnui0ibfSMbqAsBGgXinEJQG7Hg@mail.gmail.com> References: <201110051656.p95Gu6Cw020744@svn.freebsd.org> <CACfq090D6CgH=hbLxGwMaBpAnui0ibfSMbqAsBGgXinEJQG7Hg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <kib@freebsd.org> 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 <peterjeremy acm org> > > =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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111201114846.GI50300>