Date: Tue, 10 Jan 2012 00:20:21 +0100 From: Giovanni Trematerra <gianni@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: flo@freebsd.org, Attilio Rao <attilio@freebsd.org>, Konstantin Belousov <kib@freebsd.org>, freebsd-arch@freebsd.org, jilles@freebsd.org Subject: Re: pipe/fifo code merged. Message-ID: <CACfq0925TP16fLNzCQ9xM%2Bq5z=g0PjSqsXhE6zC_m1jaw8Hbxg@mail.gmail.com> In-Reply-To: <201201090848.25736.jhb@freebsd.org> References: <CACfq093o9iVZKxCj58OR2hpCLDYTUTdxg_re_bEMYn2SrNrLCQ@mail.gmail.com> <201201090848.25736.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jan 9, 2012 at 2:48 PM, John Baldwin <jhb@freebsd.org> 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 first >> 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 final >> version. >> Now fifoes derive their structures from pipes one with just special handling >> 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 end and >> a write end for the descriptor. As pipes are bidirectional in FreeBSD, for 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 like a >> call to kern_pipe and return a pipeinfo structure, while pipe_dtor releases >> all the resources for a given pipeinfo. Once a pipe was setup during a fifo_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 down >> things by some micro-benchmarking. To speed up things during >> creation/destruction of pipes, the patch allocates all the needed data structure >> 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, privately. >> Thanks a lot to attilio that answered my stupid questions and drove me in the >> right direction. > > Thanks for taking this on. In 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 an > init routine? This seems wrong. The init routine is only invoked when > memory is first allocated to a slab to create a set of umapipe or umafifo > structures. However, that umapipe/fifo may be reused multiple times, all > with the same timestamp. Setting the timestamp in the ctor routine means > it is set each time a pipepair or fifo is created which seems more > appropiate. Similarly 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 > the name to note it is only used to create a FIFO, not used to create > a 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 Sun Jan 01 19:00:13 2012 +0100 > +++ b/sys/fs/fifofs/fifo_vnops.c Mon Jan 09 00:13:55 2012 +0100 > int fi_wgen; > }; > > + > static vop_print_t fifo_print; > static vop_open_t fifo_open; > .... > @@ -1598,13 +1839,20 @@ pipe_kqfilter(struct file *fp, struct kn > return (EPIPE); > } > cpipe = cpipe->pipe_peer; > + > break; > default: > PIPE_UNLOCK(cpipe); > return (EINVAL); will do. Thank you for your review. -- Gianni
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACfq0925TP16fLNzCQ9xM%2Bq5z=g0PjSqsXhE6zC_m1jaw8Hbxg>