From owner-freebsd-arch@FreeBSD.ORG Mon Jan 9 23:20:23 2012 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A9C6B1065672; Mon, 9 Jan 2012 23:20:23 +0000 (UTC) (envelope-from giovanni.trematerra@gmail.com) Received: from mail-qw0-f47.google.com (mail-qw0-f47.google.com [209.85.216.47]) by mx1.freebsd.org (Postfix) with ESMTP id E3BDB8FC19; Mon, 9 Jan 2012 23:20:22 +0000 (UTC) Received: by qadb17 with SMTP id b17so1401199qad.13 for ; Mon, 09 Jan 2012 15:20:22 -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=GPDBTCQQcWF4euxBVIWPCz6P7qB9+a4oba+k1VMPOUg=; b=F2gplfH8YWhNpVc4ETwoVSF09+lPDNWzISczNJ7tmKlLtot1OIIU2DWuIKH0Kvr7EE LMZEZ0eLwYlRB9lucYnasC8xdiu1TvTmWbNOnavuRCxrrOvsTABCjEAY8RF8UXxVBw55 O7lQrD1lfCJ+SQy10WxJ75YYvrrX94752H95c= MIME-Version: 1.0 Received: by 10.224.175.2 with SMTP id v2mr21725918qaz.69.1326151222095; Mon, 09 Jan 2012 15:20:22 -0800 (PST) Sender: giovanni.trematerra@gmail.com Received: by 10.229.185.82 with HTTP; Mon, 9 Jan 2012 15:20:21 -0800 (PST) In-Reply-To: <201201090848.25736.jhb@freebsd.org> References: <201201090848.25736.jhb@freebsd.org> Date: Tue, 10 Jan 2012 00:20:21 +0100 X-Google-Sender-Auth: o2Hgir6CHlb9FBest_xradJFAH4 Message-ID: From: Giovanni Trematerra To: John Baldwin Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: flo@freebsd.org, Attilio Rao , Konstantin Belousov , freebsd-arch@freebsd.org, jilles@freebsd.org Subject: Re: pipe/fifo code merged. X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Jan 2012 23:20:23 -0000 On Mon, Jan 9, 2012 at 2:48 PM, John Baldwin wrote: > On Saturday, January 07, 2012 9:35:47 pm Giovanni Trematerra wrote: >> Hi, >> the patch at >> http://www.trematerra.net/patches/pipefifo_merge2.diff >> >> is a preliminary version of the FIFO optimizations project that I picked= up from >> the wiki. >> http://wiki.freebsd.org/IdeasPage#FIFO_optimizations_.28GSoC.29 >> >> zhaoshuai@ produced the following patch in the 2009 which attempted a fi= rst >> merge of the interfaces: >> http://www.trematerra.net/patches/fifo_soc2009.diff >> >> However I felt like the work was not yet completed and come up with my f= inal >> version. >> Now fifoes derive their structures from pipes one with just special hand= ling >> to support VFS operations. >> All the operations but the creation/destruction for fifoes and pipes are= handled >> by the same code. >> The heart of the patch is the new struct pipeinfo. >> pipeinfo is a per-file descriptor state. Basically it maintains a read e= nd and >> a write end for the descriptor. As pipes are bidirectional in FreeBSD, f= or a >> pipe this two fields are always equal but different for a fifo. >> To let fifo code in sys/fs/fifofs/fifo_vnops.c create/destroy the pipe, = two >> functions (pipe_ctor/pipe_dtor) were written. pipe_ctor setups things li= ke a >> call to kern_pipe and return a pipeinfo structure, while pipe_dtor relea= ses >> all the resources for a given pipeinfo. Once a pipe was setup during a f= ifo_open >> call, all the subsequent operations on the fifo are handled by the same = code >> of a pipe expect for the clean up code that calls pipe_dtor. >> Allocation of two pipeinfo structures for a pipe were showed to slow dow= n >> things by some micro-benchmarking. To speed up things during >> creation/destruction of pipes, the patch allocates all the needed data s= tructure >> zone using the umapipe struct that packing together all the needed data >> structures to be allocated at pipe creation. A similar umafifo structure= is >> used for fifoes. >> Thanks to jilles that made a review of the patch in a previous form, pri= vately. >> Thanks a lot to attilio that answered my stupid questions and drove me i= n the >> right direction. > > Thanks for taking this on. =A0In general I think this looks good, but had= a few > comments: > > - Why did you move setting the timestamps of pipes from the UMA ctor to a= n > =A0init routine? =A0This seems wrong. =A0The init routine is only invoked= when > =A0memory is first allocated to a slab to create a set of umapipe or umaf= ifo > =A0structures. =A0However, that umapipe/fifo may be reused multiple times= , all > =A0with the same timestamp. =A0Setting the timestamp in the ctor routine = means > =A0it is set each time a pipepair or fifo is created which seems more > =A0appropiate. =A0Similarly with the inode value. Ops, it was by accident. Thanks for pointed me out. > - I would maybe call pipe_ctor(), fifo_ctor() instead or otherwise adjust > =A0the name to note it is only used to create a FIFO, not used to create > =A0a normal pipe pair (which it's name implies). > - s/socket/pipe/ in the FIONREAD comment in sys_pipe.c. ok. > - Two extra blank lines in the patch that I think should be reverted: > > --- a/sys/fs/fifofs/fifo_vnops.c =A0 =A0 =A0 =A0Sun Jan 01 19:00:13 2012 = +0100 > +++ b/sys/fs/fifofs/fifo_vnops.c =A0 =A0 =A0 =A0Mon Jan 09 00:13:55 2012 = +0100 > =A0 =A0 =A0 =A0int =A0 =A0 =A0 =A0 =A0 =A0 fi_wgen; > =A0}; > > + > =A0static vop_print_t =A0 =A0 fifo_print; > =A0static vop_open_t =A0 =A0 =A0fifo_open; > .... > @@ -1598,13 +1839,20 @@ pipe_kqfilter(struct file *fp, struct kn > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (EPIPE); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cpipe =3D cpipe->pipe_peer; > + > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break; > =A0 =A0 =A0 =A0default: > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0PIPE_UNLOCK(cpipe); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (EINVAL); will do. Thank you for your review. -- Gianni