atom feed16 messages in org.freebsd.freebsd-archRe: pipe/fifo code merged.
FromSent OnAttachments
Giovanni TrematerraJan 7, 2012 6:35 pm 
John BaldwinJan 9, 2012 5:48 am 
Bruce EvansJan 9, 2012 6:34 am 
Kostik BelousovJan 9, 2012 6:46 am 
Giovanni TrematerraJan 9, 2012 2:27 pm 
Giovanni TrematerraJan 9, 2012 3:19 pm 
Bruce EvansJan 10, 2012 4:03 am 
Giovanni TrematerraJan 10, 2012 3:04 pm 
Giovanni TrematerraJan 10, 2012 3:33 pm 
Giovanni TrematerraJan 10, 2012 4:14 pm 
Giovanni TrematerraJan 12, 2012 12:55 am 
Bruce EvansJan 12, 2012 4:51 am 
Giovanni TrematerraJan 17, 2012 3:42 am 
Jilles TjoelkerJan 28, 2012 3:36 pm 
Bruce EvansJan 29, 2012 3:31 am 
Giovanni TrematerraJan 31, 2012 1:48 pm 
Subject:Re: pipe/fifo code merged.
From:Giovanni Trematerra (gia@freebsd.org)
Date:Jan 9, 2012 3:19:58 pm
List:org.freebsd.freebsd-arch

On Mon, Jan 9, 2012 at 2:48 PM, John Baldwin <jh@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.