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 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACfq0925TP16fLNzCQ9xM%2Bq5z=g0PjSqsXhE6zC_m1jaw8Hbxg>