From owner-svn-src-head@FreeBSD.ORG Thu Dec 1 13:36:31 2011 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 1CF19106564A; Thu, 1 Dec 2011 13:36:31 +0000 (UTC) (envelope-from giovanni.trematerra@gmail.com) Received: from mail-qw0-f54.google.com (mail-qw0-f54.google.com [209.85.216.54]) by mx1.freebsd.org (Postfix) with ESMTP id 589238FC18; Thu, 1 Dec 2011 13:36:30 +0000 (UTC) Received: by qadc10 with SMTP id c10so1660152qad.13 for ; Thu, 01 Dec 2011 05:36:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=UvwqCWkzAJvV6S7FRJ9d633iC8cjOTr+wIMCDqkc6Gg=; b=YWBtZy+P1fHbOkmdsnHyJ3gwMk6E6jr9YqEyLrZWeBGrG+fxTkJaLQcOxCqWI3estU wAwGQx7HvEzNtG0nUWFaNVxyqAR+Gu7UygC6TNtPV2+hB8M+bMZadO2s7LdAZDmSEpKd IPJ1MGIWx+nv4NqE3G2NHHU44JfCwC9DdwqPw= MIME-Version: 1.0 Received: by 10.229.68.196 with SMTP id w4mr1279178qci.256.1322746588990; Thu, 01 Dec 2011 05:36:28 -0800 (PST) Sender: giovanni.trematerra@gmail.com Received: by 10.229.13.6 with HTTP; Thu, 1 Dec 2011 05:36:28 -0800 (PST) In-Reply-To: <20111201114846.GI50300@deviant.kiev.zoral.com.ua> References: <201110051656.p95Gu6Cw020744@svn.freebsd.org> <20111201114846.GI50300@deviant.kiev.zoral.com.ua> Date: Thu, 1 Dec 2011 14:36:28 +0100 X-Google-Sender-Auth: 4wxPF7aGWi4QSCVdYPumzgzoWnA Message-ID: From: Giovanni Trematerra To: Kostik Belousov Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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-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: Thu, 01 Dec 2011 13:36:31 -0000 2011/12/1 Kostik Belousov : > On Wed, Nov 30, 2011 at 10:01:12PM +0100, Giovanni Trematerra wrote: >> On Wed, Oct 5, 2011 at 6:56 PM, Konstantin Belousov wr= ote: >> > Author: kib >> > Date: Wed Oct =A05 16:56:06 2011 >> > New Revision: 226042 >> > URL: http://svn.freebsd.org/changeset/base/226042 >> > >> > Log: >> > =A0Supply unique (st_dev, st_ino) value pair for the fstat(2) done on = the pipes. >> > >> > =A0Reviewed by: =A0jhb, Peter Jeremy >> > =A0MFC after: =A0 =A02 weeks >> > >> > Modified: >> > =A0head/sys/kern/sys_pipe.c >> > =A0head/sys/sys/pipe.h >> > >> >> 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 la= zy >> and alloc a new unr number inside pipe_stat instead of during pipe creat= ion. >> 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 th= e 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 f= ind here >> >> http://www.trematerra.net/patches/pipe-fstat.c >> >> Feel free to add it under tools/regression/pipe/ >> >> Here the proposed patch: >> >> http://www.trematerra.net/patches/lazy_inoalloc.diff >> > Thank you for looking at this. > I committed the test, and the fix for the call to free_unr(-1). Thank you. > 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. Ops my bad. you're right. > > Also, I find the return of ENOMEM from fstat(2) somewhat questionable. Th= e > 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. > It seems to me a good compromise. I agree with you that return ENOMEM from fstat(2) isn't ideal although Linux does. We should document this behavior in man page though IMO. I'll provide a patch to man page as soon as possible if others are no objections at your updated patch and if that is ok for you. Thank you. -- Gianni